| Message ID | 20250212131420.60026-6-vinod.govindapillai@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/fbc: FBC Dirty rect feature support | expand |
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@intel.com> > --- > 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
On Wed, 2025-02-12 at 20:30 +0200, Ville Syrjälä wrote: > 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@intel.com> > > --- > > 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; Okay. I wasn't sure if any older platforms had any such dependency on fences and stride! BR Vinod > > /* > * 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 >
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. + */ + if (HAS_FBC_DIRTY_RECT(display) && fbc->active) + return; + intel_fbc_hw_activate(fbc); intel_fbc_nuke(fbc);
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@intel.com> --- drivers/gpu/drm/i915/display/intel_fbc.c | 11 +++++++++++ 1 file changed, 11 insertions(+)