* [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 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 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 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
* 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
* [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 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
* 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 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
* [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] 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ä
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.