[PATCH 0/8] usb: gadget: uvc: fix errors reported by v4l2-compliance

[PATCH 0/8] usb: gadget: uvc: fix errors reported by v4l2-compliance

[Michael Tretter]

This series fixes various errors and warnings that are reported by
v4l2-compliance for the v4l2 output device created by the UVC gadget.

Most notably, it changes the driver to take the initial format from the
configfs instead of using a hard coded value that might be rejected later in
the SET_FORMAT call. Note that user space is still responsible for negotiating
the format with the UVC host.

Michael

To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Scally <dan.scally@ideasonboard.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Michael Grzeschik <m.grzeschik@pengutronix.de>
Cc: linux-usb@vger.kernel.org
Cc: linux-media@vger.kernel.org
Cc: kernel@pengutronix.de
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>

---
Michael Tretter (8):
      usb: gadget: uvc: use fourcc printk helper
      usb: gadget: uvc: fix return code of REQBUFS
      usb: gadget: uvc: implement s/g_output ioctl
      usb: gadget: uvc: move video format initialization to uvc_v4l2
      usb: gadget: uvc: initialize video format using configfs
      usb: gadget: uvc: try harder to find a valid format
      usb: gadget: uvc: add colorspace handling
      usb: gadget: uvc: implement s/g_parm

 drivers/usb/gadget/function/f_uvc.c     |   2 +
 drivers/usb/gadget/function/uvc.h       |   5 +
 drivers/usb/gadget/function/uvc_queue.c |   6 +-
 drivers/usb/gadget/function/uvc_v4l2.c  | 235 ++++++++++++++++++++++++++++++--
 drivers/usb/gadget/function/uvc_v4l2.h  |   3 +
 drivers/usb/gadget/function/uvc_video.c |   5 -
 6 files changed, 238 insertions(+), 18 deletions(-)
---
base-commit: 8be174835f07b2c106b9961c0775486d06112a3c
change-id: 20230323-uvc-gadget-cleanup-47b1495befb9

Best regards,
-- 
Michael Tretter <m.tretter@pengutronix.de>

[PATCH 1/8] usb: gadget: uvc: use fourcc printk helper

[Michael Tretter]

There is a format helper for printing fourcc codes. Use that one instead
of manually formatting the pixelformat for printing.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/usb/gadget/function/uvc_v4l2.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 3f0a9795c0d4..13c7ba06f994 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -240,16 +240,13 @@ uvc_v4l2_try_format(struct file *file, void *fh, struct v4l2_format *fmt)
 	struct uvc_video *video = &uvc->video;
 	struct uvcg_format *uformat;
 	struct uvcg_frame *uframe;
-	u8 *fcc;
 
 	if (fmt->type != video->queue.queue.type)
 		return -EINVAL;
 
-	fcc = (u8 *)&fmt->fmt.pix.pixelformat;
-	uvcg_dbg(&uvc->func, "Trying format 0x%08x (%c%c%c%c): %ux%u\n",
-		fmt->fmt.pix.pixelformat,
-		fcc[0], fcc[1], fcc[2], fcc[3],
-		fmt->fmt.pix.width, fmt->fmt.pix.height);
+	uvcg_dbg(&uvc->func, "Trying format %p4cc: %ux%u\n",
+		 &fmt->fmt.pix.pixelformat,
+		 fmt->fmt.pix.width, fmt->fmt.pix.height);
 
 	uformat = find_format_by_pix(uvc, fmt->fmt.pix.pixelformat);
 	if (!uformat)

-- 
2.30.2

Re: [PATCH 1/8] usb: gadget: uvc: use fourcc printk helper

[Dan Scally]

Morning Michael

On 23/03/2023 11:41, Michael Tretter wrote:
> There is a format helper for printing fourcc codes. Use that one instead
> of manually formatting the pixelformat for printing.
>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

>   drivers/usb/gadget/function/uvc_v4l2.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index 3f0a9795c0d4..13c7ba06f994 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -240,16 +240,13 @@ uvc_v4l2_try_format(struct file *file, void *fh, struct v4l2_format *fmt)
>   	struct uvc_video *video = &uvc->video;
>   	struct uvcg_format *uformat;
>   	struct uvcg_frame *uframe;
> -	u8 *fcc;
>   
>   	if (fmt->type != video->queue.queue.type)
>   		return -EINVAL;
>   
> -	fcc = (u8 *)&fmt->fmt.pix.pixelformat;
> -	uvcg_dbg(&uvc->func, "Trying format 0x%08x (%c%c%c%c): %ux%u\n",
> -		fmt->fmt.pix.pixelformat,
> -		fcc[0], fcc[1], fcc[2], fcc[3],
> -		fmt->fmt.pix.width, fmt->fmt.pix.height);
> +	uvcg_dbg(&uvc->func, "Trying format %p4cc: %ux%u\n",
> +		 &fmt->fmt.pix.pixelformat,
> +		 fmt->fmt.pix.width, fmt->fmt.pix.height);
>   
>   	uformat = find_format_by_pix(uvc, fmt->fmt.pix.pixelformat);
>   	if (!uformat)
>

Re: [PATCH 1/8] usb: gadget: uvc: use fourcc printk helper

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Mar 23, 2023 at 12:41:09PM +0100, Michael Tretter wrote:
> There is a format helper for printing fourcc codes. Use that one instead
> of manually formatting the pixelformat for printing.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>

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

> ---
>  drivers/usb/gadget/function/uvc_v4l2.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index 3f0a9795c0d4..13c7ba06f994 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -240,16 +240,13 @@ uvc_v4l2_try_format(struct file *file, void *fh, struct v4l2_format *fmt)
>  	struct uvc_video *video = &uvc->video;
>  	struct uvcg_format *uformat;
>  	struct uvcg_frame *uframe;
> -	u8 *fcc;
>  
>  	if (fmt->type != video->queue.queue.type)
>  		return -EINVAL;
>  
> -	fcc = (u8 *)&fmt->fmt.pix.pixelformat;
> -	uvcg_dbg(&uvc->func, "Trying format 0x%08x (%c%c%c%c): %ux%u\n",
> -		fmt->fmt.pix.pixelformat,
> -		fcc[0], fcc[1], fcc[2], fcc[3],
> -		fmt->fmt.pix.width, fmt->fmt.pix.height);
> +	uvcg_dbg(&uvc->func, "Trying format %p4cc: %ux%u\n",
> +		 &fmt->fmt.pix.pixelformat,
> +		 fmt->fmt.pix.width, fmt->fmt.pix.height);
>  
>  	uformat = find_format_by_pix(uvc, fmt->fmt.pix.pixelformat);
>  	if (!uformat)

-- 
Regards,

Laurent Pinchart

[PATCH 2/8] usb: gadget: uvc: fix return code of REQBUFS

[Michael Tretter]

On success, VIDIOC_REQBUFS should return 0, not the number of allocated
buffers. As uvcg_alloc_buffers() is directly called by regbufs, it has
to return the correct error codes.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/usb/gadget/function/uvc_queue.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index 0aa3d7e1f3cc..f14f75b93aaa 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -179,11 +179,7 @@ void uvcg_free_buffers(struct uvc_video_queue *queue)
 int uvcg_alloc_buffers(struct uvc_video_queue *queue,
 			      struct v4l2_requestbuffers *rb)
 {
-	int ret;
-
-	ret = vb2_reqbufs(&queue->queue, rb);
-
-	return ret ? ret : rb->count;
+	return vb2_reqbufs(&queue->queue, rb);
 }
 
 int uvcg_query_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf)

