Re: [PATCH v2 2/3] usb: gadget: uvc: add g_parm and s_parm for frame interval

[Avichal Rakesh <arakesh@google.com>]

On 6/22/24 4:48 PM, Michael Grzeschik wrote:
> The uvc gadget driver is lacking the information which frame interval
> was set by the host. We add this information by implementing the g_parm
> and s_parm callbacks.

This change requires the userspace application (uvc-gagdet equivalent) 
to call s/g_parm when the FPS negotiations are finished. This is fine, 
but we should document that in the commit message here so implementers 
know that something needs to be done to take advantage of the change.

On a similar note, the reference uvc-gadget should also be updated to 
call the added functions (and apologies if you've already put up a 
patch for it, I was unable find one).

> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> ---
> v1 -> v2: -
> ---
>  drivers/usb/gadget/function/uvc.h      |  1 +
>  drivers/usb/gadget/function/uvc_v4l2.c | 52 ++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index cb35687b11e7e..d153bd9e35e31 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -97,6 +97,7 @@ struct uvc_video {
>  	unsigned int width;
>  	unsigned int height;
>  	unsigned int imagesize;
> +	unsigned int interval;
>  	struct mutex mutex;	/* protects frame parameters */
>  
>  	unsigned int uvc_num_requests;
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index a024aecb76dc3..5b579ec1f5040 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -307,6 +307,56 @@ uvc_v4l2_set_format(struct file *file, void *fh, struct v4l2_format *fmt)
>  	return ret;
>  }
>  
> +static int uvc_v4l2_g_parm(struct file *file, void *fh,
> +			    struct v4l2_streamparm *parm)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	struct uvc_device *uvc = video_get_drvdata(vdev);
> +	struct uvc_video *video = &uvc->video;
> +	struct v4l2_fract timeperframe;
> +
> +	if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	/* Return the actual frame period. */
> +	timeperframe.numerator = video->interval;
> +	timeperframe.denominator = 10000000;
> +	v4l2_simplify_fraction(&timeperframe.numerator,
> +		&timeperframe.denominator, 8, 333);
> +
> +	uvcg_dbg(&uvc->func, "Getting frame interval of %u/%u (%u)\n",
> +		timeperframe.numerator, timeperframe.denominator,
> +		video->interval);
> +
> +	parm->parm.output.timeperframe = timeperframe;
> +	parm->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
> +
> +	return 0;
> +}
> +
> +static int uvc_v4l2_s_parm(struct file *file, void *fh,
> +			    struct v4l2_streamparm *parm)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	struct uvc_device *uvc = video_get_drvdata(vdev);
> +	struct uvc_video *video = &uvc->video;
> +	struct v4l2_fract timeperframe;
> +
> +	if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	timeperframe = parm->parm.output.timeperframe;
> +
> +	video->interval = v4l2_fraction_to_interval(timeperframe.numerator,
> +		timeperframe.denominator);
> +
> +	uvcg_dbg(&uvc->func, "Setting frame interval to %u/%u (%u)\n",
> +		timeperframe.numerator, timeperframe.denominator,
> +		video->interval);
> +
> +	return 0;
> +}
> +
>  static int
>  uvc_v4l2_enum_frameintervals(struct file *file, void *fh,
>  		struct v4l2_frmivalenum *fival)
> @@ -577,6 +627,8 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
>  	.vidioc_dqbuf = uvc_v4l2_dqbuf,
>  	.vidioc_streamon = uvc_v4l2_streamon,
>  	.vidioc_streamoff = uvc_v4l2_streamoff,
> +	.vidioc_s_parm = uvc_v4l2_s_parm,
> +	.vidioc_g_parm = uvc_v4l2_g_parm,
>  	.vidioc_subscribe_event = uvc_v4l2_subscribe_event,
>  	.vidioc_unsubscribe_event = uvc_v4l2_unsubscribe_event,
>  	.vidioc_default = uvc_v4l2_ioctl_default,
>