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
