Hi Sakari, PC, >-----Original Message----- >From: Sakari Ailus [mailto:sakari.ailus@xxxxxxxxxxxxxxx] >Sent: Wednesday, September 26, 2018 6:12 PM >To: Chen, Ping-chung <ping-chung.chen@xxxxxxxxx> >Cc: Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx>; Laurent Pinchart ><laurent.pinchart@xxxxxxxxxxxxxxxx>; Sakari Ailus <sakari.ailus@xxxxxx>; >tfiga@xxxxxxxxxxxx; sylwester.nawrocki@xxxxxxxxx; linux-media <linux- >media@xxxxxxxxxxxxxxx>; Yeh, Andy <andy.yeh@xxxxxxxxx>; Lai, Jim ><jim.lai@xxxxxxxxx>; grundler@xxxxxxxxxxxx; Mani, Rajmohan ><rajmohan.mani@xxxxxxxxx> >Subject: Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver > >Hi Ping-chung, > >On Wed, Sep 26, 2018 at 02:27:01AM +0000, Chen, Ping-chung wrote: >> Hi Sakari, >> >> >-----Original Message----- >> >From: Sakari Ailus [mailto:sakari.ailus@xxxxxxxxxxxxxxx] >> >Sent: Wednesday, September 26, 2018 5:55 AM >> >> >Hi Ping-chung, >> >> >On Tue, Sep 25, 2018 at 10:17:48AM +0000, 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. We shall ensure the expansibility of this architecture to include other kind of styles in the future. Is this API design architecture-wise ok? > >> 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. >-- >Regards, > >Sakari Ailus >sakari.ailus@xxxxxxxxxxxxxxx
