diff mbox

[DMC_BUGFIX_SKL_V2,4/5] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present

Message ID 1440533169-32265-5-git-send-email-animesh.manna@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Manna, Animesh Aug. 25, 2015, 8:06 p.m. UTC 
While display engine entering into low power state no need to disable
cdclk pll as CSR firmware of dmc will take care. If pll is already
enabled firmware execution sequence will be blocked. This is one
of the criteria for dmc to work properly.

v1: Initial version.

v2: Based on review comment from Daniel added code commnent.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Daniel Vetter Aug. 26, 2015, 1:11 p.m. UTC | #1 
On Wed, Aug 26, 2015 at 01:36:08AM +0530, Animesh Manna wrote:
> While display engine entering into low power state no need to disable
> cdclk pll as CSR firmware of dmc will take care. If pll is already
> enabled firmware execution sequence will be blocked. This is one
> of the criteria for dmc to work properly.
> 
> v1: Initial version.
> 
> v2: Based on review comment from Daniel added code commnent.
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f604ce1..b6bef20 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5687,10 +5687,16 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>  	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
>  		DRM_ERROR("DBuf power disable timeout\n");
>  
> -	/* disable DPLL0 */
> -	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
> -	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> -		DRM_ERROR("Couldn't disable DPLL0\n");
> +	/*
> +	 * DMC assumes ownership of LCPLL and will get confused if we touch it.

This should get a FIXME - once we have dmc loading fixed up we require the
firmware and there's no point in this check any more. Flexibilty just
because is something we simply don't have the developer and validation
resources for.
-Daniel

> +	 */
> +	if (dev_priv->csr.dmc_payload) {
> +		/* disable DPLL0 */
> +		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
> +					~LCPLL_PLL_ENABLE);
> +		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> +			DRM_ERROR("Couldn't disable DPLL0\n");
> +	}
>  
>  	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
>  }
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Manna, Animesh Aug. 26, 2015, 2:31 p.m. UTC | #2 
On 8/26/2015 6:41 PM, Daniel Vetter wrote:
> On Wed, Aug 26, 2015 at 01:36:08AM +0530, Animesh Manna wrote:
>> While display engine entering into low power state no need to disable
>> cdclk pll as CSR firmware of dmc will take care. If pll is already
>> enabled firmware execution sequence will be blocked. This is one
>> of the criteria for dmc to work properly.
>>
>> v1: Initial version.
>>
>> v2: Based on review comment from Daniel added code commnent.
>>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Sunil Kamath <sunil.kamath@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index f604ce1..b6bef20 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5687,10 +5687,16 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>>   	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
>>   		DRM_ERROR("DBuf power disable timeout\n");
>>   
>> -	/* disable DPLL0 */
>> -	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
>> -	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
>> -		DRM_ERROR("Couldn't disable DPLL0\n");
>> +	/*
>> +	 * DMC assumes ownership of LCPLL and will get confused if we touch it.
> This should get a FIXME - once we have dmc loading fixed up we require the
> firmware and there's no point in this check any more. Flexibilty just
> because is something we simply don't have the developer and validation
> resources for.
> -Daniel

I am not sure if dmc firmware is mandatory for all user.
But the code is written based on design principle published in below series
http://www.spinics.net/lists/intel-gfx/msg72399.html

Job of Dmc firmware is to disable pw1 and cdclk pll during suspend.
If dmc firmware is not present driver can disable cdclk pll to save some more power.

Am I missing anything?

-Animesh

>
>> +	 */
>> +	if (dev_priv->csr.dmc_payload) {
>> +		/* disable DPLL0 */
>> +		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
>> +					~LCPLL_PLL_ENABLE);
>> +		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
>> +			DRM_ERROR("Couldn't disable DPLL0\n");
>> +	}
>>   
>>   	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
>>   }
>> -- 
>> 2.0.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hindman, Gavin Aug. 31, 2015, 1:03 a.m. UTC | #3 
Unless I'm misreading that would imply that we are moving away from our previous position that DMC FW is optional, correct?    Would this not render power-sequencing broken if a distro chose not to include DMC FW?

Gavin Hindman
Senior Program Manager
SSG/OTC – Open Source Technology Center


-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Daniel Vetter

Sent: Wednesday, August 26, 2015 6:12 AM
To: Manna, Animesh
Cc: Bhardwaj, Rajneesh; intel-gfx@lists.freedesktop.org; Vetter, Daniel
Subject: Re: [Intel-gfx] [DMC_BUGFIX_SKL_V2 4/5] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present

On Wed, Aug 26, 2015 at 01:36:08AM +0530, Animesh Manna wrote:
> While display engine entering into low power state no need to disable 

> cdclk pll as CSR firmware of dmc will take care. If pll is already 

> enabled firmware execution sequence will be blocked. This is one of 

> the criteria for dmc to work properly.

> 

> v1: Initial version.

> 

> v2: Based on review comment from Daniel added code commnent.

> 

> Cc: Daniel Vetter <daniel.vetter@intel.com>

> Cc: Damien Lespiau <damien.lespiau@intel.com>

> Cc: Imre Deak <imre.deak@intel.com>

> Cc: Sunil Kamath <sunil.kamath@intel.com>

> Signed-off-by: Animesh Manna <animesh.manna@intel.com>

> Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>

> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>

> ---

>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++----

>  1 file changed, 10 insertions(+), 4 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/intel_display.c 

> b/drivers/gpu/drm/i915/intel_display.c

> index f604ce1..b6bef20 100644

> --- a/drivers/gpu/drm/i915/intel_display.c

> +++ b/drivers/gpu/drm/i915/intel_display.c

> @@ -5687,10 +5687,16 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)

>  	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)

>  		DRM_ERROR("DBuf power disable timeout\n");

>  

> -	/* disable DPLL0 */

> -	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);

> -	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))

> -		DRM_ERROR("Couldn't disable DPLL0\n");

> +	/*

> +	 * DMC assumes ownership of LCPLL and will get confused if we touch it.


This should get a FIXME - once we have dmc loading fixed up we require the firmware and there's no point in this check any more. Flexibilty just because is something we simply don't have the developer and validation resources for.
-Daniel

> +	 */

> +	if (dev_priv->csr.dmc_payload) {

> +		/* disable DPLL0 */

> +		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &

> +					~LCPLL_PLL_ENABLE);

> +		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))

> +			DRM_ERROR("Couldn't disable DPLL0\n");

> +	}

>  

>  	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);  }

> --

> 2.0.2

> 

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Sept. 2, 2015, 8:58 a.m. UTC | #4 
On Mon, Aug 31, 2015 at 01:03:03AM +0000, Hindman, Gavin wrote:
> Unless I'm misreading that would imply that we are moving away from our
> previous position that DMC FW is optional, correct?    Would this not
> render power-sequencing broken if a distro chose not to include DMC FW?

For upstream we never had the stance (and I wouldn't accept the patches
really without a really good reason) that we'll support runtime pm without
DMC firmware.

We already have a really bad time just trying to keep things working in 1
configuration (there's a patch from me to enable runtime pm by default
blocked on unfixed regressions which are open since months and no one's
working on them), we absolutely don't have the resources to support any
crazy configurations that's theoretically possible. If a distro won't
include DMC then they won't get proper runtime pm and that's it.

Yes the original DMC patches (and the code still in tree) had code to
handle that case, but follow-up patches rip that complexity out.

If we are in position where we can actually ship all the power features we
develop then we could consider supporting more combinations of features,
but right now I think that's in the very distant future.

Thanks, Daniel

> 
> Gavin Hindman
> Senior Program Manager
> SSG/OTC – Open Source Technology Center
> 
> 
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Daniel Vetter
> Sent: Wednesday, August 26, 2015 6:12 AM
> To: Manna, Animesh
> Cc: Bhardwaj, Rajneesh; intel-gfx@lists.freedesktop.org; Vetter, Daniel
> Subject: Re: [Intel-gfx] [DMC_BUGFIX_SKL_V2 4/5] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present
> 
> On Wed, Aug 26, 2015 at 01:36:08AM +0530, Animesh Manna wrote:
> > While display engine entering into low power state no need to disable 
> > cdclk pll as CSR firmware of dmc will take care. If pll is already 
> > enabled firmware execution sequence will be blocked. This is one of 
> > the criteria for dmc to work properly.
> > 
> > v1: Initial version.
> > 
> > v2: Based on review comment from Daniel added code commnent.
> > 
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Damien Lespiau <damien.lespiau@intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Sunil Kamath <sunil.kamath@intel.com>
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index f604ce1..b6bef20 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5687,10 +5687,16 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
> >  	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
> >  		DRM_ERROR("DBuf power disable timeout\n");
> >  
> > -	/* disable DPLL0 */
> > -	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
> > -	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> > -		DRM_ERROR("Couldn't disable DPLL0\n");
> > +	/*
> > +	 * DMC assumes ownership of LCPLL and will get confused if we touch it.
> 
> This should get a FIXME - once we have dmc loading fixed up we require the firmware and there's no point in this check any more. Flexibilty just because is something we simply don't have the developer and validation resources for.
> -Daniel
> 
> > +	 */
> > +	if (dev_priv->csr.dmc_payload) {
> > +		/* disable DPLL0 */
> > +		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
> > +					~LCPLL_PLL_ENABLE);
> > +		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> > +			DRM_ERROR("Couldn't disable DPLL0\n");
> > +	}
> >  
> >  	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);  }
> > --
> > 2.0.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Kamath, Sunil Sept. 7, 2015, 11:07 a.m. UTC | #5 
On Wednesday 26 August 2015 01:36 AM, Animesh Manna wrote:
> While display engine entering into low power state no need to disable
> cdclk pll as CSR firmware of dmc will take care. If pll is already
> enabled firmware execution sequence will be blocked. This is one
> of the criteria for dmc to work properly.
>
> v1: Initial version.
>
> v2: Based on review comment from Daniel added code commnent.
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f604ce1..b6bef20 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5687,10 +5687,16 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>   	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
>   		DRM_ERROR("DBuf power disable timeout\n");
>   
> -	/* disable DPLL0 */
> -	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
> -	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> -		DRM_ERROR("Couldn't disable DPLL0\n");
> +	/*
> +	 * DMC assumes ownership of LCPLL and will get confused if we touch it.
> +	 */
> +	if (dev_priv->csr.dmc_payload) {
> +		/* disable DPLL0 */
> +		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
> +					~LCPLL_PLL_ENABLE);
> +		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> +			DRM_ERROR("Couldn't disable DPLL0\n");
> +	}
>   
>   	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
>   }

Valid fix and patch is ready for merge now.

Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com> 
<mailto:sunil.kamath@intel.com>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f604ce1..b6bef20 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5687,10 +5687,16 @@  void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
 	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
 		DRM_ERROR("DBuf power disable timeout\n");
 
-	/* disable DPLL0 */
-	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
-	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
-		DRM_ERROR("Couldn't disable DPLL0\n");
+	/*
+	 * DMC assumes ownership of LCPLL and will get confused if we touch it.
+	 */
+	if (dev_priv->csr.dmc_payload) {
+		/* disable DPLL0 */
+		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
+					~LCPLL_PLL_ENABLE);
+		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
+			DRM_ERROR("Couldn't disable DPLL0\n");
+	}
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
 }