| Message ID | 20230323-uvc-gadget-cleanup-v1-3-e41f0c5d9d8e@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: uvc: fix errors reported by v4l2-compliance | expand |
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,
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,
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,
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 --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,
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(+)