Hi Sakari,
Some comments inline.
On Mon, May 8, 2017 at 11:03 PM, Sakari Ailus sakari.ailus@linux.intel.com 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.
MMAP buffers do not need cache flushing since they have been allocated using dma_alloc_coherent().
Signed-off-by: Sakari Ailus sakari.ailus@linux.intel.com Acked-by: Hans Verkuil hans.verkuil@cisco.com
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 a8a46a8..ddbbcf0 100644 --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c @@ -31,12 +31,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; refcount_t refcount;struct sg_table *sgt_base;
/* USERPTR related */struct frame_vector *vec; /* DMABUF related */ struct dma_buf_attachment *db_attach;@@ -96,7 +97,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)
While at it, can we change the comment above to actually refer to what this condition is checking? Maybe it's just me, but it's very confusing, as the condition is actually (!USERPTR), while the comment mentions DMABUF alone and not even mentioning about MMAP. Maybe we could have something like this:
/* * Only USERPTR needs cache maintenance. DMABUF exporter will flush * the cache for us, while MMAP buffers are coherent by design. */
I guess it could be done as a separate patch after this series, especially considering the message might actually change, since we are going to allow cached MMAP buffers.
return; dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents,@@ -109,7 +110,7 @@ static void vb2_dc_finish(void *buf_priv) struct sg_table *sgt = buf->dma_sgt;
/* DMABUF exporter will flush the cache for us */
Here too.
Best regards, Tomasz