Re: [PATCH v4 10/15] drm/shmem-helper: Take reservation lock instead of drm_gem_shmem locks

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 11246 bytes --]

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>
> ---
>   drivers/gpu/drm/drm_gem_shmem_helper.c  | 38 ++++++++++++-------------
>   drivers/gpu/drm/lima/lima_gem.c         |  8 +++---
>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 15 ++++++----
>   include/drm/drm_gem_shmem_helper.h      | 10 -------
>   4 files changed, 31 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 30ee46348a99..3ecef571eff3 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -86,8 +86,6 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
>   	if (ret)
>   		goto err_release;
>   
> -	mutex_init(&shmem->pages_lock);
> -	mutex_init(&shmem->vmap_lock);
>   	INIT_LIST_HEAD(&shmem->madv_list);
>   
>   	if (!private) {
> @@ -157,8 +155,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
>   	WARN_ON(shmem->pages_use_count);
>   
>   	drm_gem_object_release(obj);
> -	mutex_destroy(&shmem->pages_lock);
> -	mutex_destroy(&shmem->vmap_lock);
>   	kfree(shmem);
>   }
>   EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
> @@ -209,11 +205,11 @@ int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
>   
>   	WARN_ON(shmem->base.import_attach);
>   
> -	ret = mutex_lock_interruptible(&shmem->pages_lock);
> +	ret = dma_resv_lock_interruptible(shmem->base.resv, NULL);
>   	if (ret)
>   		return ret;
>   	ret = drm_gem_shmem_get_pages_locked(shmem);
> -	mutex_unlock(&shmem->pages_lock);
> +	dma_resv_unlock(shmem->base.resv);
>   
>   	return ret;
>   }
> @@ -248,9 +244,9 @@ static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem)
>    */
>   void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
>   {
> -	mutex_lock(&shmem->pages_lock);
> +	dma_resv_lock(shmem->base.resv, NULL);
>   	drm_gem_shmem_put_pages_locked(shmem);
> -	mutex_unlock(&shmem->pages_lock);
> +	dma_resv_unlock(shmem->base.resv);
>   }
>   EXPORT_SYMBOL(drm_gem_shmem_put_pages);
>   
> @@ -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-locking-to-dma-resv-lock



