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

[Kieran Bingham]

Quoting Hans de Goede (2024-07-04 12:03:58)
> Hi,
> 
> On 7/4/24 12:37 PM, Laurent Pinchart 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.
> > 
> >> 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.
> 
> Hmm, I see that we already default to video-capture for these /dev/video
> nodes in V4L2VideoDevice::open() :
> 
>         if (caps_.isVideoCapture()) {
>                 notifierType = EventNotifier::Read;
>                 bufferType_ = caps_.isMultiplanar()
>                             ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
>                             : V4L2_BUF_TYPE_VIDEO_CAPTURE;
>         } else if (caps_.isVideoOutput()) {
>                 notifierType = EventNotifier::Write;
>                 bufferType_ = caps_.isMultiplanar()
>                             ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
>                             : V4L2_BUF_TYPE_VIDEO_OUTPUT;
>         } else if (caps_.isMetaCapture()) {
>                 notifierType = EventNotifier::Read;
>                 bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
>         } else if (caps_.isMetaOutput()) {
>                 notifierType = EventNotifier::Write;
>                 bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
>         } else {
>                 ...
> 
> So if we change other functions to check for the bufferType_
> instead of re-calling isMeta() there then later on open()
> could get a type argument to override the default order
> from above (when set) and to select meta capture on nodes
> which support that instead.
> 
> So what I'm proposing is to e.g. change getFormat() from:
> 
>         if (caps_.isMeta())
>                 return getFormatMeta(format);
>         else if (caps_.isMultiplanar())
>                 return getFormatMultiplane(format);
>         else
>                 return getFormatSingleplane(format);
> 
> to:
> 
>         if (bufferType_ == V4L2_BUF_TYPE_META_CAPTURE || bufferType_ == V4L2_BUF_TYPE_META_OUTPUT)
>                 return getFormatMeta(format);
>         else if (caps_.isMultiplanar())
>                 return getFormatMultiplane(format);
>         else
>                 return getFormatSingleplane(format);
> 
> And probable add macros to mirror V4L2_TYPE_IS_OUTPUT so this becomes:
> 
>         if (V4L2_TYPE_IS_META(bufferType_))
>                 return getFormatMeta(format);
>         else if (V4L2_TYPE_IS_MULTI_PLANAR(bufferType_))
>                 return getFormatMultiplane(format);
>         else
>                 return getFormatSingleplane(format);
> 
> And the same for e.g. setFormat() and tryFormat()
> 
> This would make the bufferType selection in V4L2VideoDevice::open()
> the sole source of truth for which type of, well, buffers this
> V4L2VideoDevice instance is dealing with.
> 
> Note that e.g. getFormatXxxplane() already rely on
> bufferType_:
> 
> int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
> {
>         struct v4l2_format v4l2Format = {};
>         struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;
>         int ret;
> 
>         v4l2Format.type = bufferType_;
>         ...
> 
> So the suggest change to the getFormat() wrapper around
> the getFormatXxx() methods simply makes using bufferType_ as
> *the* way to determine things consistent.
> 
> Laurent, I think the change suggested above will be more to your
> liking, right ?


That all sounds pretty justifiable to me ....

Is it something you can / plan to tackle?
--
Kieran

> 
> Regards,
> 
> Hans
> 
>