Re: [PATCH 1/7] drm/shmem-helper: Add lockdep asserts to vmap/vunmap

[Bence Csókás]

Hi,

I just came across this commit while researching something else. 
Original patch had too few context lines, so I here's the diff with `-U10`.

On 3/18/25 20:22, Daniel Almeida wrote:
> From: Asahi Lina <lina@xxxxxxxxxxxxx>
>
> Since commit 21aa27ddc582 ("drm/shmem-helper: Switch to reservation
> lock"), the drm_gem_shmem_vmap and drm_gem_shmem_vunmap functions
> require that the caller holds the DMA reservation lock for the object.
> Add lockdep assertions to help validate this.

There were already lockdep assertions...

> Signed-off-by: Asahi Lina <lina@xxxxxxxxxxxxx>
> Signed-off-by: Daniel Almeida <daniel.almeida@xxxxxxxxxxxxx>
> Reviewed-by: Christian König <christian.koenig@xxxxxxx>
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> Link: https://lore.kernel.org/r/20250318-drm-gem-shmem-v1-1-64b96511a84f@xxxxxxxxxxxxx
> ---
>   drivers/gpu/drm/drm_gem_shmem_helper.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index aa43265f4f4f..0b41f0346bad 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -341,20 +341,22 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_unpin);
>    *
>    * Returns:
>    * 0 on success or a negative error code on failure.
>    */
>   int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem,
>   			      struct iosys_map *map)
>   {
>   	struct drm_gem_object *obj = &shmem->base;
>   	int ret = 0;
>   
> +	dma_resv_assert_held(obj->resv);
> +
>   	if (drm_gem_is_imported(obj)) {
>   		ret = dma_buf_vmap(obj->dma_buf, map);
>   	} else {
>   		pgprot_t prot = PAGE_KERNEL;
>   
>   		dma_resv_assert_held(shmem->base.resv);

... right here, and

>   		if (refcount_inc_not_zero(&shmem->vmap_use_count)) {
>   			iosys_map_set_vaddr(map, shmem->vaddr);
>   			return 0;
> @@ -401,20 +403,22 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_vmap_locked);
>    * drops to zero.
>    *
>    * This function hides the differences between dma-buf imported and natively
>    * allocated objects.
>    */
>   void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
>   				 struct iosys_map *map)
>   {
>   	struct drm_gem_object *obj = &shmem->base;
>   
> +	dma_resv_assert_held(obj->resv);
> +
>   	if (drm_gem_is_imported(obj)) {
>   		dma_buf_vunmap(obj->dma_buf, map);
>   	} else {
>   		dma_resv_assert_held(shmem->base.resv);

...here.

>   		if (refcount_dec_and_test(&shmem->vmap_use_count)) {
>   			vunmap(shmem->vaddr);
>   			shmem->vaddr = NULL;
>   
>   			drm_gem_shmem_unpin_locked(shmem);

Or were those insufficient for some reason? If so, should we keep both 
of them, or should the older ones have been removed?

Bence