| Message ID | 20240430171728.18073-2-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hans, Thanks 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. > > 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 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 */ > + if (isVideo()) > + return false; > + > return device_caps() & (V4L2_CAP_META_CAPTURE | > V4L2_CAP_META_OUTPUT); > } It looks like to me that some rework will be needed in determining the buffer type for a video node. The current code assumes there is just one and that it's directly derived from device capabilities. Unicam might be the only CSI-2 receiver with metadata support for which this applies. But I guess this is ok for now. Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> Cc Stanislaw, too.
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 ? > 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 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); > }
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. Regards, Hans > >> 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 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); >> } >
Hi all, On Wed, 1 May 2024 at 12:58, Hans de Goede <hdegoede@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. Regards, Naush > > Regards, > > Hans > > > > > > >> 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 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); > >> } > > >
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@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@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); >>>> } >>> >> >
On Wed, 1 May 2024 at 14:38, Hans de Goede <hdegoede@redhat.com> wrote: > > 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@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? That's correct, and this patch will not have a negative effect for Pi 5. But I'll leave it up to libcamera maintainers to decide if this workaround can/should be merged. > > 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@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); > >>>> } > >>> > >> > > >
On Wed, May 01, 2024 at 02:47:11PM +0100, Naushir Patuck wrote: > On Wed, 1 May 2024 at 14:38, Hans de Goede wrote: > > On 5/1/24 2:09 PM, Naushir Patuck wrote: > > > On Wed, 1 May 2024 at 12:58, Hans de Goede wrote: > > >> On 5/1/24 12:11 AM, Laurent Pinchart wrote: > > >>> 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? > > That's correct, and this patch will not have a negative effect for Pi > 5. But I'll leave it up to libcamera maintainers to decide if this > workaround can/should be merged. I don't think this patch will have a negative effect on any platform we support today, but I'm always a bit uncomfortable when we pile up technical debt. I'd like to see more volunteers to implement things correctly :-) > > 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. > > > > >>>> 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 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); > > >>>> }
Hi, On 5/1/24 5:43 PM, Laurent Pinchart wrote: > On Wed, May 01, 2024 at 02:47:11PM +0100, Naushir Patuck wrote: >> On Wed, 1 May 2024 at 14:38, Hans de Goede wrote: >>> On 5/1/24 2:09 PM, Naushir Patuck wrote: >>>> On Wed, 1 May 2024 at 12:58, Hans de Goede wrote: >>>>> On 5/1/24 12:11 AM, Laurent Pinchart wrote: >>>>>> 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? >> >> That's correct, and this patch will not have a negative effect for Pi >> 5. But I'll leave it up to libcamera maintainers to decide if this >> workaround can/should be merged. > > I don't think this patch will have a negative effect on any platform we > support today, but I'm always a bit uncomfortable when we pile up > technical debt. I'd like to see more volunteers to implement things > correctly :-) The way I see it the problem in this case is that we don't have a clear picture of what correctly looks like yet... Regards, Hans > >>> 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. >>> >>>>>>> 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 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); >>>>>>> } >
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 */ + 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(+)