Re: S3C244X/S3C64XX SoC camera host interface driver questions

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

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