| Message ID | 20201228180511.43486-1-ezequiel@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: v4l2-async: Add waiting subdevices debugfs | expand |
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. > *
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
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. > > * >
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. > > > * >
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
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
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
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
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
> Could you simply print the node name, %pfw?
The full path of the node, I meant. The modifier is correct.
--
Sakari Ailus
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
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. *
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(+)