[PATCH 1/3] drm/i915: call lpt_init_clock_gating on BDW too

[PATCH 1/3] drm/i915: call lpt_init_clock_gating on BDW too

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Because BDW has WPT, which is equivalent to LPT. This is just like the
CPT/PPT case.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c8f744c..b3e948f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5595,6 +5595,8 @@ static void gen8_init_clock_gating(struct drm_device *dev)
 	/* Wa4x4STCOptimizationDisable:bdw */
 	I915_WRITE(CACHE_MODE_1,
 		   _MASKED_BIT_ENABLE(GEN8_4x4_STC_OPTIMIZATION_DISABLE));
+
+	lpt_init_clock_gating(dev);
 }
 
 static void haswell_init_clock_gating(struct drm_device *dev)
-- 
2.0.1

[PATCH 2/3] drm/i915: rename gen8_init_clock_gating to broadwell_init_clock_gating

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Because CHV uses cherryview_init_clock_gating instead of
gen8_init_clock_gating.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

I really wish we were using the three-letter-acronyms instead of the full names
on the functions. If I send another patch renaming everything, will anybody
complain?

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b3e948f..35495fc 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5502,7 +5502,7 @@ static void lpt_suspend_hw(struct drm_device *dev)
 	}
 }
 
-static void gen8_init_clock_gating(struct drm_device *dev)
+static void broadwell_init_clock_gating(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum pipe pipe;
@@ -7251,7 +7251,7 @@ void intel_init_pm(struct drm_device *dev)
 		else if (IS_HASWELL(dev))
 			dev_priv->display.init_clock_gating = haswell_init_clock_gating;
 		else if (INTEL_INFO(dev)->gen == 8)
-			dev_priv->display.init_clock_gating = gen8_init_clock_gating;
+			dev_priv->display.init_clock_gating = broadwell_init_clock_gating;
 	} else if (IS_CHERRYVIEW(dev)) {
 		dev_priv->display.update_wm = cherryview_update_wm;
 		dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm;
-- 
2.0.1

[PATCH 3/3] drm/i915: send PCI_D3hot adapter opregion message on BDW RPM suspend

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

On BDW we're seeing a problem that after we runtime resume, the
outputs connected to DDI C are not detected: they don't appear in the
SDEISR register and GMBUS transactions don't work. They stop working
at the moment we call intel_opregion_notify_adapter() during runtime
suspend, but they don't go back to work when we call the same function
during runtime resume. They only work after we do a modeset and call
intel_opregion_notify_encoder(), but this point is already too late.

While debugging, I tried to pass PCI_D3hot which is the value that
matches the spec, and it seems to have solved the problem. I couldn't
find any explanation of why this solves the problem, but there's also
no documented explanation - besides our code and git log - of why
Haswell should use PCI_D1, so keep this for now in order to keep BDW
runtime PM working.

Also add a comment to point the fact that there's no spec documenting
all the weirdness involved here.

Cc: kristen.c.accardi@intel.com
Testcase: igt/pm_rpm/drm-resources-equal
Testcase: igt/pm_rpm/i2c
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2dcc0d8..6f0c95f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1455,13 +1455,29 @@ static int intel_runtime_suspend(struct device *device)
 	dev_priv->pm.suspended = true;
 
 	/*
-	 * current versions of firmware which depend on this opregion
-	 * notification have repurposed the D1 definition to mean
-	 * "runtime suspended" vs. what you would normally expect (D3)
-	 * to distinguish it from notifications that might be sent
-	 * via the suspend path.
+	 * FIXME: We really should find a document that references the arguments
+	 * used below!
 	 */
-	intel_opregion_notify_adapter(dev, PCI_D1);
+	if (IS_HASWELL(dev)) {
+		/*
+		 * current versions of firmware which depend on this opregion
+		 * notification have repurposed the D1 definition to mean
+		 * "runtime suspended" vs. what you would normally expect (D3)
+		 * to distinguish it from notifications that might be sent via
+		 * the suspend path.
+		 */
+		intel_opregion_notify_adapter(dev, PCI_D1);
+	} else {
+		/*
+		 * On Broadwell, if we use PCI_D1 the PCH DDI ports will stop
+		 * being detected, and the call we do at intel_runtime_resume()
+		 * won't be able to restore them. Since PCI_D3hot matches the
+		 * actual specification and appears to be working, use it. Let's
+		 * assume the other non-Haswell platforms will stay the same as
+		 * Broadwell.
+		 */
+		intel_opregion_notify_adapter(dev, PCI_D3hot);
+	}
 
 	DRM_DEBUG_KMS("Device suspended\n");
 	return 0;
-- 
2.0.1

Re: [PATCH 1/3] drm/i915: call lpt_init_clock_gating on BDW too

[Damien Lespiau]

On Thu, Aug 21, 2014 at 05:09:36PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Because BDW has WPT, which is equivalent to LPT. This is just like the
> CPT/PPT case.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Maybe we should mention that Satheesh suggested the change with a:
Suggested-by: Satheeshakrishna M <satheeshakrishna.m@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c8f744c..b3e948f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5595,6 +5595,8 @@ static void gen8_init_clock_gating(struct drm_device *dev)
>  	/* Wa4x4STCOptimizationDisable:bdw */
>  	I915_WRITE(CACHE_MODE_1,
>  		   _MASKED_BIT_ENABLE(GEN8_4x4_STC_OPTIMIZATION_DISABLE));
> +
> +	lpt_init_clock_gating(dev);
>  }
>  
>  static void haswell_init_clock_gating(struct drm_device *dev)
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: rename gen8_init_clock_gating to broadwell_init_clock_gating

