All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] drm/i915: Do uncore early sanitize after domain init
@ 2015-01-28 12:43 Mika Kuoppala
  2015-01-28 12:43 ` [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence Mika Kuoppala
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Mika Kuoppala @ 2015-01-28 12:43 UTC (permalink / raw)
  To: intel-gfx

intel_uncore_early_sanitize() will reset the forcewake registers. When
forcewake domains were introduced, the domain init was done after the
sanitization of the forcewake registers. And as the resetting of
registers use the domain accessors, we tried to reset the forcewake
registers with unitialized forcewake domains and failed.

Fix this by sanitizing after all the domains have been initialized. Do
per domain clearing of forcewake register on domain init so that
IVB can do early access to ECOBUS do determine the final configuration.

This regression was introduced in

commit 05a2fb157e44a53c79133805d30eaada43911941
Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Date:   Mon Jan 19 16:20:43 2015 +0200

    drm/i915: Consolidate forcewake code

v2: Carve out ellc detect, fw_domain_reset for ivb/ecobus (Chris)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88805
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reported-by: Olof Johansson <olof@lixom.net>
Tested-by: Darren Hart <dvhart@linux.intel.com> (v1)
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 38 +++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index b3951f2..be2c7fc 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -72,6 +72,7 @@ assert_device_not_suspended(struct drm_i915_private *dev_priv)
 static inline void
 fw_domain_reset(const struct intel_uncore_forcewake_domain *d)
 {
+	WARN_ON(d->reg_set == 0);
 	__raw_i915_write32(d->i915, d->reg_set, d->val_reset);
 }
 
@@ -166,6 +167,8 @@ fw_domains_reset(struct drm_i915_private *dev_priv, enum forcewake_domains fw_do
 	struct intel_uncore_forcewake_domain *d;
 	enum forcewake_domain_id id;
 
+	WARN_ON(dev_priv->uncore.fw_domains == 0);
+
 	for_each_fw_domain_mask(d, fw_domains, dev_priv, id)
 		fw_domain_reset(d);
 
@@ -321,14 +324,10 @@ void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
-static void __intel_uncore_early_sanitize(struct drm_device *dev,
-					  bool restore_forcewake)
+static void intel_uncore_ellc_detect(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (HAS_FPGA_DBG_UNCLAIMED(dev))
-		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
-
 	if ((IS_HASWELL(dev) || IS_BROADWELL(dev)) &&
 	    (__raw_i915_read32(dev_priv, HSW_EDRAM_PRESENT) == 1)) {
 		/* The docs do not explain exactly how the calculation can be
@@ -339,6 +338,15 @@ static void __intel_uncore_early_sanitize(struct drm_device *dev,
 		dev_priv->ellc_size = 128;
 		DRM_INFO("Found %zuMB of eLLC\n", dev_priv->ellc_size);
 	}
+}
+
+static void __intel_uncore_early_sanitize(struct drm_device *dev,
+					  bool restore_forcewake)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (HAS_FPGA_DBG_UNCLAIMED(dev))
+		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
 
 	/* clear out old GT FIFO errors */
 	if (IS_GEN6(dev) || IS_GEN7(dev))
@@ -982,14 +990,14 @@ static void fw_domain_init(struct drm_i915_private *dev_priv,
 	setup_timer(&d->timer, intel_uncore_fw_release_timer, (unsigned long)d);
 
 	dev_priv->uncore.fw_domains |= (1 << domain_id);
+
+	fw_domain_reset(d);
 }
 
-void intel_uncore_init(struct drm_device *dev)
+static void intel_uncore_fw_domains_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	__intel_uncore_early_sanitize(dev, false);
-
 	if (IS_GEN9(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = fw_domains_get;
 		dev_priv->uncore.funcs.force_wake_put = fw_domains_put;
@@ -1035,8 +1043,13 @@ void intel_uncore_init(struct drm_device *dev)
 		dev_priv->uncore.funcs.force_wake_put =
 			fw_domains_put_with_fifo;
 
+		/* We need to init first for ECOBUS access and then
+		 * determine later if we want to reinit, in case of MT access is
+		 * not working
+		 */
 		fw_domain_init(dev_priv, FW_DOMAIN_ID_RENDER,
 			       FORCEWAKE_MT, FORCEWAKE_MT_ACK);
+
 		mutex_lock(&dev->struct_mutex);
 		fw_domains_get_with_thread_status(dev_priv, FORCEWAKE_ALL);
 		ecobus = __raw_i915_read32(dev_priv, ECOBUS);
@@ -1057,6 +1070,15 @@ void intel_uncore_init(struct drm_device *dev)
 		fw_domain_init(dev_priv, FW_DOMAIN_ID_RENDER,
 			       FORCEWAKE, FORCEWAKE_ACK);
 	}
+}
+
+void intel_uncore_init(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	intel_uncore_ellc_detect(dev);
+	intel_uncore_fw_domains_init(dev);
+	__intel_uncore_early_sanitize(dev, false);
 
 	switch (INTEL_INFO(dev)->gen) {
 	default:
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence
  2015-01-28 12:43 [PATCH 1/3] drm/i915: Do uncore early sanitize after domain init Mika Kuoppala
@ 2015-01-28 12:43 ` Mika Kuoppala
  2015-01-28 12:58   ` Chris Wilson
  2015-01-28 12:43 ` [PATCH 3/3] drm/i915: Do only one posting read on forcewake get sequence Mika Kuoppala
  2015-01-28 13:01 ` [PATCH 1/3] drm/i915: Do uncore early sanitize after domain init Chris Wilson
  2 siblings, 1 reply; 15+ messages in thread
From: Mika Kuoppala @ 2015-01-28 12:43 UTC (permalink / raw)
  To: intel-gfx

commit 05a2fb157e44a53c79133805d30eaada43911941
Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Date:   Mon Jan 19 16:20:43 2015 +0200

    drm/i915: Consolidate forcewake code

introduced domain handling where each domain has it's own posting
read registers. This changed the forcewake sequence on 'put' side when
there is multiple domains as there would be extra read between the domain
puts. Any posting read should be enough to flush all the changes.

Do a posting read only once, at the end of the sequence and for
the first domain. Like it was before.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index be2c7fc..ebd9068 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -123,42 +123,42 @@ fw_domain_posting_read(const struct intel_uncore_forcewake_domain *d)
 }
 
 static void
-fw_domains_get(struct drm_i915_private *dev_priv, enum forcewake_domains fw_domains)
+fw_domains_posting_read(struct drm_i915_private *dev_priv)
 {
 	struct intel_uncore_forcewake_domain *d;
 	enum forcewake_domain_id id;
 
-	for_each_fw_domain_mask(d, fw_domains, dev_priv, id) {
-		fw_domain_wait_ack_clear(d);
-		fw_domain_get(d);
+	/* No need to do for all, just do for first found */
+	for_each_fw_domain(d, dev_priv, id) {
 		fw_domain_posting_read(d);
-		fw_domain_wait_ack(d);
+		break;
 	}
 }
 
 static void
-fw_domains_put(struct drm_i915_private *dev_priv, enum forcewake_domains fw_domains)
+fw_domains_get(struct drm_i915_private *dev_priv, enum forcewake_domains fw_domains)
 {
 	struct intel_uncore_forcewake_domain *d;
 	enum forcewake_domain_id id;
 
 	for_each_fw_domain_mask(d, fw_domains, dev_priv, id) {
-		fw_domain_put(d);
+		fw_domain_wait_ack_clear(d);
+		fw_domain_get(d);
 		fw_domain_posting_read(d);
+		fw_domain_wait_ack(d);
 	}
 }
 
 static void
-fw_domains_posting_read(struct drm_i915_private *dev_priv)
+fw_domains_put(struct drm_i915_private *dev_priv, enum forcewake_domains fw_domains)
 {
 	struct intel_uncore_forcewake_domain *d;
 	enum forcewake_domain_id id;
 
-	/* No need to do for all, just do for first found */
-	for_each_fw_domain(d, dev_priv, id) {
-		fw_domain_posting_read(d);
-		break;
-	}
+	for_each_fw_domain_mask(d, fw_domains, dev_priv, id)
+		fw_domain_put(d);
+
+	fw_domains_posting_read(dev_priv);
 }
 
 static void
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/3] drm/i915: Do only one posting read on forcewake get sequence
  2015-01-28 12:43 [PATCH 1/3] drm/i915: Do uncore early sanitize after domain init Mika Kuoppala
  2015-01-28 12:43 ` [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence Mika Kuoppala
@ 2015-01-28 12:43 ` Mika Kuoppala
  2015-01-28 12:59   ` Chris Wilson
  2015-01-31  7:52   ` shuang.he
  2015-01-28 13:01 ` [PATCH 1/3] drm/i915: Do uncore early sanitize after domain init Chris Wilson
  2 siblings, 2 replies; 15+ messages in thread
From: Mika Kuoppala @ 2015-01-28 12:43 UTC (permalink / raw)
  To: intel-gfx

Follow the same semantics as in put side where we go through
all domains before doing posting read.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index ebd9068..d2423e6 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -144,9 +144,10 @@ fw_domains_get(struct drm_i915_private *dev_priv, enum forcewake_domains fw_doma
 	for_each_fw_domain_mask(d, fw_domains, dev_priv, id) {
 		fw_domain_wait_ack_clear(d);
 		fw_domain_get(d);
-		fw_domain_posting_read(d);
 		fw_domain_wait_ack(d);
 	}
+
+	fw_domains_posting_read(dev_priv);
 }
 
 static void
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence
  2015-01-28 12:43 ` [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence Mika Kuoppala
@ 2015-01-28 12:58   ` Chris Wilson
  2015-01-28 13:25     ` [PATCH] drm/i915: Don't do posting reads on getting forcewake Mika Kuoppala
  2015-01-28 13:28     ` [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence Mika Kuoppala
  0 siblings, 2 replies; 15+ messages in thread
From: Chris Wilson @ 2015-01-28 12:58 UTC (permalink / raw)
  To: Mika Kuoppala; +Cc: intel-gfx

On Wed, Jan 28, 2015 at 02:43:25PM +0200, Mika Kuoppala wrote:
> commit 05a2fb157e44a53c79133805d30eaada43911941
> Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Date:   Mon Jan 19 16:20:43 2015 +0200
> 
>     drm/i915: Consolidate forcewake code
> 
> introduced domain handling where each domain has it's own posting
> read registers. This changed the forcewake sequence on 'put' side when
> there is multiple domains as there would be extra read between the domain
> puts. Any posting read should be enough to flush all the changes.
> 
> Do a posting read only once, at the end of the sequence and for
> the first domain. Like it was before.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

fwiw, I would argue that the posting read in _get() is superfluous as we
will serialise the fw with not only the ack, but any subsequent mmio.

On the _put() side we do want to flush the write so that the hw can
power down as early as possible. So just kill the posting read from _get
and otherwise drop the patch. :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] drm/i915: Do only one posting read on forcewake get sequence
  2015-01-28 12:43 ` [PATCH 3/3] drm/i915: Do only one posting read on forcewake get sequence Mika Kuoppala
@ 2015-01-28 12:59   ` Chris Wilson
  2015-01-31  7:52   ` shuang.he
  1 sibling, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2015-01-28 12:59 UTC (permalink / raw)
  To: Mika Kuoppala; +Cc: intel-gfx

On Wed, Jan 28, 2015 at 02:43:26PM +0200, Mika Kuoppala wrote:
> Follow the same semantics as in put side where we go through
> all domains before doing posting read.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

Having just argued that I don't want a posting read here at all (and
that the existing one is just overly paranoid even by our standards), we
can drop this patch as well :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] drm/i915: Do uncore early sanitize after domain init
  2015-01-28 12:43 [PATCH 1/3] drm/i915: Do uncore early sanitize after domain init Mika Kuoppala
  2015-01-28 12:43 ` [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence Mika Kuoppala
  2015-01-28 12:43 ` [PATCH 3/3] drm/i915: Do only one posting read on forcewake get sequence Mika Kuoppala
@ 2015-01-28 13:01 ` Chris Wilson
  2 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2015-01-28 13:01 UTC (permalink / raw)
  To: Mika Kuoppala; +Cc: intel-gfx

On Wed, Jan 28, 2015 at 02:43:24PM +0200, Mika Kuoppala wrote:
> intel_uncore_early_sanitize() will reset the forcewake registers. When
> forcewake domains were introduced, the domain init was done after the
> sanitization of the forcewake registers. And as the resetting of
> registers use the domain accessors, we tried to reset the forcewake
> registers with unitialized forcewake domains and failed.
> 
> Fix this by sanitizing after all the domains have been initialized. Do
> per domain clearing of forcewake register on domain init so that
> IVB can do early access to ECOBUS do determine the final configuration.
> 
> This regression was introduced in
> 
> commit 05a2fb157e44a53c79133805d30eaada43911941
> Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Date:   Mon Jan 19 16:20:43 2015 +0200
> 
>     drm/i915: Consolidate forcewake code
> 
> v2: Carve out ellc detect, fw_domain_reset for ivb/ecobus (Chris)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88805
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Reported-by: Olof Johansson <olof@lixom.net>
> Tested-by: Darren Hart <dvhart@linux.intel.com> (v1)
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

Looks good to me, I hope it survives booting...
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] drm/i915: Don't do posting reads on getting forcewake
  2015-01-28 12:58   ` Chris Wilson
@ 2015-01-28 13:25     ` Mika Kuoppala
  2015-01-28 14:04       ` Chris Wilson
  2015-01-31  9:13       ` shuang.he
  2015-01-28 13:28     ` [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence Mika Kuoppala
  1 sibling, 2 replies; 15+ messages in thread
From: Mika Kuoppala @ 2015-01-28 13:25 UTC (permalink / raw)
  To: intel-gfx

The checking for ack and also any subsequent mmio access
will serialize with setting the forcewake bit. Drop the
posting read as superfluous.

Note that in the put side we still want to keep the posting read
as it will ensure that the hw sees our forcewake release in a
timely manner and doesn't keep the hw powered up.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index be2c7fc..76b60a3 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -131,7 +131,6 @@ fw_domains_get(struct drm_i915_private *dev_priv, enum forcewake_domains fw_doma
 	for_each_fw_domain_mask(d, fw_domains, dev_priv, id) {
 		fw_domain_wait_ack_clear(d);
 		fw_domain_get(d);
-		fw_domain_posting_read(d);
 		fw_domain_wait_ack(d);
 	}
 }
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence
  2015-01-28 12:58   ` Chris Wilson
  2015-01-28 13:25     ` [PATCH] drm/i915: Don't do posting reads on getting forcewake Mika Kuoppala
@ 2015-01-28 13:28     ` Mika Kuoppala
  2015-01-28 13:48       ` Ville Syrjälä
  1 sibling, 1 reply; 15+ messages in thread
From: Mika Kuoppala @ 2015-01-28 13:28 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Wed, Jan 28, 2015 at 02:43:25PM +0200, Mika Kuoppala wrote:
>> commit 05a2fb157e44a53c79133805d30eaada43911941
>> Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> Date:   Mon Jan 19 16:20:43 2015 +0200
>> 
>>     drm/i915: Consolidate forcewake code
>> 
>> introduced domain handling where each domain has it's own posting
>> read registers. This changed the forcewake sequence on 'put' side when
>> there is multiple domains as there would be extra read between the domain
>> puts. Any posting read should be enough to flush all the changes.
>> 
>> Do a posting read only once, at the end of the sequence and for
>> the first domain. Like it was before.
>> 
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
> fwiw, I would argue that the posting read in _get() is superfluous as we
> will serialise the fw with not only the ack, but any subsequent mmio.
>
> On the _put() side we do want to flush the write so that the hw can
> power down as early as possible. So just kill the posting read from _get
> and otherwise drop the patch. :)

Yes, both put/get patches should be dropped. I posted a patch removing
the posting read on get side and with your explanations in commit message.

This all starts to make so much sense that some gen is bound to break ;)

-Mika

> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence
  2015-01-28 13:28     ` [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence Mika Kuoppala
@ 2015-01-28 13:48       ` Ville Syrjälä
  2015-01-28 15:54         ` Mika Kuoppala
  0 siblings, 1 reply; 15+ messages in thread
From: Ville Syrjälä @ 2015-01-28 13:48 UTC (permalink / raw)
  To: Mika Kuoppala; +Cc: intel-gfx

On Wed, Jan 28, 2015 at 03:28:56PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > On Wed, Jan 28, 2015 at 02:43:25PM +0200, Mika Kuoppala wrote:
> >> commit 05a2fb157e44a53c79133805d30eaada43911941
> >> Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >> Date:   Mon Jan 19 16:20:43 2015 +0200
> >> 
> >>     drm/i915: Consolidate forcewake code
> >> 
> >> introduced domain handling where each domain has it's own posting
> >> read registers. This changed the forcewake sequence on 'put' side when
> >> there is multiple domains as there would be extra read between the domain
> >> puts. Any posting read should be enough to flush all the changes.
> >> 
> >> Do a posting read only once, at the end of the sequence and for
> >> the first domain. Like it was before.
> >> 
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >
> > fwiw, I would argue that the posting read in _get() is superfluous as we
> > will serialise the fw with not only the ack, but any subsequent mmio.
> >
> > On the _put() side we do want to flush the write so that the hw can
> > power down as early as possible. So just kill the posting read from _get
> > and otherwise drop the patch. :)
> 
> Yes, both put/get patches should be dropped. I posted a patch removing
> the posting read on get side and with your explanations in commit message.
> 
> This all starts to make so much sense that some gen is bound to break ;)

