On Thu, 2025-05-22 at 15:24 +0200, Christian König wrote: > On 5/22/25 15:16, Philipp Stanner wrote: > > On Thu, 2025-05-22 at 15:09 +0200, Christian König wrote: > > > On 5/22/25 14:59, Danilo Krummrich wrote: > > > > On Thu, May 22, 2025 at 02:34:33PM +0200, Christian König > > > > wrote: > > > > > See all the functions inside include/linux/dma-fence.h can be > > > > > used by everybody. It's basically the public interface of the > > > > > dma_fence object. > > > > > > > > As you write below, in certain cases it is valid to call this > > > > from > > > > drivers, so > > > > it's not unreasonable to have it as part of the public API. > > > > > > The question is from which drivers? > > > > > > > > So testing if a fence is signaled without calling the > > > > > callback is > > > > > only allowed by whoever implemented the fence. > > > > > > > > > > In other words nouveau can test nouveau fences, i915 can test > > > > > i915 fences, amdgpu can test amdgpu fences etc... But if you > > > > > have > > > > > the wrapper that makes it officially allowed that nouveau > > > > > starts > > > > > testing i915 fences and that would be problematic. > > > > > > > > In general, I like the __dma_fence_is_signaled() helper, > > > > because > > > > this way we > > > > can document in which cases it is allowed to be used, i.e. the > > > > ones > > > > you descibe > > > > above. > > > > > > > > test_bit() can be called by anyone and there is no > > > > documentation > > > > comment > > > > explaining that it is only allowed under certain conditions. > > > > > > That's a rather good argument. > > > > > > > Having the __dma_fence_is_signaled() helper properly documented > > > > could get you > > > > rid of having to explain in which case the test_bit() dance is > > > > allowed to do > > > > over and over again. :-) > > > > > > That's an even better argument. > > > > > > > I also think the name is good, since the '__' prefix already > > > > implies that there > > > > are some restrictions on the use of this helper. > > > > > > I'm still hesitating. Adding something to the API always made it > > > usable by everybody. > > > > > > Now suddenly saying we add that to the include/linux/dma-fence.h > > > header but only certainly code can use it still sounds > > > questionable > > > to me. > > > > If I understand the current code correctly, the documentation state > > and > > the question "which driver is allowed to do it?" is the same, > > because > > the documentation for the signaled callback doesn't specify that: > > > > > > /** > > * @signaled: > > * > > * Peek whether the fence is signaled, as a fastpath > > optimization for > > * e.g. dma_fence_wait() or dma_fence_add_callback(). Note > > that this > > * callback does not need to make any guarantees beyond > > that a fence > > * once indicates as signalled must always return true > > from this > > * callback. This callback may return false even if the > > fence has > > * completed already, in this case information hasn't > > propogated throug > > * the system yet. See also dma_fence_is_signaled(). > > * > > * May set &dma_fence.error if returning true. > > * > > * This callback is optional. > > */ > > bool (*signaled)(struct dma_fence *fence); > > > > > > "optional". What if I don't ipmlement it? Who should implement it? > > > > If the callback is optional, then dma_fence_is_signaled() is the > > same > > as __dma_fence_is_signaled(). > > > > IOW, it already needs to be better documented who needs to > > implement > > the callback and who doesn't. If we get clarity on that, we also > > get > > clarity on who may use __dma_fence_is_signaled(). > > Well there is no need to implement it, but when it is implemented the > caller *must* call it when polling.
I don't understand. Please elaborate on that a bit more. If there's no need to implement it, then why can't one have a __dma_fence_is_signaled(), which is then identical? > > IIRC the background that we didn't allowed this was that we already > had the case that users only looked at the signaling bit and then > where surprised that it never changed. Why would anyone expect that a fence gets signaled by calling a function with the name "dma_fence_is_signaled()"? :-) That was my original point, the name is not intuitive at all. For example, if a driver doesn't implement that callback but signals fences in interrupt handlers, and then forgets to (re-)activate the interrupt, fences will never get signaled and callers to dma_fence_is_signaled() will never read 'true', which isn't surprising. Again, the point remains the same: the driver must guarantee that fences will get signaled. Independently from how consumers of the fence check it. Consumers could just stop calling dma_fence_is_signaled() after the point in time T alltogether and then the driver would still have to signal everything. P. > > Regards, > Christian. > > > > > > > P. > > > > > > > > Regards, > > > Christian. > > >
