| Message ID | 1597996790-21121-1-git-send-email-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] media: ov5675: correct the maximum exposure value | expand |
Hi Bingbu, On Fri, Aug 21, 2020 at 10:00 AM Bingbu Cao <bingbu.cao@intel.com> wrote: > > The unit of exposure value is different from other OmniVision sensors, > driver will divide by 2 before set register, the exposure range exposed > by v4l2 ctrl to user should be same as others, so the calculation for > the maximum exposure value in current driver need be fixed. > > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > --- > drivers/media/i2c/ov5675.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > Thanks for the patch! Please see my comments inline. > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c > index 8537cc4ca108..9540ce8918f0 100644 > --- a/drivers/media/i2c/ov5675.c > +++ b/drivers/media/i2c/ov5675.c > @@ -666,8 +666,8 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl) > /* Propagate change of current control to all related controls */ > if (ctrl->id == V4L2_CID_VBLANK) { > /* Update max exposure while meeting expected vblanking */ > - exposure_max = (ov5675->cur_mode->height + ctrl->val - > - OV5675_EXPOSURE_MAX_MARGIN) / 2; > + exposure_max = ov5675->cur_mode->height + ctrl->val - > + OV5675_EXPOSURE_MAX_MARGIN; > __v4l2_ctrl_modify_range(ov5675->exposure, > ov5675->exposure->minimum, > exposure_max, ov5675->exposure->step, > @@ -689,7 +689,13 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl) > break; > > case V4L2_CID_EXPOSURE: > - /* 3 least significant bits of expsoure are fractional part */ > + /* 4 least significant bits of expsoure are fractional part exposure > + * val = val << 4 > + * for ov5675, the unit of exposure is differnt from other different > + * OmniVision sensors, its exposure value is twice of the > + * register value, the exposure should be divided by 2 before > + * set register, e.g. val << 3. > + */ > ret = ov5675_write_reg(ov5675, OV5675_REG_EXPOSURE, > OV5675_REG_VALUE_24BIT, ctrl->val << 3); How about turning this code into (ctrl->val << 4) / 2 ? It will be compiled into exactly the same code, but will be obvious that we are handling two different factors in the computation. Another question: Since the V4L2_CID_EXPOSURE control is not specified to be in a particular unit and hardware supports fractional exposures, why not expose the exposure exactly as it is in the register? Best regards, Tomasz > break; > @@ -770,8 +776,7 @@ static int ov5675_init_controls(struct ov5675 *ov5675) > v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops, V4L2_CID_DIGITAL_GAIN, > OV5675_DGTL_GAIN_MIN, OV5675_DGTL_GAIN_MAX, > OV5675_DGTL_GAIN_STEP, OV5675_DGTL_GAIN_DEFAULT); > - exposure_max = (ov5675->cur_mode->vts_def - > - OV5675_EXPOSURE_MAX_MARGIN) / 2; > + exposure_max = (ov5675->cur_mode->vts_def - OV5675_EXPOSURE_MAX_MARGIN); > ov5675->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops, > V4L2_CID_EXPOSURE, > OV5675_EXPOSURE_MIN, exposure_max, > -- > 2.7.4 >
On 8/22/20 11:03 PM, Tomasz Figa wrote: > Hi Bingbu, > > On Fri, Aug 21, 2020 at 10:00 AM Bingbu Cao <bingbu.cao@intel.com> wrote: >> >> The unit of exposure value is different from other OmniVision sensors, >> driver will divide by 2 before set register, the exposure range exposed >> by v4l2 ctrl to user should be same as others, so the calculation for >> the maximum exposure value in current driver need be fixed. >> >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> >> --- >> drivers/media/i2c/ov5675.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> > > Thanks for the patch! Please see my comments inline. > >> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c >> index 8537cc4ca108..9540ce8918f0 100644 >> --- a/drivers/media/i2c/ov5675.c >> +++ b/drivers/media/i2c/ov5675.c >> @@ -666,8 +666,8 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl) >> /* Propagate change of current control to all related controls */ >> if (ctrl->id == V4L2_CID_VBLANK) { >> /* Update max exposure while meeting expected vblanking */ >> - exposure_max = (ov5675->cur_mode->height + ctrl->val - >> - OV5675_EXPOSURE_MAX_MARGIN) / 2; >> + exposure_max = ov5675->cur_mode->height + ctrl->val - >> + OV5675_EXPOSURE_MAX_MARGIN; >> __v4l2_ctrl_modify_range(ov5675->exposure, >> ov5675->exposure->minimum, >> exposure_max, ov5675->exposure->step, >> @@ -689,7 +689,13 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl) >> break; >> >> case V4L2_CID_EXPOSURE: >> - /* 3 least significant bits of expsoure are fractional part */ >> + /* 4 least significant bits of expsoure are fractional part > > exposure > >> + * val = val << 4 >> + * for ov5675, the unit of exposure is differnt from other > > different > >> + * OmniVision sensors, its exposure value is twice of the >> + * register value, the exposure should be divided by 2 before >> + * set register, e.g. val << 3. >> + */ >> ret = ov5675_write_reg(ov5675, OV5675_REG_EXPOSURE, >> OV5675_REG_VALUE_24BIT, ctrl->val << 3); > > How about turning this code into (ctrl->val << 4) / 2 ? It will be > compiled into exactly the same code, but will be obvious that we are > handling two different factors in the computation. > > Another question: Since the V4L2_CID_EXPOSURE control is not specified > to be in a particular unit and hardware supports fractional exposures, > why not expose the exposure exactly as it is in the register? My understanding is that the exposure calculation in userspace is commonly based on the unit of exposure - line, as some sensor did not support fractional exposures, so the common calculation only care the integral part. For ov5675, it is different from others, its register value unit is 2lines instead of lines. Sakari, do you have some idea? > > Best regards, > Tomasz > >> break; >> @@ -770,8 +776,7 @@ static int ov5675_init_controls(struct ov5675 *ov5675) >> v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops, V4L2_CID_DIGITAL_GAIN, >> OV5675_DGTL_GAIN_MIN, OV5675_DGTL_GAIN_MAX, >> OV5675_DGTL_GAIN_STEP, OV5675_DGTL_GAIN_DEFAULT); >> - exposure_max = (ov5675->cur_mode->vts_def - >> - OV5675_EXPOSURE_MAX_MARGIN) / 2; >> + exposure_max = (ov5675->cur_mode->vts_def - OV5675_EXPOSURE_MAX_MARGIN); >> ov5675->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops, >> V4L2_CID_EXPOSURE, >> OV5675_EXPOSURE_MIN, exposure_max, >> -- >> 2.7.4 >>
Hi Bingbu, On Mon, Aug 24, 2020 at 11:13:40AM +0800, Bingbu Cao wrote: > > > On 8/22/20 11:03 PM, Tomasz Figa wrote: > > Hi Bingbu, > > > > On Fri, Aug 21, 2020 at 10:00 AM Bingbu Cao <bingbu.cao@intel.com> wrote: > >> > >> The unit of exposure value is different from other OmniVision sensors, > >> driver will divide by 2 before set register, the exposure range exposed > >> by v4l2 ctrl to user should be same as others, so the calculation for > >> the maximum exposure value in current driver need be fixed. > >> > >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > >> --- > >> drivers/media/i2c/ov5675.c | 15 ++++++++++----- > >> 1 file changed, 10 insertions(+), 5 deletions(-) > >> > > > > Thanks for the patch! Please see my comments inline. > > > >> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c > >> index 8537cc4ca108..9540ce8918f0 100644 > >> --- a/drivers/media/i2c/ov5675.c > >> +++ b/drivers/media/i2c/ov5675.c > >> @@ -666,8 +666,8 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl) > >> /* Propagate change of current control to all related controls */ > >> if (ctrl->id == V4L2_CID_VBLANK) { > >> /* Update max exposure while meeting expected vblanking */ > >> - exposure_max = (ov5675->cur_mode->height + ctrl->val - > >> - OV5675_EXPOSURE_MAX_MARGIN) / 2; > >> + exposure_max = ov5675->cur_mode->height + ctrl->val - > >> + OV5675_EXPOSURE_MAX_MARGIN; > >> __v4l2_ctrl_modify_range(ov5675->exposure, > >> ov5675->exposure->minimum, > >> exposure_max, ov5675->exposure->step, > >> @@ -689,7 +689,13 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl) > >> break; > >> > >> case V4L2_CID_EXPOSURE: > >> - /* 3 least significant bits of expsoure are fractional part */ > >> + /* 4 least significant bits of expsoure are fractional part > > > > exposure > > > >> + * val = val << 4 > >> + * for ov5675, the unit of exposure is differnt from other > > > > different > > > >> + * OmniVision sensors, its exposure value is twice of the > >> + * register value, the exposure should be divided by 2 before > >> + * set register, e.g. val << 3. > >> + */ > >> ret = ov5675_write_reg(ov5675, OV5675_REG_EXPOSURE, > >> OV5675_REG_VALUE_24BIT, ctrl->val << 3); > > > > How about turning this code into (ctrl->val << 4) / 2 ? It will be > > compiled into exactly the same code, but will be obvious that we are > > handling two different factors in the computation. > > > > Another question: Since the V4L2_CID_EXPOSURE control is not specified > > to be in a particular unit and hardware supports fractional exposures, > > why not expose the exposure exactly as it is in the register? > My understanding is that the exposure calculation in userspace is commonly based > on the unit of exposure - line, as some sensor did not support fractional > exposures, so the common calculation only care the integral part. For ov5675, it > is different from others, its register value unit is 2lines instead of lines. > > Sakari, do you have some idea? I've already sent v2 in a pull request to Mauro. So if changes are needed still, please send them on top of v2. Line is commonly used for devices that natively use it (vast majority of raw Bayer camera sensors). So if possible, I'd use the same here. I.e. with two line granularity, the exposure value could be in lines and the step would be 2.
On Mon, Aug 24, 2020 at 11:00 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Bingbu, > > On Mon, Aug 24, 2020 at 11:13:40AM +0800, Bingbu Cao wrote: > > > > > > On 8/22/20 11:03 PM, Tomasz Figa wrote: > > > Hi Bingbu, > > > > > > On Fri, Aug 21, 2020 at 10:00 AM Bingbu Cao <bingbu.cao@intel.com> wrote: > > >> > > >> The unit of exposure value is different from other OmniVision sensors, > > >> driver will divide by 2 before set register, the exposure range exposed > > >> by v4l2 ctrl to user should be same as others, so the calculation for > > >> the maximum exposure value in current driver need be fixed. > > >> > > >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > > >> --- > > >> drivers/media/i2c/ov5675.c | 15 ++++++++++----- > > >> 1 file changed, 10 insertions(+), 5 deletions(-) > > >> > > > > > > Thanks for the patch! Please see my comments inline. > > > > > >> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c > > >> index 8537cc4ca108..9540ce8918f0 100644 > > >> --- a/drivers/media/i2c/ov5675.c > > >> +++ b/drivers/media/i2c/ov5675.c > > >> @@ -666,8 +666,8 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl) > > >> /* Propagate change of current control to all related controls */ > > >> if (ctrl->id == V4L2_CID_VBLANK) { > > >> /* Update max exposure while meeting expected vblanking */ > > >> - exposure_max = (ov5675->cur_mode->height + ctrl->val - > > >> - OV5675_EXPOSURE_MAX_MARGIN) / 2; > > >> + exposure_max = ov5675->cur_mode->height + ctrl->val - > > >> + OV5675_EXPOSURE_MAX_MARGIN; > > >> __v4l2_ctrl_modify_range(ov5675->exposure, > > >> ov5675->exposure->minimum, > > >> exposure_max, ov5675->exposure->step, > > >> @@ -689,7 +689,13 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl) > > >> break; > > >> > > >> case V4L2_CID_EXPOSURE: > > >> - /* 3 least significant bits of expsoure are fractional part */ > > >> + /* 4 least significant bits of expsoure are fractional part > > > > > > exposure > > > > > >> + * val = val << 4 > > >> + * for ov5675, the unit of exposure is differnt from other > > > > > > different > > > > > >> + * OmniVision sensors, its exposure value is twice of the > > >> + * register value, the exposure should be divided by 2 before > > >> + * set register, e.g. val << 3. > > >> + */ > > >> ret = ov5675_write_reg(ov5675, OV5675_REG_EXPOSURE, > > >> OV5675_REG_VALUE_24BIT, ctrl->val << 3); > > > > > > How about turning this code into (ctrl->val << 4) / 2 ? It will be > > > compiled into exactly the same code, but will be obvious that we are > > > handling two different factors in the computation. > > > > > > Another question: Since the V4L2_CID_EXPOSURE control is not specified > > > to be in a particular unit and hardware supports fractional exposures, > > > why not expose the exposure exactly as it is in the register? > > My understanding is that the exposure calculation in userspace is commonly based > > on the unit of exposure - line, as some sensor did not support fractional > > exposures, so the common calculation only care the integral part. For ov5675, it > > is different from others, its register value unit is 2lines instead of lines. > > > > Sakari, do you have some idea? > > I've already sent v2 in a pull request to Mauro. So if changes are needed > still, please send them on top of v2. > > Line is commonly used for devices that natively use it (vast majority of > raw Bayer camera sensors). So if possible, I'd use the same here. I.e. with > two line granularity, the exposure value could be in lines and the step > would be 2. The sensor unit seems to be 1/8 of a line, so by exposing it from the driver as an integer in the unit of lines, we end up losing some precision. I'm not sure how relevant it is for image quality, though. Best regards, Tomasz
On Mon, Aug 24, 2020 at 03:23:21PM +0200, Tomasz Figa wrote: > On Mon, Aug 24, 2020 at 11:00 AM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Bingbu, > > > > On Mon, Aug 24, 2020 at 11:13:40AM +0800, Bingbu Cao wrote: > > > > > > > > > On 8/22/20 11:03 PM, Tomasz Figa wrote: > > > > Hi Bingbu, > > > > > > > > On Fri, Aug 21, 2020 at 10:00 AM Bingbu Cao <bingbu.cao@intel.com> wrote: > > > >> > > > >> The unit of exposure value is different from other OmniVision sensors, > > > >> driver will divide by 2 before set register, the exposure range exposed > > > >> by v4l2 ctrl to user should be same as others, so the calculation for > > > >> the maximum exposure value in current driver need be fixed. > > > >> > > > >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > > > >> --- > > > >> drivers/media/i2c/ov5675.c | 15 ++++++++++----- > > > >> 1 file changed, 10 insertions(+), 5 deletions(-) > > > >> > > > > > > > > Thanks for the patch! Please see my comments inline. > > > > > > > >> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c > > > >> index 8537cc4ca108..9540ce8918f0 100644 > > > >> --- a/drivers/media/i2c/ov5675.c > > > >> +++ b/drivers/media/i2c/ov5675.c > > > >> @@ -666,8 +666,8 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl) > > > >> /* Propagate change of current control to all related controls */ > > > >> if (ctrl->id == V4L2_CID_VBLANK) { > > > >> /* Update max exposure while meeting expected vblanking */ > > > >> - exposure_max = (ov5675->cur_mode->height + ctrl->val - > > > >> - OV5675_EXPOSURE_MAX_MARGIN) / 2; > > > >> + exposure_max = ov5675->cur_mode->height + ctrl->val - > > > >> + OV5675_EXPOSURE_MAX_MARGIN; > > > >> __v4l2_ctrl_modify_range(ov5675->exposure, > > > >> ov5675->exposure->minimum, > > > >> exposure_max, ov5675->exposure->step, > > > >> @@ -689,7 +689,13 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl) > > > >> break; > > > >> > > > >> case V4L2_CID_EXPOSURE: > > > >> - /* 3 least significant bits of expsoure are fractional part */ > > > >> + /* 4 least significant bits of expsoure are fractional part > > > > > > > > exposure > > > > > > > >> + * val = val << 4 > > > >> + * for ov5675, the unit of exposure is differnt from other > > > > > > > > different > > > > > > > >> + * OmniVision sensors, its exposure value is twice of the > > > >> + * register value, the exposure should be divided by 2 before > > > >> + * set register, e.g. val << 3. > > > >> + */ > > > >> ret = ov5675_write_reg(ov5675, OV5675_REG_EXPOSURE, > > > >> OV5675_REG_VALUE_24BIT, ctrl->val << 3); > > > > > > > > How about turning this code into (ctrl->val << 4) / 2 ? It will be > > > > compiled into exactly the same code, but will be obvious that we are > > > > handling two different factors in the computation. > > > > > > > > Another question: Since the V4L2_CID_EXPOSURE control is not specified > > > > to be in a particular unit and hardware supports fractional exposures, > > > > why not expose the exposure exactly as it is in the register? > > > My understanding is that the exposure calculation in userspace is commonly based > > > on the unit of exposure - line, as some sensor did not support fractional > > > exposures, so the common calculation only care the integral part. For ov5675, it > > > is different from others, its register value unit is 2lines instead of lines. > > > > > > Sakari, do you have some idea? > > > > I've already sent v2 in a pull request to Mauro. So if changes are needed > > still, please send them on top of v2. > > > > Line is commonly used for devices that natively use it (vast majority of > > raw Bayer camera sensors). So if possible, I'd use the same here. I.e. with > > two line granularity, the exposure value could be in lines and the step > > would be 2. > > The sensor unit seems to be 1/8 of a line, so by exposing it from the > driver as an integer in the unit of lines, we end up losing some > precision. I'm not sure how relevant it is for image quality, though. Oh well. Generally it's time consuming to update the registers over I²C and the more registers you add, the higher the likelihood is to miss doing that in time. That's why the common pattern of lines / pixels matches really well with this as both exposure (and the supposedly added fine exposure control, when needed) match with the registers and control units. I don't have a perfect answer for this. You could do either. I think I'd be in favour of keeping the exposure in lines so that the frame rate calculation works as expected, and adding a fine exposure control if needed. Does the sub-line exposure value include horizontal blanking?
diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c index 8537cc4ca108..9540ce8918f0 100644 --- a/drivers/media/i2c/ov5675.c +++ b/drivers/media/i2c/ov5675.c @@ -666,8 +666,8 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl) /* Propagate change of current control to all related controls */ if (ctrl->id == V4L2_CID_VBLANK) { /* Update max exposure while meeting expected vblanking */ - exposure_max = (ov5675->cur_mode->height + ctrl->val - - OV5675_EXPOSURE_MAX_MARGIN) / 2; + exposure_max = ov5675->cur_mode->height + ctrl->val - + OV5675_EXPOSURE_MAX_MARGIN; __v4l2_ctrl_modify_range(ov5675->exposure, ov5675->exposure->minimum, exposure_max, ov5675->exposure->step, @@ -689,7 +689,13 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl) break; case V4L2_CID_EXPOSURE: - /* 3 least significant bits of expsoure are fractional part */ + /* 4 least significant bits of expsoure are fractional part + * val = val << 4 + * for ov5675, the unit of exposure is differnt from other + * OmniVision sensors, its exposure value is twice of the + * register value, the exposure should be divided by 2 before + * set register, e.g. val << 3. + */ ret = ov5675_write_reg(ov5675, OV5675_REG_EXPOSURE, OV5675_REG_VALUE_24BIT, ctrl->val << 3); break; @@ -770,8 +776,7 @@ static int ov5675_init_controls(struct ov5675 *ov5675) v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops, V4L2_CID_DIGITAL_GAIN, OV5675_DGTL_GAIN_MIN, OV5675_DGTL_GAIN_MAX, OV5675_DGTL_GAIN_STEP, OV5675_DGTL_GAIN_DEFAULT); - exposure_max = (ov5675->cur_mode->vts_def - - OV5675_EXPOSURE_MAX_MARGIN) / 2; + exposure_max = (ov5675->cur_mode->vts_def - OV5675_EXPOSURE_MAX_MARGIN); ov5675->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops, V4L2_CID_EXPOSURE, OV5675_EXPOSURE_MIN, exposure_max,
The unit of exposure value is different from other OmniVision sensors, driver will divide by 2 before set register, the exposure range exposed by v4l2 ctrl to user should be same as others, so the calculation for the maximum exposure value in current driver need be fixed. Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> --- drivers/media/i2c/ov5675.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)