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

From: Animesh Manna <animesh.manna@intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>,
	intel-gfx@lists.freedesktop.org,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [DMC_BUGFIX_SKL_V2 4/5] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present
Date: Wed, 26 Aug 2015 20:01:17 +0530	[thread overview]
Message-ID: <55DDCDB4.9080608@intel.com> (raw)
In-Reply-To: <20150826131159.GS1367@phenom.ffwll.local>

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

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