Hi all,
So at ks there was a lot of talk about fixing up the drm device model to stop insanity like imx. And I kinda promised Dave to resurrect my latest branch at drm demidlayering - motivation kinda dissipated when I've started this a few moons back.
So this is the first part. Patches 1-11 rework the device list. The ugly part is that drm_platform_exit survived from the horrible days of shadow attache despite that no platform drm driver with shadow attach was ever merged. And everyone dutifully copypasted the crap out of this. Compiletested for all drm drivers that work with arm multiplatforms. Luckily that now includes exynons (yay), but the new kid msm on the block fails this (shame on Rob).
The remaining stuff flattens some of the agp indirection madness away. Plus a tiny patch in between as a follow-up cleanup for David's vma offset manager work. Also includes one patch from David, rebased into my series.
Next up on my plan is to get rid of drm_bus completely. I already have a plan for the irq stuff and a few patches, which leaves the setversion/busid/... bonghits madness. But that's luckily constraint to the drm core.
Once that's settled I think we can start to untangle the driver load/setup vs. interface registration mess. David&Dave already have patches to play around with that. Then we need to untangle all the drm lifetime rules around open files, mmaps, sysfs and debugfs files so that finally we can try to fix the driver unload madness. Or at least that's my overtly optimistic plan.
Flames, comments and more good&dry wood for the pyre highly welcome.
Cheers, Daniel
Daniel Vetter (18): drm/rcar: call drm_put_dev directly in the ->remove hook drm/exynos: call drm_put_dev directly from ->remove drm/imx: directly call drm_put_dev in ->remove drm/tilcdc: call drm_put_dev directly from ->remove drm/omap: call drm_put_dev directly in ->remove drm/shmob: call drm_put_dev directly from ->remove hook drm/host1x: Call drm_put_dev directly instead of drm_platform_exit drm/armada: directly call drm_put_dev in ->remove drm/msm: call drm_put_dev directly in ->remove drm: rip out drm_platform_exit drm: restrict the device list for shadow attached drivers drm/bufs: remove handling of _DRM_GEM mappings drm: kill DRIVER_REQUIRE_AGP drm: ->agp_init can't fail drm: rip out drm_core_has_AGP drm: inline drm_agp_destroy drm: kill the ->agp_destroy callback drm: remove global_mutex locking around agp_init
David Herrmann (1): drm: remove agp_init() bus callback
drivers/gpu/drm/armada/armada_drv.c | 3 ++- drivers/gpu/drm/drm_agpsupport.c | 17 +------------- drivers/gpu/drm/drm_bufs.c | 10 ++------- drivers/gpu/drm/drm_memory.c | 9 +++----- drivers/gpu/drm/drm_pci.c | 38 +++++++++++++++++--------------- drivers/gpu/drm/drm_platform.c | 12 ---------- drivers/gpu/drm/drm_stub.c | 19 +++------------- drivers/gpu/drm/drm_usb.c | 1 - drivers/gpu/drm/drm_vm.c | 7 ++---- drivers/gpu/drm/exynos/exynos_drm_drv.c | 4 +++- drivers/gpu/drm/i810/i810_dma.c | 4 ++++ drivers/gpu/drm/i810/i810_drv.c | 2 +- drivers/gpu/drm/i915/i915_dma.c | 5 +++++ drivers/gpu/drm/i915/i915_drv.c | 5 ++--- drivers/gpu/drm/msm/msm_drv.c | 2 +- drivers/gpu/drm/omapdrm/omap_drv.c | 4 ++-- drivers/gpu/drm/radeon/radeon_ttm.c | 2 +- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 4 +++- drivers/gpu/drm/shmobile/shmob_drm_drv.c | 4 +++- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +- drivers/gpu/host1x/drm/drm.c | 3 +-- drivers/staging/imx-drm/imx-drm-core.c | 4 +++- include/drm/drmP.h | 13 ++++------- include/drm/drm_agpsupport.h | 17 -------------- include/uapi/drm/drm.h | 1 - 25 files changed, 67 insertions(+), 125 deletions(-)
The magic dance drm_platform_exit does is actually a remnant of the old legacy shadow attach support for platform devices. Modern modesetting drm drivers shouldn't do this any more (and usb/pci devices actually don't do this).
Cc: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 0023f9719cf1..4ec6272a1c11 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -224,7 +224,9 @@ static int rcar_du_probe(struct platform_device *pdev)
static int rcar_du_remove(struct platform_device *pdev) { - drm_platform_exit(&rcar_du_driver, pdev); + struct rcar_du_device *rcdu = platform_get_drvdata(pdev); + + drm_put_dev(rcdu->ddev);
return 0; }
Hi Daniel,
Thank you for the patch.
On Sunday 03 November 2013 14:31:07 Daniel Vetter wrote:
The magic dance drm_platform_exit does is actually a remnant of the old legacy shadow attach support for platform devices. Modern modesetting drm drivers shouldn't do this any more (and usb/pci devices actually don't do this).
Cc: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
For 01/19 and 06/19,
Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/rcar-du/rcar_du_drv.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 0023f9719cf1..4ec6272a1c11 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -224,7 +224,9 @@ static int rcar_du_probe(struct platform_device *pdev)
static int rcar_du_remove(struct platform_device *pdev) {
- drm_platform_exit(&rcar_du_driver, pdev);
struct rcar_du_device *rcdu = platform_get_drvdata(pdev);
drm_put_dev(rcdu->ddev);
return 0;
}
I didn't find any user of the driver data yet, so store the drm_device pointer in there.
Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/exynos/exynos_drm_drv.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 3a1e6d9b25f7..f1d066180056 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -119,6 +119,8 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
drm_vblank_offdelay = VBLANK_OFF_DELAY;
+ platform_set_drvdata(dev->platformdev, dev); + return 0;
err_drm_device: @@ -292,7 +294,7 @@ static int exynos_drm_platform_probe(struct platform_device *pdev)
static int exynos_drm_platform_remove(struct platform_device *pdev) { - drm_platform_exit(&exynos_drm_driver, pdev); + drm_put_dev(platform_get_drvdata(pdev));
return 0; }
Again no apparent user of the driver data field.
Cc: Sascha Hauer s.hauer@pengutronix.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/staging/imx-drm/imx-drm-core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c index c1014eb2907d..8ffd4a900bb9 100644 --- a/drivers/staging/imx-drm/imx-drm-core.c +++ b/drivers/staging/imx-drm/imx-drm-core.c @@ -432,6 +432,8 @@ static int imx_drm_driver_load(struct drm_device *drm, unsigned long flags) if (!imx_drm_device_get()) ret = -EINVAL;
+ platform_set_drvdata(drm->platformdev, drm); + ret = 0;
err_init: @@ -807,7 +809,7 @@ static int imx_drm_platform_probe(struct platform_device *pdev)
static int imx_drm_platform_remove(struct platform_device *pdev) { - drm_platform_exit(&imx_drm_driver, pdev); + drm_put_dev(platform_get_drvdata(pdev));
return 0; }
On Sun, Nov 03, 2013 at 02:31:09PM +0100, Daniel Vetter wrote:
Again no apparent user of the driver data field.
Cc: Sascha Hauer s.hauer@pengutronix.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
Acked-by: Sascha Hauer s.hauer@pengutronix.de
Sascha
drivers/staging/imx-drm/imx-drm-core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c index c1014eb2907d..8ffd4a900bb9 100644 --- a/drivers/staging/imx-drm/imx-drm-core.c +++ b/drivers/staging/imx-drm/imx-drm-core.c @@ -432,6 +432,8 @@ static int imx_drm_driver_load(struct drm_device *drm, unsigned long flags) if (!imx_drm_device_get()) ret = -EINVAL;
- platform_set_drvdata(drm->platformdev, drm);
- ret = 0;
err_init: @@ -807,7 +809,7 @@ static int imx_drm_platform_probe(struct platform_device *pdev)
static int imx_drm_platform_remove(struct platform_device *pdev) {
- drm_platform_exit(&imx_drm_driver, pdev);
drm_put_dev(platform_get_drvdata(pdev));
return 0;
}
1.8.4.rc3
tilcdc already stores the drm_device in the driver data pointer. So use that.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 116da199b942..e6d77d02e444 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -594,7 +594,7 @@ static int tilcdc_pdev_probe(struct platform_device *pdev)
static int tilcdc_pdev_remove(struct platform_device *pdev) { - drm_platform_exit(&tilcdc_driver, pdev); + drm_put_dev(platform_get_drvdata(pdev));
return 0; }
Again omap already sets the driver data pointer to the drm_device.
Also drop the driver unregister call, that should be (and already is) done in the module unload hook.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/omapdrm/omap_drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index e7fa3cd96743..13f294aeaefd 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -665,9 +665,9 @@ static int pdev_probe(struct platform_device *device) static int pdev_remove(struct platform_device *device) { DBG(""); - drm_platform_exit(&omap_drm_driver, device);
- platform_driver_unregister(&omap_dmm_driver); + drm_put_dev(platform_get_drvdata(device)); + return 0; }
We need to chase one pointer here.
Cc: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/shmobile/shmob_drm_drv.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/shmobile/shmob_drm_drv.c index 015551866b4a..c839c9c89efb 100644 --- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c +++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c @@ -336,7 +336,9 @@ static int shmob_drm_probe(struct platform_device *pdev)
static int shmob_drm_remove(struct platform_device *pdev) { - drm_platform_exit(&shmob_drm_driver, pdev); + struct shmob_drm_device *sdev = platform_get_drvdata(pdev); + + drm_put_dev(sdev->ddev);
return 0; }
I'm a bit confused about how this all works wrt host1x clients, but this patch looks like the right thing to me.
Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/host1x/drm/drm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/host1x/drm/drm.c b/drivers/gpu/host1x/drm/drm.c index df7d90a3a4fa..e2eabc4078d9 100644 --- a/drivers/gpu/host1x/drm/drm.c +++ b/drivers/gpu/host1x/drm/drm.c @@ -161,7 +161,6 @@ int host1x_drm_init(struct host1x_drm *host1x, struct drm_device *drm)
int host1x_drm_exit(struct host1x_drm *host1x) { - struct platform_device *pdev = to_platform_device(host1x->dev); struct host1x_client *client;
if (!host1x->drm) @@ -184,7 +183,7 @@ int host1x_drm_exit(struct host1x_drm *host1x)
mutex_unlock(&host1x->clients_lock);
- drm_platform_exit(&tegra_drm_driver, pdev); + drm_put_dev(host1x->drm); host1x->drm = NULL;
return 0;
On Sun, Nov 03, 2013 at 02:31:13PM +0100, Daniel Vetter wrote:
I'm a bit confused about how this all works wrt host1x clients, but this patch looks like the right thing to me.
Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/host1x/drm/drm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
This has been largely rewritten in the code that I submitted in my 3.13 pull request. In fact Tegra DRM no longer uses drm_platform, but a custom bus type to deal with the quirkiness of the multi-driver architecture.
So if you really want to get rid of drm_bus altogether I'd appreciate a heads-up so I know what's coming and can prepare.
Thierry
On Mon, Nov 04, 2013 at 10:10:38AM +0100, Thierry Reding wrote:
On Sun, Nov 03, 2013 at 02:31:13PM +0100, Daniel Vetter wrote:
I'm a bit confused about how this all works wrt host1x clients, but this patch looks like the right thing to me.
Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/host1x/drm/drm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
This has been largely rewritten in the code that I submitted in my 3.13 pull request. In fact Tegra DRM no longer uses drm_platform, but a custom bus type to deal with the quirkiness of the multi-driver architecture.
And for what it's worth, there's a drm_host1x_exit() that actually does only drm_put_dev(), which I guess could be simplified in the way that you've done here, but I sort of liked the symmetry between an *_init()/ *_exit() pair of functions.
Thierry
On Mon, Nov 4, 2013 at 10:10 AM, Thierry Reding thierry.reding@gmail.com wrote:
On Sun, Nov 03, 2013 at 02:31:13PM +0100, Daniel Vetter wrote:
I'm a bit confused about how this all works wrt host1x clients, but this patch looks like the right thing to me.
Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/host1x/drm/drm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
This has been largely rewritten in the code that I submitted in my 3.13 pull request. In fact Tegra DRM no longer uses drm_platform, but a custom bus type to deal with the quirkiness of the multi-driver architecture.
So if you really want to get rid of drm_bus altogether I'd appreciate a heads-up so I know what's coming and can prepare.
It's almost completely gone now, the one thing left to do is rip out bus->set_busid. But I have that planned already.
http://cgit.freedesktop.org/~danvet/drm/log/?h=drm-init-cleanup
I guess I get to rebase the entire thing again, meh.
<rant> It'd be awesome of other drm drivers would submit their stuff a bit earlier, atm there's absolutely nothing in drm-next beside all of intel and a few things for armada... Maybe we need to have a drm integration tree to make this a bit less frustrating. </rant>
Cheers, Daniel
On Mon, Nov 04, 2013 at 11:14:39AM +0100, Daniel Vetter wrote:
On Mon, Nov 4, 2013 at 10:10 AM, Thierry Reding thierry.reding@gmail.com wrote:
On Sun, Nov 03, 2013 at 02:31:13PM +0100, Daniel Vetter wrote:
I'm a bit confused about how this all works wrt host1x clients, but this patch looks like the right thing to me.
Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/host1x/drm/drm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
This has been largely rewritten in the code that I submitted in my 3.13 pull request. In fact Tegra DRM no longer uses drm_platform, but a custom bus type to deal with the quirkiness of the multi-driver architecture.
So if you really want to get rid of drm_bus altogether I'd appreciate a heads-up so I know what's coming and can prepare.
It's almost completely gone now, the one thing left to do is rip out bus->set_busid. But I have that planned already.
http://cgit.freedesktop.org/~danvet/drm/log/?h=drm-init-cleanup
I guess I get to rebase the entire thing again, meh.
<rant> It'd be awesome of other drm drivers would submit their stuff a bit earlier, atm there's absolutely nothing in drm-next beside all of intel and a few things for armada... Maybe we need to have a drm integration tree to make this a bit less frustrating. </rant>
Yeah, I know. I had actually planned on sending out this pull request much earlier, but then all the discussion around DRM panel and device tree bindings started again...
Thierry
Again no apparent user of the driver data field.
Cc: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/armada/armada_drv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c index 4f2b28354915..069f64533ac3 100644 --- a/drivers/gpu/drm/armada/armada_drv.c +++ b/drivers/gpu/drm/armada/armada_drv.c @@ -128,6 +128,7 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags) return -ENOMEM; }
+ platform_set_drvdata(dev->platformdev, dev); dev->dev_private = priv;
/* Get the implementation specific driver data. */ @@ -376,7 +377,7 @@ static int armada_drm_probe(struct platform_device *pdev)
static int armada_drm_remove(struct platform_device *pdev) { - drm_platform_exit(&armada_drm_driver, pdev); + drm_put_dev(platform_get_drvdata(pdev)); return 0; }
The drvdata pointer is already assigned to something useful.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/msm/msm_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index b3a2f1629041..b92155de0d31 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -747,7 +747,7 @@ static int msm_pdev_probe(struct platform_device *pdev)
static int msm_pdev_remove(struct platform_device *pdev) { - drm_platform_exit(&msm_driver, pdev); + drm_put_dev(platform_get_drvdata(pdev));
return 0; }
This very much looks like a remnant of the old legady ums shadow attach days. Now with the last users gone we can rip it out since we won't ever support an ums drm driver again.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_platform.c | 11 ----------- include/drm/drmP.h | 1 - 2 files changed, 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c index fc24fee8ec83..56a48033eced 100644 --- a/drivers/gpu/drm/drm_platform.c +++ b/drivers/gpu/drm/drm_platform.c @@ -151,14 +151,3 @@ int drm_platform_init(struct drm_driver *driver, struct platform_device *platfor return drm_get_platform_dev(platform_device, driver); } EXPORT_SYMBOL(drm_platform_init); - -void drm_platform_exit(struct drm_driver *driver, struct platform_device *platform_device) -{ - struct drm_device *dev, *tmp; - DRM_DEBUG("\n"); - - list_for_each_entry_safe(dev, tmp, &driver->device_list, driver_item) - drm_put_dev(dev); - DRM_INFO("Module unloaded\n"); -} -EXPORT_SYMBOL(drm_platform_exit); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 7062307a4a2d..54d6c2d36a5b 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1675,7 +1675,6 @@ extern int drm_pcie_get_speed_cap_mask(struct drm_device *dev, u32 *speed_mask);
/* platform section */ extern int drm_platform_init(struct drm_driver *driver, struct platform_device *platform_device); -extern void drm_platform_exit(struct drm_driver *driver, struct platform_device *platform_device);
/* returns true if currently okay to sleep */ static __inline__ bool drm_can_sleep(void)
There's really no need for the drm core to keep a list of all devices of a given driver - the linux device model keeps perfect track of this already for us.
The exception is old legacy ums drivers using pci shadow attaching. So rename the lists to make the use case clearer and rip out everything else.
v2: Rebase on top of David Herrmann's drm device register changes. Also drop the bogus dev_set_drvdata for platform drivers that somehow crept into the original version - drivers really should be in full control of that field.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_pci.c | 12 ++++++++++-- drivers/gpu/drm/drm_platform.c | 1 - drivers/gpu/drm/drm_stub.c | 4 ---- drivers/gpu/drm/drm_usb.c | 1 - include/drm/drmP.h | 6 +++--- 5 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index f00d7a9671ea..ec78fc805ed0 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -346,6 +346,11 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, driver->name, driver->major, driver->minor, driver->patchlevel, driver->date, pci_name(pdev), dev->primary->index);
+ /* No locking needed since shadow-attach is single-threaded since it may + * only be called from the per-driver module init hook. */ + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + list_add_tail(&dev->legacy_dev_list, &driver->legacy_dev_list); + return 0;
err_pci: @@ -375,7 +380,6 @@ int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver)
DRM_DEBUG("\n");
- INIT_LIST_HEAD(&driver->device_list); driver->kdriver.pci = pdriver; driver->bus = &drm_pci_bus;
@@ -384,6 +388,7 @@ int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver)
/* If not using KMS, fall back to stealth mode manual scanning. */ for (i = 0; pdriver->id_table[i].vendor != 0; i++) { + INIT_LIST_HEAD(&driver->legacy_dev_list); pid = &pdriver->id_table[i];
/* Loop around setting up a DRM device for each PCI device @@ -465,8 +470,11 @@ void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver) if (driver->driver_features & DRIVER_MODESET) { pci_unregister_driver(pdriver); } else { - list_for_each_entry_safe(dev, tmp, &driver->device_list, driver_item) + list_for_each_entry_safe(dev, tmp, &driver->legacy_dev_list, + legacy_dev_list) { drm_put_dev(dev); + list_del(&dev->legacy_dev_list); + } } DRM_INFO("Module unloaded\n"); } diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c index 56a48033eced..21fc82006b78 100644 --- a/drivers/gpu/drm/drm_platform.c +++ b/drivers/gpu/drm/drm_platform.c @@ -147,7 +147,6 @@ int drm_platform_init(struct drm_driver *driver, struct platform_device *platfor
driver->kdriver.platform_device = platform_device; driver->bus = &drm_platform_bus; - INIT_LIST_HEAD(&driver->device_list); return drm_get_platform_dev(platform_device, driver); } EXPORT_SYMBOL(drm_platform_init); diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 26055abf94ee..047d1d1fb0e1 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -538,8 +538,6 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) goto err_unload; }
- list_add_tail(&dev->driver_item, &dev->driver->device_list); - ret = 0; goto out_unlock;
@@ -593,7 +591,5 @@ void drm_dev_unregister(struct drm_device *dev) if (dev->render) drm_put_minor(&dev->render); drm_put_minor(&dev->primary); - - list_del(&dev->driver_item); } EXPORT_SYMBOL(drm_dev_unregister); diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c index b179b70e7853..21ae8d96880b 100644 --- a/drivers/gpu/drm/drm_usb.c +++ b/drivers/gpu/drm/drm_usb.c @@ -63,7 +63,6 @@ int drm_usb_init(struct drm_driver *driver, struct usb_driver *udriver) int res; DRM_DEBUG("\n");
- INIT_LIST_HEAD(&driver->device_list); driver->kdriver.usb = udriver; driver->bus = &drm_usb_bus;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 54d6c2d36a5b..e3dfbae963a5 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -988,8 +988,8 @@ struct drm_driver { } kdriver; struct drm_bus *bus;
- /* List of devices hanging off this driver */ - struct list_head device_list; + /* List of devices hanging off this driver with stealth attach. */ + struct list_head legacy_dev_list; };
#define DRM_MINOR_UNASSIGNED 0 @@ -1099,7 +1099,7 @@ struct drm_vblank_crtc { * may contain multiple heads. */ struct drm_device { - struct list_head driver_item; /**< list of devices per driver */ + struct list_head legacy_dev_list;/**< list of devices per driver for stealth attach cleanup */ char *devname; /**< For /proc/interrupts */ int if_version; /**< Highest interface version set */
Hi Daniel
On Sun, Nov 3, 2013 at 2:31 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
There's really no need for the drm core to keep a list of all devices of a given driver - the linux device model keeps perfect track of this already for us.
The exception is old legacy ums drivers using pci shadow attaching. So rename the lists to make the use case clearer and rip out everything else.
v2: Rebase on top of David Herrmann's drm device register changes. Also drop the bogus dev_set_drvdata for platform drivers that somehow crept into the original version - drivers really should be in full control of that field.
You didn't really change any dev_set_drvdata, did you? And I guess you mean pci_set_drvdata()? I had to keep it in place in drm_pci.c as it has been there before my device-registration changes. However, with your series you added the pci_set_drvdata() everywhere yourself, so yes, please remove it.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_pci.c | 12 ++++++++++-- drivers/gpu/drm/drm_platform.c | 1 - drivers/gpu/drm/drm_stub.c | 4 ---- drivers/gpu/drm/drm_usb.c | 1 - include/drm/drmP.h | 6 +++--- 5 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index f00d7a9671ea..ec78fc805ed0 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -346,6 +346,11 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, driver->name, driver->major, driver->minor, driver->patchlevel, driver->date, pci_name(pdev), dev->primary->index);
/* No locking needed since shadow-attach is single-threaded since it may* only be called from the per-driver module init hook. */if (!drm_core_check_feature(dev, DRIVER_MODESET))list_add_tail(&dev->legacy_dev_list, &driver->legacy_dev_list);return 0;err_pci: @@ -375,7 +380,6 @@ int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver)
DRM_DEBUG("\n");
INIT_LIST_HEAD(&driver->device_list); driver->kdriver.pci = pdriver; driver->bus = &drm_pci_bus;@@ -384,6 +388,7 @@ int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver)
/* If not using KMS, fall back to stealth mode manual scanning. */ for (i = 0; pdriver->id_table[i].vendor != 0; i++) {
INIT_LIST_HEAD(&driver->legacy_dev_list);
Why doing this inside of the loop? If the pci_get_dev() below succeeds, you clear the list again?
Thanks David
pid = &pdriver->id_table[i]; /* Loop around setting up a DRM device for each PCI device@@ -465,8 +470,11 @@ void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver) if (driver->driver_features & DRIVER_MODESET) { pci_unregister_driver(pdriver); } else {
list_for_each_entry_safe(dev, tmp, &driver->device_list, driver_item)
list_for_each_entry_safe(dev, tmp, &driver->legacy_dev_list,legacy_dev_list) { drm_put_dev(dev);list_del(&dev->legacy_dev_list);} } DRM_INFO("Module unloaded\n");} diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c index 56a48033eced..21fc82006b78 100644 --- a/drivers/gpu/drm/drm_platform.c +++ b/drivers/gpu/drm/drm_platform.c @@ -147,7 +147,6 @@ int drm_platform_init(struct drm_driver *driver, struct platform_device *platfor
driver->kdriver.platform_device = platform_device; driver->bus = &drm_platform_bus;
INIT_LIST_HEAD(&driver->device_list); return drm_get_platform_dev(platform_device, driver);} EXPORT_SYMBOL(drm_platform_init); diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 26055abf94ee..047d1d1fb0e1 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -538,8 +538,6 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) goto err_unload; }
list_add_tail(&dev->driver_item, &dev->driver->device_list);ret = 0; goto out_unlock;@@ -593,7 +591,5 @@ void drm_dev_unregister(struct drm_device *dev) if (dev->render) drm_put_minor(&dev->render); drm_put_minor(&dev->primary);
list_del(&dev->driver_item);} EXPORT_SYMBOL(drm_dev_unregister); diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c index b179b70e7853..21ae8d96880b 100644 --- a/drivers/gpu/drm/drm_usb.c +++ b/drivers/gpu/drm/drm_usb.c @@ -63,7 +63,6 @@ int drm_usb_init(struct drm_driver *driver, struct usb_driver *udriver) int res; DRM_DEBUG("\n");
INIT_LIST_HEAD(&driver->device_list); driver->kdriver.usb = udriver; driver->bus = &drm_usb_bus;diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 54d6c2d36a5b..e3dfbae963a5 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -988,8 +988,8 @@ struct drm_driver { } kdriver; struct drm_bus *bus;
/* List of devices hanging off this driver */struct list_head device_list;
/* List of devices hanging off this driver with stealth attach. */struct list_head legacy_dev_list;};
#define DRM_MINOR_UNASSIGNED 0 @@ -1099,7 +1099,7 @@ struct drm_vblank_crtc {
- may contain multiple heads.
*/ struct drm_device {
struct list_head driver_item; /**< list of devices per driver */
struct list_head legacy_dev_list;/**< list of devices per driver for stealth attach cleanup */ char *devname; /**< For /proc/interrupts */ int if_version; /**< Highest interface version set */-- 1.8.4.rc3
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sun, Nov 3, 2013 at 3:05 PM, David Herrmann dh.herrmann@gmail.com wrote:
On Sun, Nov 3, 2013 at 2:31 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
There's really no need for the drm core to keep a list of all devices of a given driver - the linux device model keeps perfect track of this already for us.
The exception is old legacy ums drivers using pci shadow attaching. So rename the lists to make the use case clearer and rip out everything else.
v2: Rebase on top of David Herrmann's drm device register changes. Also drop the bogus dev_set_drvdata for platform drivers that somehow crept into the original version - drivers really should be in full control of that field.
You didn't really change any dev_set_drvdata, did you? And I guess you mean pci_set_drvdata()? I had to keep it in place in drm_pci.c as it has been there before my device-registration changes. However, with your series you added the pci_set_drvdata() everywhere yourself, so yes, please remove it.
That was a bogus hunk in v1 of this patch, which iirc I've never posted onto the list anywhere. I added a platfrom_set_drvdata call, but with the previous series to make sure that each driver has that it's a bit redundant.
Long term, when we split up the drm init code I think the drvdata assignment should be the driver's job.
I'll fix the fumble with the list_head init, thanks for spotting that. -Daniel
There's really no need for the drm core to keep a list of all devices of a given driver - the linux device model keeps perfect track of this already for us.
The exception is old legacy ums drivers using pci shadow attaching. So rename the lists to make the use case clearer and rip out everything else.
v2: Rebase on top of David Herrmann's drm device register changes. Also drop the bogus dev_set_drvdata for platform drivers that somehow crept into the original version - drivers really should be in full control of that field.
v3: Initialize driver->legacy_dev_list outside of the loop, spotted by David Herrmann.
Cc: David Herrmann dh.herrmann@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_pci.c | 12 ++++++++++-- drivers/gpu/drm/drm_platform.c | 1 - drivers/gpu/drm/drm_stub.c | 4 ---- drivers/gpu/drm/drm_usb.c | 1 - include/drm/drmP.h | 6 +++--- 5 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index f00d7a9671ea..e92b9567485b 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -346,6 +346,11 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, driver->name, driver->major, driver->minor, driver->patchlevel, driver->date, pci_name(pdev), dev->primary->index);
+ /* No locking needed since shadow-attach is single-threaded since it may + * only be called from the per-driver module init hook. */ + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + list_add_tail(&dev->legacy_dev_list, &driver->legacy_dev_list); + return 0;
err_pci: @@ -375,7 +380,6 @@ int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver)
DRM_DEBUG("\n");
- INIT_LIST_HEAD(&driver->device_list); driver->kdriver.pci = pdriver; driver->bus = &drm_pci_bus;
@@ -383,6 +387,7 @@ int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver) return pci_register_driver(pdriver);
/* If not using KMS, fall back to stealth mode manual scanning. */ + INIT_LIST_HEAD(&driver->legacy_dev_list); for (i = 0; pdriver->id_table[i].vendor != 0; i++) { pid = &pdriver->id_table[i];
@@ -465,8 +470,11 @@ void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver) if (driver->driver_features & DRIVER_MODESET) { pci_unregister_driver(pdriver); } else { - list_for_each_entry_safe(dev, tmp, &driver->device_list, driver_item) + list_for_each_entry_safe(dev, tmp, &driver->legacy_dev_list, + legacy_dev_list) { drm_put_dev(dev); + list_del(&dev->legacy_dev_list); + } } DRM_INFO("Module unloaded\n"); } diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c index 56a48033eced..21fc82006b78 100644 --- a/drivers/gpu/drm/drm_platform.c +++ b/drivers/gpu/drm/drm_platform.c @@ -147,7 +147,6 @@ int drm_platform_init(struct drm_driver *driver, struct platform_device *platfor
driver->kdriver.platform_device = platform_device; driver->bus = &drm_platform_bus; - INIT_LIST_HEAD(&driver->device_list); return drm_get_platform_dev(platform_device, driver); } EXPORT_SYMBOL(drm_platform_init); diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 26055abf94ee..047d1d1fb0e1 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -538,8 +538,6 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) goto err_unload; }
- list_add_tail(&dev->driver_item, &dev->driver->device_list); - ret = 0; goto out_unlock;
@@ -593,7 +591,5 @@ void drm_dev_unregister(struct drm_device *dev) if (dev->render) drm_put_minor(&dev->render); drm_put_minor(&dev->primary); - - list_del(&dev->driver_item); } EXPORT_SYMBOL(drm_dev_unregister); diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c index b179b70e7853..21ae8d96880b 100644 --- a/drivers/gpu/drm/drm_usb.c +++ b/drivers/gpu/drm/drm_usb.c @@ -63,7 +63,6 @@ int drm_usb_init(struct drm_driver *driver, struct usb_driver *udriver) int res; DRM_DEBUG("\n");
- INIT_LIST_HEAD(&driver->device_list); driver->kdriver.usb = udriver; driver->bus = &drm_usb_bus;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 54d6c2d36a5b..e3dfbae963a5 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -988,8 +988,8 @@ struct drm_driver { } kdriver; struct drm_bus *bus;
- /* List of devices hanging off this driver */ - struct list_head device_list; + /* List of devices hanging off this driver with stealth attach. */ + struct list_head legacy_dev_list; };
#define DRM_MINOR_UNASSIGNED 0 @@ -1099,7 +1099,7 @@ struct drm_vblank_crtc { * may contain multiple heads. */ struct drm_device { - struct list_head driver_item; /**< list of devices per driver */ + struct list_head legacy_dev_list;/**< list of devices per driver for stealth attach cleanup */ char *devname; /**< For /proc/interrupts */ int if_version; /**< Highest interface version set */
On Sun, Nov 03, 2013 at 03:14:16PM +0100, Daniel Vetter wrote:
On Sun, Nov 3, 2013 at 3:05 PM, David Herrmann dh.herrmann@gmail.com wrote:
On Sun, Nov 3, 2013 at 2:31 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
There's really no need for the drm core to keep a list of all devices of a given driver - the linux device model keeps perfect track of this already for us.
The exception is old legacy ums drivers using pci shadow attaching. So rename the lists to make the use case clearer and rip out everything else.
v2: Rebase on top of David Herrmann's drm device register changes. Also drop the bogus dev_set_drvdata for platform drivers that somehow crept into the original version - drivers really should be in full control of that field.
You didn't really change any dev_set_drvdata, did you? And I guess you mean pci_set_drvdata()? I had to keep it in place in drm_pci.c as it has been there before my device-registration changes. However, with your series you added the pci_set_drvdata() everywhere yourself, so yes, please remove it.
That was a bogus hunk in v1 of this patch, which iirc I've never posted onto the list anywhere. I added a platfrom_set_drvdata call, but with the previous series to make sure that each driver has that it's a bit redundant.
Long term, when we split up the drm init code I think the drvdata assignment should be the driver's job.
I remember submitting a patch for that a while ago. It was applied about a year ago, see commit a16d4f86019a ('drm: platform: Don't initialize driver-private data'). The issue at the time was that I needed the drvdata for other purposes and drm_platform_init() kept overwriting it, which had me confused for days.
Thierry
Gone with the new gem vma offset manager from David.
We can also ditch the uapi header definition from the enum since userspace never used this. It ended up in there purely for historical reasons (for reusing the old drm mmap code essentially), not because userspace ever needed it.
Cc: David Herrmann dh.herrmann@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_bufs.c | 6 ------ drivers/gpu/drm/drm_vm.c | 3 --- include/uapi/drm/drm.h | 1 - 3 files changed, 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index 471e051d295e..766a5474fdbd 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -303,9 +303,6 @@ static int drm_addmap_core(struct drm_device * dev, resource_size_t offset,
break; } - case _DRM_GEM: - DRM_ERROR("tried to addmap GEM object\n"); - break; case _DRM_SCATTER_GATHER: if (!dev->sg) { kfree(map); @@ -483,9 +480,6 @@ int drm_rmmap_locked(struct drm_device *dev, struct drm_local_map *map) dmah.size = map->size; __drm_pci_free(dev, &dmah); break; - case _DRM_GEM: - DRM_ERROR("tried to rmmap GEM object\n"); - break; } kfree(map);
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index b5c5af7328df..efd424da7be6 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -266,9 +266,6 @@ static void drm_vm_shm_close(struct vm_area_struct *vma) dmah.size = map->size; __drm_pci_free(dev, &dmah); break; - case _DRM_GEM: - DRM_ERROR("tried to rmmap GEM object\n"); - break; } kfree(map); } diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 9b24d65fed72..3c9a833992e8 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -181,7 +181,6 @@ enum drm_map_type { _DRM_AGP = 3, /**< AGP/GART */ _DRM_SCATTER_GATHER = 4, /**< Scatter/gather memory for PCI DMA */ _DRM_CONSISTENT = 5, /**< Consistent memory for PCI DMA */ - _DRM_GEM = 6, /**< GEM object (obsolete) */ };
/**
Hi Daniel
On Sun, Nov 3, 2013 at 2:31 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Gone with the new gem vma offset manager from David.
We can also ditch the uapi header definition from the enum since userspace never used this. It ended up in there purely for historical reasons (for reusing the old drm mmap code essentially), not because userspace ever needed it.
Cc: David Herrmann dh.herrmann@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
Thanks for removing it. I haven't found any code using _DRM_GEM when changing the vma-manager either, so:
Reviewed-by: David Herrmann dh.herrmann@gmail.com
Thanks David
drivers/gpu/drm/drm_bufs.c | 6 ------ drivers/gpu/drm/drm_vm.c | 3 --- include/uapi/drm/drm.h | 1 - 3 files changed, 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index 471e051d295e..766a5474fdbd 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -303,9 +303,6 @@ static int drm_addmap_core(struct drm_device * dev, resource_size_t offset,
break; }
case _DRM_GEM:DRM_ERROR("tried to addmap GEM object\n");break; case _DRM_SCATTER_GATHER: if (!dev->sg) { kfree(map);@@ -483,9 +480,6 @@ int drm_rmmap_locked(struct drm_device *dev, struct drm_local_map *map) dmah.size = map->size; __drm_pci_free(dev, &dmah); break;
case _DRM_GEM:DRM_ERROR("tried to rmmap GEM object\n");break; } kfree(map);diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index b5c5af7328df..efd424da7be6 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -266,9 +266,6 @@ static void drm_vm_shm_close(struct vm_area_struct *vma) dmah.size = map->size; __drm_pci_free(dev, &dmah); break;
case _DRM_GEM:DRM_ERROR("tried to rmmap GEM object\n");break; } kfree(map); }diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 9b24d65fed72..3c9a833992e8 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -181,7 +181,6 @@ enum drm_map_type { _DRM_AGP = 3, /**< AGP/GART */ _DRM_SCATTER_GATHER = 4, /**< Scatter/gather memory for PCI DMA */ _DRM_CONSISTENT = 5, /**< Consistent memory for PCI DMA */
_DRM_GEM = 6, /**< GEM object (obsolete) */};
/**
1.8.4.rc3
Only the two intel drivers need this and they can easily check for working agp support in their driver ->load callbacks.
This is the only reason why agp initialization could fail, so allows us to rip out a bit of error handling code in the next patch.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_pci.c | 5 ----- drivers/gpu/drm/i810/i810_dma.c | 4 ++++ drivers/gpu/drm/i810/i810_drv.c | 2 +- drivers/gpu/drm/i915/i915_dma.c | 5 +++++ drivers/gpu/drm/i915/i915_drv.c | 5 ++--- include/drm/drmP.h | 1 - 6 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index ec78fc805ed0..2b0d7deb4b6d 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -267,11 +267,6 @@ static int drm_pci_agp_init(struct drm_device *dev) if (drm_core_has_AGP(dev)) { if (drm_pci_device_is_agp(dev)) dev->agp = drm_agp_init(dev); - if (drm_core_check_feature(dev, DRIVER_REQUIRE_AGP) - && (dev->agp == NULL)) { - DRM_ERROR("Cannot initialize the agpgart module.\n"); - return -EINVAL; - } if (dev->agp) { dev->agp->agp_mtrr = arch_phys_wc_add( dev->agp->agp_info.aper_base, diff --git a/drivers/gpu/drm/i810/i810_dma.c b/drivers/gpu/drm/i810/i810_dma.c index 249fdff305c6..aeace37415aa 100644 --- a/drivers/gpu/drm/i810/i810_dma.c +++ b/drivers/gpu/drm/i810/i810_dma.c @@ -1193,6 +1193,10 @@ static int i810_flip_bufs(struct drm_device *dev, void *data,
int i810_driver_load(struct drm_device *dev, unsigned long flags) { + /* Our userspace depends upon the agp mapping support. */ + if (!dev->agp) + return -EINVAL; + pci_set_master(dev->pdev);
return 0; diff --git a/drivers/gpu/drm/i810/i810_drv.c b/drivers/gpu/drm/i810/i810_drv.c index d8180d22cedd..441ccf8f5bdc 100644 --- a/drivers/gpu/drm/i810/i810_drv.c +++ b/drivers/gpu/drm/i810/i810_drv.c @@ -57,7 +57,7 @@ static const struct file_operations i810_driver_fops = {
static struct drm_driver driver = { .driver_features = - DRIVER_USE_AGP | DRIVER_REQUIRE_AGP | + DRIVER_USE_AGP | DRIVER_HAVE_DMA, .dev_priv_size = sizeof(drm_i810_buf_priv_t), .load = i810_driver_load, diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 0cab2d045135..1244280946c2 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1476,7 +1476,12 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) return -ENODEV; }
+ /* UMS needs agp support. */ + if (!drm_core_check_feature(dev, DRIVER_MODESET) && !dev->agp) + return -EINVAL; + dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL); + if (dev_priv == NULL) return -ENOMEM;
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index a0804fa1e306..d86c2e1733fd 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -801,8 +801,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) * (gasp!) to share buffers between X and the client. Hence we need to * keep around the fake agp stuff for gen3, even when kms is enabled. */ if (intel_info->gen != 3) { - driver.driver_features &= - ~(DRIVER_USE_AGP | DRIVER_REQUIRE_AGP); + driver.driver_features &= ~DRIVER_USE_AGP; } else if (!intel_agp_enabled) { DRM_ERROR("drm/i915 can't work without intel_agp module!\n"); return -ENODEV; @@ -914,7 +913,7 @@ static struct drm_driver driver = { * deal with them for Intel hardware. */ .driver_features = - DRIVER_USE_AGP | DRIVER_REQUIRE_AGP | + DRIVER_USE_AGP | DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM | DRIVER_PRIME | DRIVER_RENDER, .load = i915_driver_load, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index e3dfbae963a5..0ae0f58e1988 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -136,7 +136,6 @@ int drm_err(const char *func, const char *format, ...);
/* driver capabilities and requirements mask */ #define DRIVER_USE_AGP 0x1 -#define DRIVER_REQUIRE_AGP 0x2 #define DRIVER_PCI_DMA 0x8 #define DRIVER_SG 0x10 #define DRIVER_HAVE_DMA 0x20
Thanks to the removal of REQUIRE_AGP we can use a void return value and shed a bit of complexity.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_pci.c | 3 +-- drivers/gpu/drm/drm_stub.c | 7 ++----- include/drm/drmP.h | 2 +- 3 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 2b0d7deb4b6d..3bd8b3fc6189 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -262,7 +262,7 @@ static int drm_pci_irq_by_busid(struct drm_device *dev, struct drm_irq_busid *p) return 0; }
-static int drm_pci_agp_init(struct drm_device *dev) +static void drm_pci_agp_init(struct drm_device *dev) { if (drm_core_has_AGP(dev)) { if (drm_pci_device_is_agp(dev)) @@ -274,7 +274,6 @@ static int drm_pci_agp_init(struct drm_device *dev) 1024 * 1024); } } - return 0; }
static void drm_pci_agp_destroy(struct drm_device *dev) diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 047d1d1fb0e1..dea432b2dfcf 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -502,11 +502,8 @@ 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 (dev->driver->bus->agp_init) + dev->driver->bus->agp_init(dev);
if (drm_core_check_feature(dev, DRIVER_MODESET)) { ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 0ae0f58e1988..df502c292ef8 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -749,7 +749,7 @@ 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_init)(struct drm_device *dev); void (*agp_destroy)(struct drm_device *dev);
};
Hi Daniel
On Sun, Nov 3, 2013 at 2:31 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Thanks to the removal of REQUIRE_AGP we can use a void return value and shed a bit of complexity.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_pci.c | 3 +-- drivers/gpu/drm/drm_stub.c | 7 ++----- include/drm/drmP.h | 2 +- 3 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 2b0d7deb4b6d..3bd8b3fc6189 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -262,7 +262,7 @@ static int drm_pci_irq_by_busid(struct drm_device *dev, struct drm_irq_busid *p) return 0; }
-static int drm_pci_agp_init(struct drm_device *dev) +static void drm_pci_agp_init(struct drm_device *dev) { if (drm_core_has_AGP(dev)) { if (drm_pci_device_is_agp(dev)) @@ -274,7 +274,6 @@ static int drm_pci_agp_init(struct drm_device *dev) 1024 * 1024); } }
return 0;}
static void drm_pci_agp_destroy(struct drm_device *dev) diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 047d1d1fb0e1..dea432b2dfcf 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -502,11 +502,8 @@ 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 (dev->driver->bus->agp_init)dev->driver->bus->agp_init(dev);
Ok, technically, you need to remove "out_unlock" here. But patch #16 removes err_agp and reused out_unlock, so I think this is fine.
Reviewed-by: David Herrmann dh.herrmann@gmail.com
Thanks David
if (drm_core_check_feature(dev, DRIVER_MODESET)) { ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 0ae0f58e1988..df502c292ef8 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -749,7 +749,7 @@ 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_init)(struct drm_device *dev); void (*agp_destroy)(struct drm_device *dev);};
1.8.4.rc3
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Most place actually want to just check for dev->agp (most do, but a few don't so this fixes a few potential NULL derefs). The only exception is the agp init code which should check for the AGP driver feature flag.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_agpsupport.c | 2 +- drivers/gpu/drm/drm_bufs.c | 4 ++-- drivers/gpu/drm/drm_memory.c | 9 +++------ drivers/gpu/drm/drm_pci.c | 4 ++-- drivers/gpu/drm/drm_vm.c | 4 ++-- drivers/gpu/drm/radeon/radeon_ttm.c | 2 +- include/drm/drm_agpsupport.h | 12 ------------ 7 files changed, 11 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_agpsupport.c b/drivers/gpu/drm/drm_agpsupport.c index e301d653d97e..084a674e4b56 100644 --- a/drivers/gpu/drm/drm_agpsupport.c +++ b/drivers/gpu/drm/drm_agpsupport.c @@ -439,7 +439,7 @@ void drm_agp_clear(struct drm_device *dev) { struct drm_agp_mem *entry, *tempe;
- if (!drm_core_has_AGP(dev) || !dev->agp) + if (!dev->agp) return; if (drm_core_check_feature(dev, DRIVER_MODESET)) return; diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index 766a5474fdbd..edec31fe3fed 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -261,7 +261,7 @@ static int drm_addmap_core(struct drm_device * dev, resource_size_t offset, struct drm_agp_mem *entry; int valid = 0;
- if (!drm_core_has_AGP(dev)) { + if (!dev->agp) { kfree(map); return -EINVAL; } @@ -1390,7 +1390,7 @@ int drm_mapbufs(struct drm_device *dev, void *data, spin_unlock(&dev->count_lock);
if (request->count >= dma->buf_count) { - if ((drm_core_has_AGP(dev) && (dma->flags & _DRM_DMA_USE_AGP)) + if ((dev->agp && (dma->flags & _DRM_DMA_USE_AGP)) || (drm_core_check_feature(dev, DRIVER_SG) && (dma->flags & _DRM_DMA_USE_SG))) { struct drm_local_map *map = dev->agp_buffer_map; diff --git a/drivers/gpu/drm/drm_memory.c b/drivers/gpu/drm/drm_memory.c index 64e44fad8ae8..3359bc453e11 100644 --- a/drivers/gpu/drm/drm_memory.c +++ b/drivers/gpu/drm/drm_memory.c @@ -110,8 +110,7 @@ static inline void *agp_remap(unsigned long offset, unsigned long size,
void drm_core_ioremap(struct drm_local_map *map, struct drm_device *dev) { - if (drm_core_has_AGP(dev) && - dev->agp && dev->agp->cant_use_aperture && map->type == _DRM_AGP) + if (dev->agp && dev->agp->cant_use_aperture && map->type == _DRM_AGP) map->handle = agp_remap(map->offset, map->size, dev); else map->handle = ioremap(map->offset, map->size); @@ -120,8 +119,7 @@ EXPORT_SYMBOL(drm_core_ioremap);
void drm_core_ioremap_wc(struct drm_local_map *map, struct drm_device *dev) { - if (drm_core_has_AGP(dev) && - dev->agp && dev->agp->cant_use_aperture && map->type == _DRM_AGP) + if (dev->agp && dev->agp->cant_use_aperture && map->type == _DRM_AGP) map->handle = agp_remap(map->offset, map->size, dev); else map->handle = ioremap_wc(map->offset, map->size); @@ -133,8 +131,7 @@ void drm_core_ioremapfree(struct drm_local_map *map, struct drm_device *dev) if (!map->handle || !map->size) return;
- if (drm_core_has_AGP(dev) && - dev->agp && dev->agp->cant_use_aperture && map->type == _DRM_AGP) + if (dev->agp && dev->agp->cant_use_aperture && map->type == _DRM_AGP) vunmap(map->handle); else iounmap(map->handle); diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 3bd8b3fc6189..4ec43b8d42f5 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -264,7 +264,7 @@ static int drm_pci_irq_by_busid(struct drm_device *dev, struct drm_irq_busid *p)
static void drm_pci_agp_init(struct drm_device *dev) { - if (drm_core_has_AGP(dev)) { + if (drm_core_check_feature(dev, DRIVER_USE_AGP)) { if (drm_pci_device_is_agp(dev)) dev->agp = drm_agp_init(dev); if (dev->agp) { @@ -278,7 +278,7 @@ static void drm_pci_agp_init(struct drm_device *dev)
static void drm_pci_agp_destroy(struct drm_device *dev) { - if (drm_core_has_AGP(dev) && dev->agp) { + if (dev->agp) { arch_phys_wc_del(dev->agp->agp_mtrr); drm_agp_clear(dev); drm_agp_destroy(dev->agp); diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index efd424da7be6..008c8c0e221f 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -101,7 +101,7 @@ static int drm_do_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf) /* * Find the right map */ - if (!drm_core_has_AGP(dev)) + if (!dev->agp) goto vm_fault_error;
if (!dev->agp || !dev->agp->cant_use_aperture) @@ -592,7 +592,7 @@ int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) switch (map->type) { #if !defined(__arm__) case _DRM_AGP: - if (drm_core_has_AGP(dev) && dev->agp->cant_use_aperture) { + if (dev->agp && dev->agp->cant_use_aperture) { /* * On some platforms we can't talk to bus dma address from the CPU, so for * memory of type DRM_AGP, we'll deal with sorting out the real physical diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index 71245d6f34a2..051fa874065a 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -142,7 +142,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type, man->flags = TTM_MEMTYPE_FLAG_MAPPABLE | TTM_MEMTYPE_FLAG_CMA; #if __OS_HAS_AGP if (rdev->flags & RADEON_IS_AGP) { - if (!(drm_core_has_AGP(rdev->ddev) && rdev->ddev->agp)) { + if (!rdev->ddev->agp) { DRM_ERROR("AGP is not enabled for memory type %u\n", (unsigned)type); return -EINVAL; diff --git a/include/drm/drm_agpsupport.h b/include/drm/drm_agpsupport.h index a184eeee9c96..a12b0e011e44 100644 --- a/include/drm/drm_agpsupport.h +++ b/include/drm/drm_agpsupport.h @@ -46,12 +46,6 @@ int drm_agp_unbind_ioctl(struct drm_device *dev, void *data, int drm_agp_bind(struct drm_device *dev, struct drm_agp_binding *request); int drm_agp_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); - -static inline int drm_core_has_AGP(struct drm_device *dev) -{ - return drm_core_check_feature(dev, DRIVER_USE_AGP); -} - #else /* __OS_HAS_AGP */
static inline void drm_free_agp(DRM_AGP_MEM * handle, int pages) @@ -183,12 +177,6 @@ static inline int drm_agp_bind_ioctl(struct drm_device *dev, void *data, { return -ENODEV; } - -static inline int drm_core_has_AGP(struct drm_device *dev) -{ - return 0; -} - #endif /* __OS_HAS_AGP */
#endif /* _DRM_AGPSUPPORT_H_ */
From: David Herrmann dh.herrmann@gmail.com
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.
v2: Rebase on top of the agp_init interface change.
Signed-off-by: David Herrmann dh.herrmann@gmail.com (v1) Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_pci.c | 13 +++++++++---- drivers/gpu/drm/drm_stub.c | 8 +------- include/drm/drmP.h | 1 - 3 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 4ec43b8d42f5..54eae6d83e5d 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -292,8 +292,6 @@ static struct drm_bus drm_pci_bus = { .get_name = drm_pci_get_name, .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, };
@@ -332,9 +330,13 @@ 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);
+ mutex_lock(&drm_global_mutex); + drm_pci_agp_init(dev); + mutex_unlock(&drm_global_mutex); + ret = drm_dev_register(dev, ent->driver_data); if (ret) - goto err_pci; + 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, @@ -347,7 +349,10 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
return 0;
-err_pci: +err_agp: + mutex_lock(&drm_global_mutex); + drm_pci_agp_destroy(dev); + mutex_unlock(&drm_global_mutex); pci_disable_device(pdev); err_free: drm_dev_free(dev); diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index dea432b2dfcf..4b25f693ae89 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -502,13 +502,10 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
mutex_lock(&drm_global_mutex);
- if (dev->driver->bus->agp_init) - dev->driver->bus->agp_init(dev); - 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) { @@ -549,9 +546,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 df502c292ef8..8cf8cfef8c56 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -749,7 +749,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 */ - void (*agp_init)(struct drm_device *dev); void (*agp_destroy)(struct drm_device *dev);
};
Hi Daniel
On Sun, Nov 3, 2013 at 2:31 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
From: David Herrmann dh.herrmann@gmail.com
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.
v2: Rebase on top of the agp_init interface change.
Signed-off-by: David Herrmann dh.herrmann@gmail.com (v1) Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_pci.c | 13 +++++++++---- drivers/gpu/drm/drm_stub.c | 8 +------- include/drm/drmP.h | 1 - 3 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 4ec43b8d42f5..54eae6d83e5d 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -292,8 +292,6 @@ static struct drm_bus drm_pci_bus = { .get_name = drm_pci_get_name, .set_busid = drm_pci_set_busid, .set_unique = drm_pci_set_unique,
.irq_by_busid = drm_pci_irq_by_busid,
Err, where does that come from?
Cheers David
.agp_init = drm_pci_agp_init, .agp_destroy = drm_pci_agp_destroy,};
@@ -332,9 +330,13 @@ 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);
mutex_lock(&drm_global_mutex);drm_pci_agp_init(dev);mutex_unlock(&drm_global_mutex);ret = drm_dev_register(dev, ent->driver_data); if (ret)
goto err_pci;
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,@@ -347,7 +349,10 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
return 0;-err_pci: +err_agp:
mutex_lock(&drm_global_mutex);drm_pci_agp_destroy(dev);mutex_unlock(&drm_global_mutex); pci_disable_device(pdev);err_free: drm_dev_free(dev); diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index dea432b2dfcf..4b25f693ae89 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -502,13 +502,10 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
mutex_lock(&drm_global_mutex);
if (dev->driver->bus->agp_init)dev->driver->bus->agp_init(dev);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) {@@ -549,9 +546,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 df502c292ef8..8cf8cfef8c56 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -749,7 +749,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 */
void (*agp_init)(struct drm_device *dev); void (*agp_destroy)(struct drm_device *dev);};
1.8.4.rc3
On Sun, Nov 3, 2013 at 3:14 PM, David Herrmann dh.herrmann@gmail.com wrote:
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 4ec43b8d42f5..54eae6d83e5d 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -292,8 +292,6 @@ static struct drm_bus drm_pci_bus = { .get_name = drm_pci_get_name, .set_busid = drm_pci_set_busid, .set_unique = drm_pci_set_unique,
.irq_by_busid = drm_pci_irq_by_busid,Err, where does that come from?
Pretty bad case of rebase fail - it's all ripped out in the in-flux irq rework I have here on top of these patches. Thanks for spotting this. -Daniel
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.
v2: Rebase on top of the agp_init interface change.
v3: Remove the rebase-fail where I've accidentally killed the ->irq_by_busid callback a bit too early.
Cc: David Herrmann dh.herrmann@gmail.com Signed-off-by: David Herrmann dh.herrmann@gmail.com (v1) Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_pci.c | 12 +++++++++--- drivers/gpu/drm/drm_stub.c | 8 +------- include/drm/drmP.h | 1 - 3 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 4ec43b8d42f5..02688858785b 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -293,7 +293,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, };
@@ -332,9 +331,13 @@ 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);
+ mutex_lock(&drm_global_mutex); + drm_pci_agp_init(dev); + mutex_unlock(&drm_global_mutex); + ret = drm_dev_register(dev, ent->driver_data); if (ret) - goto err_pci; + 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, @@ -347,7 +350,10 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
return 0;
-err_pci: +err_agp: + mutex_lock(&drm_global_mutex); + drm_pci_agp_destroy(dev); + mutex_unlock(&drm_global_mutex); pci_disable_device(pdev); err_free: drm_dev_free(dev); diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index dea432b2dfcf..4b25f693ae89 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -502,13 +502,10 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
mutex_lock(&drm_global_mutex);
- if (dev->driver->bus->agp_init) - dev->driver->bus->agp_init(dev); - 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) { @@ -549,9 +546,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 df502c292ef8..8cf8cfef8c56 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -749,7 +749,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 */ - void (*agp_init)(struct drm_device *dev); void (*agp_destroy)(struct drm_device *dev);
};
Wrapping a kfree is pointless.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_agpsupport.c | 15 --------------- drivers/gpu/drm/drm_pci.c | 2 +- include/drm/drm_agpsupport.h | 5 ----- 3 files changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/drm_agpsupport.c b/drivers/gpu/drm/drm_agpsupport.c index 084a674e4b56..c5f663bb8bc1 100644 --- a/drivers/gpu/drm/drm_agpsupport.c +++ b/drivers/gpu/drm/drm_agpsupport.c @@ -460,21 +460,6 @@ void drm_agp_clear(struct drm_device *dev) }
/** - * 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_pci.c b/drivers/gpu/drm/drm_pci.c index 54eae6d83e5d..d0937dd62899 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -281,7 +281,7 @@ static void drm_pci_agp_destroy(struct drm_device *dev) if (dev->agp) { arch_phys_wc_del(dev->agp->agp_mtrr); drm_agp_clear(dev); - drm_agp_destroy(dev->agp); + kfree(dev->agp); dev->agp = NULL; } } diff --git a/include/drm/drm_agpsupport.h b/include/drm/drm_agpsupport.h index a12b0e011e44..56a861b2ceaa 100644 --- a/include/drm/drm_agpsupport.h +++ b/include/drm/drm_agpsupport.h @@ -20,7 +20,6 @@ DRM_AGP_MEM *drm_agp_bind_pages(struct drm_device *dev, uint32_t type);
struct drm_agp_head *drm_agp_init(struct drm_device *dev); -void drm_agp_destroy(struct drm_agp_head *agp); void drm_agp_clear(struct drm_device *dev); int drm_agp_acquire(struct drm_device *dev); int drm_agp_acquire_ioctl(struct drm_device *dev, void *data, @@ -76,10 +75,6 @@ static inline struct drm_agp_head *drm_agp_init(struct drm_device *dev) return NULL; }
-static inline void drm_agp_destroy(struct drm_agp_head *agp) -{ -} - static inline void drm_agp_clear(struct drm_device *dev) { }
Hi
On Sun, Nov 3, 2013 at 2:31 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Wrapping a kfree is pointless.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
I added that one dutifully to document the API in:
commit 28ec711cd427f8b61f73712a43b8100ba8ca933b Author: David Herrmann dh.herrmann@gmail.com Date: Sat Jul 27 16:37:00 2013 +0200
drm/agp: move AGP cleanup paths to drm_agpsupport.c
By removing it, we loose the comment on how to clean it up. Maybe we can copy the comment to drm_agp_init()? Other than that, I'm fine with it:
Reviewed-by: David Herrmann dh.herrmann@gmail.com
Thanks David
drivers/gpu/drm/drm_agpsupport.c | 15 --------------- drivers/gpu/drm/drm_pci.c | 2 +- include/drm/drm_agpsupport.h | 5 ----- 3 files changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/drm_agpsupport.c b/drivers/gpu/drm/drm_agpsupport.c index 084a674e4b56..c5f663bb8bc1 100644 --- a/drivers/gpu/drm/drm_agpsupport.c +++ b/drivers/gpu/drm/drm_agpsupport.c @@ -460,21 +460,6 @@ void drm_agp_clear(struct drm_device *dev) }
/**
- 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_pci.c b/drivers/gpu/drm/drm_pci.c index 54eae6d83e5d..d0937dd62899 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -281,7 +281,7 @@ static void drm_pci_agp_destroy(struct drm_device *dev) if (dev->agp) { arch_phys_wc_del(dev->agp->agp_mtrr); drm_agp_clear(dev);
drm_agp_destroy(dev->agp);
kfree(dev->agp); dev->agp = NULL; }} diff --git a/include/drm/drm_agpsupport.h b/include/drm/drm_agpsupport.h index a12b0e011e44..56a861b2ceaa 100644 --- a/include/drm/drm_agpsupport.h +++ b/include/drm/drm_agpsupport.h @@ -20,7 +20,6 @@ DRM_AGP_MEM *drm_agp_bind_pages(struct drm_device *dev, uint32_t type);
struct drm_agp_head *drm_agp_init(struct drm_device *dev); -void drm_agp_destroy(struct drm_agp_head *agp); void drm_agp_clear(struct drm_device *dev); int drm_agp_acquire(struct drm_device *dev); int drm_agp_acquire_ioctl(struct drm_device *dev, void *data, @@ -76,10 +75,6 @@ static inline struct drm_agp_head *drm_agp_init(struct drm_device *dev) return NULL; }
-static inline void drm_agp_destroy(struct drm_agp_head *agp) -{ -}
static inline void drm_agp_clear(struct drm_device *dev) { } -- 1.8.4.rc3
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sun, Nov 03, 2013 at 02:43:44PM +0100, David Herrmann wrote:
Hi
On Sun, Nov 3, 2013 at 2:31 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Wrapping a kfree is pointless.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
I added that one dutifully to document the API in:
commit 28ec711cd427f8b61f73712a43b8100ba8ca933b Author: David Herrmann dh.herrmann@gmail.com Date: Sat Jul 27 16:37:00 2013 +0200
drm/agp: move AGP cleanup paths to drm_agpsupport.cBy removing it, we loose the comment on how to clean it up. Maybe we can copy the comment to drm_agp_init()? Other than that, I'm fine with it:
Hm, I haven't really noticed your original patch fly by. But I think agp setup/teardown is neatly fubar anyway, e.g. for modesetting drivers we seem to leak refcount on the agp backend driver.
Long term (if anyone actually cares) I think we should shovel all the legacy agp stuff somewhere far away and switch the modesetting drivers to using the agp system directly and making sure that stuff gets cleaned up with devres.c. -Daniel
Reviewed-by: David Herrmann dh.herrmann@gmail.com
Thanks David
drivers/gpu/drm/drm_agpsupport.c | 15 --------------- drivers/gpu/drm/drm_pci.c | 2 +- include/drm/drm_agpsupport.h | 5 ----- 3 files changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/drm_agpsupport.c b/drivers/gpu/drm/drm_agpsupport.c index 084a674e4b56..c5f663bb8bc1 100644 --- a/drivers/gpu/drm/drm_agpsupport.c +++ b/drivers/gpu/drm/drm_agpsupport.c @@ -460,21 +460,6 @@ void drm_agp_clear(struct drm_device *dev) }
/**
- 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_pci.c b/drivers/gpu/drm/drm_pci.c index 54eae6d83e5d..d0937dd62899 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -281,7 +281,7 @@ static void drm_pci_agp_destroy(struct drm_device *dev) if (dev->agp) { arch_phys_wc_del(dev->agp->agp_mtrr); drm_agp_clear(dev);
drm_agp_destroy(dev->agp);
kfree(dev->agp); dev->agp = NULL; }} diff --git a/include/drm/drm_agpsupport.h b/include/drm/drm_agpsupport.h index a12b0e011e44..56a861b2ceaa 100644 --- a/include/drm/drm_agpsupport.h +++ b/include/drm/drm_agpsupport.h @@ -20,7 +20,6 @@ DRM_AGP_MEM *drm_agp_bind_pages(struct drm_device *dev, uint32_t type);
struct drm_agp_head *drm_agp_init(struct drm_device *dev); -void drm_agp_destroy(struct drm_agp_head *agp); void drm_agp_clear(struct drm_device *dev); int drm_agp_acquire(struct drm_device *dev); int drm_agp_acquire_ioctl(struct drm_device *dev, void *data, @@ -76,10 +75,6 @@ static inline struct drm_agp_head *drm_agp_init(struct drm_device *dev) return NULL; }
-static inline void drm_agp_destroy(struct drm_agp_head *agp) -{ -}
static inline void drm_agp_clear(struct drm_device *dev) { } -- 1.8.4.rc3
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Call drm_pci_agp_destroy directly, there's no point in the indirection. Long term we want to shuffle this into each driver's unload logic, but that needs cleared-up drm lifetime rules first.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_pci.c | 3 +-- drivers/gpu/drm/drm_stub.c | 4 ++-- include/drm/drmP.h | 4 +--- 3 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index d0937dd62899..36e7f8e74027 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -276,7 +276,7 @@ static void drm_pci_agp_init(struct drm_device *dev) } }
-static void drm_pci_agp_destroy(struct drm_device *dev) +void drm_pci_agp_destroy(struct drm_device *dev) { if (dev->agp) { arch_phys_wc_del(dev->agp->agp_mtrr); @@ -292,7 +292,6 @@ static struct drm_bus drm_pci_bus = { .get_name = drm_pci_get_name, .set_busid = drm_pci_set_busid, .set_unique = drm_pci_set_unique, - .agp_destroy = drm_pci_agp_destroy, };
/** diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 4b25f693ae89..3ec8d4f9f09a 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -569,8 +569,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->agp) + drm_pci_agp_destroy(dev);
drm_vblank_cleanup(dev);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 8cf8cfef8c56..f67104aa7b51 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,9 +748,6 @@ 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); - };
/** @@ -1658,6 +1655,7 @@ static __inline__ int drm_pci_device_is_agp(struct drm_device *dev)
return pci_find_capability(dev->pdev, PCI_CAP_ID_AGP); } +void drm_pci_agp_destroy(struct drm_device *dev);
extern int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver); extern void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver);
Hi Daniel
On Sun, Nov 3, 2013 at 2:31 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Call drm_pci_agp_destroy directly, there's no point in the indirection. Long term we want to shuffle this into each driver's unload logic, but that needs cleared-up drm lifetime rules first.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_pci.c | 3 +-- drivers/gpu/drm/drm_stub.c | 4 ++-- include/drm/drmP.h | 4 +--- 3 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index d0937dd62899..36e7f8e74027 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -276,7 +276,7 @@ static void drm_pci_agp_init(struct drm_device *dev) } }
-static void drm_pci_agp_destroy(struct drm_device *dev) +void drm_pci_agp_destroy(struct drm_device *dev)
That one is #ifdef CONFIG_PCI So please provide a dummy below, same as for drm_pci_init().
If that's fixed:
Reviewed-by: David Herrmann dh.herrmann@gmail.com
Thanks David
{ if (dev->agp) { arch_phys_wc_del(dev->agp->agp_mtrr); @@ -292,7 +292,6 @@ static struct drm_bus drm_pci_bus = { .get_name = drm_pci_get_name, .set_busid = drm_pci_set_busid, .set_unique = drm_pci_set_unique,
.agp_destroy = drm_pci_agp_destroy,};
/** diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 4b25f693ae89..3ec8d4f9f09a 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -569,8 +569,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->agp)drm_pci_agp_destroy(dev); drm_vblank_cleanup(dev);diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 8cf8cfef8c56..f67104aa7b51 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,9 +748,6 @@ 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);};
/** @@ -1658,6 +1655,7 @@ static __inline__ int drm_pci_device_is_agp(struct drm_device *dev)
return pci_find_capability(dev->pdev, PCI_CAP_ID_AGP);} +void drm_pci_agp_destroy(struct drm_device *dev);
extern int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver); extern void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver); -- 1.8.4.rc3
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Call drm_pci_agp_destroy directly, there's no point in the indirection. Long term we want to shuffle this into each driver's unload logic, but that needs cleared-up drm lifetime rules first.
v2: Add a dummy function for !CONFIG_PCI, spotted my David Herrmann.
Reviewed-by: David Herrmann dh.herrmann@gmail.com Cc: David Herrmann dh.herrmann@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_pci.c | 4 ++-- drivers/gpu/drm/drm_stub.c | 4 ++-- include/drm/drmP.h | 4 +--- 3 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 71b84997abc5..2a6ac87b2e82 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -276,7 +276,7 @@ static void drm_pci_agp_init(struct drm_device *dev) } }
-static void drm_pci_agp_destroy(struct drm_device *dev) +void drm_pci_agp_destroy(struct drm_device *dev) { if (dev->agp) { arch_phys_wc_del(dev->agp->agp_mtrr); @@ -293,7 +293,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_destroy = drm_pci_agp_destroy, };
/** @@ -457,6 +456,7 @@ int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver) return -1; }
+void drm_pci_agp_destroy(struct drm_device *dev) { }; #endif
EXPORT_SYMBOL(drm_pci_init); diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 4b25f693ae89..3ec8d4f9f09a 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -569,8 +569,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->agp) + drm_pci_agp_destroy(dev);
drm_vblank_cleanup(dev);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 8cf8cfef8c56..f67104aa7b51 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,9 +748,6 @@ 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); - };
/** @@ -1658,6 +1655,7 @@ static __inline__ int drm_pci_device_is_agp(struct drm_device *dev)
return pci_find_capability(dev->pdev, PCI_CAP_ID_AGP); } +void drm_pci_agp_destroy(struct drm_device *dev);
extern int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver); extern void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver);
Hey Daniel
On Sun, Nov 3, 2013 at 3:24 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Call drm_pci_agp_destroy directly, there's no point in the indirection. Long term we want to shuffle this into each driver's unload logic, but that needs cleared-up drm lifetime rules first.
v2: Add a dummy function for !CONFIG_PCI, spotted my David Herrmann.
Reviewed-by: David Herrmann dh.herrmann@gmail.com Cc: David Herrmann dh.herrmann@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_pci.c | 4 ++-- drivers/gpu/drm/drm_stub.c | 4 ++-- include/drm/drmP.h | 4 +--- 3 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 71b84997abc5..2a6ac87b2e82 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -276,7 +276,7 @@ static void drm_pci_agp_init(struct drm_device *dev) } }
-static void drm_pci_agp_destroy(struct drm_device *dev) +void drm_pci_agp_destroy(struct drm_device *dev) { if (dev->agp) { arch_phys_wc_del(dev->agp->agp_mtrr); @@ -293,7 +293,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_destroy = drm_pci_agp_destroy,};
/** @@ -457,6 +456,7 @@ int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver) return -1; }
+void drm_pci_agp_destroy(struct drm_device *dev) { };
Looks good, but if you want to avoid getting a coding-style fixup patch in the next week from an eager linux-next tester, I'd recommend removing that unnecessary semicolon.
Thanks David
#endif
EXPORT_SYMBOL(drm_pci_init); diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 4b25f693ae89..3ec8d4f9f09a 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -569,8 +569,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->agp)drm_pci_agp_destroy(dev); drm_vblank_cleanup(dev);diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 8cf8cfef8c56..f67104aa7b51 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,9 +748,6 @@ 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);};
/** @@ -1658,6 +1655,7 @@ static __inline__ int drm_pci_device_is_agp(struct drm_device *dev)
return pci_find_capability(dev->pdev, PCI_CAP_ID_AGP);} +void drm_pci_agp_destroy(struct drm_device *dev);
extern int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver); extern void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver); -- 1.8.4.rc3
Call drm_pci_agp_destroy directly, there's no point in the indirection. Long term we want to shuffle this into each driver's unload logic, but that needs cleared-up drm lifetime rules first.
v2: Add a dummy function for !CONFIG_PCI, spotted my David Herrmann.
v3: Fixup for the coding style police.
Reviewed-by: David Herrmann dh.herrmann@gmail.com Cc: David Herrmann dh.herrmann@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_pci.c | 4 ++-- drivers/gpu/drm/drm_stub.c | 4 ++-- include/drm/drmP.h | 4 +--- 3 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 71b84997abc5..250c3b378bd6 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -276,7 +276,7 @@ static void drm_pci_agp_init(struct drm_device *dev) } }
-static void drm_pci_agp_destroy(struct drm_device *dev) +void drm_pci_agp_destroy(struct drm_device *dev) { if (dev->agp) { arch_phys_wc_del(dev->agp->agp_mtrr); @@ -293,7 +293,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_destroy = drm_pci_agp_destroy, };
/** @@ -457,6 +456,7 @@ int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver) return -1; }
+void drm_pci_agp_destroy(struct drm_device *dev) {} #endif
EXPORT_SYMBOL(drm_pci_init); diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 4b25f693ae89..3ec8d4f9f09a 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -569,8 +569,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->agp) + drm_pci_agp_destroy(dev);
drm_vblank_cleanup(dev);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 8cf8cfef8c56..f67104aa7b51 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,9 +748,6 @@ 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); - };
/** @@ -1658,6 +1655,7 @@ static __inline__ int drm_pci_device_is_agp(struct drm_device *dev)
return pci_find_capability(dev->pdev, PCI_CAP_ID_AGP); } +void drm_pci_agp_destroy(struct drm_device *dev);
extern int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver); extern void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver);
David Herrmann dutifully moved this locking along when moving the agp_init call out of the generic drm_dev_register into the pci specific load helpers.
But afaict there's no need and the reason for that locking has been purely a historical accident - we need the lock around the driver dev node registration to paper over the midlayer init races, and the agp init simply ended up in there. The real fix for all this is of course to delay the dev (and sysfs/debugfs) interface registration until everything is fully set up.
Until then stop the cargo-cult locking from spreading and remove the locking.
Cc: David Herrmann dh.herrmann@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_pci.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 36e7f8e74027..86a8484a1bf4 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -329,9 +329,7 @@ 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);
- mutex_lock(&drm_global_mutex); drm_pci_agp_init(dev); - mutex_unlock(&drm_global_mutex);
ret = drm_dev_register(dev, ent->driver_data); if (ret) @@ -349,9 +347,7 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, return 0;
err_agp: - mutex_lock(&drm_global_mutex); drm_pci_agp_destroy(dev); - mutex_unlock(&drm_global_mutex); pci_disable_device(pdev); err_free: drm_dev_free(dev);
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
David Herrmann -
Laurent Pinchart -
Sascha Hauer -
Thierry Reding