| Message ID | 20230323-uvc-gadget-cleanup-v1-8-e41f0c5d9d8e@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: uvc: fix errors reported by v4l2-compliance | expand |
Hi Michael,
I love your patch! Yet something to improve:
[auto build test ERROR on 8be174835f07b2c106b9961c0775486d06112a3c]
url: https://github.com/intel-lab-lkp/linux/commits/Michael-Tretter/usb-gadget-uvc-use-fourcc-printk-helper/20230323-194359
base: 8be174835f07b2c106b9961c0775486d06112a3c
patch link: https://lore.kernel.org/r/20230323-uvc-gadget-cleanup-v1-8-e41f0c5d9d8e%40pengutronix.de
patch subject: [PATCH 8/8] usb: gadget: uvc: implement s/g_parm
config: hexagon-randconfig-r041-20230322 (https://download.01.org/0day-ci/archive/20230324/202303240129.Efnw3p6C-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/f6fdbbf392bbaa79e8553af32337c54a663760db
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Michael-Tretter/usb-gadget-uvc-use-fourcc-printk-helper/20230323-194359
git checkout f6fdbbf392bbaa79e8553af32337c54a663760db
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/usb/gadget/function/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303240129.Efnw3p6C-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
In file included from drivers/usb/gadget/function/uvc_v4l2.c:24:
In file included from drivers/usb/gadget/function/uvc.h:15:
In file included from include/linux/usb/composite.h:27:
In file included from include/linux/usb/gadget.h:24:
In file included from include/linux/scatterlist.h:9:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/usb/gadget/function/uvc_v4l2.c:24:
In file included from drivers/usb/gadget/function/uvc.h:15:
In file included from include/linux/usb/composite.h:27:
In file included from include/linux/usb/gadget.h:24:
In file included from include/linux/scatterlist.h:9:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/usb/gadget/function/uvc_v4l2.c:24:
In file included from drivers/usb/gadget/function/uvc.h:15:
In file included from include/linux/usb/composite.h:27:
In file included from include/linux/usb/gadget.h:24:
In file included from include/linux/scatterlist.h:9:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> drivers/usb/gadget/function/uvc_v4l2.c:289:2: warning: comparison of distinct pointer types ('typeof ((interval)) *' (aka 'unsigned long *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]
do_div(interval, timeperframe->denominator);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
(void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
>> drivers/usb/gadget/function/uvc_v4l2.c:289:2: error: incompatible pointer types passing 'unsigned long *' to parameter of type 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types]
do_div(interval, timeperframe->denominator);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:238:22: note: expanded from macro 'do_div'
__rem = __div64_32(&(n), __base); \
^~~~
include/asm-generic/div64.h:213:38: note: passing argument to parameter 'dividend' here
extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
^
>> drivers/usb/gadget/function/uvc_v4l2.c:289:2: warning: shift count >= width of type [-Wshift-count-overflow]
do_div(interval, timeperframe->denominator);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:234:25: note: expanded from macro 'do_div'
} else if (likely(((n) >> 32) == 0)) { \
^ ~~
include/linux/compiler.h:77:40: note: expanded from macro 'likely'
# define likely(x) __builtin_expect(!!(x), 1)
^
8 warnings and 1 error generated.
vim +289 drivers/usb/gadget/function/uvc_v4l2.c
265
266 static void find_closest_timeperframe(struct uvc_device *uvc,
267 struct v4l2_fract *timeperframe)
268 {
269 struct uvc_video *video = &uvc->video;
270 struct uvcg_format *uformat;
271 struct uvcg_frame *uframe;
272 unsigned long interval;
273 unsigned int best_interval;
274 unsigned int curr;
275 unsigned int dist;
276 unsigned int best_dist = UINT_MAX;
277 int i;
278
279 if (timeperframe->denominator == 0)
280 timeperframe->denominator = video->timeperframe.denominator;
281 if (timeperframe->numerator == 0)
282 timeperframe->numerator = video->timeperframe.numerator;
283
284 uformat = find_format_by_pix(uvc, video->fcc);
285 uframe = find_closest_frame_by_size(uvc, uformat,
286 video->width, video->height);
287
288 interval = timeperframe->numerator * 10000000;
> 289 do_div(interval, timeperframe->denominator);
290
291 for (i = 0; i < uframe->frame.b_frame_interval_type; i++) {
292 curr = uframe->dw_frame_interval[i];
293 dist = interval > curr ? interval - curr : curr - interval;
294 if (dist < best_dist) {
295 best_dist = dist;
296 best_interval = curr;
297 }
298 }
299
300 timeperframe->numerator = best_interval;
301 timeperframe->denominator = 10000000;
302 v4l2_simplify_fraction(&timeperframe->numerator,
303 &timeperframe->denominator, 8, 333);
304 }
305
Hi Michael, Thank you for the patch. On Thu, Mar 23, 2023 at 12:41:16PM +0100, Michael Tretter wrote: > As the UVC gadget implements ENUM_FRAMEINTERVALS it should also > implement S_PARM and G_PARM to allow to get and set the frame interval. > While the driver doesn't actually do something with the frame interval, > it should still handle and store the interval correctly, if the user > space request it. We've had a similar discussion before related to format handling. The UVC gadget driver doesn't need this information, everything below is dead code that userspace won't exercise. It will increase the kernel size for no gain at all. > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > --- > drivers/usb/gadget/function/uvc.h | 1 + > drivers/usb/gadget/function/uvc_v4l2.c | 94 ++++++++++++++++++++++++++++++++++ > 2 files changed, 95 insertions(+) > > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > index 6b4ab3e07173..a9a5a9d2f554 100644 > --- a/drivers/usb/gadget/function/uvc.h > +++ b/drivers/usb/gadget/function/uvc.h > @@ -96,6 +96,7 @@ struct uvc_video { > unsigned int width; > unsigned int height; > unsigned int imagesize; > + struct v4l2_fract timeperframe; > enum v4l2_colorspace colorspace; > enum v4l2_ycbcr_encoding ycbcr_enc; > enum v4l2_quantization quantization; > diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c > index 673532ff0faa..a9564dc2445d 100644 > --- a/drivers/usb/gadget/function/uvc_v4l2.c > +++ b/drivers/usb/gadget/function/uvc_v4l2.c > @@ -185,6 +185,9 @@ void uvc_init_default_format(struct uvc_device *uvc) > video->xfer_func = V4L2_XFER_FUNC_SRGB; > video->ycbcr_enc = V4L2_YCBCR_ENC_601; > > + video->timeperframe.numerator = 1; > + video->timeperframe.denominator = 30; > + > return; > } > > @@ -209,6 +212,11 @@ void uvc_init_default_format(struct uvc_device *uvc) > video->quantization = V4L2_QUANTIZATION_FULL_RANGE; > video->xfer_func = V4L2_XFER_FUNC_SRGB; > video->ycbcr_enc = V4L2_YCBCR_ENC_601; > + > + video->timeperframe.numerator = uframe->frame.dw_default_frame_interval; > + video->timeperframe.denominator = 10000000; > + v4l2_simplify_fraction(&video->timeperframe.numerator, > + &video->timeperframe.denominator, 8, 333); > } > > static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc, > @@ -255,6 +263,46 @@ static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc, > return uframe; > } > > +static void find_closest_timeperframe(struct uvc_device *uvc, > + struct v4l2_fract *timeperframe) > +{ > + struct uvc_video *video = &uvc->video; > + struct uvcg_format *uformat; > + struct uvcg_frame *uframe; > + unsigned long interval; > + unsigned int best_interval; > + unsigned int curr; > + unsigned int dist; > + unsigned int best_dist = UINT_MAX; > + int i; > + > + if (timeperframe->denominator == 0) > + timeperframe->denominator = video->timeperframe.denominator; > + if (timeperframe->numerator == 0) > + timeperframe->numerator = video->timeperframe.numerator; > + > + uformat = find_format_by_pix(uvc, video->fcc); > + uframe = find_closest_frame_by_size(uvc, uformat, > + video->width, video->height); > + > + interval = timeperframe->numerator * 10000000; > + do_div(interval, timeperframe->denominator); > + > + for (i = 0; i < uframe->frame.b_frame_interval_type; i++) { > + curr = uframe->dw_frame_interval[i]; > + dist = interval > curr ? interval - curr : curr - interval; > + if (dist < best_dist) { > + best_dist = dist; > + best_interval = curr; > + } > + } > + > + timeperframe->numerator = best_interval; > + timeperframe->denominator = 10000000; > + v4l2_simplify_fraction(&timeperframe->numerator, > + &timeperframe->denominator, 8, 333); > +} > + > /* -------------------------------------------------------------------------- > * Requests handling > */ > @@ -456,6 +504,50 @@ uvc_v4l2_enum_framesizes(struct file *file, void *fh, > return 0; > } > > +static int > +uvc_v4l2_s_parm(struct file *file, void *fh, struct v4l2_streamparm *parm) > +{ > + struct video_device *vdev = video_devdata(file); > + struct uvc_device *uvc = video_get_drvdata(vdev); > + struct uvc_video *video = &uvc->video; > + struct v4l2_outputparm *out; > + > + if (parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT && > + parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > + return -EINVAL; > + > + out = &parm->parm.output; > + > + memset(out->reserved, 0, sizeof(out->reserved)); > + > + out->capability = V4L2_CAP_TIMEPERFRAME; > + find_closest_timeperframe(uvc, &out->timeperframe); > + > + video->timeperframe = out->timeperframe; > + > + return 0; > +} > + > +static int > +uvc_v4l2_g_parm(struct file *file, void *fh, struct v4l2_streamparm *parm) > +{ > + struct video_device *vdev = video_devdata(file); > + struct uvc_device *uvc = video_get_drvdata(vdev); > + struct uvc_video *video = &uvc->video; > + struct v4l2_outputparm *out; > + > + if (parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT && > + parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > + return -EINVAL; > + > + out = &parm->parm.output; > + > + out->capability |= V4L2_CAP_TIMEPERFRAME; > + out->timeperframe = video->timeperframe; > + > + return 0; > +} > + > static int > uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f) > { > @@ -671,6 +763,8 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = { > .vidioc_s_fmt_vid_out = uvc_v4l2_set_format, > .vidioc_enum_frameintervals = uvc_v4l2_enum_frameintervals, > .vidioc_enum_framesizes = uvc_v4l2_enum_framesizes, > + .vidioc_g_parm = uvc_v4l2_g_parm, > + .vidioc_s_parm = uvc_v4l2_s_parm, > .vidioc_enum_fmt_vid_out = uvc_v4l2_enum_format, > .vidioc_enum_output = uvc_v4l2_enum_output, > .vidioc_g_output = uvc_v4l2_g_output, > > -- > 2.30.2
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index 6b4ab3e07173..a9a5a9d2f554 100644 --- a/drivers/usb/gadget/function/uvc.h +++ b/drivers/usb/gadget/function/uvc.h @@ -96,6 +96,7 @@ struct uvc_video { unsigned int width; unsigned int height; unsigned int imagesize; + struct v4l2_fract timeperframe; enum v4l2_colorspace colorspace; enum v4l2_ycbcr_encoding ycbcr_enc; enum v4l2_quantization quantization; diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c index 673532ff0faa..a9564dc2445d 100644 --- a/drivers/usb/gadget/function/uvc_v4l2.c +++ b/drivers/usb/gadget/function/uvc_v4l2.c @@ -185,6 +185,9 @@ void uvc_init_default_format(struct uvc_device *uvc) video->xfer_func = V4L2_XFER_FUNC_SRGB; video->ycbcr_enc = V4L2_YCBCR_ENC_601; + video->timeperframe.numerator = 1; + video->timeperframe.denominator = 30; + return; } @@ -209,6 +212,11 @@ void uvc_init_default_format(struct uvc_device *uvc) video->quantization = V4L2_QUANTIZATION_FULL_RANGE; video->xfer_func = V4L2_XFER_FUNC_SRGB; video->ycbcr_enc = V4L2_YCBCR_ENC_601; + + video->timeperframe.numerator = uframe->frame.dw_default_frame_interval; + video->timeperframe.denominator = 10000000; + v4l2_simplify_fraction(&video->timeperframe.numerator, + &video->timeperframe.denominator, 8, 333); } static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc, @@ -255,6 +263,46 @@ static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc, return uframe; } +static void find_closest_timeperframe(struct uvc_device *uvc, + struct v4l2_fract *timeperframe) +{ + struct uvc_video *video = &uvc->video; + struct uvcg_format *uformat; + struct uvcg_frame *uframe; + unsigned long interval; + unsigned int best_interval; + unsigned int curr; + unsigned int dist; + unsigned int best_dist = UINT_MAX; + int i; + + if (timeperframe->denominator == 0) + timeperframe->denominator = video->timeperframe.denominator; + if (timeperframe->numerator == 0) + timeperframe->numerator = video->timeperframe.numerator; + + uformat = find_format_by_pix(uvc, video->fcc); + uframe = find_closest_frame_by_size(uvc, uformat, + video->width, video->height); + + interval = timeperframe->numerator * 10000000; + do_div(interval, timeperframe->denominator); + + for (i = 0; i < uframe->frame.b_frame_interval_type; i++) { + curr = uframe->dw_frame_interval[i]; + dist = interval > curr ? interval - curr : curr - interval; + if (dist < best_dist) { + best_dist = dist; + best_interval = curr; + } + } + + timeperframe->numerator = best_interval; + timeperframe->denominator = 10000000; + v4l2_simplify_fraction(&timeperframe->numerator, + &timeperframe->denominator, 8, 333); +} + /* -------------------------------------------------------------------------- * Requests handling */ @@ -456,6 +504,50 @@ uvc_v4l2_enum_framesizes(struct file *file, void *fh, return 0; } +static int +uvc_v4l2_s_parm(struct file *file, void *fh, struct v4l2_streamparm *parm) +{ + struct video_device *vdev = video_devdata(file); + struct uvc_device *uvc = video_get_drvdata(vdev); + struct uvc_video *video = &uvc->video; + struct v4l2_outputparm *out; + + if (parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT && + parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) + return -EINVAL; + + out = &parm->parm.output; + + memset(out->reserved, 0, sizeof(out->reserved)); + + out->capability = V4L2_CAP_TIMEPERFRAME; + find_closest_timeperframe(uvc, &out->timeperframe); + + video->timeperframe = out->timeperframe; + + return 0; +} + +static int +uvc_v4l2_g_parm(struct file *file, void *fh, struct v4l2_streamparm *parm) +{ + struct video_device *vdev = video_devdata(file); + struct uvc_device *uvc = video_get_drvdata(vdev); + struct uvc_video *video = &uvc->video; + struct v4l2_outputparm *out; + + if (parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT && + parm->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) + return -EINVAL; + + out = &parm->parm.output; + + out->capability |= V4L2_CAP_TIMEPERFRAME; + out->timeperframe = video->timeperframe; + + return 0; +} + static int uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f) { @@ -671,6 +763,8 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = { .vidioc_s_fmt_vid_out = uvc_v4l2_set_format, .vidioc_enum_frameintervals = uvc_v4l2_enum_frameintervals, .vidioc_enum_framesizes = uvc_v4l2_enum_framesizes, + .vidioc_g_parm = uvc_v4l2_g_parm, + .vidioc_s_parm = uvc_v4l2_s_parm, .vidioc_enum_fmt_vid_out = uvc_v4l2_enum_format, .vidioc_enum_output = uvc_v4l2_enum_output, .vidioc_g_output = uvc_v4l2_g_output,
As the UVC gadget implements ENUM_FRAMEINTERVALS it should also implement S_PARM and G_PARM to allow to get and set the frame interval. While the driver doesn't actually do something with the frame interval, it should still handle and store the interval correctly, if the user space request it. Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> --- drivers/usb/gadget/function/uvc.h | 1 + drivers/usb/gadget/function/uvc_v4l2.c | 94 ++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+)