Re: [PATCH v6 03/16] media: imx219: Account for rate_factor in control steps

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Jul 20, 2026 at 12:00:35AM +0300, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Fri, Jul 17, 2026 at 05:21:21PM +0300, Laurent Pinchart wrote:
> > Hi Sakari,
> > 
> > Thank you for the patch.
> > 
> > On Wed, Jul 01, 2026 at 03:26:20PM +0300, Sakari Ailus wrote:
> > > The controls that are divided by the rate_factor before writing them to
> > > the registers have the step of the value of the rate_factor. Take this
> > > into account when the control's range is modified. The controls are
> > > created in a configuration where rate_factor is always 1, hence there's no
> > > need to change the code adding new controls.
> > > 
> > > Fixes: f513997119f4 ("media: i2c: imx219: Scale the pixel rate for analog binning")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/media/i2c/imx219.c | 18 ++++++++----------
> > >  1 file changed, 8 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index 05d9737bdc95..2aab6e7180d4 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -319,19 +319,19 @@ static const struct imx219_mode supported_modes[] = {
> > >  		/* 1080P 30fps cropped */
> > >  		.width = 1920,
> > >  		.height = 1080,
> > > -		.fll_def = 1763,
> > > +		.fll_def = 1762,
> > 
> > This is not a binned mode, an odd default value is not incorrect. Is
> > there a reason to change it ?
> 
> The fll_def field is used to calculate the VBLANK control range and odd
> number here results in an odd number for the default, which is invalid and
> so __v4l2_ctrl_modify_range() will fail as a result.

It's only invalid if the step is 2, isn't it ? The 1920x1080 mode
doesn't use binning, so the rate factor should be 1, and the step should
therefore be 1 as well as far as I understand. Am I missing something ?

> The register value isn't changed whereas the value that is visible to the
> userspace does change.
> 
> > 
> > >  	},
> > >  	{
> > >  		/* 2x2 binned 60fps mode */
> > >  		.width = 1640,
> > >  		.height = 1232,
> > > -		.fll_def = 1707,
> > > +		.fll_def = 1706,
> > >  	},
> > >  	{
> > >  		/* 640x480 60fps mode */
> > >  		.width = 640,
> > >  		.height = 480,
> > > -		.fll_def = 1707,
> > > +		.fll_def = 1706,
> > 
> > Those two are fine as they won't (AFAIU) change register values.
> > 
> > >  	},
> > >  };
> > >  
> > > @@ -458,8 +458,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > >  		ret = __v4l2_ctrl_modify_range(imx219->exposure,
> > >  					       imx219->exposure->minimum,
> > >  					       exposure_max,
> > > -					       imx219->exposure->step,
> > > -					       exposure_def);
> > > +					       rate_factor, exposure_def);
> > 
> > The change is simple so I'm fine with it, but in general I'm not sure
> > this would be worth it if it were to make the code more complex.
> > Userspace can benefit from getting a precise step value, as well as
> > seeing the control value being adjusted when setting controls. However,
> > dealing with limit updates reported through events is quite cumbersome
> > for userspace. It's a big change and not a high priority, but I'd like
> > an API where ioctls such as VIDIOC_SUBDEV_S_FMT would have the ability
> > to report control changes synchronously.
> 
> I wonder if VIDIOC_S_EXT_EXT_CTRLS could do that. :-)

It wouldn't, we would need to have new versions of all ioctls that can
result in controls being changed. That would be quick a big modification
of the API.

> > With the change to the first fll_def possibly dropped,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thank you.
> 
> > >  		if (ret)
> > >  			return ret;
> > >  
> > > @@ -887,7 +886,8 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >  
> > >  		/* Update limits and set FPS to default */
> > >  		ret = __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > > -					       IMX219_FLL_MAX - mode->height, 1,
> > > +					       IMX219_FLL_MAX - mode->height,
> > > +					       rate_factor,
> > >  					       mode->fll_def - mode->height);
> > >  		if (ret)
> > >  			return ret;
> > > @@ -905,8 +905,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >  		ret = __v4l2_ctrl_modify_range(imx219->exposure,
> > >  					       imx219->exposure->minimum,
> > >  					       exposure_max,
> > > -					       imx219->exposure->step,
> > > -					       exposure_def);
> > > +					       rate_factor, exposure_def);
> > >  		if (ret)
> > >  			return ret;
> > >  
> > > @@ -931,8 +930,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >  			return ret;
> > >  
> > >  		/* Scale the pixel rate based on the mode specific factor */
> > > -		pixel_rate = imx219_get_pixel_rate(imx219) *
> > > -			     imx219_get_rate_factor(state);
> > > +		pixel_rate = imx219_get_pixel_rate(imx219) * rate_factor;
> > >  		ret = __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate,
> > >  					       pixel_rate, 1, pixel_rate);
> > >  		if (ret)

-- 
Regards,

Laurent Pinchart