* [PATCH 1/2] drm/i915: No need to put forcewake after a reset
@ 2014-03-05 16:08 Mika Kuoppala
2014-03-05 16:08 ` [PATCH 2/2] drm/i915: Always set fifo count to zero in gen6_reset Mika Kuoppala
2014-03-05 17:59 ` [PATCH 1/2] drm/i915: No need to put forcewake after a reset Ben Widawsky
0 siblings, 2 replies; 6+ messages in thread
From: Mika Kuoppala @ 2014-03-05 16:08 UTC (permalink / raw)
To: intel-gfx
As we now have intel_uncore_forcewake_reset() no need
to do explicit put after reset.
v2: rebase
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
drivers/gpu/drm/i915/intel_uncore.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 6ca24ac..00320fd 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -952,6 +952,7 @@ static int gen6_do_reset(struct drm_device *dev)
struct drm_i915_private *dev_priv = dev->dev_private;
int ret;
unsigned long irqflags;
+ u32 fw_engine = 0;
/* Hold uncore.lock across reset to prevent any register access
* with forcewake not set correctly
@@ -971,25 +972,21 @@ static int gen6_do_reset(struct drm_device *dev)
intel_uncore_forcewake_reset(dev);
- /* If reset with a user forcewake, try to restore, otherwise turn it off */
+ /* If reset with a user forcewake, try to restore */
if (IS_VALLEYVIEW(dev)) {
if (dev_priv->uncore.fw_rendercount)
- dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_RENDER);
- else
- dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_RENDER);
+ fw_engine |= FORCEWAKE_RENDER;
if (dev_priv->uncore.fw_mediacount)
- dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_MEDIA);
- else
- dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_MEDIA);
+ fw_engine |= FORCEWAKE_MEDIA;
} else {
if (dev_priv->uncore.forcewake_count)
- dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
- else
- dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
+ fw_engine = FORCEWAKE_ALL;
}
- /* Restore fifo count */
+ if (fw_engine)
+ dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine);
+
if (IS_GEN6(dev) || IS_GEN7(dev))
dev_priv->uncore.fifo_count =
__raw_i915_read32(dev_priv, GTFIFOCTL) &
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] drm/i915: Always set fifo count to zero in gen6_reset
2014-03-05 16:08 [PATCH 1/2] drm/i915: No need to put forcewake after a reset Mika Kuoppala
@ 2014-03-05 16:08 ` Mika Kuoppala
2014-03-05 18:12 ` Ben Widawsky
2014-03-05 17:59 ` [PATCH 1/2] drm/i915: No need to put forcewake after a reset Ben Widawsky
1 sibling, 1 reply; 6+ messages in thread
From: Mika Kuoppala @ 2014-03-05 16:08 UTC (permalink / raw)
To: intel-gfx
There should not be a case where fifo count is other
than zero after a successful reset. Always set
count to zero, but be paranoid enough to warn.
v2: rebased
Suggested-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
drivers/gpu/drm/i915/intel_uncore.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 00320fd..79eaba8 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -988,9 +988,10 @@ static int gen6_do_reset(struct drm_device *dev)
dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine);
if (IS_GEN6(dev) || IS_GEN7(dev))
- dev_priv->uncore.fifo_count =
- __raw_i915_read32(dev_priv, GTFIFOCTL) &
- GT_FIFO_FREE_ENTRIES_MASK;
+ WARN_ON((__raw_i915_read32(dev_priv, GTFIFOCTL) &
+ GT_FIFO_FREE_ENTRIES_MASK) != 0);
+
+ dev_priv->uncore.fifo_count = 0;
spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
return ret;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915: Always set fifo count to zero in gen6_reset
2014-03-05 16:08 ` [PATCH 2/2] drm/i915: Always set fifo count to zero in gen6_reset Mika Kuoppala
@ 2014-03-05 18:12 ` Ben Widawsky
2014-03-05 19:33 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2014-03-05 18:12 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: intel-gfx
On Wed, Mar 05, 2014 at 06:08:19PM +0200, Mika Kuoppala wrote:
> There should not be a case where fifo count is other
> than zero after a successful reset. Always set
> count to zero, but be paranoid enough to warn.
>
> v2: rebased
>
> Suggested-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
> drivers/gpu/drm/i915/intel_uncore.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 00320fd..79eaba8 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -988,9 +988,10 @@ static int gen6_do_reset(struct drm_device *dev)
> dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine);
>
> if (IS_GEN6(dev) || IS_GEN7(dev))
> - dev_priv->uncore.fifo_count =
> - __raw_i915_read32(dev_priv, GTFIFOCTL) &
> - GT_FIFO_FREE_ENTRIES_MASK;
> + WARN_ON((__raw_i915_read32(dev_priv, GTFIFOCTL) &
> + GT_FIFO_FREE_ENTRIES_MASK) != 0);
> +
> + dev_priv->uncore.fifo_count = 0;
>
> spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> return ret;
Can you please add the following to the commit message:
"The GT FIFO is bypassed when not in RC6 from both IA and SA. As we've
just reset the GPU and not yet enabled RC6, there is no way the FIFO can
be anything but 0. If it is non-zero, it's a HW bug, and we can try to
carry on by faking it. It should be noted that RC6 is highly unlikely to
work properly if this WARN fires, however the system should continue on
just fine."
With that:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915: Always set fifo count to zero in gen6_reset
2014-03-05 18:12 ` Ben Widawsky
@ 2014-03-05 19:33 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2014-03-05 19:33 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx
On Wed, Mar 05, 2014 at 10:12:55AM -0800, Ben Widawsky wrote:
> On Wed, Mar 05, 2014 at 06:08:19PM +0200, Mika Kuoppala wrote:
> > There should not be a case where fifo count is other
> > than zero after a successful reset. Always set
> > count to zero, but be paranoid enough to warn.
> >
> > v2: rebased
> >
> > Suggested-by: Ben Widawsky <ben@bwidawsk.net>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_uncore.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 00320fd..79eaba8 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -988,9 +988,10 @@ static int gen6_do_reset(struct drm_device *dev)
> > dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine);
> >
> > if (IS_GEN6(dev) || IS_GEN7(dev))
> > - dev_priv->uncore.fifo_count =
> > - __raw_i915_read32(dev_priv, GTFIFOCTL) &
> > - GT_FIFO_FREE_ENTRIES_MASK;
> > + WARN_ON((__raw_i915_read32(dev_priv, GTFIFOCTL) &
> > + GT_FIFO_FREE_ENTRIES_MASK) != 0);
> > +
> > + dev_priv->uncore.fifo_count = 0;
> >
> > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > return ret;
>
> Can you please add the following to the commit message:
> "The GT FIFO is bypassed when not in RC6 from both IA and SA. As we've
> just reset the GPU and not yet enabled RC6, there is no way the FIFO can
> be anything but 0. If it is non-zero, it's a HW bug, and we can try to
> carry on by faking it. It should be noted that RC6 is highly unlikely to
> work properly if this WARN fires, however the system should continue on
> just fine."
Done.
>
> With that:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Queued for -next, thanks for the patch.
-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
* Re: [PATCH 1/2] drm/i915: No need to put forcewake after a reset
2014-03-05 16:08 [PATCH 1/2] drm/i915: No need to put forcewake after a reset Mika Kuoppala
2014-03-05 16:08 ` [PATCH 2/2] drm/i915: Always set fifo count to zero in gen6_reset Mika Kuoppala
@ 2014-03-05 17:59 ` Ben Widawsky
2014-03-05 18:12 ` Daniel Vetter
1 sibling, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2014-03-05 17:59 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: intel-gfx
On Wed, Mar 05, 2014 at 06:08:18PM +0200, Mika Kuoppala wrote:
> As we now have intel_uncore_forcewake_reset() no need
> to do explicit put after reset.
>
> v2: rebase
>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
> drivers/gpu/drm/i915/intel_uncore.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 6ca24ac..00320fd 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -952,6 +952,7 @@ static int gen6_do_reset(struct drm_device *dev)
> struct drm_i915_private *dev_priv = dev->dev_private;
> int ret;
> unsigned long irqflags;
> + u32 fw_engine = 0;
>
> /* Hold uncore.lock across reset to prevent any register access
> * with forcewake not set correctly
> @@ -971,25 +972,21 @@ static int gen6_do_reset(struct drm_device *dev)
>
> intel_uncore_forcewake_reset(dev);
>
> - /* If reset with a user forcewake, try to restore, otherwise turn it off */
> + /* If reset with a user forcewake, try to restore */
Technically with the introduction of the deferred forcewaker_ put, it's
not just user forcewake that this restores.
Wouldn't it be nice if we actually used something other than
FORCEWAKE_KERNEL for debugfs?
> if (IS_VALLEYVIEW(dev)) {
> if (dev_priv->uncore.fw_rendercount)
> - dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_RENDER);
> - else
> - dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_RENDER);
> + fw_engine |= FORCEWAKE_RENDER;
>
> if (dev_priv->uncore.fw_mediacount)
> - dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_MEDIA);
> - else
> - dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_MEDIA);
> + fw_engine |= FORCEWAKE_MEDIA;
> } else {
> if (dev_priv->uncore.forcewake_count)
> - dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> - else
> - dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> + fw_engine = FORCEWAKE_ALL;
> }
P
>
> - /* Restore fifo count */
> + if (fw_engine)
> + dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine);
> +
> if (IS_GEN6(dev) || IS_GEN7(dev))
> dev_priv->uncore.fifo_count =
> __raw_i915_read32(dev_priv, GTFIFOCTL) &
I for one would not be opposed to a vlv_do_reset() and a
gen8_do_reset(). I implemented such a thing at some point, but threw it
away because I didn't actually have a vlv diff at the time.
I had to re-review a lot of the uncore.lock stuff, but lgtm:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] drm/i915: No need to put forcewake after a reset
2014-03-05 17:59 ` [PATCH 1/2] drm/i915: No need to put forcewake after a reset Ben Widawsky
@ 2014-03-05 18:12 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2014-03-05 18:12 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx
On Wed, Mar 05, 2014 at 09:59:55AM -0800, Ben Widawsky wrote:
> On Wed, Mar 05, 2014 at 06:08:18PM +0200, Mika Kuoppala wrote:
> > As we now have intel_uncore_forcewake_reset() no need
> > to do explicit put after reset.
> >
> > v2: rebase
> >
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_uncore.c | 19 ++++++++-----------
> > 1 file changed, 8 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 6ca24ac..00320fd 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -952,6 +952,7 @@ static int gen6_do_reset(struct drm_device *dev)
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > int ret;
> > unsigned long irqflags;
> > + u32 fw_engine = 0;
> >
> > /* Hold uncore.lock across reset to prevent any register access
> > * with forcewake not set correctly
> > @@ -971,25 +972,21 @@ static int gen6_do_reset(struct drm_device *dev)
> >
> > intel_uncore_forcewake_reset(dev);
> >
> > - /* If reset with a user forcewake, try to restore, otherwise turn it off */
> > + /* If reset with a user forcewake, try to restore */
>
> Technically with the introduction of the deferred forcewaker_ put, it's
> not just user forcewake that this restores.
>
> Wouldn't it be nice if we actually used something other than
> FORCEWAKE_KERNEL for debugfs?
mt forcewake doesn't exist on snb :(
>
> > if (IS_VALLEYVIEW(dev)) {
> > if (dev_priv->uncore.fw_rendercount)
> > - dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_RENDER);
> > - else
> > - dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_RENDER);
> > + fw_engine |= FORCEWAKE_RENDER;
> >
> > if (dev_priv->uncore.fw_mediacount)
> > - dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_MEDIA);
> > - else
> > - dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_MEDIA);
> > + fw_engine |= FORCEWAKE_MEDIA;
> > } else {
> > if (dev_priv->uncore.forcewake_count)
> > - dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> > - else
> > - dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> > + fw_engine = FORCEWAKE_ALL;
> > }
>
> P
>
> >
> > - /* Restore fifo count */
> > + if (fw_engine)
> > + dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine);
> > +
> > if (IS_GEN6(dev) || IS_GEN7(dev))
> > dev_priv->uncore.fifo_count =
> > __raw_i915_read32(dev_priv, GTFIFOCTL) &
>
> I for one would not be opposed to a vlv_do_reset() and a
> gen8_do_reset(). I implemented such a thing at some point, but threw it
> away because I didn't actually have a vlv diff at the time.
>
> I had to re-review a lot of the uncore.lock stuff, but lgtm:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Queued for -next, thanks for the patch.
-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:[~2014-03-05 19:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-05 16:08 [PATCH 1/2] drm/i915: No need to put forcewake after a reset Mika Kuoppala
2014-03-05 16:08 ` [PATCH 2/2] drm/i915: Always set fifo count to zero in gen6_reset Mika Kuoppala
2014-03-05 18:12 ` Ben Widawsky
2014-03-05 19:33 ` Daniel Vetter
2014-03-05 17:59 ` [PATCH 1/2] drm/i915: No need to put forcewake after a reset Ben Widawsky
2014-03-05 18:12 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox