Re: [PATCH 7/8] usb: gadget: uvc: add colorspace handling

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Michael,

Thank you for the patch.

On Thu, Mar 23, 2023 at 12:41:15PM +0100, Michael Tretter wrote:
> Store the values of the configured color space in the driver. This
> allows the user space to set the color space and re-read the set values
> later.
> 
> UVC allows to announce the colorspace in the USB descriptors. The values
> of the descriptors are not evaluated by the driver, yet. Thus, the
> default is always the default specified by UVC and not the configured
> default.

As with format handling and frame interval handling, I don't like this.
It's useless code that userspace won't exercise.

> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/usb/gadget/function/uvc.h      |  4 ++++
>  drivers/usb/gadget/function/uvc_v4l2.c | 32 ++++++++++++++++++++++++++++++--
>  2 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index 100475b1363e..6b4ab3e07173 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -96,6 +96,10 @@ struct uvc_video {
>  	unsigned int width;
>  	unsigned int height;
>  	unsigned int imagesize;
> +	enum v4l2_colorspace colorspace;
> +	enum v4l2_ycbcr_encoding ycbcr_enc;
> +	enum v4l2_quantization quantization;
> +	enum v4l2_xfer_func xfer_func;
>  	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 c5983bb0a8d1..673532ff0faa 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -180,6 +180,11 @@ void uvc_init_default_format(struct uvc_device *uvc)
>  		video->height = 240;
>  		video->imagesize = 320 * 240 * 2;
>  
> +		video->colorspace = V4L2_COLORSPACE_SRGB;
> +		video->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +		video->xfer_func = V4L2_XFER_FUNC_SRGB;
> +		video->ycbcr_enc = V4L2_YCBCR_ENC_601;
> +
>  		return;
>  	}
>  
> @@ -196,6 +201,14 @@ void uvc_init_default_format(struct uvc_device *uvc)
>  	video->width = uframe->frame.w_width;
>  	video->height = uframe->frame.w_height;
>  	video->imagesize = uvc_get_frame_size(uformat, uframe);
> +
> +	if (uformat->type == UVCG_UNCOMPRESSED)
> +		video->colorspace = V4L2_COLORSPACE_SRGB;
> +	else
> +		video->colorspace = V4L2_COLORSPACE_JPEG;
> +	video->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	video->xfer_func = V4L2_XFER_FUNC_SRGB;
> +	video->ycbcr_enc = V4L2_YCBCR_ENC_601;
>  }
>  
>  static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc,
> @@ -294,7 +307,12 @@ uvc_v4l2_get_format(struct file *file, void *fh, struct v4l2_format *fmt)
>  	fmt->fmt.pix.field = V4L2_FIELD_NONE;
>  	fmt->fmt.pix.bytesperline = video->bpp * video->width / 8;
>  	fmt->fmt.pix.sizeimage = video->imagesize;
> -	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> +
> +	fmt->fmt.pix.colorspace = video->colorspace;
> +	fmt->fmt.pix.ycbcr_enc = video->ycbcr_enc;
> +	fmt->fmt.pix.quantization = video->quantization;
> +	fmt->fmt.pix.xfer_func = video->xfer_func;
> +
>  	fmt->fmt.pix.priv = 0;
>  
>  	return 0;
> @@ -335,7 +353,12 @@ uvc_v4l2_try_format(struct file *file, void *fh, struct v4l2_format *fmt)
>  	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(uformat, uframe);
>  	fmt->fmt.pix.sizeimage = uvc_get_frame_size(uformat, uframe);
>  	fmt->fmt.pix.pixelformat = to_uvc_format(uformat)->fcc;
> -	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> +
> +	fmt->fmt.pix.colorspace = video->colorspace;
> +	fmt->fmt.pix.ycbcr_enc = video->ycbcr_enc;
> +	fmt->fmt.pix.quantization = video->quantization;
> +	fmt->fmt.pix.xfer_func = video->xfer_func;
> +
>  	fmt->fmt.pix.priv = 0;
>  
>  	return 0;
> @@ -359,6 +382,11 @@ uvc_v4l2_set_format(struct file *file, void *fh, struct v4l2_format *fmt)
>  	video->height = fmt->fmt.pix.height;
>  	video->imagesize = fmt->fmt.pix.sizeimage;
>  
> +	video->colorspace = fmt->fmt.pix.colorspace;
> +	video->ycbcr_enc = fmt->fmt.pix.ycbcr_enc;
> +	video->quantization = fmt->fmt.pix.quantization;
> +	video->xfer_func = fmt->fmt.pix.xfer_func;
> +
>  	return ret;
>  }
>  

-- 
Regards,

Laurent Pinchart