Hi Jacopo,
On Mon, Nov 20, 2023 at 07:47:11PM +0100, Jacopo Mondi wrote:
> Hi Sakari
>
> On Mon, Nov 20, 2023 at 06:19:46PM +0000, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Mon, Nov 20, 2023 at 05:49:30PM +0100, Jacopo Mondi wrote:
> > > Hi Sakari
> > >
> > > On Mon, Nov 20, 2023 at 08:25:45AM +0000, Sakari Ailus wrote:
> > > > Hi Jacopo,
> > > >
> > > > Just found this old e-mail I apparently forgot to reply...
> > > >
> > >
> > > This really fell into the cracks for me as well
> > >
> > > > On Thu, Jul 27, 2023 at 11:42:45AM +0200, Jacopo Mondi wrote:
> > > > > Hi Sakari
> > > > >
> > > > > On Mon, Jul 17, 2023 at 10:24:21AM +0000, Sakari Ailus wrote:
> > > > > > Hi Jacopo,
> > > > > >
> > > > > > On Mon, Jul 10, 2023 at 03:22:40PM +0200, Jacopo Mondi wrote:
> > > > > > > Document the suggested way to exposure controls for exposure and gain
> > > > > > > for camera sensor drivers.
> > > > > > >
> > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > > > > > > Reviewed-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > > .../driver-api/media/camera-sensor.rst | 27 +++++++++++++++++++
> > > > > > > 1 file changed, 27 insertions(+)
> > > > > > >
> > > > > > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > > > > > > index cd915ca119ea..67fe77b1edb9 100644
> > > > > > > --- a/Documentation/driver-api/media/camera-sensor.rst
> > > > > > > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > > > > > > @@ -189,3 +189,30 @@ the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the
> > > > > > > a flip can potentially change the output buffer content layout. Flips should
> > > > > > > also be taken into account when enumerating and handling media bus formats
> > > > > > > on the camera sensor source pads.
> > > > > > > +
> > > > > > > +Exposure and Gain Control
> > > > > > > +-------------------------
> > > > > > > +
> > > > > > > +Camera sensor drivers that allow applications to control the image exposure
> > > > > > > +and gain should do so by exposing dedicated controls to applications.
> > > > > > > +
> > > > > > > +Exposure time is controlled by registering the ``V4L2_CID_EXPOSURE`` control.
> > > > > > > +The control definition does not specify a unit to allow maximum flexibility
> > > > > > > +for multiple device types, but when used for camera sensor drivers it should be
> > > > > > > +expressed in unit of lines whenever possible.
> > > > > >
> > > > > > This part of the documentation applies to both raw and SoC cameras.
> > > > > >
> > > > > > Should the exposure unit be something more user-friendly for SoC cameras?
> > > > >
> > > > > SoC cameras == YUV/RGB sensors ?
> > > > >
> > > > > Are you thinking about using the actual exposure time for YUV/RGB
> > > > > sensors ?
> > > >
> > > > Some devices support both but there are devices that don't natively support
> > > > it, including UVC and Alvium.
> > > >
> > > > I wonder whether we should suggest using the control method that best works
> > > > with device-native units? I.e. if the device natively uses frame length and
> > > > line length, then use blankings + the pixel clock, otherwise
> > > > [gs]_frame_interval?
> > > >
> > >
> > > Are we mixing two things here ? The above documentation block is about
> > > the suggested unit for the exposure control, while [gs]_frame_interval
> > > vs {blankings + pixel_rate} is to control the frame duration ?
> >
> > Oops. The context was apparently garbled in the meantime. X-)
> >
> > I meant using ISO units (i.e. second) in this case.
> >
> > >
> > > > >
> > > > > >
> > > > > > We have two exposure controls now, V4L2_CID_EXPOSURE and
> > > > > > V4L2_CID_EXPOSURE_ABSOLUTE. The former doesn't specity a unit whereas the
> > > > >
> > > > > Apparently only 2 drivers in mainline register V4L2_CID_EXPOSURE_ABSOLUTE
> > > >
> > > > It's not very popular, no. :-) 100 µs is also a long time, I would expect
> > > > to have issues with that large granularity.
> > > >
> > >
> > > is 100 micro-seconds a too large granularity when it comes to exposure
> > > time ??
> >
> > It's a pretty long time in bright lighting conditions. The problem is not a
> > value as such, but the granularity: a change of one has a major relative
> > effect on the exposure time.
> >
> > In practice this value is translated to some number of lines, and the
> > granularity shouldn't be worse than that. I'd therefore make this µs
> > instead.
> >
>
> Yeah, you're right, the two controls' granularity should be similar.
>
> I actually wonder if that's enough. C-PHY has a total bandwidth of 5.8Gbps
> if I'm not mistaken, and assuming 12bpp and a line lenght of 4000
> pixels (arbitrary pick) the line time is 8usec. I wonder if we
> shouldn't go for nanoseconds and be done with that.
That limits the maximum time to around 2 s, but I guess one could use
64-bit controls, too. But how about 10 ns?
>
> > >
> > > > >
> > > > > > latter suggests the unit of 100 µs.
> > > > > >
> > > > > > As exposure is specific to cameras, I think at least a part of this should
> > > > > > make it to the controls documentation. The UVC, for instance, uses
> > > > > > EXPOSURE_ABSOLUTE.
> > > > > >
> > > > > > Could we document V4L2_CID_EXPOSURE is in lines (if possible)?
> > > > >
> > > > > I would indeed be happy with something like "The suggested unit for
> > > > > the control is lines"
> > > >
> > > > Should there be another control for exposure in (µ)s then?
> > > >
> > >
> > > Isn't it V4L2_CID_EXPOSURE_ABSOLUTE ?
> >
> > I guess we could use that but the control name makes no sense, there is
> > also some amount of former use. I'd create a new one instead. At this point
> > it shouldn't really matter for the user space.
> >
> > The existing drivers will continue to also use whatever they're using now
> > (I guess?).
> >
>
> if we introduce a new control V4L2_CID_EXPOSURE_USEC (name to be
>
> V4L2_CID_EXPOSURE in lines
> V4L2_CID_EXPOSURE_ABSOLUTE in 100usec
> V4L2_CID_EXPOSURE_USEC in 1usec/nanosecs
>
> isn't it very confusing ?
>
> Ideally there should have been V4L2_CID_EXPOSURE_LINES and
> V4L2_CID_EXPOSURE_USEC from the start, but how to get there without
> breaking existing users or duplicating controls is not easy...
I wonder what Laurent and Hans think about this.
I'll form my opinion tomorrow. :-)
>
> > >
> > > > >
> > > > > >
> > > > > > > +
> > > > > > > +To convert lines into units of time, the total line length (visible and
> > > > > > > +not visible pixels) has to be divided by the pixel rate::
> > > > > > > +
> > > > > > > + line duration = total line length / pixel rate
> > > > > > > + = (image width + horizontal blanking) / pixel rate
> > > > > > > +
> > > > > > > +Camera sensor driver should try whenever possible to distinguish between the
> > > > > > > +analogue and digital gain control functions. Analogue gain is a multiplication
> > > > > > > +factor applied to all color channels on the pixel array before they get
> > > > > > > +converted into the digital domain. It should be made controllable by
> > > > > >
> > > > > > The analogue gain may not be linear. This depends on the sensor. I'd thus
> > > > > > drop the wording related to multiplication factor.
> > > > > >
> > > > >
> > > > > I might have missed why the gain being linear or not has implications
> > > > > on the fact it acts as a multiplication factor for the color
> > > > > channels...
> > > >
> > > > I must have read this as the analogue gain being the control value. Could
> > > > you still add that the analogue gain factor may have a non-linear relation
> > > > to the control value?
> > > >
> > >
> > > Sure!
> > >
> > > Thanks for digging this one out!
> >
> > You're welcome! :-)
> >
--
Regards,
Sakari Ailus
