diff mbox series

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

Message ID 20230323-uvc-gadget-cleanup-v1-3-e41f0c5d9d8e@pengutronix.de
State New
Headers show
Series usb: gadget: uvc: fix errors reported by v4l2-compliance | expand

Commit Message

Michael Tretter March 23, 2023, 11:41 a.m. UTC 
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(+)

Comments

Laurent Pinchart March 24, 2023, 9:20 a.m. UTC | #1 
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,
Dan Scally March 24, 2023, 9:21 a.m. UTC | #2 
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,
Hans Verkuil March 24, 2023, 9:39 a.m. UTC | #3 
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,
Laurent Pinchart March 24, 2023, 9:49 a.m. UTC | #4 
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,
diff mbox series

Patch

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,