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

["Christian König" <christian.koenig@amd.com>]

On 4/2/26 18:13, Bence Csókás wrote:
> [Sie erhalten nicht häufig E-Mails von bence.csokas@arm.com. Weitere Informationen, warum dies wichtig ist, finden Sie unter https://aka.ms/LearnAboutSenderIdentification ]
> 
> 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@asahilina.net>
>>
>> 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...

Good point, I completely missed that.

> 
>> Signed-off-by: Asahi Lina <lina@asahilina.net>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Lyude Paul <lyude@redhat.com>
>> Link: https://lore.kernel.org/r/20250318-drm-gem-shmem-v1-1-64b96511a84f@collabora.com
>> ---
>>   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?

The dma_buf_vmap()/dma_buf_vunmap() functions require the caller to be holding the reservation lock as well.

So it kind of makes sense to move the assertions to the beginning of the functions.

Regards,
Christian.

> 
> Bence