On Wed, Sep 16, 2026 at 07:10:11PM +0000, Teres Alexis, Alan Previn wrote: > On Tue, 2026-09-15 at 18:51 -0400, Vivi, Rodrigo wrote: > > On Tue, Sep 15, 2026 at 04:00:54PM +0000, Teres Alexis, Alan Previn wrote: > > > On Tue, 2026-09-15 at 10:34 +0300, Nikula, Jani wrote: > > > > On Mon, 14 Sep 2026, Alan Previn <[email protected]> > > > > wrote: > > > alan:snip > > > > > int xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32 > > > > > mask, u32 val, u32 timeout_us, > > > > > - u32 *out_val, bool atomic) > > > > > + u32 *out_val) > > > > > { > > > > > - return __xe_mmio_wait32(mmio, reg, mask, val, timeout_us, > > > > > out_val, atomic, true); > > > > > + u32 read; > > > > > + int ret; > > > > > + > > > > > + ret = poll_timeout_us(read = xe_mmio_read32(mmio, reg), (read & > > > > > mask) == val, > > > > > + 10, timeout_us, false); > > > > > > > > You probably do need to let the callers pass in the wait too. 10 us wait > > > > with a long timeout is going to be pretty bad. > > > > agreed > > > > > > > > > > > > > alan: okay - perhaps i can make every caller pass in a polling-wait thats > > > a fraction of their wait time. > > > (as a starting point since i dont know what's the expected behavior of > > > every caller). > > > so perhaps something like "timeout_us << 4" (i.e. 1/16th) but pass in 10 > > > us if its anything smaller than that > > > (i.e. smaller than 16 usec). > > > > I think we might be complicating this too much.. > > > > what about something simpler like: > > > > #define XE_MMIO_WAIT_MAX_BACKOFF_US 1000 > > > > ... > > - wait <<= 1; > > + wait = min_t(s64, wait << 1, XE_MMIO_WAIT_MAX_BACKOFF_US); > > > > > > alan: i dont understand your this comment on the increasing the "wait by x2" > in the loop after agreeing with Jani on the earlier statement.
What I agreed with Jani was that a hardcoded 10us poll interval against a 2s timeout is bad. That was all. > Some historical context: > > 1. current baseline code it stands to day IS in violation of linux rules for > how to use those sleep/delay functions right, and I believe that this single line change is enough to fix this violation. The problem is that we double wait every loop 10, 20, 40, 80 … 20480, 40960 and never stops doubling. udelay() is only legal up to ~5000us (perhaps 1000?!), so once the doubling passes that, we're breaking the rule. Same on the sleeping side: it ends up asking usleep_range() to sleep 1.3 s, which is not what that function is for. The one line just stops the doubling at 1000us: wait = min_t(s64, wait << 1, 1000); After that, the wait can be 10, 20, 40 … up to 1000, and then it stays at 1000 forever. It can never reach 20480 or 1300000. So the illegal value never gets passed to udelay() or usleep_range() — ever. > 2. my initial revs on fixing this was to minimize the changes so its not > complicated by simply fixing the code in place. I believe this simple line alings with your v1, but just simpler. > 3. Jani said we really should use the proper linux kernel helpers: > poll_timeout_us / poll_timeout_us_atomic. > 4. Those helpers have the "timeout_us" and the "intra-loop-wait-us" period. > I hardcoded to 10 usec. > - Jani said i should not hardcode and ensure all up-the-stack callers > of the xe_mmio_wait32 function passes in the intra-wait-loop > value. > - Then you (Rodrigo) agreed with his request but go on to propose going > back to exponential 2x intra-wait-loop. > - but that contradicts Jani's request u agreed to and also that > means we implement the intra-wait-loop? (i.e. dont use the > proper linux helper?) sorry for not being clear on my previous response. Perhaps we have a cleaner end code with the poll_timeout_us and poll_timeout_us_atomic indeed. Then perhaps we have this single fixes patch and do other attempts in get cleaner loops? Thanks, Rodrigo. > > ...alan > > > > Also the Fixes tag is not the right one... the bug was there before... > > > > > > > > ...alan >