[Damien Lespiau]

On Thu, Aug 21, 2014 at 05:09:37PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Because CHV uses cherryview_init_clock_gating instead of
> gen8_init_clock_gating.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> I really wish we were using the three-letter-acronyms instead of the full names
> on the functions. If I send another patch renaming everything, will anybody
> complain?
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b3e948f..35495fc 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5502,7 +5502,7 @@ static void lpt_suspend_hw(struct drm_device *dev)
>  	}
>  }
>  
> -static void gen8_init_clock_gating(struct drm_device *dev)
> +static void broadwell_init_clock_gating(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum pipe pipe;
> @@ -7251,7 +7251,7 @@ void intel_init_pm(struct drm_device *dev)
>  		else if (IS_HASWELL(dev))
>  			dev_priv->display.init_clock_gating = haswell_init_clock_gating;
>  		else if (INTEL_INFO(dev)->gen == 8)
> -			dev_priv->display.init_clock_gating = gen8_init_clock_gating;
> +			dev_priv->display.init_clock_gating = broadwell_init_clock_gating;
>  	} else if (IS_CHERRYVIEW(dev)) {
>  		dev_priv->display.update_wm = cherryview_update_wm;
>  		dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm;
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: send PCI_D3hot adapter opregion message on BDW RPM suspend

[Damien Lespiau]

On Thu, Aug 21, 2014 at 05:09:38PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> On BDW we're seeing a problem that after we runtime resume, the
> outputs connected to DDI C are not detected: they don't appear in the
> SDEISR register and GMBUS transactions don't work. They stop working
> at the moment we call intel_opregion_notify_adapter() during runtime
> suspend, but they don't go back to work when we call the same function
> during runtime resume. They only work after we do a modeset and call
> intel_opregion_notify_encoder(), but this point is already too late.
> 
> While debugging, I tried to pass PCI_D3hot which is the value that
> matches the spec, and it seems to have solved the problem. I couldn't
> find any explanation of why this solves the problem, but there's also
> no documented explanation - besides our code and git log - of why
> Haswell should use PCI_D1, so keep this for now in order to keep BDW
> runtime PM working.
> 
> Also add a comment to point the fact that there's no spec documenting
> all the weirdness involved here.
> 
> Cc: kristen.c.accardi@intel.com
> Testcase: igt/pm_rpm/drm-resources-equal
> Testcase: igt/pm_rpm/i2c
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

There's still investigation to understand what the firmware is doing
behind our backs, but the change looks likely to be correct and fixes
the problem at hand so:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 2dcc0d8..6f0c95f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1455,13 +1455,29 @@ static int intel_runtime_suspend(struct device *device)
>  	dev_priv->pm.suspended = true;
>  
>  	/*
> -	 * current versions of firmware which depend on this opregion
> -	 * notification have repurposed the D1 definition to mean
> -	 * "runtime suspended" vs. what you would normally expect (D3)
> -	 * to distinguish it from notifications that might be sent
> -	 * via the suspend path.
> +	 * FIXME: We really should find a document that references the arguments
> +	 * used below!
>  	 */
> -	intel_opregion_notify_adapter(dev, PCI_D1);
> +	if (IS_HASWELL(dev)) {
> +		/*
> +		 * current versions of firmware which depend on this opregion
> +		 * notification have repurposed the D1 definition to mean
> +		 * "runtime suspended" vs. what you would normally expect (D3)
> +		 * to distinguish it from notifications that might be sent via
> +		 * the suspend path.
> +		 */
> +		intel_opregion_notify_adapter(dev, PCI_D1);
> +	} else {
> +		/*
> +		 * On Broadwell, if we use PCI_D1 the PCH DDI ports will stop
> +		 * being detected, and the call we do at intel_runtime_resume()
> +		 * won't be able to restore them. Since PCI_D3hot matches the
> +		 * actual specification and appears to be working, use it. Let's
> +		 * assume the other non-Haswell platforms will stay the same as
> +		 * Broadwell.
> +		 */
> +		intel_opregion_notify_adapter(dev, PCI_D3hot);
> +	}
>  
>  	DRM_DEBUG_KMS("Device suspended\n");
>  	return 0;
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: call lpt_init_clock_gating on BDW too

[Damien Lespiau]

On Thu, Aug 21, 2014 at 05:09:36PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Because BDW has WPT, which is equivalent to LPT. This is just like the
> CPT/PPT case.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

It'd be probably good to have drm/i915/bdw: in the subject to ease back
porting for product groups, and add those patches to a list someone
maintains (Rodrigo?)

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c8f744c..b3e948f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5595,6 +5595,8 @@ static void gen8_init_clock_gating(struct drm_device *dev)
>  	/* Wa4x4STCOptimizationDisable:bdw */
>  	I915_WRITE(CACHE_MODE_1,
>  		   _MASKED_BIT_ENABLE(GEN8_4x4_STC_OPTIMIZATION_DISABLE));
> +
> +	lpt_init_clock_gating(dev);
>  }
>  
>  static void haswell_init_clock_gating(struct drm_device *dev)
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: call lpt_init_clock_gating on BDW too

[Daniel Vetter]

On Thu, Aug 21, 2014 at 10:50:38PM +0100, Damien Lespiau wrote:
> On Thu, Aug 21, 2014 at 05:09:36PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Because BDW has WPT, which is equivalent to LPT. This is just like the
> > CPT/PPT case.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> It'd be probably good to have drm/i915/bdw: in the subject to ease back
> porting for product groups, and add those patches to a list someone
> maintains (Rodrigo?)

Forgotten your r-b tag or intentionally left blank?
-Daniel

> 
> -- 
> Damien
> 
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index c8f744c..b3e948f 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5595,6 +5595,8 @@ static void gen8_init_clock_gating(struct drm_device *dev)
> >  	/* Wa4x4STCOptimizationDisable:bdw */
> >  	I915_WRITE(CACHE_MODE_1,
> >  		   _MASKED_BIT_ENABLE(GEN8_4x4_STC_OPTIMIZATION_DISABLE));
> > +
> > +	lpt_init_clock_gating(dev);
> >  }
> >  
> >  static void haswell_init_clock_gating(struct drm_device *dev)
> > -- 
> > 2.0.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/3] drm/i915: call lpt_init_clock_gating on BDW too

[Damien Lespiau]

On Tue, Aug 26, 2014 at 07:16:34PM +0200, Daniel Vetter wrote:
> On Thu, Aug 21, 2014 at 10:50:38PM +0100, Damien Lespiau wrote:
> > On Thu, Aug 21, 2014 at 05:09:36PM -0300, Paulo Zanoni wrote:
> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > Because BDW has WPT, which is equivalent to LPT. This is just like the
> > > CPT/PPT case.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > It'd be probably good to have drm/i915/bdw: in the subject to ease back
> > porting for product groups, and add those patches to a list someone
> > maintains (Rodrigo?)
> 
> Forgotten your r-b tag or intentionally left blank?

