drivers/media/usb/uvc/uvc_v4l2.c | 162 +++++++++++---------------------------- 1 file changed, 43 insertions(+), 119 deletions(-)
The *_vid_cap and *_vid_out helpers seem to be identical:
- Remove all the cap/out duplicated code.
- Remove s/g_parm helpers
- Reorder uvc_ioctl_ops
And now that we are at it, fix a comment for uvc_acquire_privileges()
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Unless I miss something, cap and out helpers are identical. So there is
no need to duplicate code
---
Changes in v3:
- if (ret < 0)
- Link to v2: https://lore.kernel.org/r/20241129-uvc-dup-cap-out-v2-1-596cb9bdd5e8@chromium.org
Changes in v2:
- Add missing acquire_privileges.
- Also remove helper for s/g_parm.
- Reorder callbacks.
- Link to v1: https://lore.kernel.org/r/20241127-uvc-dup-cap-out-v1-1-1bdcad2dabb0@chromium.org
---
drivers/media/usb/uvc/uvc_v4l2.c | 162 +++++++++++----------------------------
1 file changed, 43 insertions(+), 119 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 97c5407f6603..dee6feeba274 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -26,6 +26,8 @@
#include "uvcvideo.h"
+static int uvc_acquire_privileges(struct uvc_fh *handle);
+
static int uvc_control_add_xu_mapping(struct uvc_video_chain *chain,
struct uvc_control_mapping *map,
const struct uvc_xu_control_mapping *xmap)
@@ -361,9 +363,11 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
return ret;
}
-static int uvc_v4l2_get_format(struct uvc_streaming *stream,
- struct v4l2_format *fmt)
+static int uvc_ioctl_g_fmt(struct file *file, void *fh,
+ struct v4l2_format *fmt)
{
+ struct uvc_fh *handle = fh;
+ struct uvc_streaming *stream = handle->stream;
const struct uvc_format *format;
const struct uvc_frame *frame;
int ret = 0;
@@ -395,14 +399,20 @@ static int uvc_v4l2_get_format(struct uvc_streaming *stream,
return ret;
}
-static int uvc_v4l2_set_format(struct uvc_streaming *stream,
- struct v4l2_format *fmt)
+static int uvc_ioctl_s_fmt(struct file *file, void *fh,
+ struct v4l2_format *fmt)
{
+ struct uvc_fh *handle = fh;
+ struct uvc_streaming *stream = handle->stream;
struct uvc_streaming_control probe;
const struct uvc_format *format;
const struct uvc_frame *frame;
int ret;
+ ret = uvc_acquire_privileges(handle);
+ if (ret < 0)
+ return ret;
+
if (fmt->type != stream->type)
return -EINVAL;
@@ -426,10 +436,12 @@ static int uvc_v4l2_set_format(struct uvc_streaming *stream,
return ret;
}
-static int uvc_v4l2_get_streamparm(struct uvc_streaming *stream,
- struct v4l2_streamparm *parm)
+static int uvc_ioctl_g_parm(struct file *file, void *fh,
+ struct v4l2_streamparm *parm)
{
u32 numerator, denominator;
+ struct uvc_fh *handle = fh;
+ struct uvc_streaming *stream = handle->stream;
if (parm->type != stream->type)
return -EINVAL;
@@ -461,9 +473,11 @@ static int uvc_v4l2_get_streamparm(struct uvc_streaming *stream,
return 0;
}
-static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
- struct v4l2_streamparm *parm)
+static int uvc_ioctl_s_parm(struct file *file, void *fh,
+ struct v4l2_streamparm *parm)
{
+ struct uvc_fh *handle = fh;
+ struct uvc_streaming *stream = handle->stream;
struct uvc_streaming_control probe;
struct v4l2_fract timeperframe;
const struct uvc_format *format;
@@ -472,6 +486,10 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
unsigned int i;
int ret;
+ ret = uvc_acquire_privileges(handle);
+ if (ret < 0)
+ return ret;
+
if (parm->type != stream->type)
return -EINVAL;
@@ -573,6 +591,7 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
* - VIDIOC_S_INPUT
* - VIDIOC_S_PARM
* - VIDIOC_S_FMT
+ * - VIDIOC_CREATE_BUFS
* - VIDIOC_REQBUFS
*/
static int uvc_acquire_privileges(struct uvc_fh *handle)
@@ -685,11 +704,13 @@ static int uvc_ioctl_querycap(struct file *file, void *fh,
return 0;
}
-static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
+static int uvc_ioctl_enum_fmt(struct file *file, void *fh,
struct v4l2_fmtdesc *fmt)
{
- const struct uvc_format *format;
+ struct uvc_fh *handle = fh;
+ struct uvc_streaming *stream = handle->stream;
enum v4l2_buf_type type = fmt->type;
+ const struct uvc_format *format;
u32 index = fmt->index;
if (fmt->type != stream->type || fmt->index >= stream->nformats)
@@ -707,82 +728,8 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
return 0;
}
-static int uvc_ioctl_enum_fmt_vid_cap(struct file *file, void *fh,
- struct v4l2_fmtdesc *fmt)
-{
- struct uvc_fh *handle = fh;
- struct uvc_streaming *stream = handle->stream;
-
- return uvc_ioctl_enum_fmt(stream, fmt);
-}
-
-static int uvc_ioctl_enum_fmt_vid_out(struct file *file, void *fh,
- struct v4l2_fmtdesc *fmt)
-{
- struct uvc_fh *handle = fh;
- struct uvc_streaming *stream = handle->stream;
-
- return uvc_ioctl_enum_fmt(stream, fmt);
-}
-
-static int uvc_ioctl_g_fmt_vid_cap(struct file *file, void *fh,
- struct v4l2_format *fmt)
-{
- struct uvc_fh *handle = fh;
- struct uvc_streaming *stream = handle->stream;
-
- return uvc_v4l2_get_format(stream, fmt);
-}
-
-static int uvc_ioctl_g_fmt_vid_out(struct file *file, void *fh,
- struct v4l2_format *fmt)
-{
- struct uvc_fh *handle = fh;
- struct uvc_streaming *stream = handle->stream;
-
- return uvc_v4l2_get_format(stream, fmt);
-}
-
-static int uvc_ioctl_s_fmt_vid_cap(struct file *file, void *fh,
- struct v4l2_format *fmt)
-{
- struct uvc_fh *handle = fh;
- struct uvc_streaming *stream = handle->stream;
- int ret;
-
- ret = uvc_acquire_privileges(handle);
- if (ret < 0)
- return ret;
-
- return uvc_v4l2_set_format(stream, fmt);
-}
-
-static int uvc_ioctl_s_fmt_vid_out(struct file *file, void *fh,
- struct v4l2_format *fmt)
-{
- struct uvc_fh *handle = fh;
- struct uvc_streaming *stream = handle->stream;
- int ret;
-
- ret = uvc_acquire_privileges(handle);
- if (ret < 0)
- return ret;
-
- return uvc_v4l2_set_format(stream, fmt);
-}
-
-static int uvc_ioctl_try_fmt_vid_cap(struct file *file, void *fh,
- struct v4l2_format *fmt)
-{
- struct uvc_fh *handle = fh;
- struct uvc_streaming *stream = handle->stream;
- struct uvc_streaming_control probe;
-
- return uvc_v4l2_try_format(stream, fmt, &probe, NULL, NULL);
-}
-
-static int uvc_ioctl_try_fmt_vid_out(struct file *file, void *fh,
- struct v4l2_format *fmt)
+static int uvc_ioctl_try_fmt(struct file *file, void *fh,
+ struct v4l2_format *fmt)
{
struct uvc_fh *handle = fh;
struct uvc_streaming *stream = handle->stream;
@@ -1212,29 +1159,6 @@ static int uvc_ioctl_g_selection(struct file *file, void *fh,
return 0;
}
-static int uvc_ioctl_g_parm(struct file *file, void *fh,
- struct v4l2_streamparm *parm)
-{
- struct uvc_fh *handle = fh;
- struct uvc_streaming *stream = handle->stream;
-
- return uvc_v4l2_get_streamparm(stream, parm);
-}
-
-static int uvc_ioctl_s_parm(struct file *file, void *fh,
- struct v4l2_streamparm *parm)
-{
- struct uvc_fh *handle = fh;
- struct uvc_streaming *stream = handle->stream;
- int ret;
-
- ret = uvc_acquire_privileges(handle);
- if (ret < 0)
- return ret;
-
- return uvc_v4l2_set_streamparm(stream, parm);
-}
-
static int uvc_ioctl_enum_framesizes(struct file *file, void *fh,
struct v4l2_frmsizeenum *fsize)
{
@@ -1543,15 +1467,17 @@ static unsigned long uvc_v4l2_get_unmapped_area(struct file *file,
#endif
const struct v4l2_ioctl_ops uvc_ioctl_ops = {
+ .vidioc_g_fmt_vid_cap = uvc_ioctl_g_fmt,
+ .vidioc_g_fmt_vid_out = uvc_ioctl_g_fmt,
+ .vidioc_s_fmt_vid_cap = uvc_ioctl_s_fmt,
+ .vidioc_s_fmt_vid_out = uvc_ioctl_s_fmt,
+ .vidioc_g_parm = uvc_ioctl_g_parm,
+ .vidioc_s_parm = uvc_ioctl_s_parm,
.vidioc_querycap = uvc_ioctl_querycap,
- .vidioc_enum_fmt_vid_cap = uvc_ioctl_enum_fmt_vid_cap,
- .vidioc_enum_fmt_vid_out = uvc_ioctl_enum_fmt_vid_out,
- .vidioc_g_fmt_vid_cap = uvc_ioctl_g_fmt_vid_cap,
- .vidioc_g_fmt_vid_out = uvc_ioctl_g_fmt_vid_out,
- .vidioc_s_fmt_vid_cap = uvc_ioctl_s_fmt_vid_cap,
- .vidioc_s_fmt_vid_out = uvc_ioctl_s_fmt_vid_out,
- .vidioc_try_fmt_vid_cap = uvc_ioctl_try_fmt_vid_cap,
- .vidioc_try_fmt_vid_out = uvc_ioctl_try_fmt_vid_out,
+ .vidioc_enum_fmt_vid_cap = uvc_ioctl_enum_fmt,
+ .vidioc_enum_fmt_vid_out = uvc_ioctl_enum_fmt,
+ .vidioc_try_fmt_vid_cap = uvc_ioctl_try_fmt,
+ .vidioc_try_fmt_vid_out = uvc_ioctl_try_fmt,
.vidioc_reqbufs = uvc_ioctl_reqbufs,
.vidioc_querybuf = uvc_ioctl_querybuf,
.vidioc_qbuf = uvc_ioctl_qbuf,
@@ -1570,8 +1496,6 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
.vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,
.vidioc_querymenu = uvc_ioctl_querymenu,
.vidioc_g_selection = uvc_ioctl_g_selection,
- .vidioc_g_parm = uvc_ioctl_g_parm,
- .vidioc_s_parm = uvc_ioctl_s_parm,
.vidioc_enum_framesizes = uvc_ioctl_enum_framesizes,
.vidioc_enum_frameintervals = uvc_ioctl_enum_frameintervals,
.vidioc_subscribe_event = uvc_ioctl_subscribe_event,
---
base-commit: 72ad4ff638047bbbdf3232178fea4bec1f429319
change-id: 20241127-uvc-dup-cap-out-6a03c01e30a3
Best regards,
--
Ricardo Ribalda <ribalda@chromium.org>Hi Ricardo,
Thank you for the patch.
On Mon, Dec 02, 2024 at 01:37:35PM +0000, Ricardo Ribalda wrote:
> The *_vid_cap and *_vid_out helpers seem to be identical:
> - Remove all the cap/out duplicated code.
> - Remove s/g_parm helpers
> - Reorder uvc_ioctl_ops
>
> And now that we are at it, fix a comment for uvc_acquire_privileges()
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> Unless I miss something, cap and out helpers are identical. So there is
> no need to duplicate code
> ---
> Changes in v3:
> - if (ret < 0)
> - Link to v2: https://lore.kernel.org/r/20241129-uvc-dup-cap-out-v2-1-596cb9bdd5e8@chromium.org
>
> Changes in v2:
> - Add missing acquire_privileges.
> - Also remove helper for s/g_parm.
> - Reorder callbacks.
> - Link to v1: https://lore.kernel.org/r/20241127-uvc-dup-cap-out-v1-1-1bdcad2dabb0@chromium.org
> ---
> drivers/media/usb/uvc/uvc_v4l2.c | 162 +++++++++++----------------------------
> 1 file changed, 43 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 97c5407f6603..dee6feeba274 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -26,6 +26,8 @@
>
> #include "uvcvideo.h"
>
> +static int uvc_acquire_privileges(struct uvc_fh *handle);
> +
I don't like forward declarations, they're often a sign of bad code
design. In most cases they can be avoided just by moving functions
around. I'll send a patch to reshuffle functions in this file, as Hans
has already merged this patch and it will be easier to get the result
I'd like by doing it myself instead of asking you to do it for me.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> static int uvc_control_add_xu_mapping(struct uvc_video_chain *chain,
> struct uvc_control_mapping *map,
> const struct uvc_xu_control_mapping *xmap)
> @@ -361,9 +363,11 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
> return ret;
> }
>
> -static int uvc_v4l2_get_format(struct uvc_streaming *stream,
> - struct v4l2_format *fmt)
> +static int uvc_ioctl_g_fmt(struct file *file, void *fh,
> + struct v4l2_format *fmt)
> {
> + struct uvc_fh *handle = fh;
> + struct uvc_streaming *stream = handle->stream;
> const struct uvc_format *format;
> const struct uvc_frame *frame;
> int ret = 0;
> @@ -395,14 +399,20 @@ static int uvc_v4l2_get_format(struct uvc_streaming *stream,
> return ret;
> }
>
> -static int uvc_v4l2_set_format(struct uvc_streaming *stream,
> - struct v4l2_format *fmt)
> +static int uvc_ioctl_s_fmt(struct file *file, void *fh,
> + struct v4l2_format *fmt)
> {
> + struct uvc_fh *handle = fh;
> + struct uvc_streaming *stream = handle->stream;
> struct uvc_streaming_control probe;
> const struct uvc_format *format;
> const struct uvc_frame *frame;
> int ret;
>
> + ret = uvc_acquire_privileges(handle);
> + if (ret < 0)
> + return ret;
> +
> if (fmt->type != stream->type)
> return -EINVAL;
>
> @@ -426,10 +436,12 @@ static int uvc_v4l2_set_format(struct uvc_streaming *stream,
> return ret;
> }
>
> -static int uvc_v4l2_get_streamparm(struct uvc_streaming *stream,
> - struct v4l2_streamparm *parm)
> +static int uvc_ioctl_g_parm(struct file *file, void *fh,
> + struct v4l2_streamparm *parm)
> {
> u32 numerator, denominator;
> + struct uvc_fh *handle = fh;
> + struct uvc_streaming *stream = handle->stream;
>
> if (parm->type != stream->type)
> return -EINVAL;
> @@ -461,9 +473,11 @@ static int uvc_v4l2_get_streamparm(struct uvc_streaming *stream,
> return 0;
> }
>
> -static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
> - struct v4l2_streamparm *parm)
> +static int uvc_ioctl_s_parm(struct file *file, void *fh,
> + struct v4l2_streamparm *parm)
> {
> + struct uvc_fh *handle = fh;
> + struct uvc_streaming *stream = handle->stream;
> struct uvc_streaming_control probe;
> struct v4l2_fract timeperframe;
> const struct uvc_format *format;
> @@ -472,6 +486,10 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
> unsigned int i;
> int ret;
>
> + ret = uvc_acquire_privileges(handle);
> + if (ret < 0)
> + return ret;
> +
> if (parm->type != stream->type)
> return -EINVAL;
>
> @@ -573,6 +591,7 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
> * - VIDIOC_S_INPUT
> * - VIDIOC_S_PARM
> * - VIDIOC_S_FMT
> + * - VIDIOC_CREATE_BUFS
> * - VIDIOC_REQBUFS
> */
> static int uvc_acquire_privileges(struct uvc_fh *handle)
> @@ -685,11 +704,13 @@ static int uvc_ioctl_querycap(struct file *file, void *fh,
> return 0;
> }
>
> -static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
> +static int uvc_ioctl_enum_fmt(struct file *file, void *fh,
> struct v4l2_fmtdesc *fmt)
> {
> - const struct uvc_format *format;
> + struct uvc_fh *handle = fh;
> + struct uvc_streaming *stream = handle->stream;
> enum v4l2_buf_type type = fmt->type;
> + const struct uvc_format *format;
> u32 index = fmt->index;
>
> if (fmt->type != stream->type || fmt->index >= stream->nformats)
> @@ -707,82 +728,8 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
> return 0;
> }
>
> -static int uvc_ioctl_enum_fmt_vid_cap(struct file *file, void *fh,
> - struct v4l2_fmtdesc *fmt)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> -
> - return uvc_ioctl_enum_fmt(stream, fmt);
> -}
> -
> -static int uvc_ioctl_enum_fmt_vid_out(struct file *file, void *fh,
> - struct v4l2_fmtdesc *fmt)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> -
> - return uvc_ioctl_enum_fmt(stream, fmt);
> -}
> -
> -static int uvc_ioctl_g_fmt_vid_cap(struct file *file, void *fh,
> - struct v4l2_format *fmt)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> -
> - return uvc_v4l2_get_format(stream, fmt);
> -}
> -
> -static int uvc_ioctl_g_fmt_vid_out(struct file *file, void *fh,
> - struct v4l2_format *fmt)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> -
> - return uvc_v4l2_get_format(stream, fmt);
> -}
> -
> -static int uvc_ioctl_s_fmt_vid_cap(struct file *file, void *fh,
> - struct v4l2_format *fmt)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> - int ret;
> -
> - ret = uvc_acquire_privileges(handle);
> - if (ret < 0)
> - return ret;
> -
> - return uvc_v4l2_set_format(stream, fmt);
> -}
> -
> -static int uvc_ioctl_s_fmt_vid_out(struct file *file, void *fh,
> - struct v4l2_format *fmt)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> - int ret;
> -
> - ret = uvc_acquire_privileges(handle);
> - if (ret < 0)
> - return ret;
> -
> - return uvc_v4l2_set_format(stream, fmt);
> -}
> -
> -static int uvc_ioctl_try_fmt_vid_cap(struct file *file, void *fh,
> - struct v4l2_format *fmt)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> - struct uvc_streaming_control probe;
> -
> - return uvc_v4l2_try_format(stream, fmt, &probe, NULL, NULL);
> -}
> -
> -static int uvc_ioctl_try_fmt_vid_out(struct file *file, void *fh,
> - struct v4l2_format *fmt)
> +static int uvc_ioctl_try_fmt(struct file *file, void *fh,
> + struct v4l2_format *fmt)
> {
> struct uvc_fh *handle = fh;
> struct uvc_streaming *stream = handle->stream;
> @@ -1212,29 +1159,6 @@ static int uvc_ioctl_g_selection(struct file *file, void *fh,
> return 0;
> }
>
> -static int uvc_ioctl_g_parm(struct file *file, void *fh,
> - struct v4l2_streamparm *parm)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> -
> - return uvc_v4l2_get_streamparm(stream, parm);
> -}
> -
> -static int uvc_ioctl_s_parm(struct file *file, void *fh,
> - struct v4l2_streamparm *parm)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> - int ret;
> -
> - ret = uvc_acquire_privileges(handle);
> - if (ret < 0)
> - return ret;
> -
> - return uvc_v4l2_set_streamparm(stream, parm);
> -}
> -
> static int uvc_ioctl_enum_framesizes(struct file *file, void *fh,
> struct v4l2_frmsizeenum *fsize)
> {
> @@ -1543,15 +1467,17 @@ static unsigned long uvc_v4l2_get_unmapped_area(struct file *file,
> #endif
>
> const struct v4l2_ioctl_ops uvc_ioctl_ops = {
> + .vidioc_g_fmt_vid_cap = uvc_ioctl_g_fmt,
> + .vidioc_g_fmt_vid_out = uvc_ioctl_g_fmt,
> + .vidioc_s_fmt_vid_cap = uvc_ioctl_s_fmt,
> + .vidioc_s_fmt_vid_out = uvc_ioctl_s_fmt,
> + .vidioc_g_parm = uvc_ioctl_g_parm,
> + .vidioc_s_parm = uvc_ioctl_s_parm,
> .vidioc_querycap = uvc_ioctl_querycap,
> - .vidioc_enum_fmt_vid_cap = uvc_ioctl_enum_fmt_vid_cap,
> - .vidioc_enum_fmt_vid_out = uvc_ioctl_enum_fmt_vid_out,
> - .vidioc_g_fmt_vid_cap = uvc_ioctl_g_fmt_vid_cap,
> - .vidioc_g_fmt_vid_out = uvc_ioctl_g_fmt_vid_out,
> - .vidioc_s_fmt_vid_cap = uvc_ioctl_s_fmt_vid_cap,
> - .vidioc_s_fmt_vid_out = uvc_ioctl_s_fmt_vid_out,
> - .vidioc_try_fmt_vid_cap = uvc_ioctl_try_fmt_vid_cap,
> - .vidioc_try_fmt_vid_out = uvc_ioctl_try_fmt_vid_out,
> + .vidioc_enum_fmt_vid_cap = uvc_ioctl_enum_fmt,
> + .vidioc_enum_fmt_vid_out = uvc_ioctl_enum_fmt,
> + .vidioc_try_fmt_vid_cap = uvc_ioctl_try_fmt,
> + .vidioc_try_fmt_vid_out = uvc_ioctl_try_fmt,
> .vidioc_reqbufs = uvc_ioctl_reqbufs,
> .vidioc_querybuf = uvc_ioctl_querybuf,
> .vidioc_qbuf = uvc_ioctl_qbuf,
> @@ -1570,8 +1496,6 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
> .vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,
> .vidioc_querymenu = uvc_ioctl_querymenu,
> .vidioc_g_selection = uvc_ioctl_g_selection,
> - .vidioc_g_parm = uvc_ioctl_g_parm,
> - .vidioc_s_parm = uvc_ioctl_s_parm,
> .vidioc_enum_framesizes = uvc_ioctl_enum_framesizes,
> .vidioc_enum_frameintervals = uvc_ioctl_enum_frameintervals,
> .vidioc_subscribe_event = uvc_ioctl_subscribe_event,
>
> ---
> base-commit: 72ad4ff638047bbbdf3232178fea4bec1f429319
> change-id: 20241127-uvc-dup-cap-out-6a03c01e30a3
--
Regards,
Laurent Pinchart
Hi,
On 2-Dec-24 2:37 PM, Ricardo Ribalda wrote:
> The *_vid_cap and *_vid_out helpers seem to be identical:
> - Remove all the cap/out duplicated code.
> - Remove s/g_parm helpers
> - Reorder uvc_ioctl_ops
>
> And now that we are at it, fix a comment for uvc_acquire_privileges()
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> Unless I miss something, cap and out helpers are identical. So there is
> no need to duplicate code
Thank you for your patch.
I have merged this into:
https://gitlab.freedesktop.org/linux-media/users/uvc/-/commits/next/
Regards,
Hans
> ---
> Changes in v3:
> - if (ret < 0)
> - Link to v2: https://lore.kernel.org/r/20241129-uvc-dup-cap-out-v2-1-596cb9bdd5e8@chromium.org
>
> Changes in v2:
> - Add missing acquire_privileges.
> - Also remove helper for s/g_parm.
> - Reorder callbacks.
> - Link to v1: https://lore.kernel.org/r/20241127-uvc-dup-cap-out-v1-1-1bdcad2dabb0@chromium.org
> ---
> drivers/media/usb/uvc/uvc_v4l2.c | 162 +++++++++++----------------------------
> 1 file changed, 43 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 97c5407f6603..dee6feeba274 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -26,6 +26,8 @@
>
> #include "uvcvideo.h"
>
> +static int uvc_acquire_privileges(struct uvc_fh *handle);
> +
> static int uvc_control_add_xu_mapping(struct uvc_video_chain *chain,
> struct uvc_control_mapping *map,
> const struct uvc_xu_control_mapping *xmap)
> @@ -361,9 +363,11 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
> return ret;
> }
>
> -static int uvc_v4l2_get_format(struct uvc_streaming *stream,
> - struct v4l2_format *fmt)
> +static int uvc_ioctl_g_fmt(struct file *file, void *fh,
> + struct v4l2_format *fmt)
> {
> + struct uvc_fh *handle = fh;
> + struct uvc_streaming *stream = handle->stream;
> const struct uvc_format *format;
> const struct uvc_frame *frame;
> int ret = 0;
> @@ -395,14 +399,20 @@ static int uvc_v4l2_get_format(struct uvc_streaming *stream,
> return ret;
> }
>
> -static int uvc_v4l2_set_format(struct uvc_streaming *stream,
> - struct v4l2_format *fmt)
> +static int uvc_ioctl_s_fmt(struct file *file, void *fh,
> + struct v4l2_format *fmt)
> {
> + struct uvc_fh *handle = fh;
> + struct uvc_streaming *stream = handle->stream;
> struct uvc_streaming_control probe;
> const struct uvc_format *format;
> const struct uvc_frame *frame;
> int ret;
>
> + ret = uvc_acquire_privileges(handle);
> + if (ret < 0)
> + return ret;
> +
> if (fmt->type != stream->type)
> return -EINVAL;
>
> @@ -426,10 +436,12 @@ static int uvc_v4l2_set_format(struct uvc_streaming *stream,
> return ret;
> }
>
> -static int uvc_v4l2_get_streamparm(struct uvc_streaming *stream,
> - struct v4l2_streamparm *parm)
> +static int uvc_ioctl_g_parm(struct file *file, void *fh,
> + struct v4l2_streamparm *parm)
> {
> u32 numerator, denominator;
> + struct uvc_fh *handle = fh;
> + struct uvc_streaming *stream = handle->stream;
>
> if (parm->type != stream->type)
> return -EINVAL;
> @@ -461,9 +473,11 @@ static int uvc_v4l2_get_streamparm(struct uvc_streaming *stream,
> return 0;
> }
>
> -static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
> - struct v4l2_streamparm *parm)
> +static int uvc_ioctl_s_parm(struct file *file, void *fh,
> + struct v4l2_streamparm *parm)
> {
> + struct uvc_fh *handle = fh;
> + struct uvc_streaming *stream = handle->stream;
> struct uvc_streaming_control probe;
> struct v4l2_fract timeperframe;
> const struct uvc_format *format;
> @@ -472,6 +486,10 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
> unsigned int i;
> int ret;
>
> + ret = uvc_acquire_privileges(handle);
> + if (ret < 0)
> + return ret;
> +
> if (parm->type != stream->type)
> return -EINVAL;
>
> @@ -573,6 +591,7 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
> * - VIDIOC_S_INPUT
> * - VIDIOC_S_PARM
> * - VIDIOC_S_FMT
> + * - VIDIOC_CREATE_BUFS
> * - VIDIOC_REQBUFS
> */
> static int uvc_acquire_privileges(struct uvc_fh *handle)
> @@ -685,11 +704,13 @@ static int uvc_ioctl_querycap(struct file *file, void *fh,
> return 0;
> }
>
> -static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
> +static int uvc_ioctl_enum_fmt(struct file *file, void *fh,
> struct v4l2_fmtdesc *fmt)
> {
> - const struct uvc_format *format;
> + struct uvc_fh *handle = fh;
> + struct uvc_streaming *stream = handle->stream;
> enum v4l2_buf_type type = fmt->type;
> + const struct uvc_format *format;
> u32 index = fmt->index;
>
> if (fmt->type != stream->type || fmt->index >= stream->nformats)
> @@ -707,82 +728,8 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
> return 0;
> }
>
> -static int uvc_ioctl_enum_fmt_vid_cap(struct file *file, void *fh,
> - struct v4l2_fmtdesc *fmt)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> -
> - return uvc_ioctl_enum_fmt(stream, fmt);
> -}
> -
> -static int uvc_ioctl_enum_fmt_vid_out(struct file *file, void *fh,
> - struct v4l2_fmtdesc *fmt)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> -
> - return uvc_ioctl_enum_fmt(stream, fmt);
> -}
> -
> -static int uvc_ioctl_g_fmt_vid_cap(struct file *file, void *fh,
> - struct v4l2_format *fmt)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> -
> - return uvc_v4l2_get_format(stream, fmt);
> -}
> -
> -static int uvc_ioctl_g_fmt_vid_out(struct file *file, void *fh,
> - struct v4l2_format *fmt)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> -
> - return uvc_v4l2_get_format(stream, fmt);
> -}
> -
> -static int uvc_ioctl_s_fmt_vid_cap(struct file *file, void *fh,
> - struct v4l2_format *fmt)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> - int ret;
> -
> - ret = uvc_acquire_privileges(handle);
> - if (ret < 0)
> - return ret;
> -
> - return uvc_v4l2_set_format(stream, fmt);
> -}
> -
> -static int uvc_ioctl_s_fmt_vid_out(struct file *file, void *fh,
> - struct v4l2_format *fmt)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> - int ret;
> -
> - ret = uvc_acquire_privileges(handle);
> - if (ret < 0)
> - return ret;
> -
> - return uvc_v4l2_set_format(stream, fmt);
> -}
> -
> -static int uvc_ioctl_try_fmt_vid_cap(struct file *file, void *fh,
> - struct v4l2_format *fmt)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> - struct uvc_streaming_control probe;
> -
> - return uvc_v4l2_try_format(stream, fmt, &probe, NULL, NULL);
> -}
> -
> -static int uvc_ioctl_try_fmt_vid_out(struct file *file, void *fh,
> - struct v4l2_format *fmt)
> +static int uvc_ioctl_try_fmt(struct file *file, void *fh,
> + struct v4l2_format *fmt)
> {
> struct uvc_fh *handle = fh;
> struct uvc_streaming *stream = handle->stream;
> @@ -1212,29 +1159,6 @@ static int uvc_ioctl_g_selection(struct file *file, void *fh,
> return 0;
> }
>
> -static int uvc_ioctl_g_parm(struct file *file, void *fh,
> - struct v4l2_streamparm *parm)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> -
> - return uvc_v4l2_get_streamparm(stream, parm);
> -}
> -
> -static int uvc_ioctl_s_parm(struct file *file, void *fh,
> - struct v4l2_streamparm *parm)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> - int ret;
> -
> - ret = uvc_acquire_privileges(handle);
> - if (ret < 0)
> - return ret;
> -
> - return uvc_v4l2_set_streamparm(stream, parm);
> -}
> -
> static int uvc_ioctl_enum_framesizes(struct file *file, void *fh,
> struct v4l2_frmsizeenum *fsize)
> {
> @@ -1543,15 +1467,17 @@ static unsigned long uvc_v4l2_get_unmapped_area(struct file *file,
> #endif
>
> const struct v4l2_ioctl_ops uvc_ioctl_ops = {
> + .vidioc_g_fmt_vid_cap = uvc_ioctl_g_fmt,
> + .vidioc_g_fmt_vid_out = uvc_ioctl_g_fmt,
> + .vidioc_s_fmt_vid_cap = uvc_ioctl_s_fmt,
> + .vidioc_s_fmt_vid_out = uvc_ioctl_s_fmt,
> + .vidioc_g_parm = uvc_ioctl_g_parm,
> + .vidioc_s_parm = uvc_ioctl_s_parm,
> .vidioc_querycap = uvc_ioctl_querycap,
> - .vidioc_enum_fmt_vid_cap = uvc_ioctl_enum_fmt_vid_cap,
> - .vidioc_enum_fmt_vid_out = uvc_ioctl_enum_fmt_vid_out,
> - .vidioc_g_fmt_vid_cap = uvc_ioctl_g_fmt_vid_cap,
> - .vidioc_g_fmt_vid_out = uvc_ioctl_g_fmt_vid_out,
> - .vidioc_s_fmt_vid_cap = uvc_ioctl_s_fmt_vid_cap,
> - .vidioc_s_fmt_vid_out = uvc_ioctl_s_fmt_vid_out,
> - .vidioc_try_fmt_vid_cap = uvc_ioctl_try_fmt_vid_cap,
> - .vidioc_try_fmt_vid_out = uvc_ioctl_try_fmt_vid_out,
> + .vidioc_enum_fmt_vid_cap = uvc_ioctl_enum_fmt,
> + .vidioc_enum_fmt_vid_out = uvc_ioctl_enum_fmt,
> + .vidioc_try_fmt_vid_cap = uvc_ioctl_try_fmt,
> + .vidioc_try_fmt_vid_out = uvc_ioctl_try_fmt,
> .vidioc_reqbufs = uvc_ioctl_reqbufs,
> .vidioc_querybuf = uvc_ioctl_querybuf,
> .vidioc_qbuf = uvc_ioctl_qbuf,
> @@ -1570,8 +1496,6 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
> .vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,
> .vidioc_querymenu = uvc_ioctl_querymenu,
> .vidioc_g_selection = uvc_ioctl_g_selection,
> - .vidioc_g_parm = uvc_ioctl_g_parm,
> - .vidioc_s_parm = uvc_ioctl_s_parm,
> .vidioc_enum_framesizes = uvc_ioctl_enum_framesizes,
> .vidioc_enum_frameintervals = uvc_ioctl_enum_frameintervals,
> .vidioc_subscribe_event = uvc_ioctl_subscribe_event,
>
> ---
> base-commit: 72ad4ff638047bbbdf3232178fea4bec1f429319
> change-id: 20241127-uvc-dup-cap-out-6a03c01e30a3
>
> Best regards,
© 2016 - 2024 Red Hat, Inc.