Hello,
This series moves all drivers to a dynamic dma-buf locking specification.
From now on all dma-buf importers are made responsible for holding
dma-buf's reservation lock around all operations performed over dma-bufs
in accordance to the locking specification. This allows us to utilize
reservation lock more broadly around kernel without fearing of a potential
deadlocks.
This patchset passes all i915 selftests. It was also tested using VirtIO,
Panfrost, Lima, Tegra, udmabuf, AMDGPU and Nouveau drivers. I tested cases
of display+GPU, display+V4L and GPU+V4L dma-buf sharing (where appropriate),
which covers majority of kernel drivers since rest of the drivers share
same or similar code paths.
Changelog:
v4: - Added dma_buf_mmap() to the "locking convention" documentation,
which was missed by accident in v3.
- Added acks from Christian König, Tomasz Figa and Hans Verkuil that
they gave to couple v3 patches.
- Dropped the "_unlocked" postfix from function names that don't have
the locked variant, as was requested by Christian König.
- Factored out the per-driver preparations into separate patches
to ease reviewing of the changes, which is now doable without the
global dma-buf functions renaming.
- Factored out the dynamic locking convention enforcements into separate
patches which add the final dma_resv_assert_held(dmabuf->resv) to the
dma-buf API functions.
v3: - Factored out dma_buf_mmap_unlocked() and attachment functions
into aseparate patches, like was suggested by Christian König.
- Corrected and factored out dma-buf locking documentation into
a separate patch, like was suggested by Christian König.
- Intel driver dropped the reservation locking fews days ago from
its BO-release code path, but we need that locking for the imported
GEMs because in the end that code path unmaps the imported GEM.
So I added back the locking needed by the imported GEMs, updating
the "dma-buf attachment locking specification" patch appropriately.
- Tested Nouveau+Intel dma-buf import/export combo.
- Tested udmabuf import to i915/Nouveau/AMDGPU.
- Fixed few places in Etnaviv, Panfrost and Lima drivers that I missed
to switch to locked dma-buf vmapping in the drm/gem: Take reservation
lock for vmap/vunmap operations" patch. In a result invalidated the
Christian's r-b that he gave to v2.
- Added locked dma-buf vmap/vunmap functions that are needed for fixing
vmappping of Etnaviv, Panfrost and Lima drivers mentioned above.
I actually had this change stashed for the drm-shmem shrinker patchset,
but then realized that it's already needed by the dma-buf patches.
Also improved my tests to better cover these code paths.
v2: - Changed locking specification to avoid problems with a cross-driver
ww locking, like was suggested by Christian König. Now the attach/detach
callbacks are invoked without the held lock and exporter should take the
lock.
- Added "locking convention" documentation that explains which dma-buf
functions and callbacks are locked/unlocked for importers and exporters,
which was requested by Christian König.
- Added ack from Tomasz Figa to the V4L patches that he gave to v1.
Dmitry Osipenko (21):
dma-buf: Add unlocked variant of vmapping functions
dma-buf: Add unlocked variant of attachment-mapping functions
drm/gem: Take reservation lock for vmap/vunmap operations
drm/prime: Prepare to dynamic dma-buf locking specification
drm/armada: Prepare to dynamic dma-buf locking specification
drm/i915: Prepare to dynamic dma-buf locking specification
drm/omapdrm: Prepare to dynamic dma-buf locking specification
drm/tegra: Prepare to dynamic dma-buf locking specification
drm/etnaviv: Prepare to dynamic dma-buf locking specification
RDMA/umem: Prepare to dynamic dma-buf locking specification
misc: fastrpc: Prepare to dynamic dma-buf locking specification
xen/gntdev: Prepare to dynamic dma-buf locking specification
media: videobuf2: Prepare to dynamic dma-buf locking specification
media: tegra-vde: Prepare to dynamic dma-buf locking specification
dma-buf: Move dma_buf_vmap() to dynamic locking specification
dma-buf: Move dma_buf_attach() to dynamic locking specification
dma-buf: Move dma_buf_map_attachment() to dynamic locking
specification
dma-buf: Move dma_buf_mmap() to dynamic locking specification
dma-buf: Document dynamic locking convention
media: videobuf2: Stop using internal dma-buf lock
dma-buf: Remove obsoleted internal lock
Documentation/driver-api/dma-buf.rst | 6 +
drivers/dma-buf/dma-buf.c | 211 +++++++++++++++---
drivers/gpu/drm/armada/armada_gem.c | 8 +-
drivers/gpu/drm/drm_client.c | 4 +-
drivers/gpu/drm/drm_gem.c | 24 ++
drivers/gpu/drm/drm_gem_dma_helper.c | 6 +-
drivers/gpu/drm/drm_gem_framebuffer_helper.c | 6 +-
drivers/gpu/drm/drm_gem_ttm_helper.c | 9 +-
drivers/gpu/drm/drm_prime.c | 6 +-
drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_object.c | 12 +
.../drm/i915/gem/selftests/i915_gem_dmabuf.c | 16 +-
drivers/gpu/drm/lima/lima_sched.c | 4 +-
drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 4 +-
drivers/gpu/drm/panfrost/panfrost_dump.c | 4 +-
drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 6 +-
drivers/gpu/drm/qxl/qxl_object.c | 17 +-
drivers/gpu/drm/qxl/qxl_prime.c | 4 +-
drivers/gpu/drm/tegra/gem.c | 17 +-
drivers/infiniband/core/umem_dmabuf.c | 7 +-
.../common/videobuf2/videobuf2-dma-contig.c | 22 +-
.../media/common/videobuf2/videobuf2-dma-sg.c | 19 +-
.../common/videobuf2/videobuf2-vmalloc.c | 17 +-
.../platform/nvidia/tegra-vde/dmabuf-cache.c | 6 +-
drivers/misc/fastrpc.c | 6 +-
drivers/xen/gntdev-dmabuf.c | 8 +-
include/drm/drm_gem.h | 3 +
include/linux/dma-buf.h | 17 +-
29 files changed, 318 insertions(+), 155 deletions(-)
--
2.37.2
Prepare fastrpc to the common dynamic dma-buf locking convention by
starting to use the unlocked versions of dma-buf API functions.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/misc/fastrpc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 93ebd174d848..6fcfb2e9f7a7 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -310,8 +310,8 @@ static void fastrpc_free_map(struct kref *ref)
return;
}
}
- dma_buf_unmap_attachment(map->attach, map->table,
- DMA_BIDIRECTIONAL);
+ dma_buf_unmap_attachment_unlocked(map->attach, map->table,
+ DMA_BIDIRECTIONAL);
dma_buf_detach(map->buf, map->attach);
dma_buf_put(map->buf);
}
@@ -726,7 +726,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
goto attach_err;
}
- map->table = dma_buf_map_attachment(map->attach, DMA_BIDIRECTIONAL);
+ map->table = dma_buf_map_attachment_unlocked(map->attach, DMA_BIDIRECTIONAL);
if (IS_ERR(map->table)) {
err = PTR_ERR(map->table);
goto map_err;
--
2.37.2
Am 31.08.22 um 17:37 schrieb Dmitry Osipenko:
> Prepare fastrpc to the common dynamic dma-buf locking convention by
> starting to use the unlocked versions of dma-buf API functions.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
Acked-by: Christian König <[email protected]>
> ---
> drivers/misc/fastrpc.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 93ebd174d848..6fcfb2e9f7a7 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -310,8 +310,8 @@ static void fastrpc_free_map(struct kref *ref)
> return;
> }
> }
> - dma_buf_unmap_attachment(map->attach, map->table,
> - DMA_BIDIRECTIONAL);
> + dma_buf_unmap_attachment_unlocked(map->attach, map->table,
> + DMA_BIDIRECTIONAL);
> dma_buf_detach(map->buf, map->attach);
> dma_buf_put(map->buf);
> }
> @@ -726,7 +726,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
> goto attach_err;
> }
>
> - map->table = dma_buf_map_attachment(map->attach, DMA_BIDIRECTIONAL);
> + map->table = dma_buf_map_attachment_unlocked(map->attach, DMA_BIDIRECTIONAL);
> if (IS_ERR(map->table)) {
> err = PTR_ERR(map->table);
> goto map_err;
Hi Dmitry,
I've gone over this multiple times now and while it is still possible
that we missed something I think that this should land now.
The whole topic is just to complicated that we can 100% sure guarantee
that there isn't anything wrong with the locking, but lockdep and driver
testing should allow us to quickly find remaining issues.
Do you have commit rights to drm-misc-next or should I push it?
Thanks,
Christian.
Am 31.08.22 um 17:37 schrieb Dmitry Osipenko:
> Hello,
>
> This series moves all drivers to a dynamic dma-buf locking specification.
> From now on all dma-buf importers are made responsible for holding
> dma-buf's reservation lock around all operations performed over dma-bufs
> in accordance to the locking specification. This allows us to utilize
> reservation lock more broadly around kernel without fearing of a potential
> deadlocks.
>
> This patchset passes all i915 selftests. It was also tested using VirtIO,
> Panfrost, Lima, Tegra, udmabuf, AMDGPU and Nouveau drivers. I tested cases
> of display+GPU, display+V4L and GPU+V4L dma-buf sharing (where appropriate),
> which covers majority of kernel drivers since rest of the drivers share
> same or similar code paths.
>
> Changelog:
>
> v4: - Added dma_buf_mmap() to the "locking convention" documentation,
> which was missed by accident in v3.
>
> - Added acks from Christian König, Tomasz Figa and Hans Verkuil that
> they gave to couple v3 patches.
>
> - Dropped the "_unlocked" postfix from function names that don't have
> the locked variant, as was requested by Christian König.
>
> - Factored out the per-driver preparations into separate patches
> to ease reviewing of the changes, which is now doable without the
> global dma-buf functions renaming.
>
> - Factored out the dynamic locking convention enforcements into separate
> patches which add the final dma_resv_assert_held(dmabuf->resv) to the
> dma-buf API functions.
>
> v3: - Factored out dma_buf_mmap_unlocked() and attachment functions
> into aseparate patches, like was suggested by Christian König.
>
> - Corrected and factored out dma-buf locking documentation into
> a separate patch, like was suggested by Christian König.
>
> - Intel driver dropped the reservation locking fews days ago from
> its BO-release code path, but we need that locking for the imported
> GEMs because in the end that code path unmaps the imported GEM.
> So I added back the locking needed by the imported GEMs, updating
> the "dma-buf attachment locking specification" patch appropriately.
>
> - Tested Nouveau+Intel dma-buf import/export combo.
>
> - Tested udmabuf import to i915/Nouveau/AMDGPU.
>
> - Fixed few places in Etnaviv, Panfrost and Lima drivers that I missed
> to switch to locked dma-buf vmapping in the drm/gem: Take reservation
> lock for vmap/vunmap operations" patch. In a result invalidated the
> Christian's r-b that he gave to v2.
>
> - Added locked dma-buf vmap/vunmap functions that are needed for fixing
> vmappping of Etnaviv, Panfrost and Lima drivers mentioned above.
> I actually had this change stashed for the drm-shmem shrinker patchset,
> but then realized that it's already needed by the dma-buf patches.
> Also improved my tests to better cover these code paths.
>
> v2: - Changed locking specification to avoid problems with a cross-driver
> ww locking, like was suggested by Christian König. Now the attach/detach
> callbacks are invoked without the held lock and exporter should take the
> lock.
>
> - Added "locking convention" documentation that explains which dma-buf
> functions and callbacks are locked/unlocked for importers and exporters,
> which was requested by Christian König.
>
> - Added ack from Tomasz Figa to the V4L patches that he gave to v1.
>
> Dmitry Osipenko (21):
> dma-buf: Add unlocked variant of vmapping functions
> dma-buf: Add unlocked variant of attachment-mapping functions
> drm/gem: Take reservation lock for vmap/vunmap operations
> drm/prime: Prepare to dynamic dma-buf locking specification
> drm/armada: Prepare to dynamic dma-buf locking specification
> drm/i915: Prepare to dynamic dma-buf locking specification
> drm/omapdrm: Prepare to dynamic dma-buf locking specification
> drm/tegra: Prepare to dynamic dma-buf locking specification
> drm/etnaviv: Prepare to dynamic dma-buf locking specification
> RDMA/umem: Prepare to dynamic dma-buf locking specification
> misc: fastrpc: Prepare to dynamic dma-buf locking specification
> xen/gntdev: Prepare to dynamic dma-buf locking specification
> media: videobuf2: Prepare to dynamic dma-buf locking specification
> media: tegra-vde: Prepare to dynamic dma-buf locking specification
> dma-buf: Move dma_buf_vmap() to dynamic locking specification
> dma-buf: Move dma_buf_attach() to dynamic locking specification
> dma-buf: Move dma_buf_map_attachment() to dynamic locking
> specification
> dma-buf: Move dma_buf_mmap() to dynamic locking specification
> dma-buf: Document dynamic locking convention
> media: videobuf2: Stop using internal dma-buf lock
> dma-buf: Remove obsoleted internal lock
>
> Documentation/driver-api/dma-buf.rst | 6 +
> drivers/dma-buf/dma-buf.c | 211 +++++++++++++++---
> drivers/gpu/drm/armada/armada_gem.c | 8 +-
> drivers/gpu/drm/drm_client.c | 4 +-
> drivers/gpu/drm/drm_gem.c | 24 ++
> drivers/gpu/drm/drm_gem_dma_helper.c | 6 +-
> drivers/gpu/drm/drm_gem_framebuffer_helper.c | 6 +-
> drivers/gpu/drm/drm_gem_ttm_helper.c | 9 +-
> drivers/gpu/drm/drm_prime.c | 6 +-
> drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 12 +
> .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 16 +-
> drivers/gpu/drm/lima/lima_sched.c | 4 +-
> drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 4 +-
> drivers/gpu/drm/panfrost/panfrost_dump.c | 4 +-
> drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 6 +-
> drivers/gpu/drm/qxl/qxl_object.c | 17 +-
> drivers/gpu/drm/qxl/qxl_prime.c | 4 +-
> drivers/gpu/drm/tegra/gem.c | 17 +-
> drivers/infiniband/core/umem_dmabuf.c | 7 +-
> .../common/videobuf2/videobuf2-dma-contig.c | 22 +-
> .../media/common/videobuf2/videobuf2-dma-sg.c | 19 +-
> .../common/videobuf2/videobuf2-vmalloc.c | 17 +-
> .../platform/nvidia/tegra-vde/dmabuf-cache.c | 6 +-
> drivers/misc/fastrpc.c | 6 +-
> drivers/xen/gntdev-dmabuf.c | 8 +-
> include/drm/drm_gem.h | 3 +
> include/linux/dma-buf.h | 17 +-
> 29 files changed, 318 insertions(+), 155 deletions(-)
>
Hello Christian,
On 9/1/22 10:49, Christian König wrote:
> Hi Dmitry,
>
> I've gone over this multiple times now and while it is still possible
> that we missed something I think that this should land now.
Thank you very much for the review!
> The whole topic is just to complicated that we can 100% sure guarantee
> that there isn't anything wrong with the locking, but lockdep and driver
> testing should allow us to quickly find remaining issues.
At least the most popular drivers should be okay. If anyone will find
issue introduced by this series, then indeed shouldn't be a problem to
fix it later on.
> Do you have commit rights to drm-misc-next or should I push it?
I got the drm-misc commit right two weeks ago, haven't pushed anything
there yet. Please let me try to do the push. I'll let you know if any
kind of help will be needed from you.
I'll wait for more acks/r-bs and then do the push.
--
Best regards,
Dmitry
On 31/08/2022 16:37, Dmitry Osipenko wrote:
> Prepare fastrpc to the common dynamic dma-buf locking convention by
> starting to use the unlocked versions of dma-buf API functions.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
LGTM,
Incase you plan to take it via another tree.
Acked-by: Srinivas Kandagatla <[email protected]>
--srini
> drivers/misc/fastrpc.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 93ebd174d848..6fcfb2e9f7a7 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -310,8 +310,8 @@ static void fastrpc_free_map(struct kref *ref)
> return;
> }
> }
> - dma_buf_unmap_attachment(map->attach, map->table,
> - DMA_BIDIRECTIONAL);
> + dma_buf_unmap_attachment_unlocked(map->attach, map->table,
> + DMA_BIDIRECTIONAL);
> dma_buf_detach(map->buf, map->attach);
> dma_buf_put(map->buf);
> }
> @@ -726,7 +726,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
> goto attach_err;
> }
>
> - map->table = dma_buf_map_attachment(map->attach, DMA_BIDIRECTIONAL);
> + map->table = dma_buf_map_attachment_unlocked(map->attach, DMA_BIDIRECTIONAL);
> if (IS_ERR(map->table)) {
> err = PTR_ERR(map->table);
> goto map_err;
On 31/08/2022 16:37, Dmitry Osipenko wrote:
> Prepare fastrpc to the common dynamic dma-buf locking convention by
> starting to use the unlocked versions of dma-buf API functions.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
LGTM,
Incase you plan to take it via another tree.
Acked-by: Srinivas Kandagatla <[email protected]>
--srini
> drivers/misc/fastrpc.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 93ebd174d848..6fcfb2e9f7a7 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -310,8 +310,8 @@ static void fastrpc_free_map(struct kref *ref)
> return;
> }
> }
> - dma_buf_unmap_attachment(map->attach, map->table,
> - DMA_BIDIRECTIONAL);
> + dma_buf_unmap_attachment_unlocked(map->attach, map->table,
> + DMA_BIDIRECTIONAL);
> dma_buf_detach(map->buf, map->attach);
> dma_buf_put(map->buf);
> }
> @@ -726,7 +726,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
> goto attach_err;
> }
>
> - map->table = dma_buf_map_attachment(map->attach, DMA_BIDIRECTIONAL);
> + map->table = dma_buf_map_attachment_unlocked(map->attach, DMA_BIDIRECTIONAL);
> if (IS_ERR(map->table)) {
> err = PTR_ERR(map->table);
> goto map_err;