Re: [PATCH 1/4] drm/debugfs: Add support for dynamic debugfs initialization

[Daniel Vetter]

On Fri, Aug 31, 2018 at 07:31:10PM -0400, Lyude Paul wrote:
> On Fri, 2018-08-31 at 10:53 +0200, Daniel Vetter wrote:
> > On Mon, Aug 27, 2018 at 08:36:24PM -0400, Lyude Paul wrote:
> > > Currently all debugfs related initialization for the DRM core happens in
> > > drm_debugfs_init(), which is called when registering the minor device.
> > > While this works fine for features such as atomic modesetting and GEM,
> > > this doesn't work at all for resources like DP MST topology managers
> > > which can potentially be created both before and after the minor
> > > device has been registered.
> > > 
> > > So, in order to add driver-wide debugfs files for MST we'll need to be
> > > able to handle debugfs initialization for such resources. We do so by
> > > introducing drm_debugfs_callback_register() and
> > > drm_debugfs_callback_unregister(). These functions allow driver-agnostic
> > > parts of DRM to add additional debugfs initialization callbacks at any
> > > point during a DRM driver's lifetime.
> > > 
> > > Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > Cc: Daniel Stone <daniel@xxxxxxxxxxxxx>
> > 
> > So the debugfs lifetime rules are indeed silly and broken. I'm not sure
> > this is what we want to do though:
> > 
> > - Thanks to Noralf's cleanup you don't need a debugfs cleanup callback
> >   anymore really, it will auto-clean-up on device unregistration.
> This is true, but the cleanup there is more-so to handle the theoretical case
> that a driver uninitializes an MST topology mgr before the rest of debugfs is
> torn down.
> > 
> > - For stuff tied to connectors we have the connector->register/unregister
> >   callbacks. Imo connector-related debugfs stuff, like for mst, should be
> >   put there.
> 
> Since this would tie the lifetime of the debugfs files we make to the lifetime
> of the connector, as it should be, we'd be able to pull off a much nicer
> debugfs hierarchy:
> 
> /sys/kernel/debug/dri/0
>    DP-1
>       edid_override
>       dp_mst
>          status
>    DP-2
>       edid_override
>       dp_mst
>          status
>    DP-3
>       edid_override
>       dp_mst
>          status
> 
> The only thing I don't like about that approach though is that now we've given
> up on the idea of these debugfs nodes being added to drivers using MST
> automatically, since drivers wanting this would have to add calls into
> late_register/early_unregister.

Yup, that's part of the fallout of insisting on the core/helper split. Mst
is helper, ->late_register is core.

What we could do is combine the ->late_register callback with some of your
ideas here, so that at least the ->early_unregister callback isn't
necessary. So still explicit call to set up the mst connector files, but
the cleanup would happen automatically (using a per-connector list of
files and the fs structure you laid out above).

> Since I think this would probably be useful for more then just connectors,
> what if we added some sort of similar dynamic initialization system that could
> be used per-DRM object? That way drivers would still get the debugfs files for
> free, and we'd get a nice hierarchy and a sane way for DRM helpers to add
> additional debugfs files to drivers for free. Assumably, such a system would
> be added in addition to a device-wide dynamic init system like the one this
> patch adds.

I wouldn't start out with adding it everywhere, just for connectors.
Should be easy to refactor later on if there's a need. And to be used from
a ->late_register callback.

And I really like the per-connector debugfs files.

