Hi, Mike,
On 6/25/21 7:57 PM, Ruhl, Michael J wrote:
-----Original Message-----
From: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
Sent: Friday, June 25, 2021 1:52 PM
To: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>; intel-
gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx
Cc: Auld, Matthew <matthew.auld@xxxxxxxxx>
Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map
time
On 6/25/21 7:38 PM, Ruhl, Michael J wrote:
-----Original Message-----
From: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
Sent: Friday, June 25, 2021 12:18 PM
To: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>; intel-
gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx
Cc: Auld, Matthew <matthew.auld@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx> On Behalf
Of
Thomas Hellström
Sent: Thursday, June 24, 2021 2:31 PM
To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx
Cc: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>; Auld,
Matthew
<matthew.auld@xxxxxxxxx>
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@xxxxxxxxxxxxxxx>
---
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:
1) in the ops->pin function (export_dynamic != import_dynamic, during
attach)
2) in the ops->pin function (export_dynamic and
!CONFIG_DMABUF_MOVE_NOTIFY) during mapping
3) 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();
That is the sticky part of the move notify API.
If you do the attach_dynamic you have to have an ops with move_notify.
(https://elixir.bootlin.com/linux/v5.13-rc7/source/drivers/dma-buf/dma-buf.c#L730)
If you don't have that, i.e. just the pin interface, the attach will be
rejected, and you will not get the callbacks.
I understood that as the requirement for move_notify is only if the
*importer* declares dynamic. A dynamic exporter could choose whether to
call move_notify() on eviction or to pin and never evict. If the
importer is non-dynamic, the core calls pin() and the only choice is to
pin and never evict.
So if we temporarily choose to pin and never evict for *everything*, (as
the current code does now), I think we should be good for now, and then
we can implement all fancy p2p and move_notify stuff on top of that.
/Thomas
So I think that the only thing we can do for now is to dop the locking and add the
assert_object_held();
M
/Thomas