-- 
2.30.2

Re: [PATCH 2/8] usb: gadget: uvc: fix return code of REQBUFS

[Dan Scally]

Hi Michael

On 23/03/2023 11:41, Michael Tretter wrote:
> On success, VIDIOC_REQBUFS should return 0, not the number of allocated
> buffers. As uvcg_alloc_buffers() is directly called by regbufs, it has
> to return the correct error codes.


s/regbufs/reqbufs

>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---


This probably wants a Fixes: tag?


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

>   drivers/usb/gadget/function/uvc_queue.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> index 0aa3d7e1f3cc..f14f75b93aaa 100644
> --- a/drivers/usb/gadget/function/uvc_queue.c
> +++ b/drivers/usb/gadget/function/uvc_queue.c
> @@ -179,11 +179,7 @@ void uvcg_free_buffers(struct uvc_video_queue *queue)
>   int uvcg_alloc_buffers(struct uvc_video_queue *queue,
>   			      struct v4l2_requestbuffers *rb)
>   {
> -	int ret;
> -
> -	ret = vb2_reqbufs(&queue->queue, rb);
> -
> -	return ret ? ret : rb->count;
> +	return vb2_reqbufs(&queue->queue, rb);
>   }
>   
>   int uvcg_query_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf)
>

Re: [PATCH 2/8] usb: gadget: uvc: fix return code of REQBUFS

[Laurent Pinchart]

On Fri, Mar 24, 2023 at 07:50:11AM +0000, Dan Scally wrote:
> Hi Michael
> 
> On 23/03/2023 11:41, Michael Tretter wrote:
> > On success, VIDIOC_REQBUFS should return 0, not the number of allocated
> > buffers. As uvcg_alloc_buffers() is directly called by regbufs, it has
> > to return the correct error codes.
> 
> s/regbufs/reqbufs
> 
> >
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> 
> This probably wants a Fixes: tag?

Good idea. With that,

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

> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> 
> >   drivers/usb/gadget/function/uvc_queue.c | 6 +-----
> >   1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> > index 0aa3d7e1f3cc..f14f75b93aaa 100644
> > --- a/drivers/usb/gadget/function/uvc_queue.c
> > +++ b/drivers/usb/gadget/function/uvc_queue.c
> > @@ -179,11 +179,7 @@ void uvcg_free_buffers(struct uvc_video_queue *queue)
> >   int uvcg_alloc_buffers(struct uvc_video_queue *queue,
> >   			      struct v4l2_requestbuffers *rb)
> >   {
> > -	int ret;
> > -
> > -	ret = vb2_reqbufs(&queue->queue, rb);
> > -
> > -	return ret ? ret : rb->count;
> > +	return vb2_reqbufs(&queue->queue, rb);
> >   }
> >   
> >   int uvcg_query_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf)

-- 
Regards,

Laurent Pinchart

[PATCH 3/8] usb: gadget: uvc: implement s/g_output ioctl

[Michael Tretter]

V4L2 OUTPUT devices should implement ENUM_OUTPUT, G_OUTPUT, and
S_OUTPUT. The UVC gadget provides only a single output. Therefore, allow
only a single output 0.

According to the documentation, "_TYPE_ANALOG" is historical and should
be read as "_TYPE_VIDEO".

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/usb/gadget/function/uvc_v4l2.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 13c7ba06f994..4b8bf94e06fc 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -377,6 +377,31 @@ uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
 	return 0;
 }
 
+static int
+uvc_v4l2_enum_output(struct file *file, void *priv_fh, struct v4l2_output *out)
+{
+	if (out->index != 0)
+		return -EINVAL;
+
+	out->type = V4L2_OUTPUT_TYPE_ANALOG;
+	snprintf(out->name, sizeof(out->name), "UVC");
+
+	return 0;
+}
+
+static int
+uvc_v4l2_g_output(struct file *file, void *priv_fh, unsigned int *i)
+{
+	*i = 0;
+	return 0;
+}
+
+static int
+uvc_v4l2_s_output(struct file *file, void *priv_fh, unsigned int i)
+{
+	return i ? -EINVAL : 0;
+}
+
 static int
 uvc_v4l2_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *b)
 {
@@ -547,6 +572,9 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
 	.vidioc_enum_frameintervals = uvc_v4l2_enum_frameintervals,
 	.vidioc_enum_framesizes = uvc_v4l2_enum_framesizes,
 	.vidioc_enum_fmt_vid_out = uvc_v4l2_enum_format,
+	.vidioc_enum_output = uvc_v4l2_enum_output,
+	.vidioc_g_output = uvc_v4l2_g_output,
+	.vidioc_s_output = uvc_v4l2_s_output,
 	.vidioc_reqbufs = uvc_v4l2_reqbufs,
 	.vidioc_querybuf = uvc_v4l2_querybuf,
 	.vidioc_qbuf = uvc_v4l2_qbuf,

-- 
2.30.2

Re: [PATCH 3/8] usb: gadget: uvc: implement s/g_output ioctl

[Laurent Pinchart]

Hi Michael,

(CC'ing Hans)

Thank you for the patch.

On Thu, Mar 23, 2023 at 12:41:11PM +0100, Michael Tretter wrote:
> V4L2 OUTPUT devices should implement ENUM_OUTPUT, G_OUTPUT, and
> S_OUTPUT. The UVC gadget provides only a single output. Therefore, allow
> only a single output 0.
> 
> According to the documentation, "_TYPE_ANALOG" is historical and should
> be read as "_TYPE_VIDEO".

I think v4l2-compliance should be fixed to not require those ioctls. As
this patch clearly shows, they're useless :-)

> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/usb/gadget/function/uvc_v4l2.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index 13c7ba06f994..4b8bf94e06fc 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -377,6 +377,31 @@ uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
>  	return 0;
>  }
>  
> +static int
> +uvc_v4l2_enum_output(struct file *file, void *priv_fh, struct v4l2_output *out)
> +{
> +	if (out->index != 0)
> +		return -EINVAL;
> +
> +	out->type = V4L2_OUTPUT_TYPE_ANALOG;
> +	snprintf(out->name, sizeof(out->name), "UVC");
> +
> +	return 0;
> +}
> +
> +static int
> +uvc_v4l2_g_output(struct file *file, void *priv_fh, unsigned int *i)
> +{
> +	*i = 0;
> +	return 0;
> +}
> +
> +static int
> +uvc_v4l2_s_output(struct file *file, void *priv_fh, unsigned int i)
> +{
> +	return i ? -EINVAL : 0;
> +}
> +
>  static int
>  uvc_v4l2_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *b)
>  {
> @@ -547,6 +572,9 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
>  	.vidioc_enum_frameintervals = uvc_v4l2_enum_frameintervals,
>  	.vidioc_enum_framesizes = uvc_v4l2_enum_framesizes,
>  	.vidioc_enum_fmt_vid_out = uvc_v4l2_enum_format,
> +	.vidioc_enum_output = uvc_v4l2_enum_output,
> +	.vidioc_g_output = uvc_v4l2_g_output,
> +	.vidioc_s_output = uvc_v4l2_s_output,
>  	.vidioc_reqbufs = uvc_v4l2_reqbufs,
>  	.vidioc_querybuf = uvc_v4l2_querybuf,
>  	.vidioc_qbuf = uvc_v4l2_qbuf,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/8] usb: gadget: uvc: implement s/g_output ioctl

