[PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Shobhit Kumar]

Mostly reuse what is programmed by pre-os, but in case there is no
pre-os initialization, init the cdclk with the default value.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 2d3cc82..675c60d 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
 
 		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
 		dev_priv->skl_boot_cdclk = cdclk_freq;
-		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
-			DRM_ERROR("LCPLL1 is disabled\n");
-		else
-			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
+
+		skl_init_cdclk(dev_priv);
 	} else if (IS_BROXTON(dev)) {
 		broxton_init_cdclk(dev);
 		broxton_ddi_phy_init(dev);
-- 
2.4.3

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

Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Imre Deak]

On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
> Mostly reuse what is programmed by pre-os, but in case there is no
> pre-os initialization, init the cdclk with the default value.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 2d3cc82..675c60d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
>  
>  		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>  		dev_priv->skl_boot_cdclk = cdclk_freq;
> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> -			DRM_ERROR("LCPLL1 is disabled\n");
> -		else
> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> +
> +		skl_init_cdclk(dev_priv);

How does this prevent changing the clock if BIOS did enable some output?
We shouldn't change the clock in that case.

>  	} else if (IS_BROXTON(dev)) {
>  		broxton_init_cdclk(dev);
>  		broxton_ddi_phy_init(dev);


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

Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Jani Nikula]

On Mon, 05 Oct 2015, Imre Deak <imre.deak@intel.com> wrote:
> On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
>> Mostly reuse what is programmed by pre-os, but in case there is no
>> pre-os initialization, init the cdclk with the default value.
>> 
>> Cc: Imre Deak <imre.deak@intel.com>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 2d3cc82..675c60d 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
>>  
>>  		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>>  		dev_priv->skl_boot_cdclk = cdclk_freq;
>> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>> -			DRM_ERROR("LCPLL1 is disabled\n");
>> -		else
>> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>> +
>> +		skl_init_cdclk(dev_priv);
>
> How does this prevent changing the clock if BIOS did enable some output?
> We shouldn't change the clock in that case.

Random comment from the back, what about intentional cdclk change on
module load? We're supposed to have that for bdw [1] but last I checked
it failed... [2].

BR,
Jani.

[1] http://mid.gmane.org/1433335514-4156-8-git-send-email-mika.kahola@intel.com
[2] http://mid.gmane.org/87bngf4xk3.fsf@intel.com


>
>>  	} else if (IS_BROXTON(dev)) {
>>  		broxton_init_cdclk(dev);
>>  		broxton_ddi_phy_init(dev);
>
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Kumar, Shobhit]

On 10/05/2015 09:05 PM, Imre Deak wrote:
> On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
>> Mostly reuse what is programmed by pre-os, but in case there is no
>> pre-os initialization, init the cdclk with the default value.
>>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 2d3cc82..675c60d 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
>>
>>   		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>>   		dev_priv->skl_boot_cdclk = cdclk_freq;
>> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>> -			DRM_ERROR("LCPLL1 is disabled\n");
>> -		else
>> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>> +
>> +		skl_init_cdclk(dev_priv);
>
> How does this prevent changing the clock if BIOS did enable some output?
> We shouldn't change the clock in that case.

In that case it will try to re-apply the same clock that BIOS enabled. 
Not sure if this is allowed, but I checked the cdclock change sequence 
and it is mostly followed in skl_init_cdclk. In my tests where BIOS does 
enable this, I faced no issues in initializing again in driver. I have 
noticed on some pre-os this value is programmed correctly except for the 
decimal part. That causes AUX transactions to fail on SKl. That is what 
triggered this patch actually. So other way is to completely validate 
the value in get_display_clock_speed instead of bit[28:26] and then if 
wrong then only do the cdclk init.

Regards
Shobhit

>
>>   	} else if (IS_BROXTON(dev)) {
>>   		broxton_init_cdclk(dev);
>>   		broxton_ddi_phy_init(dev);
>
>
> _______________________________________________
> 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

Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Kumar, Shobhit]

On 10/06/2015 12:05 PM, Jani Nikula wrote:
> On Mon, 05 Oct 2015, Imre Deak <imre.deak@intel.com> wrote:
>> On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
>>> Mostly reuse what is programmed by pre-os, but in case there is no
>>> pre-os initialization, init the cdclk with the default value.
>>>
>>> Cc: Imre Deak <imre.deak@intel.com>
>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>> index 2d3cc82..675c60d 100644
>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>> @@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
>>>
>>>   		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>>>   		dev_priv->skl_boot_cdclk = cdclk_freq;
>>> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>>> -			DRM_ERROR("LCPLL1 is disabled\n");
>>> -		else
>>> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>>> +
>>> +		skl_init_cdclk(dev_priv);
>>
>> How does this prevent changing the clock if BIOS did enable some output?
>> We shouldn't change the clock in that case.
>
> Random comment from the back, what about intentional cdclk change on
> module load? We're supposed to have that for bdw [1] but last I checked
> it failed... [2].

Yes. We will require cdclk change run time as well say if we hot plug a 
DP 4k@60. So this capability is something that might really be needed.

Regards
Shobhit

>
> BR,
> Jani.
>
> [1] http://mid.gmane.org/1433335514-4156-8-git-send-email-mika.kahola@intel.com
> [2] http://mid.gmane.org/87bngf4xk3.fsf@intel.com
>
>
>>
>>>   	} else if (IS_BROXTON(dev)) {
>>>   		broxton_init_cdclk(dev);
>>>   		broxton_ddi_phy_init(dev);
>>
>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Imre Deak]

On ti, 2015-10-06 at 15:26 +0530, Kumar, Shobhit wrote:
> On 10/05/2015 09:05 PM, Imre Deak wrote:
> > On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
> >> Mostly reuse what is programmed by pre-os, but in case there is no
> >> pre-os initialization, init the cdclk with the default value.
> >>
> >> Cc: Imre Deak <imre.deak@intel.com>
> >> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
> >>   1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index 2d3cc82..675c60d 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
> >>
> >>   		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
> >>   		dev_priv->skl_boot_cdclk = cdclk_freq;
> >> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> >> -			DRM_ERROR("LCPLL1 is disabled\n");
> >> -		else
> >> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> >> +
> >> +		skl_init_cdclk(dev_priv);
> >
> > How does this prevent changing the clock if BIOS did enable some output?
> > We shouldn't change the clock in that case.
> 
> In that case it will try to re-apply the same clock that BIOS enabled. 
> Not sure if this is allowed, but I checked the cdclock change sequence 
> and it is mostly followed in skl_init_cdclk.
> In my tests where BIOS does enable this, I faced no issues in
> initializing again in driver.

The first step in that sequence:
"Disable all display engine functions using the full mode set disable
sequence on all pipes, ports, and planes."

So the problem is not that the PLL itself may be enabled here (as BIOS
left it), but that some output is also enabled.

> I have noticed on some pre-os this value is programmed correctly except
> for the decimal part. That causes AUX transactions to fail on SKl. That
> is what triggered this patch actually. So other way is to completely
> validate the value in get_display_clock_speed instead of bit[28:26] and
> then if wrong then only do the cdclk init.

I think we'd need to detect at this point if outputs are enabled and
only attempt to work around the above BIOS problem if this is not the
case. Alternatively you could also disable the active outputs as a first
step.

> 
> Regards
> Shobhit
> 
> >
> >>   	} else if (IS_BROXTON(dev)) {
> >>   		broxton_init_cdclk(dev);
> >>   		broxton_ddi_phy_init(dev);
> >
> >
> > _______________________________________________
> > 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

Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Kumar, Shobhit]

On 10/06/2015 04:11 PM, Imre Deak wrote:
> On ti, 2015-10-06 at 15:26 +0530, Kumar, Shobhit wrote:
>> On 10/05/2015 09:05 PM, Imre Deak wrote:
>>> On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
>>>> Mostly reuse what is programmed by pre-os, but in case there is no
>>>> pre-os initialization, init the cdclk with the default value.
>>>>
>>>> Cc: Imre Deak <imre.deak@intel.com>
>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
>>>>    1 file changed, 2 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>> index 2d3cc82..675c60d 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
>>>>
>>>>    		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>>>>    		dev_priv->skl_boot_cdclk = cdclk_freq;
>>>> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>>>> -			DRM_ERROR("LCPLL1 is disabled\n");
>>>> -		else
>>>> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>>>> +
>>>> +		skl_init_cdclk(dev_priv);
>>>
>>> How does this prevent changing the clock if BIOS did enable some output?
>>> We shouldn't change the clock in that case.
>>
>> In that case it will try to re-apply the same clock that BIOS enabled.
>> Not sure if this is allowed, but I checked the cdclock change sequence
>> and it is mostly followed in skl_init_cdclk.
>> In my tests where BIOS does enable this, I faced no issues in
>> initializing again in driver.
>
> The first step in that sequence:
> "Disable all display engine functions using the full mode set disable
> sequence on all pipes, ports, and planes."

Oh, yeah, I again made mistake of assuming that display is not enabled 
in the first place. You are right, though it works if I change the clock 
again.

>
> So the problem is not that the PLL itself may be enabled here (as BIOS
> left it), but that some output is also enabled.

Yes.

>
>> I have noticed on some pre-os this value is programmed correctly except
>> for the decimal part. That causes AUX transactions to fail on SKl. That
>> is what triggered this patch actually. So other way is to completely
>> validate the value in get_display_clock_speed instead of bit[28:26] and
>> then if wrong then only do the cdclk init.
>
> I think we'd need to detect at this point if outputs are enabled and
> only attempt to work around the above BIOS problem if this is not the
> case. Alternatively you could also disable the active outputs as a first
> step.

Ok, let me detect if any output is enabled by BIOS and accordingly 
initialize cdclk.

Regards
Shobhit

>
>>
>> Regards
>> Shobhit
>>
>>>
>>>>    	} else if (IS_BROXTON(dev)) {
>>>>    		broxton_init_cdclk(dev);
>>>>    		broxton_ddi_phy_init(dev);
>>>
>>>
>>> _______________________________________________
>>> 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

Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Daniel Vetter]

On Tue, Oct 06, 2015 at 04:33:43PM +0530, Kumar, Shobhit wrote:
> On 10/06/2015 04:11 PM, Imre Deak wrote:
> >On ti, 2015-10-06 at 15:26 +0530, Kumar, Shobhit wrote:
> >>On 10/05/2015 09:05 PM, Imre Deak wrote:
> >>>On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
> >>>>Mostly reuse what is programmed by pre-os, but in case there is no
> >>>>pre-os initialization, init the cdclk with the default value.
> >>>>
> >>>>Cc: Imre Deak <imre.deak@intel.com>
> >>>>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >>>>---
> >>>>   drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
> >>>>   1 file changed, 2 insertions(+), 4 deletions(-)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >>>>index 2d3cc82..675c60d 100644
> >>>>--- a/drivers/gpu/drm/i915/intel_ddi.c
> >>>>+++ b/drivers/gpu/drm/i915/intel_ddi.c
> >>>>@@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
> >>>>
> >>>>   		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
> >>>>   		dev_priv->skl_boot_cdclk = cdclk_freq;
> >>>>-		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> >>>>-			DRM_ERROR("LCPLL1 is disabled\n");
> >>>>-		else
> >>>>-			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> >>>>+
> >>>>+		skl_init_cdclk(dev_priv);
> >>>
> >>>How does this prevent changing the clock if BIOS did enable some output?
> >>>We shouldn't change the clock in that case.
> >>
> >>In that case it will try to re-apply the same clock that BIOS enabled.
> >>Not sure if this is allowed, but I checked the cdclock change sequence
> >>and it is mostly followed in skl_init_cdclk.
> >>In my tests where BIOS does enable this, I faced no issues in
> >>initializing again in driver.
> >
> >The first step in that sequence:
> >"Disable all display engine functions using the full mode set disable
> >sequence on all pipes, ports, and planes."
> 
> Oh, yeah, I again made mistake of assuming that display is not enabled in
> the first place. You are right, though it works if I change the clock again.
> 
> >
> >So the problem is not that the PLL itself may be enabled here (as BIOS
> >left it), but that some output is also enabled.
> 
> Yes.
> 
> >
> >>I have noticed on some pre-os this value is programmed correctly except
> >>for the decimal part. That causes AUX transactions to fail on SKl. That
> >>is what triggered this patch actually. So other way is to completely
> >>validate the value in get_display_clock_speed instead of bit[28:26] and
> >>then if wrong then only do the cdclk init.
> >
> >I think we'd need to detect at this point if outputs are enabled and
> >only attempt to work around the above BIOS problem if this is not the
> >case. Alternatively you could also disable the active outputs as a first
> >step.
> 
> Ok, let me detect if any output is enabled by BIOS and accordingly
> initialize cdclk.

These kind of fixiups should be done after the hw state readout. We
already have sanitize_crtc/pll/encoder functions, probably best if we add
a sanitize_cdclk or similar for this at the very end of the hw state
sanitize sequence.
-Daniel
-- 
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

Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Ville Syrjälä]

