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

[Souza, Jose]

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.

> 
> > We also would need have a PSR2 reason set in no_fbc_reason and handle it in IGT.
> 
> I think that no_fbc_reason is rather pointless as well. Would make
> life a lot simpler if we didn't have to worry about it. So tempted
> to just nuke it.
> 
> > 
> > > +
> > >  	if (!cache->plane.visible)
> > >  		return;
> > >  
> > > 
> > > 
> > > 
> > 
>