> > 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 > > > >
