Re: [PATCH] udmabuf: add vmap method to udmabuf_ops

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

On 11/11/22 15:05, Christian König wrote:
> Adding Dmitry as well.
> 
> Am 11.11.22 um 12:45 schrieb Lukasz Wiecaszek:
>> The reason behind that patch is associated with videobuf2 subsystem
>> (or more genrally with v4l2 framework) and user created
>> dma buffers (udmabuf). In some circumstances
>> when dealing with V4L2_MEMORY_DMABUF buffers videobuf2 subsystem
>> wants to use dma_buf_vmap() method on the attached dma buffer.
>> As udmabuf does not have .vmap operation implemented,
>> such dma_buf_vmap() natually fails.
>>
>> videobuf2_common: [cap-000000003473b2f1] __vb2_queue_alloc: allocated
>> 3 buffers, 1 plane(s) each
>> videobuf2_common: [cap-000000003473b2f1] __prepare_dmabuf: buffer for
>> plane 0 changed
>> videobuf2_common: [cap-000000003473b2f1] __prepare_dmabuf: failed to
>> map dmabuf for plane 0
>> videobuf2_common: [cap-000000003473b2f1] __buf_prepare: buffer
>> preparation failed: -14
>>
>> The patch itself seems to be strighforward.
>> It adds implementation of .vmap method to 'struct dma_buf_ops
>> udmabuf_ops'.
>> .vmap method itself uses vm_map_ram() to map pages linearly
>> into the kernel virtual address space (only if such mapping
>> hasn't been created yet).
> 
> Of hand that sounds sane to me.
> 
> You should probably mention somewhere in a code comment that the cached
> vaddr is protected by the reservation lock being taken. That's not
> necessary obvious to everybody.
> 
> Apart from that looks good to me.

Adding a comment won't hurt.

We have the dma_resv_assert_held() in dma_buf_vmap() that will help
spotting a missing lock at runtime by developers. While the
dmbuf_ops->vmap() shouldn't be ever used directly by importers.

-- 
Best regards,
Dmitry