Hi Jacopo On Sun, 13 Nov 2022 at 13:06, Jacopo Mondi <jacopo@xxxxxxxxxx> wrote: > > 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 I was thinking along the lines of: frame interval = (subdevice output width + horizontal blanking) * (subdevice output height + vertical blanking) / pixel rate Just checking and CCS appears to have 3 subdevices: pixel_array -> binner -> scaler -> (OUTPUT). V4L2_CID_VBLANK exists on the pixel array, and therefore defining it as the width/height on the output of that subdev is the same as specifying "analogue crop rectangle". > "The new maximum limit for the controls should be re-calculated using the > newly applied s/visibile width and heigh/dimensions/" > > in 2/3 ? IMHO Patch 2/3 doesn't need to specify the basis on which the range should be recomputed - there may be sensors that don't follow the common pattern. You ought to say that the minimum value can change as well, eg if you change binning settings whilst changing mode then often the required minimum (H or V) is altered. Dave > 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 > > > > >
