On Fri, Jun 08, 2012 at 12:16:41PM +0300, Jani Nikula wrote:
Hi Daniel -
On Thu, 07 Jun 2012, Daniel Vetter daniel.vetter@ffwll.ch wrote:
I've done an audit of everything which is being set up between the place where drm_pci_agp_init is called currently and where the driver's ->load function is called. Nothing seems to depend upon dev->agp being set up correctly.
This is one big step into squashing this giant midlayer mistaken. Though one that drm/i915 is hurting especially: We don't need (and want) the agp layer on many chips, but we need to decide whether we want to enable it too early.
By moving the agp setup into the drivers control, we can do an intelligent decision in drm/i915 whether we need agp support (for anything ums and some horrible kms userspace on specific generations) or whether it's not required.
Also, this allows us to rip out a bit of the agp invasion from generic drm core code in follow-up patches.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_pci.c | 2 +- drivers/gpu/drm/drm_stub.c | 8 -------- drivers/gpu/drm/i810/i810_dma.c | 6 ++++++ drivers/gpu/drm/i915/i915_dma.c | 4 ++++ drivers/gpu/drm/mga/mga_dma.c | 4 ++++ drivers/gpu/drm/nouveau/nouveau_state.c | 4 ++++ drivers/gpu/drm/r128/r128_drv.c | 6 ++++++ drivers/gpu/drm/radeon/radeon_cp.c | 4 ++++ drivers/gpu/drm/radeon/radeon_kms.c | 4 ++++ drivers/gpu/drm/savage/savage_bci.c | 5 +++++ drivers/gpu/drm/sis/sis_drv.c | 5 +++++ drivers/gpu/drm/via/via_map.c | 4 ++++ include/drm/drmP.h | 5 +---- 13 files changed, 48 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 13f3d93..833e599 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -286,6 +286,7 @@ int drm_pci_agp_init(struct drm_device *dev) } return 0; } +EXPORT_SYMBOL(drm_pci_agp_init);
static struct drm_bus drm_pci_bus = { .bus_type = DRIVER_BUS_PCI, @@ -294,7 +295,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,
};
/** diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 21bcd4a..4a4a1ed 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -289,14 +289,6 @@ int drm_fill_in_dev(struct drm_device *dev,
dev->driver = driver;
- if (dev->driver->bus->agp_init) {
retcode = dev->driver->bus->agp_init(dev);if (retcode)goto error_out_unreg;- }
If something fails in drm_fill_in_dev() after ->agp_init(), the error path calls drm_lastclose(), which, IIUC, deallocates anything allocated through drm_pci_agp_init(). It's all buried in a bunch of layers, so I may be mistaken... But where's this cleanup done if any of the init fails after drm_pci_agp_init() in the driver load functions below?
Tbh I haven't check the failure paths to closely, presuming that given agp being optional and me just moving it a bit down I surely can't break stuff. And I've checked, I don't break things.
The real problem is that the error handling as-is is already pretty broken. I'll look into this. -Daniel
BR, Jani.
- retcode = drm_ctxbitmap_init(dev); if (retcode) { DRM_ERROR("Cannot allocate memory for context bitmap.\n");
diff --git a/drivers/gpu/drm/i810/i810_dma.c b/drivers/gpu/drm/i810/i810_dma.c index f920fb5..eed1126 100644 --- a/drivers/gpu/drm/i810/i810_dma.c +++ b/drivers/gpu/drm/i810/i810_dma.c @@ -1198,6 +1198,12 @@ static int i810_flip_bufs(struct drm_device *dev, void *data,
int i810_driver_load(struct drm_device *dev, unsigned long flags) {
- int ret;
- ret = drm_pci_agp_init(dev);
- if (ret)
return ret;- /* i810 has 4 more counters */ dev->counters += 4; dev->types[6] = _DRM_STAT_IRQ;
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 97a5a58..b97ef73 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1422,6 +1422,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) int ret = 0, mmio_bar; uint32_t aperture_size;
ret = drm_pci_agp_init(dev);
if (ret)
return ret;info = (struct intel_device_info *) flags;
/* Refuse to load on gen6+ without kms enabled. */
diff --git a/drivers/gpu/drm/mga/mga_dma.c b/drivers/gpu/drm/mga/mga_dma.c index 507aa3d..549816e 100644 --- a/drivers/gpu/drm/mga/mga_dma.c +++ b/drivers/gpu/drm/mga/mga_dma.c @@ -394,6 +394,10 @@ int mga_driver_load(struct drm_device *dev, unsigned long flags) drm_mga_private_t *dev_priv; int ret;
- ret = drm_pci_agp_init(dev);
- if (ret)
return ret;- dev_priv = kzalloc(sizeof(drm_mga_private_t), GFP_KERNEL); if (!dev_priv) return -ENOMEM;
diff --git a/drivers/gpu/drm/nouveau/nouveau_state.c b/drivers/gpu/drm/nouveau/nouveau_state.c index 19706f0..dbc881a 100644 --- a/drivers/gpu/drm/nouveau/nouveau_state.c +++ b/drivers/gpu/drm/nouveau/nouveau_state.c @@ -1029,6 +1029,10 @@ int nouveau_load(struct drm_device *dev, unsigned long flags) uint32_t reg0 = ~0, strap; int ret;
- ret = drm_pci_agp_init(dev);
- if (ret)
return ret;- dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL); if (!dev_priv) { ret = -ENOMEM;
diff --git a/drivers/gpu/drm/r128/r128_drv.c b/drivers/gpu/drm/r128/r128_drv.c index 88718fa..22001ca 100644 --- a/drivers/gpu/drm/r128/r128_drv.c +++ b/drivers/gpu/drm/r128/r128_drv.c @@ -85,6 +85,12 @@ static struct drm_driver driver = {
int r128_driver_load(struct drm_device *dev, unsigned long flags) {
- int ret;
- ret = drm_pci_agp_init(dev);
- if (ret)
return ret;- pci_set_master(dev->pdev); return drm_vblank_init(dev, 1);
} diff --git a/drivers/gpu/drm/radeon/radeon_cp.c b/drivers/gpu/drm/radeon/radeon_cp.c index ef67e18..1096296 100644 --- a/drivers/gpu/drm/radeon/radeon_cp.c +++ b/drivers/gpu/drm/radeon/radeon_cp.c @@ -2084,6 +2084,10 @@ int radeon_driver_load(struct drm_device *dev, unsigned long flags) drm_radeon_private_t *dev_priv; int ret = 0;
- ret = drm_pci_agp_init(dev);
- if (ret)
return ret;- dev_priv = kzalloc(sizeof(drm_radeon_private_t), GFP_KERNEL); if (dev_priv == NULL) return -ENOMEM;
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index f1016a5..61ab2fd 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -51,6 +51,10 @@ int radeon_driver_load_kms(struct drm_device *dev, unsigned long flags) struct radeon_device *rdev; int r, acpi_status;
- r = drm_pci_agp_init(dev);
- if (r)
return r;- rdev = kzalloc(sizeof(struct radeon_device), GFP_KERNEL); if (rdev == NULL) { return -ENOMEM;
diff --git a/drivers/gpu/drm/savage/savage_bci.c b/drivers/gpu/drm/savage/savage_bci.c index 6eb507a..6d70c4b 100644 --- a/drivers/gpu/drm/savage/savage_bci.c +++ b/drivers/gpu/drm/savage/savage_bci.c @@ -538,6 +538,11 @@ static void savage_fake_dma_flush(drm_savage_private_t * dev_priv) int savage_driver_load(struct drm_device *dev, unsigned long chipset) { drm_savage_private_t *dev_priv;
int ret;
ret = drm_pci_agp_init(dev);
if (ret)
return ret;dev_priv = kzalloc(sizeof(drm_savage_private_t), GFP_KERNEL); if (dev_priv == NULL)
diff --git a/drivers/gpu/drm/sis/sis_drv.c b/drivers/gpu/drm/sis/sis_drv.c index 30d98d1..9726ee0 100644 --- a/drivers/gpu/drm/sis/sis_drv.c +++ b/drivers/gpu/drm/sis/sis_drv.c @@ -40,6 +40,11 @@ static struct pci_device_id pciidlist[] = { static int sis_driver_load(struct drm_device *dev, unsigned long chipset) { drm_sis_private_t *dev_priv;
int ret;
ret = drm_pci_agp_init(dev);
if (ret)
return ret;pci_set_master(dev->pdev);
diff --git a/drivers/gpu/drm/via/via_map.c b/drivers/gpu/drm/via/via_map.c index 1f18225..5ab53bf 100644 --- a/drivers/gpu/drm/via/via_map.c +++ b/drivers/gpu/drm/via/via_map.c @@ -96,6 +96,10 @@ int via_driver_load(struct drm_device *dev, unsigned long chipset) drm_via_private_t *dev_priv; int ret = 0;
- ret = drm_pci_agp_init(dev);
- if (ret)
return ret;- dev_priv = kzalloc(sizeof(drm_via_private_t), GFP_KERNEL); if (dev_priv == NULL) return -ENOMEM;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 31ad880..e3437da 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -726,9 +726,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 */
- int (*agp_init)(struct drm_device *dev);
};
/** @@ -1754,6 +1751,7 @@ static __inline__ int drm_pci_device_is_agp(struct drm_device *dev)
return pci_find_capability(dev->pdev, PCI_CAP_ID_AGP); } +extern int drm_pci_agp_init(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); @@ -1761,7 +1759,6 @@ extern int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, struct drm_driver *driver);
/* 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); -- 1.7.7.6
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx