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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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:

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


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

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux