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@xxxxxxxxx>
> ---
> 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