IIRC the posting read from same cache line actually fixed real bugs. So
I'm a bit worried about dropping them. But I suppose it's possible only
the _put side was important for those bugs.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] drm/i915: Don't do posting reads on getting forcewake
  2015-01-28 13:25     ` [PATCH] drm/i915: Don't do posting reads on getting forcewake Mika Kuoppala
@ 2015-01-28 14:04       ` Chris Wilson
  2015-01-30 16:16         ` Daniel Vetter
  2015-01-31  9:13       ` shuang.he
  1 sibling, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2015-01-28 14:04 UTC (permalink / raw)
  To: Mika Kuoppala; +Cc: intel-gfx

On Wed, Jan 28, 2015 at 03:25:05PM +0200, Mika Kuoppala wrote:
> The checking for ack and also any subsequent mmio access
> will serialize with setting the forcewake bit. Drop the
> posting read as superfluous.
> 
> Note that in the put side we still want to keep the posting read
> as it will ensure that the hw sees our forcewake release in a
> timely manner and doesn't keep the hw powered up.
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence
  2015-01-28 13:48       ` Ville Syrjälä
@ 2015-01-28 15:54         ` Mika Kuoppala
  2015-01-28 16:43           ` Chris Wilson
  0 siblings, 1 reply; 15+ messages in thread
From: Mika Kuoppala @ 2015-01-28 15:54 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

Ville Syrjälä <ville.syrjala@linux.intel.com> writes:

> On Wed, Jan 28, 2015 at 03:28:56PM +0200, Mika Kuoppala wrote:
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > On Wed, Jan 28, 2015 at 02:43:25PM +0200, Mika Kuoppala wrote:
>> >> commit 05a2fb157e44a53c79133805d30eaada43911941
>> >> Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> >> Date:   Mon Jan 19 16:20:43 2015 +0200
>> >> 
>> >>     drm/i915: Consolidate forcewake code
>> >> 
>> >> introduced domain handling where each domain has it's own posting
>> >> read registers. This changed the forcewake sequence on 'put' side when
>> >> there is multiple domains as there would be extra read between the domain
>> >> puts. Any posting read should be enough to flush all the changes.
>> >> 
>> >> Do a posting read only once, at the end of the sequence and for
>> >> the first domain. Like it was before.
>> >> 
>> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> >
>> > fwiw, I would argue that the posting read in _get() is superfluous as we
>> > will serialise the fw with not only the ack, but any subsequent mmio.
>> >
>> > On the _put() side we do want to flush the write so that the hw can
>> > power down as early as possible. So just kill the posting read from _get
>> > and otherwise drop the patch. :)
>> 
>> Yes, both put/get patches should be dropped. I posted a patch removing
>> the posting read on get side and with your explanations in commit message.
>> 
>> This all starts to make so much sense that some gen is bound to break ;)
>
> IIRC the posting read from same cache line actually fixed real bugs. So
> I'm a bit worried about dropping them. But I suppose it's possible only
> the _put side was important for those bugs.

I found these:

commit 6af2d180f82151cf3d58952e35a4f96e45bc453a
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jul 26 16:24:50 2012 +0200

    drm/i915: fix forcewake related hangs on snb

commit 8dee3eea3ccd3b6c00a8d3a08dd715d6adf737dd
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Sat Sep 1 22:59:50 2012 -0700

    drm/i915: Never read FORCEWAKE

https://bugs.freedesktop.org/show_bug.cgi?id=51738
https://bugs.freedesktop.org/show_bug.cgi?id=52424

The snb here seems to survive gem_dummy_reloc_loop and
gem_ring_sync_loop in here with the get side posting removed.

-Mika

>
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence
  2015-01-28 15:54         ` Mika Kuoppala
