* [PATCH 1/2] drm: remove agp_init() bus callback
@ 2013-10-19 11:58 David Herrmann
2013-10-19 11:58 ` [PATCH 2/2] drm: rename agp_destroy() bus-cb to destroy() David Herrmann
2013-11-03 11:06 ` [PATCH 1/2] drm: remove agp_init() bus callback David Herrmann
0 siblings, 2 replies; 6+ messages in thread
From: David Herrmann @ 2013-10-19 11:58 UTC (permalink / raw)
To: dri-devel; +Cc: Daniel Vetter
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm: rename agp_destroy() bus-cb to destroy()
2013-10-19 11:58 [PATCH 1/2] drm: remove agp_init() bus callback David Herrmann
@ 2013-10-19 11:58 ` David Herrmann
2013-11-03 11:06 ` [PATCH 1/2] drm: remove agp_init() bus callback David Herrmann
1 sibling, 0 replies; 6+ messages in thread
From: David Herrmann @ 2013-10-19 11:58 UTC (permalink / raw)
To: dri-devel; +Cc: Daniel Vetter
While the only user of agp_destroy() is the PCI-bus for agp-destruction,
this callback is not strictly bound to agp. Rename to destroy() to make
clear that any bus can use it to destroy resources once the device is
removed.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
Hi
We currently cannot remove the agp_destroy() callback as we have no bus-specific
destroy helpers (compared to bus-specific init helpers). I think we can change
that once we get the revoke/unplug series I'm working on. Until then, we have to
keep the current callbacks.
Feel free to drop it, I just thought the "agp_*" prefix was weird..
Thanks
David
drivers/gpu/drm/drm_pci.c | 2 +-
drivers/gpu/drm/drm_stub.c | 4 ++--
include/drm/drmP.h | 3 +--
3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 7d3435c..3e1c3a0 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -299,7 +299,7 @@ static struct drm_bus drm_pci_bus = {
.set_busid = drm_pci_set_busid,
.set_unique = drm_pci_set_unique,
.irq_by_busid = drm_pci_irq_by_busid,
- .agp_destroy = drm_pci_agp_destroy,
+ .destroy = drm_pci_agp_destroy,
};
/**
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index ac211c3..c181b71 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -571,8 +571,8 @@ void drm_dev_unregister(struct drm_device *dev)
if (dev->driver->unload)
dev->driver->unload(dev);
- if (dev->driver->bus->agp_destroy)
- dev->driver->bus->agp_destroy(dev);
+ if (dev->driver->bus->destroy)
+ dev->driver->bus->destroy(dev);
drm_vblank_cleanup(dev);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index dfc44ae..220013d 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -749,8 +749,7 @@ struct drm_bus {
int (*set_unique)(struct drm_device *dev, struct drm_master *master,
struct drm_unique *unique);
int (*irq_by_busid)(struct drm_device *dev, struct drm_irq_busid *p);
- /* hooks that are for PCI */
- void (*agp_destroy)(struct drm_device *dev);
+ void (*destroy)(struct drm_device *dev);
};
--
1.8.4.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm: remove agp_init() bus callback
2013-10-19 11:58 [PATCH 1/2] drm: remove agp_init() bus callback David Herrmann
2013-10-19 11:58 ` [PATCH 2/2] drm: rename agp_destroy() bus-cb to destroy() David Herrmann
@ 2013-11-03 11:06 ` David Herrmann
2013-11-03 11:36 ` Daniel Vetter
1 sibling, 1 reply; 6+ messages in thread
From: David Herrmann @ 2013-11-03 11:06 UTC (permalink / raw)
To: dri-devel@lists.freedesktop.org; +Cc: Daniel Vetter
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm: remove agp_init() bus callback
2013-11-03 11:06 ` [PATCH 1/2] drm: remove agp_init() bus callback David Herrmann
@ 2013-11-03 11:36 ` Daniel Vetter
2013-11-03 11:43 ` David Herrmann
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2013-11-03 11:36 UTC (permalink / raw)
To: David Herrmann; +Cc: Daniel Vetter, dri-devel@lists.freedesktop.org
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
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm: remove agp_init() bus callback
2013-11-03 11:36 ` Daniel Vetter
@ 2013-11-03 11:43 ` David Herrmann
2013-11-03 12:31 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: David Herrmann @ 2013-11-03 11:43 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel@lists.freedesktop.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm: remove agp_init() bus callback
2013-11-03 11:43 ` David Herrmann
@ 2013-11-03 12:31 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2013-11-03 12:31 UTC (permalink / raw)
To: David Herrmann; +Cc: dri-devel@lists.freedesktop.org
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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-11-03 12:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-19 11:58 [PATCH 1/2] drm: remove agp_init() bus callback David Herrmann
2013-10-19 11:58 ` [PATCH 2/2] drm: rename agp_destroy() bus-cb to destroy() David Herrmann
2013-11-03 11:06 ` [PATCH 1/2] drm: remove agp_init() bus callback David Herrmann
2013-11-03 11:36 ` Daniel Vetter
2013-11-03 11:43 ` David Herrmann
2013-11-03 12:31 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox