Hi Dave On Thu, Nov 10, 2022 at 07:38:29AM +0000, Dave Stevenson wrote: > Hi Jacopo > > On Tue, 8 Nov 2022 at 12:02, Jacopo Mondi <jacopo@xxxxxxxxxx> wrote: > > > > Hi Dave > > thanks for the reply > > > > On Tue, Nov 08, 2022 at 11:41:58AM +0000, Dave Stevenson wrote: > > > Hi Jacopo. > > > > > > On Mon, 7 Nov 2022 at 20:50, Jacopo Mondi <jacopo@xxxxxxxxxx> wrote: > > > > > > > > The formula to compute the frame interval uses the analogue crop > > > > rectangle dimensions to compute the total frame size in conjunction with > > > > blankings. > > > > > > > > Horizontal and vertical blankings are realized by extending the time > > > > interval during which no valid pixels are sent on the bus between > > > > visible lines and between consecutive frames, whose size is smaller than > > > > the analogue crop rectangle if any additional cropping or pixel > > > > subsampling is applied on the sensor processing pipeline. > > > > > > > > Correct the documentation to use the visible line length and frame > > > > height instead of the analogue crop dimensions. > > > > > > I'll defer to Sakari on this, but I think the original text is correct. > > > > > > Consider something like CCS where you have a separate array with > > > analogue crop, and then binning and scaling steps. AIUI the pixel rate > > > and [HV]BLANK will be defined for the array, not on the binned and > > > scaled values which finally give the visible frame. See [1]. > > > > > > For the majority of sensors where analogue cropping, binning, and > > > scaling are not broken out separately, then it may well have been > > > incorrectly implemented as they do often look at the output > > > width/height. Should they be using the w/h values returned by > > > V4L2_SEL_TGT_CROP on V4L2_SUBDEV_FORMAT_ACTIVE for the sensor modes > > > instead? Quite probably, but that also makes the userspace more > > > complex (and probably needs fixing). > > > > More or less this was my reasoning, please see the cover letter. > > A thought came to me on this one. > If defined as the output width/height of the subdev implementing the > blanking controls, then both CCS and the majority are correct as they > are. I presume so as well.. > > Doing so also avoids a load of mess in drivers where H & V binning > each double the pixel rate if VBLANK/HBLANK are with regard the > analogue cropped area, which results in more controls to update in the > driver, and more to reread in userspace. > So do you think I should continue using the "analogue crop rectangle" in the documentation as the reference for blankings calculations even if most drivers do not actually use it ? IOW drop the first patch and use a more generic "The new maximum limit for the controls should be re-calculated using the newly applied s/visibile width and heigh/dimensions/" in 2/3 ? Thanks for the review ;) > Cheers. > Dave > > > > > > > Dave > > > > > > [1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ccs/ccs-core.c#L734 > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx> > > > > --- > > > > Documentation/driver-api/media/camera-sensor.rst | 7 +++---- > > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst > > > > index c7d4891bd24e..bb7d62db4cd1 100644 > > > > --- a/Documentation/driver-api/media/camera-sensor.rst > > > > +++ b/Documentation/driver-api/media/camera-sensor.rst > > > > @@ -87,12 +87,11 @@ less all modern raw camera sensors. > > > > > > > > The frame interval is calculated using the following equation:: > > > > > > > > - frame interval = (analogue crop width + horizontal blanking) * > > > > - (analogue crop height + vertical blanking) / pixel rate > > > > + frame interval = (visible width + horizontal blanking) * > > > > + (visibile height + vertical blanking) / pixel rate > > > > > > > > The formula is bus independent and is applicable for raw timing parameters on > > > > -large variety of devices beyond camera sensors. Devices that have no analogue > > > > -crop, use the full source image size, i.e. pixel array size. > > > > +large variety of devices beyond camera sensors. > > > > > > > > Horizontal and vertical blanking are specified by ``V4L2_CID_HBLANK`` and > > > > ``V4L2_CID_VBLANK``, respectively. The unit of the ``V4L2_CID_HBLANK`` control > > > > -- > > > > 2.38.1 > > > >
