Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Helmut,

On Friday, 21 September 2018 10:23:37 EEST Helmut Grohne wrote:
> On Thu, Sep 20, 2018 at 11:00:26PM +0200, Laurent Pinchart wrote:
> > On Thursday, 20 September 2018 23:16:47 EEST Sylwester Nawrocki wrote:
> >> On 09/20/2018 06:49 PM, Grant Grundler wrote:
> >>> On Thu, Sep 20, 2018 at 1:52 AM Tomasz Figa wrote:
> >>>> We have a problem here. The sensor supports only a discrete range of
> >>>> values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
> >>>> fixed point). This makes it possible for the userspace to set values
> >>>> that are not allowed by the sensor specification and also leaves no
> >>>> way to enumerate the supported values.
> 
> I'm not sure I understand this correctly. Tomasz appears to imply here
> that the number of actually supported gain values is 5.
> 
> >> I'm not sure if it's best approach but I once did something similar for
> >> the ov9650 sensor. The gain was fixed point 10-bits value with 4 bits
> >> for fractional part. The driver reports values multiplied by 16. See
> >> ov965x_set_gain() function in drivers/media/i2c/ov9650.c and "Table 4-1.
> >> Total Gain to Control Bit Correlation" in the OV9650 datasheet for
> >> details. The integer menu control just seemed not suitable for 2^10
> >> values.
> 
> As long as values scale linearly (as is the case for fixed point
> numbers) that seems like a reasonable approach to me. That assumption
> appears to not hold for imx208 where the values scale exponentially.

And it's often neither. For instance the MT9P031 has three gain stages. 
Quoting the driver:

                /* Gain is controlled by 2 analog stages and a digital stage.
                 * Valid values for the 3 stages are
                 *
                 * Stage                Min     Max     Step
                 * ------------------------------------------
                 * First analog stage   x1      x2      1
                 * Second analog stage  x1      x4      0.125
                 * Digital stage        x1      x16     0.125

The resulting gain is the product of the three.

> > I've had a similar discussion on IRC recently with Helmut, who posted a
> > nice summary of the problem on the mailing list (see
> > https://www.mail-archive.com/
> > linux-media@vger.kernel.org/msg134502.html). This is a known issue, and
> > while I proposed the same approach, I understand that in some cases
> > userspace may need to know exactly what values are acceptable. In such a
> > case, however, I would expect userspace to have knowledge of the
> > particular sensor model, so the information may not need to come from the
> > kernel.
> 
> A big reason to use V4L2 is to abstract hardware. Having to know what
> sensor model you use runs counter that goal. Indeed, gain (whether
> digital or analogue) can be abstracted in principle. I see little reason
> to not do that.

The purpose of V4L2 is to expose the features of the hardware device to 
userspace, and we try to do so in an as much abstract as possible way. 100% 
abstraction isn't feasible, there will always be small device-specific details 
that will break whatever abstraction we design. We should thus strive for a 
good balance, abstracting the most common types of features, while avoiding 
over-engineering an API that would then become unusable (both because it would 
be complex to use by applications, and implemented in subtly differently buggy 
ways by drivers).

I don't think we'll reach an agreement here if we don't start talking about 
real use cases. Would you have some to share ?

> >> Now the gain control has range 16...1984 out of which only 1024 values
> >> are valid. It might not be best approach for a GUI but at least the
> >> driver exposes mapping of all valid values, which could be enumerated
> >> with VIDIOC_TRY_EXT_CTRLS if required, without a need for a driver-
> >> specific user space code.
> 
> If you have very many values, a reasonable compromise could be reducing
> the precision. If you try to represent a floating point number, values
> with higher exponent will have larger "gaps" when represented as
> integers for v4l2. A compromise could be increasing the step size and
> thus removing a few of the gains with lower exponent.

What if an application needs the precision ? Reducing it can cause issues in 
the 3A algorithms, including closed-loops stability problems. I think we 
should expose the device features with as much precision and coverage as 
possible. Hardcoding use case assumptions in the kernel drives us into a wall 
sooner or later.

> >>>> I can see two solutions here:
> >>>> 
> >>>> 1) Define the control range from 0 to 4 and treat it as an exponent
> >>>> of 2, so that the value for the sensor becomes (1 << val) * 256.
> >>>> (Suggested by Sakari offline.)
> >>>> 
> >>>> This approach has the problem of losing the original unit (and scale)
> >>>> of the value.
> 
> This approach is the one where users will need to know which sensor they
> talk to. The one where the hardware abstraction happens in userspace.
> Can we please not do that?

Let's talk about it based on real use cases.

> >>>> 2) Use an integer menu control, which reports only the supported
> >>>> discrete values - {1, 2, 4, 8, 16}.
> 
> That's what I ended up doing, though I kinda deferred the problem as I
> started using V4L2_CID_ISO_SENSITIVITY, which is an integer menu but not
> exactly the right one. My choice will backfire once I submit the driver
> though that'll still take a little while.
> 
> >>>> With this approach, userspace can enumerate the real gain values, but
> >>>> we would either need to introduce a new control (e.g.
> >>>> V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
> >>>> register V4L2_CID_DIGITAL_GAIN as an integer menu.
> >>>> 
> >>>> Any opinions or better ideas?
> 
> Abusing the specification sounds like it would break userspace. I'd
> avoid doing that.
> 
> We do have a history of adding "duplicate" CIDs for gaining a better
> specification. For instance, there is V4L2_CID_EXPOSURE (unspecified)
> and then there came V4L2_CID_EXPOSURE_ABSOLUTE (unit 100 µs).
> V4L2_CID_GAIN got split into V4L2_CID_ANALOGUE_GAIN and
> V4L2_CID_DIGITAL_GAIN. Further splitting that into integer menu variants
> of analogue and digital gain seems reasonable to me.
> 
> If doing so, I suggest using the following rules (up to discussion):
>  * Drivers must not provide both the integer menu and an integer control
>    for a single gain.
>  * Define which value means "no amplification". For
>    V4L2_CID_DIGITAL_GAIN this was defined as 0x100. Keeping that could
>    be reasonable, but V4L2_CID_ANALOGUE_GAIN presently leavs that
>    undefined.

This could be reported as the control's default value.

>  * If a gain is linear, use the integer control.
>  * If it is non-linear and has fewer than X (1025?) values, use the
>    integer menu.

1024 ioctl calls to query the menu values ? :-( We need a better API than 
that. I'm also concerned that it wouldn't be very usable by userspace. Having 
a list of supported values is one thing, making efficient use of it is 
another. Again, use cases :-)

>  * Otherwise use the integer control. Either increase step or choose the
>    best supported value approximating the user request.
> 
> I think that precise rules help both driver writers and users of these
> controls.
> 
> If an integer menu for V4L2_CID_ANALOGUE_GAIN is being added, I will use
> it.

In this particular case I think splitting analog and digital gain makes sense. 
They are very different beasts at the hardware level, and need to be 
controlled independently.

> >>> My $0.02: leave the user UI alone - let users specify/select anything
> >>> in the range the normal API or UI allows. But have sensor specific
> >>> code map all values in that range to values the sensor supports. Users
> >>> will notice how it works when they play with it.  One can "adjust" the
> >>> mapping so it "feels right".
> 
> Actual users trying to fiddle with these values are not the only
> consumers. Beware that algorithms may want to do so as well and
> algorithms want to know which values are valid in advance.

I expect many algorithms to need a mathematical view of the valid values, not 
just a list. What particular algorithms do you have in mind ?

> Hope this helps. It might be raising more questions beyond the scope of
> imx208 though.

-- 
Regards,

Laurent Pinchart