Prepare i915 driver to the common dynamic dma-buf locking convention
by starting to use the unlocked versions of dma-buf API functions
and handling cases where importer now holds the reservation lock.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_object.c | 12 ++++++++++++
.../gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 16 ++++++++--------
3 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index f5062d0c6333..07eee1c09aaf 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -72,7 +72,7 @@ static int i915_gem_dmabuf_vmap(struct dma_buf *dma_buf,
struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
void *vaddr;
- vaddr = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
+ vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
if (IS_ERR(vaddr))
return PTR_ERR(vaddr);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 389e9f157ca5..7e2a9b02526c 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -331,7 +331,19 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
continue;
}
+ /*
+ * dma_buf_unmap_attachment() requires reservation to be
+ * locked. The imported GEM shouldn't share reservation lock,
+ * so it's safe to take the lock.
+ */
+ if (obj->base.import_attach)
+ i915_gem_object_lock(obj, NULL);
+
__i915_gem_object_pages_fini(obj);
+
+ if (obj->base.import_attach)
+ i915_gem_object_unlock(obj);
+
__i915_gem_free_object(obj);
/* But keep the pointer alive for RCU-protected lookups */
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
index 62c61af77a42..9e3ed634aa0e 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
@@ -213,7 +213,7 @@ static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915,
goto out_import;
}
- st = dma_buf_map_attachment(import_attach, DMA_BIDIRECTIONAL);
+ st = dma_buf_map_attachment_unlocked(import_attach, DMA_BIDIRECTIONAL);
if (IS_ERR(st)) {
err = PTR_ERR(st);
goto out_detach;
@@ -226,7 +226,7 @@ static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915,
timeout = -ETIME;
}
err = timeout > 0 ? 0 : timeout;
- dma_buf_unmap_attachment(import_attach, st, DMA_BIDIRECTIONAL);
+ dma_buf_unmap_attachment_unlocked(import_attach, st, DMA_BIDIRECTIONAL);
out_detach:
dma_buf_detach(dmabuf, import_attach);
out_import:
@@ -296,7 +296,7 @@ static int igt_dmabuf_import(void *arg)
goto out_obj;
}
- err = dma_buf_vmap(dmabuf, &map);
+ err = dma_buf_vmap_unlocked(dmabuf, &map);
dma_map = err ? NULL : map.vaddr;
if (!dma_map) {
pr_err("dma_buf_vmap failed\n");
@@ -337,7 +337,7 @@ static int igt_dmabuf_import(void *arg)
err = 0;
out_dma_map:
- dma_buf_vunmap(dmabuf, &map);
+ dma_buf_vunmap_unlocked(dmabuf, &map);
out_obj:
i915_gem_object_put(obj);
out_dmabuf:
@@ -358,7 +358,7 @@ static int igt_dmabuf_import_ownership(void *arg)
if (IS_ERR(dmabuf))
return PTR_ERR(dmabuf);
- err = dma_buf_vmap(dmabuf, &map);
+ err = dma_buf_vmap_unlocked(dmabuf, &map);
ptr = err ? NULL : map.vaddr;
if (!ptr) {
pr_err("dma_buf_vmap failed\n");
@@ -367,7 +367,7 @@ static int igt_dmabuf_import_ownership(void *arg)
}
memset(ptr, 0xc5, PAGE_SIZE);
- dma_buf_vunmap(dmabuf, &map);
+ dma_buf_vunmap_unlocked(dmabuf, &map);
obj = to_intel_bo(i915_gem_prime_import(&i915->drm, dmabuf));
if (IS_ERR(obj)) {
@@ -418,7 +418,7 @@ static int igt_dmabuf_export_vmap(void *arg)
}
i915_gem_object_put(obj);
- err = dma_buf_vmap(dmabuf, &map);
+ err = dma_buf_vmap_unlocked(dmabuf, &map);
ptr = err ? NULL : map.vaddr;
if (!ptr) {
pr_err("dma_buf_vmap failed\n");
@@ -435,7 +435,7 @@ static int igt_dmabuf_export_vmap(void *arg)
memset(ptr, 0xc5, dmabuf->size);
err = 0;
- dma_buf_vunmap(dmabuf, &map);
+ dma_buf_vunmap_unlocked(dmabuf, &map);
out:
dma_buf_put(dmabuf);
return err;
--
2.37.2
Am 31.08.22 um 17:37 schrieb Dmitry Osipenko:
> Prepare i915 driver to the common dynamic dma-buf locking convention
> by starting to use the unlocked versions of dma-buf API functions
> and handling cases where importer now holds the reservation lock.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
Acked-by: Christian König <[email protected]>, but it's probably
best if somebody from the Intel guys take a look as well.
> ---
> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 12 ++++++++++++
> .../gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 16 ++++++++--------
> 3 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> index f5062d0c6333..07eee1c09aaf 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> @@ -72,7 +72,7 @@ static int i915_gem_dmabuf_vmap(struct dma_buf *dma_buf,
> struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
> void *vaddr;
>
> - vaddr = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
> + vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
> if (IS_ERR(vaddr))
> return PTR_ERR(vaddr);
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index 389e9f157ca5..7e2a9b02526c 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -331,7 +331,19 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
> continue;
> }
>
> + /*
> + * dma_buf_unmap_attachment() requires reservation to be
> + * locked. The imported GEM shouldn't share reservation lock,
> + * so it's safe to take the lock.
> + */
> + if (obj->base.import_attach)
> + i915_gem_object_lock(obj, NULL);
> +
> __i915_gem_object_pages_fini(obj);
> +
> + if (obj->base.import_attach)
> + i915_gem_object_unlock(obj);
> +
> __i915_gem_free_object(obj);
>
> /* But keep the pointer alive for RCU-protected lookups */
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
> index 62c61af77a42..9e3ed634aa0e 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
> @@ -213,7 +213,7 @@ static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915,
> goto out_import;
> }
>
> - st = dma_buf_map_attachment(import_attach, DMA_BIDIRECTIONAL);
> + st = dma_buf_map_attachment_unlocked(import_attach, DMA_BIDIRECTIONAL);
> if (IS_ERR(st)) {
> err = PTR_ERR(st);
> goto out_detach;
> @@ -226,7 +226,7 @@ static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915,
> timeout = -ETIME;
> }
> err = timeout > 0 ? 0 : timeout;
> - dma_buf_unmap_attachment(import_attach, st, DMA_BIDIRECTIONAL);
> + dma_buf_unmap_attachment_unlocked(import_attach, st, DMA_BIDIRECTIONAL);
> out_detach:
> dma_buf_detach(dmabuf, import_attach);
> out_import:
> @@ -296,7 +296,7 @@ static int igt_dmabuf_import(void *arg)
> goto out_obj;
> }
>
> - err = dma_buf_vmap(dmabuf, &map);
> + err = dma_buf_vmap_unlocked(dmabuf, &map);
> dma_map = err ? NULL : map.vaddr;
> if (!dma_map) {
> pr_err("dma_buf_vmap failed\n");
> @@ -337,7 +337,7 @@ static int igt_dmabuf_import(void *arg)
>
> err = 0;
> out_dma_map:
> - dma_buf_vunmap(dmabuf, &map);
> + dma_buf_vunmap_unlocked(dmabuf, &map);
> out_obj:
> i915_gem_object_put(obj);
> out_dmabuf:
> @@ -358,7 +358,7 @@ static int igt_dmabuf_import_ownership(void *arg)
> if (IS_ERR(dmabuf))
> return PTR_ERR(dmabuf);
>
> - err = dma_buf_vmap(dmabuf, &map);
> + err = dma_buf_vmap_unlocked(dmabuf, &map);
> ptr = err ? NULL : map.vaddr;
> if (!ptr) {
> pr_err("dma_buf_vmap failed\n");
> @@ -367,7 +367,7 @@ static int igt_dmabuf_import_ownership(void *arg)
> }
>
> memset(ptr, 0xc5, PAGE_SIZE);
> - dma_buf_vunmap(dmabuf, &map);
> + dma_buf_vunmap_unlocked(dmabuf, &map);
>
> obj = to_intel_bo(i915_gem_prime_import(&i915->drm, dmabuf));
> if (IS_ERR(obj)) {
> @@ -418,7 +418,7 @@ static int igt_dmabuf_export_vmap(void *arg)
> }
> i915_gem_object_put(obj);
>
> - err = dma_buf_vmap(dmabuf, &map);
> + err = dma_buf_vmap_unlocked(dmabuf, &map);
> ptr = err ? NULL : map.vaddr;
> if (!ptr) {
> pr_err("dma_buf_vmap failed\n");
> @@ -435,7 +435,7 @@ static int igt_dmabuf_export_vmap(void *arg)
> memset(ptr, 0xc5, dmabuf->size);
>
> err = 0;
> - dma_buf_vunmap(dmabuf, &map);
> + dma_buf_vunmap_unlocked(dmabuf, &map);
> out:
> dma_buf_put(dmabuf);
> return err;
On 9/1/22 09:45, Christian König wrote:
> Am 31.08.22 um 17:37 schrieb Dmitry Osipenko:
>> Prepare i915 driver to the common dynamic dma-buf locking convention
>> by starting to use the unlocked versions of dma-buf API functions
>> and handling cases where importer now holds the reservation lock.
>>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>
> Acked-by: Christian König <[email protected]>, but it's probably
> best if somebody from the Intel guys take a look as well.
+ Chris Wilson, who touched locks of __i915_gem_free_objects() recently
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
>> drivers/gpu/drm/i915/gem/i915_gem_object.c | 12 ++++++++++++
>> .../gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 16 ++++++++--------
>> 3 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>> b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>> index f5062d0c6333..07eee1c09aaf 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>> @@ -72,7 +72,7 @@ static int i915_gem_dmabuf_vmap(struct dma_buf
>> *dma_buf,
>> struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
>> void *vaddr;
>> - vaddr = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
>> + vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
>> if (IS_ERR(vaddr))
>> return PTR_ERR(vaddr);
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> index 389e9f157ca5..7e2a9b02526c 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> @@ -331,7 +331,19 @@ static void __i915_gem_free_objects(struct
>> drm_i915_private *i915,
>> continue;
>> }
>> + /*
>> + * dma_buf_unmap_attachment() requires reservation to be
>> + * locked. The imported GEM shouldn't share reservation lock,
>> + * so it's safe to take the lock.
>> + */
>> + if (obj->base.import_attach)
>> + i915_gem_object_lock(obj, NULL);
>> +
>> __i915_gem_object_pages_fini(obj);
>> +
>> + if (obj->base.import_attach)
>> + i915_gem_object_unlock(obj);
>> +
>> __i915_gem_free_object(obj);
>> /* But keep the pointer alive for RCU-protected lookups */
>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>> b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>> index 62c61af77a42..9e3ed634aa0e 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>> @@ -213,7 +213,7 @@ static int igt_dmabuf_import_same_driver(struct
>> drm_i915_private *i915,
>> goto out_import;
>> }
>> - st = dma_buf_map_attachment(import_attach, DMA_BIDIRECTIONAL);
>> + st = dma_buf_map_attachment_unlocked(import_attach,
>> DMA_BIDIRECTIONAL);
>> if (IS_ERR(st)) {
>> err = PTR_ERR(st);
>> goto out_detach;
>> @@ -226,7 +226,7 @@ static int igt_dmabuf_import_same_driver(struct
>> drm_i915_private *i915,
>> timeout = -ETIME;
>> }
>> err = timeout > 0 ? 0 : timeout;
>> - dma_buf_unmap_attachment(import_attach, st, DMA_BIDIRECTIONAL);
>> + dma_buf_unmap_attachment_unlocked(import_attach, st,
>> DMA_BIDIRECTIONAL);
>> out_detach:
>> dma_buf_detach(dmabuf, import_attach);
>> out_import:
>> @@ -296,7 +296,7 @@ static int igt_dmabuf_import(void *arg)
>> goto out_obj;
>> }
>> - err = dma_buf_vmap(dmabuf, &map);
>> + err = dma_buf_vmap_unlocked(dmabuf, &map);
>> dma_map = err ? NULL : map.vaddr;
>> if (!dma_map) {
>> pr_err("dma_buf_vmap failed\n");
>> @@ -337,7 +337,7 @@ static int igt_dmabuf_import(void *arg)
>> err = 0;
>> out_dma_map:
>> - dma_buf_vunmap(dmabuf, &map);
>> + dma_buf_vunmap_unlocked(dmabuf, &map);
>> out_obj:
>> i915_gem_object_put(obj);
>> out_dmabuf:
>> @@ -358,7 +358,7 @@ static int igt_dmabuf_import_ownership(void *arg)
>> if (IS_ERR(dmabuf))
>> return PTR_ERR(dmabuf);
>> - err = dma_buf_vmap(dmabuf, &map);
>> + err = dma_buf_vmap_unlocked(dmabuf, &map);
>> ptr = err ? NULL : map.vaddr;
>> if (!ptr) {
>> pr_err("dma_buf_vmap failed\n");
>> @@ -367,7 +367,7 @@ static int igt_dmabuf_import_ownership(void *arg)
>> }
>> memset(ptr, 0xc5, PAGE_SIZE);
>> - dma_buf_vunmap(dmabuf, &map);
>> + dma_buf_vunmap_unlocked(dmabuf, &map);
>> obj = to_intel_bo(i915_gem_prime_import(&i915->drm, dmabuf));
>> if (IS_ERR(obj)) {
>> @@ -418,7 +418,7 @@ static int igt_dmabuf_export_vmap(void *arg)
>> }
>> i915_gem_object_put(obj);
>> - err = dma_buf_vmap(dmabuf, &map);
>> + err = dma_buf_vmap_unlocked(dmabuf, &map);
>> ptr = err ? NULL : map.vaddr;
>> if (!ptr) {
>> pr_err("dma_buf_vmap failed\n");
>> @@ -435,7 +435,7 @@ static int igt_dmabuf_export_vmap(void *arg)
>> memset(ptr, 0xc5, dmabuf->size);
>> err = 0;
>> - dma_buf_vunmap(dmabuf, &map);
>> + dma_buf_vunmap_unlocked(dmabuf, &map);
>> out:
>> dma_buf_put(dmabuf);
>> return err;
>
--
Best regards,
Dmitry
>-----Original Message-----
>From: Dmitry Osipenko <[email protected]>
>Sent: Wednesday, August 31, 2022 11:38 AM
>To: David Airlie <[email protected]>; Gerd Hoffmann <[email protected]>;
>Gurchetan Singh <[email protected]>; Chia-I Wu
><[email protected]>; Daniel Vetter <[email protected]>; Daniel Almeida
><[email protected]>; Gert Wollny <[email protected]>;
>Gustavo Padovan <[email protected]>; Daniel Stone
><[email protected]>; Tomeu Vizoso <[email protected]>;
>Maarten Lankhorst <[email protected]>; Maxime Ripard
><[email protected]>; Thomas Zimmermann <[email protected]>;
>Rob Clark <[email protected]>; Sumit Semwal
><[email protected]>; Christian K?nig <[email protected]>;
>Pan, Xinhui <[email protected]>; Thierry Reding
><[email protected]>; Tomasz Figa <[email protected]>; Marek
>Szyprowski <[email protected]>; Mauro Carvalho Chehab
><[email protected]>; Alex Deucher <[email protected]>; Jani
>Nikula <[email protected]>; Joonas Lahtinen
><[email protected]>; Vivi, Rodrigo <[email protected]>;
>Tvrtko Ursulin <[email protected]>; Thomas Hellstr?m
><[email protected]>; Qiang Yu <[email protected]>; Srinivas
>Kandagatla <[email protected]>; Amol Maheshwari
><[email protected]>; Jason Gunthorpe <[email protected]>; Leon
>Romanovsky <[email protected]>; Gross, Jurgen <[email protected]>; Stefano
>Stabellini <[email protected]>; Oleksandr Tyshchenko
><[email protected]>; Tomi Valkeinen <[email protected]>;
>Russell King <[email protected]>; Lucas Stach <[email protected]>;
>Christian Gmeiner <[email protected]>
>Cc: [email protected]; [email protected]; Dmitry
>Osipenko <[email protected]>; [email protected]; linaro-mm-
>[email protected]; [email protected]; intel-
>[email protected]; [email protected]; [email protected]
>foundation.org; [email protected]; linux-arm-
>[email protected]
>Subject: [PATCH v4 06/21] drm/i915: Prepare to dynamic dma-buf locking
>specification
>
>Prepare i915 driver to the common dynamic dma-buf locking convention
>by starting to use the unlocked versions of dma-buf API functions
>and handling cases where importer now holds the reservation lock.
>
>Signed-off-by: Dmitry Osipenko <[email protected]>
>---
> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 12 ++++++++++++
> .../gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 16 ++++++++--------
> 3 files changed, 21 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>index f5062d0c6333..07eee1c09aaf 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>@@ -72,7 +72,7 @@ static int i915_gem_dmabuf_vmap(struct dma_buf
>*dma_buf,
> struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
> void *vaddr;
>
>- vaddr = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
>+ vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
> if (IS_ERR(vaddr))
> return PTR_ERR(vaddr);
>
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>index 389e9f157ca5..7e2a9b02526c 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>@@ -331,7 +331,19 @@ static void __i915_gem_free_objects(struct
>drm_i915_private *i915,
> continue;
> }
>
>+ /*
>+ * dma_buf_unmap_attachment() requires reservation to be
>+ * locked. The imported GEM shouldn't share reservation lock,
>+ * so it's safe to take the lock.
>+ */
>+ if (obj->base.import_attach)
>+ i915_gem_object_lock(obj, NULL);
There is a lot of stuff going here. Taking the lock may be premature...
> __i915_gem_object_pages_fini(obj);
The i915_gem_dmabuf.c:i915_gem_object_put_pages_dmabuf is where
unmap_attachment is actually called, would it make more sense to make
do the locking there?
Mike
>+
>+ if (obj->base.import_attach)
>+ i915_gem_object_unlock(obj);
>+
> __i915_gem_free_object(obj);
>
> /* But keep the pointer alive for RCU-protected lookups */
>diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>index 62c61af77a42..9e3ed634aa0e 100644
>--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>@@ -213,7 +213,7 @@ static int igt_dmabuf_import_same_driver(struct
>drm_i915_private *i915,
> goto out_import;
> }
>
>- st = dma_buf_map_attachment(import_attach,
>DMA_BIDIRECTIONAL);
>+ st = dma_buf_map_attachment_unlocked(import_attach,
>DMA_BIDIRECTIONAL);
> if (IS_ERR(st)) {
> err = PTR_ERR(st);
> goto out_detach;
>@@ -226,7 +226,7 @@ static int igt_dmabuf_import_same_driver(struct
>drm_i915_private *i915,
> timeout = -ETIME;
> }
> err = timeout > 0 ? 0 : timeout;
>- dma_buf_unmap_attachment(import_attach, st,
>DMA_BIDIRECTIONAL);
>+ dma_buf_unmap_attachment_unlocked(import_attach, st,
>DMA_BIDIRECTIONAL);
> out_detach:
> dma_buf_detach(dmabuf, import_attach);
> out_import:
>@@ -296,7 +296,7 @@ static int igt_dmabuf_import(void *arg)
> goto out_obj;
> }
>
>- err = dma_buf_vmap(dmabuf, &map);
>+ err = dma_buf_vmap_unlocked(dmabuf, &map);
> dma_map = err ? NULL : map.vaddr;
> if (!dma_map) {
> pr_err("dma_buf_vmap failed\n");
>@@ -337,7 +337,7 @@ static int igt_dmabuf_import(void *arg)
>
> err = 0;
> out_dma_map:
>- dma_buf_vunmap(dmabuf, &map);
>+ dma_buf_vunmap_unlocked(dmabuf, &map);
> out_obj:
> i915_gem_object_put(obj);
> out_dmabuf:
>@@ -358,7 +358,7 @@ static int igt_dmabuf_import_ownership(void *arg)
> if (IS_ERR(dmabuf))
> return PTR_ERR(dmabuf);
>
>- err = dma_buf_vmap(dmabuf, &map);
>+ err = dma_buf_vmap_unlocked(dmabuf, &map);
> ptr = err ? NULL : map.vaddr;
> if (!ptr) {
> pr_err("dma_buf_vmap failed\n");
>@@ -367,7 +367,7 @@ static int igt_dmabuf_import_ownership(void *arg)
> }
>
> memset(ptr, 0xc5, PAGE_SIZE);
>- dma_buf_vunmap(dmabuf, &map);
>+ dma_buf_vunmap_unlocked(dmabuf, &map);
>
> obj = to_intel_bo(i915_gem_prime_import(&i915->drm, dmabuf));
> if (IS_ERR(obj)) {
>@@ -418,7 +418,7 @@ static int igt_dmabuf_export_vmap(void *arg)
> }
> i915_gem_object_put(obj);
>
>- err = dma_buf_vmap(dmabuf, &map);
>+ err = dma_buf_vmap_unlocked(dmabuf, &map);
> ptr = err ? NULL : map.vaddr;
> if (!ptr) {
> pr_err("dma_buf_vmap failed\n");
>@@ -435,7 +435,7 @@ static int igt_dmabuf_export_vmap(void *arg)
> memset(ptr, 0xc5, dmabuf->size);
>
> err = 0;
>- dma_buf_vunmap(dmabuf, &map);
>+ dma_buf_vunmap_unlocked(dmabuf, &map);
> out:
> dma_buf_put(dmabuf);
> return err;
>--
>2.37.2
01.09.2022 17:02, Ruhl, Michael J пишет:
...
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> @@ -331,7 +331,19 @@ static void __i915_gem_free_objects(struct
>> drm_i915_private *i915,
>> continue;
>> }
>>
>> + /*
>> + * dma_buf_unmap_attachment() requires reservation to be
>> + * locked. The imported GEM shouldn't share reservation lock,
>> + * so it's safe to take the lock.
>> + */
>> + if (obj->base.import_attach)
>> + i915_gem_object_lock(obj, NULL);
>
> There is a lot of stuff going here. Taking the lock may be premature...
>
>> __i915_gem_object_pages_fini(obj);
>
> The i915_gem_dmabuf.c:i915_gem_object_put_pages_dmabuf is where
> unmap_attachment is actually called, would it make more sense to make
> do the locking there?
The __i915_gem_object_put_pages() is invoked with a held reservation
lock, while freeing object is a special time when we know that GEM is
unused.
The __i915_gem_free_objects() was taking the lock two weeks ago until
the change made Chris Wilson [1] reached linux-next.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=2826d447fbd60e6a05e53d5f918bceb8c04e315c
I don't think we can take the lock within
i915_gem_object_put_pages_dmabuf(), it may/should deadlock other code paths.
02.09.2022 13:31, Dmitry Osipenko пишет:
> 01.09.2022 17:02, Ruhl, Michael J пишет:
> ...
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>>> @@ -331,7 +331,19 @@ static void __i915_gem_free_objects(struct
>>> drm_i915_private *i915,
>>> continue;
>>> }
>>>
>>> + /*
>>> + * dma_buf_unmap_attachment() requires reservation to be
>>> + * locked. The imported GEM shouldn't share reservation lock,
>>> + * so it's safe to take the lock.
>>> + */
>>> + if (obj->base.import_attach)
>>> + i915_gem_object_lock(obj, NULL);
>>
>> There is a lot of stuff going here. Taking the lock may be premature...
>>
>>> __i915_gem_object_pages_fini(obj);
>>
>> The i915_gem_dmabuf.c:i915_gem_object_put_pages_dmabuf is where
>> unmap_attachment is actually called, would it make more sense to make
>> do the locking there?
>
> The __i915_gem_object_put_pages() is invoked with a held reservation
> lock, while freeing object is a special time when we know that GEM is
> unused.
>
> The __i915_gem_free_objects() was taking the lock two weeks ago until
> the change made Chris Wilson [1] reached linux-next.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=2826d447fbd60e6a05e53d5f918bceb8c04e315c
>
> I don't think we can take the lock within
> i915_gem_object_put_pages_dmabuf(), it may/should deadlock other code paths.
On the other hand, we can check whether the GEM's refcount number is
zero in i915_gem_object_put_pages_dmabuf() and then take the lock if
it's zero.
Also, seems it should be possible just to bail out from
i915_gem_object_put_pages_dmabuf() if refcount=0. The further
drm_prime_gem_destroy() will take care of unmapping. Perhaps this could
be the best option, I'll give it a test.
>-----Original Message-----
>From: Dmitry Osipenko <[email protected]>
>Sent: Friday, September 2, 2022 6:39 AM
>To: Ruhl, Michael J <[email protected]>; Dmitry Osipenko
><[email protected]>; Jani Nikula <[email protected]>;
>Joonas Lahtinen <[email protected]>; Vivi, Rodrigo
><[email protected]>; Tvrtko Ursulin <[email protected]>;
>Thomas Hellström <[email protected]>; Christian König
><[email protected]>; Chris Wilson <[email protected]>
>Cc: [email protected]; [email protected]; linux-
>[email protected]; [email protected]; amd-
>[email protected]; [email protected];
>[email protected]; [email protected]; linux-
>[email protected]; [email protected]; David Airlie
><[email protected]>; Gerd Hoffmann <[email protected]>; Gurchetan Singh
><[email protected]>; Chia-I Wu <[email protected]>; Daniel
>Vetter <[email protected]>; Daniel Almeida <[email protected]>;
>Gert Wollny <[email protected]>; Gustavo Padovan
><[email protected]>; Daniel Stone <[email protected]>;
>Tomeu Vizoso <[email protected]>; Maarten Lankhorst
><[email protected]>; Maxime Ripard
><[email protected]>; Thomas Zimmermann <[email protected]>;
>Rob Clark <[email protected]>; Sumit Semwal
><[email protected]>; Pan, Xinhui <[email protected]>; Thierry
>Reding <[email protected]>; Tomasz Figa <[email protected]>;
>Marek Szyprowski <[email protected]>; Mauro Carvalho Chehab
><[email protected]>; Alex Deucher <[email protected]>;
>Qiang Yu <[email protected]>; Srinivas Kandagatla
><[email protected]>; Amol Maheshwari
><[email protected]>; Jason Gunthorpe <[email protected]>; Leon
>Romanovsky <[email protected]>; Gross, Jurgen <[email protected]>; Stefano
>Stabellini <[email protected]>; Oleksandr Tyshchenko
><[email protected]>; Tomi Valkeinen <[email protected]>;
>Russell King <[email protected]>; Lucas Stach <[email protected]>;
>Christian Gmeiner <[email protected]>
>Subject: Re: [PATCH v4 06/21] drm/i915: Prepare to dynamic dma-buf locking
>specification
>
>02.09.2022 13:31, Dmitry Osipenko пишет:
>> 01.09.2022 17:02, Ruhl, Michael J пишет:
>> ...
>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>>>> @@ -331,7 +331,19 @@ static void __i915_gem_free_objects(struct
>>>> drm_i915_private *i915,
>>>> continue;
>>>> }
>>>>
>>>> + /*
>>>> + * dma_buf_unmap_attachment() requires reservation to be
>>>> + * locked. The imported GEM shouldn't share reservation lock,
>>>> + * so it's safe to take the lock.
>>>> + */
>>>> + if (obj->base.import_attach)
>>>> + i915_gem_object_lock(obj, NULL);
>>>
>>> There is a lot of stuff going here. Taking the lock may be premature...
>>>
>>>> __i915_gem_object_pages_fini(obj);
>>>
>>> The i915_gem_dmabuf.c:i915_gem_object_put_pages_dmabuf is where
>>> unmap_attachment is actually called, would it make more sense to make
>>> do the locking there?
>>
>> The __i915_gem_object_put_pages() is invoked with a held reservation
>> lock, while freeing object is a special time when we know that GEM is
>> unused.
>>
>> The __i915_gem_free_objects() was taking the lock two weeks ago until
>> the change made Chris Wilson [1] reached linux-next.
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-
>next.git/commit/?id=2826d447fbd60e6a05e53d5f918bceb8c04e315c
>>
>> I don't think we can take the lock within
>> i915_gem_object_put_pages_dmabuf(), it may/should deadlock other code
>paths.
>
>On the other hand, we can check whether the GEM's refcount number is
>zero in i915_gem_object_put_pages_dmabuf() and then take the lock if
>it's zero.
>
>Also, seems it should be possible just to bail out from
>i915_gem_object_put_pages_dmabuf() if refcount=0. The further
>drm_prime_gem_destroy() will take care of unmapping. Perhaps this could
>be the best option, I'll give it a test.
i915_gem_object_put_pages() is uses the SG, and the usage for
drm_prim_gem_destroy()
from __i915_gem_free_objects() doesn't use the SG because it has been "freed"
already, I am not sure if that would work...
Hmm.. with that in mind, maybe moving the base.import_attach check to
__i915_gem_object_put_pages with your attach check?
atomic_set(&obj->mm.pages_pin_count, 0);
if (obj->base.import)
i915_gem_object_lock(obj, NULL);
__i915_gem_object_put_pages(obj);
if (obj->base.import)
i915_gem_object_unlock(obj, NULL);
GEM_BUG_ON(i915_gem_object_has_pages(obj));
Pretty much one step up from the dmabuf interface, but we are guaranteed to
not have any pinned pages?
The other caller of __i915_gem_object_pages_fini is the i915_ttm move_notify
which should not conflict (export side, not import side).
Since it appears that not locking during the clean up is desirable, trying to make sure take the lock
is taken at the last moment might be the right path?
M
On 9/2/22 19:26, Ruhl, Michael J wrote:
>> 02.09.2022 13:31, Dmitry Osipenko пишет:
>>> 01.09.2022 17:02, Ruhl, Michael J пишет:
>>> ...
>>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>>>>> @@ -331,7 +331,19 @@ static void __i915_gem_free_objects(struct
>>>>> drm_i915_private *i915,
>>>>> continue;
>>>>> }
>>>>>
>>>>> + /*
>>>>> + * dma_buf_unmap_attachment() requires reservation to be
>>>>> + * locked. The imported GEM shouldn't share reservation lock,
>>>>> + * so it's safe to take the lock.
>>>>> + */
>>>>> + if (obj->base.import_attach)
>>>>> + i915_gem_object_lock(obj, NULL);
>>>>
>>>> There is a lot of stuff going here. Taking the lock may be premature...
>>>>
>>>>> __i915_gem_object_pages_fini(obj);
>>>>
>>>> The i915_gem_dmabuf.c:i915_gem_object_put_pages_dmabuf is where
>>>> unmap_attachment is actually called, would it make more sense to make
>>>> do the locking there?
>>>
>>> The __i915_gem_object_put_pages() is invoked with a held reservation
>>> lock, while freeing object is a special time when we know that GEM is
>>> unused.
>>>
>>> The __i915_gem_free_objects() was taking the lock two weeks ago until
>>> the change made Chris Wilson [1] reached linux-next.
>>>
>>> [1]
>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-
>> next.git/commit/?id=2826d447fbd60e6a05e53d5f918bceb8c04e315c
>>>
>>> I don't think we can take the lock within
>>> i915_gem_object_put_pages_dmabuf(), it may/should deadlock other code
>> paths.
>>
>> On the other hand, we can check whether the GEM's refcount number is
>> zero in i915_gem_object_put_pages_dmabuf() and then take the lock if
>> it's zero.
>>
>> Also, seems it should be possible just to bail out from
>> i915_gem_object_put_pages_dmabuf() if refcount=0. The further
>> drm_prime_gem_destroy() will take care of unmapping. Perhaps this could
>> be the best option, I'll give it a test.
>
> i915_gem_object_put_pages() is uses the SG, and the usage for
> drm_prim_gem_destroy()
>
> from __i915_gem_free_objects() doesn't use the SG because it has been "freed"
> already, I am not sure if that would work...
>
> Hmm.. with that in mind, maybe moving the base.import_attach check to
> __i915_gem_object_put_pages with your attach check?
I see you meant __i915_gem_object_pages_fini() here.
> atomic_set(&obj->mm.pages_pin_count, 0);
> if (obj->base.import)
> i915_gem_object_lock(obj, NULL);
>
> __i915_gem_object_put_pages(obj);
>
> if (obj->base.import)
> i915_gem_object_unlock(obj, NULL);
> GEM_BUG_ON(i915_gem_object_has_pages(obj));
>
> Pretty much one step up from the dmabuf interface, but we are guaranteed to
> not have any pinned pages?
Importer shouldn't hold pages outside of dma-buf API, otherwise it
should be a bug.
> The other caller of __i915_gem_object_pages_fini is the i915_ttm move_notify
> which should not conflict (export side, not import side).
>
> Since it appears that not locking during the clean up is desirable, trying to make sure take the lock
> is taken at the last moment might be the right path?
Reducing the scope of locking usually is preferred more. Yours
suggestion works okay, I couldn't spot any problems at least for a
non-TTM code paths.
It's indeed a bit not nice that __i915_gem_object_pages_fini() is used
by TTM, but should be safe for imported objects. Will be great if anyone
from i915 maintainers could ack this variant.
--
Best regards,
Dmitry