Hi Thierry,
On Fri, Oct 14, 2016 at 03:21:49PM +0200, Thierry Escande wrote:
> From: Heng-Ruey Hsu <henryhsu@xxxxxxxxxxxx>
>
> 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 <henryhsu@xxxxxxxxxxxx>
> Tested-by: Heng-ruey Hsu <henryhsu@xxxxxxxxxxxx>
> Reviewed-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
> Signed-off-by: Thierry Escande <thierry.escande@xxxxxxxxxxxxx>
> ---
> 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.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
