Hi Laurent

Thanks for your review!

On Thu, 10 Jun 2021 at 19:28, Laurent Pinchart
[off-list ref] wrote:

Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:34AM +0100, Ricardo Ribalda wrote:

quoted

If a control is inactive return -EACCES to let the userspace know that
the value will not be applied automatically when the control is active
again.

Isn't the device supposed to stall the control set in that case, with
the bRequestErrorCode set to "Wrong state", which uvc_query_ctrl()
translates to -EACCES already ? Could you elaborate on why this patch is
needed ?

The problem is that the hardware was not returning the equivalent of
EACCES, so we had to capture it manually.
I will try to revert the patch and capture an error.

quoted

Suggested-by: Hans Verkuil <redacted>
Signed-off-by: Ricardo Ribalda <redacted>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 71 +++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 23 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index bcebf9d1a46f..d9d4add1e813 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c

@@ -1082,13 +1082,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map)
      return "Unknown Control";
 }

+static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain,
+                              struct uvc_control *ctrl,
+                              struct uvc_control_mapping *mapping)
+{
+     struct uvc_control_mapping *master_map = NULL;
+     struct uvc_control *master_ctrl = NULL;
+     s32 val;
+     int ret;
+
+     if (!mapping->master_id)
+             return false;
+
+     __uvc_find_control(ctrl->entity, mapping->master_id, &master_map,
+                        &master_ctrl, 0);
+
+     if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
+             return false;
+
+     ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
+     if (ret < 0 || val == mapping->master_manual)
+             return false;
+
+     return true;
+}
+
 static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
      struct uvc_control *ctrl,
      struct uvc_control_mapping *mapping,
      struct v4l2_queryctrl *v4l2_ctrl)
 {
-     struct uvc_control_mapping *master_map = NULL;
-     struct uvc_control *master_ctrl = NULL;
      const struct uvc_menu_info *menu;
      unsigned int i;

@@ -1104,18 +1127,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
      if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
              v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;

-     if (mapping->master_id)
-             __uvc_find_control(ctrl->entity, mapping->master_id,
-                                &master_map, &master_ctrl, 0);
-     if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
-             s32 val;
-             int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
-             if (ret < 0)
-                     return ret;

There's a small change in behaviour here, the driver used to return an
error, now it will ignore it. Is it intentional ?

AFAIK The error did not follow the v4l2 spec. You shall always be able
to query a ctrl.

I will add it to the commit message to make it clear.

quoted

-
-             if (val != mapping->master_manual)
-                             v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
-     }
+     if (uvc_ctrl_is_inactive(chain, ctrl, mapping))
+             v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;

      if (!ctrl->cached) {
              int ret = uvc_ctrl_populate_cache(chain, ctrl);

@@ -1638,25 +1651,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
      return 0;
 }

-static int uvc_ctrl_find_ctrlidx(struct uvc_entity *entity,
+static int uvc_ctrl_commit_error(struct uvc_video_chain *chain,
+                              struct uvc_entity *entity,
                               struct v4l2_ext_controls *ctrls,
-                              struct uvc_control *uvc_control)
+                              struct uvc_control *err_control,
+                              int ret)
 {
      struct uvc_control_mapping *mapping;
      struct uvc_control *ctrl_found;
      unsigned int i;

-     if (!entity)
-             return ctrls->count;
+     if (!entity) {
+             ctrls->error_idx = ctrls->count;
+             return ret;
+     }

      for (i = 0; i < ctrls->count; i++) {
              __uvc_find_control(entity, ctrls->controls[i].id, &mapping,
                                 &ctrl_found, 0);
-             if (uvc_control == ctrl_found)
-                     return i;
+             if (err_control == ctrl_found)
+                     break;
      }
+     ctrls->error_idx = i;

I think this line should be moved after the next check.

Not really, if we cannot find a control, we cannot blame it on control 0 ;)

quoted

+
+     /* We could not find the control that failed. */
+     if (i == ctrls->count)
+             return ret;

-     return ctrls->count;
+     if (uvc_ctrl_is_inactive(chain, err_control, mapping))
+             return -EACCES;
+
+     return ret;
 }

 int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,

@@ -1679,8 +1704,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
              uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
 done:
      if (ret < 0 && ctrls)
-             ctrls->error_idx = uvc_ctrl_find_ctrlidx(entity, ctrls,
-                                                      err_ctrl);
+             ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl,
+                                         ret);
      mutex_unlock(&chain->ctrl_mutex);
      return ret;
 }

--
Regards,

Laurent Pinchart

-- 
Ricardo Ribalda