-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 3:10 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 9:07 PM, Ruhl, Michael J wrote:
-----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
Yes, it's different for dynamic importers only that would otherwise never pin, and we could mistakenly evict the object without having implemented calling move_notify. If we pin, we never evict.
Ahh. Got it. That is an interesting nuance. I need to remember that there are other things than i915... 😊
So that would definitely put the migrate code in the pin path.
M
/Thomas
/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