| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <842dbd3c-856b-e5a8-e942-545ceb6741ca@theobroma-systems.com>
Date: Tue, 31 May 2022 14:19:21 +0200
From: Quentin Schulz <quentin.schulz@...obroma-systems.com>
To: Jacopo Mondi <jacopo@...ndi.org>,
Quentin Schulz <foss+kernel@...il.net>
Cc: shawnx.tu@...el.com, mchehab@...nel.org, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, linux-media@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 4/4] media: i2c: ov5675: add .get_selection support
Hi Jacopo,
On 5/31/22 12:50, Jacopo Mondi wrote:
> Hi Quentin
>
> On Wed, May 25, 2022 at 04:58:33PM +0200, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz@...obroma-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@...obroma-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.
>
That makes a lot of sense to me, thanks for your patience and explanations.
FYI, you're looking at the wrong driver (ov5670 vs ov5675; a mistake I
make every now and then too :) ). However, the datasheet does say that
"The OV5675 supports a binning mode to provide a lower resolution output
while maintaining the field of view.[...] The OV5675 supports 2x2
binning." so I assume we're in the same scenario as you just explained.
Since the OV5675 modes currently supported by the drivers are 4/3 only
and the smaller size mode a result of subsampling, they both have the
same CROP rectangle.
>> + 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.
>
Since there's no cropping involved in the current modes, I assume we
could just hardcode the width and height and tackle this limitation
later, once we add more modes or support for configuring cropping (this
patch only adds the getter and not the setter).
Cheers,
Quentin
Powered by blists - more mailing lists