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
- if we're waiting already and new fences have been added, wont we miss
them?
Or does this all work because the poll machinery restarts everything
again?
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.koe...@amd.com>
> > > CC: sta...@vger.kernel.org
> > Working fine with
> > https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1880
> >
> > Tested-by: Michel Dänzer <mdaen...@redhat.com>
> >
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
