Re: [PATCH v3 2/2] [media] videobuf2-dc: Support cacheable MMAP

Tomasz Figa
2018-09-18T09:41:52Z

Hi Hans,

On Mon, Sep 17, 2018 at 11:41 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> I'm going through old patches in patchwork that fell through the
> cracks, and this is one of them.
>
> If this is still desired, please rebase and repost.
>
> I'm marking this series as Obsoleted in patchwork, since it no longer
> applies anyway.

The ability to have cached mappings of MMAP buffers is strongly
desired, but I'm afraid not the way this patch does it.

First of all, it's not a decision for the driver to make, but for the
user space, depending on the access pattern it does. It also isn't
something specific to vb2-dma-contig only.

I remember Sakari had a series that attempted to solve this in a more
comprehensive way[1]. I remember it had some minor problems when I
reviewed it, but generally the idea seemed sane.

Sakari, do you have any plans to revive that work?

[1] https://www.mail-archive.com/linux-media@vger.kernel.org/msg112459.html

Best regards,
Tomasz

>
> Regards,
>
> Hans
>
>
> On 10/26/2016 10:52 AM, Thierry Escande wrote:
> > From: Heng-Ruey Hsu <henryhsu@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.
> >
> > This patch enables cacheable memory for DMA coherent allocator in mmap
> > buffer allocation if non-consistent DMA attribute is set and kernel
> > mapping is present. Even if userspace doesn't mmap the buffer, sync
> > still should be happening if kernel mapping is present.
> > If not done in allocation, it is enabled when memory is mapped from
> > userspace (if non-consistent DMA attribute is set).
> >
> > Signed-off-by: Heng-Ruey Hsu <henryhsu@chromium.org>
> > Tested-by: Heng-ruey Hsu <henryhsu@chromium.org>
> > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> > ---
> > drivers/media/v4l2-core/videobuf2-dma-contig.c | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > index 0d9665d..89b534a 100644
> > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > @@ -151,6 +151,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);
> > @@ -192,6 +196,14 @@ static void *vb2_dc_alloc(struct device *dev, unsigned long attrs,
> > buf->handler.put = vb2_dc_put;
> > buf->handler.arg = buf;
> >
> > + /*
> > + * Enable cache maintenance. Even if userspace doesn't mmap the buffer,
> > + * sync still should be happening if kernel mapping is present.
> > + */
> > + 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;
> > @@ -227,6 +239,10 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)
> >
> > vma->vm_ops->open(vma);
> >
> > + /* Enable cache maintenance if not enabled in allocation. */
> > + if (!buf->dma_sgt && buf->attrs & DMA_ATTR_NON_CONSISTENT)
> > + buf->dma_sgt = vb2_dc_get_base_sgt(buf);
> > +
> > 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);
> >
>