Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Ping-chung, On Thu, Sep 27, 2018 at 03:19:07AM +, Chen, Ping-chung wrote: > Hi, > > >-Original Message- > >From: Yeh, Andy > >Sent: Wednesday, September 26, 2018 11:19 PM > >To: Sakari Ailus ; Chen, Ping-chung > > > > >Hi Sakari, PC, > > >sensors that do need >digital gain applied, too --- assuming it'd be > >combined with the TRY_EXT_CTRLS rounding flags. > >> > >> There might be many kinds of discrete DG formats. For imx208, DG=2^n, > >> but for other sensors, DG could be 2*n, 5*n, or other styles. If HAL > >> needs to > > > >I guess the most common is multiplication and a bit shift (by e.g. 8), e.g. > >multiplying the value by a 16-bit number with a 8-bit fractional part. > >The > >imx208 apparently lacks the multiplication and only has the bit shift. > > > >Usually there's some sort of technical reason for the choice of the > >digital gain implementation and therefore I expect at least the vast > >majority of the implementations to be either of the two. > > >We shall ensure the expansibility of this architecture to include other kind > >of styles in the future. Is this API design architecture-wise ok? > > Indeed. Seems it is hard to cover all rules and HAL needs complex flow to > judge the DG value. Hi Sakari, could you provide an example that how HAL > uses the modified interface to set available digital gain? It'll require more user space code no matter how you'd implement this. Thinking this again, I don't think you'd be doing harm by resorting to an integer menu here. It'll take some more time to get a decent API to provide information on the units etc. to the user space. > >> cover all cases, kernel will have to update more information to this > >> control. Another problem is should HAL take over the SMIA calculation? > >> If so, kernel will also need to update SMIA parameters to user space > >> (or create an addition filed for SMIA in the configuration XML file). > > > >The parameters for the analogue gain model should come from the driver, yes. > >We do not have controls for that purpose but they can (and should) be added. > > > > >How about still follow PC's proposal to implement in XML? It was in IQ > >tuning file before which is in userspace. Even I proposed to PC to study > >with ICG SW team whether this info could be retrieved from 3A algorithm. > > Hi Andy, because we has to use total gain instead of AG in 3A for the WA, our > tuning data of imx208 will not include SMIA of AG anymore. > So HAL has no way to retrieve correct SMIA parameters of AG from 3A. Ideally the driver would be able to provide enough information here to the user space to work with it. This needs improvement going forward, but in a way that is generic enough. -- Kind regards. Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi, On Mon, Oct 01, 2018 at 12:50:02PM +0200, Helmut Grohne wrote: > Hi Laurent, > > On Fri, Sep 28, 2018 at 03:49:38PM +0200, Laurent Pinchart wrote: > > 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 ? > > Fair enough, but at that point, we very much disconnect from the imx208 > in the subject. > > I'm working with a stereo camera. In that setup, you have two image > sensors and infer a three dimensional structure from images captured at > equal time points. For that to happen, it is important that the image > sensors use the same settings. In particular, settings such as expoure > and gain must match. That in turn means that you cannot use the > automatic exposure mode that comes with your image sensor. > > This is one reason for implementing exposure control outside of the > image sensor. Typically you can categorize your parameters into three > classes that affect the brightness of an image: aperture, shutter speed > and some kind of gain. If you know the units of these parameters, you > can estimate the effect of changing them on the resulting image. > > The algorithm for controlling brightness can be quite generic if you > know the units. V4L2_CID_EXPOSURE_ABSOLUTE is given in 100 µs. That > tends to work well. Typically, you prefer increasing exposure over > increasing gain to avoid noise. Similarly, you prefer increasing > analogue gain over increasing digital gain. On the other hand, there is > a limit on exposure to avoid motion blur. If an algorithm knows valid > values for these parameters, it can produce settings independently of > what concrete image sensor you use. > > > > 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. > > So if you change your gain from 0 to 1, your image becomes roughly twice > as bright. In the typical scenario that's too bright, so when increasing > gain, you simultaneously decrease something else (e.g. exposure). But if > you don't know the effect of your gain change, you cannot tell how much > your exposure needs to be reduced. The only way out here is just doing > it and reducing exposure afterwards. Users tend to not like the > flickering resulting from this approach. > > > > * 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 :-) > > You only need to query it once, but I'm not opposed to a better API > either. I just don't think it is that important or urgent. > > > 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 ? > > A very simple algorithm could go like this: > * Assume that exposure time and gain have a linear effect on the >brightness of a captured image. This tends to not hold exactly, but >close enough. > * Compare brightness of the previous frame with a target value. > * Compute the product of current exposure time and gain. Adapt the >product according to the brightness error. > * Distribute this product to exposure time and gain such that exposure >time is maximal below a user-defined limit and that gain is one of >the valid values. > > All you need to know for using this besides V4L2_CID_EXPOSURE_ABSOLUTE, > is the valid gain values. > I agree with Helmut, and have exactly the same problem, even without stereo involved. But : V4L2_CID_EXPOSURE_ABSOLUTE unit is too high for the sensors I use, which provides a exposure time of 15,625 us, that I often need to use. one of my sensor provides an analog gain which can take the values 1, 1.5, 2, 3, 4, 6 and 8 and a digital gain which is a floating point number with 2 bits for the exponent and 6 bits for the mantissa, giving values from 1 to (1 + 63/64) * (1, 2, 4 or 8). I currently combine them and indicate the unit in the name of the control "gain (64th)", but a more robust solution would be welcome. the other sensor provides an analog gain which is expressed in tenth of dB (cB ?) from 0 to 480. Clearly this is difficult to comunicate to the user app using the current definition of V4L2_CID_GAIN. I think I'd need to be able to
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Laurent, On Fri, Sep 28, 2018 at 03:49:38PM +0200, Laurent Pinchart wrote: > 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 ? Fair enough, but at that point, we very much disconnect from the imx208 in the subject. I'm working with a stereo camera. In that setup, you have two image sensors and infer a three dimensional structure from images captured at equal time points. For that to happen, it is important that the image sensors use the same settings. In particular, settings such as expoure and gain must match. That in turn means that you cannot use the automatic exposure mode that comes with your image sensor. This is one reason for implementing exposure control outside of the image sensor. Typically you can categorize your parameters into three classes that affect the brightness of an image: aperture, shutter speed and some kind of gain. If you know the units of these parameters, you can estimate the effect of changing them on the resulting image. The algorithm for controlling brightness can be quite generic if you know the units. V4L2_CID_EXPOSURE_ABSOLUTE is given in 100 µs. That tends to work well. Typically, you prefer increasing exposure over increasing gain to avoid noise. Similarly, you prefer increasing analogue gain over increasing digital gain. On the other hand, there is a limit on exposure to avoid motion blur. If an algorithm knows valid values for these parameters, it can produce settings independently of what concrete image sensor you use. > > 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. So if you change your gain from 0 to 1, your image becomes roughly twice as bright. In the typical scenario that's too bright, so when increasing gain, you simultaneously decrease something else (e.g. exposure). But if you don't know the effect of your gain change, you cannot tell how much your exposure needs to be reduced. The only way out here is just doing it and reducing exposure afterwards. Users tend to not like the flickering resulting from this approach. > > * 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 :-) You only need to query it once, but I'm not opposed to a better API either. I just don't think it is that important or urgent. > 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 ? A very simple algorithm could go like this: * Assume that exposure time and gain have a linear effect on the brightness of a captured image. This tends to not hold exactly, but close enough. * Compare brightness of the previous frame with a target value. * Compute the product of current exposure time and gain. Adapt the product according to the brightness error. * Distribute this product to exposure time and gain such that exposure time is maximal below a user-defined limit and that gain is one of the valid values. All you need to know for using this besides V4L2_CID_EXPOSURE_ABSOLUTE, is the valid gain values. Now I wonder, does this help in reaching a conclusion about whether querying valid gain values is a relevant use case? Helmut
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
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
*
* StageMin Max Step
* --
* First analog stage x1 x2 1
* Second analog stage x1 x4 0.125
* Digital stagex1 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
RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi, >-Original Message- >From: Yeh, Andy >Sent: Wednesday, September 26, 2018 11:19 PM >To: Sakari Ailus ; Chen, Ping-chung > >Hi Sakari, PC, >sensors that do need >digital gain applied, too --- assuming it'd be >combined with the TRY_EXT_CTRLS rounding flags. >> >> There might be many kinds of discrete DG formats. For imx208, DG=2^n, >> but for other sensors, DG could be 2*n, 5*n, or other styles. If HAL >> needs to > >I guess the most common is multiplication and a bit shift (by e.g. 8), e.g. >multiplying the value by a 16-bit number with a 8-bit fractional part. >The >imx208 apparently lacks the multiplication and only has the bit shift. > >Usually there's some sort of technical reason for the choice of the >digital gain implementation and therefore I expect at least the vast >majority of the implementations to be either of the two. >We shall ensure the expansibility of this architecture to include other kind >of styles in the future. Is this API design architecture-wise ok? Indeed. Seems it is hard to cover all rules and HAL needs complex flow to judge the DG value. Hi Sakari, could you provide an example that how HAL uses the modified interface to set available digital gain? > >> cover all cases, kernel will have to update more information to this >> control. Another problem is should HAL take over the SMIA calculation? >> If so, kernel will also need to update SMIA parameters to user space >> (or create an addition filed for SMIA in the configuration XML file). > >The parameters for the analogue gain model should come from the driver, yes. >We do not have controls for that purpose but they can (and should) be added. > >How about still follow PC's proposal to implement in XML? It was in IQ tuning >file before which is in userspace. Even I proposed to PC to study with ICG SW >team whether this info could be retrieved from 3A algorithm. Hi Andy, because we has to use total gain instead of AG in 3A for the WA, our tuning data of imx208 will not include SMIA of AG anymore. So HAL has no way to retrieve correct SMIA parameters of AG from 3A. Thanks, PC Chen >-- >Regards, > >Sakari Ailus >sakari.ai...@linux.intel.com
RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Sakari, PC,
>-Original Message-
>From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
>Sent: Wednesday, September 26, 2018 6:12 PM
>To: Chen, Ping-chung
>Cc: Ricardo Ribalda Delgado ; Laurent Pinchart
>; Sakari Ailus ;
>tf...@chromium.org; sylwester.nawro...@gmail.com; linux-media me...@vger.kernel.org>; Yeh, Andy ; Lai, Jim
>; grund...@chromium.org; Mani, Rajmohan
>
>Subject: Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
>
>Hi Ping-chung,
>
>On Wed, Sep 26, 2018 at 02:27:01AM +, Chen, Ping-chung wrote:
>> Hi Sakari,
>>
>> >-Original Message-
>> >From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
>> >Sent: Wednesday, September 26, 2018 5:55 AM
>>
>> >Hi Ping-chung,
>>
>> >On Tue, Sep 25, 2018 at 10:17:48AM +, Chen, Ping-chung wrote:
>> >...
>> > > > > Controls that have a documented unit use that unit --- as long
>> > > > > as that's the unit used by the hardware. If it's not, it tends
>> > > > > to be that another unit is used but the user space has
>> > > > > currently no way of knowing this. And the digital gain control is no
>exception to this.
>> > > > >
>> > > > > So if we want to improve the user space's ability to be
>> > > > > informed how the control values reflect to device
>> > > > > configuration, we do need to provide more information to the
>> > > > > user space. One way to do that would be through
>> > > > > VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has plenty of reserved
>fields --- just for purposes such as this one.
>> > > >
>> > > > I don't think we can come up with a good way to expose arbitrary
>> > > > mathematical formulas through an ioctl. In my opinion the
>> > > > question is how far we want to go, how precise we need to be.
>> > > >
>> > > > > > Any opinions or better ideas?
>> > >
>> > > >My 0.02 DKK. On a similar situation, where userspace was running a
>close loop calibration:
>> > >
>> > > >We have implemented two extra control: eposure_next exposure_pre
>that tell us which one is the next value. Perhaps we could embebed such
>functionality in QUERY_EXT_CTRL.
>> > >
>> > > >Cheers
>> > >
>> > > How about creating an additional control to handle the case of
>> > > V4L2_CID_GAIN in the imx208 driver? HAL can set AG and DG
>> > > separately for the general sensor usage by V4L2_CID_ANALOGUE_GAIN
>> > > and V4L2_CID_DIGITAL_GAIN but call V4L2_CID_GAIN for the condition
>> > > of setting total_gain=AGxDG.
>> >
>> > >How do you update the two other controls if the user updates the
>V4L2_CID_GAIN control?
>> >
>> > In imx208 driver:
>> >
>> > Add two pointers *digital_gain and *analog_gain of v4l2_ctrl in the global
>structure imx208.
>> > static int imx208_init_controls(struct imx208 *imx208) {
>> > Imx208->digital_gain = v4l2_ctrl_new_std(..., V4L2_CID_DIGITAL_GAIN,
>> > Imx208->...); analog_gain =
>> > Imx208->v4l2_ctrl_new_std(...,V4L2_CID_ANALOGUE_GAIN, ...);
>> >
>> > static int imx208_set_ctrl(struct v4l2_ctrl *ctrl) { ...
>> >case V4L2_CID_ANALOGUE_GAIN:
>> >ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2,
>ctrl->val);
>> >break;
>> >case V4L2_CID_DIGITAL_GAIN:
>> >ret = imx208_update_digital_gain(imx208, 2, ctrl->val);
>> >break;
>> >case V4L2_CID_ GAIN:
>> >ret = imx208_update_gain(imx208, 2, ctrl->val); // total gain
>> >break;
>> > }
>> >
>> > Then the implementation of imx208_update_gain():
>> > static int imx208_update_gain(struct imx208 *imx208, u32 len, u32
>> > val) {
>> >digital_gain = (val - 1) / ag_max;
>> >digital_gain = map_to_real_DG(digital_gain);// map to 1x,
>2x, 4x, 8x, 16x
>> >digital_gain_code = digital_gain << 8 // DGx256 for
>DG_code
>> >ret = imx208_update_digital_gain(imx208, 2, digital_gain_code);
>> >imx208->digital_gain->val = digital_gain_code;
>> >analog_gain = val/digital_gain;
>> >analog_gain_code = SMIA_AG_to_Code(analog_gain); // AG =
>256/(256-ag_code)
>> >ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2,
>analo
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Ping-chung,
On Wed, Sep 26, 2018 at 02:27:01AM +, Chen, Ping-chung wrote:
> Hi Sakari,
>
> >-Original Message-
> >From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> >Sent: Wednesday, September 26, 2018 5:55 AM
>
> >Hi Ping-chung,
>
> >On Tue, Sep 25, 2018 at 10:17:48AM +, Chen, Ping-chung wrote:
> >...
> > > > > Controls that have a documented unit use that unit --- as long
> > > > > as that's the unit used by the hardware. If it's not, it tends
> > > > > to be that another unit is used but the user space has currently
> > > > > no way of knowing this. And the digital gain control is no exception
> > > > > to this.
> > > > >
> > > > > So if we want to improve the user space's ability to be informed
> > > > > how the control values reflect to device configuration, we do
> > > > > need to provide more information to the user space. One way to
> > > > > do that would be through VIDIOC_QUERY_EXT_CTRL. The IOCTL struct
> > > > > has plenty of reserved fields --- just for purposes such as this one.
> > > >
> > > > I don't think we can come up with a good way to expose arbitrary
> > > > mathematical formulas through an ioctl. In my opinion the question
> > > > is how far we want to go, how precise we need to be.
> > > >
> > > > > > Any opinions or better ideas?
> > >
> > > >My 0.02 DKK. On a similar situation, where userspace was running a
> > > >close loop calibration:
> > >
> > > >We have implemented two extra control: eposure_next exposure_pre that
> > > >tell us which one is the next value. Perhaps we could embebed such
> > > >functionality in QUERY_EXT_CTRL.
> > >
> > > >Cheers
> > >
> > > How about creating an additional control to handle the case of
> > > V4L2_CID_GAIN in the imx208 driver? HAL can set AG and DG separately
> > > for the general sensor usage by V4L2_CID_ANALOGUE_GAIN and
> > > V4L2_CID_DIGITAL_GAIN but call V4L2_CID_GAIN for the condition of
> > > setting total_gain=AGxDG.
> >
> > >How do you update the two other controls if the user updates the
> > >V4L2_CID_GAIN control?
> >
> > In imx208 driver:
> >
> > Add two pointers *digital_gain and *analog_gain of v4l2_ctrl in the global
> > structure imx208.
> > static int imx208_init_controls(struct imx208 *imx208) {
> > Imx208->digital_gain = v4l2_ctrl_new_std(..., V4L2_CID_DIGITAL_GAIN,
> > Imx208->...); analog_gain =
> > Imx208->v4l2_ctrl_new_std(...,V4L2_CID_ANALOGUE_GAIN, ...);
> >
> > static int imx208_set_ctrl(struct v4l2_ctrl *ctrl) { ...
> > case V4L2_CID_ANALOGUE_GAIN:
> > ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2,
> > ctrl->val);
> > break;
> > case V4L2_CID_DIGITAL_GAIN:
> > ret = imx208_update_digital_gain(imx208, 2, ctrl->val);
> > break;
> > case V4L2_CID_ GAIN:
> > ret = imx208_update_gain(imx208, 2, ctrl->val); // total gain
> > break;
> > }
> >
> > Then the implementation of imx208_update_gain():
> > static int imx208_update_gain(struct imx208 *imx208, u32 len, u32 val)
> > {
> > digital_gain = (val - 1) / ag_max;
> > digital_gain = map_to_real_DG(digital_gain);// map to 1x,
> > 2x, 4x, 8x, 16x
> > digital_gain_code = digital_gain << 8 // DGx256 for
> > DG_code
> > ret = imx208_update_digital_gain(imx208, 2, digital_gain_code);
> > imx208->digital_gain->val = digital_gain_code;
> > analog_gain = val/digital_gain;
> > analog_gain_code = SMIA_AG_to_Code(analog_gain); // AG =
> > 256/(256-ag_code)
> > ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2,
> > analog_gain_code);
> > imx208->digital_gain->val = analog_gain_code;
>
> >How about putting this piece of code to the user space instead?
>
> >Some work would be needed to generalise it in order for it to work on other
> >sensors that do need >digital gain applied, too --- assuming it'd be
> >combined with the TRY_EXT_CTRLS rounding flags.
>
> There might be many kinds of discrete DG formats. For imx208, DG=2^n, but
> for other sensors, DG could be 2*n, 5*n, or other styles. If HAL needs to
I guess the most common is multiplication and a bit shift (by e.g. 8), e.g.
multiplying the value by a 16-bit number with a 8-bit fractional part. The
imx208 apparently lacks the multiplication and only has the bit shift.
Usually there's some sort of technical reason for the choice of the digital
gain implementation and therefore I expect at least the vast majority of
the implementations to be either of the two.
> cover all cases, kernel will have to update more information to this
> control. Another problem is should HAL take over the SMIA calculation? If
> so, kernel will also need to update SMIA parameters to user space (or
> create an addition filed for SMIA in the configuration XML file).
The parameters for the analogue gain model should come from the driver,
yes. We do not have controls for that purpose but they can (and should) be
added.
RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Sakari,
>-Original Message-
>From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
>Sent: Wednesday, September 26, 2018 5:55 AM
>Hi Ping-chung,
>On Tue, Sep 25, 2018 at 10:17:48AM +, Chen, Ping-chung wrote:
>...
> > > > Controls that have a documented unit use that unit --- as long
> > > > as that's the unit used by the hardware. If it's not, it tends
> > > > to be that another unit is used but the user space has currently
> > > > no way of knowing this. And the digital gain control is no exception to
> > > > this.
> > > >
> > > > So if we want to improve the user space's ability to be informed
> > > > how the control values reflect to device configuration, we do
> > > > need to provide more information to the user space. One way to
> > > > do that would be through VIDIOC_QUERY_EXT_CTRL. The IOCTL struct
> > > > has plenty of reserved fields --- just for purposes such as this one.
> > >
> > > I don't think we can come up with a good way to expose arbitrary
> > > mathematical formulas through an ioctl. In my opinion the question
> > > is how far we want to go, how precise we need to be.
> > >
> > > > > Any opinions or better ideas?
> >
> > >My 0.02 DKK. On a similar situation, where userspace was running a close
> > >loop calibration:
> >
> > >We have implemented two extra control: eposure_next exposure_pre that tell
> > >us which one is the next value. Perhaps we could embebed such
> > >functionality in QUERY_EXT_CTRL.
> >
> > >Cheers
> >
> > How about creating an additional control to handle the case of
> > V4L2_CID_GAIN in the imx208 driver? HAL can set AG and DG separately
> > for the general sensor usage by V4L2_CID_ANALOGUE_GAIN and
> > V4L2_CID_DIGITAL_GAIN but call V4L2_CID_GAIN for the condition of
> > setting total_gain=AGxDG.
>
> >How do you update the two other controls if the user updates the
> >V4L2_CID_GAIN control?
>
> In imx208 driver:
>
> Add two pointers *digital_gain and *analog_gain of v4l2_ctrl in the global
> structure imx208.
> static int imx208_init_controls(struct imx208 *imx208) {
> Imx208->digital_gain = v4l2_ctrl_new_std(..., V4L2_CID_DIGITAL_GAIN,
> Imx208->...); analog_gain =
> Imx208->v4l2_ctrl_new_std(...,V4L2_CID_ANALOGUE_GAIN, ...);
>
> static int imx208_set_ctrl(struct v4l2_ctrl *ctrl) { ...
> case V4L2_CID_ANALOGUE_GAIN:
> ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2,
> ctrl->val);
> break;
> case V4L2_CID_DIGITAL_GAIN:
> ret = imx208_update_digital_gain(imx208, 2, ctrl->val);
> break;
> case V4L2_CID_ GAIN:
> ret = imx208_update_gain(imx208, 2, ctrl->val); // total gain
> break;
> }
>
> Then the implementation of imx208_update_gain():
> static int imx208_update_gain(struct imx208 *imx208, u32 len, u32 val)
> {
> digital_gain = (val - 1) / ag_max;
> digital_gain = map_to_real_DG(digital_gain);// map to 1x,
> 2x, 4x, 8x, 16x
> digital_gain_code = digital_gain << 8 // DGx256 for
> DG_code
> ret = imx208_update_digital_gain(imx208, 2, digital_gain_code);
> imx208->digital_gain->val = digital_gain_code;
> analog_gain = val/digital_gain;
> analog_gain_code = SMIA_AG_to_Code(analog_gain); // AG =
> 256/(256-ag_code)
> ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2,
> analog_gain_code);
> imx208->digital_gain->val = analog_gain_code;
>How about putting this piece of code to the user space instead?
>Some work would be needed to generalise it in order for it to work on other
>sensors that do need >digital gain applied, too --- assuming it'd be combined
>with the TRY_EXT_CTRLS rounding flags.
There might be many kinds of discrete DG formats. For imx208, DG=2^n, but for
other sensors, DG could be 2*n, 5*n, or other styles. If HAL needs to cover all
cases, kernel will have to update more information to this control.
Another problem is should HAL take over the SMIA calculation? If so, kernel
will also need to update SMIA parameters to user space (or create an addition
filed for SMIA in the configuration XML file).
Thanks,
PC Chen
>--
>Kind regards,
>Sakari Ailus
>sakari.ai...@linux.intel.com
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Ping-chung,
On Tue, Sep 25, 2018 at 10:17:48AM +, Chen, Ping-chung wrote:
...
> > > > Controls that have a documented unit use that unit --- as long as
> > > > that's the unit used by the hardware. If it's not, it tends to be
> > > > that another unit is used but the user space has currently no way
> > > > of knowing this. And the digital gain control is no exception to this.
> > > >
> > > > So if we want to improve the user space's ability to be informed
> > > > how the control values reflect to device configuration, we do need
> > > > to provide more information to the user space. One way to do that
> > > > would be through VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has
> > > > plenty of reserved fields --- just for purposes such as this one.
> > >
> > > I don't think we can come up with a good way to expose arbitrary
> > > mathematical formulas through an ioctl. In my opinion the question
> > > is how far we want to go, how precise we need to be.
> > >
> > > > > Any opinions or better ideas?
> >
> > >My 0.02 DKK. On a similar situation, where userspace was running a close
> > >loop calibration:
> >
> > >We have implemented two extra control: eposure_next exposure_pre that tell
> > >us which one is the next value. Perhaps we could embebed such
> > >functionality in QUERY_EXT_CTRL.
> >
> > >Cheers
> >
> > How about creating an additional control to handle the case of
> > V4L2_CID_GAIN in the imx208 driver? HAL can set AG and DG separately
> > for the general sensor usage by V4L2_CID_ANALOGUE_GAIN and
> > V4L2_CID_DIGITAL_GAIN but call V4L2_CID_GAIN for the condition of
> > setting total_gain=AGxDG.
>
> >How do you update the two other controls if the user updates the
> >V4L2_CID_GAIN control?
>
> In imx208 driver:
>
> Add two pointers *digital_gain and *analog_gain of v4l2_ctrl in the global
> structure imx208.
> static int imx208_init_controls(struct imx208 *imx208) {
> Imx208->digital_gain = v4l2_ctrl_new_std(..., V4L2_CID_DIGITAL_GAIN, ...);
> Imx208->analog_gain = v4l2_ctrl_new_std(...,V4L2_CID_ANALOGUE_GAIN, ...);
>
> static int imx208_set_ctrl(struct v4l2_ctrl *ctrl)
> {
> ...
> case V4L2_CID_ANALOGUE_GAIN:
> ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2,
> ctrl->val);
> break;
> case V4L2_CID_DIGITAL_GAIN:
> ret = imx208_update_digital_gain(imx208, 2, ctrl->val);
> break;
> case V4L2_CID_ GAIN:
> ret = imx208_update_gain(imx208, 2, ctrl->val); // total gain
> break;
> }
>
> Then the implementation of imx208_update_gain():
> static int imx208_update_gain(struct imx208 *imx208, u32 len, u32 val)
> {
> digital_gain = (val - 1) / ag_max;
> digital_gain = map_to_real_DG(digital_gain);// map to 1x,
> 2x, 4x, 8x, 16x
> digital_gain_code = digital_gain << 8 // DGx256 for
> DG_code
> ret = imx208_update_digital_gain(imx208, 2, digital_gain_code);
> imx208->digital_gain->val = digital_gain_code;
> analog_gain = val/digital_gain;
> analog_gain_code = SMIA_AG_to_Code(analog_gain); // AG =
> 256/(256-ag_code)
> ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2,
> analog_gain_code);
> imx208->digital_gain->val = analog_gain_code;
How about putting this piece of code to the user space instead?
Some work would be needed to generalise it in order for it to work on other
sensors that do need digital gain applied, too --- assuming it'd be
combined with the TRY_EXT_CTRLS rounding flags.
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Sakari,
>-Original Message-
>From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
>Sent: Tuesday, September 25, 2018 5:25 PM
>To: Chen, Ping-chung
>Cc: Ricardo Ribalda Delgado ; Laurent Pinchart
>Subject: Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
>Hi Ping-chung,
>On Fri, Sep 21, 2018 at 07:08:10AM +, Chen, Ping-chung wrote:
> Typo.
>
> -Original Message-
> From: Chen, Ping-chung
> Sent: Friday, September 21, 2018 3:07 PM
> To: 'Ricardo Ribalda Delgado' ; Laurent
> Pinchart
> Cc: Sakari Ailus ; tf...@chromium.org; Sakari
> Ailus ; sylwester.nawro...@gmail.com;
> linux-media ; Yeh, Andy
> ; Lai, Jim ;
> grund...@chromium.org; Mani, Rajmohan
> Subject: RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver
>
> Hi Sakari,
>
> >-Original Message-
> >From: Ricardo Ribalda Delgado [mailto:ricardo.riba...@gmail.com]
> >Sent: Friday, September 21, 2018 5:55 AM
>
> >HI
> On Thu, Sep 20, 2018 at 11:13 PM Laurent Pinchart
> wrote:
> >
> > Hi Sakari,
> >
> > On Thursday, 20 September 2018 23:56:59 EEST Sakari Ailus wrote:
> > > On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> > > > On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> > > > [snip]
> > > >
> > > > > +
> > > > > +/* Digital gain control */
> > > > > +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e
> > > > > +#define IMX208_REG_R_DIGITAL_GAIN 0x0210
> > > > > +#define IMX208_REG_B_DIGITAL_GAIN 0x0212
> > > > > +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214
> > > > > +#define IMX208_DGTL_GAIN_MIN 0
> > > > > +#define IMX208_DGTL_GAIN_MAX 4096
> > > > > +#define IMX208_DGTL_GAIN_DEFAULT 0x100
> > > > > +#define IMX208_DGTL_GAIN_STEP 1
> > > > > +
> > > >
> > > > [snip]
> > > >
> > > > > +/* Initialize control handlers */ static int
> > > > > +imx208_init_controls(struct imx208 *imx208) {
> > > >
> > > > [snip]
> > > >
> > > > > + v4l2_ctrl_new_std(ctrl_hdlr, _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.
> > > >
> > > > 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.
> > >
> > > I'd like to add that this is not a property of the proposed solution.
> > >
> > > Rather, the above needs to be accompanied by additional
> > > information provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit,
> > > prefix as well as other information such as whether the control is
> > > linear or exponential (as in this case).
> > >
> > > > 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.
> > >
> > > New controls in V4L2 are, for the most part, created when there's
> > > something new to control. The documentation of some controls
> > > (similar to e.g. gain) documents a unit as well as a prefix but
> > > that's the case only because there's been no way to tell the unit or
> > > prefix otherwise in the API.
> > >
> > > An exception to this are EX
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Ping-chung,
On Fri, Sep 21, 2018 at 07:08:10AM +, Chen, Ping-chung wrote:
> Typo.
>
> -Original Message-
> From: Chen, Ping-chung
> Sent: Friday, September 21, 2018 3:07 PM
> To: 'Ricardo Ribalda Delgado' ; Laurent Pinchart
>
> Cc: Sakari Ailus ; tf...@chromium.org; Sakari Ailus
> ; sylwester.nawro...@gmail.com; linux-media
> ; Yeh, Andy ; Lai, Jim
> ; grund...@chromium.org; Mani, Rajmohan
>
> Subject: RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver
>
> Hi Sakari,
>
> >-Original Message-
> >From: Ricardo Ribalda Delgado [mailto:ricardo.riba...@gmail.com]
> >Sent: Friday, September 21, 2018 5:55 AM
>
> >HI
> On Thu, Sep 20, 2018 at 11:13 PM Laurent Pinchart
> wrote:
> >
> > Hi Sakari,
> >
> > On Thursday, 20 September 2018 23:56:59 EEST Sakari Ailus wrote:
> > > On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> > > > On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> > > > [snip]
> > > >
> > > > > +
> > > > > +/* Digital gain control */
> > > > > +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e
> > > > > +#define IMX208_REG_R_DIGITAL_GAIN 0x0210
> > > > > +#define IMX208_REG_B_DIGITAL_GAIN 0x0212
> > > > > +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214
> > > > > +#define IMX208_DGTL_GAIN_MIN 0
> > > > > +#define IMX208_DGTL_GAIN_MAX 4096
> > > > > +#define IMX208_DGTL_GAIN_DEFAULT 0x100
> > > > > +#define IMX208_DGTL_GAIN_STEP 1
> > > > > +
> > > >
> > > > [snip]
> > > >
> > > > > +/* Initialize control handlers */ static int
> > > > > +imx208_init_controls(struct imx208 *imx208) {
> > > >
> > > > [snip]
> > > >
> > > > > + v4l2_ctrl_new_std(ctrl_hdlr, _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.
> > > >
> > > > 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.
> > >
> > > I'd like to add that this is not a property of the proposed solution.
> > >
> > > Rather, the above needs to be accompanied by additional information
> > > provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, prefix as
> > > well as other information such as whether the control is linear or
> > > exponential (as in this case).
> > >
> > > > 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.
> > >
> > > New controls in V4L2 are, for the most part, created when there's
> > > something new to control. The documentation of some controls
> > > (similar to e.g. gain) documents a unit as well as a prefix but
> > > that's the case only because there's been no way to tell the unit or
> > > prefix otherwise in the API.
> > >
> > > An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not
> > > entirely sure how they came to be though. An accident is a
> > > possibility as far as I see.
> >
> > If I remember correctly I introduced the absolute variant for the UVC
> > driver (even though git blame points to Brandon Phil
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
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.
> 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.
> > 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.
> > >> 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?
> > >> 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.
* 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.
* Otherwise use the integer
RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Typo.
-Original Message-
From: Chen, Ping-chung
Sent: Friday, September 21, 2018 3:07 PM
To: 'Ricardo Ribalda Delgado' ; Laurent Pinchart
Cc: Sakari Ailus ; tf...@chromium.org; Sakari Ailus
; sylwester.nawro...@gmail.com; linux-media
; Yeh, Andy ; Lai, Jim
; grund...@chromium.org; Mani, Rajmohan
Subject: RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Sakari,
>-Original Message-
>From: Ricardo Ribalda Delgado [mailto:ricardo.riba...@gmail.com]
>Sent: Friday, September 21, 2018 5:55 AM
>HI
On Thu, Sep 20, 2018 at 11:13 PM Laurent Pinchart
wrote:
>
> Hi Sakari,
>
> On Thursday, 20 September 2018 23:56:59 EEST Sakari Ailus wrote:
> > On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> > > On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> > > [snip]
> > >
> > > > +
> > > > +/* Digital gain control */
> > > > +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e
> > > > +#define IMX208_REG_R_DIGITAL_GAIN 0x0210
> > > > +#define IMX208_REG_B_DIGITAL_GAIN 0x0212
> > > > +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214
> > > > +#define IMX208_DGTL_GAIN_MIN 0
> > > > +#define IMX208_DGTL_GAIN_MAX 4096
> > > > +#define IMX208_DGTL_GAIN_DEFAULT 0x100
> > > > +#define IMX208_DGTL_GAIN_STEP 1
> > > > +
> > >
> > > [snip]
> > >
> > > > +/* Initialize control handlers */ static int
> > > > +imx208_init_controls(struct imx208 *imx208) {
> > >
> > > [snip]
> > >
> > > > + v4l2_ctrl_new_std(ctrl_hdlr, _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.
> > >
> > > 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.
> >
> > I'd like to add that this is not a property of the proposed solution.
> >
> > Rather, the above needs to be accompanied by additional information
> > provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, prefix as
> > well as other information such as whether the control is linear or
> > exponential (as in this case).
> >
> > > 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.
> >
> > New controls in V4L2 are, for the most part, created when there's
> > something new to control. The documentation of some controls
> > (similar to e.g. gain) documents a unit as well as a prefix but
> > that's the case only because there's been no way to tell the unit or prefix
> > otherwise in the API.
> >
> > An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not
> > entirely sure how they came to be though. An accident is a
> > possibility as far as I see.
>
> If I remember correctly I introduced the absolute variant for the UVC
> driver (even though git blame points to Brandon Philips). I don't
> really remember why though.
>
> > Controls that have a documented unit use that unit --- as long as
> > that's the unit used by the hardware. If it's not, it tends to be
> > that another unit is used but the user space has currently no way of
> > knowing this. And the digital gain control is no exception to this.
> >
> > So if we want to improve the user space's ability to be informed how
> > the control values reflect to device configuration, we do need to
> > provide more information to the user space.
RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Sakari,
>-Original Message-
>From: Ricardo Ribalda Delgado [mailto:ricardo.riba...@gmail.com]
>Sent: Friday, September 21, 2018 5:55 AM
>HI
On Thu, Sep 20, 2018 at 11:13 PM Laurent Pinchart
wrote:
>
> Hi Sakari,
>
> On Thursday, 20 September 2018 23:56:59 EEST Sakari Ailus wrote:
> > On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> > > On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> > > [snip]
> > >
> > > > +
> > > > +/* Digital gain control */
> > > > +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e
> > > > +#define IMX208_REG_R_DIGITAL_GAIN 0x0210
> > > > +#define IMX208_REG_B_DIGITAL_GAIN 0x0212
> > > > +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214
> > > > +#define IMX208_DGTL_GAIN_MIN 0
> > > > +#define IMX208_DGTL_GAIN_MAX 4096
> > > > +#define IMX208_DGTL_GAIN_DEFAULT 0x100
> > > > +#define IMX208_DGTL_GAIN_STEP 1
> > > > +
> > >
> > > [snip]
> > >
> > > > +/* Initialize control handlers */ static int
> > > > +imx208_init_controls(struct imx208 *imx208) {
> > >
> > > [snip]
> > >
> > > > + v4l2_ctrl_new_std(ctrl_hdlr, _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.
> > >
> > > 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.
> >
> > I'd like to add that this is not a property of the proposed solution.
> >
> > Rather, the above needs to be accompanied by additional information
> > provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, prefix as
> > well as other information such as whether the control is linear or
> > exponential (as in this case).
> >
> > > 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.
> >
> > New controls in V4L2 are, for the most part, created when there's
> > something new to control. The documentation of some controls
> > (similar to e.g. gain) documents a unit as well as a prefix but
> > that's the case only because there's been no way to tell the unit or prefix
> > otherwise in the API.
> >
> > An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not
> > entirely sure how they came to be though. An accident is a
> > possibility as far as I see.
>
> If I remember correctly I introduced the absolute variant for the UVC
> driver (even though git blame points to Brandon Philips). I don't
> really remember why though.
>
> > Controls that have a documented unit use that unit --- as long as
> > that's the unit used by the hardware. If it's not, it tends to be
> > that another unit is used but the user space has currently no way of
> > knowing this. And the digital gain control is no exception to this.
> >
> > So if we want to improve the user space's ability to be informed how
> > the control values reflect to device configuration, we do need to
> > provide more information to the user space. One way to do that would
> > be through VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has plenty of
> > reserved fields --- just for purposes such as this one.
>
> I don't think we can come up with a good way to expose arbitrary
> mathematical formulas through an ioctl. In my opinion the question is
> how far we want to go, how precise we need to be.
>
> > > Any opinions or better ideas?
>My 0.02 DKK. On a similar situation, where userspace was running a close loop
>calibration:
>We have implemented two extra control: eposure_next exposure_pre that tell us
>which one is the next value. Perhaps we could embebed such functionality in
>QUERY_EXT_CTRL.
>Cheers
How about creating an additional control to handle the case of V4L2_CID_GAIN in
the imx208 driver? HAL can set AG and DG separately for the general sensor
usage by V4L2_CID_ANALOGUE_GAIN and V4L2_CID_DIGITAL_GAIN but call
V4L2_CID_GAIN for the condition of setting total_gain=AGxDG.
In the case of V4L2_CID_ANALOGUE_GAIN and V4L2_CID_EXPOSURE of
imx208_set_ctrl(), it is no need to do any change for WA.
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
HI
On Thu, Sep 20, 2018 at 11:13 PM Laurent Pinchart
wrote:
>
> Hi Sakari,
>
> On Thursday, 20 September 2018 23:56:59 EEST Sakari Ailus wrote:
> > On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> > > On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> > > [snip]
> > >
> > > > +
> > > > +/* Digital gain control */
> > > > +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e
> > > > +#define IMX208_REG_R_DIGITAL_GAIN 0x0210
> > > > +#define IMX208_REG_B_DIGITAL_GAIN 0x0212
> > > > +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214
> > > > +#define IMX208_DGTL_GAIN_MIN 0
> > > > +#define IMX208_DGTL_GAIN_MAX 4096
> > > > +#define IMX208_DGTL_GAIN_DEFAULT 0x100
> > > > +#define IMX208_DGTL_GAIN_STEP 1
> > > > +
> > >
> > > [snip]
> > >
> > > > +/* Initialize control handlers */
> > > > +static int imx208_init_controls(struct imx208 *imx208)
> > > > +{
> > >
> > > [snip]
> > >
> > > > + v4l2_ctrl_new_std(ctrl_hdlr, _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.
> > >
> > > 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.
> >
> > I'd like to add that this is not a property of the proposed solution.
> >
> > Rather, the above needs to be accompanied by additional information
> > provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, prefix as well as
> > other information such as whether the control is linear or exponential (as
> > in this case).
> >
> > > 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.
> >
> > New controls in V4L2 are, for the most part, created when there's something
> > new to control. The documentation of some controls (similar to e.g. gain)
> > documents a unit as well as a prefix but that's the case only because
> > there's been no way to tell the unit or prefix otherwise in the API.
> >
> > An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not entirely
> > sure how they came to be though. An accident is a possibility as far as I
> > see.
>
> If I remember correctly I introduced the absolute variant for the UVC driver
> (even though git blame points to Brandon Philips). I don't really remember why
> though.
>
> > Controls that have a documented unit use that unit --- as long as that's
> > the unit used by the hardware. If it's not, it tends to be that another
> > unit is used but the user space has currently no way of knowing this. And
> > the digital gain control is no exception to this.
> >
> > So if we want to improve the user space's ability to be informed how the
> > control values reflect to device configuration, we do need to provide more
> > information to the user space. One way to do that would be through
> > VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has plenty of reserved fields ---
> > just for purposes such as this one.
>
> I don't think we can come up with a good way to expose arbitrary mathematical
> formulas through an ioctl. In my opinion the question is how far we want to
> go, how precise we need to be.
>
> > > Any opinions or better ideas?
My 0.02 DKK. On a similar situation, where userspace was running a
close loop calibration:
We have implemented two extra control: eposure_next exposure_pre that
tell us which one is the next value. Perhaps we could embebed such
functionality in QUERY_EXT_CTRL.
Cheers
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>
--
Ricardo Ribalda
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Sakari,
On Thursday, 20 September 2018 23:56:59 EEST Sakari Ailus wrote:
> On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> > On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> > [snip]
> >
> > > +
> > > +/* Digital gain control */
> > > +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e
> > > +#define IMX208_REG_R_DIGITAL_GAIN 0x0210
> > > +#define IMX208_REG_B_DIGITAL_GAIN 0x0212
> > > +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214
> > > +#define IMX208_DGTL_GAIN_MIN 0
> > > +#define IMX208_DGTL_GAIN_MAX 4096
> > > +#define IMX208_DGTL_GAIN_DEFAULT 0x100
> > > +#define IMX208_DGTL_GAIN_STEP 1
> > > +
> >
> > [snip]
> >
> > > +/* Initialize control handlers */
> > > +static int imx208_init_controls(struct imx208 *imx208)
> > > +{
> >
> > [snip]
> >
> > > + v4l2_ctrl_new_std(ctrl_hdlr, _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.
> >
> > 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.
>
> I'd like to add that this is not a property of the proposed solution.
>
> Rather, the above needs to be accompanied by additional information
> provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, prefix as well as
> other information such as whether the control is linear or exponential (as
> in this case).
>
> > 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.
>
> New controls in V4L2 are, for the most part, created when there's something
> new to control. The documentation of some controls (similar to e.g. gain)
> documents a unit as well as a prefix but that's the case only because
> there's been no way to tell the unit or prefix otherwise in the API.
>
> An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not entirely
> sure how they came to be though. An accident is a possibility as far as I
> see.
If I remember correctly I introduced the absolute variant for the UVC driver
(even though git blame points to Brandon Philips). I don't really remember why
though.
> Controls that have a documented unit use that unit --- as long as that's
> the unit used by the hardware. If it's not, it tends to be that another
> unit is used but the user space has currently no way of knowing this. And
> the digital gain control is no exception to this.
>
> So if we want to improve the user space's ability to be informed how the
> control values reflect to device configuration, we do need to provide more
> information to the user space. One way to do that would be through
> VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has plenty of reserved fields ---
> just for purposes such as this one.
I don't think we can come up with a good way to expose arbitrary mathematical
formulas through an ioctl. In my opinion the question is how far we want to
go, how precise we need to be.
> > Any opinions or better ideas?
--
Regards,
Laurent Pinchart
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
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, _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
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Tomasz,
On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> [+Laurent and Sylwester]
>
> On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen
> wrote:
> [snip]
> > +
> > +/* Digital gain control */
> > +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e
> > +#define IMX208_REG_R_DIGITAL_GAIN 0x0210
> > +#define IMX208_REG_B_DIGITAL_GAIN 0x0212
> > +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214
> > +#define IMX208_DGTL_GAIN_MIN 0
> > +#define IMX208_DGTL_GAIN_MAX 4096
> > +#define IMX208_DGTL_GAIN_DEFAULT 0x100
> > +#define IMX208_DGTL_GAIN_STEP 1
> > +
> [snip]
> > +/* Initialize control handlers */
> > +static int imx208_init_controls(struct imx208 *imx208)
> > +{
> [snip]
> > + v4l2_ctrl_new_std(ctrl_hdlr, _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.
>
> 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.
I'd like to add that this is not a property of the proposed solution.
Rather, the above needs to be accompanied by additional information
provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, prefix as well as
other information such as whether the control is linear or exponential (as
in this case).
>
> 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.
New controls in V4L2 are, for the most part, created when there's something
new to control. The documentation of some controls (similar to e.g. gain)
documents a unit as well as a prefix but that's the case only because
there's been no way to tell the unit or prefix otherwise in the API.
An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not entirely
sure how they came to be though. An accident is a possibility as far as I
see.
Controls that have a documented unit use that unit --- as long as that's
the unit used by the hardware. If it's not, it tends to be that another
unit is used but the user space has currently no way of knowing this. And
the digital gain control is no exception to this.
So if we want to improve the user space's ability to be informed how the
control values reflect to device configuration, we do need to provide more
information to the user space. One way to do that would be through
VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has plenty of reserved fields ---
just for purposes such as this one.
>
> Any opinions or better ideas?
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
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, _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.
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.
>> 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,
Sylwester
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
[resend in plain text - sorry!]
On Thu, Sep 20, 2018 at 1:52 AM Tomasz Figa wrote:
>
> [+Laurent and Sylwester]
>
> On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen
> wrote:
> [snip]
> > +
> > +/* Digital gain control */
> > +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e
> > +#define IMX208_REG_R_DIGITAL_GAIN 0x0210
> > +#define IMX208_REG_B_DIGITAL_GAIN 0x0212
> > +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214
> > +#define IMX208_DGTL_GAIN_MIN 0
> > +#define IMX208_DGTL_GAIN_MAX 4096
> > +#define IMX208_DGTL_GAIN_DEFAULT 0x100
> > +#define IMX208_DGTL_GAIN_STEP 1
> > +
> [snip]
> > +/* Initialize control handlers */
> > +static int imx208_init_controls(struct imx208 *imx208)
> > +{
> [snip]
> > + v4l2_ctrl_new_std(ctrl_hdlr, _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.
>
> 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".
cheers,
grant
>
> Best regards,
> Tomasz
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
[+Laurent and Sylwester]
On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen
wrote:
[snip]
> +
> +/* Digital gain control */
> +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e
> +#define IMX208_REG_R_DIGITAL_GAIN 0x0210
> +#define IMX208_REG_B_DIGITAL_GAIN 0x0212
> +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214
> +#define IMX208_DGTL_GAIN_MIN 0
> +#define IMX208_DGTL_GAIN_MAX 4096
> +#define IMX208_DGTL_GAIN_DEFAULT 0x100
> +#define IMX208_DGTL_GAIN_STEP 1
> +
[snip]
> +/* Initialize control handlers */
> +static int imx208_init_controls(struct imx208 *imx208)
> +{
[snip]
> + v4l2_ctrl_new_std(ctrl_hdlr, _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.
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.
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?
Best regards,
Tomasz
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Grant,
On Mon, Sep 17, 2018 at 03:52:30PM -0700, Grant Grundler wrote:
> On Fri, Sep 14, 2018 at 4:41 AM Sakari Ailus
> wrote:
> >
> > Hi Ping-chung,
> >
> > My apologies for the late review.
>
> Yeah...I had the impression this was already accepted. Though it
> should be straight forward to fix up additional things as normal
> patches.
The remaining issues are rather small and there's still time to get the
driver to v4.20, so I see no need to postpone these either.
>
> [sorry pruning heavily]
> ...
> > > +/* HBLANK control - read only */
> > > +#define IMX208_PPL_384MHZ2248
> > > +#define IMX208_PPL_96MHZ 2248
> >
> > Does this generally depend on the link frequency?
>
> This was discussed in earlier patch version: in a nutshell, yes.
>
> ...
> > > +/* Configurations for supported link frequencies */
> > > +#define IMX208_MHZ (1000*1000ULL)
> > > +#define IMX208_LINK_FREQ_384MHZ (384ULL * IMX208_MHZ)
> > > +#define IMX208_LINK_FREQ_96MHZ (96ULL * IMX208_MHZ)
> >
> > You could simply write these as 38400 and 9600.
>
> The original code did that. I agree IMX208_MHZ makes this much easier to read.
It is not customary to add driver specific defines for that sort of things;
mostly if you need a plain number you do write a plain number. A sort of an
exception are the SZ_* macros.
The above breaks grep, too.
>
> ...
> > > + /* Current mode */
> > > + const struct imx208_mode *cur_mode;
> > > +
> > > + /*
> > > + * Mutex for serialized access:
> > > + * Protect sensor set pad format and start/stop streaming safely.
> > > + * Protect access to sensor v4l2 controls.
> > > + */
> > > + struct mutex imx208_mx;
> >
> > How about calling it simply e.g. a "mutex"? The struct is already specific
> > to imx208.
>
> I specifically asked the code not use "mutex" because trying to find
> this specific use of "mutex" with cscope (ctags) is impossible.
The mutex is local to the driver, and in this case also to the file.
Mutexes are commonly called either "mutex" or "lock".
>
> Defining "mutex" in multiple name spaces is asking for trouble even
> though technically it's "safe" to do.
>
> ...
> > > +static int imx208_set_pad_format(struct v4l2_subdev *sd,
> > > +struct v4l2_subdev_pad_config *cfg,
> > > +struct v4l2_subdev_format *fmt)
> > > +{
> > > + struct imx208 *imx208 = to_imx208(sd);
> > > + const struct imx208_mode *mode;
> > > + s32 vblank_def;
> > > + s32 vblank_min;
> > > + s64 h_blank;
> > > + s64 pixel_rate;
> > > + s64 link_freq;
> > > +
> > > + mutex_lock(>imx208_mx);
> > > +
> > > + fmt->format.code = imx208_get_format_code(imx208);
> > > + mode = v4l2_find_nearest_size(
> > > + supported_modes, ARRAY_SIZE(supported_modes), width, height,
> > > + fmt->format.width, fmt->format.height);
> > > + imx208_mode_to_pad_format(imx208, mode, fmt);
> > > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > > + *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) =
> > > fmt->format;
> > > + } else {
> > > + imx208->cur_mode = mode;
> > > + __v4l2_ctrl_s_ctrl(imx208->link_freq,
> > > mode->link_freq_index);
> > > + link_freq = link_freq_menu_items[mode->link_freq_index];
> >
> > Same as on the imx319 driver --- the link frequencies that are available
> > need to reflect what is specified in firmware.
>
> :)
>
> ...
> > > +static int imx208_set_stream(struct v4l2_subdev *sd, int enable)
> > > +{
> > > + struct imx208 *imx208 = to_imx208(sd);
> > > + struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > + int ret = 0;
> > > +
> > > + mutex_lock(>imx208_mx);
> > > + if (imx208->streaming == enable) {
> > > + mutex_unlock(>imx208_mx);
> > > + return 0;
> > > + }
> > > +
> > > + if (enable) {
> > > + ret = pm_runtime_get_sync(>dev);
> > > + if (ret < 0)
> > > + goto err_rpm_put;
> > > +
> > > + /*
> > > + * Apply default & customized values
> > > + * and then start streaming.
> > > + */
> > > + ret = imx208_start_streaming(imx208);
> > > + if (ret)
> > > + goto err_rpm_put;
> > > + } else {
> > > + imx208_stop_streaming(imx208);
> > > + pm_runtime_put(>dev);
> > > + }
> > > +
> > > + imx208->streaming = enable;
> > > + mutex_unlock(>imx208_mx);
> > > +
> > > + /* vflip and hflip cannot change during streaming */
> > > + v4l2_ctrl_grab(imx208->vflip, enable);
> > > + v4l2_ctrl_grab(imx208->hflip, enable);
> >
> > Please grab before releasing the lock; use __v4l2_ctrl_grab() here:
> >
> > https://git.linuxtv.org/sailus/media_tree.git/log/?h=unlocked-ctrl-grab>
>
> Is the current implementation
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
On Fri, Sep 14, 2018 at 4:41 AM Sakari Ailus
wrote:
>
> Hi Ping-chung,
>
> My apologies for the late review.
Yeah...I had the impression this was already accepted. Though it
should be straight forward to fix up additional things as normal
patches.
[sorry pruning heavily]
...
> > +/* HBLANK control - read only */
> > +#define IMX208_PPL_384MHZ2248
> > +#define IMX208_PPL_96MHZ 2248
>
> Does this generally depend on the link frequency?
This was discussed in earlier patch version: in a nutshell, yes.
...
> > +/* Configurations for supported link frequencies */
> > +#define IMX208_MHZ (1000*1000ULL)
> > +#define IMX208_LINK_FREQ_384MHZ (384ULL * IMX208_MHZ)
> > +#define IMX208_LINK_FREQ_96MHZ (96ULL * IMX208_MHZ)
>
> You could simply write these as 38400 and 9600.
The original code did that. I agree IMX208_MHZ makes this much easier to read.
...
> > + /* Current mode */
> > + const struct imx208_mode *cur_mode;
> > +
> > + /*
> > + * Mutex for serialized access:
> > + * Protect sensor set pad format and start/stop streaming safely.
> > + * Protect access to sensor v4l2 controls.
> > + */
> > + struct mutex imx208_mx;
>
> How about calling it simply e.g. a "mutex"? The struct is already specific
> to imx208.
I specifically asked the code not use "mutex" because trying to find
this specific use of "mutex" with cscope (ctags) is impossible.
Defining "mutex" in multiple name spaces is asking for trouble even
though technically it's "safe" to do.
...
> > +static int imx208_set_pad_format(struct v4l2_subdev *sd,
> > +struct v4l2_subdev_pad_config *cfg,
> > +struct v4l2_subdev_format *fmt)
> > +{
> > + struct imx208 *imx208 = to_imx208(sd);
> > + const struct imx208_mode *mode;
> > + s32 vblank_def;
> > + s32 vblank_min;
> > + s64 h_blank;
> > + s64 pixel_rate;
> > + s64 link_freq;
> > +
> > + mutex_lock(>imx208_mx);
> > +
> > + fmt->format.code = imx208_get_format_code(imx208);
> > + mode = v4l2_find_nearest_size(
> > + supported_modes, ARRAY_SIZE(supported_modes), width, height,
> > + fmt->format.width, fmt->format.height);
> > + imx208_mode_to_pad_format(imx208, mode, fmt);
> > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > + *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format;
> > + } else {
> > + imx208->cur_mode = mode;
> > + __v4l2_ctrl_s_ctrl(imx208->link_freq, mode->link_freq_index);
> > + link_freq = link_freq_menu_items[mode->link_freq_index];
>
> Same as on the imx319 driver --- the link frequencies that are available
> need to reflect what is specified in firmware.
:)
...
> > +static int imx208_set_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > + struct imx208 *imx208 = to_imx208(sd);
> > + struct i2c_client *client = v4l2_get_subdevdata(sd);
> > + int ret = 0;
> > +
> > + mutex_lock(>imx208_mx);
> > + if (imx208->streaming == enable) {
> > + mutex_unlock(>imx208_mx);
> > + return 0;
> > + }
> > +
> > + if (enable) {
> > + ret = pm_runtime_get_sync(>dev);
> > + if (ret < 0)
> > + goto err_rpm_put;
> > +
> > + /*
> > + * Apply default & customized values
> > + * and then start streaming.
> > + */
> > + ret = imx208_start_streaming(imx208);
> > + if (ret)
> > + goto err_rpm_put;
> > + } else {
> > + imx208_stop_streaming(imx208);
> > + pm_runtime_put(>dev);
> > + }
> > +
> > + imx208->streaming = enable;
> > + mutex_unlock(>imx208_mx);
> > +
> > + /* vflip and hflip cannot change during streaming */
> > + v4l2_ctrl_grab(imx208->vflip, enable);
> > + v4l2_ctrl_grab(imx208->hflip, enable);
>
> Please grab before releasing the lock; use __v4l2_ctrl_grab() here:
>
> https://git.linuxtv.org/sailus/media_tree.git/log/?h=unlocked-ctrl-grab>
Is the current implementation not correct or is this just the
preferred way to "grab"?
(And thanks for pointing at the patch which adds the new "API")
(and I'm ignoring the remaining nit on the assumption it can be
addressed in the next patch)
cheers,
grant
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Ping-chung, My apologies for the late review. On Wed, Aug 08, 2018 at 03:16:00PM +0800, Ping-chung Chen wrote: > From: "Chen, Ping-chung" > > Add a V4L2 sub-device driver for the Sony IMX208 image sensor. > This is a camera sensor using the I2C bus for control and the > CSI-2 bus for data. > > Signed-off-by: Ping-Chung Chen > Reviewed-by: Tomasz Figa > > --- > since v1: > -- Update the function media_entity_pads_init for upstreaming. > -- Change the structure name mutex as imx208_mx. > -- Refine the control flow of test pattern function. > -- vflip/hflip control support (will impact the output bayer order) > -- support 4 bayer orders output (via change v/hflip) > - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10 > -- Simplify error handling in the set_stream function. > since v2: > -- Refine coding style. > -- Fix the if statement to use pm_runtime_get_if_in_use(). > -- Print more error log during error handling. > -- Remove mutex_destroy() from imx208_free_controls(). > -- Add more comments. > since v3: > -- Set explicit indices to link frequencies. > since v4: > -- Fix the wrong index in link_freq_menu_items. > > MAINTAINERS| 7 + > drivers/media/i2c/Kconfig | 11 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/imx208.c | 995 > + > 4 files changed, 1014 insertions(+) > create mode 100644 drivers/media/i2c/imx208.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index bbd9b9b..896c1df 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13268,6 +13268,13 @@ S: Maintained > F: drivers/ssb/ > F: include/linux/ssb/ > > +SONY IMX208 SENSOR DRIVER > +M: Sakari Ailus > +L: linux-media@vger.kernel.org > +T: git git://linuxtv.org/media_tree.git > +S: Maintained > +F: drivers/media/i2c/imx208.c > + > SONY IMX258 SENSOR DRIVER > M: Sakari Ailus > L: linux-media@vger.kernel.org > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 8669853..ae11f1e 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -595,6 +595,17 @@ config VIDEO_APTINA_PLL > config VIDEO_SMIAPP_PLL > tristate > > +config VIDEO_IMX208 > + tristate "Sony IMX208 sensor support" > + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API > + depends on MEDIA_CAMERA_SUPPORT > + ---help--- > + This is a Video4Linux2 sensor driver for the Sony > + IMX208 camera. > + > + To compile this driver as a module, choose M here: the > + module will be called imx208. > + > config VIDEO_IMX258 > tristate "Sony IMX258 sensor support" > depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index 837c428..47604c2 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -104,6 +104,7 @@ obj-$(CONFIG_VIDEO_I2C) += video-i2c.o > obj-$(CONFIG_VIDEO_ML86V7667)+= ml86v7667.o > obj-$(CONFIG_VIDEO_OV2659) += ov2659.o > obj-$(CONFIG_VIDEO_TC358743) += tc358743.o > +obj-$(CONFIG_VIDEO_IMX208) += imx208.o > obj-$(CONFIG_VIDEO_IMX258) += imx258.o > obj-$(CONFIG_VIDEO_IMX274) += imx274.o > > diff --git a/drivers/media/i2c/imx208.c b/drivers/media/i2c/imx208.c > new file mode 100644 > index 000..61de0c8 > --- /dev/null > +++ b/drivers/media/i2c/imx208.c > @@ -0,0 +1,995 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2018 Intel Corporation > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define IMX208_REG_MODE_SELECT 0x0100 > +#define IMX208_MODE_STANDBY 0x00 > +#define IMX208_MODE_STREAMING0x01 > + > +/* Chip ID */ > +#define IMX208_REG_CHIP_ID 0x > +#define IMX208_CHIP_ID 0x0208 > + > +/* V_TIMING internal */ > +#define IMX208_REG_VTS 0x0340 > +#define IMX208_VTS_60FPS 0x0472 > +#define IMX208_VTS_BINNING 0x0239 > +#define IMX208_VTS_60FPS_MIN 0x0458 > +#define IMX208_VTS_BINNING_MIN 0x0230 > +#define IMX208_VTS_MAX 0x > + > +/* HBLANK control - read only */ > +#define IMX208_PPL_384MHZ2248 > +#define IMX208_PPL_96MHZ 2248 Does this generally depend on the link frequency? > + > +/* Exposure control */ > +#define IMX208_REG_EXPOSURE 0x0202 > +#define IMX208_EXPOSURE_MIN 4 > +#define IMX208_EXPOSURE_STEP 1 > +#define IMX208_EXPOSURE_DEFAULT 0x190 > +#define IMX208_EXPOSURE_MAX 65535 > + > +/* Analog gain control */ > +#define IMX208_REG_ANALOG_GAIN 0x0204 > +#define IMX208_ANA_GAIN_MIN 0 > +#define IMX208_ANA_GAIN_MAX 0x00e0 > +#define IMX208_ANA_GAIN_STEP 1 > +#define IMX208_ANA_GAIN_DEFAULT 0x0 > + > +/* Digital gain control */ > +#define
