diff mbox

[2/5] drm: merge device setup into drm_dev_register()

Message ID 1380705818-4065-3-git-send-email-dh.herrmann@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

David Herrmann Oct. 2, 2013, 9:23 a.m. UTC
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(-)

Comments

Daniel Vetter Oct. 3, 2013, 1:15 p.m. UTC | #1
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
David Herrmann Oct. 3, 2013, 1:19 p.m. UTC | #2
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
Daniel Vetter Oct. 3, 2013, 1:21 p.m. UTC | #3
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
diff mbox

Patch

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);
 /*@}*/