On Tue, Oct 06, 2015 at 01:19:52PM +0200, Daniel Vetter wrote:
> On Tue, Oct 06, 2015 at 04:33:43PM +0530, Kumar, Shobhit wrote:
> > On 10/06/2015 04:11 PM, Imre Deak wrote:
> > >On ti, 2015-10-06 at 15:26 +0530, Kumar, Shobhit wrote:
> > >>On 10/05/2015 09:05 PM, Imre Deak wrote:
> > >>>On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
> > >>>>Mostly reuse what is programmed by pre-os, but in case there is no
> > >>>>pre-os initialization, init the cdclk with the default value.
> > >>>>
> > >>>>Cc: Imre Deak <imre.deak@intel.com>
> > >>>>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> > >>>>---
> > >>>>   drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
> > >>>>   1 file changed, 2 insertions(+), 4 deletions(-)
> > >>>>
> > >>>>diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > >>>>index 2d3cc82..675c60d 100644
> > >>>>--- a/drivers/gpu/drm/i915/intel_ddi.c
> > >>>>+++ b/drivers/gpu/drm/i915/intel_ddi.c
> > >>>>@@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
> > >>>>
> > >>>>   		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
> > >>>>   		dev_priv->skl_boot_cdclk = cdclk_freq;
> > >>>>-		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> > >>>>-			DRM_ERROR("LCPLL1 is disabled\n");
> > >>>>-		else
> > >>>>-			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> > >>>>+
> > >>>>+		skl_init_cdclk(dev_priv);
> > >>>
> > >>>How does this prevent changing the clock if BIOS did enable some output?
> > >>>We shouldn't change the clock in that case.
> > >>
> > >>In that case it will try to re-apply the same clock that BIOS enabled.
> > >>Not sure if this is allowed, but I checked the cdclock change sequence
> > >>and it is mostly followed in skl_init_cdclk.
> > >>In my tests where BIOS does enable this, I faced no issues in
> > >>initializing again in driver.
> > >
> > >The first step in that sequence:
> > >"Disable all display engine functions using the full mode set disable
> > >sequence on all pipes, ports, and planes."
> > 
> > Oh, yeah, I again made mistake of assuming that display is not enabled in
> > the first place. You are right, though it works if I change the clock again.
> > 
> > >
> > >So the problem is not that the PLL itself may be enabled here (as BIOS
> > >left it), but that some output is also enabled.
> > 
> > Yes.
> > 
> > >
> > >>I have noticed on some pre-os this value is programmed correctly except
> > >>for the decimal part. That causes AUX transactions to fail on SKl. That
> > >>is what triggered this patch actually. So other way is to completely
> > >>validate the value in get_display_clock_speed instead of bit[28:26] and
> > >>then if wrong then only do the cdclk init.
> > >
> > >I think we'd need to detect at this point if outputs are enabled and
> > >only attempt to work around the above BIOS problem if this is not the
> > >case. Alternatively you could also disable the active outputs as a first
> > >step.
> > 
> > Ok, let me detect if any output is enabled by BIOS and accordingly
> > initialize cdclk.
> 
> These kind of fixiups should be done after the hw state readout. We
> already have sanitize_crtc/pll/encoder functions, probably best if we add
> a sanitize_cdclk or similar for this at the very end of the hw state
> sanitize sequence.

Can't be done if we already need a somewhat sane cdclk for the
eDP AUX probing and whatnot.

For actually enabling the cdclk for pushing pixels, we wouldn't need
to do anything except actually plug ia a calc_cdclk for SKL. No idea
why we're not doing that currently. Some extra care may be needed
due to the eDP DPLL0 usag IIRC.

> -Daniel
> -- 
> 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

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

Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Daniel Vetter]

On Tue, Oct 06, 2015 at 02:41:44PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 06, 2015 at 01:19:52PM +0200, Daniel Vetter wrote:
> > On Tue, Oct 06, 2015 at 04:33:43PM +0530, Kumar, Shobhit wrote:
> > > On 10/06/2015 04:11 PM, Imre Deak wrote:
> > > >On ti, 2015-10-06 at 15:26 +0530, Kumar, Shobhit wrote:
> > > >>On 10/05/2015 09:05 PM, Imre Deak wrote:
> > > >>>On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
> > > >>>>Mostly reuse what is programmed by pre-os, but in case there is no
> > > >>>>pre-os initialization, init the cdclk with the default value.
> > > >>>>
> > > >>>>Cc: Imre Deak <imre.deak@intel.com>
> > > >>>>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> > > >>>>---
> > > >>>>   drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
> > > >>>>   1 file changed, 2 insertions(+), 4 deletions(-)
> > > >>>>
> > > >>>>diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > >>>>index 2d3cc82..675c60d 100644
> > > >>>>--- a/drivers/gpu/drm/i915/intel_ddi.c
> > > >>>>+++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > >>>>@@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
> > > >>>>
> > > >>>>   		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
> > > >>>>   		dev_priv->skl_boot_cdclk = cdclk_freq;
> > > >>>>-		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> > > >>>>-			DRM_ERROR("LCPLL1 is disabled\n");
> > > >>>>-		else
> > > >>>>-			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> > > >>>>+
> > > >>>>+		skl_init_cdclk(dev_priv);
> > > >>>
> > > >>>How does this prevent changing the clock if BIOS did enable some output?
> > > >>>We shouldn't change the clock in that case.
> > > >>
> > > >>In that case it will try to re-apply the same clock that BIOS enabled.
> > > >>Not sure if this is allowed, but I checked the cdclock change sequence
> > > >>and it is mostly followed in skl_init_cdclk.
> > > >>In my tests where BIOS does enable this, I faced no issues in
> > > >>initializing again in driver.
> > > >
> > > >The first step in that sequence:
> > > >"Disable all display engine functions using the full mode set disable
> > > >sequence on all pipes, ports, and planes."
> > > 
> > > Oh, yeah, I again made mistake of assuming that display is not enabled in
> > > the first place. You are right, though it works if I change the clock again.
> > > 
> > > >
> > > >So the problem is not that the PLL itself may be enabled here (as BIOS
> > > >left it), but that some output is also enabled.
> > > 
> > > Yes.
> > > 
> > > >
> > > >>I have noticed on some pre-os this value is programmed correctly except
> > > >>for the decimal part. That causes AUX transactions to fail on SKl. That
> > > >>is what triggered this patch actually. So other way is to completely
> > > >>validate the value in get_display_clock_speed instead of bit[28:26] and
> > > >>then if wrong then only do the cdclk init.
> > > >
> > > >I think we'd need to detect at this point if outputs are enabled and
> > > >only attempt to work around the above BIOS problem if this is not the
> > > >case. Alternatively you could also disable the active outputs as a first
> > > >step.
> > > 
> > > Ok, let me detect if any output is enabled by BIOS and accordingly
> > > initialize cdclk.
> > 
> > These kind of fixiups should be done after the hw state readout. We
> > already have sanitize_crtc/pll/encoder functions, probably best if we add
> > a sanitize_cdclk or similar for this at the very end of the hw state
> > sanitize sequence.
> 
> Can't be done if we already need a somewhat sane cdclk for the
> eDP AUX probing and whatnot.
> 
> For actually enabling the cdclk for pushing pixels, we wouldn't need
> to do anything except actually plug ia a calc_cdclk for SKL. No idea
> why we're not doing that currently. Some extra care may be needed
> due to the eDP DPLL0 usag IIRC.

Hm right, cdlck is in the top-level power domain. Added fun is that with
dmc the firmware is supposed to handle it. Messy :(
-Daniel
-- 
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

Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Kumar, Shobhit]

On 10/06/2015 05:49 PM, Daniel Vetter wrote:
> On Tue, Oct 06, 2015 at 02:41:44PM +0300, Ville Syrjälä wrote:
>> On Tue, Oct 06, 2015 at 01:19:52PM +0200, Daniel Vetter wrote:
>>> On Tue, Oct 06, 2015 at 04:33:43PM +0530, Kumar, Shobhit wrote:
>>>> On 10/06/2015 04:11 PM, Imre Deak wrote:
>>>>> On ti, 2015-10-06 at 15:26 +0530, Kumar, Shobhit wrote:
>>>>>> On 10/05/2015 09:05 PM, Imre Deak wrote:
>>>>>>> On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
>>>>>>>> Mostly reuse what is programmed by pre-os, but in case there is no
>>>>>>>> pre-os initialization, init the cdclk with the default value.
>>>>>>>>
>>>>>>>> Cc: Imre Deak <imre.deak@intel.com>
>>>>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>>>>>> ---
>>>>>>>>    drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
>>>>>>>>    1 file changed, 2 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>>> index 2d3cc82..675c60d 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>>> @@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
>>>>>>>>
>>>>>>>>    		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>>>>>>>>    		dev_priv->skl_boot_cdclk = cdclk_freq;
>>>>>>>> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>>>>>>>> -			DRM_ERROR("LCPLL1 is disabled\n");
>>>>>>>> -		else
>>>>>>>> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>>>>>>>> +
>>>>>>>> +		skl_init_cdclk(dev_priv);
>>>>>>>
>>>>>>> How does this prevent changing the clock if BIOS did enable some output?
>>>>>>> We shouldn't change the clock in that case.
>>>>>>
>>>>>> In that case it will try to re-apply the same clock that BIOS enabled.
>>>>>> Not sure if this is allowed, but I checked the cdclock change sequence
>>>>>> and it is mostly followed in skl_init_cdclk.
>>>>>> In my tests where BIOS does enable this, I faced no issues in
>>>>>> initializing again in driver.
>>>>>
>>>>> The first step in that sequence:
>>>>> "Disable all display engine functions using the full mode set disable
>>>>> sequence on all pipes, ports, and planes."
>>>>
>>>> Oh, yeah, I again made mistake of assuming that display is not enabled in
>>>> the first place. You are right, though it works if I change the clock again.
>>>>
>>>>>
>>>>> So the problem is not that the PLL itself may be enabled here (as BIOS
>>>>> left it), but that some output is also enabled.
>>>>
>>>> Yes.
>>>>
>>>>>
>>>>>> I have noticed on some pre-os this value is programmed correctly except
>>>>>> for the decimal part. That causes AUX transactions to fail on SKl. That
>>>>>> is what triggered this patch actually. So other way is to completely
>>>>>> validate the value in get_display_clock_speed instead of bit[28:26] and
>>>>>> then if wrong then only do the cdclk init.
>>>>>
>>>>> I think we'd need to detect at this point if outputs are enabled and
>>>>> only attempt to work around the above BIOS problem if this is not the
>>>>> case. Alternatively you could also disable the active outputs as a first
>>>>> step.
>>>>
>>>> Ok, let me detect if any output is enabled by BIOS and accordingly
>>>> initialize cdclk.
>>>
>>> These kind of fixiups should be done after the hw state readout. We
>>> already have sanitize_crtc/pll/encoder functions, probably best if we add
>>> a sanitize_cdclk or similar for this at the very end of the hw state
>>> sanitize sequence.
>>
>> Can't be done if we already need a somewhat sane cdclk for the
>> eDP AUX probing and whatnot.
>>
>> For actually enabling the cdclk for pushing pixels, we wouldn't need
>> to do anything except actually plug ia a calc_cdclk for SKL. No idea
>> why we're not doing that currently. Some extra care may be needed
>> due to the eDP DPLL0 usag IIRC.
>
> Hm right, cdlck is in the top-level power domain. Added fun is that with
> dmc the firmware is supposed to handle it. Messy :(

Yes, exactly. How about just adding verify_cdclk and calling in 
get_display_clock_speed to check if cdclk is programmed correctly along 
with related DPLL0 VCO settings for now. If all looks good, then skip 
else initialize. Now in that case if we have to initialize where do we 
get the cdclock to initialize with at this point ? Any default in VBT ? 
Or go with minimum by default and it can be bumped up later if needed.

Regards
Shobhit

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

Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Daniel Vetter]

On Tue, Oct 06, 2015 at 06:13:52PM +0530, Kumar, Shobhit wrote:
> On 10/06/2015 05:49 PM, Daniel Vetter wrote:
> >On Tue, Oct 06, 2015 at 02:41:44PM +0300, Ville Syrjälä wrote:
> >>On Tue, Oct 06, 2015 at 01:19:52PM +0200, Daniel Vetter wrote:
> >>>On Tue, Oct 06, 2015 at 04:33:43PM +0530, Kumar, Shobhit wrote:
> >>>>On 10/06/2015 04:11 PM, Imre Deak wrote:
> >>>>>On ti, 2015-10-06 at 15:26 +0530, Kumar, Shobhit wrote:
> >>>>>>On 10/05/2015 09:05 PM, Imre Deak wrote:
> >>>>>>>On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
> >>>>>>>>Mostly reuse what is programmed by pre-os, but in case there is no
> >>>>>>>>pre-os initialization, init the cdclk with the default value.
> >>>>>>>>
> >>>>>>>>Cc: Imre Deak <imre.deak@intel.com>
> >>>>>>>>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >>>>>>>>---
> >>>>>>>>   drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
> >>>>>>>>   1 file changed, 2 insertions(+), 4 deletions(-)
> >>>>>>>>
> >>>>>>>>diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >>>>>>>>index 2d3cc82..675c60d 100644
> >>>>>>>>--- a/drivers/gpu/drm/i915/intel_ddi.c
> >>>>>>>>+++ b/drivers/gpu/drm/i915/intel_ddi.c
> >>>>>>>>@@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
> >>>>>>>>
> >>>>>>>>   		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
> >>>>>>>>   		dev_priv->skl_boot_cdclk = cdclk_freq;
> >>>>>>>>-		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> >>>>>>>>-			DRM_ERROR("LCPLL1 is disabled\n");
> >>>>>>>>-		else
> >>>>>>>>-			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> >>>>>>>>+
> >>>>>>>>+		skl_init_cdclk(dev_priv);
> >>>>>>>
> >>>>>>>How does this prevent changing the clock if BIOS did enable some output?
> >>>>>>>We shouldn't change the clock in that case.
> >>>>>>
> >>>>>>In that case it will try to re-apply the same clock that BIOS enabled.
> >>>>>>Not sure if this is allowed, but I checked the cdclock change sequence
> >>>>>>and it is mostly followed in skl_init_cdclk.
> >>>>>>In my tests where BIOS does enable this, I faced no issues in
> >>>>>>initializing again in driver.
> >>>>>
> >>>>>The first step in that sequence:
> >>>>>"Disable all display engine functions using the full mode set disable
> >>>>>sequence on all pipes, ports, and planes."
> >>>>
> >>>>Oh, yeah, I again made mistake of assuming that display is not enabled in
> >>>>the first place. You are right, though it works if I change the clock again.
> >>>>
> >>>>>
> >>>>>So the problem is not that the PLL itself may be enabled here (as BIOS
> >>>>>left it), but that some output is also enabled.
> >>>>
> >>>>Yes.
> >>>>
> >>>>>
> >>>>>>I have noticed on some pre-os this value is programmed correctly except
> >>>>>>for the decimal part. That causes AUX transactions to fail on SKl. That
> >>>>>>is what triggered this patch actually. So other way is to completely
> >>>>>>validate the value in get_display_clock_speed instead of bit[28:26] and
> >>>>>>then if wrong then only do the cdclk init.
> >>>>>
> >>>>>I think we'd need to detect at this point if outputs are enabled and
> >>>>>only attempt to work around the above BIOS problem if this is not the
> >>>>>case. Alternatively you could also disable the active outputs as a first
> >>>>>step.
> >>>>
> >>>>Ok, let me detect if any output is enabled by BIOS and accordingly
> >>>>initialize cdclk.
> >>>
> >>>These kind of fixiups should be done after the hw state readout. We
> >>>already have sanitize_crtc/pll/encoder functions, probably best if we add
> >>>a sanitize_cdclk or similar for this at the very end of the hw state
> >>>sanitize sequence.
> >>
> >>Can't be done if we already need a somewhat sane cdclk for the
> >>eDP AUX probing and whatnot.
> >>
> >>For actually enabling the cdclk for pushing pixels, we wouldn't need
> >>to do anything except actually plug ia a calc_cdclk for SKL. No idea
> >>why we're not doing that currently. Some extra care may be needed
> >>due to the eDP DPLL0 usag IIRC.
> >
> >Hm right, cdlck is in the top-level power domain. Added fun is that with
> >dmc the firmware is supposed to handle it. Messy :(
> 
> Yes, exactly. How about just adding verify_cdclk and calling in
> get_display_clock_speed to check if cdclk is programmed correctly along with
> related DPLL0 VCO settings for now. If all looks good, then skip else
> initialize. Now in that case if we have to initialize where do we get the
> cdclock to initialize with at this point ? Any default in VBT ? Or go with
> minimum by default and it can be bumped up later if needed.

Just initialize to the slowest possible value, once we have dynamic cdclk
switching for skl. But that seems to be stuck behind resolving that big
confusion around dmc and the sequence for runtime pm. Without dynamic
cdclk we have to pick the maximum.
-Daniel
-- 
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

Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Ville Syrjälä]

