Re: [PATCH v2 23/25] media: i2c: imx283: Fix binned mode blanking timings

[Jai Luthra]

Hi Kieran,

Thanks for the patch!

Quoting Kieran Bingham (2026-02-13 19:32:02)
> The IMX283 supports binning modes which combine multiple measured pixels
> into a single output pixel.
> 
> The minimum timings for this must account for the operations on all
> measured pixels, not the output pixel sizes.
> 
> Determine and calculate all hmax and vmax values in respect of the
> HBLANK and VBLANK controls against the native resolution of the mode as
> specified in the mode crop rectangle as opposed to the output mode width
> and height.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> ---
>  drivers/media/i2c/imx283.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
> index 7fb654512c20..25e669370751 100644
> --- a/drivers/media/i2c/imx283.c
> +++ b/drivers/media/i2c/imx283.c
> @@ -875,7 +875,7 @@ static int imx283_set_ctrl(struct v4l2_ctrl *ctrl)
>                 /* Honour the VBLANK limits when setting exposure. */
>                 s64 current_exposure, max_exposure, min_exposure;
>  
> -               imx283->vmax = mode->height + ctrl->val;
> +               imx283->vmax = mode->crop.height + ctrl->val;
>  
>                 imx283_exposure_limits(imx283, mode,
>                                        &min_exposure, &max_exposure);
> @@ -905,7 +905,7 @@ static int imx283_set_ctrl(struct v4l2_ctrl *ctrl)
>  
>         case V4L2_CID_HBLANK:
>                 pixel_rate = imx283_pixel_rate(imx283, mode);
> -               imx283->hmax = imx283_internal_clock(pixel_rate, mode->width + ctrl->val);
> +               imx283->hmax = imx283_internal_clock(pixel_rate, mode->crop.width + ctrl->val);
>                 dev_dbg(imx283->dev, "V4L2_CID_HBLANK : %d  HMAX : %u\n",
>                         ctrl->val, imx283->hmax);
>                 ret = cci_write(imx283->cci, IMX283_REG_HMAX, imx283->hmax, NULL);
> @@ -917,7 +917,7 @@ static int imx283_set_ctrl(struct v4l2_ctrl *ctrl)
>                 break;
>  
>         case V4L2_CID_VBLANK:
> -               imx283->vmax = mode->height + ctrl->val;
> +               imx283->vmax = mode->crop.height + ctrl->val;
>                 dev_dbg(imx283->dev, "V4L2_CID_VBLANK : %d  VMAX : %u\n",
>                         ctrl->val, imx283->vmax);
>                 ret = cci_write(imx283->cci, IMX283_REG_VMAX, imx283->vmax, NULL);
> @@ -1042,6 +1042,9 @@ static void imx283_set_framing_limits(struct imx283 *imx283,
>         u64 pixel_rate = imx283_pixel_rate(imx283, mode);
>         u64 min_hblank, max_hblank, def_hblank;
>  
> +       /* Use crop for timings from native sensor units */
> +       const struct v4l2_rect *crop = &mode->crop;
> +
>         /* Initialise hmax and vmax for exposure calculations */
>         imx283->hmax = imx283_internal_clock(pixel_rate, mode->default_hmax);
>         imx283->vmax = mode->default_vmax;
> @@ -1050,18 +1053,18 @@ static void imx283_set_framing_limits(struct imx283 *imx283,
>          * Horizontal Blanking
>          * Convert the HMAX_MAX (72MHz) to Pixel rate values for HBLANK_MAX
>          */
> -       min_hblank = mode->min_hmax - mode->width;
> -       max_hblank = imx283_iclk_to_pix(pixel_rate, IMX283_HMAX_MAX) - mode->width;
> -       def_hblank = mode->default_hmax - mode->width;
> +       min_hblank = mode->min_hmax - crop->width;
> +       max_hblank = imx283_iclk_to_pix(pixel_rate, IMX283_HMAX_MAX) - crop->width;
> +       def_hblank = mode->default_hmax - crop->width;
>         __v4l2_ctrl_modify_range(imx283->hblank, min_hblank, max_hblank, 1,
>                                  def_hblank);
>         __v4l2_ctrl_s_ctrl(imx283->hblank, def_hblank);
>  
>         /* Vertical Blanking */
> -       __v4l2_ctrl_modify_range(imx283->vblank, mode->min_vmax - mode->height,
> -                                IMX283_VMAX_MAX - mode->height, 1,
> +       __v4l2_ctrl_modify_range(imx283->vblank, mode->min_vmax - crop->height,
> +                                IMX283_VMAX_MAX - crop->height, 1,
>                                  mode->default_vmax - mode->height);

Shouldn't this be `- crop->height` too?

> -       __v4l2_ctrl_s_ctrl(imx283->vblank, mode->default_vmax - mode->height);
> +       __v4l2_ctrl_s_ctrl(imx283->vblank, mode->default_vmax - crop->height);
>  }
>  
>  static int imx283_set_pad_format(struct v4l2_subdev *sd,
> @@ -1509,13 +1512,13 @@ static int imx283_init_controls(struct imx283 *imx283)
>         /* Initialise vblank/hblank/exposure based on the current mode. */
>         imx283->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx283_ctrl_ops,
>                                            V4L2_CID_VBLANK,
> -                                          mode->min_vmax - mode->height,
> +                                          mode->min_vmax - mode->crop.height,
>                                            IMX283_VMAX_MAX, 1,
> -                                          mode->default_vmax - mode->height);
> +                                          mode->default_vmax - mode->crop.height);
>  
> -       min_hblank = mode->min_hmax - mode->width;
> -       max_hblank = imx283_iclk_to_pix(pixel_rate, IMX283_HMAX_MAX) - mode->width;
> -       def_hblank = mode->default_hmax - mode->width;
> +       min_hblank = mode->min_hmax - mode->crop.width;
> +       max_hblank = imx283_iclk_to_pix(pixel_rate, IMX283_HMAX_MAX) - mode->crop.width;
> +       def_hblank = mode->default_hmax - mode->crop.width;
>         imx283->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx283_ctrl_ops,
>                                            V4L2_CID_HBLANK, min_hblank, max_hblank,
>                                            1, def_hblank);
> 
> -- 
> 2.52.0
> 

