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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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;

/*
 * 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

-- 
Ville Syrjälä
Intel