On Tue, Oct 06, 2015 at 06:13:52PM +0530, Kumar, Shobhit wrote:
> On 10/06/2015 05:49 PM, Daniel Vetter wrote:
> > On Tue, Oct 06, 2015 at 02:41:44PM +0300, Ville Syrjälä wrote:
> >> On Tue, Oct 06, 2015 at 01:19:52PM +0200, Daniel Vetter wrote:
> >>> On Tue, Oct 06, 2015 at 04:33:43PM +0530, Kumar, Shobhit wrote:
> >>>> On 10/06/2015 04:11 PM, Imre Deak wrote:
> >>>>> On ti, 2015-10-06 at 15:26 +0530, Kumar, Shobhit wrote:
> >>>>>> On 10/05/2015 09:05 PM, Imre Deak wrote:
> >>>>>>> On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
> >>>>>>>> Mostly reuse what is programmed by pre-os, but in case there is no
> >>>>>>>> pre-os initialization, init the cdclk with the default value.
> >>>>>>>>
> >>>>>>>> Cc: Imre Deak <imre.deak@intel.com>
> >>>>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >>>>>>>> ---
> >>>>>>>>    drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
> >>>>>>>>    1 file changed, 2 insertions(+), 4 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >>>>>>>> index 2d3cc82..675c60d 100644
> >>>>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >>>>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >>>>>>>> @@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
> >>>>>>>>
> >>>>>>>>    		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
> >>>>>>>>    		dev_priv->skl_boot_cdclk = cdclk_freq;
> >>>>>>>> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> >>>>>>>> -			DRM_ERROR("LCPLL1 is disabled\n");
> >>>>>>>> -		else
> >>>>>>>> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> >>>>>>>> +
> >>>>>>>> +		skl_init_cdclk(dev_priv);
> >>>>>>>
> >>>>>>> How does this prevent changing the clock if BIOS did enable some output?
> >>>>>>> We shouldn't change the clock in that case.
> >>>>>>
> >>>>>> In that case it will try to re-apply the same clock that BIOS enabled.
> >>>>>> Not sure if this is allowed, but I checked the cdclock change sequence
> >>>>>> and it is mostly followed in skl_init_cdclk.
> >>>>>> In my tests where BIOS does enable this, I faced no issues in
> >>>>>> initializing again in driver.
> >>>>>
> >>>>> The first step in that sequence:
> >>>>> "Disable all display engine functions using the full mode set disable
> >>>>> sequence on all pipes, ports, and planes."
> >>>>
> >>>> Oh, yeah, I again made mistake of assuming that display is not enabled in
> >>>> the first place. You are right, though it works if I change the clock again.
> >>>>
> >>>>>
> >>>>> So the problem is not that the PLL itself may be enabled here (as BIOS
> >>>>> left it), but that some output is also enabled.
> >>>>
> >>>> Yes.
> >>>>
> >>>>>
> >>>>>> I have noticed on some pre-os this value is programmed correctly except
> >>>>>> for the decimal part. That causes AUX transactions to fail on SKl. That
> >>>>>> is what triggered this patch actually. So other way is to completely
> >>>>>> validate the value in get_display_clock_speed instead of bit[28:26] and
> >>>>>> then if wrong then only do the cdclk init.
> >>>>>
> >>>>> I think we'd need to detect at this point if outputs are enabled and
> >>>>> only attempt to work around the above BIOS problem if this is not the
> >>>>> case. Alternatively you could also disable the active outputs as a first
> >>>>> step.
> >>>>
> >>>> Ok, let me detect if any output is enabled by BIOS and accordingly
> >>>> initialize cdclk.
> >>>
> >>> These kind of fixiups should be done after the hw state readout. We
> >>> already have sanitize_crtc/pll/encoder functions, probably best if we add
> >>> a sanitize_cdclk or similar for this at the very end of the hw state
> >>> sanitize sequence.
> >>
> >> Can't be done if we already need a somewhat sane cdclk for the
> >> eDP AUX probing and whatnot.
> >>
> >> For actually enabling the cdclk for pushing pixels, we wouldn't need
> >> to do anything except actually plug ia a calc_cdclk for SKL. No idea
> >> why we're not doing that currently. Some extra care may be needed
> >> due to the eDP DPLL0 usag IIRC.
> >
> > Hm right, cdlck is in the top-level power domain. Added fun is that with
> > dmc the firmware is supposed to handle it. Messy :(
> 
> Yes, exactly. How about just adding verify_cdclk and calling in 
> get_display_clock_speed to check if cdclk is programmed correctly along 
> with related DPLL0 VCO settings for now.

I would just keep it somewhere in init/resume path rather than polluting
.get_display_clock_speed with it. We should be calling
intel_update_cdclk() in those paths somewhere, so doing the fixup just
before that should be sufficient.

> If all looks good, then skip 
> else initialize. Now in that case if we have to initialize where do we 
> get the cdclock to initialize with at this point ? Any default in VBT ? 
> Or go with minimum by default and it can be bumped up later if needed.

Can we figure out the eDP link rate requirements ahead of time from
VBT? That should dictate what the VCO frequency has to be. If we
get it wrong, then we'd have to change the VCO again after the eDP
probe has told us what we actually need. Such a second fixup could
actually be left up to the normal modeset calc_cdclk stuff, which
shouldn't really need any special handling for it apart from actually
updating the DPLL0 settings if needed, in addition to the normal cdclk
divider stuff.

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

Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Ville Syrjälä]

On Tue, Oct 06, 2015 at 03:04:28PM +0200, Daniel Vetter wrote:
> On Tue, Oct 06, 2015 at 06:13:52PM +0530, Kumar, Shobhit wrote:
> > On 10/06/2015 05:49 PM, Daniel Vetter wrote:
> > >On Tue, Oct 06, 2015 at 02:41:44PM +0300, Ville Syrjälä wrote:
> > >>On Tue, Oct 06, 2015 at 01:19:52PM +0200, Daniel Vetter wrote:
> > >>>On Tue, Oct 06, 2015 at 04:33:43PM +0530, Kumar, Shobhit wrote:
> > >>>>On 10/06/2015 04:11 PM, Imre Deak wrote:
> > >>>>>On ti, 2015-10-06 at 15:26 +0530, Kumar, Shobhit wrote:
> > >>>>>>On 10/05/2015 09:05 PM, Imre Deak wrote:
> > >>>>>>>On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
> > >>>>>>>>Mostly reuse what is programmed by pre-os, but in case there is no
> > >>>>>>>>pre-os initialization, init the cdclk with the default value.
> > >>>>>>>>
> > >>>>>>>>Cc: Imre Deak <imre.deak@intel.com>
> > >>>>>>>>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> > >>>>>>>>---
> > >>>>>>>>   drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
> > >>>>>>>>   1 file changed, 2 insertions(+), 4 deletions(-)
> > >>>>>>>>
> > >>>>>>>>diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > >>>>>>>>index 2d3cc82..675c60d 100644
> > >>>>>>>>--- a/drivers/gpu/drm/i915/intel_ddi.c
> > >>>>>>>>+++ b/drivers/gpu/drm/i915/intel_ddi.c
> > >>>>>>>>@@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
> > >>>>>>>>
> > >>>>>>>>   		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
> > >>>>>>>>   		dev_priv->skl_boot_cdclk = cdclk_freq;
> > >>>>>>>>-		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> > >>>>>>>>-			DRM_ERROR("LCPLL1 is disabled\n");
> > >>>>>>>>-		else
> > >>>>>>>>-			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> > >>>>>>>>+
> > >>>>>>>>+		skl_init_cdclk(dev_priv);
> > >>>>>>>
> > >>>>>>>How does this prevent changing the clock if BIOS did enable some output?
> > >>>>>>>We shouldn't change the clock in that case.
> > >>>>>>
> > >>>>>>In that case it will try to re-apply the same clock that BIOS enabled.
> > >>>>>>Not sure if this is allowed, but I checked the cdclock change sequence
> > >>>>>>and it is mostly followed in skl_init_cdclk.
> > >>>>>>In my tests where BIOS does enable this, I faced no issues in
> > >>>>>>initializing again in driver.
> > >>>>>
> > >>>>>The first step in that sequence:
> > >>>>>"Disable all display engine functions using the full mode set disable
> > >>>>>sequence on all pipes, ports, and planes."
> > >>>>
> > >>>>Oh, yeah, I again made mistake of assuming that display is not enabled in
> > >>>>the first place. You are right, though it works if I change the clock again.
> > >>>>
> > >>>>>
> > >>>>>So the problem is not that the PLL itself may be enabled here (as BIOS
> > >>>>>left it), but that some output is also enabled.
> > >>>>
> > >>>>Yes.
> > >>>>
> > >>>>>
> > >>>>>>I have noticed on some pre-os this value is programmed correctly except
> > >>>>>>for the decimal part. That causes AUX transactions to fail on SKl. That
> > >>>>>>is what triggered this patch actually. So other way is to completely
> > >>>>>>validate the value in get_display_clock_speed instead of bit[28:26] and
> > >>>>>>then if wrong then only do the cdclk init.
> > >>>>>
> > >>>>>I think we'd need to detect at this point if outputs are enabled and
> > >>>>>only attempt to work around the above BIOS problem if this is not the
> > >>>>>case. Alternatively you could also disable the active outputs as a first
> > >>>>>step.
> > >>>>
> > >>>>Ok, let me detect if any output is enabled by BIOS and accordingly
> > >>>>initialize cdclk.
> > >>>
> > >>>These kind of fixiups should be done after the hw state readout. We
> > >>>already have sanitize_crtc/pll/encoder functions, probably best if we add
> > >>>a sanitize_cdclk or similar for this at the very end of the hw state
> > >>>sanitize sequence.
> > >>
> > >>Can't be done if we already need a somewhat sane cdclk for the
> > >>eDP AUX probing and whatnot.
> > >>
> > >>For actually enabling the cdclk for pushing pixels, we wouldn't need
> > >>to do anything except actually plug ia a calc_cdclk for SKL. No idea
> > >>why we're not doing that currently. Some extra care may be needed
> > >>due to the eDP DPLL0 usag IIRC.
> > >
> > >Hm right, cdlck is in the top-level power domain. Added fun is that with
> > >dmc the firmware is supposed to handle it. Messy :(
> > 
> > Yes, exactly. How about just adding verify_cdclk and calling in
> > get_display_clock_speed to check if cdclk is programmed correctly along with
> > related DPLL0 VCO settings for now. If all looks good, then skip else
> > initialize. Now in that case if we have to initialize where do we get the
> > cdclock to initialize with at this point ? Any default in VBT ? Or go with
> > minimum by default and it can be bumped up later if needed.
> 
> Just initialize to the slowest possible value, once we have dynamic cdclk
> switching for skl. But that seems to be stuck behind resolving that big
> confusion around dmc and the sequence for runtime pm. Without dynamic
> cdclk we have to pick the maximum.

I suppose we could simply disable DC5/6 around the cdclk update if
needed. But I'm not sure we'd need to do that since any register access
should anyway bring it out of DC5/6, and when it goes back into DC5/6
it'll save the current values and restore them again on the next wakeup.

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

Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Kumar, Shobhit]

On 10/06/2015 06:55 PM, Ville Syrjälä wrote:
> On Tue, Oct 06, 2015 at 06:13:52PM +0530, Kumar, Shobhit wrote:
>> On 10/06/2015 05:49 PM, Daniel Vetter wrote:
>>> On Tue, Oct 06, 2015 at 02:41:44PM +0300, Ville Syrjälä wrote:
>>>> On Tue, Oct 06, 2015 at 01:19:52PM +0200, Daniel Vetter wrote:
>>>>> On Tue, Oct 06, 2015 at 04:33:43PM +0530, Kumar, Shobhit wrote:
>>>>>> On 10/06/2015 04:11 PM, Imre Deak wrote:
>>>>>>> On ti, 2015-10-06 at 15:26 +0530, Kumar, Shobhit wrote:
>>>>>>>> On 10/05/2015 09:05 PM, Imre Deak wrote:
>>>>>>>>> On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
>>>>>>>>>> Mostly reuse what is programmed by pre-os, but in case there is no
>>>>>>>>>> pre-os initialization, init the cdclk with the default value.
>>>>>>>>>>
>>>>>>>>>> Cc: Imre Deak <imre.deak@intel.com>
>>>>>>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>>>>>>>> ---
>>>>>>>>>>     drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
>>>>>>>>>>     1 file changed, 2 insertions(+), 4 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>>>>> index 2d3cc82..675c60d 100644
>>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>>>>> @@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
>>>>>>>>>>
>>>>>>>>>>     		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>>>>>>>>>>     		dev_priv->skl_boot_cdclk = cdclk_freq;
>>>>>>>>>> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>>>>>>>>>> -			DRM_ERROR("LCPLL1 is disabled\n");
>>>>>>>>>> -		else
>>>>>>>>>> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>>>>>>>>>> +
>>>>>>>>>> +		skl_init_cdclk(dev_priv);
>>>>>>>>>
>>>>>>>>> How does this prevent changing the clock if BIOS did enable some output?
>>>>>>>>> We shouldn't change the clock in that case.
>>>>>>>>
>>>>>>>> In that case it will try to re-apply the same clock that BIOS enabled.
>>>>>>>> Not sure if this is allowed, but I checked the cdclock change sequence
>>>>>>>> and it is mostly followed in skl_init_cdclk.
>>>>>>>> In my tests where BIOS does enable this, I faced no issues in
>>>>>>>> initializing again in driver.
>>>>>>>
>>>>>>> The first step in that sequence:
>>>>>>> "Disable all display engine functions using the full mode set disable
>>>>>>> sequence on all pipes, ports, and planes."
>>>>>>
>>>>>> Oh, yeah, I again made mistake of assuming that display is not enabled in
>>>>>> the first place. You are right, though it works if I change the clock again.
>>>>>>
>>>>>>>
>>>>>>> So the problem is not that the PLL itself may be enabled here (as BIOS
>>>>>>> left it), but that some output is also enabled.
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>>
>>>>>>>> I have noticed on some pre-os this value is programmed correctly except
>>>>>>>> for the decimal part. That causes AUX transactions to fail on SKl. That
>>>>>>>> is what triggered this patch actually. So other way is to completely
>>>>>>>> validate the value in get_display_clock_speed instead of bit[28:26] and
>>>>>>>> then if wrong then only do the cdclk init.
>>>>>>>
>>>>>>> I think we'd need to detect at this point if outputs are enabled and
>>>>>>> only attempt to work around the above BIOS problem if this is not the
>>>>>>> case. Alternatively you could also disable the active outputs as a first
>>>>>>> step.
>>>>>>
>>>>>> Ok, let me detect if any output is enabled by BIOS and accordingly
>>>>>> initialize cdclk.
>>>>>
>>>>> These kind of fixiups should be done after the hw state readout. We
>>>>> already have sanitize_crtc/pll/encoder functions, probably best if we add
>>>>> a sanitize_cdclk or similar for this at the very end of the hw state
>>>>> sanitize sequence.
>>>>
>>>> Can't be done if we already need a somewhat sane cdclk for the
>>>> eDP AUX probing and whatnot.
>>>>
>>>> For actually enabling the cdclk for pushing pixels, we wouldn't need
>>>> to do anything except actually plug ia a calc_cdclk for SKL. No idea
>>>> why we're not doing that currently. Some extra care may be needed
>>>> due to the eDP DPLL0 usag IIRC.
>>>
>>> Hm right, cdlck is in the top-level power domain. Added fun is that with
>>> dmc the firmware is supposed to handle it. Messy :(
>>
>> Yes, exactly. How about just adding verify_cdclk and calling in
>> get_display_clock_speed to check if cdclk is programmed correctly along
>> with related DPLL0 VCO settings for now.
>
> I would just keep it somewhere in init/resume path rather than polluting
> .get_display_clock_speed with it. We should be calling
> intel_update_cdclk() in those paths somewhere, so doing the fixup just
> before that should be sufficient.

I think, get_display_clock_speed should be okay where it returns valid 
cdclk if programmed correctly else returns -EINVAL. In case of boot, 
invalid means pre-OS did not try to enable display and based on the 
error, during ddi_pll_init, we can safely call skl_init_cdclk.

>
>> If all looks good, then skip
>> else initialize. Now in that case if we have to initialize where do we
>> get the cdclock to initialize with at this point ? Any default in VBT ?
>> Or go with minimum by default and it can be bumped up later if needed.
>
> Can we figure out the eDP link rate requirements ahead of time from
> VBT? That should dictate what the VCO frequency has to be. If we
> get it wrong, then we'd have to change the VCO again after the eDP
> probe has told us what we actually need. Such a second fixup could
> actually be left up to the normal modeset calc_cdclk stuff, which
> shouldn't really need any special handling for it apart from actually
> updating the DPLL0 settings if needed, in addition to the normal cdclk
> divider stuff.
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[v2] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Shobhit Kumar]

Reuse what is programmed by pre-os, but in case there is no pre-os
initialization, init the cdclk with the max value by default untill
dynamic cdclk support comes.

v2: Check if BIOS programmed correctly rather than always calling init
    - Do validation of programmed cdctl and what it is expected
    - Only do slk_init_cdclk if validation failed else reuse BIOS
      programmed value

Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     | 18 ++++++++++++-----
 drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++---------
 2 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 2d3cc82..3ec5618 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2946,11 +2946,19 @@ void intel_ddi_pll_init(struct drm_device *dev)
 		int cdclk_freq;
 
 		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
-		dev_priv->skl_boot_cdclk = cdclk_freq;
-		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
-			DRM_ERROR("LCPLL1 is disabled\n");
-		else
-			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
+
+		/* Invalid CDCLK from BIOS ? */
+		if (cdclk_freq < 0) {
+			/* program to maximum cdclk till we have dynamic cdclk support */
+			dev_priv->skl_boot_cdclk = 675000;
+			skl_init_cdclk(dev_priv);
+		} else {
+			dev_priv->skl_boot_cdclk = cdclk_freq;
+			if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
+				DRM_ERROR("LCPLL1 is disabled\n");
+			else
+				intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
+		}
 	} else if (IS_BROXTON(dev)) {
 		broxton_init_cdclk(dev);
 		broxton_ddi_phy_init(dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bbeb6d3..f734410 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6634,12 +6634,15 @@ static int skylake_get_display_clock_speed(struct drm_device *dev)
 	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
 	uint32_t cdctl = I915_READ(CDCLK_CTL);
 	uint32_t linkrate;
+	int freq;
 
 	if (!(lcpll1 & LCPLL_PLL_ENABLE))
 		return 24000; /* 24MHz is the cd freq with NSSC ref */
 
-	if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540)
-		return 540000;
+	if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) {
+		freq = 540000;
+		goto verify;
+	}
 
 	linkrate = (I915_READ(DPLL_CTRL1) &
 		    DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0)) >> 1;
@@ -6649,30 +6652,46 @@ static int skylake_get_display_clock_speed(struct drm_device *dev)
 		/* vco 8640 */
 		switch (cdctl & CDCLK_FREQ_SEL_MASK) {
 		case CDCLK_FREQ_450_432:
-			return 432000;
+			freq = 432000;
+			break;
 		case CDCLK_FREQ_337_308:
-			return 308570;
+			freq = 308570;
+			break;
 		case CDCLK_FREQ_675_617:
-			return 617140;
+			freq = 617140;
+			break;
 		default:
 			WARN(1, "Unknown cd freq selection\n");
+			return -EINVAL;
 		}
 	} else {
 		/* vco 8100 */
 		switch (cdctl & CDCLK_FREQ_SEL_MASK) {
 		case CDCLK_FREQ_450_432:
-			return 450000;
+			freq = 450000;
+			break;
 		case CDCLK_FREQ_337_308:
-			return 337500;
+			freq = 337500;
+			break;
 		case CDCLK_FREQ_675_617:
-			return 675000;
+			freq = 675000;
+			break;
 		default:
 			WARN(1, "Unknown cd freq selection\n");
+			return -EINVAL;
 		}
 	}
 
-	/* error case, do as if DPLL0 isn't enabled */
-	return 24000;
+verify:
+	/*
+	 * Noticed in some instances that the freq selection is correct but
+	 * decimal part is programmed wrong from BIOS where pre-os does not
+	 * enable display. Verify the same as well.
+	 */
+	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
+		return freq;
+	else
+		return -EINVAL;
 }
 
 static int broxton_get_display_clock_speed(struct drm_device *dev)
-- 
2.4.3

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

Re: [v2] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Imre Deak]

On to, 2015-10-08 at 09:58 +0530, Shobhit Kumar wrote:
> Reuse what is programmed by pre-os, but in case there is no pre-os
> initialization, init the cdclk with the max value by default untill
> dynamic cdclk support comes.
> 
> v2: Check if BIOS programmed correctly rather than always calling init
>     - Do validation of programmed cdctl and what it is expected
>     - Only do slk_init_cdclk if validation failed else reuse BIOS
>       programmed value
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     | 18 ++++++++++++-----
>  drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++---------
>  2 files changed, 42 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 2d3cc82..3ec5618 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2946,11 +2946,19 @@ void intel_ddi_pll_init(struct drm_device *dev)
>  		int cdclk_freq;
>  
>  		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
> -		dev_priv->skl_boot_cdclk = cdclk_freq;
> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> -			DRM_ERROR("LCPLL1 is disabled\n");
> -		else
> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> +
> +		/* Invalid CDCLK from BIOS ? */
> +		if (cdclk_freq < 0) {
> +			/* program to maximum cdclk till we have dynamic cdclk support */
> +			dev_priv->skl_boot_cdclk = 675000;
> +			skl_init_cdclk(dev_priv);

This would still try to reprogram CDCLK if BIOS enabled an output with
an incorrect CDCLK decimal part. Isn't this the exact scenario you're
seeing? As said before reprogramming CDCLK in that case would require
disabling the outputs first.

We would also need to call skl_init_cdclk if the BIOS hasn't enabled the
PLL for some reason. But I guess that could be a separate change.

> +		} else {
> +			dev_priv->skl_boot_cdclk = cdclk_freq;
> +			if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> +				DRM_ERROR("LCPLL1 is disabled\n");
> +			else
> +				intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> +		}
>  	} else if (IS_BROXTON(dev)) {
>  		broxton_init_cdclk(dev);
>  		broxton_ddi_phy_init(dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bbeb6d3..f734410 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6634,12 +6634,15 @@ static int skylake_get_display_clock_speed(struct drm_device *dev)
>  	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
>  	uint32_t cdctl = I915_READ(CDCLK_CTL);
>  	uint32_t linkrate;
> +	int freq;
>  
>  	if (!(lcpll1 & LCPLL_PLL_ENABLE))
>  		return 24000; /* 24MHz is the cd freq with NSSC ref */
>  
> -	if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540)
> -		return 540000;
> +	if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) {
> +		freq = 540000;
> +		goto verify;
> +	}
>  
>  	linkrate = (I915_READ(DPLL_CTRL1) &
>  		    DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0)) >> 1;
> @@ -6649,30 +6652,46 @@ static int skylake_get_display_clock_speed(struct drm_device *dev)
>  		/* vco 8640 */
>  		switch (cdctl & CDCLK_FREQ_SEL_MASK) {
>  		case CDCLK_FREQ_450_432:
> -			return 432000;
> +			freq = 432000;
> +			break;
>  		case CDCLK_FREQ_337_308:
> -			return 308570;
> +			freq = 308570;
> +			break;
>  		case CDCLK_FREQ_675_617:
> -			return 617140;
> +			freq = 617140;
> +			break;
>  		default:
>  			WARN(1, "Unknown cd freq selection\n");
> +			return -EINVAL;
>  		}
>  	} else {
>  		/* vco 8100 */
>  		switch (cdctl & CDCLK_FREQ_SEL_MASK) {
>  		case CDCLK_FREQ_450_432:
> -			return 450000;
> +			freq = 450000;
> +			break;
>  		case CDCLK_FREQ_337_308:
> -			return 337500;
> +			freq = 337500;
> +			break;
>  		case CDCLK_FREQ_675_617:
> -			return 675000;
> +			freq = 675000;
> +			break;
>  		default:
>  			WARN(1, "Unknown cd freq selection\n");
> +			return -EINVAL;
>  		}
>  	}
>  
> -	/* error case, do as if DPLL0 isn't enabled */
> -	return 24000;
> +verify:
> +	/*
> +	 * Noticed in some instances that the freq selection is correct but
> +	 * decimal part is programmed wrong from BIOS where pre-os does not
> +	 * enable display. Verify the same as well.
> +	 */
> +	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
> +		return freq;
> +	else
> +		return -EINVAL;
>  }
>  
>  static int broxton_get_display_clock_speed(struct drm_device *dev)


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

Re: [v2] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Kumar, Shobhit]