[Dan Scally]

On 24/03/2023 09:20, Laurent Pinchart wrote:
> Hi Michael,
>
> (CC'ing Hans)
>
> Thank you for the patch.
>
> On Thu, Mar 23, 2023 at 12:41:11PM +0100, Michael Tretter wrote:
>> V4L2 OUTPUT devices should implement ENUM_OUTPUT, G_OUTPUT, and
>> S_OUTPUT. The UVC gadget provides only a single output. Therefore, allow
>> only a single output 0.
>>
>> According to the documentation, "_TYPE_ANALOG" is historical and should
>> be read as "_TYPE_VIDEO".
> I think v4l2-compliance should be fixed to not require those ioctls. As
> this patch clearly shows, they're useless :-)


+1 for this vote

>
>> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
>> ---
>>   drivers/usb/gadget/function/uvc_v4l2.c | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
>> index 13c7ba06f994..4b8bf94e06fc 100644
>> --- a/drivers/usb/gadget/function/uvc_v4l2.c
>> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
>> @@ -377,6 +377,31 @@ uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
>>   	return 0;
>>   }
>>   
>> +static int
>> +uvc_v4l2_enum_output(struct file *file, void *priv_fh, struct v4l2_output *out)
>> +{
>> +	if (out->index != 0)
>> +		return -EINVAL;
>> +
>> +	out->type = V4L2_OUTPUT_TYPE_ANALOG;
>> +	snprintf(out->name, sizeof(out->name), "UVC");
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +uvc_v4l2_g_output(struct file *file, void *priv_fh, unsigned int *i)
>> +{
>> +	*i = 0;
>> +	return 0;
>> +}
>> +
>> +static int
>> +uvc_v4l2_s_output(struct file *file, void *priv_fh, unsigned int i)
>> +{
>> +	return i ? -EINVAL : 0;
>> +}
>> +
>>   static int
>>   uvc_v4l2_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *b)
>>   {
>> @@ -547,6 +572,9 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
>>   	.vidioc_enum_frameintervals = uvc_v4l2_enum_frameintervals,
>>   	.vidioc_enum_framesizes = uvc_v4l2_enum_framesizes,
>>   	.vidioc_enum_fmt_vid_out = uvc_v4l2_enum_format,
>> +	.vidioc_enum_output = uvc_v4l2_enum_output,
>> +	.vidioc_g_output = uvc_v4l2_g_output,
>> +	.vidioc_s_output = uvc_v4l2_s_output,
>>   	.vidioc_reqbufs = uvc_v4l2_reqbufs,
>>   	.vidioc_querybuf = uvc_v4l2_querybuf,
>>   	.vidioc_qbuf = uvc_v4l2_qbuf,

Re: [PATCH 3/8] usb: gadget: uvc: implement s/g_output ioctl

[Hans Verkuil]

On 24/03/2023 10:21, Dan Scally wrote:
> 
> On 24/03/2023 09:20, Laurent Pinchart wrote:
>> Hi Michael,
>>
>> (CC'ing Hans)
>>
>> Thank you for the patch.
>>
>> On Thu, Mar 23, 2023 at 12:41:11PM +0100, Michael Tretter wrote:
>>> V4L2 OUTPUT devices should implement ENUM_OUTPUT, G_OUTPUT, and
>>> S_OUTPUT. The UVC gadget provides only a single output. Therefore, allow
>>> only a single output 0.
>>>
>>> According to the documentation, "_TYPE_ANALOG" is historical and should
>>> be read as "_TYPE_VIDEO".
>> I think v4l2-compliance should be fixed to not require those ioctls. As
>> this patch clearly shows, they're useless :-)

They are not useless. An application doesn't know how many outputs there are,
and what type they are. Just because there is only one output, doesn't mean
you can skip this.

The application also has to know the capabilities of the output.

Now, it can be useful to add some helper functions for this to v4l2-common.c,
at least for g/s_output.

Regards,

	Hans

> 
> 
> +1 for this vote
> 
>>
>>> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
>>> ---
>>>   drivers/usb/gadget/function/uvc_v4l2.c | 28 ++++++++++++++++++++++++++++
>>>   1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
>>> index 13c7ba06f994..4b8bf94e06fc 100644
>>> --- a/drivers/usb/gadget/function/uvc_v4l2.c
>>> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
>>> @@ -377,6 +377,31 @@ uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
>>>       return 0;
>>>   }
>>>   +static int
>>> +uvc_v4l2_enum_output(struct file *file, void *priv_fh, struct v4l2_output *out)
>>> +{
>>> +    if (out->index != 0)
>>> +        return -EINVAL;
>>> +
>>> +    out->type = V4L2_OUTPUT_TYPE_ANALOG;
>>> +    snprintf(out->name, sizeof(out->name), "UVC");
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int
>>> +uvc_v4l2_g_output(struct file *file, void *priv_fh, unsigned int *i)
>>> +{
>>> +    *i = 0;
>>> +    return 0;
>>> +}
>>> +
>>> +static int
>>> +uvc_v4l2_s_output(struct file *file, void *priv_fh, unsigned int i)
>>> +{
>>> +    return i ? -EINVAL : 0;
>>> +}
>>> +
>>>   static int
>>>   uvc_v4l2_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *b)
>>>   {
>>> @@ -547,6 +572,9 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
>>>       .vidioc_enum_frameintervals = uvc_v4l2_enum_frameintervals,
>>>       .vidioc_enum_framesizes = uvc_v4l2_enum_framesizes,
>>>       .vidioc_enum_fmt_vid_out = uvc_v4l2_enum_format,
>>> +    .vidioc_enum_output = uvc_v4l2_enum_output,
>>> +    .vidioc_g_output = uvc_v4l2_g_output,
>>> +    .vidioc_s_output = uvc_v4l2_s_output,
>>>       .vidioc_reqbufs = uvc_v4l2_reqbufs,
>>>       .vidioc_querybuf = uvc_v4l2_querybuf,
>>>       .vidioc_qbuf = uvc_v4l2_qbuf,

Re: [PATCH 3/8] usb: gadget: uvc: implement s/g_output ioctl

[Laurent Pinchart]

Hi Hans,

On Fri, Mar 24, 2023 at 10:39:13AM +0100, Hans Verkuil wrote:
> On 24/03/2023 10:21, Dan Scally wrote:
> > On 24/03/2023 09:20, Laurent Pinchart wrote:
> >> Hi Michael,
> >>
> >> (CC'ing Hans)
> >>
> >> Thank you for the patch.
> >>
> >> On Thu, Mar 23, 2023 at 12:41:11PM +0100, Michael Tretter wrote:
> >>> V4L2 OUTPUT devices should implement ENUM_OUTPUT, G_OUTPUT, and
> >>> S_OUTPUT. The UVC gadget provides only a single output. Therefore, allow
> >>> only a single output 0.
> >>>
> >>> According to the documentation, "_TYPE_ANALOG" is historical and should
> >>> be read as "_TYPE_VIDEO".
> >> I think v4l2-compliance should be fixed to not require those ioctls. As
> >> this patch clearly shows, they're useless :-)
> 
> They are not useless. An application doesn't know how many outputs there are,
> and what type they are. Just because there is only one output, doesn't mean
> you can skip this.
> 
> The application also has to know the capabilities of the output.

In the generic case, possibly, but for the UVC gadget that's not
relevant. The driver requires a specialized userspace application that
handles driver-specific events and ioctls to operate, so there's no need
for output enumeration.

> Now, it can be useful to add some helper functions for this to v4l2-common.c,
> at least for g/s_output.

I would indeed much rather provide default implementations in
v4l2-common.c, and call them automatically from v4l2-ioctl.c when the
driver doesn't provide custom handlers for those ioctls.

> Regards,
> 
> 	Hans
> 
> > +1 for this vote
> 
> >>> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> >>> ---
> >>>   drivers/usb/gadget/function/uvc_v4l2.c | 28 ++++++++++++++++++++++++++++
> >>>   1 file changed, 28 insertions(+)
> >>>
> >>> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> >>> index 13c7ba06f994..4b8bf94e06fc 100644
> >>> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> >>> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> >>> @@ -377,6 +377,31 @@ uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
> >>>       return 0;
> >>>   }
> >>>   +static int
> >>> +uvc_v4l2_enum_output(struct file *file, void *priv_fh, struct v4l2_output *out)
> >>> +{
> >>> +    if (out->index != 0)
> >>> +        return -EINVAL;
> >>> +
> >>> +    out->type = V4L2_OUTPUT_TYPE_ANALOG;
> >>> +    snprintf(out->name, sizeof(out->name), "UVC");
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static int
> >>> +uvc_v4l2_g_output(struct file *file, void *priv_fh, unsigned int *i)
> >>> +{
> >>> +    *i = 0;
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static int
> >>> +uvc_v4l2_s_output(struct file *file, void *priv_fh, unsigned int i)
> >>> +{
> >>> +    return i ? -EINVAL : 0;
> >>> +}
> >>> +
> >>>   static int
> >>>   uvc_v4l2_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *b)
> >>>   {
> >>> @@ -547,6 +572,9 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
> >>>       .vidioc_enum_frameintervals = uvc_v4l2_enum_frameintervals,
> >>>       .vidioc_enum_framesizes = uvc_v4l2_enum_framesizes,
> >>>       .vidioc_enum_fmt_vid_out = uvc_v4l2_enum_format,
> >>> +    .vidioc_enum_output = uvc_v4l2_enum_output,
> >>> +    .vidioc_g_output = uvc_v4l2_g_output,
> >>> +    .vidioc_s_output = uvc_v4l2_s_output,
> >>>       .vidioc_reqbufs = uvc_v4l2_reqbufs,
> >>>       .vidioc_querybuf = uvc_v4l2_querybuf,
> >>>       .vidioc_qbuf = uvc_v4l2_qbuf,

-- 
Regards,

Laurent Pinchart

[PATCH 4/8] usb: gadget: uvc: move video format initialization to uvc_v4l2

[Michael Tretter]

Move the setup of the initial video format to uvc_v4l2.c that handles
all the format negotiation. This keeps all format setup and
configuration code in uvc_v4l2.c and avoids scattering the format setup
across multiple files.

Furthermore, it allows to setup the default format using the format
configured in the configfs.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/usb/gadget/function/f_uvc.c     |  2 ++
 drivers/usb/gadget/function/uvc_v4l2.c  | 11 +++++++++++
 drivers/usb/gadget/function/uvc_v4l2.h  |  3 +++
 drivers/usb/gadget/function/uvc_video.c |  5 -----
 4 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 5e919fb65833..a16c8f80a50a 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -434,6 +434,8 @@ uvc_register_video(struct uvc_device *uvc)
 	struct usb_composite_dev *cdev = uvc->func.config->cdev;
 	int ret;
 
+	uvc_init_default_format(uvc);
+
 	/* TODO reference counting. */
 	memset(&uvc->vdev, 0, sizeof(uvc->vdev));
 	uvc->vdev.v4l2_dev = &uvc->v4l2_dev;
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 4b8bf94e06fc..5620546eb43b 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -130,6 +130,17 @@ static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc,
 	return uformat;
 }
 
+void uvc_init_default_format(struct uvc_device *uvc)
+{
+	struct uvc_video *video = &uvc->video;
+
+	video->fcc = V4L2_PIX_FMT_YUYV;
+	video->bpp = 16;
+	video->width = 320;
+	video->height = 240;
+	video->imagesize = 320 * 240 * 2;
+}
+
 static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc,
 					   struct uvcg_format *uformat,
 					   u16 rw, u16 rh)
diff --git a/drivers/usb/gadget/function/uvc_v4l2.h b/drivers/usb/gadget/function/uvc_v4l2.h
index 1576005b61fd..5c3a97de0776 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.h
+++ b/drivers/usb/gadget/function/uvc_v4l2.h
@@ -16,4 +16,7 @@
 extern const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops;
 extern const struct v4l2_file_operations uvc_v4l2_fops;
 
+struct uvc_device;
+void uvc_init_default_format(struct uvc_device *uvc);
+
 #endif /* __UVC_V4L2_H__ */
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index dd1c6b2ca7c6..27ff9ef49e16 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -516,11 +516,6 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
 		return -EINVAL;
 
 	video->uvc = uvc;
-	video->fcc = V4L2_PIX_FMT_YUYV;
-	video->bpp = 16;
-	video->width = 320;
-	video->height = 240;
-	video->imagesize = 320 * 240 * 2;
 
 	/* Initialize the video buffers queue. */
 	uvcg_queue_init(&video->queue, uvc->v4l2_dev.dev->parent,

-- 
2.30.2

Re: [PATCH 4/8] usb: gadget: uvc: move video format initialization to uvc_v4l2

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Mar 23, 2023 at 12:41:12PM +0100, Michael Tretter wrote:
> Move the setup of the initial video format to uvc_v4l2.c that handles
> all the format negotiation. This keeps all format setup and
> configuration code in uvc_v4l2.c and avoids scattering the format setup
> across multiple files.
> 
> Furthermore, it allows to setup the default format using the format
> configured in the configfs.

I'm afraid I don't see how that last sentence matches the patch.

> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/usb/gadget/function/f_uvc.c     |  2 ++
>  drivers/usb/gadget/function/uvc_v4l2.c  | 11 +++++++++++
>  drivers/usb/gadget/function/uvc_v4l2.h  |  3 +++
>  drivers/usb/gadget/function/uvc_video.c |  5 -----
>  4 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 5e919fb65833..a16c8f80a50a 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -434,6 +434,8 @@ uvc_register_video(struct uvc_device *uvc)
>  	struct usb_composite_dev *cdev = uvc->func.config->cdev;
>  	int ret;
>  
> +	uvc_init_default_format(uvc);
> +
>  	/* TODO reference counting. */
>  	memset(&uvc->vdev, 0, sizeof(uvc->vdev));
>  	uvc->vdev.v4l2_dev = &uvc->v4l2_dev;
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index 4b8bf94e06fc..5620546eb43b 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -130,6 +130,17 @@ static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc,
>  	return uformat;
>  }
>  
> +void uvc_init_default_format(struct uvc_device *uvc)
> +{
> +	struct uvc_video *video = &uvc->video;
> +
> +	video->fcc = V4L2_PIX_FMT_YUYV;
> +	video->bpp = 16;
> +	video->width = 320;
> +	video->height = 240;
> +	video->imagesize = 320 * 240 * 2;
> +}
> +

Please place the function in a better location, not in the middle of
related helpers.

>  static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc,
>  					   struct uvcg_format *uformat,
>  					   u16 rw, u16 rh)
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.h b/drivers/usb/gadget/function/uvc_v4l2.h
> index 1576005b61fd..5c3a97de0776 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.h
> +++ b/drivers/usb/gadget/function/uvc_v4l2.h
> @@ -16,4 +16,7 @@
>  extern const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops;
>  extern const struct v4l2_file_operations uvc_v4l2_fops;
>  
> +struct uvc_device;
> +void uvc_init_default_format(struct uvc_device *uvc);
> +
>  #endif /* __UVC_V4L2_H__ */
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index dd1c6b2ca7c6..27ff9ef49e16 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -516,11 +516,6 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
>  		return -EINVAL;
>  
>  	video->uvc = uvc;
> -	video->fcc = V4L2_PIX_FMT_YUYV;
> -	video->bpp = 16;
> -	video->width = 320;
> -	video->height = 240;
> -	video->imagesize = 320 * 240 * 2;

Honestly I'm not sure I see any improvement with this change. The active
format stored in the uvc_video structure is initialized in the function
that initializes the uvc_video structure, and you're moving it to an
unrelated location. I don't like this.

>  
>  	/* Initialize the video buffers queue. */
>  	uvcg_queue_init(&video->queue, uvc->v4l2_dev.dev->parent,

-- 
Regards,

Laurent Pinchart

[PATCH 5/8] usb: gadget: uvc: initialize video format using configfs

[Michael Tretter]

The configfs is supported to get a list of valid formats, which will be
checked in TRY_FMT and ENUM_FMT. The device should be initialized with a
valid format instead of some arbitrary format that the user space won't
be able to set.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/usb/gadget/function/uvc_v4l2.c | 67 +++++++++++++++++++++++++++++++---
 1 file changed, 62 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 5620546eb43b..3f728f451ed5 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -90,6 +90,15 @@ static struct uvcg_format *find_format_by_index(struct uvc_device *uvc, int inde
 	return uformat;
 }
 
+static struct uvcg_format *get_default_format(struct uvc_device *uvc)
+{
+	/*
+	 * UVC does not specify which format index shall be used as default.
+	 * Use the first format of the descriptor as default.
+	 */
+	return find_format_by_index(uvc, 1);
+}
+
 static struct uvcg_frame *find_frame_by_index(struct uvc_device *uvc,
 				       struct uvcg_format *uformat,
 				       int index)
@@ -112,6 +121,29 @@ static struct uvcg_frame *find_frame_by_index(struct uvc_device *uvc,
 	return uframe;
 }
 
+static struct uvcg_frame *get_default_frame(struct uvc_device *uvc,
+					    struct uvcg_format *uformat)
+{
+	struct uvcg_frame *frame = NULL;
+	int frame_index = 0;
+
+	if (uformat->type == UVCG_UNCOMPRESSED) {
+		struct uvcg_uncompressed *u;
+
+		u = to_uvcg_uncompressed(&uformat->group.cg_item);
+		frame_index = u->desc.bDefaultFrameIndex;
+	} else if (uformat->type == UVCG_MJPEG) {
+		struct uvcg_mjpeg *u;
+
+		u = to_uvcg_mjpeg(&uformat->group.cg_item);
+		frame_index = u->desc.bDefaultFrameIndex;
+	}
+	if (frame_index != 0)
+		frame = find_frame_by_index(uvc, uformat, frame_index);
+
+	return frame;
+}
+
 static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc,
 					      u32 pixelformat)
 {
@@ -133,12 +165,37 @@ static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc,
 void uvc_init_default_format(struct uvc_device *uvc)
 {
 	struct uvc_video *video = &uvc->video;
+	struct uvcg_format *uformat;
+	struct uvcg_frame *uframe = NULL;
 
-	video->fcc = V4L2_PIX_FMT_YUYV;
-	video->bpp = 16;
-	video->width = 320;
-	video->height = 240;
-	video->imagesize = 320 * 240 * 2;
+	uformat = get_default_format(uvc);
+	if (uformat)
+		uframe = get_default_frame(uvc, uformat);
+
+	/* Fallback on some arbitrary default */
+	if (!uframe) {
+		video->fcc = V4L2_PIX_FMT_YUYV;
+		video->bpp = 16;
+		video->width = 320;
+		video->height = 240;
+		video->imagesize = 320 * 240 * 2;
+
+		return;
+	}
+
+	video->fcc = to_uvc_format(uformat)->fcc;
+
+	if (uformat->type == UVCG_UNCOMPRESSED) {
+		struct uvcg_uncompressed *u;
+
+		u = to_uvcg_uncompressed(&uformat->group.cg_item);
+		video->bpp = u->desc.bBitsPerPixel;
+	} else {
+		video->bpp = 0;
+	}
+	video->width = uframe->frame.w_width;
+	video->height = uframe->frame.w_height;
+	video->imagesize = uvc_get_frame_size(uformat, uframe);
 }
 
 static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc,

-- 
2.30.2

[PATCH 6/8] usb: gadget: uvc: try harder to find a valid format

[Michael Tretter]

The TRY_FMT call should try to avoid returning EINVAL. If the requested
pixelformat is not supported by the driver, the driver should fall back
to it's own default and only then fail the request.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/usb/gadget/function/uvc_v4l2.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 3f728f451ed5..c5983bb0a8d1 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -317,11 +317,15 @@ uvc_v4l2_try_format(struct file *file, void *fh, struct v4l2_format *fmt)
 		 fmt->fmt.pix.width, fmt->fmt.pix.height);
 
 	uformat = find_format_by_pix(uvc, fmt->fmt.pix.pixelformat);
+	if (!uformat)
+		uformat = get_default_format(uvc);
 	if (!uformat)
 		return -EINVAL;
 
 	uframe = find_closest_frame_by_size(uvc, uformat,
 				fmt->fmt.pix.width, fmt->fmt.pix.height);
+	if (!uframe)
+		uframe = get_default_frame(uvc, uformat);
 	if (!uframe)
 		return -EINVAL;
 

-- 
2.30.2

Re: [PATCH 6/8] usb: gadget: uvc: try harder to find a valid format

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Mar 23, 2023 at 12:41:14PM +0100, Michael Tretter wrote:
> The TRY_FMT call should try to avoid returning EINVAL. If the requested
> pixelformat is not supported by the driver, the driver should fall back
> to it's own default and only then fail the request.

It should actually never fail.

You may recall that I didn't like the TRY_FMT implementation, and I
still stand by the arguments I provided by then. If you want to handle
this properly, revert the commits that added format handling to the
driver.

> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/usb/gadget/function/uvc_v4l2.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index 3f728f451ed5..c5983bb0a8d1 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -317,11 +317,15 @@ uvc_v4l2_try_format(struct file *file, void *fh, struct v4l2_format *fmt)
>  		 fmt->fmt.pix.width, fmt->fmt.pix.height);
>  
>  	uformat = find_format_by_pix(uvc, fmt->fmt.pix.pixelformat);
> +	if (!uformat)
> +		uformat = get_default_format(uvc);
>  	if (!uformat)
>  		return -EINVAL;
>  
>  	uframe = find_closest_frame_by_size(uvc, uformat,
>  				fmt->fmt.pix.width, fmt->fmt.pix.height);
> +	if (!uframe)
> +		uframe = get_default_frame(uvc, uformat);
>  	if (!uframe)
>  		return -EINVAL;
>  

