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