On 10/08/2015 04:59 PM, Imre Deak wrote:
> On to, 2015-10-08 at 09:58 +0530, Shobhit Kumar wrote:
>> Reuse what is programmed by pre-os, but in case there is no pre-os
>> initialization, init the cdclk with the max value by default untill
>> dynamic cdclk support comes.
>>
>> v2: Check if BIOS programmed correctly rather than always calling init
>>      - Do validation of programmed cdctl and what it is expected
>>      - Only do slk_init_cdclk if validation failed else reuse BIOS
>>        programmed value
>>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_ddi.c     | 18 ++++++++++++-----
>>   drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++---------
>>   2 files changed, 42 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 2d3cc82..3ec5618 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -2946,11 +2946,19 @@ void intel_ddi_pll_init(struct drm_device *dev)
>>   		int cdclk_freq;
>>
>>   		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>> -		dev_priv->skl_boot_cdclk = cdclk_freq;
>> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>> -			DRM_ERROR("LCPLL1 is disabled\n");
>> -		else
>> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>> +
>> +		/* Invalid CDCLK from BIOS ? */
>> +		if (cdclk_freq < 0) {
>> +			/* program to maximum cdclk till we have dynamic cdclk support */
>> +			dev_priv->skl_boot_cdclk = 675000;
>> +			skl_init_cdclk(dev_priv);
>
> This would still try to reprogram CDCLK if BIOS enabled an output with
> an incorrect CDCLK decimal part. Isn't this the exact scenario you're
> seeing? As said before reprogramming CDCLK in that case would require
> disabling the outputs first.

I went with the hypothesis that if VBIOS/GOP was loaded it would have to 
correct the cdclock, with wrong decimal value display cannot be enabled. 
For example AUX will fail on SKL. So for correct display output enabled 
cdclock has to be correctly programmed. If it is wrong display was not 
enabled at all.

The scenario which I am seeing is VBIOS/GOP is not loaded at all, and 
the pre-os is not leaving cdclock in correct state.

>
> We would also need to call skl_init_cdclk if the BIOS hasn't enabled the
> PLL for some reason. But I guess that could be a separate change.
>
>> +		} else {
>> +			dev_priv->skl_boot_cdclk = cdclk_freq;
>> +			if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>> +				DRM_ERROR("LCPLL1 is disabled\n");
>> +			else
>> +				intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>> +		}
>>   	} else if (IS_BROXTON(dev)) {
>>   		broxton_init_cdclk(dev);
>>   		broxton_ddi_phy_init(dev);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index bbeb6d3..f734410 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6634,12 +6634,15 @@ static int skylake_get_display_clock_speed(struct drm_device *dev)
>>   	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
>>   	uint32_t cdctl = I915_READ(CDCLK_CTL);
>>   	uint32_t linkrate;
>> +	int freq;
>>
>>   	if (!(lcpll1 & LCPLL_PLL_ENABLE))
>>   		return 24000; /* 24MHz is the cd freq with NSSC ref */
>>
>> -	if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540)
>> -		return 540000;
>> +	if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) {
>> +		freq = 540000;
>> +		goto verify;
>> +	}
>>
>>   	linkrate = (I915_READ(DPLL_CTRL1) &
>>   		    DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0)) >> 1;
>> @@ -6649,30 +6652,46 @@ static int skylake_get_display_clock_speed(struct drm_device *dev)
>>   		/* vco 8640 */
>>   		switch (cdctl & CDCLK_FREQ_SEL_MASK) {
>>   		case CDCLK_FREQ_450_432:
>> -			return 432000;
>> +			freq = 432000;
>> +			break;
>>   		case CDCLK_FREQ_337_308:
>> -			return 308570;
>> +			freq = 308570;
>> +			break;
>>   		case CDCLK_FREQ_675_617:
>> -			return 617140;
>> +			freq = 617140;
>> +			break;
>>   		default:
>>   			WARN(1, "Unknown cd freq selection\n");
>> +			return -EINVAL;
>>   		}
>>   	} else {
>>   		/* vco 8100 */
>>   		switch (cdctl & CDCLK_FREQ_SEL_MASK) {
>>   		case CDCLK_FREQ_450_432:
>> -			return 450000;
>> +			freq = 450000;
>> +			break;
>>   		case CDCLK_FREQ_337_308:
>> -			return 337500;
>> +			freq = 337500;
>> +			break;
>>   		case CDCLK_FREQ_675_617:
>> -			return 675000;
>> +			freq = 675000;
>> +			break;
>>   		default:
>>   			WARN(1, "Unknown cd freq selection\n");
>> +			return -EINVAL;
>>   		}
>>   	}
>>
>> -	/* error case, do as if DPLL0 isn't enabled */
>> -	return 24000;
>> +verify:
>> +	/*
>> +	 * Noticed in some instances that the freq selection is correct but
>> +	 * decimal part is programmed wrong from BIOS where pre-os does not
>> +	 * enable display. Verify the same as well.
>> +	 */
>> +	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
>> +		return freq;
>> +	else
>> +		return -EINVAL;
>>   }
>>
>>   static int broxton_get_display_clock_speed(struct drm_device *dev)
>
>
> _______________________________________________
> 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

Re: [v2] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Ville Syrjälä]

On Thu, Oct 08, 2015 at 05:43:41PM +0530, Kumar, Shobhit wrote:
> On 10/08/2015 04:59 PM, Imre Deak wrote:
> > On to, 2015-10-08 at 09:58 +0530, Shobhit Kumar wrote:
> >> Reuse what is programmed by pre-os, but in case there is no pre-os
> >> initialization, init the cdclk with the max value by default untill
> >> dynamic cdclk support comes.
> >>
> >> v2: Check if BIOS programmed correctly rather than always calling init
> >>      - Do validation of programmed cdctl and what it is expected
> >>      - Only do slk_init_cdclk if validation failed else reuse BIOS
> >>        programmed value
> >>
> >> Cc: Imre Deak <imre.deak@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_ddi.c     | 18 ++++++++++++-----
> >>   drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++---------
> >>   2 files changed, 42 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index 2d3cc82..3ec5618 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -2946,11 +2946,19 @@ void intel_ddi_pll_init(struct drm_device *dev)
> >>   		int cdclk_freq;
> >>
> >>   		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
> >> -		dev_priv->skl_boot_cdclk = cdclk_freq;
> >> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> >> -			DRM_ERROR("LCPLL1 is disabled\n");
> >> -		else
> >> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> >> +
> >> +		/* Invalid CDCLK from BIOS ? */
> >> +		if (cdclk_freq < 0) {
> >> +			/* program to maximum cdclk till we have dynamic cdclk support */
> >> +			dev_priv->skl_boot_cdclk = 675000;
> >> +			skl_init_cdclk(dev_priv);
> >
> > This would still try to reprogram CDCLK if BIOS enabled an output with
> > an incorrect CDCLK decimal part. Isn't this the exact scenario you're
> > seeing? As said before reprogramming CDCLK in that case would require
> > disabling the outputs first.
> 
> I went with the hypothesis that if VBIOS/GOP was loaded it would have to 
> correct the cdclock, with wrong decimal value display cannot be enabled. 
> For example AUX will fail on SKL. So for correct display output enabled 
> cdclock has to be correctly programmed. If it is wrong display was not 
> enabled at all.
> 
> The scenario which I am seeing is VBIOS/GOP is not loaded at all, and 
> the pre-os is not leaving cdclock in correct state.

But it still enabled DPLL0? Why does it do that if it doesn't set up
cdclk?

> 
> >
> > We would also need to call skl_init_cdclk if the BIOS hasn't enabled the
> > PLL for some reason. But I guess that could be a separate change.
> >
> >> +		} else {
> >> +			dev_priv->skl_boot_cdclk = cdclk_freq;
> >> +			if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> >> +				DRM_ERROR("LCPLL1 is disabled\n");
> >> +			else
> >> +				intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> >> +		}
> >>   	} else if (IS_BROXTON(dev)) {
> >>   		broxton_init_cdclk(dev);
> >>   		broxton_ddi_phy_init(dev);
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index bbeb6d3..f734410 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -6634,12 +6634,15 @@ static int skylake_get_display_clock_speed(struct drm_device *dev)
> >>   	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
> >>   	uint32_t cdctl = I915_READ(CDCLK_CTL);
> >>   	uint32_t linkrate;
> >> +	int freq;
> >>
> >>   	if (!(lcpll1 & LCPLL_PLL_ENABLE))
> >>   		return 24000; /* 24MHz is the cd freq with NSSC ref */
> >>
> >> -	if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540)
> >> -		return 540000;
> >> +	if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) {
> >> +		freq = 540000;
> >> +		goto verify;
> >> +	}
> >>
> >>   	linkrate = (I915_READ(DPLL_CTRL1) &
> >>   		    DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0)) >> 1;
> >> @@ -6649,30 +6652,46 @@ static int skylake_get_display_clock_speed(struct drm_device *dev)
> >>   		/* vco 8640 */
> >>   		switch (cdctl & CDCLK_FREQ_SEL_MASK) {
> >>   		case CDCLK_FREQ_450_432:
> >> -			return 432000;
> >> +			freq = 432000;
> >> +			break;
> >>   		case CDCLK_FREQ_337_308:
> >> -			return 308570;
> >> +			freq = 308570;
> >> +			break;
> >>   		case CDCLK_FREQ_675_617:
> >> -			return 617140;
> >> +			freq = 617140;
> >> +			break;
> >>   		default:
> >>   			WARN(1, "Unknown cd freq selection\n");
> >> +			return -EINVAL;
> >>   		}
> >>   	} else {
> >>   		/* vco 8100 */
> >>   		switch (cdctl & CDCLK_FREQ_SEL_MASK) {
> >>   		case CDCLK_FREQ_450_432:
> >> -			return 450000;
> >> +			freq = 450000;
> >> +			break;
> >>   		case CDCLK_FREQ_337_308:
> >> -			return 337500;
> >> +			freq = 337500;
> >> +			break;
> >>   		case CDCLK_FREQ_675_617:
> >> -			return 675000;
> >> +			freq = 675000;
> >> +			break;
> >>   		default:
> >>   			WARN(1, "Unknown cd freq selection\n");
> >> +			return -EINVAL;
> >>   		}
> >>   	}
> >>
> >> -	/* error case, do as if DPLL0 isn't enabled */
> >> -	return 24000;
> >> +verify:
> >> +	/*
> >> +	 * Noticed in some instances that the freq selection is correct but
> >> +	 * decimal part is programmed wrong from BIOS where pre-os does not
> >> +	 * enable display. Verify the same as well.
> >> +	 */
> >> +	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
> >> +		return freq;
> >> +	else
> >> +		return -EINVAL;
> >>   }
> >>
> >>   static int broxton_get_display_clock_speed(struct drm_device *dev)
> >
> >
> > _______________________________________________
> > 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

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

Re: [v2] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Kumar, Shobhit]

On 10/08/2015 05:54 PM, Ville Syrjälä wrote:
> On Thu, Oct 08, 2015 at 05:43:41PM +0530, Kumar, Shobhit wrote:
>> On 10/08/2015 04:59 PM, Imre Deak wrote:
>>> On to, 2015-10-08 at 09:58 +0530, Shobhit Kumar wrote:
>>>> Reuse what is programmed by pre-os, but in case there is no pre-os
>>>> initialization, init the cdclk with the max value by default untill
>>>> dynamic cdclk support comes.
>>>>
>>>> v2: Check if BIOS programmed correctly rather than always calling init
>>>>       - Do validation of programmed cdctl and what it is expected
>>>>       - Only do slk_init_cdclk if validation failed else reuse BIOS
>>>>         programmed value
>>>>
>>>> Cc: Imre Deak <imre.deak@intel.com>
>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_ddi.c     | 18 ++++++++++++-----
>>>>    drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++---------
>>>>    2 files changed, 42 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>> index 2d3cc82..3ec5618 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -2946,11 +2946,19 @@ void intel_ddi_pll_init(struct drm_device *dev)
>>>>    		int cdclk_freq;
>>>>
>>>>    		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>>>> -		dev_priv->skl_boot_cdclk = cdclk_freq;
>>>> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>>>> -			DRM_ERROR("LCPLL1 is disabled\n");
>>>> -		else
>>>> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>>>> +
>>>> +		/* Invalid CDCLK from BIOS ? */
>>>> +		if (cdclk_freq < 0) {
>>>> +			/* program to maximum cdclk till we have dynamic cdclk support */
>>>> +			dev_priv->skl_boot_cdclk = 675000;
>>>> +			skl_init_cdclk(dev_priv);
>>>
>>> This would still try to reprogram CDCLK if BIOS enabled an output with
>>> an incorrect CDCLK decimal part. Isn't this the exact scenario you're
>>> seeing? As said before reprogramming CDCLK in that case would require
>>> disabling the outputs first.
>>
>> I went with the hypothesis that if VBIOS/GOP was loaded it would have to
>> correct the cdclock, with wrong decimal value display cannot be enabled.
>> For example AUX will fail on SKL. So for correct display output enabled
>> cdclock has to be correctly programmed. If it is wrong display was not
>> enabled at all.
>>
>> The scenario which I am seeing is VBIOS/GOP is not loaded at all, and
>> the pre-os is not leaving cdclock in correct state.
>
> But it still enabled DPLL0? Why does it do that if it doesn't set up
> cdclk?

So I had discussion with some folks in BIOS team and came to know that 
on some boards even if higher cdclocks are supported, we might want to 
limit maximum cdclock. This is done by BIOS programming the cdclock to 
allowed maximum and VBIOS/GOP uses that as the max value rather than 
the real possible max. So the BIOS is supposed to program cdclock 
correctly and hence DPLL0 as well. My only assumption is in this case it 
has not correctly programmed due to some bug. I know if I work with them 
and ensure that BIOS programs correctly, the problem will be solved, but 
do you think it is worth while to remove the tight dependency altogether ?

>
>>
>>>
>>> We would also need to call skl_init_cdclk if the BIOS hasn't enabled the
>>> PLL for some reason. But I guess that could be a separate change.
>>>
>>>> +		} else {
>>>> +			dev_priv->skl_boot_cdclk = cdclk_freq;
>>>> +			if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>>>> +				DRM_ERROR("LCPLL1 is disabled\n");
>>>> +			else
>>>> +				intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>>>> +		}
>>>>    	} else if (IS_BROXTON(dev)) {
>>>>    		broxton_init_cdclk(dev);
>>>>    		broxton_ddi_phy_init(dev);
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index bbeb6d3..f734410 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -6634,12 +6634,15 @@ static int skylake_get_display_clock_speed(struct drm_device *dev)
>>>>    	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
>>>>    	uint32_t cdctl = I915_READ(CDCLK_CTL);
>>>>    	uint32_t linkrate;
>>>> +	int freq;
>>>>
>>>>    	if (!(lcpll1 & LCPLL_PLL_ENABLE))
>>>>    		return 24000; /* 24MHz is the cd freq with NSSC ref */
>>>>
>>>> -	if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540)
>>>> -		return 540000;
>>>> +	if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) {
>>>> +		freq = 540000;
>>>> +		goto verify;
>>>> +	}
>>>>
>>>>    	linkrate = (I915_READ(DPLL_CTRL1) &
>>>>    		    DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0)) >> 1;
>>>> @@ -6649,30 +6652,46 @@ static int skylake_get_display_clock_speed(struct drm_device *dev)
>>>>    		/* vco 8640 */
>>>>    		switch (cdctl & CDCLK_FREQ_SEL_MASK) {
>>>>    		case CDCLK_FREQ_450_432:
>>>> -			return 432000;
>>>> +			freq = 432000;
>>>> +			break;
>>>>    		case CDCLK_FREQ_337_308:
>>>> -			return 308570;
>>>> +			freq = 308570;
>>>> +			break;
>>>>    		case CDCLK_FREQ_675_617:
>>>> -			return 617140;
>>>> +			freq = 617140;
>>>> +			break;
>>>>    		default:
>>>>    			WARN(1, "Unknown cd freq selection\n");
>>>> +			return -EINVAL;
>>>>    		}
>>>>    	} else {
>>>>    		/* vco 8100 */
>>>>    		switch (cdctl & CDCLK_FREQ_SEL_MASK) {
>>>>    		case CDCLK_FREQ_450_432:
>>>> -			return 450000;
>>>> +			freq = 450000;
>>>> +			break;
>>>>    		case CDCLK_FREQ_337_308:
>>>> -			return 337500;
>>>> +			freq = 337500;
>>>> +			break;
>>>>    		case CDCLK_FREQ_675_617:
>>>> -			return 675000;
>>>> +			freq = 675000;
>>>> +			break;
>>>>    		default:
>>>>    			WARN(1, "Unknown cd freq selection\n");
>>>> +			return -EINVAL;
>>>>    		}
>>>>    	}
>>>>
>>>> -	/* error case, do as if DPLL0 isn't enabled */
>>>> -	return 24000;
>>>> +verify:
>>>> +	/*
>>>> +	 * Noticed in some instances that the freq selection is correct but
>>>> +	 * decimal part is programmed wrong from BIOS where pre-os does not
>>>> +	 * enable display. Verify the same as well.
>>>> +	 */
>>>> +	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
>>>> +		return freq;
>>>> +	else
>>>> +		return -EINVAL;
>>>>    }
>>>>
>>>>    static int broxton_get_display_clock_speed(struct drm_device *dev)
>>>
>>>
>>> _______________________________________________
>>> 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Kumar, Shobhit]

