Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191217032034.54897-1-senozhatsky@chromium.org>
 <20191217032034.54897-13-senozhatsky@chromium.org> <1c5198dc-db4e-47d6-0d8b-259fbbb6372f@xs4all.nl>
 <CAAFQd5DN0FSJ=pXG3J32AXocnbkR+AB8yKKDk0tZS4s7K04Z9Q@mail.gmail.com> <560ba621-5396-1ea9-625e-a9f83622e052@xs4all.nl>
In-Reply-To: <560ba621-5396-1ea9-625e-a9f83622e052@xs4all.nl>
From:   Tomasz Figa <tfiga@chromium.org>
Date:   Thu, 30 Jan 2020 20:02:04 +0900
Message-ID: <CAAFQd5D27xaKhxg8UuPH6XXdzgBBsCeDL8wYw37r6AK+6sWcbg@mail.gmail.com>
Subject: Re: [RFC][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks
 to dma-sg
To:     Hans Verkuil <hverkuil@xs4all.nl>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Hans Verkuil <hans.verkuil@cisco.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Kyungmin Park <kyungmin.park@samsung.com>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Sakari Ailus <sakari.ailus@iki.fi>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Pawel Osciak <posciak@chromium.org>,
        Linux Media Mailing List <linux-media@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jan 28, 2020 at 5:36 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 1/28/20 5:38 AM, Tomasz Figa wrote:
> > On Fri, Jan 10, 2020 at 7:13 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> >>> Provide begin_cpu_access() and end_cpu_access() dma_buf_ops
> >>> callbacks for cache synchronisation on exported buffers.
> >>>
> >>> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> >>> ---
> >>>  .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++
> >>>  1 file changed, 22 insertions(+)
> >>>
> >>> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> >>> index 6db60e9d5183..bfc99a0cb7b9 100644
> >>> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> >>> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> >>> @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf)
> >>>       vb2_dma_sg_put(dbuf->priv);
> >>>  }
> >>>
> >>
> >> There is no corresponding vb2_sg_buffer_consistent function here.
> >>
> >> Looking more closely I see that vb2_dma_sg_alloc doesn't pass the dma_attrs
> >> argument to dma_map_sg_attrs, thus V4L2_FLAG_MEMORY_NON_CONSISTENT has no
> >> effect on dma-sg buffers.
> >
> > videobuf2-dma-sg allocates the memory using the page allocator
> > directly, which means that there is no memory consistency guarantee.
> >
> >>
> >> Is there a reason why dma_attrs isn't passed on to dma_map_sg_attrs()?
> >>
> >
> > V4L2_FLAG_MEMORY_NON_CONSISTENT is a flag for dma_alloc_attrs(). It
> > isn't supposed to do anything for dma_map_sg_attrs(), which is only
> > supposed to create the device (e.g. IOMMU) mapping for already
> > allocated memory.
>
> Ah, right.
>
> But could vb2_dma_sg_alloc_compacted() be modified so that is uses
> dma_alloc_attrs() instead of alloc_pages()? Sorry, that might be a stupid
> question, I'm not an expert in this area. All I know is that I hate inconsistent
> APIs where something works for one thing, but not another.
>

dma_alloc_attrs() would allocate contiguous memory, which kind of goes
against the vb2_dma_sg allocator idea. Technically we could call it N
times with size = 1 page to achieve what we want, but is this really
what we want?

Given that vb2_dma_sg has always been returning non-consistent memory,
do we have any good reason to add consistent memory to it? If so,
perhaps we could still do that in an incremental patch, to avoid
complicating this already complex series? :)

Best regards,
Tomasz

> Regards,
>
>         Hans
>
> >
> >> I suspect it was just laziness in the past, and that it should be wired
> >> up, just as for dma-contig.
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >>> +static int vb2_dma_sg_dmabuf_ops_begin_cpu_access(struct dma_buf *dbuf,
> >>> +                                     enum dma_data_direction direction)
> >>> +{
> >>> +     struct vb2_dma_sg_buf *buf = dbuf->priv;
> >>> +     struct sg_table *sgt = buf->dma_sgt;
> >>> +
> >>> +     dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static int vb2_dma_sg_dmabuf_ops_end_cpu_access(struct dma_buf *dbuf,
> >>> +                                     enum dma_data_direction direction)
> >>> +{
> >>> +     struct vb2_dma_sg_buf *buf = dbuf->priv;
> >>> +     struct sg_table *sgt = buf->dma_sgt;
> >>> +
> >>> +     dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> >>> +     return 0;
> >>> +}
> >>> +
> >>>  static void *vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf)
> >>>  {
> >>>       struct vb2_dma_sg_buf *buf = dbuf->priv;
> >>> @@ -488,6 +508,8 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = {
> >>>       .detach = vb2_dma_sg_dmabuf_ops_detach,
> >>>       .map_dma_buf = vb2_dma_sg_dmabuf_ops_map,
> >>>       .unmap_dma_buf = vb2_dma_sg_dmabuf_ops_unmap,
> >>> +     .begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access,
> >>> +     .end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access,
> >>>       .vmap = vb2_dma_sg_dmabuf_ops_vmap,
> >>>       .mmap = vb2_dma_sg_dmabuf_ops_mmap,
> >>>       .release = vb2_dma_sg_dmabuf_ops_release,
> >>>
> >>
>