Re: [PATCH v2 2/7] drm/i915/ddi: use intel_de_wait_custom() instead of wait_for_us()

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 31 Jul 2025, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> Quoting Jani Nikula (2025-07-31 07:05:09-03:00)
>>Prefer the register read specific wait function over i915 wait_for_us().
>>
>>v2: Wait for bits to clear in mtl_ddi_disable_d2d()
>>
>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>---
>> drivers/gpu/drm/i915/display/intel_ddi.c | 14 ++++++++++++--
>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
>>index 0405396c7750..866ed3e46664 100644
>>--- a/drivers/gpu/drm/i915/display/intel_ddi.c
>>+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>>@@ -2561,6 +2561,7 @@ mtl_ddi_enable_d2d(struct intel_encoder *encoder)
>>         enum port port = encoder->port;
>>         i915_reg_t reg;
>>         u32 set_bits, wait_bits;
>>+        int ret;
>> 
>>         if (DISPLAY_VER(display) < 14)
>>                 return;
>>@@ -2576,7 +2577,11 @@ mtl_ddi_enable_d2d(struct intel_encoder *encoder)
>>         }
>> 
>>         intel_de_rmw(display, reg, 0, set_bits);
>>-        if (wait_for_us(intel_de_read(display, reg) & wait_bits, 100)) {
>>+
>>+        ret = intel_de_wait_custom(display, reg,
>>+                                   wait_bits, wait_bits,
>>+                                   100, 0, NULL);
>
> Hm... Strictly speaking, in the previous version, we are waiting for the
> register value with wait_bits mask applied to be any non-zero value.
> But, since wait_bits is just a single bit, I think using
> intel_de_wait_custom() works fine here.
>
> Perhaps using plural in wait_bits was not precise?

Yeah, I guess you could say there's a slight semantic change here, but
no functional change.

And if you ask me, I usually dislike "bits" in naming, and generally
prefer "mask". When "mask" is not appropriate, I think I try to find a
more meaningful name than "bits". But I digress.

> Anyways,
>
> Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

Thanks, pushed the series to din.

BR,
Jani.

>
>
>>+        if (ret) {
>>                 drm_err(display->drm, "Timeout waiting for D2D Link enable for DDI/PORT_BUF_CTL %c\n",
>>                         port_name(port));
>>         }
>>@@ -3058,6 +3063,7 @@ mtl_ddi_disable_d2d(struct intel_encoder *encoder)
>>         enum port port = encoder->port;
>>         i915_reg_t reg;
>>         u32 clr_bits, wait_bits;
>>+        int ret;
>> 
>>         if (DISPLAY_VER(display) < 14)
>>                 return;
>>@@ -3073,7 +3079,11 @@ mtl_ddi_disable_d2d(struct intel_encoder *encoder)
>>         }
>> 
>>         intel_de_rmw(display, reg, clr_bits, 0);
>>-        if (wait_for_us(!(intel_de_read(display, reg) & wait_bits), 100))
>>+
>>+        ret = intel_de_wait_custom(display, reg,
>>+                                   wait_bits, 0,
>>+                                   100, 0, NULL);
>>+        if (ret)
>>                 drm_err(display->drm, "Timeout waiting for D2D Link disable for DDI/PORT_BUF_CTL %c\n",
>>                         port_name(port));
>> }
>>-- 
>>2.39.5
>>

-- 
Jani Nikula, Intel