Avoids ugly hacks in drivers debugfs code, if it depends on dev->dev_private having already been initialized.
Signed-off-by: Rob Clark robdclark@gmail.com --- 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(-)
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c index b4b51d4..231d1ab 100644 --- a/drivers/gpu/drm/drm_debugfs.c +++ b/drivers/gpu/drm/drm_debugfs.c @@ -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); diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 3727ac8..94e3742 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -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, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 12f10bc..76e6e03 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -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; }
On Fri, May 30, 2014 at 12:37 AM, Rob Clark robdclark@gmail.com wrote:
Avoids ugly hacks in drivers debugfs code, if it depends on dev->dev_private having already been initialized.
Signed-off-by: Rob Clark robdclark@gmail.com
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. -Daniel
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(-)
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c index b4b51d4..231d1ab 100644 --- a/drivers/gpu/drm/drm_debugfs.c +++ b/drivers/gpu/drm/drm_debugfs.c @@ -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);diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 3727ac8..94e3742 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -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,diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 12f10bc..76e6e03 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -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
On Fri, May 30, 2014 at 12:46 PM, Daniel Vetter daniel@ffwll.ch wrote:
On Fri, May 30, 2014 at 12:37 AM, Rob Clark robdclark@gmail.com wrote:
Avoids ugly hacks in drivers debugfs code, if it depends on dev->dev_private having already been initialized.
Signed-off-by: Rob Clark robdclark@gmail.com
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.
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
On Fri, May 30, 2014 at 6:49 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Fri, May 30, 2014 at 12:46 PM, Daniel Vetter daniel@ffwll.ch wrote:
On Fri, May 30, 2014 at 12:37 AM, Rob Clark robdclark@gmail.com wrote:
Avoids ugly hacks in drivers debugfs code, if it depends on dev->dev_private having already been initialized.
Signed-off-by: Rob Clark robdclark@gmail.com
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.
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.
It is because some of the debugfs files need to "register" w/ rest of driver.. ie, 'priv->perf = perf' type stuff. Which doesn't work so well in debugfs_init if priv is still null. Changing the order lets this work. I can hack around this by deferring some initialization until first open. But that seemed ugly.
I was kinda hoping to be able to merge this to get rid of those hacks, in a slightly less intrusive way that what you propose. But if that breaks tegradrm, then I'll just hack around this in my debugfs code for now.
BR, -R
-Daniel
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Rob Clark