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

[Hans de Goede]

Hi,

On 7/8/24 1:19 AM, Kieran Bingham wrote:
> 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?

Yes I plan to work on this this week.

Regards,

Hans