[Intel-gfx] [PATCH] drm/i915/dsi: Silence atomic update failure with DSI panel

[Shankar, Uma]

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