| Message ID | 1504618504-20561-1-git-send-email-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 05, 2017 at 04:35:04PM +0300, Mika Kahola wrote: > It appears that we cannot trust scanline counters when MIPI/DSI display is > connected. In CI system this appears as flickering errors that randomly > appear in test cases. To avoid this flickering, let's just silence atomic > update failure in case with DSI panel. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102403 > Signed-off-by: Mika Kahola <mika.kahola@intel.com> This just changes a loud atomic failure to a silent atomic failure. What we instead need to do is actually fix the bug, not hide it. This means DSI is atm blacklisted almost entirely, but well it's broken, so no harm in that. Please no polishing of just results in CI, it needs to give us honest results. -Daniel > --- > drivers/gpu/drm/i915/intel_sprite.c | 32 +++++++++++++++++--------------- > 1 file changed, 17 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index b0d6e3e..8511072 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -205,23 +205,25 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state) > if (intel_vgpu_active(dev_priv)) > return; > > - 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, scanline_end); > - } > + if (!intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI)) { > + 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, scanline_end); > + } > #ifdef CONFIG_DRM_I915_DEBUG_VBLANK_EVADE > - else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) > > - VBLANK_EVASION_TIME_US) > - DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n", > - pipe_name(pipe), > - ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time), > - VBLANK_EVASION_TIME_US); > + else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) > > + VBLANK_EVASION_TIME_US) > + DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n", > + pipe_name(pipe), > + ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time), > + VBLANK_EVASION_TIME_US); > #endif > + } > } > > static void > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2017-09-05 at 18:11 +0200, Daniel Vetter wrote: > On Tue, Sep 05, 2017 at 04:35:04PM +0300, Mika Kahola wrote: > > > > It appears that we cannot trust scanline counters when MIPI/DSI > > display is > > connected. In CI system this appears as flickering errors that > > randomly > > appear in test cases. To avoid this flickering, let's just silence > > atomic > > update failure in case with DSI panel. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102403 > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > This just changes a loud atomic failure to a silent atomic failure. > What > we instead need to do is actually fix the bug, not hide it. > BSpec has a notion that (PIPE_SCANLINE) that is "Not supported with MIPI DSI." That's why I thought it might be ok to silence the error as the computation that we try to accomplish wouldn't work anyway. Maybe this way we could remove DSI from being blackllisted. > This means DSI is atm blacklisted almost entirely, but well it's > broken, > so no harm in that. > > Please no polishing of just results in CI, it needs to give us honest > results. > -Daniel > > > > --- > > drivers/gpu/drm/i915/intel_sprite.c | 32 +++++++++++++++++------ > > --------- > > 1 file changed, 17 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > b/drivers/gpu/drm/i915/intel_sprite.c > > index b0d6e3e..8511072 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -205,23 +205,25 @@ void intel_pipe_update_end(struct > > intel_crtc_state *new_crtc_state) > > if (intel_vgpu_active(dev_priv)) > > return; > > > > - 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, > > scanline_end); > > - } > > + if (!intel_crtc_has_type(new_crtc_state, > > INTEL_OUTPUT_DSI)) { > > + 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, > > scanline_end); > > + } > > #ifdef CONFIG_DRM_I915_DEBUG_VBLANK_EVADE > > - else if (ktime_us_delta(end_vbl_time, crtc- > > >debug.start_vbl_time) > > > - VBLANK_EVASION_TIME_US) > > - DRM_WARN("Atomic update on pipe (%c) took %lld us, > > max time under evasion is %u us\n", > > - pipe_name(pipe), > > - ktime_us_delta(end_vbl_time, crtc- > > >debug.start_vbl_time), > > - VBLANK_EVASION_TIME_US); > > + else if (ktime_us_delta(end_vbl_time, crtc- > > >debug.start_vbl_time) > > > + VBLANK_EVASION_TIME_US) > > + DRM_WARN("Atomic update on pipe (%c) took > > %lld us, max time under evasion is %u us\n", > > + pipe_name(pipe), > > + ktime_us_delta(end_vbl_time, > > crtc->debug.start_vbl_time), > > + VBLANK_EVASION_TIME_US); > > #endif > > + } > > } > > > > static void > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 06/09/17 13:09, Mika Kahola wrote: > On Tue, 2017-09-05 at 18:11 +0200, Daniel Vetter wrote: >> On Tue, Sep 05, 2017 at 04:35:04PM +0300, Mika Kahola wrote: >>> >>> It appears that we cannot trust scanline counters when MIPI/DSI >>> display is >>> connected. In CI system this appears as flickering errors that >>> randomly >>> appear in test cases. To avoid this flickering, let's just silence >>> atomic >>> update failure in case with DSI panel. >>> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102403 >>> Signed-off-by: Mika Kahola <mika.kahola@intel.com> >> This just changes a loud atomic failure to a silent atomic failure. >> What >> we instead need to do is actually fix the bug, not hide it. >> > BSpec has a notion that (PIPE_SCANLINE) that is "Not supported with > MIPI DSI." > > That's why I thought it might be ok to silence the error as the > computation that we try to accomplish wouldn't work anyway. Maybe this > way we could remove DSI from being blackllisted. I agree. If the HW can't do it, so what can we do here? As long as this is well documented and the userspace knows about this issue (if anything relies on this feature), then what else can we do? With the relevant BSpec quotes added above the changes, I can give my: Acked-by: Martin Peres <martin.peres@linux.intel.com> I would however like to know if this breaks any feature the userspace relies on. Martin
Op 05-09-17 om 15:35 schreef Mika Kahola: > It appears that we cannot trust scanline counters when MIPI/DSI display is > connected. In CI system this appears as flickering errors that randomly > appear in test cases. To avoid this flickering, let's just silence atomic > update failure in case with DSI panel. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102403 > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > --- > drivers/gpu/drm/i915/intel_sprite.c | 32 +++++++++++++++++--------------- > 1 file changed, 17 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index b0d6e3e..8511072 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -205,23 +205,25 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state) > if (intel_vgpu_active(dev_priv)) > return; > > - 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, scanline_end); > - } > + if (!intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI)) { > + 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, scanline_end); > + } > #ifdef CONFIG_DRM_I915_DEBUG_VBLANK_EVADE > - else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) > > - VBLANK_EVASION_TIME_US) > - DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n", > - pipe_name(pipe), > - ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time), > - VBLANK_EVASION_TIME_US); > + else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) > > + VBLANK_EVASION_TIME_US) > + DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n", > + pipe_name(pipe), > + ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time), > + VBLANK_EVASION_TIME_US); > #endif > + } > } > > static void I don't think this goes far enough. We should stop claiming accurate vblanks when MIPI/DSI is used. intel_get_crtc_scanline will currently spin for 100 us to see if we can move from scanline offset = 0, this means that we add an additional 100 us wait for MIPI/DSI always. i915_get_crtc_scanoutpos should return false as well. Does this mean need_vlv_dsi_wa in intel_pipe_update_start is now a noop? Should we perhaps only apply this for gen9+ MIPI/DSI? ~Maarten
On Thu, Sep 07, 2017 at 01:29:22PM +0200, Maarten Lankhorst wrote: > Op 05-09-17 om 15:35 schreef Mika Kahola: > > It appears that we cannot trust scanline counters when MIPI/DSI display is > > connected. In CI system this appears as flickering errors that randomly > > appear in test cases. To avoid this flickering, let's just silence atomic > > update failure in case with DSI panel. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102403 > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > --- > > drivers/gpu/drm/i915/intel_sprite.c | 32 +++++++++++++++++--------------- > > 1 file changed, 17 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > index b0d6e3e..8511072 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -205,23 +205,25 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state) > > if (intel_vgpu_active(dev_priv)) > > return; > > > > - 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, scanline_end); > > - } > > + if (!intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI)) { > > + 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, scanline_end); > > + } > > #ifdef CONFIG_DRM_I915_DEBUG_VBLANK_EVADE > > - else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) > > > - VBLANK_EVASION_TIME_US) > > - DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n", > > - pipe_name(pipe), > > - ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time), > > - VBLANK_EVASION_TIME_US); > > + else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) > > > + VBLANK_EVASION_TIME_US) > > + DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n", > > + pipe_name(pipe), > > + ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time), > > + VBLANK_EVASION_TIME_US); > > #endif > > + } > > } > > > > static void > > I don't think this goes far enough. We should stop claiming accurate vblanks when MIPI/DSI is used. > intel_get_crtc_scanline will currently spin for 100 us to see if we can move from scanline offset = 0, > this means that we add an additional 100 us wait for MIPI/DSI always. > > i915_get_crtc_scanoutpos should return false as well. > > Does this mean need_vlv_dsi_wa in intel_pipe_update_start is now a noop? Should we perhaps only apply this > for gen9+ MIPI/DSI? VLV/CHV scanline counter (more or less) works with DSI. It's only BXT+ that's totally broken.
On Thu, 2017-09-07 at 13:29 +0200, Maarten Lankhorst wrote: > Op 05-09-17 om 15:35 schreef Mika Kahola: > > > > It appears that we cannot trust scanline counters when MIPI/DSI > > display is > > connected. In CI system this appears as flickering errors that > > randomly > > appear in test cases. To avoid this flickering, let's just silence > > atomic > > update failure in case with DSI panel. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102403 > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > --- > > drivers/gpu/drm/i915/intel_sprite.c | 32 +++++++++++++++++------ > > --------- > > 1 file changed, 17 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > b/drivers/gpu/drm/i915/intel_sprite.c > > index b0d6e3e..8511072 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -205,23 +205,25 @@ void intel_pipe_update_end(struct > > intel_crtc_state *new_crtc_state) > > if (intel_vgpu_active(dev_priv)) > > return; > > > > - 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, > > scanline_end); > > - } > > + if (!intel_crtc_has_type(new_crtc_state, > > INTEL_OUTPUT_DSI)) { > > + 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, > > scanline_end); > > + } > > #ifdef CONFIG_DRM_I915_DEBUG_VBLANK_EVADE > > - else if (ktime_us_delta(end_vbl_time, crtc- > > >debug.start_vbl_time) > > > - VBLANK_EVASION_TIME_US) > > - DRM_WARN("Atomic update on pipe (%c) took %lld us, > > max time under evasion is %u us\n", > > - pipe_name(pipe), > > - ktime_us_delta(end_vbl_time, crtc- > > >debug.start_vbl_time), > > - VBLANK_EVASION_TIME_US); > > + else if (ktime_us_delta(end_vbl_time, crtc- > > >debug.start_vbl_time) > > > + VBLANK_EVASION_TIME_US) > > + DRM_WARN("Atomic update on pipe (%c) took > > %lld us, max time under evasion is %u us\n", > > + pipe_name(pipe), > > + ktime_us_delta(end_vbl_time, > > crtc->debug.start_vbl_time), > > + VBLANK_EVASION_TIME_US); > > #endif > > + } > > } > > > > static void > I don't think this goes far enough. We should stop claiming accurate > vblanks when MIPI/DSI is used. > intel_get_crtc_scanline will currently spin for 100 us to see if we > can move from scanline offset = 0, > this means that we add an additional 100 us wait for MIPI/DSI always. > Definitely there's room for optimization here. We could get rid of those delays. > i915_get_crtc_scanoutpos should return false as well. > > Does this mean need_vlv_dsi_wa in intel_pipe_update_start is now a > noop? Should we perhaps only apply this > for gen9+ MIPI/DSI? We should limit this to gen9+ MIPI/DSI. The BSpec says that MIPI DSI is not supported from Broxton onwards. > > ~Maarten >
On Thu, Sep 07, 2017 at 01:29:22PM +0200, Maarten Lankhorst wrote: > Op 05-09-17 om 15:35 schreef Mika Kahola: > > It appears that we cannot trust scanline counters when MIPI/DSI display is > > connected. In CI system this appears as flickering errors that randomly > > appear in test cases. To avoid this flickering, let's just silence atomic > > update failure in case with DSI panel. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102403 > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > --- > > drivers/gpu/drm/i915/intel_sprite.c | 32 +++++++++++++++++--------------- > > 1 file changed, 17 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > index b0d6e3e..8511072 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -205,23 +205,25 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state) > > if (intel_vgpu_active(dev_priv)) > > return; > > > > - 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, scanline_end); > > - } > > + if (!intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI)) { > > + 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, scanline_end); > > + } > > #ifdef CONFIG_DRM_I915_DEBUG_VBLANK_EVADE > > - else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) > > > - VBLANK_EVASION_TIME_US) > > - DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n", > > - pipe_name(pipe), > > - ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time), > > - VBLANK_EVASION_TIME_US); > > + else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) > > > + VBLANK_EVASION_TIME_US) > > + DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n", > > + pipe_name(pipe), > > + ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time), > > + VBLANK_EVASION_TIME_US); > > #endif > > + } > > } > > > > static void > > I don't think this goes far enough. We should stop claiming accurate vblanks when MIPI/DSI is used. > intel_get_crtc_scanline will currently spin for 100 us to see if we can move from scanline offset = 0, > this means that we add an additional 100 us wait for MIPI/DSI always. > > i915_get_crtc_scanoutpos should return false as well. Oh and the bigger problem is that we can't actually guarantee atomic updates without the vblank evade currently. I can't recall if BXT has the lock bit already somewhere. If it does we should probably start using it. Oh and we also have to make sure we sample the frame counter _after_ the lock has been released to make sure we do the necessary vblank waits and whatnot after the flip has been commited to the hardware.
On Wed, Sep 06, 2017 at 07:48:33PM +0300, Martin Peres wrote: > On 06/09/17 13:09, Mika Kahola wrote: > > On Tue, 2017-09-05 at 18:11 +0200, Daniel Vetter wrote: > > > On Tue, Sep 05, 2017 at 04:35:04PM +0300, Mika Kahola wrote: > > > > > > > > It appears that we cannot trust scanline counters when MIPI/DSI > > > > display is > > > > connected. In CI system this appears as flickering errors that > > > > randomly > > > > appear in test cases. To avoid this flickering, let's just silence > > > > atomic > > > > update failure in case with DSI panel. > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102403 > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > > This just changes a loud atomic failure to a silent atomic failure. > > > What > > > we instead need to do is actually fix the bug, not hide it. > > > > > BSpec has a notion that (PIPE_SCANLINE) that is "Not supported with > > MIPI DSI." > > > > That's why I thought it might be ok to silence the error as the > > computation that we try to accomplish wouldn't work anyway. Maybe this > > way we could remove DSI from being blackllisted. > > I agree. If the HW can't do it, so what can we do here? > > As long as this is well documented and the userspace knows about this issue > (if anything relies on this feature), then what else can we do? > > With the relevant BSpec quotes added above the changes, I can give my: > Acked-by: Martin Peres <martin.peres@linux.intel.com> > > I would however like to know if this breaks any feature the userspace relies > on. Catching up on this, for the record: Lack of this breaks atomic. So yeah, not really an optional feature, and definitely not a failure mode we should flush out to hide it. And if the scanline trick doesn't work, then we need a different way to achieve the same. Just because the one way we use everywhere else doesn't work, doesn't mean atomic on dsi is going to be impossible. It would be real bad if that's the case. -Daniel
On Thu, Sep 07, 2017 at 02:59:11PM +0300, Ville Syrjälä wrote: > On Thu, Sep 07, 2017 at 01:29:22PM +0200, Maarten Lankhorst wrote: > > Op 05-09-17 om 15:35 schreef Mika Kahola: > > > It appears that we cannot trust scanline counters when MIPI/DSI display is > > > connected. In CI system this appears as flickering errors that randomly > > > appear in test cases. To avoid this flickering, let's just silence atomic > > > update failure in case with DSI panel. > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102403 > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_sprite.c | 32 +++++++++++++++++--------------- > > > 1 file changed, 17 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > > index b0d6e3e..8511072 100644 > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > @@ -205,23 +205,25 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state) > > > if (intel_vgpu_active(dev_priv)) > > > return; > > > > > > - 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, scanline_end); > > > - } > > > + if (!intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI)) { > > > + 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, scanline_end); > > > + } > > > #ifdef CONFIG_DRM_I915_DEBUG_VBLANK_EVADE > > > - else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) > > > > - VBLANK_EVASION_TIME_US) > > > - DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n", > > > - pipe_name(pipe), > > > - ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time), > > > - VBLANK_EVASION_TIME_US); > > > + else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) > > > > + VBLANK_EVASION_TIME_US) > > > + DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n", > > > + pipe_name(pipe), > > > + ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time), > > > + VBLANK_EVASION_TIME_US); > > > #endif > > > + } > > > } > > > > > > static void > > > > I don't think this goes far enough. We should stop claiming accurate vblanks when MIPI/DSI is used. > > intel_get_crtc_scanline will currently spin for 100 us to see if we can move from scanline offset = 0, > > this means that we add an additional 100 us wait for MIPI/DSI always. > > > > i915_get_crtc_scanoutpos should return false as well. > > Oh and the bigger problem is that we can't actually guarantee atomic > updates without the vblank evade currently. I can't recall if BXT has > the lock bit already somewhere. If it does we should probably start > using it. Oh and we also have to make sure we sample the frame counter > _after_ the lock has been released to make sure we do the necessary > vblank waits and whatnot after the flip has been commited to the > hardware. I thought that even on gen9 we still need the evasion because there's a bunch of stuff not under the lock bit? But yeah lock bit should be there, it's gen9. -Daniel
Hi Mika,
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.13 next-20170908]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Mika-Kahola/drm-i915-dsi-Silence-atomic-update-failure-with-DSI-panel/20170908-160933
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x001-201736 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_sprite.c: In function 'intel_pipe_update_end':
>> drivers/gpu/drm/i915/intel_sprite.c:209:27: error: 'new_crtc_state' undeclared (first use in this function)
if (!intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI)) {
^~~~~~~~~~~~~~
drivers/gpu/drm/i915/intel_sprite.c:209:27: note: each undeclared identifier is reported only once for each function it appears in
vim +/new_crtc_state +209 drivers/gpu/drm/i915/intel_sprite.c
170
171 /**
172 * intel_pipe_update_end() - end update of a set of display registers
173 * @crtc: the crtc of which the registers were updated
174 * @start_vbl_count: start vblank counter (used for error checking)
175 *
176 * Mark the end of an update started with intel_pipe_update_start(). This
177 * re-enables interrupts and verifies the update was actually completed
178 * before a vblank using the value of @start_vbl_count.
179 */
180 void intel_pipe_update_end(struct intel_crtc *crtc)
181 {
182 enum pipe pipe = crtc->pipe;
183 int scanline_end = intel_get_crtc_scanline(crtc);
184 u32 end_vbl_count = intel_crtc_get_vblank_counter(crtc);
185 ktime_t end_vbl_time = ktime_get();
186 struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
187
188 trace_i915_pipe_update_end(crtc, end_vbl_count, scanline_end);
189
190 /* We're still in the vblank-evade critical section, this can't race.
191 * Would be slightly nice to just grab the vblank count and arm the
192 * event outside of the critical section - the spinlock might spin for a
193 * while ... */
194 if (crtc->base.state->event) {
195 WARN_ON(drm_crtc_vblank_get(&crtc->base) != 0);
196
197 spin_lock(&crtc->base.dev->event_lock);
198 drm_crtc_arm_vblank_event(&crtc->base, crtc->base.state->event);
199 spin_unlock(&crtc->base.dev->event_lock);
200
201 crtc->base.state->event = NULL;
202 }
203
204 local_irq_enable();
205
206 if (intel_vgpu_active(dev_priv))
207 return;
208
> 209 if (!intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI)) {
210 if (crtc->debug.start_vbl_count &&
211 crtc->debug.start_vbl_count != end_vbl_count) {
212 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",
213 pipe_name(pipe), crtc->debug.start_vbl_count,
214 end_vbl_count,
215 ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time),
216 crtc->debug.min_vbl, crtc->debug.max_vbl,
217 crtc->debug.scanline_start, scanline_end);
218 }
219 #ifdef CONFIG_DRM_I915_DEBUG_VBLANK_EVADE
220 else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) >
221 VBLANK_EVASION_TIME_US)
222 DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n",
223 pipe_name(pipe),
224 ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time),
225 VBLANK_EVASION_TIME_US);
226 #endif
227 }
228 }
229
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
>-----Original Message----- >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of >Daniel Vetter >Sent: Friday, September 8, 2017 12:30 PM >To: Ville Syrjälä <ville.syrjala@linux.intel.com> >Cc: intel-gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [PATCH] drm/i915/dsi: Silence atomic update failure with >DSI panel > >On Thu, Sep 07, 2017 at 02:59:11PM +0300, Ville Syrjälä wrote: >> On Thu, Sep 07, 2017 at 01:29:22PM +0200, Maarten Lankhorst wrote: >> > Op 05-09-17 om 15:35 schreef Mika Kahola: >> > > It appears that we cannot trust scanline counters when MIPI/DSI >> > > display is connected. In CI system this appears as flickering >> > > errors that randomly appear in test cases. To avoid this >> > > flickering, let's just silence atomic update failure in case with DSI panel. >> > > >> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102403 >> > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> >> > > --- >> > > drivers/gpu/drm/i915/intel_sprite.c | 32 >> > > +++++++++++++++++--------------- >> > > 1 file changed, 17 insertions(+), 15 deletions(-) >> > > >> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c >> > > b/drivers/gpu/drm/i915/intel_sprite.c >> > > index b0d6e3e..8511072 100644 >> > > --- a/drivers/gpu/drm/i915/intel_sprite.c >> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c >> > > @@ -205,23 +205,25 @@ void intel_pipe_update_end(struct >intel_crtc_state *new_crtc_state) >> > > if (intel_vgpu_active(dev_priv)) >> > > return; >> > > >> > > - 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, scanline_end); >> > > - } >> > > + if (!intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI)) { >> > > + 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, scanline_end); >> > > + } >> > > #ifdef CONFIG_DRM_I915_DEBUG_VBLANK_EVADE >> > > - else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) > >> > > - VBLANK_EVASION_TIME_US) >> > > - DRM_WARN("Atomic update on pipe (%c) took %lld us, max time >under evasion is %u us\n", >> > > - pipe_name(pipe), >> > > - ktime_us_delta(end_vbl_time, crtc- >>debug.start_vbl_time), >> > > - VBLANK_EVASION_TIME_US); >> > > + else if (ktime_us_delta(end_vbl_time, crtc- >>debug.start_vbl_time) > >> > > + VBLANK_EVASION_TIME_US) >> > > + DRM_WARN("Atomic update on pipe (%c) took %lld us, >max time under evasion is %u us\n", >> > > + pipe_name(pipe), >> > > + ktime_us_delta(end_vbl_time, crtc- >>debug.start_vbl_time), >> > > + VBLANK_EVASION_TIME_US); >> > > #endif >> > > + } >> > > } >> > > >> > > static void >> > >> > I don't think this goes far enough. We should stop claiming accurate vblanks >when MIPI/DSI is used. >> > intel_get_crtc_scanline will currently spin for 100 us to see if we >> > can move from scanline offset = 0, this means that we add an additional 100 >us wait for MIPI/DSI always. >> > >> > i915_get_crtc_scanoutpos should return false as well. >> >> Oh and the bigger problem is that we can't actually guarantee atomic >> updates without the vblank evade currently. I can't recall if BXT has >> the lock bit already somewhere. If it does we should probably start >> using it. Oh and we also have to make sure we sample the frame counter >> _after_ the lock has been released to make sure we do the necessary >> vblank waits and whatnot after the flip has been commited to the >> hardware. > Yes, we definitely need the evasion even for DSI. Pipe Scanline register will not work for Gen9 DSI (it works on BYT/CHT though). The timings for DSI are driven from port unlike DDI. We can however use Pipe frame timestamp and current timestamp registers to logically calculate that. We have submitted a patch for the same. Please have a look https://patchwork.kernel.org/patch/9944249/. Regards, Uma Shankar >I thought that even on gen9 we still need the evasion because there's a bunch of >stuff not under the lock bit? But yeah lock bit should be there, it's gen9. >-Daniel >-- >Daniel Vetter >Software Engineer, Intel Corporation >http://blog.ffwll.ch >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index b0d6e3e..8511072 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -205,23 +205,25 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state) if (intel_vgpu_active(dev_priv)) return; - 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, scanline_end); - } + if (!intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI)) { + 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, scanline_end); + } #ifdef CONFIG_DRM_I915_DEBUG_VBLANK_EVADE - else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) > - VBLANK_EVASION_TIME_US) - DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n", - pipe_name(pipe), - ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time), - VBLANK_EVASION_TIME_US); + else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) > + VBLANK_EVASION_TIME_US) + DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n", + pipe_name(pipe), + ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time), + VBLANK_EVASION_TIME_US); #endif + } } static void
It appears that we cannot trust scanline counters when MIPI/DSI display is connected. In CI system this appears as flickering errors that randomly appear in test cases. To avoid this flickering, let's just silence atomic update failure in case with DSI panel. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102403 Signed-off-by: Mika Kahola <mika.kahola@intel.com> --- drivers/gpu/drm/i915/intel_sprite.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)