Re: [PATCH 03/14] media: videobuf2: Allow exporting of a struct dmabuf

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Dave Stevenson <dave.stevenson@raspberrypi.com>
Cc: Umang Jain <umang.jain@ideasonboard.com>,
	linux-media@vger.kernel.org, kernel-list@raspberrypi.com,
	linux-kernel@vger.kernel.org,
	linux-rpi-kernel@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-staging@lists.linux.dev,
	Broadcom internal kernel review list
	<bcm-kernel-feedback-list@broadcom.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Naushir Patuck <naush@raspberrypi.com>,
	David Plowman <david.plowman@raspberrypi.com>,
	Kieran Bingham <kieran.bingham@ideasonboard.com>,
	Dave Stevenson <dave.stevenson@raspberrypi.org>
Subject: Re: [PATCH 03/14] media: videobuf2: Allow exporting of a struct dmabuf
Date: Fri, 25 Nov 2022 03:49:52 +0200	[thread overview]
Message-ID: <Y4AfQLE7eh2gLZJl@pendragon.ideasonboard.com> (raw)
In-Reply-To: <CAPY8ntAtbnUqhJRd7+bWLRUz+K=1Hnd33XwSR930hPVfc7EPPg@mail.gmail.com>

Hi Dave,

On Tue, Nov 22, 2022 at 11:35:31AM +0000, Dave Stevenson wrote:
> On Mon, 21 Nov 2022 at 23:18, Laurent Pinchart wrote:
> > On Tue, Nov 22, 2022 at 03:17:11AM +0530, Umang Jain wrote:
> > > From: Dave Stevenson <dave.stevenson@raspberrypi.org>
> > >
> > > videobuf2 only allowed exporting a dmabuf as a file descriptor,
> > > but there are instances where having the struct dma_buf is
> > > useful within the kernel.
> > >
> > > Split the current implementation into two, one step which
> > > exports a struct dma_buf, and the second which converts that
> > > into an fd.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > ---
> > >  .../media/common/videobuf2/videobuf2-core.c   | 36 +++++++++++++------
> > >  include/media/videobuf2-core.h                | 15 ++++++++
> > >  2 files changed, 40 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > > index ab9697f3b5f1..32b26737cac4 100644
> > > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > > @@ -2184,49 +2184,49 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off,
> > >       return -EINVAL;
> > >  }
> > >
> > > -int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
> > > -             unsigned int index, unsigned int plane, unsigned int flags)
> > > +struct dma_buf *vb2_core_expbuf_dmabuf(struct vb2_queue *q, unsigned int type,
> > > +                                    unsigned int index, unsigned int plane,
> > > +                                    unsigned int flags)
> >
> > This function is used in the ISP driver, in bcm2835_isp_buf_prepare(),
> > for MMAP buffers, and as far as I can tell, its only purpose is to
> > create a dma_buf instance to then be imported in
> > vchiq_mmal_submit_buffer() with a call to vc_sm_cma_import_dmabuf().
> > That sounds like a very complicated set of operations, and quite
> > inefficient :-(
> 
> Are you saying that dmabufs are not the preferred route for sharing
> buffers between kernel subsystems? What are you suggesting instead?
> 
> If the VPU (firmware) has a handle to the buffer then we need to
> manage the lifetime such that it is not freed until the VPU has
> released it. That is handled for you with dmabufs, therefore why
> reinvent the wheel?

When we go through userspace, dmabuf is certainly the way to go. Here,
we need to share buffer information between two drivers that are
specific to the platform, so we could avoid going through so many layers
by using a custom abstraction. However, that would require additional
development, and probably reinventing the wheel to some extent, so it's
probably not worth it.

I'd like to explore if we could avoid creating a second dmabuf in
vc_sm_cma_import_dmabuf_internal() to make all this a bit more
lightweight. Let's discuss it in replies to patch 01/14.

> > >  {
> > >       struct vb2_buffer *vb = NULL;
> > >       struct vb2_plane *vb_plane;
> > > -     int ret;
> > >       struct dma_buf *dbuf;
> > >
> > >       if (q->memory != VB2_MEMORY_MMAP) {
> > >               dprintk(q, 1, "queue is not currently set up for mmap\n");
> > > -             return -EINVAL;
> > > +             return ERR_PTR(-EINVAL);
> > >       }
> > >
> > >       if (!q->mem_ops->get_dmabuf) {
> > >               dprintk(q, 1, "queue does not support DMA buffer exporting\n");
> > > -             return -EINVAL;
> > > +             return ERR_PTR(-EINVAL);
> > >       }
> > >
> > >       if (flags & ~(O_CLOEXEC | O_ACCMODE)) {
> > >               dprintk(q, 1, "queue does support only O_CLOEXEC and access mode flags\n");
> > > -             return -EINVAL;
> > > +             return ERR_PTR(-EINVAL);
> > >       }
> > >
> > >       if (type != q->type) {
> > >               dprintk(q, 1, "invalid buffer type\n");
> > > -             return -EINVAL;
> > > +             return ERR_PTR(-EINVAL);
> > >       }
> > >
> > >       if (index >= q->num_buffers) {
> > >               dprintk(q, 1, "buffer index out of range\n");
> > > -             return -EINVAL;
> > > +             return ERR_PTR(-EINVAL);
> > >       }
> > >
> > >       vb = q->bufs[index];
> > >
> > >       if (plane >= vb->num_planes) {
> > >               dprintk(q, 1, "buffer plane out of range\n");
> > > -             return -EINVAL;
> > > +             return ERR_PTR(-EINVAL);
> > >       }
> > >
> > >       if (vb2_fileio_is_active(q)) {
> > >               dprintk(q, 1, "expbuf: file io in progress\n");
> > > -             return -EBUSY;
> > > +             return ERR_PTR(-EBUSY);
> > >       }
> > >
> > >       vb_plane = &vb->planes[plane];
> > > @@ -2238,9 +2238,23 @@ int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
> > >       if (IS_ERR_OR_NULL(dbuf)) {
> > >               dprintk(q, 1, "failed to export buffer %d, plane %d\n",
> > >                       index, plane);
> > > -             return -EINVAL;
> > > +             return ERR_PTR(-EINVAL);
> > >       }
> > >
> > > +     return dbuf;
> > > +}
> > > +EXPORT_SYMBOL_GPL(vb2_core_expbuf_dmabuf);
> > > +
> > > +int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
> > > +                 unsigned int index, unsigned int plane, unsigned int flags)
> > > +{
> > > +     struct dma_buf *dbuf;
> > > +     int ret;
> > > +
> > > +     dbuf = vb2_core_expbuf_dmabuf(q, type, index, plane, flags);
> > > +     if (IS_ERR(dbuf))
> > > +             return PTR_ERR(dbuf);
> > > +
> > >       ret = dma_buf_fd(dbuf, flags & ~O_ACCMODE);
> > >       if (ret < 0) {
> > >               dprintk(q, 3, "buffer %d, plane %d failed to export (%d)\n",
> > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> > > index 3253bd2f6fee..33629ed2b64f 100644
> > > --- a/include/media/videobuf2-core.h
> > > +++ b/include/media/videobuf2-core.h
> > > @@ -911,6 +911,21 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type);
> > >   */
> > >  int vb2_core_streamoff(struct vb2_queue *q, unsigned int type);
> > >
> > > +/**
> > > + * vb2_core_expbuf_dmabuf() - Export a buffer as a dma_buf structure
> > > + * @q:         videobuf2 queue
> > > + * @type:      buffer type
> > > + * @index:     id number of the buffer
> > > + * @plane:     index of the plane to be exported, 0 for single plane queues
> > > + * @flags:     flags for newly created file, currently only O_CLOEXEC is
> > > + *             supported, refer to manual of open syscall for more details
> > > + *
> > > + * Return: Returns the dmabuf pointer
> > > + */
> > > +struct dma_buf *vb2_core_expbuf_dmabuf(struct vb2_queue *q, unsigned int type,
> > > +                                    unsigned int index, unsigned int plane,
> > > +                                    unsigned int flags);
> > > +
> > >  /**
> > >   * vb2_core_expbuf() - Export a buffer as a file descriptor.
> > >   * @q:               pointer to &struct vb2_queue with videobuf2 queue.

-- 
Regards,

Laurent Pinchart