Re: [PATCH 01/13] drm/xe: Mimic i915 behavior for non-sleeping MMIO wait

[Gustavo Sousa <gustavo.sousa@intel.com>]

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