| 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
| ||
|
Date: Wed, 8 Jun 2022 08:42:09 +0200
From: Jacopo Mondi <jacopo@...ndi.org>
To: 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,
Quentin Schulz <quentin.schulz@...obroma-systems.com>
Subject: Re: [PATCH v6 4/4] media: i2c: ov5675: add .get_selection support
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 ?
>
> 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;
>
> What do you think about?
>
> Thanks,
> Tommaso
>
>
We have extensively discussed this:
https://patchwork.linuxtv.org/project/linux-media/patch/20220509143226.531117-4-foss+kernel@0leil.net/
https://patchwork.linuxtv.org/project/linux-media/patch/20220525145833.1165437-4-foss+kernel@0leil.net/
>From the CROP_BOUNDS definition:
Bounds of the crop rectangle. All valid crop rectangles fit inside the
crop bounds rectangle.
>From CROP_DEFAULT:
Suggested cropping rectangle that covers the “whole picture”. This
includes only active pixels and excludes other non-active pixels such
as black pixels.
If (and only if) dummy/inactive pixels can be read out, the BOUNDS
rectangle should contain them. In this case, as the analog crop
rectangle is defined with a 16x16 top-left corner (and with a visible
size of 2592x1944) from a larger rectangle, I presume it means
dummy/invalid pixls can be read (IOW you can give to the ISP a rectangle
that includes them).
Anyway, we're discussing details really. I think v5 was almost there
+ 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;
+ 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;
+ }
Apart from the fact that TGT_CROP should not report the final image
size (after binning/digital crop) but the size of the pixel array
portion processed to obtain the final image.
+ 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:
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ sel->r.top = 16;
+ sel->r.left = 16;
+ sel->r.width = 2592;
+ sel->r.height = 1944;
+ return 0;
+ }
Let me remind that (in the context of pleasing libcamera requirements
as Quentin is doing) all targets apart TGT_CROP are only useful to
report static properties of the camera, which are of no real use
unless you start actually looking into reading out black pixels etc
etc.
> >
> > Thanks
> > j
> >
> >
> > > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > > + sel->r.top = 16;
> > > + sel->r.left = 16;
> > > + sel->r.width = 2592;
> > > + sel->r.height = 1944;
> > > + return 0;
> > > + }
> > > + 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 +1190,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
> > >
>
> --
> Tommaso Merciai
> Embedded Linux Engineer
> tommaso.merciai@...rulasolutions.com
> __________________________________
>
> Amarula Solutions SRL
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
> T. +39 042 243 5310
> info@...rulasolutions.com
> www.amarulasolutions.com
Powered by blists - more mailing lists