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@xxxxxxxx> (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.
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@xxxxxxx>
CC: stable@xxxxxxxxxxxxxxx
Working fine with https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1880
Tested-by: Michel Dänzer <mdaenzer@xxxxxxxxxx>
