linux-kernel - Re: [PATCH v6 4/4] media: i2c: ov5675: add .get

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 8 Jun 2022 15:05:29 +0200
From:   Quentin Schulz <quentin.schulz@...obroma-systems.com>
To:     Jacopo Mondi <jacopo@...ndi.org>,
        Tommaso Merciai <tommaso.merciai@...rulasolutions.com>
Cc:     Quentin Schulz <foss+kernel@...il.net>, 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 v6 4/4] media: i2c: ov5675: add .get_selection support

Jacopo, Tommaso,

On 6/8/22 08:42, Jacopo Mondi wrote:
> Hi
> 
> On Wed, Jun 08, 2022 at 12:04:05AM +0200, Tommaso Merciai wrote:
>> Hi Quentin/Jacopo,
>>
>> On Tue, Jun 07, 2022 at 06:51:36PM +0200, Jacopo Mondi wrote:
>>> Hi Quentin,
>>>
>>> On Tue, Jun 07, 2022 at 05:33:35PM +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.
>>>>
>>>> There are currently two modes supported by this driver: 2592*1944 and
>>>> 1296*972. The second mode is obtained thanks to subsampling while
>>>> keeping the same field of view (FoV). No cropping involved, hence the
>>>> harcoded values.
>>>>
>>>> Signed-off-by: Quentin Schulz <quentin.schulz@...obroma-systems.com>
>>>> ---
>>>>
>>>> v6:
>>>>   - explicit a bit more the commit log around subsampling for lower
>>>>   resolution modes,
>>>>   - (again) fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
>>>>
>>>> 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 | 21 +++++++++++++++++++++
>>>>   1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
>>>> index 80840ad7bbb0..2230ff47ef49 100644
>>>> --- a/drivers/media/i2c/ov5675.c
>>>> +++ b/drivers/media/i2c/ov5675.c
>>>> @@ -1121,6 +1121,26 @@ 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_BOUNDS:
>>>
>>> Seem like we have trouble understanding each other, or better, I have
>>> troubles explaining myself most probably :)
>>>
>>> If the dummy/black area is readable, this should just be (0, 0, 2624,
>>> 2000) like it was in your previous version. What has changed that I
>>> have missed ?
>>

I wouldn't say there's some misunderstanding, it's just super hard to 
figure out how to match what the datasheet says to what the kernel 
wants. Yay to obscure/confusing datasheets \o/

I just did things too quickly, nothing changed. Sorry, will send a v7.

>> Taking as reference drivers/media/i2c/ov5693.c and others,
>> seems ok what Quentin have done from my side.
>>
>> Just one thing: maybe is better to avoid magic numbers with more
>> explicit defines like:
>>
>>   + case V4L2_SEL_TGT_CROP_DEFAULT:
>>   +           sel->r.top = OV5675_ACTIVE_START_TOP;
>>   +           sel->r.left = OV5693_ACTIVE_START_LEFT;
>>   +           sel->r.width = OV5693_ACTIVE_WIDTH;
>>   +           sel->r.height = OV5693_ACTIVE_HEIGHT;
>>

They are hardcoded today but actually depend on what;s set in the 
registers too, which might differ if we add more modes in the future? 
It's anyway auto-magic and it's the only place it's used, so not sure it 
brings much especially since the variable names on the left hand side of 
the operator are pretty self-explanatory (not talking about 
V4L2_SEL_TGT_CROP_* :p)? Not that I'm against it.

Cheers,
Quentin