Re: [PATCH 18/18] drm/i915/ddi: prefer read_poll_timeout() over readx_poll_timeout()

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

On Fri, Jun 27, 2025 at 07:26:22PM +0300, Jani Nikula wrote:
> On Fri, 27 Jun 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Jun 27, 2025 at 04:34:23PM +0300, Jani Nikula wrote:
> >> Internally the macro has:
> >> 
> >> #define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
> >> 				sleep_before_read, args...) \
> >> 
> >> ...
> >> 
> >> 		(val) = op(args); \
> >> 
> >> So you do need to provide an lvalue val, and you need to be able to add
> >> () after op. I think GCC allows not passing varargs. IOW you'd need to
> >> implement another macro (which could be used to implement the existing
> >> one, but not the other way round).
> >
> > Just get rid of the 'args' and 'val' and it'll work just fine.
> > Then it'll be almost identical to wait_for() (basically just missing the
> > increasing backoff stuff).
> >
> > AFAICS this thing was originally added just for reading a single
> > register and checking some bit/etc, so the name made some sense.
> > But now we're abusing it for all kinds of random things, so even
> > the name no longer makes that much sense.
> 
> Yeah, evolution not intelligent design.
> 
> > So I think just something like this would work fine for us:
> 
> LGTM, with the _atomic version for completeness.

The other differences between wait_for() and read_poll_timeout()
I see are:

- read_poll_timeout() always evaluates 'cond' at least twice.
  For some things I think it would make sense to omit 'op'
  entirely so we don't have to introduce pointless variables
  in the caller (eg. poll_timeout(, pipe_scanline_is_moving(...), ...))

  but the double evaluation of 'cond' there is not desirable.
  Should be an easy change to make read_poll_timeout() more
  like wait_for() and stash the return value into a variable.

- ktime_get() vs. ktime_get_raw(). I suppose it doesn't really
  matter too much which is used?

- 'op' and 'cond' are evaluated twice during the same iteration of
  the loop for the timeout case in read_poll_timeout(). wait_for()
  is a bit more optimal here by sampling the timeout first, then
  doing the 'op'+'cond', and finally checking whether the timeout
  happened.

  I suppose optimizing the timeout case isn't very critical. Though
  the code would be a bit less repetitive, with the caveat that we
  need an extra variable for the timeout result.

- wait_for() has an explicit compiler barrier to make sure 'cond'
  and the timeout evaluation aren't reordered. Though I think it's
  in the wrong spot for the cases where 'op' is the one that samples
  the thing that 'cond' checks.

  However I *think* ktime_get() being a function call should be enough
  to prevent that reordering from happening?

I guess I'll see what I can cook up to make this stuff
more agreeable...

-- 
Ville Syrjälä
Intel