* [PATCH] media: v4l2-async: Add waiting subdevices debugfs @ 2020-12-28 18:05 Ezequiel Garcia 2020-12-28 18:35 ` Sakari Ailus 2020-12-29 1:09 ` kernel test robot 0 siblings, 2 replies; 14+ messages in thread From: Ezequiel Garcia @ 2020-12-28 18:05 UTC (permalink / raw) To: linux-media, Hans Verkuil Cc: kernel, Laurent Pinchart, Sakari Ailus, Ezequiel Garcia There is currently little to none information available about the reasons why a v4l2-async device hasn't probed completely. Inspired by the "devices_deferred" debugfs file, add a file to list information about the subdevices that are on waiting lists, for each notifier. This is useful to debug v4l2-async subdevices and notifiers, for instance when doing device bring-up. For instance, a typical output would be: $ cat /sys/kernel/debug/video4linux/waiting_subdevices [fwnode] 1-003c [fwnode] 20e0000.iomuxc-gpr:ipu1_csi1_mux [fwnode] 20e0000.iomuxc-gpr:ipu1_csi0_mux It's possible to provide some more information, detecting the type of fwnode and printing of-specific or acpi-specific details. For now, the implementation is kept simple. Also, note that match-type "custom" prints no information. Since there are no in-tree users of this match-type, the implementation doesn't bother. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/media/v4l2-core/v4l2-async.c | 54 ++++++++++++++++++++++++++++ drivers/media/v4l2-core/v4l2-dev.c | 5 +++ include/media/v4l2-async.h | 7 ++++ 3 files changed, 66 insertions(+) diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index e3ab003a6c85..32cd1ecced97 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -5,6 +5,7 @@ * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de> */ +#include <linux/debugfs.h> #include <linux/device.h> #include <linux/err.h> #include <linux/i2c.h> @@ -14,6 +15,7 @@ #include <linux/mutex.h> #include <linux/of.h> #include <linux/platform_device.h> +#include <linux/seq_file.h> #include <linux/slab.h> #include <linux/types.h> @@ -837,3 +839,55 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) mutex_unlock(&list_lock); } EXPORT_SYMBOL(v4l2_async_unregister_subdev); + +static void print_waiting_subdev(struct seq_file *s, + struct v4l2_async_subdev *asd) +{ + switch (asd->match_type) { + case V4L2_ASYNC_MATCH_CUSTOM: + seq_puts(s, "[custom]\n"); + break; + case V4L2_ASYNC_MATCH_DEVNAME: + seq_printf(s, "[devname] %s\n", + asd->match.device_name); + break; + case V4L2_ASYNC_MATCH_I2C: + seq_printf(s, "[i2c] %d-%04x\n", + asd->match.i2c.adapter_id, + asd->match.i2c.address); + break; + case V4L2_ASYNC_MATCH_FWNODE: { + struct fwnode_handle *fwnode = asd->match.fwnode; + + if (fwnode_graph_is_endpoint(fwnode)) + fwnode = fwnode_graph_get_port_parent(fwnode); + + seq_printf(s, "[fwnode] %s\n", + fwnode->dev ? dev_name(fwnode->dev) : "nil"); + break; + } + } +} + +static int waiting_subdevs_show(struct seq_file *s, void *data) +{ + struct v4l2_async_notifier *notifier; + struct v4l2_async_subdev *asd; + + mutex_lock(&list_lock); + + list_for_each_entry(notifier, ¬ifier_list, list) + list_for_each_entry(asd, ¬ifier->waiting, list) + print_waiting_subdev(s, asd); + + mutex_unlock(&list_lock); + + return 0; +} +DEFINE_SHOW_ATTRIBUTE(waiting_subdevs); + +void v4l2_async_debug_init(struct dentry *debugfs_dir) +{ + debugfs_create_file("waiting_subdevices", 0444, debugfs_dir, NULL, + &waiting_subdevs_fops); +} diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c index a593ea0598b5..8d3813e6ab56 100644 --- a/drivers/media/v4l2-core/v4l2-dev.c +++ b/drivers/media/v4l2-core/v4l2-dev.c @@ -14,6 +14,7 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt +#include <linux/debugfs.h> #include <linux/module.h> #include <linux/types.h> #include <linux/kernel.h> @@ -37,6 +38,7 @@ __func__, ##arg); \ } while (0) +static struct dentry *v4l2_debugfs_dir; /* * sysfs stuff @@ -1113,6 +1115,8 @@ static int __init videodev_init(void) return -EIO; } + v4l2_debugfs_dir = debugfs_create_dir("video4linux", NULL); + v4l2_async_debug_init(v4l2_debugfs_dir); return 0; } @@ -1120,6 +1124,7 @@ static void __exit videodev_exit(void) { dev_t dev = MKDEV(VIDEO_MAJOR, 0); + debugfs_remove_recursive(v4l2_debugfs_dir); class_unregister(&video_class); unregister_chrdev_region(dev, VIDEO_NUM_DEVICES); } diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h index d6e31234826f..312ab421aa40 100644 --- a/include/media/v4l2-async.h +++ b/include/media/v4l2-async.h @@ -137,6 +137,13 @@ struct v4l2_async_notifier { struct list_head list; }; +/** + * v4l2_async_debug_init - Initialize debugging tools. + * + * @debugfs_dir: pointer to the parent debugfs &struct dentry + */ +void v4l2_async_debug_init(struct dentry *debugfs_dir); + /** * v4l2_async_notifier_init - Initialize a notifier. * -- 2.29.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] media: v4l2-async: Add waiting subdevices debugfs 2020-12-28 18:05 [PATCH] media: v4l2-async: Add waiting subdevices debugfs Ezequiel Garcia @ 2020-12-28 18:35 ` Sakari Ailus 2020-12-28 21:28 ` Laurent Pinchart 2020-12-29 9:54 ` Ezequiel Garcia 2020-12-29 1:09 ` kernel test robot 1 sibling, 2 replies; 14+ messages in thread From: Sakari Ailus @ 2020-12-28 18:35 UTC (permalink / raw) To: Ezequiel Garcia; +Cc: linux-media, Hans Verkuil, kernel, Laurent Pinchart Hi Ezequiel, Thanks for the patch. On Mon, Dec 28, 2020 at 03:05:11PM -0300, Ezequiel Garcia wrote: > There is currently little to none information available > about the reasons why a v4l2-async device hasn't > probed completely. > > Inspired by the "devices_deferred" debugfs file, > add a file to list information about the subdevices > that are on waiting lists, for each notifier. > > This is useful to debug v4l2-async subdevices > and notifiers, for instance when doing device bring-up. > > For instance, a typical output would be: > > $ cat /sys/kernel/debug/video4linux/waiting_subdevices > [fwnode] 1-003c > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi1_mux > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi0_mux > > It's possible to provide some more information, detecting > the type of fwnode and printing of-specific or acpi-specific > details. For now, the implementation is kept simple. The rest of the debug information we're effectively providing through kernel messages on DEBUG level (pr_debug/dev_dbg). Could we do the same here? Would just printing the names of the pending sub-devices at notifier register and async subdevice register time be sufficient? That way you'd also be fine with just dmesg output if you're asking someone to provide you information from another system. > > Also, note that match-type "custom" prints no information. > Since there are no in-tree users of this match-type, > the implementation doesn't bother. Lines up to 74 characters are fine. Only in Gerrit it's 60 or 40 or whatever characters. ;-) > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 54 ++++++++++++++++++++++++++++ > drivers/media/v4l2-core/v4l2-dev.c | 5 +++ > include/media/v4l2-async.h | 7 ++++ > 3 files changed, 66 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index e3ab003a6c85..32cd1ecced97 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -5,6 +5,7 @@ > * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > */ > > +#include <linux/debugfs.h> > #include <linux/device.h> > #include <linux/err.h> > #include <linux/i2c.h> > @@ -14,6 +15,7 @@ > #include <linux/mutex.h> > #include <linux/of.h> > #include <linux/platform_device.h> > +#include <linux/seq_file.h> > #include <linux/slab.h> > #include <linux/types.h> > > @@ -837,3 +839,55 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > mutex_unlock(&list_lock); > } > EXPORT_SYMBOL(v4l2_async_unregister_subdev); > + > +static void print_waiting_subdev(struct seq_file *s, > + struct v4l2_async_subdev *asd) > +{ > + switch (asd->match_type) { > + case V4L2_ASYNC_MATCH_CUSTOM: > + seq_puts(s, "[custom]\n"); > + break; > + case V4L2_ASYNC_MATCH_DEVNAME: > + seq_printf(s, "[devname] %s\n", > + asd->match.device_name); > + break; > + case V4L2_ASYNC_MATCH_I2C: > + seq_printf(s, "[i2c] %d-%04x\n", > + asd->match.i2c.adapter_id, > + asd->match.i2c.address); > + break; > + case V4L2_ASYNC_MATCH_FWNODE: { > + struct fwnode_handle *fwnode = asd->match.fwnode; > + > + if (fwnode_graph_is_endpoint(fwnode)) > + fwnode = fwnode_graph_get_port_parent(fwnode); > + > + seq_printf(s, "[fwnode] %s\n", > + fwnode->dev ? dev_name(fwnode->dev) : "nil"); > + break; > + } > + } > +} > + > +static int waiting_subdevs_show(struct seq_file *s, void *data) > +{ > + struct v4l2_async_notifier *notifier; > + struct v4l2_async_subdev *asd; > + > + mutex_lock(&list_lock); > + > + list_for_each_entry(notifier, ¬ifier_list, list) > + list_for_each_entry(asd, ¬ifier->waiting, list) > + print_waiting_subdev(s, asd); > + > + mutex_unlock(&list_lock); > + > + return 0; > +} > +DEFINE_SHOW_ATTRIBUTE(waiting_subdevs); > + > +void v4l2_async_debug_init(struct dentry *debugfs_dir) > +{ > + debugfs_create_file("waiting_subdevices", 0444, debugfs_dir, NULL, > + &waiting_subdevs_fops); > +} > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c > index a593ea0598b5..8d3813e6ab56 100644 > --- a/drivers/media/v4l2-core/v4l2-dev.c > +++ b/drivers/media/v4l2-core/v4l2-dev.c > @@ -14,6 +14,7 @@ > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > +#include <linux/debugfs.h> > #include <linux/module.h> > #include <linux/types.h> > #include <linux/kernel.h> > @@ -37,6 +38,7 @@ > __func__, ##arg); \ > } while (0) > > +static struct dentry *v4l2_debugfs_dir; > > /* > * sysfs stuff > @@ -1113,6 +1115,8 @@ static int __init videodev_init(void) > return -EIO; > } > > + v4l2_debugfs_dir = debugfs_create_dir("video4linux", NULL); > + v4l2_async_debug_init(v4l2_debugfs_dir); > return 0; > } > > @@ -1120,6 +1124,7 @@ static void __exit videodev_exit(void) > { > dev_t dev = MKDEV(VIDEO_MAJOR, 0); > > + debugfs_remove_recursive(v4l2_debugfs_dir); > class_unregister(&video_class); > unregister_chrdev_region(dev, VIDEO_NUM_DEVICES); > } > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > index d6e31234826f..312ab421aa40 100644 > --- a/include/media/v4l2-async.h > +++ b/include/media/v4l2-async.h > @@ -137,6 +137,13 @@ struct v4l2_async_notifier { > struct list_head list; > }; > > +/** > + * v4l2_async_debug_init - Initialize debugging tools. > + * > + * @debugfs_dir: pointer to the parent debugfs &struct dentry > + */ > +void v4l2_async_debug_init(struct dentry *debugfs_dir); > + > /** > * v4l2_async_notifier_init - Initialize a notifier. > * -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: v4l2-async: Add waiting subdevices debugfs 2020-12-28 18:35 ` Sakari Ailus @ 2020-12-28 21:28 ` Laurent Pinchart 2020-12-29 10:16 ` Ezequiel Garcia 2021-01-02 15:24 ` Sakari Ailus 2020-12-29 9:54 ` Ezequiel Garcia 1 sibling, 2 replies; 14+ messages in thread From: Laurent Pinchart @ 2020-12-28 21:28 UTC (permalink / raw) To: Sakari Ailus; +Cc: Ezequiel Garcia, linux-media, Hans Verkuil, kernel Hello, On Mon, Dec 28, 2020 at 08:35:20PM +0200, Sakari Ailus wrote: > Hi Ezequiel, > > Thanks for the patch. Likewise :-) > On Mon, Dec 28, 2020 at 03:05:11PM -0300, Ezequiel Garcia wrote: > > There is currently little to none information available > > about the reasons why a v4l2-async device hasn't > > probed completely. > > > > Inspired by the "devices_deferred" debugfs file, > > add a file to list information about the subdevices > > that are on waiting lists, for each notifier. > > > > This is useful to debug v4l2-async subdevices > > and notifiers, for instance when doing device bring-up. > > > > For instance, a typical output would be: > > > > $ cat /sys/kernel/debug/video4linux/waiting_subdevices > > [fwnode] 1-003c > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi1_mux > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi0_mux > > > > It's possible to provide some more information, detecting > > the type of fwnode and printing of-specific or acpi-specific > > details. For now, the implementation is kept simple. > > The rest of the debug information we're effectively providing through > kernel messages on DEBUG level (pr_debug/dev_dbg). Could we do the same > here? > > Would just printing the names of the pending sub-devices at notifier > register and async subdevice register time be sufficient? That way you'd > also be fine with just dmesg output if you're asking someone to provide you > information from another system. I think debugfs would be better. It can show the current state of an async notifier in a single place, which is easier to parse than reconstructing it from kernel messages and implicit knowledge of the code. I'd expect users to have an easier time debugging probe issues with such centralized information. > > Also, note that match-type "custom" prints no information. > > Since there are no in-tree users of this match-type, > > the implementation doesn't bother. > > Lines up to 74 characters are fine. Only in Gerrit it's 60 or 40 or > whatever characters. ;-) > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > drivers/media/v4l2-core/v4l2-async.c | 54 ++++++++++++++++++++++++++++ > > drivers/media/v4l2-core/v4l2-dev.c | 5 +++ > > include/media/v4l2-async.h | 7 ++++ > > 3 files changed, 66 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index e3ab003a6c85..32cd1ecced97 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -5,6 +5,7 @@ > > * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > */ > > > > +#include <linux/debugfs.h> > > #include <linux/device.h> > > #include <linux/err.h> > > #include <linux/i2c.h> > > @@ -14,6 +15,7 @@ > > #include <linux/mutex.h> > > #include <linux/of.h> > > #include <linux/platform_device.h> > > +#include <linux/seq_file.h> > > #include <linux/slab.h> > > #include <linux/types.h> > > > > @@ -837,3 +839,55 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > > mutex_unlock(&list_lock); > > } > > EXPORT_SYMBOL(v4l2_async_unregister_subdev); > > + > > +static void print_waiting_subdev(struct seq_file *s, > > + struct v4l2_async_subdev *asd) > > +{ > > + switch (asd->match_type) { > > + case V4L2_ASYNC_MATCH_CUSTOM: > > + seq_puts(s, "[custom]\n"); > > + break; > > + case V4L2_ASYNC_MATCH_DEVNAME: > > + seq_printf(s, "[devname] %s\n", > > + asd->match.device_name); > > + break; > > + case V4L2_ASYNC_MATCH_I2C: > > + seq_printf(s, "[i2c] %d-%04x\n", > > + asd->match.i2c.adapter_id, > > + asd->match.i2c.address); > > + break; > > + case V4L2_ASYNC_MATCH_FWNODE: { > > + struct fwnode_handle *fwnode = asd->match.fwnode; > > + > > + if (fwnode_graph_is_endpoint(fwnode)) > > + fwnode = fwnode_graph_get_port_parent(fwnode); Can we also print endpoint information ? > > + > > + seq_printf(s, "[fwnode] %s\n", > > + fwnode->dev ? dev_name(fwnode->dev) : "nil"); Having no device created for a fwnode is an issue that could explain probe problems, so we should print the node name as well, not just the device. > > + break; > > + } For all of those cases, the state of the asd (matched or not matched) would be useful too, to figure out which ones are missing. > > + } > > +} > > + > > +static int waiting_subdevs_show(struct seq_file *s, void *data) > > +{ > > + struct v4l2_async_notifier *notifier; > > + struct v4l2_async_subdev *asd; > > + > > + mutex_lock(&list_lock); > > + > > + list_for_each_entry(notifier, ¬ifier_list, list) Can we print a header for each notifier, possibly with a device name ? Otherwise all async subdev entries will be printed in one big list without making it clear which notifier they belong to. > > + list_for_each_entry(asd, ¬ifier->waiting, list) > > + print_waiting_subdev(s, asd); > > + > > + mutex_unlock(&list_lock); > > + > > + return 0; > > +} > > +DEFINE_SHOW_ATTRIBUTE(waiting_subdevs); > > + > > +void v4l2_async_debug_init(struct dentry *debugfs_dir) > > +{ > > + debugfs_create_file("waiting_subdevices", 0444, debugfs_dir, NULL, > > + &waiting_subdevs_fops); > > +} > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c > > index a593ea0598b5..8d3813e6ab56 100644 > > --- a/drivers/media/v4l2-core/v4l2-dev.c > > +++ b/drivers/media/v4l2-core/v4l2-dev.c > > @@ -14,6 +14,7 @@ > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > +#include <linux/debugfs.h> > > #include <linux/module.h> > > #include <linux/types.h> > > #include <linux/kernel.h> > > @@ -37,6 +38,7 @@ > > __func__, ##arg); \ > > } while (0) > > > > +static struct dentry *v4l2_debugfs_dir; > > > > /* > > * sysfs stuff > > @@ -1113,6 +1115,8 @@ static int __init videodev_init(void) > > return -EIO; > > } > > > > + v4l2_debugfs_dir = debugfs_create_dir("video4linux", NULL); > > + v4l2_async_debug_init(v4l2_debugfs_dir); > > return 0; > > } > > > > @@ -1120,6 +1124,7 @@ static void __exit videodev_exit(void) > > { > > dev_t dev = MKDEV(VIDEO_MAJOR, 0); > > > > + debugfs_remove_recursive(v4l2_debugfs_dir); > > class_unregister(&video_class); > > unregister_chrdev_region(dev, VIDEO_NUM_DEVICES); > > } > > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > > index d6e31234826f..312ab421aa40 100644 > > --- a/include/media/v4l2-async.h > > +++ b/include/media/v4l2-async.h > > @@ -137,6 +137,13 @@ struct v4l2_async_notifier { > > struct list_head list; > > }; > > > > +/** > > + * v4l2_async_debug_init - Initialize debugging tools. > > + * > > + * @debugfs_dir: pointer to the parent debugfs &struct dentry > > + */ > > +void v4l2_async_debug_init(struct dentry *debugfs_dir); > > + > > /** > > * v4l2_async_notifier_init - Initialize a notifier. > > * -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: v4l2-async: Add waiting subdevices debugfs 2020-12-28 21:28 ` Laurent Pinchart @ 2020-12-29 10:16 ` Ezequiel Garcia 2020-12-29 14:14 ` Laurent Pinchart 2021-01-02 15:24 ` Sakari Ailus 1 sibling, 1 reply; 14+ messages in thread From: Ezequiel Garcia @ 2020-12-29 10:16 UTC (permalink / raw) To: Laurent Pinchart, Sakari Ailus; +Cc: linux-media, Hans Verkuil, kernel On Mon, 2020-12-28 at 23:28 +0200, Laurent Pinchart wrote: > Hello, > > On Mon, Dec 28, 2020 at 08:35:20PM +0200, Sakari Ailus wrote: > > Hi Ezequiel, > > > > Thanks for the patch. > > Likewise :-) > > > On Mon, Dec 28, 2020 at 03:05:11PM -0300, Ezequiel Garcia wrote: > > > There is currently little to none information available > > > about the reasons why a v4l2-async device hasn't > > > probed completely. > > > > > > Inspired by the "devices_deferred" debugfs file, > > > add a file to list information about the subdevices > > > that are on waiting lists, for each notifier. > > > > > > This is useful to debug v4l2-async subdevices > > > and notifiers, for instance when doing device bring-up. > > > > > > For instance, a typical output would be: > > > > > > $ cat /sys/kernel/debug/video4linux/waiting_subdevices > > > [fwnode] 1-003c > > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi1_mux > > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi0_mux > > > > > > It's possible to provide some more information, detecting > > > the type of fwnode and printing of-specific or acpi-specific > > > details. For now, the implementation is kept simple. > > > > The rest of the debug information we're effectively providing through > > kernel messages on DEBUG level (pr_debug/dev_dbg). Could we do the same > > here? > > > > Would just printing the names of the pending sub-devices at notifier > > register and async subdevice register time be sufficient? That way you'd > > also be fine with just dmesg output if you're asking someone to provide you > > information from another system. > > I think debugfs would be better. It can show the current state of an > async notifier in a single place, which is easier to parse than > reconstructing it from kernel messages and implicit knowledge of the > code. I'd expect users to have an easier time debugging probe issues > with such centralized information. > > > > Also, note that match-type "custom" prints no information. > > > Since there are no in-tree users of this match-type, > > > the implementation doesn't bother. > > > > Lines up to 74 characters are fine. Only in Gerrit it's 60 or 40 or > > whatever characters. ;-) > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > --- > > > drivers/media/v4l2-core/v4l2-async.c | 54 ++++++++++++++++++++++++++++ > > > drivers/media/v4l2-core/v4l2-dev.c | 5 +++ > > > include/media/v4l2-async.h | 7 ++++ > > > 3 files changed, 66 insertions(+) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > > index e3ab003a6c85..32cd1ecced97 100644 > > > --- a/drivers/media/v4l2-core/v4l2-async.c > > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > > @@ -5,6 +5,7 @@ > > > * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > > */ > > > > > > +#include <linux/debugfs.h> > > > #include <linux/device.h> > > > #include <linux/err.h> > > > #include <linux/i2c.h> > > > @@ -14,6 +15,7 @@ > > > #include <linux/mutex.h> > > > #include <linux/of.h> > > > #include <linux/platform_device.h> > > > +#include <linux/seq_file.h> > > > #include <linux/slab.h> > > > #include <linux/types.h> > > > > > > @@ -837,3 +839,55 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > > > mutex_unlock(&list_lock); > > > } > > > EXPORT_SYMBOL(v4l2_async_unregister_subdev); > > > + > > > +static void print_waiting_subdev(struct seq_file *s, > > > + struct v4l2_async_subdev *asd) > > > +{ > > > + switch (asd->match_type) { > > > + case V4L2_ASYNC_MATCH_CUSTOM: > > > + seq_puts(s, "[custom]\n"); > > > + break; > > > + case V4L2_ASYNC_MATCH_DEVNAME: > > > + seq_printf(s, "[devname] %s\n", > > > + asd->match.device_name); > > > + break; > > > + case V4L2_ASYNC_MATCH_I2C: > > > + seq_printf(s, "[i2c] %d-%04x\n", > > > + asd->match.i2c.adapter_id, > > > + asd->match.i2c.address); > > > + break; > > > + case V4L2_ASYNC_MATCH_FWNODE: { > > > + struct fwnode_handle *fwnode = asd->match.fwnode; > > > + > > > + if (fwnode_graph_is_endpoint(fwnode)) > > > + fwnode = fwnode_graph_get_port_parent(fwnode); > > Can we also print endpoint information ? > What endpoint information do you have in mind? I'm asking this because I printed endpoint OF node full names, only to find so many of them named "endpoint" :) > > > + > > > + seq_printf(s, "[fwnode] %s\n", > > > + fwnode->dev ? dev_name(fwnode->dev) : "nil"); > > Having no device created for a fwnode is an issue that could explain > probe problems, so we should print the node name as well, not just the > device. > Sure. AFAICS, there's not fwnode generic name, so we need to move one level down. For OF and software-node devices we have some name field. However ACPI device nodes don't seem to have one. Any idea what name we should print there? I'm also unsure if ACPI nodes will typically be ACPI device or ACPI data nodes. > > > + break; > > > + } > > For all of those cases, the state of the asd (matched or not matched) > would be useful too, to figure out which ones are missing. > The matched state is not kept in struct v4l2_async_subdev, or is it? AFAICS, when the asd matches, it's removed from the waiting list. You suggest to iterate over the done list and print that as well? > > > + } > > > +} > > > + > > > +static int waiting_subdevs_show(struct seq_file *s, void *data) > > > +{ > > > + struct v4l2_async_notifier *notifier; > > > + struct v4l2_async_subdev *asd; > > > + > > > + mutex_lock(&list_lock); > > > + > > > + list_for_each_entry(notifier, ¬ifier_list, list) > > Can we print a header for each notifier, possibly with a device name ? > Otherwise all async subdev entries will be printed in one big list > without making it clear which notifier they belong to. > We can try :) Thanks, Ezequiel > > > + list_for_each_entry(asd, ¬ifier->waiting, list) > > > + print_waiting_subdev(s, asd); > > > + > > > + mutex_unlock(&list_lock); > > > + > > > + return 0; > > > +} > > > +DEFINE_SHOW_ATTRIBUTE(waiting_subdevs); > > > + > > > +void v4l2_async_debug_init(struct dentry *debugfs_dir) > > > +{ > > > + debugfs_create_file("waiting_subdevices", 0444, debugfs_dir, NULL, > > > + &waiting_subdevs_fops); > > > +} > > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c > > > index a593ea0598b5..8d3813e6ab56 100644 > > > --- a/drivers/media/v4l2-core/v4l2-dev.c > > > +++ b/drivers/media/v4l2-core/v4l2-dev.c > > > @@ -14,6 +14,7 @@ > > > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > > > +#include <linux/debugfs.h> > > > #include <linux/module.h> > > > #include <linux/types.h> > > > #include <linux/kernel.h> > > > @@ -37,6 +38,7 @@ > > > __func__, ##arg); \ > > > } while (0) > > > > > > +static struct dentry *v4l2_debugfs_dir; > > > > > > /* > > > * sysfs stuff > > > @@ -1113,6 +1115,8 @@ static int __init videodev_init(void) > > > return -EIO; > > > } > > > > > > + v4l2_debugfs_dir = debugfs_create_dir("video4linux", NULL); > > > + v4l2_async_debug_init(v4l2_debugfs_dir); > > > return 0; > > > } > > > > > > @@ -1120,6 +1124,7 @@ static void __exit videodev_exit(void) > > > { > > > dev_t dev = MKDEV(VIDEO_MAJOR, 0); > > > > > > + debugfs_remove_recursive(v4l2_debugfs_dir); > > > class_unregister(&video_class); > > > unregister_chrdev_region(dev, VIDEO_NUM_DEVICES); > > > } > > > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > > > index d6e31234826f..312ab421aa40 100644 > > > --- a/include/media/v4l2-async.h > > > +++ b/include/media/v4l2-async.h > > > @@ -137,6 +137,13 @@ struct v4l2_async_notifier { > > > struct list_head list; > > > }; > > > > > > +/** > > > + * v4l2_async_debug_init - Initialize debugging tools. > > > + * > > > + * @debugfs_dir: pointer to the parent debugfs &struct dentry > > > + */ > > > +void v4l2_async_debug_init(struct dentry *debugfs_dir); > > > + > > > /** > > > * v4l2_async_notifier_init - Initialize a notifier. > > > * > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: v4l2-async: Add waiting subdevices debugfs 2020-12-29 10:16 ` Ezequiel Garcia @ 2020-12-29 14:14 ` Laurent Pinchart 2020-12-29 17:52 ` Ezequiel Garcia 0 siblings, 1 reply; 14+ messages in thread From: Laurent Pinchart @ 2020-12-29 14:14 UTC (permalink / raw) To: Ezequiel Garcia; +Cc: Sakari Ailus, linux-media, Hans Verkuil, kernel Hi Ezequiel, On Tue, Dec 29, 2020 at 07:16:41AM -0300, Ezequiel Garcia wrote: > On Mon, 2020-12-28 at 23:28 +0200, Laurent Pinchart wrote: > > On Mon, Dec 28, 2020 at 08:35:20PM +0200, Sakari Ailus wrote: > > > On Mon, Dec 28, 2020 at 03:05:11PM -0300, Ezequiel Garcia wrote: > > > > There is currently little to none information available > > > > about the reasons why a v4l2-async device hasn't > > > > probed completely. > > > > > > > > Inspired by the "devices_deferred" debugfs file, > > > > add a file to list information about the subdevices > > > > that are on waiting lists, for each notifier. > > > > > > > > This is useful to debug v4l2-async subdevices > > > > and notifiers, for instance when doing device bring-up. > > > > > > > > For instance, a typical output would be: > > > > > > > > $ cat /sys/kernel/debug/video4linux/waiting_subdevices > > > > [fwnode] 1-003c > > > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi1_mux > > > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi0_mux > > > > > > > > It's possible to provide some more information, detecting > > > > the type of fwnode and printing of-specific or acpi-specific > > > > details. For now, the implementation is kept simple. > > > > > > The rest of the debug information we're effectively providing through > > > kernel messages on DEBUG level (pr_debug/dev_dbg). Could we do the same > > > here? > > > > > > Would just printing the names of the pending sub-devices at notifier > > > register and async subdevice register time be sufficient? That way you'd > > > also be fine with just dmesg output if you're asking someone to provide you > > > information from another system. > > > > I think debugfs would be better. It can show the current state of an > > async notifier in a single place, which is easier to parse than > > reconstructing it from kernel messages and implicit knowledge of the > > code. I'd expect users to have an easier time debugging probe issues > > with such centralized information. > > > > > > Also, note that match-type "custom" prints no information. > > > > Since there are no in-tree users of this match-type, > > > > the implementation doesn't bother. > > > > > > Lines up to 74 characters are fine. Only in Gerrit it's 60 or 40 or > > > whatever characters. ;-) > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > --- > > > > drivers/media/v4l2-core/v4l2-async.c | 54 ++++++++++++++++++++++++++++ > > > > drivers/media/v4l2-core/v4l2-dev.c | 5 +++ > > > > include/media/v4l2-async.h | 7 ++++ > > > > 3 files changed, 66 insertions(+) > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > > > index e3ab003a6c85..32cd1ecced97 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-async.c > > > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > > > @@ -5,6 +5,7 @@ > > > > * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > > > */ > > > > > > > > +#include <linux/debugfs.h> > > > > #include <linux/device.h> > > > > #include <linux/err.h> > > > > #include <linux/i2c.h> > > > > @@ -14,6 +15,7 @@ > > > > #include <linux/mutex.h> > > > > #include <linux/of.h> > > > > #include <linux/platform_device.h> > > > > +#include <linux/seq_file.h> > > > > #include <linux/slab.h> > > > > #include <linux/types.h> > > > > > > > > @@ -837,3 +839,55 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > > > > mutex_unlock(&list_lock); > > > > } > > > > EXPORT_SYMBOL(v4l2_async_unregister_subdev); > > > > + > > > > +static void print_waiting_subdev(struct seq_file *s, > > > > + struct v4l2_async_subdev *asd) > > > > +{ > > > > + switch (asd->match_type) { > > > > + case V4L2_ASYNC_MATCH_CUSTOM: > > > > + seq_puts(s, "[custom]\n"); > > > > + break; > > > > + case V4L2_ASYNC_MATCH_DEVNAME: > > > > + seq_printf(s, "[devname] %s\n", > > > > + asd->match.device_name); > > > > + break; > > > > + case V4L2_ASYNC_MATCH_I2C: > > > > + seq_printf(s, "[i2c] %d-%04x\n", > > > > + asd->match.i2c.adapter_id, > > > > + asd->match.i2c.address); > > > > + break; > > > > + case V4L2_ASYNC_MATCH_FWNODE: { > > > > + struct fwnode_handle *fwnode = asd->match.fwnode; > > > > + > > > > + if (fwnode_graph_is_endpoint(fwnode)) > > > > + fwnode = fwnode_graph_get_port_parent(fwnode); > > > > Can we also print endpoint information ? > > What endpoint information do you have in mind? I'm asking this > because I printed endpoint OF node full names, only to find > so many of them named "endpoint" :) The port name and endpoint name would be useful. The full fwnode name would be an acceptable way to print that I think. > > > > + > > > > + seq_printf(s, "[fwnode] %s\n", > > > > + fwnode->dev ? dev_name(fwnode->dev) : "nil"); > > > > Having no device created for a fwnode is an issue that could explain > > probe problems, so we should print the node name as well, not just the > > device. > > Sure. > > AFAICS, there's not fwnode generic name, so we need to move one level > down. For OF and software-node devices we have some name field. > > However ACPI device nodes don't seem to have one. Any idea > what name we should print there? I'm also unsure if ACPI nodes > will typically be ACPI device or ACPI data nodes. I'll let Sakari, our ACPI expert, shime in on that :-) > > > > + break; > > > > + } > > > > For all of those cases, the state of the asd (matched or not matched) > > would be useful too, to figure out which ones are missing. > > The matched state is not kept in struct v4l2_async_subdev, or is it? > > AFAICS, when the asd matches, it's removed from the waiting list. > You suggest to iterate over the done list and print that as well? Good point and good question. I suppose there's less practical value in doing that. Maybe we could print a header at the top to mention that the list contains unmatched asds ? > > > > + } > > > > +} > > > > + > > > > +static int waiting_subdevs_show(struct seq_file *s, void *data) > > > > +{ > > > > + struct v4l2_async_notifier *notifier; > > > > + struct v4l2_async_subdev *asd; > > > > + > > > > + mutex_lock(&list_lock); > > > > + > > > > + list_for_each_entry(notifier, ¬ifier_list, list) > > > > Can we print a header for each notifier, possibly with a device name ? > > Otherwise all async subdev entries will be printed in one big list > > without making it clear which notifier they belong to. > > We can try :) > > > > > + list_for_each_entry(asd, ¬ifier->waiting, list) > > > > + print_waiting_subdev(s, asd); > > > > + > > > > + mutex_unlock(&list_lock); > > > > + > > > > + return 0; > > > > +} > > > > +DEFINE_SHOW_ATTRIBUTE(waiting_subdevs); > > > > + > > > > +void v4l2_async_debug_init(struct dentry *debugfs_dir) > > > > +{ > > > > + debugfs_create_file("waiting_subdevices", 0444, debugfs_dir, NULL, > > > > + &waiting_subdevs_fops); > > > > +} > > > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c > > > > index a593ea0598b5..8d3813e6ab56 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-dev.c > > > > +++ b/drivers/media/v4l2-core/v4l2-dev.c > > > > @@ -14,6 +14,7 @@ > > > > > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > > > > > +#include <linux/debugfs.h> > > > > #include <linux/module.h> > > > > #include <linux/types.h> > > > > #include <linux/kernel.h> > > > > @@ -37,6 +38,7 @@ > > > > __func__, ##arg); \ > > > > } while (0) > > > > > > > > +static struct dentry *v4l2_debugfs_dir; > > > > > > > > /* > > > > * sysfs stuff > > > > @@ -1113,6 +1115,8 @@ static int __init videodev_init(void) > > > > return -EIO; > > > > } > > > > > > > > + v4l2_debugfs_dir = debugfs_create_dir("video4linux", NULL); > > > > + v4l2_async_debug_init(v4l2_debugfs_dir); > > > > return 0; > > > > } > > > > > > > > @@ -1120,6 +1124,7 @@ static void __exit videodev_exit(void) > > > > { > > > > dev_t dev = MKDEV(VIDEO_MAJOR, 0); > > > > > > > > + debugfs_remove_recursive(v4l2_debugfs_dir); > > > > class_unregister(&video_class); > > > > unregister_chrdev_region(dev, VIDEO_NUM_DEVICES); > > > > } > > > > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > > > > index d6e31234826f..312ab421aa40 100644 > > > > --- a/include/media/v4l2-async.h > > > > +++ b/include/media/v4l2-async.h > > > > @@ -137,6 +137,13 @@ struct v4l2_async_notifier { > > > > struct list_head list; > > > > }; > > > > > > > > +/** > > > > + * v4l2_async_debug_init - Initialize debugging tools. > > > > + * > > > > + * @debugfs_dir: pointer to the parent debugfs &struct dentry > > > > + */ > > > > +void v4l2_async_debug_init(struct dentry *debugfs_dir); > > > > + > > > > /** > > > > * v4l2_async_notifier_init - Initialize a notifier. > > > > * -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: v4l2-async: Add waiting subdevices debugfs 2020-12-29 14:14 ` Laurent Pinchart @ 2020-12-29 17:52 ` Ezequiel Garcia 2020-12-30 1:26 ` Laurent Pinchart 2021-01-02 15:28 ` Sakari Ailus 0 siblings, 2 replies; 14+ messages in thread From: Ezequiel Garcia @ 2020-12-29 17:52 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Sakari Ailus, linux-media, Hans Verkuil, kernel On Tue, 2020-12-29 at 16:14 +0200, Laurent Pinchart wrote: > Hi Ezequiel, > > On Tue, Dec 29, 2020 at 07:16:41AM -0300, Ezequiel Garcia wrote: > > On Mon, 2020-12-28 at 23:28 +0200, Laurent Pinchart wrote: > > > On Mon, Dec 28, 2020 at 08:35:20PM +0200, Sakari Ailus wrote: > > > > On Mon, Dec 28, 2020 at 03:05:11PM -0300, Ezequiel Garcia wrote: > > > > > There is currently little to none information available > > > > > about the reasons why a v4l2-async device hasn't > > > > > probed completely. > > > > > > > > > > Inspired by the "devices_deferred" debugfs file, > > > > > add a file to list information about the subdevices > > > > > that are on waiting lists, for each notifier. > > > > > > > > > > This is useful to debug v4l2-async subdevices > > > > > and notifiers, for instance when doing device bring-up. > > > > > > > > > > For instance, a typical output would be: > > > > > > > > > > $ cat /sys/kernel/debug/video4linux/waiting_subdevices > > > > > [fwnode] 1-003c > > > > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi1_mux > > > > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi0_mux > > > > > > > > > > It's possible to provide some more information, detecting > > > > > the type of fwnode and printing of-specific or acpi-specific > > > > > details. For now, the implementation is kept simple. > > > > > > > > The rest of the debug information we're effectively providing through > > > > kernel messages on DEBUG level (pr_debug/dev_dbg). Could we do the same > > > > here? > > > > > > > > Would just printing the names of the pending sub-devices at notifier > > > > register and async subdevice register time be sufficient? That way you'd > > > > also be fine with just dmesg output if you're asking someone to provide you > > > > information from another system. > > > > > > I think debugfs would be better. It can show the current state of an > > > async notifier in a single place, which is easier to parse than > > > reconstructing it from kernel messages and implicit knowledge of the > > > code. I'd expect users to have an easier time debugging probe issues > > > with such centralized information. > > > > > > > > Also, note that match-type "custom" prints no information. > > > > > Since there are no in-tree users of this match-type, > > > > > the implementation doesn't bother. > > > > > > > > Lines up to 74 characters are fine. Only in Gerrit it's 60 or 40 or > > > > whatever characters. ;-) > > > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > > --- > > > > > drivers/media/v4l2-core/v4l2-async.c | 54 ++++++++++++++++++++++++++++ > > > > > drivers/media/v4l2-core/v4l2-dev.c | 5 +++ > > > > > include/media/v4l2-async.h | 7 ++++ > > > > > 3 files changed, 66 insertions(+) > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > > > > index e3ab003a6c85..32cd1ecced97 100644 > > > > > --- a/drivers/media/v4l2-core/v4l2-async.c > > > > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > > > > @@ -5,6 +5,7 @@ > > > > > * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > > > > */ > > > > > > > > > > +#include <linux/debugfs.h> > > > > > #include <linux/device.h> > > > > > #include <linux/err.h> > > > > > #include <linux/i2c.h> > > > > > @@ -14,6 +15,7 @@ > > > > > #include <linux/mutex.h> > > > > > #include <linux/of.h> > > > > > #include <linux/platform_device.h> > > > > > +#include <linux/seq_file.h> > > > > > #include <linux/slab.h> > > > > > #include <linux/types.h> > > > > > > > > > > @@ -837,3 +839,55 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > > > > > mutex_unlock(&list_lock); > > > > > } > > > > > EXPORT_SYMBOL(v4l2_async_unregister_subdev); > > > > > + > > > > > +static void print_waiting_subdev(struct seq_file *s, > > > > > + struct v4l2_async_subdev *asd) > > > > > +{ > > > > > + switch (asd->match_type) { > > > > > + case V4L2_ASYNC_MATCH_CUSTOM: > > > > > + seq_puts(s, "[custom]\n"); > > > > > + break; > > > > > + case V4L2_ASYNC_MATCH_DEVNAME: > > > > > + seq_printf(s, "[devname] %s\n", > > > > > + asd->match.device_name); > > > > > + break; > > > > > + case V4L2_ASYNC_MATCH_I2C: > > > > > + seq_printf(s, "[i2c] %d-%04x\n", > > > > > + asd->match.i2c.adapter_id, > > > > > + asd->match.i2c.address); > > > > > + break; > > > > > + case V4L2_ASYNC_MATCH_FWNODE: { > > > > > + struct fwnode_handle *fwnode = asd->match.fwnode; > > > > > + > > > > > + if (fwnode_graph_is_endpoint(fwnode)) > > > > > + fwnode = fwnode_graph_get_port_parent(fwnode); > > > > > > Can we also print endpoint information ? > > > > What endpoint information do you have in mind? I'm asking this > > because I printed endpoint OF node full names, only to find > > so many of them named "endpoint" :) > > The port name and endpoint name would be useful. The full fwnode name > would be an acceptable way to print that I think. > Makes sense, and since we'd be parsing the fwnode subtype, we'll be able to do something like: [of] dev=%s, node=%s [swnode] ... [acpi] ... > > > > > + > > > > > + seq_printf(s, "[fwnode] %s\n", > > > > > + fwnode->dev ? dev_name(fwnode->dev) : "nil"); > > > > > > Having no device created for a fwnode is an issue that could explain > > > probe problems, so we should print the node name as well, not just the > > > device. > > > > Sure. > > > > AFAICS, there's not fwnode generic name, so we need to move one level > > down. For OF and software-node devices we have some name field. > > > > However ACPI device nodes don't seem to have one. Any idea > > what name we should print there? I'm also unsure if ACPI nodes > > will typically be ACPI device or ACPI data nodes. > > I'll let Sakari, our ACPI expert, shime in on that :-) > > > > > > + break; > > > > > + } > > > > > > For all of those cases, the state of the asd (matched or not matched) > > > would be useful too, to figure out which ones are missing. > > > > The matched state is not kept in struct v4l2_async_subdev, or is it? > > > > AFAICS, when the asd matches, it's removed from the waiting list. > > You suggest to iterate over the done list and print that as well? > > Good point and good question. I suppose there's less practical value in > doing that. Maybe we could print a header at the top to mention that the > list contains unmatched asds ? > I was under the impression that the name of the file implied it was only unmatched/waiting subdevices. We can rename this as "unmatched_devices" or "pending_devices" if that makes things clearer. Thanks, Ezequiel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: v4l2-async: Add waiting subdevices debugfs 2020-12-29 17:52 ` Ezequiel Garcia @ 2020-12-30 1:26 ` Laurent Pinchart 2021-01-02 15:28 ` Sakari Ailus 1 sibling, 0 replies; 14+ messages in thread From: Laurent Pinchart @ 2020-12-30 1:26 UTC (permalink / raw) To: Ezequiel Garcia; +Cc: Sakari Ailus, linux-media, Hans Verkuil, kernel Hi Ezequiel, On Tue, Dec 29, 2020 at 02:52:52PM -0300, Ezequiel Garcia wrote: > On Tue, 2020-12-29 at 16:14 +0200, Laurent Pinchart wrote: > > On Tue, Dec 29, 2020 at 07:16:41AM -0300, Ezequiel Garcia wrote: > > > On Mon, 2020-12-28 at 23:28 +0200, Laurent Pinchart wrote: > > > > On Mon, Dec 28, 2020 at 08:35:20PM +0200, Sakari Ailus wrote: > > > > > On Mon, Dec 28, 2020 at 03:05:11PM -0300, Ezequiel Garcia wrote: > > > > > > There is currently little to none information available > > > > > > about the reasons why a v4l2-async device hasn't > > > > > > probed completely. > > > > > > > > > > > > Inspired by the "devices_deferred" debugfs file, > > > > > > add a file to list information about the subdevices > > > > > > that are on waiting lists, for each notifier. > > > > > > > > > > > > This is useful to debug v4l2-async subdevices > > > > > > and notifiers, for instance when doing device bring-up. > > > > > > > > > > > > For instance, a typical output would be: > > > > > > > > > > > > $ cat /sys/kernel/debug/video4linux/waiting_subdevices > > > > > > [fwnode] 1-003c > > > > > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi1_mux > > > > > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi0_mux > > > > > > > > > > > > It's possible to provide some more information, detecting > > > > > > the type of fwnode and printing of-specific or acpi-specific > > > > > > details. For now, the implementation is kept simple. > > > > > > > > > > The rest of the debug information we're effectively providing through > > > > > kernel messages on DEBUG level (pr_debug/dev_dbg). Could we do the same > > > > > here? > > > > > > > > > > Would just printing the names of the pending sub-devices at notifier > > > > > register and async subdevice register time be sufficient? That way you'd > > > > > also be fine with just dmesg output if you're asking someone to provide you > > > > > information from another system. > > > > > > > > I think debugfs would be better. It can show the current state of an > > > > async notifier in a single place, which is easier to parse than > > > > reconstructing it from kernel messages and implicit knowledge of the > > > > code. I'd expect users to have an easier time debugging probe issues > > > > with such centralized information. > > > > > > > > > > Also, note that match-type "custom" prints no information. > > > > > > Since there are no in-tree users of this match-type, > > > > > > the implementation doesn't bother. > > > > > > > > > > Lines up to 74 characters are fine. Only in Gerrit it's 60 or 40 or > > > > > whatever characters. ;-) > > > > > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > > > --- > > > > > > drivers/media/v4l2-core/v4l2-async.c | 54 ++++++++++++++++++++++++++++ > > > > > > drivers/media/v4l2-core/v4l2-dev.c | 5 +++ > > > > > > include/media/v4l2-async.h | 7 ++++ > > > > > > 3 files changed, 66 insertions(+) > > > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > > > > > index e3ab003a6c85..32cd1ecced97 100644 > > > > > > --- a/drivers/media/v4l2-core/v4l2-async.c > > > > > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > > > > > @@ -5,6 +5,7 @@ > > > > > > * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > > > > > */ > > > > > > > > > > > > +#include <linux/debugfs.h> > > > > > > #include <linux/device.h> > > > > > > #include <linux/err.h> > > > > > > #include <linux/i2c.h> > > > > > > @@ -14,6 +15,7 @@ > > > > > > #include <linux/mutex.h> > > > > > > #include <linux/of.h> > > > > > > #include <linux/platform_device.h> > > > > > > +#include <linux/seq_file.h> > > > > > > #include <linux/slab.h> > > > > > > #include <linux/types.h> > > > > > > > > > > > > @@ -837,3 +839,55 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > > > > > > mutex_unlock(&list_lock); > > > > > > } > > > > > > EXPORT_SYMBOL(v4l2_async_unregister_subdev); > > > > > > + > > > > > > +static void print_waiting_subdev(struct seq_file *s, > > > > > > + struct v4l2_async_subdev *asd) > > > > > > +{ > > > > > > + switch (asd->match_type) { > > > > > > + case V4L2_ASYNC_MATCH_CUSTOM: > > > > > > + seq_puts(s, "[custom]\n"); > > > > > > + break; > > > > > > + case V4L2_ASYNC_MATCH_DEVNAME: > > > > > > + seq_printf(s, "[devname] %s\n", > > > > > > + asd->match.device_name); > > > > > > + break; > > > > > > + case V4L2_ASYNC_MATCH_I2C: > > > > > > + seq_printf(s, "[i2c] %d-%04x\n", > > > > > > + asd->match.i2c.adapter_id, > > > > > > + asd->match.i2c.address); > > > > > > + break; > > > > > > + case V4L2_ASYNC_MATCH_FWNODE: { > > > > > > + struct fwnode_handle *fwnode = asd->match.fwnode; > > > > > > + > > > > > > + if (fwnode_graph_is_endpoint(fwnode)) > > > > > > + fwnode = fwnode_graph_get_port_parent(fwnode); > > > > > > > > Can we also print endpoint information ? > > > > > > What endpoint information do you have in mind? I'm asking this > > > because I printed endpoint OF node full names, only to find > > > so many of them named "endpoint" :) > > > > The port name and endpoint name would be useful. The full fwnode name > > would be an acceptable way to print that I think. > > Makes sense, and since we'd be parsing the fwnode subtype, > we'll be able to do something like: > > [of] dev=%s, node=%s > [swnode] ... > [acpi] ... > > > > > > > + > > > > > > + seq_printf(s, "[fwnode] %s\n", > > > > > > + fwnode->dev ? dev_name(fwnode->dev) : "nil"); > > > > > > > > Having no device created for a fwnode is an issue that could explain > > > > probe problems, so we should print the node name as well, not just the > > > > device. > > > > > > Sure. > > > > > > AFAICS, there's not fwnode generic name, so we need to move one level > > > down. For OF and software-node devices we have some name field. > > > > > > However ACPI device nodes don't seem to have one. Any idea > > > what name we should print there? I'm also unsure if ACPI nodes > > > will typically be ACPI device or ACPI data nodes. > > > > I'll let Sakari, our ACPI expert, shime in on that :-) > > > > > > > > + break; > > > > > > + } > > > > > > > > For all of those cases, the state of the asd (matched or not matched) > > > > would be useful too, to figure out which ones are missing. > > > > > > The matched state is not kept in struct v4l2_async_subdev, or is it? > > > > > > AFAICS, when the asd matches, it's removed from the waiting list. > > > You suggest to iterate over the done list and print that as well? > > > > Good point and good question. I suppose there's less practical value in > > doing that. Maybe we could print a header at the top to mention that the > > list contains unmatched asds ? > > > > I was under the impression that the name of the file implied > it was only unmatched/waiting subdevices. I had forgotten that the file name also gives information :-) > We can rename this as "unmatched_devices" or "pending_devices" > if that makes things clearer. How about async-subdev-pending or something similar ? -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: v4l2-async: Add waiting subdevices debugfs 2020-12-29 17:52 ` Ezequiel Garcia 2020-12-30 1:26 ` Laurent Pinchart @ 2021-01-02 15:28 ` Sakari Ailus 2021-01-02 15:28 ` Sakari Ailus 2021-01-04 17:55 ` Ezequiel Garcia 1 sibling, 2 replies; 14+ messages in thread From: Sakari Ailus @ 2021-01-02 15:28 UTC (permalink / raw) To: Ezequiel Garcia; +Cc: Laurent Pinchart, linux-media, Hans Verkuil, kernel On Tue, Dec 29, 2020 at 02:52:52PM -0300, Ezequiel Garcia wrote: > On Tue, 2020-12-29 at 16:14 +0200, Laurent Pinchart wrote: > > Hi Ezequiel, > > > > On Tue, Dec 29, 2020 at 07:16:41AM -0300, Ezequiel Garcia wrote: > > > On Mon, 2020-12-28 at 23:28 +0200, Laurent Pinchart wrote: > > > > On Mon, Dec 28, 2020 at 08:35:20PM +0200, Sakari Ailus wrote: > > > > > On Mon, Dec 28, 2020 at 03:05:11PM -0300, Ezequiel Garcia wrote: > > > > > > There is currently little to none information available > > > > > > about the reasons why a v4l2-async device hasn't > > > > > > probed completely. > > > > > > > > > > > > Inspired by the "devices_deferred" debugfs file, > > > > > > add a file to list information about the subdevices > > > > > > that are on waiting lists, for each notifier. > > > > > > > > > > > > This is useful to debug v4l2-async subdevices > > > > > > and notifiers, for instance when doing device bring-up. > > > > > > > > > > > > For instance, a typical output would be: > > > > > > > > > > > > $ cat /sys/kernel/debug/video4linux/waiting_subdevices > > > > > > [fwnode] 1-003c > > > > > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi1_mux > > > > > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi0_mux > > > > > > > > > > > > It's possible to provide some more information, detecting > > > > > > the type of fwnode and printing of-specific or acpi-specific > > > > > > details. For now, the implementation is kept simple. > > > > > > > > > > The rest of the debug information we're effectively providing through > > > > > kernel messages on DEBUG level (pr_debug/dev_dbg). Could we do the same > > > > > here? > > > > > > > > > > Would just printing the names of the pending sub-devices at notifier > > > > > register and async subdevice register time be sufficient? That way you'd > > > > > also be fine with just dmesg output if you're asking someone to provide you > > > > > information from another system. > > > > > > > > I think debugfs would be better. It can show the current state of an > > > > async notifier in a single place, which is easier to parse than > > > > reconstructing it from kernel messages and implicit knowledge of the > > > > code. I'd expect users to have an easier time debugging probe issues > > > > with such centralized information. > > > > > > > > > > Also, note that match-type "custom" prints no information. > > > > > > Since there are no in-tree users of this match-type, > > > > > > the implementation doesn't bother. > > > > > > > > > > Lines up to 74 characters are fine. Only in Gerrit it's 60 or 40 or > > > > > whatever characters. ;-) > > > > > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > > > --- > > > > > > drivers/media/v4l2-core/v4l2-async.c | 54 ++++++++++++++++++++++++++++ > > > > > > drivers/media/v4l2-core/v4l2-dev.c | 5 +++ > > > > > > include/media/v4l2-async.h | 7 ++++ > > > > > > 3 files changed, 66 insertions(+) > > > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > > > > > index e3ab003a6c85..32cd1ecced97 100644 > > > > > > --- a/drivers/media/v4l2-core/v4l2-async.c > > > > > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > > > > > @@ -5,6 +5,7 @@ > > > > > > * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > > > > > */ > > > > > > > > > > > > +#include <linux/debugfs.h> > > > > > > #include <linux/device.h> > > > > > > #include <linux/err.h> > > > > > > #include <linux/i2c.h> > > > > > > @@ -14,6 +15,7 @@ > > > > > > #include <linux/mutex.h> > > > > > > #include <linux/of.h> > > > > > > #include <linux/platform_device.h> > > > > > > +#include <linux/seq_file.h> > > > > > > #include <linux/slab.h> > > > > > > #include <linux/types.h> > > > > > > > > > > > > @@ -837,3 +839,55 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > > > > > > mutex_unlock(&list_lock); > > > > > > } > > > > > > EXPORT_SYMBOL(v4l2_async_unregister_subdev); > > > > > > + > > > > > > +static void print_waiting_subdev(struct seq_file *s, > > > > > > + struct v4l2_async_subdev *asd) > > > > > > +{ > > > > > > + switch (asd->match_type) { > > > > > > + case V4L2_ASYNC_MATCH_CUSTOM: > > > > > > + seq_puts(s, "[custom]\n"); > > > > > > + break; > > > > > > + case V4L2_ASYNC_MATCH_DEVNAME: > > > > > > + seq_printf(s, "[devname] %s\n", > > > > > > + asd->match.device_name); > > > > > > + break; > > > > > > + case V4L2_ASYNC_MATCH_I2C: > > > > > > + seq_printf(s, "[i2c] %d-%04x\n", > > > > > > + asd->match.i2c.adapter_id, > > > > > > + asd->match.i2c.address); > > > > > > + break; > > > > > > + case V4L2_ASYNC_MATCH_FWNODE: { > > > > > > + struct fwnode_handle *fwnode = asd->match.fwnode; > > > > > > + > > > > > > + if (fwnode_graph_is_endpoint(fwnode)) > > > > > > + fwnode = fwnode_graph_get_port_parent(fwnode); > > > > > > > > Can we also print endpoint information ? > > > > > > What endpoint information do you have in mind? I'm asking this > > > because I printed endpoint OF node full names, only to find > > > so many of them named "endpoint" :) > > > > The port name and endpoint name would be useful. The full fwnode name > > would be an acceptable way to print that I think. > > > > Makes sense, and since we'd be parsing the fwnode subtype, > we'll be able to do something like: > > [of] dev=%s, node=%s > [swnode] ... > [acpi] ... Could you simply print the node name, %pfw? It works independently of the firmware interface and contains all the relevant information. -- Sakari Ailus ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: v4l2-async: Add waiting subdevices debugfs 2021-01-02 15:28 ` Sakari Ailus @ 2021-01-02 15:28 ` Sakari Ailus 2021-01-04 17:55 ` Ezequiel Garcia 1 sibling, 0 replies; 14+ messages in thread From: Sakari Ailus @ 2021-01-02 15:28 UTC (permalink / raw) To: Ezequiel Garcia; +Cc: Laurent Pinchart, linux-media, Hans Verkuil, kernel > Could you simply print the node name, %pfw? The full path of the node, I meant. The modifier is correct. -- Sakari Ailus ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: v4l2-async: Add waiting subdevices debugfs 2021-01-02 15:28 ` Sakari Ailus 2021-01-02 15:28 ` Sakari Ailus @ 2021-01-04 17:55 ` Ezequiel Garcia 1 sibling, 0 replies; 14+ messages in thread From: Ezequiel Garcia @ 2021-01-04 17:55 UTC (permalink / raw) To: Sakari Ailus; +Cc: Laurent Pinchart, linux-media, Hans Verkuil, kernel On Sat, 2021-01-02 at 17:28 +0200, Sakari Ailus wrote: > On Tue, Dec 29, 2020 at 02:52:52PM -0300, Ezequiel Garcia wrote: > > On Tue, 2020-12-29 at 16:14 +0200, Laurent Pinchart wrote: > > > Hi Ezequiel, > > > > > > On Tue, Dec 29, 2020 at 07:16:41AM -0300, Ezequiel Garcia wrote: > > > > On Mon, 2020-12-28 at 23:28 +0200, Laurent Pinchart wrote: > > > > > On Mon, Dec 28, 2020 at 08:35:20PM +0200, Sakari Ailus wrote: > > > > > > On Mon, Dec 28, 2020 at 03:05:11PM -0300, Ezequiel Garcia wrote: > > > > > > > There is currently little to none information available > > > > > > > about the reasons why a v4l2-async device hasn't > > > > > > > probed completely. > > > > > > > > > > > > > > Inspired by the "devices_deferred" debugfs file, > > > > > > > add a file to list information about the subdevices > > > > > > > that are on waiting lists, for each notifier. > > > > > > > > > > > > > > This is useful to debug v4l2-async subdevices > > > > > > > and notifiers, for instance when doing device bring-up. > > > > > > > > > > > > > > For instance, a typical output would be: > > > > > > > > > > > > > > $ cat /sys/kernel/debug/video4linux/waiting_subdevices > > > > > > > [fwnode] 1-003c > > > > > > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi1_mux > > > > > > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi0_mux > > > > > > > > > > > > > > It's possible to provide some more information, detecting > > > > > > > the type of fwnode and printing of-specific or acpi-specific > > > > > > > details. For now, the implementation is kept simple. > > > > > > > > > > > > The rest of the debug information we're effectively providing through > > > > > > kernel messages on DEBUG level (pr_debug/dev_dbg). Could we do the same > > > > > > here? > > > > > > > > > > > > Would just printing the names of the pending sub-devices at notifier > > > > > > register and async subdevice register time be sufficient? That way you'd > > > > > > also be fine with just dmesg output if you're asking someone to provide you > > > > > > information from another system. > > > > > > > > > > I think debugfs would be better. It can show the current state of an > > > > > async notifier in a single place, which is easier to parse than > > > > > reconstructing it from kernel messages and implicit knowledge of the > > > > > code. I'd expect users to have an easier time debugging probe issues > > > > > with such centralized information. > > > > > > > > > > > > Also, note that match-type "custom" prints no information. > > > > > > > Since there are no in-tree users of this match-type, > > > > > > > the implementation doesn't bother. > > > > > > > > > > > > Lines up to 74 characters are fine. Only in Gerrit it's 60 or 40 or > > > > > > whatever characters. ;-) > > > > > > > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > > > > --- > > > > > > > drivers/media/v4l2-core/v4l2-async.c | 54 ++++++++++++++++++++++++++++ > > > > > > > drivers/media/v4l2-core/v4l2-dev.c | 5 +++ > > > > > > > include/media/v4l2-async.h | 7 ++++ > > > > > > > 3 files changed, 66 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > > > > > > index e3ab003a6c85..32cd1ecced97 100644 > > > > > > > --- a/drivers/media/v4l2-core/v4l2-async.c > > > > > > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > > > > > > @@ -5,6 +5,7 @@ > > > > > > > * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > > > > > > */ > > > > > > > > > > > > > > +#include <linux/debugfs.h> > > > > > > > #include <linux/device.h> > > > > > > > #include <linux/err.h> > > > > > > > #include <linux/i2c.h> > > > > > > > @@ -14,6 +15,7 @@ > > > > > > > #include <linux/mutex.h> > > > > > > > #include <linux/of.h> > > > > > > > #include <linux/platform_device.h> > > > > > > > +#include <linux/seq_file.h> > > > > > > > #include <linux/slab.h> > > > > > > > #include <linux/types.h> > > > > > > > > > > > > > > @@ -837,3 +839,55 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > > > > > > > mutex_unlock(&list_lock); > > > > > > > } > > > > > > > EXPORT_SYMBOL(v4l2_async_unregister_subdev); > > > > > > > + > > > > > > > +static void print_waiting_subdev(struct seq_file *s, > > > > > > > + struct v4l2_async_subdev *asd) > > > > > > > +{ > > > > > > > + switch (asd->match_type) { > > > > > > > + case V4L2_ASYNC_MATCH_CUSTOM: > > > > > > > + seq_puts(s, "[custom]\n"); > > > > > > > + break; > > > > > > > + case V4L2_ASYNC_MATCH_DEVNAME: > > > > > > > + seq_printf(s, "[devname] %s\n", > > > > > > > + asd->match.device_name); > > > > > > > + break; > > > > > > > + case V4L2_ASYNC_MATCH_I2C: > > > > > > > + seq_printf(s, "[i2c] %d-%04x\n", > > > > > > > + asd->match.i2c.adapter_id, > > > > > > > + asd->match.i2c.address); > > > > > > > + break; > > > > > > > + case V4L2_ASYNC_MATCH_FWNODE: { > > > > > > > + struct fwnode_handle *fwnode = asd->match.fwnode; > > > > > > > + > > > > > > > + if (fwnode_graph_is_endpoint(fwnode)) > > > > > > > + fwnode = fwnode_graph_get_port_parent(fwnode); > > > > > > > > > > Can we also print endpoint information ? > > > > > > > > What endpoint information do you have in mind? I'm asking this > > > > because I printed endpoint OF node full names, only to find > > > > so many of them named "endpoint" :) > > > > > > The port name and endpoint name would be useful. The full fwnode name > > > would be an acceptable way to print that I think. > > > > > > > Makes sense, and since we'd be parsing the fwnode subtype, > > we'll be able to do something like: > > > > [of] dev=%s, node=%s > > [swnode] ... > > [acpi] ... > > Could you simply print the node name, %pfw? > > It works independently of the firmware interface and contains all the > relevant information. > Oh, great, that is really cool. Thanks for the hint, Ezequiel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: v4l2-async: Add waiting subdevices debugfs 2020-12-28 21:28 ` Laurent Pinchart 2020-12-29 10:16 ` Ezequiel Garcia @ 2021-01-02 15:24 ` Sakari Ailus 1 sibling, 0 replies; 14+ messages in thread From: Sakari Ailus @ 2021-01-02 15:24 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Ezequiel Garcia, linux-media, Hans Verkuil, kernel Hi Laurent, On Mon, Dec 28, 2020 at 11:28:01PM +0200, Laurent Pinchart wrote: > Hello, > > On Mon, Dec 28, 2020 at 08:35:20PM +0200, Sakari Ailus wrote: > > Hi Ezequiel, > > > > Thanks for the patch. > > Likewise :-) > > > On Mon, Dec 28, 2020 at 03:05:11PM -0300, Ezequiel Garcia wrote: > > > There is currently little to none information available > > > about the reasons why a v4l2-async device hasn't > > > probed completely. > > > > > > Inspired by the "devices_deferred" debugfs file, > > > add a file to list information about the subdevices > > > that are on waiting lists, for each notifier. > > > > > > This is useful to debug v4l2-async subdevices > > > and notifiers, for instance when doing device bring-up. > > > > > > For instance, a typical output would be: > > > > > > $ cat /sys/kernel/debug/video4linux/waiting_subdevices > > > [fwnode] 1-003c > > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi1_mux > > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi0_mux > > > > > > It's possible to provide some more information, detecting > > > the type of fwnode and printing of-specific or acpi-specific > > > details. For now, the implementation is kept simple. > > > > The rest of the debug information we're effectively providing through > > kernel messages on DEBUG level (pr_debug/dev_dbg). Could we do the same > > here? > > > > Would just printing the names of the pending sub-devices at notifier > > register and async subdevice register time be sufficient? That way you'd > > also be fine with just dmesg output if you're asking someone to provide you > > information from another system. > > I think debugfs would be better. It can show the current state of an > async notifier in a single place, which is easier to parse than > reconstructing it from kernel messages and implicit knowledge of the > code. I'd expect users to have an easier time debugging probe issues > with such centralized information. If something goes wrong, you still need the kernel messages as the debugfs file would only be able to tell what's waiting --- which is usually not enough to fix it. I don't mind adding a debugfs file for this if you think it's needed, but it'd still be nice to have the information in the kernel messages (in terms of which endpoints a notifier is still expecting). That could be a separate patch, too. -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: v4l2-async: Add waiting subdevices debugfs 2020-12-28 18:35 ` Sakari Ailus 2020-12-28 21:28 ` Laurent Pinchart @ 2020-12-29 9:54 ` Ezequiel Garcia 1 sibling, 0 replies; 14+ messages in thread From: Ezequiel Garcia @ 2020-12-29 9:54 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-media, Hans Verkuil, kernel, Laurent Pinchart On Mon, 2020-12-28 at 20:35 +0200, Sakari Ailus wrote: > Hi Ezequiel, > > Thanks for the patch. > > On Mon, Dec 28, 2020 at 03:05:11PM -0300, Ezequiel Garcia wrote: > > There is currently little to none information available > > about the reasons why a v4l2-async device hasn't > > probed completely. > > > > Inspired by the "devices_deferred" debugfs file, > > add a file to list information about the subdevices > > that are on waiting lists, for each notifier. > > > > This is useful to debug v4l2-async subdevices > > and notifiers, for instance when doing device bring-up. > > > > For instance, a typical output would be: > > > > $ cat /sys/kernel/debug/video4linux/waiting_subdevices > > [fwnode] 1-003c > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi1_mux > > [fwnode] 20e0000.iomuxc-gpr:ipu1_csi0_mux > > > > It's possible to provide some more information, detecting > > the type of fwnode and printing of-specific or acpi-specific > > details. For now, the implementation is kept simple. > > The rest of the debug information we're effectively providing through > kernel messages on DEBUG level (pr_debug/dev_dbg). Could we do the same > here? > > Would just printing the names of the pending sub-devices at notifier > register and async subdevice register time be sufficient? That way you'd > also be fine with just dmesg output if you're asking someone to provide you > information from another system. > Well, that's how this patch started, with some pr_info()s in v4l2_async_notifier_can_complete :) However, note that dev_dbg or pr_debug is more involved to enable than just debugfs. For instance, using dev_dbg requires DYNAMIC_DEBUG (which depends on DEBUGFS) and then the callsites have to be enabled. OTOH, a debugfs file allows easier enablement (just DEBUGFS, which is getting more common, and probably mandatory if you are doing bring-up work) and easy usage. And you can ask someone to provide the information on another system, just cat'ing the file. > > > > Also, note that match-type "custom" prints no information. > > Since there are no in-tree users of this match-type, > > the implementation doesn't bother. > > Lines up to 74 characters are fine. Only in Gerrit it's 60 or 40 or > whatever characters. ;-) > Oops, didn't realize I was using so few columns! Thanks, Ezequiel > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > drivers/media/v4l2-core/v4l2-async.c | 54 ++++++++++++++++++++++++++++ > > drivers/media/v4l2-core/v4l2-dev.c | 5 +++ > > include/media/v4l2-async.h | 7 ++++ > > 3 files changed, 66 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index e3ab003a6c85..32cd1ecced97 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -5,6 +5,7 @@ > > * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > */ > > > > +#include <linux/debugfs.h> > > #include <linux/device.h> > > #include <linux/err.h> > > #include <linux/i2c.h> > > @@ -14,6 +15,7 @@ > > #include <linux/mutex.h> > > #include <linux/of.h> > > #include <linux/platform_device.h> > > +#include <linux/seq_file.h> > > #include <linux/slab.h> > > #include <linux/types.h> > > > > @@ -837,3 +839,55 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > > mutex_unlock(&list_lock); > > } > > EXPORT_SYMBOL(v4l2_async_unregister_subdev); > > + > > +static void print_waiting_subdev(struct seq_file *s, > > + struct v4l2_async_subdev *asd) > > +{ > > + switch (asd->match_type) { > > + case V4L2_ASYNC_MATCH_CUSTOM: > > + seq_puts(s, "[custom]\n"); > > + break; > > + case V4L2_ASYNC_MATCH_DEVNAME: > > + seq_printf(s, "[devname] %s\n", > > + asd->match.device_name); > > + break; > > + case V4L2_ASYNC_MATCH_I2C: > > + seq_printf(s, "[i2c] %d-%04x\n", > > + asd->match.i2c.adapter_id, > > + asd->match.i2c.address); > > + break; > > + case V4L2_ASYNC_MATCH_FWNODE: { > > + struct fwnode_handle *fwnode = asd->match.fwnode; > > + > > + if (fwnode_graph_is_endpoint(fwnode)) > > + fwnode = fwnode_graph_get_port_parent(fwnode); > > + > > + seq_printf(s, "[fwnode] %s\n", > > + fwnode->dev ? dev_name(fwnode->dev) : "nil"); > > + break; > > + } > > + } > > +} > > + > > +static int waiting_subdevs_show(struct seq_file *s, void *data) > > +{ > > + struct v4l2_async_notifier *notifier; > > + struct v4l2_async_subdev *asd; > > + > > + mutex_lock(&list_lock); > > + > > + list_for_each_entry(notifier, ¬ifier_list, list) > > + list_for_each_entry(asd, ¬ifier->waiting, list) > > + print_waiting_subdev(s, asd); > > + > > + mutex_unlock(&list_lock); > > + > > + return 0; > > +} > > +DEFINE_SHOW_ATTRIBUTE(waiting_subdevs); > > + > > +void v4l2_async_debug_init(struct dentry *debugfs_dir) > > +{ > > + debugfs_create_file("waiting_subdevices", 0444, debugfs_dir, NULL, > > + &waiting_subdevs_fops); > > +} > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c > > index a593ea0598b5..8d3813e6ab56 100644 > > --- a/drivers/media/v4l2-core/v4l2-dev.c > > +++ b/drivers/media/v4l2-core/v4l2-dev.c > > @@ -14,6 +14,7 @@ > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > +#include <linux/debugfs.h> > > #include <linux/module.h> > > #include <linux/types.h> > > #include <linux/kernel.h> > > @@ -37,6 +38,7 @@ > > __func__, ##arg); \ > > } while (0) > > > > +static struct dentry *v4l2_debugfs_dir; > > > > /* > > * sysfs stuff > > @@ -1113,6 +1115,8 @@ static int __init videodev_init(void) > > return -EIO; > > } > > > > + v4l2_debugfs_dir = debugfs_create_dir("video4linux", NULL); > > + v4l2_async_debug_init(v4l2_debugfs_dir); > > return 0; > > } > > > > @@ -1120,6 +1124,7 @@ static void __exit videodev_exit(void) > > { > > dev_t dev = MKDEV(VIDEO_MAJOR, 0); > > > > + debugfs_remove_recursive(v4l2_debugfs_dir); > > class_unregister(&video_class); > > unregister_chrdev_region(dev, VIDEO_NUM_DEVICES); > > } > > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > > index d6e31234826f..312ab421aa40 100644 > > --- a/include/media/v4l2-async.h > > +++ b/include/media/v4l2-async.h > > @@ -137,6 +137,13 @@ struct v4l2_async_notifier { > > struct list_head list; > > }; > > > > +/** > > + * v4l2_async_debug_init - Initialize debugging tools. > > + * > > + * @debugfs_dir: pointer to the parent debugfs &struct dentry > > + */ > > +void v4l2_async_debug_init(struct dentry *debugfs_dir); > > + > > /** > > * v4l2_async_notifier_init - Initialize a notifier. > > * > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: v4l2-async: Add waiting subdevices debugfs 2020-12-28 18:05 [PATCH] media: v4l2-async: Add waiting subdevices debugfs Ezequiel Garcia @ 2020-12-29 1:09 ` kernel test robot 2020-12-29 1:09 ` kernel test robot 1 sibling, 0 replies; 14+ messages in thread From: kernel test robot @ 2020-12-29 1:09 UTC (permalink / raw) To: Ezequiel Garcia, linux-media, Hans Verkuil Cc: kbuild-all, kernel, Laurent Pinchart, Sakari Ailus, Ezequiel Garcia [-- Attachment #1: Type: text/plain, Size: 2352 bytes --] Hi Ezequiel, I love your patch! Perhaps something to improve: [auto build test WARNING on linuxtv-media/master] [also build test WARNING on v5.11-rc1 next-20201223] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Ezequiel-Garcia/media-v4l2-async-Add-waiting-subdevices-debugfs/20201229-020838 base: git://linuxtv.org/media_tree.git master config: parisc-randconfig-s032-20201228 (attached as .config) compiler: hppa-linux-gcc (GCC) 9.3.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.3-184-g1b896707-dirty # https://github.com/0day-ci/linux/commit/6dd20991dbcacc09544c9b4fce904253171b9329 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Ezequiel-Garcia/media-v4l2-async-Add-waiting-subdevices-debugfs/20201229-020838 git checkout 6dd20991dbcacc09544c9b4fce904253171b9329 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=parisc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): In file included from include/media/v4l2-subdev.h:14, from drivers/media/test-drivers/vimc/vimc-scaler.c:12: >> include/media/v4l2-async.h:145:35: warning: 'struct dentry' declared inside parameter list will not be visible outside of this definition or declaration 145 | void v4l2_async_debug_init(struct dentry *debugfs_dir); | ^~~~~~ vim +145 include/media/v4l2-async.h 139 140 /** 141 * v4l2_async_debug_init - Initialize debugging tools. 142 * 143 * @debugfs_dir: pointer to the parent debugfs &struct dentry 144 */ > 145 void v4l2_async_debug_init(struct dentry *debugfs_dir); 146 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 30705 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: v4l2-async: Add waiting subdevices debugfs @ 2020-12-29 1:09 ` kernel test robot 0 siblings, 0 replies; 14+ messages in thread From: kernel test robot @ 2020-12-29 1:09 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 2406 bytes --] Hi Ezequiel, I love your patch! Perhaps something to improve: [auto build test WARNING on linuxtv-media/master] [also build test WARNING on v5.11-rc1 next-20201223] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Ezequiel-Garcia/media-v4l2-async-Add-waiting-subdevices-debugfs/20201229-020838 base: git://linuxtv.org/media_tree.git master config: parisc-randconfig-s032-20201228 (attached as .config) compiler: hppa-linux-gcc (GCC) 9.3.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.3-184-g1b896707-dirty # https://github.com/0day-ci/linux/commit/6dd20991dbcacc09544c9b4fce904253171b9329 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Ezequiel-Garcia/media-v4l2-async-Add-waiting-subdevices-debugfs/20201229-020838 git checkout 6dd20991dbcacc09544c9b4fce904253171b9329 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=parisc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): In file included from include/media/v4l2-subdev.h:14, from drivers/media/test-drivers/vimc/vimc-scaler.c:12: >> include/media/v4l2-async.h:145:35: warning: 'struct dentry' declared inside parameter list will not be visible outside of this definition or declaration 145 | void v4l2_async_debug_init(struct dentry *debugfs_dir); | ^~~~~~ vim +145 include/media/v4l2-async.h 139 140 /** 141 * v4l2_async_debug_init - Initialize debugging tools. 142 * 143 * @debugfs_dir: pointer to the parent debugfs &struct dentry 144 */ > 145 void v4l2_async_debug_init(struct dentry *debugfs_dir); 146 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 30705 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-01-04 17:56 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-28 18:05 [PATCH] media: v4l2-async: Add waiting subdevices debugfs Ezequiel Garcia 2020-12-28 18:35 ` Sakari Ailus 2020-12-28 21:28 ` Laurent Pinchart 2020-12-29 10:16 ` Ezequiel Garcia 2020-12-29 14:14 ` Laurent Pinchart 2020-12-29 17:52 ` Ezequiel Garcia 2020-12-30 1:26 ` Laurent Pinchart 2021-01-02 15:28 ` Sakari Ailus 2021-01-02 15:28 ` Sakari Ailus 2021-01-04 17:55 ` Ezequiel Garcia 2021-01-02 15:24 ` Sakari Ailus 2020-12-29 9:54 ` Ezequiel Garcia 2020-12-29 1:09 ` kernel test robot 2020-12-29 1:09 ` kernel test robot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.