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

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

Hello,

(CC'ing Helmut Grohne)

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:
> >> On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> >>> +/* Digital gain control */
> >>> 
> >>> +#define IMX208_DGTL_GAIN_MIN           0
> >>> +#define IMX208_DGTL_GAIN_MAX           4096
> >>> +#define IMX208_DGTL_GAIN_DEFAULT       0x100
> >>> +#define IMX208_DGTL_GAIN_STEP           1
> >>> 
> >>> +/* Initialize control handlers */
> >>> +static int imx208_init_controls(struct imx208 *imx208)
> >>> +{
> >> 
> >> [snip]
> >> 
> >>> +       v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops,
> >>> V4L2_CID_DIGITAL_GAIN, +                         IMX208_DGTL_GAIN_MIN,
> >>> IMX208_DGTL_GAIN_MAX, +                         IMX208_DGTL_GAIN_STEP,
> >>> +                         IMX208_DGTL_GAIN_DEFAULT);
> >> 
> >> 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.
> 
> The driver could always adjust the value in set_ctrl callback so invalid
> settings are not allowed.
> 
> 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.

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.

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

That would be ~2000 ioctl calls, I don't think that's very practical :-S

> >> 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.
> > 
> > Exactly - will users be confused by this? If we have to explain it,
> > probably not the best choice.
> > 
> >> 2) Use an integer menu control, which reports only the supported
> >> discrete values - {1, 2, 4, 8, 16}.
> >> 
> >> 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?
> > 
> > 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".

-- 
Regards,

Laurent Pinchart