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:
v7: - Rebased on top of recent drm-misc-next.
- Added ack from Jason Gunthorpe to the RDMA patch.
- Added iosys_map_clear() to dma_buf_vmap_unlocked(), making it fully
consistent with dma_buf_vmap().
v6: - Added r-b from Michael Ruhl to the i915 patch.
- Added acks from Sumit Semwal and updated commit message of the
"Move dma_buf_vmap() to dynamic locking specification" patch like
was suggested by Sumit.
- Added "!dmabuf" check to dma_buf_vmap_unlocked() to match the locked
variant of the function, for consistency.
v5: - Added acks and r-bs that were given to v4.
- Changed i915 preparation patch like was suggested by Michael Ruhl.
The scope of reservation locking is smaller now.
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 | 216 +++++++++++++++---
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 | 14 ++
.../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, 325 insertions(+), 155 deletions(-)
--
2.37.3
Add unlocked variant of dma_buf_map/unmap_attachment() that will
be used by drivers that don't take the reservation lock explicitly.
Acked-by: Sumit Semwal <[email protected]>
Acked-by: Christian König <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/dma-buf/dma-buf.c | 53 +++++++++++++++++++++++++++++++++++++++
include/linux/dma-buf.h | 6 +++++
2 files changed, 59 insertions(+)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index e95fc8dc3aed..3e4060dadb74 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -1100,6 +1100,34 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
}
EXPORT_SYMBOL_NS_GPL(dma_buf_map_attachment, DMA_BUF);
+/**
+ * dma_buf_map_attachment_unlocked - Returns the scatterlist table of the attachment;
+ * mapped into _device_ address space. Is a wrapper for map_dma_buf() of the
+ * dma_buf_ops.
+ * @attach: [in] attachment whose scatterlist is to be returned
+ * @direction: [in] direction of DMA transfer
+ *
+ * Unlocked variant of dma_buf_map_attachment().
+ */
+struct sg_table *
+dma_buf_map_attachment_unlocked(struct dma_buf_attachment *attach,
+ enum dma_data_direction direction)
+{
+ struct sg_table *sg_table;
+
+ might_sleep();
+
+ if (WARN_ON(!attach || !attach->dmabuf))
+ return ERR_PTR(-EINVAL);
+
+ dma_resv_lock(attach->dmabuf->resv, NULL);
+ sg_table = dma_buf_map_attachment(attach, direction);
+ dma_resv_unlock(attach->dmabuf->resv);
+
+ return sg_table;
+}
+EXPORT_SYMBOL_NS_GPL(dma_buf_map_attachment_unlocked, DMA_BUF);
+
/**
* dma_buf_unmap_attachment - unmaps and decreases usecount of the buffer;might
* deallocate the scatterlist associated. Is a wrapper for unmap_dma_buf() of
@@ -1136,6 +1164,31 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
}
EXPORT_SYMBOL_NS_GPL(dma_buf_unmap_attachment, DMA_BUF);
+/**
+ * dma_buf_unmap_attachment_unlocked - unmaps and decreases usecount of the buffer;might
+ * deallocate the scatterlist associated. Is a wrapper for unmap_dma_buf() of
+ * dma_buf_ops.
+ * @attach: [in] attachment to unmap buffer from
+ * @sg_table: [in] scatterlist info of the buffer to unmap
+ * @direction: [in] direction of DMA transfer
+ *
+ * Unlocked variant of dma_buf_unmap_attachment().
+ */
+void dma_buf_unmap_attachment_unlocked(struct dma_buf_attachment *attach,
+ struct sg_table *sg_table,
+ enum dma_data_direction direction)
+{
+ might_sleep();
+
+ if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
+ return;
+
+ dma_resv_lock(attach->dmabuf->resv, NULL);
+ dma_buf_unmap_attachment(attach, sg_table, direction);
+ dma_resv_unlock(attach->dmabuf->resv);
+}
+EXPORT_SYMBOL_NS_GPL(dma_buf_unmap_attachment_unlocked, DMA_BUF);
+
/**
* dma_buf_move_notify - notify attachments that DMA-buf is moving
*
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 8daa054dd7fe..f11b5bbc2f37 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -627,6 +627,12 @@ int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
enum dma_data_direction dir);
int dma_buf_end_cpu_access(struct dma_buf *dma_buf,
enum dma_data_direction dir);
+struct sg_table *
+dma_buf_map_attachment_unlocked(struct dma_buf_attachment *attach,
+ enum dma_data_direction direction);
+void dma_buf_unmap_attachment_unlocked(struct dma_buf_attachment *attach,
+ struct sg_table *sg_table,
+ enum dma_data_direction direction);
int dma_buf_mmap(struct dma_buf *, struct vm_area_struct *,
unsigned long);
--
2.37.3
Move dma_buf_vmap/vunmap() functions to the dynamic locking
specification by asserting that the reservation lock is held.
Acked-by: Sumit Semwal <[email protected]>
Acked-by: Christian König <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/dma-buf/dma-buf.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 3e4060dadb74..2c4381bb9478 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -1450,6 +1450,8 @@ int dma_buf_vmap(struct dma_buf *dmabuf, struct iosys_map *map)
if (WARN_ON(!dmabuf))
return -EINVAL;
+ dma_resv_assert_held(dmabuf->resv);
+
if (!dmabuf->ops->vmap)
return -EINVAL;
@@ -1515,6 +1517,8 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, struct iosys_map *map)
if (WARN_ON(!dmabuf))
return;
+ dma_resv_assert_held(dmabuf->resv);
+
BUG_ON(iosys_map_is_null(&dmabuf->vmap_ptr));
BUG_ON(dmabuf->vmapping_counter == 0);
BUG_ON(!iosys_map_is_equal(&dmabuf->vmap_ptr, map));
--
2.37.3
Move dma_buf_mmap() function to the dynamic locking specification by
taking the reservation lock. Neither of the today's drivers take the
reservation lock within the mmap() callback, hence it's safe to enforce
the locking.
Acked-by: Sumit Semwal <[email protected]>
Acked-by: Christian König <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/dma-buf/dma-buf.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index f54c649f922a..f149b384f4dd 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -1390,6 +1390,8 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_end_cpu_access, DMA_BUF);
int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
unsigned long pgoff)
{
+ int ret;
+
if (WARN_ON(!dmabuf || !vma))
return -EINVAL;
@@ -1410,7 +1412,11 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
vma_set_file(vma, dmabuf->file);
vma->vm_pgoff = pgoff;
- return dmabuf->ops->mmap(dmabuf, vma);
+ dma_resv_lock(dmabuf->resv, NULL);
+ ret = dmabuf->ops->mmap(dmabuf, vma);
+ dma_resv_unlock(dmabuf->resv);
+
+ return ret;
}
EXPORT_SYMBOL_NS_GPL(dma_buf_mmap, DMA_BUF);
--
2.37.3
Prepare Armada driver to the common dynamic dma-buf locking convention
by starting to use the unlocked versions of dma-buf API functions.
Acked-by: Christian König <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/gpu/drm/armada/armada_gem.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
index 5430265ad458..26d10065d534 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -66,8 +66,8 @@ void armada_gem_free_object(struct drm_gem_object *obj)
if (dobj->obj.import_attach) {
/* We only ever display imported data */
if (dobj->sgt)
- dma_buf_unmap_attachment(dobj->obj.import_attach,
- dobj->sgt, DMA_TO_DEVICE);
+ dma_buf_unmap_attachment_unlocked(dobj->obj.import_attach,
+ dobj->sgt, DMA_TO_DEVICE);
drm_prime_gem_destroy(&dobj->obj, NULL);
}
@@ -539,8 +539,8 @@ int armada_gem_map_import(struct armada_gem_object *dobj)
{
int ret;
- dobj->sgt = dma_buf_map_attachment(dobj->obj.import_attach,
- DMA_TO_DEVICE);
+ dobj->sgt = dma_buf_map_attachment_unlocked(dobj->obj.import_attach,
+ DMA_TO_DEVICE);
if (IS_ERR(dobj->sgt)) {
ret = PTR_ERR(dobj->sgt);
dobj->sgt = NULL;
--
2.37.3
On 10/17/22 20:22, Dmitry Osipenko wrote:
> 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:
>
> v7: - Rebased on top of recent drm-misc-next.
>
> - Added ack from Jason Gunthorpe to the RDMA patch.
>
> - Added iosys_map_clear() to dma_buf_vmap_unlocked(), making it fully
> consistent with dma_buf_vmap().
>
> v6: - Added r-b from Michael Ruhl to the i915 patch.
>
> - Added acks from Sumit Semwal and updated commit message of the
> "Move dma_buf_vmap() to dynamic locking specification" patch like
> was suggested by Sumit.
>
> - Added "!dmabuf" check to dma_buf_vmap_unlocked() to match the locked
> variant of the function, for consistency.
>
> v5: - Added acks and r-bs that were given to v4.
>
> - Changed i915 preparation patch like was suggested by Michael Ruhl.
> The scope of reservation locking is smaller now.
>
> 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 | 216 +++++++++++++++---
> 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 | 14 ++
> .../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, 325 insertions(+), 155 deletions(-)
>
Applied to drm-misc-next
--
Best regards,
Dmitry
Am 18.10.22 um 01:07 schrieb Dmitry Osipenko:
> On 10/17/22 20:22, Dmitry Osipenko wrote:
>> 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:
>>
>> v7: - Rebased on top of recent drm-misc-next.
>>
>> - Added ack from Jason Gunthorpe to the RDMA patch.
>>
>> - Added iosys_map_clear() to dma_buf_vmap_unlocked(), making it fully
>> consistent with dma_buf_vmap().
>>
>> v6: - Added r-b from Michael Ruhl to the i915 patch.
>>
>> - Added acks from Sumit Semwal and updated commit message of the
>> "Move dma_buf_vmap() to dynamic locking specification" patch like
>> was suggested by Sumit.
>>
>> - Added "!dmabuf" check to dma_buf_vmap_unlocked() to match the locked
>> variant of the function, for consistency.
>>
>> v5: - Added acks and r-bs that were given to v4.
>>
>> - Changed i915 preparation patch like was suggested by Michael Ruhl.
>> The scope of reservation locking is smaller now.
>>
>> 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 | 216 +++++++++++++++---
>> 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 | 14 ++
>> .../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, 325 insertions(+), 155 deletions(-)
>>
> Applied to drm-misc-next
Finally! Fingers crossed that all corner cases where fixed during the
review.
But if anything shows up feel free to loop me in to help fixing things.
Thanks a lot for doing this,
Christian.
On 10/18/22 14:34, Christian König wrote:
> Am 18.10.22 um 01:07 schrieb Dmitry Osipenko:
>> On 10/17/22 20:22, Dmitry Osipenko wrote:
>>> 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:
>>>
>>> v7: - Rebased on top of recent drm-misc-next.
>>>
>>> - Added ack from Jason Gunthorpe to the RDMA patch.
>>>
>>> - Added iosys_map_clear() to dma_buf_vmap_unlocked(), making it
>>> fully
>>> consistent with dma_buf_vmap().
>>>
>>> v6: - Added r-b from Michael Ruhl to the i915 patch.
>>>
>>> - Added acks from Sumit Semwal and updated commit message of the
>>> "Move dma_buf_vmap() to dynamic locking specification" patch like
>>> was suggested by Sumit.
>>>
>>> - Added "!dmabuf" check to dma_buf_vmap_unlocked() to match the
>>> locked
>>> variant of the function, for consistency.
>>>
>>> v5: - Added acks and r-bs that were given to v4.
>>>
>>> - Changed i915 preparation patch like was suggested by Michael
>>> Ruhl.
>>> The scope of reservation locking is smaller now.
>>>
>>> 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 | 216 +++++++++++++++---
>>> 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 | 14 ++
>>> .../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, 325 insertions(+), 155 deletions(-)
>>>
>> Applied to drm-misc-next
>
> Finally! Fingers crossed that all corner cases where fixed during the
> review.
>
> But if anything shows up feel free to loop me in to help fixing things.
>
> Thanks a lot for doing this,
> Christian.
I'll ping you if anything will show up. Thank you!
--
Best regards,
Dmitry
On 11/7/22 20:25, Daniel Vetter wrote:
>> Move dma_buf_mmap() function to the dynamic locking specification by
>> taking the reservation lock. Neither of the today's drivers take the
>> reservation lock within the mmap() callback, hence it's safe to enforce
>> the locking.
>>
>> Acked-by: Sumit Semwal <[email protected]>
>> Acked-by: Christian König <[email protected]>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
> Just noticed this while reading code ... this patch seems to have
> missed dma_buf_mmap_internal()?
>
> Might be good if at least some drivers gain a dma_resv_assert_held in
> that path to make sure we're not quite this bad, together with fixing
> this issue.
Good catch! I'll prepare the patches.
--
Best regards,
Dmitry
On Mon, 17 Oct 2022 at 19:25, Dmitry Osipenko
<[email protected]> wrote:
>
> Move dma_buf_mmap() function to the dynamic locking specification by
> taking the reservation lock. Neither of the today's drivers take the
> reservation lock within the mmap() callback, hence it's safe to enforce
> the locking.
>
> Acked-by: Sumit Semwal <[email protected]>
> Acked-by: Christian König <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
Just noticed this while reading code ... this patch seems to have
missed dma_buf_mmap_internal()?
Might be good if at least some drivers gain a dma_resv_assert_held in
that path to make sure we're not quite this bad, together with fixing
this issue.
-Daniel
> ---
> drivers/dma-buf/dma-buf.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index f54c649f922a..f149b384f4dd 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -1390,6 +1390,8 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_end_cpu_access, DMA_BUF);
> int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
> unsigned long pgoff)
> {
> + int ret;
> +
> if (WARN_ON(!dmabuf || !vma))
> return -EINVAL;
>
> @@ -1410,7 +1412,11 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
> vma_set_file(vma, dmabuf->file);
> vma->vm_pgoff = pgoff;
>
> - return dmabuf->ops->mmap(dmabuf, vma);
> + dma_resv_lock(dmabuf->resv, NULL);
> + ret = dmabuf->ops->mmap(dmabuf, vma);
> + dma_resv_unlock(dmabuf->resv);
> +
> + return ret;
> }
> EXPORT_SYMBOL_NS_GPL(dma_buf_mmap, DMA_BUF);
>
> --
> 2.37.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch