Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210427131344.139443-1-senozhatsky@chromium.org>
 <20210427131344.139443-9-senozhatsky@chromium.org> <10a0903a-e295-5cba-683a-1eb89a0804ed@xs4all.nl>
 <YMsAIVs7G2hUDR2F@google.com> <YNVJJhP69KPJ+DHv@google.com>
In-Reply-To: <YNVJJhP69KPJ+DHv@google.com>
From:   Tomasz Figa <tfiga@chromium.org>
Date:   Wed, 7 Jul 2021 21:48:49 +0900
Message-ID: <CAAFQd5BB6JghdgGf9SjAWYuZFsZaAeU11rV1a1xrwws=w7j7_w@mail.gmail.com>
Subject: Re: [PATCHv2 8/8] videobuf2: handle non-contiguous DMA allocations
To:     Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Sergey Senozhatsky <senozhatsky@chromium.org>
Cc:     Ricardo Ribalda <ribalda@chromium.org>,
        Christoph Hellwig <hch@lst.de>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Jun 25, 2021 at 12:10 PM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> Hi Hans,
>
> On (21/06/17 16:56), Sergey Senozhatsky wrote:
> [..]
> > static void *vb2_dc_vaddr(struct vb2_buffer *vb, void *buf_priv)
> > {
> >         struct vb2_dc_buf *buf = buf_priv;
> >
> >         if (buf->vaddr)
> >                 return buf->vaddr;
> >
> >         if (buf->db_attach) {
> >                 struct dma_buf_map map;
> >
> >                 if (!dma_buf_vmap(buf->db_attach->dmabuf, &map))
> >                         buf->vaddr = map.vaddr;
> >
> >                 return buf->vaddr;
> >         }
> >
> >         if (!buf->coherent_mem)
> >                 buf->vaddr = dma_vmap_noncontiguous(buf->dev, buf->size,
> >                                                     buf->dma_sgt);
> >         return buf->vaddr;
> > }
> >
> > And in vb2_dc_alloc functions set vaddr for !DMA_ATTR_NO_KERNEL_MAPPING
> > in both coherent and non-coherent. So that we probably can have less
> > branches when ->vaddr is NULL for one type of allocations, and is not
> > NULL for another.

I'd prefer if it stayed as is. This opportunistic mapping as in the
current revision is quite nice, because most of the drivers don't
bother to set DMA_ATTR_NO_KERNEL_MAPPING even if they don't need the
kernel mapping. Also, even if the driver itself doesn't need the
kernel mapping, we can still create one on demand if the DMA-buf
importer demands it from us.

> >
> > static int vb2_dc_alloc_coherent(struct vb2_dc_buf *buf)
> > {
> >         struct vb2_queue *q = buf->vb->vb2_queue;
> >
> >         buf->cookie = dma_alloc_attrs(buf->dev,
> >                                       buf->size,
> >                                       &buf->dma_addr,
> >                                       GFP_KERNEL | q->gfp_flags,
> >                                       buf->attrs);
> >         if (!buf->cookie)
> >                 return -ENOMEM;
> >
> >         if (q->dma_attrs & DMA_ATTR_NO_KERNEL_MAPPING)
> >                 return 0;
> >
> >         buf->vaddr = buf->cookie;
> >         return 0;
> > }
> >
> > 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;
> >
> >         if (q->dma_attrs & DMA_ATTR_NO_KERNEL_MAPPING)
> >                 return 0;
> >
> >         buf->vaddr = dma_vmap_noncontiguous(buf->dev, buf->size, buf->dma_sgt);
> >         if (!buf->vaddr) {
> >                 dma_free_noncontiguous(buf->dev, buf->size,
> >                                        buf->dma_sgt, buf->dma_addr);
> >                 return -ENOMEM;
> >         }
> >         return 0;
> > }
>
> I guess this should address the case when
>
> "after allocating the buffer, the buffer is exported as a dma_buf and
> another device calls dma_buf_ops vb2_dc_dmabuf_ops_vmap, which in turn
> calls dma_buf_map_set_vaddr(map, buf->vaddr); with a NULL buf->vaddr"

Sorry, I fail to get what this is about. Where does this quote come from?

>
> Because ->vaddr will not be NULL now after allocation for both coherent
> and non-coherent buffers (modulo DMA_ATTR_NO_KERNEL_MAPPING requests).
>
> What do you think?

Hans, any feedback on this? Thanks.

Best regards,
Tomasz