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