Re: [PATCH] drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

> > 
Actually, hold off further review. let me post a new series with 2 patches
- first patch = re-rev of this one - but aligned to Rodrigo's direction to keep
it simple (for "fixes" management) with simple intra-loop caps. 
- second patch to align with Jani's (as well as other offline inputs) that
we SHOULD actually be using the kernel polling helpers.

but i think i wont update all callers to separate atomic vs non-atomic (as per Jani's)
request. I agree with him, but needs to be a separate series because since i see
us using those param-based-atomic-separation elsewhere in the driver in other subsystems.


thanks again everyone for all the inputs. will work on the re-rev.


alan:snip
> > > 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: I think i understand what u mean. But i believe a single line patch will NOT suffice
> to maintain existing behavior for the non-atomic case. In the old code, if caller was
> requesting for non-atomic, 'wait' would continue doubling in usleep_range (which has no
> overflow limit for its params) so we actually CAN keep doubling. Thus callers that have
> very large timeouts, we'll be saving on CPU cycles as it increases. But with a 1 ms change
> as you proposed, we'd be increasing CPU cycles to get us to the end. Ofc I agree that
> doubling forever is not ideal even for the non-atomic usleep_range case, (kernel
> documentation stating that its good for both short and long timeouts), however kernel
> documentation DOES state it's an expensive operation with the hrttimer, which is why
> i wanted to allow the doubling of 'wait' for usleep_range case (like original code) but
> cap it to 25% of the caller provided final timeout. (while capping the atomic version to
> MAX_UDELAY_MS for short delays or the same 25% for long delays). This will ensure the
> overall behavior in terms of cpu cyles and expensive hrttimer calls are similiar as before
> AND also fixing in accordance to the rules. Actually if u see rev3, this is what i mean
> Its not 1 line but its much simpler fix and does I believe it does align with your vision.
> 
> > > 
> > >  ...alan
> > > 
> > > 
> > > > Also the Fixes tag is not the right one... the bug was there before...
> > > > 
> > > > > 
> > > > > ...alan
> > > 
>