On Fri, 16 Jul 2021 at 15:14, Jason Ekstrand <jason@xxxxxxxxxxxxxx> wrote:
>
> From: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
>
> If our exported dma-bufs are imported by another instance of our driver,
> that instance will typically have the imported dma-bufs locked during
> dma_buf_map_attachment(). But the exporter also locks the same reservation
> object in the map_dma_buf() callback, which leads to recursive locking.
>
> So taking the lock inside _pin_pages_unlocked() is incorrect.
>
> Additionally, the current pinning code path is contrary to the defined
> way that pinning should occur.
>
> Remove the explicit pin/unpin from the map/umap functions and move them
> to the attach/detach allowing correct locking to occur, and to match
> the static dma-buf drm_prime pattern.
>
> Add a live selftest to exercise both dynamic and non-dynamic
> exports.
>
> v2:
> - Extend the selftest with a fake dynamic importer.
> - Provide real pin and unpin callbacks to not abuse the interface.
> v3: (ruhl)
> - Remove the dynamic export support and move the pinning into the
> attach/detach path.
> v4: (ruhl)
> - Put pages does not need to assert on the dma-resv
> v5: (jason)
> - Lock around dma_buf_unmap_attachment() when emulating a dynamic
> importer in the subtests.
> - Use pin_pages_unlocked
> v6: (jason)
> - Use dma_buf_attach instead of dma_buf_attach_dynamic in the selftests
>
> Reported-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx>
> Signed-off-by: Jason Ekstrand <jason@xxxxxxxxxxxxxx>
> Reviewed-by: Jason Ekstrand <jason@xxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 43 ++++++--
> .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 103 +++++++++++++++++-
> 2 files changed, 132 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> index 616c3a2f1baf0..9a655f69a0671 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> @@ -12,6 +12,8 @@
> #include "i915_gem_object.h"
> #include "i915_scatterlist.h"
>
> +I915_SELFTEST_DECLARE(static bool force_different_devices;)
> +
> static struct drm_i915_gem_object *dma_buf_to_obj(struct dma_buf *buf)
> {
> return to_intel_bo(buf->priv);
> @@ -25,15 +27,11 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme
> struct scatterlist *src, *dst;
> int ret, i;
>
> - ret = i915_gem_object_pin_pages_unlocked(obj);
> - if (ret)
> - goto err;
> -
> /* Copy sg so that we make an independent mapping */
> st = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
> if (st == NULL) {
> ret = -ENOMEM;
> - goto err_unpin_pages;
> + goto err;
> }
>
> ret = sg_alloc_table(st, obj->mm.pages->nents, GFP_KERNEL);
> @@ -58,8 +56,6 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme
> sg_free_table(st);
> err_free:
> kfree(st);
> -err_unpin_pages:
> - i915_gem_object_unpin_pages(obj);
> err:
> return ERR_PTR(ret);
> }
> @@ -68,13 +64,9 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
> struct sg_table *sg,
> enum dma_data_direction dir)
> {
> - struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf);
> -
> dma_unmap_sgtable(attachment->dev, sg, dir, DMA_ATTR_SKIP_CPU_SYNC);
> sg_free_table(sg);
> kfree(sg);
> -
> - i915_gem_object_unpin_pages(obj);
> }
>
> static int i915_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map *map)
> @@ -168,7 +160,31 @@ static int i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum dma_data_direct
> return err;
> }
>
> +/**
> + * i915_gem_dmabuf_attach - Do any extra attach work necessary
> + * @dmabuf: imported dma-buf
> + * @attach: new attach to do work on
> + *
> + */
> +static int i915_gem_dmabuf_attach(struct dma_buf *dmabuf,
> + struct dma_buf_attachment *attach)
> +{
> + struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf);
> +
> + return i915_gem_object_pin_pages_unlocked(obj);
> +}
> +
> +static void i915_gem_dmabuf_detach(struct dma_buf *dmabuf,
> + struct dma_buf_attachment *attach)
> +{
> + struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf);
> +
> + i915_gem_object_unpin_pages(obj);
> +}
> +
We don't normally add kernel-doc for static functions? Otherwise
dmabuf_detach() needs matching kernel-doc.
<snip>
> +
> +static int igt_dmabuf_import_same_driver(void *arg)
> +{
> + struct drm_i915_private *i915 = arg;
> + struct drm_i915_gem_object *obj, *import_obj;
> + struct drm_gem_object *import;
> + struct dma_buf *dmabuf;
> + struct dma_buf_attachment *import_attach;
> + struct sg_table *st;
> + long timeout;
> + int err;
> +
> + force_different_devices = true;
> + obj = i915_gem_object_create_shmem(i915, PAGE_SIZE);
> + if (IS_ERR(obj))
err = PTR_ERR(obj)
<snip>
> + /* Now try a fake an importer */
> + import_attach = dma_buf_attach(dmabuf, obj->base.dev->dev);
> + if (IS_ERR(import_attach))
> + goto out_import;
> +
> + st = dma_buf_map_attachment(import_attach, DMA_BIDIRECTIONAL);
> + if (IS_ERR(st))
> + goto out_detach;
For these two maybe missing err = ?
