[PATCH] libcamera: v4l2_videodevice: Use bufferType_ in [get|try|set]Format()

[Jacopo Mondi]

Hi Hans

On Tue, Jul 16, 2024 at 12:16:39PM GMT, Hans de Goede wrote:
> Hi Jacopo,
>
> On 7/16/24 11:18 AM, Jacopo Mondi wrote:
> > Hi Hans
> >
> > On Sat, Jul 13, 2024 at 08:43:27PM GMT, Hans de Goede wrote:
> >> V4L2VideoDevice is using the caps to determine which kind of buffers to
> >> use with the video-device in 2 different cases:
> >>
> >> 1. V4L2VideoDevice::open()
> >> 2. V4L2VideoDevice::[get|try|set]Format()
> >>
> >> And the order in which the caps are checked is different between
> >> these 2 cases. This is a problem for /dev/video# nodes which support
> >> both video-capture and metadata buffers. open() sets bufferType_ to
> >> V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as
> >> [get|try|set]Format() will call [get|set]FormatMeta() which does not
> >> work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers.
> >>
> >> Switch [get|try|set]Format() to use the bufferType_ to determine on what
> >> sort of buffers they should be operating, leaving the V4L2VideoDevice
> >> code with only a single place where the decision is made what sort
> >> of buffers it should operate on for a specific /dev/video# node.
> >>
> >> This will also allow to modify open() in the future to take a bufferType
> >> argument to allow overriding the default bufferType it selects for
> >> /dev/video# nodes which are capable of supporting more then 1 buffer type.
> >
> > Does this mean that, at the moment, on a device that supports both
> > capture and metadata buffer types, video-capture is selected
> > unconditionally and the metadata interface cannot be used ?
>
> Yes, this is an improvement over the current situation where
> neither works :)
>
> Note the metadata API's are still not entirely set in stone in
> the kernel and are still disabled (behind an #ifdef) upstream atm.

Ah so we're not talking about the usual META_CAPTURE and META_OUTPUT
interfaces used to exchange ISP parameters, but we're here looking at
capturing sensor's produced metadata (which require streams and
routing support which are currently disabled behind an #ifdef)

> But the IPU6 driver already advertises V4L2_CAP_META_CAPTURE. So
> for now using video-capture instead of the not yet enabled metadata
> support is definitely the right thing to do.
>
> > If you plan to add an option to override the bufferType_ at open()
> > time, does it mean it is needed to go through a open-close-open cycle
> > to switch buffer type or:
>
> At least the IPU6 has multiple dma engines represented by multiple
> /dev/video# nodes. So the idea would be to open one for video capture
> and one for metadata capture and then open both requesting capture
> buffers while opening one and metadata buffers while opening the other.

I think I got lost in the sequence of "open" and "while opening" but I
presume you mean that a video node can be used either for
video-capture and for meta-data capture, depending on the buffer type
it is initialized with (I had a brief look at ipu6-isys-video.c and I
see the vb2 queue being initialized with the VIDEO_CAPTURE type
unconditionally, I wonder how the META_CAPTURE interface will be
selected in future, but that's a different topic).

>
> But yes switching type on a single node would require a close + open,
> note that changing type not only requires changing the bufferType_
> of V4L2VideoDevice if switching between output/capture it also
> requires changing the fdBufferNotifier_ .  So requiring a close()
> + open() to switch type does not seem like a strange requirement.
>
> > 1) The device can be opened multiple times and bufferType_ gets
> > overriden ? (I'm not sure this is a good ideas as the last caller of
> > open() selects the bufferType_)
>
> /dev/video# nodes can be opened multiple times, but only to allow
> querying caps / setting controls. Only one fd referring to it
> can requestBuffers at a time. So basically from a libcamera pov
> /dev/video# nodes should not be opened multiple times.
>

ok

> > 2) Should we instead allow overriding the bufferType_ to use with an
> > optional parameter to [get|try|set]Format() instead ?
>
> bufferType_ is also used internally by the V4L2VideoDevice code for
> enumPixelformats(), setSelection(), requestBuffers(), createBuffer(),
> exportDmabufFd(), etc.
>
> The normal way of operating really is for there to be a fixed bufferType
> for one specific /dev/video# nodes. Which is why I'm suggesting to
> (in the future when needed for metadata support) to add an optional
> bufferType argument to open(), which basically fixates the bufferType
> at open() time for a /dev/video# node which can support multiple types.

We should probably make sure then that if a video device is 'forced'
to use a buffer type at open() time, all the follwing open() call
won't force it to a different buffer type ? But this is indeed for
later.

Thanks
  j

>
> Regards,
>
> Hans
>
>
>
>
> >
> >>
> >> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> >> ---
> >>  src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++--------
> >>  1 file changed, 41 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> >> index 4947aa3d..a8dc5355 100644
> >> --- a/src/libcamera/v4l2_videodevice.cpp
> >> +++ b/src/libcamera/v4l2_videodevice.cpp
> >> @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const
> >>   */
> >>  int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)
> >>  {
> >> -	if (caps_.isMeta())
> >> -		return getFormatMeta(format);
> >> -	else if (caps_.isMultiplanar())
> >> -		return getFormatMultiplane(format);
> >> -	else
> >> +	switch (bufferType_) {
> >> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> >> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> >>  		return getFormatSingleplane(format);
> >> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> >> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> >> +		return getFormatMultiplane(format);
> >> +	case V4L2_BUF_TYPE_META_CAPTURE:
> >> +	case V4L2_BUF_TYPE_META_OUTPUT:
> >> +		return getFormatMeta(format);
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >>  }
> >>
> >>  /**
> >> @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)
> >>   */
> >>  int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)
> >>  {
> >> -	if (caps_.isMeta())
> >> -		return trySetFormatMeta(format, false);
> >> -	else if (caps_.isMultiplanar())
> >> -		return trySetFormatMultiplane(format, false);
> >> -	else
> >> +	switch (bufferType_) {
> >> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> >> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> >>  		return trySetFormatSingleplane(format, false);
> >> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> >> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> >> +		return trySetFormatMultiplane(format, false);
> >> +	case V4L2_BUF_TYPE_META_CAPTURE:
> >> +	case V4L2_BUF_TYPE_META_OUTPUT:
> >> +		return trySetFormatMeta(format, false);
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >>  }
> >>
> >>  /**
> >> @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)
> >>  int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)
> >>  {
> >>  	int ret = 0;
> >
> > I don't think it is necessary to initialize ret, but it wasn't already
> > so
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
> > Thanks
> >   j
> >
> >> -	if (caps_.isMeta())
> >> -		ret = trySetFormatMeta(format, true);
> >> -	else if (caps_.isMultiplanar())
> >> -		ret = trySetFormatMultiplane(format, true);
> >> -	else
> >> +
> >> +	switch (bufferType_) {
> >> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> >> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> >>  		ret = trySetFormatSingleplane(format, true);
> >> +		break;
> >> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> >> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> >> +		ret = trySetFormatMultiplane(format, true);
> >> +		break;
> >> +	case V4L2_BUF_TYPE_META_CAPTURE:
> >> +	case V4L2_BUF_TYPE_META_OUTPUT:
> >> +		ret = trySetFormatMeta(format, true);
> >> +		break;
> >> +	default:
> >> +		ret = -EINVAL;
> >> +		break;
> >> +	}
> >>
> >>  	/* Cache the set format on success. */
> >>  	if (ret)
> >> --
> >> 2.45.2
> >>
> >
>