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

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

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