Hi
This cleans up the bus drivers in DRM. Instead of copying the device alloc/free semantics into each bus driver (drm_{pci,platform,usb}.c) we now have a central place in drm_stub.c.
This introduces drm_dev_{alloc,free,register,unregister}(). They have the same semantics as most other kernel subsystems. *_alloc() allocates a new device and populates the static fields and sub-objects. *_free() frees an unregistered object allocated via *_alloc(). *_register() registers a DRM device and *_unregister() obviously unregisters a DRM device. A *_free() is still needed after calling *_unregister() (same as for "struct device"). No ref-counting is added as it is not required by any driver.
Note that the bus drivers are modified to use the new helpers directly. However, I didn't modify the drivers to use *_unregister() and *_free() directly. Instead, the drm_put_dev() helper was modified to use these. Reason for that is that I have pending patches to make device hotplugging safer regarding mmaps. But these aren't ready, yet. Hopefully I can get them ready for rc5 or rc6.
Tested on nouveau.
David
David Herrmann (5): drm: add drm_dev_alloc() helper drm: merge device setup into drm_dev_register() drm: move drm_lastclose() to drm_fops.c drm: introduce drm_dev_free() to fix error paths drm: move device unregistration into drm_dev_unregister()
drivers/gpu/drm/drm_drv.c | 70 ---------- drivers/gpu/drm/drm_fops.c | 70 ++++++++++ drivers/gpu/drm/drm_pci.c | 62 ++------- drivers/gpu/drm/drm_platform.c | 59 +------- drivers/gpu/drm/drm_stub.c | 310 ++++++++++++++++++++++++++++------------- drivers/gpu/drm/drm_usb.c | 57 +------- include/drm/drmP.h | 8 +- 7 files changed, 308 insertions(+), 328 deletions(-)
Instead of managing device allocation+initialization in each bus-driver, we should do that in a central place. drm_fill_in_dev() already does most of it, but also requires the global drm lock for partial AGP device registration.
Split both apart so we have a clean device initialization/allocation phase, and a registration phase.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_pci.c | 4 +- drivers/gpu/drm/drm_platform.c | 3 +- drivers/gpu/drm/drm_stub.c | 121 +++++++++++++++++++++++++---------------- drivers/gpu/drm/drm_usb.c | 7 +-- include/drm/drmP.h | 2 + 5 files changed, 80 insertions(+), 57 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 1f96cee..d2758be 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -322,7 +322,7 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
DRM_DEBUG("\n");
- dev = kzalloc(sizeof(*dev), GFP_KERNEL); + dev = drm_dev_alloc(driver, &pdev->dev); if (!dev) return -ENOMEM;
@@ -331,8 +331,6 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, goto err_g1;
dev->pdev = pdev; - dev->dev = &pdev->dev; - dev->pci_device = pdev->device; dev->pci_vendor = pdev->vendor;
diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c index f7a18c6..fb27217 100644 --- a/drivers/gpu/drm/drm_platform.c +++ b/drivers/gpu/drm/drm_platform.c @@ -47,12 +47,11 @@ static int drm_get_platform_dev(struct platform_device *platdev,
DRM_DEBUG("\n");
- dev = kzalloc(sizeof(*dev), GFP_KERNEL); + dev = drm_dev_alloc(driver, &platdev->dev); if (!dev) return -ENOMEM;
dev->platformdev = platdev; - dev->dev = &platdev->dev;
mutex_lock(&drm_global_mutex);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 39d8645..64bd52f 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -260,60 +260,15 @@ int drm_fill_in_dev(struct drm_device *dev, { int retcode;
- INIT_LIST_HEAD(&dev->filelist); - INIT_LIST_HEAD(&dev->ctxlist); - INIT_LIST_HEAD(&dev->vmalist); - INIT_LIST_HEAD(&dev->maplist); - INIT_LIST_HEAD(&dev->vblank_event_list); - - spin_lock_init(&dev->count_lock); - spin_lock_init(&dev->event_lock); - mutex_init(&dev->struct_mutex); - mutex_init(&dev->ctxlist_mutex); - - if (drm_ht_create(&dev->map_hash, 12)) { - return -ENOMEM; - } - - /* the DRM has 6 basic counters */ - dev->counters = 6; - dev->types[0] = _DRM_STAT_LOCK; - dev->types[1] = _DRM_STAT_OPENS; - dev->types[2] = _DRM_STAT_CLOSES; - dev->types[3] = _DRM_STAT_IOCTLS; - dev->types[4] = _DRM_STAT_LOCKS; - dev->types[5] = _DRM_STAT_UNLOCKS; - - dev->driver = driver; - if (dev->driver->bus->agp_init) { retcode = dev->driver->bus->agp_init(dev); - if (retcode) - goto error_out_unreg; - } - - - - retcode = drm_ctxbitmap_init(dev); - if (retcode) { - DRM_ERROR("Cannot allocate memory for context bitmap.\n"); - goto error_out_unreg; - } - - if (driver->driver_features & DRIVER_GEM) { - retcode = drm_gem_init(dev); if (retcode) { - DRM_ERROR("Cannot initialize graphics execution " - "manager (GEM)\n"); - goto error_out_unreg; + drm_lastclose(dev); + return retcode; } }
return 0; - - error_out_unreg: - drm_lastclose(dev); - return retcode; } EXPORT_SYMBOL(drm_fill_in_dev);
@@ -490,3 +445,75 @@ void drm_unplug_dev(struct drm_device *dev) mutex_unlock(&drm_global_mutex); } EXPORT_SYMBOL(drm_unplug_dev); + +/** + * drm_dev_alloc - Allocate new drm device + * @driver: DRM driver to allocate device for + * @parent: Parent device object + * + * Allocate and initialize a new DRM device. No device registration is done. + * + * RETURNS: + * Pointer to new DRM device, or NULL if out of memory. + */ +struct drm_device *drm_dev_alloc(struct drm_driver *driver, + struct device *parent) +{ + struct drm_device *dev; + int ret; + + dev = kzalloc(sizeof(*dev), GFP_KERNEL); + if (!dev) + return NULL; + + dev->dev = parent; + dev->driver = driver; + + INIT_LIST_HEAD(&dev->filelist); + INIT_LIST_HEAD(&dev->ctxlist); + INIT_LIST_HEAD(&dev->vmalist); + INIT_LIST_HEAD(&dev->maplist); + INIT_LIST_HEAD(&dev->vblank_event_list); + + spin_lock_init(&dev->count_lock); + spin_lock_init(&dev->event_lock); + mutex_init(&dev->struct_mutex); + mutex_init(&dev->ctxlist_mutex); + + /* the DRM has 6 basic counters */ + dev->counters = 6; + dev->types[0] = _DRM_STAT_LOCK; + dev->types[1] = _DRM_STAT_OPENS; + dev->types[2] = _DRM_STAT_CLOSES; + dev->types[3] = _DRM_STAT_IOCTLS; + dev->types[4] = _DRM_STAT_LOCKS; + dev->types[5] = _DRM_STAT_UNLOCKS; + + if (drm_ht_create(&dev->map_hash, 12)) + goto err_free; + + ret = drm_ctxbitmap_init(dev); + if (ret) { + DRM_ERROR("Cannot allocate memory for context bitmap.\n"); + goto err_ht; + } + + if (driver->driver_features & DRIVER_GEM) { + ret = drm_gem_init(dev); + if (ret) { + DRM_ERROR("Cannot initialize graphics execution manager (GEM)\n"); + goto err_ctxbitmap; + } + } + + return dev; + +err_ctxbitmap: + drm_ctxbitmap_cleanup(dev); +err_ht: + drm_ht_remove(&dev->map_hash); +err_free: + kfree(dev); + return NULL; +} +EXPORT_SYMBOL(drm_dev_alloc); diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c index 8766472..34ad8ed 100644 --- a/drivers/gpu/drm/drm_usb.c +++ b/drivers/gpu/drm/drm_usb.c @@ -7,18 +7,15 @@ int drm_get_usb_dev(struct usb_interface *interface, struct drm_driver *driver) { struct drm_device *dev; - struct usb_device *usbdev; int ret;
DRM_DEBUG("\n");
- dev = kzalloc(sizeof(*dev), GFP_KERNEL); + dev = drm_dev_alloc(driver, &interface->dev); if (!dev) return -ENOMEM;
- usbdev = interface_to_usbdev(interface); - dev->usbdev = usbdev; - dev->dev = &interface->dev; + dev->usbdev = interface_to_usbdev(interface);
mutex_lock(&drm_global_mutex);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index b46fb45..1d4c1d9 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1648,6 +1648,8 @@ static __inline__ void drm_core_dropmap(struct drm_local_map *map) extern int drm_fill_in_dev(struct drm_device *dev, const struct pci_device_id *ent, struct drm_driver *driver); +struct drm_device *drm_dev_alloc(struct drm_driver *driver, + struct device *parent); int drm_get_minor(struct drm_device *dev, struct drm_minor **minor, int type); /*@}*/
All bus drivers do device setup themselves. This requires us to adjust all of them if we introduce new core features. Thus, merge all these into a uniform drm_dev_register() helper.
Note that this removes the drm_lastclose() error path for AGP as it is horribly broken. Moreover, no bus driver called this in any other error path either. Instead, we use the recently introduced AGP cleanup helpers.
We also keep a DRIVER_MODESET condition around pci_set_drvdata() to keep semantics.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_pci.c | 56 +++-------------------- drivers/gpu/drm/drm_platform.c | 54 ++-------------------- drivers/gpu/drm/drm_stub.c | 101 +++++++++++++++++++++++++++++++++-------- drivers/gpu/drm/drm_usb.c | 48 ++------------------ include/drm/drmP.h | 4 +- 5 files changed, 96 insertions(+), 167 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index d2758be..ffea035 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -328,7 +328,7 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
ret = pci_enable_device(pdev); if (ret) - goto err_g1; + goto err_free;
dev->pdev = pdev; dev->pci_device = pdev->device; @@ -338,65 +338,23 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, dev->hose = pdev->sysdata; #endif
- mutex_lock(&drm_global_mutex); - - if ((ret = drm_fill_in_dev(dev, ent, driver))) { - printk(KERN_ERR "DRM: Fill_in_dev failed.\n"); - goto err_g2; - } - - if (drm_core_check_feature(dev, DRIVER_MODESET)) { + if (drm_core_check_feature(dev, DRIVER_MODESET)) pci_set_drvdata(pdev, dev); - ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL); - if (ret) - goto err_g2; - } - - if (drm_core_check_feature(dev, DRIVER_RENDER) && drm_rnodes) { - ret = drm_get_minor(dev, &dev->render, DRM_MINOR_RENDER); - if (ret) - goto err_g21; - }
- if ((ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY))) - goto err_g3; - - if (dev->driver->load) { - ret = dev->driver->load(dev, ent->driver_data); - if (ret) - goto err_g4; - } - - /* setup the grouping for the legacy output */ - if (drm_core_check_feature(dev, DRIVER_MODESET)) { - ret = drm_mode_group_init_legacy_group(dev, - &dev->primary->mode_group); - if (ret) - goto err_g4; - } - - list_add_tail(&dev->driver_item, &driver->device_list); + ret = drm_dev_register(dev); + if (ret) + goto err_pci;
DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n", driver->name, driver->major, driver->minor, driver->patchlevel, driver->date, pci_name(pdev), dev->primary->index);
- mutex_unlock(&drm_global_mutex); return 0;
-err_g4: - drm_put_minor(&dev->primary); -err_g3: - if (dev->render) - drm_put_minor(&dev->render); -err_g21: - if (drm_core_check_feature(dev, DRIVER_MODESET)) - drm_put_minor(&dev->control); -err_g2: +err_pci: pci_disable_device(pdev); -err_g1: +err_free: kfree(dev); - mutex_unlock(&drm_global_mutex); return ret; } EXPORT_SYMBOL(drm_get_pci_dev); diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c index fb27217..a0e402e 100644 --- a/drivers/gpu/drm/drm_platform.c +++ b/drivers/gpu/drm/drm_platform.c @@ -53,48 +53,9 @@ static int drm_get_platform_dev(struct platform_device *platdev,
dev->platformdev = platdev;
- mutex_lock(&drm_global_mutex); - - ret = drm_fill_in_dev(dev, NULL, driver); - - if (ret) { - printk(KERN_ERR "DRM: Fill_in_dev failed.\n"); - goto err_g1; - } - - if (drm_core_check_feature(dev, DRIVER_MODESET)) { - ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL); - if (ret) - goto err_g1; - } - - if (drm_core_check_feature(dev, DRIVER_RENDER) && drm_rnodes) { - ret = drm_get_minor(dev, &dev->render, DRM_MINOR_RENDER); - if (ret) - goto err_g11; - } - - ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY); + ret = drm_dev_register(dev); if (ret) - goto err_g2; - - if (dev->driver->load) { - ret = dev->driver->load(dev, 0); - if (ret) - goto err_g3; - } - - /* setup the grouping for the legacy output */ - if (drm_core_check_feature(dev, DRIVER_MODESET)) { - ret = drm_mode_group_init_legacy_group(dev, - &dev->primary->mode_group); - if (ret) - goto err_g3; - } - - list_add_tail(&dev->driver_item, &driver->device_list); - - mutex_unlock(&drm_global_mutex); + goto err_free;
DRM_INFO("Initialized %s %d.%d.%d %s on minor %d\n", driver->name, driver->major, driver->minor, driver->patchlevel, @@ -102,17 +63,8 @@ static int drm_get_platform_dev(struct platform_device *platdev,
return 0;
-err_g3: - drm_put_minor(&dev->primary); -err_g2: - if (dev->render) - drm_put_minor(&dev->render); -err_g11: - if (drm_core_check_feature(dev, DRIVER_MODESET)) - drm_put_minor(&dev->control); -err_g1: +err_free: kfree(dev); - mutex_unlock(&drm_global_mutex); return ret; }
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 64bd52f..4c8b7d8 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -254,25 +254,6 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data, return 0; }
-int drm_fill_in_dev(struct drm_device *dev, - const struct pci_device_id *ent, - struct drm_driver *driver) -{ - int retcode; - - if (dev->driver->bus->agp_init) { - retcode = dev->driver->bus->agp_init(dev); - if (retcode) { - drm_lastclose(dev); - return retcode; - } - } - - return 0; -} -EXPORT_SYMBOL(drm_fill_in_dev); - - /** * Get a secondary minor number. * @@ -452,6 +433,8 @@ EXPORT_SYMBOL(drm_unplug_dev); * @parent: Parent device object * * Allocate and initialize a new DRM device. No device registration is done. + * Call drm_dev_register() to advertice the device to user space and register it + * with other core subsystems. * * RETURNS: * Pointer to new DRM device, or NULL if out of memory. @@ -517,3 +500,83 @@ err_free: return NULL; } EXPORT_SYMBOL(drm_dev_alloc); + +/** + * drm_dev_register - Register DRM device + * @dev: Device to register + * + * Register the DRM device @dev with the system, advertise device to user-space + * and start normal device operation. @dev must be allocated via drm_dev_alloc() + * previously. + * + * Never call this twice on any device! + * + * RETURNS: + * 0 on success, negative error code on failure. + */ +int drm_dev_register(struct drm_device *dev) +{ + int ret; + + mutex_lock(&drm_global_mutex); + + if (dev->driver->bus->agp_init) { + ret = dev->driver->bus->agp_init(dev); + if (ret) + goto out_unlock; + } + + if (drm_core_check_feature(dev, DRIVER_MODESET)) { + ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL); + if (ret) + goto err_agp; + } + + if (drm_core_check_feature(dev, DRIVER_RENDER) && drm_rnodes) { + ret = drm_get_minor(dev, &dev->render, DRM_MINOR_RENDER); + if (ret) + goto err_control_node; + } + + ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY); + if (ret) + goto err_render_node; + + if (dev->driver->load) { + ret = dev->driver->load(dev, 0); + if (ret) + goto err_primary_node; + } + + /* setup grouping for legacy outputs */ + if (drm_core_check_feature(dev, DRIVER_MODESET)) { + ret = drm_mode_group_init_legacy_group(dev, + &dev->primary->mode_group); + if (ret) + goto err_unload; + } + + list_add_tail(&dev->driver_item, &dev->driver->device_list); + + ret = 0; + goto out_unlock; + +err_unload: + if (dev->driver->unload) + dev->driver->unload(dev); +err_primary_node: + drm_put_minor(&dev->primary); +err_render_node: + if (dev->render) + drm_put_minor(&dev->render); +err_control_node: + if (dev->control) + drm_put_minor(&dev->control); +err_agp: + if (dev->driver->bus->agp_destroy) + dev->driver->bus->agp_destroy(dev); +out_unlock: + mutex_unlock(&drm_global_mutex); + return ret; +} +EXPORT_SYMBOL(drm_dev_register); diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c index 34ad8ed..54d60f2 100644 --- a/drivers/gpu/drm/drm_usb.c +++ b/drivers/gpu/drm/drm_usb.c @@ -16,45 +16,11 @@ int drm_get_usb_dev(struct usb_interface *interface, return -ENOMEM;
dev->usbdev = interface_to_usbdev(interface); - - mutex_lock(&drm_global_mutex); - - ret = drm_fill_in_dev(dev, NULL, driver); - if (ret) { - printk(KERN_ERR "DRM: Fill_in_dev failed.\n"); - goto err_g1; - } - usb_set_intfdata(interface, dev); - ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL); - if (ret) - goto err_g1; - - if (drm_core_check_feature(dev, DRIVER_RENDER) && drm_rnodes) { - ret = drm_get_minor(dev, &dev->render, DRM_MINOR_RENDER); - if (ret) - goto err_g11; - }
- ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY); + ret = drm_dev_register(dev); if (ret) - goto err_g2; - - if (dev->driver->load) { - ret = dev->driver->load(dev, 0); - if (ret) - goto err_g3; - } - - /* setup the grouping for the legacy output */ - ret = drm_mode_group_init_legacy_group(dev, - &dev->primary->mode_group); - if (ret) - goto err_g3; - - list_add_tail(&dev->driver_item, &driver->device_list); - - mutex_unlock(&drm_global_mutex); + goto err_free;
DRM_INFO("Initialized %s %d.%d.%d %s on minor %d\n", driver->name, driver->major, driver->minor, driver->patchlevel, @@ -62,16 +28,8 @@ int drm_get_usb_dev(struct usb_interface *interface,
return 0;
-err_g3: - drm_put_minor(&dev->primary); -err_g2: - if (dev->render) - drm_put_minor(&dev->render); -err_g11: - drm_put_minor(&dev->control); -err_g1: +err_free: kfree(dev); - mutex_unlock(&drm_global_mutex); return ret;
} diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 1d4c1d9..88e17b3 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1645,11 +1645,9 @@ static __inline__ void drm_core_dropmap(struct drm_local_map *map)
#include <drm/drm_mem_util.h>
-extern int drm_fill_in_dev(struct drm_device *dev, - const struct pci_device_id *ent, - struct drm_driver *driver); struct drm_device *drm_dev_alloc(struct drm_driver *driver, struct device *parent); +int drm_dev_register(struct drm_device *dev); int drm_get_minor(struct drm_device *dev, struct drm_minor **minor, int type); /*@}*/
On Wed, Oct 02, 2013 at 11:23:35AM +0200, David Herrmann wrote:
All bus drivers do device setup themselves. This requires us to adjust all of them if we introduce new core features. Thus, merge all these into a uniform drm_dev_register() helper.
Note that this removes the drm_lastclose() error path for AGP as it is horribly broken. Moreover, no bus driver called this in any other error path either. Instead, we use the recently introduced AGP cleanup helpers.
We also keep a DRIVER_MODESET condition around pci_set_drvdata() to keep semantics.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
[snip]
+int drm_dev_register(struct drm_device *dev) +{
- int ret;
- mutex_lock(&drm_global_mutex);
- if (dev->driver->bus->agp_init) {
ret = dev->driver->bus->agp_init(dev);if (ret)goto out_unlock;- }
Imo this should stay in drm_get_pci_dev since its pci specific - no other bus type should ever bother with this really.
Looks good otherwise. -Daniel
Hi Daniel
On Thu, Oct 3, 2013 at 3:15 PM, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Oct 02, 2013 at 11:23:35AM +0200, David Herrmann wrote:
All bus drivers do device setup themselves. This requires us to adjust all of them if we introduce new core features. Thus, merge all these into a uniform drm_dev_register() helper.
Note that this removes the drm_lastclose() error path for AGP as it is horribly broken. Moreover, no bus driver called this in any other error path either. Instead, we use the recently introduced AGP cleanup helpers.
We also keep a DRIVER_MODESET condition around pci_set_drvdata() to keep semantics.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
[snip]
+int drm_dev_register(struct drm_device *dev) +{
int ret;mutex_lock(&drm_global_mutex);if (dev->driver->bus->agp_init) {ret = dev->driver->bus->agp_init(dev);if (ret)goto out_unlock;}Imo this should stay in drm_get_pci_dev since its pci specific - no other bus type should ever bother with this really.
You mean I should _move_ it to drm_pci.c? It has never been pci-specific. But sure, I can do that. It actually makes sense. But I'd like to do that in a followup patch as it's a semantic change that's not directly related. I will include it in v2.
Thanks David
On Thu, Oct 3, 2013 at 3:19 PM, David Herrmann dh.herrmann@gmail.com wrote:
You mean I should _move_ it to drm_pci.c? It has never been pci-specific. But sure, I can do that. It actually makes sense. But I'd like to do that in a followup patch as it's a semantic change that's not directly related. I will include it in v2.
Oh right it's always been in fill_in_dev, but never should have been there ;-) Follow-up patch like you've suggested it better. -Daniel
Try to keep all functions that handle DRM file_operations in drm_fops.c so internal helpers can be marked static later.
This makes the split between the 3 core files more obvious: - drm_stub.c: DRM device allocation/destruction and management - drm_fops.c: DRM file_operations (except for ioctl) - drm_drv.c: Global DRM init + ioctl handling Well, ioctl handling is still spread throughout hundreds of source files, but at least the others are clearly defined this way.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_drv.c | 70 ---------------------------------------------- drivers/gpu/drm/drm_fops.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index e572dd2..34d372e 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -170,76 +170,6 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
#define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
-/** - * drm_legacy_dev_reinit - * - * Reinitializes a legacy/ums drm device in it's lastclose function. - */ -static void drm_legacy_dev_reinit(struct drm_device *dev) -{ - int i; - - if (drm_core_check_feature(dev, DRIVER_MODESET)) - return; - - atomic_set(&dev->ioctl_count, 0); - atomic_set(&dev->vma_count, 0); - - for (i = 0; i < ARRAY_SIZE(dev->counts); i++) - atomic_set(&dev->counts[i], 0); - - dev->sigdata.lock = NULL; - - dev->context_flag = 0; - dev->last_context = 0; - dev->if_version = 0; -} - -/** - * Take down the DRM device. - * - * \param dev DRM device structure. - * - * Frees every resource in \p dev. - * - * \sa drm_device - */ -int drm_lastclose(struct drm_device * dev) -{ - struct drm_vma_entry *vma, *vma_temp; - - DRM_DEBUG("\n"); - - if (dev->driver->lastclose) - dev->driver->lastclose(dev); - DRM_DEBUG("driver lastclose completed\n"); - - if (dev->irq_enabled && !drm_core_check_feature(dev, DRIVER_MODESET)) - drm_irq_uninstall(dev); - - mutex_lock(&dev->struct_mutex); - - drm_agp_clear(dev); - - drm_legacy_sg_cleanup(dev); - - /* Clear vma list (only built for debugging) */ - list_for_each_entry_safe(vma, vma_temp, &dev->vmalist, head) { - list_del(&vma->head); - kfree(vma); - } - - drm_legacy_dma_takedown(dev); - - dev->dev_mapping = NULL; - mutex_unlock(&dev->struct_mutex); - - drm_legacy_dev_reinit(dev); - - DRM_DEBUG("lastclose completed\n"); - return 0; -} - /** File operations structure */ static const struct file_operations drm_stub_fops = { .owner = THIS_MODULE, diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 3f84277..8bc94ea 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -386,6 +386,76 @@ static void drm_events_release(struct drm_file *file_priv) }
/** + * drm_legacy_dev_reinit + * + * Reinitializes a legacy/ums drm device in it's lastclose function. + */ +static void drm_legacy_dev_reinit(struct drm_device *dev) +{ + int i; + + if (drm_core_check_feature(dev, DRIVER_MODESET)) + return; + + atomic_set(&dev->ioctl_count, 0); + atomic_set(&dev->vma_count, 0); + + for (i = 0; i < ARRAY_SIZE(dev->counts); i++) + atomic_set(&dev->counts[i], 0); + + dev->sigdata.lock = NULL; + + dev->context_flag = 0; + dev->last_context = 0; + dev->if_version = 0; +} + +/** + * Take down the DRM device. + * + * \param dev DRM device structure. + * + * Frees every resource in \p dev. + * + * \sa drm_device + */ +int drm_lastclose(struct drm_device * dev) +{ + struct drm_vma_entry *vma, *vma_temp; + + DRM_DEBUG("\n"); + + if (dev->driver->lastclose) + dev->driver->lastclose(dev); + DRM_DEBUG("driver lastclose completed\n"); + + if (dev->irq_enabled && !drm_core_check_feature(dev, DRIVER_MODESET)) + drm_irq_uninstall(dev); + + mutex_lock(&dev->struct_mutex); + + drm_agp_clear(dev); + + drm_legacy_sg_cleanup(dev); + + /* Clear vma list (only built for debugging) */ + list_for_each_entry_safe(vma, vma_temp, &dev->vmalist, head) { + list_del(&vma->head); + kfree(vma); + } + + drm_legacy_dma_takedown(dev); + + dev->dev_mapping = NULL; + mutex_unlock(&dev->struct_mutex); + + drm_legacy_dev_reinit(dev); + + DRM_DEBUG("lastclose completed\n"); + return 0; +} + +/** * Release file. * * \param inode device inode
The error paths in DRM bus drivers currently leak memory as they don't correctly revert drm_dev_alloc(). Introduce drm_dev_free() to free DRM devices which haven't been registered, yet.
We must be careful not to introduce any side-effects with cleanups done in drm_dev_free(). drm_ht_remove(), drm_ctxbitmap_cleanup() and drm_gem_destroy() are all fine in that regard.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_pci.c | 2 +- drivers/gpu/drm/drm_platform.c | 2 +- drivers/gpu/drm/drm_stub.c | 35 +++++++++++++++++++++++++---------- drivers/gpu/drm/drm_usb.c | 2 +- include/drm/drmP.h | 1 + 5 files changed, 29 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index ffea035..f0dc4f9 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -354,7 +354,7 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, err_pci: pci_disable_device(pdev); err_free: - kfree(dev); + drm_dev_free(dev); return ret; } EXPORT_SYMBOL(drm_get_pci_dev); diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c index a0e402e..6b37409 100644 --- a/drivers/gpu/drm/drm_platform.c +++ b/drivers/gpu/drm/drm_platform.c @@ -64,7 +64,7 @@ static int drm_get_platform_dev(struct platform_device *platdev, return 0;
err_free: - kfree(dev); + drm_dev_free(dev); return ret; }
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 4c8b7d8..397ab90 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -363,7 +363,6 @@ static void drm_unplug_minor(struct drm_minor *minor) */ void drm_put_dev(struct drm_device *dev) { - struct drm_driver *driver; struct drm_map_list *r_list, *list_temp;
DRM_DEBUG("\n"); @@ -372,7 +371,6 @@ void drm_put_dev(struct drm_device *dev) DRM_ERROR("cleanup called no dev\n"); return; } - driver = dev->driver;
drm_lastclose(dev);
@@ -386,9 +384,6 @@ void drm_put_dev(struct drm_device *dev)
list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head) drm_rmmap(dev, r_list->map); - drm_ht_remove(&dev->map_hash); - - drm_ctxbitmap_cleanup(dev);
if (drm_core_check_feature(dev, DRIVER_MODESET)) drm_put_minor(&dev->control); @@ -396,14 +391,11 @@ void drm_put_dev(struct drm_device *dev) if (dev->render) drm_put_minor(&dev->render);
- if (driver->driver_features & DRIVER_GEM) - drm_gem_destroy(dev); - drm_put_minor(&dev->primary);
list_del(&dev->driver_item); - kfree(dev->devname); - kfree(dev); + + drm_dev_free(dev); } EXPORT_SYMBOL(drm_put_dev);
@@ -502,6 +494,29 @@ err_free: EXPORT_SYMBOL(drm_dev_alloc);
/** + * drm_dev_free - Free DRM device + * @dev: DRM device to free + * + * Free a DRM device that has previously been allocated via drm_dev_alloc(). + * You must not use kfree() instead or you will leak memory. + * + * This must not be called once the device got registered. Use drm_put_dev() + * instead, which then calls drm_dev_free(). + */ +void drm_dev_free(struct drm_device *dev) +{ + if (dev->driver->driver_features & DRIVER_GEM) + drm_gem_destroy(dev); + + drm_ctxbitmap_cleanup(dev); + drm_ht_remove(&dev->map_hash); + + kfree(dev->devname); + kfree(dev); +} +EXPORT_SYMBOL(drm_dev_free); + +/** * drm_dev_register - Register DRM device * @dev: Device to register * diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c index 54d60f2..bace9d9 100644 --- a/drivers/gpu/drm/drm_usb.c +++ b/drivers/gpu/drm/drm_usb.c @@ -29,7 +29,7 @@ int drm_get_usb_dev(struct usb_interface *interface, return 0;
err_free: - kfree(dev); + drm_dev_free(dev); return ret;
} diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 88e17b3..f4ec6b3 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1647,6 +1647,7 @@ static __inline__ void drm_core_dropmap(struct drm_local_map *map)
struct drm_device *drm_dev_alloc(struct drm_driver *driver, struct device *parent); +void drm_dev_free(struct drm_device *dev); int drm_dev_register(struct drm_device *dev); int drm_get_minor(struct drm_device *dev, struct drm_minor **minor, int type); /*@}*/
Analog to drm_dev_register(), we now provide drm_dev_unregister() which does the reverse. drm_dev_put() is still in place and combines the calls to drm_dev_unregister() and drm_dev_free() so buses don't have to change.
*_get() and *_put() are used for reference-counting in the kernel. However, drm_dev_put() definitely does not do any kind of ref-counting. Hence, use the more appropriate *_register(), *_unregister(), *_alloc() and *_free() names.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_stub.c | 61 +++++++++++++++++++++++++++------------------- include/drm/drmP.h | 1 + 2 files changed, 37 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 397ab90..a5290b7 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -363,8 +363,6 @@ static void drm_unplug_minor(struct drm_minor *minor) */ void drm_put_dev(struct drm_device *dev) { - struct drm_map_list *r_list, *list_temp; - DRM_DEBUG("\n");
if (!dev) { @@ -372,29 +370,7 @@ void drm_put_dev(struct drm_device *dev) return; }
- drm_lastclose(dev); - - if (dev->driver->unload) - dev->driver->unload(dev); - - if (dev->driver->bus->agp_destroy) - dev->driver->bus->agp_destroy(dev); - - drm_vblank_cleanup(dev); - - list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head) - drm_rmmap(dev, r_list->map); - - if (drm_core_check_feature(dev, DRIVER_MODESET)) - drm_put_minor(&dev->control); - - if (dev->render) - drm_put_minor(&dev->render); - - drm_put_minor(&dev->primary); - - list_del(&dev->driver_item); - + drm_dev_unregister(dev); drm_dev_free(dev); } EXPORT_SYMBOL(drm_put_dev); @@ -595,3 +571,38 @@ out_unlock: return ret; } EXPORT_SYMBOL(drm_dev_register); + +/** + * drm_dev_unregister - Unregister DRM device + * @dev: Device to unregister + * + * Unregister the DRM device from the system. This does the reverse of + * drm_dev_register() but does not deallocate the device. The caller must call + * drm_dev_free() to free all resources. + */ +void drm_dev_unregister(struct drm_device *dev) +{ + struct drm_map_list *r_list, *list_temp; + + drm_lastclose(dev); + + if (dev->driver->unload) + dev->driver->unload(dev); + + if (dev->driver->bus->agp_destroy) + dev->driver->bus->agp_destroy(dev); + + drm_vblank_cleanup(dev); + + list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head) + drm_rmmap(dev, r_list->map); + + if (dev->control) + drm_put_minor(&dev->control); + if (dev->render) + drm_put_minor(&dev->render); + drm_put_minor(&dev->primary); + + list_del(&dev->driver_item); +} +EXPORT_SYMBOL(drm_dev_unregister); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index f4ec6b3..7516d22 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1649,6 +1649,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver, struct device *parent); void drm_dev_free(struct drm_device *dev); int drm_dev_register(struct drm_device *dev); +void drm_dev_unregister(struct drm_device *dev); int drm_get_minor(struct drm_device *dev, struct drm_minor **minor, int type); /*@}*/
On Wed, Oct 02, 2013 at 11:23:38AM +0200, David Herrmann wrote:
Analog to drm_dev_register(), we now provide drm_dev_unregister() which does the reverse. drm_dev_put() is still in place and combines the calls to drm_dev_unregister() and drm_dev_free() so buses don't have to change.
*_get() and *_put() are used for reference-counting in the kernel. However, drm_dev_put() definitely does not do any kind of ref-counting. Hence, use the more appropriate *_register(), *_unregister(), *_alloc() and *_free() names.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
With the agp_init call moved back into pci code this series is
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
We still have a bit a midlayer smell here, and to correctly fix the init/cleanup order I think a bit more work is required. But this is a definit step up imo. -Daniel
drivers/gpu/drm/drm_stub.c | 61 +++++++++++++++++++++++++++------------------- include/drm/drmP.h | 1 + 2 files changed, 37 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 397ab90..a5290b7 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -363,8 +363,6 @@ static void drm_unplug_minor(struct drm_minor *minor) */ void drm_put_dev(struct drm_device *dev) {
struct drm_map_list *r_list, *list_temp;
DRM_DEBUG("\n");
if (!dev) {
@@ -372,29 +370,7 @@ void drm_put_dev(struct drm_device *dev) return; }
- drm_lastclose(dev);
- if (dev->driver->unload)
dev->driver->unload(dev);- if (dev->driver->bus->agp_destroy)
dev->driver->bus->agp_destroy(dev);- drm_vblank_cleanup(dev);
- list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head)
drm_rmmap(dev, r_list->map);- if (drm_core_check_feature(dev, DRIVER_MODESET))
drm_put_minor(&dev->control);- if (dev->render)
drm_put_minor(&dev->render);- drm_put_minor(&dev->primary);
- list_del(&dev->driver_item);
- drm_dev_unregister(dev); drm_dev_free(dev);
} EXPORT_SYMBOL(drm_put_dev); @@ -595,3 +571,38 @@ out_unlock: return ret; } EXPORT_SYMBOL(drm_dev_register);
+/**
- drm_dev_unregister - Unregister DRM device
- @dev: Device to unregister
- Unregister the DRM device from the system. This does the reverse of
- drm_dev_register() but does not deallocate the device. The caller must call
- drm_dev_free() to free all resources.
- */
+void drm_dev_unregister(struct drm_device *dev) +{
- struct drm_map_list *r_list, *list_temp;
- drm_lastclose(dev);
- if (dev->driver->unload)
dev->driver->unload(dev);- if (dev->driver->bus->agp_destroy)
dev->driver->bus->agp_destroy(dev);- drm_vblank_cleanup(dev);
- list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head)
drm_rmmap(dev, r_list->map);- if (dev->control)
drm_put_minor(&dev->control);- if (dev->render)
drm_put_minor(&dev->render);- drm_put_minor(&dev->primary);
- list_del(&dev->driver_item);
+} +EXPORT_SYMBOL(drm_dev_unregister); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index f4ec6b3..7516d22 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1649,6 +1649,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver, struct device *parent); void drm_dev_free(struct drm_device *dev); int drm_dev_register(struct drm_device *dev); +void drm_dev_unregister(struct drm_device *dev); int drm_get_minor(struct drm_device *dev, struct drm_minor **minor, int type); /*@}*/
-- 1.8.4
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Oct 2, 2013 at 7:23 PM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
This cleans up the bus drivers in DRM. Instead of copying the device alloc/free semantics into each bus driver (drm_{pci,platform,usb}.c) we now have a central place in drm_stub.c.
This introduces drm_dev_{alloc,free,register,unregister}(). They have the same semantics as most other kernel subsystems. *_alloc() allocates a new device and populates the static fields and sub-objects. *_free() frees an unregistered object allocated via *_alloc(). *_register() registers a DRM device and *_unregister() obviously unregisters a DRM device. A *_free() is still needed after calling *_unregister() (same as for "struct device"). No ref-counting is added as it is not required by any driver.
Note that the bus drivers are modified to use the new helpers directly. However, I didn't modify the drivers to use *_unregister() and *_free() directly. Instead, the drm_put_dev() helper was modified to use these. Reason for that is that I have pending patches to make device hotplugging safer regarding mmaps. But these aren't ready, yet. Hopefully I can get them ready for rc5 or rc6.
Tested on nouveau.
Okay I've merged this series, a follow-on to fix the AGP bits like Daniel suggested would be good.
Dave.
On Wed, Oct 9, 2013 at 3:31 PM, Dave Airlie airlied@gmail.com wrote:
On Wed, Oct 2, 2013 at 7:23 PM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
This cleans up the bus drivers in DRM. Instead of copying the device alloc/free semantics into each bus driver (drm_{pci,platform,usb}.c) we now have a central place in drm_stub.c.
This introduces drm_dev_{alloc,free,register,unregister}(). They have the same semantics as most other kernel subsystems. *_alloc() allocates a new device and populates the static fields and sub-objects. *_free() frees an unregistered object allocated via *_alloc(). *_register() registers a DRM device and *_unregister() obviously unregisters a DRM device. A *_free() is still needed after calling *_unregister() (same as for "struct device"). No ref-counting is added as it is not required by any driver.
Note that the bus drivers are modified to use the new helpers directly. However, I didn't modify the drivers to use *_unregister() and *_free() directly. Instead, the drm_put_dev() helper was modified to use these. Reason for that is that I have pending patches to make device hotplugging safer regarding mmaps. But these aren't ready, yet. Hopefully I can get them ready for rc5 or rc6.
Tested on nouveau.
Okay I've merged this series, a follow-on to fix the AGP bits like Daniel suggested would be good.
Actually this oopses i915 on startup, I fixed up the lack of passing flags into drm_dev_register, so it can be passed to load, and it seems to work now, I've squashed my fix into the series in my tree
please read/review the series for your learning pleasure.
Dave.
On Wed, Oct 09, 2013 at 04:00:00PM +1000, Dave Airlie wrote:
On Wed, Oct 9, 2013 at 3:31 PM, Dave Airlie airlied@gmail.com wrote:
On Wed, Oct 2, 2013 at 7:23 PM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
This cleans up the bus drivers in DRM. Instead of copying the device alloc/free semantics into each bus driver (drm_{pci,platform,usb}.c) we now have a central place in drm_stub.c.
This introduces drm_dev_{alloc,free,register,unregister}(). They have the same semantics as most other kernel subsystems. *_alloc() allocates a new device and populates the static fields and sub-objects. *_free() frees an unregistered object allocated via *_alloc(). *_register() registers a DRM device and *_unregister() obviously unregisters a DRM device. A *_free() is still needed after calling *_unregister() (same as for "struct device"). No ref-counting is added as it is not required by any driver.
Note that the bus drivers are modified to use the new helpers directly. However, I didn't modify the drivers to use *_unregister() and *_free() directly. Instead, the drm_put_dev() helper was modified to use these. Reason for that is that I have pending patches to make device hotplugging safer regarding mmaps. But these aren't ready, yet. Hopefully I can get them ready for rc5 or rc6.
Tested on nouveau.
Okay I've merged this series, a follow-on to fix the AGP bits like Daniel suggested would be good.
Actually this oopses i915 on startup, I fixed up the lack of passing flags into drm_dev_register, so it can be passed to load, and it seems to work now, I've squashed my fix into the series in my tree
please read/review the series for your learning pleasure.
Oops, shame on me for failing to spot the unconditional 0 passed for flags. i915.ko really doesn't like that ;-) -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Dave Airlie -
David Herrmann