| Message ID | 20170322083617.13361-5-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Daniel Vetter <daniel.vetter@ffwll.ch> 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@ffwll.ch> > --- > 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@gmail.com> > - */ > - > /* > * Created: Sun Dec 21 13:08:50 2008 by bgamari@gmail.com > * > @@ -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@collabora.co.uk> > const struct drm_info_list *info_ent; > + /* private: */ > + struct list_head list; > struct dentry *dent; > };
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. + +- 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@gmail.com> - */ - /* * Created: Sun Dec 21 13:08:50 2008 by bgamari@gmail.com * @@ -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. */ const struct drm_info_list *info_ent; + /* private: */ + struct list_head list; struct dentry *dent; };
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. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- 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(-)