It's in 20140821214159.GA29975@strange.ger.corp.intel.com

-- 
Damien

Re: [PATCH 3/3] drm/i915: send PCI_D3hot adapter opregion message on BDW RPM suspend

[Daniel Vetter]

On Thu, Aug 21, 2014 at 10:47:12PM +0100, Damien Lespiau wrote:
> On Thu, Aug 21, 2014 at 05:09:38PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > On BDW we're seeing a problem that after we runtime resume, the
> > outputs connected to DDI C are not detected: they don't appear in the
> > SDEISR register and GMBUS transactions don't work. They stop working
> > at the moment we call intel_opregion_notify_adapter() during runtime
> > suspend, but they don't go back to work when we call the same function
> > during runtime resume. They only work after we do a modeset and call
> > intel_opregion_notify_encoder(), but this point is already too late.
> > 
> > While debugging, I tried to pass PCI_D3hot which is the value that
> > matches the spec, and it seems to have solved the problem. I couldn't
> > find any explanation of why this solves the problem, but there's also
> > no documented explanation - besides our code and git log - of why
> > Haswell should use PCI_D1, so keep this for now in order to keep BDW
> > runtime PM working.
> > 
> > Also add a comment to point the fact that there's no spec documenting
> > all the weirdness involved here.
> > 
> > Cc: kristen.c.accardi@intel.com
> > Testcase: igt/pm_rpm/drm-resources-equal
> > Testcase: igt/pm_rpm/i2c
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> There's still investigation to understand what the firmware is doing
> behind our backs, but the change looks likely to be correct and fixes
> the problem at hand so:
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> 
> -- 
> Damien
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 28 ++++++++++++++++++++++------
> >  1 file changed, 22 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 2dcc0d8..6f0c95f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1455,13 +1455,29 @@ static int intel_runtime_suspend(struct device *device)
> >  	dev_priv->pm.suspended = true;
> >  
> >  	/*
> > -	 * current versions of firmware which depend on this opregion
> > -	 * notification have repurposed the D1 definition to mean
> > -	 * "runtime suspended" vs. what you would normally expect (D3)
> > -	 * to distinguish it from notifications that might be sent
> > -	 * via the suspend path.
> > +	 * FIXME: We really should find a document that references the arguments
> > +	 * used below!
> >  	 */
> > -	intel_opregion_notify_adapter(dev, PCI_D1);
> > +	if (IS_HASWELL(dev)) {
> > +		/*
> > +		 * current versions of firmware which depend on this opregion
> > +		 * notification have repurposed the D1 definition to mean
> > +		 * "runtime suspended" vs. what you would normally expect (D3)
> > +		 * to distinguish it from notifications that might be sent via
> > +		 * the suspend path.
> > +		 */
> > +		intel_opregion_notify_adapter(dev, PCI_D1);
> > +	} else {
> > +		/*
> > +		 * On Broadwell, if we use PCI_D1 the PCH DDI ports will stop
> > +		 * being detected, and the call we do at intel_runtime_resume()
> > +		 * won't be able to restore them. Since PCI_D3hot matches the
> > +		 * actual specification and appears to be working, use it. Let's
> > +		 * assume the other non-Haswell platforms will stay the same as
> > +		 * Broadwell.
> > +		 */
> > +		intel_opregion_notify_adapter(dev, PCI_D3hot);

I guess we could start to open betting pools whether this will grow until
we have all the pci states or not ;-)

Anyway, all 3 patches merged to dinq, thanks.
-Daniel

> > +	}
> >  
> >  	DRM_DEBUG_KMS("Device suspended\n");
> >  	return 0;
> > -- 
> > 2.0.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch