To allow a smooth transition from pinning buffer objects to dynamic invalidation we first start to cache the sg_table for an attachment unless the driver explicitly says to not do so.

Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/dma-buf.c | 24 ++++++++++++++++++++++++ include/linux/dma-buf.h | 11 +++++++++++ 2 files changed, 35 insertions(+)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 7c858020d14b..65161a82d4d5 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -573,6 +573,20 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, list_add(&attach->node, &dmabuf->attachments);

mutex_unlock(&dmabuf->lock); + + if (!dmabuf->ops->dynamic_sgt_mapping) { + struct sg_table *sgt; + + sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL); + if (!sgt) + sgt = ERR_PTR(-ENOMEM); + if (IS_ERR(sgt)) { + dma_buf_detach(dmabuf, attach); + return ERR_CAST(sgt); + } + attach->sgt = sgt; + } + return attach;

err_attach: @@ -595,6 +609,10 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) if (WARN_ON(!dmabuf || !attach)) return;

+ if (attach->sgt) + dmabuf->ops->unmap_dma_buf(attach, attach->sgt, + DMA_BIDIRECTIONAL); + mutex_lock(&dmabuf->lock); list_del(&attach->node); if (dmabuf->ops->detach) @@ -630,6 +648,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, if (WARN_ON(!attach || !attach->dmabuf)) return ERR_PTR(-EINVAL);

+ if (attach->sgt) + return attach->sgt; + sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); if (!sg_table) sg_table = ERR_PTR(-ENOMEM); @@ -657,6 +678,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, if (WARN_ON(!attach || !attach->dmabuf || !sg_table)) return;

+ if (attach->sgt == sg_table) + return; + attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction); } diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 58725f890b5b..0d9c3c13c9fb 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -51,6 +51,16 @@ struct dma_buf_attachment; * @vunmap: [optional] unmaps a vmap from the buffer */ struct dma_buf_ops { + /** + * @dynamic_sgt_mapping: + * + * Flag controlling the caching of the sg_table in the DMA-buf helpers. + * If not set the sg_table is created during device attaching, if set + * the sg_table is created dynamically when dma_buf_map_attachment() is + * called. + */ + bool dynamic_sgt_mapping; + /** * @attach: * @@ -323,6 +333,7 @@ struct dma_buf_attachment { struct device *dev; struct list_head node; void *priv; + struct sg_table *sgt; };

/**

Make it mandatory for dynamic dma-buf callbacks to be called with the reservation lock held.

For static dma-buf exporters we still have the fallback of using cached sgt.

v2: reordered v3: rebased on sgt caching v4: use the cached sgt when possible

Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/dma-buf.c | 24 ++++++++++--------- drivers/gpu/drm/armada/armada_gem.c | 6 ++++- drivers/gpu/drm/drm_prime.c | 6 ++++- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 6 ++++- drivers/gpu/drm/tegra/gem.c | 6 ++++- drivers/gpu/drm/udl/udl_dmabuf.c | 6 ++++- .../common/videobuf2/videobuf2-dma-contig.c | 6 ++++- .../media/common/videobuf2/videobuf2-dma-sg.c | 6 ++++- drivers/staging/media/tegra-vde/tegra-vde.c | 6 ++++- include/linux/dma-buf.h | 23 ++++++++++++++++-- 10 files changed, 74 insertions(+), 21 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index ef480e5fb239..83c92bfd964c 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -532,8 +532,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put); /** * dma_buf_attach - Add the device to dma_buf's attachments list; optionally, * calls attach() of dma_buf_ops to allow device-specific attach functionality - * @dmabuf: [in] buffer to attach device to. - * @dev: [in] device to be attached. + * @info: [in] holds all the attach related information provided + * by the importer. see &struct dma_buf_attach_info + * for further details. * * Returns struct dma_buf_attachment pointer for this attachment. Attachments * must be cleaned up by calling dma_buf_detach(). @@ -547,20 +548,20 @@ EXPORT_SYMBOL_GPL(dma_buf_put); * accessible to @dev, and cannot be moved to a more suitable place. This is * indicated with the error code -EBUSY. */ -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, - struct device *dev) +struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info) { + struct dma_buf *dmabuf = info->dmabuf; struct dma_buf_attachment *attach; int ret;

- if (WARN_ON(!dmabuf || !dev)) + if (WARN_ON(!dmabuf || !info->dev)) return ERR_PTR(-EINVAL);

attach = kzalloc(sizeof(*attach), GFP_KERNEL); if (!attach) return ERR_PTR(-ENOMEM);

- attach->dev = dev; + attach->dev = info->dev; attach->dmabuf = dmabuf;

mutex_lock(&dmabuf->lock); @@ -688,9 +689,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, if (attach->sgt) return attach->sgt;

- sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); - if (!sg_table) - sg_table = ERR_PTR(-ENOMEM); + reservation_object_lock(attach->dmabuf->resv, NULL); + sg_table = dma_buf_map_attachment_locked(attach, direction); + reservation_object_unlock(attach->dmabuf->resv);

return sg_table; } @@ -744,8 +745,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, if (attach->sgt == sg_table) return;

- attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, - direction); + reservation_object_lock(attach->dmabuf->resv, NULL); + dma_buf_unmap_attachment_locked(attach, sg_table, direction); + reservation_object_unlock(attach->dmabuf->resv); } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);

diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index 642d0e70d0f8..19c47821032f 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -501,6 +501,10 @@ armada_gem_prime_export(struct drm_device *dev, struct drm_gem_object *obj, struct drm_gem_object * armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf) { + struct dma_buf_attach_info attach_info = { + .dev = dev->dev, + .dmabuf = buf + }; struct dma_buf_attachment *attach; struct armada_gem_object *dobj;

@@ -516,7 +520,7 @@ armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf) } }

- attach = dma_buf_attach(buf, dev->dev); + attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) return ERR_CAST(attach);

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 231e3f6d5f41..1fadf5d5ed33 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -709,6 +709,10 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, struct dma_buf *dma_buf, struct device *attach_dev) { + struct dma_buf_attach_info attach_info = { + .dev = attach_dev, + .dmabuf = dma_buf + }; struct dma_buf_attachment *attach; struct sg_table *sgt; struct drm_gem_object *obj; @@ -729,7 +733,7 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, if (!dev->driver->gem_prime_import_sg_table) return ERR_PTR(-EINVAL);

- attach = dma_buf_attach(dma_buf, attach_dev); + attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) return ERR_CAST(attach);

diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index 82e2ca17a441..aa7f685bd6ca 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -277,6 +277,10 @@ static const struct drm_i915_gem_object_ops i915_gem_object_dmabuf_ops = { struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf) { + struct dma_buf_attach_info attach_info = { + .dev = dev->dev, + .dmabuf = dma_buf + }; struct dma_buf_attachment *attach; struct drm_i915_gem_object *obj; int ret; @@ -295,7 +299,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, }

/* need to attach */ - attach = dma_buf_attach(dma_buf, dev->dev); + attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) return ERR_CAST(attach);

diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index 4f80100ff5f3..8e6b6c879add 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -332,6 +332,10 @@ struct tegra_bo *tegra_bo_create_with_handle(struct drm_file *file, static struct tegra_bo *tegra_bo_import(struct drm_device *drm, struct dma_buf *buf) { + struct dma_buf_attach_info attach_info = { + .dev = drm->dev, + .dmabuf = buf + }; struct tegra_drm *tegra = drm->dev_private; struct dma_buf_attachment *attach; struct tegra_bo *bo; @@ -341,7 +345,7 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm, if (IS_ERR(bo)) return bo;

- attach = dma_buf_attach(buf, drm->dev); + attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) { err = PTR_ERR(attach); goto free; diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c index 556f62662aa9..86b928f9742f 100644 --- a/drivers/gpu/drm/udl/udl_dmabuf.c +++ b/drivers/gpu/drm/udl/udl_dmabuf.c @@ -226,6 +226,10 @@ static int udl_prime_create(struct drm_device *dev, struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf) { + struct dma_buf_attach_info attach_info = { + .dev = dev->dev, + .dmabuf = dma_buf + }; struct dma_buf_attachment *attach; struct sg_table *sg; struct udl_gem_object *uobj; @@ -233,7 +237,7 @@ struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev,

/* need to attach */ get_device(dev->dev); - attach = dma_buf_attach(dma_buf, dev->dev); + attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) { put_device(dev->dev); return ERR_CAST(attach); diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c index aff0ab7bf83d..1f2687b5eb0e 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c @@ -676,6 +676,10 @@ static void vb2_dc_detach_dmabuf(void *mem_priv) static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf, unsigned long size, enum dma_data_direction dma_dir) { + struct dma_buf_attach_info attach_info = { + .dev = dev, + .dmabuf = dbuf + }; struct vb2_dc_buf *buf; struct dma_buf_attachment *dba;

@@ -691,7 +695,7 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,

buf->dev = dev; /* create attachment for the dmabuf with the user device */ - dba = dma_buf_attach(dbuf, buf->dev); + dba = dma_buf_attach(&attach_info); if (IS_ERR(dba)) { pr_err("failed to attach dmabuf\n"); kfree(buf); diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index 015e737095cd..cbd626d2393a 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c @@ -608,6 +608,10 @@ static void vb2_dma_sg_detach_dmabuf(void *mem_priv) static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf, unsigned long size, enum dma_data_direction dma_dir) { + struct dma_buf_attach_info attach_info = { + .dev = dev, + .dmabuf = dbuf + }; struct vb2_dma_sg_buf *buf; struct dma_buf_attachment *dba;

@@ -623,7 +627,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,

buf->dev = dev; /* create attachment for the dmabuf with the user device */ - dba = dma_buf_attach(dbuf, buf->dev); + dba = dma_buf_attach(&attach_info); if (IS_ERR(dba)) { pr_err("failed to attach dmabuf\n"); kfree(buf); diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c b/drivers/staging/media/tegra-vde/tegra-vde.c index aa6c6bba961e..5a10c1facc27 100644 --- a/drivers/staging/media/tegra-vde/tegra-vde.c +++ b/drivers/staging/media/tegra-vde/tegra-vde.c @@ -568,6 +568,10 @@ static int tegra_vde_attach_dmabuf(struct device *dev, size_t *size, enum dma_data_direction dma_dir) { + struct dma_buf_attach_info attach_info = { + .dev = dev, + .dmabuf = dmabuf + }; struct dma_buf_attachment *attachment; struct dma_buf *dmabuf; struct sg_table *sgt; @@ -591,7 +595,7 @@ static int tegra_vde_attach_dmabuf(struct device *dev, return -EINVAL; }

- attachment = dma_buf_attach(dmabuf, dev); + attachment = dma_buf_attach(&attach_info); if (IS_ERR(attachment)) { dev_err(dev, "Failed to attach dmabuf\n"); err = PTR_ERR(attachment); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 18a78be53541..7e23758db3a4 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -128,6 +128,9 @@ struct dma_buf_ops { * any other kind of sharing that the exporter might wish to make * available to buffer-users. * + * This is always called with the dmabuf->resv object locked when + * no_sgt_cache is true. + * * Returns: * * A &sg_table scatter list of or the backing storage of the DMA buffer, @@ -148,6 +151,9 @@ struct dma_buf_ops { * It should also unpin the backing storage if this is the last mapping * of the DMA buffer, it the exporter supports backing storage * migration. + * + * This is always called with the dmabuf->resv object locked when + * no_sgt_cache is true. */ void (*unmap_dma_buf)(struct dma_buf_attachment *, struct sg_table *, @@ -370,6 +376,19 @@ struct dma_buf_export_info { struct dma_buf_export_info name = { .exp_name = KBUILD_MODNAME, \ .owner = THIS_MODULE }

+/** + * struct dma_buf_attach_info - holds information needed to attach to a dma_buf + * @dmabuf: the exported dma_buf + * @dev: the device which wants to import the attachment + * + * This structure holds the information required to attach to a buffer. Used + * with dma_buf_attach() only. + */ +struct dma_buf_attach_info { + struct dma_buf *dmabuf; + struct device *dev; +}; + /** * get_dma_buf - convenience wrapper for get_file. * @dmabuf: [in] pointer to dma_buf @@ -384,8 +403,8 @@ static inline void get_dma_buf(struct dma_buf *dmabuf) get_file(dmabuf->file); }

-struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, - struct device *dev); +struct dma_buf_attachment * +dma_buf_attach(const struct dma_buf_attach_info *info); void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *dmabuf_attach);

Add optional explicit pinning callbacks instead of implicitly assume the exporter pins the buffer when a mapping is created.

Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/dma-buf.c | 39 +++++++++++++++++++++++++++++++++++++++ include/linux/dma-buf.h | 37 +++++++++++++++++++++++++++++++------ 2 files changed, 70 insertions(+), 6 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index a3738fab3927..f23ff8355505 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -630,6 +630,41 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) } EXPORT_SYMBOL_GPL(dma_buf_detach);

+/** + * dma_buf_pin - Lock down the DMA-buf + * + * @dmabuf: [in] DMA-buf to lock down. + * + * Returns: + * 0 on success, negative error code on failure. + */ +int dma_buf_pin(struct dma_buf *dmabuf) +{ + int ret = 0; + + reservation_object_assert_held(dmabuf->resv); + + if (dmabuf->ops->pin) + ret = dmabuf->ops->pin(dmabuf); + + return ret; +} +EXPORT_SYMBOL_GPL(dma_buf_pin); + +/** + * dma_buf_unpin - Remove lock from DMA-buf + * + * @dmabuf: [in] DMA-buf to unlock. + */ +void dma_buf_unpin(struct dma_buf *dmabuf) +{ + reservation_object_assert_held(dmabuf->resv); + + if (dmabuf->ops->unpin) + dmabuf->ops->unpin(dmabuf); +} +EXPORT_SYMBOL_GPL(dma_buf_unpin); + /** * dma_buf_map_attachment_locked - Maps the buffer into _device_ address space * with the reservation lock held. Is a wrapper for map_dma_buf() of the @@ -666,6 +701,8 @@ dma_buf_map_attachment_locked(struct dma_buf_attachment *attach, */ if (attach->invalidate) list_del(&attach->node); + else + dma_buf_pin(attach->dmabuf); sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); if (attach->invalidate) list_add(&attach->node, &attach->dmabuf->attachments); @@ -735,6 +772,8 @@ void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *attach,

attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction); + if (!attach->invalidate) + dma_buf_unpin(attach->dmabuf); } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment_locked);

diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index ece4638359a8..a615b74e5894 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -100,14 +100,40 @@ struct dma_buf_ops { */ void (*detach)(struct dma_buf *, struct dma_buf_attachment *);

+ /** + * @pin_dma_buf: + * + * This is called by dma_buf_pin and lets the exporter know that an + * importer assumes that the DMA-buf can't be invalidated any more. + * + * This is called with the dmabuf->resv object locked. + * + * This callback is optional. + * + * Returns: + * + * 0 on success, negative error code on failure. + */ + int (*pin)(struct dma_buf *); + + /** + * @unpin_dma_buf: + * + * This is called by dma_buf_unpin and lets the exporter know that an + * importer doesn't need to the DMA-buf to stay were it is any more. + * + * This is called with the dmabuf->resv object locked. + * + * This callback is optional. + */ + void (*unpin)(struct dma_buf *); + /** * @map_dma_buf: * * This is called by dma_buf_map_attachment() and is used to map a * shared &dma_buf into device address space, and it is mandatory. It - * can only be called if @attach has been called successfully. This - * essentially pins the DMA buffer into place, and it cannot be moved - * any more + * can only be called if @attach has been called successfully. * * This call may sleep, e.g. when the backing storage first needs to be * allocated, or moved to a location suitable for all currently attached @@ -148,9 +174,6 @@ struct dma_buf_ops { * * This is called by dma_buf_unmap_attachment() and should unmap and * release the &sg_table allocated in @map_dma_buf, and it is mandatory. - * It should also unpin the backing storage if this is the last mapping - * of the DMA buffer, it the exporter supports backing storage - * migration. * * This is always called with the dmabuf->resv object locked when * no_sgt_cache is true. @@ -442,6 +465,8 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags); struct dma_buf *dma_buf_get(int fd); void dma_buf_put(struct dma_buf *dmabuf);

+int dma_buf_pin(struct dma_buf *dmabuf); +void dma_buf_unpin(struct dma_buf *dmabuf); struct sg_table *dma_buf_map_attachment_locked(struct dma_buf_attachment *, enum dma_data_direction); struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,

Pipeline removal of the BOs backing store when no placement is given during validation.

Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/ttm/ttm_bo.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 41d07faa2eae..8e7e7caee9d5 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -1161,6 +1161,18 @@ int ttm_bo_validate(struct ttm_buffer_object *bo, uint32_t new_flags;

reservation_object_assert_held(bo->resv); + + /* + * Remove the backing store if no placement is given. + */ + if (!placement->num_placement && !placement->num_busy_placement) { + ret = ttm_bo_pipeline_gutting(bo); + if (ret) + return ret; + + return ttm_tt_create(bo, false); + } + /* * Check whether we need to move buffer. */

The caching of SGT's is actually quite harmful and should probably removed altogether when all drivers are audited.

Start by providing a separate DMA-buf export implementation in amdgpu. This is also a prerequisite of unpinned DMA-buf handling.

v2: fix unintended recursion, remove debugging leftovers v3: split out from unpinned DMA-buf work v4: rebase on top of new no_sgt_cache flag

Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 - drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h | 1 - drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 94 ++++++++++------------- 3 files changed, 40 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 13a68f62bcc8..f1815223a1a1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -1254,7 +1254,6 @@ static struct drm_driver kms_driver = { .gem_prime_export = amdgpu_gem_prime_export, .gem_prime_import = amdgpu_gem_prime_import, .gem_prime_res_obj = amdgpu_gem_prime_res_obj, - .gem_prime_get_sg_table = amdgpu_gem_prime_get_sg_table, .gem_prime_import_sg_table = amdgpu_gem_prime_import_sg_table, .gem_prime_vmap = amdgpu_gem_prime_vmap, .gem_prime_vunmap = amdgpu_gem_prime_vunmap, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h index f1ddfc50bcc7..0c50d14a9739 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h @@ -39,7 +39,6 @@ int amdgpu_gem_object_open(struct drm_gem_object *obj, void amdgpu_gem_object_close(struct drm_gem_object *obj, struct drm_file *file_priv); unsigned long amdgpu_gem_timeout(uint64_t timeout_ns); -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object *obj); struct drm_gem_object * amdgpu_gem_prime_import_sg_table(struct drm_device *dev, struct dma_buf_attachment *attach, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c index a38e0fb4a6fe..8d748f9d0292 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c @@ -40,22 +40,6 @@ #include <linux/dma-buf.h> #include <linux/dma-fence-array.h>

-/** - * amdgpu_gem_prime_get_sg_table - &drm_driver.gem_prime_get_sg_table - * implementation - * @obj: GEM buffer object (BO) - * - * Returns: - * A scatter/gather table for the pinned pages of the BO's memory. - */ -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object *obj) -{ - struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); - int npages = bo->tbo.num_pages; - - return drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages); -} - /** * amdgpu_gem_prime_vmap - &dma_buf_ops.vmap implementation * @obj: GEM BO @@ -231,34 +215,29 @@ __reservation_object_make_exclusive(struct reservation_object *obj) }

/** - * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation - * @dma_buf: Shared DMA buffer + * amdgpu_gem_map_dma_buf - &dma_buf_ops.map_dma_buf implementation * @attach: DMA-buf attachment + * @dir: DMA direction * * Makes sure that the shared DMA buffer can be accessed by the target device. * For now, simply pins it to the GTT domain, where it should be accessible by * all DMA devices. * * Returns: - * 0 on success or a negative error code on failure. + * sg_table filled with the DMA addresses to use or ERR_PRT with negative error + * code. */ -static int amdgpu_gem_map_attach(struct dma_buf *dma_buf, - struct dma_buf_attachment *attach) +static struct sg_table * +amdgpu_gem_map_dma_buf(struct dma_buf_attachment *attach, + enum dma_data_direction dir) { + struct dma_buf *dma_buf = attach->dmabuf; struct drm_gem_object *obj = dma_buf->priv; struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); + struct sg_table *sgt; long r;

- r = drm_gem_map_attach(dma_buf, attach); - if (r) - return r; - - r = amdgpu_bo_reserve(bo, false); - if (unlikely(r != 0)) - goto error_detach; - - if (attach->dev->driver != adev->dev->driver) { /* * We only create shared fences for internal use, but importers @@ -270,53 +249,61 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf, */ r = __reservation_object_make_exclusive(bo->tbo.resv); if (r) - goto error_unreserve; + return ERR_PTR(r); }

/* pin buffer into GTT */ r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); if (r) - goto error_unreserve; + return ERR_PTR(r); + + sgt = drm_prime_pages_to_sg(bo->tbo.ttm->pages, bo->tbo.num_pages); + if (IS_ERR(sgt)) + return sgt; + + if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir, + DMA_ATTR_SKIP_CPU_SYNC)) + goto error_free;

if (attach->dev->driver != adev->dev->driver) bo->prime_shared_count++;

-error_unreserve: - amdgpu_bo_unreserve(bo); + return sgt;

-error_detach: - if (r) - drm_gem_map_detach(dma_buf, attach); - return r; +error_free: + sg_free_table(sgt); + kfree(sgt); + return ERR_PTR(-ENOMEM); }

/** - * amdgpu_gem_map_detach - &dma_buf_ops.detach implementation - * @dma_buf: Shared DMA buffer + * amdgpu_gem_unmap_dma_buf - &dma_buf_ops.unmap_dma_buf implementation * @attach: DMA-buf attachment + * @sgt: sg_table to unmap + * @dir: DMA direction * * This is called when a shared DMA buffer no longer needs to be accessible by * another device. For now, simply unpins the buffer from GTT. */ -static void amdgpu_gem_map_detach(struct dma_buf *dma_buf, - struct dma_buf_attachment *attach) +static void amdgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach, + struct sg_table *sgt, + enum dma_data_direction dir) { + struct dma_buf *dma_buf = attach->dmabuf; struct drm_gem_object *obj = dma_buf->priv; struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); - int ret = 0; - - ret = amdgpu_bo_reserve(bo, true); - if (unlikely(ret != 0)) - goto error;

amdgpu_bo_unpin(bo); + if (attach->dev->driver != adev->dev->driver && bo->prime_shared_count) bo->prime_shared_count--; - amdgpu_bo_unreserve(bo);

-error: - drm_gem_map_detach(dma_buf, attach); + if (sgt) { + dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir); + sg_free_table(sgt); + kfree(sgt); + } }

/** @@ -374,10 +361,9 @@ static int amdgpu_gem_begin_cpu_access(struct dma_buf *dma_buf, }

const struct dma_buf_ops amdgpu_dmabuf_ops = { - .attach = amdgpu_gem_map_attach, - .detach = amdgpu_gem_map_detach, - .map_dma_buf = drm_gem_map_dma_buf, - .unmap_dma_buf = drm_gem_unmap_dma_buf, + .dynamic_sgt_mapping = true, + .map_dma_buf = amdgpu_gem_map_dma_buf, + .unmap_dma_buf = amdgpu_gem_unmap_dma_buf, .release = drm_gem_dmabuf_release, .begin_cpu_access = amdgpu_gem_begin_cpu_access, .mmap = drm_gem_dmabuf_mmap,

Pin and unpin the DMA-buf exported BOs only on demand and invalidate all DMA-buf mappings when the underlying BO moves.

Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 5 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 73 ++++++++++++++++++++-- 2 files changed, 72 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index ec9e45004bff..fdb98eb562db 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -31,6 +31,7 @@ */ #include <linux/list.h> #include <linux/slab.h> +#include <linux/dma-buf.h> #include <drm/drmP.h> #include <drm/amdgpu_drm.h> #include <drm/drm_cache.h> @@ -1192,6 +1193,10 @@ void amdgpu_bo_move_notify(struct ttm_buffer_object *bo,

amdgpu_bo_kunmap(abo);

+ if (abo->gem_base.dma_buf && !abo->gem_base.import_attach && + bo->mem.mem_type != TTM_PL_SYSTEM) + dma_buf_invalidate_mappings(abo->gem_base.dma_buf); + /* remember the eviction */ if (evict) atomic64_inc(&adev->num_evictions); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c index 56e2a606b9a1..40cd89271b20 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c @@ -209,6 +209,61 @@ __reservation_object_make_exclusive(struct reservation_object *obj) return -ENOMEM; }

+/** + * amdgpu_gem_pin_dma_buf - &dma_buf_ops.pin_dma_buf implementation + * + * @dma_buf: DMA-buf to pin in memory + * + * Pin the BO which is backing the DMA-buf so that it can't move any more. + */ +static int amdgpu_gem_pin_dma_buf(struct dma_buf *dma_buf) +{ + struct drm_gem_object *obj = dma_buf->priv; + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); + + /* pin buffer into GTT */ + return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); +} + +/** + * amdgpu_gem_unpin_dma_buf - &dma_buf_ops.unpin_dma_buf implementation + * + * @dma_buf: DMA-buf to unpin + * + * Unpin a previously pinned BO to make it movable again. + */ +static void amdgpu_gem_unpin_dma_buf(struct dma_buf *dma_buf) +{ + struct drm_gem_object *obj = dma_buf->priv; + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); + + amdgpu_bo_unpin(bo); +} + +/** + * amdgpu_gem_dma_buf_attach - &dma_buf_ops.attach implementation + * + * @dma_buf: DMA-buf we attach to + * @attach: DMA-buf attachment + * + * Returns: + * Always zero for success. + */ +static int amdgpu_gem_dma_buf_attach(struct dma_buf *dma_buf, + struct dma_buf_attachment *attach) +{ + struct drm_gem_object *obj = dma_buf->priv; + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); + + /* Make sure the buffer is pinned when userspace didn't set GTT as + * preferred domain. This avoid ping/pong situations with scan out BOs. + */ + if (!(bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT)) + attach->invalidate = NULL; + + return 0; +} + /** * amdgpu_gem_map_dma_buf - &dma_buf_ops.map_dma_buf implementation * @attach: DMA-buf attachment @@ -247,10 +302,15 @@ amdgpu_gem_map_dma_buf(struct dma_buf_attachment *attach, return ERR_PTR(r); }

- /* pin buffer into GTT */ - r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); - if (r) - return ERR_PTR(r); + if (attach->invalidate) { + /* move buffer into GTT */ + struct ttm_operation_ctx ctx = { false, false }; + + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT); + r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); + if (r) + return ERR_PTR(r); + }

sgt = drm_prime_pages_to_sg(bo->tbo.ttm->pages, bo->tbo.num_pages); if (IS_ERR(sgt)) @@ -289,8 +349,6 @@ static void amdgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach, struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);

- amdgpu_bo_unpin(bo); - if (attach->dev->driver != adev->dev->driver && bo->prime_shared_count) bo->prime_shared_count--;

@@ -357,6 +415,9 @@ static int amdgpu_gem_begin_cpu_access(struct dma_buf *dma_buf,

const struct dma_buf_ops amdgpu_dmabuf_ops = { .dynamic_sgt_mapping = true, + .attach = amdgpu_gem_dma_buf_attach, + .pin = amdgpu_gem_pin_dma_buf, + .unpin = amdgpu_gem_unpin_dma_buf, .map_dma_buf = amdgpu_gem_map_dma_buf, .unmap_dma_buf = amdgpu_gem_unmap_dma_buf, .release = drm_gem_dmabuf_release,

On Tue, Apr 16, 2019 at 08:38:33PM +0200, Christian König wrote:

Each importer can now provide an invalidate_mappings callback.

This allows the exporter to provide the mappings without the need to pin the backing store.

v2: don't try to invalidate mappings when the callback is NULL, lock the reservation obj while using the attachments, add helper to set the callback v3: move flag for invalidation support into the DMA-buf, use new attach_info structure to set the callback v4: use importer_priv field instead of mangling exporter priv. v5: drop invalidation_supported flag

Signed-off-by: Christian König christian.koenig@amd.com

drivers/dma-buf/dma-buf.c | 37 +++++++++++++++++++++++++++++++++++++ include/linux/dma-buf.h | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 83c92bfd964c..a3738fab3927 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -563,6 +563,8 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info attach->dev = info->dev; attach->dmabuf = dmabuf;

• attach->importer_priv = info->importer_priv;
• attach->invalidate = info->invalidate;

mutex_lock(&dmabuf->lock); @@ -571,7 +573,9 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info if (ret) goto err_attach; }

• reservation_object_lock(dmabuf->resv, NULL); list_add(&attach->node, &dmabuf->attachments);
• reservation_object_unlock(dmabuf->resv);

mutex_unlock(&dmabuf->lock); @@ -615,7 +619,9 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) DMA_BIDIRECTIONAL); mutex_lock(&dmabuf->lock);

• reservation_object_lock(dmabuf->resv, NULL); list_del(&attach->node);
• reservation_object_unlock(dmabuf->resv); if (dmabuf->ops->detach) dmabuf->ops->detach(dmabuf, attach);

@@ -653,7 +659,16 @@ dma_buf_map_attachment_locked(struct dma_buf_attachment *attach, if (attach->sgt) return attach->sgt;

• /*

• * Mapping a DMA-buf can trigger its invalidation, prevent sending this
  

• * event to the caller by temporary removing this attachment from the
  

• * list.
  

• */
  

• if (attach->invalidate)

• list_del(&attach->node);
  

  sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
• if (attach->invalidate)

• list_add(&attach->node, &attach->dmabuf->attachments);
  

  if (!sg_table) sg_table = ERR_PTR(-ENOMEM);

@@ -751,6 +766,26 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); +/**

• • dma_buf_invalidate_mappings - invalidate all mappings of this dma_buf
• • @dmabuf: [in] buffer which mappings should be invalidated
• • Informs all attachmenst that they need to destroy and recreated all their
• • mappings.
• */

+void dma_buf_invalidate_mappings(struct dma_buf *dmabuf) +{

• struct dma_buf_attachment *attach;
• reservation_object_assert_held(dmabuf->resv);
• list_for_each_entry(attach, &dmabuf->attachments, node)

• if (attach->invalidate)
  

• 	attach->invalidate(attach);
  

+} +EXPORT_SYMBOL_GPL(dma_buf_invalidate_mappings);

/**

• DOC: cpu access

@@ -1163,10 +1198,12 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) seq_puts(s, "\tAttached Devices:\n"); attach_count = 0;

• reservation_object_lock(buf_obj->resv, NULL);
  

  list_for_each_entry(attach_obj, &buf_obj->attachments, node) { seq_printf(s, "\t%s\n", dev_name(attach_obj->dev)); attach_count++; }

• reservation_object_unlock(buf_obj->resv);
  

seq_printf(s, "Total %d devices attached\n\n", attach_count); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 7e23758db3a4..ece4638359a8 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -324,6 +324,7 @@ struct dma_buf {

• @dev: device attached to the buffer.
• @node: list of dma_buf_attachment.
• @priv: exporter specific attachment data.

• • @importer_priv: importer specific attachment data.
  • This structure holds the attachment information between the dma_buf buffer
  • and its user device(s). The list contains one attachment struct per device

@@ -340,6 +341,29 @@ struct dma_buf_attachment { struct list_head node; void *priv; struct sg_table *sgt;

• void *importer_priv;
• /**

• * @invalidate:
  

• *
  

• * Optional callback provided by the importer of the dma-buf.
  

• *
  

• * If provided the exporter can avoid pinning the backing store while
  

• * mappings exists.
  

• *
  

• * The function is called with the lock of the reservation object
  

• * associated with the dma_buf held and the mapping function must be
  

• * called with this lock held as well. This makes sure that no mapping
  

• * is created concurrently with an ongoing invalidation.
  

• *
  

• * After the callback all existing mappings are still valid until all
  

• * fences in the dma_bufs reservation object are signaled, but should be
  

• * destroyed by the importer as soon as possible.
  

• *
  

• * New mappings can be created immediately, but can't be used before the
  

• * exclusive fence in the dma_bufs reservation object is signaled.
  

• */
  

• void (*invalidate)(struct dma_buf_attachment *attach);

I would put the long kerneldoc into dma_buf_attach_info (as an inline comment, you can mix the styles). And reference it from here with something like

"Set from &dma_buf_attach_info.invalidate in dma_buf_attach(), see there for more information."

This here feels a bit too much hidden. -Daniel

}; /** @@ -378,8 +402,10 @@ struct dma_buf_export_info { /**

• struct dma_buf_attach_info - holds information needed to attach to a dma_buf

• • @dmabuf: the exported dma_buf
• • @dev: the device which wants to import the attachment

• • @dmabuf: the exported dma_buf
• • @dev: the device which wants to import the attachment
• • @importer_priv: private data of importer to this attachment
• • @invalidate: callback to use for invalidating mappings
  • This structure holds the information required to attach to a buffer. Used
  • with dma_buf_attach() only.

@@ -387,6 +413,8 @@ struct dma_buf_export_info { struct dma_buf_attach_info { struct dma_buf *dmabuf; struct device *dev;

• void *importer_priv;
• void (*invalidate)(struct dma_buf_attachment *attach);

}; /** @@ -423,6 +451,7 @@ void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *, enum dma_data_direction); void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *, enum dma_data_direction); +void dma_buf_invalidate_mappings(struct dma_buf *dma_buf); 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, -- 2.17.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Am 17.04.19 um 16:08 schrieb Daniel Vetter:

On Tue, Apr 16, 2019 at 08:38:32PM +0200, Christian König wrote:

...

Make it mandatory for dynamic dma-buf callbacks to be called with the reservation lock held.

For static dma-buf exporters we still have the fallback of using cached sgt.

v2: reordered v3: rebased on sgt caching v4: use the cached sgt when possible

Signed-off-by: Christian König christian.koenig@amd.com

I think there's a bit a rebase chaos going on:

• some comments left behind with no_sgt_cache, which I can't find anymore
• the function signature rework of dma_buf_attach should imo be split out

Ah, crap thought I've got all of those. Going to fix that.

Next issue is that the reservation object locking is still in the path of dma_buf_map, so probably still going to result in tons of lockdep splats. Except the i915+amdgpu path should now work due to the fastpath.

I actually found a solution for that :)

The idea is now that we always cache the sgt in the attachment unless the dynamic flag (previously no_sgt_cache flag) is set. And this cached sgt is created without holding the lock.

We either need to document that really well or maybe split the mapping callbacks into map/unmap and map_lock/unmap_locked. Opinions?

Not sure that's a solution that really works, just hides that fundamentally we still have that issue of incompatible locking chains between different drivers.

We can now make a slow transition between static and dynamic DMA-buf handling, so only driver who can do the locking will be affected.

Christian.

-Daniel

...

---

drivers/dma-buf/dma-buf.c | 24 ++++++++++--------- drivers/gpu/drm/armada/armada_gem.c | 6 ++++- drivers/gpu/drm/drm_prime.c | 6 ++++- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 6 ++++- drivers/gpu/drm/tegra/gem.c | 6 ++++- drivers/gpu/drm/udl/udl_dmabuf.c | 6 ++++- .../common/videobuf2/videobuf2-dma-contig.c | 6 ++++- .../media/common/videobuf2/videobuf2-dma-sg.c | 6 ++++- drivers/staging/media/tegra-vde/tegra-vde.c | 6 ++++- include/linux/dma-buf.h | 23 ++++++++++++++++-- 10 files changed, 74 insertions(+), 21 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index ef480e5fb239..83c92bfd964c 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -532,8 +532,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put); /**

• dma_buf_attach - Add the device to dma_buf's attachments list; optionally,
• calls attach() of dma_buf_ops to allow device-specific attach functionality

• • @dmabuf: [in] buffer to attach device to.
• • @dev: [in] device to be attached.

• • @info: [in] holds all the attach related information provided

• • 	by the importer. see &struct dma_buf_attach_info
    

• • 	for further details.
    

  • Returns struct dma_buf_attachment pointer for this attachment. Attachments
  • must be cleaned up by calling dma_buf_detach().

@@ -547,20 +548,20 @@ EXPORT_SYMBOL_GPL(dma_buf_put);

• accessible to @dev, and cannot be moved to a more suitable place. This is
• indicated with the error code -EBUSY.

*/ -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,

