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] [day] [month] [year] [list]
Message-ID: <CAPY8ntCy94L+5vCu20Ed_dV3nchaxF4Qmy1BZ_HD7sWnaQmy5w@mail.gmail.com>
Date: Tue, 28 Jan 2025 18:08:19 +0000
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Michael Riesch <michael.riesch@...fvision.net>, Mauro Carvalho Chehab <mchehab@...nel.org>
Cc: Gerald Loacker <Gerald.Loacker@...fvision.net>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] media: i2c: imx415: Make HBLANK controllable and
 in consistent units

On Thu, 16 Jan 2025 at 17:43, Dave Stevenson
<dave.stevenson@...pberrypi.com> wrote:
>
> The control of HMAX documented in the datasheet is consistent
> with being in terms of a scaled INCK, being always 72MHz or
> 74.25MHz. It is NOT link frequency dependent, but the minimum
> value for HMAX is dictated by the link frequency.
>
> If PIXEL_RATE is defined as being 12 times the 72 or 74.25MHz,
> and all values are scaled down again when writing HMAX, then
> the numbers all work out regardless of INCK or link frequency.
> Retain an hmax_min (set to the same value as the previous fixed
> hmax register value) to set as the default value to avoid changing
> the behaviour for existing users.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> ---
>  drivers/media/i2c/imx415.c | 87 +++++++++++++++++++++-------------------------
>  1 file changed, 39 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/media/i2c/imx415.c b/drivers/media/i2c/imx415.c
> index fa7ffb9220e5..24633d17cb09 100644
> --- a/drivers/media/i2c/imx415.c
> +++ b/drivers/media/i2c/imx415.c
> @@ -28,6 +28,9 @@
>  #define IMX415_PIXEL_ARRAY_VBLANK 58
>  #define IMX415_EXPOSURE_OFFSET   8
>
> +#define IMX415_PIXEL_RATE_74_25MHZ     891000000
> +#define IMX415_PIXEL_RATE_72MHZ                864000000
> +
>  #define IMX415_NUM_CLK_PARAM_REGS 11
>
>  #define IMX415_MODE              CCI_REG8(0x3000)
> @@ -54,6 +57,8 @@
>  #define IMX415_VMAX              CCI_REG24_LE(0x3024)
>  #define IMX415_VMAX_MAX                  0xfffff
>  #define IMX415_HMAX              CCI_REG16_LE(0x3028)
> +#define IMX415_HMAX_MAX                  0xffff
> +#define IMX415_HMAX_MULTIPLIER   12
>  #define IMX415_SHR0              CCI_REG24_LE(0x3050)
>  #define IMX415_GAIN_PCG_0        CCI_REG16_LE(0x3090)
>  #define IMX415_AGAIN_MIN         0
> @@ -449,7 +454,6 @@ static const struct imx415_clk_params imx415_clk_params[] = {
>
>  /* all-pixel 2-lane 720 Mbps 15.74 Hz mode */
>  static const struct cci_reg_sequence imx415_mode_2_720[] = {
> -       { IMX415_HMAX, 0x07F0 },
>         { IMX415_LANEMODE, IMX415_LANEMODE_2 },
>         { IMX415_TCLKPOST, 0x006F },
>         { IMX415_TCLKPREPARE, 0x002F },
> @@ -464,7 +468,6 @@ static const struct cci_reg_sequence imx415_mode_2_720[] = {
>
>  /* all-pixel 2-lane 1440 Mbps 30.01 Hz mode */
>  static const struct cci_reg_sequence imx415_mode_2_1440[] = {
> -       { IMX415_HMAX, 0x042A },
>         { IMX415_LANEMODE, IMX415_LANEMODE_2 },
>         { IMX415_TCLKPOST, 0x009F },
>         { IMX415_TCLKPREPARE, 0x0057 },
> @@ -479,7 +482,6 @@ static const struct cci_reg_sequence imx415_mode_2_1440[] = {
>
>  /* all-pixel 4-lane 891 Mbps 30 Hz mode */
>  static const struct cci_reg_sequence imx415_mode_4_891[] = {
> -       { IMX415_HMAX, 0x044C },
>         { IMX415_LANEMODE, IMX415_LANEMODE_4 },
>         { IMX415_TCLKPOST, 0x007F },
>         { IMX415_TCLKPREPARE, 0x0037 },
> @@ -497,39 +499,10 @@ struct imx415_mode_reg_list {
>         const struct cci_reg_sequence *regs;
>  };
>
> -/*
> - * Mode : number of lanes, lane rate and frame rate dependent settings
> - *
> - * pixel_rate and hmax_pix are needed to calculate hblank for the v4l2 ctrl
> - * interface. These values can not be found in the data sheet and should be
> - * treated as virtual values. Use following table when adding new modes.
> - *
> - * lane_rate  lanes    fps     hmax_pix   pixel_rate
> - *
> - *     594      2     10.000     4400       99000000
> - *     891      2     15.000     4400      148500000
> - *     720      2     15.748     4064      144000000
> - *    1782      2     30.000     4400      297000000
> - *    2079      2     30.000     4400      297000000
> - *    1440      2     30.019     4510      304615385
> - *
> - *     594      4     20.000     5500      247500000
> - *     594      4     25.000     4400      247500000
> - *     720      4     25.000     4400      247500000
> - *     720      4     30.019     4510      304615385
> - *     891      4     30.000     4400      297000000
> - *    1440      4     30.019     4510      304615385
> - *    1440      4     60.038     4510      609230769
> - *    1485      4     60.000     4400      594000000
> - *    1782      4     60.000     4400      594000000
> - *    2079      4     60.000     4400      594000000
> - *    2376      4     90.164     4392      891000000
> - */
>  struct imx415_mode {
>         u64 lane_rate;
>         u32 lanes;
> -       u32 hmax_pix;
> -       u64 pixel_rate;
> +       u32 hmax_min;
>         struct imx415_mode_reg_list reg_list;
>  };
>
> @@ -538,8 +511,7 @@ static const struct imx415_mode supported_modes[] = {
>         {
>                 .lane_rate = 720000000,
>                 .lanes = 2,
> -               .hmax_pix = 4064,
> -               .pixel_rate = 144000000,
> +               .hmax_min = 2032,
>                 .reg_list = {
>                         .num_of_regs = ARRAY_SIZE(imx415_mode_2_720),
>                         .regs = imx415_mode_2_720,
> @@ -548,8 +520,7 @@ static const struct imx415_mode supported_modes[] = {
>         {
>                 .lane_rate = 1440000000,
>                 .lanes = 2,
> -               .hmax_pix = 4510,
> -               .pixel_rate = 304615385,
> +               .hmax_min = 1066,
>                 .reg_list = {
>                         .num_of_regs = ARRAY_SIZE(imx415_mode_2_1440),
>                         .regs = imx415_mode_2_1440,
> @@ -558,8 +529,7 @@ static const struct imx415_mode supported_modes[] = {
>         {
>                 .lane_rate = 891000000,
>                 .lanes = 4,
> -               .hmax_pix = 4400,
> -               .pixel_rate = 297000000,
> +               .hmax_min = 1100,
>                 .reg_list = {
>                         .num_of_regs = ARRAY_SIZE(imx415_mode_4_891),
>                         .regs = imx415_mode_4_891,
> @@ -586,6 +556,7 @@ static const char *const imx415_test_pattern_menu[] = {
>  struct imx415 {
>         struct device *dev;
>         struct clk *clk;
> +       unsigned long pixel_rate;
>         struct regulator_bulk_data supplies[ARRAY_SIZE(imx415_supply_names)];
>         struct gpio_desc *reset;
>         struct regmap *regmap;
> @@ -597,6 +568,7 @@ struct imx415 {
>
>         struct v4l2_ctrl_handler ctrls;
>         struct v4l2_ctrl *vblank;
> +       struct v4l2_ctrl *hblank;
>         struct v4l2_ctrl *hflip;
>         struct v4l2_ctrl *vflip;
>         struct v4l2_ctrl *exposure;
> @@ -787,6 +759,12 @@ static int imx415_s_ctrl(struct v4l2_ctrl *ctrl)
>                 ret = imx415_set_testpattern(sensor, ctrl->val);
>                 break;
>
> +       case V4L2_CID_HBLANK:
> +               return cci_write(sensor->regmap, IMX415_HMAX,
> +                                (format->width + ctrl->val) /
> +                                               IMX415_HMAX_MULTIPLIER,
> +                                NULL);
> +

Come back to a patch and the obvious errors jump out.
This function has a pm_runtime_put at the end, so we can't just return.

v3 tomorrow.

  Dave

>         default:
>                 ret = -EINVAL;
>                 break;
> @@ -805,12 +783,11 @@ static int imx415_ctrls_init(struct imx415 *sensor)
>  {
>         struct v4l2_fwnode_device_properties props;
>         struct v4l2_ctrl *ctrl;
> -       u64 pixel_rate = supported_modes[sensor->cur_mode].pixel_rate;
>         u64 lane_rate = supported_modes[sensor->cur_mode].lane_rate;
>         u32 exposure_max = IMX415_PIXEL_ARRAY_HEIGHT +
>                            IMX415_PIXEL_ARRAY_VBLANK -
>                            IMX415_EXPOSURE_OFFSET;
> -       u32 hblank;
> +       u32 hblank_min, hblank_max;
>         unsigned int i;
>         int ret;
>
> @@ -847,12 +824,14 @@ static int imx415_ctrls_init(struct imx415 *sensor)
>                           IMX415_AGAIN_MAX, IMX415_AGAIN_STEP,
>                           IMX415_AGAIN_MIN);
>
> -       hblank = supported_modes[sensor->cur_mode].hmax_pix -
> -                IMX415_PIXEL_ARRAY_WIDTH;
> +       hblank_min = (supported_modes[sensor->cur_mode].hmax_min *
> +                     IMX415_HMAX_MULTIPLIER) - IMX415_PIXEL_ARRAY_WIDTH;
> +       hblank_max = (IMX415_HMAX_MAX * IMX415_HMAX_MULTIPLIER) -
> +                    IMX415_PIXEL_ARRAY_WIDTH;
>         ctrl = v4l2_ctrl_new_std(&sensor->ctrls, &imx415_ctrl_ops,
> -                                V4L2_CID_HBLANK, hblank, hblank, 1, hblank);
> -       if (ctrl)
> -               ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +                                V4L2_CID_HBLANK, hblank_min,
> +                                hblank_max, IMX415_HMAX_MULTIPLIER,
> +                                hblank_min);
>
>         sensor->vblank = v4l2_ctrl_new_std(&sensor->ctrls, &imx415_ctrl_ops,
>                                            V4L2_CID_VBLANK,
> @@ -860,8 +839,9 @@ static int imx415_ctrls_init(struct imx415 *sensor)
>                                            IMX415_VMAX_MAX - IMX415_PIXEL_ARRAY_HEIGHT,
>                                            1, IMX415_PIXEL_ARRAY_VBLANK);
>
> -       v4l2_ctrl_new_std(&sensor->ctrls, NULL, V4L2_CID_PIXEL_RATE, pixel_rate,
> -                         pixel_rate, 1, pixel_rate);
> +       v4l2_ctrl_new_std(&sensor->ctrls, NULL, V4L2_CID_PIXEL_RATE,
> +                         sensor->pixel_rate, sensor->pixel_rate, 1,
> +                         sensor->pixel_rate);
>
>         sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls, &imx415_ctrl_ops,
>                                           V4L2_CID_HFLIP, 0, 1, 1, 0);
> @@ -1333,6 +1313,17 @@ static int imx415_parse_hw_config(struct imx415 *sensor)
>                                     "no valid sensor mode defined\n");
>                 goto done_endpoint_free;
>         }
> +       switch (inck) {
> +       case 27000000:
> +       case 37125000:
> +       case 74250000:
> +               sensor->pixel_rate = IMX415_PIXEL_RATE_74_25MHZ;
> +               break;
> +       case 24000000:
> +       case 72000000:
> +               sensor->pixel_rate = IMX415_PIXEL_RATE_72MHZ;
> +               break;
> +       }
>
>         lane_rate = supported_modes[sensor->cur_mode].lane_rate;
>         for (i = 0; i < ARRAY_SIZE(imx415_clk_params); ++i) {
>
> --
> 2.34.1
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