[PATCH resend 1/2] libcamera: v4l2_videodevice: Fix devices which set both V4L2_CAP_VIDEO and V4L2_CAP_META
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jul 4 12:37:09 CEST 2024
On Thu, Jul 04, 2024 at 09:29:31AM +0100, Kieran Bingham wrote:
> Hi Hans,
>
> So the IPU6 enablement in the simple pipeline handler is straight
> forward ... lets see what we can do here.
>
> Quoting Hans de Goede (2024-07-03 16:09:22)
> > The /dev/video# nodes of the IPU6 driver set both V4L2_CAP_VIDEO_CAPTURE
> > and V4L2_CAP_META_CAPTURE.
>
> Is this a driver that is already upstream?
It's the ISYS (input subsystem, CSI-2 RX) driver, it will be in v6.10.
The PSYS (processing subsystem, ISP) driver isn't anywhere close to
mainline yet.
> Is it really valid to set both of those in the V4L2 spec?
>
> If so - then we have to do something here... if not - what control do we
> have to fix the current driver?
It's not forbidden at least. The ISYS has multiple DMA engines connected
to each CSI-2 RX, and they're not dedicated to image data or embedded
data. Streams are filtered based on VC/DT, and routed to those generic
DMA engines.
It's ugly though. The driver reports both capabilities, it maintains
separate image and metadata formats that can be accessed through ioctl
handlers, and selects the effective vb2 queue type when VIDIOC_REQBUFS
is called.
> > This was resulting in V4L2VideoDevice::getFormat() / tryFormat() /
> > setFormat() calling the metadata related ioctls instead of the videocap
> > ioctls causing things to not work.
> >
> > Fix this by making V4L2Capability::isMeta() return false when the device
> > also has one of the video capabilities.
> >
> > Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> > ---
> > include/libcamera/internal/v4l2_videodevice.h | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > index 9057be08..fdd877c8 100644
> > --- a/include/libcamera/internal/v4l2_videodevice.h
> > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > @@ -92,6 +92,10 @@ struct V4L2Capability final : v4l2_capability {
> > }
> > bool isMeta() const
> > {
> > + /* Treat devs which combine video and meta as video not meta */
>
> If this is a workaround, and really not expected I would want more
> detail here. Especially if this is a short term workaround for any
> possibly out of tree or staging drivers (I haven't checked where IPU6
> state is currently), including a \todo: to remove it in the future
> when the driver is fixed/upstreamed.
>
> Howevfer if it's simply entirely within the rights of a v4l2 driver to
> set both - then I think the comment would stand and be valid on it's
> own, but some how we'd have to stay aware or document somewhere else in
> the doxygen for this class that we directly treat devices as video
> devices if they report both video and metadata capture.
That won't be enough as soon as we need to capture embedded data from
the ISYS. We need to extend the V4L2 helper classes to allow selecting
the type, among the supported types. I fear this mode of operation,
while supported by the kernel, has seen little testing beyond
applications that are highly specific to particular devices, so I expect
rough edges.
> > + if (isVideo())
> > + return false;
> > +
> > return device_caps() & (V4L2_CAP_META_CAPTURE |
> > V4L2_CAP_META_OUTPUT);
> > }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list