-- 
Regards,

Laurent Pinchart

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

[Michael Tretter]

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.

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;
 }
 

-- 
2.30.2

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

[Laurent Pinchart]

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

[PATCH 8/8] usb: gadget: uvc: implement s/g_parm

[Michael Tretter]

As the UVC gadget implements ENUM_FRAMEINTERVALS it should also
implement S_PARM and G_PARM to allow to get and set the frame interval.
While the driver doesn't actually do something with the frame interval,
it should still handle and store the interval correctly, if the user
space request it.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/usb/gadget/function/uvc.h      |  1 +
 drivers/usb/gadget/function/uvc_v4l2.c | 94 ++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+)

diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 6b4ab3e07173..a9a5a9d2f554 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -96,6 +96,7 @@ struct uvc_video {
 	unsigned int width;
 	unsigned int height;
 	unsigned int imagesize;
+	struct v4l2_fract timeperframe;
 	enum v4l2_colorspace colorspace;
 	enum v4l2_ycbcr_encoding ycbcr_enc;
 	enum v4l2_quantization quantization;
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 673532ff0faa..a9564dc2445d 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -185,6 +185,9 @@ void uvc_init_default_format(struct uvc_device *uvc)
 		video->xfer_func = V4L2_XFER_FUNC_SRGB;
 		video->ycbcr_enc = V4L2_YCBCR_ENC_601;
 
+		video->timeperframe.numerator = 1;
+		video->timeperframe.denominator = 30;
+
 		return;
 	}
 
