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

[Jacopo Mondi]

Hi Laurent

On Sun, Oct 02, 2022 at 03:36:08AM +0300, Laurent Pinchart via libcamera-devel 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 really seems something that should be improved on the kernel
side. If I got it right, bytesused == 0 was used by old codec drivers
to signal end of streaming, and a flag has been added to signal to
videobuf2 that bytesused == 0 is "fine" and maintain compatibility
with such drivers.

I wonder if instead we should have a flag to signal to videobuf that
it should care about bytesused for OUTPUT buffers, so that such old
codec drivers can continue using the == 0 trick, and ignore it for all
other drivers and use the buffer/plane length.

I've cc-ed Hans to know his opinion here..

>
> This is triggered by the RkISP1 and IPU3 pipeline handlers, which don't
> set bytesused for the parameters buffers.
>
> There are multiple ways to fix this issue, and as I couldn't decide
> which one is best, I've included two in this series.
>
> Patch 1/4 is the simple fix, it sets bytesused to the buffer or plane
> length before calling VIDIOC_QBUF, essentially replicating in userspace
> the kernel behaviour that videobuf2 considers deprecated. It may however
> not be considered right, one may argue that the user of the
> V4L2VideoDevice should always set the bytesused values correctly.
>

Since you've already gone for the full solution in patches 2, 3 and
4...

> Patches 2/4 to 4/4 are an attempt at setting bytesused correctly. 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 2/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 3/4 and 4/4 then use that
> accessor to set the bytesused values in the RkISP1 and IPU3 pipeline
> handlers.

... I don't see why we should instead go for the simple fix if we
expect it to fall short as soon as we have variable lenght devices.
Unless of course, we need a short term fix and it's possible to fix it
on the kernel side...

>
> One nice side effect of patch 2/4 is that the V4L2VideoDevice class
> doesn't need to be listed as a friend of the FrameBuffer class anymore.
> If 1/4 is considered a good fix, I would thus be tempted to still merge
> 2/4 just to drop the friend statement.
>
> 3/4 and 4/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.
>
> Note also that, if a future device requires variable-size buffers, the
> approach in patch 1/4 won't work anymore, the more complex fix will then
> be required.
>
> Laurent Pinchart (4):
>   libcamera: v4l2_videodevice: Ensure non-zero bytesused for output
>     buffers
>   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
>
>  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            | 30 +++++-----
>  8 files changed, 85 insertions(+), 57 deletions(-)
>
>
> base-commit: 79f0fc937d95cbf1bd39f04dfd8b83206bda5098
> --
> Regards,
>
> Laurent Pinchart
>