[Intel-gfx] [PATCH 6/7] drm/i915/gem: Correct the locking and pin pattern for dma-buf (v6)

[Matthew Auld]

On Fri, 16 Jul 2021 at 15:14, Jason Ekstrand <jason at jlekstrand.net> wrote:
>
> From: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>
> 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 at intel.com>
> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl at intel.com>
> Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
> ---
>  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 = ?