linux-kernel - Re: [PATCH v6 5/5] media: i2c: imx219: Scale the pixel rate for analog binning

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: <n544346jf6wbzvrewfqs53fi6vlilw2kqirm4rh6fg7pfexkss@gynbikwrhmdh>
Date: Tue, 11 Feb 2025 13:14:12 +0530
From: Jai Luthra <jai.luthra@...asonboard.com>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>, 
	Naushir Patuck <naush@...pberrypi.com>
Cc: Dave Stevenson <dave.stevenson@...pberrypi.com>, 
	Sakari Ailus <sakari.ailus@...ux.intel.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Laurent Pinchart <laurent.pinchart@...asonboard.com>, Vinay Varma <varmavinaym@...il.com>
Subject: Re: [PATCH v6 5/5] media: i2c: imx219: Scale the pixel rate for
 analog binning

Hi Jacopo, Naush,

On Feb 07, 2025 at 17:49:19 +0100, Jacopo Mondi wrote:
> Hi Jai
> 
> On Tue, Feb 04, 2025 at 12:34:40PM +0530, Jai Luthra wrote:
> > When the analog binning mode is used for high framerate operation, the
> > pixel rate is effectively doubled. Account for this when setting up the
> > pixel clock rate, and applying the vblank and exposure controls.
> >
> > The previous logic only used analog binning for RAW8, but normal binning
> > limits the framerate on RAW10 480p [1]. So with this patch we switch to
> > using special binning (with 2x pixel rate) wherever possible.
> >
> > [1]: https://github.com/raspberrypi/linux/issues/5493
> >
> > Co-developed-by: Naushir Patuck <naush@...pberrypi.com>
> > Signed-off-by: Naushir Patuck <naush@...pberrypi.com>
> > Co-developed-by: Vinay Varma <varmavinaym@...il.com>
> > Signed-off-by: Vinay Varma <varmavinaym@...il.com>
> > Signed-off-by: Jai Luthra <jai.luthra@...asonboard.com>
> > ---

[snip]

> > @@ -367,10 +426,12 @@ static int imx219_set_ctrl(struct v4l2_ctrl 
> > *ctrl)
> >  	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> >  	const struct v4l2_mbus_framefmt *format;
> >  	struct v4l2_subdev_state *state;
> > +	u32 rate_factor;
> >  	int ret = 0;
> >
> >  	state = v4l2_subdev_get_locked_active_state(&imx219->sd);
> >  	format = v4l2_subdev_state_get_format(state, 0);
> > +	rate_factor = imx219_get_rate_factor(imx219);
> >
> >  	if (ctrl->id == V4L2_CID_VBLANK) {
> >  		int exposure_max, exposure_def;
> > @@ -399,7 +460,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> >  		break;
> >  	case V4L2_CID_EXPOSURE:
> >  		cci_write(imx219->regmap, IMX219_REG_EXPOSURE,
> > -			  ctrl->val, &ret);
> > +			  ctrl->val / rate_factor, &ret);
> >  		break;
> >  	case V4L2_CID_DIGITAL_GAIN:
> >  		cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN,
> > @@ -416,7 +477,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> >  		break;
> >  	case V4L2_CID_VBLANK:
> >  		cci_write(imx219->regmap, IMX219_REG_FRM_LENGTH_A,
> > -			  format->height + ctrl->val, &ret);
> > +			  (format->height + ctrl->val) / rate_factor, &ret);
> 
> 
> Isn't this (and exposure) compensatd by the doubled pixel rate ?
> 

The datasheet mentions that FRM_LENGTH_A register is in units of 2xLines 
when analog binning mode is selected. And the exposure is also usually 
in unit of lines, so I assume that is why the same division was made in 
the original commit by Naush [1]

[1] https://github.com/raspberrypi/linux/commit/caebe4fe817b5079

> Applications use the pixel rate to compute the line duration and from
> there transform the frame duration and the exposure in lines, don't
> they ?

While this change doesn't update the user-side of the control values, 
only the register values, I wonder if there is a clean way to handle 
this without updating some assumptions in the application.

The IMX219 pixel clock behaves differently with analog binning compared 
to (most of our) intuitions, where rather than doubling the horizontal 
pixel clock, each line is still read-out in the same time but the number 
of lines read are halved.. at least that's the best explanation I have 
from these results:
https://lore.kernel.org/linux-media/zla2sogd7ov3yz2k2je6zrgh3uzeermowlaixt3qkcioturppo@tswbw354tpdk/

And that is why the total pixel rate is doubled, but the actual line 
duration should be the same as a digitally binned or cropped format.

> 
> Overall, very nice to be able to double the achievable frame rate
> without any artifacts! Good job!
> 
> Thanks
>   j
> 

Thanks,
Jai

> >  		break;
> >  	case V4L2_CID_HBLANK:
> >  		cci_write(imx219->regmap, IMX219_REG_LINE_LENGTH_A,

[snip]