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

Dave Stevenson
2026-04-16T14:38:18Z

Hi Sakari and Jacopo

On Fri, 10 Apr 2026 at 08:27, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> 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.

The folks that it's more likely to catch out are those that are
driving the sensor directly with V4L2 in a video-device centred world,
not those using libcamera. They are in a definite minority though.
Overall I'm happy enough that it won't cause any real issues. I've
already given a Reviewed-by tag.

Dave

> >
> > 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
> >
> >