| Message ID | 20221002003612.13603-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
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 >
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
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. 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. 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. 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