On Thu, Dec 15, 2016 at 11:08:37PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Friday 11 Sep 2015 14:50:30 Sakari Ailus wrote:
> > The struct vb2_dc_buf contains two struct sg_table fields: sgt_base and
> > dma_sgt. The former is used by DMA-BUF buffers whereas the latter is used
> > by USERPTR.
> >
> > Unify the two, leaving dma_sgt.
>
> Looks good to me. I initially thought this would prevent exporting a USERPTR
> buffer through dmabuf, but that's forbidden by the videobuf2 core (and rightly
> so).
>
> > MMAP buffers do not need cache flushing since they have been allocated
> > using dma_alloc_coherent().
>
> I had understood this as meaning that the patch changes the behaviour for MMAP
> buffers. How about
>
> "The prepare and finish implementation currently skip cache sync for MMAP
> buffers and identify them based on dma_sgt being NULL. Now that dma_sgt is
> used to export the MMAP buffers the condition must be updated."
>
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/v4l2-core/videobuf2-dma-contig.c | 21 ++++++++++-----------
> > 1 file changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > b/drivers/media/v4l2-core/videobuf2-dma-contig.c index 94c1e64..26a0a0f
> > 100644
> > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > @@ -36,7 +36,6 @@ struct vb2_dc_buf {
> > /* MMAP related */
> > struct vb2_vmarea_handler handler;
> > atomic_t refcount;
> > - struct sg_table *sgt_base;
> >
> > /* USERPTR related */
> > struct vm_area_struct *vma;
> > @@ -117,7 +116,7 @@ static void vb2_dc_prepare(void *buf_priv)
> > struct sg_table *sgt = buf->dma_sgt;
> >
> > /* DMABUF exporter will flush the cache for us */
>
> How about updating the comment to
>
> /*
> * Skip cache sync for MMAP buffers (they don't need it) and DMABUF
> * buffers (the exporter will sync the cache for us).
> */
Both are fine for me.
>
> > - if (!sgt || buf->db_attach)
> > + if (!buf->vma)
> > return;
> >
> > dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> > @@ -129,7 +128,7 @@ static void vb2_dc_finish(void *buf_priv)
> > struct sg_table *sgt = buf->dma_sgt;
> >
> > /* DMABUF exporter will flush the cache for us */
> > - if (!sgt || buf->db_attach)
> > + if (!buf->vma)
> > return;
> >
> > dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> > @@ -146,9 +145,9 @@ static void vb2_dc_put(void *buf_priv)
> > if (!atomic_dec_and_test(&buf->refcount))
> > return;
> >
> > - if (buf->sgt_base) {
> > - 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_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr);
> > put_device(buf->dev);
> > @@ -252,13 +251,13 @@ static int vb2_dc_dmabuf_ops_attach(struct dma_buf
> > *dbuf, struct device *dev, /* Copy the buf->base_sgt scatter list to the
> > attachment, as we can't * map the same scatter list to multiple attachments
> > at the same time. */
> > - ret = sg_alloc_table(sgt, buf->sgt_base->orig_nents, GFP_KERNEL);
> > + ret = sg_alloc_table(sgt, buf->dma_sgt->orig_nents, GFP_KERNEL);
> > if (ret) {
> > kfree(attach);
> > return -ENOMEM;
> > }
> >
> > - rd = buf->sgt_base->sgl;
> > + rd = buf->dma_sgt->sgl;
> > wr = sgt->sgl;
> > for (i = 0; i < sgt->orig_nents; ++i) {
> > sg_set_page(wr, sg_page(rd), rd->length, rd->offset);
> > @@ -409,10 +408,10 @@ static struct dma_buf *vb2_dc_get_dmabuf(void
> > *buf_priv, unsigned long flags) exp_info.flags = flags;
> > exp_info.priv = buf;
> >
> > - if (!buf->sgt_base)
> > - buf->sgt_base = vb2_dc_get_base_sgt(buf);
> > + if (!buf->dma_sgt)
> > + buf->dma_sgt = vb2_dc_get_base_sgt(buf);
> >
> > - if (WARN_ON(!buf->sgt_base))
> > + if (WARN_ON(!buf->dma_sgt))
> > return NULL;
> >
> > dbuf = dma_buf_export(&exp_info);
>
--
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
