Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 31 May 2022 12:50:11 +0200
From:   Jacopo Mondi <jacopo@jmondi.org>
To:     Quentin Schulz <foss+kernel@0leil.net>
Cc:     shawnx.tu@intel.com, mchehab@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux-media@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Quentin Schulz <quentin.schulz@theobroma-systems.com>
Subject: Re: [PATCH v5 4/4] media: i2c: ov5675: add .get_selection support
Message-ID: <20220531105011.yxrosmwtw3mpaomb@uno.localdomain>
References: <20220525145833.1165437-1-foss+kernel@0leil.net>
 <20220525145833.1165437-4-foss+kernel@0leil.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220525145833.1165437-4-foss+kernel@0leil.net>
Precedence: bulk

Hi Quentin

On Wed, May 25, 2022 at 04:58:33PM +0200, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> The sensor has 2592*1944 active pixels, surrounded by 16 active dummy
> pixels and there are an additional 24 black rows "at the bottom".
>
>                      [2624]
>         +-----+------------------+-----+
>         |     |     16 dummy     |     |
>         +-----+------------------+-----+
>         |     |                  |     |
>         |     |     [2592]       |     |
>         |     |                  |     |
>         |16   |      valid       | 16  |[2000]
>         |dummy|                  |dummy|
>         |     |            [1944]|     |
>         |     |                  |     |
>         +-----+------------------+-----+
>         |     |     16 dummy     |     |
>         +-----+------------------+-----+
>         |     |  24 black lines  |     |
>         +-----+------------------+-----+
>
> The top-left coordinate is gotten from the registers specified in the
> modes which are identical for both currently supported modes.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>
> v4:
>  - explicit a bit more the commit log,
>  - added drawing in the commit log,
>  - fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
>
> added in v3
>
>  drivers/media/i2c/ov5675.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> index c1f3c387afde0..384a9ea2372c3 100644
> --- a/drivers/media/i2c/ov5675.c
> +++ b/drivers/media/i2c/ov5675.c
> @@ -1121,6 +1121,38 @@ static int ov5675_get_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>
> +static int ov5675_get_selection(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_state *state,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	struct ov5675 *ov5675 = to_ov5675(sd);
> +
> +	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> +		return -EINVAL;
> +
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = 2624;
> +		sel->r.height = 2000;
> +		return 0;
> +	case V4L2_SEL_TGT_CROP:
> +		sel->r.top = 16;
> +		sel->r.left = 16;
> +		sel->r.width = ov5675->cur_mode->width;
> +		sel->r.height = ov5675->cur_mode->height;
> +		return 0;

I'm afraid this doesn't match exactly my understanding of the
discussion we had.

The driver defines the following modes

/*
 * OV5670 sensor supports following resolutions with full FOV:
 * 4:3  ==> {2592x1944, 1296x972, 648x486}
 * 16:9 ==> {2560x1440, 1280x720, 640x360}
 */
static const struct ov5670_mode supported_modes[] = {
	{
		.width = 2592,
		.height = 1944,
	},
	{
		.width = 1296,
		.height = 972,
	},
	{
		.width = 648,
		.height = 486,
	},
	{
		.width = 2560,
		.height = 1440,
	},
	{
		.width = 1280,
		.height = 720,
	},
	{
		.width = 640,
		.height = 360,
	}
};

The comment says all modes retain the "full FOV", which I assume it
implies they are obtained by sub-sampling and not cropping.

The first three modes (4:3) are indeed obtained by subsampling the
full active pixel array:

        (2592,1944) / 2 = (1296,972) / 2 = (648,486)

The last three are obtained by subsampling a slightly cropped portion
of the pixel array

        (2560,1440) / 2 = (1280,720) / 2 = (640,360)

If you set CROP = cur_mode->[width/height] you will instead report the
visible width/height, which as said it's obtained by subsampling (of a
slightly cropped portion of the pixel array for the last three ones)

The CROP rectangle is then (2592, 1944) for the first three and (2560,
1440) for the last three.

I would add a v4l2_rect to struct ov5670_mode where to record that and
report it here.

> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +		sel->r.top = 16;
> +		sel->r.left = 16;
> +		sel->r.width = supported_modes[0].width;
> +		sel->r.height = supported_modes[0].height;
> +		return 0;

You could also define these values instead of fishing in the
supported_modes array, to protect against future changes to the array
itself. Up to you.


> +	}
> +	return -EINVAL;
> +}
> +
>  static int ov5675_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum *code)
> @@ -1170,6 +1202,7 @@ static const struct v4l2_subdev_video_ops ov5675_video_ops = {
>  static const struct v4l2_subdev_pad_ops ov5675_pad_ops = {
>  	.set_fmt = ov5675_set_format,
>  	.get_fmt = ov5675_get_format,
> +	.get_selection = ov5675_get_selection,
>  	.enum_mbus_code = ov5675_enum_mbus_code,
>  	.enum_frame_size = ov5675_enum_frame_size,
>  };
> --
> 2.36.1
>