On Wed, May 13, 2015 at 03:54:32PM +0100, Tvrtko Ursulin wrote:
> 
> Hi,

Hi Tvrtko,

> >+static bool
> >+skl_ddi_pll_select(struct intel_crtc *intel_crtc,
> >+               struct intel_encoder *intel_encoder,
> >+               int clock)
> >+{
> >+    struct intel_shared_dpll *pll;
> >+    uint32_t ctrl1, cfgcr1, cfgcr2;
> >+
> >+    /*
> >+     * See comment in intel_dpll_hw_state to understand why we always use 0
> >+     * as the DPLL id in this function.
> >+     */
> >+
> >+    ctrl1 = DPLL_CTRL1_OVERRIDE(0);
> >+
> >+    if (intel_encoder->type == INTEL_OUTPUT_HDMI) {
> >+            struct skl_wrpll_params wrpll_params = { 0, };
> >+
> >+            ctrl1 |= DPLL_CTRL1_HDMI_MODE(0);
> >+
> >+            skl_ddi_calculate_wrpll(clock * 1000, &wrpll_params);
> >+
> >+            cfgcr1 = DPLL_CFGCR1_FREQ_ENABLE |
> >+                     DPLL_CFGCR1_DCO_FRACTION(wrpll_params.dco_fraction) |
> >+                     wrpll_params.dco_integer;
> >+
> >+            cfgcr2 = DPLL_CFGCR2_QDIV_RATIO(wrpll_params.qdiv_ratio) |
> >+                     DPLL_CFGCR2_QDIV_MODE(wrpll_params.qdiv_mode) |
> >+                     DPLL_CFGCR2_KDIV(wrpll_params.kdiv) |
> >+                     DPLL_CFGCR2_PDIV(wrpll_params.pdiv) |
> >+                     wrpll_params.central_freq;
> >+    } else if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) {
> >+            struct drm_encoder *encoder = &intel_encoder->base;
> >+            struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >+
> >+            switch (intel_dp->link_bw) {
> >+            case DP_LINK_BW_1_62:
> >+                    ctrl1 |= DPLL_CRTL1_LINK_RATE(DPLL_CRTL1_LINK_RATE_810, 
> >0);
> >+                    break;
> >+            case DP_LINK_BW_2_7:
> >+                    ctrl1 |= 
> >DPLL_CRTL1_LINK_RATE(DPLL_CRTL1_LINK_RATE_1350, 0);
> >+                    break;
> >+            case DP_LINK_BW_5_4:
> >+                    ctrl1 |= 
> >DPLL_CRTL1_LINK_RATE(DPLL_CRTL1_LINK_RATE_2700, 0);
> >+                    break;
> >+            }
> >+
> >+            cfgcr1 = cfgcr2 = 0;
> >+    } else /* eDP */
> >+            return true;
> >
> >+    intel_crtc->config.dpll_hw_state.ctrl1 = ctrl1;
> >+    intel_crtc->config.dpll_hw_state.cfgcr1 = cfgcr1;
> >+    intel_crtc->config.dpll_hw_state.cfgcr2 = cfgcr2;
> 
> The return before updating the dpll_hw_state in the eDP case interacts with:
> 
> commit 4978cc93d9ac240b435ce60431aef24239b4c270
> Author: Ander Conselvan de Oliveira <ander.conselvan.de.olive...@intel.com>
> Date:   Tue Apr 21 17:13:21 2015 +0300
> 
>     drm/i915: Preserve shared DPLL information in new pipe_config
> 
> And results with a bunch of:
> 
> [drm:check_crtc_state [i915]] *ERROR* mismatch in dpll_hw_state.ctrl1
> (expected 0x00000000, found 0x00000001)
> 
> On boot and afterwards.
> 
> Not sure what is the fix so maybe someone knows?

I, since then, added the hint:

/*
 * Tries to find a *shared* PLL for the CRTC and store it in
 * intel_crtc->ddi_pll_sel.
 *
 * For private DPLLs, compute_config() should do the selection for us. This
 * function should be folded into compute_config() eventually.
 */

The mess is because of the shared vs private PLLs. For eDP 
skl_edp_set_pll_config()
in intel_dp.c should take care of computing the state.

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

Reply via email to