| Message ID | 20220831153757.97381-7-dmitry.osipenko@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Move all drivers to a common dma-buf locking convention | expand |
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 <dmitry.osipenko@collabora.com> > > Acked-by: Christian König <christian.koenig@amd.com>, 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; >
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.
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;
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 <dmitry.osipenko@collabora.com> --- 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(-)