| Message ID | 1441899263-12986-1-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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) {
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(-)