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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Nov 11, 2025 at 10:01:14AM +0200, Jani Nikula wrote:
> 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.

At the end of this series we have intel_de_wait_fw_us_atomic()
which is the only thing that anyone should call from atomic
context. Granted the _us() stuff all still work in atomic
context due to the underlying uncore implementation. But
when we switch to poll_timeout_us() I plan to make all the
other stuff unusable from atomic contexts.

Ideally we probably shouldn't even have intel_de_wait_fw_us_atomic().
I think the wakelock code is the only place that needs it,
due to the spinlock presumably. I haven't checked if we could
just eg. make that a mutex and use the non-atomic thing there
as well.

-- 
Ville Syrjälä
Intel