Re: [Linaro-mm-sig] [PATCH v3 6/9] dma-buf: Move dma-buf attachment to dynamic locking specification

[Christian König]

Am 24.08.22 um 17:49 schrieb Dmitry Osipenko:
> On 8/24/22 18:24, Christian König wrote:
> > Am 24.08.22 um 12:22 schrieb Dmitry Osipenko:
> > > Move dma-buf attachment API functions to the dynamic locking
> > > specification.
> > > The strict locking convention prevents deadlock situations for dma-buf
> > > importers and exporters.
> > >
> > > Previously, the "unlocked" versions of the attachment API functions
> > > weren't taking the reservation lock and this patch makes them to take
> > > the lock.
> > >
> > > Intel and AMD GPU drivers already were mapping the attached dma-bufs
> > > under
> > > the held lock during attachment, hence these drivers are updated to use
> > > the locked functions.
> > >
> > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
> > > ---
> > >    drivers/dma-buf/dma-buf.c                  | 115 ++++++++++++++-------
> > >    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    |   4 +-
> > >    drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |   8 +-
> > >    drivers/gpu/drm/i915/gem/i915_gem_object.c |  12 +++
> > >    include/linux/dma-buf.h                    |  20 ++--
> > >    5 files changed, 110 insertions(+), 49 deletions(-)
> > >
> > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > > index 4556a12bd741..f2a5a122da4a 100644
> > > --- a/drivers/dma-buf/dma-buf.c
> > > +++ b/drivers/dma-buf/dma-buf.c
> > > @@ -559,7 +559,7 @@ static struct file *dma_buf_getfile(struct dma_buf
> > > *dmabuf, int flags)
> > >     * 2. Userspace passes this file-descriptors to all drivers it wants
> > > this buffer
> > >     *    to share with: First the file descriptor is converted to a
> > > &dma_buf using
> > >     *    dma_buf_get(). Then the buffer is attached to the device using
> > > - *    dma_buf_attach().
> > > + *    dma_buf_attach_unlocked().
> > Now I get why this is confusing me so much.
> >
> > The _unlocked postfix implies that there is another function which
> > should be called with the locks already held, but this is not the case
> > for attach/detach (because they always need to grab the lock themselves).
> That's correct. The attach/detach ops of exporter can take the lock
> (like i915 exporter does it), hence importer must not grab the lock
> around dma_buf_attach() invocation.
>
> > So I suggest to drop the _unlocked postfix for the attach/detach
> > functions. Another step would then be to unify attach/detach with
> > dynamic_attach/dynamic_detach when both have the same locking convention
> > anyway.
> It's not a problem to change the name, but it's unclear to me why we
> should do it. The _unlocked postfix tells importer that reservation must
> be unlocked and it must be unlocked in case of dma_buf_attach().
>
> Dropping the postfix will make dma_buf_attach() inconsistent with the
> rest of the _unlocked functions(?). Are you sure we need to rename it?

The idea of the postfix was to distinguish between two different 
versions of the same function, e.g. dma_buf_vmap_unlocked() vs normal 
dma_buf_vmap().

When we don't have those two types of the same function I don't think it 
makes to much sense to keep that. We should just properly document which 
functions expect what and that's what your documentation patch does.

Regards,
Christian.

> > Sorry that this is going so much back and forth, it's really complicated
> > to keep all the stuff in my head at the moment :)
> Not a problem at all, I expected that it will take some time for this
> patchset to settle down.