>-----Original Message-----
>From: Daniel Vetter <daniel@xxxxxxxx>
>Sent: Wednesday, June 30, 2021 10:02 AM
>To: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>; Christian König
><ckoenig.leichtzumerken@xxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Auld,
>Matthew <matthew.auld@xxxxxxxxx>; maarten.lankhorst@xxxxxxxxxxxxxxx;
>daniel@xxxxxxxx; Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>
>Subject: Re: [PATCH 1/2] drm/i915/gem: Make our dma-buf exporter dynamic
>
>On Wed, Jun 30, 2021 at 03:07:00PM +0200, Thomas Hellström wrote:
>> 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.
>>
>> Add a live selftest to exercise both dynamic and non-dynamic exports,
>> and as a workaround until we fully support dynamic import and export,
>> declare the exporter dynamic by providing pin() and unpin()
>implementations.
>> For dynamic importers, make sure we keep the pinning also in
>map_dma_buf(),
>> to ensure we never need to call dma_buf_move_notify().
>> Calling dma_buf_move_notify() is at the discretion of the exporter.
>>
>> v2:
>> - Extend the selftest with a fake dynamic importer.
>> - Provide real pin and unpin callbacks to not abuse the interface.
>>
>> Reported-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
>
>I'm not happy with this, because i915 is currently violating the dma-resv
>fencing rules for dynamic dma-buf.
>
>Yes since this is just the exporter we can probably get away with yolo'ing
>things, but Christian and me just spend a lot of angry typing figuring out
>what the rules actually are, so I really don't like bending them even more
>just because it's less typing.
>
>All we need for a quick interim fix is to not take the dma_resv_lock from
>our map/unamp callbacks. Pinning our backing storage from attach/detach
>callbacks (which are also called under dma_resv_lock) would also achieve
>that, without mudding any waters. So essentially just moving the
>pin/unpin_pages_unlocked and we should be good, which is almost as little
>typing.
>
>Michael, since Thomas is on vacations now, care to type that up? The
>selftest is imo solid.
Yes, I will get that done.
Mike
>This is also consistent with what all other ttm based drivers do (aside
>from amdgpu, which is fully dynamic), see drm_gem_map_attach in
>drm_prime.c
>
>Adding Christian as fyi.
>-Daniel
>
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 31 ++++-
>> .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 116
>+++++++++++++++++-
>> 2 files changed, 143 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>> index 616c3a2f1baf..918c19df7b66 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,7 +27,14 @@ 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);
>> + assert_object_held(obj);
>> +
>> + /*
>> + * Note. In the dynamic importer case, the object is not yet pinned.
>> + * Let's pin it here to avoid having to call the move_notify
>> + * callback, The call of which is not yet implemented.
>> + */
>> + ret = i915_gem_object_pin_pages(obj);
>> if (ret)
>> goto err;
>>
>> @@ -168,6 +177,21 @@ static int i915_gem_end_cpu_access(struct
>dma_buf *dma_buf, enum dma_data_direct
>> return err;
>> }
>>
>> +static int i915_gem_dmabuf_pin(struct dma_buf_attachment *attach)
>> +{
>> + struct drm_i915_gem_object *obj = dma_buf_to_obj(attach-
>>dmabuf);
>> +
>> + assert_object_held(obj);
>> + return i915_gem_object_pin_pages(obj);
>> +}
>> +
>> +static void i915_gem_dmabuf_unpin(struct dma_buf_attachment *attach)
>> +{
>> + struct drm_i915_gem_object *obj = dma_buf_to_obj(attach-
>>dmabuf);
>> +
>> + i915_gem_object_unpin_pages(obj);
>> +}
>> +
>> static const struct dma_buf_ops i915_dmabuf_ops = {
>> .map_dma_buf = i915_gem_map_dma_buf,
>> .unmap_dma_buf = i915_gem_unmap_dma_buf,
>> @@ -177,6 +201,8 @@ static const struct dma_buf_ops i915_dmabuf_ops =
>{
>> .vunmap = i915_gem_dmabuf_vunmap,
>> .begin_cpu_access = i915_gem_begin_cpu_access,
>> .end_cpu_access = i915_gem_end_cpu_access,
>> + .pin = i915_gem_dmabuf_pin,
>> + .unpin = i915_gem_dmabuf_unpin,
>> };
>>
>> struct dma_buf *i915_gem_prime_export(struct drm_gem_object
>*gem_obj, int flags)
>> @@ -241,7 +267,8 @@ struct drm_gem_object
>*i915_gem_prime_import(struct drm_device *dev,
>> if (dma_buf->ops == &i915_dmabuf_ops) {
>> obj = dma_buf_to_obj(dma_buf);
>> /* is it from our device? */
>> - if (obj->base.dev == dev) {
>> + if (obj->base.dev == dev &&
>> + !I915_SELFTEST_ONLY(force_different_devices)) {
>> /*
>> * Importing dmabuf exported from out own gem
>increases
>> * refcount on gem itself instead of f_count of
>dmabuf.
>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>> index dd74bc09ec88..868b3469ecbd 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>> @@ -35,7 +35,7 @@ static int igt_dmabuf_export(void *arg)
>> static int igt_dmabuf_import_self(void *arg)
>> {
>> struct drm_i915_private *i915 = arg;
>> - struct drm_i915_gem_object *obj;
>> + struct drm_i915_gem_object *obj, *import_obj;
>> struct drm_gem_object *import;
>> struct dma_buf *dmabuf;
>> int err;
>> @@ -65,14 +65,125 @@ static int igt_dmabuf_import_self(void *arg)
>> err = -EINVAL;
>> goto out_import;
>> }
>> + import_obj = to_intel_bo(import);
>> +
>> + i915_gem_object_lock(import_obj, NULL);
>> + err = ____i915_gem_object_get_pages(import_obj);
>> + i915_gem_object_unlock(import_obj);
>> + if (err) {
>> + pr_err("Same object dma-buf get_pages failed!\n");
>> + goto out_import;
>> + }
>>
>> err = 0;
>> out_import:
>> - i915_gem_object_put(to_intel_bo(import));
>> + i915_gem_object_put(import_obj);
>> +out_dmabuf:
>> + dma_buf_put(dmabuf);
>> +out:
>> + i915_gem_object_put(obj);
>> + return err;
>> +}
>> +
>> +static void igt_dmabuf_move_notify(struct dma_buf_attachment *attach)
>> +{
>> + GEM_WARN_ON(1);
>> +}
>> +
>> +static const struct dma_buf_attach_ops igt_dmabuf_attach_ops = {
>> + .move_notify = igt_dmabuf_move_notify,
>> +};
>> +
>> +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))
>> + goto out_ret;
>> +
>> + dmabuf = i915_gem_prime_export(&obj->base, 0);
>> + if (IS_ERR(dmabuf)) {
>> + pr_err("i915_gem_prime_export failed with err=%d\n",
>> + (int)PTR_ERR(dmabuf));
>> + err = PTR_ERR(dmabuf);
>> + goto out;
>> + }
>> +
>> + import = i915_gem_prime_import(&i915->drm, dmabuf);
>> + if (IS_ERR(import)) {
>> + pr_err("i915_gem_prime_import failed with err=%d\n",
>> + (int)PTR_ERR(import));
>> + err = PTR_ERR(import);
>> + goto out_dmabuf;
>> + }
>> +
>> + if (import == &obj->base) {
>> + pr_err("i915_gem_prime_import reused gem object!\n");
>> + err = -EINVAL;
>> + goto out_import;
>> + }
>> +
>> + import_obj = to_intel_bo(import);
>> +
>> + i915_gem_object_lock(import_obj, NULL);
>> + err = ____i915_gem_object_get_pages(import_obj);
>> + if (err) {
>> + pr_err("Different objects dma-buf get_pages failed!\n");
>> + i915_gem_object_unlock(import_obj);
>> + goto out_import;
>> + }
>> +
>> + /*
>> + * If the exported object is not in system memory, something
>> + * weird is going on. TODO: When p2p is supported, this is no
>> + * longer considered weird.
>> + */
>> + if (obj->mm.region != i915->mm.regions[INTEL_REGION_SMEM]) {
>> + pr_err("Exported dma-buf is not in system memory\n");
>> + err = -EINVAL;
>> + }
>> +
>> + i915_gem_object_unlock(import_obj);
>> +
>> + /* Now try a fake dynamic importer */
>> + import_attach = dma_buf_dynamic_attach(dmabuf, obj->base.dev-
>>dev,
>> + &igt_dmabuf_attach_ops,
>> + NULL);
>> + if (IS_ERR(import_attach))
>> + goto out_import;
>> +
>> + dma_resv_lock(dmabuf->resv, NULL);
>> + st = dma_buf_map_attachment(import_attach,
>DMA_BIDIRECTIONAL);
>> + dma_resv_unlock(dmabuf->resv);
>> + if (IS_ERR(st))
>> + goto out_detach;
>> +
>> + timeout = dma_resv_wait_timeout(dmabuf->resv, false, true, 5 *
>HZ);
>> + if (!timeout) {
>> + pr_err("dmabuf wait for exclusive fence timed out.\n");
>> + timeout = -ETIME;
>> + }
>> + err = timeout > 0 ? 0 : timeout;
>> + dma_buf_unmap_attachment(import_attach, st,
>DMA_BIDIRECTIONAL);
>> +out_detach:
>> + dma_buf_detach(dmabuf, import_attach);
>> +out_import:
>> + i915_gem_object_put(import_obj);
>> out_dmabuf:
>> dma_buf_put(dmabuf);
>> out:
>> i915_gem_object_put(obj);
>> +out_ret:
>> + force_different_devices = false;
>> return err;
>> }
>>
>> @@ -286,6 +397,7 @@ int i915_gem_dmabuf_live_selftests(struct
>drm_i915_private *i915)
>> {
>> static const struct i915_subtest tests[] = {
>> SUBTEST(igt_dmabuf_export),
>> + SUBTEST(igt_dmabuf_import_same_driver),
>> };
>>
>> return i915_subtests(tests, i915);
>> --
>> 2.31.1
>>
>
>--
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
