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