drm: move debugfs init after dev->load()

Avoids ugly hacks in drivers debugfs code, if it depends on dev->dev_private having already been initialized.

Signed-off-by Rob Clark <[email protected]>

---
Some cleanup that Daniel Vetter wants to do may make this unnecessary in the future, but in order to unblock some msm patches that I'd like to send for 3.16, I propose this patch. If no objections, I could include this in msm pull req.

 drivers/gpu/drm/drm_debugfs.c | 15 +++++++++------
 drivers/gpu/drm/drm_stub.c    | 24 ++++++++++++++++++------
 include/drm/drmP.h            |  6 ++----
 3 files changed, 29 insertions(+), 16 deletions(-)

drivers/gpu/drm/drm_debugfs.c +9 -6

index b4b51d4..231d1ab 100644
@@ -134,23 +134,26 @@ 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)
+int drm_debugfs_init(struct drm_minor *minor)
 {
-       struct drm_device *dev = minor->dev;
+       struct drm_device *dev;
+       struct dentry *root = drm_debugfs_root;
        char name[64];
        int ret;
 
+       if (!minor)
+               return 0;
+
+       dev = minor->dev;
+
        INIT_LIST_HEAD(&minor->debugfs_list);
        mutex_init(&minor->debugfs_lock);
-       sprintf(name, "%d", minor_id);
+       sprintf(name, "%d", minor->index);
        minor->debugfs_root = debugfs_create_dir(name, root);
        if (!minor->debugfs_root) {
                DRM_ERROR("Cannot create /sys/kernel/debug/dri/%s\n", name);

drivers/gpu/drm/drm_stub.c +18 -6

index 3727ac8..94e3742 100644
@@ -328,12 +328,6 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
 
        new_minor->index = minor_id;
 
-       ret = drm_debugfs_init(new_minor, minor_id, drm_debugfs_root);
-       if (ret) {
-               DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
-               goto err_id;
-       }
-
        ret = drm_sysfs_device_add(new_minor);
        if (ret) {
                DRM_ERROR("DRM: Error sysfs_device_add.\n");

@@ -723,6 +717,24 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
                        goto err_minors;
        }
 
+       ret = drm_debugfs_init(dev->control);
+       if (ret) {
+               DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
+               goto err_minors;
+       }
+
+       ret = drm_debugfs_init(dev->render);
+       if (ret) {
+               DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
+               goto err_minors;
+       }
+
+       ret = drm_debugfs_init(dev->primary);
+       if (ret) {
+               DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
+               goto err_minors;
+       }
+
        /* setup grouping for legacy outputs */
        if (drm_core_check_feature(dev, DRIVER_MODESET)) {
                ret = drm_mode_group_init_legacy_group(dev,

include/drm/drmP.h +2 -5

index 12f10bc..76e6e03 100644
@@ -1409,8 +1409,7 @@ extern struct drm_local_map *drm_getsarea(struct drm_device *dev);
 
                                /* Debugfs support */
 #if defined(CONFIG_DEBUG_FS)
-extern int drm_debugfs_init(struct drm_minor *minor, int minor_id,
-                           struct dentry *root);
+extern int drm_debugfs_init(struct drm_minor *minor);
 extern int drm_debugfs_create_files(const struct drm_info_list *files,
                                    int count, struct dentry *root,
                                    struct drm_minor *minor);

@@ -1418,8 +1417,7 @@ extern int drm_debugfs_remove_files(const struct drm_info_list *files,
                                    int count, struct drm_minor *minor);
 extern int drm_debugfs_cleanup(struct drm_minor *minor);
 #else
-static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
-                                  struct dentry *root)
+static inline int drm_debugfs_init(struct drm_minor *minor)
 {
        return 0;
 }
--

1.9.3

Quoted 4 lines · Rob Clark

Avoids ugly hacks in drivers debugfs code, if it depends on dev->dev_private having already been initialized.

Signed-off-by Rob Clark <[email protected]>

So what I had in mind:
- stop using drm_platform_init, instead roll your own copy of drm_get_platform_dev
- don't driver->bus, instead use Thierry's set_busid interface

Then as a second step you can stop calling your ->load callback and instead insert stuff between drm_dev_alloc and drm_dev_register. We have pretty much the same problem with sysfs as with debugfs, so your patch here doesn't really solve all that much - ->load should be called mostly before drm_dev_register sets up all the userspace interfaces.

Long term I want to phase-out ->load except for legacy non-kms drivers.

Quoted 18 lines · Daniel Vetter

Quoted 4 lines · Rob Clark

Avoids ugly hacks in drivers debugfs code, if it depends on dev->dev_private having already been initialized.

Signed-off-by Rob Clark <[email protected]>

So what I had in mind:
- stop using drm_platform_init, instead roll your own copy of drm_get_platform_dev
- don't driver->bus, instead use Thierry's set_busid interface

Then as a second step you can stop calling your ->load callback and instead insert stuff between drm_dev_alloc and drm_dev_register. We have pretty much the same problem with sysfs as with debugfs, so your patch here doesn't really solve all that much - ->load should be called mostly before drm_dev_register sets up all the userspace interfaces.

Quoted 1 line

Long term I want to phase-out ->load except for legacy non-kms drivers.

Also this is debugfs. If someone races debugfs access with driver load it will blow up pretty much everywhere in drm drivers (because of the wrong init order). Imo there's absolutely no need for duct-tape like this, nor for band-aids in drivers. At least in i915 we have 0 dev_priv != NULL checks.

-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch