[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/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 ?

Regards,

Hans