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]
Message-ID: <22fbl6eyjlruexv3cw7qegs2ekexyxvw5xocuads2eyws22ul7@dlerrm334bhp>
Date: Tue, 26 Nov 2024 19:00:40 +0530
From: Jai Luthra <jai.luthra@...asonboard.com>
To: Dave Stevenson <dave.stevenson@...pberrypi.com>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	Sakari Ailus <sakari.ailus@...ux.intel.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Jacopo Mondi <jacopo.mondi@...asonboard.com>
Subject: Re: [PATCH v3 2/3] media: i2c: imx219: make HBLANK r/w to allow
 longer exposures

Hi Dave,

On Nov 26, 2024 at 13:21:27 +0000, Dave Stevenson wrote:
> Hi Laurent
> 
> On Tue, 26 Nov 2024 at 12:16, Laurent Pinchart
> <laurent.pinchart@...asonboard.com> wrote:
> >
> > On Mon, Nov 25, 2024 at 08:36:26PM +0530, Jai Luthra wrote:
> > > From: Dave Stevenson <dave.stevenson@...pberrypi.com>
> > >
> > > The HBLANK control was read-only, and always configured such
> > > that the sensor HTS register was 3448. This limited the maximum
> > > exposure time that could be achieved to around 1.26 secs.
> > >
> > > Make HBLANK read/write so that the line time can be extended,
> > > and thereby allow longer exposures (and slower frame rates).
> > > Retain the overall HTS setting when changing modes rather than
> > > resetting it to a default.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@...asonboard.com>
> > > Signed-off-by: Jai Luthra <jai.luthra@...asonboard.com>
> > > ---
> > >  drivers/media/i2c/imx219.c | 37 ++++++++++++++++++++++++-------------
> > >  1 file changed, 24 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index f98aad74fe584a18e2fe7126f92bf294762a54e3..970e6362d0ae3a9078daf337155e83d637bc1ca1 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -76,8 +76,10 @@
> > >
> > >  #define IMX219_VBLANK_MIN            32
> > >
> > > -/* HBLANK control - read only */
> > > -#define IMX219_PPL_DEFAULT           3448
> > > +/* HBLANK control range */
> >
> > Just drop the comment, and drop the blank lines, this belongs to the
> > "V_TIMING internal" section.
> >
> > > +#define IMX219_PPL_MIN                       0x0d78
> >
> > Why PPL and not HTS ?
> 
> The IMX219 datasheet defines the register as LINE_LENGTH_A, with
> comment line_length_pck.
> 
> HTS is not a term used in the imx219 datasheet, so why introduce it to
> the driver? I'd go along with a rename to LLP if you wish.
> (HTS is more commonly an Omnivision term, not a Sony one).

The FRAME_LENGTH_A register is also named IMX219_REG_VTS in the driver, 
so I think calling this PPL would be confusing, unless we also rename 
VTS to something else (frame_length_lines -> so FLL?)

Just my two cents but VTS/HTS is at least instantly parsable to my 
eyes.. compared to the sony names.

> 
> > > +#define IMX219_PPL_MAX                       0x7ff0
> > > +#define IMX219_REG_HTS                       CCI_REG16(0x0162)
> >
> > The min/max should go below the register definition.
> >
> > >
> > >  #define IMX219_REG_LINE_LENGTH_A     CCI_REG16(0x0162)
> > >  #define IMX219_REG_X_ADD_STA_A               CCI_REG16(0x0164)
> > > @@ -422,6 +424,10 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > >               cci_write(imx219->regmap, IMX219_REG_VTS,
> > >                         format->height + ctrl->val, &ret);
> > >               break;
> > > +     case V4L2_CID_HBLANK:
> > > +             cci_write(imx219->regmap, IMX219_REG_HTS,
> > > +                       format->width + ctrl->val, &ret);
> > > +             break;
> > >       case V4L2_CID_TEST_PATTERN_RED:
> > >               cci_write(imx219->regmap, IMX219_REG_TESTP_RED,
> > >                         ctrl->val, &ret);
> > > @@ -496,12 +502,10 @@ static int imx219_init_controls(struct imx219 *imx219)
> > >                                          V4L2_CID_VBLANK, IMX219_VBLANK_MIN,
> > >                                          IMX219_VTS_MAX - mode->height, 1,
> > >                                          mode->vts_def - mode->height);
> > > -     hblank = IMX219_PPL_DEFAULT - mode->width;
> > > +     hblank = IMX219_PPL_MIN - mode->width;
> > >       imx219->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> > >                                          V4L2_CID_HBLANK, hblank, hblank,
> >
> > The minimum and maximum are identical, is this intentional ?
> 
> The limits should be updated when the format is set, so the values
> shouldn't matter when created. However I'd want to check that did
> happen seeing as vblank computes the limits here.
> It is as easy to set them correctly here too (max = IMX219_PPL_MAX -
> mode->width), so you may as well.
> 
> > >                                          1, hblank);
> > > -     if (imx219->hblank)
> > > -             imx219->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > >       exposure_max = mode->vts_def - 4;
> > >       exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
> > >               exposure_max : IMX219_EXPOSURE_DEFAULT;
> > > @@ -817,6 +821,10 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >       struct v4l2_mbus_framefmt *format;
> > >       struct v4l2_rect *crop;
> > >       unsigned int bin_h, bin_v;
> > > +     u32 prev_hts;
> > > +
> > > +     format = v4l2_subdev_state_get_format(state, 0);
> > > +     prev_hts = format->width + imx219->hblank->val;
> > >
> > >       mode = v4l2_find_nearest_size(supported_modes,
> > >                                     ARRAY_SIZE(supported_modes),
> > > @@ -824,8 +832,6 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >                                     fmt->format.width, fmt->format.height);
> > >
> > >       imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
> > > -
> > > -     format = v4l2_subdev_state_get_format(state, 0);
> > >       *format = fmt->format;
> > >
> > >       /*
> > > @@ -861,13 +867,18 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >                                        exposure_max, imx219->exposure->step,
> > >                                        exposure_def);
> > >               /*
> > > -              * Currently PPL is fixed to IMX219_PPL_DEFAULT, so hblank
> > > -              * depends on mode->width only, and is not changeble in any
> > > -              * way other than changing the mode.
> > > +              * Retain PPL setting from previous mode so that the
> >
> > Rename PPL to HTS here too.
> 
> Disagree as above. The local variable here should be renamed prev_ppl
> or prev_llp.
> 
>   Dave
> 
> > > +              * line time does not change on a mode change.
> > > +              * Limits have to be recomputed as the controls define
> > > +              * the blanking only, so PPL values need to have the
> > > +              * mode width subtracted.
> > >                */
> > > -             hblank = IMX219_PPL_DEFAULT - mode->width;
> > > -             __v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1,
> > > -                                      hblank);
> > > +             hblank = prev_hts - mode->width;
> > > +             __v4l2_ctrl_modify_range(imx219->hblank,
> > > +                                      IMX219_PPL_MIN - mode->width,
> > > +                                      IMX219_PPL_MAX - mode->width,
> > > +                                      1, IMX219_PPL_MIN - mode->width);
> > > +             __v4l2_ctrl_s_ctrl(imx219->hblank, hblank);
> > >       }
> > >
> > >       return 0;
> > >
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

Thanks,
Jai

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