| 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 |
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,
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(+)