On Fri, Jun 07, 2019 at 08:41:29PM +0300, Imre Deak wrote: > When enabling a TypeC port we need to reserve all the required PLLs for > it, the TBT PLL for TBT-alt and the MG PHY PLL for DP-alt/legacy sinks. > We can select the proper PLL for the current port mode from the reserved > PLLs only once we selected and locked down the port mode for the whole > duration of the port's active state. Resetting and locking down the port > mode can in turn happen only during the modeset commit phase once we > disabled the given port and the PLL it used. > > To support the above reserve-and-select PLL semantic we store the > reserved PLLs along with their HW state in the CRTC state and provide a > way to select the active PLL from these. The selected PLL along with its > HW state will be pointed at by crtc_state->shared_dpll/dpll_hw_state as > in the case of other port types. > > Besides reserving all required PLLs no functional changes. > > v2: > - Fix releasing the ICL PLLs, not clearing the PLLs from the old > crtc_state. > > Cc: Ville Syrjälä <ville.syrj...@linux.intel.com> > Cc: Daniel Vetter <daniel.vet...@ffwll.ch> > Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com> > Signed-off-by: Imre Deak <imre.d...@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 11 +- > drivers/gpu/drm/i915/intel_dpll_mgr.c | 151 +++++++++++++++++++------- > drivers/gpu/drm/i915/intel_dpll_mgr.h | 9 ++ > drivers/gpu/drm/i915/intel_drv.h | 9 ++ > 4 files changed, 138 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 7381fb2e1240..006be3c3f1bd 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9880,6 +9880,7 @@ static void icelake_get_ddi_pll(struct drm_i915_private > *dev_priv, > enum port port, > struct intel_crtc_state *pipe_config) > { > + enum icl_port_dpll_id port_dpll_id; > enum intel_dpll_id id; > u32 temp; > > @@ -9887,22 +9888,28 @@ static void icelake_get_ddi_pll(struct > drm_i915_private *dev_priv, > temp = I915_READ(DPCLKA_CFGCR0_ICL) & > DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port); > id = temp >> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port); > + port_dpll_id = ICL_PORT_DPLL_DEFAULT; > } else if (intel_port_is_tc(dev_priv, port)) { > u32 clk_sel = I915_READ(DDI_CLK_SEL(port)) & DDI_CLK_SEL_MASK; > > if (clk_sel == DDI_CLK_SEL_MG) { > id = icl_tc_port_to_pll_id(intel_port_to_tc(dev_priv, > port)); > + port_dpll_id = ICL_PORT_DPLL_MG_PHY; > } else { > WARN_ON(clk_sel < DDI_CLK_SEL_TBT_162); > id = DPLL_ID_ICL_TBTPLL; > + port_dpll_id = ICL_PORT_DPLL_DEFAULT; > } > } else { > WARN(1, "Invalid port %x\n", port); > return; > } > > - pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id); > + pipe_config->icl_port_dplls[port_dpll_id].pll = > + intel_get_shared_dpll_by_id(dev_priv, id); > + > + icl_set_active_port_dpll(pipe_config, port_dpll_id); > } > > static void bxt_get_ddi_pll(struct drm_i915_private *dev_priv, > @@ -12041,6 +12048,8 @@ clear_intel_crtc_state(struct intel_crtc_state > *crtc_state) > saved_state->scaler_state = crtc_state->scaler_state; > saved_state->shared_dpll = crtc_state->shared_dpll; > saved_state->dpll_hw_state = crtc_state->dpll_hw_state; > + memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls, > + sizeof(saved_state->icl_port_dplls)); > saved_state->crc_enabled = crtc_state->crc_enabled; > if (IS_G4X(dev_priv) || > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c > b/drivers/gpu/drm/i915/intel_dpll_mgr.c > index 8ac293db43a5..17441d5f990e 100644 > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > @@ -2856,34 +2856,79 @@ static bool icl_calc_mg_pll_state(struct > intel_crtc_state *crtc_state, > return true; > } > > +/** > + * icl_set_active_port_dpll - select the active port DPLL for a given CRTC > + * @crtc_state: state for the CRTC to select the DPLL for > + * @port_dpll_id: the active @port_dpll_id to select > + * > + * Select the given @port_dpll_id instance from the DPLLs reserved for the > + * CRTC. > + */ > +void icl_set_active_port_dpll(struct intel_crtc_state *crtc_state, > + enum icl_port_dpll_id port_dpll_id) > +{ > + struct icl_port_dpll *port_dpll = > + &crtc_state->icl_port_dplls[port_dpll_id]; > + > + crtc_state->shared_dpll = port_dpll->pll; > + crtc_state->dpll_hw_state = port_dpll->hw_state; > +} > + > +static void icl_update_active_dpll(struct intel_atomic_state *state, > + struct intel_crtc *crtc, > + struct intel_encoder *encoder) > +{ > + struct intel_crtc_state *crtc_state = > + intel_atomic_get_new_crtc_state(state, crtc); > + struct intel_digital_port *primary_port; > + enum icl_port_dpll_id port_dpll_id; > + > + primary_port = encoder->type == INTEL_OUTPUT_DP_MST ? > + enc_to_mst(&encoder->base)->primary : > + enc_to_dig_port(&encoder->base); > + > + switch (primary_port->tc_mode) { > + case TC_PORT_TBT_ALT: > + port_dpll_id = ICL_PORT_DPLL_DEFAULT; > + break; > + case TC_PORT_DP_ALT: > + case TC_PORT_LEGACY: > + port_dpll_id = ICL_PORT_DPLL_MG_PHY; > + break; > + } > + > + icl_set_active_port_dpll(crtc_state, port_dpll_id); > +} > + > static bool icl_get_combo_phy_dpll(struct intel_atomic_state *state, > struct intel_crtc *crtc, > struct intel_encoder *encoder) > { > struct intel_crtc_state *crtc_state = > intel_atomic_get_new_crtc_state(state, crtc); > - struct intel_shared_dpll *pll; > + struct icl_port_dpll *port_dpll = > + &crtc_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT]; > > - if (!icl_calc_dpll_state(crtc_state, encoder, > - &crtc_state->dpll_hw_state)) { > + if (!icl_calc_dpll_state(crtc_state, encoder, &port_dpll->hw_state)) { > DRM_DEBUG_KMS("Could not calculate combo PHY PLL state.\n"); > > return false; > } > > - pll = intel_find_shared_dpll(state, crtc, &crtc_state->dpll_hw_state, > - DPLL_ID_ICL_DPLL0, > - DPLL_ID_ICL_DPLL1); > - if (!pll) { > + port_dpll->pll = intel_find_shared_dpll(state, crtc, > + &port_dpll->hw_state, > + DPLL_ID_ICL_DPLL0, > + DPLL_ID_ICL_DPLL1); > + if (!port_dpll->pll) { > DRM_DEBUG_KMS("No combo PHY PLL found for port %c\n", > port_name(encoder->port)); > return false; > } > > intel_reference_shared_dpll(state, crtc, > - pll, &crtc_state->dpll_hw_state); > + port_dpll->pll, &port_dpll->hw_state); > > - crtc_state->shared_dpll = pll; > + icl_update_active_dpll(state, crtc, encoder); > > return true; > } > @@ -2895,49 +2940,55 @@ static bool icl_get_tc_phy_dplls(struct > intel_atomic_state *state, > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > struct intel_crtc_state *crtc_state = > intel_atomic_get_new_crtc_state(state, crtc); > - enum tc_port tc_port = intel_port_to_tc(dev_priv, encoder->port); > - struct intel_digital_port *dig_port; > - struct intel_shared_dpll *pll; > - enum intel_dpll_id min, max; > - bool ret; > - > - if (encoder->type == INTEL_OUTPUT_DP_MST) > - dig_port = enc_to_mst(&encoder->base)->primary; > - else > - dig_port = enc_to_dig_port(&encoder->base); > + struct icl_port_dpll *port_dpll = > + &crtc_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];
I would move that initialization next to the code so that the tbt vs. mg cases look more like twins. > + enum intel_dpll_id dpll_id; > > - if (dig_port->tc_mode == TC_PORT_TBT_ALT) { > - min = DPLL_ID_ICL_TBTPLL; > - max = min; > - ret = icl_calc_dpll_state(crtc_state, encoder, > - &crtc_state->dpll_hw_state); > - } else { > - min = icl_tc_port_to_pll_id(tc_port); > - max = min; > - ret = icl_calc_mg_pll_state(crtc_state, > - &crtc_state->dpll_hw_state); > + if (!icl_calc_dpll_state(crtc_state, encoder, &port_dpll->hw_state)) { > + DRM_DEBUG_KMS("Could not calculate TBT PLL state.\n"); > + return false; > } > > - if (!ret) { > - DRM_DEBUG_KMS("Could not calculate PLL state.\n"); > + port_dpll->pll = intel_find_shared_dpll(state, crtc, > + &port_dpll->hw_state, > + DPLL_ID_ICL_TBTPLL, > + DPLL_ID_ICL_TBTPLL); > + if (!port_dpll->pll) { > + DRM_DEBUG_KMS("No TBT-ALT PLL found\n"); > return false; > } > + intel_reference_shared_dpll(state, crtc, > + port_dpll->pll, &port_dpll->hw_state); > > > - pll = intel_find_shared_dpll(state, crtc, > - &crtc_state->dpll_hw_state, > - min, max); > - if (!pll) { > - DRM_DEBUG_KMS("No PLL selected\n"); > - return false; > + port_dpll = &crtc_state->icl_port_dplls[ICL_PORT_DPLL_MG_PHY]; > + if (!icl_calc_mg_pll_state(crtc_state, &port_dpll->hw_state)) { > + DRM_DEBUG_KMS("Could not calculate MG PHY PLL state.\n"); > + goto err_unreference_tbt_pll; > } > > + dpll_id = icl_tc_port_to_pll_id(intel_port_to_tc(dev_priv, > + encoder->port)); > + port_dpll->pll = intel_find_shared_dpll(state, crtc, > + &port_dpll->hw_state, > + dpll_id, > + dpll_id); > + if (!port_dpll->pll) { > + DRM_DEBUG_KMS("No MG PHY PLL found\n"); > + goto err_unreference_tbt_pll; > + } > intel_reference_shared_dpll(state, crtc, > - pll, &crtc_state->dpll_hw_state); > + port_dpll->pll, &port_dpll->hw_state); > > - crtc_state->shared_dpll = pll; > + icl_update_active_dpll(state, crtc, encoder); > > return true; > + > +err_unreference_tbt_pll: > + port_dpll = &crtc_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT]; > + intel_unreference_shared_dpll(state, crtc, port_dpll->pll); > + > + return false; > } > > static bool icl_get_dplls(struct intel_atomic_state *state, > @@ -2957,6 +3008,24 @@ static bool icl_get_dplls(struct intel_atomic_state > *state, > return false; > } > > +static void icl_put_dplls(struct intel_atomic_state *state, > + struct intel_crtc *crtc) > +{ > + struct intel_crtc_state *crtc_state = > + intel_atomic_get_old_crtc_state(state, crtc); > + enum icl_port_dpll_id id; > + > + for (id = ICL_PORT_DPLL_DEFAULT; id < ICL_PORT_DPLL_COUNT; id++) { > + struct icl_port_dpll *port_dpll = > + &crtc_state->icl_port_dplls[id]; > + > + if (!port_dpll->pll) > + continue; > + > + intel_unreference_shared_dpll(state, crtc, port_dpll->pll); > + } > +} > + > static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv, > struct intel_shared_dpll *pll, > struct intel_dpll_hw_state *hw_state) > @@ -3330,7 +3399,7 @@ static const struct dpll_info icl_plls[] = { > static const struct intel_dpll_mgr icl_pll_mgr = { > .dpll_info = icl_plls, > .get_dplls = icl_get_dplls, > - .put_dplls = intel_put_dpll, > + .put_dplls = icl_put_dplls, > .dump_hw_state = icl_dump_hw_state, > }; > > @@ -3343,7 +3412,7 @@ static const struct dpll_info ehl_plls[] = { > static const struct intel_dpll_mgr ehl_pll_mgr = { > .dpll_info = ehl_plls, > .get_dplls = icl_get_dplls, > - .put_dplls = intel_put_dpll, > + .put_dplls = icl_put_dplls, > .dump_hw_state = icl_dump_hw_state, > }; > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h > b/drivers/gpu/drm/i915/intel_dpll_mgr.h > index 6ffdcc06ad23..3bea81bde343 100644 > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h > @@ -141,6 +141,13 @@ enum intel_dpll_id { > }; > #define I915_NUM_PLLS 7 > > +enum icl_port_dpll_id { > + ICL_PORT_DPLL_DEFAULT, > + ICL_PORT_DPLL_MG_PHY, > + > + ICL_PORT_DPLL_COUNT, > +}; > + > struct intel_dpll_hw_state { > /* i9xx, pch plls */ > u32 dpll; > @@ -337,6 +344,8 @@ bool intel_reserve_shared_dplls(struct intel_atomic_state > *state, > struct intel_encoder *encoder); > void intel_release_shared_dplls(struct intel_atomic_state *state, > struct intel_crtc *crtc); > +void icl_set_active_port_dpll(struct intel_crtc_state *crtc_state, > + enum icl_port_dpll_id port_dpll_id); > void intel_prepare_shared_dpll(const struct intel_crtc_state *crtc_state); > void intel_enable_shared_dpll(const struct intel_crtc_state *crtc_state); > void intel_disable_shared_dpll(const struct intel_crtc_state *crtc_state); > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index f9b6f63ebbfe..7f1dcb8d8a28 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -812,6 +812,15 @@ struct intel_crtc_state { > /* Actual register state of the dpll, for shared dpll cross-checking. */ > struct intel_dpll_hw_state dpll_hw_state; > > + /* > + * ICL reserved DPLLs for the CRTC/port. The active PLL is selected by > + * setting shared_dpll and dpll_hw_state to one of these reserved ones. > + */ > + struct icl_port_dpll { > + struct intel_shared_dpll *pll; > + struct intel_dpll_hw_state hw_state; I was pondering if we should use this array on non-icl too. Would avoid having three instances of this state embedded in our crtc state. But I don't know how big the struct is so not sure if it's worth it. Anyways, something to think about for a future optimization. > + } icl_port_dplls[ICL_PORT_DPLL_COUNT]; > + > /* DSI PLL registers */ > struct { > u32 ctrl, div; > -- > 2.17.1 -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx