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

[Hans de Goede]

Hi,

On 5/1/24 2:09 PM, Naushir Patuck wrote:
> Hi all,
> 
> On Wed, 1 May 2024 at 12:58, Hans de Goede <hdegoede at redhat.com> wrote:
>>
>> Hi Laurent,
>>
>> On 5/1/24 12:11 AM, Laurent Pinchart wrote:
>>> Hi Hans,
>>>
>>> Thank you for the patch.
>>>
>>> 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?

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.

Regards,

Hans





>>>> 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);
>>>>      }
>>>
>>
>