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@xxxxxxxxx>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>> 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
>>
>
