Re: [PATCH 04/16] drm/debugfs: Add kerneldoc

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Daniel Vetter <daniel.vetter@xxxxxxxx> writes:

> I've decided to not document drm_debugfs_remove_files, it's on the way
> out.
>
> The biggest part is a huge todor.rst entry with what all should be
> improved.

todo.rst

>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
>  Documentation/gpu/drm-uapi.rst |  9 ++++++++
>  Documentation/gpu/todo.rst     | 26 +++++++++++++++++++++
>  drivers/gpu/drm/drm_debugfs.c  | 51 ++++++------------------------------------
>  drivers/gpu/drm/drm_internal.h |  2 +-
>  include/drm/drm_debugfs.h      | 38 +++++++++++++++++++++++++------
>  5 files changed, 74 insertions(+), 52 deletions(-)
>
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 369e8ca12b8e..76356c86e358 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -210,6 +210,15 @@ Display CRC Support
>  .. kernel-doc:: drivers/gpu/drm/drm_debugfs_crc.c
>     :export:
>  
> +Debugfs Support
> +---------------
> +
> +.. kernel-doc:: include/drm/drm_debugfs.h
> +   :internal:
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_debugfs.c
> +   :export:
> +
>  VBlank event handling
>  =====================
>  
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 64e9d16170ce..9ecd2ebb8dd8 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -272,6 +272,32 @@ This is a really varied tasks with lots of little bits and pieces:
>  
>  Contact: Daniel Vetter
>  
> +Clean up the debugfs support
> +----------------------------
> +
> +There's a bunch of issues with it:
> +
> +- The drm_info_list -> show function doesn't even bother to cast to the drm
> +  structure for you. This is lazy.
> +
> +- We probably want to have some support for debugfs files on crtc/connectors and
> +  maybe other kms objects directly in core. There's even drm_print support in
> +  the funcs for these objects to dump kms state, so it's all there. And then the
> +  ->show functions should obviously give you a pointer to the right object.

should you use ->show() ?  not sure.  Either way, extra space on the
previuous "->show" instance.

> +
> +- The drm_info_list stuff is centered on drm_minor instead of drm_device. For
> +  anything we want to print drm_device (or maybe drm_file) is the right thing.
> +
> +- The drm_driver->debugfs_init hooks we have is just an artifact of the old
> +  midlayered load sequence. DRM debugfs should work more like sysfs, where you
> +  can create properties/files for an object anytime you want, and the core
> +  takes care of publishing/unpuplishing all the files at register/unregister
> +  time. Drivers shouldn't need to worry about these technicalities, and fixing
> +  this (together with the drm_minor->drm_device move) would allow us to remove
> +  debugfs_init.
> +
> +Contact: Daniel Vetter
> +
>  Better Testing
>  ==============
>  
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 4b02f4230562..c1807d5754b2 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -1,10 +1,3 @@
> -/**
> - * \file drm_debugfs.c
> - * debugfs support for DRM
> - *
> - * \author Ben Gamari <bgamari@xxxxxxxxx>
> - */
> -
>  /*
>   * Created: Sun Dec 21 13:08:50 2008 by bgamari@xxxxxxxxx
>   *
> @@ -75,16 +68,15 @@ static const struct file_operations drm_debugfs_fops = {
>  
>  
>  /**
> - * Initialize a given set of debugfs files for a device
> - *
> - * \param files The array of files to create
> - * \param count The number of files given
> - * \param root DRI debugfs dir entry.
> - * \param minor device minor number
> - * \return Zero on success, non-zero on failure
> + * drm_debugfs_create_files - Initialize a given set of debugfs files for DRM
> + * 			minor
> + * @files: The array of files to create
> + * @count: The number of files given
> + * @root: DRI debugfs dir entry.
> + * @minor: device minor number
>   *
>   * Create a given set of debugfs files represented by an array of
> - * &drm_info_list in the given root directory. These files will be removed
> + * &struct drm_info_list in the given root directory. These files will be removed
>   * automatically on drm_debugfs_cleanup().
>   */
>  int drm_debugfs_create_files(const struct drm_info_list *files, int count,
> @@ -133,17 +125,6 @@ int drm_debugfs_create_files(const struct drm_info_list *files, int count,
>  }
>  EXPORT_SYMBOL(drm_debugfs_create_files);
>  
> -/**
> - * Initialize the DRI debugfs filesystem for a device
> - *
> - * \param dev DRM device
> - * \param minor device minor number
> - * \param root DRI debugfs dir entry.
> - *
> - * Create the DRI debugfs root entry "/sys/kernel/debug/dri", the device debugfs root entry
> - * "/sys/kernel/debug/dri/%minor%/", and each entry in debugfs_list as
> - * "/sys/kernel/debug/dri/%minor%/%name%".
> - */
>  int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  		     struct dentry *root)
>  {
> @@ -189,16 +170,6 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  }
>  
>  
> -/**
> - * Remove a list of debugfs files
> - *
> - * \param files The list of files
> - * \param count The number of files
> - * \param minor The minor of which we should remove the files
> - * \return always zero.
> - *
> - * Remove all debugfs entries created by debugfs_init().
> - */
>  int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
>  			     struct drm_minor *minor)
>  {
> @@ -235,14 +206,6 @@ static void drm_debugfs_remove_all_files(struct drm_minor *minor)
>  	mutex_unlock(&minor->debugfs_lock);
>  }
>  
> -/**
> - * Cleanup the debugfs filesystem resources.
> - *
> - * \param minor device minor number.
> - * \return always zero.
> - *
> - * Remove all debugfs entries created by debugfs_init().
> - */
>  int drm_debugfs_cleanup(struct drm_minor *minor)
>  {
>  	if (!minor->debugfs_root)
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 92ff4b9393b1..3d8e8f878924 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -100,7 +100,7 @@ int drm_gem_open_ioctl(struct drm_device *dev, void *data,
>  void drm_gem_open(struct drm_device *dev, struct drm_file *file_private);
>  void drm_gem_release(struct drm_device *dev, struct drm_file *file_private);
>  
> -/* drm_debugfs.c */
> +/* drm_debugfs.c drm_debugfs_crc.c */
>  #if defined(CONFIG_DEBUG_FS)
>  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 17e47c073fa9..d45bc1879412 100644
> --- a/include/drm/drm_debugfs.h
> +++ b/include/drm/drm_debugfs.h
> @@ -33,23 +33,47 @@
>  #define _DRM_DEBUGFS_H_
>  
>  /**
> - * Info file list entry. This structure represents a debugfs or proc file to
> - * be created by the drm core
> + * struct drm_info_list - debugfs info list entry
> + *
> + * This structure represents a debugfs file to be created by the drm
> + * core.
>   */
>  struct drm_info_list {
> -	const char *name; /** file name */
> -	int (*show)(struct seq_file*, void*); /** show callback */
> -	u32 driver_features; /**< Required driver features for this entry */
> +	/** @name: file name */
> +	const char *name;
> +	/**
> +	 * @show:
> +	 *
> +	 * Show callback. &seq_file->private will be set to the &struct
> +	 * drm_info_node corresponding to the instance of this info on a given
> +	 * &struct drm_minor.
> +	 */
> +	int (*show)(struct seq_file*, void*);
> +	/** @driver_features: Required driver features for this entry */
> +	u32 driver_features;
> +	/** @data: Driver-private data, should not be device-specific. */
>  	void *data;
>  };
>  
>  /**
> - * debugfs node structure. This structure represents a debugfs file.
> + * struct drm_info_node - Per-minor debugfs node structure
> + *
> + * This structure represents a debugfs file, as an instantiation of a &struct
> + * drm_info_list on a &struct drm_minor.
> + *
> + * FIXME:
> + *
> + * No it doesn't make a hole lot of sense that we duplicate debugfs entries for
> + * both the render and the primary nodes, but that's how this has organically
> + * grown. It should probably be fixed, with a compatibility link, if needed.
>   */
>  struct drm_info_node {
> -	struct list_head list;
> +	/** @minor: &struct drm_minor for this node. */
>  	struct drm_minor *minor;
> +	/** @info_ent: template for thise node. */

this node

With this nit-picks, feel free to add:

Reviewed-by: Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxxxx>


>  	const struct drm_info_list *info_ent;
> +	/* private: */
> +	struct list_head list;
>  	struct dentry *dent;
>  };

-- 
Gabriel Krisman Bertazi
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux