* [RFC 1/9] drm: add drm_dev_alloc() helper
2013-07-24 13:43 [RFC 0/9] DRM: Device Handling Cleanup David Herrmann
@ 2013-07-24 13:43 ` David Herrmann
2013-07-24 13:43 ` [RFC 2/9] drm: merge device setup into drm_dev_register() David Herrmann
` (7 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: David Herrmann @ 2013-07-24 13:43 UTC (permalink / raw)
To: dri-devel
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 a7b46ff..fa1de13 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -312,7 +312,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;
@@ -321,8 +321,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 b8a282e..f7cfbb5 100644
--- a/drivers/gpu/drm/drm_platform.c
+++ b/drivers/gpu/drm/drm_platform.c
@@ -47,12 +47,11 @@ 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 327ca19..7b37906 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -256,60 +256,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);
@@ -501,3 +456,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_map_hash;
+ }
+
+ 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_map_hash:
+ 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 34a156f..7f016e4 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 0ab6a09..b1f00c5 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1705,6 +1705,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);
/*@}*/
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* [RFC 2/9] drm: merge device setup into drm_dev_register()
2013-07-24 13:43 [RFC 0/9] DRM: Device Handling Cleanup David Herrmann
2013-07-24 13:43 ` [RFC 1/9] drm: add drm_dev_alloc() helper David Herrmann
@ 2013-07-24 13:43 ` David Herrmann
2013-07-24 13:43 ` [RFC 3/9] drm/agp: fix AGP cleanup paths David Herrmann
` (6 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: David Herrmann @ 2013-07-24 13:43 UTC (permalink / raw)
To: dri-devel
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. Follow up patches will fix AGP error paths.
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 | 47 ++++------------------
drivers/gpu/drm/drm_platform.c | 45 ++-------------------
drivers/gpu/drm/drm_stub.c | 91 +++++++++++++++++++++++++++++++++---------
drivers/gpu/drm/drm_usb.c | 39 ++----------------
include/drm/drmP.h | 4 +-
5 files changed, 86 insertions(+), 140 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index fa1de13..ab861ca 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -318,7 +318,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;
@@ -328,56 +328,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 ((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 (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 f7cfbb5..396713e 100644
--- a/drivers/gpu/drm/drm_platform.c
+++ b/drivers/gpu/drm/drm_platform.c
@@ -53,42 +53,9 @@ 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;
- }
-
- 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,
@@ -96,14 +63,8 @@ int drm_get_platform_dev(struct platform_device *platdev,
return 0;
-err_g3:
- drm_put_minor(&dev->primary);
-err_g2:
- 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;
}
EXPORT_SYMBOL(drm_get_platform_dev);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 7b37906..bc1d860 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -250,25 +250,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.
*
@@ -463,6 +444,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.
@@ -528,3 +511,73 @@ 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;
+ }
+
+ ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY);
+ if (ret)
+ goto err_control_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_control_node:
+ if (drm_core_check_feature(dev, DRIVER_MODESET))
+ drm_put_minor(&dev->control);
+err_agp:
+ /* FIXME: cleanup AGP leftovers */
+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 7f016e4..54d60f2 100644
--- a/drivers/gpu/drm/drm_usb.c
+++ b/drivers/gpu/drm/drm_usb.c
@@ -16,39 +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;
- 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,
@@ -56,13 +28,8 @@ int drm_get_usb_dev(struct usb_interface *interface,
return 0;
-err_g3:
- drm_put_minor(&dev->primary);
-err_g2:
- 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 b1f00c5..ea5e130 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1702,11 +1702,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);
/*@}*/
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* [RFC 3/9] drm/agp: fix AGP cleanup paths
2013-07-24 13:43 [RFC 0/9] DRM: Device Handling Cleanup David Herrmann
2013-07-24 13:43 ` [RFC 1/9] drm: add drm_dev_alloc() helper David Herrmann
2013-07-24 13:43 ` [RFC 2/9] drm: merge device setup into drm_dev_register() David Herrmann
@ 2013-07-24 13:43 ` David Herrmann
2013-07-27 6:05 ` Dave Airlie
2013-07-24 13:43 ` [RFC 4/9] drm: move drm_lastclose() to drm_fops.c David Herrmann
` (5 subsequent siblings)
8 siblings, 1 reply; 15+ messages in thread
From: David Herrmann @ 2013-07-24 13:43 UTC (permalink / raw)
To: dri-devel
Introduce two new helpers, drm_agp_clear() and drm_agp_destroy() which
clear all AGP mappings and destroy the AGP head. This allows to reduce the
AGP code in core DRM and move it all to drm_agpsupport.c.
Also use these new helpers to clean up AGP allocations in the
drm_dev_register() error path.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/gpu/drm/drm_agpsupport.c | 51 ++++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/drm_drv.c | 21 +----------------
drivers/gpu/drm/drm_pci.c | 12 ++++++++++
drivers/gpu/drm/drm_stub.c | 12 ++++------
include/drm/drmP.h | 3 +++
5 files changed, 71 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/drm_agpsupport.c b/drivers/gpu/drm/drm_agpsupport.c
index 3d8fed1..e301d65 100644
--- a/drivers/gpu/drm/drm_agpsupport.c
+++ b/drivers/gpu/drm/drm_agpsupport.c
@@ -424,6 +424,57 @@ struct drm_agp_head *drm_agp_init(struct drm_device *dev)
}
/**
+ * drm_agp_clear - Clear AGP resource list
+ * @dev: DRM device
+ *
+ * Iterate over all AGP resources and remove them. But keep the AGP head
+ * intact so it can still be used. It is safe to call this if AGP is disabled or
+ * was already removed.
+ *
+ * If DRIVER_MODESET is active, nothing is done to protect the modesetting
+ * resources from getting destroyed. Drivers are responsible of cleaning them up
+ * during device shutdown.
+ */
+void drm_agp_clear(struct drm_device *dev)
+{
+ struct drm_agp_mem *entry, *tempe;
+
+ if (!drm_core_has_AGP(dev) || !dev->agp)
+ return;
+ if (drm_core_check_feature(dev, DRIVER_MODESET))
+ return;
+
+ list_for_each_entry_safe(entry, tempe, &dev->agp->memory, head) {
+ if (entry->bound)
+ drm_unbind_agp(entry->memory);
+ drm_free_agp(entry->memory, entry->pages);
+ kfree(entry);
+ }
+ INIT_LIST_HEAD(&dev->agp->memory);
+
+ if (dev->agp->acquired)
+ drm_agp_release(dev);
+
+ dev->agp->acquired = 0;
+ dev->agp->enabled = 0;
+}
+
+/**
+ * drm_agp_destroy - Destroy AGP head
+ * @dev: DRM device
+ *
+ * Destroy resources that were previously allocated via drm_agp_initp. Caller
+ * must ensure to clean up all AGP resources before calling this. See
+ * drm_agp_clear().
+ *
+ * Call this to destroy AGP heads allocated via drm_agp_init().
+ */
+void drm_agp_destroy(struct drm_agp_head *agp)
+{
+ kfree(agp);
+}
+
+/**
* Binds a collection of pages into AGP memory at the given offset, returning
* the AGP memory structure containing them.
*
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 36103d1..dddd799 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -195,27 +195,8 @@ int drm_lastclose(struct drm_device * dev)
mutex_lock(&dev->struct_mutex);
- /* Clear AGP information */
- if (drm_core_has_AGP(dev) && dev->agp &&
- !drm_core_check_feature(dev, DRIVER_MODESET)) {
- struct drm_agp_mem *entry, *tempe;
-
- /* Remove AGP resources, but leave dev->agp
- intact until drv_cleanup is called. */
- list_for_each_entry_safe(entry, tempe, &dev->agp->memory, head) {
- if (entry->bound)
- drm_unbind_agp(entry->memory);
- drm_free_agp(entry->memory, entry->pages);
- kfree(entry);
- }
- INIT_LIST_HEAD(&dev->agp->memory);
-
- if (dev->agp->acquired)
- drm_agp_release(dev);
+ drm_agp_clear(dev);
- dev->agp->acquired = 0;
- dev->agp->enabled = 0;
- }
if (drm_core_check_feature(dev, DRIVER_SG) && dev->sg &&
!drm_core_check_feature(dev, DRIVER_MODESET)) {
drm_sg_cleanup(dev->sg);
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index ab861ca..0daee24 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -283,6 +283,17 @@ static int drm_pci_agp_init(struct drm_device *dev)
return 0;
}
+static void drm_pci_agp_destroy(struct drm_device *dev)
+{
+ if (drm_core_has_AGP(dev) && dev->agp) {
+ if (drm_core_has_MTRR(dev))
+ arch_phys_wc_del(dev->agp->agp_mtrr);
+ drm_agp_clear(dev);
+ drm_agp_destroy(dev->agp);
+ dev->agp = NULL;
+ }
+}
+
static struct drm_bus drm_pci_bus = {
.bus_type = DRIVER_BUS_PCI,
.get_irq = drm_pci_get_irq,
@@ -291,6 +302,7 @@ static struct drm_bus drm_pci_bus = {
.set_unique = drm_pci_set_unique,
.irq_by_busid = drm_pci_irq_by_busid,
.agp_init = drm_pci_agp_init,
+ .agp_destroy = drm_pci_agp_destroy,
};
/**
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index bc1d860..f016ed8 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -387,16 +387,11 @@ void drm_put_dev(struct drm_device *dev)
drm_lastclose(dev);
- if (drm_core_has_MTRR(dev) && drm_core_has_AGP(dev) && dev->agp)
- arch_phys_wc_del(dev->agp->agp_mtrr);
-
if (dev->driver->unload)
dev->driver->unload(dev);
- if (drm_core_has_AGP(dev) && dev->agp) {
- kfree(dev->agp);
- dev->agp = NULL;
- }
+ if (dev->driver->bus->agp_destroy)
+ dev->driver->bus->agp_destroy(dev);
drm_vblank_cleanup(dev);
@@ -575,7 +570,8 @@ err_control_node:
if (drm_core_check_feature(dev, DRIVER_MODESET))
drm_put_minor(&dev->control);
err_agp:
- /* FIXME: cleanup AGP leftovers */
+ if (dev->driver->bus->agp_destroy)
+ dev->driver->bus->agp_destroy(dev);
out_unlock:
mutex_unlock(&drm_global_mutex);
return ret;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index ea5e130..30f83ee 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -737,6 +737,7 @@ struct drm_bus {
int (*irq_by_busid)(struct drm_device *dev, struct drm_irq_busid *p);
/* hooks that are for PCI */
int (*agp_init)(struct drm_device *dev);
+ void (*agp_destroy)(struct drm_device *dev);
};
@@ -1454,6 +1455,8 @@ extern int drm_modeset_ctl(struct drm_device *dev, void *data,
/* AGP/GART support (drm_agpsupport.h) */
extern struct drm_agp_head *drm_agp_init(struct drm_device *dev);
+extern void drm_agp_destroy(struct drm_agp_head *agp);
+extern void drm_agp_clear(struct drm_device *dev);
extern int drm_agp_acquire(struct drm_device *dev);
extern int drm_agp_acquire_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [RFC 3/9] drm/agp: fix AGP cleanup paths
2013-07-24 13:43 ` [RFC 3/9] drm/agp: fix AGP cleanup paths David Herrmann
@ 2013-07-27 6:05 ` Dave Airlie
2013-07-27 12:09 ` David Herrmann
0 siblings, 1 reply; 15+ messages in thread
From: Dave Airlie @ 2013-07-27 6:05 UTC (permalink / raw)
To: David Herrmann; +Cc: dri-devel
On Wed, Jul 24, 2013 at 11:43 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Introduce two new helpers, drm_agp_clear() and drm_agp_destroy() which
> clear all AGP mappings and destroy the AGP head. This allows to reduce the
> AGP code in core DRM and move it all to drm_agpsupport.c.
>
Could we do this as the first patch? it seems like it would be a nice
thing to have standalone even with the current init paths.
Dave.
> Also use these new helpers to clean up AGP allocations in the
> drm_dev_register() error path.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> drivers/gpu/drm/drm_agpsupport.c | 51 ++++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/drm_drv.c | 21 +----------------
> drivers/gpu/drm/drm_pci.c | 12 ++++++++++
> drivers/gpu/drm/drm_stub.c | 12 ++++------
> include/drm/drmP.h | 3 +++
> 5 files changed, 71 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_agpsupport.c b/drivers/gpu/drm/drm_agpsupport.c
> index 3d8fed1..e301d65 100644
> --- a/drivers/gpu/drm/drm_agpsupport.c
> +++ b/drivers/gpu/drm/drm_agpsupport.c
> @@ -424,6 +424,57 @@ struct drm_agp_head *drm_agp_init(struct drm_device *dev)
> }
>
> /**
> + * drm_agp_clear - Clear AGP resource list
> + * @dev: DRM device
> + *
> + * Iterate over all AGP resources and remove them. But keep the AGP head
> + * intact so it can still be used. It is safe to call this if AGP is disabled or
> + * was already removed.
> + *
> + * If DRIVER_MODESET is active, nothing is done to protect the modesetting
> + * resources from getting destroyed. Drivers are responsible of cleaning them up
> + * during device shutdown.
> + */
> +void drm_agp_clear(struct drm_device *dev)
> +{
> + struct drm_agp_mem *entry, *tempe;
> +
> + if (!drm_core_has_AGP(dev) || !dev->agp)
> + return;
> + if (drm_core_check_feature(dev, DRIVER_MODESET))
> + return;
> +
> + list_for_each_entry_safe(entry, tempe, &dev->agp->memory, head) {
> + if (entry->bound)
> + drm_unbind_agp(entry->memory);
> + drm_free_agp(entry->memory, entry->pages);
> + kfree(entry);
> + }
> + INIT_LIST_HEAD(&dev->agp->memory);
> +
> + if (dev->agp->acquired)
> + drm_agp_release(dev);
> +
> + dev->agp->acquired = 0;
> + dev->agp->enabled = 0;
> +}
> +
> +/**
> + * drm_agp_destroy - Destroy AGP head
> + * @dev: DRM device
> + *
> + * Destroy resources that were previously allocated via drm_agp_initp. Caller
> + * must ensure to clean up all AGP resources before calling this. See
> + * drm_agp_clear().
> + *
> + * Call this to destroy AGP heads allocated via drm_agp_init().
> + */
> +void drm_agp_destroy(struct drm_agp_head *agp)
> +{
> + kfree(agp);
> +}
> +
> +/**
> * Binds a collection of pages into AGP memory at the given offset, returning
> * the AGP memory structure containing them.
> *
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 36103d1..dddd799 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -195,27 +195,8 @@ int drm_lastclose(struct drm_device * dev)
>
> mutex_lock(&dev->struct_mutex);
>
> - /* Clear AGP information */
> - if (drm_core_has_AGP(dev) && dev->agp &&
> - !drm_core_check_feature(dev, DRIVER_MODESET)) {
> - struct drm_agp_mem *entry, *tempe;
> -
> - /* Remove AGP resources, but leave dev->agp
> - intact until drv_cleanup is called. */
> - list_for_each_entry_safe(entry, tempe, &dev->agp->memory, head) {
> - if (entry->bound)
> - drm_unbind_agp(entry->memory);
> - drm_free_agp(entry->memory, entry->pages);
> - kfree(entry);
> - }
> - INIT_LIST_HEAD(&dev->agp->memory);
> -
> - if (dev->agp->acquired)
> - drm_agp_release(dev);
> + drm_agp_clear(dev);
>
> - dev->agp->acquired = 0;
> - dev->agp->enabled = 0;
> - }
> if (drm_core_check_feature(dev, DRIVER_SG) && dev->sg &&
> !drm_core_check_feature(dev, DRIVER_MODESET)) {
> drm_sg_cleanup(dev->sg);
> diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
> index ab861ca..0daee24 100644
> --- a/drivers/gpu/drm/drm_pci.c
> +++ b/drivers/gpu/drm/drm_pci.c
> @@ -283,6 +283,17 @@ static int drm_pci_agp_init(struct drm_device *dev)
> return 0;
> }
>
> +static void drm_pci_agp_destroy(struct drm_device *dev)
> +{
> + if (drm_core_has_AGP(dev) && dev->agp) {
> + if (drm_core_has_MTRR(dev))
> + arch_phys_wc_del(dev->agp->agp_mtrr);
> + drm_agp_clear(dev);
> + drm_agp_destroy(dev->agp);
> + dev->agp = NULL;
> + }
> +}
> +
> static struct drm_bus drm_pci_bus = {
> .bus_type = DRIVER_BUS_PCI,
> .get_irq = drm_pci_get_irq,
> @@ -291,6 +302,7 @@ static struct drm_bus drm_pci_bus = {
> .set_unique = drm_pci_set_unique,
> .irq_by_busid = drm_pci_irq_by_busid,
> .agp_init = drm_pci_agp_init,
> + .agp_destroy = drm_pci_agp_destroy,
> };
>
> /**
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index bc1d860..f016ed8 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -387,16 +387,11 @@ void drm_put_dev(struct drm_device *dev)
>
> drm_lastclose(dev);
>
> - if (drm_core_has_MTRR(dev) && drm_core_has_AGP(dev) && dev->agp)
> - arch_phys_wc_del(dev->agp->agp_mtrr);
> -
> if (dev->driver->unload)
> dev->driver->unload(dev);
>
> - if (drm_core_has_AGP(dev) && dev->agp) {
> - kfree(dev->agp);
> - dev->agp = NULL;
> - }
> + if (dev->driver->bus->agp_destroy)
> + dev->driver->bus->agp_destroy(dev);
>
> drm_vblank_cleanup(dev);
>
> @@ -575,7 +570,8 @@ err_control_node:
> if (drm_core_check_feature(dev, DRIVER_MODESET))
> drm_put_minor(&dev->control);
> err_agp:
> - /* FIXME: cleanup AGP leftovers */
> + if (dev->driver->bus->agp_destroy)
> + dev->driver->bus->agp_destroy(dev);
> out_unlock:
> mutex_unlock(&drm_global_mutex);
> return ret;
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index ea5e130..30f83ee 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -737,6 +737,7 @@ struct drm_bus {
> int (*irq_by_busid)(struct drm_device *dev, struct drm_irq_busid *p);
> /* hooks that are for PCI */
> int (*agp_init)(struct drm_device *dev);
> + void (*agp_destroy)(struct drm_device *dev);
>
> };
>
> @@ -1454,6 +1455,8 @@ extern int drm_modeset_ctl(struct drm_device *dev, void *data,
>
> /* AGP/GART support (drm_agpsupport.h) */
> extern struct drm_agp_head *drm_agp_init(struct drm_device *dev);
> +extern void drm_agp_destroy(struct drm_agp_head *agp);
> +extern void drm_agp_clear(struct drm_device *dev);
> extern int drm_agp_acquire(struct drm_device *dev);
> extern int drm_agp_acquire_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
> --
> 1.8.3.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [RFC 3/9] drm/agp: fix AGP cleanup paths
2013-07-27 6:05 ` Dave Airlie
@ 2013-07-27 12:09 ` David Herrmann
0 siblings, 0 replies; 15+ messages in thread
From: David Herrmann @ 2013-07-27 12:09 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel@lists.freedesktop.org
Hi
On Sat, Jul 27, 2013 at 8:05 AM, Dave Airlie <airlied@gmail.com> wrote:
> On Wed, Jul 24, 2013 at 11:43 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> Introduce two new helpers, drm_agp_clear() and drm_agp_destroy() which
>> clear all AGP mappings and destroy the AGP head. This allows to reduce the
>> AGP code in core DRM and move it all to drm_agpsupport.c.
>>
>
> Could we do this as the first patch? it seems like it would be a nice
> thing to have standalone even with the current init paths.
I can do that. But note the error paths in all DRM bus drivers are
currently non-existent and I'd like to avoid adding them just to
remove them with drm_dev_free() again. And non-existent error-paths
means we don't catch AGP errors, except in drm_fill_in_dev().
But I guess the "->agp_destroy()" cleanup is still nicer than the
current approach. I will send the patch standalone as the rest of the
series needs more work, anyway.
Cheers
David
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC 4/9] drm: move drm_lastclose() to drm_fops.c
2013-07-24 13:43 [RFC 0/9] DRM: Device Handling Cleanup David Herrmann
` (2 preceding siblings ...)
2013-07-24 13:43 ` [RFC 3/9] drm/agp: fix AGP cleanup paths David Herrmann
@ 2013-07-24 13:43 ` David Herrmann
2013-07-24 13:43 ` [RFC 5/9] drm: introduce drm_dev_free() to fix error paths David Herrmann
` (4 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: David Herrmann @ 2013-07-24 13:43 UTC (permalink / raw)
To: dri-devel
Try to keep all functions that handle DRM file_operations into 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
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/gpu/drm/drm_drv.c | 49 ----------------------------------------------
drivers/gpu/drm/drm_fops.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 49 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index dddd799..c294e89 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -171,55 +171,6 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
#define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
-/**
- * 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);
-
- if (drm_core_check_feature(dev, DRIVER_SG) && dev->sg &&
- !drm_core_check_feature(dev, DRIVER_MODESET)) {
- drm_sg_cleanup(dev->sg);
- dev->sg = NULL;
- }
-
- /* Clear vma list (only built for debugging) */
- list_for_each_entry_safe(vma, vma_temp, &dev->vmalist, head) {
- list_del(&vma->head);
- kfree(vma);
- }
-
- if (drm_core_check_feature(dev, DRIVER_HAVE_DMA) &&
- !drm_core_check_feature(dev, DRIVER_MODESET))
- drm_dma_takedown(dev);
-
- dev->dev_mapping = NULL;
- mutex_unlock(&dev->struct_mutex);
-
- 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 72acae9..f8a3ebc 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -427,6 +427,55 @@ static void drm_events_release(struct drm_file *file_priv)
}
/**
+ * 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);
+
+ if (drm_core_check_feature(dev, DRIVER_SG) && dev->sg &&
+ !drm_core_check_feature(dev, DRIVER_MODESET)) {
+ drm_sg_cleanup(dev->sg);
+ dev->sg = NULL;
+ }
+
+ /* Clear vma list (only built for debugging) */
+ list_for_each_entry_safe(vma, vma_temp, &dev->vmalist, head) {
+ list_del(&vma->head);
+ kfree(vma);
+ }
+
+ if (drm_core_check_feature(dev, DRIVER_HAVE_DMA) &&
+ !drm_core_check_feature(dev, DRIVER_MODESET))
+ drm_dma_takedown(dev);
+
+ dev->dev_mapping = NULL;
+ mutex_unlock(&dev->struct_mutex);
+
+ DRM_DEBUG("lastclose completed\n");
+ return 0;
+}
+
+/**
* Release file.
*
* \param inode device inode
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* [RFC 5/9] drm: introduce drm_dev_free() to fix error paths
2013-07-24 13:43 [RFC 0/9] DRM: Device Handling Cleanup David Herrmann
` (3 preceding siblings ...)
2013-07-24 13:43 ` [RFC 4/9] drm: move drm_lastclose() to drm_fops.c David Herrmann
@ 2013-07-24 13:43 ` David Herrmann
2013-07-24 13:43 ` [RFC 6/9] drm: move device unregistration into drm_dev_unregister() David Herrmann
` (3 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: David Herrmann @ 2013-07-24 13:43 UTC (permalink / raw)
To: dri-devel
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 0daee24..8ffc23d 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -356,7 +356,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 396713e..2b99ef0 100644
--- a/drivers/gpu/drm/drm_platform.c
+++ b/drivers/gpu/drm/drm_platform.c
@@ -64,7 +64,7 @@ int drm_get_platform_dev(struct platform_device *platdev,
return 0;
err_free:
- kfree(dev);
+ drm_dev_free(dev);
return ret;
}
EXPORT_SYMBOL(drm_get_platform_dev);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index f016ed8..8497dd1 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -374,7 +374,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");
@@ -383,7 +382,6 @@ void drm_put_dev(struct drm_device *dev)
DRM_ERROR("cleanup called no dev\n");
return;
}
- driver = dev->driver;
drm_lastclose(dev);
@@ -397,21 +395,15 @@ 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);
- 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);
@@ -508,6 +500,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 30f83ee..d75f061 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1707,6 +1707,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);
/*@}*/
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* [RFC 6/9] drm: move device unregistration into drm_dev_unregister()
2013-07-24 13:43 [RFC 0/9] DRM: Device Handling Cleanup David Herrmann
` (4 preceding siblings ...)
2013-07-24 13:43 ` [RFC 5/9] drm: introduce drm_dev_free() to fix error paths David Herrmann
@ 2013-07-24 13:43 ` David Herrmann
2013-07-24 13:43 ` [RFC 7/9] percpu_rw_sempahore: export symbols for modules David Herrmann
` (2 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: David Herrmann @ 2013-07-24 13:43 UTC (permalink / raw)
To: dri-devel
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 | 57 ++++++++++++++++++++++++++++------------------
include/drm/drmP.h | 1 +
2 files changed, 36 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 8497dd1..4ff1227 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -374,8 +374,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) {
@@ -383,26 +381,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);
-
- drm_put_minor(&dev->primary);
-
- list_del(&dev->driver_item);
-
+ drm_dev_unregister(dev);
drm_dev_free(dev);
}
EXPORT_SYMBOL(drm_put_dev);
@@ -592,3 +571,37 @@ 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 (drm_core_check_feature(dev, DRIVER_MODESET))
+ drm_put_minor(&dev->control);
+
+ 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 d75f061..381679e 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1709,6 +1709,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.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* [RFC 7/9] percpu_rw_sempahore: export symbols for modules
2013-07-24 13:43 [RFC 0/9] DRM: Device Handling Cleanup David Herrmann
` (5 preceding siblings ...)
2013-07-24 13:43 ` [RFC 6/9] drm: move device unregistration into drm_dev_unregister() David Herrmann
@ 2013-07-24 13:43 ` David Herrmann
2013-07-24 13:43 ` [RFC 8/9] drm: track pending user-space actions David Herrmann
2013-07-24 13:43 ` [RFC 9/9] drm: support immediate unplugging David Herrmann
8 siblings, 0 replies; 15+ messages in thread
From: David Herrmann @ 2013-07-24 13:43 UTC (permalink / raw)
To: dri-devel
DRM could make great use of percpu-rwsems to track active users and wait
for them during hw hotplugging. Export symbols and allow using them in
runtime loadable modules.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
lib/Makefile | 2 +-
lib/percpu-rwsem.c | 7 +++++++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/lib/Makefile b/lib/Makefile
index 7baccfd..f5b8669 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -43,7 +43,7 @@ obj-$(CONFIG_DEBUG_LOCKING_API_SELFTESTS) += locking-selftest.o
obj-$(CONFIG_DEBUG_SPINLOCK) += spinlock_debug.o
lib-$(CONFIG_RWSEM_GENERIC_SPINLOCK) += rwsem-spinlock.o
lib-$(CONFIG_RWSEM_XCHGADD_ALGORITHM) += rwsem.o
-lib-$(CONFIG_PERCPU_RWSEM) += percpu-rwsem.o
+obj-$(CONFIG_PERCPU_RWSEM) += percpu-rwsem.o
CFLAGS_hweight.o = $(subst $(quote),,$(CONFIG_ARCH_HWEIGHT_CFLAGS))
obj-$(CONFIG_GENERIC_HWEIGHT) += hweight.o
diff --git a/lib/percpu-rwsem.c b/lib/percpu-rwsem.c
index 652a8ee..893586c 100644
--- a/lib/percpu-rwsem.c
+++ b/lib/percpu-rwsem.c
@@ -7,6 +7,7 @@
#include <linux/rcupdate.h>
#include <linux/sched.h>
#include <linux/errno.h>
+#include <linux/export.h>
int __percpu_init_rwsem(struct percpu_rw_semaphore *brw,
const char *name, struct lock_class_key *rwsem_key)
@@ -22,12 +23,14 @@ int __percpu_init_rwsem(struct percpu_rw_semaphore *brw,
init_waitqueue_head(&brw->write_waitq);
return 0;
}
+EXPORT_SYMBOL(__percpu_init_rwsem);
void percpu_free_rwsem(struct percpu_rw_semaphore *brw)
{
free_percpu(brw->fast_read_ctr);
brw->fast_read_ctr = NULL; /* catch use after free bugs */
}
+EXPORT_SYMBOL(percpu_free_rwsem);
/*
* This is the fast-path for down_read/up_read, it only needs to ensure
@@ -87,6 +90,7 @@ void percpu_down_read(struct percpu_rw_semaphore *brw)
/* avoid up_read()->rwsem_release() */
__up_read(&brw->rw_sem);
}
+EXPORT_SYMBOL(percpu_down_read);
void percpu_up_read(struct percpu_rw_semaphore *brw)
{
@@ -99,6 +103,7 @@ void percpu_up_read(struct percpu_rw_semaphore *brw)
if (atomic_dec_and_test(&brw->slow_read_ctr))
wake_up_all(&brw->write_waitq);
}
+EXPORT_SYMBOL(percpu_up_read);
static int clear_fast_ctr(struct percpu_rw_semaphore *brw)
{
@@ -150,6 +155,7 @@ void percpu_down_write(struct percpu_rw_semaphore *brw)
/* wait for all readers to complete their percpu_up_read() */
wait_event(brw->write_waitq, !atomic_read(&brw->slow_read_ctr));
}
+EXPORT_SYMBOL(percpu_down_write);
void percpu_up_write(struct percpu_rw_semaphore *brw)
{
@@ -163,3 +169,4 @@ void percpu_up_write(struct percpu_rw_semaphore *brw)
/* the last writer unblocks update_fast_ctr() */
atomic_dec(&brw->write_ctr);
}
+EXPORT_SYMBOL(percpu_up_write);
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* [RFC 8/9] drm: track pending user-space actions
2013-07-24 13:43 [RFC 0/9] DRM: Device Handling Cleanup David Herrmann
` (6 preceding siblings ...)
2013-07-24 13:43 ` [RFC 7/9] percpu_rw_sempahore: export symbols for modules David Herrmann
@ 2013-07-24 13:43 ` David Herrmann
2013-07-24 14:03 ` Maarten Lankhorst
2013-07-24 13:43 ` [RFC 9/9] drm: support immediate unplugging David Herrmann
8 siblings, 1 reply; 15+ messages in thread
From: David Herrmann @ 2013-07-24 13:43 UTC (permalink / raw)
To: dri-devel
If we want to support real hotplugging, we need to block new syscalls from
entering any drm_* fops. We also need to wait for these to finish before
unregistering a device.
This patch introduces drm_dev_get/put_active() which mark a device as
active during file-ops. If a device is unplugged, drm_dev_get_active()
will fail and prevent users from using this device.
Internally, we use rwsems to implement this. It allows simultaneous users
(down_read) and we can block on them (down_write) to wait until they are
done. This way, a drm_dev_unregister() can be called at any time and does
not have to wait for the last drm_release() call.
Note that the current "atomic_t unplugged" approach is not safe. Imagine
an unplugged device but a user-space context which already is beyong the
"drm_device_is_unplugged()" check. We have no way to prevent any following
mmap operation or buffer access. The percpu-rwsem avoids this by
protecting a whole file-op call and waiting with unplugging a device until
all pending calls are finished.
FIXME: We still need to update all the driver's fops in case they don't
use the DRM core stubs. A quick look showed only custom mmap callbacks.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/gpu/drm/Kconfig | 1 +
drivers/gpu/drm/drm_drv.c | 6 +++--
drivers/gpu/drm/drm_fops.c | 31 +++++++++++++++++-----
drivers/gpu/drm/drm_stub.c | 64 +++++++++++++++++++++++++++++++++++++++++++---
include/drm/drmP.h | 6 +++++
5 files changed, 96 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index a7c54c8..e2a399e 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -11,6 +11,7 @@ menuconfig DRM
select I2C
select I2C_ALGOBIT
select DMA_SHARED_BUFFER
+ select PERCPU_RWSEM
help
Kernel-level support for the Direct Rendering Infrastructure (DRI)
introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index c294e89..db5e57b 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -318,7 +318,7 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
- if (drm_device_is_unplugged(dev))
+ if (!drm_dev_get_active(dev))
return -ENODEV;
atomic_inc(&dev->ioctl_count);
@@ -412,7 +412,9 @@ long drm_ioctl(struct file *filp,
if (kdata != stack_kdata)
kfree(kdata);
- atomic_dec(&dev->ioctl_count);
+
+ drm_dev_put_active(dev);
+
if (retcode)
DRM_DEBUG("ret = %d\n", retcode);
return retcode;
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index f8a3ebc..b75af7d 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -122,7 +122,7 @@ int drm_open(struct inode *inode, struct file *filp)
if (!(dev = minor->dev))
return -ENODEV;
- if (drm_device_is_unplugged(dev))
+ if (!drm_dev_get_active(dev))
return -ENODEV;
if (!dev->open_count++)
@@ -147,7 +147,9 @@ int drm_open(struct inode *inode, struct file *filp)
if (retcode)
goto err_undo;
}
- return 0;
+
+ retcode = 0;
+ goto out_active;
err_undo:
mutex_lock(&dev->struct_mutex);
@@ -157,6 +159,8 @@ err_undo:
dev->dev_mapping = old_mapping;
mutex_unlock(&dev->struct_mutex);
dev->open_count--;
+out_active:
+ drm_dev_put_active(dev);
return retcode;
}
EXPORT_SYMBOL(drm_open);
@@ -188,9 +192,6 @@ int drm_stub_open(struct inode *inode, struct file *filp)
if (!(dev = minor->dev))
goto out;
- if (drm_device_is_unplugged(dev))
- goto out;
-
old_fops = filp->f_op;
filp->f_op = fops_get(dev->driver->fops);
if (filp->f_op == NULL) {
@@ -654,14 +655,24 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
size_t count, loff_t *offset)
{
struct drm_file *file_priv = filp->private_data;
+ struct drm_device *dev = file_priv->minor->dev;
struct drm_pending_event *e;
size_t total;
ssize_t ret;
+ /* No locking needed around "unplugged" as we sleep during wait_event()
+ * below, anyway. We acquire the active device after we woke up so we
+ * never use unplugged devices. */
+ if (dev->unplugged)
+ return -ENODEV;
+
ret = wait_event_interruptible(file_priv->event_wait,
- !list_empty(&file_priv->event_list));
+ !list_empty(&file_priv->event_list) ||
+ dev->unplugged);
if (ret < 0)
return ret;
+ if (!drm_dev_get_active(dev))
+ return -ENODEV;
total = 0;
while (drm_dequeue_event(file_priv, total, count, &e)) {
@@ -675,6 +686,7 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
e->destroy(e);
}
+ drm_dev_put_active(dev);
return total;
}
EXPORT_SYMBOL(drm_read);
@@ -682,13 +694,20 @@ EXPORT_SYMBOL(drm_read);
unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait)
{
struct drm_file *file_priv = filp->private_data;
+ struct drm_device *dev = file_priv->minor->dev;
unsigned int mask = 0;
+ /* signal HUP/IN on device removal */
+ if (!drm_dev_get_active(dev))
+ return POLLHUP | POLLIN | POLLRDNORM;
+
poll_wait(filp, &file_priv->event_wait, wait);
if (!list_empty(&file_priv->event_list))
mask |= POLLIN | POLLRDNORM;
+ drm_dev_put_active(dev);
+
return mask;
}
EXPORT_SYMBOL(drm_poll);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 4ff1227..c0e76c0 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -449,9 +449,12 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
dev->types[4] = _DRM_STAT_LOCKS;
dev->types[5] = _DRM_STAT_UNLOCKS;
- if (drm_ht_create(&dev->map_hash, 12))
+ if (percpu_init_rwsem(&dev->active))
goto err_free;
+ if (drm_ht_create(&dev->map_hash, 12))
+ goto err_rwsem;
+
ret = drm_ctxbitmap_init(dev);
if (ret) {
DRM_ERROR("Cannot allocate memory for context bitmap.\n");
@@ -472,6 +475,8 @@ err_ctxbitmap:
drm_ctxbitmap_cleanup(dev);
err_map_hash:
drm_ht_remove(&dev->map_hash);
+err_rwsem:
+ percpu_free_rwsem(&dev->active);
err_free:
kfree(dev);
return NULL;
@@ -496,6 +501,7 @@ void drm_dev_free(struct drm_device *dev)
drm_ctxbitmap_cleanup(dev);
drm_ht_remove(&dev->map_hash);
+ percpu_free_rwsem(&dev->active);
kfree(dev->devname);
kfree(dev);
}
@@ -576,14 +582,21 @@ 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.
+ * Mark DRM device as unplugged, wait for any pending user request and then
+ * unregister the DRM device from the system. This does the reverse of
+ * drm_dev_register().
*/
void drm_dev_unregister(struct drm_device *dev)
{
struct drm_map_list *r_list, *list_temp;
+ /* Wait for pending users and then mark the device as unplugged. We
+ * must not hold the global-mutex while doing this. Otherwise, calls
+ * like drm_ioctl() or drm_lock() will dead-lock. */
+ percpu_down_write(&dev->active);
+ dev->unplugged = true;
+ percpu_up_write(&dev->active);
+
drm_lastclose(dev);
if (dev->driver->unload)
@@ -605,3 +618,46 @@ void drm_dev_unregister(struct drm_device *dev)
list_del(&dev->driver_item);
}
EXPORT_SYMBOL(drm_dev_unregister);
+
+/**
+ * drm_dev_get_active - Mark device as active
+ * @dev: Device to mark
+ *
+ * Whenever a DRM driver performs an action on behalf of user-space, it should
+ * mark the DRM device as active. Once it is done, call drm_dev_put_active() to
+ * release that mark. This allows DRM core to wait for pending user-space
+ * actions before unplugging a device. But this also means, user-space must
+ * not sleep for an indefinite period while a device is marked active.
+ * If you have to sleep for an indefinite period, call drm_dev_put_active() and
+ * try to reacquire the device once you wake up.
+ *
+ * Recursive calls are not allowed! They will dead-lock!
+ *
+ * RETURNS:
+ * True iff the device was marked active and can be used. False if the device
+ * was unplugged and must not be used.
+ */
+bool drm_dev_get_active(struct drm_device *dev)
+{
+ percpu_down_read(&dev->active);
+ if (!dev->unplugged)
+ return true;
+ percpu_up_read(&dev->active);
+ return false;
+}
+EXPORT_SYMBOL(drm_dev_get_active);
+
+/**
+ * drm_dev_put_active - Unmark active device
+ * @dev: Active device to unmark
+ *
+ * This finished a call to drm_dev_get_active(). You must not call it if
+ * drm_dev_get_active() failed.
+ * This marks the device as inactive again, iff no other user currently has the
+ * device marked as active. See drm_dev_get_active().
+ */
+void drm_dev_put_active(struct drm_device *dev)
+{
+ percpu_up_read(&dev->active);
+}
+EXPORT_SYMBOL(drm_dev_put_active);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 381679e..9689173 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1209,7 +1209,11 @@ struct drm_device {
/*@} */
int switch_power_state;
+ /** \name Hotplug Management */
+ /*@{ */
+ struct percpu_rw_semaphore active; /**< protect active users */
atomic_t unplugged; /* device has been unplugged or gone away */
+ /*@} */
};
#define DRM_SWITCH_POWER_ON 0
@@ -1710,6 +1714,8 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
void drm_dev_free(struct drm_device *dev);
int drm_dev_register(struct drm_device *dev);
void drm_dev_unregister(struct drm_device *dev);
+bool drm_dev_get_active(struct drm_device *dev);
+void drm_dev_put_active(struct drm_device *dev);
int drm_get_minor(struct drm_device *dev, struct drm_minor **minor, int type);
/*@}*/
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [RFC 8/9] drm: track pending user-space actions
2013-07-24 13:43 ` [RFC 8/9] drm: track pending user-space actions David Herrmann
@ 2013-07-24 14:03 ` Maarten Lankhorst
2013-07-24 14:24 ` David Herrmann
0 siblings, 1 reply; 15+ messages in thread
From: Maarten Lankhorst @ 2013-07-24 14:03 UTC (permalink / raw)
To: David Herrmann; +Cc: dri-devel
Op 24-07-13 15:43, David Herrmann schreef:
> If we want to support real hotplugging, we need to block new syscalls from
> entering any drm_* fops. We also need to wait for these to finish before
> unregistering a device.
>
> This patch introduces drm_dev_get/put_active() which mark a device as
> active during file-ops. If a device is unplugged, drm_dev_get_active()
> will fail and prevent users from using this device.
>
> Internally, we use rwsems to implement this. It allows simultaneous users
> (down_read) and we can block on them (down_write) to wait until they are
> done. This way, a drm_dev_unregister() can be called at any time and does
> not have to wait for the last drm_release() call.
>
> Note that the current "atomic_t unplugged" approach is not safe. Imagine
> an unplugged device but a user-space context which already is beyong the
> "drm_device_is_unplugged()" check. We have no way to prevent any following
> mmap operation or buffer access. The percpu-rwsem avoids this by
> protecting a whole file-op call and waiting with unplugging a device until
> all pending calls are finished.
>
> FIXME: We still need to update all the driver's fops in case they don't
> use the DRM core stubs. A quick look showed only custom mmap callbacks.
Meh, I don't think it's possible to make the mmaps completely race free.
I 'solved' this by taking mapping->i_mmap_mutex, it reduces the window,
because all the mmap callbacks are called while holding that lock, so at least
new mappings from splitting mappings up cannot happen.
Why did you make the percpu rwsem local to the device?
It's only going to be taking in write mode during device destruction, which
isn't exactly fast anyway. A single global rwsem wouldn't have any negative effect.
My original patch was locking mapping->i_mmap_mutex, then the rwsem in write mode, then the
global mutex lock during unplug, to ensure that all code saw the unplugged correctly.
That way using any of those 3 locks would be sufficient to check dev->unplugged safely.
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> drivers/gpu/drm/Kconfig | 1 +
> drivers/gpu/drm/drm_drv.c | 6 +++--
> drivers/gpu/drm/drm_fops.c | 31 +++++++++++++++++-----
> drivers/gpu/drm/drm_stub.c | 64 +++++++++++++++++++++++++++++++++++++++++++---
> include/drm/drmP.h | 6 +++++
> 5 files changed, 96 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index a7c54c8..e2a399e 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -11,6 +11,7 @@ menuconfig DRM
> select I2C
> select I2C_ALGOBIT
> select DMA_SHARED_BUFFER
> + select PERCPU_RWSEM
> help
> Kernel-level support for the Direct Rendering Infrastructure (DRI)
> introduced in XFree86 4.0. If you say Y here, you need to select
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index c294e89..db5e57b 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -318,7 +318,7 @@ long drm_ioctl(struct file *filp,
>
> dev = file_priv->minor->dev;
>
> - if (drm_device_is_unplugged(dev))
> + if (!drm_dev_get_active(dev))
> return -ENODEV;
>
> atomic_inc(&dev->ioctl_count);
> @@ -412,7 +412,9 @@ long drm_ioctl(struct file *filp,
>
> if (kdata != stack_kdata)
> kfree(kdata);
> - atomic_dec(&dev->ioctl_count);
> +
> + drm_dev_put_active(dev);
> +
> if (retcode)
> DRM_DEBUG("ret = %d\n", retcode);
> return retcode;
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index f8a3ebc..b75af7d 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -122,7 +122,7 @@ int drm_open(struct inode *inode, struct file *filp)
> if (!(dev = minor->dev))
> return -ENODEV;
>
> - if (drm_device_is_unplugged(dev))
> + if (!drm_dev_get_active(dev))
> return -ENODEV;
>
> if (!dev->open_count++)
> @@ -147,7 +147,9 @@ int drm_open(struct inode *inode, struct file *filp)
> if (retcode)
> goto err_undo;
> }
> - return 0;
> +
> + retcode = 0;
> + goto out_active;
>
> err_undo:
> mutex_lock(&dev->struct_mutex);
> @@ -157,6 +159,8 @@ err_undo:
> dev->dev_mapping = old_mapping;
> mutex_unlock(&dev->struct_mutex);
> dev->open_count--;
> +out_active:
> + drm_dev_put_active(dev);
> return retcode;
> }
> EXPORT_SYMBOL(drm_open);
> @@ -188,9 +192,6 @@ int drm_stub_open(struct inode *inode, struct file *filp)
> if (!(dev = minor->dev))
> goto out;
>
> - if (drm_device_is_unplugged(dev))
> - goto out;
> -
> old_fops = filp->f_op;
> filp->f_op = fops_get(dev->driver->fops);
> if (filp->f_op == NULL) {
> @@ -654,14 +655,24 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
> size_t count, loff_t *offset)
> {
> struct drm_file *file_priv = filp->private_data;
> + struct drm_device *dev = file_priv->minor->dev;
> struct drm_pending_event *e;
> size_t total;
> ssize_t ret;
>
> + /* No locking needed around "unplugged" as we sleep during wait_event()
> + * below, anyway. We acquire the active device after we woke up so we
> + * never use unplugged devices. */
> + if (dev->unplugged)
> + return -ENODEV;
> +
> ret = wait_event_interruptible(file_priv->event_wait,
> - !list_empty(&file_priv->event_list));
> + !list_empty(&file_priv->event_list) ||
> + dev->unplugged);
> if (ret < 0)
> return ret;
> + if (!drm_dev_get_active(dev))
> + return -ENODEV;
>
> total = 0;
> while (drm_dequeue_event(file_priv, total, count, &e)) {
> @@ -675,6 +686,7 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
> e->destroy(e);
> }
>
> + drm_dev_put_active(dev);
> return total;
> }
> EXPORT_SYMBOL(drm_read);
> @@ -682,13 +694,20 @@ EXPORT_SYMBOL(drm_read);
> unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait)
> {
> struct drm_file *file_priv = filp->private_data;
> + struct drm_device *dev = file_priv->minor->dev;
> unsigned int mask = 0;
>
> + /* signal HUP/IN on device removal */
> + if (!drm_dev_get_active(dev))
> + return POLLHUP | POLLIN | POLLRDNORM;
> +
> poll_wait(filp, &file_priv->event_wait, wait);
>
> if (!list_empty(&file_priv->event_list))
> mask |= POLLIN | POLLRDNORM;
>
> + drm_dev_put_active(dev);
> +
> return mask;
> }
> EXPORT_SYMBOL(drm_poll);
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index 4ff1227..c0e76c0 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -449,9 +449,12 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
> dev->types[4] = _DRM_STAT_LOCKS;
> dev->types[5] = _DRM_STAT_UNLOCKS;
>
> - if (drm_ht_create(&dev->map_hash, 12))
> + if (percpu_init_rwsem(&dev->active))
> goto err_free;
>
> + if (drm_ht_create(&dev->map_hash, 12))
> + goto err_rwsem;
> +
> ret = drm_ctxbitmap_init(dev);
> if (ret) {
> DRM_ERROR("Cannot allocate memory for context bitmap.\n");
> @@ -472,6 +475,8 @@ err_ctxbitmap:
> drm_ctxbitmap_cleanup(dev);
> err_map_hash:
> drm_ht_remove(&dev->map_hash);
> +err_rwsem:
> + percpu_free_rwsem(&dev->active);
> err_free:
> kfree(dev);
> return NULL;
> @@ -496,6 +501,7 @@ void drm_dev_free(struct drm_device *dev)
> drm_ctxbitmap_cleanup(dev);
> drm_ht_remove(&dev->map_hash);
>
> + percpu_free_rwsem(&dev->active);
> kfree(dev->devname);
> kfree(dev);
> }
> @@ -576,14 +582,21 @@ 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.
> + * Mark DRM device as unplugged, wait for any pending user request and then
> + * unregister the DRM device from the system. This does the reverse of
> + * drm_dev_register().
> */
> void drm_dev_unregister(struct drm_device *dev)
> {
> struct drm_map_list *r_list, *list_temp;
>
> + /* Wait for pending users and then mark the device as unplugged. We
> + * must not hold the global-mutex while doing this. Otherwise, calls
> + * like drm_ioctl() or drm_lock() will dead-lock. */
> + percpu_down_write(&dev->active);
> + dev->unplugged = true;
> + percpu_up_write(&dev->active);
> +
> drm_lastclose(dev);
>
> if (dev->driver->unload)
> @@ -605,3 +618,46 @@ void drm_dev_unregister(struct drm_device *dev)
> list_del(&dev->driver_item);
> }
> EXPORT_SYMBOL(drm_dev_unregister);
> +
> +/**
> + * drm_dev_get_active - Mark device as active
> + * @dev: Device to mark
> + *
> + * Whenever a DRM driver performs an action on behalf of user-space, it should
> + * mark the DRM device as active. Once it is done, call drm_dev_put_active() to
> + * release that mark. This allows DRM core to wait for pending user-space
> + * actions before unplugging a device. But this also means, user-space must
> + * not sleep for an indefinite period while a device is marked active.
> + * If you have to sleep for an indefinite period, call drm_dev_put_active() and
> + * try to reacquire the device once you wake up.
> + *
> + * Recursive calls are not allowed! They will dead-lock!
> + *
> + * RETURNS:
> + * True iff the device was marked active and can be used. False if the device
> + * was unplugged and must not be used.
> + */
> +bool drm_dev_get_active(struct drm_device *dev)
> +{
> + percpu_down_read(&dev->active);
> + if (!dev->unplugged)
> + return true;
> + percpu_up_read(&dev->active);
> + return false;
> +}
> +EXPORT_SYMBOL(drm_dev_get_active);
> +
> +/**
> + * drm_dev_put_active - Unmark active device
> + * @dev: Active device to unmark
> + *
> + * This finished a call to drm_dev_get_active(). You must not call it if
> + * drm_dev_get_active() failed.
> + * This marks the device as inactive again, iff no other user currently has the
> + * device marked as active. See drm_dev_get_active().
> + */
> +void drm_dev_put_active(struct drm_device *dev)
> +{
> + percpu_up_read(&dev->active);
> +}
> +EXPORT_SYMBOL(drm_dev_put_active);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 381679e..9689173 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1209,7 +1209,11 @@ struct drm_device {
> /*@} */
> int switch_power_state;
>
> + /** \name Hotplug Management */
> + /*@{ */
> + struct percpu_rw_semaphore active; /**< protect active users */
> atomic_t unplugged; /* device has been unplugged or gone away */
> + /*@} */
> };
>
> #define DRM_SWITCH_POWER_ON 0
> @@ -1710,6 +1714,8 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
> void drm_dev_free(struct drm_device *dev);
> int drm_dev_register(struct drm_device *dev);
> void drm_dev_unregister(struct drm_device *dev);
> +bool drm_dev_get_active(struct drm_device *dev);
> +void drm_dev_put_active(struct drm_device *dev);
> int drm_get_minor(struct drm_device *dev, struct drm_minor **minor, int type);
> /*@}*/
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [RFC 8/9] drm: track pending user-space actions
2013-07-24 14:03 ` Maarten Lankhorst
@ 2013-07-24 14:24 ` David Herrmann
2013-07-24 14:45 ` Maarten Lankhorst
0 siblings, 1 reply; 15+ messages in thread
From: David Herrmann @ 2013-07-24 14:24 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: dri-devel@lists.freedesktop.org
Hi
On Wed, Jul 24, 2013 at 4:03 PM, Maarten Lankhorst
<maarten.lankhorst@canonical.com> wrote:
> Op 24-07-13 15:43, David Herrmann schreef:
>> If we want to support real hotplugging, we need to block new syscalls from
>> entering any drm_* fops. We also need to wait for these to finish before
>> unregistering a device.
>>
>> This patch introduces drm_dev_get/put_active() which mark a device as
>> active during file-ops. If a device is unplugged, drm_dev_get_active()
>> will fail and prevent users from using this device.
>>
>> Internally, we use rwsems to implement this. It allows simultaneous users
>> (down_read) and we can block on them (down_write) to wait until they are
>> done. This way, a drm_dev_unregister() can be called at any time and does
>> not have to wait for the last drm_release() call.
>>
>> Note that the current "atomic_t unplugged" approach is not safe. Imagine
>> an unplugged device but a user-space context which already is beyong the
>> "drm_device_is_unplugged()" check. We have no way to prevent any following
>> mmap operation or buffer access. The percpu-rwsem avoids this by
>> protecting a whole file-op call and waiting with unplugging a device until
>> all pending calls are finished.
>>
>> FIXME: We still need to update all the driver's fops in case they don't
>> use the DRM core stubs. A quick look showed only custom mmap callbacks.
> Meh, I don't think it's possible to make the mmaps completely race free.
It is. See this scenario:
drm_dev_unregister() sets "plugged = true;" in write_lock(). This
guarantees, there is currently no other pending user in any file-ops
or mmap-fault handler (assuming the mmap fault handler is wrapped in
drm_dev_get_active(), drm_dev_put_active()).
After that, I call unmap_mapping_range() which resets all DRM-mapped
PTEs of user-space processes. So if a mmap is accessed afterwards, it
will trap and the fault() handler will fail with SIGBUS because
drm_dev_get_active() will return false. New mmaps cannot be created,
because drm_mmap() and alike will also be protected by
drm_dev_get_active().
I don't see any race here, do you?
> I 'solved' this by taking mapping->i_mmap_mutex, it reduces the window,
> because all the mmap callbacks are called while holding that lock, so at least
> new mappings from splitting mappings up cannot happen.
Is this somehow faster/better than wrapping fault callbacks in
get_active/put_active?
> Why did you make the percpu rwsem local to the device?
> It's only going to be taking in write mode during device destruction, which
> isn't exactly fast anyway. A single global rwsem wouldn't have any negative effect.
I have 4 UDL cards at home and I dislike that all cards are blocked
for a quite long time if I unplug just one card. As long as we have
drm_global_mutex with this broad use, we cannot fix it. But I thought,
I'd avoid introducing more global locks so maybe I can some day
improve that, too.
So I disagree, once we reduce drm_global_mutex, a single global rwsem
_will_ have a negative effect. Besides, if you have only 1 GPU, it
doesn't matter as you still only get a single rwsem.
>
> My original patch was locking mapping->i_mmap_mutex, then the rwsem in write mode, then the
> global mutex lock during unplug, to ensure that all code saw the unplugged correctly.
>
> That way using any of those 3 locks would be sufficient to check dev->unplugged safely.
If we want to fix the races completely, I don't think using
i_mmap_mutex helps us.
Regarding drm_global_mutex, as I said, my intention is to reduce this
lock to a minimum. The only place where we need it is minor-allocation
and driver-device-lists (ignoring the legacy users like drm-lock). For
both, we could easily use spinlocks. So it's the same reason as above:
I'd like to avoid contention on drm_global_mutex.
Besides, I dislike broad locks. It's not like we safe a lot of memory
here if we have one spinlock per driver instead of a global drm mutex.
And if we have small-ranging locks, it is much easier to review them
(at least to me).
Cheers
David
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC 8/9] drm: track pending user-space actions
2013-07-24 14:24 ` David Herrmann
@ 2013-07-24 14:45 ` Maarten Lankhorst
0 siblings, 0 replies; 15+ messages in thread
From: Maarten Lankhorst @ 2013-07-24 14:45 UTC (permalink / raw)
To: David Herrmann; +Cc: dri-devel@lists.freedesktop.org
Op 24-07-13 16:24, David Herrmann schreef:
> Hi
>
> On Wed, Jul 24, 2013 at 4:03 PM, Maarten Lankhorst
> <maarten.lankhorst@canonical.com> wrote:
>> Op 24-07-13 15:43, David Herrmann schreef:
>>> If we want to support real hotplugging, we need to block new syscalls from
>>> entering any drm_* fops. We also need to wait for these to finish before
>>> unregistering a device.
>>>
>>> This patch introduces drm_dev_get/put_active() which mark a device as
>>> active during file-ops. If a device is unplugged, drm_dev_get_active()
>>> will fail and prevent users from using this device.
>>>
>>> Internally, we use rwsems to implement this. It allows simultaneous users
>>> (down_read) and we can block on them (down_write) to wait until they are
>>> done. This way, a drm_dev_unregister() can be called at any time and does
>>> not have to wait for the last drm_release() call.
>>>
>>> Note that the current "atomic_t unplugged" approach is not safe. Imagine
>>> an unplugged device but a user-space context which already is beyong the
>>> "drm_device_is_unplugged()" check. We have no way to prevent any following
>>> mmap operation or buffer access. The percpu-rwsem avoids this by
>>> protecting a whole file-op call and waiting with unplugging a device until
>>> all pending calls are finished.
>>>
>>> FIXME: We still need to update all the driver's fops in case they don't
>>> use the DRM core stubs. A quick look showed only custom mmap callbacks.
>> Meh, I don't think it's possible to make the mmaps completely race free.
> It is. See this scenario:
>
> drm_dev_unregister() sets "plugged = true;" in write_lock(). This
> guarantees, there is currently no other pending user in any file-ops
> or mmap-fault handler (assuming the mmap fault handler is wrapped in
> drm_dev_get_active(), drm_dev_put_active()).
>
> After that, I call unmap_mapping_range() which resets all DRM-mapped
> PTEs of user-space processes. So if a mmap is accessed afterwards, it
> will trap and the fault() handler will fail with SIGBUS because
> drm_dev_get_active() will return false. New mmaps cannot be created,
> because drm_mmap() and alike will also be protected by
> drm_dev_get_active().
>
> I don't see any race here, do you?
>
>> I 'solved' this by taking mapping->i_mmap_mutex, it reduces the window,
>> because all the mmap callbacks are called while holding that lock, so at least
>> new mappings from splitting mappings up cannot happen.
> Is this somehow faster/better than wrapping fault callbacks in
> get_active/put_active?
Did you test the last patch with PROVE_LOCKING?
>> Why did you make the percpu rwsem local to the device?
>> It's only going to be taking in write mode during device destruction, which
>> isn't exactly fast anyway. A single global rwsem wouldn't have any negative effect.
> I have 4 UDL cards at home and I dislike that all cards are blocked
> for a quite long time if I unplug just one card. As long as we have
> drm_global_mutex with this broad use, we cannot fix it. But I thought,
> I'd avoid introducing more global locks so maybe I can some day
> improve that, too.
>
> So I disagree, once we reduce drm_global_mutex, a single global rwsem
> _will_ have a negative effect. Besides, if you have only 1 GPU, it
> doesn't matter as you still only get a single rwsem.
>
>> My original patch was locking mapping->i_mmap_mutex, then the rwsem in write mode, then the
>> global mutex lock during unplug, to ensure that all code saw the unplugged correctly.
>>
>> That way using any of those 3 locks would be sufficient to check dev->unplugged safely.
> If we want to fix the races completely, I don't think using
> i_mmap_mutex helps us.
>
> Regarding drm_global_mutex, as I said, my intention is to reduce this
> lock to a minimum. The only place where we need it is minor-allocation
> and driver-device-lists (ignoring the legacy users like drm-lock). For
> both, we could easily use spinlocks. So it's the same reason as above:
> I'd like to avoid contention on drm_global_mutex.
> Besides, I dislike broad locks. It's not like we safe a lot of memory
> here if we have one spinlock per driver instead of a global drm mutex.
> And if we have small-ranging locks, it is much easier to review them
> (at least to me).
>
> Cheers
> David
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC 9/9] drm: support immediate unplugging
2013-07-24 13:43 [RFC 0/9] DRM: Device Handling Cleanup David Herrmann
` (7 preceding siblings ...)
2013-07-24 13:43 ` [RFC 8/9] drm: track pending user-space actions David Herrmann
@ 2013-07-24 13:43 ` David Herrmann
8 siblings, 0 replies; 15+ messages in thread
From: David Herrmann @ 2013-07-24 13:43 UTC (permalink / raw)
To: dri-devel
Our current unplug-helpers remove all nodes from user-space and mark the
device as unplugged. However, any pending user-space call is still
continued. We wait for the last close() call to actually unload the
device.
This patch uses the drm_dev_get_active() infrastructure to allow immediate
unplugging. Whenever drm_put_dev() or drm_unplug_dev() is called, we now
call drm_dev_unregister(). This waits for outstanding user-space calls,
marks the device as unplugged, "fake"-closes all open files, zaps mmaps
and unregisters the device.
If there are pending open-files, we will mark them as invalid but keep
them around. The drm_release() callback will notice that and free the
device when the last open-file is closed.
So we still keep the device allocated as we did before, but we no longer
keep it registered. This is analogous to driver-core which also keeps
devices around (until last put_device() call), but allows immediate device
deregistration via unregister_device().
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
Documentation/DocBook/drm.tmpl | 5 ++
drivers/gpu/drm/drm_fops.c | 85 +++++++++++++++++---------
drivers/gpu/drm/drm_stub.c | 133 ++++++++++++++++++++++++++++++-----------
include/drm/drmP.h | 13 +---
4 files changed, 162 insertions(+), 74 deletions(-)
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 7d1278e..f668f0f 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -439,6 +439,11 @@ char *date;</synopsis>
</para>
</sect3>
</sect2>
+ <sect2>
+ <title>DRM Device Management</title>
+!Pdrivers/gpu/drm/drm_stub.c device management
+!Edrivers/gpu/drm/drm_stub.c
+ </sect2>
</sect1>
<!-- Internals: memory management -->
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index b75af7d..d6af563 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -477,34 +477,30 @@ int drm_lastclose(struct drm_device * dev)
}
/**
- * Release file.
+ * drm_close() - Close file
+ * @filp: Open file to close
*
- * \param inode device inode
- * \param file_priv DRM file private.
- * \return zero on success or a negative number on failure.
+ * Close all open handles and clean up all resources associated with this open
+ * file. The open-file must not be used after this call returns.
+ *
+ * This does not actually free "file_priv" or "file_priv->minor" so following
+ * user-space entries can still test for file_priv->minor->dev and see whether
+ * it is valid.
+ *
+ * You should free "file_priv" in the real file ->release() callback.
*
- * If the hardware lock is held then free it, and take it again for the kernel
- * context since it's necessary to reclaim buffers. Unlink the file private
- * data from its list and free it. Decreases the open count and if it reaches
- * zero calls drm_lastclose().
+ * Caller must hold the global DRM mutex.
*/
-int drm_release(struct inode *inode, struct file *filp)
+void drm_close(struct file *filp)
{
struct drm_file *file_priv = filp->private_data;
struct drm_device *dev = file_priv->minor->dev;
- int retcode = 0;
-
- mutex_lock(&drm_global_mutex);
DRM_DEBUG("open_count = %d\n", dev->open_count);
if (dev->driver->preclose)
dev->driver->preclose(dev, file_priv);
- /* ========================================================
- * Begin inline drm_release
- */
-
DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n",
task_pid_nr(current),
(long)old_encode_dev(file_priv->minor->device),
@@ -599,26 +595,57 @@ int drm_release(struct inode *inode, struct file *filp)
drm_prime_destroy_file_private(&file_priv->prime);
put_pid(file_priv->pid);
- kfree(file_priv);
+}
+EXPORT_SYMBOL(drm_close);
- /* ========================================================
- * End inline drm_release
- */
+/**
+ * drm_release - Release file
+ * @inode: Inode of DRM device node
+ * @filp: Open file to close
+ *
+ * Release callback which should be used for DRM device file-ops. Calls
+ * drm_close() for @filp and releases the DRM device if is is unplugged and we
+ * are the last user.
+ *
+ * RETURNS:
+ * This always returns 0.
+ */
+int drm_release(struct inode *inode, struct file *filp)
+{
+ struct drm_file *file_priv = filp->private_data;
+ struct drm_device *dev = file_priv->minor->dev;
+ bool active;
+
+ mutex_lock(&drm_global_mutex);
+ /* If the device is still active, our context is valid and we need to
+ * close it properly. If it is not active, drm_dev_unregister() will
+ * have called drm_close() for us already (protected by
+ * drm_global_mutex). So skip it in this case. */
+ active = drm_dev_get_active(dev);
+
+ if (active)
+ drm_close(filp);
+ kfree(file_priv);
+
+ /* If we are the last open-file and the device is still active,
+ * call lastclose() and continue. If the device is unplugged,
+ * then drm_dev_unregister() already called lastclose() and we
+ * can finally free the device as we are the last user. */
atomic_inc(&dev->counts[_DRM_STAT_CLOSES]);
if (!--dev->open_count) {
- if (atomic_read(&dev->ioctl_count)) {
- DRM_ERROR("Device busy: %d\n",
- atomic_read(&dev->ioctl_count));
- retcode = -EBUSY;
- } else
- retcode = drm_lastclose(dev);
- if (drm_device_is_unplugged(dev))
- drm_put_dev(dev);
+ if (active)
+ drm_lastclose(dev);
+ else
+ drm_dev_free(dev);
}
+
+ if (active)
+ drm_dev_put_active(dev);
+
mutex_unlock(&drm_global_mutex);
- return retcode;
+ return 0;
}
EXPORT_SYMBOL(drm_release);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index c0e76c0..85a0292 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -1,13 +1,4 @@
-/**
- * \file drm_stub.h
- * Stub support
- *
- * \author Rickard E. (Rik) Faith <faith@valinux.com>
- */
-
/*
- * Created: Fri Jan 19 10:48:35 2001 by faith@acm.org
- *
* Copyright 2001 VA Linux Systems, Inc., Sunnyvale, California.
* All Rights Reserved.
*
@@ -29,6 +20,45 @@
* OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
* ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
* DEALINGS IN THE SOFTWARE.
+ *
+ * Authors:
+ * Rickard E. (Rik) Faith <faith@valinux.com>
+ */
+
+/**
+ * DOC: device management
+ *
+ * DRM core provides basic device management and defines the lifetime. A bus
+ * driver is responsible of finding suitable devices and reacting to hotplug
+ * events. The normal device-core operations are available for DRM devices:
+ * drm_dev_alloc() allocates a new device which can be freed via drm_dev_free().
+ * This device is not advertised to user-space and is not considered active
+ * until you call drm_dev_register(). This call will start the DRM device and
+ * notify user-space about it. Once a device is registered, any device callbacks
+ * are considered active and may be entered.
+ *
+ * For platform drivers, this is all you need to do. For hotpluggable drivers, a
+ * bus needs to listen for unplug events. Once a device is unplugged, use
+ * drm_dev_unregister() to deactivate the device, interrupt all pending
+ * user-space processes waiting for the device and mark the device as gone. Once
+ * the device is unregistered, DRM core will take care of freeing it so you must
+ * not access it afterwards.
+ *
+ * DRM core tracks device usage via drm_dev_get_active() and
+ * drm_dev_put_active(). Every user-space entry point must mark a device as
+ * active as long as it is used. The DRM-core helpers do this automatically, but
+ * if a driver provides their own file-ops, they must take care of this. DRM
+ * core guarantees that drm_dev_unregister() will not be called as long as the
+ * device is active. But if you don't mark the device as active, it may get
+ * unregistered (and even freed!) at any time.
+ *
+ * Drivers must use the ->load() and ->unload() callbacks to allocate/free
+ * private device resources. DRM core does not provide device ref-counting as
+ * this isn't needed for now. Instead, drivers must assume a device is gone once
+ * ->unload() was called. Internally, a device is kept alive until
+ * drm_dev_unregister() is called. It is kept allocated until
+ * drm_dev_unregister() is called _and_ the last user-space process closed the
+ * last node of the device.
*/
#include <linux/module.h>
@@ -342,6 +372,9 @@ int drm_put_minor(struct drm_minor **minor_p)
{
struct drm_minor *minor = *minor_p;
+ if (!minor)
+ return 0;
+
DRM_DEBUG("release secondary minor %d\n", minor->index);
if (minor->type == DRM_MINOR_LEGACY)
@@ -362,7 +395,8 @@ EXPORT_SYMBOL(drm_put_minor);
static void drm_unplug_minor(struct drm_minor *minor)
{
- drm_sysfs_device_remove(minor);
+ if (minor)
+ drm_sysfs_device_remove(minor);
}
/**
@@ -374,33 +408,20 @@ static void drm_unplug_minor(struct drm_minor *minor)
*/
void drm_put_dev(struct drm_device *dev)
{
- DRM_DEBUG("\n");
-
- if (!dev) {
- DRM_ERROR("cleanup called no dev\n");
- return;
- }
-
drm_dev_unregister(dev);
- drm_dev_free(dev);
}
EXPORT_SYMBOL(drm_put_dev);
+/**
+ * drm_unplug_dev() - Unplug DRM device
+ * @dev: Device to unplug
+ *
+ * Virtually unplug DRM device, mark it as invalid and wait for any pending
+ * user-space actions. @dev might be freed after this returns.
+ */
void drm_unplug_dev(struct drm_device *dev)
{
- /* for a USB device */
- if (drm_core_check_feature(dev, DRIVER_MODESET))
- drm_unplug_minor(dev->control);
- drm_unplug_minor(dev->primary);
-
- mutex_lock(&drm_global_mutex);
-
- drm_device_set_unplugged(dev);
-
- if (dev->open_count == 0) {
- drm_put_dev(dev);
- }
- mutex_unlock(&drm_global_mutex);
+ drm_dev_unregister(dev);
}
EXPORT_SYMBOL(drm_unplug_dev);
@@ -495,6 +516,9 @@ EXPORT_SYMBOL(drm_dev_alloc);
*/
void drm_dev_free(struct drm_device *dev)
{
+ drm_put_minor(&dev->control);
+ drm_put_minor(&dev->primary);
+
if (dev->driver->driver_features & DRIVER_GEM)
drm_gem_destroy(dev);
@@ -585,10 +609,21 @@ EXPORT_SYMBOL(drm_dev_register);
* Mark DRM device as unplugged, wait for any pending user request and then
* unregister the DRM device from the system. This does the reverse of
* drm_dev_register().
+ *
+ * If there is no pending open-file, this also frees the DRM device by calling
+ * drm_dev_free(). Otherwise, it leaves the device allocated and the last real
+ * ->release() callback will free the device.
*/
void drm_dev_unregister(struct drm_device *dev)
{
struct drm_map_list *r_list, *list_temp;
+ struct drm_file *file;
+
+ /* Take fake open_count to prevent concurrent drm_release() calls from
+ * freeing the device. */
+ mutex_lock(&drm_global_mutex);
+ ++dev->open_count;
+ mutex_unlock(&drm_global_mutex);
/* Wait for pending users and then mark the device as unplugged. We
* must not hold the global-mutex while doing this. Otherwise, calls
@@ -597,6 +632,26 @@ void drm_dev_unregister(struct drm_device *dev)
dev->unplugged = true;
percpu_up_write(&dev->active);
+ mutex_lock(&drm_global_mutex);
+
+ /* By setting "unplugged" we guarantee that no new drm_open will
+ * succeed. We also know, that no user-space process can call into a DRM
+ * device, anymore. So instead of waiting for the last close() we
+ * simulate close() for all active users.
+ * This allows us to unregister the device immediately but wait for the
+ * last real release to free it actually. */
+ while (!list_empty(&dev->filelist)) {
+ file = list_entry(dev->filelist.next, struct drm_file, lhead);
+
+ /* wake up pending tasks in drm_read() */
+ wake_up_interruptible(&file->event_wait);
+ drm_close(file->filp);
+ }
+
+ /* zap all memory mappings */
+ if (dev->dev_mapping)
+ unmap_mapping_range(dev->dev_mapping, 0, ULLONG_MAX, 1);
+
drm_lastclose(dev);
if (dev->driver->unload)
@@ -610,12 +665,20 @@ void drm_dev_unregister(struct drm_device *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);
-
- drm_put_minor(&dev->primary);
+ /* Only unplug minors, do not free them. Following drm_open() calls
+ * access file_priv->minor->dev to see whether the device is still
+ * active so keep it around. drm_dev_free() frees them eventually. */
+ drm_unplug_minor(dev->control);
+ drm_unplug_minor(dev->primary);
list_del(&dev->driver_item);
+
+ /* Release our fake open_count. If there are no pending open-files,
+ * free the device directly. */
+ if (!--dev->open_count)
+ drm_dev_free(dev);
+
+ mutex_unlock(&drm_global_mutex);
}
EXPORT_SYMBOL(drm_dev_unregister);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 9689173..7e13d5d 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1212,7 +1212,7 @@ struct drm_device {
/** \name Hotplug Management */
/*@{ */
struct percpu_rw_semaphore active; /**< protect active users */
- atomic_t unplugged; /* device has been unplugged or gone away */
+ bool unplugged; /* device has been unplugged or gone away */
/*@} */
};
@@ -1250,17 +1250,9 @@ static inline int drm_core_has_MTRR(struct drm_device *dev)
#define drm_core_has_MTRR(dev) (0)
#endif
-static inline void drm_device_set_unplugged(struct drm_device *dev)
-{
- smp_wmb();
- atomic_set(&dev->unplugged, 1);
-}
-
static inline int drm_device_is_unplugged(struct drm_device *dev)
{
- int ret = atomic_read(&dev->unplugged);
- smp_rmb();
- return ret;
+ return dev->unplugged;
}
static inline bool drm_modeset_is_locked(struct drm_device *dev)
@@ -1287,6 +1279,7 @@ extern int drm_fasync(int fd, struct file *filp, int on);
extern ssize_t drm_read(struct file *filp, char __user *buffer,
size_t count, loff_t *offset);
extern int drm_release(struct inode *inode, struct file *filp);
+extern void drm_close(struct file *filp);
/* Mapping support (drm_vm.h) */
extern int drm_mmap(struct file *filp, struct vm_area_struct *vma);
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread