| Message ID | 20200418103216.140572-5-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: Register read-only sub-dev devnode | expand |
Hi Jacopo,
I love your patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on linus/master v5.7-rc2 next-20200420]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Jacopo-Mondi/media-Register-read-only-sub-dev-devnode/20200421-053419
base: git://linuxtv.org/media_tree.git master
config: arm-bcm2835_defconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project a9b137f9ffba8cb25dfd7dd1fb613e8aac121b37)
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/media/v4l2-core/v4l2-subdev.c:345:23: error: use of undeclared identifier 'ro_subdev'
cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV
^
1 error generated.
vim +/ro_subdev +345 drivers/media/v4l2-core/v4l2-subdev.c
327
328 static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
329 {
330 struct video_device *vdev = video_devdata(file);
331 struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
332 struct v4l2_fh *vfh = file->private_data;
333 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
334 struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh);
335 bool ro_subdev = test_bit(V4L2_FL_SUBDEV_RO_DEVNODE, &vdev->flags);
336 #endif
337 int rval;
338
339 switch (cmd) {
340 case VIDIOC_SUBDEV_QUERYCAP: {
341 struct v4l2_subdev_capability *cap = arg;
342
343 memset(cap, 0, sizeof(*cap));
344 cap->version = LINUX_VERSION_CODE;
> 345 cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV
346 : V4L2_SUBDEV_CAP_RW_SUBDEV;
347
348 return 0;
349 }
350
351 case VIDIOC_QUERYCTRL:
352 /*
353 * TODO: this really should be folded into v4l2_queryctrl (this
354 * currently returns -EINVAL for NULL control handlers).
355 * However, v4l2_queryctrl() is still called directly by
356 * drivers as well and until that has been addressed I believe
357 * it is safer to do the check here. The same is true for the
358 * other control ioctls below.
359 */
360 if (!vfh->ctrl_handler)
361 return -ENOTTY;
362 return v4l2_queryctrl(vfh->ctrl_handler, arg);
363
364 case VIDIOC_QUERY_EXT_CTRL:
365 if (!vfh->ctrl_handler)
366 return -ENOTTY;
367 return v4l2_query_ext_ctrl(vfh->ctrl_handler, arg);
368
369 case VIDIOC_QUERYMENU:
370 if (!vfh->ctrl_handler)
371 return -ENOTTY;
372 return v4l2_querymenu(vfh->ctrl_handler, arg);
373
374 case VIDIOC_G_CTRL:
375 if (!vfh->ctrl_handler)
376 return -ENOTTY;
377 return v4l2_g_ctrl(vfh->ctrl_handler, arg);
378
379 case VIDIOC_S_CTRL:
380 if (!vfh->ctrl_handler)
381 return -ENOTTY;
382 return v4l2_s_ctrl(vfh, vfh->ctrl_handler, arg);
383
384 case VIDIOC_G_EXT_CTRLS:
385 if (!vfh->ctrl_handler)
386 return -ENOTTY;
387 return v4l2_g_ext_ctrls(vfh->ctrl_handler,
388 vdev, sd->v4l2_dev->mdev, arg);
389
390 case VIDIOC_S_EXT_CTRLS:
391 if (!vfh->ctrl_handler)
392 return -ENOTTY;
393 return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler,
394 vdev, sd->v4l2_dev->mdev, arg);
395
396 case VIDIOC_TRY_EXT_CTRLS:
397 if (!vfh->ctrl_handler)
398 return -ENOTTY;
399 return v4l2_try_ext_ctrls(vfh->ctrl_handler,
400 vdev, sd->v4l2_dev->mdev, arg);
401
402 case VIDIOC_DQEVENT:
403 if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
404 return -ENOIOCTLCMD;
405
406 return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);
407
408 case VIDIOC_DQEVENT_TIME32: {
409 struct v4l2_event_time32 *ev32 = arg;
410 struct v4l2_event ev = { };
411
412 if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
413 return -ENOIOCTLCMD;
414
415 rval = v4l2_event_dequeue(vfh, &ev, file->f_flags & O_NONBLOCK);
416
417 *ev32 = (struct v4l2_event_time32) {
418 .type = ev.type,
419 .pending = ev.pending,
420 .sequence = ev.sequence,
421 .timestamp.tv_sec = ev.timestamp.tv_sec,
422 .timestamp.tv_nsec = ev.timestamp.tv_nsec,
423 .id = ev.id,
424 };
425
426 memcpy(&ev32->u, &ev.u, sizeof(ev.u));
427 memcpy(&ev32->reserved, &ev.reserved, sizeof(ev.reserved));
428
429 return rval;
430 }
431
432 case VIDIOC_SUBSCRIBE_EVENT:
433 return v4l2_subdev_call(sd, core, subscribe_event, vfh, arg);
434
435 case VIDIOC_UNSUBSCRIBE_EVENT:
436 return v4l2_subdev_call(sd, core, unsubscribe_event, vfh, arg);
437
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Jacopo, Thanks for the patch. On Sat, Apr 18, 2020 at 12:32:15PM +0200, Jacopo Mondi wrote: > From: Hans Verkuil <hans.verkuil@cisco.com> > > While normal video/radio/vbi/swradio nodes have a proper QUERYCAP ioctl > that apps can call to determine that it is indeed a V4L2 device, there > is currently no equivalent for v4l-subdev nodes. Adding this ioctl will > solve that, and it will allow utilities like v4l2-compliance to be used > with these devices as well. > > SUBDEV_QUERYCAP currently returns the version and subdev_caps of the > subdevice. Define as the initial set of subdev_caps the read-only or > read/write flags, to signal to userspace which set of IOCTLs are > available on the subdevice. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++++++++ > include/uapi/linux/v4l2-subdev.h | 15 +++++++++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 1dc263c2ca0a..ca0aa54429c5 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -15,6 +15,7 @@ > #include <linux/types.h> > #include <linux/videodev2.h> > #include <linux/export.h> > +#include <linux/version.h> > > #include <media/v4l2-ctrls.h> > #include <media/v4l2-device.h> > @@ -336,6 +337,17 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > int rval; > > switch (cmd) { > + case VIDIOC_SUBDEV_QUERYCAP: { > + struct v4l2_subdev_capability *cap = arg; > + > + memset(cap, 0, sizeof(*cap)); > + cap->version = LINUX_VERSION_CODE; > + cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV > + : V4L2_SUBDEV_CAP_RW_SUBDEV; > + > + return 0; > + } > + > case VIDIOC_QUERYCTRL: > /* > * TODO: this really should be folded into v4l2_queryctrl (this > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > index 03970ce30741..0886f88be193 100644 > --- a/include/uapi/linux/v4l2-subdev.h > +++ b/include/uapi/linux/v4l2-subdev.h > @@ -155,9 +155,24 @@ struct v4l2_subdev_selection { > __u32 reserved[8]; > }; > > +/** > + * struct v4l2_subdev_capability - subdev capabilities > + * @device_caps: the subdev capabilities, see V4L2_SUBDEV_CAP_*. > + */ > +struct v4l2_subdev_capability { > + __u32 version; > + __u32 subdev_caps; No reserved fields? Is the intent to extend this later on based on the size of the IOCTL argument? > +}; > + > +/* The v4l2 sub-device video device node is registered in read-only mode. */ > +#define V4L2_SUBDEV_CAP_RO_SUBDEV (1 << 0) 1U << 0 > +/* The v4l2 sub-device video device node is registered in read/write mode. */ > +#define V4L2_SUBDEV_CAP_RW_SUBDEV (1 << 1) 1U << 1 > + > /* Backwards compatibility define --- to be removed */ > #define v4l2_subdev_edid v4l2_edid > > +#define VIDIOC_SUBDEV_QUERYCAP _IOR('V', 0, struct v4l2_subdev_capability) > #define VIDIOC_SUBDEV_G_FMT _IOWR('V', 4, struct v4l2_subdev_format) > #define VIDIOC_SUBDEV_S_FMT _IOWR('V', 5, struct v4l2_subdev_format) > #define VIDIOC_SUBDEV_G_FRAME_INTERVAL _IOWR('V', 21, struct v4l2_subdev_frame_interval)
Hi Sakari, On Wed, Apr 22, 2020 at 12:49:58AM +0300, Sakari Ailus wrote: > On Sat, Apr 18, 2020 at 12:32:15PM +0200, Jacopo Mondi wrote: > > From: Hans Verkuil <hans.verkuil@cisco.com> > > > > While normal video/radio/vbi/swradio nodes have a proper QUERYCAP ioctl > > that apps can call to determine that it is indeed a V4L2 device, there > > is currently no equivalent for v4l-subdev nodes. Adding this ioctl will > > solve that, and it will allow utilities like v4l2-compliance to be used > > with these devices as well. > > > > SUBDEV_QUERYCAP currently returns the version and subdev_caps of the > > subdevice. Define as the initial set of subdev_caps the read-only or > > read/write flags, to signal to userspace which set of IOCTLs are > > available on the subdevice. > > > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++++++++ > > include/uapi/linux/v4l2-subdev.h | 15 +++++++++++++++ > > 2 files changed, 27 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index 1dc263c2ca0a..ca0aa54429c5 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -15,6 +15,7 @@ > > #include <linux/types.h> > > #include <linux/videodev2.h> > > #include <linux/export.h> > > +#include <linux/version.h> > > > > #include <media/v4l2-ctrls.h> > > #include <media/v4l2-device.h> > > @@ -336,6 +337,17 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > int rval; > > > > switch (cmd) { > > + case VIDIOC_SUBDEV_QUERYCAP: { > > + struct v4l2_subdev_capability *cap = arg; > > + > > + memset(cap, 0, sizeof(*cap)); > > + cap->version = LINUX_VERSION_CODE; > > + cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV > > + : V4L2_SUBDEV_CAP_RW_SUBDEV; > > + > > + return 0; > > + } > > + > > case VIDIOC_QUERYCTRL: > > /* > > * TODO: this really should be folded into v4l2_queryctrl (this > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > > index 03970ce30741..0886f88be193 100644 > > --- a/include/uapi/linux/v4l2-subdev.h > > +++ b/include/uapi/linux/v4l2-subdev.h > > @@ -155,9 +155,24 @@ struct v4l2_subdev_selection { > > __u32 reserved[8]; > > }; > > > > +/** > > + * struct v4l2_subdev_capability - subdev capabilities > > + * @device_caps: the subdev capabilities, see V4L2_SUBDEV_CAP_*. > > + */ > > +struct v4l2_subdev_capability { > > + __u32 version; > > + __u32 subdev_caps; > > No reserved fields? Is the intent to extend this later on based on the size > of the IOCTL argument? That would be my preferred option. > > +}; > > + > > +/* The v4l2 sub-device video device node is registered in read-only mode. */ > > +#define V4L2_SUBDEV_CAP_RO_SUBDEV (1 << 0) > > 1U << 0 BIT(0) > > > +/* The v4l2 sub-device video device node is registered in read/write mode. */ > > +#define V4L2_SUBDEV_CAP_RW_SUBDEV (1 << 1) > > 1U << 1 BIT(1) > > + > > /* Backwards compatibility define --- to be removed */ > > #define v4l2_subdev_edid v4l2_edid > > > > +#define VIDIOC_SUBDEV_QUERYCAP _IOR('V', 0, struct v4l2_subdev_capability) > > #define VIDIOC_SUBDEV_G_FMT _IOWR('V', 4, struct v4l2_subdev_format) > > #define VIDIOC_SUBDEV_S_FMT _IOWR('V', 5, struct v4l2_subdev_format) > > #define VIDIOC_SUBDEV_G_FRAME_INTERVAL _IOWR('V', 21, struct v4l2_subdev_frame_interval)
Hi Sakari, On Wed, Apr 22, 2020 at 02:42:32AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Wed, Apr 22, 2020 at 12:49:58AM +0300, Sakari Ailus wrote: > > On Sat, Apr 18, 2020 at 12:32:15PM +0200, Jacopo Mondi wrote: > > > From: Hans Verkuil <hans.verkuil@cisco.com> > > > > > > While normal video/radio/vbi/swradio nodes have a proper QUERYCAP ioctl > > > that apps can call to determine that it is indeed a V4L2 device, there > > > is currently no equivalent for v4l-subdev nodes. Adding this ioctl will > > > solve that, and it will allow utilities like v4l2-compliance to be used > > > with these devices as well. > > > > > > SUBDEV_QUERYCAP currently returns the version and subdev_caps of the > > > subdevice. Define as the initial set of subdev_caps the read-only or > > > read/write flags, to signal to userspace which set of IOCTLs are > > > available on the subdevice. > > > > > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++++++++ > > > include/uapi/linux/v4l2-subdev.h | 15 +++++++++++++++ > > > 2 files changed, 27 insertions(+) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > index 1dc263c2ca0a..ca0aa54429c5 100644 > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > @@ -15,6 +15,7 @@ > > > #include <linux/types.h> > > > #include <linux/videodev2.h> > > > #include <linux/export.h> > > > +#include <linux/version.h> > > > > > > #include <media/v4l2-ctrls.h> > > > #include <media/v4l2-device.h> > > > @@ -336,6 +337,17 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > > int rval; > > > > > > switch (cmd) { > > > + case VIDIOC_SUBDEV_QUERYCAP: { > > > + struct v4l2_subdev_capability *cap = arg; > > > + > > > + memset(cap, 0, sizeof(*cap)); > > > + cap->version = LINUX_VERSION_CODE; > > > + cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV > > > + : V4L2_SUBDEV_CAP_RW_SUBDEV; > > > + > > > + return 0; > > > + } > > > + > > > case VIDIOC_QUERYCTRL: > > > /* > > > * TODO: this really should be folded into v4l2_queryctrl (this > > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > > > index 03970ce30741..0886f88be193 100644 > > > --- a/include/uapi/linux/v4l2-subdev.h > > > +++ b/include/uapi/linux/v4l2-subdev.h > > > @@ -155,9 +155,24 @@ struct v4l2_subdev_selection { > > > __u32 reserved[8]; > > > }; > > > > > > +/** > > > + * struct v4l2_subdev_capability - subdev capabilities > > > + * @device_caps: the subdev capabilities, see V4L2_SUBDEV_CAP_*. > > > + */ > > > +struct v4l2_subdev_capability { > > > + __u32 version; > > > + __u32 subdev_caps; > > > > No reserved fields? Is the intent to extend this later on based on the size > > of the IOCTL argument? > > That would be my preferred option. > That would be my preferred opinion as well, we would have versioning and would not be tied to the available reserved fields.
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 1dc263c2ca0a..ca0aa54429c5 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -15,6 +15,7 @@ #include <linux/types.h> #include <linux/videodev2.h> #include <linux/export.h> +#include <linux/version.h> #include <media/v4l2-ctrls.h> #include <media/v4l2-device.h> @@ -336,6 +337,17 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) int rval; switch (cmd) { + case VIDIOC_SUBDEV_QUERYCAP: { + struct v4l2_subdev_capability *cap = arg; + + memset(cap, 0, sizeof(*cap)); + cap->version = LINUX_VERSION_CODE; + cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV + : V4L2_SUBDEV_CAP_RW_SUBDEV; + + return 0; + } + case VIDIOC_QUERYCTRL: /* * TODO: this really should be folded into v4l2_queryctrl (this diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h index 03970ce30741..0886f88be193 100644 --- a/include/uapi/linux/v4l2-subdev.h +++ b/include/uapi/linux/v4l2-subdev.h @@ -155,9 +155,24 @@ struct v4l2_subdev_selection { __u32 reserved[8]; }; +/** + * struct v4l2_subdev_capability - subdev capabilities + * @device_caps: the subdev capabilities, see V4L2_SUBDEV_CAP_*. + */ +struct v4l2_subdev_capability { + __u32 version; + __u32 subdev_caps; +}; + +/* The v4l2 sub-device video device node is registered in read-only mode. */ +#define V4L2_SUBDEV_CAP_RO_SUBDEV (1 << 0) +/* The v4l2 sub-device video device node is registered in read/write mode. */ +#define V4L2_SUBDEV_CAP_RW_SUBDEV (1 << 1) + /* Backwards compatibility define --- to be removed */ #define v4l2_subdev_edid v4l2_edid +#define VIDIOC_SUBDEV_QUERYCAP _IOR('V', 0, struct v4l2_subdev_capability) #define VIDIOC_SUBDEV_G_FMT _IOWR('V', 4, struct v4l2_subdev_format) #define VIDIOC_SUBDEV_S_FMT _IOWR('V', 5, struct v4l2_subdev_format) #define VIDIOC_SUBDEV_G_FRAME_INTERVAL _IOWR('V', 21, struct v4l2_subdev_frame_interval)