@ 2015-01-28 16:43           ` Chris Wilson
  0 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2015-01-28 16:43 UTC (permalink / raw)
  To: Mika Kuoppala; +Cc: intel-gfx

On Wed, Jan 28, 2015 at 05:54:14PM +0200, Mika Kuoppala wrote:
> Ville Syrjälä <ville.syrjala@linux.intel.com> writes:
> 
> > On Wed, Jan 28, 2015 at 03:28:56PM +0200, Mika Kuoppala wrote:
> >> Chris Wilson <chris@chris-wilson.co.uk> writes:
> >> 
> >> > On Wed, Jan 28, 2015 at 02:43:25PM +0200, Mika Kuoppala wrote:
> >> >> commit 05a2fb157e44a53c79133805d30eaada43911941
> >> >> Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >> >> Date:   Mon Jan 19 16:20:43 2015 +0200
> >> >> 
> >> >>     drm/i915: Consolidate forcewake code
> >> >> 
> >> >> introduced domain handling where each domain has it's own posting
> >> >> read registers. This changed the forcewake sequence on 'put' side when
> >> >> there is multiple domains as there would be extra read between the domain
> >> >> puts. Any posting read should be enough to flush all the changes.
> >> >> 
> >> >> Do a posting read only once, at the end of the sequence and for
> >> >> the first domain. Like it was before.
> >> >> 
> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >> >
> >> > fwiw, I would argue that the posting read in _get() is superfluous as we
> >> > will serialise the fw with not only the ack, but any subsequent mmio.
> >> >
> >> > On the _put() side we do want to flush the write so that the hw can
> >> > power down as early as possible. So just kill the posting read from _get
> >> > and otherwise drop the patch. :)
> >> 
> >> Yes, both put/get patches should be dropped. I posted a patch removing
> >> the posting read on get side and with your explanations in commit message.
> >> 
> >> This all starts to make so much sense that some gen is bound to break ;)
> >
> > IIRC the posting read from same cache line actually fixed real bugs. So
> > I'm a bit worried about dropping them. But I suppose it's possible only
> > the _put side was important for those bugs.
> 
> I found these:
> 
> commit 6af2d180f82151cf3d58952e35a4f96e45bc453a
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Jul 26 16:24:50 2012 +0200
> 
>     drm/i915: fix forcewake related hangs on snb
> 
> commit 8dee3eea3ccd3b6c00a8d3a08dd715d6adf737dd
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Sat Sep 1 22:59:50 2012 -0700
> 
>     drm/i915: Never read FORCEWAKE
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=51738
> https://bugs.freedesktop.org/show_bug.cgi?id=52424
> 
> The snb here seems to survive gem_dummy_reloc_loop and
> gem_ring_sync_loop in here with the get side posting removed.

Note that we kept the once associated with #52424, but judging by my
comments in #51738 the posting read is just a band aid anyway as a full
mb() itself was not adequate.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] drm/i915: Don't do posting reads on getting forcewake
  2015-01-28 14:04       ` Chris Wilson
@ 2015-01-30 16:16         ` Daniel Vetter
  0 siblings, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2015-01-30 16:16 UTC (permalink / raw)
  To: Chris Wilson, Mika Kuoppala, intel-gfx

On Wed, Jan 28, 2015 at 02:04:27PM +0000, Chris Wilson wrote:
> On Wed, Jan 28, 2015 at 03:25:05PM +0200, Mika Kuoppala wrote:
> > The checking for ack and also any subsequent mmio access
> > will serialize with setting the forcewake bit. Drop the
> > posting read as superfluous.
> > 
> > Note that in the put side we still want to keep the posting read
> > as it will ensure that the hw sees our forcewake release in a
> > timely manner and doesn't keep the hw powered up.
> > 
> > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

This and 1/3 merged, thansk for patches&review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] drm/i915: Do only one posting read on forcewake get sequence
  2015-01-28 12:43 ` [PATCH 3/3] drm/i915: Do only one posting read on forcewake get sequence Mika Kuoppala
  2015-01-28 12:59   ` Chris Wilson
@ 2015-01-31  7:52   ` shuang.he
  1 sibling, 0 replies; 15+ messages in thread
From: shuang.he @ 2015-01-31  7:52 UTC (permalink / raw)
  To: shuang.he, ethan.gao, intel-gfx, mika.kuoppala

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5674
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  353/353              353/353
ILK                 -2              353/353              351/353
SNB              +1                 400/422              401/422
IVB                                  485/487              485/487
BYT                                  296/296              296/296
HSW              +1-2              507/508              506/508
BDW                                  401/402              401/402
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_drv_suspend_debugfs-reader      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_gem_workarounds_suspend-resume      PASS(2, M26)      DMESG_WARN(1, M26)
*SNB  igt_kms_flip_event_leak      NSPT(5, M35M22)      PASS(1, M35)
 HSW  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance      DMESG_WARN(1, M40)PASS(18, M40M20)      PASS(1, M20)
