On Wed, Nov 01, 2023 at 12:48:21PM +0900, Tomasz Figa wrote:
Hi Michael, On Wed, Nov 1, 2023 at 8:04 AM Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> wrote:For dmabuf import users to be able to use the vaddr from another videobuf2-dma-sg source, the exporter needs to set a proper vaddr on vb2_dma_sg_dmabuf_ops_vmap callback. This patch adds vmap on map if buf->vaddr was not set, and also adds the vb2_dma_sg_dmabuf_ops_vunmap function to remove the mapping afterwards. Cc: stable@xxxxxxxxxx Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> --- v2 -> v3: resend as a single patch v1 -> v2: using vmap and vunmap instead of vm_map_ram and vm_unmap_ramFirst of all, thanks for the patch! Please check my comments inline..../media/common/videobuf2/videobuf2-dma-sg.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index 28f3fdfe23a298..05b43edda94a7e 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c @@ -489,11 +489,26 @@ static int vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf, { struct vb2_dma_sg_buf *buf = dbuf->priv; + if (!buf->vaddr) + buf->vaddr = vmap(buf->pages, buf->num_pages, VM_MAP, + PAGE_KERNEL); +Would it make sense to use vb2_dma_sg_vaddr() instead of open coding the mapping again?
So in the end the whole patch would look like this: buf->vaddr = vb2_dma_sg_vaddr(NULL, buf); if (!buf->vaddr) return -ENOMEM;
iosys_map_set_vaddr(map, buf->vaddr); return 0; } +static void vb2_dma_sg_dmabuf_ops_vunmap(struct dma_buf *dbuf, + struct iosys_map *map) +{ + struct vb2_dma_sg_buf *buf = dbuf->priv; + + if (buf->vaddr) + vunmap(buf->vaddr); + + buf->vaddr = NULL; +}This could be potentially dangerous. Please consider the situation when the exporting V4L2 driver needs the CPU mapping for its own usage. It would call vb2_dma_sg_vaddr(), which would return the mapping. Then the importer could call vunmap, which would destroy the mapping that is still in use by the exporter internally. The idea of the current implementation is that we just create a kernel mapping when it's needed first and just keep it around until the entire buffer is destroyed.
In this patch I will drop the while vunmap callback then. Michael -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
