* [PATCH 2/4] drm/i915: initialize the context idr unconditionally
2012-06-19 14:52 [PATCH 1/4] drm/i915: fix module unload after context merge Daniel Vetter
@ 2012-06-19 14:52 ` Daniel Vetter
2012-06-19 17:43 ` Ben Widawsky
2012-06-19 14:52 ` [PATCH 3/4] drm/i915: return -ENOENT if the context doesn't exist Daniel Vetter
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2012-06-19 14:52 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Ben Widawsky
It doesn't hurt and it at least prevents us from OOPSing left and
right at quite a few places. This also allows us to simplify the code
a bit by folding the only line of context_open into the callsite.
We obviuosly also need to run the cleanup code unconditionally, too.
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_dma.c | 2 +-
drivers/gpu/drm/i915/i915_drv.h | 1 -
drivers/gpu/drm/i915/i915_gem_context.c | 15 ---------------
3 files changed, 1 insertion(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index a47ed44..e36f6ce 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1766,7 +1766,7 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file)
spin_lock_init(&file_priv->mm.lock);
INIT_LIST_HEAD(&file_priv->mm.request_list);
- i915_gem_context_open(dev, file);
+ idr_init(&file_priv->context_idr);
return 0;
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b65d156..d1b4950 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1392,7 +1392,6 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
/* i915_gem_context.c */
void i915_gem_context_init(struct drm_device *dev);
void i915_gem_context_fini(struct drm_device *dev);
-void i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
int i915_switch_context(struct intel_ring_buffer *ring,
struct drm_file *file, int to_id);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 693718b..671927a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -282,17 +282,6 @@ void i915_gem_context_fini(struct drm_device *dev)
do_destroy(dev_priv->ring[RCS].default_context);
}
-void i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
-{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct drm_i915_file_private *file_priv = file->driver_priv;
-
- if (dev_priv->hw_contexts_disabled)
- return;
-
- idr_init(&file_priv->context_idr);
-}
-
static int context_idr_cleanup(int id, void *p, void *data)
{
struct drm_file *file = (struct drm_file *)data;
@@ -311,12 +300,8 @@ static int context_idr_cleanup(int id, void *p, void *data)
void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_i915_file_private *file_priv = file->driver_priv;
- if (dev_priv->hw_contexts_disabled)
- return;
-
mutex_lock(&dev->struct_mutex);
idr_for_each(&file_priv->context_idr, context_idr_cleanup, file);
idr_destroy(&file_priv->context_idr);
--
1.7.10
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 2/4] drm/i915: initialize the context idr unconditionally
2012-06-19 14:52 ` [PATCH 2/4] drm/i915: initialize the context idr unconditionally Daniel Vetter
@ 2012-06-19 17:43 ` Ben Widawsky
0 siblings, 0 replies; 11+ messages in thread
From: Ben Widawsky @ 2012-06-19 17:43 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
On Tue, 19 Jun 2012 16:52:30 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> It doesn't hurt and it at least prevents us from OOPSing left and
> right at quite a few places. This also allows us to simplify the code
> a bit by folding the only line of context_open into the callsite.
>
> We obviuosly also need to run the cleanup code unconditionally, too.
>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Open did a couple more things in previous incantations. I liked the idea
of having a discrete open function, so it's easy, and obvious where to
add stuff when needed. It also adds symmetry for close. One such use for
open in the past was to shoehorn the default context as a locatable
context per fd. This eliminated the need for having special
DEFAULT_CONTEXT_ID conditions.
Anyway, I like the fact that this fixes bugs, but dislike the fact that
open went away. However, since you found, and fixed the issue, I'll
defer to your wishes.
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 1 -
> drivers/gpu/drm/i915/i915_gem_context.c | 15 ---------------
> 3 files changed, 1 insertion(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index a47ed44..e36f6ce 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1766,7 +1766,7 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file)
> spin_lock_init(&file_priv->mm.lock);
> INIT_LIST_HEAD(&file_priv->mm.request_list);
>
> - i915_gem_context_open(dev, file);
> + idr_init(&file_priv->context_idr);
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b65d156..d1b4950 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1392,7 +1392,6 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
> /* i915_gem_context.c */
> void i915_gem_context_init(struct drm_device *dev);
> void i915_gem_context_fini(struct drm_device *dev);
> -void i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
> void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
> int i915_switch_context(struct intel_ring_buffer *ring,
> struct drm_file *file, int to_id);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 693718b..671927a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -282,17 +282,6 @@ void i915_gem_context_fini(struct drm_device *dev)
> do_destroy(dev_priv->ring[RCS].default_context);
> }
>
> -void i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
> -{
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_i915_file_private *file_priv = file->driver_priv;
> -
> - if (dev_priv->hw_contexts_disabled)
> - return;
> -
> - idr_init(&file_priv->context_idr);
> -}
> -
> static int context_idr_cleanup(int id, void *p, void *data)
> {
> struct drm_file *file = (struct drm_file *)data;
> @@ -311,12 +300,8 @@ static int context_idr_cleanup(int id, void *p, void *data)
>
> void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_i915_file_private *file_priv = file->driver_priv;
>
> - if (dev_priv->hw_contexts_disabled)
> - return;
> -
> mutex_lock(&dev->struct_mutex);
> idr_for_each(&file_priv->context_idr, context_idr_cleanup, file);
> idr_destroy(&file_priv->context_idr);
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/4] drm/i915: return -ENOENT if the context doesn't exist
2012-06-19 14:52 [PATCH 1/4] drm/i915: fix module unload after context merge Daniel Vetter
2012-06-19 14:52 ` [PATCH 2/4] drm/i915: initialize the context idr unconditionally Daniel Vetter
@ 2012-06-19 14:52 ` Daniel Vetter
2012-06-19 16:19 ` Ben Widawsky
2012-06-19 14:52 ` [PATCH 4/4] drm/i915/context: shut up compiler Daniel Vetter
2012-06-19 17:32 ` [PATCH 1/4] drm/i915: fix module unload after context merge Ben Widawsky
3 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2012-06-19 14:52 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Ben Widawsky
This is our customary "no such object" errno, not -EINVAL.
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_gem_context.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 671927a..3d8ae6c 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -455,7 +455,7 @@ int i915_switch_context(struct intel_ring_buffer *ring,
} else {
to = i915_gem_context_get(file_priv, to_id);
if (to == NULL)
- return -EINVAL;
+ return -ENOENT;
}
if (from_obj == to->obj)
@@ -521,7 +521,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
ctx = i915_gem_context_get(file_priv, args->ctx_id);
if (!ctx) {
mutex_unlock(&dev->struct_mutex);
- return -EINVAL;
+ return -ENOENT;
}
do_destroy(ctx);
--
1.7.10
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 3/4] drm/i915: return -ENOENT if the context doesn't exist
2012-06-19 14:52 ` [PATCH 3/4] drm/i915: return -ENOENT if the context doesn't exist Daniel Vetter
@ 2012-06-19 16:19 ` Ben Widawsky
2012-06-19 16:27 ` Daniel Vetter
0 siblings, 1 reply; 11+ messages in thread
From: Ben Widawsky @ 2012-06-19 16:19 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
On Tue, 19 Jun 2012 16:52:31 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> This is our customary "no such object" errno, not -EINVAL.
>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
You've missed idr_cleanup for this. That doesn't get back down to
userspace, but it keeps things consistent.
With that, this is
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_gem_context.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 671927a..3d8ae6c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -455,7 +455,7 @@ int i915_switch_context(struct intel_ring_buffer *ring,
> } else {
> to = i915_gem_context_get(file_priv, to_id);
> if (to == NULL)
> - return -EINVAL;
> + return -ENOENT;
> }
>
> if (from_obj == to->obj)
> @@ -521,7 +521,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> ctx = i915_gem_context_get(file_priv, args->ctx_id);
> if (!ctx) {
> mutex_unlock(&dev->struct_mutex);
> - return -EINVAL;
> + return -ENOENT;
> }
>
> do_destroy(ctx);
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/4] drm/i915: return -ENOENT if the context doesn't exist
2012-06-19 16:19 ` Ben Widawsky
@ 2012-06-19 16:27 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2012-06-19 16:27 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Daniel Vetter, Intel Graphics Development
On Tue, Jun 19, 2012 at 09:19:11AM -0700, Ben Widawsky wrote:
> On Tue, 19 Jun 2012 16:52:31 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> > This is our customary "no such object" errno, not -EINVAL.
> >
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> You've missed idr_cleanup for this. That doesn't get back down to
> userspace, but it keeps things consistent.
Yeah, I've noticed the -ENXIO in the cleanup code. I think we can
completely rip that one out (and it won't ever reach userspace anyway), so
I've decided to ingore that one. I'll stitch toghet that patch quickly.
-Daniel
>
> With that, this is
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> > drivers/gpu/drm/i915/i915_gem_context.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 671927a..3d8ae6c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -455,7 +455,7 @@ int i915_switch_context(struct intel_ring_buffer *ring,
> > } else {
> > to = i915_gem_context_get(file_priv, to_id);
> > if (to == NULL)
> > - return -EINVAL;
> > + return -ENOENT;
> > }
> >
> > if (from_obj == to->obj)
> > @@ -521,7 +521,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> > ctx = i915_gem_context_get(file_priv, args->ctx_id);
> > if (!ctx) {
> > mutex_unlock(&dev->struct_mutex);
> > - return -EINVAL;
> > + return -ENOENT;
> > }
> >
> > do_destroy(ctx);
>
>
>
> --
> Ben Widawsky, Intel Open Source Technology Center
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] drm/i915/context: shut up compiler
2012-06-19 14:52 [PATCH 1/4] drm/i915: fix module unload after context merge Daniel Vetter
2012-06-19 14:52 ` [PATCH 2/4] drm/i915: initialize the context idr unconditionally Daniel Vetter
2012-06-19 14:52 ` [PATCH 3/4] drm/i915: return -ENOENT if the context doesn't exist Daniel Vetter
@ 2012-06-19 14:52 ` Daniel Vetter
2012-06-19 16:16 ` Ben Widawsky
2012-06-19 17:32 ` [PATCH 1/4] drm/i915: fix module unload after context merge Ben Widawsky
3 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2012-06-19 14:52 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Ben Widawsky
It found some unused variables.
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_gem_context.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 3d8ae6c..fe3f21f 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -480,7 +480,6 @@ int i915_switch_context(struct intel_ring_buffer *ring,
int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
struct drm_file *file)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_i915_gem_context_create *args = data;
struct drm_i915_file_private *file_priv = file->driver_priv;
struct i915_hw_context *ctx;
@@ -507,7 +506,6 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
{
struct drm_i915_gem_context_destroy *args = data;
struct drm_i915_file_private *file_priv = file->driver_priv;
- struct drm_i915_private *dev_priv = dev->dev_private;
struct i915_hw_context *ctx;
int ret;
--
1.7.10
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 4/4] drm/i915/context: shut up compiler
2012-06-19 14:52 ` [PATCH 4/4] drm/i915/context: shut up compiler Daniel Vetter
@ 2012-06-19 16:16 ` Ben Widawsky
0 siblings, 0 replies; 11+ messages in thread
From: Ben Widawsky @ 2012-06-19 16:16 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
On Tue, 19 Jun 2012 16:52:32 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> It found some unused variables.
>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
I saw this recently as well. I know exactly when those variables stopped
being used too.
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_gem_context.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 3d8ae6c..fe3f21f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -480,7 +480,6 @@ int i915_switch_context(struct intel_ring_buffer *ring,
> int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_i915_gem_context_create *args = data;
> struct drm_i915_file_private *file_priv = file->driver_priv;
> struct i915_hw_context *ctx;
> @@ -507,7 +506,6 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> {
> struct drm_i915_gem_context_destroy *args = data;
> struct drm_i915_file_private *file_priv = file->driver_priv;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> struct i915_hw_context *ctx;
> int ret;
>
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] drm/i915: fix module unload after context merge
2012-06-19 14:52 [PATCH 1/4] drm/i915: fix module unload after context merge Daniel Vetter
` (2 preceding siblings ...)
2012-06-19 14:52 ` [PATCH 4/4] drm/i915/context: shut up compiler Daniel Vetter
@ 2012-06-19 17:32 ` Ben Widawsky
2012-06-19 19:55 ` [PATCH] " Daniel Vetter
3 siblings, 1 reply; 11+ messages in thread
From: Ben Widawsky @ 2012-06-19 17:32 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
On Tue, 19 Jun 2012 16:52:29 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> commit 8e96d9c4d9843f00ebeb4a9b33596d96602ea101
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date: Mon Jun 4 14:42:56 2012 -0700
>
> drm/i915: reset the GPU on context fini
>
> broke module unload because it reset the gpu before we've stopped
> touching it. Later on in the unload sequence the ringbuffer code
> complained that the gpu would idle properly (because intel_gpu_reset
> only resets the hw and not our sw state).
>
> Hence we need to reset the gpu hw as the very last thing before we
> unmap the register mmio space.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=51183
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
One thing which is both "wrong" with this patch, and the existing code
is it does the reset after the default context has been destroyed.
Ideally we want to reset before we make the object go away, as it's the
true way of getting the GPU to stop referencing the object.
It's *probably* fine that we do this in the "wrong" fashion as long as
we can avoid doing a context switch... which I think we can only
guarantee if we hold forcewake for the duration of unload, or don't have
RC6 enabled.
> ---
> drivers/gpu/drm/i915/i915_dma.c | 7 +++++++
> drivers/gpu/drm/i915/i915_gem_context.c | 2 --
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index f1544c5..a47ed44 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1731,6 +1731,13 @@ int i915_driver_unload(struct drm_device *dev)
> i915_free_hws(dev);
> }
>
> +
> + /* The only known way to stop the gpu from accessing the hw context is
> + * to reset it. Do this as the very last operation to avoid confusing
> + * other code, leading to spurious errors. */
> + if (!dev_priv->hw_contexts_disabled)
> + intel_gpu_reset(dev);
> +
> if (dev_priv->regs != NULL)
> pci_iounmap(dev->pdev, dev_priv->regs);
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 8fb8cd8..693718b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -280,8 +280,6 @@ void i915_gem_context_fini(struct drm_device *dev)
> i915_gem_object_unpin(dev_priv->ring[RCS].default_context->obj);
>
> do_destroy(dev_priv->ring[RCS].default_context);
> -
> - intel_gpu_reset(dev);
> }
>
> void i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH] drm/i915: fix module unload after context merge
2012-06-19 17:32 ` [PATCH 1/4] drm/i915: fix module unload after context merge Ben Widawsky
@ 2012-06-19 19:55 ` Daniel Vetter
2012-06-19 21:19 ` Ben Widawsky
0 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2012-06-19 19:55 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Ben Widawsky
commit 8e96d9c4d9843f00ebeb4a9b33596d96602ea101
Author: Ben Widawsky <ben@bwidawsk.net>
Date: Mon Jun 4 14:42:56 2012 -0700
drm/i915: reset the GPU on context fini
broke module unload because it reset the gpu before we've stopped
touching it. Later on in the unload sequence the ringbuffer code
complained that the gpu would idle properly (because intel_gpu_reset
only resets the hw and not our sw state).
v2: Reorder things so that we reset the gpu _before_ we release the
backing storage of the default context.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=51183
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_dma.c | 2 +-
drivers/gpu/drm/i915/i915_gem_context.c | 7 +++++--
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index f1544c5..efba4e8 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1669,7 +1669,6 @@ int i915_driver_unload(struct drm_device *dev)
if (ret)
DRM_ERROR("failed to idle hardware: %d\n", ret);
i915_gem_retire_requests(dev);
- i915_gem_context_fini(dev);
mutex_unlock(&dev->struct_mutex);
/* Cancel the retire work handler, which should be idle now. */
@@ -1720,6 +1719,7 @@ int i915_driver_unload(struct drm_device *dev)
mutex_lock(&dev->struct_mutex);
i915_gem_free_all_phys_object(dev);
i915_gem_cleanup_ringbuffer(dev);
+ i915_gem_context_fini(dev);
mutex_unlock(&dev->struct_mutex);
i915_gem_cleanup_aliasing_ppgtt(dev);
i915_gem_cleanup_stolen(dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 8fb8cd8..48e41df 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -277,11 +277,14 @@ void i915_gem_context_fini(struct drm_device *dev)
if (dev_priv->hw_contexts_disabled)
return;
+ /* The only known way to stop the gpu from accessing the hw context is
+ * to reset it. Do this as the very last operation to avoid confusing
+ * other code, leading to spurious errors. */
+ intel_gpu_reset(dev);
+
i915_gem_object_unpin(dev_priv->ring[RCS].default_context->obj);
do_destroy(dev_priv->ring[RCS].default_context);
-
- intel_gpu_reset(dev);
}
void i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
--
1.7.10
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] drm/i915: fix module unload after context merge
2012-06-19 19:55 ` [PATCH] " Daniel Vetter
@ 2012-06-19 21:19 ` Ben Widawsky
0 siblings, 0 replies; 11+ messages in thread
From: Ben Widawsky @ 2012-06-19 21:19 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
On Tue, 19 Jun 2012 21:55:32 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> commit 8e96d9c4d9843f00ebeb4a9b33596d96602ea101
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date: Mon Jun 4 14:42:56 2012 -0700
>
> drm/i915: reset the GPU on context fini
>
> broke module unload because it reset the gpu before we've stopped
> touching it. Later on in the unload sequence the ringbuffer code
> complained that the gpu would idle properly (because intel_gpu_reset
> only resets the hw and not our sw state).
>
> v2: Reorder things so that we reset the gpu _before_ we release the
> backing storage of the default context.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=51183
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 2 +-
> drivers/gpu/drm/i915/i915_gem_context.c | 7 +++++--
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index f1544c5..efba4e8 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1669,7 +1669,6 @@ int i915_driver_unload(struct drm_device *dev)
> if (ret)
> DRM_ERROR("failed to idle hardware: %d\n", ret);
> i915_gem_retire_requests(dev);
> - i915_gem_context_fini(dev);
> mutex_unlock(&dev->struct_mutex);
>
> /* Cancel the retire work handler, which should be idle now. */
> @@ -1720,6 +1719,7 @@ int i915_driver_unload(struct drm_device *dev)
> mutex_lock(&dev->struct_mutex);
> i915_gem_free_all_phys_object(dev);
> i915_gem_cleanup_ringbuffer(dev);
> + i915_gem_context_fini(dev);
> mutex_unlock(&dev->struct_mutex);
> i915_gem_cleanup_aliasing_ppgtt(dev);
> i915_gem_cleanup_stolen(dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 8fb8cd8..48e41df 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -277,11 +277,14 @@ void i915_gem_context_fini(struct drm_device *dev)
> if (dev_priv->hw_contexts_disabled)
> return;
>
> + /* The only known way to stop the gpu from accessing the hw context is
> + * to reset it. Do this as the very last operation to avoid confusing
> + * other code, leading to spurious errors. */
> + intel_gpu_reset(dev);
> +
> i915_gem_object_unpin(dev_priv->ring[RCS].default_context->obj);
>
> do_destroy(dev_priv->ring[RCS].default_context);
> -
> - intel_gpu_reset(dev);
> }
>
> void i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread