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

[Hans de Goede]

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

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.

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

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