Re: [PATCH v9 15/22] media: uvcvideo: Set error_idx during ctrl_commit errors

From: Laurent Pinchart
Date: Thu Jun 10 2021 - 13:05:46 EST


Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:33AM +0100, Ricardo Ribalda wrote:
> If we have an error setting a control, return the affected control in
> the error_idx field.
>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 42 ++++++++++++++++++++++++++------
> drivers/media/usb/uvc/uvc_v4l2.c | 2 +-
> drivers/media/usb/uvc/uvcvideo.h | 10 +++-----
> 3 files changed, 40 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 24fd5afc4e4f..bcebf9d1a46f 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1586,7 +1586,7 @@ int uvc_ctrl_begin(struct uvc_video_chain *chain)
> }
>
> static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> - struct uvc_entity *entity, int rollback)
> + struct uvc_entity *entity, int rollback, struct uvc_control **err_ctrl)
> {
> struct uvc_control *ctrl;
> unsigned int i;
> @@ -1628,31 +1628,59 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>
> ctrl->dirty = 0;
>
> - if (ret < 0)
> + if (ret < 0) {
> + if (err_ctrl)
> + *err_ctrl = ctrl;
> return ret;
> + }
> }
>
> return 0;
> }
>
> +static int uvc_ctrl_find_ctrlidx(struct uvc_entity *entity,

s/uvc_ctrl_find_ctrlidx/uvc_ctrl_find_ctrl_idx/

Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>

> + struct v4l2_ext_controls *ctrls,
> + struct uvc_control *uvc_control)
> +{
> + struct uvc_control_mapping *mapping;
> + struct uvc_control *ctrl_found;
> + unsigned int i;
> +
> + if (!entity)
> + return ctrls->count;
> +
> + 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;
> + }
> +
> + return ctrls->count;
> +}
> +
> int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> - const struct v4l2_ext_control *xctrls,
> - unsigned int xctrls_count)
> + struct v4l2_ext_controls *ctrls)
> {
> struct uvc_video_chain *chain = handle->chain;
> + struct uvc_control *err_ctrl;
> struct uvc_entity *entity;
> int ret = 0;
>
> /* Find the control. */
> list_for_each_entry(entity, &chain->entities, chain) {
> - ret = uvc_ctrl_commit_entity(chain->dev, entity, rollback);
> + ret = uvc_ctrl_commit_entity(chain->dev, entity, rollback,
> + &err_ctrl);
> if (ret < 0)
> goto done;
> }
>
> if (!rollback)
> - uvc_ctrl_send_events(handle, xctrls, xctrls_count);
> + 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);
> mutex_unlock(&chain->ctrl_mutex);
> return ret;
> }
> @@ -2110,7 +2138,7 @@ int uvc_ctrl_restore_values(struct uvc_device *dev)
> ctrl->dirty = 1;
> }
>
> - ret = uvc_ctrl_commit_entity(dev, entity, 0);
> + ret = uvc_ctrl_commit_entity(dev, entity, 0, NULL);
> if (ret < 0)
> return ret;
> }
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index a3ee1dc003fc..8d8b12a4db34 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1088,7 +1088,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
> ctrls->error_idx = 0;
>
> if (ioctl == VIDIOC_S_EXT_CTRLS)
> - return uvc_ctrl_commit(handle, ctrls->controls, ctrls->count);
> + return uvc_ctrl_commit(handle, ctrls);
> else
> return uvc_ctrl_rollback(handle);
> }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 9471c342a310..0313b30f0cea 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -887,17 +887,15 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
>
> int uvc_ctrl_begin(struct uvc_video_chain *chain);
> int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> - const struct v4l2_ext_control *xctrls,
> - unsigned int xctrls_count);
> + struct v4l2_ext_controls *ctrls);
> static inline int uvc_ctrl_commit(struct uvc_fh *handle,
> - const struct v4l2_ext_control *xctrls,
> - unsigned int xctrls_count)
> + struct v4l2_ext_controls *ctrls)
> {
> - return __uvc_ctrl_commit(handle, 0, xctrls, xctrls_count);
> + return __uvc_ctrl_commit(handle, 0, ctrls);
> }
> static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
> {
> - return __uvc_ctrl_commit(handle, 1, NULL, 0);
> + return __uvc_ctrl_commit(handle, 1, NULL);
> }
>
> int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);

--
Regards,

Laurent Pinchart