On 10/08/2015 05:54 PM, Ville Syrjälä wrote:
> On Thu, Oct 08, 2015 at 05:43:41PM +0530, Kumar, Shobhit wrote:
>> On 10/08/2015 04:59 PM, Imre Deak wrote:
>>> On to, 2015-10-08 at 09:58 +0530, Shobhit Kumar wrote:
>>>> Reuse what is programmed by pre-os, but in case there is no pre-os
>>>> initialization, init the cdclk with the max value by default untill
>>>> dynamic cdclk support comes.
>>>>
>>>> v2: Check if BIOS programmed correctly rather than always calling init
>>>>       - Do validation of programmed cdctl and what it is expected
>>>>       - Only do slk_init_cdclk if validation failed else reuse BIOS
>>>>         programmed value
>>>>
>>>> Cc: Imre Deak <imre.deak@intel.com>
>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_ddi.c     | 18 ++++++++++++-----
>>>>    drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++---------
>>>>    2 files changed, 42 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>> index 2d3cc82..3ec5618 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -2946,11 +2946,19 @@ void intel_ddi_pll_init(struct drm_device *dev)
>>>>    		int cdclk_freq;
>>>>
>>>>    		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>>>> -		dev_priv->skl_boot_cdclk = cdclk_freq;
>>>> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>>>> -			DRM_ERROR("LCPLL1 is disabled\n");
>>>> -		else
>>>> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>>>> +
>>>> +		/* Invalid CDCLK from BIOS ? */
>>>> +		if (cdclk_freq < 0) {
>>>> +			/* program to maximum cdclk till we have dynamic cdclk support */
>>>> +			dev_priv->skl_boot_cdclk = 675000;
>>>> +			skl_init_cdclk(dev_priv);
>>>
>>> This would still try to reprogram CDCLK if BIOS enabled an output with
>>> an incorrect CDCLK decimal part. Isn't this the exact scenario you're
>>> seeing? As said before reprogramming CDCLK in that case would require
>>> disabling the outputs first.
>>
>> I went with the hypothesis that if VBIOS/GOP was loaded it would have to
>> correct the cdclock, with wrong decimal value display cannot be enabled.
>> For example AUX will fail on SKL. So for correct display output enabled
>> cdclock has to be correctly programmed. If it is wrong display was not
>> enabled at all.
>>
>> The scenario which I am seeing is VBIOS/GOP is not loaded at all, and
>> the pre-os is not leaving cdclock in correct state.
>
> But it still enabled DPLL0? Why does it do that if it doesn't set up
> cdclk?
>

Okay digging more into FSP code, found a bug where even when display is 
not initialized in pre-os, still there was an initialization path for 
enabling display audio for HDMI which was enabling DPLL0. That also 
should have been not invoked when display itself is not being loaded. 
Not sure what going on in there !! Will work with FSP team and get this 
rectified. After that sanitize patch(following up this mail) can check 
the clock related programming and sanitize if needed as discussed with 
you Ville on IRC.

Regards
Shobhit

>>
>>>
>>> We would also need to call skl_init_cdclk if the BIOS hasn't enabled the
>>> PLL for some reason. But I guess that could be a separate change.
>>>
>>>> +		} else {
>>>> +			dev_priv->skl_boot_cdclk = cdclk_freq;
>>>> +			if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>>>> +				DRM_ERROR("LCPLL1 is disabled\n");
>>>> +			else
>>>> +				intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>>>> +		}
>>>>    	} else if (IS_BROXTON(dev)) {
>>>>    		broxton_init_cdclk(dev);
>>>>    		broxton_ddi_phy_init(dev);
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index bbeb6d3..f734410 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -6634,12 +6634,15 @@ static int skylake_get_display_clock_speed(struct drm_device *dev)
>>>>    	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
>>>>    	uint32_t cdctl = I915_READ(CDCLK_CTL);
>>>>    	uint32_t linkrate;
>>>> +	int freq;
>>>>
>>>>    	if (!(lcpll1 & LCPLL_PLL_ENABLE))
>>>>    		return 24000; /* 24MHz is the cd freq with NSSC ref */
>>>>
>>>> -	if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540)
>>>> -		return 540000;
>>>> +	if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) {
>>>> +		freq = 540000;
>>>> +		goto verify;
>>>> +	}
>>>>
>>>>    	linkrate = (I915_READ(DPLL_CTRL1) &
>>>>    		    DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0)) >> 1;
>>>> @@ -6649,30 +6652,46 @@ static int skylake_get_display_clock_speed(struct drm_device *dev)
>>>>    		/* vco 8640 */
>>>>    		switch (cdctl & CDCLK_FREQ_SEL_MASK) {
>>>>    		case CDCLK_FREQ_450_432:
>>>> -			return 432000;
>>>> +			freq = 432000;
>>>> +			break;
>>>>    		case CDCLK_FREQ_337_308:
>>>> -			return 308570;
>>>> +			freq = 308570;
>>>> +			break;
>>>>    		case CDCLK_FREQ_675_617:
>>>> -			return 617140;
>>>> +			freq = 617140;
>>>> +			break;
>>>>    		default:
>>>>    			WARN(1, "Unknown cd freq selection\n");
>>>> +			return -EINVAL;
>>>>    		}
>>>>    	} else {
>>>>    		/* vco 8100 */
>>>>    		switch (cdctl & CDCLK_FREQ_SEL_MASK) {
>>>>    		case CDCLK_FREQ_450_432:
>>>> -			return 450000;
>>>> +			freq = 450000;
>>>> +			break;
>>>>    		case CDCLK_FREQ_337_308:
>>>> -			return 337500;
>>>> +			freq = 337500;
>>>> +			break;
>>>>    		case CDCLK_FREQ_675_617:
>>>> -			return 675000;
>>>> +			freq = 675000;
>>>> +			break;
>>>>    		default:
>>>>    			WARN(1, "Unknown cd freq selection\n");
>>>> +			return -EINVAL;
>>>>    		}
>>>>    	}
>>>>
>>>> -	/* error case, do as if DPLL0 isn't enabled */
>>>> -	return 24000;
>>>> +verify:
>>>> +	/*
>>>> +	 * Noticed in some instances that the freq selection is correct but
>>>> +	 * decimal part is programmed wrong from BIOS where pre-os does not
>>>> +	 * enable display. Verify the same as well.
>>>> +	 */
>>>> +	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
>>>> +		return freq;
>>>> +	else
>>>> +		return -EINVAL;
>>>>    }
>>>>
>>>>    static int broxton_get_display_clock_speed(struct drm_device *dev)
>>>
>>>
>>> _______________________________________________
>>> 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[v3] drm/i915/skl: If needed sanitize bios programmed cdclk

[Shobhit Kumar]

Especially in cases where pre-os does not enable display, cdclk might
not be in sane state. During sanitization initialize cdclk with maximum
value till we get dynamic cdclk support.

v2: Check if BIOS programmed correctly rather than always calling init
    - Do validation of programmed cdctl and what it is expected
    - Only do slk_init_cdclk if validation failed else reuse BIOS
      programmed value

v3: Move the validation logic in a separate sanitize function (Ville)

Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     | 12 ++++++++----
 drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index b25e99a..86d43e6 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2949,10 +2949,14 @@ void intel_ddi_pll_init(struct drm_device *dev)
 
 		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
 		dev_priv->skl_boot_cdclk = cdclk_freq;
-		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
-			DRM_ERROR("LCPLL1 is disabled\n");
-		else
-			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
+		if (skl_sanitize_cdclk(dev_priv))
+			DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n");
+		else {
+			if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
+				DRM_ERROR("LCPLL1 is disabled\n");
+			else
+				intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
+		}
 	} else if (IS_BROXTON(dev)) {
 		broxton_init_cdclk(dev);
 		broxton_ddi_phy_init(dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5f37f84..98333d3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5784,6 +5784,37 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
 		DRM_ERROR("DBuf power enable timeout\n");
 }
 
+int skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
+{
+	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
+	uint32_t cdctl = I915_READ(CDCLK_CTL);
+	int freq = dev_priv->skl_boot_cdclk;
+
+	/* Is PLL enabled and locked ? */
+	if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK)))
+		goto sanitize;
+
+	/* DPLL okay; verify the cdclock
+	 *
+	 * Noticed in some instances that the freq selection is correct but
+	 * decimal part is programmed wrong from BIOS where pre-os does not
+	 * enable display. Verify the same as well.
+	 */
+	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
+		/* All well; nothing to sanitize */
+		return false;
+sanitize:
+	/*
+	 * As of now initialize with max cdclk till
+	 * we get dynamic cdclk support
+	 * */
+	dev_priv->skl_boot_cdclk = 675000;
+	skl_init_cdclk(dev_priv);
+
+	/* we did have to sanitize */
+	return true;
+}
+
 /* Adjust CDclk dividers to allow high res or save power if possible */
 static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0598932..ec10e6a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1152,6 +1152,7 @@ void broxton_ddi_phy_uninit(struct drm_device *dev);
 void bxt_enable_dc9(struct drm_i915_private *dev_priv);
 void bxt_disable_dc9(struct drm_i915_private *dev_priv);
 void skl_init_cdclk(struct drm_i915_private *dev_priv);
+int skl_sanitize_cdclk(struct drm_i915_private *dev_priv);
 void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
 void intel_dp_get_m_n(struct intel_crtc *crtc,
 		      struct intel_crtc_state *pipe_config);
-- 
2.4.3

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

Re: [v3] drm/i915/skl: If needed sanitize bios programmed cdclk

[Kumar, Shobhit]

On 10/16/2015 06:48 PM, Shobhit Kumar wrote:
> Especially in cases where pre-os does not enable display, cdclk might
> not be in sane state. During sanitization initialize cdclk with maximum
> value till we get dynamic cdclk support.
>
> v2: Check if BIOS programmed correctly rather than always calling init
>      - Do validation of programmed cdctl and what it is expected
>      - Only do slk_init_cdclk if validation failed else reuse BIOS
>        programmed value
>
> v3: Move the validation logic in a separate sanitize function (Ville)

Ville, does  this come even close to what you had in mind ?

