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@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. Hm yeah. I guess Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> 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@xxxxxxx> > > > > > > > CC: stable@xxxxxxxxxxxxxxx > > > > > > Working fine with https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1880 > > > > > > > > > > > > Tested-by: Michel Dänzer <mdaenzer@xxxxxxxxxx> > > > > > > > > > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
