| Message ID | 20241021222744.294371-2-gustavo.sousa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dmc_wl: Fixes and enablement for Xe3_LPD | expand |
On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote: > In upcoming display changes, we will modify the DMC wakelock MMIO > waiting code to choose a non-sleeping variant implementation, because > the wakelock is also taking in atomic context. > > While xe provides an explicit parameter (namely "atomic") to prevent > xe_mmio_wait32() from sleeping, i915 does not and implements that > behavior when slow_timeout_ms is zero. > > So, for now, let's mimic what i915 does to allow for display to use > non-sleeping MMIO wait. In the future, we should come up with a better > and explicit interface for this behavior in i915, at least while display > code is not an independent entity with proper interfaces between xe and > i915. > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> > --- Makes sense. Reviewed-by: Luca Coelho <luciano.coelho@intel.com> Just one question/comment below. > .../gpu/drm/xe/compat-i915-headers/intel_uncore.h | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h > index 0382beb4035b..5a57f76c1760 100644 > --- a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h > +++ b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h > @@ -117,10 +117,21 @@ __intel_wait_for_register(struct intel_uncore *uncore, i915_reg_t i915_reg, > unsigned int slow_timeout_ms, u32 *out_value) > { > struct xe_reg reg = XE_REG(i915_mmio_reg_offset(i915_reg)); > + bool atomic; > + > + /* > + * FIXME: We are trying to replicate the behavior from i915 here, in > + * which sleep is not performed if slow_timeout_ms == 0. This hack is > + * necessary because of paths in display code that are executed in > + * atomic context. Setting the atomic flag based on timeout values > + * doesn't feel very robust. Ideally, we should have a proper interface > + * for explicitly choosing non-sleeping behavior. I think this is just a matter of semantics. It would look nicer to have a more intuitive interface, but I don't think the i915 implementation is any less robust per se. If this behavior is documented properly, I don't see it as a real issue. -- Cheers, Luca.
Quoting Luca Coelho (2024-11-01 07:57:58-03:00) >On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote: >> In upcoming display changes, we will modify the DMC wakelock MMIO >> waiting code to choose a non-sleeping variant implementation, because >> the wakelock is also taking in atomic context. >> >> While xe provides an explicit parameter (namely "atomic") to prevent >> xe_mmio_wait32() from sleeping, i915 does not and implements that >> behavior when slow_timeout_ms is zero. >> >> So, for now, let's mimic what i915 does to allow for display to use >> non-sleeping MMIO wait. In the future, we should come up with a better >> and explicit interface for this behavior in i915, at least while display >> code is not an independent entity with proper interfaces between xe and >> i915. >> >> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> >> --- > >Makes sense. > >Reviewed-by: Luca Coelho <luciano.coelho@intel.com> Thanks! > >Just one question/comment below. > > >> .../gpu/drm/xe/compat-i915-headers/intel_uncore.h | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h >> index 0382beb4035b..5a57f76c1760 100644 >> --- a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h >> +++ b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h >> @@ -117,10 +117,21 @@ __intel_wait_for_register(struct intel_uncore *uncore, i915_reg_t i915_reg, >> unsigned int slow_timeout_ms, u32 *out_value) >> { >> struct xe_reg reg = XE_REG(i915_mmio_reg_offset(i915_reg)); >> + bool atomic; >> + >> + /* >> + * FIXME: We are trying to replicate the behavior from i915 here, in >> + * which sleep is not performed if slow_timeout_ms == 0. This hack is >> + * necessary because of paths in display code that are executed in >> + * atomic context. Setting the atomic flag based on timeout values >> + * doesn't feel very robust. Ideally, we should have a proper interface >> + * for explicitly choosing non-sleeping behavior. > >I think this is just a matter of semantics. It would look nicer to >have a more intuitive interface, but I don't think the i915 >implementation is any less robust per se. If this behavior is >documented properly, I don't see it as a real issue. Ah, well... Yeah, I guess I was too hard on i915. I'll replace this comment with a quick note only mentioning that we are replicating the behavior then. Thanks! -- Gustavo Sousa
diff --git a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h index 0382beb4035b..5a57f76c1760 100644 --- a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h +++ b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h @@ -117,10 +117,21 @@ __intel_wait_for_register(struct intel_uncore *uncore, i915_reg_t i915_reg, unsigned int slow_timeout_ms, u32 *out_value) { struct xe_reg reg = XE_REG(i915_mmio_reg_offset(i915_reg)); + bool atomic; + + /* + * FIXME: We are trying to replicate the behavior from i915 here, in + * which sleep is not performed if slow_timeout_ms == 0. This hack is + * necessary because of paths in display code that are executed in + * atomic context. Setting the atomic flag based on timeout values + * doesn't feel very robust. Ideally, we should have a proper interface + * for explicitly choosing non-sleeping behavior. + */ + atomic = !slow_timeout_ms && fast_timeout_us > 0; return xe_mmio_wait32(__compat_uncore_to_mmio(uncore), reg, mask, value, fast_timeout_us + 1000 * slow_timeout_ms, - out_value, false); + out_value, atomic); } static inline u32 intel_uncore_read_fw(struct intel_uncore *uncore,
In upcoming display changes, we will modify the DMC wakelock MMIO waiting code to choose a non-sleeping variant implementation, because the wakelock is also taking in atomic context. While xe provides an explicit parameter (namely "atomic") to prevent xe_mmio_wait32() from sleeping, i915 does not and implements that behavior when slow_timeout_ms is zero. So, for now, let's mimic what i915 does to allow for display to use non-sleeping MMIO wait. In the future, we should come up with a better and explicit interface for this behavior in i915, at least while display code is not an independent entity with proper interfaces between xe and i915. Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> --- .../gpu/drm/xe/compat-i915-headers/intel_uncore.h | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)