Re: [PATCH] media: vb2: unify calling of set_page_dirty_lock

[Nicolas Dufresne]

Le mardi 29 aoÃt 2017 Ã 14:26 +0300, Stanimir Varbanov a Ãcrit :
> Currently videobuf2-dma-sg checks for dma direction for
> every single page and videobuf2-dc lacks any dma direction
> checks and calls set_page_dirty_lock unconditionally.
> 
> Thus unify and align the invocations of set_page_dirty_lock
> for videobuf2-dc, videobuf2-sg  memory allocators with
> videobuf2-vmalloc, i.e. the pattern used in vmalloc has been
> copied to dc and dma-sg.

Just before we go too far in "doing like vmalloc", I would like to
share this small video that display coherency issues when rendering
vmalloc backed DMABuf over various KMS/DRM driver. I can reproduce this
easily with Intel and MSM display drivers using UVC or Vivid as source.

The following is an HDMI capture of the following GStreamer pipeline
running on Dragonboard 410c.

    gst-launch-1.0 -v v4l2src device=/dev/video2 ! video/x-raw,format=NV16,width=1280,height=720 ! kmssink
    https://people.collabora.com/~nicolas/vmalloc-issue.mov

Feedback on this issue would be more then welcome. It's not clear to me
who's bug is this (v4l2, drm or iommu). The software is unlikely to be
blamed as this same pipeline works fine with non-vmalloc based sources.

regards,
Nicolas

> 
> Suggested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx>
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 6 ++++--
>  drivers/media/v4l2-core/videobuf2-dma-sg.c     | 7 +++----
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 9f389f36566d..696e24f9128d 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -434,8 +434,10 @@ static void vb2_dc_put_userptr(void *buf_priv)
>  		pages = frame_vector_pages(buf->vec);
>  		/* sgt should exist only if vector contains pages... */
>  		BUG_ON(IS_ERR(pages));
> -		for (i = 0; i < frame_vector_count(buf->vec); i++)
> -			set_page_dirty_lock(pages[i]);
> +		if (buf->dma_dir == DMA_FROM_DEVICE ||
> +		    buf->dma_dir == DMA_BIDIRECTIONAL)
> +			for (i = 0; i < frame_vector_count(buf->vec); i++)
> +				set_page_dirty_lock(pages[i]);
>  		sg_free_table(sgt);
>  		kfree(sgt);
>  	}
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> index 6808231a6bdc..753ed3138dcc 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> @@ -292,11 +292,10 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
>  	if (buf->vaddr)
>  		vm_unmap_ram(buf->vaddr, buf->num_pages);
>  	sg_free_table(buf->dma_sgt);
> -	while (--i >= 0) {
> -		if (buf->dma_dir == DMA_FROM_DEVICE ||
> -		    buf->dma_dir == DMA_BIDIRECTIONAL)
> +	if (buf->dma_dir == DMA_FROM_DEVICE ||
> +	    buf->dma_dir == DMA_BIDIRECTIONAL)
> +		while (--i >= 0)
>  			set_page_dirty_lock(buf->pages[i]);
> -	}
>  	vb2_destroy_framevec(buf->vec);
>  	kfree(buf);
>  }

Attachment: signature.asc
Description: This is a digitally signed message part