Re: [PATCHv5 8/8] videobuf2: handle non-contiguous DMA allocations

[Sergey Senozhatsky <senozhatsky@chromium.org>]

Hi Tomasz,

On (21/08/30 22:12), Tomasz Figa wrote:
[..]
> > +	/* This takes care of DMABUF and user-enforced cache sync hint */
> >  	if (buf->vb->skip_cache_sync_on_prepare)
> >  		return;
> >  
> > +	/*
> > +	 * Coherent MMAP buffers do not need to be synced, unlike USERPTR
> > +	 * and non-coherent MMAP buffers.
> > +	 */
> > +	if (buf->vb->memory == V4L2_MEMORY_MMAP && !buf->non_coherent_mem)
> > +		return;
> 
> nit: Would it make sense to also set buf->non_coherent_mem to 1 in
> vb2_dc_get_userptr() and simplify this check?

Sounds good. Done.

> > +
> >  	if (!sgt)
> 
> Is there a case when this would be true at this point?

We always set ->dma_sgt only for non-coherent buffers at allocation time.

The rest (including this if-condition) is form the upstream code.
Why was it added there. For USERPTR and coherent MMAP?

> >  		return;
> >  
> > +	/* For both USERPTR and non-coherent MMAP */
> >  	dma_sync_sgtable_for_device(buf->dev, sgt, buf->dma_dir);
> > +
> > +	/* Non-coherent MMAP only */
> > +	if (buf->non_coherent_mem && buf->vaddr)
> 
> Then this could check only for buf->vaddr.
> 
> > +		flush_kernel_vmap_range(buf->vaddr, buf->size);
> >  }
> >  
> >  static void vb2_dc_finish(void *buf_priv)
> > @@ -115,13 +152,26 @@ static void vb2_dc_finish(void *buf_priv)
> 
> Same comments as for _prepare.

Done.


> > +static int vb2_dc_alloc_non_coherent(struct vb2_dc_buf *buf)
> > +{
> > +	struct vb2_queue *q = buf->vb->vb2_queue;
> > +
> > +	buf->dma_sgt = dma_alloc_noncontiguous(buf->dev,
> > +					       buf->size,
> > +					       buf->dma_dir,
> > +					       GFP_KERNEL | q->gfp_flags,
> > +					       buf->attrs);
> > +	if (!buf->dma_sgt)
> > +		return -ENOMEM;
> > +
> > +	buf->dma_addr = sg_dma_address(buf->dma_sgt->sgl);
> > +
> > +	/*
> > +	 * For requests that need kernel mapping (DMA_ATTR_NO_KERNEL_MAPPING
> > +	 * bit is cleared) we perform dma_vmap_noncontiguous() in vb2_dc_vaddr()
> > +	 */
> 
> Current code now ignores the attribute, so this comment isn't entirely
> accurate. Maybe it's better to remove the mention of the attribute and
> instead say that for non_coherent buffers the kernel mapping is created on
> demand?

Done.

> >  static int vb2_dc_dmabuf_ops_vmap(struct dma_buf *dbuf, struct dma_buf_map *map)
> >  {
> > -	struct vb2_dc_buf *buf = dbuf->priv;
> > +	struct vb2_dc_buf *buf;
> > +	void *vaddr;
> > +
> > +	buf = dbuf->priv;
> > +	vaddr = vb2_dc_vaddr(buf->vb, buf);
> > +	if (!vaddr)
> > +		return -EINVAL;
> >  
> > -	dma_buf_map_set_vaddr(map, buf->vaddr);
> > +	dma_buf_map_set_vaddr(map, vaddr);
> >  
> >  	return 0;
> >  }
> > @@ -390,6 +499,9 @@ static struct sg_table *vb2_dc_get_base_sgt(struct vb2_dc_buf *buf)
> >  	int ret;
> >  	struct sg_table *sgt;
> >  
> > +	if (buf->non_coherent_mem)
> > +		return buf->dma_sgt;
> 
> Wouldn't this lead to a double free in vb2_dc_put()?

Most likely. Done, thank you.