On Thu, Sep 30, 2021 at 01:32:28PM +0200, Christian König wrote:
Am 30.09.21 um 12:26 schrieb Daniel Vetter:
On Thu, Sep 30, 2021 at 11:48:42AM +0200, Christian König wrote:
Am 30.09.21 um 11:00 schrieb Daniel Vetter:
On Wed, Sep 22, 2021 at 01:08:44PM +0200, Christian König wrote:
Totally forgotten to ping once more about that.
Michel has tested this now and I think we should push it ASAP. So can I get an rb?
spin_lock_irq(&dmabuf->poll.lock); if (dcb->active) events &= ~EPOLLIN; else dcb->active = EPOLLIN; spin_unlock_irq(&dmabuf->poll.lock);This pattern (and same for EPOLLOUT) makes no sense to me. I guess the intent is that this filters out events for which we're already listening, but:
- it checks for any active event, not the specific one
Which is correct. We now use one dcb for EPOLLIN and another one for EPOLLOUT.
We could make that a boolean instead if that makes it cleaner.
Ha yes I missed that. Boolean sounds much better.
- if we're waiting already and new fences have been added, wont we miss them?
No, when we are already waiting the callback will sooner or later fire and result in a re-check.
Or does this all work because the poll machinery restarts everything again?
Yes, exactly that. Otherwise waiting for multiple fences wouldn't work either.
Ok with that clarified can you cut a v8 with booleans and I whack an r-b on that? Or just include it, I'm massively burried here and trying to dig out :-/
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch (with the booleans)
My bad, boolean won't work because we also use the flags for the call to "wake_up_locked_poll(dcb->poll, dcb->active);".
Anyway that doesn't really change anything on the logic and I inherited that handling from the existing code, just moved it around a bit.
Hm yeah. I guess
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
But this poll stuff just massively confuses me. -Daniel
Christian.
-Daniel
Regards, Christian.
I'm totally confused here for sure. The other changes in the patch look good though. -Daniel
Thanks, Christian.
Am 23.07.21 um 10:04 schrieb Michel Dänzer:
On 2021-07-20 3:11 p.m., Christian König wrote: > Daniel pointed me towards this function and there are multiple obvious problems > in the implementation. > > First of all the retry loop is not working as intended. In general the retry > makes only sense if you grab the reference first and then check the sequence > values. > > Then we should always also wait for the exclusive fence. > > It's also good practice to keep the reference around when installing callbacks > to fences you don't own. > > And last the whole implementation was unnecessary complex and rather hard to > understand which could lead to probably unexpected behavior of the IOCTL. > > Fix all this by reworking the implementation from scratch. Dropping the > whole RCU approach and taking the lock instead. > > Only mildly tested and needs a thoughtful review of the code. > > v2: fix the reference counting as well > v3: keep the excl fence handling as is for stable > v4: back to testing all fences, drop RCU > v5: handle in and out separately > v6: add missing clear of events > v7: change coding style as suggested by Michel, drop unused variables > > Signed-off-by: Christian König christian.koenig@amd.com > CC: stable@vger.kernel.org Working fine with https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1880
Tested-by: Michel Dänzer mdaenzer@redhat.com