From patchwork Sat Nov 17 17:24:26 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sylwester Nawrocki X-Patchwork-Id: 1759841 Return-Path: X-Original-To: patchwork-linux-media@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 46BEBDF2A2 for ; Sat, 17 Nov 2012 17:24:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752003Ab2KQRYb (ORCPT ); Sat, 17 Nov 2012 12:24:31 -0500 Received: from mail-ee0-f46.google.com ([74.125.83.46]:48060 "EHLO mail-ee0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751967Ab2KQRYa (ORCPT ); Sat, 17 Nov 2012 12:24:30 -0500 Received: by mail-ee0-f46.google.com with SMTP id e53so425606eek.19 for ; Sat, 17 Nov 2012 09:24:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=bnd8GDxyigoW+gcXPROLn2QMvjS2vstNejLKJAr0yzw=; b=fKBx/eZIGFwzWq763D78cn2zYAci5/8BiLyMc5efcxiF+dcOAlS+f0JDqoUvCda90x 2eFimmQShEUWkwd6cJgIFoWjH2RyeGL9sFb1gqEhYN5GvORNpa4RbnWVzDsgkheL7dmj ibR1+Pkaf4D6M0EhJdaWU8EnAUH82v5QSoRX6MP0KwSE0adCynBEzGr+dHe2SR/ql0aP yyYocCEbVbLkUnS8/NSn/CdUDxzLcZLuyg8ryg0jLQgBQRf+TpVGHth7Zq6Kl0pPMzbI DEVOlGco26fiC1pNh9fT6NVzZ5xGACsJEA5FfsPhlrHNw+rPYxpGuP/1pjcGahSg3zuv 6IYg== Received: by 10.14.203.69 with SMTP id e45mr8151580eeo.38.1353173068785; Sat, 17 Nov 2012 09:24:28 -0800 (PST) Received: from [192.168.1.110] (178235118103.warszawa.vectranet.pl. [178.235.118.103]) by mx.google.com with ESMTPS id z43sm11747865een.16.2012.11.17.09.24.27 (version=SSLv3 cipher=OTHER); Sat, 17 Nov 2012 09:24:28 -0800 (PST) Message-ID: <50A7C84A.4090804@gmail.com> Date: Sat, 17 Nov 2012 18:24:26 +0100 From: Sylwester Nawrocki User-Agent: Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20120412 Thunderbird/11.0.1 MIME-Version: 1.0 To: Andrey Gusakov CC: Sylwester Nawrocki , Tomasz Figa , In-Bae Jeong , =?ISO-8859-1?Q?Heiko_St=FCbner?= , LMML , linux-samsung-soc Subject: Re: S3C244X/S3C64XX SoC camera host interface driver questions References: <5096C561.5000108@gmail.com> <5096E8D7.4070304@gmail.com> <50979998.8090809@gmail.com> <50983CFD.2030104@gmail.com> <509AD957.5070301@gmail.com> <509CBB61.40206@gmail.com> <50A2CF95.4000202@gmail.com> In-Reply-To: Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org 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<------- 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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: