[PATCH 2/2] drm/nouveau: Don't signal when killing the fence context

[Philipp Stanner]

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.
> > 
>