The PCI bus helper is the only user of it. Call it directly before device-registration to get rid of the callback.
Note that all drm_agp_*() calls are locked with the drm-global-mutex so we need to explicitly lock it during initialization. It's not really clear why it's needed, but lets be safe.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- Hi
If someone wants to review drm_agpsupport.c and tell me why _all_ calls to drm_agp_*() functions currently hold the global mutex, I'd be happy to hear it. Otherwise, I will just not care for that old stuff and keep the semantics with the kind of ugly locks around drm_pci_agp_*().
Thanks David
drivers/gpu/drm/drm_pci.c | 13 +++++++++++-- drivers/gpu/drm/drm_stub.c | 11 +---------- include/drm/drmP.h | 1 - 3 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index f00d7a9..7d3435c 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -299,7 +299,6 @@ static struct drm_bus drm_pci_bus = { .set_busid = drm_pci_set_busid, .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, };
@@ -338,16 +337,26 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, if (drm_core_check_feature(dev, DRIVER_MODESET)) pci_set_drvdata(pdev, dev);
- ret = drm_dev_register(dev, ent->driver_data); + mutex_lock(&drm_global_mutex); + ret = drm_pci_agp_init(dev); + mutex_unlock(&drm_global_mutex); if (ret) goto err_pci;
+ ret = drm_dev_register(dev, ent->driver_data); + if (ret) + goto err_agp; + 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);
return 0;
+err_agp: + mutex_lock(&drm_global_mutex); + drm_pci_agp_destroy(dev); + mutex_unlock(&drm_global_mutex); err_pci: pci_disable_device(pdev); err_free: diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 26055ab..ac211c3 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -502,16 +502,10 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
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; + goto out_unlock; }
if (drm_core_check_feature(dev, DRIVER_RENDER) && drm_rnodes) { @@ -554,9 +548,6 @@ err_render_node: 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; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 2b954ad..dfc44ae 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -750,7 +750,6 @@ struct drm_bus { struct drm_unique *unique); 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);
};
While the only user of agp_destroy() is the PCI-bus for agp-destruction, this callback is not strictly bound to agp. Rename to destroy() to make clear that any bus can use it to destroy resources once the device is removed.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- Hi
We currently cannot remove the agp_destroy() callback as we have no bus-specific destroy helpers (compared to bus-specific init helpers). I think we can change that once we get the revoke/unplug series I'm working on. Until then, we have to keep the current callbacks.
Feel free to drop it, I just thought the "agp_*" prefix was weird..
Thanks David
drivers/gpu/drm/drm_pci.c | 2 +- drivers/gpu/drm/drm_stub.c | 4 ++-- include/drm/drmP.h | 3 +-- 3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 7d3435c..3e1c3a0 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -299,7 +299,7 @@ static struct drm_bus drm_pci_bus = { .set_busid = drm_pci_set_busid, .set_unique = drm_pci_set_unique, .irq_by_busid = drm_pci_irq_by_busid, - .agp_destroy = drm_pci_agp_destroy, + .destroy = drm_pci_agp_destroy, };
/** diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index ac211c3..c181b71 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -571,8 +571,8 @@ void drm_dev_unregister(struct drm_device *dev) if (dev->driver->unload) dev->driver->unload(dev);
- if (dev->driver->bus->agp_destroy) - dev->driver->bus->agp_destroy(dev); + if (dev->driver->bus->destroy) + dev->driver->bus->destroy(dev);
drm_vblank_cleanup(dev);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index dfc44ae..220013d 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -749,8 +749,7 @@ struct drm_bus { int (*set_unique)(struct drm_device *dev, struct drm_master *master, struct drm_unique *unique); int (*irq_by_busid)(struct drm_device *dev, struct drm_irq_busid *p); - /* hooks that are for PCI */ - void (*agp_destroy)(struct drm_device *dev); + void (*destroy)(struct drm_device *dev);
};
Hi Dave
This one can also go into 3.13. This and 2/2 provide the agp_init() cleanup that Daniel suggested for the drm_dev_*() patches. 2/2 is not required, but I thought it was a nice addition.
Thanks David
On Sat, Oct 19, 2013 at 1:58 PM, David Herrmann dh.herrmann@gmail.com wrote:
The PCI bus helper is the only user of it. Call it directly before device-registration to get rid of the callback.
Note that all drm_agp_*() calls are locked with the drm-global-mutex so we need to explicitly lock it during initialization. It's not really clear why it's needed, but lets be safe.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
Hi
If someone wants to review drm_agpsupport.c and tell me why _all_ calls to drm_agp_*() functions currently hold the global mutex, I'd be happy to hear it. Otherwise, I will just not care for that old stuff and keep the semantics with the kind of ugly locks around drm_pci_agp_*().
Thanks David
drivers/gpu/drm/drm_pci.c | 13 +++++++++++-- drivers/gpu/drm/drm_stub.c | 11 +---------- include/drm/drmP.h | 1 - 3 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index f00d7a9..7d3435c 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -299,7 +299,6 @@ static struct drm_bus drm_pci_bus = { .set_busid = drm_pci_set_busid, .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,};
@@ -338,16 +337,26 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, if (drm_core_check_feature(dev, DRIVER_MODESET)) pci_set_drvdata(pdev, dev);
ret = drm_dev_register(dev, ent->driver_data);
mutex_lock(&drm_global_mutex);ret = drm_pci_agp_init(dev);mutex_unlock(&drm_global_mutex); if (ret) goto err_pci;ret = drm_dev_register(dev, ent->driver_data);if (ret)goto err_agp;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); return 0;+err_agp:
mutex_lock(&drm_global_mutex);drm_pci_agp_destroy(dev);mutex_unlock(&drm_global_mutex);err_pci: pci_disable_device(pdev); err_free: diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 26055ab..ac211c3 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -502,16 +502,10 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
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;
goto out_unlock; } if (drm_core_check_feature(dev, DRIVER_RENDER) && drm_rnodes) {@@ -554,9 +548,6 @@ err_render_node: 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; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 2b954ad..dfc44ae 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -750,7 +750,6 @@ struct drm_bus { struct drm_unique *unique); 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);};
1.8.4.1
On Sun, Nov 03, 2013 at 12:06:05PM +0100, David Herrmann wrote:
Hi Dave
This one can also go into 3.13. This and 2/2 provide the agp_init() cleanup that Daniel suggested for the drm_dev_*() patches. 2/2 is not required, but I thought it was a nice addition.
I have a few more patches to rip out some of the agp init cruft in my demidlayer branch. And they'll conflict with your patch here. If you don't mind I'll pick this one up and rebase it on top of my series.
Wrt patch 2 I don't see the point - better to just outright kill this callback and inline it like the agp_init one. -Daniel
Thanks David
On Sat, Oct 19, 2013 at 1:58 PM, David Herrmann dh.herrmann@gmail.com wrote:
The PCI bus helper is the only user of it. Call it directly before device-registration to get rid of the callback.
Note that all drm_agp_*() calls are locked with the drm-global-mutex so we need to explicitly lock it during initialization. It's not really clear why it's needed, but lets be safe.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
Hi
If someone wants to review drm_agpsupport.c and tell me why _all_ calls to drm_agp_*() functions currently hold the global mutex, I'd be happy to hear it. Otherwise, I will just not care for that old stuff and keep the semantics with the kind of ugly locks around drm_pci_agp_*().
Thanks David
drivers/gpu/drm/drm_pci.c | 13 +++++++++++-- drivers/gpu/drm/drm_stub.c | 11 +---------- include/drm/drmP.h | 1 - 3 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index f00d7a9..7d3435c 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -299,7 +299,6 @@ static struct drm_bus drm_pci_bus = { .set_busid = drm_pci_set_busid, .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,};
@@ -338,16 +337,26 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, if (drm_core_check_feature(dev, DRIVER_MODESET)) pci_set_drvdata(pdev, dev);
ret = drm_dev_register(dev, ent->driver_data);
mutex_lock(&drm_global_mutex);ret = drm_pci_agp_init(dev);mutex_unlock(&drm_global_mutex); if (ret) goto err_pci;ret = drm_dev_register(dev, ent->driver_data);if (ret)goto err_agp;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); return 0;+err_agp:
mutex_lock(&drm_global_mutex);drm_pci_agp_destroy(dev);mutex_unlock(&drm_global_mutex);err_pci: pci_disable_device(pdev); err_free: diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 26055ab..ac211c3 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -502,16 +502,10 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
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;
goto out_unlock; } if (drm_core_check_feature(dev, DRIVER_RENDER) && drm_rnodes) {@@ -554,9 +548,6 @@ err_render_node: 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; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 2b954ad..dfc44ae 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -750,7 +750,6 @@ struct drm_bus { struct drm_unique *unique); 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);};
1.8.4.1
Hi Daniel
On Sun, Nov 3, 2013 at 12:36 PM, Daniel Vetter daniel@ffwll.ch wrote:
On Sun, Nov 03, 2013 at 12:06:05PM +0100, David Herrmann wrote:
Hi Dave
This one can also go into 3.13. This and 2/2 provide the agp_init() cleanup that Daniel suggested for the drm_dev_*() patches. 2/2 is not required, but I thought it was a nice addition.
I have a few more patches to rip out some of the agp init cruft in my demidlayer branch. And they'll conflict with your patch here. If you don't mind I'll pick this one up and rebase it on top of my series.
Fine with me. Then I can drop and ignore them.
Wrt patch 2 I don't see the point - better to just outright kill this callback and inline it like the agp_init one.
2/2 just does a rename. Where do you want to inline it? In drm_pci_exit()? It calls drm_put_dev() which already does a kfree() so we cannot call anything after it. And we cannot call anything before it as ->unload() is called from drm_put_dev().
We could replace drm_put_dev() with: drm_dev_unregister() drm_pci_agp_destroy() drm_dev_free() Or what did you have in mind?
Thanks David
On Sun, Nov 3, 2013 at 12:43 PM, David Herrmann dh.herrmann@gmail.com wrote:
Wrt patch 2 I don't see the point - better to just outright kill this callback and inline it like the agp_init one.
2/2 just does a rename. Where do you want to inline it? In drm_pci_exit()? It calls drm_put_dev() which already does a kfree() so we cannot call anything after it. And we cannot call anything before it as ->unload() is called from drm_put_dev().
We could replace drm_put_dev() with: drm_dev_unregister() drm_pci_agp_destroy() drm_dev_free() Or what did you have in mind?
For now just inline it and make it stick out really badly. Long-term we need to unbundle the teardown sequence. But imo doing that before we've sorted out all the lifetime stuff is no much use. And the lifetime rework needs a reworked init sequence first imo, or at least for end users we care much more about solid loading than solid unloading (there's udl ofc, but meh ...). -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
David Herrmann