| Message ID | 20221107204959.37691-2-jacopo@jmondi.org |
|---|---|
| State | New |
| Headers | show |
| Series | Documentation: media: camera_sensor: Document blankings handling | expand |
Hi Jacopo. On Mon, 7 Nov 2022 at 20:50, Jacopo Mondi <jacopo@jmondi.org> 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). Dave [1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ccs/ccs-core.c#L734 > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > 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 >
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@jmondi.org> 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. > > Dave > > [1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ccs/ccs-core.c#L734 > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > 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 > >
Hi Jacopo On Tue, 8 Nov 2022 at 12:02, Jacopo Mondi <jacopo@jmondi.org> 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@jmondi.org> 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@jmondi.org> > > > --- > > > 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 > > >
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@jmondi.org> 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@jmondi.org> 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@jmondi.org> > > > > --- > > > > 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 > > > >
Hi Jacopo, On Mon, Nov 07, 2022 at 09:49:57PM +0100, Jacopo Mondi 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. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > 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. This would break frame interval computation for CCS and possibly for some other devices as well. Analogue crop effectively means leaving some parts of the pixel array unread, which is why it affects sensor timing. Digital crop, on the other hand, involves throwing out data later in the process and so it does not have such an effect on timing. There are two ways the issue could be addressed for drivers exposing a single sub-device: 1) adding FLL and LLP controls that are independent of analogue crop or 2) adding a special analogue crop rectangle. I think 1) is preferred, as it always has been somewhat cumbersome that you have to find the size the blanking controls are related using another interface. Some drivers could take digital crop into account in blanking values already but that is just working around the problem, although for single sub-device drivers there haven't been proper solutions provided either. > > Horizontal and vertical blanking are specified by ``V4L2_CID_HBLANK`` and > ``V4L2_CID_VBLANK``, respectively. The unit of the ``V4L2_CID_HBLANK`` control
Hi Jacopo On Sun, 13 Nov 2022 at 13:06, Jacopo Mondi <jacopo@jmondi.org> 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@jmondi.org> 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@jmondi.org> 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@jmondi.org> > > > > > --- > > > > > 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 > > > > >
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
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. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- Documentation/driver-api/media/camera-sensor.rst | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)