On 6/25/21 7:38 PM, Ruhl, Michael J wrote:
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 12:18 PM To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map time
Hi, Michael,
thanks for looking at this.
On 6/25/21 6:02 PM, Ruhl, Michael J wrote:
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Thomas Hellström Sent: Thursday, June 24, 2021 2:31 PM To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Thomas Hellström thomas.hellstrom@linux.intel.com; Auld,
Matthew
matthew.auld@intel.com Subject: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map
time
Until we support p2p dma or as a complement to that, migrate data to system memory at dma-buf map time if possible.
Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 616c3a2f1baf..a52f885bc09a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -25,7 +25,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);
- ret = i915_gem_object_lock_interruptible(obj, NULL);
Hmm, I believe in most cases that the caller should be holding the lock (object dma-resv) on this object already.
Yes, I agree, In particular for other instances of our own driver, at least since the dma_resv introduction.
But I also think that's a pre-existing bug, since i915_gem_object_pin_pages_unlocked() will also take the lock.
Ouch yes. Missed that.
I Think we need to initially make the exporter dynamic-capable to resolve this, and drop the locking here completely, as dma-buf docs says that we're then guaranteed to get called with the object lock held.
I figure if we make the exporter dynamic, we need to migrate already at dma_buf_pin time so we don't pin the object in the wrong location.
The exporter as dynamic (ops->pin is available) is optional, but importer dynamic (ops->move_notify) is required.
With that in mind, it would seem that there are three possible combinations for the migrate to be attempted:
- in the ops->pin function (export_dynamic != import_dynamic, during attach)
- in the ops->pin function (export_dynamic and !CONFIG_DMABUF_MOVE_NOTIFY) during mapping
- and possibly in ops->map_dma_buf (exort_dynamic iand CONFIG_DMABUF_MOVE_NOTIFY)
Since one possibility has to be in the mapping function, it seems that if we can figure out the locking, that the migrate should probably be available here.
Mike
So perhaps just to initially fix the bug, we could just implement NOP pin() and unpin() callbacks and drop the locking in map_attach() and replace it with an assert_object_held();
/Thomas