On Wed, 2026-09-09 at 11:00 +0300, Jani Nikula wrote:
> On Tue, 08 Sep 2026, Alan Previn <[email protected]> wrote:
> > Check for overflow in udelay/usleep_range use in __xe_mmio_wait32
> > and pick the correct helper according to the wait time and atomic.
> > Implement a similar helper to replace DIV_ROUND_UP for 32-bit CPUs.
> > Avoid growing delays becoming intollerably large by capping the
> > in-loop wait time.
>
> I think it might be a better idea to remove the exponentially growing
> wait time instead. It might have seemed good on paper, but you should do
> the math in typical use cases and see how it actually behaves. (Spoiler:
> Lots of useless back-to-back reads in the beginning, and then quickly
> increasing to waits beyond the timeout and delay maximums.)
>
> This is code that should be possible to understand. I'd suggest
> simplifying the whole thing instead of making it more complex.
>
> I think it would be much better to migrate xe mmio to use
> poll_timeout_us() and poll_timeout_us_atomic() instead. For the rare
> cases that actually need the quick hammering followed by slower waits,
> you can trivially do two calls with delays and timeouts considered for
> *that* particular use case.
alan: right - will do.
>
> I also think it's a mistake (copy-pasted from i915) to specify "atomic"
> as a parameter. They should be separate functions all the way from the
> caller.
>
alan: not sure i understand why this is a mistake - the caller needs to
ensure they call with the correct parameter - so its either they call with
the correct parameter or the correct suffix version of the xe_mmio_wait
(if we decide to go with separate functions). Its basically the same no?
In any case, sure - will do that too.
>
> BR,
> Jani.
>
>
> >
> > v2: - Fixed checkpatch failure.
> > - Added helper for 64-bit DIV_ROUND_UP on 32-bit CPU (Shasiko
> > review). Fixed bug max range in usleep_range(Shasiko review)
> >
> > Fixes: 5c09bd6ccd41 ("drm/xe/mmio: Move xe_mmio_wait32() to xe_mmio.c")
> > Signed-off-by: Alan Previn <[email protected]>
> > Assisted-by: Github-Copilot:Claude-Sonnet-5-0
> > ---
> > drivers/gpu/drm/xe/xe_mmio.c | 50 ++++++++++++++++++++++++++++++++----
> > 1 file changed, 45 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> > index 7fa18dfcb5a2..1ae382f009de 100644
> > --- a/drivers/gpu/drm/xe/xe_mmio.c
> > +++ b/drivers/gpu/drm/xe/xe_mmio.c
> > @@ -7,6 +7,8 @@
> >
> > #include <linux/delay.h>
> > #include <linux/io-64-nonatomic-lo-hi.h>
> > +#include <linux/math.h>
> > +#include <linux/math64.h>
> > #include <linux/minmax.h>
> > #include <linux/pci.h>
> >
> > @@ -320,6 +322,30 @@ u64 xe_mmio_read64_2x32(struct xe_mmio *mmio, struct
> > xe_reg reg)
> > return (u64)udw << 32 | ldw;
> > }
> >
> > +/**
> > + * __div_round_up64() - alternative to DIV_ROUND_UP for use by
> > __xe_mmio_wait32
> > + * @dividend: 64 bit positive number to divide
> > + * @divisor: 64 bit positive divisor
> > + *
> > + * DIV_ROUND_UP() relies on plain '/' and '%' operators, which for 64-bit
> > + * operands on a 32-bit CPU get turned into calls to libgcc's __divdi3()/
> > + * __moddi3(), routines the kernel does not link against. Provide a
> > + * do_div()-based equivalent that works for signed 64-bit inputs on any
> > + * architecture.
> > + *
> > + * Returns: rounded up division result
> > + */
> > +static inline s64 __div_round_up64(s64 dividend, s64 divisor)
> > +{
> > + u64 abs_dividend = abs(dividend);
> > + u64 abs_divisor = abs(divisor);
> > + u64 result = abs_dividend + abs_divisor - 1;
> > +
> > + do_div(result, abs_divisor);
> > + /* dont check for negative values as local caller only uses positive
> > numbers */
> > + return (s64)result;
> > +}
> > +
> > static int __xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32
> > mask, u32 val,
> > u32 timeout_us, u32 *out_val, bool atomic, bool
> > expect_match)
> > {
> > @@ -349,11 +375,25 @@ static int __xe_mmio_wait32(struct xe_mmio *mmio,
> > struct xe_reg reg, u32 mask, u
> > if (ktime_after(ktime_add_us(cur, wait), end))
> > wait = ktime_us_delta(end, cur);
> >
> > - if (atomic)
> > - udelay(wait);
> > - else
> > - usleep_range(wait, wait << 1);
> > - wait <<= 1;
> > +#define __XE_MMIO_WAIT_MAX_INLOOP_100MS (100 * USEC_PER_MSEC)
> > + if (atomic) {
> > + if (wait <= MAX_UDELAY_MS * USEC_PER_MSEC)
> > + udelay(wait);
> > + else if (BITS_PER_LONG == 32)
> > + mdelay(DIV_ROUND_UP(wait, USEC_PER_MSEC));
> > + else
> > + mdelay(__div_round_up64(wait, USEC_PER_MSEC));
> > + } else {
> > + usleep_range(wait, wait + (wait >> 2)); /* range till
> > wait + 25% */
> > + }
> > + /*
> > + * As we keep doubling the wait time for every check that
> > fails, cap the
> > + * in-loop delay-or-sleep to less than 2x 100 milliseconds to
> > prevent from
> > + * expanding 'wait' into exponentially longer wait times per
> > loop that
> > + * end up delaying the next completion check way later than
> > tolerable.
> > + */
> > + wait = wait < __XE_MMIO_WAIT_MAX_INLOOP_100MS >> 1 ?
> > + wait << 1 : __XE_MMIO_WAIT_MAX_INLOOP_100MS;
> > }
> >
> > if (ret != 0) {
>
