RE: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map time

["Ruhl, Michael J" <michael.j.ruhl@intel.com>]

>-----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:
>>>>>>
>>>>>> 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.
>> /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
>>>>>