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