[libcamera-devel] [PATCH] libcamera: v4l2_subdevice: use G_FORMAT if ENUM_FRAME_SIZE isn't implemented

[Jacopo Mondi]

Hello Andrey,
   thanks for the patch

On Fri, Oct 25, 2019 at 04:32:08PM +0300, Andrey Konovalov wrote:
> Many camera sensor drivers support only one frame size, and don't
> implement VIDIOC_SUBDEV_ENUM_FRAME_SIZE ioctl. And libcamera fails

Is this true ? A single frame size? Do you see this often ?

> to enumerate such cameras as it doesn't get the list of resolutions
> the sensor driver supports. Fix this by using VIDIOC_SUBDEV_G_FORMAT
> to get the size of the sensor's active format when the ENUM_FRAME_SIZE
> ioctl isn't implemented by the sensor driver.
>

As a general note, personally, I would prefer drivers to implement the
proper ioctls, and I think we should not provide incentives to work
this around. But if this fixes a real issue with several drivers I
think we could consider this. What do others think here?

> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> ---
>  src/libcamera/v4l2_subdevice.cpp | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index f2bcd7f..fac7586 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -317,7 +317,25 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
>  				   sizeEnum.max_width, sizeEnum.max_height);
>  	}
>
> -	if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
> +	if (ret == -ENOTTY) {
> +		struct v4l2_subdev_format format = {};
> +
> +		LOG(V4L2, Debug)
> +			<< "VIDIOC_SUBDEV_ENUM_FRAME_SIZE not implemented "
> +			<< "- trying VIDIOC_SUBDEV_G_FMT";
> +		format.pad = pad;
> +		format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +
> +		ret = ioctl(VIDIOC_SUBDEV_G_FMT, &format);
> +		if (ret == 0) {
> +			sizes.emplace_back(format.format.width,
> +					   format.format.height,
> +					   format.format.width,
> +					   format.format.height);

However, I don't think this works... ENUM_FRAME_SIZE has a .code field
in struct v4l2_subdev_frame_size_enum, and enumerates sizes for that
specific mbus code. As you can see the 'sizes' vector this operations
returns is associated with said mbus code in the caller's 'formats'
map.

G_FMT returns the current format regardless of the mbus code, so if we
want to work around faulty drivers using this approach, I would do
that in the caller, and adjust the 'formats' map, not only the here
enumerated sizes.

Thanks
   j

> +		}
> +	}
> +
> +	if (ret < 0 && ret != -EINVAL) {
>  		LOG(V4L2, Error)
>  			<< "Unable to enumerate sizes on pad " << pad
>  			<< ": " << strerror(-ret);
> --
> 2.17.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191109/6bd79a36/attachment.sig>