[Intel-gfx] [PATCH] drm/i915/display: Add a vblank wait for FBC activation within same frame

[Shankar, Uma]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: Thursday, September 10, 2020 7:22 PM
> To: Shankar, Uma <uma.shankar at intel.com>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>; intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Add a vblank wait for FBC
> activation within same frame
> 
> On Thu, Sep 10, 2020 at 01:42:23PM +0000, Shankar, Uma wrote:
> >
> >
> > > -----Original Message-----
> > > From: Jani Nikula <jani.nikula at linux.intel.com>
> > > Sent: Thursday, September 10, 2020 7:04 PM
> > > To: Shankar, Uma <uma.shankar at intel.com>;
> > > intel-gfx at lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Add a vblank wait
> > > for FBC activation within same frame
> > >
> > > On Thu, 10 Sep 2020, Uma Shankar <uma.shankar at intel.com> wrote:
> > > > Add a vblank wait when fbc activation request comes for the same
> > > > frame on TGL. This helps fix underrun related to fbc.
> > > >
> > > > Signed-off-by: Uma Shankar <uma.shankar at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_fbc.c | 13 +++++++++++--
> > > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > index 135f5e8a4d70..3e1d715e4a4e 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > @@ -1055,6 +1055,7 @@ static void __intel_fbc_post_update(struct
> > > > intel_crtc *crtc)  {
> > > >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > >  	struct intel_fbc *fbc = &dev_priv->fbc;
> > > > +	static u32 old_frame_cnt, new_frame_cnt;
> > >
> > > In the strongest terms, do not add static data. These would be
> > > shared across devices, which is certainly a bug.
> >
> > Oh ok, yeah will remove this. I guess adding them to intel_fbc should be ok
> right ?
> 
> I didn't think we found what's really going on here. So this still seems like
> random duct tape which just changs some timings.

Yeah Ville, this still is a WA to avoid multiple FBC activation within frame. Adding the frame counter checks
to avoid waits on all updates.

More analysis still ongoing and issue is raised with hw team as well for further analysis.
My idea was to get the health of this WA on CI and general feedback if this can be considered as a
fallback option.

Regards,
Uma Shankar

 > > >
> > > >  	drm_WARN_ON(&dev_priv->drm, !mutex_is_locked(&fbc->lock));
> > > >
> > > > @@ -1075,10 +1076,18 @@ static void __intel_fbc_post_update(struct
> > > intel_crtc *crtc)
> > > >  	if (!intel_fbc_can_activate(crtc))
> > > >  		return;
> > > >
> > > > -	if (!fbc->busy_bits)
> > > > +	old_frame_cnt = new_frame_cnt;
> > > > +	new_frame_cnt = intel_crtc_get_vblank_counter(crtc);
> > > > +	if (!fbc->busy_bits) {
> > > > +		if (IS_TIGERLAKE(dev_priv) &&
> > > > +		    old_frame_cnt == new_frame_cnt) {
> > > > +			drm_dbg_kms(&dev_priv->drm, "Wait for vblank before
> > > Activating FBC");
> > > > +			intel_wait_for_vblank_if_active(dev_priv, crtc->pipe);
> > > > +		}
> > > >  		intel_fbc_hw_activate(dev_priv);
> > > > -	else
> > > > +	} else {
> > > >  		intel_fbc_deactivate(dev_priv, "frontbuffer write");
> > > > +	}
> > > >  }
> > > >
> > > >  void intel_fbc_post_update(struct intel_atomic_state *state,
> > >
> > > --
> > > Jani Nikula, Intel Open Source Graphics Center
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel