| Message ID | 20161216012425.11179-8-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 15, 2016 at 6:24 PM, Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > From: Sakari Ailus <sakari.ailus@linux.intel.com> > > 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. I think this patch should be split in two. 1. Unifying dma_sgt and sgt_base > > MMAP buffers do not need cache flushing since they have been allocated > using dma_alloc_coherent(). 2. That uses vec to check for checking for no flush needed condition. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > Changes since v1: > > - Test for MMAP or DMABUF type through the vec field instead of the now > gone vma field. What is this gone vma field? Did I miss a patch in the series that makes this change? This check that is changed used dma_sgt and db_attach vma These comments don't agree with the code change. > - Move the vec field to a USERPTR section in struct vb2_dc_buf, where > the vma field was located. > --- > drivers/media/v4l2-core/videobuf2-dma-contig.c | 25 +++++++++++++------------ > 1 file changed, 13 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c > index fb6a177be461..2a00d12ffee2 100644 > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c > @@ -30,12 +30,13 @@ struct vb2_dc_buf { > unsigned long attrs; > enum dma_data_direction dma_dir; > struct sg_table *dma_sgt; > - struct frame_vector *vec; > > /* MMAP related */ > struct vb2_vmarea_handler handler; > atomic_t refcount; > - struct sg_table *sgt_base; > + > + /* USERPTR related */ > + struct frame_vector *vec; > > /* DMABUF related */ > struct dma_buf_attachment *db_attach; > @@ -95,7 +96,7 @@ static void vb2_dc_prepare(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->vec) > return; With the unification dma_sgt is valid for MMAP buffers after vb2_dma_sg_alloc() if dma_sgt is not null, sync happens - the patch description doesn't seem to be in sync with the change. I might be missing something. I think it would help if these two changes are split since they are really separate changes. thanks, -- Shuah > > dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents, > @@ -108,7 +109,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->vec) > return; > > dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir); > @@ -125,9 +126,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_attrs(buf->dev, buf->size, buf->cookie, buf->dma_addr, > buf->attrs); > @@ -239,13 +240,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); > @@ -396,10 +397,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); > -- > Regards, > > Laurent Pinchart > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Shuah, On Mon, Mar 27, 2017 at 04:51:40PM -0600, Shuah Khan wrote: > On Thu, Dec 15, 2016 at 6:24 PM, Laurent Pinchart > <laurent.pinchart+renesas@ideasonboard.com> wrote: > > From: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > 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. > > I think this patch should be split in two. > > 1. Unifying dma_sgt and sgt_base > > > > > MMAP buffers do not need cache flushing since they have been allocated > > using dma_alloc_coherent(). > > 2. That uses vec to check for checking for no flush needed condition. I can split this, sure. A non-NULL vec indicates a USERPTR buffer. Before this patch, non-NULL buf->dma_sgt did the same. > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > Changes since v1: > > > > - Test for MMAP or DMABUF type through the vec field instead of the now > > gone vma field. > > What is this gone vma field? Did I miss a patch in the series that > makes this change? This check that is changed used dma_sgt and > db_attach vma > The field existed on bc0195aad0daa2ad5b0d76cce22b167bc3435590, i.e. v4.2-rc2 from which the earlier version of this patch was rebased from. > These comments don't agree with the code change. > > > - Move the vec field to a USERPTR section in struct vb2_dc_buf, where > > the vma field was located. > > --- > > drivers/media/v4l2-core/videobuf2-dma-contig.c | 25 +++++++++++++------------ > > 1 file changed, 13 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c > > index fb6a177be461..2a00d12ffee2 100644 > > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c > > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c > > @@ -30,12 +30,13 @@ struct vb2_dc_buf { > > unsigned long attrs; > > enum dma_data_direction dma_dir; > > struct sg_table *dma_sgt; > > - struct frame_vector *vec; > > > > /* MMAP related */ > > struct vb2_vmarea_handler handler; > > atomic_t refcount; > > - struct sg_table *sgt_base; > > + > > + /* USERPTR related */ > > + struct frame_vector *vec; > > > > /* DMABUF related */ > > struct dma_buf_attachment *db_attach; > > @@ -95,7 +96,7 @@ static void vb2_dc_prepare(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->vec) > > return; > > With the unification dma_sgt is valid for MMAP buffers after vb2_dma_sg_alloc() > if dma_sgt is not null, sync happens - the patch description doesn't seem to be > in sync with the change. I'm not sure what you're referring to. The condition for sync is changed to use buf->vec instead, i.e. the functionality is not affected.
diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c index fb6a177be461..2a00d12ffee2 100644 --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c @@ -30,12 +30,13 @@ struct vb2_dc_buf { unsigned long attrs; enum dma_data_direction dma_dir; struct sg_table *dma_sgt; - struct frame_vector *vec; /* MMAP related */ struct vb2_vmarea_handler handler; atomic_t refcount; - struct sg_table *sgt_base; + + /* USERPTR related */ + struct frame_vector *vec; /* DMABUF related */ struct dma_buf_attachment *db_attach; @@ -95,7 +96,7 @@ static void vb2_dc_prepare(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->vec) return; dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents, @@ -108,7 +109,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->vec) return; dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir); @@ -125,9 +126,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_attrs(buf->dev, buf->size, buf->cookie, buf->dma_addr, buf->attrs); @@ -239,13 +240,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); @@ -396,10 +397,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);