| Message ID | 20240603152548.107158-1-jacopo.mondi@ideasonboard.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: videobuf2: Allow driver to override vb2_queue.buf_ops | expand |
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 >