Message-ID: <8eaab4c04fad84bde279ee2cd228fac4f84c5184.camel@mailbox.org>
Date: Mon, 01 Dec 2025 16:34:32 +0100
From: Philipp Stanner <phasta@...lbox.org>
To: Christian König <christian.koenig@....com>, 
 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 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?

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


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