Testing this series on Pi 5, I notice that this patch causes a regression.

Before this patch I could get ~45 and ~60fps for the 2x and 3x binned
modes, while now I only get ~20fps.

----- thinking out loud -------
The sensor expects HMAX to be programmed in units of sensor's internal
readout clock @ 72MHz.

This is the supported minimum for HMAX for various readout modes:

No binning:     887 units per line
2x2 binning:    362 ..
3x3 binning:    284 ..

Scaling these up from "72 Mhz" units to the pixel rate with 720Mhz link
frequency on Pi 5 (that is 480Mpixels/sec), I get:

No binning:     5914 pixels per line
2x2 binning:    2414 .. (*)
3x3 binning:    1894 ..

Which matches the min_hmax described in the mode table.

(*) 2414 < 2736 (width of the 2x2 mode) explains why I can only capture
max 45fps instead of intended 50fps. The sensor might be able to read a
line faster than what the link rate of 720Mhz supports.
------------------------------

Looking at the patch again

> -       min_hblank = mode->min_hmax - mode->width;
> +       min_hblank = mode->min_hmax - crop->width;

hblank = min_hmax (1894 for 3x binning) - crop.width (5472) is negative

> -               imx283->hmax = imx283_internal_clock(pixel_rate, mode->width + ctrl->val);
> +               imx283->hmax = imx283_internal_clock(pixel_rate, mode->crop.width + ctrl->val);

Passing "pixels" in crop dimensions to the internal_clock function seems
wrong to me.

What exactly do we get by representing HMAX/VMAX in "full resolution" pixel
units?

TBH both this new approach and the older one make it too confusing. The
sensor's HMAX is in the unit of "time" w.r.t the 72Mhz internal clock,
regardless of the pixel rate/link frequency.

IMHO the mode table should store that information directly, instead of in
units of pre or post-binned pixels, that too for an arbitrary link
frequency.

Thanks,
    Jai