Re: [PATCH v2 19/23] drm/i915/icl: Reserve all required PLLs for TypeC ports

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@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