Message ID | 20151007100524.3fc05282628a153591f5c13e@ao2.it (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Hans Verkuil |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) 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 <rfc822;mkrufky@linuxtv.org> + 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 <rfc822;linux-media@vger.kernel.org>); 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 <ao2@ao2.it> To: linux-media@vger.kernel.org Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>, Hans Verkuil <hans.verkuil@cisco.com>, Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> 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<pyWR#k60!#=#>/Vb; ]yA5<GWI5`6u&+ ; 6b'@y|8w"wB; 4/e!7wYYrcqdJFY,~%Gk_4]cq$Ei/7<j&N3ah(m`ku?pX.&+~:_/wC~dwn^)MizBG !pE^+iDQQ1yC6^,)YDKkxDd!T>\I~93>J<_`<4)A{':UrE Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> 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 ' |
Commit Message
Antonio Ospite
Oct. 7, 2015, 8:05 a.m. UTC
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
Comments
Hi Antonio, On 10/07/2015 10:05 AM, Antonio Ospite wrote: > 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. Excellent analysis! > > A quick and dirty fixup could look like this: > > 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++) > I agree, this is the right fix. > > 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? Just post the patch to linux-media, but add this line after your Signed-off-by: Cc: <stable@vger.kernel.org> # for v3.17 and up Thanks for looking at this! Regards, Hans -- 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
On Mon, 12 Oct 2015 09:56:08 +0200 Hans Verkuil <hverkuil@xs4all.nl> wrote: > Hi Antonio, > > On 10/07/2015 10:05 AM, Antonio Ospite wrote: [...] > > 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. > > Excellent analysis! > I think we can make the code paths of v4l2_s_ctrl() and v4l2_s_ext_ctrls() more look alike to ease similar investigations. In v4l2_s_ext_ctrls(): 1. Call user_to_new() before update_from_auto_cluster(master), I am still not 100% sure if this can introduce regressions. 2. Use cur_to_user() instead of new_to_user(), to match the code path of v4l2_s_ctrl(), the effect of using either one or the other should be equivalent right _after_ a call to try_or_set_cluster(), shouldn't it? I'll try to test these changes as time permits, but if anyone can squeeze that in their paid time feel free to anticipate me. [...] > Thanks for looking at this! > FWIW I got interested by this in particular: $ strace v4l2-ctl --set-ctrl=gain_automatic=0 ... ioctl(3, VIDIOC_S_CTRL, {id=V4L2_CID_AUTOGAIN, value=1}) = 0 ^ Ciao ciao, 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++)