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

[Thomas Hellström]

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.

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.

/Thomas


>
> I know for the dynamic version of dma-buf, there is a check to make
> sure that the lock is held when called.
>
> I think you will run into some issues if you try to get it here as well.
>
> Mike
>
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	ret = i915_gem_object_migrate(obj, NULL, INTEL_REGION_SMEM);
>> +	if (!ret)
>> +		ret = i915_gem_object_pin_pages(obj);
>> +	i915_gem_object_unlock(obj);
>> 	if (ret)
>> 		goto err;
>>
>> --
>> 2.31.1