*HSW  igt_gem_storedw_loop_bsd      PASS(2, M40M20)      DMESG_WARN(1, M20)
 HSW  igt_gem_storedw_loop_vebox      DMESG_WARN(2, M20)PASS(3, M40M20)      DMESG_WARN(1, M20)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] drm/i915: Don't do posting reads on getting forcewake
  2015-01-28 13:25     ` [PATCH] drm/i915: Don't do posting reads on getting forcewake Mika Kuoppala
  2015-01-28 14:04       ` Chris Wilson
@ 2015-01-31  9:13       ` shuang.he
  1 sibling, 0 replies; 15+ messages in thread
From: shuang.he @ 2015-01-31  9:13 UTC (permalink / raw)
  To: shuang.he, ethan.gao, intel-gfx, mika.kuoppala

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5675
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -1              353/353              352/353
ILK                                  200/200              200/200
SNB                                  400/422              400/422
IVB              +1                 485/487              486/487
BYT                                  296/296              296/296
HSW              +1-2              507/508              506/508
BDW                                  401/402              401/402
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gen3_render_linear_blits      PASS(4, M25M23)      CRASH(1, M23)
 IVB  igt_gem_storedw_batches_loop_normal      DMESG_WARN(5, M34M4)PASS(15, M34M4M21)      PASS(1, M21)
*HSW  igt_gem_pwrite_pread_snooped-copy-performance      PASS(2, M40)      DMESG_WARN(1, M40)
 HSW  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance      DMESG_WARN(1, M40)PASS(18, M40M20)      PASS(1, M40)
*HSW  igt_gem_storedw_loop_blt      PASS(2, M40)      DMESG_WARN(1, M40)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2015-01-31  9:13 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-28 12:43 [PATCH 1/3] drm/i915: Do uncore early sanitize after domain init Mika Kuoppala
2015-01-28 12:43 ` [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence Mika Kuoppala
2015-01-28 12:58   ` Chris Wilson
2015-01-28 13:25     ` [PATCH] drm/i915: Don't do posting reads on getting forcewake Mika Kuoppala
2015-01-28 14:04       ` Chris Wilson
2015-01-30 16:16         ` Daniel Vetter
2015-01-31  9:13       ` shuang.he
2015-01-28 13:28     ` [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence Mika Kuoppala
2015-01-28 13:48       ` Ville Syrjälä
2015-01-28 15:54         ` Mika Kuoppala
2015-01-28 16:43           ` Chris Wilson
2015-01-28 12:43 ` [PATCH 3/3] drm/i915: Do only one posting read on forcewake get sequence Mika Kuoppala
2015-01-28 12:59   ` Chris Wilson
2015-01-31  7:52   ` shuang.he
2015-01-28 13:01 ` [PATCH 1/3] drm/i915: Do uncore early sanitize after domain init Chris Wilson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.