>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_ddi.c     | 12 ++++++++----
>   drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_drv.h     |  1 +
>   3 files changed, 40 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index b25e99a..86d43e6 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2949,10 +2949,14 @@ void intel_ddi_pll_init(struct drm_device *dev)
>
>   		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>   		dev_priv->skl_boot_cdclk = cdclk_freq;
> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> -			DRM_ERROR("LCPLL1 is disabled\n");
> -		else
> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> +		if (skl_sanitize_cdclk(dev_priv))
> +			DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n");
> +		else {
> +			if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> +				DRM_ERROR("LCPLL1 is disabled\n");
> +			else
> +				intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> +		}
>   	} else if (IS_BROXTON(dev)) {
>   		broxton_init_cdclk(dev);
>   		broxton_ddi_phy_init(dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5f37f84..98333d3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5784,6 +5784,37 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
>   		DRM_ERROR("DBuf power enable timeout\n");
>   }
>
> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
> +{
> +	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
> +	uint32_t cdctl = I915_READ(CDCLK_CTL);
> +	int freq = dev_priv->skl_boot_cdclk;
> +
> +	/* Is PLL enabled and locked ? */
> +	if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK)))
> +		goto sanitize;
> +
> +	/* DPLL okay; verify the cdclock
> +	 *
> +	 * Noticed in some instances that the freq selection is correct but
> +	 * decimal part is programmed wrong from BIOS where pre-os does not
> +	 * enable display. Verify the same as well.
> +	 */
> +	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
> +		/* All well; nothing to sanitize */
> +		return false;
> +sanitize:
> +	/*
> +	 * As of now initialize with max cdclk till
> +	 * we get dynamic cdclk support
> +	 * */
> +	dev_priv->skl_boot_cdclk = 675000;
> +	skl_init_cdclk(dev_priv);
> +
> +	/* we did have to sanitize */
> +	return true;
> +}
> +
>   /* Adjust CDclk dividers to allow high res or save power if possible */
>   static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>   {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0598932..ec10e6a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1152,6 +1152,7 @@ void broxton_ddi_phy_uninit(struct drm_device *dev);
>   void bxt_enable_dc9(struct drm_i915_private *dev_priv);
>   void bxt_disable_dc9(struct drm_i915_private *dev_priv);
>   void skl_init_cdclk(struct drm_i915_private *dev_priv);
> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv);
>   void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
>   void intel_dp_get_m_n(struct intel_crtc *crtc,
>   		      struct intel_crtc_state *pipe_config);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v3] drm/i915/skl: If needed sanitize bios programmed cdclk

[Ville Syrjälä]

On Fri, Oct 16, 2015 at 06:48:53PM +0530, Shobhit Kumar wrote:
> Especially in cases where pre-os does not enable display, cdclk might
> not be in sane state. During sanitization initialize cdclk with maximum
> value till we get dynamic cdclk support.
> 
> v2: Check if BIOS programmed correctly rather than always calling init
>     - Do validation of programmed cdctl and what it is expected
>     - Only do slk_init_cdclk if validation failed else reuse BIOS
>       programmed value
> 
> v3: Move the validation logic in a separate sanitize function (Ville)
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     | 12 ++++++++----
>  drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  3 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index b25e99a..86d43e6 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2949,10 +2949,14 @@ void intel_ddi_pll_init(struct drm_device *dev)
>  
>  		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>  		dev_priv->skl_boot_cdclk = cdclk_freq;
> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> -			DRM_ERROR("LCPLL1 is disabled\n");
> -		else
> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> +		if (skl_sanitize_cdclk(dev_priv))
> +			DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n");
> +		else {
> +			if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> +				DRM_ERROR("LCPLL1 is disabled\n");

Since skl_sanitize_cdclk() will enable the PLL this shouldn't
happen anymore, right?

> +			else
> +				intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> +		}
>  	} else if (IS_BROXTON(dev)) {
>  		broxton_init_cdclk(dev);
>  		broxton_ddi_phy_init(dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5f37f84..98333d3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5784,6 +5784,37 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
>  		DRM_ERROR("DBuf power enable timeout\n");
>  }
>  
> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
> +{
> +	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
> +	uint32_t cdctl = I915_READ(CDCLK_CTL);
> +	int freq = dev_priv->skl_boot_cdclk;
> +
> +	/* Is PLL enabled and locked ? */
> +	if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK)))
> +		goto sanitize;
> +
> +	/* DPLL okay; verify the cdclock
> +	 *
> +	 * Noticed in some instances that the freq selection is correct but
> +	 * decimal part is programmed wrong from BIOS where pre-os does not
> +	 * enable display. Verify the same as well.
> +	 */
> +	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
> +		/* All well; nothing to sanitize */
> +		return false;
> +sanitize:
> +	/*
> +	 * As of now initialize with max cdclk till
> +	 * we get dynamic cdclk support
> +	 * */
> +	dev_priv->skl_boot_cdclk = 675000;

Should be '= dev_priv->max_cdclk'

I think we end up doing the intel_update_cdclk() before this gets
called, so max_cdclk should already contain something sensible. The
whole init sequence is a bit messy currently, but I think we can put
off cleaning it up after the dc6 stuff gets sorted.

> +	skl_init_cdclk(dev_priv);
> +
> +	/* we did have to sanitize */
> +	return true;
> +}
> +
>  /* Adjust CDclk dividers to allow high res or save power if possible */
>  static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0598932..ec10e6a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1152,6 +1152,7 @@ void broxton_ddi_phy_uninit(struct drm_device *dev);
>  void bxt_enable_dc9(struct drm_i915_private *dev_priv);
>  void bxt_disable_dc9(struct drm_i915_private *dev_priv);
>  void skl_init_cdclk(struct drm_i915_private *dev_priv);
> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv);
>  void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
>  void intel_dp_get_m_n(struct intel_crtc *crtc,
>  		      struct intel_crtc_state *pipe_config);
> -- 
> 2.4.3

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

Re: [v3] drm/i915/skl: If needed sanitize bios programmed cdclk

[Kumar, Shobhit]

On 10/19/2015 07:18 PM, Ville Syrjälä wrote:
> On Fri, Oct 16, 2015 at 06:48:53PM +0530, Shobhit Kumar wrote:
>> Especially in cases where pre-os does not enable display, cdclk might
>> not be in sane state. During sanitization initialize cdclk with maximum
>> value till we get dynamic cdclk support.
>>
>> v2: Check if BIOS programmed correctly rather than always calling init
>>      - Do validation of programmed cdctl and what it is expected
>>      - Only do slk_init_cdclk if validation failed else reuse BIOS
>>        programmed value
>>
>> v3: Move the validation logic in a separate sanitize function (Ville)
>>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_ddi.c     | 12 ++++++++----
>>   drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>   3 files changed, 40 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index b25e99a..86d43e6 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -2949,10 +2949,14 @@ void intel_ddi_pll_init(struct drm_device *dev)
>>
>>   		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>>   		dev_priv->skl_boot_cdclk = cdclk_freq;
>> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>> -			DRM_ERROR("LCPLL1 is disabled\n");
>> -		else
>> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>> +		if (skl_sanitize_cdclk(dev_priv))
>> +			DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n");
>> +		else {
>> +			if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>> +				DRM_ERROR("LCPLL1 is disabled\n");
>
> Since skl_sanitize_cdclk() will enable the PLL this shouldn't
> happen anymore, right?
>

Yeah right. Will remove.

>> +			else
>> +				intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>> +		}
>>   	} else if (IS_BROXTON(dev)) {
>>   		broxton_init_cdclk(dev);
>>   		broxton_ddi_phy_init(dev);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 5f37f84..98333d3 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5784,6 +5784,37 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
>>   		DRM_ERROR("DBuf power enable timeout\n");
>>   }
>>
>> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
>> +{
>> +	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
>> +	uint32_t cdctl = I915_READ(CDCLK_CTL);
>> +	int freq = dev_priv->skl_boot_cdclk;
>> +
>> +	/* Is PLL enabled and locked ? */
>> +	if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK)))
>> +		goto sanitize;
>> +
>> +	/* DPLL okay; verify the cdclock
>> +	 *
>> +	 * Noticed in some instances that the freq selection is correct but
>> +	 * decimal part is programmed wrong from BIOS where pre-os does not
>> +	 * enable display. Verify the same as well.
>> +	 */
>> +	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
>> +		/* All well; nothing to sanitize */
>> +		return false;
>> +sanitize:
>> +	/*
>> +	 * As of now initialize with max cdclk till
>> +	 * we get dynamic cdclk support
>> +	 * */
>> +	dev_priv->skl_boot_cdclk = 675000;
>
> Should be '= dev_priv->max_cdclk'
>
> I think we end up doing the intel_update_cdclk() before this gets
> called, so max_cdclk should already contain something sensible. The
> whole init sequence is a bit messy currently, but I think we can put
> off cleaning it up after the dc6 stuff gets sorted.
>

Actually at the end of skl_init_cdclock, intel_update_cdclk will be 
called and initialize the max_cdclk correctly. In case there was no 
sanitization needed, the BIOS programmed cdclk is stored in 
skl_boot_cdclk and that is used inside the skl_init_cdclk. Same is 
invoked resume time, so we need to update this variable or else change 
the logic.

>> +	skl_init_cdclk(dev_priv);
>> +
>> +	/* we did have to sanitize */
>> +	return true;
>> +}
>> +
>>   /* Adjust CDclk dividers to allow high res or save power if possible */
>>   static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>   {
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 0598932..ec10e6a 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1152,6 +1152,7 @@ void broxton_ddi_phy_uninit(struct drm_device *dev);
>>   void bxt_enable_dc9(struct drm_i915_private *dev_priv);
>>   void bxt_disable_dc9(struct drm_i915_private *dev_priv);
>>   void skl_init_cdclk(struct drm_i915_private *dev_priv);
>> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv);
>>   void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
>>   void intel_dp_get_m_n(struct intel_crtc *crtc,
>>   		      struct intel_crtc_state *pipe_config);
>> --
>> 2.4.3
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v3] drm/i915/skl: If needed sanitize bios programmed cdclk

[Ville Syrjälä]

On Tue, Oct 20, 2015 at 03:27:24PM +0530, Kumar, Shobhit wrote:
> On 10/19/2015 07:18 PM, Ville Syrjälä wrote:
> > On Fri, Oct 16, 2015 at 06:48:53PM +0530, Shobhit Kumar wrote:
> >> Especially in cases where pre-os does not enable display, cdclk might
> >> not be in sane state. During sanitization initialize cdclk with maximum
> >> value till we get dynamic cdclk support.
> >>
> >> v2: Check if BIOS programmed correctly rather than always calling init
> >>      - Do validation of programmed cdctl and what it is expected
> >>      - Only do slk_init_cdclk if validation failed else reuse BIOS
> >>        programmed value
> >>
> >> v3: Move the validation logic in a separate sanitize function (Ville)
> >>
> >> Cc: Imre Deak <imre.deak@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_ddi.c     | 12 ++++++++----
> >>   drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++
> >>   drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >>   3 files changed, 40 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index b25e99a..86d43e6 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -2949,10 +2949,14 @@ void intel_ddi_pll_init(struct drm_device *dev)
> >>
> >>   		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
> >>   		dev_priv->skl_boot_cdclk = cdclk_freq;
> >> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> >> -			DRM_ERROR("LCPLL1 is disabled\n");
> >> -		else
> >> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> >> +		if (skl_sanitize_cdclk(dev_priv))
> >> +			DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n");
> >> +		else {
> >> +			if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> >> +				DRM_ERROR("LCPLL1 is disabled\n");
> >
> > Since skl_sanitize_cdclk() will enable the PLL this shouldn't
> > happen anymore, right?
> >
> 
> Yeah right. Will remove.
> 
> >> +			else
> >> +				intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> >> +		}
> >>   	} else if (IS_BROXTON(dev)) {
> >>   		broxton_init_cdclk(dev);
> >>   		broxton_ddi_phy_init(dev);
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 5f37f84..98333d3 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -5784,6 +5784,37 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
> >>   		DRM_ERROR("DBuf power enable timeout\n");
> >>   }
> >>
> >> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
> >> +{
> >> +	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
> >> +	uint32_t cdctl = I915_READ(CDCLK_CTL);
> >> +	int freq = dev_priv->skl_boot_cdclk;
> >> +
> >> +	/* Is PLL enabled and locked ? */
> >> +	if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK)))
> >> +		goto sanitize;
> >> +
> >> +	/* DPLL okay; verify the cdclock
> >> +	 *
> >> +	 * Noticed in some instances that the freq selection is correct but
> >> +	 * decimal part is programmed wrong from BIOS where pre-os does not
> >> +	 * enable display. Verify the same as well.
> >> +	 */
> >> +	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
> >> +		/* All well; nothing to sanitize */
> >> +		return false;
> >> +sanitize:
> >> +	/*
> >> +	 * As of now initialize with max cdclk till
> >> +	 * we get dynamic cdclk support
> >> +	 * */
> >> +	dev_priv->skl_boot_cdclk = 675000;
> >
> > Should be '= dev_priv->max_cdclk'
> >
> > I think we end up doing the intel_update_cdclk() before this gets
> > called, so max_cdclk should already contain something sensible. The
> > whole init sequence is a bit messy currently, but I think we can put
> > off cleaning it up after the dc6 stuff gets sorted.
> >
> 
> Actually at the end of skl_init_cdclock, intel_update_cdclk will be 
> called and initialize the max_cdclk correctly.

It gets called already earlier from intel_modeset_init().

> In case there was no 
> sanitization needed, the BIOS programmed cdclk is stored in 
> skl_boot_cdclk and that is used inside the skl_init_cdclk. Same is 
> invoked resume time, so we need to update this variable or else change 
> the logic.
> 
> >> +	skl_init_cdclk(dev_priv);
> >> +
> >> +	/* we did have to sanitize */
> >> +	return true;
> >> +}
> >> +
> >>   /* Adjust CDclk dividers to allow high res or save power if possible */
> >>   static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> >>   {
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 0598932..ec10e6a 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -1152,6 +1152,7 @@ void broxton_ddi_phy_uninit(struct drm_device *dev);
> >>   void bxt_enable_dc9(struct drm_i915_private *dev_priv);
> >>   void bxt_disable_dc9(struct drm_i915_private *dev_priv);
> >>   void skl_init_cdclk(struct drm_i915_private *dev_priv);
> >> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv);
> >>   void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
> >>   void intel_dp_get_m_n(struct intel_crtc *crtc,
> >>   		      struct intel_crtc_state *pipe_config);
> >> --
> >> 2.4.3
> >

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

