All of lore.kernel.org
 help / color / mirror / Atom feed
* [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, &notifier_list, list)
+		list_for_each_entry(asd, &notifier->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, &notifier_list, list)
> +		list_for_each_entry(asd, &notifier->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, &notifier_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, &notifier->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 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

* 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, &notifier_list, list)
> > +               list_for_each_entry(asd, &notifier->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 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, &notifier_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, &notifier->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, &notifier_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, &notifier->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-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-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

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.