> > - debugfs_init is a dead idea, as you point out it's fundamentally racy.
> > 
> > - Dropping the silly notion that we have to duplicate all debugfs entries
> >   per-minor would be really sweet (last time I checked there's not a
> >   single debugfs file that's actually different per minor).
> 
> Yeah I'm down for this as well, I've never once seen anything actually use the
> minor debugfs files. Plus, we're supposed to be able to do whatever we want in
> debugfs anyway! So I don't see the harm in just gutting the duplicate minor
> debugfs stuff entirely

There's epic amounts of scripts and libraries (e.g. igt) hard-coding the
dri/0/ thing. But I think we can at least convert all the others into a
symlink to the main drm minor number.
-Daniel

> > 
> > Cheers, Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/drm_debugfs.c  | 173 +++++++++++++++++++++++++++++++--
> > >  drivers/gpu/drm/drm_drv.c      |   3 +
> > >  drivers/gpu/drm/drm_internal.h |   5 +
> > >  include/drm/drm_debugfs.h      |  27 +++++
> > >  include/drm/drm_file.h         |   4 +
> > >  5 files changed, 203 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> > > index 6f28fe58f169..a53a454b167f 100644
> > > --- a/drivers/gpu/drm/drm_debugfs.c
> > > +++ b/drivers/gpu/drm/drm_debugfs.c
> > > @@ -39,6 +39,13 @@
> > >  
> > >  #if defined(CONFIG_DEBUG_FS)
> > >  
> > > +struct drm_debugfs_callback {
> > > +	void (*init)(void *data);
> > > +	void (*cleanup_cb)(void *data);
> > > +	void *data;
> > > +	struct list_head list;
> > > +};
> > > +
> > >  /***************************************************
> > >   * Initialization, etc.
> > >   **************************************************/
> > > @@ -67,6 +74,113 @@ static const struct file_operations drm_debugfs_fops =
> > > {
> > >  	.release = single_release,
> > >  };
> > >  
> > > +/**
> > > + * drm_debugfs_register_callback - Register a callback for initializing
> > > + *                                 dynamic driver-agnostic debugfs files
> > > + * @minor: device minor number
> > > + * @init: The callback to invoke to perform the debugfs initialization
> > > + * @cleanup_cb: The callback to invoke to cleanup any resources for the
> > > + * callback
> > > + * @data: Data to pass to @init and @cleanup_cb
> > > + * @out: Where to store the pointer to the callback struct
> > > + *
> > > + * Register an initialization callback with debugfs. This callback can be
> > > used
> > > + * to creating debugfs nodes for DRM resources that might get created
> > > before
> > > + * the debugfs node for @minor has been created.
> > > + *
> > > + * When a callback is registered with this function before the debugfs
> > > root
> > > + * has been created, the callback's execution will be delayed until all
> > > other
> > > + * debugfs nodes (including those owned by the DRM device's driver) have
> > > been
> > > + * instantiated. If a callback is registered with this function after the
> > > + * debugfs root has been created, @init and @cleanup_cb will be executed
> > > + * immediately without creating a &struct drm_debugfs_callback.
> > > + *
> > > + * In the event that debugfs creation for the device fails; all
> > > registered
> > > + * debugfs callbacks will have their @cleanup_cb callbacks invoked
> > > without
> > > + * having their @init callbacks invoked. This is to ensure that no
> > > resources
> > > + * are leaked during initialization of debugfs, even if the
> > > initialization
> > > + * process fails. Likewise; any callbacks that are registered after DRM
> > > has
> > > + * failed to initialize it's debugfs files will have their @cleanup_cb
> > > + * callbacks invoked immediately and all of their respective resources
> > > + * destroyed.
> > > + *
> > > + * Implementations of @cleanup_cb should clean up all resources for the
> > > + * callback, with the exception of freeing the memory for @out. Freeing
> > > @out
> > > + * will be handled by the DRM core automatically.
> > > + *
> > > + * Users of this function should take care to add a symmetric call to
> > > + * @drm_debugfs_unregister_callback to handle destroying a registered
> > > callback
> > > + * in case the resources for the user of this function are destroyed
> > > before
> > > + * debugfs root is initialized.
> > > + *
> > > + */
> > > +int
> > > +drm_debugfs_register_callback(struct drm_minor *minor,
> > > +			      void (*init)(void *),
> > > +			      void (*cleanup_cb)(void *),
> > > +			      void *data, struct drm_debugfs_callback **out)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	mutex_lock(&minor->debugfs_callback_lock);
> > > +	if (minor->debugfs_callbacks_done) {
> > > +		/* debugfs is already setup, so just handle the callback
> > > +		 * immediately
> > > +		 */
> > > +		if (minor->debugfs_root)
> > > +			(*init)(data);
> > > +		(*cleanup_cb)(data);
> > > +		goto out_unlock;
> > > +	}
> > > +
> > > +	*out = kzalloc(sizeof(**out), GFP_KERNEL);
> > > +	if (!*out) {
> > > +		ret = -ENOMEM;
> > > +		goto out_unlock;
> > > +	}
> > > +
> > > +	(*out)->init = init;
> > > +	(*out)->cleanup_cb = cleanup_cb;
> > > +	(*out)->data = data;
> > > +	list_add(&(*out)->list, &minor->debugfs_callback_list);
> > > +
> > > +out_unlock:
> > > +	mutex_unlock(&minor->debugfs_callback_lock);
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL(drm_debugfs_register_callback);
> > > +
> > > +/**
> > > + * drm_debugfs_unregister_callback - Unregister and release the resources
> > > + *                                   associated with a debugfs init
> > > callback
> > > + * @minor: device minor number
> > > + * @cb: A pointer to the &struct drm_debugfs_callback struct returned by
> > > + * drm_debugfs_register_callback(). May be NULL
> > > + *
> > > + * Unregisters a &struct drm_debugfs_callback struct with debugfs and
> > > destroys
> > > + * all of it's associated resources. This includes a call to the
> > > callback's
> > > + * @cleanup_cb implementation.
> > > + *
> > > + * Once the debugfs root is initialized or has failed initialization, all
> > > + * registered callbacks are automatically destroyed. If this function is
> > > + * called after that point; it will automatically be a no-op.
> > > + */
> > > +void
> > > +drm_debugfs_unregister_callback(struct drm_minor *minor,
> > > +				struct drm_debugfs_callback *cb)
> > > +{
> > > +	mutex_lock(&minor->debugfs_callback_lock);
> > > +	/* We don't have to do anything if we've already completed any
> > > +	 * registered callbacks, as they will have already been destroyed
> > > +	 */
> > > +	if (!minor->debugfs_callbacks_done) {
> > > +		cb->cleanup_cb(cb->data);
> > > +		list_del(&cb->list);
> > > +		kfree(cb);
> > > +	}
> > > +	mutex_unlock(&minor->debugfs_callback_lock);
> > > +}
> > > +EXPORT_SYMBOL(drm_debugfs_unregister_callback);
> > >  
> > >  /**
> > >   * drm_debugfs_create_files - Initialize a given set of debugfs files for
> > > DRM
> > > @@ -126,12 +240,24 @@ int drm_debugfs_create_files(const struct
> > > drm_info_list *files, int count,
> > >  }
> > >  EXPORT_SYMBOL(drm_debugfs_create_files);
> > >  
> > > +int drm_debugfs_alloc(struct drm_minor *minor)
> > > +{
> > > +	INIT_LIST_HEAD(&minor->debugfs_callback_list);
> > > +	mutex_init(&minor->debugfs_callback_lock);
> > > +	return 0;
> > > +}
> > > +
> > >  int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> > >  		     struct dentry *root)
> > >  {
> > >  	struct drm_device *dev = minor->dev;
> > > +	struct drm_debugfs_callback *pos, *tmp;
> > >  	char name[64];
> > > -	int ret;
> > > +	int ret = 0;
> > > +
> > > +	/* Don't allow any more callbacks to be registered while we setup */
> > > +	mutex_lock(&minor->debugfs_callback_lock);
> > > +	minor->debugfs_callbacks_done = true;
> > >  
> > >  	INIT_LIST_HEAD(&minor->debugfs_list);
> > >  	mutex_init(&minor->debugfs_lock);
> > > @@ -139,7 +265,8 @@ int drm_debugfs_init(struct drm_minor *minor, int
> > > minor_id,
> > >  	minor->debugfs_root = debugfs_create_dir(name, root);
> > >  	if (!minor->debugfs_root) {
> > >  		DRM_ERROR("Cannot create /sys/kernel/debug/dri/%s\n", name);
> > > -		return -1;
> > > +		ret = -1;
> > > +		goto out_unlock;
> > >  	}
> > >  
> > >  	ret = drm_debugfs_create_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES,
> > > @@ -148,14 +275,14 @@ int drm_debugfs_init(struct drm_minor *minor, int
> > > minor_id,
> > >  		debugfs_remove(minor->debugfs_root);
> > >  		minor->debugfs_root = NULL;
> > >  		DRM_ERROR("Failed to create core drm debugfs files\n");
> > > -		return ret;
> > > +		goto out_unlock;
> > >  	}
> > >  
> > >  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> > >  		ret = drm_atomic_debugfs_init(minor);
> > >  		if (ret) {
> > >  			DRM_ERROR("Failed to create atomic debugfs files\n");
> > > -			return ret;
> > > +			goto out_unlock;
> > >  		}
> > >  	}
> > >  
> > > @@ -163,13 +290,13 @@ int drm_debugfs_init(struct drm_minor *minor, int
> > > minor_id,
> > >  		ret = drm_framebuffer_debugfs_init(minor);
> > >  		if (ret) {
> > >  			DRM_ERROR("Failed to create framebuffer debugfs
> > > file\n");
> > > -			return ret;
> > > +			goto out_unlock;
> > >  		}
> > >  
> > >  		ret = drm_client_debugfs_init(minor);
> > >  		if (ret) {
> > >  			DRM_ERROR("Failed to create client debugfs file\n");
> > > -			return ret;
> > > +			goto out_unlock;
> > >  		}
> > >  	}
> > >  
> > > @@ -178,10 +305,23 @@ int drm_debugfs_init(struct drm_minor *minor, int
> > > minor_id,
> > >  		if (ret) {
> > >  			DRM_ERROR("DRM: Driver failed to initialize "
> > >  				  "/sys/kernel/debug/dri.\n");
> > > -			return ret;
> > > +			goto out_unlock;
> > >  		}
> > >  	}
> > > -	return 0;
> > > +
> > > +out_unlock:
> > > +	/* Execute any delayed callbacks if we succeeded, or just clean them
> > > +	 * up without running them if we failed
> > > +	 */
> > > +	list_for_each_entry_safe(pos, tmp, &minor->debugfs_callback_list,
> > > +				 list) {
> > > +		if (!ret)
> > > +			pos->init(pos->data);
> > > +		pos->cleanup_cb(pos->data);
> > > +		kfree(pos);
> > > +	}
> > > +	mutex_unlock(&minor->debugfs_callback_lock);
> > > +	return ret;
> > >  }
> > >  
> > >  
> > > @@ -223,14 +363,29 @@ static void drm_debugfs_remove_all_files(struct
> > > drm_minor *minor)
> > >  
> > >  int drm_debugfs_cleanup(struct drm_minor *minor)
> > >  {
> > > +	struct drm_debugfs_callback *pos, *tmp;
> > > +
> > > +	mutex_lock(&minor->debugfs_callback_lock);
> > > +	if (!minor->debugfs_callbacks_done) {
> > > +		list_for_each_entry_safe(pos, tmp,
> > > +					 &minor->debugfs_callback_list,
> > > +					 list) {
> > > +			pos->cleanup_cb(pos->data);
> > > +			kfree(pos);
> > > +		}
> > > +	}
> > > +	minor->debugfs_callbacks_done = true;
> > > +
> > >  	if (!minor->debugfs_root)
> > > -		return 0;
> > > +		goto out;
> > >  
> > >  	drm_debugfs_remove_all_files(minor);
> > >  
> > >  	debugfs_remove_recursive(minor->debugfs_root);
> > >  	minor->debugfs_root = NULL;
> > >  
> > > +out:
> > > +	mutex_unlock(&minor->debugfs_callback_lock);
> > >  	return 0;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > index ea4941da9b27..7041b3137229 100644
> > > --- a/drivers/gpu/drm/drm_drv.c
> > > +++ b/drivers/gpu/drm/drm_drv.c
> > > @@ -118,6 +118,9 @@ static int drm_minor_alloc(struct drm_device *dev,
> > > unsigned int type)
> > >  
> > >  	minor->type = type;
> > >  	minor->dev = dev;
> > > +	r = drm_debugfs_alloc(minor);
> > > +	if (r)
> > > +		goto err_free;
> > >  
> > >  	idr_preload(GFP_KERNEL);
> > >  	spin_lock_irqsave(&drm_minor_lock, flags);
> > > diff --git a/drivers/gpu/drm/drm_internal.h
> > > b/drivers/gpu/drm/drm_internal.h
> > > index 40179c5fc6b8..d6394246967d 100644
> > > --- a/drivers/gpu/drm/drm_internal.h
> > > +++ b/drivers/gpu/drm/drm_internal.h
> > > @@ -118,6 +118,7 @@ void drm_gem_print_info(struct drm_printer *p,
> > > unsigned int indent,
> > >  
> > >  /* drm_debugfs.c drm_debugfs_crc.c */
> > >  #if defined(CONFIG_DEBUG_FS)
> > > +int drm_debugfs_alloc(struct drm_minor *minor);
> > >  int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> > >  		     struct dentry *root);
> > >  int drm_debugfs_cleanup(struct drm_minor *minor);
> > > @@ -127,6 +128,10 @@ int drm_debugfs_crtc_add(struct drm_crtc *crtc);
> > >  void drm_debugfs_crtc_remove(struct drm_crtc *crtc);
> > >  int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc);
> > >  #else
> > > +static inline int drm_debugfs_alloc(struct drm_minor *minor)
> > > +{
> > > +	return 0;
> > > +}
> > >  static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> > >  				   struct dentry *root)
> > >  {
> > > diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
> > > index ac0f75df1ac9..6ac45d96fcd1 100644
> > > --- a/include/drm/drm_debugfs.h
> > > +++ b/include/drm/drm_debugfs.h
> > > @@ -77,12 +77,23 @@ struct drm_info_node {
> > >  	struct dentry *dent;
> > >  };
> > >  
> > > +struct drm_debugfs_callback;
> > > +
> > >  #if defined(CONFIG_DEBUG_FS)
> > >  int drm_debugfs_create_files(const struct drm_info_list *files,
> > >  			     int count, struct dentry *root,
> > >  			     struct drm_minor *minor);
> > >  int drm_debugfs_remove_files(const struct drm_info_list *files,
> > >  			     int count, struct drm_minor *minor);
> > > +
> > > +int drm_debugfs_register_callback(struct drm_minor *minor,
> > > +				  void (*init)(void *),
> > > +				  void (*cleanup_cb)(void *),
> > > +				  void *data,
> > > +				  struct drm_debugfs_callback **out);
> > > +void drm_debugfs_unregister_callback(struct drm_minor *minor,
> > > +				     struct drm_debugfs_callback *cb);
> > > +
> > >  #else
> > >  static inline int drm_debugfs_create_files(const struct drm_info_list
> > > *files,
> > >  					   int count, struct dentry *root,
> > > @@ -96,6 +107,22 @@ static inline int drm_debugfs_remove_files(const
> > > struct drm_info_list *files,
> > >  {
> > >  	return 0;
> > >  }
> > > +
> > > +static inline int
> > > +drm_debugfs_register_callback(struct drm_minor *minor,
> > > +			      void (*init)(void *),
> > > +			      void (*cleanup_cb)(void *),
> > > +			      void *data,
> > > +			      struct drm_debugfs_callback **out)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static inline void
> > > +drm_debugfs_unregister_callback(struct drm_minor *minor,
> > > +				struct drm_debugfs_callback *cb)
> > > +{
> > > +}
> > >  #endif
> > >  
> > >  #endif /* _DRM_DEBUGFS_H_ */
> > > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > > index 26485acc51d7..180052b51712 100644
> > > --- a/include/drm/drm_file.h
> > > +++ b/include/drm/drm_file.h
> > > @@ -74,6 +74,10 @@ struct drm_minor {
> > >  
> > >  	struct dentry *debugfs_root;
> > >  
> > > +	bool debugfs_callbacks_done;
> > > +	struct list_head debugfs_callback_list;
> > > +	struct mutex debugfs_callback_lock;
> > > +
> > >  	struct list_head debugfs_list;
> > >  	struct mutex debugfs_lock; /* Protects debugfs_list. */
> > >  };
> > > -- 
> > > 2.17.1
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > 
> -- 
> Cheers,
> 	Lyude Paul
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch