Message-ID: <4121e5a3-2fde-4867-96a4-bf06e2c7150d@amd.com>
Date: Mon, 1 Dec 2025 17:06:52 +0100
From: Christian König <christian.koenig@....com>
To: phasta@...nel.org, Sumit Semwal <sumit.semwal@...aro.org>,
 Gustavo Padovan <gustavo@...ovan.org>,
 Felix Kuehling <Felix.Kuehling@....com>,
 Alex Deucher <alexander.deucher@....com>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>, Jani Nikula <jani.nikula@...ux.intel.com>,
 Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
 Rodrigo Vivi <rodrigo.vivi@...el.com>, Tvrtko Ursulin
 <tursulin@...ulin.net>, Huang Rui <ray.huang@....com>,
 Matthew Auld <matthew.auld@...el.com>,
 Matthew Brost <matthew.brost@...el.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 Lucas De Marchi <lucas.demarchi@...el.com>,
 Thomas Hellström <thomas.hellstrom@...ux.intel.com>
Cc: linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, amd-gfx@...ts.freedesktop.org,
 intel-gfx@...ts.freedesktop.org, intel-xe@...ts.freedesktop.org
Subject: Re: [PATCH v2 2/8] dma-buf/dma-fence: Add
 dma_fence_check_and_signal()

On 12/1/25 16:34, Philipp Stanner wrote:
> On Mon, 2025-12-01 at 16:20 +0100, Christian König wrote:
>> On 12/1/25 14:55, Philipp Stanner wrote:
>>> On Mon, 2025-12-01 at 14:23 +0100, Christian König wrote:
>>>> On 12/1/25 11:50, Philipp Stanner wrote:
>>>>> The overwhelming majority of users of dma_fence signaling functions
>>>>> don't care about whether the fence had already been signaled by someone
>>>>> else. Therefore, the return code shall be removed from those functions.
>>>>>
>>>>> For the few users who rely on the check, a new, specialized function
>>>>> shall be provided.
>>>>>
>>>>> Add dma_fence_check_and_signal(), which signals a fence if it had not
>>>>> yet been signaled, and informs the user about that.
>>>>>
>>>>> Add a counter part, dma_fence_check_and_signal_locked(), which doesn't
>>>>> take the spinlock.
>>>>>
>>>>> Signed-off-by: Philipp Stanner <phasta@...nel.org>
>>>>> ---
>>>>>  drivers/dma-buf/dma-fence.c | 44 +++++++++++++++++++++++++++++++++++++
>>>>>  include/linux/dma-fence.h   |  2 ++
>>>>>  2 files changed, 46 insertions(+)
>>>>>
>>>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>>>>> index 96d72ffc0750..146de62887cf 100644
>>>>> --- a/drivers/dma-buf/dma-fence.c
>>>>> +++ b/drivers/dma-buf/dma-fence.c
>>>>> @@ -445,6 +445,50 @@ int dma_fence_signal_locked(struct dma_fence *fence)
>>>>>  }
>>>>>  EXPORT_SYMBOL(dma_fence_signal_locked);
>>>>>  
>>>>> +/**
>>>>> + * dma_fence_check_and_signal_locked - signal the fence if it's not yet signaled
>>>>> + * @fence: the fence to check and signal
>>>>> + *
>>>>> + * Checks whether a fence was signaled and signals it if it was not yet signaled.
>>>>> + *
>>>>> + * Unlike dma_fence_check_and_signal(), this function must be called with
>>>>> + * &struct dma_fence.lock being held.
>>>>> + *
>>>>> + * Return: true if fence has been signaled already, false otherwise.
>>>>> + */
>>>>> +bool dma_fence_check_and_signal_locked(struct dma_fence *fence)
>>>>
>>>> I'm seriously considering to nuke all the unlocked variants of dma_fence functions and just make it mandatory for callers to grab the lock manually.
>>>>
>>>
>>> You mean "nuke the *locked* variants.
>>
>> Sorry, that wasn't specific enough.
>>
>> What I meant was making the locked variants the default instead of the unlocked ones.
> 
> Well, no :D
> 
> What you want to do is:
> - Delete / deprecate the *locked* variants
> - Force all users to take the fence lock manually, then use the (now
> all unlocked) dma fence functions.
> 
> ACK?

I'm sick with cold/flu like symptoms at the moment, but that sounds mixed up to me (but maybe I should get a bit sleep first).

>>
>>>
>>> Why, though? Aren't they enough for most users?
>>> I suppose you have all those subtle races in mind..
>>
>> Yeah, exactly that.
>>
>>>
>>>>> +{
>>>>> +	bool ret;
>>>>> +
>>>>> +	ret = dma_fence_test_signaled_flag(fence);
>>>>> +	dma_fence_signal_locked(fence);
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +EXPORT_SYMBOL(dma_fence_check_and_signal_locked);
>>>>> +
>>>>> +/**
>>>>> + * dma_fence_check_and_signal - signal the fence if it's not yet signaled
>>>>> + * @fence: the fence to check and signal
>>>>> + *
>>>>> + * Checks whether a fence was signaled and signals it if it was not yet signaled.
>>>>> + * All this is done in a race-free manner.
>>>>> + *
>>>>> + * Return: true if fence has been signaled already, false otherwise.
>>>>> + */
>>>>> +bool dma_fence_check_and_signal(struct dma_fence *fence)
>>>>
>>>> So I think we should name this one here dma_fence_check_and_signal_unlocked() and drop the postfix from the locked variant.
>>>
>>> postfix?
>>>
>>> Well, now, IDK. Can't we, for this series, keep the _locked() variant
>>> so that it's congruent with all the other dma_fence code?
>>
>> Good point. That thought was not really related to this series here.
> 
> OK, then let's progress with this here for now.

Works for me, give me a day to go over it again and review it.

Regards,
Christian.

> 
> 
> P.
> 
>>
>>>
>>> And then later if you want to force manual locking you can add that
>>> kernel-wide in a separate series, since it'll be a discussion-worthy,
>>> bigger chunk of work.
>>>
>>> That's cleaner, and my series here won't prevent that once merged.
>>>
>>>>
>>>>> +{
>>>>> +	unsigned long flags;
>>>>> +	bool ret;
>>>>> +
>>>>> +	spin_lock_irqsave(fence->lock, flags);
>>>>> +	ret = dma_fence_check_and_signal_locked(fence);
>>>>> +	spin_unlock_irqrestore(fence->lock, flags);
>>>>
>>>> Could this use guard(fence->lock, flags) ?
>>>
>>> guard? You mean a lockdep guard? Do you have a pointer to someplace in
>>> dma_fence who does what you mean / want?
>>
>> E.g. like guard(spinlock_irqsave)(&fence->lock);
>>
>> Regards,
>> Christian.
>>
>>>
>>>
>>> P.
>>>
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +EXPORT_SYMBOL(dma_fence_check_and_signal);
>>>>> +
>>>>>  /**
>>>>>   * dma_fence_signal - signal completion of a fence
>>>>>   * @fence: the fence to signal
>>>>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>>>>> index 19972f5d176f..0504afe52c2a 100644
>>>>> --- a/include/linux/dma-fence.h
>>>>> +++ b/include/linux/dma-fence.h
>>>>> @@ -365,6 +365,8 @@ static inline void __dma_fence_might_wait(void) {}
>>>>>  #endif
>>>>>  
>>>>>  int dma_fence_signal(struct dma_fence *fence);
>>>>> +bool dma_fence_check_and_signal(struct dma_fence *fence);
>>>>> +bool dma_fence_check_and_signal_locked(struct dma_fence *fence);
>>>>>  int dma_fence_signal_locked(struct dma_fence *fence);
>>>>>  int dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp);
>>>>>  int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>>>
>>>
>>
> 

Powered by blists - more mailing lists