[libcamera-devel,v2,0/4] libcamera: Fix kernel deprecation warning with output buffers

17523 mbox series

Hello,

This patch series fixes a warning printed by videobuf2.

The V4L2 API specification indicates that, for an output video device,
the driver will interpret a zero bytesused value as the buffer length
(for v4l2_buffer) or the plane length (for v4l2_plane). The videobuf2
framework implements this behaviour, but also considers this case as
deprecated and prints a warning:

[   54.375534] use of bytesused == 0 is deprecated and will be removed in the future,
[   54.388026] use the actual size instead.

This is triggered by the RkISP1 and IPU3 pipeline handlers, which don't
set bytesused for the parameters buffers.

As the FrameBuffer metadata is private and only accessible in const form
outside of the class (with the exception of V4L2VideoDevice that is
listed as a friend), patch 1/4 first makes mutable access to the
metadata possible. In order to avoid exposing mutable access to
applications, it moves the metadata (and the other remaining private
members of the FrameBuffer class) to the FrameBuffer::Private class, and
exposes a mutable accessor there.

Patches 2/4 and 3/4 then use this new metadata accessor to set the
bytesused values in the RkISP1 and IPU3 pipeline handlers. Finally,
patch 4/4 prints a warning message in V4L2VideoDevice::queueBuffer() to
catch callers that still use the deprecated API.

One nice side effect of patch 1/4 is that the V4L2VideoDevice class
doesn't need to be listed as a friend of the FrameBuffer class anymore.

2/4 and 3/4 currently hardcode the bytesused value to the size of the
parameters buffer structure. This works fine with the RkISP1 and IPU3
pipeline handlers as the parameters buffers have a fixed size, but other
devices in the future may use a variable-size buffer which would require
the IPA module to pass the data size to the pipeline handler. I was
tempted to do the same for RkISP1 and IPU3 for correctness, but decided
it was probably not worth it.

As far as I can tell, the Raspberry Pi pipeline handler doesn't need any
change. Naush, would you be able to test this series to confirm that it
doesn't introduce any regression for you ?

Laurent Pinchart (4):
  libcamera: framebuffer: Move remaining private data to Private class
  pipeline: ipu3: Set bytesused before queuing parameters buffer
  pipeline: rkisp1: Set bytesused before queuing parameters buffer
  libcamera: v4l2_videodevice: Warn if bytesused == 0 when queuing
    output buffer

 include/libcamera/framebuffer.h               | 19 ++----
 include/libcamera/internal/framebuffer.h      | 10 +++-
 .../mm/cros_frame_buffer_allocator.cpp        |  8 +--
 .../mm/generic_frame_buffer_allocator.cpp     |  9 +--
 src/libcamera/framebuffer.cpp                 | 58 ++++++++++++-------
 src/libcamera/pipeline/ipu3/ipu3.cpp          |  4 ++
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  4 ++
 src/libcamera/v4l2_videodevice.cpp            | 25 +++++---
 8 files changed, 84 insertions(+), 53 deletions(-)


base-commit: 12f4708e35cde15ff9607d59eb053ece1b2bd081

Hi Laurent,

On Tue, 4 Oct 2022 at 23:29, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hello,
>
> This patch series fixes a warning printed by videobuf2.
>
> The V4L2 API specification indicates that, for an output video device,
> the driver will interpret a zero bytesused value as the buffer length
> (for v4l2_buffer) or the plane length (for v4l2_plane). The videobuf2
> framework implements this behaviour, but also considers this case as
> deprecated and prints a warning:
>
> [   54.375534] use of bytesused == 0 is deprecated and will be removed in
> the future,
> [   54.388026] use the actual size instead.
>
> This is triggered by the RkISP1 and IPU3 pipeline handlers, which don't
> set bytesused for the parameters buffers.
>
> As the FrameBuffer metadata is private and only accessible in const form
> outside of the class (with the exception of V4L2VideoDevice that is
> listed as a friend), patch 1/4 first makes mutable access to the
> metadata possible. In order to avoid exposing mutable access to
> applications, it moves the metadata (and the other remaining private
> members of the FrameBuffer class) to the FrameBuffer::Private class, and
> exposes a mutable accessor there.
>
> Patches 2/4 and 3/4 then use this new metadata accessor to set the
> bytesused values in the RkISP1 and IPU3 pipeline handlers. Finally,
> patch 4/4 prints a warning message in V4L2VideoDevice::queueBuffer() to
> catch callers that still use the deprecated API.
>
> One nice side effect of patch 1/4 is that the V4L2VideoDevice class
> doesn't need to be listed as a friend of the FrameBuffer class anymore.
>
> 2/4 and 3/4 currently hardcode the bytesused value to the size of the
> parameters buffer structure. This works fine with the RkISP1 and IPU3
> pipeline handlers as the parameters buffers have a fixed size, but other
> devices in the future may use a variable-size buffer which would require
> the IPA module to pass the data size to the pipeline handler. I was
> tempted to do the same for RkISP1 and IPU3 for correctness, but decided
> it was probably not worth it.
>
> As far as I can tell, the Raspberry Pi pipeline handler doesn't need any
> change. Naush, would you be able to test this series to confirm that it
> doesn't introduce any regression for you ?
>

Done some testing on this series, and I cannot see any obvious regressions
for our platform.  So...

Tested-by: Naushir Patuck <naush@raspberrypi.com>



> Laurent Pinchart (4):
>   libcamera: framebuffer: Move remaining private data to Private class
>   pipeline: ipu3: Set bytesused before queuing parameters buffer
>   pipeline: rkisp1: Set bytesused before queuing parameters buffer
>   libcamera: v4l2_videodevice: Warn if bytesused == 0 when queuing
>     output buffer
>
>  include/libcamera/framebuffer.h               | 19 ++----
>  include/libcamera/internal/framebuffer.h      | 10 +++-
>  .../mm/cros_frame_buffer_allocator.cpp        |  8 +--
>  .../mm/generic_frame_buffer_allocator.cpp     |  9 +--
>  src/libcamera/framebuffer.cpp                 | 58 ++++++++++++-------
>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  4 ++
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  4 ++
>  src/libcamera/v4l2_videodevice.cpp            | 25 +++++---
>  8 files changed, 84 insertions(+), 53 deletions(-)
>
>
> base-commit: 12f4708e35cde15ff9607d59eb053ece1b2bd081
> --
> Regards,
>
> Laurent Pinchart
>
>