> -	mutex_unlock(&shmem->vmap_lock);
> +	dma_resv_unlock(shmem->base.resv);
>   
>   	return ret;
>   }
> @@ -385,7 +381,7 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
>   		dma_buf_vunmap(obj->import_attach->dmabuf, map);
>   	} else {
>   		vunmap(shmem->vaddr);
> -		drm_gem_shmem_put_pages(shmem);
> +		drm_gem_shmem_put_pages_locked(shmem);
>   	}
>   
>   	shmem->vaddr = NULL;
> @@ -406,9 +402,11 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
>   void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
>   			  struct iosys_map *map)
>   {
> -	mutex_lock(&shmem->vmap_lock);
> +	dma_resv_lock(shmem->base.resv, NULL);
>   	drm_gem_shmem_vunmap_locked(shmem, map);
> -	mutex_unlock(&shmem->vmap_lock);
> +	dma_resv_unlock(shmem->base.resv);
> +
> +	drm_gem_shmem_update_purgeable_status(shmem);
>   }
>   EXPORT_SYMBOL(drm_gem_shmem_vunmap);
>   
> @@ -442,14 +440,14 @@ drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
>    */
>   int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv)
>   {
> -	mutex_lock(&shmem->pages_lock);
> +	dma_resv_lock(shmem->base.resv, NULL);
>   
>   	if (shmem->madv >= 0)
>   		shmem->madv = madv;
>   
>   	madv = shmem->madv;
>   
> -	mutex_unlock(&shmem->pages_lock);
> +	dma_resv_unlock(shmem->base.resv);
>   
>   	return (madv >= 0);
>   }
> @@ -487,10 +485,10 @@ EXPORT_SYMBOL(drm_gem_shmem_purge_locked);
>   
>   bool drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem)
>   {
> -	if (!mutex_trylock(&shmem->pages_lock))
> +	if (!dma_resv_trylock(shmem->base.resv))
>   		return false;
>   	drm_gem_shmem_purge_locked(shmem);
> -	mutex_unlock(&shmem->pages_lock);
> +	dma_resv_unlock(shmem->base.resv);
>   
>   	return true;
>   }
> @@ -549,7 +547,7 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
>   	/* We don't use vmf->pgoff since that has the fake offset */
>   	page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
>   
> -	mutex_lock(&shmem->pages_lock);
> +	dma_resv_lock(shmem->base.resv, NULL);
>   
>   	if (page_offset >= num_pages ||
>   	    WARN_ON_ONCE(!shmem->pages) ||
> @@ -561,7 +559,7 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
>   		ret = vmf_insert_pfn(vma, vmf->address, page_to_pfn(page));
>   	}
>   
> -	mutex_unlock(&shmem->pages_lock);
> +	dma_resv_unlock(shmem->base.resv);
>   
>   	return ret;
>   }
> diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c
> index 0f1ca0b0db49..5008f0c2428f 100644
> --- a/drivers/gpu/drm/lima/lima_gem.c
> +++ b/drivers/gpu/drm/lima/lima_gem.c
> @@ -34,7 +34,7 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
>   
>   	new_size = min(new_size, bo->base.base.size);
>   
> -	mutex_lock(&bo->base.pages_lock);
> +	dma_resv_lock(bo->base.base.resv, NULL);
>   
>   	if (bo->base.pages) {
>   		pages = bo->base.pages;
> @@ -42,7 +42,7 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
>   		pages = kvmalloc_array(bo->base.base.size >> PAGE_SHIFT,
>   				       sizeof(*pages), GFP_KERNEL | __GFP_ZERO);
>   		if (!pages) {
> -			mutex_unlock(&bo->base.pages_lock);
> +			dma_resv_unlock(bo->base.base.resv);
>   			return -ENOMEM;
>   		}
>   
> @@ -56,13 +56,13 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
>   		struct page *page = shmem_read_mapping_page(mapping, i);
>   
>   		if (IS_ERR(page)) {
> -			mutex_unlock(&bo->base.pages_lock);
> +			dma_resv_unlock(bo->base.base.resv);
>   			return PTR_ERR(page);
>   		}
>   		pages[i] = page;
>   	}
>   
> -	mutex_unlock(&bo->base.pages_lock);
> +	dma_resv_unlock(bo->base.base.resv);
>   
>   	ret = sg_alloc_table_from_pages(&sgt, pages, i, 0,
>   					new_size, GFP_KERNEL);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index d3f82b26a631..404b8f67e2df 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -424,6 +424,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>   	struct panfrost_gem_mapping *bomapping;
>   	struct panfrost_gem_object *bo;
>   	struct address_space *mapping;
> +	struct drm_gem_object *obj;
>   	pgoff_t page_offset;
>   	struct sg_table *sgt;
>   	struct page **pages;
> @@ -446,13 +447,15 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>   	page_offset = addr >> PAGE_SHIFT;
>   	page_offset -= bomapping->mmnode.start;
>   
> -	mutex_lock(&bo->base.pages_lock);
> +	obj = &bo->base.base;
> +
> +	dma_resv_lock(obj->resv, NULL);
>   
>   	if (!bo->base.pages) {
>   		bo->sgts = kvmalloc_array(bo->base.base.size / SZ_2M,
>   				     sizeof(struct sg_table), GFP_KERNEL | __GFP_ZERO);
>   		if (!bo->sgts) {
> -			mutex_unlock(&bo->base.pages_lock);
> +			dma_resv_unlock(obj->resv);
>   			ret = -ENOMEM;
>   			goto err_bo;
>   		}
> @@ -462,7 +465,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>   		if (!pages) {
>   			kvfree(bo->sgts);
>   			bo->sgts = NULL;
> -			mutex_unlock(&bo->base.pages_lock);
> +			dma_resv_unlock(obj->resv);
>   			ret = -ENOMEM;
>   			goto err_bo;
>   		}
> @@ -472,7 +475,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>   		pages = bo->base.pages;
>   		if (pages[page_offset]) {
>   			/* Pages are already mapped, bail out. */
> -			mutex_unlock(&bo->base.pages_lock);
> +			dma_resv_unlock(obj->resv);
>   			goto out;
>   		}
>   	}
> @@ -483,13 +486,13 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>   	for (i = page_offset; i < page_offset + NUM_FAULT_PAGES; i++) {
>   		pages[i] = shmem_read_mapping_page(mapping, i);
>   		if (IS_ERR(pages[i])) {
> -			mutex_unlock(&bo->base.pages_lock);
> +			dma_resv_unlock(obj->resv);
>   			ret = PTR_ERR(pages[i]);
>   			goto err_pages;
>   		}
>   	}
>   
> -	mutex_unlock(&bo->base.pages_lock);
> +	dma_resv_unlock(obj->resv);
>   
>   	sgt = &bo->sgts[page_offset / (SZ_2M / PAGE_SIZE)];
>   	ret = sg_alloc_table_from_pages(sgt, pages + page_offset,
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index d0a57853c188..70889533962a 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -26,11 +26,6 @@ struct drm_gem_shmem_object {
>   	 */
>   	struct drm_gem_object base;
>   
> -	/**
> -	 * @pages_lock: Protects the page table and use count
> -	 */
> -	struct mutex pages_lock;
> -
>   	/**
>   	 * @pages: Page table
>   	 */
> @@ -79,11 +74,6 @@ struct drm_gem_shmem_object {
>   	 */
>   	struct sg_table *sgt;
>   
> -	/**
> -	 * @vmap_lock: Protects the vmap address and use count
> -	 */
> -	struct mutex vmap_lock;
> -
>   	/**
>   	 * @vaddr: Kernel virtual address of the backing memory
>   	 */

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]