[PATCH] drm/i915: Do uncore early sanitize after domain init

[PATCH] drm/i915: Do uncore early sanitize after domain init

[Mika Kuoppala]

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.
On ivb we need special care as there we need early forcewake access to
determine the final configuration for the forcewake domain.

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

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88805
Reported-by: Olof Johansson <olof@lixom.net>
Tested-by: Darren Hart <dvhart@linux.intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index b3951f2..c438ca4 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);
 
@@ -987,8 +990,7 @@ static void fw_domain_init(struct drm_i915_private *dev_priv,
 void intel_uncore_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	__intel_uncore_early_sanitize(dev, false);
+	bool sanitize_done = false;
 
 	if (IS_GEN9(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = fw_domains_get;
@@ -1037,6 +1039,8 @@ void intel_uncore_init(struct drm_device *dev)
 
 		fw_domain_init(dev_priv, FW_DOMAIN_ID_RENDER,
 			       FORCEWAKE_MT, FORCEWAKE_MT_ACK);
+		__intel_uncore_early_sanitize(dev, false);
+		sanitize_done = true;
 		mutex_lock(&dev->struct_mutex);
 		fw_domains_get_with_thread_status(dev_priv, FORCEWAKE_ALL);
 		ecobus = __raw_i915_read32(dev_priv, ECOBUS);
@@ -1058,6 +1062,9 @@ void intel_uncore_init(struct drm_device *dev)
 			       FORCEWAKE, FORCEWAKE_ACK);
 	}
 
+	if (sanitize_done == false)
+		__intel_uncore_early_sanitize(dev, false);
+
 	switch (INTEL_INFO(dev)->gen) {
 	default:
 		MISSING_CASE(INTEL_INFO(dev)->gen);
-- 
1.9.1

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

Re: [PATCH] drm/i915: Do uncore early sanitize after domain init

[Chris Wilson]

On Wed, Jan 28, 2015 at 11:45:04AM +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.
> On ivb we need special care as there we need early forcewake access to
> determine the final configuration for the forcewake domain.
> 
> 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
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88805
> Reported-by: Olof Johansson <olof@lixom.net>
> Tested-by: Darren Hart <dvhart@linux.intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index b3951f2..c438ca4 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);
>  
> @@ -987,8 +990,7 @@ static void fw_domain_init(struct drm_i915_private *dev_priv,
>  void intel_uncore_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	__intel_uncore_early_sanitize(dev, false);
> +	bool sanitize_done = false;

I felt this looks quite clumsy. The only reason why you want to restrict
calling __intel_uncore_early_sanitize() is that it does ellc_size
detection and has a DRM_INFO right?

I think you want to pull that out of __intel_uncore_early_santize() into
intel_uncore_init() itself (or better, it's own
intel_uncore_ellc_detect()). ellc_size detection has nothing to do with
sanitizing register state.

Then it should be simple to enough to sanitize twice, once with a
comment in the code explaining how we verify that FORCEWAKE_MT is
enabled by a manual forcewaked read of ECOBUS.
-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

Re: [PATCH] drm/i915: Do uncore early sanitize after domain init

[Chris Wilson]

On Wed, Jan 28, 2015 at 10:17:39AM +0000, Chris Wilson wrote:
> On Wed, Jan 28, 2015 at 11:45:04AM +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.
> > On ivb we need special care as there we need early forcewake access to
> > determine the final configuration for the forcewake domain.
> > 
> > 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
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88805
> > Reported-by: Olof Johansson <olof@lixom.net>
> > Tested-by: Darren Hart <dvhart@linux.intel.com>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_uncore.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index b3951f2..c438ca4 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);
> >  
> > @@ -987,8 +990,7 @@ static void fw_domain_init(struct drm_i915_private *dev_priv,
> >  void intel_uncore_init(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -
> > -	__intel_uncore_early_sanitize(dev, false);
> > +	bool sanitize_done = false;
> 
> I felt this looks quite clumsy. The only reason why you want to restrict
> calling __intel_uncore_early_sanitize() is that it does ellc_size
> detection and has a DRM_INFO right?
> 
> I think you want to pull that out of __intel_uncore_early_santize() into
> intel_uncore_init() itself (or better, it's own
> intel_uncore_ellc_detect()). ellc_size detection has nothing to do with
> sanitizing register state.
> 
> Then it should be simple to enough to sanitize twice, once with a
> comment in the code explaining how we verify that FORCEWAKE_MT is
> enabled by a manual forcewaked read of ECOBUS.

Also, why are we not calling fw_domain_reset() from fw_domain_init()?
That would be enough to avoid the early santize required for ivb, right?
-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

Re: [PATCH] drm/i915: Do uncore early sanitize after domain init

[Mika Kuoppala]

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

> On Wed, Jan 28, 2015 at 10:17:39AM +0000, Chris Wilson wrote:
>> On Wed, Jan 28, 2015 at 11:45:04AM +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.
>> > On ivb we need special care as there we need early forcewake access to
>> > determine the final configuration for the forcewake domain.
>> > 
>> > 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
>> > 
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88805
>> > Reported-by: Olof Johansson <olof@lixom.net>
>> > Tested-by: Darren Hart <dvhart@linux.intel.com>
>> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_uncore.c | 11 +++++++++--
>> >  1 file changed, 9 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> > index b3951f2..c438ca4 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);
>> >  
>> > @@ -987,8 +990,7 @@ static void fw_domain_init(struct drm_i915_private *dev_priv,
>> >  void intel_uncore_init(struct drm_device *dev)
>> >  {
>> >  	struct drm_i915_private *dev_priv = dev->dev_private;
>> > -
>> > -	__intel_uncore_early_sanitize(dev, false);
>> > +	bool sanitize_done = false;
>> 
>> I felt this looks quite clumsy. The only reason why you want to restrict
>> calling __intel_uncore_early_sanitize() is that it does ellc_size
>> detection and has a DRM_INFO right?
>> 
>> I think you want to pull that out of __intel_uncore_early_santize() into
>> intel_uncore_init() itself (or better, it's own
>> intel_uncore_ellc_detect()). ellc_size detection has nothing to do with
>> sanitizing register state.
>> 
>> Then it should be simple to enough to sanitize twice, once with a
>> comment in the code explaining how we verify that FORCEWAKE_MT is
>> enabled by a manual forcewaked read of ECOBUS.
>
> Also, why are we not calling fw_domain_reset() from fw_domain_init()?
> That would be enough to avoid the early santize required for ivb,
> right?

Agreed here. That was my plan originally, doing the sanitize inside 
in domain inits. But I wanted to fix this particular item by trying to
be as close as possible to the previous init/forcewake ordering on all gens. 

Reasoning is that I would like to see this stabilize a short while
before introducing further changes. I burned my fingers already touching
these, so they need to heal :)

Ok if this is for future work?

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

Re: [PATCH] drm/i915: Do uncore early sanitize after domain init

[Chris Wilson]

On Wed, Jan 28, 2015 at 12:59:52PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> > Also, why are we not calling fw_domain_reset() from fw_domain_init()?
> > That would be enough to avoid the early santize required for ivb,
> > right?
> 
> Agreed here. That was my plan originally, doing the sanitize inside 
> in domain inits. But I wanted to fix this particular item by trying to
> be as close as possible to the previous init/forcewake ordering on all gens. 
> 
> Reasoning is that I would like to see this stabilize a short while
> before introducing further changes. I burned my fingers already touching
> these, so they need to heal :)
> 
> Ok if this is for future work?

The bug is in -next, so take advantage of the guinea pigs...
-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

Re: [PATCH] drm/i915: Do uncore early sanitize after domain init

[Mika Kuoppala]

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

> On Wed, Jan 28, 2015 at 12:59:52PM +0200, Mika Kuoppala wrote:
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> > Also, why are we not calling fw_domain_reset() from fw_domain_init()?
>> > That would be enough to avoid the early santize required for ivb,
>> > right?
>> 
>> Agreed here. That was my plan originally, doing the sanitize inside 
>> in domain inits. But I wanted to fix this particular item by trying to
>> be as close as possible to the previous init/forcewake ordering on all gens. 
>> 
>> Reasoning is that I would like to see this stabilize a short while
>> before introducing further changes. I burned my fingers already touching
>> these, so they need to heal :)
>> 
>> Ok if this is for future work?
>
> The bug is in -next, so take advantage of the guinea pigs...

v2 in:

1422449006-4028-1-git-send-email-mika.kuoppala@intel.com

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

Re: [PATCH] drm/i915: Do uncore early sanitize after domain init

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5673
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  353/353              353/353
ILK                 -3              355/355              352/355
SNB                                  400/422              400/422
IVB              +1-1              485/487              485/487
BYT                                  296/296              296/296
HSW              +1                 507/508              508/508
BDW                                  401/402              401/402
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_drv_suspend_debugfs-reader      PASS(3, M26)      DMESG_WARN(1, M26)
*ILK  igt_gem_unfence_active_buffers      PASS(3, M26)      DMESG_WARN(1, M26)
*ILK  igt_gem_workarounds_suspend-resume      PASS(3, M26)      DMESG_WARN(1, M26)
 IVB  igt_gem_storedw_batches_loop_normal      DMESG_WARN(6, M34M4)PASS(16, M34M4M21)      PASS(1, M34)
 IVB  igt_gem_storedw_batches_loop_secure-dispatch      DMESG_WARN(1, M34)PASS(7, M34M4)      DMESG_WARN(1, M34)
 HSW  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance      DMESG_WARN(1, M40)PASS(20, M40M20)      PASS(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