Re: [PATCH 14/21] media: ov5640: Implement get_selection

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

Thank you for the patch.

On Mon, Jan 31, 2022 at 03:44:42PM +0100, Jacopo Mondi wrote:
> Implement the get_selection pad operation for the OV5640 sensor driver.
> 
> The supported targets report the sensor's native size, the active pixel
> array size and the analog crop rectangle from which the image is
> produced.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5640.c | 61 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 762bdca83aec..ae22300b9655 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -35,6 +35,13 @@
>  #define OV5640_MIN_VBLANK	24
>  #define OV5640_MAX_VTS		1968
>  
> +#define OV5640_NATIVE_WIDTH		2624
> +#define OV5640_NATIVE_HEIGHT		1964
> +#define OV5640_PIXEL_ARRAY_TOP		8
> +#define OV5640_PIXEL_ARRAY_LEFT		16
> +#define OV5640_PIXEL_ARRAY_WIDTH	2592
> +#define OV5640_PIXEL_ARRAY_HEIGHT	1944

According to the datasheet, the sensor has 8 black lines, 6 dummy lines,
1944 active lines and 6 dummy lines. Horizontally, it has 16 dummy
columns, 2592 active columns, and 16 dummy columns. If "pixel array" is
meant to refer to the active area (I dislike the "active" name here, as
the dummy lines and columns are typically "active" too, but that's a
digression), then top should be 14.

> +
>  #define OV5640_DEFAULT_SLAVE_ID 0x3c
>  
>  #define OV5640_REG_SYS_RESET02		0x3002
> @@ -2667,6 +2674,52 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
>  	return ret;
>  }
>  
> +static int ov5640_get_selection(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_state *sd_state,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	struct ov5640_dev *sensor = to_ov5640_dev(sd);
> +	const struct ov5640_mode_info *mode = sensor->current_mode;
> +	const struct v4l2_rect *analog_crop = &mode->analog_crop;
> +	const struct v4l2_rect *crop = &mode->crop;
> +
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP: {
> +		mutex_lock(&sensor->lock);
> +
> +		sel->r.top = analog_crop->top + OV5640_PIXEL_ARRAY_TOP;
> +		sel->r.left = analog_crop->left + OV5640_PIXEL_ARRAY_LEFT;
> +		sel->r.width = analog_crop->width
> +			     - analog_crop->left - crop->left;

Why do you subtract the left coordinates here ?

> +		sel->r.height = analog_crop->height
> +			      - analog_crop->top - crop->top;
> +
> +		mutex_unlock(&sensor->lock);
> +
> +		return 0;
> +	}
> +
> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = OV5640_NATIVE_WIDTH;
> +		sel->r.height = OV5640_NATIVE_HEIGHT;
> +
> +		return 0;
> +
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +		sel->r.top = OV5640_PIXEL_ARRAY_TOP;
> +		sel->r.left = OV5640_PIXEL_ARRAY_LEFT;
> +		sel->r.width = OV5640_PIXEL_ARRAY_WIDTH;
> +		sel->r.height = OV5640_PIXEL_ARRAY_HEIGHT;

In libcamera we use V4L2_SEL_TGT_CROP_BOUNDS to set PixelArraySize,
ignoring the left and top coordinates, and V4L2_SEL_TGT_CROP_DEFAULT to
to set the PixelArrayActiveAreas property relative to PixelArraySize.
This means that non-zero values for the left and top coordinates of
V4L2_SEL_TGT_CROP_BOUNDS will cause issues. Is this an issue in
libcamera, or should V4L2_SEL_TGT_CROP_BOUNDS be changed here ?

The related question is, can we read the optical black lines and the
dummy lines and columns ?

> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static int ov5640_set_framefmt(struct ov5640_dev *sensor,
>  			       struct v4l2_mbus_framefmt *format)
>  {
> @@ -3369,6 +3422,7 @@ static const struct v4l2_subdev_pad_ops ov5640_pad_ops = {
>  	.enum_mbus_code = ov5640_enum_mbus_code,
>  	.get_fmt = ov5640_get_fmt,
>  	.set_fmt = ov5640_set_fmt,
> +	.get_selection = ov5640_get_selection,
>  	.enum_frame_size = ov5640_enum_frame_size,
>  	.enum_frame_interval = ov5640_enum_frame_interval,
>  };
> @@ -3383,9 +3437,16 @@ static int ov5640_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  {
>  	struct v4l2_mbus_framefmt *try_fmt =
>  		v4l2_subdev_get_try_format(sd, fh->state, 0);
> +	struct v4l2_rect *try_crop =
> +		v4l2_subdev_get_try_crop(sd, fh->state, 0);
>  
>  	*try_fmt = ov5640_default_fmt;
>  
> +	try_crop->left = OV5640_PIXEL_ARRAY_LEFT;
> +	try_crop->top = OV5640_PIXEL_ARRAY_TOP;
> +	try_crop->width = OV5640_PIXEL_ARRAY_WIDTH;
> +	try_crop->height = OV5640_PIXEL_ARRAY_HEIGHT;
> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart