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

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Dan Scally <dan.scally@ideasonboard.com>,
	Michael Tretter <m.tretter@pengutronix.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Michael Grzeschik <m.grzeschik@pengutronix.de>,
	linux-usb@vger.kernel.org, linux-media@vger.kernel.org,
	kernel@pengutronix.de
Subject: Re: [PATCH 3/8] usb: gadget: uvc: implement s/g_output ioctl
Date: Fri, 24 Mar 2023 11:49:00 +0200	[thread overview]
Message-ID: <20230324094900.GL18895@pendragon.ideasonboard.com> (raw)
In-Reply-To: <caa72a5c-a40f-e5e1-84c5-44d376cbe87c@xs4all.nl>

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