[libcamera-devel] [PATCH v2 03/10] libcamera: v4l2_subdevice: Extend [gs]etFormat() to specify format type

[Jacopo Mondi]

HI Laurent,

On Mon, Mar 16, 2020 at 11:43:03PM +0200, Laurent Pinchart wrote:
> The V4L2 subdevice API exposes ACTIVE and TRY formats, but the
> V4L2Subdevice getFormat() and setFormat() operations only apply on
> ACTIVE formats. Extend them to support TRY formats as well.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/include/v4l2_subdevice.h | 11 +++++++++--
>  src/libcamera/v4l2_subdevice.cpp       | 23 +++++++++++++++++++----
>  2 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> index 9c077674f997..4a04eadfb1f9 100644
> --- a/src/libcamera/include/v4l2_subdevice.h
> +++ b/src/libcamera/include/v4l2_subdevice.h
> @@ -32,6 +32,11 @@ struct V4L2SubdeviceFormat {
>  class V4L2Subdevice : public V4L2Device
>  {
>  public:
> +	enum Whence {
> +		ActiveFormat,
> +		TryFormat,
> +	};

Just wondering if
        ActiveFormat = V4L2_SUBDEV_FORMAT_ACTIVE,
        TryFormat = V4L2_SUBDEV_FORMAT_TRY

would not save you the ternary operator in set/get functions

> +
>  	explicit V4L2Subdevice(const MediaEntity *entity);
>  	V4L2Subdevice(const V4L2Subdevice &) = delete;
>  	V4L2Subdevice &operator=(const V4L2Subdevice &) = delete;
> @@ -46,8 +51,10 @@ public:
>
>  	ImageFormats formats(unsigned int pad);
>
> -	int getFormat(unsigned int pad, V4L2SubdeviceFormat *format);
> -	int setFormat(unsigned int pad, V4L2SubdeviceFormat *format);
> +	int getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> +		      Whence whence = ActiveFormat);
> +	int setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> +		      Whence whence = ActiveFormat);

This mimics the V4L2 APIs, so it's certainly safe, but considering the
use case for TRY is actually to set a try format to then get it back
to see how the driver changed it, have you considered a tryFormat()
function that does both steps in one call ?

>
>  	static V4L2Subdevice *fromEntityName(const MediaDevice *media,
>  					     const std::string &entity);
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 8b9da81e8ab3..fb9bdbce2610 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -95,6 +95,15 @@ const std::string V4L2SubdeviceFormat::toString() const
>   * any device left open will be closed, and any resources released.
>   */
>
> +/**
> + * \enum V4L2Subdevice::Whence
> + * \brief Specify the type of format for getFormat() and setFormat() operations
> + * \var V4L2Subdevice::ActiveFormat
> + * \brief The format operation applies to ACTIVE formats
> + * \var V4L2Subdevice::TryFormat
> + * \brief The format operation applies to TRY formats
> + */
> +
>  /**
>   * \brief Create a V4L2 subdevice from a MediaEntity using its device node
>   * path
> @@ -184,12 +193,15 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
>   * \brief Retrieve the image format set on one of the V4L2 subdevice pads
>   * \param[in] pad The 0-indexed pad number the format is to be retrieved from
>   * \param[out] format The image bus format
> + * \param[in] whence The format to retrieve, ACTIVE or TRY
>   * \return 0 on success or a negative error code otherwise
>   */
> -int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> +int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> +			     Whence whence)
>  {
>  	struct v4l2_subdev_format subdevFmt = {};
> -	subdevFmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	subdevFmt.which = whence == ActiveFormat ? V4L2_SUBDEV_FORMAT_ACTIVE
> +			: V4L2_SUBDEV_FORMAT_TRY;
>  	subdevFmt.pad = pad;
>
>  	int ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> @@ -211,6 +223,7 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)
>   * \brief Set an image format on one of the V4L2 subdevice pads
>   * \param[in] pad The 0-indexed pad number the format is to be applied to
>   * \param[inout] format The image bus format to apply to the subdevice's pad
> + * \param[in] whence The format to set, ACTIVE or TRY

Here and above "ActiveFormat or TryFormat".
If you could cross-reference to the enum it's even better

Thanks
  j

>   *
>   * Apply the requested image format to the desired media pad and return the
>   * actually applied format parameters, as \ref V4L2Subdevice::getFormat would
> @@ -218,10 +231,12 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)
>   *
>   * \return 0 on success or a negative error code otherwise
>   */
> -int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> +int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> +			     Whence whence)
>  {
>  	struct v4l2_subdev_format subdevFmt = {};
> -	subdevFmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	subdevFmt.which = whence == ActiveFormat ? V4L2_SUBDEV_FORMAT_ACTIVE
> +			: V4L2_SUBDEV_FORMAT_TRY;
>  	subdevFmt.pad = pad;
>  	subdevFmt.format.width = format->size.width;
>  	subdevFmt.format.height = format->size.height;
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel