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

[Christian König]

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.

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.

Regards,
Christian.

> 
> 
> P.
> 
>>
>> Regards,
>> Christian.
>