Am 23.09.2016 um 13:30 schrieb Gustavo Padovan: > 2016-09-22 Christian König <christian.koenig at amd.com>: > >> Am 22.09.2016 um 13:16 schrieb Gustavo Padovan: >>> 2016-09-22 Christian König <christian.koenig at amd.com>: >>> >>>> Dropping the rest of the patch, cause that really doesn't make sense any >>>> more. >>>> >>>> Am 22.09.2016 um 12:40 schrieb Gustavo Padovan: >>>>>> E.g. for example it is illegal to do something like >>>>>>> "while(!fence_is_signaled(f)) sleep();" without enabling signaling >>>>>>> before >>>>>>> doing this. >>>>>>> >>>>>>> Could just be a misunderstanding, but the comments on your patch >>>>>>> actually >>>>>>> sounds a bit like somebody is trying to do exactly that. >>>>> I think the usecase in mind here is poll(fd, timeout=0) >>>> Exactly as I feared. Even if userspace polls with timeout=0 you still need >>>> to call enable_signaling(). >>>> >>>> Otherwise you can run into a situation where userspace only uses timeout=0 >>>> and so never activates the signaling check in the driver. >>>> >>>> This would in turn result in an endless loop on implementations where the >>>> driver never signals fences on their own. >>> Polling is optional, userspace may never call it. And DRM/KMS or GPU >>> drivers will be doing fence_wait() themselves so signaling is enabled at >>> some point. >> No they won't. We have an use case where we clearly want to avoid that as >> much as possible because and so the driver never calls enable_signaling() on >> it's own. >> >> Exposing this poll function to userspace without enabling signaling is a >> clear NAK from my side. > Okay. So you are NAK'ing the does_not_pool_wait change. Should we revert > that one then? It is already broken.
Yeah, that would probably a good idea. The AMD driver changes which really rely on this aren't upstream yet, so if you point me to the commit hash I could revert that as well when we send that out. On the other hand the idea behind fence_is_signaled() is really that you check the status multiple times after enabling signaling. So I would prefer if you would revert this change preliminary. Double checking this patch (and thinking about it a bit more) reveals that it is most likely correct. So feel free to commit this one if it is still needed for something. Regards, Christian. > > Gustavo >
