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.syrjala@xxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