• 			  struct device *dev)
  

+struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info) {

• struct dma_buf *dmabuf = info->dmabuf; struct dma_buf_attachment *attach; int ret;

• if (WARN_ON(!dmabuf || !dev))

• if (WARN_ON(!dmabuf || !info->dev)) return ERR_PTR(-EINVAL);

attach = kzalloc(sizeof(*attach), GFP_KERNEL); if (!attach) return ERR_PTR(-ENOMEM);

• attach->dev = dev;

• attach->dev = info->dev; attach->dmabuf = dmabuf;

mutex_lock(&dmabuf->lock); @@ -688,9 +689,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, if (attach->sgt) return attach->sgt;

• sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
• if (!sg_table)

• sg_table = ERR_PTR(-ENOMEM);
  

• reservation_object_lock(attach->dmabuf->resv, NULL);
• sg_table = dma_buf_map_attachment_locked(attach, direction);
• reservation_object_unlock(attach->dmabuf->resv);

return sg_table; } @@ -744,8 +745,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, if (attach->sgt == sg_table) return;

• attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,

• 				direction);
  

• reservation_object_lock(attach->dmabuf->resv, NULL);
• dma_buf_unmap_attachment_locked(attach, sg_table, direction);
• reservation_object_unlock(attach->dmabuf->resv); } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);

diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index 642d0e70d0f8..19c47821032f 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -501,6 +501,10 @@ armada_gem_prime_export(struct drm_device *dev, struct drm_gem_object *obj, struct drm_gem_object * armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev->dev,
  

• .dmabuf = buf
  

• }; struct dma_buf_attachment *attach; struct armada_gem_object *dobj;

@@ -516,7 +520,7 @@ armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf) } }

• attach = dma_buf_attach(buf, dev->dev);

• attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) return ERR_CAST(attach);

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 231e3f6d5f41..1fadf5d5ed33 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -709,6 +709,10 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, struct dma_buf *dma_buf, struct device *attach_dev) {

• struct dma_buf_attach_info attach_info = {

• .dev = attach_dev,
  

• .dmabuf = dma_buf
  

• }; struct dma_buf_attachment *attach; struct sg_table *sgt; struct drm_gem_object *obj;

@@ -729,7 +733,7 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, if (!dev->driver->gem_prime_import_sg_table) return ERR_PTR(-EINVAL);

• attach = dma_buf_attach(dma_buf, attach_dev);

• attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) return ERR_CAST(attach);

diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index 82e2ca17a441..aa7f685bd6ca 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -277,6 +277,10 @@ static const struct drm_i915_gem_object_ops i915_gem_object_dmabuf_ops = { struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev->dev,
  

• .dmabuf = dma_buf
  

• }; struct dma_buf_attachment *attach; struct drm_i915_gem_object *obj; int ret;

@@ -295,7 +299,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, } /* need to attach */

• attach = dma_buf_attach(dma_buf, dev->dev);

• attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) return ERR_CAST(attach);

diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index 4f80100ff5f3..8e6b6c879add 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -332,6 +332,10 @@ struct tegra_bo *tegra_bo_create_with_handle(struct drm_file *file, static struct tegra_bo *tegra_bo_import(struct drm_device *drm, struct dma_buf *buf) {

• struct dma_buf_attach_info attach_info = {

• .dev = drm->dev,
  

• .dmabuf = buf
  

• }; struct tegra_drm *tegra = drm->dev_private; struct dma_buf_attachment *attach; struct tegra_bo *bo;

@@ -341,7 +345,7 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm, if (IS_ERR(bo)) return bo;

• attach = dma_buf_attach(buf, drm->dev);

• attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) { err = PTR_ERR(attach); goto free;

diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c index 556f62662aa9..86b928f9742f 100644 --- a/drivers/gpu/drm/udl/udl_dmabuf.c +++ b/drivers/gpu/drm/udl/udl_dmabuf.c @@ -226,6 +226,10 @@ static int udl_prime_create(struct drm_device *dev, struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev->dev,
  

• .dmabuf = dma_buf
  

• }; struct dma_buf_attachment *attach; struct sg_table *sg; struct udl_gem_object *uobj;

@@ -233,7 +237,7 @@ struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev, /* need to attach */ get_device(dev->dev);

• attach = dma_buf_attach(dma_buf, dev->dev);

• attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) { put_device(dev->dev); return ERR_CAST(attach);

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c index aff0ab7bf83d..1f2687b5eb0e 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c @@ -676,6 +676,10 @@ static void vb2_dc_detach_dmabuf(void *mem_priv) static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf, unsigned long size, enum dma_data_direction dma_dir) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev,
  

• .dmabuf = dbuf
  

• }; struct vb2_dc_buf *buf; struct dma_buf_attachment *dba;

@@ -691,7 +695,7 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf, buf->dev = dev; /* create attachment for the dmabuf with the user device */

• dba = dma_buf_attach(dbuf, buf->dev);

• dba = dma_buf_attach(&attach_info); if (IS_ERR(dba)) { pr_err("failed to attach dmabuf\n"); kfree(buf);

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index 015e737095cd..cbd626d2393a 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c @@ -608,6 +608,10 @@ static void vb2_dma_sg_detach_dmabuf(void *mem_priv) static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf, unsigned long size, enum dma_data_direction dma_dir) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev,
  

• .dmabuf = dbuf
  

• }; struct vb2_dma_sg_buf *buf; struct dma_buf_attachment *dba;

@@ -623,7 +627,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf, buf->dev = dev; /* create attachment for the dmabuf with the user device */

• dba = dma_buf_attach(dbuf, buf->dev);

• dba = dma_buf_attach(&attach_info); if (IS_ERR(dba)) { pr_err("failed to attach dmabuf\n"); kfree(buf);

diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c b/drivers/staging/media/tegra-vde/tegra-vde.c index aa6c6bba961e..5a10c1facc27 100644 --- a/drivers/staging/media/tegra-vde/tegra-vde.c +++ b/drivers/staging/media/tegra-vde/tegra-vde.c @@ -568,6 +568,10 @@ static int tegra_vde_attach_dmabuf(struct device *dev, size_t *size, enum dma_data_direction dma_dir) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev,
  

• .dmabuf = dmabuf
  

• }; struct dma_buf_attachment *attachment; struct dma_buf *dmabuf; struct sg_table *sgt;

@@ -591,7 +595,7 @@ static int tegra_vde_attach_dmabuf(struct device *dev, return -EINVAL; }

• attachment = dma_buf_attach(dmabuf, dev);

• attachment = dma_buf_attach(&attach_info); if (IS_ERR(attachment)) { dev_err(dev, "Failed to attach dmabuf\n"); err = PTR_ERR(attachment);

diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 18a78be53541..7e23758db3a4 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -128,6 +128,9 @@ struct dma_buf_ops { * any other kind of sharing that the exporter might wish to make * available to buffer-users. *

• * This is always called with the dmabuf->resv object locked when
  

• * no_sgt_cache is true.
  

• *
  

  • Returns:
  • A &sg_table scatter list of or the backing storage of the DMA buffer,

@@ -148,6 +151,9 @@ struct dma_buf_ops { * It should also unpin the backing storage if this is the last mapping * of the DMA buffer, it the exporter supports backing storage * migration.

• *
  

• * This is always called with the dmabuf->resv object locked when
  

• * no_sgt_cache is true.
  

  */ void (*unmap_dma_buf)(struct dma_buf_attachment *, struct sg_table *,

@@ -370,6 +376,19 @@ struct dma_buf_export_info { struct dma_buf_export_info name = { .exp_name = KBUILD_MODNAME, \ .owner = THIS_MODULE } +/**

• • struct dma_buf_attach_info - holds information needed to attach to a dma_buf
• • @dmabuf: the exported dma_buf
• • @dev: the device which wants to import the attachment
• • This structure holds the information required to attach to a buffer. Used
• • with dma_buf_attach() only.
• */

+struct dma_buf_attach_info {

• struct dma_buf *dmabuf;
• struct device *dev;

+};

• /**
  • get_dma_buf - convenience wrapper for get_file.
  • @dmabuf: [in] pointer to dma_buf

@@ -384,8 +403,8 @@ static inline void get_dma_buf(struct dma_buf *dmabuf) get_file(dmabuf->file); } -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,

• 					struct device *dev);
  

+struct dma_buf_attachment * +dma_buf_attach(const struct dma_buf_attach_info *info); void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *dmabuf_attach); -- 2.17.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Wed, Apr 17, 2019 at 04:14:32PM +0200, Christian König wrote:

Am 17.04.19 um 16:08 schrieb Daniel Vetter:

...

On Tue, Apr 16, 2019 at 08:38:32PM +0200, Christian König wrote:

...

Make it mandatory for dynamic dma-buf callbacks to be called with the reservation lock held.

For static dma-buf exporters we still have the fallback of using cached sgt.

v2: reordered v3: rebased on sgt caching v4: use the cached sgt when possible

Signed-off-by: Christian König christian.koenig@amd.com

I think there's a bit a rebase chaos going on:

• some comments left behind with no_sgt_cache, which I can't find anymore
• the function signature rework of dma_buf_attach should imo be split out

Ah, crap thought I've got all of those. Going to fix that.

...

Next issue is that the reservation object locking is still in the path of dma_buf_map, so probably still going to result in tons of lockdep splats. Except the i915+amdgpu path should now work due to the fastpath.

I actually found a solution for that :)

The idea is now that we always cache the sgt in the attachment unless the dynamic flag (previously no_sgt_cache flag) is set. And this cached sgt is created without holding the lock.

Yeah I think that idea works.

We either need to document that really well or maybe split the mapping callbacks into map/unmap and map_lock/unmap_locked. Opinions?

I think the implementation doesn't. Exporter can't decide on its own whether dynamic/static is what's needed, we need to decide that for each attachment, taking both exporter and importer capabilities into account.

I think if we do that, then it should work out. I replied on the pin/unpin interface patch with some more concrete thoughts. Maybe best to continue that discussion there, with more context. -Daniel

...

Not sure that's a solution that really works, just hides that fundamentally we still have that issue of incompatible locking chains between different drivers.

We can now make a slow transition between static and dynamic DMA-buf handling, so only driver who can do the locking will be affected.

Christian.

...

-Daniel

...

---

drivers/dma-buf/dma-buf.c | 24 ++++++++++--------- drivers/gpu/drm/armada/armada_gem.c | 6 ++++- drivers/gpu/drm/drm_prime.c | 6 ++++- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 6 ++++- drivers/gpu/drm/tegra/gem.c | 6 ++++- drivers/gpu/drm/udl/udl_dmabuf.c | 6 ++++- .../common/videobuf2/videobuf2-dma-contig.c | 6 ++++- .../media/common/videobuf2/videobuf2-dma-sg.c | 6 ++++- drivers/staging/media/tegra-vde/tegra-vde.c | 6 ++++- include/linux/dma-buf.h | 23 ++++++++++++++++-- 10 files changed, 74 insertions(+), 21 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index ef480e5fb239..83c92bfd964c 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -532,8 +532,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put); /**

• dma_buf_attach - Add the device to dma_buf's attachments list; optionally,
• calls attach() of dma_buf_ops to allow device-specific attach functionality

• • @dmabuf: [in] buffer to attach device to.
• • @dev: [in] device to be attached.

• • @info: [in] holds all the attach related information provided

• • 	by the importer. see &struct dma_buf_attach_info
    

• • 	for further details.
    

  • Returns struct dma_buf_attachment pointer for this attachment. Attachments
  • must be cleaned up by calling dma_buf_detach().

@@ -547,20 +548,20 @@ EXPORT_SYMBOL_GPL(dma_buf_put);

• accessible to @dev, and cannot be moved to a more suitable place. This is
• indicated with the error code -EBUSY.

*/ -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,

• 			  struct device *dev)
  

+struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info) {

• struct dma_buf *dmabuf = info->dmabuf; struct dma_buf_attachment *attach; int ret;

• if (WARN_ON(!dmabuf || !dev))

• if (WARN_ON(!dmabuf || !info->dev)) return ERR_PTR(-EINVAL); attach = kzalloc(sizeof(*attach), GFP_KERNEL); if (!attach) return ERR_PTR(-ENOMEM);

• attach->dev = dev;

• attach->dev = info->dev; attach->dmabuf = dmabuf; mutex_lock(&dmabuf->lock);

@@ -688,9 +689,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, if (attach->sgt) return attach->sgt;

• sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
• if (!sg_table)

• sg_table = ERR_PTR(-ENOMEM);
  

• reservation_object_lock(attach->dmabuf->resv, NULL);
• sg_table = dma_buf_map_attachment_locked(attach, direction);
• reservation_object_unlock(attach->dmabuf->resv); return sg_table; }

@@ -744,8 +745,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, if (attach->sgt == sg_table) return;

• attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,

• 				direction);
  

• reservation_object_lock(attach->dmabuf->resv, NULL);
• dma_buf_unmap_attachment_locked(attach, sg_table, direction);
• reservation_object_unlock(attach->dmabuf->resv); } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);

diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index 642d0e70d0f8..19c47821032f 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -501,6 +501,10 @@ armada_gem_prime_export(struct drm_device *dev, struct drm_gem_object *obj, struct drm_gem_object * armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev->dev,
  

• .dmabuf = buf
  

• }; struct dma_buf_attachment *attach; struct armada_gem_object *dobj;

@@ -516,7 +520,7 @@ armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf) } }

• attach = dma_buf_attach(buf, dev->dev);

• attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) return ERR_CAST(attach);

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 231e3f6d5f41..1fadf5d5ed33 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -709,6 +709,10 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, struct dma_buf *dma_buf, struct device *attach_dev) {

• struct dma_buf_attach_info attach_info = {

• .dev = attach_dev,
  

• .dmabuf = dma_buf
  

• }; struct dma_buf_attachment *attach; struct sg_table *sgt; struct drm_gem_object *obj;

@@ -729,7 +733,7 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, if (!dev->driver->gem_prime_import_sg_table) return ERR_PTR(-EINVAL);

• attach = dma_buf_attach(dma_buf, attach_dev);

• attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) return ERR_CAST(attach);

diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index 82e2ca17a441..aa7f685bd6ca 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -277,6 +277,10 @@ static const struct drm_i915_gem_object_ops i915_gem_object_dmabuf_ops = { struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev->dev,
  

• .dmabuf = dma_buf
  

• }; struct dma_buf_attachment *attach; struct drm_i915_gem_object *obj; int ret;

@@ -295,7 +299,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, } /* need to attach */

• attach = dma_buf_attach(dma_buf, dev->dev);

• attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) return ERR_CAST(attach);

diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index 4f80100ff5f3..8e6b6c879add 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -332,6 +332,10 @@ struct tegra_bo *tegra_bo_create_with_handle(struct drm_file *file, static struct tegra_bo *tegra_bo_import(struct drm_device *drm, struct dma_buf *buf) {

• struct dma_buf_attach_info attach_info = {

• .dev = drm->dev,
  

• .dmabuf = buf
  

• }; struct tegra_drm *tegra = drm->dev_private; struct dma_buf_attachment *attach; struct tegra_bo *bo;

@@ -341,7 +345,7 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm, if (IS_ERR(bo)) return bo;

• attach = dma_buf_attach(buf, drm->dev);

• attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) { err = PTR_ERR(attach); goto free;

diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c index 556f62662aa9..86b928f9742f 100644 --- a/drivers/gpu/drm/udl/udl_dmabuf.c +++ b/drivers/gpu/drm/udl/udl_dmabuf.c @@ -226,6 +226,10 @@ static int udl_prime_create(struct drm_device *dev, struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev->dev,
  

• .dmabuf = dma_buf
  

• }; struct dma_buf_attachment *attach; struct sg_table *sg; struct udl_gem_object *uobj;

@@ -233,7 +237,7 @@ struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev, /* need to attach */ get_device(dev->dev);

• attach = dma_buf_attach(dma_buf, dev->dev);

• attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) { put_device(dev->dev); return ERR_CAST(attach);

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c index aff0ab7bf83d..1f2687b5eb0e 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c @@ -676,6 +676,10 @@ static void vb2_dc_detach_dmabuf(void *mem_priv) static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf, unsigned long size, enum dma_data_direction dma_dir) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev,
  

• .dmabuf = dbuf
  

• }; struct vb2_dc_buf *buf; struct dma_buf_attachment *dba;

@@ -691,7 +695,7 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf, buf->dev = dev; /* create attachment for the dmabuf with the user device */

• dba = dma_buf_attach(dbuf, buf->dev);

• dba = dma_buf_attach(&attach_info); if (IS_ERR(dba)) { pr_err("failed to attach dmabuf\n"); kfree(buf);

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index 015e737095cd..cbd626d2393a 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c @@ -608,6 +608,10 @@ static void vb2_dma_sg_detach_dmabuf(void *mem_priv) static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf, unsigned long size, enum dma_data_direction dma_dir) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev,
  

• .dmabuf = dbuf
  

• }; struct vb2_dma_sg_buf *buf; struct dma_buf_attachment *dba;

@@ -623,7 +627,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf, buf->dev = dev; /* create attachment for the dmabuf with the user device */

• dba = dma_buf_attach(dbuf, buf->dev);

• dba = dma_buf_attach(&attach_info); if (IS_ERR(dba)) { pr_err("failed to attach dmabuf\n"); kfree(buf);

diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c b/drivers/staging/media/tegra-vde/tegra-vde.c index aa6c6bba961e..5a10c1facc27 100644 --- a/drivers/staging/media/tegra-vde/tegra-vde.c +++ b/drivers/staging/media/tegra-vde/tegra-vde.c @@ -568,6 +568,10 @@ static int tegra_vde_attach_dmabuf(struct device *dev, size_t *size, enum dma_data_direction dma_dir) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev,
  

• .dmabuf = dmabuf
  

• }; struct dma_buf_attachment *attachment; struct dma_buf *dmabuf; struct sg_table *sgt;

@@ -591,7 +595,7 @@ static int tegra_vde_attach_dmabuf(struct device *dev, return -EINVAL; }

• attachment = dma_buf_attach(dmabuf, dev);

