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

["Kumar, Shobhit" <shobhit.kumar@linux.intel.com>]

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