>-----Original Message----- >From: Intel-gfx [mailto:[email protected]] On Behalf Of >Daniel Vetter >Sent: Friday, September 8, 2017 12:30 PM >To: Ville Syrjälä <[email protected]> >Cc: [email protected] >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 <[email protected]> >> > > --- >> > > 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 >[email protected] >https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
