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@xxxxxxxxx>
> ---
Makes sense.
Reviewed-by: Luca Coelho <luciano.coelho@xxxxxxxxx>
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.
