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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Oct 03, 2022 at 09:55:52AM +0200, Jacopo Mondi wrote:
> 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..

I've also raised the issue on the linux-media mailing list:
https://lore.kernel.org/linux-media/YzjR5ajfLfMXvC4D@pendragon.ideasonboard.com/

Nonetheless, even if we drop the warning on the kernel side, I think
libcamera should pass the correct value in bytesused, in order to avoid
triggering the warning on older kernels if nothing else.

> > 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...

Works for me :-) I'll then merge patches 2/4 to 4/4 once they get
reviewed.

> > 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