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

[Laurent Pinchart]

On Wed, May 01, 2024 at 02:47:11PM +0100, Naushir Patuck wrote:
> On Wed, 1 May 2024 at 14:38, Hans de Goede wrote:
> > On 5/1/24 2:09 PM, Naushir Patuck wrote:
> > > On Wed, 1 May 2024 at 12:58, Hans de Goede wrote:
> > >> On 5/1/24 12:11 AM, Laurent Pinchart wrote:
> > >>> On Tue, Apr 30, 2024 at 07:17:27PM +0200, Hans de Goede wrote:
> > >>>> The /dev/video# nodes of the IPU6 driver set both V4L2_CAP_VIDEO_CAPTURE
> > >>>> and V4L2_CAP_META_CAPTURE.
> > >>>>
> > >>>> 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.
> > >>>
> > >>> This seems to be a quick hack. What would it take to solve the issue
> > >>> correctly ?
> > >>
> > >> I honestly don't know. Since the whole metadata stuff is all new
> > >> and somewhat in flux I think properly fixing this is best
> > >> delayed until we actually figure out what we want to do here /
> > >> how this all is going to work.
> > >
> > > The Pi 5 CFE driver also ran into this issue as we had a dev node that
> > > advertised both meta and video caps.  I briefly looked into a fix, but
> > > quickly came to the conclusion that it might need a larger rework job
> > > than I was able to do at the time.  Instead we fixed the dev node to
> > > only advertise either meta or video cap type, not both.
> > >
> > > IIRC, a change would be needed to look at the pixel format set into
> > > the V4L2VideoDevice object, decide if it is a meta or video format,
> > > and then make the appropriate IOCTL calls after that.
> >
> > Interesting. So if I understand things correctly then on the Pi 5
> > you no longer have nodes which advertise metadata + video support on
> > a single /dev/video# node, right ?  IOW this workaround patch should not
> > cause any issues for the Pi 5?
> 
> That's correct, and this patch will not have a negative effect for Pi
> 5.  But I'll leave it up to libcamera maintainers to decide if this
> workaround can/should be merged.

I don't think this patch will have a negative effect on any platform we
support today, but I'm always a bit uncomfortable when we pile up
technical debt. I'd like to see more volunteers to implement things
correctly :-)

> > Assuming I have that right I still think just going with this workaround
> > patch for now (so as a temporary solution, until things are more clear)
> > is best.
> >
> > >>>> 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 d157a447..5816c18d 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 */
> > >>>
> > >>> s/devs/devices/
> > >>> s/meta/metadata/g
> > >>>
> > >>>> +            if (isVideo())
> > >>>> +                    return false;
> > >>>> +
> > >>>>              return device_caps() & (V4L2_CAP_META_CAPTURE |
> > >>>>                                      V4L2_CAP_META_OUTPUT);
> > >>>>      }

-- 
Regards,

Laurent Pinchart