Hi Hans,
Thank you for the review!
On 01/07/2013 02:38 PM, Hans Verkuil wrote:
+
+/* V4L2 private controls */
+
+/* Auto exposure frame reference area */
+#define V4L2_CID_EXPOSURE_REFERENCE_AREA (V4L2_CTRL_CLASS_CAMERA | 0x1001)
+/* Maximum gain value */
+#define V4L2_CID_GAIN_CEILING (V4L2_CTRL_CLASS_CAMERA | 0x1002)
Private controls should be added to uapi/linux/v4l2-controls.h. By having
all controls in the same header it is easy to ensure that there are no
duplicate IDs in use.
The name of the driver should be part of the control name, so something like:
V4L2_CID_OV9650_EXP_REFERENCE_AREA
V4L2_CID_OV9650_GAIN_CEILING
Ok, to avoid overlapping with couple of existing camera class private
controls
I have defined them as:
+/* OV965X image sensor driver private controls */
+
+/* Auto exposure frame reference area */
+#define V4L2_CID_OV965X_EXPOSURE_REF_AREA (V4L2_CTRL_CLASS_CAMERA
| 0x1010)
+/* Automatic gain algorithm's gain limit */
+#define V4L2_CID_OV965X_GAIN_CEILING (V4L2_CTRL_CLASS_CAMERA
| 0x1011)
I've checked the datasheets and the gain ceiling control is supported by
virtually every Omnivision sensor: OV2655, OV3640, OV5630, OV9650, OV9655,
OV7690, with even identical range 2x...128x.
The _OV965X prefix for the control doesn't seem right then. Should I make
it something (ugly) like V4L2_CID_OVXXXX_GAIN_CEILING ?
And should ranges be reserved for each driver ? Or maybe only per
manufacturer?
If I get it right, there is room for 0xffff - 0x1000 = 61439 private
controls
in each control class for all drivers.
According to the notes from the Kernel Summit 2012 Media Workshop
(http://lwn.net/Articles/514527):
"New controls should not overlap.
Having all driver-specific controls in a single header file would
probably be
overkill. We can instead reserve a range of CIDs for each driver, and
define
the range base CID only in a common header file.
Driver-specific CIDs themselves would be defined in driver-specific
headers."
Since there shouldn't generally be many private controls per driver it may
make more sense to have all put in v4l2-controls.h.
...
+static void ov965x_update_exposure_ctrl(struct ov965x *ov965x)
+{
+ struct v4l2_ctrl *ctrl = ov965x->ctrls.exposure;
+ unsigned long fint, trow;
+ int max;
+ u8 clkrc;
+
+ mutex_lock(&ov965x->lock);
+
+ if (WARN_ON(!ctrl || !ov965x->frame_size)) {
+ mutex_unlock(&ov965x->lock);
+ return;
+ }
+ clkrc = DEF_CLKRC + ov965x->fiv->clkrc_div;
+ /* Calculate internal clock frequency */
+ fint = ov965x->mclk_frequency * ((clkrc>> 7) + 1) /
+ ((2 * ((clkrc& 0x3f) + 1)));
+ /* and the row interval (in us). */
+ trow = (2 * 1520 * 1000000UL) / fint;
+ max = ov965x->frame_size->max_exp_lines * trow;
+ ov965x->exp_row_interval = trow;
+
+ mutex_unlock(&ov965x->lock);
+ v4l2_dbg(1, debug,&ov965x->sd, "clkrc: %#x, fi: %lu, tr: %lu, %d\n",
+ clkrc, fint, trow, max);
+
+ /* Update exposure time min/max to match current frame format. */
+ v4l2_ctrl_lock(ctrl);
+
+ ctrl->minimum = (trow + 100) / 100;
+ ctrl->maximum = (max - 100) / 100;
+ if (ctrl->cur.val> ctrl->maximum)
+ ctrl->cur.val = ctrl->maximum;
+ if (ctrl->cur.val< ctrl->minimum)
+ ctrl->cur.val = ctrl->minimum;
You can't do this like that. To do this correctly you need to create a new
function in v4l2-ctrl.c that allows you to change the control attributes
minimum, maximum, step and default_value.
That function can then call send_event() to tell external apps that these
attributes have changed. That also requires a new flag V4L2_EVENT_CTRL_CH_RANGE.
v4l2_ctrl_modify_range() would probably be a good name for such a function.
Hh, extra work, great! ;)
So I've created v4l2_ctrl_modify_range() function, with this old patch as
a reference: http://patchwork.linuxtv.org/patch/8654.
I'm going to add missing documentation and post it in few days.
+
+ v4l2_ctrl_unlock(ctrl);
+}
+
...
+static int ov965x_set_contrast(struct ov965x *ov965x, int value)
+{
+ /* TODO */
+ return -EINVAL;
+}
Perhaps this should just be removed?
OK, let me remove it. If I find more time I'll implement it as a separate
patch. I left it out for a moment since it requires quite a few register
values to be rewritten from the datasheet. Not sure if there is any sane
method to calculate those arrays dynamically in the driver.
+static int ov965x_set_gain(struct ov965x *ov965x, int auto_gain, bool init)
+{
+ struct i2c_client *client = ov965x->client;
+ struct ov965x_ctrls *ctrls =&ov965x->ctrls;
+ int ret = 0;
+ u8 reg;
+ /*
+ * For manual mode we need to disable AGC first, so
+ * gain value in REG_VREF, REG_GAIN is not overwritten.
+ */
+ if (ctrls->auto_gain->is_new || init) {
+ ret = ov965x_read(client, REG_COM8,®);
+ if (ret< 0)
+ return ret;
+ if (ctrls->auto_gain->val)
+ reg |= COM8_AGC;
+ else
+ reg&= ~COM8_AGC;
+ ret = ov965x_write(client, REG_COM8, reg);
+ if (ret< 0)
+ return ret;
+ }
+
+ if ((ctrls->gain->is_new || init)&& !auto_gain) {
+ unsigned int gain = ctrls->gain->val;
+ unsigned int rgain;
+ int m;
+ /*
+ * Convert gain control value to the sensor's gain
+ * registers (VREF[7:6], GAIN[7:0]) format.
+ */
+ for (m = 6; m>= 0; m--)
+ if (gain>= (1<< m) * 16)
+ break;
+ rgain = (gain - ((1<< m) * 16)) / (1<< m);
+ rgain |= (((1<< m) - 1)<< 4);
+
+ ret = ov965x_write(client, REG_GAIN, rgain& 0xff);
+ if (ret< 0)
+ return ret;
+ ret = ov965x_read(client, REG_VREF,®);
+ if (ret< 0)
+ return ret;
+ reg&= ~VREF_GAIN_MASK;
+ reg |= (((rgain>> 8)& 0x3)<< 6);
+ ret = ov965x_write(client, REG_VREF, reg);
+ if (ret< 0)
+ return ret;
+ /* Return updated control's value to userspace */
+ ctrls->gain->val = (1<< m) * (16 + (rgain& 0xf));
+ }
+
+ if (ctrls->gain_ceiling->is_new || init) {
+ u8 gc = ctrls->gain_ceiling->val;
+ ret = ov965x_read(client, REG_COM9,®);
+ if (!ret) {
+ reg&= ~COM9_GAIN_CEIL_MASK;
+ reg |= ((gc& 0x07)<< 4);
+ ret = ov965x_write(client, REG_COM9, reg);
+ }
+ }
+ if (auto_gain)
+ ctrls->gain->flags |= CTRL_FLAG_READ_ONLY_VOLATILE;
+ else
+ ctrls->gain->flags&= ~CTRL_FLAG_READ_ONLY_VOLATILE;
+
+ return ret;
+}
...
+static int ov965x_set_exposure(struct ov965x *ov965x, int exp, bool init)
+{
+ struct i2c_client *client = ov965x->client;
+ struct ov965x_ctrls *ctrls =&ov965x->ctrls;
+ bool auto_exposure = (exp == V4L2_EXPOSURE_AUTO);
+ int ret;
+ u8 reg;
+
+ if (ctrls->auto_exp->is_new || init) {
+ ret = ov965x_read(client, REG_COM8,®);
+ if (ret< 0)
+ return ret;
+ if (auto_exposure)
+ reg |= (COM8_AEC | COM8_AGC);
+ else
+ reg&= ~(COM8_AEC | COM8_AGC);
+ ret = ov965x_write(client, REG_COM8, reg);
+ if (ret< 0)
+ return ret;
+ }
+
+ if (!auto_exposure&& (ctrls->exposure->is_new || init)) {
+ unsigned int exposure = (ctrls->exposure->val * 100)
+ / ov965x->exp_row_interval;
+ /*
+ * Manual exposure value
+ * [b15:b0] - AECHM (b15:b10), AECH (b9:b2), COM1 (b1:b0)
+ */
+ ret = ov965x_write(client, REG_COM1, exposure& 0x3);
+ if (!ret)
+ ret = ov965x_write(client, REG_AECH,
+ (exposure>> 2)& 0xff);
+ if (!ret)
+ ret = ov965x_write(client, REG_AECHM,
+ (exposure>> 10)& 0x3f);
+ /* Update the value to minimize rounding errors */
+ ctrls->exposure->val = ((exposure * ov965x->exp_row_interval)
+ + 50) / 100;
+ if (ret< 0)
+ return ret;
+ }
+
+ if (ctrls->ae_frame_area->is_new || init) {
+ ret = ov965x_read(client, REG_COM11,®);
+ if (ret< 0)
+ return ret;
+ reg&= ~COM11_AEC_REF_MASK;
+ reg |= ((ctrls->ae_frame_area->val& 0x3)<< 3);
+ ret = ov965x_write(client, REG_COM11, reg);
+ if (ret< 0)
+ return ret;
+ }
+
+ if (auto_exposure)
+ ctrls->exposure->flags |= CTRL_FLAG_READ_ONLY_VOLATILE;
+ else
+ ctrls->exposure->flags&= ~CTRL_FLAG_READ_ONLY_VOLATILE;
+
+ v4l2_ctrl_activate(ov965x->ctrls.brightness, !exp);
+ return 0;
+}
...
+/*
+ * Configure sensor register to match default control values. We can't use
+ * v4l2_ctrl_handler_setup() here as s_ctrl() also takes ov965x->lock mutex.
I don't think it is a good idea to do it like this. It's much better to call
v4l2_ctrl_handler_setup directly unless there are really good reasons for not
doing that.
My main concern was the initial registers write sequence. I've based on some
reference settings which looked like there could be some write sequence
dependencies, e.g. disabling some algorithm, setting some magic
"reserved" I2C
registers and then enabling the algorithm. Anyway I've tested it with
v4l2_ctrl_handler_setup() and nothing seems to be broken after that
modification.
With regards to the locking: I think you might need two locks here: one that
protects struct ov965x and one that is used to serialize between calls from
a bridge driver and calls directly to the subdev file handle (although I am
not certain the latter lock is needed at all).
I think it is. Some parameters are distributed across multiple I2C
registers,
and multiple parameters sometimes share same register. If there is no lock,
I2C read/modify/write operations will just fail.
In this case I think ov965x->lock shouldn't be held, and v4l2_ctrl_handler_setup
should be called.
I would rather have just one lock. There is an additional one in the
control
framework that one must not be forgetting about, when analysing possible
locking issues.
I just moved v4l2_ctrl_handler_setup() call to the s_stream() op, and do
release ov965x->lock for the time of v4l2_ctrl_handler_setup() call. Not
perfect but still seems better to me, than introducing an additional lock.
+ * Also, explicit function calls allow to better specify the register write
+ * sequence.
+ */
+static int __ov965x_restore_controls(struct ov965x *ov965x)
+{
+ struct ov965x_ctrls *ctrls =&ov965x->ctrls;
+ int ret;
+
+ ret = ov965x_set_gain(ov965x, ctrls->auto_gain->val, true);
+ if (!ret)
+ ret = ov965x_set_brightness(ov965x, ctrls->brightness->val);
+ if (!ret)
+ ret = ov965x_set_exposure(ov965x, ctrls->auto_exp->val, true);
+ if (!ret)
+ ret = ov965x_set_sharpness(ov965x, ctrls->sharpness->val);
+ if (!ret)
+ ret = ov965x_set_saturation(ov965x, ctrls->saturation->val);
+ if (!ret)
+ ret = ov965x_set_white_balance(ov965x, ctrls->auto_wb->val);
+
+ return ret;
+}
+
+static int __g_volatile_ctrl(struct ov965x *ov965x, struct v4l2_ctrl *ctrl)
+{
+ struct i2c_client *client = ov965x->client;
+ unsigned int exposure, gain, m;
+ u8 reg0, reg1, reg2;
+ int ret;
+
+ if (!ov965x->power)
+ return -EAGAIN;
How about 'return 0;'? If the power is off, then it seems reasonable to
just return the last gain/exposure value. Without power the autogain and
autoexposure hardware is turned off as well, so gain and exposure aren't
updated.
That's a good idea actually, thanks! With this modification there is no
errors now, when querying controls with the sensor's power turned off.
Not sure how I've missed this simple and best option..
I mention this because EAGAIN suggests that you can just try it again a bit
later, but that won't help as long as the power isn't turned on.
It wasn't perfect, indeed.
+ switch (ctrl->id) {
+ case V4L2_CID_AUTOGAIN:
+ if (!ctrl->val)
+ return 0;
+ ret = ov965x_read(client, REG_GAIN,®0);
+ if (ret< 0)
+ return ret;
+ ret = ov965x_read(client, REG_VREF,®1);
+ if (ret< 0)
+ return ret;
+ gain = ((reg1>> 6)<< 8) | reg0;
+ m = 0x01<< fls(gain>> 4);
+ ov965x->ctrls.gain->val = m * (16 + (gain& 0xf));
+ break;
+
+ case V4L2_CID_EXPOSURE_AUTO:
+ if (ctrl->val == V4L2_EXPOSURE_MANUAL)
+ return 0;
+ ret = ov965x_read(client, REG_COM1,®0);
+ if (!ret)
+ ret = ov965x_read(client, REG_AECH,®1);
+ if (!ret)
+ ret = ov965x_read(client, REG_AECHM,®2);
+ if (ret< 0)
+ return ret;
+ exposure = ((reg2& 0x3f)<< 10) | (reg1<< 2) |
+ (reg0& 0x3);
+ ov965x->ctrls.exposure->val = ((exposure *
+ ov965x->exp_row_interval) + 50) / 100;
+ break;
+ }
+
+ return 0;
+}
+
...
+static int ov965x_initialize_controls(struct ov965x *ov965x)
+{
+ const struct v4l2_ctrl_ops *ops =&ov965x_ctrl_ops;
+ struct ov965x_ctrls *ctrls =&ov965x->ctrls;
+ struct v4l2_ctrl_handler *hdl =&ctrls->handler;
+ int ret;
+
+ ret = v4l2_ctrl_handler_init(hdl, 13);
+ if (ret< 0)
+ return ret;
+
+ /* Auto/manual white balance */
+ ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops,
+ V4L2_CID_AUTO_WHITE_BALANCE,
+ 0, 1, 1, 1);
+ ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BLUE_BALANCE,
+ 0, 0xff, 1, 0x80);
+ ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_RED_BALANCE,
+ 0, 0xff, 1, 0x80);
+ /* Auto/manual exposure */
+ ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
+ V4L2_CID_EXPOSURE_AUTO,
+ V4L2_EXPOSURE_MANUAL, 0, V4L2_EXPOSURE_AUTO);
+ /* Exposure time, in 100 us units. min/max is updated dynamically. */
+ ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
+ V4L2_CID_EXPOSURE_ABSOLUTE,
+ 2, 1500, 1, 500);
+ /* Auto exposure reference frame area */
+ ctrls->ae_frame_area = v4l2_ctrl_new_custom(hdl,
+ &ov965x_ctrls[1], NULL);
+ /* Auto/manual gain */
+ ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN,
+ 0, 1, 1, 1);
+ ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN,
+ 16, 64 * (16 + 15), 1, 64 * 16);
+ ctrls->gain_ceiling = v4l2_ctrl_new_custom(hdl,&ov965x_ctrls[0], NULL);
+
+ ctrls->saturation = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SATURATION,
+ -2, 2, 1, 0);
+ ctrls->brightness = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BRIGHTNESS,
+ -3, 3, 1, 0);
+ ctrls->contrast = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST,
+ -2, 2, 1, 0);
+ ctrls->sharpness = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SHARPNESS,
+ 0, 32, 1, 6);
+
+ ctrls->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
+ ctrls->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
+
+ ctrls->light_freq = v4l2_ctrl_new_std_menu(hdl, ops,
+ V4L2_CID_POWER_LINE_FREQUENCY,
+ V4L2_CID_POWER_LINE_FREQUENCY_60HZ, ~0x7,
+ V4L2_CID_POWER_LINE_FREQUENCY_50HZ);
+
+ v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
+ ARRAY_SIZE(test_pattern_menu) - 1, 0, 0,
+ test_pattern_menu);
+ if (hdl->error) {
+ ret = hdl->error;
+ v4l2_ctrl_handler_free(hdl);
+ return ret;
+ }
+
+ ctrls->gain->flags |= V4L2_CTRL_FLAG_VOLATILE;
+ ctrls->exposure->flags |= V4L2_CTRL_FLAG_VOLATILE;
+
+ v4l2_ctrl_auto_cluster(3,&ctrls->auto_wb, 0, false);
+ v4l2_ctrl_cluster(3,&ctrls->auto_exp);
+ v4l2_ctrl_cluster(2,&ctrls->hflip);
+ v4l2_ctrl_cluster(3,&ctrls->auto_gain);
Why don't you use auto_cluster for gain and exposure? It should simplify your
code quite a bit.
I tried, but it didn't work in these use cases.
Note there are 3 controls in each cluster, e.g. auto/manual gain,
manual_gain,
gain_ceiling (max auto gain limit). gain_ceiling is only valid for automatic
gain, and the manual_gain value of course only for manual gain mode. With
auto_cluster gain_ceiling would be deactivated when gain is set to auto
mode,
which is not what I want. The situation with auto exposure is analogous.
I also would expect to see a call to v4l2_ctrl_handler_setup() somewhere to
initialize the hardware. Not strictly necessary if the initial hardware state
is the same as that of the initial controls.
When the sensor is powered on there is a call to
__ov965x_restore_controls(),
which ensures the state of registers is matching the controls values. The
sensor is left in power off state by the probe() callback, so there is not
much sense in doing lengthy I2C communication there, just to lose the device
state when it is powered off.
Anyway I've reworked it to use v4l2_ctrl_handler_setup() and removed
__ov965x_restore_controls() function, which was a bit ugly and I didn't like
it either.
+
+ ov965x->sd.ctrl_handler = hdl;
+ return 0;
+}
+
...
+/*
+ * V4L2 subdev internal operations
+ */
+static int ov965x_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ struct v4l2_mbus_framefmt *mf = v4l2_subdev_get_try_format(fh, 0);
+
+ ov965x_get_default_format(mf);
+ v4l2_info(sd, "%s:%d\n", __func__, __LINE__);
Shouldn't v4l2_dbg be better? Or don't print anything. Normal usage of a driver
should produce output to the kernel log.
Sure, I suppose you meant "should not"? I missed to remove this trace.
It's dropped now.
+ return 0;
+}
+
...
diff --git a/include/media/ov9650.h b/include/media/ov9650.h
new file mode 100644
index 0000000..ba548a4
--- /dev/null
+++ b/include/media/ov9650.h
@@ -0,0 +1,20 @@
+/*
+ * OV9650/OV9652 camera sensors driver
+ *
+ * Copyright (C) 2012 Sylwester Nawrocki<sylvester.nawrocki@xxxxxxxxx>
2012 -> 2013 :-)
Thanks, updated. I must have left some imperfections for reviewers;)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef OV9650_H_
+#define OV9650_H_
+
+struct ov9650_platform_data {
+ unsigned long mclk_frequency;
+ int gpio_pwdn;
+ int gpio_reset;
Some comments for these fields would be welcome.
Added, thanks for pointing out.
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
