Re: [PATCH] media: v4l2-async: Add waiting subdevices debugfs

[Ezequiel Garcia <ezequiel@collabora.com>]

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