diff mbox series

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

Message ID 20250114120719.191372-2-vinod.govindapillai@intel.com (mailing list archive)
State New
Headers show
Series drm/i915/xe3: FBC Dirty rect feature support | expand

Commit Message

Govindapillai, Vinod Jan. 14, 2025, 12:07 p.m. UTC
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(-)

Comments

Ville Syrjala Jan. 17, 2025, 12:49 p.m. UTC | #1
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).

>  }
>  
>  void intel_fbc_post_update(struct intel_atomic_state *state,
> -- 
> 2.43.0
Govindapillai, Vinod Jan. 21, 2025, 8:55 a.m. UTC | #2
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
>
diff mbox series

Patch

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);
 }
 
 void intel_fbc_post_update(struct intel_atomic_state *state,