| Message ID | 20230627131830.54601-25-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov2680: Bugfixes + ACPI + selection(crop-tgt) API support | expand |
Hi Hans another drive-by question On Tue, Jun 27, 2023 at 03:18:25PM +0200, Hans de Goede wrote: > The exposure control's max effective value is VTS - 8, set the control > range to match this. Thas means that if/when a future commit makes VTS > configurable as a control itself the exposure range needs to be > updated dynamically to match the VTS value. > > The gain control goes from 0 - 1023, higher values wrap around to > the lowest gain setting. > > Also stop setting 0 as default for both controls this leads to > a totally black picture which is no good. This is esp. important > for tests of the sensor driver without (userspace driven) > auto exposure / gain. I see the driver uses V4L2_CID_GAIN. Is this intentional or should this be V4L2_CID_ANALOGUE_GAIN? As you're plumbing libcamera support in, this is the control libcamera expects to use to control analogue gain. > > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/i2c/ov2680.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > index 6591ce3b9244..e26a6487d421 100644 > --- a/drivers/media/i2c/ov2680.c > +++ b/drivers/media/i2c/ov2680.c > @@ -81,6 +81,9 @@ > /* If possible send 16 extra rows / lines to the ISP as padding */ > #define OV2680_END_MARGIN 16 > > +/* Max exposure time is VTS - 8 */ > +#define OV2680_INTEGRATION_TIME_MARGIN 8 > + > #define OV2680_DEFAULT_WIDTH 800 > #define OV2680_DEFAULT_HEIGHT 600 > > @@ -823,6 +826,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) > const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops; > struct ov2680_ctrls *ctrls = &sensor->ctrls; > struct v4l2_ctrl_handler *hdl = &ctrls->handler; > + int exp_max = OV2680_LINES_PER_FRAME - OV2680_INTEGRATION_TIME_MARGIN; > int ret = 0; > > v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_subdev_ops); > @@ -849,9 +853,10 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) > 0, 0, test_pattern_menu); > > ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, > - 0, 32767, 1, 0); > + 0, exp_max, 1, exp_max); > > - ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 2047, 1, 0); > + ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, > + 0, 1023, 1, 250); > > if (hdl->error) { > ret = hdl->error; > -- > 2.41.0 >
Hi Jacopo, On 6/27/23 17:16, Jacopo Mondi wrote: > Hi Hans > another drive-by question > > On Tue, Jun 27, 2023 at 03:18:25PM +0200, Hans de Goede wrote: >> The exposure control's max effective value is VTS - 8, set the control >> range to match this. Thas means that if/when a future commit makes VTS >> configurable as a control itself the exposure range needs to be >> updated dynamically to match the VTS value. >> >> The gain control goes from 0 - 1023, higher values wrap around to >> the lowest gain setting. >> >> Also stop setting 0 as default for both controls this leads to >> a totally black picture which is no good. This is esp. important >> for tests of the sensor driver without (userspace driven) >> auto exposure / gain. > > I see the driver uses V4L2_CID_GAIN. Is this intentional or should > this be V4L2_CID_ANALOGUE_GAIN? As you're plumbing libcamera support > in, this is the control libcamera expects to use to control analogue > gain. That is a good question. I've not changed this for worries about existing users. Although given the previous state of the existing code I wonder if there are any existing users? So what is the policy on this ? Also I still need to figure out what the actual range (as in amplification at lowest + highest setting) of the gain control is, because AFAIk libcamera wants to know this. Any hints on how to do this ? Also are there any docs on how a driver should implement V4L2_CID_ANALOGUE_GAIN wrt range? E.g. is the driver expected do to some conversion of values to make the control always set a specific amplification for a specific value? Regards, Hans > >> >> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/media/i2c/ov2680.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c >> index 6591ce3b9244..e26a6487d421 100644 >> --- a/drivers/media/i2c/ov2680.c >> +++ b/drivers/media/i2c/ov2680.c >> @@ -81,6 +81,9 @@ >> /* If possible send 16 extra rows / lines to the ISP as padding */ >> #define OV2680_END_MARGIN 16 >> >> +/* Max exposure time is VTS - 8 */ >> +#define OV2680_INTEGRATION_TIME_MARGIN 8 >> + >> #define OV2680_DEFAULT_WIDTH 800 >> #define OV2680_DEFAULT_HEIGHT 600 >> >> @@ -823,6 +826,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) >> const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops; >> struct ov2680_ctrls *ctrls = &sensor->ctrls; >> struct v4l2_ctrl_handler *hdl = &ctrls->handler; >> + int exp_max = OV2680_LINES_PER_FRAME - OV2680_INTEGRATION_TIME_MARGIN; >> int ret = 0; >> >> v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_subdev_ops); >> @@ -849,9 +853,10 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) >> 0, 0, test_pattern_menu); >> >> ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, >> - 0, 32767, 1, 0); >> + 0, exp_max, 1, exp_max); >> >> - ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 2047, 1, 0); >> + ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, >> + 0, 1023, 1, 250); >> >> if (hdl->error) { >> ret = hdl->error; >> -- >> 2.41.0 >> >
Hi Han On Tue, Jun 27, 2023 at 06:26:08PM +0200, Hans de Goede wrote: > Hi Jacopo, > > On 6/27/23 17:16, Jacopo Mondi wrote: > > Hi Hans > > another drive-by question > > > > On Tue, Jun 27, 2023 at 03:18:25PM +0200, Hans de Goede wrote: > >> The exposure control's max effective value is VTS - 8, set the control > >> range to match this. Thas means that if/when a future commit makes VTS > >> configurable as a control itself the exposure range needs to be > >> updated dynamically to match the VTS value. > >> > >> The gain control goes from 0 - 1023, higher values wrap around to > >> the lowest gain setting. > >> > >> Also stop setting 0 as default for both controls this leads to > >> a totally black picture which is no good. This is esp. important > >> for tests of the sensor driver without (userspace driven) > >> auto exposure / gain. > > > > I see the driver uses V4L2_CID_GAIN. Is this intentional or should > > this be V4L2_CID_ANALOGUE_GAIN? As you're plumbing libcamera support > > in, this is the control libcamera expects to use to control analogue > > gain. > > That is a good question. I've not changed this for worries about > existing users. Although given the previous state of the existing > code I wonder if there are any existing users? That's a fair concern > > So what is the policy on this ? > > Also I still need to figure out what the actual range > (as in amplification at lowest + highest setting) of the gain > control is, because AFAIk libcamera wants to know this. libcamera has helpers that translate the scalar gain values computed by the AEGC algorithms (or supplied by the application) to sensor's device specific gain codes (and the other way around) https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/libipa/camera_sensor_helper.cpp So it's fine if the driver exposes the device specific gain codes from a libcamera perspective. True, there are sensors drivers where the gain codes interval has been linearized and made continuous to make it easier for userspace to control it. I tried to dig it out an example, but so far memory is failing me... Does this answer your question or have I misinterpreted it ? When it comes to gain vs analogue gain, it should be clarified if OV2680_REG_GAIN_PK only controls the analogue gain amplification or also the digital gain part. I don't have a datasheet so I can't help much there.... Also see afa4805799c1 ("media: ov5640: Fix analogue gain control") as an example for a driver being ported from GAIN to ANALOGUE_GAIN (so far nobody complained, so I presume this doesn't count as a regression :) > > Any hints on how to do this ? Also are there any docs on > how a driver should implement V4L2_CID_ANALOGUE_GAIN wrt range? > > E.g. is the driver expected do to some conversion of values > to make the control always set a specific amplification for > a specific value? > > Regards, > > Hans > > > > > > >> > >> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> drivers/media/i2c/ov2680.c | 9 +++++++-- > >> 1 file changed, 7 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > >> index 6591ce3b9244..e26a6487d421 100644 > >> --- a/drivers/media/i2c/ov2680.c > >> +++ b/drivers/media/i2c/ov2680.c > >> @@ -81,6 +81,9 @@ > >> /* If possible send 16 extra rows / lines to the ISP as padding */ > >> #define OV2680_END_MARGIN 16 > >> > >> +/* Max exposure time is VTS - 8 */ > >> +#define OV2680_INTEGRATION_TIME_MARGIN 8 > >> + > >> #define OV2680_DEFAULT_WIDTH 800 > >> #define OV2680_DEFAULT_HEIGHT 600 > >> > >> @@ -823,6 +826,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) > >> const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops; > >> struct ov2680_ctrls *ctrls = &sensor->ctrls; > >> struct v4l2_ctrl_handler *hdl = &ctrls->handler; > >> + int exp_max = OV2680_LINES_PER_FRAME - OV2680_INTEGRATION_TIME_MARGIN; > >> int ret = 0; > >> > >> v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_subdev_ops); > >> @@ -849,9 +853,10 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) > >> 0, 0, test_pattern_menu); > >> > >> ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, > >> - 0, 32767, 1, 0); > >> + 0, exp_max, 1, exp_max); > >> > >> - ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 2047, 1, 0); > >> + ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, > >> + 0, 1023, 1, 250); > >> > >> if (hdl->error) { > >> ret = hdl->error; > >> -- > >> 2.41.0 > >> > > >
diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c index 6591ce3b9244..e26a6487d421 100644 --- a/drivers/media/i2c/ov2680.c +++ b/drivers/media/i2c/ov2680.c @@ -81,6 +81,9 @@ /* If possible send 16 extra rows / lines to the ISP as padding */ #define OV2680_END_MARGIN 16 +/* Max exposure time is VTS - 8 */ +#define OV2680_INTEGRATION_TIME_MARGIN 8 + #define OV2680_DEFAULT_WIDTH 800 #define OV2680_DEFAULT_HEIGHT 600 @@ -823,6 +826,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops; struct ov2680_ctrls *ctrls = &sensor->ctrls; struct v4l2_ctrl_handler *hdl = &ctrls->handler; + int exp_max = OV2680_LINES_PER_FRAME - OV2680_INTEGRATION_TIME_MARGIN; int ret = 0; v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_subdev_ops); @@ -849,9 +853,10 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) 0, 0, test_pattern_menu); ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, - 0, 32767, 1, 0); + 0, exp_max, 1, exp_max); - ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 2047, 1, 0); + ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, + 0, 1023, 1, 250); if (hdl->error) { ret = hdl->error;