Re: [PATCH 00/16] drm/i915/de: Register polling cleanup

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

On Mon, 10 Nov 2025, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Clean up the register polling stuff:
> - rename the current wait stuff to
>   intel_de_wait_{,for_set,for_clear}_ms()
> - introduce intel_de_wait_{,for_set,for_clear}_us()
> - nuke intel_de_wait_custom()
> - change the wakelock stuff to use _fw() instead of
>   hand rolling yet another level of register accessors
> - a few other minor cleanups
>
> After this it should be fairly easy to switch over to
> poll_timeout_us().

Overall the series looks fine.

Acked-by: Jani Nikula <jani.nikula@intel.com>

since Suraj already did the detailed review.

One questions remains unanswered, and I guess I'll have to wait for
follow-up to see the answer. I really *really* dislike how the i915
variants are somewhat ambiguously atomic, i.e. atomic when slow timeout
is 0, and the xe wrappers replicate that behaviour. xe_mmio_wait32() has
atomic as parameter.

I would like the atomic variants to be explicit, and separate. Similar
to poll_timeout_us() and poll_timeout_us_atomic(). You immediately know
from the call whether you're doing the right thing or not. And that
really should not directly depend on the timeout length. Since you plan
on using the generic poll helpers, I presume this will propagate to the
register polling helpers.

Since we do a lot of non-atomic millisecond waits, I guess it's worth
having the _ms variants, although like I said before, I'd kind of like
going for _us all over the place. But no big deal, in the end the _ms
variants can be trivial wrappers on the non-atomic _us ones.

BR,
Jani.

>
> Ville Syrjälä (16):
>   drm/i915/de: Implement register waits one way
>   drm/i915/de: Have intel_de_wait() hand out the final register value
>   drm/i915/de: Include units in intel_de_wait*() function names
>   drm/i915/de: Introduce intel_de_wait_us()
>   drm/i915/de: Use intel_de_wait_us()
>   drm/i915/de: Use intel_de_wait_ms() for the obvious cases
>   drm/i915/de: Nuke intel_de_wait_custom()
>   drm/i915/de: Introduce intel_de_wait_for_{set,clear}_us()
>   drm/i915/de: Use intel_de_wait_for_{set,clear}_us()
>   drm/i915/de: Use intel_de_wait_for_{set,clear}_ms()
>   drm/1915/dpio: Stop using intel_de_wait_fw_ms()
>   drm/u195/de: Replace __intel_de_rmw_nowl() with intel_de_rmw_fw()
>   drm/i915/de: Nuke wakelocks from intel_de_wait_fw_ms()
>   drm/i915/de: Replace __intel_de_wait_for_register_nowl() with
>     intel_de_wait_fw_us_atomic()
>   drm/i915/power: Use the intel_de_wait_ms() out value
>   drm/i915/dpio: Use the intel_de_wait_ms() out value
>
>  drivers/gpu/drm/i915/display/hsw_ips.c        |   4 +-
>  drivers/gpu/drm/i915/display/icl_dsi.c        |  35 +++---
>  drivers/gpu/drm/i915/display/intel_cdclk.c    |  30 +++--
>  drivers/gpu/drm/i915/display/intel_crt.c      |  16 +--
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c  |  98 ++++++++--------
>  drivers/gpu/drm/i915/display/intel_ddi.c      |  34 +++---
>  drivers/gpu/drm/i915/display/intel_de.h       | 107 +++++++++---------
>  drivers/gpu/drm/i915/display/intel_display.c  |   4 +-
>  .../drm/i915/display/intel_display_power.c    |  14 +--
>  .../i915/display/intel_display_power_well.c   |  42 +++----
>  drivers/gpu/drm/i915/display/intel_dmc_wl.c   |  25 ++--
>  drivers/gpu/drm/i915/display/intel_dp_aux.c   |   6 +-
>  drivers/gpu/drm/i915/display/intel_dp_hdcp.c  |  14 +--
>  drivers/gpu/drm/i915/display/intel_dpio_phy.c |  10 +-
>  drivers/gpu/drm/i915/display/intel_dpll.c     |   4 +-
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c |  27 +++--
>  drivers/gpu/drm/i915/display/intel_fbc.c      |   4 +-
>  drivers/gpu/drm/i915/display/intel_flipq.c    |   8 +-
>  drivers/gpu/drm/i915/display/intel_gmbus.c    |   2 +-
>  drivers/gpu/drm/i915/display/intel_hdcp.c     |  49 ++++----
>  drivers/gpu/drm/i915/display/intel_hdmi.c     |   4 +-
>  drivers/gpu/drm/i915/display/intel_lt_phy.c   |  57 +++++-----
>  drivers/gpu/drm/i915/display/intel_lvds.c     |   6 +-
>  .../gpu/drm/i915/display/intel_pch_display.c  |  12 +-
>  .../gpu/drm/i915/display/intel_pch_refclk.c   |  10 +-
>  drivers/gpu/drm/i915/display/intel_pmdemand.c |  18 +--
>  drivers/gpu/drm/i915/display/intel_psr.c      |  10 +-
>  drivers/gpu/drm/i915/display/intel_sbi.c      |   6 +-
>  drivers/gpu/drm/i915/display/intel_snps_phy.c |   8 +-
>  drivers/gpu/drm/i915/display/intel_tc.c       |   8 +-
>  drivers/gpu/drm/i915/display/intel_vrr.c      |   6 +-
>  drivers/gpu/drm/i915/display/vlv_dsi.c        |  54 ++++-----
>  drivers/gpu/drm/i915/display/vlv_dsi_pll.c    |   8 +-
>  .../drm/xe/compat-i915-headers/intel_uncore.h |  31 ++---
>  34 files changed, 369 insertions(+), 402 deletions(-)

-- 
Jani Nikula, Intel