From: "Chen, Ping-chung" <ping-chung.chen@intel.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Sakari Ailus <sakari.ailus@iki.fi>,
"tfiga@chromium.org" <tfiga@chromium.org>,
"sylwester.nawrocki@gmail.com" <sylwester.nawrocki@gmail.com>,
linux-media <linux-media@vger.kernel.org>,
"Yeh, Andy" <andy.yeh@intel.com>, "Lai, Jim" <jim.lai@intel.com>,
"grundler@chromium.org" <grundler@chromium.org>,
"Mani, Rajmohan" <rajmohan.mani@intel.com>
Subject: RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Date: Tue, 25 Sep 2018 10:17:48 +0000 [thread overview]
Message-ID: <5E40A82D0551C84FA2888225EDABBE093FACD5E5@PGSMSX105.gar.corp.intel.com> (raw)
In-Reply-To: <20180925092527.4apdggynxleigvbv@paasikivi.fi.intel.com>
Hi Sakari,
>-----Original Message-----
>From: Sakari Ailus [mailto:sakari.ailus@linux.intel.com]
>Sent: Tuesday, September 25, 2018 5:25 PM
>To: Chen, Ping-chung <ping-chung.chen@intel.com>
>Cc: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>; 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 +0000, Chen, Ping-chung wrote:
> Typo.
>
> -----Original Message-----
> From: Chen, Ping-chung
> Sent: Friday, September 21, 2018 3:07 PM
> To: 'Ricardo Ribalda Delgado' <ricardo.ribalda@gmail.com>; Laurent
> Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Sakari Ailus <sakari.ailus@iki.fi>; tfiga@chromium.org; Sakari
> Ailus <sakari.ailus@linux.intel.com>; sylwester.nawrocki@gmail.com;
> linux-media <linux-media@vger.kernel.org>; Yeh, Andy
> <andy.yeh@intel.com>; Lai, Jim <jim.lai@intel.com>;
> grundler@chromium.org; Mani, Rajmohan <rajmohan.mani@intel.com>
> Subject: RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver
>
> Hi Sakari,
>
> >-----Original Message-----
> >From: Ricardo Ribalda Delgado [mailto:ricardo.ribalda@gmail.com]
> >Sent: Friday, September 21, 2018 5:55 AM
>
> >HI
> On Thu, Sep 20, 2018 at 11:13 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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, &imx208_ctrl_ops,
> > > > > V4L2_CID_DIGITAL_GAIN, + IMX208_DGTL_GAIN_MIN,
> > > > > IMX208_DGTL_GAIN_MAX, + IMX208_DGTL_GAIN_STEP,
> > > > > + IMX208_DGTL_GAIN_DEFAULT);
> > > >
> > > > We have a problem here. The sensor supports only a discrete
> > > > range of values here - {1, 2, 4, 8, 16} (multiplied by 256,
> > > > since the value is fixed point). This makes it possible for the
> > > > userspace to set values that are not allowed by the sensor
> > > > specification and also leaves no way to enumerate the supported values.
> > > >
> > > > 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.
>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;
return ret;
}
Thanks,
PC Chen
>--
>Sakari Ailus
>sakari.ailus@linux.intel.com
next prev parent reply other threads:[~2018-09-25 16:26 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-08 7:16 [PATCH v5] media: imx208: Add imx208 camera sensor driver Ping-chung Chen
2018-09-14 11:41 ` Sakari Ailus
2018-09-17 22:52 ` Grant Grundler
2018-09-18 10:52 ` Sakari Ailus
2018-09-20 8:51 ` Tomasz Figa
2018-09-20 16:49 ` Grant Grundler
2018-09-20 20:16 ` Sylwester Nawrocki
2018-09-20 21:00 ` Laurent Pinchart
2018-09-21 7:23 ` Helmut Grohne
2018-09-28 13:49 ` Laurent Pinchart
2018-10-01 10:50 ` Helmut Grohne
2018-10-01 12:04 ` Philippe De Muyter
2018-09-20 20:56 ` Sakari Ailus
2018-09-20 21:12 ` Laurent Pinchart
2018-09-20 21:55 ` Ricardo Ribalda Delgado
2018-09-21 7:06 ` Chen, Ping-chung
2018-09-21 7:08 ` Chen, Ping-chung
2018-09-25 9:25 ` Sakari Ailus
2018-09-25 10:17 ` Chen, Ping-chung [this message]
2018-09-25 21:54 ` Sakari Ailus
2018-09-26 2:27 ` Chen, Ping-chung
2018-09-26 10:11 ` Sakari Ailus
2018-09-26 15:19 ` Yeh, Andy
2018-09-27 3:19 ` Chen, Ping-chung
2018-10-04 15:57 ` Sakari Ailus
2021-04-22 7:21 ` Tu, ShawnX
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5E40A82D0551C84FA2888225EDABBE093FACD5E5@PGSMSX105.gar.corp.intel.com \
--to=ping-chung.chen@intel.com \
--cc=andy.yeh@intel.com \
--cc=grundler@chromium.org \
--cc=jim.lai@intel.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=rajmohan.mani@intel.com \
--cc=ricardo.ribalda@gmail.com \
--cc=sakari.ailus@iki.fi \
--cc=sakari.ailus@linux.intel.com \
--cc=sylwester.nawrocki@gmail.com \
--cc=tfiga@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).