Re: [PATCH v3 4/4] media: i2c: ov5675: add .get_selection support

[Quentin Schulz <quentin.schulz@theobroma-systems.com>]

Hi Jacopo,

On 5/12/22 11:05, Jacopo Mondi wrote:
> Hi Quentin,
> 
> On Mon, May 09, 2022 at 04:32:26PM +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".
>>
>> As recommended in the SELECTION API documentation, let's put the first
>> useful active pixel at the top/left corner (0,0).
>>
>> This window is the default and maximal crop allowed by the sensor.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> ---
>>
>> added in v3
>>
>>   drivers/media/i2c/ov5675.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
>> index 5544e1ae444e..8e3a5bc6c027 100644
>> --- a/drivers/media/i2c/ov5675.c
>> +++ b/drivers/media/i2c/ov5675.c
>> @@ -78,6 +78,9 @@
>>   #define OV5675_REG_FORMAT1		0x3820
>>   #define OV5675_REG_FORMAT2		0x373d
>>
>> +#define OV5675_PIXEL_ARRAY_WIDTH	2592U
>> +#define OV5675_PIXEL_ARRAY_HEIGHT	1944U
>> +
>>   #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
>>
>>   static const char * const ov5675_supply_names[] = {
>> @@ -1115,6 +1118,27 @@ 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)
>> +{
>> +	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
>> +		return -EINVAL;
>> +
>> +	switch (sel->target) {
>> +	case V4L2_SEL_TGT_CROP:
>> +	case V4L2_SEL_TGT_CROP_DEFAULT:
>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
>> +		/* In HW, top/left corner is actually at (16,16) */
>> +		sel->r.top = 0;
>> +		sel->r.left = 0;
>> +		sel->r.width = OV5675_PIXEL_ARRAY_WIDTH;
>> +		sel->r.height = OV5675_PIXEL_ARRAY_HEIGHT;
>> +		return 0;
>> +	}
> 
> CROP_NATIVE = the full pixel array size = 2592x1944
> 
> CROP_BOUNDS = the rectangle that contains all possible crop
>                rectangles, aka the readable portion of your pixel array.
>                If in your case the sensor can read out dummy and non
>                active lines this is == NATIVE
> 
> CROP_DEFAULT = the active/valid pixel area. If there are any
>                 dummy/invalid lines the DEFAULT rectangle should not
>                 include them
> 
> CROP = the portion of the active pixel area cropped to produce the
>         final image. You should look into the modes definition and
>         inspect what values are programmed in register 0x380x (I don't
>         have a datasheet hence I don't know what corresponds to what)
> 
> Does this make any sense to you ?
> 

It's difficult to make sense of the datasheet to be honest.

There's a 3296x2480px "full-size" rectangle, then the sensor array 
output area called CROP and configurable through registers 0x380x, then 
another output area called WIN (for window) also configurable through 
registers 0x380x. The WIN area seems to be just a mask applied on top of 
CROP area ("timing is not affected").

On top of that, the schema shows 24 black lines - I assume - 
incorrectly, one after the start of the full-size rectangle, one after 
the end of the full-size rectangle.

Then the sensor array area region in another section explicitly 
specifies the sensor to be 2624x2000px, active dummy pixels (2 16-pixel 
rows and columns) and black lines (1 24-pixel line) included. Which 
makes the actual useful area 2592x1944px.

In the datasheet, the default size for the CROP area is
2624x1952px, offset (0,12) and for WIN it is 2592x1944px, offset (16,4) 
(assumed relative to CROP given the second coordinate).

In the kernel driver though, the 2592x1944px mode configures the CROP 
area to be 2624x1968px offset (0,4) and the WIN area to be 2592x1944px, 
offset (16,13).

The datasheet only ever mentions 2592x1944px as being the max resolution 
of the sensor though the sensor output area documentation and registers 
do not mention such a limitation.

Since we're not modifying the crop area at the moment, CROP_DEFAULT and 
CROP would be the same, which would both be 2592x1944px.

For the two others, I'm not sure. Any clue or hint with the info I just 
gave?

Cheers,
Quentin