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

[Laurent Pinchart]

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