| Message ID | 20210108171728.39434-2-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] media: v4l2-async: Add waiting subdevices debugfs | expand |
Hi Ezequiel, On Fri, Jan 08, 2021 at 02:17:28PM -0300, Ezequiel Garcia wrote: > There is currently little to no 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/pending_async_subdevices > ipu1_csi1: > [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi1_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi1_mux > ipu1_csi0: > [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi0_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi0_mux > imx6-mipi-csi2: > [fwnode] dev=1-003c, node=/soc/bus@2100000/i2c@21a4000/camera@3c > imx-media: > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > -- > v3: > * Drop MATCH_CUSTOM > * Use the actual fwnode, to print the real full path. endpoint name. > * Fix refcounted handle leak. > v2: > * Print fwnode path, as suggested by Sakari. > * Print the subdevices under their corresponding notifier. > * Rename the file as suggested by Laurent. The patch itself seems fine. But lease put such notes below the '---' line below going forward. What's above it will be part of the commit message. > --- > drivers/media/v4l2-core/v4l2-async.c | 66 ++++++++++++++++++++++++++++ > drivers/media/v4l2-core/v4l2-dev.c | 5 +++ > include/media/v4l2-async.h | 8 ++++ > 3 files changed, 79 insertions(+) >
On 08/01/2021 18:17, Ezequiel Garcia wrote: > There is currently little to no 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/pending_async_subdevices > ipu1_csi1: > [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi1_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi1_mux > ipu1_csi0: > [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi0_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi0_mux > imx6-mipi-csi2: > [fwnode] dev=1-003c, node=/soc/bus@2100000/i2c@21a4000/camera@3c > imx-media: > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Nice, this should be quite useful. Regards, Hans > -- > v3: > * Drop MATCH_CUSTOM > * Use the actual fwnode, to print the real full path. endpoint name. > * Fix refcounted handle leak. > v2: > * Print fwnode path, as suggested by Sakari. > * Print the subdevices under their corresponding notifier. > * Rename the file as suggested by Laurent. > --- > drivers/media/v4l2-core/v4l2-async.c | 66 ++++++++++++++++++++++++++++ > drivers/media/v4l2-core/v4l2-dev.c | 5 +++ > include/media/v4l2-async.h | 8 ++++ > 3 files changed, 79 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 3faf1d12d49d..7c8a97205b3e 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> > > @@ -823,3 +825,67 @@ 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_DEVNAME: > + seq_printf(s, " [devname] dev=%s\n", asd->match.device_name); > + break; > + case V4L2_ASYNC_MATCH_I2C: > + seq_printf(s, " [i2c] dev=%d-%04x\n", asd->match.i2c.adapter_id, > + asd->match.i2c.address); > + break; > + case V4L2_ASYNC_MATCH_FWNODE: { > + struct fwnode_handle *devnode, *fwnode = asd->match.fwnode; > + > + devnode = fwnode_graph_is_endpoint(fwnode) ? > + fwnode_graph_get_port_parent(fwnode) : > + fwnode_handle_get(fwnode); > + > + seq_printf(s, " [fwnode] dev=%s, node=%pfw\n", > + devnode->dev ? dev_name(devnode->dev) : "nil", > + fwnode); > + > + fwnode_handle_put(devnode); > + break; > + } > + } > +} > + > +static const char * > +v4l2_async_notifier_name(struct v4l2_async_notifier *notifier) > +{ > + if (notifier->v4l2_dev) > + return notifier->v4l2_dev->name; > + else if (notifier->sd) > + return notifier->sd->name; > + else > + return "nil"; > +} > + > +static int pending_subdevs_show(struct seq_file *s, void *data) > +{ > + struct v4l2_async_notifier *notif; > + struct v4l2_async_subdev *asd; > + > + mutex_lock(&list_lock); > + > + list_for_each_entry(notif, ¬ifier_list, list) { > + seq_printf(s, "%s:\n", v4l2_async_notifier_name(notif)); > + list_for_each_entry(asd, ¬if->waiting, list) > + print_waiting_subdev(s, asd); > + } > + > + mutex_unlock(&list_lock); > + > + return 0; > +} > +DEFINE_SHOW_ATTRIBUTE(pending_subdevs); > + > +void v4l2_async_debug_init(struct dentry *debugfs_dir) > +{ > + debugfs_create_file("pending_async_subdevices", 0444, debugfs_dir, NULL, > + &pending_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 8ed42188e7c9..26296d4cb660 100644 > --- a/include/media/v4l2-async.h > +++ b/include/media/v4l2-async.h > @@ -11,6 +11,7 @@ > #include <linux/list.h> > #include <linux/mutex.h> > > +struct dentry; > struct device; > struct device_node; > struct v4l2_device; > @@ -120,6 +121,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. > * >
On Fri, 2021-01-15 at 01:36 +0200, Sakari Ailus wrote: > Hi Ezequiel, > > On Fri, Jan 08, 2021 at 02:17:28PM -0300, Ezequiel Garcia wrote: > > There is currently little to no 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/pending_async_subdevices > > ipu1_csi1: > > [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi1_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi1_mux > > ipu1_csi0: > > [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi0_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi0_mux > > imx6-mipi-csi2: > > [fwnode] dev=1-003c, node=/soc/bus@2100000/i2c@21a4000/camera@3c > > imx-media: > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > -- > > v3: > > * Drop MATCH_CUSTOM > > * Use the actual fwnode, to print the real full path. endpoint name. > > * Fix refcounted handle leak. > > v2: > > * Print fwnode path, as suggested by Sakari. > > * Print the subdevices under their corresponding notifier. > > * Rename the file as suggested by Laurent. > > The patch itself seems fine. But lease put such notes below the '---' line > below going forward. What's above it will be part of the commit message. > Oops, sorry for the typo. I have a v4 for this one, dropping MATCH_DEVNAME too. Thanks, Ezequiel
On Fri, Jan 15, 2021 at 11:08:21AM -0300, Ezequiel Garcia wrote: > On Fri, 2021-01-15 at 01:36 +0200, Sakari Ailus wrote: > > Hi Ezequiel, > > > > On Fri, Jan 08, 2021 at 02:17:28PM -0300, Ezequiel Garcia wrote: > > > There is currently little to no 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/pending_async_subdevices > > > ipu1_csi1: > > > [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi1_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi1_mux > > > ipu1_csi0: > > > [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi0_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi0_mux > > > imx6-mipi-csi2: > > > [fwnode] dev=1-003c, node=/soc/bus@2100000/i2c@21a4000/camera@3c > > > imx-media: > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > -- > > > v3: > > > * Drop MATCH_CUSTOM > > > * Use the actual fwnode, to print the real full path. endpoint name. > > > * Fix refcounted handle leak. > > > v2: > > > * Print fwnode path, as suggested by Sakari. > > > * Print the subdevices under their corresponding notifier. > > > * Rename the file as suggested by Laurent. > > > > The patch itself seems fine. But lease put such notes below the '---' line > > below going forward. What's above it will be part of the commit message. > > > > Oops, sorry for the typo. > > I have a v4 for this one, dropping MATCH_DEVNAME too. Could you remove it as a separate patch? It's unrelated to debugging.
On Fri, 2021-01-15 at 17:05 +0200, Sakari Ailus wrote: > On Fri, Jan 15, 2021 at 11:08:21AM -0300, Ezequiel Garcia wrote: > > On Fri, 2021-01-15 at 01:36 +0200, Sakari Ailus wrote: > > > Hi Ezequiel, > > > > > > On Fri, Jan 08, 2021 at 02:17:28PM -0300, Ezequiel Garcia wrote: > > > > There is currently little to no 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/pending_async_subdevices > > > > ipu1_csi1: > > > > [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi1_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi1_mux > > > > ipu1_csi0: > > > > [fwnode] dev=20e0000.iomuxc-gpr:ipu1_csi0_mux, node=/soc/bus@2000000/iomuxc-gpr@20e0000/ipu1_csi0_mux > > > > imx6-mipi-csi2: > > > > [fwnode] dev=1-003c, node=/soc/bus@2100000/i2c@21a4000/camera@3c > > > > imx-media: > > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > -- > > > > v3: > > > > * Drop MATCH_CUSTOM > > > > * Use the actual fwnode, to print the real full path. endpoint name. > > > > * Fix refcounted handle leak. > > > > v2: > > > > * Print fwnode path, as suggested by Sakari. > > > > * Print the subdevices under their corresponding notifier. > > > > * Rename the file as suggested by Laurent. > > > > > > The patch itself seems fine. But lease put such notes below the '---' line > > > below going forward. What's above it will be part of the commit message. > > > > > > > Oops, sorry for the typo. > > > > I have a v4 for this one, dropping MATCH_DEVNAME too. > > Could you remove it as a separate patch? It's unrelated to debugging. > Yes, but still we won't be printing MATCH_DEVNAME types so this patch needs a v4 dropping that. I think it's worth it as it simplifies the patch a bit. Thanks, Ezequiel
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index 3faf1d12d49d..7c8a97205b3e 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> @@ -823,3 +825,67 @@ 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_DEVNAME: + seq_printf(s, " [devname] dev=%s\n", asd->match.device_name); + break; + case V4L2_ASYNC_MATCH_I2C: + seq_printf(s, " [i2c] dev=%d-%04x\n", asd->match.i2c.adapter_id, + asd->match.i2c.address); + break; + case V4L2_ASYNC_MATCH_FWNODE: { + struct fwnode_handle *devnode, *fwnode = asd->match.fwnode; + + devnode = fwnode_graph_is_endpoint(fwnode) ? + fwnode_graph_get_port_parent(fwnode) : + fwnode_handle_get(fwnode); + + seq_printf(s, " [fwnode] dev=%s, node=%pfw\n", + devnode->dev ? dev_name(devnode->dev) : "nil", + fwnode); + + fwnode_handle_put(devnode); + break; + } + } +} + +static const char * +v4l2_async_notifier_name(struct v4l2_async_notifier *notifier) +{ + if (notifier->v4l2_dev) + return notifier->v4l2_dev->name; + else if (notifier->sd) + return notifier->sd->name; + else + return "nil"; +} + +static int pending_subdevs_show(struct seq_file *s, void *data) +{ + struct v4l2_async_notifier *notif; + struct v4l2_async_subdev *asd; + + mutex_lock(&list_lock); + + list_for_each_entry(notif, ¬ifier_list, list) { + seq_printf(s, "%s:\n", v4l2_async_notifier_name(notif)); + list_for_each_entry(asd, ¬if->waiting, list) + print_waiting_subdev(s, asd); + } + + mutex_unlock(&list_lock); + + return 0; +} +DEFINE_SHOW_ATTRIBUTE(pending_subdevs); + +void v4l2_async_debug_init(struct dentry *debugfs_dir) +{ + debugfs_create_file("pending_async_subdevices", 0444, debugfs_dir, NULL, + &pending_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 8ed42188e7c9..26296d4cb660 100644 --- a/include/media/v4l2-async.h +++ b/include/media/v4l2-async.h @@ -11,6 +11,7 @@ #include <linux/list.h> #include <linux/mutex.h> +struct dentry; struct device; struct device_node; struct v4l2_device; @@ -120,6 +121,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. *