| Message ID | alpine.DEB.2.02.1108171553540.17550@ipanema (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Bastian, Thanks for the patch. On Wednesday 17 August 2011 18:02:07 Bastian Hecht wrote: > The driver now supports automatic/manual gain, automatic/manual white > balance, automatic/manual exposure control, vertical flip, brightness > control, contrast control and saturation control. Additionally the > following effects are available now: rotating the hue in the colorspace, > gray scale image and solarize effect. Any chance to port soc-camera to the control framework ? :-) > Signed-off-by: Bastian Hecht <hechtb@gmail.com> > --- > > diff --git a/drivers/media/video/ov5642.c b/drivers/media/video/ov5642.c > index 1b40d90..069a720 100644 > --- a/drivers/media/video/ov5642.c > +++ b/drivers/media/video/ov5642.c > @@ -74,6 +74,34 @@ > #define REG_AVG_WINDOW_END_Y_HIGH 0x5686 > #define REG_AVG_WINDOW_END_Y_LOW 0x5687 > > +/* default values in native space */ > +#define DEFAULT_RBBALANCE 0x400 > +#define DEFAULT_CONTRAST 0x20 > +#define DEFAULT_SATURATION 0x40 > + > +#define MAX_EXP_NATIVE 0x01ffff > +#define MAX_GAIN_NATIVE 0x1f > +#define MAX_RBBALANCE_NATIVE 0x0fff > +#define MAX_EXP 0xffff > +#define MAX_GAIN 0xff > +#define MAX_RBBALANCE 0xff > +#define MAX_HUE_TRIG_NATIVE 0x80 > + > +#define OV5642_CONTROL_BLUE_SATURATION (V4L2_CID_PRIVATE_BASE + 0) > +#define OV5642_CONTROL_RED_SATURATION (V4L2_CID_PRIVATE_BASE + 1) > +#define OV5642_CONTROL_GRAY_SCALE (V4L2_CID_PRIVATE_BASE + 2) > +#define OV5642_CONTROL_SOLARIZE (V4L2_CID_PRIVATE_BASE + 3) If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. > + > +#define EXP_V4L2_TO_NATIVE(x) ((x) << 4) > +#define EXP_NATIVE_TO_V4L2(x) ((x) >> 4) > +#define GAIN_V4L2_TO_NATIVE(x) ((x) * MAX_GAIN_NATIVE / MAX_GAIN) > +#define GAIN_NATIVE_TO_V4L2(x) ((x) * MAX_GAIN / MAX_GAIN_NATIVE) > +#define RBBALANCE_V4L2_TO_NATIVE(x) ((x) * MAX_RBBALANCE_NATIVE / > MAX_RBBALANCE) +#define RBBALANCE_NATIVE_TO_V4L2(x) ((x) * MAX_RBBALANCE / > MAX_RBBALANCE_NATIVE) + > +/* flaw in the datasheet. we need some extra lines */ > +#define MANUAL_LONG_EXP_SAFETY_DISTANCE 20 > + > /* active pixel array size */ > #define OV5642_SENSOR_SIZE_X 2592 > #define OV5642_SENSOR_SIZE_Y 1944 [snip] > @@ -804,6 +1013,100 @@ static int ov5642_set_resolution(struct v4l2_subdev > *sd) return ret; > } > > +static int ov5642_restore_state(struct v4l2_subdev *sd) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + struct ov5642 *priv = to_ov5642(client); > + struct v4l2_control set_ctrl; > + int tmp_red_balance = priv->red_balance; > + int tmp_blue_balance = priv->blue_balance; > + int tmp_gain = priv->gain; > + int tmp_exp = priv->exposure; > + int ret; > + > + set_ctrl.id = V4L2_CID_AUTOGAIN; > + set_ctrl.value = priv->agc; > + ret = ov5642_s_ctrl(sd, &set_ctrl); What about writing to registers directly ? > + > + if (!priv->agc) { > + set_ctrl.id = V4L2_CID_GAIN; > + set_ctrl.value = tmp_gain; > + if (!ret) > + ret = ov5642_s_ctrl(sd, &set_ctrl); > + } > + > + set_ctrl.id = V4L2_CID_AUTO_WHITE_BALANCE; > + set_ctrl.value = priv->awb; > + if (!ret) > + ret = ov5642_s_ctrl(sd, &set_ctrl); > + > + if (!priv->awb) { > + set_ctrl.id = V4L2_CID_RED_BALANCE; > + set_ctrl.value = tmp_red_balance; > + if (!ret) > + ret = ov5642_s_ctrl(sd, &set_ctrl); > + > + set_ctrl.id = V4L2_CID_BLUE_BALANCE; > + set_ctrl.value = tmp_blue_balance; > + if (!ret) > + ret = ov5642_s_ctrl(sd, &set_ctrl); > + } > + > + set_ctrl.id = V4L2_CID_EXPOSURE_AUTO; > + set_ctrl.value = priv->aec; > + if (!ret) > + ret = ov5642_s_ctrl(sd, &set_ctrl); > + > + if (priv->aec == V4L2_EXPOSURE_MANUAL) { > + set_ctrl.id = V4L2_CID_EXPOSURE; > + set_ctrl.value = tmp_exp; > + if (!ret) > + ret = ov5642_s_ctrl(sd, &set_ctrl); > + } > + > + set_ctrl.id = V4L2_CID_VFLIP; > + set_ctrl.value = priv->vflip; > + if (!ret) > + ret = ov5642_s_ctrl(sd, &set_ctrl); > + > + set_ctrl.id = OV5642_CONTROL_GRAY_SCALE; > + set_ctrl.value = priv->grayscale; > + if (!ret) > + ret = ov5642_s_ctrl(sd, &set_ctrl); > + > + set_ctrl.id = OV5642_CONTROL_SOLARIZE; > + set_ctrl.value = priv->solarize; > + if (!ret) > + ret = ov5642_s_ctrl(sd, &set_ctrl); > + > + set_ctrl.id = OV5642_CONTROL_BLUE_SATURATION; > + set_ctrl.value = priv->blue_saturation; > + if (!ret) > + ret = ov5642_s_ctrl(sd, &set_ctrl); > + > + set_ctrl.id = OV5642_CONTROL_RED_SATURATION; > + set_ctrl.value = priv->red_saturation; > + if (!ret) > + ret = ov5642_s_ctrl(sd, &set_ctrl); > + > + set_ctrl.id = V4L2_CID_BRIGHTNESS; > + set_ctrl.value = priv->brightness; > + if (!ret) > + ret = ov5642_s_ctrl(sd, &set_ctrl); > + > + set_ctrl.id = V4L2_CID_CONTRAST; > + set_ctrl.value = priv->contrast; > + if (!ret) > + ret = ov5642_s_ctrl(sd, &set_ctrl); > + > + set_ctrl.id = V4L2_CID_HUE; > + set_ctrl.value = priv->hue; > + if (!ret) > + ret = ov5642_s_ctrl(sd, &set_ctrl); > + > + return ret; > +} > + > static int ov5642_try_fmt(struct v4l2_subdev *sd, struct > v4l2_mbus_framefmt *mf) { > const struct ov5642_datafmt *fmt = ov5642_find_datafmt(mf->code); > @@ -856,6 +1159,9 @@ static int ov5642_s_fmt(struct v4l2_subdev *sd, struct > v4l2_mbus_framefmt *mf) if (!ret) > ret = ov5642_write_array(client, ov5642_default_regs_finalise); > > + /* the chip has been reset, so configure it again */ > + if (!ret) > + ret = ov5642_restore_state(sd); I suppose there's no way to avoid resetting the chip ? > return ret; > } > [snip] > +static int ov5642_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control > *ctrl) +{ > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + struct ov5642 *priv = to_ov5642(client); > + int ret = 0; > + u8 val8; > + u16 val16; > + u32 val32; > + int trig; > + struct v4l2_control aux_ctrl; > + > + switch (ctrl->id) { > + case V4L2_CID_AUTOGAIN: > + if (!ctrl->value) { > + aux_ctrl.id = V4L2_CID_GAIN; > + ret = ov5642_g_ctrl(sd, &aux_ctrl); > + if (ret) > + break; > + priv->gain = aux_ctrl.value; > + } > + > + ret = reg_read(client, REG_EXP_GAIN_CTRL, &val8); > + if (ret) > + break; > + val8 = ctrl->value ? val8 & ~BIT(1) : val8 | BIT(1); > + ret = reg_write(client, REG_EXP_GAIN_CTRL, val8); > + if (!ret) > + priv->agc = ctrl->value; What about caching the content of this register (and of other registers below) instead of reading it back ? If you can't do that, a reg_clr_set() function would make the code simpler. > + break;
2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > Hi Bastian, > > Thanks for the patch. > > On Wednesday 17 August 2011 18:02:07 Bastian Hecht wrote: >> The driver now supports automatic/manual gain, automatic/manual white >> balance, automatic/manual exposure control, vertical flip, brightness >> control, contrast control and saturation control. Additionally the >> following effects are available now: rotating the hue in the colorspace, >> gray scale image and solarize effect. > > Any chance to port soc-camera to the control framework ? :-) I redirect that to Guennadi :-) >> Signed-off-by: Bastian Hecht <hechtb@gmail.com> >> --- >> >> diff --git a/drivers/media/video/ov5642.c b/drivers/media/video/ov5642.c >> index 1b40d90..069a720 100644 >> --- a/drivers/media/video/ov5642.c >> +++ b/drivers/media/video/ov5642.c >> @@ -74,6 +74,34 @@ >> #define REG_AVG_WINDOW_END_Y_HIGH 0x5686 >> #define REG_AVG_WINDOW_END_Y_LOW 0x5687 >> >> +/* default values in native space */ >> +#define DEFAULT_RBBALANCE 0x400 >> +#define DEFAULT_CONTRAST 0x20 >> +#define DEFAULT_SATURATION 0x40 >> + >> +#define MAX_EXP_NATIVE 0x01ffff >> +#define MAX_GAIN_NATIVE 0x1f >> +#define MAX_RBBALANCE_NATIVE 0x0fff >> +#define MAX_EXP 0xffff >> +#define MAX_GAIN 0xff >> +#define MAX_RBBALANCE 0xff >> +#define MAX_HUE_TRIG_NATIVE 0x80 >> + >> +#define OV5642_CONTROL_BLUE_SATURATION (V4L2_CID_PRIVATE_BASE + 0) >> +#define OV5642_CONTROL_RED_SATURATION (V4L2_CID_PRIVATE_BASE + 1) >> +#define OV5642_CONTROL_GRAY_SCALE (V4L2_CID_PRIVATE_BASE + 2) >> +#define OV5642_CONTROL_SOLARIZE (V4L2_CID_PRIVATE_BASE + 3) > > If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled "V4L2_CID_PRIVATE_BASE deprecated" and read Documentation/feature-removal-schedule.txt. I couldn't find anything. If it is deprecated, do you have an idea how to offer device specific features to the user? >> + >> +#define EXP_V4L2_TO_NATIVE(x) ((x) << 4) >> +#define EXP_NATIVE_TO_V4L2(x) ((x) >> 4) >> +#define GAIN_V4L2_TO_NATIVE(x) ((x) * MAX_GAIN_NATIVE / MAX_GAIN) >> +#define GAIN_NATIVE_TO_V4L2(x) ((x) * MAX_GAIN / MAX_GAIN_NATIVE) >> +#define RBBALANCE_V4L2_TO_NATIVE(x) ((x) * MAX_RBBALANCE_NATIVE / >> MAX_RBBALANCE) +#define RBBALANCE_NATIVE_TO_V4L2(x) ((x) * MAX_RBBALANCE / >> MAX_RBBALANCE_NATIVE) + >> +/* flaw in the datasheet. we need some extra lines */ >> +#define MANUAL_LONG_EXP_SAFETY_DISTANCE 20 >> + >> /* active pixel array size */ >> #define OV5642_SENSOR_SIZE_X 2592 >> #define OV5642_SENSOR_SIZE_Y 1944 > > [snip] > >> @@ -804,6 +1013,100 @@ static int ov5642_set_resolution(struct v4l2_subdev >> *sd) return ret; >> } >> >> +static int ov5642_restore_state(struct v4l2_subdev *sd) >> +{ >> + struct i2c_client *client = v4l2_get_subdevdata(sd); >> + struct ov5642 *priv = to_ov5642(client); >> + struct v4l2_control set_ctrl; >> + int tmp_red_balance = priv->red_balance; >> + int tmp_blue_balance = priv->blue_balance; >> + int tmp_gain = priv->gain; >> + int tmp_exp = priv->exposure; >> + int ret; >> + >> + set_ctrl.id = V4L2_CID_AUTOGAIN; >> + set_ctrl.value = priv->agc; >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > > What about writing to registers directly ? I thought about that too, but code duplication would be very large (e.g. take the hue control). I considered the speedup of avoiding function calls less than the error-proness of this duplication. It is just cleaner imho. >> + >> + if (!priv->agc) { >> + set_ctrl.id = V4L2_CID_GAIN; >> + set_ctrl.value = tmp_gain; >> + if (!ret) >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> + } >> + >> + set_ctrl.id = V4L2_CID_AUTO_WHITE_BALANCE; >> + set_ctrl.value = priv->awb; >> + if (!ret) >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> + >> + if (!priv->awb) { >> + set_ctrl.id = V4L2_CID_RED_BALANCE; >> + set_ctrl.value = tmp_red_balance; >> + if (!ret) >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> + >> + set_ctrl.id = V4L2_CID_BLUE_BALANCE; >> + set_ctrl.value = tmp_blue_balance; >> + if (!ret) >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> + } >> + >> + set_ctrl.id = V4L2_CID_EXPOSURE_AUTO; >> + set_ctrl.value = priv->aec; >> + if (!ret) >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> + >> + if (priv->aec == V4L2_EXPOSURE_MANUAL) { >> + set_ctrl.id = V4L2_CID_EXPOSURE; >> + set_ctrl.value = tmp_exp; >> + if (!ret) >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> + } >> + >> + set_ctrl.id = V4L2_CID_VFLIP; >> + set_ctrl.value = priv->vflip; >> + if (!ret) >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> + >> + set_ctrl.id = OV5642_CONTROL_GRAY_SCALE; >> + set_ctrl.value = priv->grayscale; >> + if (!ret) >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> + >> + set_ctrl.id = OV5642_CONTROL_SOLARIZE; >> + set_ctrl.value = priv->solarize; >> + if (!ret) >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> + >> + set_ctrl.id = OV5642_CONTROL_BLUE_SATURATION; >> + set_ctrl.value = priv->blue_saturation; >> + if (!ret) >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> + >> + set_ctrl.id = OV5642_CONTROL_RED_SATURATION; >> + set_ctrl.value = priv->red_saturation; >> + if (!ret) >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> + >> + set_ctrl.id = V4L2_CID_BRIGHTNESS; >> + set_ctrl.value = priv->brightness; >> + if (!ret) >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> + >> + set_ctrl.id = V4L2_CID_CONTRAST; >> + set_ctrl.value = priv->contrast; >> + if (!ret) >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> + >> + set_ctrl.id = V4L2_CID_HUE; >> + set_ctrl.value = priv->hue; >> + if (!ret) >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> + >> + return ret; >> +} >> + >> static int ov5642_try_fmt(struct v4l2_subdev *sd, struct >> v4l2_mbus_framefmt *mf) { >> const struct ov5642_datafmt *fmt = ov5642_find_datafmt(mf->code); >> @@ -856,6 +1159,9 @@ static int ov5642_s_fmt(struct v4l2_subdev *sd, struct >> v4l2_mbus_framefmt *mf) if (!ret) >> ret = ov5642_write_array(client, ov5642_default_regs_finalise); >> >> + /* the chip has been reset, so configure it again */ >> + if (!ret) >> + ret = ov5642_restore_state(sd); > > I suppose there's no way to avoid resetting the chip ? Whenever the clock is down, the chip looses its state. >> return ret; >> } >> > > [snip] > >> +static int ov5642_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control >> *ctrl) +{ >> + struct i2c_client *client = v4l2_get_subdevdata(sd); >> + struct ov5642 *priv = to_ov5642(client); >> + int ret = 0; >> + u8 val8; >> + u16 val16; >> + u32 val32; >> + int trig; >> + struct v4l2_control aux_ctrl; >> + >> + switch (ctrl->id) { >> + case V4L2_CID_AUTOGAIN: >> + if (!ctrl->value) { >> + aux_ctrl.id = V4L2_CID_GAIN; >> + ret = ov5642_g_ctrl(sd, &aux_ctrl); >> + if (ret) >> + break; >> + priv->gain = aux_ctrl.value; >> + } >> + >> + ret = reg_read(client, REG_EXP_GAIN_CTRL, &val8); >> + if (ret) >> + break; >> + val8 = ctrl->value ? val8 & ~BIT(1) : val8 | BIT(1); >> + ret = reg_write(client, REG_EXP_GAIN_CTRL, val8); >> + if (!ret) >> + priv->agc = ctrl->value; > > What about caching the content of this register (and of other registers below) > instead of reading it back ? If you can't do that, a reg_clr_set() function > would make the code simpler. Ok I will do the caching. >> + break; > > -- > Regards, > > Laurent Pinchart > Thanks again, Bastian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bastian, On Wednesday 31 August 2011 17:27:49 Bastian Hecht wrote: > 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > > On Wednesday 17 August 2011 18:02:07 Bastian Hecht wrote: > >> The driver now supports automatic/manual gain, automatic/manual white > >> balance, automatic/manual exposure control, vertical flip, brightness > >> control, contrast control and saturation control. Additionally the > >> following effects are available now: rotating the hue in the colorspace, > >> gray scale image and solarize effect. > > > > Any chance to port soc-camera to the control framework ? :-) > > I redirect that to Guennadi :-) Guennadi, do you have any update on this ? Hans posted patches early this year, do you know if there has been any work on them since then ? > >> Signed-off-by: Bastian Hecht <hechtb@gmail.com> > >> --- > >> > >> diff --git a/drivers/media/video/ov5642.c b/drivers/media/video/ov5642.c > >> index 1b40d90..069a720 100644 > >> --- a/drivers/media/video/ov5642.c > >> +++ b/drivers/media/video/ov5642.c > >> @@ -74,6 +74,34 @@ > >> #define REG_AVG_WINDOW_END_Y_HIGH 0x5686 > >> #define REG_AVG_WINDOW_END_Y_LOW 0x5687 > >> > >> +/* default values in native space */ > >> +#define DEFAULT_RBBALANCE 0x400 > >> +#define DEFAULT_CONTRAST 0x20 > >> +#define DEFAULT_SATURATION 0x40 > >> + > >> +#define MAX_EXP_NATIVE 0x01ffff > >> +#define MAX_GAIN_NATIVE 0x1f > >> +#define MAX_RBBALANCE_NATIVE 0x0fff > >> +#define MAX_EXP 0xffff > >> +#define MAX_GAIN 0xff > >> +#define MAX_RBBALANCE 0xff > >> +#define MAX_HUE_TRIG_NATIVE 0x80 > >> + > >> +#define OV5642_CONTROL_BLUE_SATURATION (V4L2_CID_PRIVATE_BASE + > >> 0) +#define OV5642_CONTROL_RED_SATURATION (V4L2_CID_PRIVATE_BASE > >> + 1) +#define OV5642_CONTROL_GRAY_SCALE (V4L2_CID_PRIVATE_BASE + 2) > >> +#define OV5642_CONTROL_SOLARIZE (V4L2_CID_PRIVATE_BASE + > >> 3) > > > > If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. > > I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled > "V4L2_CID_PRIVATE_BASE deprecated" and read > Documentation/feature-removal-schedule.txt. I couldn't find anything. > If it is deprecated, do you have an idea how to offer device specific > features to the user? The basic idea is that controls should be standardized, or at least documented and added to the V4L2 spec. Controls should belong to a class, so you should select the proper base class and add a big offset (I've used 0x1000) in the meantime if you want to export private controls. > >> + > >> +#define EXP_V4L2_TO_NATIVE(x) ((x) << 4) > >> +#define EXP_NATIVE_TO_V4L2(x) ((x) >> 4) > >> +#define GAIN_V4L2_TO_NATIVE(x) ((x) * MAX_GAIN_NATIVE / MAX_GAIN) > >> +#define GAIN_NATIVE_TO_V4L2(x) ((x) * MAX_GAIN / MAX_GAIN_NATIVE) > >> +#define RBBALANCE_V4L2_TO_NATIVE(x) ((x) * MAX_RBBALANCE_NATIVE / > >> MAX_RBBALANCE) +#define RBBALANCE_NATIVE_TO_V4L2(x) ((x) * MAX_RBBALANCE > >> / MAX_RBBALANCE_NATIVE) + > >> +/* flaw in the datasheet. we need some extra lines */ > >> +#define MANUAL_LONG_EXP_SAFETY_DISTANCE 20 > >> + > >> /* active pixel array size */ > >> #define OV5642_SENSOR_SIZE_X 2592 > >> #define OV5642_SENSOR_SIZE_Y 1944 > > > > [snip] > > > >> @@ -804,6 +1013,100 @@ static int ov5642_set_resolution(struct > >> v4l2_subdev *sd) return ret; > >> } > >> > >> +static int ov5642_restore_state(struct v4l2_subdev *sd) > >> +{ > >> + struct i2c_client *client = v4l2_get_subdevdata(sd); > >> + struct ov5642 *priv = to_ov5642(client); > >> + struct v4l2_control set_ctrl; > >> + int tmp_red_balance = priv->red_balance; > >> + int tmp_blue_balance = priv->blue_balance; > >> + int tmp_gain = priv->gain; > >> + int tmp_exp = priv->exposure; > >> + int ret; > >> + > >> + set_ctrl.id = V4L2_CID_AUTOGAIN; > >> + set_ctrl.value = priv->agc; > >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > > > > What about writing to registers directly ? > > I thought about that too, but code duplication would be very large > (e.g. take the hue control). I considered the speedup of avoiding > function calls less than the error-proness of this duplication. It is > just cleaner imho. OK. This will be replaced when soc-camera will use the control framework anyway. > >> + > >> + if (!priv->agc) { > >> + set_ctrl.id = V4L2_CID_GAIN; > >> + set_ctrl.value = tmp_gain; > >> + if (!ret) > >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > >> + } > >> + > >> + set_ctrl.id = V4L2_CID_AUTO_WHITE_BALANCE; > >> + set_ctrl.value = priv->awb; > >> + if (!ret) > >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > >> + > >> + if (!priv->awb) { > >> + set_ctrl.id = V4L2_CID_RED_BALANCE; > >> + set_ctrl.value = tmp_red_balance; > >> + if (!ret) > >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > >> + > >> + set_ctrl.id = V4L2_CID_BLUE_BALANCE; > >> + set_ctrl.value = tmp_blue_balance; > >> + if (!ret) > >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > >> + } > >> + > >> + set_ctrl.id = V4L2_CID_EXPOSURE_AUTO; > >> + set_ctrl.value = priv->aec; > >> + if (!ret) > >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > >> + > >> + if (priv->aec == V4L2_EXPOSURE_MANUAL) { > >> + set_ctrl.id = V4L2_CID_EXPOSURE; > >> + set_ctrl.value = tmp_exp; > >> + if (!ret) > >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > >> + } > >> + > >> + set_ctrl.id = V4L2_CID_VFLIP; > >> + set_ctrl.value = priv->vflip; > >> + if (!ret) > >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > >> + > >> + set_ctrl.id = OV5642_CONTROL_GRAY_SCALE; > >> + set_ctrl.value = priv->grayscale; > >> + if (!ret) > >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > >> + > >> + set_ctrl.id = OV5642_CONTROL_SOLARIZE; > >> + set_ctrl.value = priv->solarize; > >> + if (!ret) > >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > >> + > >> + set_ctrl.id = OV5642_CONTROL_BLUE_SATURATION; > >> + set_ctrl.value = priv->blue_saturation; > >> + if (!ret) > >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > >> + > >> + set_ctrl.id = OV5642_CONTROL_RED_SATURATION; > >> + set_ctrl.value = priv->red_saturation; > >> + if (!ret) > >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > >> + > >> + set_ctrl.id = V4L2_CID_BRIGHTNESS; > >> + set_ctrl.value = priv->brightness; > >> + if (!ret) > >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > >> + > >> + set_ctrl.id = V4L2_CID_CONTRAST; > >> + set_ctrl.value = priv->contrast; > >> + if (!ret) > >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > >> + > >> + set_ctrl.id = V4L2_CID_HUE; > >> + set_ctrl.value = priv->hue; > >> + if (!ret) > >> + ret = ov5642_s_ctrl(sd, &set_ctrl); > >> + > >> + return ret; > >> +} > >> + > >> static int ov5642_try_fmt(struct v4l2_subdev *sd, struct > >> v4l2_mbus_framefmt *mf) { > >> const struct ov5642_datafmt *fmt = > >> ov5642_find_datafmt(mf->code); @@ -856,6 +1159,9 @@ static int > >> ov5642_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) if > >> (!ret) > >> ret = ov5642_write_array(client, > >> ov5642_default_regs_finalise); > >> > >> + /* the chip has been reset, so configure it again */ > >> + if (!ret) > >> + ret = ov5642_restore_state(sd); > > > > I suppose there's no way to avoid resetting the chip ? > > Whenever the clock is down, the chip looses its state. But the clock isn't turned down at every s_fmt call. Would it be possible reinit the chip in the .s_power operation instead ? > >> return ret; > >> } > > > > [snip] > > > >> +static int ov5642_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control > >> *ctrl) +{ > >> + struct i2c_client *client = v4l2_get_subdevdata(sd); > >> + struct ov5642 *priv = to_ov5642(client); > >> + int ret = 0; > >> + u8 val8; > >> + u16 val16; > >> + u32 val32; > >> + int trig; > >> + struct v4l2_control aux_ctrl; > >> + > >> + switch (ctrl->id) { > >> + case V4L2_CID_AUTOGAIN: > >> + if (!ctrl->value) { > >> + aux_ctrl.id = V4L2_CID_GAIN; > >> + ret = ov5642_g_ctrl(sd, &aux_ctrl); > >> + if (ret) > >> + break; > >> + priv->gain = aux_ctrl.value; > >> + } > >> + > >> + ret = reg_read(client, REG_EXP_GAIN_CTRL, &val8); > >> + if (ret) > >> + break; > >> + val8 = ctrl->value ? val8 & ~BIT(1) : val8 | BIT(1); > >> + ret = reg_write(client, REG_EXP_GAIN_CTRL, val8); > >> + if (!ret) > >> + priv->agc = ctrl->value; > > > > What about caching the content of this register (and of other registers > > below) instead of reading it back ? If you can't do that, a > > reg_clr_set() function would make the code simpler. > > Ok I will do the caching. > > >> + break;
On Wed, 31 Aug 2011, Bastian Hecht wrote: > 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > > Hi Bastian, > > > > Thanks for the patch. > > > > On Wednesday 17 August 2011 18:02:07 Bastian Hecht wrote: > >> The driver now supports automatic/manual gain, automatic/manual white > >> balance, automatic/manual exposure control, vertical flip, brightness > >> control, contrast control and saturation control. Additionally the > >> following effects are available now: rotating the hue in the colorspace, > >> gray scale image and solarize effect. > > > > Any chance to port soc-camera to the control framework ? :-) > > I redirect that to Guennadi :-) Hans is prepaing an update of his port, then we'll integrate it... This all will take time, so, it's better to do this driver now the "old soc-camera" way, and port it later. [snip] > >> +static int ov5642_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control > >> *ctrl) +{ > >> + struct i2c_client *client = v4l2_get_subdevdata(sd); > >> + struct ov5642 *priv = to_ov5642(client); > >> + int ret = 0; > >> + u8 val8; > >> + u16 val16; > >> + u32 val32; > >> + int trig; > >> + struct v4l2_control aux_ctrl; > >> + > >> + switch (ctrl->id) { > >> + case V4L2_CID_AUTOGAIN: > >> + if (!ctrl->value) { > >> + aux_ctrl.id = V4L2_CID_GAIN; > >> + ret = ov5642_g_ctrl(sd, &aux_ctrl); > >> + if (ret) > >> + break; > >> + priv->gain = aux_ctrl.value; > >> + } > >> + > >> + ret = reg_read(client, REG_EXP_GAIN_CTRL, &val8); > >> + if (ret) > >> + break; > >> + val8 = ctrl->value ? val8 & ~BIT(1) : val8 | BIT(1); > >> + ret = reg_write(client, REG_EXP_GAIN_CTRL, val8); > >> + if (!ret) > >> + priv->agc = ctrl->value; > > > > What about caching the content of this register (and of other registers below) > > instead of reading it back ? If you can't do that, a reg_clr_set() function > > would make the code simpler. > > Ok I will do the caching. Wasn't the reason for reading those registers from the hardware, that the sensor changes them in auto* modes (autogain in this case)? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2011/8/31 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > Hi Bastian, > > On Wednesday 31 August 2011 17:27:49 Bastian Hecht wrote: >> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: >> > On Wednesday 17 August 2011 18:02:07 Bastian Hecht wrote: >> >> The driver now supports automatic/manual gain, automatic/manual white >> >> balance, automatic/manual exposure control, vertical flip, brightness >> >> control, contrast control and saturation control. Additionally the >> >> following effects are available now: rotating the hue in the colorspace, >> >> gray scale image and solarize effect. >> > >> > Any chance to port soc-camera to the control framework ? :-) >> >> I redirect that to Guennadi :-) > > Guennadi, do you have any update on this ? Hans posted patches early this > year, do you know if there has been any work on them since then ? > >> >> Signed-off-by: Bastian Hecht <hechtb@gmail.com> >> >> --- >> >> >> >> diff --git a/drivers/media/video/ov5642.c b/drivers/media/video/ov5642.c >> >> index 1b40d90..069a720 100644 >> >> --- a/drivers/media/video/ov5642.c >> >> +++ b/drivers/media/video/ov5642.c >> >> @@ -74,6 +74,34 @@ >> >> #define REG_AVG_WINDOW_END_Y_HIGH 0x5686 >> >> #define REG_AVG_WINDOW_END_Y_LOW 0x5687 >> >> >> >> +/* default values in native space */ >> >> +#define DEFAULT_RBBALANCE 0x400 >> >> +#define DEFAULT_CONTRAST 0x20 >> >> +#define DEFAULT_SATURATION 0x40 >> >> + >> >> +#define MAX_EXP_NATIVE 0x01ffff >> >> +#define MAX_GAIN_NATIVE 0x1f >> >> +#define MAX_RBBALANCE_NATIVE 0x0fff >> >> +#define MAX_EXP 0xffff >> >> +#define MAX_GAIN 0xff >> >> +#define MAX_RBBALANCE 0xff >> >> +#define MAX_HUE_TRIG_NATIVE 0x80 >> >> + >> >> +#define OV5642_CONTROL_BLUE_SATURATION (V4L2_CID_PRIVATE_BASE + >> >> 0) +#define OV5642_CONTROL_RED_SATURATION (V4L2_CID_PRIVATE_BASE >> >> + 1) +#define OV5642_CONTROL_GRAY_SCALE (V4L2_CID_PRIVATE_BASE + 2) >> >> +#define OV5642_CONTROL_SOLARIZE (V4L2_CID_PRIVATE_BASE + >> >> 3) >> > >> > If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. >> >> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled >> "V4L2_CID_PRIVATE_BASE deprecated" and read >> Documentation/feature-removal-schedule.txt. I couldn't find anything. >> If it is deprecated, do you have an idea how to offer device specific >> features to the user? > > The basic idea is that controls should be standardized, or at least documented > and added to the V4L2 spec. Controls should belong to a class, so you should > select the proper base class and add a big offset (I've used 0x1000) in the > meantime if you want to export private controls. Is this code accessable? Then I can just copy the scheme. >> >> + >> >> +#define EXP_V4L2_TO_NATIVE(x) ((x) << 4) >> >> +#define EXP_NATIVE_TO_V4L2(x) ((x) >> 4) >> >> +#define GAIN_V4L2_TO_NATIVE(x) ((x) * MAX_GAIN_NATIVE / MAX_GAIN) >> >> +#define GAIN_NATIVE_TO_V4L2(x) ((x) * MAX_GAIN / MAX_GAIN_NATIVE) >> >> +#define RBBALANCE_V4L2_TO_NATIVE(x) ((x) * MAX_RBBALANCE_NATIVE / >> >> MAX_RBBALANCE) +#define RBBALANCE_NATIVE_TO_V4L2(x) ((x) * MAX_RBBALANCE >> >> / MAX_RBBALANCE_NATIVE) + >> >> +/* flaw in the datasheet. we need some extra lines */ >> >> +#define MANUAL_LONG_EXP_SAFETY_DISTANCE 20 >> >> + >> >> /* active pixel array size */ >> >> #define OV5642_SENSOR_SIZE_X 2592 >> >> #define OV5642_SENSOR_SIZE_Y 1944 >> > >> > [snip] >> > >> >> @@ -804,6 +1013,100 @@ static int ov5642_set_resolution(struct >> >> v4l2_subdev *sd) return ret; >> >> } >> >> >> >> +static int ov5642_restore_state(struct v4l2_subdev *sd) >> >> +{ >> >> + struct i2c_client *client = v4l2_get_subdevdata(sd); >> >> + struct ov5642 *priv = to_ov5642(client); >> >> + struct v4l2_control set_ctrl; >> >> + int tmp_red_balance = priv->red_balance; >> >> + int tmp_blue_balance = priv->blue_balance; >> >> + int tmp_gain = priv->gain; >> >> + int tmp_exp = priv->exposure; >> >> + int ret; >> >> + >> >> + set_ctrl.id = V4L2_CID_AUTOGAIN; >> >> + set_ctrl.value = priv->agc; >> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> > >> > What about writing to registers directly ? >> >> I thought about that too, but code duplication would be very large >> (e.g. take the hue control). I considered the speedup of avoiding >> function calls less than the error-proness of this duplication. It is >> just cleaner imho. > > OK. This will be replaced when soc-camera will use the control framework > anyway. > >> >> + >> >> + if (!priv->agc) { >> >> + set_ctrl.id = V4L2_CID_GAIN; >> >> + set_ctrl.value = tmp_gain; >> >> + if (!ret) >> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> >> + } >> >> + >> >> + set_ctrl.id = V4L2_CID_AUTO_WHITE_BALANCE; >> >> + set_ctrl.value = priv->awb; >> >> + if (!ret) >> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> >> + >> >> + if (!priv->awb) { >> >> + set_ctrl.id = V4L2_CID_RED_BALANCE; >> >> + set_ctrl.value = tmp_red_balance; >> >> + if (!ret) >> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> >> + >> >> + set_ctrl.id = V4L2_CID_BLUE_BALANCE; >> >> + set_ctrl.value = tmp_blue_balance; >> >> + if (!ret) >> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> >> + } >> >> + >> >> + set_ctrl.id = V4L2_CID_EXPOSURE_AUTO; >> >> + set_ctrl.value = priv->aec; >> >> + if (!ret) >> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> >> + >> >> + if (priv->aec == V4L2_EXPOSURE_MANUAL) { >> >> + set_ctrl.id = V4L2_CID_EXPOSURE; >> >> + set_ctrl.value = tmp_exp; >> >> + if (!ret) >> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> >> + } >> >> + >> >> + set_ctrl.id = V4L2_CID_VFLIP; >> >> + set_ctrl.value = priv->vflip; >> >> + if (!ret) >> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> >> + >> >> + set_ctrl.id = OV5642_CONTROL_GRAY_SCALE; >> >> + set_ctrl.value = priv->grayscale; >> >> + if (!ret) >> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> >> + >> >> + set_ctrl.id = OV5642_CONTROL_SOLARIZE; >> >> + set_ctrl.value = priv->solarize; >> >> + if (!ret) >> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> >> + >> >> + set_ctrl.id = OV5642_CONTROL_BLUE_SATURATION; >> >> + set_ctrl.value = priv->blue_saturation; >> >> + if (!ret) >> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> >> + >> >> + set_ctrl.id = OV5642_CONTROL_RED_SATURATION; >> >> + set_ctrl.value = priv->red_saturation; >> >> + if (!ret) >> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> >> + >> >> + set_ctrl.id = V4L2_CID_BRIGHTNESS; >> >> + set_ctrl.value = priv->brightness; >> >> + if (!ret) >> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> >> + >> >> + set_ctrl.id = V4L2_CID_CONTRAST; >> >> + set_ctrl.value = priv->contrast; >> >> + if (!ret) >> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> >> + >> >> + set_ctrl.id = V4L2_CID_HUE; >> >> + set_ctrl.value = priv->hue; >> >> + if (!ret) >> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >> >> + >> >> + return ret; >> >> +} >> >> + >> >> static int ov5642_try_fmt(struct v4l2_subdev *sd, struct >> >> v4l2_mbus_framefmt *mf) { >> >> const struct ov5642_datafmt *fmt = >> >> ov5642_find_datafmt(mf->code); @@ -856,6 +1159,9 @@ static int >> >> ov5642_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) if >> >> (!ret) >> >> ret = ov5642_write_array(client, >> >> ov5642_default_regs_finalise); >> >> >> >> + /* the chip has been reset, so configure it again */ >> >> + if (!ret) >> >> + ret = ov5642_restore_state(sd); >> > >> > I suppose there's no way to avoid resetting the chip ? >> >> Whenever the clock is down, the chip looses its state. > > But the clock isn't turned down at every s_fmt call. Would it be possible > reinit the chip in the .s_power operation instead ? Guennadi had the same idea. I tried it out already to do it in s_power, but the chip hangs most times then. Even when I use mplayer and the s_power() is closely followed by the s_fmt() the chip crashes. Witch the same register writes but a small time gap. The chip has suuuch a strange behavior that I gave up trying to solve it. It sounds quite unbelievable I must admit, but meantime I stopped being amazed by the ov5642. >> >> return ret; >> >> } >> > >> > [snip] >> > >> >> +static int ov5642_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control >> >> *ctrl) +{ >> >> + struct i2c_client *client = v4l2_get_subdevdata(sd); >> >> + struct ov5642 *priv = to_ov5642(client); >> >> + int ret = 0; >> >> + u8 val8; >> >> + u16 val16; >> >> + u32 val32; >> >> + int trig; >> >> + struct v4l2_control aux_ctrl; >> >> + >> >> + switch (ctrl->id) { >> >> + case V4L2_CID_AUTOGAIN: >> >> + if (!ctrl->value) { >> >> + aux_ctrl.id = V4L2_CID_GAIN; >> >> + ret = ov5642_g_ctrl(sd, &aux_ctrl); >> >> + if (ret) >> >> + break; >> >> + priv->gain = aux_ctrl.value; >> >> + } >> >> + >> >> + ret = reg_read(client, REG_EXP_GAIN_CTRL, &val8); >> >> + if (ret) >> >> + break; >> >> + val8 = ctrl->value ? val8 & ~BIT(1) : val8 | BIT(1); >> >> + ret = reg_write(client, REG_EXP_GAIN_CTRL, val8); >> >> + if (!ret) >> >> + priv->agc = ctrl->value; >> > >> > What about caching the content of this register (and of other registers >> > below) instead of reading it back ? If you can't do that, a >> > reg_clr_set() function would make the code simpler. >> >> Ok I will do the caching. >> >> >> + break; > > -- > Regards, > > Laurent Pinchart > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2011/8/31 Bastian Hecht <hechtb@googlemail.com>: > 2011/8/31 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: >> Hi Bastian, >> >> On Wednesday 31 August 2011 17:27:49 Bastian Hecht wrote: >>> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: >>> > On Wednesday 17 August 2011 18:02:07 Bastian Hecht wrote: >>> >> The driver now supports automatic/manual gain, automatic/manual white >>> >> balance, automatic/manual exposure control, vertical flip, brightness >>> >> control, contrast control and saturation control. Additionally the >>> >> following effects are available now: rotating the hue in the colorspace, >>> >> gray scale image and solarize effect. >>> > >>> > Any chance to port soc-camera to the control framework ? :-) >>> >>> I redirect that to Guennadi :-) >> >> Guennadi, do you have any update on this ? Hans posted patches early this >> year, do you know if there has been any work on them since then ? >> >>> >> Signed-off-by: Bastian Hecht <hechtb@gmail.com> >>> >> --- >>> >> >>> >> diff --git a/drivers/media/video/ov5642.c b/drivers/media/video/ov5642.c >>> >> index 1b40d90..069a720 100644 >>> >> --- a/drivers/media/video/ov5642.c >>> >> +++ b/drivers/media/video/ov5642.c >>> >> @@ -74,6 +74,34 @@ >>> >> #define REG_AVG_WINDOW_END_Y_HIGH 0x5686 >>> >> #define REG_AVG_WINDOW_END_Y_LOW 0x5687 >>> >> >>> >> +/* default values in native space */ >>> >> +#define DEFAULT_RBBALANCE 0x400 >>> >> +#define DEFAULT_CONTRAST 0x20 >>> >> +#define DEFAULT_SATURATION 0x40 >>> >> + >>> >> +#define MAX_EXP_NATIVE 0x01ffff >>> >> +#define MAX_GAIN_NATIVE 0x1f >>> >> +#define MAX_RBBALANCE_NATIVE 0x0fff >>> >> +#define MAX_EXP 0xffff >>> >> +#define MAX_GAIN 0xff >>> >> +#define MAX_RBBALANCE 0xff >>> >> +#define MAX_HUE_TRIG_NATIVE 0x80 >>> >> + >>> >> +#define OV5642_CONTROL_BLUE_SATURATION (V4L2_CID_PRIVATE_BASE + >>> >> 0) +#define OV5642_CONTROL_RED_SATURATION (V4L2_CID_PRIVATE_BASE >>> >> + 1) +#define OV5642_CONTROL_GRAY_SCALE (V4L2_CID_PRIVATE_BASE + 2) >>> >> +#define OV5642_CONTROL_SOLARIZE (V4L2_CID_PRIVATE_BASE + >>> >> 3) >>> > >>> > If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. >>> >>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled >>> "V4L2_CID_PRIVATE_BASE deprecated" and read >>> Documentation/feature-removal-schedule.txt. I couldn't find anything. >>> If it is deprecated, do you have an idea how to offer device specific >>> features to the user? >> >> The basic idea is that controls should be standardized, or at least documented >> and added to the V4L2 spec. Controls should belong to a class, so you should >> select the proper base class and add a big offset (I've used 0x1000) in the >> meantime if you want to export private controls. > > Is this code accessable? Then I can just copy the scheme. > >>> >> + >>> >> +#define EXP_V4L2_TO_NATIVE(x) ((x) << 4) >>> >> +#define EXP_NATIVE_TO_V4L2(x) ((x) >> 4) >>> >> +#define GAIN_V4L2_TO_NATIVE(x) ((x) * MAX_GAIN_NATIVE / MAX_GAIN) >>> >> +#define GAIN_NATIVE_TO_V4L2(x) ((x) * MAX_GAIN / MAX_GAIN_NATIVE) >>> >> +#define RBBALANCE_V4L2_TO_NATIVE(x) ((x) * MAX_RBBALANCE_NATIVE / >>> >> MAX_RBBALANCE) +#define RBBALANCE_NATIVE_TO_V4L2(x) ((x) * MAX_RBBALANCE >>> >> / MAX_RBBALANCE_NATIVE) + >>> >> +/* flaw in the datasheet. we need some extra lines */ >>> >> +#define MANUAL_LONG_EXP_SAFETY_DISTANCE 20 >>> >> + >>> >> /* active pixel array size */ >>> >> #define OV5642_SENSOR_SIZE_X 2592 >>> >> #define OV5642_SENSOR_SIZE_Y 1944 >>> > >>> > [snip] >>> > >>> >> @@ -804,6 +1013,100 @@ static int ov5642_set_resolution(struct >>> >> v4l2_subdev *sd) return ret; >>> >> } >>> >> >>> >> +static int ov5642_restore_state(struct v4l2_subdev *sd) >>> >> +{ >>> >> + struct i2c_client *client = v4l2_get_subdevdata(sd); >>> >> + struct ov5642 *priv = to_ov5642(client); >>> >> + struct v4l2_control set_ctrl; >>> >> + int tmp_red_balance = priv->red_balance; >>> >> + int tmp_blue_balance = priv->blue_balance; >>> >> + int tmp_gain = priv->gain; >>> >> + int tmp_exp = priv->exposure; >>> >> + int ret; >>> >> + >>> >> + set_ctrl.id = V4L2_CID_AUTOGAIN; >>> >> + set_ctrl.value = priv->agc; >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> > >>> > What about writing to registers directly ? >>> >>> I thought about that too, but code duplication would be very large >>> (e.g. take the hue control). I considered the speedup of avoiding >>> function calls less than the error-proness of this duplication. It is >>> just cleaner imho. >> >> OK. This will be replaced when soc-camera will use the control framework >> anyway. >> >>> >> + >>> >> + if (!priv->agc) { >>> >> + set_ctrl.id = V4L2_CID_GAIN; >>> >> + set_ctrl.value = tmp_gain; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + } >>> >> + >>> >> + set_ctrl.id = V4L2_CID_AUTO_WHITE_BALANCE; >>> >> + set_ctrl.value = priv->awb; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + if (!priv->awb) { >>> >> + set_ctrl.id = V4L2_CID_RED_BALANCE; >>> >> + set_ctrl.value = tmp_red_balance; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = V4L2_CID_BLUE_BALANCE; >>> >> + set_ctrl.value = tmp_blue_balance; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + } >>> >> + >>> >> + set_ctrl.id = V4L2_CID_EXPOSURE_AUTO; >>> >> + set_ctrl.value = priv->aec; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + if (priv->aec == V4L2_EXPOSURE_MANUAL) { >>> >> + set_ctrl.id = V4L2_CID_EXPOSURE; >>> >> + set_ctrl.value = tmp_exp; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + } >>> >> + >>> >> + set_ctrl.id = V4L2_CID_VFLIP; >>> >> + set_ctrl.value = priv->vflip; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = OV5642_CONTROL_GRAY_SCALE; >>> >> + set_ctrl.value = priv->grayscale; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = OV5642_CONTROL_SOLARIZE; >>> >> + set_ctrl.value = priv->solarize; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = OV5642_CONTROL_BLUE_SATURATION; >>> >> + set_ctrl.value = priv->blue_saturation; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = OV5642_CONTROL_RED_SATURATION; >>> >> + set_ctrl.value = priv->red_saturation; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = V4L2_CID_BRIGHTNESS; >>> >> + set_ctrl.value = priv->brightness; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = V4L2_CID_CONTRAST; >>> >> + set_ctrl.value = priv->contrast; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = V4L2_CID_HUE; >>> >> + set_ctrl.value = priv->hue; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + return ret; >>> >> +} >>> >> + >>> >> static int ov5642_try_fmt(struct v4l2_subdev *sd, struct >>> >> v4l2_mbus_framefmt *mf) { >>> >> const struct ov5642_datafmt *fmt = >>> >> ov5642_find_datafmt(mf->code); @@ -856,6 +1159,9 @@ static int >>> >> ov5642_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) if >>> >> (!ret) >>> >> ret = ov5642_write_array(client, >>> >> ov5642_default_regs_finalise); >>> >> >>> >> + /* the chip has been reset, so configure it again */ >>> >> + if (!ret) >>> >> + ret = ov5642_restore_state(sd); >>> > >>> > I suppose there's no way to avoid resetting the chip ? >>> >>> Whenever the clock is down, the chip looses its state. >> >> But the clock isn't turned down at every s_fmt call. Would it be possible >> reinit the chip in the .s_power operation instead ? > > Guennadi had the same idea. I tried it out already to do it in > s_power, but the chip hangs most times then. Even when I use mplayer > and the s_power() is closely followed by the s_fmt() the chip crashes. > Witch the same register writes but a small time gap. The chip has > suuuch a strange behavior that I gave up trying to solve it. It sounds > quite unbelievable I must admit, but meantime I stopped being amazed > by the ov5642. To be more precise I tried to separate it to inialise in s_power, set_resolution in s_crop() and s_fmt() and finalise in streamon(). >>> >> return ret; >>> >> } >>> > >>> > [snip] >>> > >>> >> +static int ov5642_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control >>> >> *ctrl) +{ >>> >> + struct i2c_client *client = v4l2_get_subdevdata(sd); >>> >> + struct ov5642 *priv = to_ov5642(client); >>> >> + int ret = 0; >>> >> + u8 val8; >>> >> + u16 val16; >>> >> + u32 val32; >>> >> + int trig; >>> >> + struct v4l2_control aux_ctrl; >>> >> + >>> >> + switch (ctrl->id) { >>> >> + case V4L2_CID_AUTOGAIN: >>> >> + if (!ctrl->value) { >>> >> + aux_ctrl.id = V4L2_CID_GAIN; >>> >> + ret = ov5642_g_ctrl(sd, &aux_ctrl); >>> >> + if (ret) >>> >> + break; >>> >> + priv->gain = aux_ctrl.value; >>> >> + } >>> >> + >>> >> + ret = reg_read(client, REG_EXP_GAIN_CTRL, &val8); >>> >> + if (ret) >>> >> + break; >>> >> + val8 = ctrl->value ? val8 & ~BIT(1) : val8 | BIT(1); >>> >> + ret = reg_write(client, REG_EXP_GAIN_CTRL, val8); >>> >> + if (!ret) >>> >> + priv->agc = ctrl->value; >>> > >>> > What about caching the content of this register (and of other registers >>> > below) instead of reading it back ? If you can't do that, a >>> > reg_clr_set() function would make the code simpler. >>> >>> Ok I will do the caching. >>> >>> >> + break; >> >> -- >> Regards, >> >> Laurent Pinchart >> > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bastian, On Wednesday 31 August 2011 19:35:41 Bastian Hecht wrote: > 2011/8/31 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > > On Wednesday 31 August 2011 17:27:49 Bastian Hecht wrote: > >> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > >> > On Wednesday 17 August 2011 18:02:07 Bastian Hecht wrote: [snip] > >> >> +#define OV5642_CONTROL_BLUE_SATURATION (V4L2_CID_PRIVATE_BASE > >> >> + 0) +#define OV5642_CONTROL_RED_SATURATION > >> >> (V4L2_CID_PRIVATE_BASE + 1) +#define OV5642_CONTROL_GRAY_SCALE > >> >> (V4L2_CID_PRIVATE_BASE + 2) +#define OV5642_CONTROL_SOLARIZE > >> >> (V4L2_CID_PRIVATE_BASE + 3) > >> > > >> > If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. > >> > >> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled > >> "V4L2_CID_PRIVATE_BASE deprecated" and read > >> Documentation/feature-removal-schedule.txt. I couldn't find anything. > >> If it is deprecated, do you have an idea how to offer device specific > >> features to the user? > > > > The basic idea is that controls should be standardized, or at least > > documented and added to the V4L2 spec. Controls should belong to a > > class, so you should select the proper base class and add a big offset > > (I've used 0x1000) in the meantime if you want to export private > > controls. > > Is this code accessable? Then I can just copy the scheme. [snip] I just mean something like #define OV5642_CONTROL_BLUE_SATURATION (V4L2_CID_CAMERA_CLASS_BASE | 0x1001) I'm not sure which class those controls belong to though. > >> >> static int ov5642_try_fmt(struct v4l2_subdev *sd, struct > >> >> v4l2_mbus_framefmt *mf) { > >> >> const struct ov5642_datafmt *fmt = > >> >> ov5642_find_datafmt(mf->code); @@ -856,6 +1159,9 @@ static int > >> >> ov5642_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) > >> >> if (!ret) > >> >> ret = ov5642_write_array(client, > >> >> ov5642_default_regs_finalise); > >> >> > >> >> + /* the chip has been reset, so configure it again */ > >> >> + if (!ret) > >> >> + ret = ov5642_restore_state(sd); > >> > > >> > I suppose there's no way to avoid resetting the chip ? > >> > >> Whenever the clock is down, the chip looses its state. > > > > But the clock isn't turned down at every s_fmt call. Would it be possible > > reinit the chip in the .s_power operation instead ? > > Guennadi had the same idea. I tried it out already to do it in > s_power, but the chip hangs most times then. Even when I use mplayer > and the s_power() is closely followed by the s_fmt() the chip crashes. > Witch the same register writes but a small time gap. The chip has > suuuch a strange behavior that I gave up trying to solve it. It sounds > quite unbelievable I must admit, but meantime I stopped being amazed > by the ov5642. Given the outstanding quality of Omnivision chips let's keep it as-is then :-)
On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: > 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: [clip] > > If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. > > I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled > "V4L2_CID_PRIVATE_BASE deprecated" and read > Documentation/feature-removal-schedule.txt. I couldn't find anything. Hmm. Did you happen to check when that has been written? :) Please use this one instead: <URL:http://hverkuil.home.xs4all.nl/spec/media.html>
On Thu, 1 Sep 2011, Sakari Ailus wrote: > On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: > > 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > [clip] > > > If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. > > > > I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled > > "V4L2_CID_PRIVATE_BASE deprecated" and read > > Documentation/feature-removal-schedule.txt. I couldn't find anything. > > Hmm. Did you happen to check when that has been written? :) > > Please use this one instead: > > <URL:http://hverkuil.home.xs4all.nl/spec/media.html> "Drivers can also implement their own custom controls using V4L2_CID_PRIVATE_BASE and higher values." Which specific location describes V4L2_CID_PRIVATE_BASE differently there? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 01, 2011 at 09:15:20AM +0200, Guennadi Liakhovetski wrote: > On Thu, 1 Sep 2011, Sakari Ailus wrote: > > > On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: > > > 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > > [clip] > > > > If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. > > > > > > I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled > > > "V4L2_CID_PRIVATE_BASE deprecated" and read > > > Documentation/feature-removal-schedule.txt. I couldn't find anything. > > > > Hmm. Did you happen to check when that has been written? :) > > > > Please use this one instead: > > > > <URL:http://hverkuil.home.xs4all.nl/spec/media.html> > > "Drivers can also implement their own custom controls using > V4L2_CID_PRIVATE_BASE and higher values." > > Which specific location describes V4L2_CID_PRIVATE_BASE differently there? That was a general comment, not related to the private base. There's no use for a three-year-old spec as a reference! The control framework does not support private controls, for example. The controls should be put to their own class in videodev2.h nowadays, that's my understanding. Cc Hans.
Hi Sakari, On 09/01/2011 10:47 AM, Sakari Ailus wrote: > On Thu, Sep 01, 2011 at 09:15:20AM +0200, Guennadi Liakhovetski wrote: >> On Thu, 1 Sep 2011, Sakari Ailus wrote: >> >>> On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: >>>> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: >>> [clip] >>>>> If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. >>>> >>>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled >>>> "V4L2_CID_PRIVATE_BASE deprecated" and read >>>> Documentation/feature-removal-schedule.txt. I couldn't find anything. >>> >>> Hmm. Did you happen to check when that has been written? :) >>> >>> Please use this one instead: >>> >>> <URL:http://hverkuil.home.xs4all.nl/spec/media.html> >> >> "Drivers can also implement their own custom controls using >> V4L2_CID_PRIVATE_BASE and higher values." >> >> Which specific location describes V4L2_CID_PRIVATE_BASE differently there? > > That was a general comment, not related to the private base. There's no > use for a three-year-old spec as a reference! > > The control framework does not support private controls, for example. The > controls should be put to their own class in videodev2.h nowadays, that's my > understanding. Cc Hans. Is this really the case that we close the door for private controls in the mainline kernel ? Or am I misunderstanding something ? How about v4l2_ctrl_new_custom() ? What if there are controls applicable to single driver only ? Do we really want to have plenty of such in videodev2.h ?
On Thu, Sep 01, 2011 at 11:14:08AM +0200, Sylwester Nawrocki wrote: > Hi Sakari, > > On 09/01/2011 10:47 AM, Sakari Ailus wrote: > > On Thu, Sep 01, 2011 at 09:15:20AM +0200, Guennadi Liakhovetski wrote: > >> On Thu, 1 Sep 2011, Sakari Ailus wrote: > >> > >>> On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: > >>>> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > >>> [clip] > >>>>> If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. > >>>> > >>>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled > >>>> "V4L2_CID_PRIVATE_BASE deprecated" and read > >>>> Documentation/feature-removal-schedule.txt. I couldn't find anything. > >>> > >>> Hmm. Did you happen to check when that has been written? :) > >>> > >>> Please use this one instead: > >>> > >>> <URL:http://hverkuil.home.xs4all.nl/spec/media.html> > >> > >> "Drivers can also implement their own custom controls using > >> V4L2_CID_PRIVATE_BASE and higher values." > >> > >> Which specific location describes V4L2_CID_PRIVATE_BASE differently there? > > > > That was a general comment, not related to the private base. There's no > > use for a three-year-old spec as a reference! > > > > The control framework does not support private controls, for example. The > > controls should be put to their own class in videodev2.h nowadays, that's my > > understanding. Cc Hans. > > Is this really the case that we close the door for private controls in > the mainline kernel ? Or am I misunderstanding something ? > How about v4l2_ctrl_new_custom() ? > > What if there are controls applicable to single driver only ? > Do we really want to have plenty of such in videodev2.h ? We have some of those already in videodev2.h. I'm not certain if I'm happy with this myself, considering e.g. that we could get a few truckloads of only camera lens hardware specific controls in the near future.
2011/9/1 Sakari Ailus <sakari.ailus@iki.fi>: > On Thu, Sep 01, 2011 at 11:14:08AM +0200, Sylwester Nawrocki wrote: >> Hi Sakari, >> >> On 09/01/2011 10:47 AM, Sakari Ailus wrote: >> > On Thu, Sep 01, 2011 at 09:15:20AM +0200, Guennadi Liakhovetski wrote: >> >> On Thu, 1 Sep 2011, Sakari Ailus wrote: >> >> >> >>> On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: >> >>>> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: >> >>> [clip] >> >>>>> If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. >> >>>> >> >>>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled >> >>>> "V4L2_CID_PRIVATE_BASE deprecated" and read >> >>>> Documentation/feature-removal-schedule.txt. I couldn't find anything. >> >>> >> >>> Hmm. Did you happen to check when that has been written? :) >> >>> >> >>> Please use this one instead: >> >>> >> >>> <URL:http://hverkuil.home.xs4all.nl/spec/media.html> >> >> >> >> "Drivers can also implement their own custom controls using >> >> V4L2_CID_PRIVATE_BASE and higher values." >> >> >> >> Which specific location describes V4L2_CID_PRIVATE_BASE differently there? >> > >> > That was a general comment, not related to the private base. There's no >> > use for a three-year-old spec as a reference! >> > >> > The control framework does not support private controls, for example. The >> > controls should be put to their own class in videodev2.h nowadays, that's my >> > understanding. Cc Hans. >> >> Is this really the case that we close the door for private controls in >> the mainline kernel ? Or am I misunderstanding something ? >> How about v4l2_ctrl_new_custom() ? >> >> What if there are controls applicable to single driver only ? >> Do we really want to have plenty of such in videodev2.h ? > > We have some of those already in videodev2.h. I'm not certain if I'm happy > with this myself, considering e.g. that we could get a few truckloads of > only camera lens hardware specific controls in the near future. So in my case (as these are controls that might be used by others too) I should add something like #define V4L2_CID_BLUE_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+19) #define V4L2_CID_RED_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+20) #define V4L2_CID_GRAY_SCALE_IMAGE (V4L2_CID_CAMERA_CLASS_BASE+21) #define V4L2_CID_SOLARIZE_EFFECT (V4L2_CID_CAMERA_CLASS_BASE+22) to videodev2.h? > -- > Sakari Ailus > sakari.ailus@iki.fi > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2011/8/31 Guennadi Liakhovetski <g.liakhovetski@gmx.de>: > On Wed, 31 Aug 2011, Bastian Hecht wrote: > >> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: >> > Hi Bastian, >> > >> > Thanks for the patch. >> > >> > On Wednesday 17 August 2011 18:02:07 Bastian Hecht wrote: >> >> The driver now supports automatic/manual gain, automatic/manual white >> >> balance, automatic/manual exposure control, vertical flip, brightness >> >> control, contrast control and saturation control. Additionally the >> >> following effects are available now: rotating the hue in the colorspace, >> >> gray scale image and solarize effect. >> > >> > Any chance to port soc-camera to the control framework ? :-) >> >> I redirect that to Guennadi :-) > > Hans is prepaing an update of his port, then we'll integrate it... This > all will take time, so, it's better to do this driver now the "old > soc-camera" way, and port it later. > > [snip] > >> >> +static int ov5642_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control >> >> *ctrl) +{ >> >> + struct i2c_client *client = v4l2_get_subdevdata(sd); >> >> + struct ov5642 *priv = to_ov5642(client); >> >> + int ret = 0; >> >> + u8 val8; >> >> + u16 val16; >> >> + u32 val32; >> >> + int trig; >> >> + struct v4l2_control aux_ctrl; >> >> + >> >> + switch (ctrl->id) { >> >> + case V4L2_CID_AUTOGAIN: >> >> + if (!ctrl->value) { >> >> + aux_ctrl.id = V4L2_CID_GAIN; >> >> + ret = ov5642_g_ctrl(sd, &aux_ctrl); >> >> + if (ret) >> >> + break; >> >> + priv->gain = aux_ctrl.value; >> >> + } >> >> + >> >> + ret = reg_read(client, REG_EXP_GAIN_CTRL, &val8); >> >> + if (ret) >> >> + break; >> >> + val8 = ctrl->value ? val8 & ~BIT(1) : val8 | BIT(1); >> >> + ret = reg_write(client, REG_EXP_GAIN_CTRL, val8); >> >> + if (!ret) >> >> + priv->agc = ctrl->value; >> > >> > What about caching the content of this register (and of other registers below) >> > instead of reading it back ? If you can't do that, a reg_clr_set() function >> > would make the code simpler. >> >> Ok I will do the caching. > > Wasn't the reason for reading those registers from the hardware, that the > sensor changes them in auto* modes (autogain in this case)? There are different cases. E.g. this register sets the autogain and autoexposure on/off not the gain value itself. So indeed one should cache it (sometimes I registered side-effects on the ov5642 - setting 1 register changed others. I hope I won't run into trouble with these). best, Bastian > Thanks > Guennadi > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bastian, On Mon, Sep 05, 2011 at 09:32:55AM +0000, Bastian Hecht wrote: > 2011/9/1 Sakari Ailus <sakari.ailus@iki.fi>: > > On Thu, Sep 01, 2011 at 11:14:08AM +0200, Sylwester Nawrocki wrote: > >> Hi Sakari, > >> > >> On 09/01/2011 10:47 AM, Sakari Ailus wrote: > >> > On Thu, Sep 01, 2011 at 09:15:20AM +0200, Guennadi Liakhovetski wrote: > >> >> On Thu, 1 Sep 2011, Sakari Ailus wrote: > >> >> > >> >>> On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: > >> >>>> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > >> >>> [clip] > >> >>>>> If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. > >> >>>> > >> >>>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled > >> >>>> "V4L2_CID_PRIVATE_BASE deprecated" and read > >> >>>> Documentation/feature-removal-schedule.txt. I couldn't find anything. > >> >>> > >> >>> Hmm. Did you happen to check when that has been written? :) > >> >>> > >> >>> Please use this one instead: > >> >>> > >> >>> <URL:http://hverkuil.home.xs4all.nl/spec/media.html> > >> >> > >> >> "Drivers can also implement their own custom controls using > >> >> V4L2_CID_PRIVATE_BASE and higher values." > >> >> > >> >> Which specific location describes V4L2_CID_PRIVATE_BASE differently there? > >> > > >> > That was a general comment, not related to the private base. There's no > >> > use for a three-year-old spec as a reference! > >> > > >> > The control framework does not support private controls, for example. The > >> > controls should be put to their own class in videodev2.h nowadays, that's my > >> > understanding. Cc Hans. > >> > >> Is this really the case that we close the door for private controls in > >> the mainline kernel ? Or am I misunderstanding something ? > >> How about v4l2_ctrl_new_custom() ? > >> > >> What if there are controls applicable to single driver only ? > >> Do we really want to have plenty of such in videodev2.h ? > > > > We have some of those already in videodev2.h. I'm not certain if I'm happy > > with this myself, considering e.g. that we could get a few truckloads of > > only camera lens hardware specific controls in the near future. > > So in my case (as these are controls that might be used by others too) > I should add something like > > #define V4L2_CID_BLUE_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+19) > #define V4L2_CID_RED_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+20) What do these two controls do? Do they control gain or something else? > #define V4L2_CID_GRAY_SCALE_IMAGE (V4L2_CID_CAMERA_CLASS_BASE+21) V4L2_CID_COLOR_KILLER looks like something which would fit for the purpose. > #define V4L2_CID_SOLARIZE_EFFECT (V4L2_CID_CAMERA_CLASS_BASE+22) Sounds interesting for a sensor. I wonder if this would fall under a menu control, V4L2_CID_COLORFX.
Hello Sakari! 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: > Hi Bastian, > > On Mon, Sep 05, 2011 at 09:32:55AM +0000, Bastian Hecht wrote: >> 2011/9/1 Sakari Ailus <sakari.ailus@iki.fi>: >> > On Thu, Sep 01, 2011 at 11:14:08AM +0200, Sylwester Nawrocki wrote: >> >> Hi Sakari, >> >> >> >> On 09/01/2011 10:47 AM, Sakari Ailus wrote: >> >> > On Thu, Sep 01, 2011 at 09:15:20AM +0200, Guennadi Liakhovetski wrote: >> >> >> On Thu, 1 Sep 2011, Sakari Ailus wrote: >> >> >> >> >> >>> On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: >> >> >>>> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: >> >> >>> [clip] >> >> >>>>> If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. >> >> >>>> >> >> >>>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled >> >> >>>> "V4L2_CID_PRIVATE_BASE deprecated" and read >> >> >>>> Documentation/feature-removal-schedule.txt. I couldn't find anything. >> >> >>> >> >> >>> Hmm. Did you happen to check when that has been written? :) >> >> >>> >> >> >>> Please use this one instead: >> >> >>> >> >> >>> <URL:http://hverkuil.home.xs4all.nl/spec/media.html> >> >> >> >> >> >> "Drivers can also implement their own custom controls using >> >> >> V4L2_CID_PRIVATE_BASE and higher values." >> >> >> >> >> >> Which specific location describes V4L2_CID_PRIVATE_BASE differently there? >> >> > >> >> > That was a general comment, not related to the private base. There's no >> >> > use for a three-year-old spec as a reference! >> >> > >> >> > The control framework does not support private controls, for example. The >> >> > controls should be put to their own class in videodev2.h nowadays, that's my >> >> > understanding. Cc Hans. >> >> >> >> Is this really the case that we close the door for private controls in >> >> the mainline kernel ? Or am I misunderstanding something ? >> >> How about v4l2_ctrl_new_custom() ? >> >> >> >> What if there are controls applicable to single driver only ? >> >> Do we really want to have plenty of such in videodev2.h ? >> > >> > We have some of those already in videodev2.h. I'm not certain if I'm happy >> > with this myself, considering e.g. that we could get a few truckloads of >> > only camera lens hardware specific controls in the near future. >> >> So in my case (as these are controls that might be used by others too) >> I should add something like >> >> #define V4L2_CID_BLUE_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+19) >> #define V4L2_CID_RED_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+20) > > What do these two controls do? Do they control gain or something else? Hmm. Maybe I named them a bit unsharp. It is the U Saturation and V Saturation. To me it looks like turning up the saturation in HSV space, but only for either the blue or the red channel. This would correspond to V4L2_CID_{RED,BLUE}_BALANCE when I read the docs. They say it is "{Red,Blue} chroma balance". I have other controls for that I used V4L2_CID_{RED,BLUE}_BALANCE. These are gains. So in fact I should swap them in my code and the remaining question is, how to name the red and blue gain controls. >> #define V4L2_CID_GRAY_SCALE_IMAGE (V4L2_CID_CAMERA_CLASS_BASE+21) > > V4L2_CID_COLOR_KILLER looks like something which would fit for the purpose. Oh great! So I just take this. >> #define V4L2_CID_SOLARIZE_EFFECT (V4L2_CID_CAMERA_CLASS_BASE+22) > > Sounds interesting for a sensor. I wonder if this would fall under a menu > control, V4L2_CID_COLORFX. When I read the the possible enums for V4L2_CID_COLORFX, it indeed sounds very much like my solarize effect should be added there too. I found V4L2_COLORFX_BW there, too. Isn't that a duplicate of the color killer control then? > -- > Sakari Ailus > e-mail: sakari.ailus@iki.fi jabber/XMPP/Gmail: sailus@retiisi.org.uk > Thanks for your help, Bastian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 06, 2011 at 07:56:40AM +0000, Bastian Hecht wrote: > Hello Sakari! Hi Bastian, > 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: > > Hi Bastian, > > > > On Mon, Sep 05, 2011 at 09:32:55AM +0000, Bastian Hecht wrote: > >> 2011/9/1 Sakari Ailus <sakari.ailus@iki.fi>: > >> > On Thu, Sep 01, 2011 at 11:14:08AM +0200, Sylwester Nawrocki wrote: > >> >> Hi Sakari, > >> >> > >> >> On 09/01/2011 10:47 AM, Sakari Ailus wrote: > >> >> > On Thu, Sep 01, 2011 at 09:15:20AM +0200, Guennadi Liakhovetski wrote: > >> >> >> On Thu, 1 Sep 2011, Sakari Ailus wrote: > >> >> >> > >> >> >>> On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: > >> >> >>>> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > >> >> >>> [clip] > >> >> >>>>> If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. > >> >> >>>> > >> >> >>>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled > >> >> >>>> "V4L2_CID_PRIVATE_BASE deprecated" and read > >> >> >>>> Documentation/feature-removal-schedule.txt. I couldn't find anything. > >> >> >>> > >> >> >>> Hmm. Did you happen to check when that has been written? :) > >> >> >>> > >> >> >>> Please use this one instead: > >> >> >>> > >> >> >>> <URL:http://hverkuil.home.xs4all.nl/spec/media.html> > >> >> >> > >> >> >> "Drivers can also implement their own custom controls using > >> >> >> V4L2_CID_PRIVATE_BASE and higher values." > >> >> >> > >> >> >> Which specific location describes V4L2_CID_PRIVATE_BASE differently there? > >> >> > > >> >> > That was a general comment, not related to the private base. There's no > >> >> > use for a three-year-old spec as a reference! > >> >> > > >> >> > The control framework does not support private controls, for example. The > >> >> > controls should be put to their own class in videodev2.h nowadays, that's my > >> >> > understanding. Cc Hans. > >> >> > >> >> Is this really the case that we close the door for private controls in > >> >> the mainline kernel ? Or am I misunderstanding something ? > >> >> How about v4l2_ctrl_new_custom() ? > >> >> > >> >> What if there are controls applicable to single driver only ? > >> >> Do we really want to have plenty of such in videodev2.h ? > >> > > >> > We have some of those already in videodev2.h. I'm not certain if I'm happy > >> > with this myself, considering e.g. that we could get a few truckloads of > >> > only camera lens hardware specific controls in the near future. > >> > >> So in my case (as these are controls that might be used by others too) > >> I should add something like > >> > >> #define V4L2_CID_BLUE_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+19) > >> #define V4L2_CID_RED_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+20) > > > > What do these two controls do? Do they control gain or something else? > > Hmm. Maybe I named them a bit unsharp. It is the U Saturation and V > Saturation. To me it looks like turning up the saturation in HSV > space, but only for either the blue or the red channel. This would > correspond to V4L2_CID_{RED,BLUE}_BALANCE when I read the docs. They > say it is "{Red,Blue} chroma balance". > > I have other controls for that I used V4L2_CID_{RED,BLUE}_BALANCE. > These are gains. So in fact I should swap them in my code and the > remaining question is, how to name the red and blue gain controls. I think Laurent had a similar issue in his Aptina sensor driver. In my opinion we need a class for low level controls such as the gain ones. Do I understand correctly they control the red and blue pixel gain in the sensor pixel matrix? Do you also have gain controls for the two greens? > >> #define V4L2_CID_GRAY_SCALE_IMAGE (V4L2_CID_CAMERA_CLASS_BASE+21) > > > > V4L2_CID_COLOR_KILLER looks like something which would fit for the purpose. > > Oh great! So I just take this. > > >> #define V4L2_CID_SOLARIZE_EFFECT (V4L2_CID_CAMERA_CLASS_BASE+22) > > > > Sounds interesting for a sensor. I wonder if this would fall under a menu > > control, V4L2_CID_COLORFX. > > When I read the the possible enums for V4L2_CID_COLORFX, it indeed > sounds very much like my solarize effect should be added there too. I > found V4L2_COLORFX_BW there, too. Isn't that a duplicate of the color > killer control then? In my opinion V4L2_CID_COLORFX should never be implemented in drivers for which the hardware doesn't implement these effects in a non-parametrisable way. This control was originally added for the OMAP 3 ISP driver but the driver never implemented it. I think you have a valid case using this control. I think the main difference between the two is that V4L2_COLORFX_BW is something that you can't use with other effects while V4L2_CID_COLOR_KILLER can be used with any of the effects. Based on your original proposal the black/white should stay as a separate control but the solarise should be configurable through V4L2_CID_COLORFX menu control. So it boils down to the question whether you can use them at the same time.
2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: > On Tue, Sep 06, 2011 at 07:56:40AM +0000, Bastian Hecht wrote: >> Hello Sakari! > > Hi Bastian, > >> 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: >> > Hi Bastian, >> > >> > On Mon, Sep 05, 2011 at 09:32:55AM +0000, Bastian Hecht wrote: >> >> 2011/9/1 Sakari Ailus <sakari.ailus@iki.fi>: >> >> > On Thu, Sep 01, 2011 at 11:14:08AM +0200, Sylwester Nawrocki wrote: >> >> >> Hi Sakari, >> >> >> >> >> >> On 09/01/2011 10:47 AM, Sakari Ailus wrote: >> >> >> > On Thu, Sep 01, 2011 at 09:15:20AM +0200, Guennadi Liakhovetski wrote: >> >> >> >> On Thu, 1 Sep 2011, Sakari Ailus wrote: >> >> >> >> >> >> >> >>> On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: >> >> >> >>>> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: >> >> >> >>> [clip] >> >> >> >>>>> If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. >> >> >> >>>> >> >> >> >>>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled >> >> >> >>>> "V4L2_CID_PRIVATE_BASE deprecated" and read >> >> >> >>>> Documentation/feature-removal-schedule.txt. I couldn't find anything. >> >> >> >>> >> >> >> >>> Hmm. Did you happen to check when that has been written? :) >> >> >> >>> >> >> >> >>> Please use this one instead: >> >> >> >>> >> >> >> >>> <URL:http://hverkuil.home.xs4all.nl/spec/media.html> >> >> >> >> >> >> >> >> "Drivers can also implement their own custom controls using >> >> >> >> V4L2_CID_PRIVATE_BASE and higher values." >> >> >> >> >> >> >> >> Which specific location describes V4L2_CID_PRIVATE_BASE differently there? >> >> >> > >> >> >> > That was a general comment, not related to the private base. There's no >> >> >> > use for a three-year-old spec as a reference! >> >> >> > >> >> >> > The control framework does not support private controls, for example. The >> >> >> > controls should be put to their own class in videodev2.h nowadays, that's my >> >> >> > understanding. Cc Hans. >> >> >> >> >> >> Is this really the case that we close the door for private controls in >> >> >> the mainline kernel ? Or am I misunderstanding something ? >> >> >> How about v4l2_ctrl_new_custom() ? >> >> >> >> >> >> What if there are controls applicable to single driver only ? >> >> >> Do we really want to have plenty of such in videodev2.h ? >> >> > >> >> > We have some of those already in videodev2.h. I'm not certain if I'm happy >> >> > with this myself, considering e.g. that we could get a few truckloads of >> >> > only camera lens hardware specific controls in the near future. >> >> >> >> So in my case (as these are controls that might be used by others too) >> >> I should add something like >> >> >> >> #define V4L2_CID_BLUE_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+19) >> >> #define V4L2_CID_RED_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+20) >> > >> > What do these two controls do? Do they control gain or something else? >> >> Hmm. Maybe I named them a bit unsharp. It is the U Saturation and V >> Saturation. To me it looks like turning up the saturation in HSV >> space, but only for either the blue or the red channel. This would >> correspond to V4L2_CID_{RED,BLUE}_BALANCE when I read the docs. They >> say it is "{Red,Blue} chroma balance". >> >> I have other controls for that I used V4L2_CID_{RED,BLUE}_BALANCE. >> These are gains. So in fact I should swap them in my code and the >> remaining question is, how to name the red and blue gain controls. > > I think Laurent had a similar issue in his Aptina sensor driver. In my > opinion we need a class for low level controls such as the gain ones. Do I > understand correctly they control the red and blue pixel gain in the sensor > pixel matrix? Do you also have gain controls for the two greens? Yes, I assume that this is done there. Either in the analog circuit by decreasing the preload or digitally then. Don't know exactly. There are registers for the green pixels as well. As I used the V4L2_CID_{RED,BLUE}_BALANCE controls and there was no V4L2_CID_GREEN_BALANCE, I just skipped green as one can increase/decrease the global gain and get an arbitrary mix as well. So for these gain settings we should add these? V4L2_CID_RED_GAIN V4L2_CID_BLUE_GAIN V4L2_CID_GREEN_GAIN >> >> #define V4L2_CID_GRAY_SCALE_IMAGE (V4L2_CID_CAMERA_CLASS_BASE+21) >> > >> > V4L2_CID_COLOR_KILLER looks like something which would fit for the purpose. >> >> Oh great! So I just take this. >> >> >> #define V4L2_CID_SOLARIZE_EFFECT (V4L2_CID_CAMERA_CLASS_BASE+22) >> > >> > Sounds interesting for a sensor. I wonder if this would fall under a menu >> > control, V4L2_CID_COLORFX. >> >> When I read the the possible enums for V4L2_CID_COLORFX, it indeed >> sounds very much like my solarize effect should be added there too. I >> found V4L2_COLORFX_BW there, too. Isn't that a duplicate of the color >> killer control then? > > In my opinion V4L2_CID_COLORFX should never be implemented in drivers for > which the hardware doesn't implement these effects in a non-parametrisable > way. This control was originally added for the OMAP 3 ISP driver but the > driver never implemented it. Your triple negation (never, doesn't, non-) is quite tricky xD If I get it right, you say that one should not use V4L2_CID_COLORFX for hardware with parametrisable effects. My BW and Solarize effects are non-parametrisable and they can be turned on together (which makes not so much sense though - but these fun-effects like "solarize" aren't here to make sense, I guess :-) ). > I think you have a valid case using this control. I think the main > difference between the two is that V4L2_COLORFX_BW is something that you > can't use with other effects while V4L2_CID_COLOR_KILLER can be used with > any of the effects. > Based on your original proposal the black/white should stay as a separate > control but the solarise should be configurable through V4L2_CID_COLORFX > menu control. So it boils down to the question whether you can use them at > the same time. I can - so it is still working to enable V4L2_COLORFX_BW and V4L2_CID_COLORFX with a new enum value, right? Is that the way to go now? > -- > Sakari Ailus > e-mail: sakari.ailus@iki.fi jabber/XMPP/Gmail: sailus@retiisi.org.uk > Thanks, Bastian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 06, 2011 at 09:01:15AM +0000, Bastian Hecht wrote: > 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: > > On Tue, Sep 06, 2011 at 07:56:40AM +0000, Bastian Hecht wrote: > >> Hello Sakari! > > > > Hi Bastian, > > > >> 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: > >> > Hi Bastian, > >> > > >> > On Mon, Sep 05, 2011 at 09:32:55AM +0000, Bastian Hecht wrote: > >> >> 2011/9/1 Sakari Ailus <sakari.ailus@iki.fi>: > >> >> > On Thu, Sep 01, 2011 at 11:14:08AM +0200, Sylwester Nawrocki wrote: > >> >> >> Hi Sakari, > >> >> >> > >> >> >> On 09/01/2011 10:47 AM, Sakari Ailus wrote: > >> >> >> > On Thu, Sep 01, 2011 at 09:15:20AM +0200, Guennadi Liakhovetski wrote: > >> >> >> >> On Thu, 1 Sep 2011, Sakari Ailus wrote: > >> >> >> >> > >> >> >> >>> On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: > >> >> >> >>>> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > >> >> >> >>> [clip] > >> >> >> >>>>> If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. > >> >> >> >>>> > >> >> >> >>>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled > >> >> >> >>>> "V4L2_CID_PRIVATE_BASE deprecated" and read > >> >> >> >>>> Documentation/feature-removal-schedule.txt. I couldn't find anything. > >> >> >> >>> > >> >> >> >>> Hmm. Did you happen to check when that has been written? :) > >> >> >> >>> > >> >> >> >>> Please use this one instead: > >> >> >> >>> > >> >> >> >>> <URL:http://hverkuil.home.xs4all.nl/spec/media.html> > >> >> >> >> > >> >> >> >> "Drivers can also implement their own custom controls using > >> >> >> >> V4L2_CID_PRIVATE_BASE and higher values." > >> >> >> >> > >> >> >> >> Which specific location describes V4L2_CID_PRIVATE_BASE differently there? > >> >> >> > > >> >> >> > That was a general comment, not related to the private base. There's no > >> >> >> > use for a three-year-old spec as a reference! > >> >> >> > > >> >> >> > The control framework does not support private controls, for example. The > >> >> >> > controls should be put to their own class in videodev2.h nowadays, that's my > >> >> >> > understanding. Cc Hans. > >> >> >> > >> >> >> Is this really the case that we close the door for private controls in > >> >> >> the mainline kernel ? Or am I misunderstanding something ? > >> >> >> How about v4l2_ctrl_new_custom() ? > >> >> >> > >> >> >> What if there are controls applicable to single driver only ? > >> >> >> Do we really want to have plenty of such in videodev2.h ? > >> >> > > >> >> > We have some of those already in videodev2.h. I'm not certain if I'm happy > >> >> > with this myself, considering e.g. that we could get a few truckloads of > >> >> > only camera lens hardware specific controls in the near future. > >> >> > >> >> So in my case (as these are controls that might be used by others too) > >> >> I should add something like > >> >> > >> >> #define V4L2_CID_BLUE_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+19) > >> >> #define V4L2_CID_RED_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+20) > >> > > >> > What do these two controls do? Do they control gain or something else? > >> > >> Hmm. Maybe I named them a bit unsharp. It is the U Saturation and V > >> Saturation. To me it looks like turning up the saturation in HSV > >> space, but only for either the blue or the red channel. This would > >> correspond to V4L2_CID_{RED,BLUE}_BALANCE when I read the docs. They > >> say it is "{Red,Blue} chroma balance". > >> > >> I have other controls for that I used V4L2_CID_{RED,BLUE}_BALANCE. > >> These are gains. So in fact I should swap them in my code and the > >> remaining question is, how to name the red and blue gain controls. > > > > I think Laurent had a similar issue in his Aptina sensor driver. In my > > opinion we need a class for low level controls such as the gain ones. Do I > > understand correctly they control the red and blue pixel gain in the sensor > > pixel matrix? Do you also have gain controls for the two greens? > > Yes, I assume that this is done there. Either in the analog circuit by > decreasing the preload or digitally then. Don't know exactly. There > are registers for the green pixels as well. As I used the > V4L2_CID_{RED,BLUE}_BALANCE controls and there was no > V4L2_CID_GREEN_BALANCE, I just skipped green as one can > increase/decrease the global gain and get an arbitrary mix as well. > > So for these gain settings we should add these? > V4L2_CID_RED_GAIN > V4L2_CID_BLUE_GAIN > V4L2_CID_GREEN_GAIN Do you have two or just one green gains? In all sensors I've seen there are two. I think I could send an RFC on this to the list and cc you and Laurent. > >> >> #define V4L2_CID_GRAY_SCALE_IMAGE (V4L2_CID_CAMERA_CLASS_BASE+21) > >> > > >> > V4L2_CID_COLOR_KILLER looks like something which would fit for the purpose. > >> > >> Oh great! So I just take this. > >> > >> >> #define V4L2_CID_SOLARIZE_EFFECT (V4L2_CID_CAMERA_CLASS_BASE+22) > >> > > >> > Sounds interesting for a sensor. I wonder if this would fall under a menu > >> > control, V4L2_CID_COLORFX. > >> > >> When I read the the possible enums for V4L2_CID_COLORFX, it indeed > >> sounds very much like my solarize effect should be added there too. I > >> found V4L2_COLORFX_BW there, too. Isn't that a duplicate of the color > >> killer control then? > > > > In my opinion V4L2_CID_COLORFX should never be implemented in drivers for > > which the hardware doesn't implement these effects in a non-parametrisable > > way. This control was originally added for the OMAP 3 ISP driver but the > > driver never implemented it. > > Your triple negation (never, doesn't, non-) is quite tricky xD > If I get it right, you say that one should not use V4L2_CID_COLORFX > for hardware with parametrisable effects. Yes. I could have written that in a more clear way. ;-) > My BW and Solarize effects are non-parametrisable and they can be > turned on together (which makes not so much sense though - but these > fun-effects like "solarize" aren't here to make sense, I guess :-) ). Good. The OMAP 3 ISP actually provides a way to set gamma tables, any effects implemented using them are more or less use case specific. There are also other uses for those same gamma tables, making a driver implementation for effects using them non-functional in practice. > > I think you have a valid case using this control. I think the main > > difference between the two is that V4L2_COLORFX_BW is something that you > > can't use with other effects while V4L2_CID_COLOR_KILLER can be used with > > any of the effects. > > > Based on your original proposal the black/white should stay as a separate > > control but the solarise should be configurable through V4L2_CID_COLORFX > > menu control. So it boils down to the question whether you can use them at > > the same time. > > I can - so it is still working to enable V4L2_COLORFX_BW and > V4L2_CID_COLORFX with a new enum value, right? Is that the way to go > now? That's my opinion, yes.
Hello Sakari, 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: > On Tue, Sep 06, 2011 at 09:01:15AM +0000, Bastian Hecht wrote: >> 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: >> > On Tue, Sep 06, 2011 at 07:56:40AM +0000, Bastian Hecht wrote: >> >> Hello Sakari! >> > >> > Hi Bastian, >> > >> >> 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: >> >> > Hi Bastian, >> >> > >> >> > On Mon, Sep 05, 2011 at 09:32:55AM +0000, Bastian Hecht wrote: >> >> >> 2011/9/1 Sakari Ailus <sakari.ailus@iki.fi>: >> >> >> > On Thu, Sep 01, 2011 at 11:14:08AM +0200, Sylwester Nawrocki wrote: >> >> >> >> Hi Sakari, >> >> >> >> >> >> >> >> On 09/01/2011 10:47 AM, Sakari Ailus wrote: >> >> >> >> > On Thu, Sep 01, 2011 at 09:15:20AM +0200, Guennadi Liakhovetski wrote: >> >> >> >> >> On Thu, 1 Sep 2011, Sakari Ailus wrote: >> >> >> >> >> >> >> >> >> >>> On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: >> >> >> >> >>>> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: >> >> >> >> >>> [clip] >> >> >> >> >>>>> If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. >> >> >> >> >>>> >> >> >> >> >>>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled >> >> >> >> >>>> "V4L2_CID_PRIVATE_BASE deprecated" and read >> >> >> >> >>>> Documentation/feature-removal-schedule.txt. I couldn't find anything. >> >> >> >> >>> >> >> >> >> >>> Hmm. Did you happen to check when that has been written? :) >> >> >> >> >>> >> >> >> >> >>> Please use this one instead: >> >> >> >> >>> >> >> >> >> >>> <URL:http://hverkuil.home.xs4all.nl/spec/media.html> >> >> >> >> >> >> >> >> >> >> "Drivers can also implement their own custom controls using >> >> >> >> >> V4L2_CID_PRIVATE_BASE and higher values." >> >> >> >> >> >> >> >> >> >> Which specific location describes V4L2_CID_PRIVATE_BASE differently there? >> >> >> >> > >> >> >> >> > That was a general comment, not related to the private base. There's no >> >> >> >> > use for a three-year-old spec as a reference! >> >> >> >> > >> >> >> >> > The control framework does not support private controls, for example. The >> >> >> >> > controls should be put to their own class in videodev2.h nowadays, that's my >> >> >> >> > understanding. Cc Hans. >> >> >> >> >> >> >> >> Is this really the case that we close the door for private controls in >> >> >> >> the mainline kernel ? Or am I misunderstanding something ? >> >> >> >> How about v4l2_ctrl_new_custom() ? >> >> >> >> >> >> >> >> What if there are controls applicable to single driver only ? >> >> >> >> Do we really want to have plenty of such in videodev2.h ? >> >> >> > >> >> >> > We have some of those already in videodev2.h. I'm not certain if I'm happy >> >> >> > with this myself, considering e.g. that we could get a few truckloads of >> >> >> > only camera lens hardware specific controls in the near future. >> >> >> >> >> >> So in my case (as these are controls that might be used by others too) >> >> >> I should add something like >> >> >> >> >> >> #define V4L2_CID_BLUE_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+19) >> >> >> #define V4L2_CID_RED_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+20) >> >> > >> >> > What do these two controls do? Do they control gain or something else? >> >> >> >> Hmm. Maybe I named them a bit unsharp. It is the U Saturation and V >> >> Saturation. To me it looks like turning up the saturation in HSV >> >> space, but only for either the blue or the red channel. This would >> >> correspond to V4L2_CID_{RED,BLUE}_BALANCE when I read the docs. They >> >> say it is "{Red,Blue} chroma balance". >> >> >> >> I have other controls for that I used V4L2_CID_{RED,BLUE}_BALANCE. >> >> These are gains. So in fact I should swap them in my code and the >> >> remaining question is, how to name the red and blue gain controls. >> > >> > I think Laurent had a similar issue in his Aptina sensor driver. In my >> > opinion we need a class for low level controls such as the gain ones. Do I >> > understand correctly they control the red and blue pixel gain in the sensor >> > pixel matrix? Do you also have gain controls for the two greens? >> >> Yes, I assume that this is done there. Either in the analog circuit by >> decreasing the preload or digitally then. Don't know exactly. There >> are registers for the green pixels as well. As I used the >> V4L2_CID_{RED,BLUE}_BALANCE controls and there was no >> V4L2_CID_GREEN_BALANCE, I just skipped green as one can >> increase/decrease the global gain and get an arbitrary mix as well. >> >> So for these gain settings we should add these? >> V4L2_CID_RED_GAIN >> V4L2_CID_BLUE_GAIN >> V4L2_CID_GREEN_GAIN > > Do you have two or just one green gains? In all sensors I've seen there are > two. No, here is only one. > I think I could send an RFC on this to the list and cc you and Laurent. Ok fine, thanks! But hmmm - what do I do with my driver in the meantime actually? Stall the upstream process or remove my controls temporarily - or is there a better way? >> >> >> #define V4L2_CID_GRAY_SCALE_IMAGE (V4L2_CID_CAMERA_CLASS_BASE+21) >> >> > >> >> > V4L2_CID_COLOR_KILLER looks like something which would fit for the purpose. >> >> >> >> Oh great! So I just take this. >> >> >> >> >> #define V4L2_CID_SOLARIZE_EFFECT (V4L2_CID_CAMERA_CLASS_BASE+22) >> >> > >> >> > Sounds interesting for a sensor. I wonder if this would fall under a menu >> >> > control, V4L2_CID_COLORFX. >> >> >> >> When I read the the possible enums for V4L2_CID_COLORFX, it indeed >> >> sounds very much like my solarize effect should be added there too. I >> >> found V4L2_COLORFX_BW there, too. Isn't that a duplicate of the color >> >> killer control then? >> > >> > In my opinion V4L2_CID_COLORFX should never be implemented in drivers for >> > which the hardware doesn't implement these effects in a non-parametrisable >> > way. This control was originally added for the OMAP 3 ISP driver but the >> > driver never implemented it. >> >> Your triple negation (never, doesn't, non-) is quite tricky xD >> If I get it right, you say that one should not use V4L2_CID_COLORFX >> for hardware with parametrisable effects. > > Yes. I could have written that in a more clear way. ;-) After starring dazzled for 2 minutes on it, I realized at some point that formal logic is your friend ;) >> My BW and Solarize effects are non-parametrisable and they can be >> turned on together (which makes not so much sense though - but these >> fun-effects like "solarize" aren't here to make sense, I guess :-) ). > > Good. > > The OMAP 3 ISP actually provides a way to set gamma tables, any effects > implemented using them are more or less use case specific. There are also > other uses for those same gamma tables, making a driver implementation for > effects using them non-functional in practice. Ok I see. Luckily (for me) in my sensor it is binary on/off only. >> > I think you have a valid case using this control. I think the main >> > difference between the two is that V4L2_COLORFX_BW is something that you >> > can't use with other effects while V4L2_CID_COLOR_KILLER can be used with >> > any of the effects. >> >> > Based on your original proposal the black/white should stay as a separate >> > control but the solarise should be configurable through V4L2_CID_COLORFX >> > menu control. So it boils down to the question whether you can use them at >> > the same time. >> >> I can - so it is still working to enable V4L2_COLORFX_BW and >> V4L2_CID_COLORFX with a new enum value, right? Is that the way to go >> now? > > That's my opinion, yes. So I will post an additional patch for videodev2.h with enum v4l2_colorfx { ... V4L2_COLORFX_SOLARIZE = 10, }; > -- > Sakari Ailus > e-mail: sakari.ailus@iki.fi jabber/XMPP/Gmail: sailus@retiisi.org.uk > Thanks, Bastian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 06, 2011 at 09:35:24AM +0000, Bastian Hecht wrote: > Hello Sakari, > > 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: > > On Tue, Sep 06, 2011 at 09:01:15AM +0000, Bastian Hecht wrote: > >> 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: > >> > On Tue, Sep 06, 2011 at 07:56:40AM +0000, Bastian Hecht wrote: > >> >> Hello Sakari! > >> > > >> > Hi Bastian, > >> > > >> >> 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: > >> >> > Hi Bastian, > >> >> > > >> >> > On Mon, Sep 05, 2011 at 09:32:55AM +0000, Bastian Hecht wrote: > >> >> >> 2011/9/1 Sakari Ailus <sakari.ailus@iki.fi>: > >> >> >> > On Thu, Sep 01, 2011 at 11:14:08AM +0200, Sylwester Nawrocki wrote: > >> >> >> >> Hi Sakari, > >> >> >> >> > >> >> >> >> On 09/01/2011 10:47 AM, Sakari Ailus wrote: > >> >> >> >> > On Thu, Sep 01, 2011 at 09:15:20AM +0200, Guennadi Liakhovetski wrote: > >> >> >> >> >> On Thu, 1 Sep 2011, Sakari Ailus wrote: > >> >> >> >> >> > >> >> >> >> >>> On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: > >> >> >> >> >>>> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > >> >> >> >> >>> [clip] > >> >> >> >> >>>>> If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. > >> >> >> >> >>>> > >> >> >> >> >>>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled > >> >> >> >> >>>> "V4L2_CID_PRIVATE_BASE deprecated" and read > >> >> >> >> >>>> Documentation/feature-removal-schedule.txt. I couldn't find anything. > >> >> >> >> >>> > >> >> >> >> >>> Hmm. Did you happen to check when that has been written? :) > >> >> >> >> >>> > >> >> >> >> >>> Please use this one instead: > >> >> >> >> >>> > >> >> >> >> >>> <URL:http://hverkuil.home.xs4all.nl/spec/media.html> > >> >> >> >> >> > >> >> >> >> >> "Drivers can also implement their own custom controls using > >> >> >> >> >> V4L2_CID_PRIVATE_BASE and higher values." > >> >> >> >> >> > >> >> >> >> >> Which specific location describes V4L2_CID_PRIVATE_BASE differently there? > >> >> >> >> > > >> >> >> >> > That was a general comment, not related to the private base. There's no > >> >> >> >> > use for a three-year-old spec as a reference! > >> >> >> >> > > >> >> >> >> > The control framework does not support private controls, for example. The > >> >> >> >> > controls should be put to their own class in videodev2.h nowadays, that's my > >> >> >> >> > understanding. Cc Hans. > >> >> >> >> > >> >> >> >> Is this really the case that we close the door for private controls in > >> >> >> >> the mainline kernel ? Or am I misunderstanding something ? > >> >> >> >> How about v4l2_ctrl_new_custom() ? > >> >> >> >> > >> >> >> >> What if there are controls applicable to single driver only ? > >> >> >> >> Do we really want to have plenty of such in videodev2.h ? > >> >> >> > > >> >> >> > We have some of those already in videodev2.h. I'm not certain if I'm happy > >> >> >> > with this myself, considering e.g. that we could get a few truckloads of > >> >> >> > only camera lens hardware specific controls in the near future. > >> >> >> > >> >> >> So in my case (as these are controls that might be used by others too) > >> >> >> I should add something like > >> >> >> > >> >> >> #define V4L2_CID_BLUE_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+19) > >> >> >> #define V4L2_CID_RED_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+20) > >> >> > > >> >> > What do these two controls do? Do they control gain or something else? > >> >> > >> >> Hmm. Maybe I named them a bit unsharp. It is the U Saturation and V > >> >> Saturation. To me it looks like turning up the saturation in HSV > >> >> space, but only for either the blue or the red channel. This would > >> >> correspond to V4L2_CID_{RED,BLUE}_BALANCE when I read the docs. They > >> >> say it is "{Red,Blue} chroma balance". > >> >> > >> >> I have other controls for that I used V4L2_CID_{RED,BLUE}_BALANCE. > >> >> These are gains. So in fact I should swap them in my code and the > >> >> remaining question is, how to name the red and blue gain controls. > >> > > >> > I think Laurent had a similar issue in his Aptina sensor driver. In my > >> > opinion we need a class for low level controls such as the gain ones. Do I > >> > understand correctly they control the red and blue pixel gain in the sensor > >> > pixel matrix? Do you also have gain controls for the two greens? > >> > >> Yes, I assume that this is done there. Either in the analog circuit by > >> decreasing the preload or digitally then. Don't know exactly. There > >> are registers for the green pixels as well. As I used the > >> V4L2_CID_{RED,BLUE}_BALANCE controls and there was no > >> V4L2_CID_GREEN_BALANCE, I just skipped green as one can > >> increase/decrease the global gain and get an arbitrary mix as well. > >> > >> So for these gain settings we should add these? > >> V4L2_CID_RED_GAIN > >> V4L2_CID_BLUE_GAIN > >> V4L2_CID_GREEN_GAIN > > > > Do you have two or just one green gains? In all sensors I've seen there are > > two. > > No, here is only one. It is a raw bayer sensor, isn't it? > > I think I could send an RFC on this to the list and cc you and Laurent. > > Ok fine, thanks! But hmmm - what do I do with my driver in the > meantime actually? Stall the upstream process or remove my controls > temporarily - or is there a better way? It is also possible to expose these controls just for this sensor, but I would wait a little bit if that's okay for you. Your sensor driver isn't the only one depending on these new controls --- Laurent also has one. I don't think this should take too long, but I can't promise that. :-) If you want the driver to mainline fast, then you could also submit it without these controls implemented, and implement them in another patch when the controls have been standardised. > >> >> >> #define V4L2_CID_GRAY_SCALE_IMAGE (V4L2_CID_CAMERA_CLASS_BASE+21) > >> >> > > >> >> > V4L2_CID_COLOR_KILLER looks like something which would fit for the purpose. > >> >> > >> >> Oh great! So I just take this. > >> >> > >> >> >> #define V4L2_CID_SOLARIZE_EFFECT (V4L2_CID_CAMERA_CLASS_BASE+22) > >> >> > > >> >> > Sounds interesting for a sensor. I wonder if this would fall under a menu > >> >> > control, V4L2_CID_COLORFX. > >> >> > >> >> When I read the the possible enums for V4L2_CID_COLORFX, it indeed > >> >> sounds very much like my solarize effect should be added there too. I > >> >> found V4L2_COLORFX_BW there, too. Isn't that a duplicate of the color > >> >> killer control then? > >> > > >> > In my opinion V4L2_CID_COLORFX should never be implemented in drivers for > >> > which the hardware doesn't implement these effects in a non-parametrisable > >> > way. This control was originally added for the OMAP 3 ISP driver but the > >> > driver never implemented it. > >> > >> Your triple negation (never, doesn't, non-) is quite tricky xD > >> If I get it right, you say that one should not use V4L2_CID_COLORFX > >> for hardware with parametrisable effects. > > > > Yes. I could have written that in a more clear way. ;-) > > After starring dazzled for 2 minutes on it, I realized at some point > that formal logic is your friend ;) > > >> My BW and Solarize effects are non-parametrisable and they can be > >> turned on together (which makes not so much sense though - but these > >> fun-effects like "solarize" aren't here to make sense, I guess :-) ). > > > > Good. > > > > The OMAP 3 ISP actually provides a way to set gamma tables, any effects > > implemented using them are more or less use case specific. There are also > > other uses for those same gamma tables, making a driver implementation for > > effects using them non-functional in practice. > > Ok I see. Luckily (for me) in my sensor it is binary on/off only. > > >> > I think you have a valid case using this control. I think the main > >> > difference between the two is that V4L2_COLORFX_BW is something that you > >> > can't use with other effects while V4L2_CID_COLOR_KILLER can be used with > >> > any of the effects. > >> > >> > Based on your original proposal the black/white should stay as a separate > >> > control but the solarise should be configurable through V4L2_CID_COLORFX > >> > menu control. So it boils down to the question whether you can use them at > >> > the same time. > >> > >> I can - so it is still working to enable V4L2_COLORFX_BW and > >> V4L2_CID_COLORFX with a new enum value, right? Is that the way to go > >> now? > > > > That's my opinion, yes. > > So I will post an additional patch for videodev2.h with > enum v4l2_colorfx { > ... > V4L2_COLORFX_SOLARIZE = 10, > }; That's correct. I think the COLORFX hasn't been very well documented so far, but it should be. I don't think many know what the solarize effect would do. :-) I think the patch should also add that. The user control documentation may be found in Documentation/DocBook/media/v4l/controls.xml . Cheers,
Hello Sakari, 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: > On Tue, Sep 06, 2011 at 09:35:24AM +0000, Bastian Hecht wrote: >> Hello Sakari, >> >> 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: >> > On Tue, Sep 06, 2011 at 09:01:15AM +0000, Bastian Hecht wrote: >> >> 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: >> >> > On Tue, Sep 06, 2011 at 07:56:40AM +0000, Bastian Hecht wrote: >> >> >> Hello Sakari! >> >> > >> >> > Hi Bastian, >> >> > >> >> >> 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: >> >> >> > Hi Bastian, >> >> >> > >> >> >> > On Mon, Sep 05, 2011 at 09:32:55AM +0000, Bastian Hecht wrote: >> >> >> >> 2011/9/1 Sakari Ailus <sakari.ailus@iki.fi>: >> >> >> >> > On Thu, Sep 01, 2011 at 11:14:08AM +0200, Sylwester Nawrocki wrote: >> >> >> >> >> Hi Sakari, >> >> >> >> >> >> >> >> >> >> On 09/01/2011 10:47 AM, Sakari Ailus wrote: >> >> >> >> >> > On Thu, Sep 01, 2011 at 09:15:20AM +0200, Guennadi Liakhovetski wrote: >> >> >> >> >> >> On Thu, 1 Sep 2011, Sakari Ailus wrote: >> >> >> >> >> >> >> >> >> >> >> >>> On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: >> >> >> >> >> >>>> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: >> >> >> >> >> >>> [clip] >> >> >> >> >> >>>>> If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. >> >> >> >> >> >>>> >> >> >> >> >> >>>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled >> >> >> >> >> >>>> "V4L2_CID_PRIVATE_BASE deprecated" and read >> >> >> >> >> >>>> Documentation/feature-removal-schedule.txt. I couldn't find anything. >> >> >> >> >> >>> >> >> >> >> >> >>> Hmm. Did you happen to check when that has been written? :) >> >> >> >> >> >>> >> >> >> >> >> >>> Please use this one instead: >> >> >> >> >> >>> >> >> >> >> >> >>> <URL:http://hverkuil.home.xs4all.nl/spec/media.html> >> >> >> >> >> >> >> >> >> >> >> >> "Drivers can also implement their own custom controls using >> >> >> >> >> >> V4L2_CID_PRIVATE_BASE and higher values." >> >> >> >> >> >> >> >> >> >> >> >> Which specific location describes V4L2_CID_PRIVATE_BASE differently there? >> >> >> >> >> > >> >> >> >> >> > That was a general comment, not related to the private base. There's no >> >> >> >> >> > use for a three-year-old spec as a reference! >> >> >> >> >> > >> >> >> >> >> > The control framework does not support private controls, for example. The >> >> >> >> >> > controls should be put to their own class in videodev2.h nowadays, that's my >> >> >> >> >> > understanding. Cc Hans. >> >> >> >> >> >> >> >> >> >> Is this really the case that we close the door for private controls in >> >> >> >> >> the mainline kernel ? Or am I misunderstanding something ? >> >> >> >> >> How about v4l2_ctrl_new_custom() ? >> >> >> >> >> >> >> >> >> >> What if there are controls applicable to single driver only ? >> >> >> >> >> Do we really want to have plenty of such in videodev2.h ? >> >> >> >> > >> >> >> >> > We have some of those already in videodev2.h. I'm not certain if I'm happy >> >> >> >> > with this myself, considering e.g. that we could get a few truckloads of >> >> >> >> > only camera lens hardware specific controls in the near future. >> >> >> >> >> >> >> >> So in my case (as these are controls that might be used by others too) >> >> >> >> I should add something like >> >> >> >> >> >> >> >> #define V4L2_CID_BLUE_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+19) >> >> >> >> #define V4L2_CID_RED_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+20) >> >> >> > >> >> >> > What do these two controls do? Do they control gain or something else? >> >> >> >> >> >> Hmm. Maybe I named them a bit unsharp. It is the U Saturation and V >> >> >> Saturation. To me it looks like turning up the saturation in HSV >> >> >> space, but only for either the blue or the red channel. This would >> >> >> correspond to V4L2_CID_{RED,BLUE}_BALANCE when I read the docs. They >> >> >> say it is "{Red,Blue} chroma balance". >> >> >> >> >> >> I have other controls for that I used V4L2_CID_{RED,BLUE}_BALANCE. >> >> >> These are gains. So in fact I should swap them in my code and the >> >> >> remaining question is, how to name the red and blue gain controls. >> >> > >> >> > I think Laurent had a similar issue in his Aptina sensor driver. In my >> >> > opinion we need a class for low level controls such as the gain ones. Do I >> >> > understand correctly they control the red and blue pixel gain in the sensor >> >> > pixel matrix? Do you also have gain controls for the two greens? >> >> >> >> Yes, I assume that this is done there. Either in the analog circuit by >> >> decreasing the preload or digitally then. Don't know exactly. There >> >> are registers for the green pixels as well. As I used the >> >> V4L2_CID_{RED,BLUE}_BALANCE controls and there was no >> >> V4L2_CID_GREEN_BALANCE, I just skipped green as one can >> >> increase/decrease the global gain and get an arbitrary mix as well. >> >> >> >> So for these gain settings we should add these? >> >> V4L2_CID_RED_GAIN >> >> V4L2_CID_BLUE_GAIN >> >> V4L2_CID_GREEN_GAIN >> > >> > Do you have two or just one green gains? In all sensors I've seen there are >> > two. >> >> No, here is only one. > > It is a raw bayer sensor, isn't it? > >> > I think I could send an RFC on this to the list and cc you and Laurent. >> >> Ok fine, thanks! But hmmm - what do I do with my driver in the >> meantime actually? Stall the upstream process or remove my controls >> temporarily - or is there a better way? > > It is also possible to expose these controls just for this sensor, but I > would wait a little bit if that's okay for you. Your sensor driver isn't the > only one depending on these new controls --- Laurent also has one. > > I don't think this should take too long, but I can't promise that. :-) > > If you want the driver to mainline fast, then you could also submit it > without these controls implemented, and implement them in another patch when > the controls have been standardised. Ok, thanks for the info. I'll try it this way: I'll post an [PATCH/RFC] with the final version except the right control names for gain and chroma balance. So when this rfc gets through, I can simply change names and am done. >> >> >> >> #define V4L2_CID_GRAY_SCALE_IMAGE (V4L2_CID_CAMERA_CLASS_BASE+21) >> >> >> > >> >> >> > V4L2_CID_COLOR_KILLER looks like something which would fit for the purpose. >> >> >> >> >> >> Oh great! So I just take this. >> >> >> >> >> >> >> #define V4L2_CID_SOLARIZE_EFFECT (V4L2_CID_CAMERA_CLASS_BASE+22) >> >> >> > >> >> >> > Sounds interesting for a sensor. I wonder if this would fall under a menu >> >> >> > control, V4L2_CID_COLORFX. >> >> >> >> >> >> When I read the the possible enums for V4L2_CID_COLORFX, it indeed >> >> >> sounds very much like my solarize effect should be added there too. I >> >> >> found V4L2_COLORFX_BW there, too. Isn't that a duplicate of the color >> >> >> killer control then? >> >> > >> >> > In my opinion V4L2_CID_COLORFX should never be implemented in drivers for >> >> > which the hardware doesn't implement these effects in a non-parametrisable >> >> > way. This control was originally added for the OMAP 3 ISP driver but the >> >> > driver never implemented it. >> >> >> >> Your triple negation (never, doesn't, non-) is quite tricky xD >> >> If I get it right, you say that one should not use V4L2_CID_COLORFX >> >> for hardware with parametrisable effects. >> > >> > Yes. I could have written that in a more clear way. ;-) >> >> After starring dazzled for 2 minutes on it, I realized at some point >> that formal logic is your friend ;) >> >> >> My BW and Solarize effects are non-parametrisable and they can be >> >> turned on together (which makes not so much sense though - but these >> >> fun-effects like "solarize" aren't here to make sense, I guess :-) ). >> > >> > Good. >> > >> > The OMAP 3 ISP actually provides a way to set gamma tables, any effects >> > implemented using them are more or less use case specific. There are also >> > other uses for those same gamma tables, making a driver implementation for >> > effects using them non-functional in practice. >> >> Ok I see. Luckily (for me) in my sensor it is binary on/off only. >> >> >> > I think you have a valid case using this control. I think the main >> >> > difference between the two is that V4L2_COLORFX_BW is something that you >> >> > can't use with other effects while V4L2_CID_COLOR_KILLER can be used with >> >> > any of the effects. >> >> >> >> > Based on your original proposal the black/white should stay as a separate >> >> > control but the solarise should be configurable through V4L2_CID_COLORFX >> >> > menu control. So it boils down to the question whether you can use them at >> >> > the same time. >> >> >> >> I can - so it is still working to enable V4L2_COLORFX_BW and >> >> V4L2_CID_COLORFX with a new enum value, right? Is that the way to go >> >> now? >> > >> > That's my opinion, yes. >> >> So I will post an additional patch for videodev2.h with >> enum v4l2_colorfx { >> ... >> V4L2_COLORFX_SOLARIZE = 10, >> }; > > That's correct. I think the COLORFX hasn't been very well documented so far, > but it should be. I don't think many know what the solarize effect would do. > :-) I think the patch should also add that. The user control documentation > may be found in Documentation/DocBook/media/v4l/controls.xml . > > Cheers, In fact, I had no idea as well, but wikipedia teached me :) So I will prepare a documentation patch as well. http://en.wikipedia.org/wiki/Solarisation > -- > Sakari Ailus > e-mail: sakari.ailus@iki.fi jabber/XMPP/Gmail: sailus@retiisi.org.uk > best, Bastian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 06, 2011 at 01:03:57PM +0000, Bastian Hecht wrote: > Hello Sakari, > > 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: > > On Tue, Sep 06, 2011 at 09:35:24AM +0000, Bastian Hecht wrote: > >> Hello Sakari, > >> > >> 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: > >> > On Tue, Sep 06, 2011 at 09:01:15AM +0000, Bastian Hecht wrote: > >> >> 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: > >> >> > On Tue, Sep 06, 2011 at 07:56:40AM +0000, Bastian Hecht wrote: > >> >> >> Hello Sakari! > >> >> > > >> >> > Hi Bastian, > >> >> > > >> >> >> 2011/9/6 Sakari Ailus <sakari.ailus@iki.fi>: > >> >> >> > Hi Bastian, > >> >> >> > > >> >> >> > On Mon, Sep 05, 2011 at 09:32:55AM +0000, Bastian Hecht wrote: > >> >> >> >> 2011/9/1 Sakari Ailus <sakari.ailus@iki.fi>: > >> >> >> >> > On Thu, Sep 01, 2011 at 11:14:08AM +0200, Sylwester Nawrocki wrote: > >> >> >> >> >> Hi Sakari, > >> >> >> >> >> > >> >> >> >> >> On 09/01/2011 10:47 AM, Sakari Ailus wrote: > >> >> >> >> >> > On Thu, Sep 01, 2011 at 09:15:20AM +0200, Guennadi Liakhovetski wrote: > >> >> >> >> >> >> On Thu, 1 Sep 2011, Sakari Ailus wrote: > >> >> >> >> >> >> > >> >> >> >> >> >>> On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: > >> >> >> >> >> >>>> 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > >> >> >> >> >> >>> [clip] > >> >> >> >> >> >>>>> If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. > >> >> >> >> >> >>>> > >> >> >> >> >> >>>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled > >> >> >> >> >> >>>> "V4L2_CID_PRIVATE_BASE deprecated" and read > >> >> >> >> >> >>>> Documentation/feature-removal-schedule.txt. I couldn't find anything. > >> >> >> >> >> >>> > >> >> >> >> >> >>> Hmm. Did you happen to check when that has been written? :) > >> >> >> >> >> >>> > >> >> >> >> >> >>> Please use this one instead: > >> >> >> >> >> >>> > >> >> >> >> >> >>> <URL:http://hverkuil.home.xs4all.nl/spec/media.html> > >> >> >> >> >> >> > >> >> >> >> >> >> "Drivers can also implement their own custom controls using > >> >> >> >> >> >> V4L2_CID_PRIVATE_BASE and higher values." > >> >> >> >> >> >> > >> >> >> >> >> >> Which specific location describes V4L2_CID_PRIVATE_BASE differently there? > >> >> >> >> >> > > >> >> >> >> >> > That was a general comment, not related to the private base. There's no > >> >> >> >> >> > use for a three-year-old spec as a reference! > >> >> >> >> >> > > >> >> >> >> >> > The control framework does not support private controls, for example. The > >> >> >> >> >> > controls should be put to their own class in videodev2.h nowadays, that's my > >> >> >> >> >> > understanding. Cc Hans. > >> >> >> >> >> > >> >> >> >> >> Is this really the case that we close the door for private controls in > >> >> >> >> >> the mainline kernel ? Or am I misunderstanding something ? > >> >> >> >> >> How about v4l2_ctrl_new_custom() ? > >> >> >> >> >> > >> >> >> >> >> What if there are controls applicable to single driver only ? > >> >> >> >> >> Do we really want to have plenty of such in videodev2.h ? > >> >> >> >> > > >> >> >> >> > We have some of those already in videodev2.h. I'm not certain if I'm happy > >> >> >> >> > with this myself, considering e.g. that we could get a few truckloads of > >> >> >> >> > only camera lens hardware specific controls in the near future. > >> >> >> >> > >> >> >> >> So in my case (as these are controls that might be used by others too) > >> >> >> >> I should add something like > >> >> >> >> > >> >> >> >> #define V4L2_CID_BLUE_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+19) > >> >> >> >> #define V4L2_CID_RED_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+20) > >> >> >> > > >> >> >> > What do these two controls do? Do they control gain or something else? > >> >> >> > >> >> >> Hmm. Maybe I named them a bit unsharp. It is the U Saturation and V > >> >> >> Saturation. To me it looks like turning up the saturation in HSV > >> >> >> space, but only for either the blue or the red channel. This would > >> >> >> correspond to V4L2_CID_{RED,BLUE}_BALANCE when I read the docs. They > >> >> >> say it is "{Red,Blue} chroma balance". > >> >> >> > >> >> >> I have other controls for that I used V4L2_CID_{RED,BLUE}_BALANCE. > >> >> >> These are gains. So in fact I should swap them in my code and the > >> >> >> remaining question is, how to name the red and blue gain controls. > >> >> > > >> >> > I think Laurent had a similar issue in his Aptina sensor driver. In my > >> >> > opinion we need a class for low level controls such as the gain ones. Do I > >> >> > understand correctly they control the red and blue pixel gain in the sensor > >> >> > pixel matrix? Do you also have gain controls for the two greens? > >> >> > >> >> Yes, I assume that this is done there. Either in the analog circuit by > >> >> decreasing the preload or digitally then. Don't know exactly. There > >> >> are registers for the green pixels as well. As I used the > >> >> V4L2_CID_{RED,BLUE}_BALANCE controls and there was no > >> >> V4L2_CID_GREEN_BALANCE, I just skipped green as one can > >> >> increase/decrease the global gain and get an arbitrary mix as well. > >> >> > >> >> So for these gain settings we should add these? > >> >> V4L2_CID_RED_GAIN > >> >> V4L2_CID_BLUE_GAIN > >> >> V4L2_CID_GREEN_GAIN > >> > > >> > Do you have two or just one green gains? In all sensors I've seen there are > >> > two. > >> > >> No, here is only one. > > > > It is a raw bayer sensor, isn't it? > > > >> > I think I could send an RFC on this to the list and cc you and Laurent. > >> > >> Ok fine, thanks! But hmmm - what do I do with my driver in the > >> meantime actually? Stall the upstream process or remove my controls > >> temporarily - or is there a better way? > > > > It is also possible to expose these controls just for this sensor, but I > > would wait a little bit if that's okay for you. Your sensor driver isn't the > > only one depending on these new controls --- Laurent also has one. > > > > I don't think this should take too long, but I can't promise that. :-) > > > > If you want the driver to mainline fast, then you could also submit it > > without these controls implemented, and implement them in another patch when > > the controls have been standardised. > > Ok, thanks for the info. I'll try it this way: I'll post an > [PATCH/RFC] with the final version except the right control names for > gain and chroma balance. So when this rfc gets through, I can simply > change names and am done. Sounds good to me. > >> >> >> >> #define V4L2_CID_GRAY_SCALE_IMAGE (V4L2_CID_CAMERA_CLASS_BASE+21) > >> >> >> > > >> >> >> > V4L2_CID_COLOR_KILLER looks like something which would fit for the purpose. > >> >> >> > >> >> >> Oh great! So I just take this. > >> >> >> > >> >> >> >> #define V4L2_CID_SOLARIZE_EFFECT (V4L2_CID_CAMERA_CLASS_BASE+22) > >> >> >> > > >> >> >> > Sounds interesting for a sensor. I wonder if this would fall under a menu > >> >> >> > control, V4L2_CID_COLORFX. > >> >> >> > >> >> >> When I read the the possible enums for V4L2_CID_COLORFX, it indeed > >> >> >> sounds very much like my solarize effect should be added there too. I > >> >> >> found V4L2_COLORFX_BW there, too. Isn't that a duplicate of the color > >> >> >> killer control then? > >> >> > > >> >> > In my opinion V4L2_CID_COLORFX should never be implemented in drivers for > >> >> > which the hardware doesn't implement these effects in a non-parametrisable > >> >> > way. This control was originally added for the OMAP 3 ISP driver but the > >> >> > driver never implemented it. > >> >> > >> >> Your triple negation (never, doesn't, non-) is quite tricky xD > >> >> If I get it right, you say that one should not use V4L2_CID_COLORFX > >> >> for hardware with parametrisable effects. > >> > > >> > Yes. I could have written that in a more clear way. ;-) > >> > >> After starring dazzled for 2 minutes on it, I realized at some point > >> that formal logic is your friend ;) > >> > >> >> My BW and Solarize effects are non-parametrisable and they can be > >> >> turned on together (which makes not so much sense though - but these > >> >> fun-effects like "solarize" aren't here to make sense, I guess :-) ). > >> > > >> > Good. > >> > > >> > The OMAP 3 ISP actually provides a way to set gamma tables, any effects > >> > implemented using them are more or less use case specific. There are also > >> > other uses for those same gamma tables, making a driver implementation for > >> > effects using them non-functional in practice. > >> > >> Ok I see. Luckily (for me) in my sensor it is binary on/off only. > >> > >> >> > I think you have a valid case using this control. I think the main > >> >> > difference between the two is that V4L2_COLORFX_BW is something that you > >> >> > can't use with other effects while V4L2_CID_COLOR_KILLER can be used with > >> >> > any of the effects. > >> >> > >> >> > Based on your original proposal the black/white should stay as a separate > >> >> > control but the solarise should be configurable through V4L2_CID_COLORFX > >> >> > menu control. So it boils down to the question whether you can use them at > >> >> > the same time. > >> >> > >> >> I can - so it is still working to enable V4L2_COLORFX_BW and > >> >> V4L2_CID_COLORFX with a new enum value, right? Is that the way to go > >> >> now? > >> > > >> > That's my opinion, yes. > >> > >> So I will post an additional patch for videodev2.h with > >> enum v4l2_colorfx { > >> ... > >> V4L2_COLORFX_SOLARIZE = 10, > >> }; > > > > That's correct. I think the COLORFX hasn't been very well documented so far, > > but it should be. I don't think many know what the solarize effect would do. > > :-) I think the patch should also add that. The user control documentation > > may be found in Documentation/DocBook/media/v4l/controls.xml . > > > > Cheers, > > In fact, I had no idea as well, but wikipedia teached me :) > So I will prepare a documentation patch as well. > http://en.wikipedia.org/wiki/Solarisation Nice. I didn't know that. :-)
diff --git a/drivers/media/video/ov5642.c b/drivers/media/video/ov5642.c index 1b40d90..069a720 100644 --- a/drivers/media/video/ov5642.c +++ b/drivers/media/video/ov5642.c @@ -74,6 +74,34 @@ #define REG_AVG_WINDOW_END_Y_HIGH 0x5686 #define REG_AVG_WINDOW_END_Y_LOW 0x5687 +/* default values in native space */ +#define DEFAULT_RBBALANCE 0x400 +#define DEFAULT_CONTRAST 0x20 +#define DEFAULT_SATURATION 0x40 + +#define MAX_EXP_NATIVE 0x01ffff +#define MAX_GAIN_NATIVE 0x1f +#define MAX_RBBALANCE_NATIVE 0x0fff +#define MAX_EXP 0xffff +#define MAX_GAIN 0xff +#define MAX_RBBALANCE 0xff +#define MAX_HUE_TRIG_NATIVE 0x80 + +#define OV5642_CONTROL_BLUE_SATURATION (V4L2_CID_PRIVATE_BASE + 0) +#define OV5642_CONTROL_RED_SATURATION (V4L2_CID_PRIVATE_BASE + 1) +#define OV5642_CONTROL_GRAY_SCALE (V4L2_CID_PRIVATE_BASE + 2) +#define OV5642_CONTROL_SOLARIZE (V4L2_CID_PRIVATE_BASE + 3) + +#define EXP_V4L2_TO_NATIVE(x) ((x) << 4) +#define EXP_NATIVE_TO_V4L2(x) ((x) >> 4) +#define GAIN_V4L2_TO_NATIVE(x) ((x) * MAX_GAIN_NATIVE / MAX_GAIN) +#define GAIN_NATIVE_TO_V4L2(x) ((x) * MAX_GAIN / MAX_GAIN_NATIVE) +#define RBBALANCE_V4L2_TO_NATIVE(x) ((x) * MAX_RBBALANCE_NATIVE / MAX_RBBALANCE) +#define RBBALANCE_NATIVE_TO_V4L2(x) ((x) * MAX_RBBALANCE / MAX_RBBALANCE_NATIVE) + +/* flaw in the datasheet. we need some extra lines */ +#define MANUAL_LONG_EXP_SAFETY_DISTANCE 20 + /* active pixel array size */ #define OV5642_SENSOR_SIZE_X 2592 #define OV5642_SENSOR_SIZE_Y 1944 @@ -109,6 +137,9 @@ * 1:1 scale. Hopefully these restrictions will be removed in the future. */ +static int ov5642_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl); +static int ov5642_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl); + struct regval_list { u16 reg_num; u8 value; @@ -608,6 +639,145 @@ static struct regval_list ov5642_default_regs_finalise[] = { { 0xffff, 0xff }, }; +static const struct v4l2_queryctrl ov5642_controls[] = { + { + .id = V4L2_CID_AUTOGAIN, + .type = V4L2_CTRL_TYPE_BOOLEAN, + .name = "Automatic Gain Control", + .minimum = 0, + .maximum = 1, + .step = 1, + .default_value = 1, + }, + { + .id = V4L2_CID_GAIN, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Gain", + .minimum = 0, + .maximum = MAX_GAIN, + .step = 1, + .default_value = 0, + }, + { + .id = V4L2_CID_AUTO_WHITE_BALANCE, + .type = V4L2_CTRL_TYPE_BOOLEAN, + .name = "Automatic White Balance", + .minimum = 0, + .maximum = 1, + .step = 1, + .default_value = 1, + }, + { + .id = V4L2_CID_BLUE_BALANCE, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Blue Balance", + .minimum = 0, + .maximum = MAX_RBBALANCE, + .step = 1, + .default_value = DEFAULT_RBBALANCE, + }, + { + .id = V4L2_CID_RED_BALANCE, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Red Balance", + .minimum = 0, + .maximum = MAX_RBBALANCE, + .step = 1, + .default_value = DEFAULT_RBBALANCE, + }, + { + .id = V4L2_CID_EXPOSURE_AUTO, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Automatic Exposure Control", + .minimum = 0, + .maximum = 1, + .step = 1, + .default_value = V4L2_EXPOSURE_AUTO, + }, + { + .id = V4L2_CID_EXPOSURE, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Exposure", + .minimum = 0, + .maximum = MAX_EXP, + .step = 1, + .default_value = 0, + }, + /* vflip works out of the box. hflip doesn't work. */ + { + .id = V4L2_CID_VFLIP, + .type = V4L2_CTRL_TYPE_BOOLEAN, + .name = "Flip Vertically", + .minimum = 0, + .maximum = 1, + .step = 1, + .default_value = 0, + }, + { + .id = V4L2_CID_BRIGHTNESS, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Brightness", + .minimum = 0, + .maximum = 255, + .step = 1, + .default_value = 0, + }, + { + .id = V4L2_CID_CONTRAST, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Contrast", + .minimum = 0, + .maximum = 255, + .step = 1, + .default_value = DEFAULT_CONTRAST, + }, + { + .id = OV5642_CONTROL_BLUE_SATURATION, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Blue Saturation", + .minimum = 0, + .maximum = 255, + .step = 1, + .default_value = DEFAULT_SATURATION, + }, + { + .id = OV5642_CONTROL_RED_SATURATION, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Red Saturation", + .minimum = 0, + .maximum = 255, + .step = 1, + .default_value = DEFAULT_SATURATION, + }, + { + .id = V4L2_CID_HUE, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Hue", + .minimum = -180, + .maximum = 175, + .step = 5, + .default_value = 0, + }, + { + .id = OV5642_CONTROL_SOLARIZE, + .type = V4L2_CTRL_TYPE_BOOLEAN, + .name = "Solarize", + .minimum = 0, + .maximum = 1, + .step = 1, + .default_value = 0, + }, + { + .id = OV5642_CONTROL_GRAY_SCALE, + .type = V4L2_CTRL_TYPE_BOOLEAN, + .name = "Gray Scale Image", + .minimum = 0, + .maximum = 1, + .step = 1, + .default_value = 0, + }, +}; + struct ov5642_datafmt { enum v4l2_mbus_pixelcode code; enum v4l2_colorspace colorspace; @@ -627,6 +797,24 @@ struct ov5642 { const struct ov5642_datafmt *fmt; struct v4l2_rect crop_rect; struct ov5642_out_size out_size; + + bool agc; + bool awb; + bool aec; + bool vflip; + bool grayscale; + bool solarize; + + /* values in v4l2 space */ + int gain; + int blue_balance; + int red_balance; + int blue_saturation; + int red_saturation; + int exposure; + int brightness; + int contrast; + int hue; }; static const struct ov5642_datafmt ov5642_colour_fmts[] = { @@ -672,6 +860,27 @@ static int reg_read(struct i2c_client *client, u16 reg, u8 *val) return 0; } +/* + * convenience function to read 16 bit register values that are split up + * into two consecutive high and low parts + */ +static int reg_read16(struct i2c_client *client, u16 reg, u16 *val16) +{ + int ret; + u8 val8; + + ret = reg_read(client, reg, &val8); + if (ret) + return ret; + *val16 = val8 << 8; + ret = reg_read(client, reg + 1, &val8); + if (ret) + return ret; + *val16 |= val8; + + return 0; +} + static int reg_write(struct i2c_client *client, u16 reg, u8 val) { int ret; @@ -804,6 +1013,100 @@ static int ov5642_set_resolution(struct v4l2_subdev *sd) return ret; } +static int ov5642_restore_state(struct v4l2_subdev *sd) +{ + struct i2c_client *client = v4l2_get_subdevdata(sd); + struct ov5642 *priv = to_ov5642(client); + struct v4l2_control set_ctrl; + int tmp_red_balance = priv->red_balance; + int tmp_blue_balance = priv->blue_balance; + int tmp_gain = priv->gain; + int tmp_exp = priv->exposure; + int ret; + + set_ctrl.id = V4L2_CID_AUTOGAIN; + set_ctrl.value = priv->agc; + ret = ov5642_s_ctrl(sd, &set_ctrl); + + if (!priv->agc) { + set_ctrl.id = V4L2_CID_GAIN; + set_ctrl.value = tmp_gain; + if (!ret) + ret = ov5642_s_ctrl(sd, &set_ctrl); + } + + set_ctrl.id = V4L2_CID_AUTO_WHITE_BALANCE; + set_ctrl.value = priv->awb; + if (!ret) + ret = ov5642_s_ctrl(sd, &set_ctrl); + + if (!priv->awb) { + set_ctrl.id = V4L2_CID_RED_BALANCE; + set_ctrl.value = tmp_red_balance; + if (!ret) + ret = ov5642_s_ctrl(sd, &set_ctrl); + + set_ctrl.id = V4L2_CID_BLUE_BALANCE; + set_ctrl.value = tmp_blue_balance; + if (!ret) + ret = ov5642_s_ctrl(sd, &set_ctrl); + } + + set_ctrl.id = V4L2_CID_EXPOSURE_AUTO; + set_ctrl.value = priv->aec; + if (!ret) + ret = ov5642_s_ctrl(sd, &set_ctrl); + + if (priv->aec == V4L2_EXPOSURE_MANUAL) { + set_ctrl.id = V4L2_CID_EXPOSURE; + set_ctrl.value = tmp_exp; + if (!ret) + ret = ov5642_s_ctrl(sd, &set_ctrl); + } + + set_ctrl.id = V4L2_CID_VFLIP; + set_ctrl.value = priv->vflip; + if (!ret) + ret = ov5642_s_ctrl(sd, &set_ctrl); + + set_ctrl.id = OV5642_CONTROL_GRAY_SCALE; + set_ctrl.value = priv->grayscale; + if (!ret) + ret = ov5642_s_ctrl(sd, &set_ctrl); + + set_ctrl.id = OV5642_CONTROL_SOLARIZE; + set_ctrl.value = priv->solarize; + if (!ret) + ret = ov5642_s_ctrl(sd, &set_ctrl); + + set_ctrl.id = OV5642_CONTROL_BLUE_SATURATION; + set_ctrl.value = priv->blue_saturation; + if (!ret) + ret = ov5642_s_ctrl(sd, &set_ctrl); + + set_ctrl.id = OV5642_CONTROL_RED_SATURATION; + set_ctrl.value = priv->red_saturation; + if (!ret) + ret = ov5642_s_ctrl(sd, &set_ctrl); + + set_ctrl.id = V4L2_CID_BRIGHTNESS; + set_ctrl.value = priv->brightness; + if (!ret) + ret = ov5642_s_ctrl(sd, &set_ctrl); + + set_ctrl.id = V4L2_CID_CONTRAST; + set_ctrl.value = priv->contrast; + if (!ret) + ret = ov5642_s_ctrl(sd, &set_ctrl); + + set_ctrl.id = V4L2_CID_HUE; + set_ctrl.value = priv->hue; + if (!ret) + ret = ov5642_s_ctrl(sd, &set_ctrl); + + return ret; +} + static int ov5642_try_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) { const struct ov5642_datafmt *fmt = ov5642_find_datafmt(mf->code); @@ -856,6 +1159,9 @@ static int ov5642_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) if (!ret) ret = ov5642_write_array(client, ov5642_default_regs_finalise); + /* the chip has been reset, so configure it again */ + if (!ret) + ret = ov5642_restore_state(sd); return ret; } @@ -904,6 +1210,387 @@ static int ov5642_g_chip_ident(struct v4l2_subdev *sd, return 0; } +/* + * Hue needs trigonometric functions. There is a switch on the ov5642 to use + * angles directly, but this seems to massively increase saturation as well. + * + * The following naive approximate trig functions require an argument + * carefully limited to -180 <= theta <= 180. + * + * Taken directly from ov7670.c + */ +#define SIN_STEP 5 +static const int ov5642_sin_table[] = { + 0, 87, 173, 258, 342, 422, + 499, 573, 642, 707, 766, 819, + 866, 906, 939, 965, 984, 996, + 1000 +}; + +static int ov5642_sin(int theta) +{ + int chs = 1; + int sin; + + if (theta < 0) { + theta = -theta; + chs = -1; + } + if (theta <= 90) + sin = ov5642_sin_table[theta / SIN_STEP]; + else { + theta -= 90; + sin = 1000 - ov5642_sin_table[theta / SIN_STEP]; + } + return sin * chs; +} + +static int ov5642_cosine(int theta) +{ + theta = 90 - theta; + if (theta > 180) + theta -= 360; + else if (theta < -180) + theta += 360; + return ov5642_sin(theta); +} + +static int ov5642_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl) +{ + struct i2c_client *client = v4l2_get_subdevdata(sd); + struct ov5642 *priv = to_ov5642(client); + int ret = 0; + u8 val8; + u16 val16; + u32 val32; + + switch (ctrl->id) { + case V4L2_CID_AUTOGAIN: + ctrl->value = priv->agc; + break; + case V4L2_CID_GAIN: + if (priv->agc) { + ret = reg_read(client, REG_GAIN, &val8); + if (ret) + break; + ctrl->value = GAIN_NATIVE_TO_V4L2(val8); + } else { + ctrl->value = priv->gain; + } + break; + case V4L2_CID_AUTO_WHITE_BALANCE: + ctrl->value = priv->awb; + break; + case V4L2_CID_BLUE_BALANCE: + if (priv->awb) { + ret = reg_read16(client, REG_BLUE_GAIN_HIGH, &val16); + if (!ret) + ctrl->value = RBBALANCE_NATIVE_TO_V4L2(val16); + } else { + ctrl->value = priv->blue_balance; + } + break; + case V4L2_CID_RED_BALANCE: + if (priv->awb) { + ret = reg_read16(client, REG_RED_GAIN_HIGH, &val16); + if (!ret) + ctrl->value = RBBALANCE_NATIVE_TO_V4L2(val16); + } else { + ctrl->value = priv->red_balance; + } + break; + case V4L2_CID_EXPOSURE_AUTO: + ctrl->value = priv->aec; + break; + case V4L2_CID_EXPOSURE: + if (priv->aec == V4L2_EXPOSURE_AUTO) { + ret = reg_read(client, REG_EXP_HIGH, &val8); + if (ret) + break; + val32 = val8 << 16; + ret = reg_read16(client, REG_EXP_MIDDLE, &val16); + if (ret) + break; + val32 |= val16; + ctrl->value = EXP_NATIVE_TO_V4L2(val32); + } else { + ctrl->value = priv->exposure; + } + break; + case V4L2_CID_VFLIP: + ctrl->value = priv->vflip; + break; + case V4L2_CID_BRIGHTNESS: + ctrl->value = priv->brightness; + break; + case V4L2_CID_CONTRAST: + ctrl->value = priv->contrast; + break; + case OV5642_CONTROL_BLUE_SATURATION: + ctrl->value = priv->blue_saturation; + break; + case OV5642_CONTROL_RED_SATURATION: + ctrl->value = priv->red_saturation; + break; + case V4L2_CID_HUE: + ctrl->value = priv->hue; + break; + case OV5642_CONTROL_GRAY_SCALE: + ctrl->value = priv->grayscale; + break; + case OV5642_CONTROL_SOLARIZE: + ctrl->value = priv->solarize; + break; + }; + + return ret; +} + +static int ov5642_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl) +{ + struct i2c_client *client = v4l2_get_subdevdata(sd); + struct ov5642 *priv = to_ov5642(client); + int ret = 0; + u8 val8; + u16 val16; + u32 val32; + int trig; + struct v4l2_control aux_ctrl; + + switch (ctrl->id) { + case V4L2_CID_AUTOGAIN: + if (!ctrl->value) { + aux_ctrl.id = V4L2_CID_GAIN; + ret = ov5642_g_ctrl(sd, &aux_ctrl); + if (ret) + break; + priv->gain = aux_ctrl.value; + } + + ret = reg_read(client, REG_EXP_GAIN_CTRL, &val8); + if (ret) + break; + val8 = ctrl->value ? val8 & ~BIT(1) : val8 | BIT(1); + ret = reg_write(client, REG_EXP_GAIN_CTRL, val8); + if (!ret) + priv->agc = ctrl->value; + break; + case V4L2_CID_GAIN: + /* turn auto function off */ + if (priv->agc) { + aux_ctrl.id = V4L2_CID_AUTOGAIN; + aux_ctrl.value = false; + ret = ov5642_s_ctrl(sd, &aux_ctrl); + if (ret) + break; + } + + val8 = GAIN_V4L2_TO_NATIVE(ctrl->value); + ret = reg_write(client, REG_GAIN, val8); + if (!ret) + priv->gain = ctrl->value; + break; + case V4L2_CID_AUTO_WHITE_BALANCE: + if (!ctrl->value) { + aux_ctrl.id = V4L2_CID_BLUE_BALANCE; + ret = ov5642_g_ctrl(sd, &aux_ctrl); + if (ret) + break; + priv->blue_balance = aux_ctrl.value; + aux_ctrl.id = V4L2_CID_RED_BALANCE; + ret = ov5642_g_ctrl(sd, &aux_ctrl); + if (ret) + break; + priv->red_balance = aux_ctrl.value; + } + ret = reg_write(client, REG_AWB_MANUAL, !ctrl->value); + if (!ret) + priv->awb = ctrl->value; + break; + case V4L2_CID_BLUE_BALANCE: + case V4L2_CID_RED_BALANCE: + /* turn auto function off */ + if (priv->awb) { + aux_ctrl.id = V4L2_CID_AUTO_WHITE_BALANCE; + aux_ctrl.value = false; + ret = ov5642_s_ctrl(sd, &aux_ctrl); + if (ret) + break; + } + val16 = RBBALANCE_V4L2_TO_NATIVE(ctrl->value); + if (ctrl->id == V4L2_CID_BLUE_BALANCE) { + ret = reg_write16(client, REG_BLUE_GAIN_HIGH, val16); + if (!ret) + priv->blue_balance = ctrl->value; + } else { + ret = reg_write16(client, REG_RED_GAIN_HIGH, val16); + if (!ret) + priv->red_balance = ctrl->value; + } + break; + case V4L2_CID_EXPOSURE_AUTO: + if (ctrl->value == V4L2_EXPOSURE_MANUAL) { + aux_ctrl.id = V4L2_CID_EXPOSURE; + ret = ov5642_g_ctrl(sd, &aux_ctrl); + if (ret) + break; + priv->exposure = aux_ctrl.value; + } + ret = reg_read(client, REG_EXP_GAIN_CTRL, &val8); + if (ret) + break; + val8 = ctrl->value == V4L2_EXPOSURE_MANUAL ? val8 | BIT(0) | BIT(2) + : val8 & ~(BIT(0) | BIT(2)); + ret = reg_write(client, REG_EXP_GAIN_CTRL, val8); + if (ret) + break; + priv->aec = ctrl->value; + break; + case V4L2_CID_EXPOSURE: + /* + * the exposure time is given in lines, i.e. how much time is + * needed to transmit 1 line. The register value is divided by + * 16 in the form of xxxx.x. + * So the last nibble (0x3502[0:3]) is the time for a fraction + * of 1 line: x/16 + */ + + /* turn auto function off */ + if (priv->aec) { + aux_ctrl.id = V4L2_CID_EXPOSURE_AUTO; + aux_ctrl.value = V4L2_EXPOSURE_MANUAL; + ret = ov5642_s_ctrl(sd, &aux_ctrl); + if (ret) + break; + } + + val32 = EXP_V4L2_TO_NATIVE(ctrl->value); + + /* + * if we expose longer than 1 frame, we must lower the fps + * by increasing blanking + */ + if (val32 / 16 + MANUAL_LONG_EXP_SAFETY_DISTANCE > + priv->out_size.total_height) { + ret = reg_write16(client, REG_EXTEND_FRAME_TIME_HIGH, + val32 / 16 + BLANKING_EXTRA_HEIGHT); + if (ret) + break; + dev_dbg(sd->v4l2_dev->dev, + "%s: Increasing vert. blanking. Total height: %d.\n", + __func__, val32 / 16 + BLANKING_EXTRA_HEIGHT); + } else if (priv->exposure + MANUAL_LONG_EXP_SAFETY_DISTANCE > + priv->out_size.total_height) { + /* + * if we increased blanking in the past, but lowered the + * exposure now sufficiently, we can go back to normal + * timings + */ + ret = reg_write16(client, REG_EXTEND_FRAME_TIME_HIGH, + priv->out_size.total_height); + if (ret) + break; + dev_dbg(sd->v4l2_dev->dev, + "%s: Resetting to normal frame time\n", __func__); + } + ret = reg_write(client, REG_EXP_HIGH, val32 >> 16); + if (ret) + break; + ret = reg_write16(client, REG_EXP_MIDDLE, val32); + if (!ret) + priv->exposure = ctrl->value; + break; + case V4L2_CID_VFLIP: + ret = reg_read(client, REG_FLIP_SUBSAMPLE, &val8); + if (ret) + break; + val8 = ctrl->value ? val8 | BIT(5) : val8 & ~BIT(5); + ret = reg_write(client, REG_FLIP_SUBSAMPLE, val8); + if (!ret) + priv->vflip = ctrl->value; + break; + case V4L2_CID_BRIGHTNESS: + ret = reg_write(client, REG_BRIGHTNESS, ctrl->value); + if (!ret) + priv->brightness = ctrl->value; + break; + case V4L2_CID_CONTRAST: + ret = reg_write(client, REG_CONTRAST, ctrl->value); + if (!ret) + priv->contrast = ctrl->value; + break; + case OV5642_CONTROL_BLUE_SATURATION: + ret = reg_write(client, REG_BLUE_SATURATION, ctrl->value); + if (!ret) + priv->blue_saturation = ctrl->value; + break; + case OV5642_CONTROL_RED_SATURATION: + ret = reg_write(client, REG_RED_SATURATION, ctrl->value); + if (!ret) + priv->red_saturation = ctrl->value; + break; + case V4L2_CID_HUE: + ret = reg_read(client, REG_DIGITAL_EFFECTS, &val8); + if (ret) + break; + val8 = ctrl->value != 0 ? val8 | BIT(0) : val8 & ~BIT(0); + ret = reg_write(client, REG_DIGITAL_EFFECTS, val8); + if (ret) + break; + + if (!ctrl->value) { + priv->hue = 0; + break; + } else { + trig = ov5642_sin(ctrl->value) * + MAX_HUE_TRIG_NATIVE / 1000; + ret = reg_write(client, REG_HUE_SIN, abs(trig)); + if (ret) + break; + ret = reg_read(client, REG_D_E_AUXILLARY, &val8); + if (ret) + break; + /* determine bits for sine signs */ + val8 = trig < 0 ? (val8 & ~BIT(0)) | BIT(1) : + (val8 | BIT(0)) & ~BIT(1); + + trig = ov5642_cosine(ctrl->value) * + MAX_HUE_TRIG_NATIVE / 1000; + ret = reg_write(client, REG_HUE_COS, abs(trig)); + + /* determine bits for cosine signs */ + val8 = trig < 0 ? val8 | BIT(4) | BIT(5) : + val8 & ~(BIT(4) | BIT(5)); + + ret = reg_write(client, REG_D_E_AUXILLARY, val8); + if (ret) + break; + + priv->hue = ctrl->value; + break; + } + case OV5642_CONTROL_GRAY_SCALE: + ret = reg_read(client, REG_DIGITAL_EFFECTS, &val8); + if (ret) + break; + val8 = ctrl->value ? val8 | BIT(5) : val8 & ~BIT(5); + ret = reg_write(client, REG_DIGITAL_EFFECTS, val8); + if (!ret) + priv->grayscale = ctrl->value; + break; + case OV5642_CONTROL_SOLARIZE: + ret = reg_read(client, REG_D_E_AUXILLARY, &val8); + if (ret) + break; + val8 = ctrl->value ? val8 | BIT(7) : val8 & ~BIT(7); + ret = reg_write(client, REG_D_E_AUXILLARY, val8); + if (!ret) + priv->solarize = ctrl->value; + break; + } + return ret; +} + static int ov5642_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a) { struct i2c_client *client = v4l2_get_subdevdata(sd); @@ -956,6 +1643,8 @@ static struct v4l2_subdev_video_ops ov5642_subdev_video_ops = { static struct v4l2_subdev_core_ops ov5642_subdev_core_ops = { .g_chip_ident = ov5642_g_chip_ident, + .g_ctrl = ov5642_g_ctrl, + .s_ctrl = ov5642_s_ctrl, #ifdef CONFIG_VIDEO_ADV_DEBUG .g_register = ov5642_get_register, .s_register = ov5642_set_register, @@ -967,6 +1656,11 @@ static struct v4l2_subdev_ops ov5642_subdev_ops = { .video = &ov5642_subdev_video_ops, }; +static struct soc_camera_ops soc_ov5642_ops = { + .controls = ov5642_controls, + .num_controls = ARRAY_SIZE(ov5642_controls), +}; + static int ov5642_video_probe(struct soc_camera_device *icd, struct i2c_client *client) { @@ -1020,7 +1714,7 @@ static int ov5642_probe(struct i2c_client *client, v4l2_i2c_subdev_init(&priv->subdev, client, &ov5642_subdev_ops); - icd->ops = NULL; + icd->ops = &soc_ov5642_ops; priv->fmt = &ov5642_colour_fmts[0]; priv->crop_rect.width = OV5642_DEFAULT_WIDTH; @@ -1030,6 +1724,15 @@ static int ov5642_probe(struct i2c_client *client, priv->out_size.width = OV5642_DEFAULT_WIDTH; priv->out_size.height = OV5642_DEFAULT_HEIGHT; + priv->aec = V4L2_EXPOSURE_AUTO; + priv->agc = true; + priv->awb = true; + priv->blue_balance = RBBALANCE_NATIVE_TO_V4L2(DEFAULT_RBBALANCE); + priv->red_balance = RBBALANCE_NATIVE_TO_V4L2(DEFAULT_RBBALANCE); + priv->contrast = DEFAULT_CONTRAST; + priv->blue_saturation = DEFAULT_SATURATION; + priv->red_saturation = DEFAULT_SATURATION; + ret = ov5642_video_probe(icd, client); if (ret < 0) goto error;
The driver now supports automatic/manual gain, automatic/manual white balance, automatic/manual exposure control, vertical flip, brightness control, contrast control and saturation control. Additionally the following effects are available now: rotating the hue in the colorspace, gray scale image and solarize effect. Signed-off-by: Bastian Hecht <hechtb@gmail.com> --- -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html