• attachment = dma_buf_attach(&attach_info); if (IS_ERR(attachment)) { dev_err(dev, "Failed to attach dmabuf\n"); err = PTR_ERR(attachment);

diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 18a78be53541..7e23758db3a4 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -128,6 +128,9 @@ struct dma_buf_ops { * any other kind of sharing that the exporter might wish to make * available to buffer-users. *

• * This is always called with the dmabuf->resv object locked when
  

• * no_sgt_cache is true.
  

• *
  

  • Returns:
  • A &sg_table scatter list of or the backing storage of the DMA buffer,

@@ -148,6 +151,9 @@ struct dma_buf_ops { * It should also unpin the backing storage if this is the last mapping * of the DMA buffer, it the exporter supports backing storage * migration.

• *
  

• * This is always called with the dmabuf->resv object locked when
  

• * no_sgt_cache is true.
  

  */ void (*unmap_dma_buf)(struct dma_buf_attachment *, struct sg_table *,

@@ -370,6 +376,19 @@ struct dma_buf_export_info { struct dma_buf_export_info name = { .exp_name = KBUILD_MODNAME, \ .owner = THIS_MODULE } +/**

• • struct dma_buf_attach_info - holds information needed to attach to a dma_buf
• • @dmabuf: the exported dma_buf
• • @dev: the device which wants to import the attachment
• • This structure holds the information required to attach to a buffer. Used
• • with dma_buf_attach() only.
• */

+struct dma_buf_attach_info {

• struct dma_buf *dmabuf;
• struct device *dev;

+};

• /**
  • get_dma_buf - convenience wrapper for get_file.
  • @dmabuf: [in] pointer to dma_buf

@@ -384,8 +403,8 @@ static inline void get_dma_buf(struct dma_buf *dmabuf) get_file(dmabuf->file); } -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,

• 					struct device *dev);
  

+struct dma_buf_attachment * +dma_buf_attach(const struct dma_buf_attach_info *info); void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *dmabuf_attach); -- 2.17.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Wed, Apr 17, 2019 at 04:01:16PM +0200, Daniel Vetter wrote:

On Tue, Apr 16, 2019 at 08:38:33PM +0200, Christian König wrote:

...

Each importer can now provide an invalidate_mappings callback.

This allows the exporter to provide the mappings without the need to pin the backing store.

v2: don't try to invalidate mappings when the callback is NULL, lock the reservation obj while using the attachments, add helper to set the callback v3: move flag for invalidation support into the DMA-buf, use new attach_info structure to set the callback v4: use importer_priv field instead of mangling exporter priv. v5: drop invalidation_supported flag

Signed-off-by: Christian König christian.koenig@amd.com

drivers/dma-buf/dma-buf.c | 37 +++++++++++++++++++++++++++++++++++++ include/linux/dma-buf.h | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 83c92bfd964c..a3738fab3927 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -563,6 +563,8 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info attach->dev = info->dev; attach->dmabuf = dmabuf;

• attach->importer_priv = info->importer_priv;
• attach->invalidate = info->invalidate;

mutex_lock(&dmabuf->lock); @@ -571,7 +573,9 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info if (ret) goto err_attach; }

• reservation_object_lock(dmabuf->resv, NULL); list_add(&attach->node, &dmabuf->attachments);
• reservation_object_unlock(dmabuf->resv);

mutex_unlock(&dmabuf->lock); @@ -615,7 +619,9 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) DMA_BIDIRECTIONAL); mutex_lock(&dmabuf->lock);

• reservation_object_lock(dmabuf->resv, NULL); list_del(&attach->node);
• reservation_object_unlock(dmabuf->resv); if (dmabuf->ops->detach) dmabuf->ops->detach(dmabuf, attach);

@@ -653,7 +659,16 @@ dma_buf_map_attachment_locked(struct dma_buf_attachment *attach, if (attach->sgt) return attach->sgt;

• /*

• * Mapping a DMA-buf can trigger its invalidation, prevent sending this
  

• * event to the caller by temporary removing this attachment from the
  

• * list.
  

• */
  

• if (attach->invalidate)

• list_del(&attach->node);
  

  sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
• if (attach->invalidate)

• list_add(&attach->node, &attach->dmabuf->attachments);
  

  if (!sg_table) sg_table = ERR_PTR(-ENOMEM);

@@ -751,6 +766,26 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); +/**

• • dma_buf_invalidate_mappings - invalidate all mappings of this dma_buf
• • @dmabuf: [in] buffer which mappings should be invalidated
• • Informs all attachmenst that they need to destroy and recreated all their
• • mappings.
• */

+void dma_buf_invalidate_mappings(struct dma_buf *dmabuf) +{

• struct dma_buf_attachment *attach;
• reservation_object_assert_held(dmabuf->resv);
• list_for_each_entry(attach, &dmabuf->attachments, node)

• if (attach->invalidate)
  

• 	attach->invalidate(attach);
  

+} +EXPORT_SYMBOL_GPL(dma_buf_invalidate_mappings);

/**

• DOC: cpu access

@@ -1163,10 +1198,12 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) seq_puts(s, "\tAttached Devices:\n"); attach_count = 0;

• reservation_object_lock(buf_obj->resv, NULL);
  

  list_for_each_entry(attach_obj, &buf_obj->attachments, node) { seq_printf(s, "\t%s\n", dev_name(attach_obj->dev)); attach_count++; }

• reservation_object_unlock(buf_obj->resv);
  

seq_printf(s, "Total %d devices attached\n\n", attach_count); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 7e23758db3a4..ece4638359a8 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -324,6 +324,7 @@ struct dma_buf {

• @dev: device attached to the buffer.
• @node: list of dma_buf_attachment.
• @priv: exporter specific attachment data.

• • @importer_priv: importer specific attachment data.
  • This structure holds the attachment information between the dma_buf buffer
  • and its user device(s). The list contains one attachment struct per device

@@ -340,6 +341,29 @@ struct dma_buf_attachment { struct list_head node; void *priv; struct sg_table *sgt;

• void *importer_priv;
• /**

• * @invalidate:
  

• *
  

• * Optional callback provided by the importer of the dma-buf.
  

• *
  

• * If provided the exporter can avoid pinning the backing store while
  

• * mappings exists.
  

• *
  

• * The function is called with the lock of the reservation object
  

• * associated with the dma_buf held and the mapping function must be
  

• * called with this lock held as well. This makes sure that no mapping
  

• * is created concurrently with an ongoing invalidation.
  

• *
  

• * After the callback all existing mappings are still valid until all
  

• * fences in the dma_bufs reservation object are signaled, but should be
  

• * destroyed by the importer as soon as possible.
  

• *
  

• * New mappings can be created immediately, but can't be used before the
  

• * exclusive fence in the dma_bufs reservation object is signaled.
  

• */
  

• void (*invalidate)(struct dma_buf_attachment *attach);

I would put the long kerneldoc into dma_buf_attach_info (as an inline comment, you can mix the styles). And reference it from here with something like

"Set from &dma_buf_attach_info.invalidate in dma_buf_attach(), see there for more information."

This here feels a bit too much hidden.

Question on semantics: Is invalidate allowed to add new fences? I think we need that for pipelined buffer moves and stuff perhaps (or pipeline pagetable invalidates or whatever you feel like pipelining). And it should be possible (we already hold the reservation lock), and I think ttm copes (but no idea really).

Either way, docs need to be clear on this. -Daniel

Am 17.04.19 um 16:26 schrieb Daniel Vetter:

On Wed, Apr 17, 2019 at 04:14:32PM +0200, Christian König wrote:

...

Am 17.04.19 um 16:08 schrieb Daniel Vetter:

...

On Tue, Apr 16, 2019 at 08:38:32PM +0200, Christian König wrote:

...

Make it mandatory for dynamic dma-buf callbacks to be called with the reservation lock held.

For static dma-buf exporters we still have the fallback of using cached sgt.

v2: reordered v3: rebased on sgt caching v4: use the cached sgt when possible

Signed-off-by: Christian König christian.koenig@amd.com

I think there's a bit a rebase chaos going on:

• some comments left behind with no_sgt_cache, which I can't find anymore
• the function signature rework of dma_buf_attach should imo be split out

Ah, crap thought I've got all of those. Going to fix that.

...

Next issue is that the reservation object locking is still in the path of dma_buf_map, so probably still going to result in tons of lockdep splats. Except the i915+amdgpu path should now work due to the fastpath.

I actually found a solution for that :)

The idea is now that we always cache the sgt in the attachment unless the dynamic flag (previously no_sgt_cache flag) is set. And this cached sgt is created without holding the lock.

Yeah I think that idea works.

...

We either need to document that really well or maybe split the mapping callbacks into map/unmap and map_lock/unmap_locked. Opinions?

I think the implementation doesn't. Exporter can't decide on its own whether dynamic/static is what's needed, we need to decide that for each attachment, taking both exporter and importer capabilities into account.

Well that's what the pin/unpin callbacks are good for :)

Essentially we have to handle the following cases: a) dynamic exporter and dynamic importer         Nothing special here and we don't need the sgt caching nor pinning.

b) dynamic exporter and static importer         The pin/unpin callbacks are used to inform the exporter that it needs to keep the buffer in the current place.

c) static exporter and dynamic importer         We use the sgt caching to avoid calling the exporter with the common lock held.

d) static exporter and static importer         We use the sgt caching, but that is actually only optional.

I think if we do that, then it should work out. I replied on the pin/unpin interface patch with some more concrete thoughts. Maybe best to continue that discussion there, with more context.

Yeah, that is probably a good idea.

Regards, Christian.

-Daniel

...

...

Not sure that's a solution that really works, just hides that fundamentally we still have that issue of incompatible locking chains between different drivers.

We can now make a slow transition between static and dynamic DMA-buf handling, so only driver who can do the locking will be affected.

Christian.

...

-Daniel

...

---

drivers/dma-buf/dma-buf.c | 24 ++++++++++--------- drivers/gpu/drm/armada/armada_gem.c | 6 ++++- drivers/gpu/drm/drm_prime.c | 6 ++++- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 6 ++++- drivers/gpu/drm/tegra/gem.c | 6 ++++- drivers/gpu/drm/udl/udl_dmabuf.c | 6 ++++- .../common/videobuf2/videobuf2-dma-contig.c | 6 ++++- .../media/common/videobuf2/videobuf2-dma-sg.c | 6 ++++- drivers/staging/media/tegra-vde/tegra-vde.c | 6 ++++- include/linux/dma-buf.h | 23 ++++++++++++++++-- 10 files changed, 74 insertions(+), 21 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index ef480e5fb239..83c92bfd964c 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -532,8 +532,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put); /** * dma_buf_attach - Add the device to dma_buf's attachments list; optionally, * calls attach() of dma_buf_ops to allow device-specific attach functionality

• • @dmabuf: [in] buffer to attach device to.
• • @dev: [in] device to be attached.

• • @info: [in] holds all the attach related information provided

• • 	by the importer. see &struct dma_buf_attach_info
    

• • 	for further details.
    

    • Returns struct dma_buf_attachment pointer for this attachment. Attachments
    • must be cleaned up by calling dma_buf_detach().

@@ -547,20 +548,20 @@ EXPORT_SYMBOL_GPL(dma_buf_put); * accessible to @dev, and cannot be moved to a more suitable place. This is * indicated with the error code -EBUSY. */ -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,

• 			  struct device *dev)
  

+struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info) {

• struct dma_buf *dmabuf = info->dmabuf; struct dma_buf_attachment *attach; int ret;

• if (WARN_ON(!dmabuf || !dev))

• if (WARN_ON(!dmabuf || !info->dev)) return ERR_PTR(-EINVAL); attach = kzalloc(sizeof(*attach), GFP_KERNEL); if (!attach) return ERR_PTR(-ENOMEM);

• attach->dev = dev;

• attach->dev = info->dev; attach->dmabuf = dmabuf; mutex_lock(&dmabuf->lock);

@@ -688,9 +689,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, if (attach->sgt) return attach->sgt;

• sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
• if (!sg_table)

• sg_table = ERR_PTR(-ENOMEM);
  

• reservation_object_lock(attach->dmabuf->resv, NULL);
• sg_table = dma_buf_map_attachment_locked(attach, direction);
• reservation_object_unlock(attach->dmabuf->resv); return sg_table; }

@@ -744,8 +745,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, if (attach->sgt == sg_table) return;

• attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,

• 				direction);
  

• reservation_object_lock(attach->dmabuf->resv, NULL);
• dma_buf_unmap_attachment_locked(attach, sg_table, direction);
• reservation_object_unlock(attach->dmabuf->resv); } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);

diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index 642d0e70d0f8..19c47821032f 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -501,6 +501,10 @@ armada_gem_prime_export(struct drm_device *dev, struct drm_gem_object *obj, struct drm_gem_object * armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev->dev,
  

• .dmabuf = buf
  

• }; struct dma_buf_attachment *attach; struct armada_gem_object *dobj;

@@ -516,7 +520,7 @@ armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf) } }

• attach = dma_buf_attach(buf, dev->dev);

• attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) return ERR_CAST(attach);

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 231e3f6d5f41..1fadf5d5ed33 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -709,6 +709,10 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, struct dma_buf *dma_buf, struct device *attach_dev) {

• struct dma_buf_attach_info attach_info = {

• .dev = attach_dev,
  

• .dmabuf = dma_buf
  

• }; struct dma_buf_attachment *attach; struct sg_table *sgt; struct drm_gem_object *obj;

@@ -729,7 +733,7 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, if (!dev->driver->gem_prime_import_sg_table) return ERR_PTR(-EINVAL);

• attach = dma_buf_attach(dma_buf, attach_dev);

• attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) return ERR_CAST(attach);

diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index 82e2ca17a441..aa7f685bd6ca 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -277,6 +277,10 @@ static const struct drm_i915_gem_object_ops i915_gem_object_dmabuf_ops = { struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev->dev,
  

• .dmabuf = dma_buf
  

• }; struct dma_buf_attachment *attach; struct drm_i915_gem_object *obj; int ret;

@@ -295,7 +299,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, } /* need to attach */

• attach = dma_buf_attach(dma_buf, dev->dev);

• attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) return ERR_CAST(attach);

diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index 4f80100ff5f3..8e6b6c879add 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -332,6 +332,10 @@ struct tegra_bo *tegra_bo_create_with_handle(struct drm_file *file, static struct tegra_bo *tegra_bo_import(struct drm_device *drm, struct dma_buf *buf) {

• struct dma_buf_attach_info attach_info = {

• .dev = drm->dev,
  

• .dmabuf = buf
  

• }; struct tegra_drm *tegra = drm->dev_private; struct dma_buf_attachment *attach; struct tegra_bo *bo;

@@ -341,7 +345,7 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm, if (IS_ERR(bo)) return bo;

• attach = dma_buf_attach(buf, drm->dev);

• attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) { err = PTR_ERR(attach); goto free;

diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c index 556f62662aa9..86b928f9742f 100644 --- a/drivers/gpu/drm/udl/udl_dmabuf.c +++ b/drivers/gpu/drm/udl/udl_dmabuf.c @@ -226,6 +226,10 @@ static int udl_prime_create(struct drm_device *dev, struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev->dev,
  

• .dmabuf = dma_buf
  

• }; struct dma_buf_attachment *attach; struct sg_table *sg; struct udl_gem_object *uobj;

@@ -233,7 +237,7 @@ struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev, /* need to attach */ get_device(dev->dev);

• attach = dma_buf_attach(dma_buf, dev->dev);

• attach = dma_buf_attach(&attach_info); if (IS_ERR(attach)) { put_device(dev->dev); return ERR_CAST(attach);

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c index aff0ab7bf83d..1f2687b5eb0e 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c @@ -676,6 +676,10 @@ static void vb2_dc_detach_dmabuf(void *mem_priv) static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf, unsigned long size, enum dma_data_direction dma_dir) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev,
  

• .dmabuf = dbuf
  

• }; struct vb2_dc_buf *buf; struct dma_buf_attachment *dba;

@@ -691,7 +695,7 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf, buf->dev = dev; /* create attachment for the dmabuf with the user device */

• dba = dma_buf_attach(dbuf, buf->dev);

• dba = dma_buf_attach(&attach_info); if (IS_ERR(dba)) { pr_err("failed to attach dmabuf\n"); kfree(buf);

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index 015e737095cd..cbd626d2393a 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c @@ -608,6 +608,10 @@ static void vb2_dma_sg_detach_dmabuf(void *mem_priv) static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf, unsigned long size, enum dma_data_direction dma_dir) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev,
  

• .dmabuf = dbuf
  

• }; struct vb2_dma_sg_buf *buf; struct dma_buf_attachment *dba;

@@ -623,7 +627,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf, buf->dev = dev; /* create attachment for the dmabuf with the user device */

• dba = dma_buf_attach(dbuf, buf->dev);

• dba = dma_buf_attach(&attach_info); if (IS_ERR(dba)) { pr_err("failed to attach dmabuf\n"); kfree(buf);

diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c b/drivers/staging/media/tegra-vde/tegra-vde.c index aa6c6bba961e..5a10c1facc27 100644 --- a/drivers/staging/media/tegra-vde/tegra-vde.c +++ b/drivers/staging/media/tegra-vde/tegra-vde.c @@ -568,6 +568,10 @@ static int tegra_vde_attach_dmabuf(struct device *dev, size_t *size, enum dma_data_direction dma_dir) {

• struct dma_buf_attach_info attach_info = {

• .dev = dev,
  

• .dmabuf = dmabuf
  

• }; struct dma_buf_attachment *attachment; struct dma_buf *dmabuf; struct sg_table *sgt;

@@ -591,7 +595,7 @@ static int tegra_vde_attach_dmabuf(struct device *dev, return -EINVAL; }

• attachment = dma_buf_attach(dmabuf, dev);

• attachment = dma_buf_attach(&attach_info); if (IS_ERR(attachment)) { dev_err(dev, "Failed to attach dmabuf\n"); err = PTR_ERR(attachment);

diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 18a78be53541..7e23758db3a4 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -128,6 +128,9 @@ struct dma_buf_ops { * any other kind of sharing that the exporter might wish to make * available to buffer-users. *

• * This is always called with the dmabuf->resv object locked when
  

• * no_sgt_cache is true.
  

• *
  

  • Returns:
  • A &sg_table scatter list of or the backing storage of the DMA buffer,

@@ -148,6 +151,9 @@ struct dma_buf_ops { * It should also unpin the backing storage if this is the last mapping * of the DMA buffer, it the exporter supports backing storage * migration.

• *
  

• * This is always called with the dmabuf->resv object locked when
  

• * no_sgt_cache is true.
  

  */ void (*unmap_dma_buf)(struct dma_buf_attachment *, struct sg_table *,

@@ -370,6 +376,19 @@ struct dma_buf_export_info { struct dma_buf_export_info name = { .exp_name = KBUILD_MODNAME, \ .owner = THIS_MODULE } +/**

• • struct dma_buf_attach_info - holds information needed to attach to a dma_buf
• • @dmabuf: the exported dma_buf
• • @dev: the device which wants to import the attachment
• • This structure holds the information required to attach to a buffer. Used
• • with dma_buf_attach() only.
• */

+struct dma_buf_attach_info {

• struct dma_buf *dmabuf;
• struct device *dev;

+};

• /**
  • get_dma_buf - convenience wrapper for get_file.
  • @dmabuf: [in] pointer to dma_buf

@@ -384,8 +403,8 @@ static inline void get_dma_buf(struct dma_buf *dmabuf) get_file(dmabuf->file); } -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,

• 					struct device *dev);
  

+struct dma_buf_attachment * +dma_buf_attach(const struct dma_buf_attach_info *info); void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *dmabuf_attach); -- 2.17.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, Apr 16, 2019 at 08:38:33PM +0200, Christian König wrote:

Each importer can now provide an invalidate_mappings callback.

This allows the exporter to provide the mappings without the need to pin the backing store.

v2: don't try to invalidate mappings when the callback is NULL, lock the reservation obj while using the attachments, add helper to set the callback v3: move flag for invalidation support into the DMA-buf, use new attach_info structure to set the callback v4: use importer_priv field instead of mangling exporter priv. v5: drop invalidation_supported flag

Signed-off-by: Christian König christian.koenig@amd.com

drivers/dma-buf/dma-buf.c | 37 +++++++++++++++++++++++++++++++++++++ include/linux/dma-buf.h | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 83c92bfd964c..a3738fab3927 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -563,6 +563,8 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info attach->dev = info->dev; attach->dmabuf = dmabuf;

• attach->importer_priv = info->importer_priv;
• attach->invalidate = info->invalidate;

mutex_lock(&dmabuf->lock); @@ -571,7 +573,9 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info if (ret) goto err_attach; }

• reservation_object_lock(dmabuf->resv, NULL); list_add(&attach->node, &dmabuf->attachments);
• reservation_object_unlock(dmabuf->resv);

mutex_unlock(&dmabuf->lock); @@ -615,7 +619,9 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) DMA_BIDIRECTIONAL); mutex_lock(&dmabuf->lock);

• reservation_object_lock(dmabuf->resv, NULL); list_del(&attach->node);
• reservation_object_unlock(dmabuf->resv); if (dmabuf->ops->detach) dmabuf->ops->detach(dmabuf, attach);

@@ -653,7 +659,16 @@ dma_buf_map_attachment_locked(struct dma_buf_attachment *attach, if (attach->sgt) return attach->sgt;

• /*

• * Mapping a DMA-buf can trigger its invalidation, prevent sending this
  

• * event to the caller by temporary removing this attachment from the
  

• * list.
  

• */
  

• if (attach->invalidate)

• list_del(&attach->node);
  

Just noticed this: Why do we need this? invalidate needs the reservation lock, as does map_attachment. It should be impssoble to have someone else sneak in here. -Daniel

sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);

• if (attach->invalidate)

• list_add(&attach->node, &attach->dmabuf->attachments);
  

  if (!sg_table) sg_table = ERR_PTR(-ENOMEM);

@@ -751,6 +766,26 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); +/**

• • dma_buf_invalidate_mappings - invalidate all mappings of this dma_buf
• • @dmabuf: [in] buffer which mappings should be invalidated
• • Informs all attachmenst that they need to destroy and recreated all their
• • mappings.
• */

+void dma_buf_invalidate_mappings(struct dma_buf *dmabuf) +{

• struct dma_buf_attachment *attach;
• reservation_object_assert_held(dmabuf->resv);
• list_for_each_entry(attach, &dmabuf->attachments, node)

• if (attach->invalidate)
  

• 	attach->invalidate(attach);
  

+} +EXPORT_SYMBOL_GPL(dma_buf_invalidate_mappings);

/**

• DOC: cpu access

@@ -1163,10 +1198,12 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) seq_puts(s, "\tAttached Devices:\n"); attach_count = 0;

• reservation_object_lock(buf_obj->resv, NULL);
  

  list_for_each_entry(attach_obj, &buf_obj->attachments, node) { seq_printf(s, "\t%s\n", dev_name(attach_obj->dev)); attach_count++; }

• reservation_object_unlock(buf_obj->resv);
  

seq_printf(s, "Total %d devices attached\n\n", attach_count); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 7e23758db3a4..ece4638359a8 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -324,6 +324,7 @@ struct dma_buf {

• @dev: device attached to the buffer.
• @node: list of dma_buf_attachment.
• @priv: exporter specific attachment data.

• • @importer_priv: importer specific attachment data.
  • This structure holds the attachment information between the dma_buf buffer
  • and its user device(s). The list contains one attachment struct per device

@@ -340,6 +341,29 @@ struct dma_buf_attachment { struct list_head node; void *priv; struct sg_table *sgt;

• void *importer_priv;
• /**

• * @invalidate:
  

• *
  

• * Optional callback provided by the importer of the dma-buf.
  

• *
  

• * If provided the exporter can avoid pinning the backing store while
  

• * mappings exists.
  

• *
  

• * The function is called with the lock of the reservation object
  

• * associated with the dma_buf held and the mapping function must be
  

• * called with this lock held as well. This makes sure that no mapping
  

• * is created concurrently with an ongoing invalidation.
  

• *
  

• * After the callback all existing mappings are still valid until all
  

• * fences in the dma_bufs reservation object are signaled, but should be
  

• * destroyed by the importer as soon as possible.
  

• *
  

• * New mappings can be created immediately, but can't be used before the
  

• * exclusive fence in the dma_bufs reservation object is signaled.
  

• */
  

• void (*invalidate)(struct dma_buf_attachment *attach);

}; /** @@ -378,8 +402,10 @@ struct dma_buf_export_info { /**

• struct dma_buf_attach_info - holds information needed to attach to a dma_buf

• • @dmabuf: the exported dma_buf
• • @dev: the device which wants to import the attachment

• • @dmabuf: the exported dma_buf
• • @dev: the device which wants to import the attachment
• • @importer_priv: private data of importer to this attachment
• • @invalidate: callback to use for invalidating mappings
  • This structure holds the information required to attach to a buffer. Used
  • with dma_buf_attach() only.

@@ -387,6 +413,8 @@ struct dma_buf_export_info { struct dma_buf_attach_info { struct dma_buf *dmabuf; struct device *dev;

• void *importer_priv;
• void (*invalidate)(struct dma_buf_attachment *attach);

}; /** @@ -423,6 +451,7 @@ void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *, enum dma_data_direction); void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *, enum dma_data_direction); +void dma_buf_invalidate_mappings(struct dma_buf *dma_buf); 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, -- 2.17.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Wed, Apr 17, 2019 at 09:13:22PM +0200, Christian König wrote:

Am 17.04.19 um 21:07 schrieb Daniel Vetter:

...

On Tue, Apr 16, 2019 at 08:38:33PM +0200, Christian König wrote:

...

Each importer can now provide an invalidate_mappings callback.

This allows the exporter to provide the mappings without the need to pin the backing store.

v2: don't try to invalidate mappings when the callback is NULL, lock the reservation obj while using the attachments, add helper to set the callback v3: move flag for invalidation support into the DMA-buf, use new attach_info structure to set the callback v4: use importer_priv field instead of mangling exporter priv. v5: drop invalidation_supported flag

Signed-off-by: Christian König christian.koenig@amd.com

drivers/dma-buf/dma-buf.c | 37 +++++++++++++++++++++++++++++++++++++ include/linux/dma-buf.h | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 83c92bfd964c..a3738fab3927 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -563,6 +563,8 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info attach->dev = info->dev; attach->dmabuf = dmabuf;

• attach->importer_priv = info->importer_priv;
• attach->invalidate = info->invalidate; mutex_lock(&dmabuf->lock);

@@ -571,7 +573,9 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info if (ret) goto err_attach; }

• reservation_object_lock(dmabuf->resv, NULL); list_add(&attach->node, &dmabuf->attachments);
• reservation_object_unlock(dmabuf->resv); mutex_unlock(&dmabuf->lock);

@@ -615,7 +619,9 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) DMA_BIDIRECTIONAL); mutex_lock(&dmabuf->lock);

• reservation_object_lock(dmabuf->resv, NULL); list_del(&attach->node);
• reservation_object_unlock(dmabuf->resv); if (dmabuf->ops->detach) dmabuf->ops->detach(dmabuf, attach);

@@ -653,7 +659,16 @@ dma_buf_map_attachment_locked(struct dma_buf_attachment *attach, if (attach->sgt) return attach->sgt;

• /*

• * Mapping a DMA-buf can trigger its invalidation, prevent sending this
  

• * event to the caller by temporary removing this attachment from the
  

• * list.
  

• */
  

• if (attach->invalidate)

• list_del(&attach->node);
  

Just noticed this: Why do we need this? invalidate needs the reservation lock, as does map_attachment. It should be impssoble to have someone else sneak in here.

I was having problems with self triggered invalidations.

E.g. client A tries to map an attachment, that in turn causes the buffer to move to a new place and client A is informed about that movement with an invalidation.

Uh, that sounds like a bug in ttm or somewhere else in the exporter. If you evict the bo that you're trying to map, that's bad.

Or maybe it's a framework bug, and we need to track whether an attachment has a map or not. That would make more sense ... -Daniel

On Thu, Apr 18, 2019 at 08:28:51AM +0000, Koenig, Christian wrote:

Am 18.04.19 um 10:08 schrieb Daniel Vetter:

...

On Wed, Apr 17, 2019 at 09:13:22PM +0200, Christian König wrote:

...

Am 17.04.19 um 21:07 schrieb Daniel Vetter:

...

On Tue, Apr 16, 2019 at 08:38:33PM +0200, Christian König wrote:

...

Each importer can now provide an invalidate_mappings callback.

This allows the exporter to provide the mappings without the need to pin the backing store.

v2: don't try to invalidate mappings when the callback is NULL, lock the reservation obj while using the attachments, add helper to set the callback v3: move flag for invalidation support into the DMA-buf, use new attach_info structure to set the callback v4: use importer_priv field instead of mangling exporter priv. v5: drop invalidation_supported flag

Signed-off-by: Christian König christian.koenig@amd.com

drivers/dma-buf/dma-buf.c | 37 +++++++++++++++++++++++++++++++++++++ include/linux/dma-buf.h | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 83c92bfd964c..a3738fab3927 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -563,6 +563,8 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info attach->dev = info->dev; attach->dmabuf = dmabuf;

• attach->importer_priv = info->importer_priv;
• attach->invalidate = info->invalidate; mutex_lock(&dmabuf->lock);

@@ -571,7 +573,9 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info if (ret) goto err_attach; }

• reservation_object_lock(dmabuf->resv, NULL); list_add(&attach->node, &dmabuf->attachments);
• reservation_object_unlock(dmabuf->resv); mutex_unlock(&dmabuf->lock);

@@ -615,7 +619,9 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) DMA_BIDIRECTIONAL); mutex_lock(&dmabuf->lock);

• reservation_object_lock(dmabuf->resv, NULL); list_del(&attach->node);
• reservation_object_unlock(dmabuf->resv); if (dmabuf->ops->detach) dmabuf->ops->detach(dmabuf, attach);

@@ -653,7 +659,16 @@ dma_buf_map_attachment_locked(struct dma_buf_attachment *attach, if (attach->sgt) return attach->sgt;

• /*

• * Mapping a DMA-buf can trigger its invalidation, prevent sending this
  

• * event to the caller by temporary removing this attachment from the
  

• * list.
  

• */
  

• if (attach->invalidate)

• list_del(&attach->node);
  

Just noticed this: Why do we need this? invalidate needs the reservation lock, as does map_attachment. It should be impssoble to have someone else sneak in here.

I was having problems with self triggered invalidations.

E.g. client A tries to map an attachment, that in turn causes the buffer to move to a new place and client A is informed about that movement with an invalidation.

Uh, that sounds like a bug in ttm or somewhere else in the exporter. If you evict the bo that you're trying to map, that's bad.

Or maybe it's a framework bug, and we need to track whether an attachment has a map or not. That would make more sense ...

Well neither, as far as I can see this is perfectly normal behavior.

We just don't want any invalidation send to a driver which is currently making a mapping.

If you want I can do this in the driver as well, but at least of hand it looks like a good idea to have that in common code.

Hm. This sounds like we'd want to invalidate a specific mapping.

Tracking the mappings could work as well, but the problem here is that I actually want the lifetime of old and new mappings to overlap for pipelining.

Aside: Overlapping mappings being explicitly allowed should be in the docs. The current kerneldoc for invalidate leaves that up for interpretation. This answers one of the questions I had overnight, about whether we expect ->invalidate to tear down the mapping or not.

Imo a better semantics would be that ->invalidate must tear down the mapping, but the exporter must delay actual unmap until all fences have cleared. Otherwise you could end up with fun stuff where the exporter releases the memory (it wanted to invalidate after all), while the importer still has a mapping around. That's not going to end well I think.

That would also solve the issue of getting an invalidate while you map, at least if we filter per attachment. -Daniel

On Tue, 16 Apr 2019, Christian König wrote:

To allow a smooth transition from pinning buffer objects to dynamic invalidation we first start to cache the sg_table for an attachment unless the driver explicitly says to not do so.

---

drivers/dma-buf/dma-buf.c | 24 ++++++++++++++++++++++++ include/linux/dma-buf.h | 11 +++++++++++ 2 files changed, 35 insertions(+)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 7c858020d14b..65161a82d4d5 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -573,6 +573,20 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, list_add(&attach->node, &dmabuf->attachments); mutex_unlock(&dmabuf->lock);

• if (!dmabuf->ops->dynamic_sgt_mapping) {

• struct sg_table *sgt;
  

• sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL);
  

• if (!sgt)
  

• 	sgt = ERR_PTR(-ENOMEM);
  

• if (IS_ERR(sgt)) {
  

• 	dma_buf_detach(dmabuf, attach);
  

• 	return ERR_CAST(sgt);
  

• }
  

• attach->sgt = sgt;
  

• }
• return attach;

err_attach: @@ -595,6 +609,10 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) if (WARN_ON(!dmabuf || !attach)) return;

