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.
