| Message ID | 20201007084557.25843-96-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Em Wed, 7 Oct 2020 11:45:56 +0300 Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > Add controls for supporting lens shading correction. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> For patches 098 to 105, we should at least have those new controls documented at the uAPI documents. I'm not convinced yet that we shouldn't instead place them inside V4L2_CTRL_CLASS_CAMERA. As those are part of a MIPI standard, I won't doubt that sooner or later, other drivers may need them. Regards, Mauro > --- > drivers/media/i2c/ccs/ccs-core.c | 74 ++++++++++++++++++++++++++++++++ > 1 file changed, 74 insertions(+) > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > index 0ba06a580951..10ed3d01af16 100644 > --- a/drivers/media/i2c/ccs/ccs-core.c > +++ b/drivers/media/i2c/ccs/ccs-core.c > @@ -757,6 +757,25 @@ static int ccs_set_ctrl(struct v4l2_ctrl *ctrl) > case V4L2_CID_TEST_PATTERN_GREENB: > rval = ccs_write(sensor, TEST_DATA_GREENB, ctrl->val); > > + break; > + case V4L2_CID_CCS_SHADING_CORRECTION: > + if (!(CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > + (CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING | > + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION))) > + break; > + > + rval = ccs_write(sensor, SHADING_CORRECTION_EN, > + ctrl->val ? CCS_SHADING_CORRECTION_EN_ENABLE : > + 0); > + > + break; > + case V4L2_CID_CCS_LUMINANCE_SHADING_CORRECTION: > + if (!(CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION)) > + break; > + > + rval = ccs_write(sensor, LUMINANCE_CORRECTION_LEVEL, ctrl->val); > + > break; > case V4L2_CID_PIXEL_RATE: > /* For v4l2_ctrl_s_ctrl_int64() used internally. */ > @@ -878,6 +897,61 @@ static int ccs_init_controls(struct ccs_sensor *sensor) > } > } > > + if (CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > + CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING) { > + const struct v4l2_ctrl_config ctrl_cfg = { > + .name = "Shading Correction", > + .type = V4L2_CTRL_TYPE_BOOLEAN, > + .id = V4L2_CID_CCS_SHADING_CORRECTION, > + .ops = &ccs_ctrl_ops, > + .max = 1, > + .step = 1, > + }; > + > + v4l2_ctrl_new_custom(&sensor->pixel_array->ctrl_handler, > + &ctrl_cfg, NULL); > + } > + > + if (CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION) { > + const struct v4l2_ctrl_config ctrl_cfg = { > + .name = "Luminance Shading Correction", > + .type = V4L2_CTRL_TYPE_BOOLEAN, > + .id = V4L2_CID_CCS_LUMINANCE_SHADING_CORRECTION, > + .ops = &ccs_ctrl_ops, > + .max = 255, > + .step = 1, > + .def = 128, > + }; > + > + v4l2_ctrl_new_custom(&sensor->pixel_array->ctrl_handler, > + &ctrl_cfg, NULL); > + } > + > + if (CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > + (CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING | > + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION)) { > + u32 val = > + ((CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > + CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING) ? > + V4L2_CCS_SHADING_CORRECTION_COLOUR : 0) | > + ((CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION) ? > + V4L2_CCS_SHADING_CORRECTION_LUMINANCE : 0); > + const struct v4l2_ctrl_config ctrl_cfg = { > + .name = "Shading Correction Capability", > + .type = V4L2_CTRL_TYPE_BITMASK, > + .id = V4L2_CID_CCS_SHADING_CORRECTION_CAPABILITY, > + .ops = &ccs_ctrl_ops, > + .max = val, > + .def = val, > + .flags = V4L2_CTRL_FLAG_READ_ONLY, > + }; > + > + v4l2_ctrl_new_custom(&sensor->pixel_array->ctrl_handler, > + &ctrl_cfg, NULL); > + } > + > if (CCS_LIM(sensor, DIGITAL_GAIN_CAPABILITY) == > CCS_DIGITAL_GAIN_CAPABILITY_GLOBAL || > CCS_LIM(sensor, DIGITAL_GAIN_CAPABILITY) == Thanks, Mauro
On 07/10/2020 10:45, Sakari Ailus wrote: > Add controls for supporting lens shading correction. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/i2c/ccs/ccs-core.c | 74 ++++++++++++++++++++++++++++++++ > 1 file changed, 74 insertions(+) > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > index 0ba06a580951..10ed3d01af16 100644 > --- a/drivers/media/i2c/ccs/ccs-core.c > +++ b/drivers/media/i2c/ccs/ccs-core.c > @@ -757,6 +757,25 @@ static int ccs_set_ctrl(struct v4l2_ctrl *ctrl) > case V4L2_CID_TEST_PATTERN_GREENB: > rval = ccs_write(sensor, TEST_DATA_GREENB, ctrl->val); > > + break; > + case V4L2_CID_CCS_SHADING_CORRECTION: > + if (!(CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > + (CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING | > + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION))) > + break; > + > + rval = ccs_write(sensor, SHADING_CORRECTION_EN, > + ctrl->val ? CCS_SHADING_CORRECTION_EN_ENABLE : > + 0); > + > + break; > + case V4L2_CID_CCS_LUMINANCE_SHADING_CORRECTION: > + if (!(CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION)) > + break; > + > + rval = ccs_write(sensor, LUMINANCE_CORRECTION_LEVEL, ctrl->val); > + > break; > case V4L2_CID_PIXEL_RATE: > /* For v4l2_ctrl_s_ctrl_int64() used internally. */ > @@ -878,6 +897,61 @@ static int ccs_init_controls(struct ccs_sensor *sensor) > } > } > > + if (CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > + CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING) { > + const struct v4l2_ctrl_config ctrl_cfg = { Can be static. > + .name = "Shading Correction", Should this perhaps be called "Chroma Shading Correction"? Since there is a luminance shading correction as well... Since these controls are all CCS specific, should the names of these controls begin with "CCS "? (Not just the controls in this patch, but also from previous patches). > + .type = V4L2_CTRL_TYPE_BOOLEAN, > + .id = V4L2_CID_CCS_SHADING_CORRECTION, > + .ops = &ccs_ctrl_ops, > + .max = 1, > + .step = 1, > + }; > + > + v4l2_ctrl_new_custom(&sensor->pixel_array->ctrl_handler, > + &ctrl_cfg, NULL); > + } > + > + if (CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION) { > + const struct v4l2_ctrl_config ctrl_cfg = { Can be static. > + .name = "Luminance Shading Correction", > + .type = V4L2_CTRL_TYPE_BOOLEAN, > + .id = V4L2_CID_CCS_LUMINANCE_SHADING_CORRECTION, > + .ops = &ccs_ctrl_ops, > + .max = 255, > + .step = 1, > + .def = 128, > + }; > + > + v4l2_ctrl_new_custom(&sensor->pixel_array->ctrl_handler, > + &ctrl_cfg, NULL); > + } > + > + if (CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > + (CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING | > + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION)) { > + u32 val = > + ((CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > + CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING) ? > + V4L2_CCS_SHADING_CORRECTION_COLOUR : 0) | > + ((CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION) ? > + V4L2_CCS_SHADING_CORRECTION_LUMINANCE : 0); > + const struct v4l2_ctrl_config ctrl_cfg = { > + .name = "Shading Correction Capability", > + .type = V4L2_CTRL_TYPE_BITMASK, > + .id = V4L2_CID_CCS_SHADING_CORRECTION_CAPABILITY, > + .ops = &ccs_ctrl_ops, > + .max = val, > + .def = val, > + .flags = V4L2_CTRL_FLAG_READ_ONLY, > + }; Is this needed? If e.g. V4L2_CCS_SHADING_CORRECTION_COLOUR is not supported, then the V4L2_CID_CCS_SHADING_CORRECTION control is simply not created. So calling VIDIOC_QUERYCTRL would simply fail and so indicate that this capability is not present. If it really is needed, then having two bool controls makes more sense because a bitmask is less intuitive. Regards, Hans > + > + v4l2_ctrl_new_custom(&sensor->pixel_array->ctrl_handler, > + &ctrl_cfg, NULL); > + } > + > if (CCS_LIM(sensor, DIGITAL_GAIN_CAPABILITY) == > CCS_DIGITAL_GAIN_CAPABILITY_GLOBAL || > CCS_LIM(sensor, DIGITAL_GAIN_CAPABILITY) == >
Hi Mauro, Thanks for the review comments. On Thu, Nov 05, 2020 at 01:42:43PM +0100, Mauro Carvalho Chehab wrote: > Em Wed, 7 Oct 2020 11:45:56 +0300 > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > > > Add controls for supporting lens shading correction. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > For patches 098 to 105, we should at least have those new controls > documented at the uAPI documents. Agreed. > > I'm not convinced yet that we shouldn't instead place them inside > V4L2_CTRL_CLASS_CAMERA. > > As those are part of a MIPI standard, I won't doubt that sooner or > later, other drivers may need them. They are part of a MIPI standard, but that standard defines a control interface for camera sensors which this driver uses. I don't see a need to implement other drivers for devices this driver already supports. Note that while MIPI standards are originally centered around cross-chip busses, the functionality that is being controlled here is entirely local to the device. Quite a few of the controls are still somehow specific to the device. That said, the same analogue gain model is very likely present on a range of devices even though they are not CCS (or SMIA) compatible, for historical reasons. Perhaps these could be actually made a single array control in the camera control class, with indices defined for the different factors. -- Kind regards, Sakari Ailus
Hi Hans, On Thu, Nov 05, 2020 at 02:03:37PM +0100, Hans Verkuil wrote: > On 07/10/2020 10:45, Sakari Ailus wrote: ... > > + .name = "Luminance Shading Correction", > > + .type = V4L2_CTRL_TYPE_BOOLEAN, > > + .id = V4L2_CID_CCS_LUMINANCE_SHADING_CORRECTION, > > + .ops = &ccs_ctrl_ops, > > + .max = 255, > > + .step = 1, > > + .def = 128, > > + }; > > + > > + v4l2_ctrl_new_custom(&sensor->pixel_array->ctrl_handler, > > + &ctrl_cfg, NULL); > > + } > > + > > + if (CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > > + (CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING | > > + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION)) { > > + u32 val = > > + ((CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > > + CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING) ? > > + V4L2_CCS_SHADING_CORRECTION_COLOUR : 0) | > > + ((CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & > > + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION) ? > > + V4L2_CCS_SHADING_CORRECTION_LUMINANCE : 0); > > + const struct v4l2_ctrl_config ctrl_cfg = { > > + .name = "Shading Correction Capability", > > + .type = V4L2_CTRL_TYPE_BITMASK, > > + .id = V4L2_CID_CCS_SHADING_CORRECTION_CAPABILITY, > > + .ops = &ccs_ctrl_ops, > > + .max = val, > > + .def = val, > > + .flags = V4L2_CTRL_FLAG_READ_ONLY, > > + }; > > Is this needed? If e.g. V4L2_CCS_SHADING_CORRECTION_COLOUR is not supported, > then the V4L2_CID_CCS_SHADING_CORRECTION control is simply not created. > So calling VIDIOC_QUERYCTRL would simply fail and so indicate that this > capability is not present. > > If it really is needed, then having two bool controls makes more sense > because a bitmask is less intuitive. The CCS shading correction support has two capabilities but one bit to control both. Another (luminance correction) has the correction level (buggy in this patch, I'll fix for v4), so two controls aren't enough to tell what is being corrected; is it colour shading or not? Luminance correction level support is revealed by the luminance correction level control as well. So I guess you could also have a boolean control to tell colour correction is supported. I guess could also just omit the capability control now and see if someone needs it. I'd expect this to be rarely needed information. -- Kind regards, Sakari Ailus
diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c index 0ba06a580951..10ed3d01af16 100644 --- a/drivers/media/i2c/ccs/ccs-core.c +++ b/drivers/media/i2c/ccs/ccs-core.c @@ -757,6 +757,25 @@ static int ccs_set_ctrl(struct v4l2_ctrl *ctrl) case V4L2_CID_TEST_PATTERN_GREENB: rval = ccs_write(sensor, TEST_DATA_GREENB, ctrl->val); + break; + case V4L2_CID_CCS_SHADING_CORRECTION: + if (!(CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & + (CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING | + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION))) + break; + + rval = ccs_write(sensor, SHADING_CORRECTION_EN, + ctrl->val ? CCS_SHADING_CORRECTION_EN_ENABLE : + 0); + + break; + case V4L2_CID_CCS_LUMINANCE_SHADING_CORRECTION: + if (!(CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION)) + break; + + rval = ccs_write(sensor, LUMINANCE_CORRECTION_LEVEL, ctrl->val); + break; case V4L2_CID_PIXEL_RATE: /* For v4l2_ctrl_s_ctrl_int64() used internally. */ @@ -878,6 +897,61 @@ static int ccs_init_controls(struct ccs_sensor *sensor) } } + if (CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & + CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING) { + const struct v4l2_ctrl_config ctrl_cfg = { + .name = "Shading Correction", + .type = V4L2_CTRL_TYPE_BOOLEAN, + .id = V4L2_CID_CCS_SHADING_CORRECTION, + .ops = &ccs_ctrl_ops, + .max = 1, + .step = 1, + }; + + v4l2_ctrl_new_custom(&sensor->pixel_array->ctrl_handler, + &ctrl_cfg, NULL); + } + + if (CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION) { + const struct v4l2_ctrl_config ctrl_cfg = { + .name = "Luminance Shading Correction", + .type = V4L2_CTRL_TYPE_BOOLEAN, + .id = V4L2_CID_CCS_LUMINANCE_SHADING_CORRECTION, + .ops = &ccs_ctrl_ops, + .max = 255, + .step = 1, + .def = 128, + }; + + v4l2_ctrl_new_custom(&sensor->pixel_array->ctrl_handler, + &ctrl_cfg, NULL); + } + + if (CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & + (CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING | + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION)) { + u32 val = + ((CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & + CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING) ? + V4L2_CCS_SHADING_CORRECTION_COLOUR : 0) | + ((CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) & + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION) ? + V4L2_CCS_SHADING_CORRECTION_LUMINANCE : 0); + const struct v4l2_ctrl_config ctrl_cfg = { + .name = "Shading Correction Capability", + .type = V4L2_CTRL_TYPE_BITMASK, + .id = V4L2_CID_CCS_SHADING_CORRECTION_CAPABILITY, + .ops = &ccs_ctrl_ops, + .max = val, + .def = val, + .flags = V4L2_CTRL_FLAG_READ_ONLY, + }; + + v4l2_ctrl_new_custom(&sensor->pixel_array->ctrl_handler, + &ctrl_cfg, NULL); + } + if (CCS_LIM(sensor, DIGITAL_GAIN_CAPABILITY) == CCS_DIGITAL_GAIN_CAPABILITY_GLOBAL || CCS_LIM(sensor, DIGITAL_GAIN_CAPABILITY) ==
Add controls for supporting lens shading correction. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/i2c/ccs/ccs-core.c | 74 ++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)