[resend,1/2] libcamera: v4l2_videodevice: Fix devices which set both V4L2_CAP_VIDEO and V4L2_CAP_META
diff mbox series

Message ID 20240703150923.25994-1-hdegoede@redhat.com
State New
Headers show
Series
  • [resend,1/2] libcamera: v4l2_videodevice: Fix devices which set both V4L2_CAP_VIDEO and V4L2_CAP_META
Related show

Commit Message

Hans de Goede July 3, 2024, 3:09 p.m. UTC
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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 include/libcamera/internal/v4l2_videodevice.h | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Kieran Bingham July 4, 2024, 8:29 a.m. UTC | #1
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?

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?

> 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@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.

--
Kieran

> +               if (isVideo())
> +                       return false;
> +
>                 return device_caps() & (V4L2_CAP_META_CAPTURE |
>                                         V4L2_CAP_META_OUTPUT);
>         }
> -- 
> 2.45.1
>
Laurent Pinchart July 4, 2024, 10:37 a.m. UTC | #2
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@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.

> > +               if (isVideo())
> > +                       return false;
> > +
> >                 return device_caps() & (V4L2_CAP_META_CAPTURE |
> >                                         V4L2_CAP_META_OUTPUT);
> >         }
Naushir Patuck July 4, 2024, 10:48 a.m. UTC | #3
On Thu, 4 Jul 2024 at 11:37, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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.

The original proposal for the Pi5 CFE driver also did this, but we
reverted back to a single queue type per-node because of the ambiguity
introduced in the V4L2VideoDevice class.

Naush

>
> > 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@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.
>
> > > +               if (isVideo())
> > > +                       return false;
> > > +
> > >                 return device_caps() & (V4L2_CAP_META_CAPTURE |
> > >                                         V4L2_CAP_META_OUTPUT);
> > >         }
>
> --
> Regards,
>
> Laurent Pinchart
Hans de Goede July 4, 2024, 11:03 a.m. UTC | #4
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@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
Kieran Bingham July 7, 2024, 11:19 p.m. UTC | #5
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@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
> 
>
Hans de Goede July 8, 2024, 8:53 a.m. UTC | #6
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@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

Patch
diff mbox series

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 (isVideo())
+			return false;
+
 		return device_caps() & (V4L2_CAP_META_CAPTURE |
 					V4L2_CAP_META_OUTPUT);
 	}