v4l2-ctrl is unable to set autogain to 0 with gspca/ov534

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

Hans Verkuil Oct. 12, 2015, 7:56 a.m. UTC | #1

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: <[e-mail]>      # 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Antonio Ospite Oct. 14, 2015, 3:37 p.m. UTC | #2

On Mon, 12 Oct 2015 09:56:08 +0200
Hans Verkuil <[e-mail]> 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

Patch

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++)