Re: [PATCH v4 03/29] media: imx219: Set horizontal blanking on mode change

Jacopo Mondi
2026-04-10T07:27:41Z

Hi Sakari

On Wed, Apr 08, 2026 at 06:39:12PM +0300, Sakari Ailus wrote:
> The driver UAPI is mode-based, allowing the user to choose a mode from a
> small list based on the output size. The vertical blanking is set based on
> the mode, do the same for horizontal blanking so the frame rate obtained
> is constant.
>
> Additionally, it's best to use a known-good horizontal blanking value as
> choosing the value freely may affect image quality. While the minimum
> value may not be the best value for horizontal blanking, at least it is
> constant rather than a minimum value of a different configuration.

As Dave suggested, we should probably better define the desired behaviour.

As far as I can see the driver doesn't specify a line lenght in the
supported_modes array, and I guess we're always running with the min
valid blanking. From a libcamera perspective only RPi changes the
HBLANK control value, all other pipelines use the default, so if
Dave's fine with this, I'm fine as well.

>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> ---
> drivers/media/i2c/imx219.c | 15 +++------------
> 1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 89061dc1842d..62a23541b1dc 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -837,11 +837,9 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> struct v4l2_mbus_framefmt *format;
> struct v4l2_rect *crop;
> u8 bin_h, bin_v, binning;
> - u32 prev_line_len;
> int ret;
>
> format = v4l2_subdev_state_get_format(state, 0);
> - prev_line_len = format->width + imx219->hblank->val;
>
> /*
> * Adjust the requested format to match the closest mode. The Bayer
> @@ -882,7 +880,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> int exposure_max;
> int exposure_def;
> - int hblank, llp_min;
> + int llp_min;
> int pixel_rate;
>
> /* Update limits and set FPS to default */
> @@ -924,15 +922,8 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> llp_min - mode->width);
> if (ret)
> return ret;
> - /*
> - * Retain PPL setting from previous mode so that the
> - * 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 = prev_line_len - mode->width;
> - ret = __v4l2_ctrl_s_ctrl(imx219->hblank, hblank);
> +
> + ret = __v4l2_ctrl_s_ctrl(imx219->hblank, llp_min - mode->width);
> if (ret)
> return ret;
>
> --
> 2.47.3
>
>