| Message ID | 20240703150923.25994-1-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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 >
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); > > }
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
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
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 > >
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
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); }
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(+)