Re: [PATCH v3 1/4] drm/i915/display: avoid calling fbc activate if fbc is active

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

On Fri, 2025-01-17 at 14:49 +0200, Ville Syrjälä wrote:
> On Tue, Jan 14, 2025 at 02:07:16PM +0200, Vinod Govindapillai wrote:
> > If FBC is already active, we don't need to call FBC activate
> > routine again during the post plane update. As this will
> > explicitly call the nuke and also rewrite the FBC ctl registers.
> > "intel_atomic_commit_tail-> intel_post_plane_update->
> > intel_fbc_post_update-> _intel_fbc_post_update" path will be
> > executed during the normal flip cases. FBC HW will nuke on sync
> > flip event and driver do not need to call the nuke explicitly.
> > This is much more relevant in case of dirty rectangle support
> > in FBC with the followup patches. Nuke on flip in that case will
> > remove all the benefits of fetching only the modified region.
> > 
> > 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.
> > 
> > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_fbc.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index df05904bac8a..fd540ff5e57e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -1561,7 +1561,8 @@ static void __intel_fbc_post_update(struct intel_fbc *fbc)
> >  	fbc->flip_pending = false;
> >  	fbc->busy_bits = 0;
> >  
> > -	intel_fbc_activate(fbc);
> > +	if (!fbc->active)
> > +		intel_fbc_activate(fbc);
> 
> We'll need to keep the actual activate part (eg. to update the fence).
> But we should be able to elide the explicit nuke if FBC was already
> active (that implies a flip nuke has occurred anyway, vs. if FBC was
> previously disabled then it might have been disabled by a frontbuffer
> invalidate and if it hasn't been disabled for a full frame then the
> hardware won't automagically cause a nuke when we reactivate it).

Thanks Ville!

Okay! I have something like this now! But facing some weird issues!

index df05904bac8a..f05c61040d19 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -739,10 +739,22 @@ static void intel_fbc_nuke(struct intel_fbc *fbc)
 
 static void intel_fbc_activate(struct intel_fbc *fbc)
 {
+       bool fbc_already_active;
+
        lockdep_assert_held(&fbc->lock);
 
+       fbc_already_active = fbc->active;
+
        intel_fbc_hw_activate(fbc);
-       intel_fbc_nuke(fbc);
+
+       /*
+        * If FBC is already active, don't nuke.
+        * In normal flips after FBC is enabled, FBC hw will nuke on flip
+        * In case of frontbuffer rendering cases, invalidate, flush sequence
+        * will handle the nuke
+        */
+       if (!fbc_already_active)
+               intel_fbc_nuke(fbc);

So the nuke won't be called from normal flips if fbc is already active. So the intel_fbc_hw_activate
will be called always - which programs override stride, no fences in case of xe, and reprograms
FBC_CTL and FBC_DIRTYRECT_CTL registers with the same values!

But the weird thing is, with this damaged area update don't have any effect. The whole region is
getting updated! But if I avoid calling the intel_fbc_hw_activate() completely, i can see only those
damaged rect area being updated!

Initially I thought as we rewrite FBC_DIRTYRECT_CTL enable again, that could cause the first frame
being taking the whole plane size as the update region. But after experimenting with  those,
narrowed it to glk_fbc_program_cfb_stride() call! Somehow programming glk_fbc_program_cfb_stride()
is causing entire region being updated!  Do you have any pointers on this?

Thanks
Vinod


> 
> >  }
> >  
> >  void intel_fbc_post_update(struct intel_atomic_state *state,
> > -- 
> > 2.43.0
>