Re: [v3] drm/i915/skl: If needed sanitize bios programmed cdclk

[Kumar, Shobhit]

On 10/20/2015 04:49 PM, Ville Syrjälä wrote:
> On Tue, Oct 20, 2015 at 03:27:24PM +0530, Kumar, Shobhit wrote:
>> On 10/19/2015 07:18 PM, Ville Syrjälä wrote:
>>> On Fri, Oct 16, 2015 at 06:48:53PM +0530, Shobhit Kumar wrote:
>>>> Especially in cases where pre-os does not enable display, cdclk might
>>>> not be in sane state. During sanitization initialize cdclk with maximum
>>>> value till we get dynamic cdclk support.
>>>>
>>>> v2: Check if BIOS programmed correctly rather than always calling init
>>>>       - Do validation of programmed cdctl and what it is expected
>>>>       - Only do slk_init_cdclk if validation failed else reuse BIOS
>>>>         programmed value
>>>>
>>>> v3: Move the validation logic in a separate sanitize function (Ville)
>>>>
>>>> Cc: Imre Deak <imre.deak@intel.com>
>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_ddi.c     | 12 ++++++++----
>>>>    drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>>>    3 files changed, 40 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>> index b25e99a..86d43e6 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -2949,10 +2949,14 @@ void intel_ddi_pll_init(struct drm_device *dev)
>>>>
>>>>    		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>>>>    		dev_priv->skl_boot_cdclk = cdclk_freq;
>>>> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>>>> -			DRM_ERROR("LCPLL1 is disabled\n");
>>>> -		else
>>>> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>>>> +		if (skl_sanitize_cdclk(dev_priv))
>>>> +			DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n");
>>>> +		else {
>>>> +			if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>>>> +				DRM_ERROR("LCPLL1 is disabled\n");
>>>
>>> Since skl_sanitize_cdclk() will enable the PLL this shouldn't
>>> happen anymore, right?
>>>
>>
>> Yeah right. Will remove.
>>
>>>> +			else
>>>> +				intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>>>> +		}
>>>>    	} else if (IS_BROXTON(dev)) {
>>>>    		broxton_init_cdclk(dev);
>>>>    		broxton_ddi_phy_init(dev);
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 5f37f84..98333d3 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -5784,6 +5784,37 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
>>>>    		DRM_ERROR("DBuf power enable timeout\n");
>>>>    }
>>>>
>>>> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
>>>> +{
>>>> +	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
>>>> +	uint32_t cdctl = I915_READ(CDCLK_CTL);
>>>> +	int freq = dev_priv->skl_boot_cdclk;
>>>> +
>>>> +	/* Is PLL enabled and locked ? */
>>>> +	if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK)))
>>>> +		goto sanitize;
>>>> +
>>>> +	/* DPLL okay; verify the cdclock
>>>> +	 *
>>>> +	 * Noticed in some instances that the freq selection is correct but
>>>> +	 * decimal part is programmed wrong from BIOS where pre-os does not
>>>> +	 * enable display. Verify the same as well.
>>>> +	 */
>>>> +	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
>>>> +		/* All well; nothing to sanitize */
>>>> +		return false;
>>>> +sanitize:
>>>> +	/*
>>>> +	 * As of now initialize with max cdclk till
>>>> +	 * we get dynamic cdclk support
>>>> +	 * */
>>>> +	dev_priv->skl_boot_cdclk = 675000;
>>>
>>> Should be '= dev_priv->max_cdclk'
>>>
>>> I think we end up doing the intel_update_cdclk() before this gets
>>> called, so max_cdclk should already contain something sensible. The
>>> whole init sequence is a bit messy currently, but I think we can put
>>> off cleaning it up after the dc6 stuff gets sorted.
>>>
>>
>> Actually at the end of skl_init_cdclock, intel_update_cdclk will be
>> called and initialize the max_cdclk correctly.
>
> It gets called already earlier from intel_modeset_init().

Yeah, I misunderstood your comment earlier.

>
>> In case there was no
>> sanitization needed, the BIOS programmed cdclk is stored in
>> skl_boot_cdclk and that is used inside the skl_init_cdclk. Same is
>> invoked resume time, so we need to update this variable or else change
>> the logic.
>>
>>>> +	skl_init_cdclk(dev_priv);
>>>> +
>>>> +	/* we did have to sanitize */
>>>> +	return true;
>>>> +}
>>>> +
>>>>    /* Adjust CDclk dividers to allow high res or save power if possible */
>>>>    static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>    {
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>> index 0598932..ec10e6a 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -1152,6 +1152,7 @@ void broxton_ddi_phy_uninit(struct drm_device *dev);
>>>>    void bxt_enable_dc9(struct drm_i915_private *dev_priv);
>>>>    void bxt_disable_dc9(struct drm_i915_private *dev_priv);
>>>>    void skl_init_cdclk(struct drm_i915_private *dev_priv);
>>>> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv);
>>>>    void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
>>>>    void intel_dp_get_m_n(struct intel_crtc *crtc,
>>>>    		      struct intel_crtc_state *pipe_config);
>>>> --
>>>> 2.4.3
>>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[v4] drm/i915/skl: If needed sanitize bios programmed cdclk

[Shobhit Kumar]

Especially in cases where pre-os does not enable display, cdclk might
not be in sane state. During sanitization initialize cdclk with maximum
value till we get dynamic cdclk support.

v2: Check if BIOS programmed correctly rather than always calling init
    - Do validation of programmed cdctl and what it is expected
    - Only do slk_init_cdclk if validation failed else reuse BIOS
      programmed value

v3: Move the validation logic in a separate sanitize function (Ville)

v4: No need to check LCPLL after sanitize and use max_cdclk_freq instead
    of hardcoded value (Ville)

Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     |  4 ++--
 drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index b25e99a..824b863 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2949,8 +2949,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
 
 		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
 		dev_priv->skl_boot_cdclk = cdclk_freq;
-		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
-			DRM_ERROR("LCPLL1 is disabled\n");
+		if (skl_sanitize_cdclk(dev_priv))
+			DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n");
 		else
 			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
 	} else if (IS_BROXTON(dev)) {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5f37f84..4933b72 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5784,6 +5784,37 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
 		DRM_ERROR("DBuf power enable timeout\n");
 }
 
+int skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
+{
+	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
+	uint32_t cdctl = I915_READ(CDCLK_CTL);
+	int freq = dev_priv->skl_boot_cdclk;
+
+	/* Is PLL enabled and locked ? */
+	if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK)))
+		goto sanitize;
+
+	/* DPLL okay; verify the cdclock
+	 *
+	 * Noticed in some instances that the freq selection is correct but
+	 * decimal part is programmed wrong from BIOS where pre-os does not
+	 * enable display. Verify the same as well.
+	 */
+	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
+		/* All well; nothing to sanitize */
+		return false;
+sanitize:
+	/*
+	 * As of now initialize with max cdclk till
+	 * we get dynamic cdclk support
+	 * */
+	dev_priv->skl_boot_cdclk = dev_priv->max_cdclk_freq;
+	skl_init_cdclk(dev_priv);
+
+	/* we did have to sanitize */
+	return true;
+}
+
 /* Adjust CDclk dividers to allow high res or save power if possible */
 static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0598932..ec10e6a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1152,6 +1152,7 @@ void broxton_ddi_phy_uninit(struct drm_device *dev);
 void bxt_enable_dc9(struct drm_i915_private *dev_priv);
 void bxt_disable_dc9(struct drm_i915_private *dev_priv);
 void skl_init_cdclk(struct drm_i915_private *dev_priv);
+int skl_sanitize_cdclk(struct drm_i915_private *dev_priv);
 void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
 void intel_dp_get_m_n(struct intel_crtc *crtc,
 		      struct intel_crtc_state *pipe_config);
-- 
2.4.3

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

Re: [v4] drm/i915/skl: If needed sanitize bios programmed cdclk

[Ville Syrjälä]

On Tue, Oct 20, 2015 at 06:13:12PM +0530, Shobhit Kumar wrote:
> Especially in cases where pre-os does not enable display, cdclk might
> not be in sane state. During sanitization initialize cdclk with maximum
> value till we get dynamic cdclk support.
> 
> v2: Check if BIOS programmed correctly rather than always calling init
>     - Do validation of programmed cdctl and what it is expected
>     - Only do slk_init_cdclk if validation failed else reuse BIOS
>       programmed value
> 
> v3: Move the validation logic in a separate sanitize function (Ville)
> 
> v4: No need to check LCPLL after sanitize and use max_cdclk_freq instead
>     of hardcoded value (Ville)
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |  4 ++--
>  drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  3 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index b25e99a..824b863 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2949,8 +2949,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
>  
>  		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>  		dev_priv->skl_boot_cdclk = cdclk_freq;
> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> -			DRM_ERROR("LCPLL1 is disabled\n");
> +		if (skl_sanitize_cdclk(dev_priv))
> +			DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n");
>  		else
>  			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>  	} else if (IS_BROXTON(dev)) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5f37f84..4933b72 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5784,6 +5784,37 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
>  		DRM_ERROR("DBuf power enable timeout\n");
>  }
>  
> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
> +{
> +	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
> +	uint32_t cdctl = I915_READ(CDCLK_CTL);
> +	int freq = dev_priv->skl_boot_cdclk;
> +
> +	/* Is PLL enabled and locked ? */
> +	if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK)))
> +		goto sanitize;
> +
> +	/* DPLL okay; verify the cdclock
> +	 *
> +	 * Noticed in some instances that the freq selection is correct but
> +	 * decimal part is programmed wrong from BIOS where pre-os does not
> +	 * enable display. Verify the same as well.
> +	 */
> +	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
> +		/* All well; nothing to sanitize */
> +		return false;
> +sanitize:
> +	/*
> +	 * As of now initialize with max cdclk till
> +	 * we get dynamic cdclk support
> +	 * */
> +	dev_priv->skl_boot_cdclk = dev_priv->max_cdclk_freq;
> +	skl_init_cdclk(dev_priv);
> +
> +	/* we did have to sanitize */
> +	return true;
> +}
> +
>  /* Adjust CDclk dividers to allow high res or save power if possible */
>  static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0598932..ec10e6a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1152,6 +1152,7 @@ void broxton_ddi_phy_uninit(struct drm_device *dev);
>  void bxt_enable_dc9(struct drm_i915_private *dev_priv);
>  void bxt_disable_dc9(struct drm_i915_private *dev_priv);
>  void skl_init_cdclk(struct drm_i915_private *dev_priv);
> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv);
>  void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
>  void intel_dp_get_m_n(struct intel_crtc *crtc,
>  		      struct intel_crtc_state *pipe_config);
> -- 
> 2.4.3

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

Re: [v4] drm/i915/skl: If needed sanitize bios programmed cdclk

[Daniel Vetter]

On Tue, Oct 20, 2015 at 03:52:10PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 20, 2015 at 06:13:12PM +0530, Shobhit Kumar wrote:
> > Especially in cases where pre-os does not enable display, cdclk might
> > not be in sane state. During sanitization initialize cdclk with maximum
> > value till we get dynamic cdclk support.
> > 
> > v2: Check if BIOS programmed correctly rather than always calling init
> >     - Do validation of programmed cdctl and what it is expected
> >     - Only do slk_init_cdclk if validation failed else reuse BIOS
> >       programmed value
> > 
> > v3: Move the validation logic in a separate sanitize function (Ville)
> > 
> > v4: No need to check LCPLL after sanitize and use max_cdclk_freq instead
> >     of hardcoded value (Ville)
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Queued for -next, thanks for the patch.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c     |  4 ++--
> >  drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >  3 files changed, 34 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index b25e99a..824b863 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2949,8 +2949,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
> >  
> >  		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
> >  		dev_priv->skl_boot_cdclk = cdclk_freq;
> > -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> > -			DRM_ERROR("LCPLL1 is disabled\n");
> > +		if (skl_sanitize_cdclk(dev_priv))
> > +			DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n");
> >  		else
> >  			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> >  	} else if (IS_BROXTON(dev)) {
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 5f37f84..4933b72 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5784,6 +5784,37 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
> >  		DRM_ERROR("DBuf power enable timeout\n");
> >  }
> >  
> > +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
> > +{
> > +	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
> > +	uint32_t cdctl = I915_READ(CDCLK_CTL);
> > +	int freq = dev_priv->skl_boot_cdclk;
> > +
> > +	/* Is PLL enabled and locked ? */
> > +	if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK)))
> > +		goto sanitize;
> > +
> > +	/* DPLL okay; verify the cdclock
> > +	 *
> > +	 * Noticed in some instances that the freq selection is correct but
> > +	 * decimal part is programmed wrong from BIOS where pre-os does not
> > +	 * enable display. Verify the same as well.
> > +	 */
> > +	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
> > +		/* All well; nothing to sanitize */
> > +		return false;
> > +sanitize:
> > +	/*
> > +	 * As of now initialize with max cdclk till
> > +	 * we get dynamic cdclk support
> > +	 * */
> > +	dev_priv->skl_boot_cdclk = dev_priv->max_cdclk_freq;
> > +	skl_init_cdclk(dev_priv);
> > +
> > +	/* we did have to sanitize */
> > +	return true;
> > +}
> > +
> >  /* Adjust CDclk dividers to allow high res or save power if possible */
> >  static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 0598932..ec10e6a 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1152,6 +1152,7 @@ void broxton_ddi_phy_uninit(struct drm_device *dev);
> >  void bxt_enable_dc9(struct drm_i915_private *dev_priv);
> >  void bxt_disable_dc9(struct drm_i915_private *dev_priv);
> >  void skl_init_cdclk(struct drm_i915_private *dev_priv);
> > +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv);
> >  void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
> >  void intel_dp_get_m_n(struct intel_crtc *crtc,
> >  		      struct intel_crtc_state *pipe_config);
> > -- 
> > 2.4.3
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
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