diff mbox series

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

Message ID 20210624183110.22582-5-thomas.hellstrom@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/gem: Introduce a migrate interface | expand

Commit Message

Thomas Hellström June 24, 2021, 6:31 p.m. UTC
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(-)

Comments

Ruhl, Michael J June 25, 2021, 4:02 p.m. UTC | #1
>-----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.

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
Thomas Hellström June 25, 2021, 4:17 p.m. UTC | #2
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.

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
Ruhl, Michael J June 25, 2021, 5:38 p.m. UTC | #3
>-----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


>/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
Thomas Hellström June 25, 2021, 5:52 p.m. UTC | #4
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();

/Thomas
Ruhl, Michael J June 25, 2021, 5:57 p.m. UTC | #5
>-----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.

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
>
Thomas Hellström June 25, 2021, 6:49 p.m. UTC | #6
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.

/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
>>
Ruhl, Michael J June 25, 2021, 7:07 p.m. UTC | #7
>-----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. 
Thomas Hellström June 25, 2021, 7:10 p.m. UTC | #8
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. 
Ruhl, Michael J June 25, 2021, 7:21 p.m. UTC | #9
>-----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. 
diff mbox series

Patch

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);
+	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;