• if (attach->sgt)

• dmabuf->ops->unmap_dma_buf(attach, attach->sgt,
  

• 			   DMA_BIDIRECTIONAL);
  

• mutex_lock(&dmabuf->lock); list_del(&attach->node); if (dmabuf->ops->detach)

@@ -630,6 +648,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, if (WARN_ON(!attach || !attach->dmabuf)) return ERR_PTR(-EINVAL);

• if (attach->sgt)

• return attach->sgt;
  

I am concerned by this change to make caching the sg_table the default behavior as this will result in the exporter's map_dma_buf/unmap_dma_buf calls are no longer being called in dma_buf_map_attachment/dma_buf_unmap_attachment. This seems concerning to me as it appears to ignore the cache maintenance aspect of the map_dma_buf/unmap_dma_buf calls. For example won't this potentially cause issues for clients of ION.

If we had the following - #1 dma_buf_attach coherent_device - #2 dma_buf attach non_coherent_device - #3 dma_buf_map_attachment non_coherent_device - #4 non_coherent_device writes to buffer - #5 dma_buf_unmap_attachment non_coherent_device - #6 dma_buf_map_attachment coherent_device - #7 coherent_device reads buffer - #8 dma_buf_unmap_attachment coherent_device

There wouldn't be any CMO at step #5 anymore (specifically no invalidate) so now at step #7 the coherent_device could read a stale cache line.

Also, now by default dma_buf_unmap_attachment no longer removes the mappings from the iommu, so now by default dma_buf_unmap_attachment is not doing what I would expect and clients are losing the potential sandboxing benefits of removing the mappings. Shouldn't this caching behavior be something that clients opt into instead of being the default?

Liam

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Am 22.05.19 um 18:17 schrieb Sumit Semwal:

Hi Christian,

On Sat, 27 Apr 2019 at 05:31, Liam Mark lmark@codeaurora.org wrote:

...

On Tue, 16 Apr 2019, Christian König wrote:

...

To allow a smooth transition from pinning buffer objects to dynamic invalidation we first start to cache the sg_table for an attachment unless the driver explicitly says to not do so.

---

drivers/dma-buf/dma-buf.c | 24 ++++++++++++++++++++++++ include/linux/dma-buf.h | 11 +++++++++++ 2 files changed, 35 insertions(+)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 7c858020d14b..65161a82d4d5 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -573,6 +573,20 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, list_add(&attach->node, &dmabuf->attachments);

   mutex_unlock(&dmabuf->lock);

• if (!dmabuf->ops->dynamic_sgt_mapping) {
  

•         struct sg_table *sgt;
  

•         sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL);
  

•         if (!sgt)
  

•                 sgt = ERR_PTR(-ENOMEM);
  

•         if (IS_ERR(sgt)) {
  

•                 dma_buf_detach(dmabuf, attach);
  

•                 return ERR_CAST(sgt);
  

•         }
  

•         attach->sgt = sgt;
  

• }
  

•  return attach;
  
  

  err_attach:

@@ -595,6 +609,10 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) if (WARN_ON(!dmabuf || !attach)) return;

• if (attach->sgt)
  

•         dmabuf->ops->unmap_dma_buf(attach, attach->sgt,
  

•                                    DMA_BIDIRECTIONAL);
  

•  mutex_lock(&dmabuf->lock);
   list_del(&attach->node);
   if (dmabuf->ops->detach)
  

@@ -630,6 +648,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, if (WARN_ON(!attach || !attach->dmabuf)) return ERR_PTR(-EINVAL);

• if (attach->sgt)
  

•         return attach->sgt;
  

I am concerned by this change to make caching the sg_table the default behavior as this will result in the exporter's map_dma_buf/unmap_dma_buf calls are no longer being called in dma_buf_map_attachment/dma_buf_unmap_attachment.

Probably this concern from Liam got lost between versions of your patches; could we please request a reply to these points here?

Sorry I indeed never got this mail, but this is actually not an issue because Daniel had similar concerns and we didn't made this the default in the final version.

...

This seems concerning to me as it appears to ignore the cache maintenance aspect of the map_dma_buf/unmap_dma_buf calls. For example won't this potentially cause issues for clients of ION.

If we had the following

• #1 dma_buf_attach coherent_device
• #2 dma_buf attach non_coherent_device
• #3 dma_buf_map_attachment non_coherent_device
• #4 non_coherent_device writes to buffer
• #5 dma_buf_unmap_attachment non_coherent_device
• #6 dma_buf_map_attachment coherent_device
• #7 coherent_device reads buffer
• #8 dma_buf_unmap_attachment coherent_device

There wouldn't be any CMO at step #5 anymore (specifically no invalidate) so now at step #7 the coherent_device could read a stale cache line.

Also, now by default dma_buf_unmap_attachment no longer removes the mappings from the iommu, so now by default dma_buf_unmap_attachment is not doing what I would expect and clients are losing the potential sandboxing benefits of removing the mappings. Shouldn't this caching behavior be something that clients opt into instead of being the default?

Well, it seems you are making incorrect assumptions about the cache maintenance of DMA-buf here.

At least for all DRM devices I'm aware of mapping/unmapping an attachment does *NOT* have any cache maintenance implications.

E.g. the use case you describe above would certainly fail with amdgpu, radeon, nouveau and i915 because mapping a DMA-buf doesn't stop the exporter from reading/writing to that buffer (just the opposite actually).

All of them assume perfectly coherent access to the underlying memory. As far as I know there is no documented cache maintenance requirements for DMA-buf.

The IOMMU concern on the other hand is certainly valid and I perfectly agree that keeping the mapping time as short as possible is desirable.

Regards, Christian.

...

Liam

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Best, Sumit.