-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 2:50 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, Mike,
On 6/25/21 7:57 PM, Ruhl, Michael J wrote:
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 1:52 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
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();
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.
/sigh.
You are correct. I was mistakenly placing the pin API (dma_buf_ops) in the attach_ops. 😐 Must be Friday.
Upon further reflection, I think that your path will work.
However, is doing a pin (with no locking) from the dma_buf_mapping any different from using the pin API + export_dynamic?
M
/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