[libcamera-devel,03/15] v4l2: v4l2_camera_proxy: Fix v4l2-compliance support for extended formats

4047 diff mbox series

Fix the following v4l2-compliance error:

fail: v4l2-compliance.cpp(652): !(caps & V4L2_CAP_EXT_PIX_FORMAT)

Simply add V4L2_CAP_EXT_PIX_FORMAT to capabilities in querycap.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/v4l2/v4l2_camera_proxy.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Hi Paul,

On Tue, Jun 16, 2020 at 10:12:32PM +0900, Paul Elder wrote:
> Fix the following v4l2-compliance error:
>
> fail: v4l2-compliance.cpp(652): !(caps & V4L2_CAP_EXT_PIX_FORMAT)
>
> Simply add V4L2_CAP_EXT_PIX_FORMAT to capabilities in querycap.

This is really a relic from the past, as the V4L2 sets this flag
unconditionally, but I don't see it used anywhere in mainline :/

>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

However, let's please the compliance tool

Acked-by: Jacopo Mondi <jacopo@jmondi.org>

> ---
>  src/v4l2/v4l2_camera_proxy.cpp | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index 5b74b53..d899853 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -202,7 +202,9 @@ void V4L2CameraProxy::querycap(std::shared_ptr<Camera> camera)
>  		       sizeof(capabilities_.bus_info));
>  	/* \todo Put this in a header/config somewhere. */
>  	capabilities_.version = KERNEL_VERSION(5, 2, 0);

Just noticed, this could be exposed by the CameraManager by quering
the pipeline handlers media devices, I presume this will soon be
required, as hardcoding the kernel version is not optimal :)

> -	capabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> +	capabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE
> +				  | V4L2_CAP_STREAMING
> +				  | V4L2_CAP_EXT_PIX_FORMAT;
>  	capabilities_.capabilities = capabilities_.device_caps
>  				   | V4L2_CAP_DEVICE_CAPS;
>  	memset(capabilities_.reserved, 0, sizeof(capabilities_.reserved));
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Wed, Jun 17, 2020 at 02:43:45PM +0200, Jacopo Mondi wrote:
> On Tue, Jun 16, 2020 at 10:12:32PM +0900, Paul Elder wrote:
> > Fix the following v4l2-compliance error:
> >
> > fail: v4l2-compliance.cpp(652): !(caps & V4L2_CAP_EXT_PIX_FORMAT)
> >
> > Simply add V4L2_CAP_EXT_PIX_FORMAT to capabilities in querycap.

Don't you also need to set fmt.pix.priv to V4L2_PIX_FMT_PRIV_MAGIC for
the format-related ioctls ? The priv field is documented by
https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/pixfmt-v4l2.html#c.v4l2_pix_format
as

"This field indicates whether the remaining fields of the struct
v4l2_pix_format, also called the extended fields, are valid. When set to
V4L2_PIX_FMT_PRIV_MAGIC, it indicates that the extended fields have been
correctly initialized. When set to any other value it indicates that the
extended fields contain undefined values.

Applications that wish to use the pixel format extended fields must
first ensure that the feature is supported by querying the device for
the V4L2_CAP_EXT_PIX_FORMAT capability. If the capability isn’t set the
pixel format extended fields are not supported and using the extended
fields will lead to undefined results.

To use the extended fields, applications must set the priv field to
V4L2_PIX_FMT_PRIV_MAGIC, initialize all the extended fields and zero the
unused bytes of the struct v4l2_format raw_data field.

When the priv field isn’t set to V4L2_PIX_FMT_PRIV_MAGIC drivers must
act as if all the extended fields were set to zero. On return drivers
must set the priv field to V4L2_PIX_FMT_PRIV_MAGIC and all the extended
fields to applicable values."

And the commit that introduces V4L2_CAP_EXT_PIX_FORMAT sets
V4L2_PIX_FMT_PRIV_MAGIC for the get, set and try format ioctls.

> This is really a relic from the past, as the V4L2 sets this flag
> unconditionally, but I don't see it used anywhere in mainline :/
> 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> However, let's please the compliance tool
> 
> Acked-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> > ---
> >  src/v4l2/v4l2_camera_proxy.cpp | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index 5b74b53..d899853 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -202,7 +202,9 @@ void V4L2CameraProxy::querycap(std::shared_ptr<Camera> camera)
> >  		       sizeof(capabilities_.bus_info));
> >  	/* \todo Put this in a header/config somewhere. */
> >  	capabilities_.version = KERNEL_VERSION(5, 2, 0);
> 
> Just noticed, this could be exposed by the CameraManager by quering
> the pipeline handlers media devices, I presume this will soon be
> required, as hardcoding the kernel version is not optimal :)

I don't think we should do that. The version field here reports the
version of V4L2 API implemented by the device. The version we implement
is a property of the V4L2 compat layer. When running on a newer kernel,
the compat layer won't automatically expose new V4L2 features. I thus
think hardcoding the version here is the best option.

> > -	capabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> > +	capabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE
> > +				  | V4L2_CAP_STREAMING
> > +				  | V4L2_CAP_EXT_PIX_FORMAT;
> >  	capabilities_.capabilities = capabilities_.device_caps
> >  				   | V4L2_CAP_DEVICE_CAPS;
> >  	memset(capabilities_.reserved, 0, sizeof(capabilities_.reserved));