@@ -209,6 +212,11 @@ void uvc_init_default_format(struct uvc_device *uvc)
 	video->quantization = V4L2_QUANTIZATION_FULL_RANGE;
 	video->xfer_func = V4L2_XFER_FUNC_SRGB;
 	video->ycbcr_enc = V4L2_YCBCR_ENC_601;
+
+	video->timeperframe.numerator = uframe->frame.dw_default_frame_interval;
+	video->timeperframe.denominator = 10000000;
+	v4l2_simplify_fraction(&video->timeperframe.numerator,
+			       &video->timeperframe.denominator, 8, 333);
 }
 
 static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc,
@@ -255,6 +263,46 @@ static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc,
 	return uframe;
 }
 
+static void find_closest_timeperframe(struct uvc_device *uvc,
+				      struct v4l2_fract *timeperframe)
+{
+	struct uvc_video *video = &uvc->video;
+	struct uvcg_format *uformat;
+	struct uvcg_frame *uframe;
+	unsigned long interval;
+	unsigned int best_interval;
+	unsigned int curr;
+	unsigned int dist;
+	unsigned int best_dist = UINT_MAX;
+	int i;
+
+	if (timeperframe->denominator == 0)
+		timeperframe->denominator = video->timeperframe.denominator;
+	if (timeperframe->numerator == 0)
+		timeperframe->numerator = video->timeperframe.numerator;
+
+	uformat = find_format_by_pix(uvc, video->fcc);
+	uframe = find_closest_frame_by_size(uvc, uformat,
+					    video->width, video->height);
+
+	interval = timeperframe->numerator * 10000000;
+	do_div(interval, timeperframe->denominator);
+
+	for (i = 0; i < uframe->frame.b_frame_interval_type; i++) {
+		curr = uframe->dw_frame_interval[i];
+		dist = interval > curr ? interval - curr : curr - interval;
+		if (dist < best_dist) {
+			best_dist = dist;
+			best_interval = curr;
+		}
+	}
+
+	timeperframe->numerator = best_interval;
+	timeperframe->denominator = 10000000;
+	v4l2_simplify_fraction(&timeperframe->numerator,
+			       &timeperframe->denominator, 8, 333);
+}
+
 /* --------------------------------------------------------------------------
  * Requests handling
  */
@@ -456,6 +504,50 @@ uvc_v4l2_enum_framesizes(struct file *file, void *fh,
 	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_outputparm *out;
+
+	if (parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT &&
+	    parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
+		return -EINVAL;
+
+	out = &parm->parm.output;
+
+	memset(out->reserved, 0, sizeof(out->reserved));
+
+	out->capability = V4L2_CAP_TIMEPERFRAME;
+	find_closest_timeperframe(uvc, &out->timeperframe);
+
+	video->timeperframe = out->timeperframe;
+
+	return 0;
+}
+
+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_outputparm *out;
+
+	if (parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT &&
+	    parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
+		return -EINVAL;
+
+	out = &parm->parm.output;
+
+	out->capability |= V4L2_CAP_TIMEPERFRAME;
+	out->timeperframe = video->timeperframe;
+
+	return 0;
+}
+
 static int
 uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
 {
@@ -671,6 +763,8 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
 	.vidioc_s_fmt_vid_out = uvc_v4l2_set_format,
 	.vidioc_enum_frameintervals = uvc_v4l2_enum_frameintervals,
 	.vidioc_enum_framesizes = uvc_v4l2_enum_framesizes,
+	.vidioc_g_parm = uvc_v4l2_g_parm,
+	.vidioc_s_parm = uvc_v4l2_s_parm,
 	.vidioc_enum_fmt_vid_out = uvc_v4l2_enum_format,
 	.vidioc_enum_output = uvc_v4l2_enum_output,
 	.vidioc_g_output = uvc_v4l2_g_output,

-- 
2.30.2

Re: [PATCH 8/8] usb: gadget: uvc: implement s/g_parm

[kernel test robot]

Hi Michael,

I love your patch! Yet something to improve:

[auto build test ERROR on 8be174835f07b2c106b9961c0775486d06112a3c]

url:    https://github.com/intel-lab-lkp/linux/commits/Michael-Tretter/usb-gadget-uvc-use-fourcc-printk-helper/20230323-194359
base:   8be174835f07b2c106b9961c0775486d06112a3c
patch link:    https://lore.kernel.org/r/20230323-uvc-gadget-cleanup-v1-8-e41f0c5d9d8e%40pengutronix.de
patch subject: [PATCH 8/8] usb: gadget: uvc: implement s/g_parm
config: hexagon-randconfig-r041-20230322 (https://download.01.org/0day-ci/archive/20230324/202303240129.Efnw3p6C-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/f6fdbbf392bbaa79e8553af32337c54a663760db
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Michael-Tretter/usb-gadget-uvc-use-fourcc-printk-helper/20230323-194359
        git checkout f6fdbbf392bbaa79e8553af32337c54a663760db
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/usb/gadget/function/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303240129.Efnw3p6C-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   In file included from drivers/usb/gadget/function/uvc_v4l2.c:24:
   In file included from drivers/usb/gadget/function/uvc.h:15:
   In file included from include/linux/usb/composite.h:27:
   In file included from include/linux/usb/gadget.h:24:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/usb/gadget/function/uvc_v4l2.c:24:
   In file included from drivers/usb/gadget/function/uvc.h:15:
   In file included from include/linux/usb/composite.h:27:
   In file included from include/linux/usb/gadget.h:24:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/usb/gadget/function/uvc_v4l2.c:24:
   In file included from drivers/usb/gadget/function/uvc.h:15:
   In file included from include/linux/usb/composite.h:27:
   In file included from include/linux/usb/gadget.h:24:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
>> drivers/usb/gadget/function/uvc_v4l2.c:289:2: warning: comparison of distinct pointer types ('typeof ((interval)) *' (aka 'unsigned long *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]
           do_div(interval, timeperframe->denominator);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
           (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
                  ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~
>> drivers/usb/gadget/function/uvc_v4l2.c:289:2: error: incompatible pointer types passing 'unsigned long *' to parameter of type 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types]
           do_div(interval, timeperframe->denominator);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:238:22: note: expanded from macro 'do_div'
                   __rem = __div64_32(&(n), __base);       \
                                      ^~~~
   include/asm-generic/div64.h:213:38: note: passing argument to parameter 'dividend' here
   extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
                                        ^
>> drivers/usb/gadget/function/uvc_v4l2.c:289:2: warning: shift count >= width of type [-Wshift-count-overflow]
           do_div(interval, timeperframe->denominator);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:234:25: note: expanded from macro 'do_div'
           } else if (likely(((n) >> 32) == 0)) {          \
                                  ^  ~~
   include/linux/compiler.h:77:40: note: expanded from macro 'likely'
   # define likely(x)      __builtin_expect(!!(x), 1)
                                               ^
   8 warnings and 1 error generated.


vim +289 drivers/usb/gadget/function/uvc_v4l2.c

   265	
   266	static void find_closest_timeperframe(struct uvc_device *uvc,
   267					      struct v4l2_fract *timeperframe)
   268	{
   269		struct uvc_video *video = &uvc->video;
   270		struct uvcg_format *uformat;
   271		struct uvcg_frame *uframe;
   272		unsigned long interval;
   273		unsigned int best_interval;
   274		unsigned int curr;
   275		unsigned int dist;
   276		unsigned int best_dist = UINT_MAX;
   277		int i;
   278	
   279		if (timeperframe->denominator == 0)
   280			timeperframe->denominator = video->timeperframe.denominator;
   281		if (timeperframe->numerator == 0)
   282			timeperframe->numerator = video->timeperframe.numerator;
   283	
   284		uformat = find_format_by_pix(uvc, video->fcc);
   285		uframe = find_closest_frame_by_size(uvc, uformat,
   286						    video->width, video->height);
   287	
   288		interval = timeperframe->numerator * 10000000;
 > 289		do_div(interval, timeperframe->denominator);
   290	
   291		for (i = 0; i < uframe->frame.b_frame_interval_type; i++) {
   292			curr = uframe->dw_frame_interval[i];
   293			dist = interval > curr ? interval - curr : curr - interval;
   294			if (dist < best_dist) {
   295				best_dist = dist;
   296				best_interval = curr;
   297			}
   298		}
   299	
   300		timeperframe->numerator = best_interval;
   301		timeperframe->denominator = 10000000;
   302		v4l2_simplify_fraction(&timeperframe->numerator,
   303				       &timeperframe->denominator, 8, 333);
   304	}
   305	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 8/8] usb: gadget: uvc: implement s/g_parm

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Mar 23, 2023 at 12:41:16PM +0100, Michael Tretter wrote:
> As the UVC gadget implements ENUM_FRAMEINTERVALS it should also
> implement S_PARM and G_PARM to allow to get and set the frame interval.
> While the driver doesn't actually do something with the frame interval,
> it should still handle and store the interval correctly, if the user
> space request it.

We've had a similar discussion before related to format handling. The
UVC gadget driver doesn't need this information, everything below is
dead code that userspace won't exercise. It will increase the kernel
size for no gain at all.

> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/usb/gadget/function/uvc.h      |  1 +
>  drivers/usb/gadget/function/uvc_v4l2.c | 94 ++++++++++++++++++++++++++++++++++
>  2 files changed, 95 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index 6b4ab3e07173..a9a5a9d2f554 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -96,6 +96,7 @@ struct uvc_video {
>  	unsigned int width;
>  	unsigned int height;
>  	unsigned int imagesize;
> +	struct v4l2_fract timeperframe;
>  	enum v4l2_colorspace colorspace;
>  	enum v4l2_ycbcr_encoding ycbcr_enc;
>  	enum v4l2_quantization quantization;
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index 673532ff0faa..a9564dc2445d 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -185,6 +185,9 @@ void uvc_init_default_format(struct uvc_device *uvc)
>  		video->xfer_func = V4L2_XFER_FUNC_SRGB;
>  		video->ycbcr_enc = V4L2_YCBCR_ENC_601;
>  
> +		video->timeperframe.numerator = 1;
> +		video->timeperframe.denominator = 30;
> +
>  		return;
>  	}
>  
> @@ -209,6 +212,11 @@ void uvc_init_default_format(struct uvc_device *uvc)
>  	video->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>  	video->xfer_func = V4L2_XFER_FUNC_SRGB;
>  	video->ycbcr_enc = V4L2_YCBCR_ENC_601;
> +
> +	video->timeperframe.numerator = uframe->frame.dw_default_frame_interval;
> +	video->timeperframe.denominator = 10000000;
> +	v4l2_simplify_fraction(&video->timeperframe.numerator,
> +			       &video->timeperframe.denominator, 8, 333);
>  }
>  
>  static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc,
> @@ -255,6 +263,46 @@ static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc,
>  	return uframe;
>  }
>  
> +static void find_closest_timeperframe(struct uvc_device *uvc,
> +				      struct v4l2_fract *timeperframe)
> +{
> +	struct uvc_video *video = &uvc->video;
> +	struct uvcg_format *uformat;
> +	struct uvcg_frame *uframe;
> +	unsigned long interval;
> +	unsigned int best_interval;
> +	unsigned int curr;
> +	unsigned int dist;
> +	unsigned int best_dist = UINT_MAX;
> +	int i;
> +
> +	if (timeperframe->denominator == 0)
> +		timeperframe->denominator = video->timeperframe.denominator;
> +	if (timeperframe->numerator == 0)
> +		timeperframe->numerator = video->timeperframe.numerator;
> +
> +	uformat = find_format_by_pix(uvc, video->fcc);
> +	uframe = find_closest_frame_by_size(uvc, uformat,
> +					    video->width, video->height);
> +
> +	interval = timeperframe->numerator * 10000000;
> +	do_div(interval, timeperframe->denominator);
> +
> +	for (i = 0; i < uframe->frame.b_frame_interval_type; i++) {
> +		curr = uframe->dw_frame_interval[i];
> +		dist = interval > curr ? interval - curr : curr - interval;
> +		if (dist < best_dist) {
> +			best_dist = dist;
> +			best_interval = curr;
> +		}
> +	}
> +
> +	timeperframe->numerator = best_interval;
> +	timeperframe->denominator = 10000000;
> +	v4l2_simplify_fraction(&timeperframe->numerator,
> +			       &timeperframe->denominator, 8, 333);
> +}
> +
>  /* --------------------------------------------------------------------------
>   * Requests handling
>   */
> @@ -456,6 +504,50 @@ uvc_v4l2_enum_framesizes(struct file *file, void *fh,
>  	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_outputparm *out;
> +
> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> +	    parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> +		return -EINVAL;
> +
> +	out = &parm->parm.output;
> +
> +	memset(out->reserved, 0, sizeof(out->reserved));
> +
> +	out->capability = V4L2_CAP_TIMEPERFRAME;
> +	find_closest_timeperframe(uvc, &out->timeperframe);
> +
> +	video->timeperframe = out->timeperframe;
> +
> +	return 0;
> +}
> +
> +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_outputparm *out;
> +
> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> +	    parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> +		return -EINVAL;
> +
> +	out = &parm->parm.output;
> +
> +	out->capability |= V4L2_CAP_TIMEPERFRAME;
> +	out->timeperframe = video->timeperframe;
> +
> +	return 0;
> +}
> +
>  static int
>  uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
>  {
> @@ -671,6 +763,8 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
>  	.vidioc_s_fmt_vid_out = uvc_v4l2_set_format,
>  	.vidioc_enum_frameintervals = uvc_v4l2_enum_frameintervals,
>  	.vidioc_enum_framesizes = uvc_v4l2_enum_framesizes,
> +	.vidioc_g_parm = uvc_v4l2_g_parm,
> +	.vidioc_s_parm = uvc_v4l2_s_parm,
>  	.vidioc_enum_fmt_vid_out = uvc_v4l2_enum_format,
>  	.vidioc_enum_output = uvc_v4l2_enum_output,
>  	.vidioc_g_output = uvc_v4l2_g_output,
> 
> -- 
> 2.30.2

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/8] usb: gadget: uvc: fix errors reported by v4l2-compliance

[Laurent Pinchart]

Hi Michael,

On Thu, Mar 23, 2023 at 12:41:09PM +0100, Michael Tretter wrote:
> This series fixes various errors and warnings that are reported by
> v4l2-compliance for the v4l2 output device created by the UVC gadget.
> 
> Most notably, it changes the driver to take the initial format from the
> configfs instead of using a hard coded value that might be rejected later in
> the SET_FORMAT call. Note that user space is still responsible for negotiating
> the format with the UVC host.

I'm afraid I dislike most of this series (apart from the fix to
REQBUFS). It was a bad idea to add format handling to the driver in the
first place, and this series adds up code on top of that to please a
compliance tool but without any added value. This code should never be
exercised by userspace and will just bloat the kernel. That's not good.
I'd recommend reverting the series that add format handling if you want
to improve the UVC gadget driver.

> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> To: Daniel Scally <dan.scally@ideasonboard.com>
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Cc: linux-usb@vger.kernel.org
> Cc: linux-media@vger.kernel.org
> Cc: kernel@pengutronix.de
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> 
> ---
> Michael Tretter (8):
>       usb: gadget: uvc: use fourcc printk helper
>       usb: gadget: uvc: fix return code of REQBUFS
>       usb: gadget: uvc: implement s/g_output ioctl
>       usb: gadget: uvc: move video format initialization to uvc_v4l2
>       usb: gadget: uvc: initialize video format using configfs
>       usb: gadget: uvc: try harder to find a valid format
>       usb: gadget: uvc: add colorspace handling
>       usb: gadget: uvc: implement s/g_parm
> 
>  drivers/usb/gadget/function/f_uvc.c     |   2 +
>  drivers/usb/gadget/function/uvc.h       |   5 +
>  drivers/usb/gadget/function/uvc_queue.c |   6 +-
>  drivers/usb/gadget/function/uvc_v4l2.c  | 235 ++++++++++++++++++++++++++++++--
>  drivers/usb/gadget/function/uvc_v4l2.h  |   3 +
>  drivers/usb/gadget/function/uvc_video.c |   5 -
>  6 files changed, 238 insertions(+), 18 deletions(-)
> ---
> base-commit: 8be174835f07b2c106b9961c0775486d06112a3c
> change-id: 20230323-uvc-gadget-cleanup-47b1495befb9

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/8] usb: gadget: uvc: fix errors reported by v4l2-compliance

[Michael Tretter]

Hi Laurent,

On Fri, 24 Mar 2023 11:38:34 +0200, Laurent Pinchart wrote:
> On Thu, Mar 23, 2023 at 12:41:09PM +0100, Michael Tretter wrote:
> > This series fixes various errors and warnings that are reported by
> > v4l2-compliance for the v4l2 output device created by the UVC gadget.
> > 
> > Most notably, it changes the driver to take the initial format from the
> > configfs instead of using a hard coded value that might be rejected later in
> > the SET_FORMAT call. Note that user space is still responsible for negotiating
> > the format with the UVC host.
> 
> I'm afraid I dislike most of this series (apart from the fix to
> REQBUFS). It was a bad idea to add format handling to the driver in the
> first place, and this series adds up code on top of that to please a
> compliance tool but without any added value. This code should never be
> exercised by userspace and will just bloat the kernel. That's not good.

There is a userspace implementation that exercises this code: The GStreamer
v4l2uvcsink element [0] handles the format negotiation with the UVC host, but
internally uses a v4l2sink to pass the video buffers to the V4L2 output device
of the UVC gadget. The v4l2sink element treats the UVC V4L2 device like any
other V4L2 device, uses the specified ioctls, and, thus, expects compliant
behavior from the V4L2 device.

Being able to use a GStreamer pipeline with a v4l2uvcsink and v4l2sink element
to pass video data from any GStreamer source element to a UVC gadget is so
beneficial that it justifies adding the few missing V4L2 callbacks to make the
device's behavior compliant to V4L2. Especially, as the behavior of the code
can easily be exercised and tested with v4l2-compliance.

Therefore, I think the added code actually has value, as it allows to reuse
existing userspace code for V4L2 devices with the V4L2 device of the UVC
gadget.

Michael

[0] https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1304

> I'd recommend reverting the series that add format handling if you want
> to improve the UVC gadget driver.
> 
> > To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > To: Daniel Scally <dan.scally@ideasonboard.com>
> > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > Cc: linux-usb@vger.kernel.org
> > Cc: linux-media@vger.kernel.org
> > Cc: kernel@pengutronix.de
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > 
> > ---
> > Michael Tretter (8):
> >       usb: gadget: uvc: use fourcc printk helper
> >       usb: gadget: uvc: fix return code of REQBUFS
> >       usb: gadget: uvc: implement s/g_output ioctl
> >       usb: gadget: uvc: move video format initialization to uvc_v4l2
> >       usb: gadget: uvc: initialize video format using configfs
> >       usb: gadget: uvc: try harder to find a valid format
> >       usb: gadget: uvc: add colorspace handling
> >       usb: gadget: uvc: implement s/g_parm
> > 
> >  drivers/usb/gadget/function/f_uvc.c     |   2 +
> >  drivers/usb/gadget/function/uvc.h       |   5 +
> >  drivers/usb/gadget/function/uvc_queue.c |   6 +-
> >  drivers/usb/gadget/function/uvc_v4l2.c  | 235 ++++++++++++++++++++++++++++++--
> >  drivers/usb/gadget/function/uvc_v4l2.h  |   3 +
> >  drivers/usb/gadget/function/uvc_video.c |   5 -
> >  6 files changed, 238 insertions(+), 18 deletions(-)
> > ---
> > base-commit: 8be174835f07b2c106b9961c0775486d06112a3c
> > change-id: 20230323-uvc-gadget-cleanup-47b1495befb9