| Message ID | 1374673438-26251-4-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 24, 2013 at 11:43 PM, David Herrmann <dh.herrmann@gmail.com> wrote: > Introduce two new helpers, drm_agp_clear() and drm_agp_destroy() which > clear all AGP mappings and destroy the AGP head. This allows to reduce the > AGP code in core DRM and move it all to drm_agpsupport.c. > Could we do this as the first patch? it seems like it would be a nice thing to have standalone even with the current init paths. Dave. > Also use these new helpers to clean up AGP allocations in the > drm_dev_register() error path. > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > --- > drivers/gpu/drm/drm_agpsupport.c | 51 ++++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_drv.c | 21 +---------------- > drivers/gpu/drm/drm_pci.c | 12 ++++++++++ > drivers/gpu/drm/drm_stub.c | 12 ++++------ > include/drm/drmP.h | 3 +++ > 5 files changed, 71 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/drm_agpsupport.c b/drivers/gpu/drm/drm_agpsupport.c > index 3d8fed1..e301d65 100644 > --- a/drivers/gpu/drm/drm_agpsupport.c > +++ b/drivers/gpu/drm/drm_agpsupport.c > @@ -424,6 +424,57 @@ struct drm_agp_head *drm_agp_init(struct drm_device *dev) > } > > /** > + * drm_agp_clear - Clear AGP resource list > + * @dev: DRM device > + * > + * Iterate over all AGP resources and remove them. But keep the AGP head > + * intact so it can still be used. It is safe to call this if AGP is disabled or > + * was already removed. > + * > + * If DRIVER_MODESET is active, nothing is done to protect the modesetting > + * resources from getting destroyed. Drivers are responsible of cleaning them up > + * during device shutdown. > + */ > +void drm_agp_clear(struct drm_device *dev) > +{ > + struct drm_agp_mem *entry, *tempe; > + > + if (!drm_core_has_AGP(dev) || !dev->agp) > + return; > + if (drm_core_check_feature(dev, DRIVER_MODESET)) > + return; > + > + list_for_each_entry_safe(entry, tempe, &dev->agp->memory, head) { > + if (entry->bound) > + drm_unbind_agp(entry->memory); > + drm_free_agp(entry->memory, entry->pages); > + kfree(entry); > + } > + INIT_LIST_HEAD(&dev->agp->memory); > + > + if (dev->agp->acquired) > + drm_agp_release(dev); > + > + dev->agp->acquired = 0; > + dev->agp->enabled = 0; > +} > + > +/** > + * drm_agp_destroy - Destroy AGP head > + * @dev: DRM device > + * > + * Destroy resources that were previously allocated via drm_agp_initp. Caller > + * must ensure to clean up all AGP resources before calling this. See > + * drm_agp_clear(). > + * > + * Call this to destroy AGP heads allocated via drm_agp_init(). > + */ > +void drm_agp_destroy(struct drm_agp_head *agp) > +{ > + kfree(agp); > +} > + > +/** > * Binds a collection of pages into AGP memory at the given offset, returning > * the AGP memory structure containing them. > * > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 36103d1..dddd799 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -195,27 +195,8 @@ int drm_lastclose(struct drm_device * dev) > > mutex_lock(&dev->struct_mutex); > > - /* Clear AGP information */ > - if (drm_core_has_AGP(dev) && dev->agp && > - !drm_core_check_feature(dev, DRIVER_MODESET)) { > - struct drm_agp_mem *entry, *tempe; > - > - /* Remove AGP resources, but leave dev->agp > - intact until drv_cleanup is called. */ > - list_for_each_entry_safe(entry, tempe, &dev->agp->memory, head) { > - if (entry->bound) > - drm_unbind_agp(entry->memory); > - drm_free_agp(entry->memory, entry->pages); > - kfree(entry); > - } > - INIT_LIST_HEAD(&dev->agp->memory); > - > - if (dev->agp->acquired) > - drm_agp_release(dev); > + drm_agp_clear(dev); > > - dev->agp->acquired = 0; > - dev->agp->enabled = 0; > - } > if (drm_core_check_feature(dev, DRIVER_SG) && dev->sg && > !drm_core_check_feature(dev, DRIVER_MODESET)) { > drm_sg_cleanup(dev->sg); > diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c > index ab861ca..0daee24 100644 > --- a/drivers/gpu/drm/drm_pci.c > +++ b/drivers/gpu/drm/drm_pci.c > @@ -283,6 +283,17 @@ static int drm_pci_agp_init(struct drm_device *dev) > return 0; > } > > +static void drm_pci_agp_destroy(struct drm_device *dev) > +{ > + if (drm_core_has_AGP(dev) && dev->agp) { > + if (drm_core_has_MTRR(dev)) > + arch_phys_wc_del(dev->agp->agp_mtrr); > + drm_agp_clear(dev); > + drm_agp_destroy(dev->agp); > + dev->agp = NULL; > + } > +} > + > static struct drm_bus drm_pci_bus = { > .bus_type = DRIVER_BUS_PCI, > .get_irq = drm_pci_get_irq, > @@ -291,6 +302,7 @@ static struct drm_bus drm_pci_bus = { > .set_unique = drm_pci_set_unique, > .irq_by_busid = drm_pci_irq_by_busid, > .agp_init = drm_pci_agp_init, > + .agp_destroy = drm_pci_agp_destroy, > }; > > /** > diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c > index bc1d860..f016ed8 100644 > --- a/drivers/gpu/drm/drm_stub.c > +++ b/drivers/gpu/drm/drm_stub.c > @@ -387,16 +387,11 @@ void drm_put_dev(struct drm_device *dev) > > drm_lastclose(dev); > > - if (drm_core_has_MTRR(dev) && drm_core_has_AGP(dev) && dev->agp) > - arch_phys_wc_del(dev->agp->agp_mtrr); > - > if (dev->driver->unload) > dev->driver->unload(dev); > > - if (drm_core_has_AGP(dev) && dev->agp) { > - kfree(dev->agp); > - dev->agp = NULL; > - } > + if (dev->driver->bus->agp_destroy) > + dev->driver->bus->agp_destroy(dev); > > drm_vblank_cleanup(dev); > > @@ -575,7 +570,8 @@ err_control_node: > if (drm_core_check_feature(dev, DRIVER_MODESET)) > drm_put_minor(&dev->control); > err_agp: > - /* FIXME: cleanup AGP leftovers */ > + if (dev->driver->bus->agp_destroy) > + dev->driver->bus->agp_destroy(dev); > out_unlock: > mutex_unlock(&drm_global_mutex); > return ret; > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index ea5e130..30f83ee 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -737,6 +737,7 @@ struct drm_bus { > int (*irq_by_busid)(struct drm_device *dev, struct drm_irq_busid *p); > /* hooks that are for PCI */ > int (*agp_init)(struct drm_device *dev); > + void (*agp_destroy)(struct drm_device *dev); > > }; > > @@ -1454,6 +1455,8 @@ extern int drm_modeset_ctl(struct drm_device *dev, void *data, > > /* AGP/GART support (drm_agpsupport.h) */ > extern struct drm_agp_head *drm_agp_init(struct drm_device *dev); > +extern void drm_agp_destroy(struct drm_agp_head *agp); > +extern void drm_agp_clear(struct drm_device *dev); > extern int drm_agp_acquire(struct drm_device *dev); > extern int drm_agp_acquire_ioctl(struct drm_device *dev, void *data, > struct drm_file *file_priv); > -- > 1.8.3.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi On Sat, Jul 27, 2013 at 8:05 AM, Dave Airlie <airlied@gmail.com> wrote: > On Wed, Jul 24, 2013 at 11:43 PM, David Herrmann <dh.herrmann@gmail.com> wrote: >> Introduce two new helpers, drm_agp_clear() and drm_agp_destroy() which >> clear all AGP mappings and destroy the AGP head. This allows to reduce the >> AGP code in core DRM and move it all to drm_agpsupport.c. >> > > Could we do this as the first patch? it seems like it would be a nice > thing to have standalone even with the current init paths. I can do that. But note the error paths in all DRM bus drivers are currently non-existent and I'd like to avoid adding them just to remove them with drm_dev_free() again. And non-existent error-paths means we don't catch AGP errors, except in drm_fill_in_dev(). But I guess the "->agp_destroy()" cleanup is still nicer than the current approach. I will send the patch standalone as the rest of the series needs more work, anyway. Cheers David
diff --git a/drivers/gpu/drm/drm_agpsupport.c b/drivers/gpu/drm/drm_agpsupport.c index 3d8fed1..e301d65 100644 --- a/drivers/gpu/drm/drm_agpsupport.c +++ b/drivers/gpu/drm/drm_agpsupport.c @@ -424,6 +424,57 @@ struct drm_agp_head *drm_agp_init(struct drm_device *dev) } /** + * drm_agp_clear - Clear AGP resource list + * @dev: DRM device + * + * Iterate over all AGP resources and remove them. But keep the AGP head + * intact so it can still be used. It is safe to call this if AGP is disabled or + * was already removed. + * + * If DRIVER_MODESET is active, nothing is done to protect the modesetting + * resources from getting destroyed. Drivers are responsible of cleaning them up + * during device shutdown. + */ +void drm_agp_clear(struct drm_device *dev) +{ + struct drm_agp_mem *entry, *tempe; + + if (!drm_core_has_AGP(dev) || !dev->agp) + return; + if (drm_core_check_feature(dev, DRIVER_MODESET)) + return; + + list_for_each_entry_safe(entry, tempe, &dev->agp->memory, head) { + if (entry->bound) + drm_unbind_agp(entry->memory); + drm_free_agp(entry->memory, entry->pages); + kfree(entry); + } + INIT_LIST_HEAD(&dev->agp->memory); + + if (dev->agp->acquired) + drm_agp_release(dev); + + dev->agp->acquired = 0; + dev->agp->enabled = 0; +} + +/** + * drm_agp_destroy - Destroy AGP head + * @dev: DRM device + * + * Destroy resources that were previously allocated via drm_agp_initp. Caller + * must ensure to clean up all AGP resources before calling this. See + * drm_agp_clear(). + * + * Call this to destroy AGP heads allocated via drm_agp_init(). + */ +void drm_agp_destroy(struct drm_agp_head *agp) +{ + kfree(agp); +} + +/** * Binds a collection of pages into AGP memory at the given offset, returning * the AGP memory structure containing them. * diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 36103d1..dddd799 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -195,27 +195,8 @@ int drm_lastclose(struct drm_device * dev) mutex_lock(&dev->struct_mutex); - /* Clear AGP information */ - if (drm_core_has_AGP(dev) && dev->agp && - !drm_core_check_feature(dev, DRIVER_MODESET)) { - struct drm_agp_mem *entry, *tempe; - - /* Remove AGP resources, but leave dev->agp - intact until drv_cleanup is called. */ - list_for_each_entry_safe(entry, tempe, &dev->agp->memory, head) { - if (entry->bound) - drm_unbind_agp(entry->memory); - drm_free_agp(entry->memory, entry->pages); - kfree(entry); - } - INIT_LIST_HEAD(&dev->agp->memory); - - if (dev->agp->acquired) - drm_agp_release(dev); + drm_agp_clear(dev); - dev->agp->acquired = 0; - dev->agp->enabled = 0; - } if (drm_core_check_feature(dev, DRIVER_SG) && dev->sg && !drm_core_check_feature(dev, DRIVER_MODESET)) { drm_sg_cleanup(dev->sg); diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index ab861ca..0daee24 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -283,6 +283,17 @@ static int drm_pci_agp_init(struct drm_device *dev) return 0; } +static void drm_pci_agp_destroy(struct drm_device *dev) +{ + if (drm_core_has_AGP(dev) && dev->agp) { + if (drm_core_has_MTRR(dev)) + arch_phys_wc_del(dev->agp->agp_mtrr); + drm_agp_clear(dev); + drm_agp_destroy(dev->agp); + dev->agp = NULL; + } +} + static struct drm_bus drm_pci_bus = { .bus_type = DRIVER_BUS_PCI, .get_irq = drm_pci_get_irq, @@ -291,6 +302,7 @@ static struct drm_bus drm_pci_bus = { .set_unique = drm_pci_set_unique, .irq_by_busid = drm_pci_irq_by_busid, .agp_init = drm_pci_agp_init, + .agp_destroy = drm_pci_agp_destroy, }; /** diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index bc1d860..f016ed8 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -387,16 +387,11 @@ void drm_put_dev(struct drm_device *dev) drm_lastclose(dev); - if (drm_core_has_MTRR(dev) && drm_core_has_AGP(dev) && dev->agp) - arch_phys_wc_del(dev->agp->agp_mtrr); - if (dev->driver->unload) dev->driver->unload(dev); - if (drm_core_has_AGP(dev) && dev->agp) { - kfree(dev->agp); - dev->agp = NULL; - } + if (dev->driver->bus->agp_destroy) + dev->driver->bus->agp_destroy(dev); drm_vblank_cleanup(dev); @@ -575,7 +570,8 @@ err_control_node: if (drm_core_check_feature(dev, DRIVER_MODESET)) drm_put_minor(&dev->control); err_agp: - /* FIXME: cleanup AGP leftovers */ + if (dev->driver->bus->agp_destroy) + dev->driver->bus->agp_destroy(dev); out_unlock: mutex_unlock(&drm_global_mutex); return ret; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index ea5e130..30f83ee 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -737,6 +737,7 @@ struct drm_bus { int (*irq_by_busid)(struct drm_device *dev, struct drm_irq_busid *p); /* hooks that are for PCI */ int (*agp_init)(struct drm_device *dev); + void (*agp_destroy)(struct drm_device *dev); }; @@ -1454,6 +1455,8 @@ extern int drm_modeset_ctl(struct drm_device *dev, void *data, /* AGP/GART support (drm_agpsupport.h) */ extern struct drm_agp_head *drm_agp_init(struct drm_device *dev); +extern void drm_agp_destroy(struct drm_agp_head *agp); +extern void drm_agp_clear(struct drm_device *dev); extern int drm_agp_acquire(struct drm_device *dev); extern int drm_agp_acquire_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv);
Introduce two new helpers, drm_agp_clear() and drm_agp_destroy() which clear all AGP mappings and destroy the AGP head. This allows to reduce the AGP code in core DRM and move it all to drm_agpsupport.c. Also use these new helpers to clean up AGP allocations in the drm_dev_register() error path. Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- drivers/gpu/drm/drm_agpsupport.c | 51 ++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_drv.c | 21 +---------------- drivers/gpu/drm/drm_pci.c | 12 ++++++++++ drivers/gpu/drm/drm_stub.c | 12 ++++------ include/drm/drmP.h | 3 +++ 5 files changed, 71 insertions(+), 28 deletions(-)