Hi,
On 11/17/2012 01:07 PM, Andrey Gusakov wrote:
Hi.
Just have time to test. I apologize for delay.
No problem, thanks for the feedback.
I'd like to squash all the s3c-camif patches before sending upstream,
if you don't mind. And to add your Signed-off at the final patch.
Ok. Squash.
I might have introduced bugs in the image effects handling, hopefully
there is none. I couldn't test it though. Could you test that on your
side with s3c64xx ?
Got some error. Seems effect updated only when I set new CrCb value.
Seems it's incorrect:
case V4L2_CID_COLORFX:
if (camif->ctrl_colorfx_cbcr->is_new) {
Uh, copy/paste error, this should have been
if (camif->ctrl_colorfx->is_new) {
camif->colorfx = camif->ctrl_colorfx->val;
/* Set Cb, Cr */
switch (ctrl->val) {
case V4L2_COLORFX_SEPIA:
camif->ctrl_colorfx_cbcr->val = 0x7391;
break;
case V4L2_COLORFX_SET_CBCR: /* noop */
break;
default:
/* for V4L2_COLORFX_BW and others */
camif->ctrl_colorfx_cbcr->val = 0x8080;
}
}
camif->colorfx_cb = camif->ctrl_colorfx_cbcr->val& 0xff;
camif->colorfx_cr = camif->ctrl_colorfx_cbcr->val>> 8;
break;
Moving "camif->colorfx = camif->ctrl_colorfx->val;" out of condition
fixes the problem, but setting CrCb value control affect all effects
(sepia and BW), not only V4L2_COLORFX_SET_CBCR. Seems condition should
be removed and colorfx value should be checked set on every effect
change.
diff --git a/drivers/media/platform/s3c-camif/camif-capture.c
b/drivers/media/platform/s3c-camif/camif-capture.c
index ceab03a..9c01f4f 100644
--- a/drivers/media/platform/s3c-camif/camif-capture.c
+++ b/drivers/media/platform/s3c-camif/camif-capture.c
@@ -1526,19 +1526,17 @@ static int s3c_camif_subdev_s_ctrl(struct
v4l2_ctrl *ctrl)
switch (ctrl->id) {
case V4L2_CID_COLORFX:
- if (camif->ctrl_colorfx_cbcr->is_new) {
- camif->colorfx = camif->ctrl_colorfx->val;
- /* Set Cb, Cr */
- switch (ctrl->val) {
- case V4L2_COLORFX_SEPIA:
- camif->ctrl_colorfx_cbcr->val = 0x7391;
- break;
- case V4L2_COLORFX_SET_CBCR: /* noop */
- break;
- default:
- /* for V4L2_COLORFX_BW and others */
- camif->ctrl_colorfx_cbcr->val = 0x8080;
- }
+ camif->colorfx = camif->ctrl_colorfx->val;
+ /* Set Cb, Cr */
+ switch (ctrl->val) {
+ case V4L2_COLORFX_SEPIA:
+ camif->ctrl_colorfx_cbcr->val = 0x7391;
+ break;
+ case V4L2_COLORFX_SET_CBCR: /* noop */
+ break;
+ default:
+ /* for V4L2_COLORFX_BW and others */
+ camif->ctrl_colorfx_cbcr->val = 0x8080;
}
camif->colorfx_cb = camif->ctrl_colorfx_cbcr->val& 0xff;
camif->colorfx_cr = camif->ctrl_colorfx_cbcr->val>> 8;
With this modification got another issue: set CRCB effect, set CRCB
value, set BW effect, set CRCB effect back cause CRCB-value to be
reseted to default 0x8080. Is it correct?
We could do better. The control values are already cached twice - in
struct v4l2_ctrl and struct camif_dev.
It seems more intuitive to save CB/CR coefficients for V4L2_COLORFX_SET_CBCR
and then restore them as needed. There is probably not much use of letting
user space know that the driver uses CBCR for V4L2_COLORFX_SEPIA and
V4L2_COLORFX_BW internally.
I propose change as below, it includes disabling the control for SoCs that
don't support it and a fixed cbcr order, to match documentation:
"V4L2_CID_COLORFX_CBCR integer Determines the Cb and Cr coefficients for
V4L2_COLORFX_SET_CBCR color effect. Bits [7:0] of the supplied 32 bit
value are
interpreted as Cr component, bits [15:8] as Cb component and bits
[31:16] must
be zero."
I have pushed it all to the testing/s3c-camif branch. Please let me know
if you find any further issues.
-----8<-------
diff --git a/drivers/media/platform/s3c-camif/camif-capture.c
b/drivers/media/platform/s3c-camif/camif-capture.c
index 6401fdb..b52cc59 100644
--- a/drivers/media/platform/s3c-camif/camif-capture.c
+++ b/drivers/media/platform/s3c-camif/camif-capture.c
@@ -1520,22 +1520,22 @@ static int s3c_camif_subdev_s_ctrl(struct
v4l2_ctrl *ctrl)
switch (ctrl->id) {
case V4L2_CID_COLORFX:
- if (camif->ctrl_colorfx_cbcr->is_new) {
- camif->colorfx = camif->ctrl_colorfx->val;
- /* Set Cb, Cr */
- switch (ctrl->val) {
- case V4L2_COLORFX_SEPIA:
- camif->ctrl_colorfx_cbcr->val = 0x7391;
- break;
- case V4L2_COLORFX_SET_CBCR: /* noop */
- break;
- default:
- /* for V4L2_COLORFX_BW and others */
- camif->ctrl_colorfx_cbcr->val = 0x8080;
- }
+ camif->colorfx = camif->ctrl_colorfx->val;
+ /* Set Cb, Cr */
+ switch (ctrl->val) {
+ case V4L2_COLORFX_SEPIA:
+ camif->colorfx_cb = 115;
+ camif->colorfx_cr = 145;
+ break;
+ case V4L2_COLORFX_SET_CBCR:
+ camif->colorfx_cb = camif->ctrl_colorfx_cbcr->val >> 8;
+ camif->colorfx_cr = camif->ctrl_colorfx_cbcr->val & 0xff;
+ break;
+ default:
+ /* for V4L2_COLORFX_BW and others */
+ camif->colorfx_cb = 128;
+ camif->colorfx_cr = 128;
}
- camif->colorfx_cb = camif->ctrl_colorfx_cbcr->val & 0xff;
- camif->colorfx_cr = camif->ctrl_colorfx_cbcr->val >> 8;
break;
case V4L2_CID_TEST_PATTERN:
camif->test_pattern = camif->ctrl_test_pattern->val;
@@ -1603,6 +1603,10 @@ int s3c_camif_create_subdev(struct camif_dev *camif)
v4l2_ctrl_auto_cluster(2, &camif->ctrl_colorfx,
V4L2_COLORFX_SET_CBCR, false);
+ if (!camif->variant->has_img_effect) {
+ camif->ctrl_colorfx->flags |= V4L2_CTRL_FLAG_DISABLED;
+ camif->ctrl_colorfx_cbcr->flags |= V4L2_CTRL_FLAG_DISABLED;
+ }
sd->ctrl_handler = handler;
v4l2_set_subdevdata(sd, camif);
----->8-------
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
