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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 18, 2019 at 08:25:52PM +0300, Ville Syrjälä wrote:
> 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.

Ok.

> 
> > +	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.

Yep, would be good to make it more generic, expressing it also better
that hw_state is pll's state.

I suppose that would be instead of the above

	struct intel_port_dpll {
		struct intel_shared_dpll *pll;
		struct intel_dpll_hw_state hw_state;
	} port_dplls[PORT_DPLL_COUNT];

	struct intel_port_dpll *active_dpll;

removing also intel_crtc_state::dpll_hw_state and shared_dpll.

I chickened out of doing that too now, affecting many platforms, but
would be worth doing as a follow-up.

> 
> > +	} 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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux