[RFC,0/3] media: videobuf2: Allow driver to override vb2_queue.buf_ops

  Hello

  I have the need to allocate a scratch buffer to mirror the content of the
vb2_buffer planes (more detail on this on request).

The allocation of such 'scratch' buffer should ideally be done once, at buffer
creation time (and released at buffer release time ?)

Looking at the videobuf2 framework implementation I noticed that the ideal entry
point for this would be vb2_queue.buf_ops.init_buffer, which is called in the
__vb2_queue_alloc() call path.

I have noticed that the vb2_queue.buf_ops members seems to be there to be made
overridable by drivers, but are instead:

1) unconditionally set by the framework in vb2_queue_init_name()
2) the core helpers are not exported

hence I was wondering if this is the result some half-baked attempt to make
them ovverridable or the possibility of override them was instead deprecated.
As I found no traces of this in the log, I thought it was easier to send an
RFC.

I also checked what other entry points I could have used to allocate backing
memory for a buffer, and I have considered vb2_queue.vb2_ops.buf_init which
is however called in the vb2_req_prepare() call path (I'm not using the request
API atm) or in the VIDIOC_PREPARE_BUF call path, which requires ad-hoc
instrumentation in user space (something I would like to avoid if possibile).

What do you think ?

Jacopo Mondi (3):
  media: videobuf2: WARN if !vb2_queue.buf_ops
  media: Allow drivers to overwrite vb2_queue.buf_ops
  media: rkisp1-params: Override vb2_queue.buf_ops

 .../media/common/videobuf2/videobuf2-core.c   | 12 ++++---
 .../media/common/videobuf2/videobuf2-v4l2.c   | 34 +++++++++++--------
 .../platform/rockchip/rkisp1/rkisp1-params.c  | 18 +++++++++-
 include/media/videobuf2-core.h                |  7 ++++
 include/media/videobuf2-v4l2.h                |  8 +++++
 5 files changed, 60 insertions(+), 19 deletions(-)

--
2.45.1
  

Hi Jacopo,

On Mon, Jun 03, 2024 at 05:25:44PM +0200, Jacopo Mondi wrote:
> Hello
> 
>   I have the need to allocate a scratch buffer to mirror the content of the
> vb2_buffer planes (more detail on this on request).
> 
> The allocation of such 'scratch' buffer should ideally be done once, at buffer
> creation time (and released at buffer release time ?)
>
> Looking at the videobuf2 framework implementation I noticed that the ideal entry
> point for this would be vb2_queue.buf_ops.init_buffer, which is called in the
> __vb2_queue_alloc() call path.
> 
> I have noticed that the vb2_queue.buf_ops members seems to be there to be made
> overridable by drivers, but are instead:
> 
> 1) unconditionally set by the framework in vb2_queue_init_name()
> 2) the core helpers are not exported
> 
> hence I was wondering if this is the result some half-baked attempt to make
> them ovverridable or the possibility of override them was instead deprecated.
> As I found no traces of this in the log, I thought it was easier to send an
> RFC.
> 
> I also checked what other entry points I could have used to allocate backing
> memory for a buffer, and I have considered vb2_queue.vb2_ops.buf_init which
> is however called in the vb2_req_prepare() call path (I'm not using the request
> API atm) or in the VIDIOC_PREPARE_BUF call path, which requires ad-hoc
> instrumentation in user space (something I would like to avoid if possibile).
> 
> What do you think ?

I've been thinking more about this. I wonder if you could use
.buf_init() for your use case. It's called in three places:

- __vb2_queue_alloc()
- __prepare_userptr()
- __prepare_dmabuf()

As your scratch buffer needs are limited to the ISP parameters queue,
which should use MMAP only, I think .buf_init() would be just fine.

> Jacopo Mondi (3):
>   media: videobuf2: WARN if !vb2_queue.buf_ops
>   media: Allow drivers to overwrite vb2_queue.buf_ops
>   media: rkisp1-params: Override vb2_queue.buf_ops
> 
>  .../media/common/videobuf2/videobuf2-core.c   | 12 ++++---
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 34 +++++++++++--------
>  .../platform/rockchip/rkisp1/rkisp1-params.c  | 18 +++++++++-
>  include/media/videobuf2-core.h                |  7 ++++
>  include/media/videobuf2-v4l2.h                |  8 +++++
>  5 files changed, 60 insertions(+), 19 deletions(-)
  

Hi Laurent

On Wed, Jun 05, 2024 at 09:49:09AM GMT, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Jun 03, 2024 at 05:25:44PM +0200, Jacopo Mondi wrote:
> > Hello
> >
> >   I have the need to allocate a scratch buffer to mirror the content of the
> > vb2_buffer planes (more detail on this on request).
> >
> > The allocation of such 'scratch' buffer should ideally be done once, at buffer
> > creation time (and released at buffer release time ?)
> >
> > Looking at the videobuf2 framework implementation I noticed that the ideal entry
> > point for this would be vb2_queue.buf_ops.init_buffer, which is called in the
> > __vb2_queue_alloc() call path.
> >
> > I have noticed that the vb2_queue.buf_ops members seems to be there to be made
> > overridable by drivers, but are instead:
> >
> > 1) unconditionally set by the framework in vb2_queue_init_name()
> > 2) the core helpers are not exported
> >
> > hence I was wondering if this is the result some half-baked attempt to make
> > them ovverridable or the possibility of override them was instead deprecated.
> > As I found no traces of this in the log, I thought it was easier to send an
> > RFC.
> >
> > I also checked what other entry points I could have used to allocate backing
> > memory for a buffer, and I have considered vb2_queue.vb2_ops.buf_init which
> > is however called in the vb2_req_prepare() call path (I'm not using the request
> > API atm) or in the VIDIOC_PREPARE_BUF call path, which requires ad-hoc
> > instrumentation in user space (something I would like to avoid if possibile).
> >
> > What do you think ?
>
> I've been thinking more about this. I wonder if you could use
> .buf_init() for your use case. It's called in three places:
>
> - __vb2_queue_alloc()

This is only called

        if (memory == VB2_MEMORY_MMAP)

and I originally considered it a non viable solution, as it only
supports the MMAP use case. Now that I thought about it a few more
seconds, I realized that MMAP it's the only actual use case where
memory is allocated by the driver and thus the only memory management
method that makes sense to pair with buf_init

> - __prepare_userptr()
> - __prepare_dmabuf()

These, if I'm not mistaken are in VIDIOC_PREPARE_BUF call

>
> As your scratch buffer needs are limited to the ISP parameters queue,
> which should use MMAP only, I think .buf_init() would be just fine.
>

Probably so, I'll give it a go

Thanks!

> > Jacopo Mondi (3):
> >   media: videobuf2: WARN if !vb2_queue.buf_ops
> >   media: Allow drivers to overwrite vb2_queue.buf_ops
> >   media: rkisp1-params: Override vb2_queue.buf_ops
> >
> >  .../media/common/videobuf2/videobuf2-core.c   | 12 ++++---
> >  .../media/common/videobuf2/videobuf2-v4l2.c   | 34 +++++++++++--------
> >  .../platform/rockchip/rkisp1/rkisp1-params.c  | 18 +++++++++-
> >  include/media/videobuf2-core.h                |  7 ++++
> >  include/media/videobuf2-v4l2.h                |  8 +++++
> >  5 files changed, 60 insertions(+), 19 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
  

On Wed, Jun 05, 2024 at 09:37:01AM +0200, Jacopo Mondi wrote:
> Hi Laurent
> 
> On Wed, Jun 05, 2024 at 09:49:09AM GMT, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > On Mon, Jun 03, 2024 at 05:25:44PM +0200, Jacopo Mondi wrote:
> > > Hello
> > >
> > >   I have the need to allocate a scratch buffer to mirror the content of the
> > > vb2_buffer planes (more detail on this on request).
> > >
> > > The allocation of such 'scratch' buffer should ideally be done once, at buffer
> > > creation time (and released at buffer release time ?)
> > >
> > > Looking at the videobuf2 framework implementation I noticed that the ideal entry
> > > point for this would be vb2_queue.buf_ops.init_buffer, which is called in the
> > > __vb2_queue_alloc() call path.
> > >
> > > I have noticed that the vb2_queue.buf_ops members seems to be there to be made
> > > overridable by drivers, but are instead:
> > >
> > > 1) unconditionally set by the framework in vb2_queue_init_name()
> > > 2) the core helpers are not exported
> > >
> > > hence I was wondering if this is the result some half-baked attempt to make
> > > them ovverridable or the possibility of override them was instead deprecated.
> > > As I found no traces of this in the log, I thought it was easier to send an
> > > RFC.
> > >
> > > I also checked what other entry points I could have used to allocate backing
> > > memory for a buffer, and I have considered vb2_queue.vb2_ops.buf_init which
> > > is however called in the vb2_req_prepare() call path (I'm not using the request
> > > API atm) or in the VIDIOC_PREPARE_BUF call path, which requires ad-hoc
> > > instrumentation in user space (something I would like to avoid if possibile).
> > >
> > > What do you think ?
> >
> > I've been thinking more about this. I wonder if you could use
> > .buf_init() for your use case. It's called in three places:
> >
> > - __vb2_queue_alloc()
> 
> This is only called
> 
>         if (memory == VB2_MEMORY_MMAP)
> 
> and I originally considered it a non viable solution, as it only
> supports the MMAP use case. Now that I thought about it a few more
> seconds, I realized that MMAP it's the only actual use case where
> memory is allocated by the driver and thus the only memory management
> method that makes sense to pair with buf_init
> 
> > - __prepare_userptr()
> > - __prepare_dmabuf()
> 
> These, if I'm not mistaken are in VIDIOC_PREPARE_BUF call
> 
> >
> > As your scratch buffer needs are limited to the ISP parameters queue,
> > which should use MMAP only, I think .buf_init() would be just fine.
> >
> 
> Probably so, I'll give it a go

I agree with Laurent that .buf_init() should be a good place for this.
Please let us know if it works for you.

Best regards,
Tomasz

> 
> Thanks!
> 
> > > Jacopo Mondi (3):
> > >   media: videobuf2: WARN if !vb2_queue.buf_ops
> > >   media: Allow drivers to overwrite vb2_queue.buf_ops
> > >   media: rkisp1-params: Override vb2_queue.buf_ops
> > >
> > >  .../media/common/videobuf2/videobuf2-core.c   | 12 ++++---
> > >  .../media/common/videobuf2/videobuf2-v4l2.c   | 34 +++++++++++--------
> > >  .../platform/rockchip/rkisp1/rkisp1-params.c  | 18 +++++++++-
> > >  include/media/videobuf2-core.h                |  7 ++++
> > >  include/media/videobuf2-v4l2.h                |  8 +++++
> > >  5 files changed, 60 insertions(+), 19 deletions(-)
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>