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 |
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
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 --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,
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(-)