From patchwork Wed Oct 7 08:05:24 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Antonio Ospite X-Patchwork-Id: 31615 X-Patchwork-Delegate: hverkuil@xs4all.nl Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from ) id 1ZjjjQ-0001nP-5x; Wed, 07 Oct 2015 10:05:48 +0200 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.76/mailfrontend-5) with esmtp id 1ZjjjN-00029R-9E; Wed, 07 Oct 2015 10:05:47 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751588AbbJGIFn (ORCPT + 1 other); Wed, 7 Oct 2015 04:05:43 -0400 Received: from smtp205.alice.it ([82.57.200.101]:40461 "EHLO smtp205.alice.it" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751246AbbJGIFh (ORCPT ); Wed, 7 Oct 2015 04:05:37 -0400 Received: from jcn (87.3.192.30) by smtp205.alice.it (8.6.060.28) id 55F97357054D43D8; Wed, 7 Oct 2015 10:05:27 +0200 Date: Wed, 7 Oct 2015 10:05:24 +0200 From: Antonio Ospite To: linux-media@vger.kernel.org Cc: Mauro Carvalho Chehab , Hans Verkuil , Ricardo Ribalda Delgado Subject: v4l2-ctrl is unable to set autogain to 0 with gspca/ov534 Message-Id: <20151007100524.3fc05282628a153591f5c13e@ao2.it> X-Mailer: Sylpheed 3.5.0beta3 (GTK+ 2.24.28; x86_64-pc-linux-gnu) X-Face: z*RaLf`X<@C75u6Ig9}{oW$H; 1_\2t5)({*|jhM/Vb; ]yA5\I~93>J<_`<4)A{':UrE Mime-Version: 1.0 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2015.10.7.75716 X-PMX-Spam: Gauge=IIIIIIIII, Probability=9%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, MIME_LOWER_CASE 0.05, BODY_SIZE_4000_4999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, NO_URI_HTTPS 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __CP_URI_IN_BODY 0, __CT 0, __CTE 0, __CT_TEXT_PLAIN 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __MIME_TEXT_ONLY 0, __MIME_VERSION 0, __MULTIPLE_RCPTS_CC_X2 0, __MULTIPLE_URI_TEXT 0, __SANE_MSGID 0, __SUBJ_ALPHA_START 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_IN_BODY 0, __URI_NO_WWW 0, __URI_NS ' Hi, It looks like it is not possible to set the autogain from 1 to 0 using v4l2-ctrl with the driver I am using. I am testing with the gspca/ov534 driver, and this sequence of commands does not change the value of the control: v4l2-ctl --set-ctrl=gain_automatic=1 v4l2-ctl --list-ctrls | grep gain_automatic # The following does not work v4l2-ctl --set-ctrl=gain_automatic=0 v4l2-ctl --list-ctrls | grep gain_automatic The same thing happens with guvcview, but setting the control with qv4l2 works fine. After a little investigation I figured out some more details: in my use case the autogain is a master control in an auto cluster, and switching it from auto to manual does not work when using VIDIOC_S_CTRL i.e. when calling set_ctrl(). It works with qv4l2 because it uses VIDIOC_S_EXT_CTRLS. So the difference is between v4l2-ctrls.c::v4l2_s_ctrl() and v4l2-ctrls.c::v4l2_s_ext_ctrls(). Wrt. auto clusters going from auto to manual the two functions do basically this: v4l2_s_ctrl() set_ctrl_lock() user_to_new() set_ctrl() update_from_auto_cluster(master) try_or_set_cluster() cur_to_user() v4l2_s_ext_ctrls() try_set_ext_ctrls() update_from_auto_cluster(master) user_to_new() for each control try_or_set_cluster() new_to_user() I think the problem is that when update_from_auto_cluster(master) is called it overrides the new master control value from userspace by calling cur_to_new(). This also happens when calling VIDIOC_S_EXT_CTRLS (in try_set_ext_ctrls), but in that case, AFTER the call to update_from_auto_cluster(master), the code calls user_to_new() that sets back again the correct new value in the control before making the value permanent with try_or_set_cluster(). The regression may have been introduced in 5d0360a4f027576e5419d4a7c711c9ca0f1be8ca, in fact by just reverting these two interdependent commits: 7a7f1ab37dc8f66cf0ef10f3d3f1b79ac4bc67fc 5d0360a4f027576e5419d4a7c711c9ca0f1be8ca the problem goes away, so the regression is about user_to_new() not being called AFTER update_from_auto_cluster(master) anymore in set_ctrl(), as per 5d0360a4f027576e5419d4a7c711c9ca0f1be8ca. A quick and dirty fixup could look like this: We can assume that the master control in an auto cluster is always the first one, can't we? With the change above we don't override the new value of the master control, in this case when it's being changed from auto to manual. I may be missing some details tho, so I am asking if my reasoning is correct before sending a proper patch. And should I CC stable on it as the change fixes a regression? Thanks, Antonio diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index b6b7dcc..55d78fc 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -3198,8 +3198,11 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags) manual mode we have to update the current volatile values since those will become the initial manual values after such a switch. */ if (master->is_auto && master->has_volatiles && ctrl == master && - !is_cur_manual(master) && ctrl->val == master->manual_mode_value) + !is_cur_manual(master) && ctrl->val == master->manual_mode_value) { + s32 new_auto_val = master->val; update_from_auto_cluster(master); + master->val = new_auto_val; + } ctrl->is_new = 1; return try_or_set_cluster(fh, master, true, ch_flags); However I think that calling user_to_new() after update_from_auto_cluster() has always been masking a bug in the latter. Maybe this is a better fix, in place of the one above. diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index b6b7dcc..19fc06e 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -3043,7 +3043,7 @@ static void update_from_auto_cluster(struct v4l2_ctrl *master) { int i; - for (i = 0; i < master->ncontrols; i++) + for (i = 1; i < master->ncontrols; i++) cur_to_new(master->cluster[i]); if (!call_op(master, g_volatile_ctrl)) for (i = 1; i < master->ncontrols; i++)