[libcamera-devel] [PATCH v3 04/13] libcamera: v4l2_subdevice: Implement get selection

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Apr 24, 2020 at 11:52:55PM +0200, Jacopo Mondi wrote:
> Implement get selection for the V4L2Subdevice class to support
> retrieving three targets: the subdevice native size, the analog crop and
> the active pixel area.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/include/v4l2_subdevice.h |  6 ++
>  src/libcamera/v4l2_subdevice.cpp       | 92 ++++++++++++++++++++++++++
>  2 files changed, 98 insertions(+)
> 
> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> index 4a04eadfb1f9..763ffadb61fb 100644
> --- a/src/libcamera/include/v4l2_subdevice.h
> +++ b/src/libcamera/include/v4l2_subdevice.h
> @@ -46,6 +46,10 @@ public:
>  
>  	const MediaEntity *entity() const { return entity_; }
>  
> +	int getNativeSize(unsigned int pad, Size *size);
> +	int getActiveArea(unsigned int pad, Rectangle *rect);

I think we're mixing two layers here. The V4L2 API defines selection
targets. The fact the crop bounds or crop default targets map to the
full size and active area is specific to camera sensors. I would thus
move these two functions to the CameraSensor class. As this will require
accessing different selection targets, we could add a target argument to
getCrop(), but I think it may just be simpler to drop setCrop() and
setCompose() and make getSelection() and setSelection() public.

> +	int getCropRectangle(unsigned int pad, Rectangle *rect);

As the counterpart function is called setCrop(), shouldn't this be
called getCrop() ?

> +
>  	int setCrop(unsigned int pad, Rectangle *rect);
>  	int setCompose(unsigned int pad, Rectangle *rect);
>  
> @@ -67,6 +71,8 @@ private:
>  	std::vector<SizeRange> enumPadSizes(unsigned int pad,
>  					    unsigned int code);
>  
> +	int getSelection(unsigned int pad, unsigned int target,
> +			 Rectangle *rect);
>  	int setSelection(unsigned int pad, unsigned int target,
>  			 Rectangle *rect);
>  
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 5a479a96b795..4dcf8ce48754 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -133,6 +133,68 @@ int V4L2Subdevice::open()
>   * \return The subdevice's associated media entity.
>   */
>  
> +/**
> + * \brief Get the sub-device native size
> + * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> + * \param[out] size The sub-device native area size
> + *
> + * Retrieve the size of the sub-device native area.
> + * If the sub-device represent an image sensor, the native area describes
> + * the pixel array dimensions, including inactive and calibration pixels.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -ENOTTY The native size information is not available
> + */
> +int V4L2Subdevice::getNativeSize(unsigned int pad, Size *size)
> +{
> +	Rectangle rect = {};
> +	int ret = getSelection(pad, V4L2_SEL_TGT_NATIVE_SIZE, &rect);
> +	if (ret)
> +		return ret;
> +
> +	size->width = rect.width;
> +	size->height = rect.height;
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Get the active area rectangle
> + * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> + * \param[out] rect The rectangle describing the sub-device active area
> + *
> + * Retrieve the rectangle describing the sub-device active area.
> + * If the sub-device represent an image sensor, the active area describes
> + * the pixel array active portion.
> + *
> + * The returned \a rect 'x' and 'y' fields represent the displacement from the
> + * native size rectangle top-left corner.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -ENOTTY The active areas size information is not available
> + */
> +int V4L2Subdevice::getActiveArea(unsigned int pad, Rectangle *rect)
> +{
> +	return getSelection(pad, V4L2_SEL_TGT_CROP_DEFAULT, rect);
> +}
> +
> +/**
> + * \brief Get the crop rectangle
> + * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> + * \param[out] rect The rectangle describing the cropped area rectangle
> + *
> + * Retrieve the rectangle representing the cropped area. If the sub-device
> + * represents an image sensor, the cropped area describes the pixel array
> + * portion from which the final image is produced from.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -ENOTTY The crop rectangle size information is not available
> + */
> +int V4L2Subdevice::getCropRectangle(unsigned int pad, Rectangle *rect)
> +{
> +	return getSelection(pad, V4L2_SEL_TGT_CROP, rect);
> +}
> +
>  /**
>   * \brief Set a crop rectangle on one of the V4L2 subdevice pads
>   * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> @@ -343,6 +405,36 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
>  	return sizes;
>  }
>  
> +int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> +				Rectangle *rect)
> +{
> +	struct v4l2_subdev_selection sel = {};
> +
> +	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	sel.pad = pad;
> +	sel.target = target;
> +	sel.flags = 0;
> +
> +	int ret = ioctl(VIDIOC_SUBDEV_G_SELECTION, &sel);
> +	if (ret < 0 && ret != -ENOTTY) {
> +		LOG(V4L2, Error)
> +			<< "Unable to get rectangle " << target << " on pad "
> +			<< pad << ": " << strerror(-ret);
> +		return ret;
> +	}
> +	if (ret == -ENOTTY) {
> +		LOG(V4L2, Debug) << "Selection API not available";
> +		return ret;
> +	}

Do you expect valid users of this function when the subdev doesn't
implement the selection API ? I can imagine that a caller would check
for -ENOTTY and use some fallback mechanism to make this error
non-fatal, in which case a debug message could be better than an error
message, but do we have any practical use case for that ? Is it only
meant to cover a transition period until sensor drivers get converted ?

> +
> +	rect->x = sel.r.left;
> +	rect->y = sel.r.top;
> +	rect->width = sel.r.width;
> +	rect->height = sel.r.height;
> +
> +	return 0;
> +}
> +
>  int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
>  				Rectangle *rect)
>  {

-- 
Regards,

Laurent Pinchart