[PATCH resend 1/2] libcamera: v4l2_videodevice: Fix devices which set both V4L2_CAP_VIDEO and V4L2_CAP_META

[Naushir Patuck]

On Thu, 4 Jul 2024 at 11:37, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.

The original proposal for the Pi5 CFE driver also did this, but we
reverted back to a single queue type per-node because of the ambiguity
introduced in the V4L2VideoDevice class.

Naush

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