[Intel-gfx] [v3 1/2] drm/i915/display/tgl: Disable FBC with PSR2

[Shankar, Uma]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: Friday, November 27, 2020 8:16 PM
> To: Souza, Jose <jose.souza at intel.com>
> Cc: Shankar, Uma <uma.shankar at intel.com>; intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [v3 1/2] drm/i915/display/tgl: Disable FBC with PSR2
> 
> On Wed, Nov 25, 2020 at 05:52:10PM +0000, Souza, Jose wrote:
> > On Wed, 2020-11-25 at 18:17 +0200, Ville Syrjälä wrote:
> > > On Tue, Nov 24, 2020 at 10:03:35PM +0000, Souza, Jose wrote:
> > > > On Fri, 2020-11-20 at 01:06 +0530, Uma Shankar wrote:
> > > > > There are some corner cases wrt underrun when we enable FBC with
> > > > > PSR2 on TGL. Recommendation from hardware is to keep this
> > > > > combination disabled.
> > > > >
> > > > > Bspec: 50422 HSD: 14010260002
> > > > >
> > > > > v2: Added psr2 enabled check from crtc_state (Anshuman) Added
> > > > > Bspec link and HSD referneces (Jose)
> > > > >
> > > > > v3: Moved the logic to disable fbc to
> > > > > intel_fbc_update_state_cache and removed the crtc->config usages, as
> per Ville's recommendation.
> > > > >
> > > > > Signed-off-by: Uma Shankar <uma.shankar at intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_fbc.c | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > > index a5b072816a7b..cb29c6f068f9 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > > @@ -701,6 +701,15 @@ static void intel_fbc_update_state_cache(struct
> intel_crtc *crtc,
> > > > >  	struct drm_framebuffer *fb = plane_state->hw.fb;
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >  	cache->plane.visible = plane_state->uapi.visible;
> > > > > +
> > > > > +	/*
> > > > > +	 * Tigerlake is not supporting FBC with PSR2.
> > > > > +	 * Recommendation is to keep this combination disabled
> > > > > +	 * Bspec: 50422 HSD: 14010260002
> > > > > +	 */
> > > > > +	if (crtc_state->has_psr2 && IS_TIGERLAKE(dev_priv))
> > > > > +		cache->plane.visible = false;
> > > >
> > > > Looks like a hack to me, would be better add a psr2 variable in
> intel_fbc_state_cache.
> > >
> > > The plan is to remove most things from that cache anyway since it's
> > > mostly pointless stuff that should just be handled directly via the
> > > plane/crtc states. Not really convinced it makes sense to add more
> > > crap to it at this time. So IMO this is good enough for now.
> >
> > When this will happen? if soon okay.
> > If there is no ETA IMHO is better do the right thing.
> 
> I was hoping to get back to it soon, but looks like there's quite a bit more urgent
> work ahead for the moment. So don't know when I'll get back to this.
> 
> So I guess path of least resitance would be for Uma to respin with your
> suggested approach. It was one of the solutions I also suggested originally, but I
> did also suggest this simpler version Uma actually did.

Ok, let me send out a version with psr2 variable in state cache. Please review if that looks
better.

Regards,
Uma Shankar

> --
> Ville Syrjälä
> Intel