Hi Thierry,

On Fri, Oct 14, 2016 at 03:21:49PM +0200, Thierry Escande wrote:
> From: Heng-Ruey Hsu <henry...@chromium.org>
> 
> DMA allocations for MMAP type are uncached by default. But for
> some cases, CPU has to access the buffers. ie: memcpy for format
> converter. Supporting cacheable MMAP improves huge performance.

Does this patch basically make videobuf2-dma-contig work if you use the
DMA_ATTR_NO_KERNEL_MAPPING and DMA_ATTR_NON_CONSISTENT attributes? If so, it
should be mentioned in the commit message. No driver appears to be using
either of the two flags right now.

> 
> Signed-off-by: Heng-Ruey Hsu <henry...@chromium.org>
> Tested-by: Heng-ruey Hsu <henry...@chromium.org>
> Reviewed-by: Tomasz Figa <tf...@chromium.org>
> Signed-off-by: Thierry Escande <thierry.esca...@collabora.com>
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c 
> b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index fb6a177..c953c24 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -42,6 +42,12 @@ struct vb2_dc_buf {
>  };
>  
>  /*********************************************/
> +/*           Forward declarations            */
> +/*********************************************/
> +
> +static struct sg_table *vb2_dc_get_base_sgt(struct vb2_dc_buf *buf);

If you need this above from where the function is right now, could you move
it up instead in a separate patch?

> +
> +/*********************************************/
>  /*        scatterlist table functions        */
>  /*********************************************/
>  
> @@ -129,6 +135,10 @@ static void vb2_dc_put(void *buf_priv)
>               sg_free_table(buf->sgt_base);
>               kfree(buf->sgt_base);
>       }
> +     if (buf->dma_sgt) {
> +             sg_free_table(buf->dma_sgt);
> +             kfree(buf->dma_sgt);
> +     }
>       dma_free_attrs(buf->dev, buf->size, buf->cookie, buf->dma_addr,
>                      buf->attrs);
>       put_device(buf->dev);
> @@ -170,6 +180,10 @@ static void *vb2_dc_alloc(struct device *dev, unsigned 
> long attrs,
>       buf->handler.put = vb2_dc_put;
>       buf->handler.arg = buf;
>  

It'd be nice to add a comment why this is being done. I don't think it's
entirely trivial.

> +     if (!(buf->attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
> +          (buf->attrs & DMA_ATTR_NON_CONSISTENT))
> +             buf->dma_sgt = vb2_dc_get_base_sgt(buf);
> +
>       atomic_inc(&buf->refcount);
>  
>       return buf;
> @@ -205,6 +219,11 @@ static int vb2_dc_mmap(void *buf_priv, struct 
> vm_area_struct *vma)
>  
>       vma->vm_ops->open(vma);
>  
> +     if ((buf->attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&

Is the DMA_ATTR_NO_KERNEL_MAPPING flag relevant here? I understand the
mapping is used to flush the cache, but you do need it independently of
whether or not the DMA_ATTR_NO_KERNEL_MAPPING flag is present, don't you?

> +         (buf->attrs & DMA_ATTR_NON_CONSISTENT) &&
> +         !buf->dma_sgt)

I think it'd be nicer to check for buf->dma_sgt first; up to you. You don't
need the extra parentheses either (some extra in the above chunk as well).

> +             buf->dma_sgt = vb2_dc_get_base_sgt(buf);

What if vb2_dc_get_base_sgt() returns NULL here or in the chunk above?

> +
>       pr_debug("%s: mapped dma addr 0x%08lx at 0x%08lx, size %ld\n",
>               __func__, (unsigned long)buf->dma_addr, vma->vm_start,
>               buf->size);

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi     XMPP: sai...@retiisi.org.uk

Reply via email to