Hello,
On 4/18/22 21:38, Thomas Zimmermann wrote:
Hi
Am 18.04.22 um 00:37 schrieb Dmitry Osipenko:
Replace drm_gem_shmem locks with the reservation lock to make GEM lockings more consistent.
Previously drm_gem_shmem_vmap() and drm_gem_shmem_get_pages() were protected by separate locks, now it's the same lock, but it doesn't make any difference for the current GEM SHMEM users. Only Panfrost and Lima drivers use vmap() and they do it in the slow code paths, hence there was no practical justification for the usage of separate lock in the vmap().
Suggested-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Dmitry Osipenko dmitry.osipenko@collabora.com
...
@@ -310,7 +306,7 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, } else { pgprot_t prot = PAGE_KERNEL; - ret = drm_gem_shmem_get_pages(shmem); + ret = drm_gem_shmem_get_pages_locked(shmem); if (ret) goto err_zero_use; @@ -360,11 +356,11 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem, { int ret; - ret = mutex_lock_interruptible(&shmem->vmap_lock); + ret = dma_resv_lock_interruptible(shmem->base.resv, NULL); if (ret) return ret; ret = drm_gem_shmem_vmap_locked(shmem, map);
Within drm_gem_shmem_vmap_locked(), there's a call to dma_buf_vmap() for imported pages. If the exporter side also holds/acquires the same reservation lock as our object, the whole thing can deadlock. We cannot move dma_buf_vmap() out of the CS, because we still need to increment the reference counter. I honestly don't know how to easily fix this problem. There's a TODO item about replacing these locks at [1]. As Daniel suggested this patch, we should talk to him about the issue.
Best regards Thomas
[1] https://www.kernel.org/doc/html/latest/gpu/todo.html#move-buffer-object-lock...
Indeed, good catch! Perhaps we could simply use a separate lock for the vmapping of the *imported* GEMs? The vmap_use_count is used only by vmap/vunmap, so it doesn't matter which lock is used by these functions in the case of imported GEMs since we only need to protect the vmap_use_count.