Re: [PATCH v7 5/7] drm/i915/xe3: avoid calling fbc activate if fbc is active

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

On Wed, 2025-02-12 at 20:30 +0200, Ville Syrjälä wrote:
> On Wed, Feb 12, 2025 at 03:14:18PM +0200, Vinod Govindapillai wrote:
> > If FBC is already active, we don't need to call FBC activate
> > routine again. This is more relevant in case of dirty rect
> > support in FBC. Xe doesn't support legacy fences. Hence fence
> > programming also not required as part of this fbc_hw_activate.
> > Any FBC related register updates done after enabling the dirty
> > rect support in xe3 will trigger nuke by FBC HW. So avoid
> > calling fbc activate routine again if the FBC is already active.
> > 
> > The front buffer rendering sequence will call intel_fbc_flush()
> > and which will call intel_fbc_nuke() or intel_fbc_activate()
> > based on FBC status explicitly and won't get impacted by this
> > change.
> > 
> > v2: use HAS_FBC_DIRTY_RECT()
> >     move this functionality within intel_fbc_activate()
> > 
> > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_fbc.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index df05904bac8a..951dc81b7b97 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -739,8 +739,19 @@ static void intel_fbc_nuke(struct intel_fbc *fbc)
> >  
> >  static void intel_fbc_activate(struct intel_fbc *fbc)
> >  {
> > +	struct intel_display *display = fbc->display;
> > +
> >  	lockdep_assert_held(&fbc->lock);
> >  
> > +	/*
> > +	 * When dirty rectangle is enabled, any updates to FBC registers will
> > +	 * trigger nuke. So avoid calling intel_fbc_activate if fbc is already
> > +	 * active and for XE3 cases. Xe doesn't support legacy fences. So
> > +	 * no need to update the fences as well.
> 
> I have no idea what XE3 and Xe here mean. I would just state
> that platforms which have dirty rect don't have fences.
> 
> > +	 */
> > +	if (HAS_FBC_DIRTY_RECT(display) && fbc->active)
> > +		return;
> 
> I don't quite like the assumptions being made here.
> 
> Since only the fence can change upon flip nuke we should
> probably check for intel_fbc_has_fences() instead of
> HAS_DIRTY_RECT() and thus just skip this on all platforms
> that don't have fences. That also increases our testing
> coverage for this short circuit path, which is a good thing.
> 
> Ideally I guess we should check if the fence is actually
> changing or not, but we don't have the old state around
> anymore so can't do it right now.
> 
> So I guess we could do something like:
> /* only the fence can change for a flip nuke */
> if (fbc->active && !has_fences())
> 	return;

Okay. I wasn't sure if any older platforms had any such dependency on fences and stride!

BR
Vinod

> 
> /*
>  * the explanation about the FBC register write
>  * nuke vs. dirty rect stuff.
>  */
> drm_WARN_ON(fbc->active && HAS_DIRTY_RECT());
> 
> > +
> >  	intel_fbc_hw_activate(fbc);
> >  	intel_fbc_nuke(fbc);
> >  
> > -- 
> > 2.43.0
>