* [PATCH 1/2] drm/i915: workaround bad DSL readout in start of pipe update
@ 2015-09-10 15:34 Jesse Barnes
2015-09-10 15:34 ` [PATCH 2/2] drm/i915: add more debug info for when atomic updates fail Jesse Barnes
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Jesse Barnes @ 2015-09-10 15:34 UTC (permalink / raw)
To: intel-gfx
On HSW at least (still testing other platforms, but should be harmless
elsewhere), the DSL reg reads back as 0 when read around vblank start
time. This ends up confusing the atomic start/end checking code, since
it causes the update to appear as if it crossed a frame count boundary.
Workaround that by avoiding updates in the first couple of scanlines.
In testing, even a delay of a single microsecond is enough to give us a
good DSL value again, so the millisecond we'll wait when we hit this
case occasionally ought to be plenty.
References: https://bugs.freedesktop.org/show_bug.cgi?id=91579
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
drivers/gpu/drm/i915/intel_sprite.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index ca7e264..0c2c62f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -113,8 +113,16 @@ void intel_pipe_update_start(struct intel_crtc *crtc)
*/
prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
+ /*
+ * On HSW, the DSL reg (0x70000) appears to return 0 if we
+ * read it right around the start of vblank. So skip past it
+ * so we don't accidentally end up spanning a vblank frame
+ * increment, causing the update_end() code to squak at us.
+ * (We use 2 in the comparison to account for the
+ * scanline_offset used to correct the DSL readout.)
+ */
scanline = intel_get_crtc_scanline(crtc);
- if (scanline < min || scanline > max)
+ if (scanline > 2 && (scanline < min || scanline > max))
break;
if (timeout <= 0) {
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 2/2] drm/i915: add more debug info for when atomic updates fail 2015-09-10 15:34 [PATCH 1/2] drm/i915: workaround bad DSL readout in start of pipe update Jesse Barnes @ 2015-09-10 15:34 ` Jesse Barnes 2015-09-10 16:05 ` Ville Syrjälä 2015-09-15 17:03 ` [PATCH] drm/i915: add more debug info for when atomic updates fail v2 Jesse Barnes 2015-09-10 16:11 ` [PATCH 1/2] drm/i915: workaround bad DSL readout in start of pipe update Ville Syrjälä 2015-09-10 21:38 ` [PATCH] drm/i915: workaround bad DSL readout v2 Jesse Barnes 2 siblings, 2 replies; 23+ messages in thread From: Jesse Barnes @ 2015-09-10 15:34 UTC (permalink / raw) To: intel-gfx I used these additional fields to track down the issue I saw on HSW. References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_sprite.c | 16 ++++++++++++---- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 46484e4..90cad50 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -565,6 +565,8 @@ struct intel_crtc { unsigned start_vbl_count; ktime_t start_vbl_time; + int min_vbl, max_vbl; + int scanline_start; struct intel_crtc_atomic_commit atomic; diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 0c2c62f..ff8c9d5 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -94,6 +94,9 @@ void intel_pipe_update_start(struct intel_crtc *crtc) min = vblank_start - usecs_to_scanlines(mode, 100); max = vblank_start - 1; + crtc->min_vbl = min; + crtc->max_vbl = max; + local_irq_disable(); crtc->start_vbl_count = 0; @@ -122,8 +125,10 @@ void intel_pipe_update_start(struct intel_crtc *crtc) * scanline_offset used to correct the DSL readout.) */ scanline = intel_get_crtc_scanline(crtc); - if (scanline > 2 && (scanline < min || scanline > max)) + if (scanline > 2 && (scanline < min || scanline > max)) { + crtc->scanline_start = scanline; break; + } if (timeout <= 0) { DRM_ERROR("Potential atomic update failure on pipe %c\n", @@ -169,10 +174,13 @@ void intel_pipe_update_end(struct intel_crtc *crtc) local_irq_enable(); - if (crtc->start_vbl_count && crtc->start_vbl_count != end_vbl_count) - DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us\n", + if (crtc->start_vbl_count && crtc->start_vbl_count != end_vbl_count) { + DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us, min %d, max %d, scanline start %d, end %d\n", pipe_name(pipe), crtc->start_vbl_count, end_vbl_count, - ktime_us_delta(end_vbl_time, crtc->start_vbl_time)); + ktime_us_delta(end_vbl_time, crtc->start_vbl_time), + crtc->min_vbl, crtc->max_vbl, crtc->scanline_start, + intel_get_crtc_scanline(crtc)); + } } static void -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] drm/i915: add more debug info for when atomic updates fail 2015-09-10 15:34 ` [PATCH 2/2] drm/i915: add more debug info for when atomic updates fail Jesse Barnes @ 2015-09-10 16:05 ` Ville Syrjälä 2015-09-10 16:14 ` Jesse Barnes 2015-09-10 20:23 ` Jesse Barnes 2015-09-15 17:03 ` [PATCH] drm/i915: add more debug info for when atomic updates fail v2 Jesse Barnes 1 sibling, 2 replies; 23+ messages in thread From: Ville Syrjälä @ 2015-09-10 16:05 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx On Thu, Sep 10, 2015 at 08:34:23AM -0700, Jesse Barnes wrote: > I used these additional fields to track down the issue I saw on HSW. We already have the tracepoints with the scanline information. Not sure what extra this would give. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_sprite.c | 16 ++++++++++++---- > 2 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 46484e4..90cad50 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -565,6 +565,8 @@ struct intel_crtc { > > unsigned start_vbl_count; > ktime_t start_vbl_time; > + int min_vbl, max_vbl; > + int scanline_start; > > struct intel_crtc_atomic_commit atomic; > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index 0c2c62f..ff8c9d5 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -94,6 +94,9 @@ void intel_pipe_update_start(struct intel_crtc *crtc) > min = vblank_start - usecs_to_scanlines(mode, 100); > max = vblank_start - 1; > > + crtc->min_vbl = min; > + crtc->max_vbl = max; > + > local_irq_disable(); > crtc->start_vbl_count = 0; > > @@ -122,8 +125,10 @@ void intel_pipe_update_start(struct intel_crtc *crtc) > * scanline_offset used to correct the DSL readout.) > */ > scanline = intel_get_crtc_scanline(crtc); > - if (scanline > 2 && (scanline < min || scanline > max)) > + if (scanline > 2 && (scanline < min || scanline > max)) { > + crtc->scanline_start = scanline; > break; > + } > > if (timeout <= 0) { > DRM_ERROR("Potential atomic update failure on pipe %c\n", > @@ -169,10 +174,13 @@ void intel_pipe_update_end(struct intel_crtc *crtc) > > local_irq_enable(); > > - if (crtc->start_vbl_count && crtc->start_vbl_count != end_vbl_count) > - DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us\n", > + if (crtc->start_vbl_count && crtc->start_vbl_count != end_vbl_count) { > + DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us, min %d, max %d, scanline start %d, end %d\n", > pipe_name(pipe), crtc->start_vbl_count, end_vbl_count, > - ktime_us_delta(end_vbl_time, crtc->start_vbl_time)); > + ktime_us_delta(end_vbl_time, crtc->start_vbl_time), > + crtc->min_vbl, crtc->max_vbl, crtc->scanline_start, > + intel_get_crtc_scanline(crtc)); > + } > } > > static void > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] drm/i915: add more debug info for when atomic updates fail 2015-09-10 16:05 ` Ville Syrjälä @ 2015-09-10 16:14 ` Jesse Barnes 2015-09-10 20:23 ` Jesse Barnes 1 sibling, 0 replies; 23+ messages in thread From: Jesse Barnes @ 2015-09-10 16:14 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On 09/10/2015 09:05 AM, Ville Syrjälä wrote: > On Thu, Sep 10, 2015 at 08:34:23AM -0700, Jesse Barnes wrote: >> I used these additional fields to track down the issue I saw on HSW. > > We already have the tracepoints with the scanline information. Not sure > what extra this would give. Saves the trouble of having to dig through tons of tracepoints? _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] drm/i915: add more debug info for when atomic updates fail 2015-09-10 16:05 ` Ville Syrjälä 2015-09-10 16:14 ` Jesse Barnes @ 2015-09-10 20:23 ` Jesse Barnes 2015-09-14 9:04 ` Daniel Vetter 1 sibling, 1 reply; 23+ messages in thread From: Jesse Barnes @ 2015-09-10 20:23 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On 09/10/2015 09:05 AM, Ville Syrjälä wrote: > On Thu, Sep 10, 2015 at 08:34:23AM -0700, Jesse Barnes wrote: >> I used these additional fields to track down the issue I saw on HSW. > > We already have the tracepoints with the scanline information. Not sure > what extra this would give. Also when this hits users or test runners, the needed info will be immediately available, rather than having to enable trace points and re-run. I think that's important for error cases like this, as opposed to runtime debugging where tracepoints are fine. Jesse _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] drm/i915: add more debug info for when atomic updates fail 2015-09-10 20:23 ` Jesse Barnes @ 2015-09-14 9:04 ` Daniel Vetter 0 siblings, 0 replies; 23+ messages in thread From: Daniel Vetter @ 2015-09-14 9:04 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx On Thu, Sep 10, 2015 at 01:23:53PM -0700, Jesse Barnes wrote: > On 09/10/2015 09:05 AM, Ville Syrjälä wrote: > > On Thu, Sep 10, 2015 at 08:34:23AM -0700, Jesse Barnes wrote: > >> I used these additional fields to track down the issue I saw on HSW. > > > > We already have the tracepoints with the scanline information. Not sure > > what extra this would give. > > Also when this hits users or test runners, the needed info will be > immediately available, rather than having to enable trace points and > re-run. I think that's important for error cases like this, as opposed > to runtime debugging where tracepoints are fine. I agree with Jesse here, if things go south we should be able to print useful debug info. And we do have a bunch of rather mysterious pipe update failure backtraces all over already. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] drm/i915: add more debug info for when atomic updates fail v2 2015-09-10 15:34 ` [PATCH 2/2] drm/i915: add more debug info for when atomic updates fail Jesse Barnes 2015-09-10 16:05 ` Ville Syrjälä @ 2015-09-15 17:03 ` Jesse Barnes 2015-09-15 17:47 ` Ville Syrjälä 1 sibling, 1 reply; 23+ messages in thread From: Jesse Barnes @ 2015-09-15 17:03 UTC (permalink / raw) To: intel-gfx I used these additional fields to track down the issue I saw on HSW. v2: move debug fields into a substruct (Ville) References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/intel_drv.h | 8 ++++++-- drivers/gpu/drm/i915/intel_sprite.c | 30 +++++++++++++++++++++--------- 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 02a755a..1df6ebf 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -565,8 +565,12 @@ struct intel_crtc { int scanline_offset; - unsigned start_vbl_count; - ktime_t start_vbl_time; + struct { + unsigned start_vbl_count; + ktime_t start_vbl_time; + int min_vbl, max_vbl; + int scanline_start; + } debug; struct intel_crtc_atomic_commit atomic; diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 4d27243..4c627c4 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -94,8 +94,11 @@ void intel_pipe_update_start(struct intel_crtc *crtc) min = vblank_start - usecs_to_scanlines(mode, 100); max = vblank_start - 1; + crtc->debug.min_vbl = min; + crtc->debug.max_vbl = max; + local_irq_disable(); - crtc->start_vbl_count = 0; + crtc->debug.start_vbl_count = 0; if (min <= 0 || max <= 0) return; @@ -114,8 +117,10 @@ void intel_pipe_update_start(struct intel_crtc *crtc) prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE); scanline = intel_get_crtc_scanline(crtc); - if (scanline < min || scanline > max) + if (scanline < min || scanline > max) { + crtc->debug.scanline_start = scanline; break; + } if (timeout <= 0) { DRM_ERROR("Potential atomic update failure on pipe %c\n", @@ -134,11 +139,12 @@ void intel_pipe_update_start(struct intel_crtc *crtc) drm_crtc_vblank_put(&crtc->base); - crtc->start_vbl_time = ktime_get(); - crtc->start_vbl_count = dev->driver->get_vblank_counter(dev, pipe); + crtc->debug.start_vbl_time = ktime_get(); + crtc->debug.start_vbl_count = + dev->driver->get_vblank_counter(dev, pipe); trace_i915_pipe_update_vblank_evaded(crtc, min, max, - crtc->start_vbl_count); + crtc->debug.start_vbl_count); } /** @@ -161,10 +167,16 @@ void intel_pipe_update_end(struct intel_crtc *crtc) local_irq_enable(); - if (crtc->start_vbl_count && crtc->start_vbl_count != end_vbl_count) - DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us\n", - pipe_name(pipe), crtc->start_vbl_count, end_vbl_count, - ktime_us_delta(end_vbl_time, crtc->start_vbl_time)); + if (crtc->debug.start_vbl_count && + crtc->debug.start_vbl_count != end_vbl_count) { + DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us, min %d, max %d, scanline start %d, end %d\n", + pipe_name(pipe), crtc->debug.start_vbl_count, + end_vbl_count, + ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time), + crtc->debug.min_vbl, crtc->debug.max_vbl, + crtc->debug.scanline_start, + intel_get_crtc_scanline(crtc)); + } } static void -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: add more debug info for when atomic updates fail v2 2015-09-15 17:03 ` [PATCH] drm/i915: add more debug info for when atomic updates fail v2 Jesse Barnes @ 2015-09-15 17:47 ` Ville Syrjälä 0 siblings, 0 replies; 23+ messages in thread From: Ville Syrjälä @ 2015-09-15 17:47 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx On Tue, Sep 15, 2015 at 10:03:45AM -0700, Jesse Barnes wrote: > I used these additional fields to track down the issue I saw on HSW. > > v2: move debug fields into a substruct (Ville) > > References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_drv.h | 8 ++++++-- > drivers/gpu/drm/i915/intel_sprite.c | 30 +++++++++++++++++++++--------- > 2 files changed, 27 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 02a755a..1df6ebf 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -565,8 +565,12 @@ struct intel_crtc { > > int scanline_offset; > > - unsigned start_vbl_count; > - ktime_t start_vbl_time; > + struct { > + unsigned start_vbl_count; > + ktime_t start_vbl_time; > + int min_vbl, max_vbl; > + int scanline_start; > + } debug; > > struct intel_crtc_atomic_commit atomic; > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index 4d27243..4c627c4 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -94,8 +94,11 @@ void intel_pipe_update_start(struct intel_crtc *crtc) > min = vblank_start - usecs_to_scanlines(mode, 100); > max = vblank_start - 1; > > + crtc->debug.min_vbl = min; > + crtc->debug.max_vbl = max; > + > local_irq_disable(); > - crtc->start_vbl_count = 0; > + crtc->debug.start_vbl_count = 0; Could group the debug stuff together. > > if (min <= 0 || max <= 0) > return; > @@ -114,8 +117,10 @@ void intel_pipe_update_start(struct intel_crtc *crtc) > prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE); > > scanline = intel_get_crtc_scanline(crtc); > - if (scanline < min || scanline > max) > + if (scanline < min || scanline > max) { > + crtc->debug.scanline_start = scanline; > break; > + } That'll leave the scanline unset if we ever hit the timeout. Shouldn't happen of course, but would still be safer to move the assignment to happen after the loop. It would also allow you to group the debug assignments into one place. > > if (timeout <= 0) { > DRM_ERROR("Potential atomic update failure on pipe %c\n", > @@ -134,11 +139,12 @@ void intel_pipe_update_start(struct intel_crtc *crtc) > > drm_crtc_vblank_put(&crtc->base); > > - crtc->start_vbl_time = ktime_get(); > - crtc->start_vbl_count = dev->driver->get_vblank_counter(dev, pipe); > + crtc->debug.start_vbl_time = ktime_get(); > + crtc->debug.start_vbl_count = > + dev->driver->get_vblank_counter(dev, pipe); > > trace_i915_pipe_update_vblank_evaded(crtc, min, max, > - crtc->start_vbl_count); > + crtc->debug.start_vbl_count); Could also simplify the tracepoint calls a bit by not passing in the debug stuff separately. 'crtc' already gets passed in so the tracepoint can look up everything internally. trace_i915_pipe_update_start() could some similar treatment. > } > > /** > @@ -161,10 +167,16 @@ void intel_pipe_update_end(struct intel_crtc *crtc) > > local_irq_enable(); > > - if (crtc->start_vbl_count && crtc->start_vbl_count != end_vbl_count) > - DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us\n", > - pipe_name(pipe), crtc->start_vbl_count, end_vbl_count, > - ktime_us_delta(end_vbl_time, crtc->start_vbl_time)); > + if (crtc->debug.start_vbl_count && > + crtc->debug.start_vbl_count != end_vbl_count) { > + DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us, min %d, max %d, scanline start %d, end %d\n", > + pipe_name(pipe), crtc->debug.start_vbl_count, > + end_vbl_count, > + ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time), > + crtc->debug.min_vbl, crtc->debug.max_vbl, > + crtc->debug.scanline_start, > + intel_get_crtc_scanline(crtc)); > + } As with the frame counter and ktime, we should do the read before local_irq_enable() to make sure we get an accurate number. It does mean taking a minor hit for the non-failure case, but if that's a worry we can of course skip the ktime_get() and scanline read unless the frame counter shows a mismatch. > } > > static void > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] drm/i915: workaround bad DSL readout in start of pipe update 2015-09-10 15:34 [PATCH 1/2] drm/i915: workaround bad DSL readout in start of pipe update Jesse Barnes 2015-09-10 15:34 ` [PATCH 2/2] drm/i915: add more debug info for when atomic updates fail Jesse Barnes @ 2015-09-10 16:11 ` Ville Syrjälä 2015-09-10 16:17 ` Jesse Barnes 2015-09-10 21:38 ` [PATCH] drm/i915: workaround bad DSL readout v2 Jesse Barnes 2 siblings, 1 reply; 23+ messages in thread From: Ville Syrjälä @ 2015-09-10 16:11 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx On Thu, Sep 10, 2015 at 08:34:22AM -0700, Jesse Barnes wrote: > On HSW at least (still testing other platforms, but should be harmless > elsewhere), the DSL reg reads back as 0 when read around vblank start > time. This ends up confusing the atomic start/end checking code, since > it causes the update to appear as if it crossed a frame count boundary. > Workaround that by avoiding updates in the first couple of scanlines. > In testing, even a delay of a single microsecond is enough to give us a > good DSL value again, so the millisecond we'll wait when we hit this > case occasionally ought to be plenty. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_sprite.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index ca7e264..0c2c62f 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -113,8 +113,16 @@ void intel_pipe_update_start(struct intel_crtc *crtc) > */ > prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE); > > + /* > + * On HSW, the DSL reg (0x70000) appears to return 0 if we > + * read it right around the start of vblank. So skip past it > + * so we don't accidentally end up spanning a vblank frame > + * increment, causing the update_end() code to squak at us. > + * (We use 2 in the comparison to account for the > + * scanline_offset used to correct the DSL readout.) > + */ > scanline = intel_get_crtc_scanline(crtc); > - if (scanline < min || scanline > max) > + if (scanline > 2 && (scanline < min || scanline > max)) > break; And it means we'll miss a frame whenever the scanline is 0-2 even on a non-broken. So I don't kike it. Dunno maybe something more targeted like: read dsl if (IS_HASWELL && scanline == crtc->scanline_offset) { udelay(1); read dsl again } in __intel_get_crtc_scanline()? The udelay() is a bit unfortunate, but we'll need accurate scanline informnation for the vblank timestamps too. > > if (timeout <= 0) { > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] drm/i915: workaround bad DSL readout in start of pipe update 2015-09-10 16:11 ` [PATCH 1/2] drm/i915: workaround bad DSL readout in start of pipe update Ville Syrjälä @ 2015-09-10 16:17 ` Jesse Barnes 2015-09-10 16:27 ` Ville Syrjälä 0 siblings, 1 reply; 23+ messages in thread From: Jesse Barnes @ 2015-09-10 16:17 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On 09/10/2015 09:11 AM, Ville Syrjälä wrote: > On Thu, Sep 10, 2015 at 08:34:22AM -0700, Jesse Barnes wrote: >> On HSW at least (still testing other platforms, but should be harmless >> elsewhere), the DSL reg reads back as 0 when read around vblank start >> time. This ends up confusing the atomic start/end checking code, since >> it causes the update to appear as if it crossed a frame count boundary. >> Workaround that by avoiding updates in the first couple of scanlines. >> In testing, even a delay of a single microsecond is enough to give us a >> good DSL value again, so the millisecond we'll wait when we hit this >> case occasionally ought to be plenty. >> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> >> --- >> drivers/gpu/drm/i915/intel_sprite.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c >> index ca7e264..0c2c62f 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -113,8 +113,16 @@ void intel_pipe_update_start(struct intel_crtc *crtc) >> */ >> prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE); >> >> + /* >> + * On HSW, the DSL reg (0x70000) appears to return 0 if we >> + * read it right around the start of vblank. So skip past it >> + * so we don't accidentally end up spanning a vblank frame >> + * increment, causing the update_end() code to squak at us. >> + * (We use 2 in the comparison to account for the >> + * scanline_offset used to correct the DSL readout.) >> + */ >> scanline = intel_get_crtc_scanline(crtc); >> - if (scanline < min || scanline > max) >> + if (scanline > 2 && (scanline < min || scanline > max)) >> break; > > And it means we'll miss a frame whenever the scanline is 0-2 even on a > non-broken. So I don't kike it. We only stall 1ms in the timeout later, so we shouldn't miss a frame, we'll just queue the update in the middle of it instead, right? > > Dunno maybe something more targeted like: > > read dsl > if (IS_HASWELL && scanline == crtc->scanline_offset) { > udelay(1); > read dsl again > } > in __intel_get_crtc_scanline()? > > The udelay() is a bit unfortunate, but we'll need accurate scanline > informnation for the vblank timestamps too. Yeah, hiding it in the get_crtc_scanline() might be better; it would be good to know if this affects other platforms as well though. More testing is needed for that. Jesse _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] drm/i915: workaround bad DSL readout in start of pipe update 2015-09-10 16:17 ` Jesse Barnes @ 2015-09-10 16:27 ` Ville Syrjälä 0 siblings, 0 replies; 23+ messages in thread From: Ville Syrjälä @ 2015-09-10 16:27 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx On Thu, Sep 10, 2015 at 09:17:23AM -0700, Jesse Barnes wrote: > On 09/10/2015 09:11 AM, Ville Syrjälä wrote: > > On Thu, Sep 10, 2015 at 08:34:22AM -0700, Jesse Barnes wrote: > >> On HSW at least (still testing other platforms, but should be harmless > >> elsewhere), the DSL reg reads back as 0 when read around vblank start > >> time. This ends up confusing the atomic start/end checking code, since > >> it causes the update to appear as if it crossed a frame count boundary. > >> Workaround that by avoiding updates in the first couple of scanlines. > >> In testing, even a delay of a single microsecond is enough to give us a > >> good DSL value again, so the millisecond we'll wait when we hit this > >> case occasionally ought to be plenty. > >> > >> References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 > >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > >> --- > >> drivers/gpu/drm/i915/intel_sprite.c | 10 +++++++++- > >> 1 file changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > >> index ca7e264..0c2c62f 100644 > >> --- a/drivers/gpu/drm/i915/intel_sprite.c > >> +++ b/drivers/gpu/drm/i915/intel_sprite.c > >> @@ -113,8 +113,16 @@ void intel_pipe_update_start(struct intel_crtc *crtc) > >> */ > >> prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE); > >> > >> + /* > >> + * On HSW, the DSL reg (0x70000) appears to return 0 if we > >> + * read it right around the start of vblank. So skip past it > >> + * so we don't accidentally end up spanning a vblank frame > >> + * increment, causing the update_end() code to squak at us. > >> + * (We use 2 in the comparison to account for the > >> + * scanline_offset used to correct the DSL readout.) > >> + */ > >> scanline = intel_get_crtc_scanline(crtc); > >> - if (scanline < min || scanline > max) > >> + if (scanline > 2 && (scanline < min || scanline > max)) > >> break; > > > > And it means we'll miss a frame whenever the scanline is 0-2 even on a > > non-broken. So I don't kike it. > > We only stall 1ms in the timeout later, so we shouldn't miss a frame, we'll just queue the update in the middle of it instead, right? Hmm. Right I forgot the 1ms timeout. Well, it's susceptible to scheduling delays since we've re-enabled the interrupts and all, so there's no guarantee that it's just 1ms. Also once we'll retry the loop we'll just bail out if the timeout has already expired, so there's really no guarantee anymore that we won't be in the danger zone when we enter the crirical section. > > > > > Dunno maybe something more targeted like: > > > > read dsl > > if (IS_HASWELL && scanline == crtc->scanline_offset) { > > udelay(1); > > read dsl again > > } > > in __intel_get_crtc_scanline()? > > > > The udelay() is a bit unfortunate, but we'll need accurate scanline > > informnation for the vblank timestamps too. > > Yeah, hiding it in the get_crtc_scanline() might be better; it would be good to know if this affects other platforms as well though. More testing is needed for that. > > Jesse -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] drm/i915: workaround bad DSL readout v2 2015-09-10 15:34 [PATCH 1/2] drm/i915: workaround bad DSL readout in start of pipe update Jesse Barnes 2015-09-10 15:34 ` [PATCH 2/2] drm/i915: add more debug info for when atomic updates fail Jesse Barnes 2015-09-10 16:11 ` [PATCH 1/2] drm/i915: workaround bad DSL readout in start of pipe update Ville Syrjälä @ 2015-09-10 21:38 ` Jesse Barnes 2015-09-10 21:53 ` Ville Syrjälä ` (2 more replies) 2 siblings, 3 replies; 23+ messages in thread From: Jesse Barnes @ 2015-09-10 21:38 UTC (permalink / raw) To: intel-gfx On HSW at least (still testing other platforms, but should be harmless elsewhere), the DSL reg reads back as 0 when read around vblank start time. This ends up confusing the atomic start/end checking code, since it causes the update to appear as if it crossed a frame count boundary. Avoid the problem by making sure we don't return scanline_offset from the get_crtc_scanline function. In moving the code there, I add to add an additional delay since it could be called and have a legitimate 0 result for some time (depending on the pixel clock). v2: move hsw dsl read hack to get_crtc_scanline (Ville) References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_irq.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 90bc6c2..97e5d52 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -697,6 +697,27 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3; /* + * On HSW, the DSL reg (0x70000) appears to return 0 if we + * read it right around the start of vblank. So try it again + * so we don't accidentally end up spanning a vblank frame + * increment, causing the pipe_update_end() code to squak at us. + */ + if (IS_HASWELL(dev) && !position) { + int i, temp; + + for (i = 0; i < 100; i++) { + udelay(1); + temp = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & + DSL_LINEMASK_GEN3; + if (temp != position) { + position = temp; + goto out; + } + } + } + +out: + /* * See update_scanline_offset() for the details on the * scanline_offset adjustment. */ -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: workaround bad DSL readout v2 2015-09-10 21:38 ` [PATCH] drm/i915: workaround bad DSL readout v2 Jesse Barnes @ 2015-09-10 21:53 ` Ville Syrjälä 2015-09-10 21:57 ` Jesse Barnes 2015-09-14 18:47 ` Ville Syrjälä 2015-09-22 16:41 ` Ville Syrjälä 2 siblings, 1 reply; 23+ messages in thread From: Ville Syrjälä @ 2015-09-10 21:53 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx On Thu, Sep 10, 2015 at 02:38:53PM -0700, Jesse Barnes wrote: > On HSW at least (still testing other platforms, but should be harmless > elsewhere), the DSL reg reads back as 0 when read around vblank start > time. This ends up confusing the atomic start/end checking code, since > it causes the update to appear as if it crossed a frame count boundary. > Avoid the problem by making sure we don't return scanline_offset from > the get_crtc_scanline function. In moving the code there, I add to add > an additional delay since it could be called and have a legitimate 0 > result for some time (depending on the pixel clock). > > v2: move hsw dsl read hack to get_crtc_scanline (Ville) > > References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_irq.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 90bc6c2..97e5d52 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -697,6 +697,27 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) > position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3; > > /* > + * On HSW, the DSL reg (0x70000) appears to return 0 if we > + * read it right around the start of vblank. So try it again > + * so we don't accidentally end up spanning a vblank frame > + * increment, causing the pipe_update_end() code to squak at us. > + */ > + if (IS_HASWELL(dev) && !position) { > + int i, temp; > + > + for (i = 0; i < 100; i++) { > + udelay(1); > + temp = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & > + DSL_LINEMASK_GEN3; > + if (temp != position) { > + position = temp; > + goto out; > + } > + } > + } Hmm. Another idea. If it always happens at start of vbl, maybe have a look at the ISR. If scanline reads 0, but ISR says we're in vblank, just return vblank_start. That's assming ISR would in fact show that it's in vblank when this happens. > + > +out: > + /* > * See update_scanline_offset() for the details on the > * scanline_offset adjustment. > */ > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: workaround bad DSL readout v2 2015-09-10 21:53 ` Ville Syrjälä @ 2015-09-10 21:57 ` Jesse Barnes 2015-09-10 22:33 ` Ville Syrjälä 0 siblings, 1 reply; 23+ messages in thread From: Jesse Barnes @ 2015-09-10 21:57 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On 09/10/2015 02:53 PM, Ville Syrjälä wrote: > On Thu, Sep 10, 2015 at 02:38:53PM -0700, Jesse Barnes wrote: >> On HSW at least (still testing other platforms, but should be harmless >> elsewhere), the DSL reg reads back as 0 when read around vblank start >> time. This ends up confusing the atomic start/end checking code, since >> it causes the update to appear as if it crossed a frame count boundary. >> Avoid the problem by making sure we don't return scanline_offset from >> the get_crtc_scanline function. In moving the code there, I add to add >> an additional delay since it could be called and have a legitimate 0 >> result for some time (depending on the pixel clock). >> >> v2: move hsw dsl read hack to get_crtc_scanline (Ville) >> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> >> --- >> drivers/gpu/drm/i915/i915_irq.c | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c >> index 90bc6c2..97e5d52 100644 >> --- a/drivers/gpu/drm/i915/i915_irq.c >> +++ b/drivers/gpu/drm/i915/i915_irq.c >> @@ -697,6 +697,27 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) >> position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3; >> >> /* >> + * On HSW, the DSL reg (0x70000) appears to return 0 if we >> + * read it right around the start of vblank. So try it again >> + * so we don't accidentally end up spanning a vblank frame >> + * increment, causing the pipe_update_end() code to squak at us. >> + */ >> + if (IS_HASWELL(dev) && !position) { >> + int i, temp; >> + >> + for (i = 0; i < 100; i++) { >> + udelay(1); >> + temp = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & >> + DSL_LINEMASK_GEN3; >> + if (temp != position) { >> + position = temp; >> + goto out; >> + } >> + } >> + } > > Hmm. Another idea. If it always happens at start of vbl, maybe have a > look at the ISR. If scanline reads 0, but ISR says we're in vblank, just > return vblank_start. That's assming ISR would in fact show that it's in > vblank when this happens. Sounds a bit racy, though I guess it wouldn't hurt. I'm actually a little dubious about this change anyway since it will mean longer delays when we really are at the top of the field/frame. You sure you don't like the first one better? Thanks, Jesse _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: workaround bad DSL readout v2 2015-09-10 21:57 ` Jesse Barnes @ 2015-09-10 22:33 ` Ville Syrjälä 2015-09-14 9:10 ` Daniel Vetter 2015-09-15 21:00 ` Jesse Barnes 0 siblings, 2 replies; 23+ messages in thread From: Ville Syrjälä @ 2015-09-10 22:33 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx On Thu, Sep 10, 2015 at 02:57:32PM -0700, Jesse Barnes wrote: > On 09/10/2015 02:53 PM, Ville Syrjälä wrote: > > On Thu, Sep 10, 2015 at 02:38:53PM -0700, Jesse Barnes wrote: > >> On HSW at least (still testing other platforms, but should be harmless > >> elsewhere), the DSL reg reads back as 0 when read around vblank start > >> time. This ends up confusing the atomic start/end checking code, since > >> it causes the update to appear as if it crossed a frame count boundary. > >> Avoid the problem by making sure we don't return scanline_offset from > >> the get_crtc_scanline function. In moving the code there, I add to add > >> an additional delay since it could be called and have a legitimate 0 > >> result for some time (depending on the pixel clock). > >> > >> v2: move hsw dsl read hack to get_crtc_scanline (Ville) > >> > >> References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 > >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > >> --- > >> drivers/gpu/drm/i915/i915_irq.c | 21 +++++++++++++++++++++ > >> 1 file changed, 21 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > >> index 90bc6c2..97e5d52 100644 > >> --- a/drivers/gpu/drm/i915/i915_irq.c > >> +++ b/drivers/gpu/drm/i915/i915_irq.c > >> @@ -697,6 +697,27 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) > >> position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3; > >> > >> /* > >> + * On HSW, the DSL reg (0x70000) appears to return 0 if we > >> + * read it right around the start of vblank. So try it again > >> + * so we don't accidentally end up spanning a vblank frame > >> + * increment, causing the pipe_update_end() code to squak at us. > >> + */ > >> + if (IS_HASWELL(dev) && !position) { > >> + int i, temp; > >> + > >> + for (i = 0; i < 100; i++) { > >> + udelay(1); > >> + temp = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & > >> + DSL_LINEMASK_GEN3; > >> + if (temp != position) { > >> + position = temp; > >> + goto out; > >> + } > >> + } > >> + } > > > > Hmm. Another idea. If it always happens at start of vbl, maybe have a > > look at the ISR. If scanline reads 0, but ISR says we're in vblank, just > > return vblank_start. That's assming ISR would in fact show that it's in > > vblank when this happens. > > Sounds a bit racy, Not too much I think. We're under spinlock_irqsave here, and if the scanline should magically start to tick just after we've read it we would still give almost the right answer, even if we got magically delayed by one or two scanlines. That's assuming that it has already latched the double buffered registers when it goes wonky. If the register latching happens only when the counter starts working again, well then we're in deeper doodoo. But then I wouldn't expect ISR to indicate that we're in vblank then either. So I think you should definitely check what the ISR actually tells you while things are in the bad state. In case the badness happens just before start of vblank, well, then I have no good idea how to handle it. This sort of retry loop is the best that comes to mind right now :( > though I guess it wouldn't hurt. I'm actually a > little dubious about this change anyway since it will mean longer delays > when we really are at the top of the field/frame. You sure you don't > like the first one better? The first one can't help vblank timestamps. Although I suppose that if the badness always cures itself before the interrupt gets raised, we would not hit this normally when we update the timestamp from the irq handler. We could hit it when updating the timestamp outside the irq handler though (ie. during vblank_get/put etc.). -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: workaround bad DSL readout v2 2015-09-10 22:33 ` Ville Syrjälä @ 2015-09-14 9:10 ` Daniel Vetter 2015-09-14 13:02 ` Ville Syrjälä 2015-09-15 21:00 ` Jesse Barnes 1 sibling, 1 reply; 23+ messages in thread From: Daniel Vetter @ 2015-09-14 9:10 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, Sep 11, 2015 at 01:33:08AM +0300, Ville Syrjälä wrote: > On Thu, Sep 10, 2015 at 02:57:32PM -0700, Jesse Barnes wrote: > > On 09/10/2015 02:53 PM, Ville Syrjälä wrote: > > > On Thu, Sep 10, 2015 at 02:38:53PM -0700, Jesse Barnes wrote: > > >> On HSW at least (still testing other platforms, but should be harmless > > >> elsewhere), the DSL reg reads back as 0 when read around vblank start > > >> time. This ends up confusing the atomic start/end checking code, since > > >> it causes the update to appear as if it crossed a frame count boundary. > > >> Avoid the problem by making sure we don't return scanline_offset from > > >> the get_crtc_scanline function. In moving the code there, I add to add > > >> an additional delay since it could be called and have a legitimate 0 > > >> result for some time (depending on the pixel clock). > > >> > > >> v2: move hsw dsl read hack to get_crtc_scanline (Ville) > > >> > > >> References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 > > >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > >> --- > > >> drivers/gpu/drm/i915/i915_irq.c | 21 +++++++++++++++++++++ > > >> 1 file changed, 21 insertions(+) > > >> > > >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > >> index 90bc6c2..97e5d52 100644 > > >> --- a/drivers/gpu/drm/i915/i915_irq.c > > >> +++ b/drivers/gpu/drm/i915/i915_irq.c > > >> @@ -697,6 +697,27 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) > > >> position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3; > > >> > > >> /* > > >> + * On HSW, the DSL reg (0x70000) appears to return 0 if we > > >> + * read it right around the start of vblank. So try it again > > >> + * so we don't accidentally end up spanning a vblank frame > > >> + * increment, causing the pipe_update_end() code to squak at us. > > >> + */ > > >> + if (IS_HASWELL(dev) && !position) { > > >> + int i, temp; > > >> + > > >> + for (i = 0; i < 100; i++) { > > >> + udelay(1); > > >> + temp = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & > > >> + DSL_LINEMASK_GEN3; > > >> + if (temp != position) { > > >> + position = temp; > > >> + goto out; > > >> + } > > >> + } > > >> + } > > > > > > Hmm. Another idea. If it always happens at start of vbl, maybe have a > > > look at the ISR. If scanline reads 0, but ISR says we're in vblank, just > > > return vblank_start. That's assming ISR would in fact show that it's in > > > vblank when this happens. > > > > Sounds a bit racy, > > Not too much I think. We're under spinlock_irqsave here, and if the > scanline should magically start to tick just after we've read it we > would still give almost the right answer, even if we got magically > delayed by one or two scanlines. That's assuming that it has already > latched the double buffered registers when it goes wonky. If the > register latching happens only when the counter starts working again, > well then we're in deeper doodoo. But then I wouldn't expect ISR to > indicate that we're in vblank then either. So I think you should > definitely check what the ISR actually tells you while things are > in the bad state. There's still a race 1. we read bogus vblank counter 2. vblank processing completes in hw 3. ISR indicates where out of vblank. So you'd need at least a loop to make sure you're 0 vblank counter is stable across the ISR read. -Daniel > In case the badness happens just before start of vblank, well, then > I have no good idea how to handle it. This sort of retry loop is > the best that comes to mind right now :( > > > though I guess it wouldn't hurt. I'm actually a > > little dubious about this change anyway since it will mean longer delays > > when we really are at the top of the field/frame. You sure you don't > > like the first one better? > > The first one can't help vblank timestamps. Although I suppose that if > the badness always cures itself before the interrupt gets raised, we > would not hit this normally when we update the timestamp from the irq > handler. We could hit it when updating the timestamp outside the irq > handler though (ie. during vblank_get/put etc.). On top of that there's patches floating from Chris to get a vblank timestamp without grabbing the interrupts temporarily or any spinlock. That helps when we immediately disable the interrupt and then a bit later the compositors asks for the current vblank to estimate the next frame. So if possible I'd definitely vote to share this "fix up bonghits vblank" logic if possible ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: workaround bad DSL readout v2 2015-09-14 9:10 ` Daniel Vetter @ 2015-09-14 13:02 ` Ville Syrjälä 2015-09-14 14:29 ` Daniel Vetter 0 siblings, 1 reply; 23+ messages in thread From: Ville Syrjälä @ 2015-09-14 13:02 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On Mon, Sep 14, 2015 at 11:10:04AM +0200, Daniel Vetter wrote: > On Fri, Sep 11, 2015 at 01:33:08AM +0300, Ville Syrjälä wrote: > > On Thu, Sep 10, 2015 at 02:57:32PM -0700, Jesse Barnes wrote: > > > On 09/10/2015 02:53 PM, Ville Syrjälä wrote: > > > > On Thu, Sep 10, 2015 at 02:38:53PM -0700, Jesse Barnes wrote: > > > >> On HSW at least (still testing other platforms, but should be harmless > > > >> elsewhere), the DSL reg reads back as 0 when read around vblank start > > > >> time. This ends up confusing the atomic start/end checking code, since > > > >> it causes the update to appear as if it crossed a frame count boundary. > > > >> Avoid the problem by making sure we don't return scanline_offset from > > > >> the get_crtc_scanline function. In moving the code there, I add to add > > > >> an additional delay since it could be called and have a legitimate 0 > > > >> result for some time (depending on the pixel clock). > > > >> > > > >> v2: move hsw dsl read hack to get_crtc_scanline (Ville) > > > >> > > > >> References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 > > > >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > >> --- > > > >> drivers/gpu/drm/i915/i915_irq.c | 21 +++++++++++++++++++++ > > > >> 1 file changed, 21 insertions(+) > > > >> > > > >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > > >> index 90bc6c2..97e5d52 100644 > > > >> --- a/drivers/gpu/drm/i915/i915_irq.c > > > >> +++ b/drivers/gpu/drm/i915/i915_irq.c > > > >> @@ -697,6 +697,27 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) > > > >> position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3; > > > >> > > > >> /* > > > >> + * On HSW, the DSL reg (0x70000) appears to return 0 if we > > > >> + * read it right around the start of vblank. So try it again > > > >> + * so we don't accidentally end up spanning a vblank frame > > > >> + * increment, causing the pipe_update_end() code to squak at us. > > > >> + */ > > > >> + if (IS_HASWELL(dev) && !position) { > > > >> + int i, temp; > > > >> + > > > >> + for (i = 0; i < 100; i++) { > > > >> + udelay(1); > > > >> + temp = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & > > > >> + DSL_LINEMASK_GEN3; > > > >> + if (temp != position) { > > > >> + position = temp; > > > >> + goto out; > > > >> + } > > > >> + } > > > >> + } > > > > > > > > Hmm. Another idea. If it always happens at start of vbl, maybe have a > > > > look at the ISR. If scanline reads 0, but ISR says we're in vblank, just > > > > return vblank_start. That's assming ISR would in fact show that it's in > > > > vblank when this happens. > > > > > > Sounds a bit racy, > > > > Not too much I think. We're under spinlock_irqsave here, and if the > > scanline should magically start to tick just after we've read it we > > would still give almost the right answer, even if we got magically > > delayed by one or two scanlines. That's assuming that it has already > > latched the double buffered registers when it goes wonky. If the > > register latching happens only when the counter starts working again, > > well then we're in deeper doodoo. But then I wouldn't expect ISR to > > indicate that we're in vblank then either. So I think you should > > definitely check what the ISR actually tells you while things are > > in the bad state. > > There's still a race > 1. we read bogus vblank counter > 2. vblank processing completes in hw > 3. ISR indicates where out of vblank. > > So you'd need at least a loop to make sure you're 0 vblank counter is > stable across the ISR read. Seems rather unlikely. That would mean something blocked us for the entire duration of the vblank. > -Daniel > > > In case the badness happens just before start of vblank, well, then > > I have no good idea how to handle it. This sort of retry loop is > > the best that comes to mind right now :( > > > > > though I guess it wouldn't hurt. I'm actually a > > > little dubious about this change anyway since it will mean longer delays > > > when we really are at the top of the field/frame. You sure you don't > > > like the first one better? > > > > The first one can't help vblank timestamps. Although I suppose that if > > the badness always cures itself before the interrupt gets raised, we > > would not hit this normally when we update the timestamp from the irq > > handler. We could hit it when updating the timestamp outside the irq > > handler though (ie. during vblank_get/put etc.). > > On top of that there's patches floating from Chris to get a vblank > timestamp without grabbing the interrupts temporarily or any spinlock. > That helps when we immediately disable the interrupt and then a bit later > the compositors asks for the current vblank to estimate the next frame. I think we all already agreed that it's not going to fly. If we don't ask the hw how many vblanks we lost we can't give userspace any kind of accurate information. > So if possible I'd definitely vote to share this "fix up bonghits vblank" > logic if possible ... I have no idea what you're looking to share and with whom. I do not want workarounds for hardware specific bugs in the shared vblank code. We already had that due to radeon, and that stuff has been doing nothing for us except hinder attempts at making the code work sanely for sane Intel hardware. Which reminds me, I need to post a pile of vblank patches... -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: workaround bad DSL readout v2 2015-09-14 13:02 ` Ville Syrjälä @ 2015-09-14 14:29 ` Daniel Vetter 0 siblings, 0 replies; 23+ messages in thread From: Daniel Vetter @ 2015-09-14 14:29 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Mon, Sep 14, 2015 at 04:02:44PM +0300, Ville Syrjälä wrote: > On Mon, Sep 14, 2015 at 11:10:04AM +0200, Daniel Vetter wrote: > > On Fri, Sep 11, 2015 at 01:33:08AM +0300, Ville Syrjälä wrote: > > > On Thu, Sep 10, 2015 at 02:57:32PM -0700, Jesse Barnes wrote: > > > > On 09/10/2015 02:53 PM, Ville Syrjälä wrote: > > > > > On Thu, Sep 10, 2015 at 02:38:53PM -0700, Jesse Barnes wrote: > > > > >> On HSW at least (still testing other platforms, but should be harmless > > > > >> elsewhere), the DSL reg reads back as 0 when read around vblank start > > > > >> time. This ends up confusing the atomic start/end checking code, since > > > > >> it causes the update to appear as if it crossed a frame count boundary. > > > > >> Avoid the problem by making sure we don't return scanline_offset from > > > > >> the get_crtc_scanline function. In moving the code there, I add to add > > > > >> an additional delay since it could be called and have a legitimate 0 > > > > >> result for some time (depending on the pixel clock). > > > > >> > > > > >> v2: move hsw dsl read hack to get_crtc_scanline (Ville) > > > > >> > > > > >> References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 > > > > >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > >> --- > > > > >> drivers/gpu/drm/i915/i915_irq.c | 21 +++++++++++++++++++++ > > > > >> 1 file changed, 21 insertions(+) > > > > >> > > > > >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > > > >> index 90bc6c2..97e5d52 100644 > > > > >> --- a/drivers/gpu/drm/i915/i915_irq.c > > > > >> +++ b/drivers/gpu/drm/i915/i915_irq.c > > > > >> @@ -697,6 +697,27 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) > > > > >> position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3; > > > > >> > > > > >> /* > > > > >> + * On HSW, the DSL reg (0x70000) appears to return 0 if we > > > > >> + * read it right around the start of vblank. So try it again > > > > >> + * so we don't accidentally end up spanning a vblank frame > > > > >> + * increment, causing the pipe_update_end() code to squak at us. > > > > >> + */ > > > > >> + if (IS_HASWELL(dev) && !position) { > > > > >> + int i, temp; > > > > >> + > > > > >> + for (i = 0; i < 100; i++) { > > > > >> + udelay(1); > > > > >> + temp = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & > > > > >> + DSL_LINEMASK_GEN3; > > > > >> + if (temp != position) { > > > > >> + position = temp; > > > > >> + goto out; > > > > >> + } > > > > >> + } > > > > >> + } > > > > > > > > > > Hmm. Another idea. If it always happens at start of vbl, maybe have a > > > > > look at the ISR. If scanline reads 0, but ISR says we're in vblank, just > > > > > return vblank_start. That's assming ISR would in fact show that it's in > > > > > vblank when this happens. > > > > > > > > Sounds a bit racy, > > > > > > Not too much I think. We're under spinlock_irqsave here, and if the > > > scanline should magically start to tick just after we've read it we > > > would still give almost the right answer, even if we got magically > > > delayed by one or two scanlines. That's assuming that it has already > > > latched the double buffered registers when it goes wonky. If the > > > register latching happens only when the counter starts working again, > > > well then we're in deeper doodoo. But then I wouldn't expect ISR to > > > indicate that we're in vblank then either. So I think you should > > > definitely check what the ISR actually tells you while things are > > > in the bad state. > > > > There's still a race > > 1. we read bogus vblank counter > > 2. vblank processing completes in hw > > 3. ISR indicates where out of vblank. > > > > So you'd need at least a loop to make sure you're 0 vblank counter is > > stable across the ISR read. > > Seems rather unlikely. That would mean something blocked us for the entire > duration of the vblank. > > > -Daniel > > > > > In case the badness happens just before start of vblank, well, then > > > I have no good idea how to handle it. This sort of retry loop is > > > the best that comes to mind right now :( > > > > > > > though I guess it wouldn't hurt. I'm actually a > > > > little dubious about this change anyway since it will mean longer delays > > > > when we really are at the top of the field/frame. You sure you don't > > > > like the first one better? > > > > > > The first one can't help vblank timestamps. Although I suppose that if > > > the badness always cures itself before the interrupt gets raised, we > > > would not hit this normally when we update the timestamp from the irq > > > handler. We could hit it when updating the timestamp outside the irq > > > handler though (ie. during vblank_get/put etc.). > > > > On top of that there's patches floating from Chris to get a vblank > > timestamp without grabbing the interrupts temporarily or any spinlock. > > That helps when we immediately disable the interrupt and then a bit later > > the compositors asks for the current vblank to estimate the next frame. > > I think we all already agreed that it's not going to fly. If we don't > ask the hw how many vblanks we lost we can't give userspace any kind > of accurate information. > > > So if possible I'd definitely vote to share this "fix up bonghits vblank" > > logic if possible ... > > I have no idea what you're looking to share and with whom. I do not want > workarounds for hardware specific bugs in the shared vblank code. We > already had that due to radeon, and that stuff has been doing nothing > for us except hinder attempts at making the code work sanely for sane > Intel hardware. > > Which reminds me, I need to post a pile of vblank patches... s/vblank/scanline/ in all of the above since I was confused again. And I just thought if we have some special hacks to make the get_scanline function cope with hw oddity that should be used by the vblank time stamp code too. But that's the case already (if we put the hack into __intel_get_crtc_scanline). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: workaround bad DSL readout v2 2015-09-10 22:33 ` Ville Syrjälä 2015-09-14 9:10 ` Daniel Vetter @ 2015-09-15 21:00 ` Jesse Barnes 1 sibling, 0 replies; 23+ messages in thread From: Jesse Barnes @ 2015-09-15 21:00 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On 09/10/2015 03:33 PM, Ville Syrjälä wrote: > On Thu, Sep 10, 2015 at 02:57:32PM -0700, Jesse Barnes wrote: >> On 09/10/2015 02:53 PM, Ville Syrjälä wrote: >>> On Thu, Sep 10, 2015 at 02:38:53PM -0700, Jesse Barnes wrote: >>>> On HSW at least (still testing other platforms, but should be harmless >>>> elsewhere), the DSL reg reads back as 0 when read around vblank start >>>> time. This ends up confusing the atomic start/end checking code, since >>>> it causes the update to appear as if it crossed a frame count boundary. >>>> Avoid the problem by making sure we don't return scanline_offset from >>>> the get_crtc_scanline function. In moving the code there, I add to add >>>> an additional delay since it could be called and have a legitimate 0 >>>> result for some time (depending on the pixel clock). >>>> >>>> v2: move hsw dsl read hack to get_crtc_scanline (Ville) >>>> >>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 >>>> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> >>>> --- >>>> drivers/gpu/drm/i915/i915_irq.c | 21 +++++++++++++++++++++ >>>> 1 file changed, 21 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c >>>> index 90bc6c2..97e5d52 100644 >>>> --- a/drivers/gpu/drm/i915/i915_irq.c >>>> +++ b/drivers/gpu/drm/i915/i915_irq.c >>>> @@ -697,6 +697,27 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) >>>> position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3; >>>> >>>> /* >>>> + * On HSW, the DSL reg (0x70000) appears to return 0 if we >>>> + * read it right around the start of vblank. So try it again >>>> + * so we don't accidentally end up spanning a vblank frame >>>> + * increment, causing the pipe_update_end() code to squak at us. >>>> + */ >>>> + if (IS_HASWELL(dev) && !position) { >>>> + int i, temp; >>>> + >>>> + for (i = 0; i < 100; i++) { >>>> + udelay(1); >>>> + temp = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & >>>> + DSL_LINEMASK_GEN3; >>>> + if (temp != position) { >>>> + position = temp; >>>> + goto out; >>>> + } >>>> + } >>>> + } >>> >>> Hmm. Another idea. If it always happens at start of vbl, maybe have a >>> look at the ISR. If scanline reads 0, but ISR says we're in vblank, just >>> return vblank_start. That's assming ISR would in fact show that it's in >>> vblank when this happens. >> >> Sounds a bit racy, > > Not too much I think. We're under spinlock_irqsave here, and if the > scanline should magically start to tick just after we've read it we > would still give almost the right answer, even if we got magically > delayed by one or two scanlines. That's assuming that it has already > latched the double buffered registers when it goes wonky. If the > register latching happens only when the counter starts working again, > well then we're in deeper doodoo. But then I wouldn't expect ISR to > indicate that we're in vblank then either. So I think you should > definitely check what the ISR actually tells you while things are > in the bad state. > > In case the badness happens just before start of vblank, well, then > I have no good idea how to handle it. This sort of retry loop is > the best that comes to mind right now :( > >> though I guess it wouldn't hurt. I'm actually a >> little dubious about this change anyway since it will mean longer delays >> when we really are at the top of the field/frame. You sure you don't >> like the first one better? > > The first one can't help vblank timestamps. Although I suppose that if > the badness always cures itself before the interrupt gets raised, we > would not hit this normally when we update the timestamp from the irq > handler. We could hit it when updating the timestamp outside the irq > handler though (ie. during vblank_get/put etc.). > Ok so just for the record, I've tried a few things: - using the ISR to check the vblank and vsync status when this occurs (both are clear) - using the transcoder debug reg for reading the scanline (also returns 0) so I think we're stuck with one of the two approaches already posted. As for affecting the vblank timestamps, I think you're right that this bug shouldn't affect that. The issue (a 0 return from the PIPEDSL read) seems to manifest just as the frame counter should be getting incremented. By the time we actually raise an interrupt or move to the next scanline, the correct value is returned. So even if we stuff the workaround into the pipe update start function (which should be more tolerant of delays in general) I think we'll be safe. What do you prefer? Do you want any changes on top of either of the previous patches given the above? Thanks, Jesse _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: workaround bad DSL readout v2 2015-09-10 21:38 ` [PATCH] drm/i915: workaround bad DSL readout v2 Jesse Barnes 2015-09-10 21:53 ` Ville Syrjälä @ 2015-09-14 18:47 ` Ville Syrjälä 2015-09-22 16:41 ` Ville Syrjälä 2 siblings, 0 replies; 23+ messages in thread From: Ville Syrjälä @ 2015-09-14 18:47 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx On Thu, Sep 10, 2015 at 02:38:53PM -0700, Jesse Barnes wrote: > On HSW at least (still testing other platforms, but should be harmless > elsewhere), the DSL reg reads back as 0 when read around vblank start > time. This ends up confusing the atomic start/end checking code, since > it causes the update to appear as if it crossed a frame count boundary. > Avoid the problem by making sure we don't return scanline_offset from > the get_crtc_scanline function. In moving the code there, I add to add > an additional delay since it could be called and have a legitimate 0 > result for some time (depending on the pixel clock). > > v2: move hsw dsl read hack to get_crtc_scanline (Ville) > > References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_irq.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 90bc6c2..97e5d52 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -697,6 +697,27 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) > position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3; > > /* > + * On HSW, the DSL reg (0x70000) appears to return 0 if we > + * read it right around the start of vblank. So try it again > + * so we don't accidentally end up spanning a vblank frame > + * increment, causing the pipe_update_end() code to squak at us. > + */ > + if (IS_HASWELL(dev) && !position) { > + int i, temp; > + > + for (i = 0; i < 100; i++) { > + udelay(1); > + temp = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & > + DSL_LINEMASK_GEN3; > + if (temp != position) { > + position = temp; > + goto out; > + } > + } > + } I mentioned this on irc, but I'll repeat here so we have it on record somewhere; There's another scanline register on hsw+ in the transcoder. We should definitely check if that suffers from the same fault. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: workaround bad DSL readout v2 2015-09-10 21:38 ` [PATCH] drm/i915: workaround bad DSL readout v2 Jesse Barnes 2015-09-10 21:53 ` Ville Syrjälä 2015-09-14 18:47 ` Ville Syrjälä @ 2015-09-22 16:41 ` Ville Syrjälä 2015-09-22 19:15 ` [PATCH] drm/i915: workaround bad DSL readout v3 Jesse Barnes 2 siblings, 1 reply; 23+ messages in thread From: Ville Syrjälä @ 2015-09-22 16:41 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx On Thu, Sep 10, 2015 at 02:38:53PM -0700, Jesse Barnes wrote: > On HSW at least (still testing other platforms, but should be harmless > elsewhere), the DSL reg reads back as 0 when read around vblank start > time. This ends up confusing the atomic start/end checking code, since > it causes the update to appear as if it crossed a frame count boundary. > Avoid the problem by making sure we don't return scanline_offset from > the get_crtc_scanline function. In moving the code there, I add to add > an additional delay since it could be called and have a legitimate 0 > result for some time (depending on the pixel clock). > > v2: move hsw dsl read hack to get_crtc_scanline (Ville) > > References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_irq.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 90bc6c2..97e5d52 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -697,6 +697,27 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) > position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3; > > /* > + * On HSW, the DSL reg (0x70000) appears to return 0 if we > + * read it right around the start of vblank. So try it again Make that "just before the start of vblank". Otherwise I'm sure I'll forget the details and start to think again about using ISR tricks. > + * so we don't accidentally end up spanning a vblank frame > + * increment, causing the pipe_update_end() code to squak at us. > + */ > + if (IS_HASWELL(dev) && !position) { > + int i, temp; > + > + for (i = 0; i < 100; i++) { > + udelay(1); > + temp = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & > + DSL_LINEMASK_GEN3; > + if (temp != position) { > + position = temp; > + goto out; break is enough A bit nasty this loop with irqs off, but it's the only way we can avoid corrupted vblank timestamps from happening when the hardware is on the fritz. With the comment updatead and goto killed: Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + } > + } > + } > + > +out: > + /* > * See update_scanline_offset() for the details on the > * scanline_offset adjustment. > */ > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] drm/i915: workaround bad DSL readout v3 2015-09-22 16:41 ` Ville Syrjälä @ 2015-09-22 19:15 ` Jesse Barnes 2015-09-23 7:48 ` Jani Nikula 0 siblings, 1 reply; 23+ messages in thread From: Jesse Barnes @ 2015-09-22 19:15 UTC (permalink / raw) To: intel-gfx On HSW at least (still testing other platforms, but should be harmless elsewhere), the DSL reg reads back as 0 when read around vblank start time. This ends up confusing the atomic start/end checking code, since it causes the update to appear as if it crossed a frame count boundary. Avoid the problem by making sure we don't return scanline_offset from the get_crtc_scanline function. In moving the code there, I add to add an additional delay since it could be called and have a legitimate 0 result for some time (depending on the pixel clock). v2: move hsw dsl read hack to get_crtc_scanline (Ville) v3: use break instead of goto (Ville) update comment with workaround details (Ville) References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_irq.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 1f53061..a8aa797 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -697,6 +697,32 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3; /* + * On HSW, the DSL reg (0x70000) appears to return 0 if we + * read it just before the start of vblank. So try it again + * so we don't accidentally end up spanning a vblank frame + * increment, causing the pipe_update_end() code to squak at us. + * + * The nature of this problem means we can't simply check the ISR + * bit and return the vblank start value; nor can we use the scanline + * debug register in the transcoder as it appears to have the same + * problem. We may need to extend this to include other platforms, + * but so far testing only shows the problem on HSW. + */ + if (IS_HASWELL(dev) && !position) { + int i, temp; + + for (i = 0; i < 100; i++) { + udelay(1); + temp = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & + DSL_LINEMASK_GEN3; + if (temp != position) { + position = temp; + break; + } + } + } + + /* * See update_scanline_offset() for the details on the * scanline_offset adjustment. */ -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: workaround bad DSL readout v3 2015-09-22 19:15 ` [PATCH] drm/i915: workaround bad DSL readout v3 Jesse Barnes @ 2015-09-23 7:48 ` Jani Nikula 0 siblings, 0 replies; 23+ messages in thread From: Jani Nikula @ 2015-09-23 7:48 UTC (permalink / raw) To: Jesse Barnes, intel-gfx On Tue, 22 Sep 2015, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On HSW at least (still testing other platforms, but should be harmless > elsewhere), the DSL reg reads back as 0 when read around vblank start > time. This ends up confusing the atomic start/end checking code, since > it causes the update to appear as if it crossed a frame count boundary. > Avoid the problem by making sure we don't return scanline_offset from > the get_crtc_scanline function. In moving the code there, I add to add > an additional delay since it could be called and have a legitimate 0 > result for some time (depending on the pixel clock). > > v2: move hsw dsl read hack to get_crtc_scanline (Ville) > v3: use break instead of goto (Ville) > update comment with workaround details (Ville) > > References: https://bugs.freedesktop.org/show_bug.cgi?id=91579 > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> Pushed to drm-intel-fixes, thanks for the patch and review. Sorry for the delay, I failed to notice the patch had been reviewed. It would have helped if the patch had the Reviewed-by: tag already in place, or the reviewer (or someone else pointing at the review) had replied with the Reviewed-by: tag. I spend very little time looking at individual patches, especially if the appearance is that there's discussion going on. BR, Jani. > --- > drivers/gpu/drm/i915/i915_irq.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 1f53061..a8aa797 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -697,6 +697,32 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) > position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3; > > /* > + * On HSW, the DSL reg (0x70000) appears to return 0 if we > + * read it just before the start of vblank. So try it again > + * so we don't accidentally end up spanning a vblank frame > + * increment, causing the pipe_update_end() code to squak at us. > + * > + * The nature of this problem means we can't simply check the ISR > + * bit and return the vblank start value; nor can we use the scanline > + * debug register in the transcoder as it appears to have the same > + * problem. We may need to extend this to include other platforms, > + * but so far testing only shows the problem on HSW. > + */ > + if (IS_HASWELL(dev) && !position) { > + int i, temp; > + > + for (i = 0; i < 100; i++) { > + udelay(1); > + temp = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & > + DSL_LINEMASK_GEN3; > + if (temp != position) { > + position = temp; > + break; > + } > + } > + } > + > + /* > * See update_scanline_offset() for the details on the > * scanline_offset adjustment. > */ > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2015-09-23 7:45 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-09-10 15:34 [PATCH 1/2] drm/i915: workaround bad DSL readout in start of pipe update Jesse Barnes 2015-09-10 15:34 ` [PATCH 2/2] drm/i915: add more debug info for when atomic updates fail Jesse Barnes 2015-09-10 16:05 ` Ville Syrjälä 2015-09-10 16:14 ` Jesse Barnes 2015-09-10 20:23 ` Jesse Barnes 2015-09-14 9:04 ` Daniel Vetter 2015-09-15 17:03 ` [PATCH] drm/i915: add more debug info for when atomic updates fail v2 Jesse Barnes 2015-09-15 17:47 ` Ville Syrjälä 2015-09-10 16:11 ` [PATCH 1/2] drm/i915: workaround bad DSL readout in start of pipe update Ville Syrjälä 2015-09-10 16:17 ` Jesse Barnes 2015-09-10 16:27 ` Ville Syrjälä 2015-09-10 21:38 ` [PATCH] drm/i915: workaround bad DSL readout v2 Jesse Barnes 2015-09-10 21:53 ` Ville Syrjälä 2015-09-10 21:57 ` Jesse Barnes 2015-09-10 22:33 ` Ville Syrjälä 2015-09-14 9:10 ` Daniel Vetter 2015-09-14 13:02 ` Ville Syrjälä 2015-09-14 14:29 ` Daniel Vetter 2015-09-15 21:00 ` Jesse Barnes 2015-09-14 18:47 ` Ville Syrjälä 2015-09-22 16:41 ` Ville Syrjälä 2015-09-22 19:15 ` [PATCH] drm/i915: workaround bad DSL readout v3 Jesse Barnes 2015-09-23 7:48 ` Jani Nikula
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.