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

[Ruhl, Michael J]

>-----Original Message-----
>From: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>Sent: Friday, June 25, 2021 1:52 PM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>; intel-
>gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
>Cc: Auld, Matthew <matthew.auld at 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 at linux.intel.com>
>>> Sent: Friday, June 25, 2021 12:18 PM
>>> To: Ruhl, Michael J <michael.j.ruhl at intel.com>; intel-
>>> gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
>>> Cc: Auld, Matthew <matthew.auld at 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 at lists.freedesktop.org> On Behalf
>Of
>>>>> Thomas Hellström
>>>>> Sent: Thursday, June 24, 2021 2:31 PM
>>>>> To: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
>>>>> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>; Auld,
>>> Matthew
>>>>> <matthew.auld at 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 at 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.

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
>