| Message ID | 20190620140600.11357-17-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix TypeC port mode switching | expand |
On Thu, Jun 20, 2019 at 05:05:53PM +0300, Imre Deak wrote: > For consistency s/intel_get_shared_dpll()/intel_reserve_shared_dplls()/ > to better match intel_release_shared_dplls(). Also, pass to the > reserve/release and get_dplls/put_dplls hooks the intel_atomic_state and > CRTC object, that way these functions can look up the old or new state > as needed. > > Also release the PLLs from the atomic state via a new > put_dplls->intel_unreference_shared_dpll() call chain for better > symmetry with the reservation via the > get_dplls->intel_reference_shared_dpll() call chain. > > Since nothing uses the PLL returned by intel_reserve_shared_dplls(), > make it return only a bool. > > While at it also clarify the reserve/release function docbook headers > making it clear that multiple DPLLs will be reserved/released and > whether the new or old atomic CRTC state is affected. > > This refactoring is also a preparation for a follow-up change that needs > to reserve multiple DPLLs. > > Kudos to Ville for the idea to pass intel_atomic_state around, to make > things clearer locally where an object's old/new atomic state is > required. > > No functional changes. > > v2: > - Fix checkpatch issue: typo in code comment. > > 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/display/intel_display.c | 19 +- > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 221 +++++++++++------- > drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 13 +- > 3 files changed, 153 insertions(+), 100 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 468ca6d84bd8..688137524179 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -9504,6 +9504,8 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, > struct intel_crtc_state *crtc_state) > { > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + struct intel_atomic_state *state = > + to_intel_atomic_state(crtc_state->base.state); > const struct intel_limit *limit; > int refclk = 120000; > > @@ -9545,7 +9547,7 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, > > ironlake_compute_dpll(crtc, crtc_state, NULL); > > - if (!intel_get_shared_dpll(crtc_state, NULL)) { > + if (!intel_reserve_shared_dplls(state, crtc, NULL)) { > DRM_DEBUG_KMS("failed to find PLL for pipe %c\n", > pipe_name(crtc->pipe)); > return -EINVAL; > @@ -9926,7 +9928,7 @@ static int haswell_crtc_compute_clock(struct intel_crtc *crtc, > struct intel_encoder *encoder = > intel_get_crtc_new_encoder(state, crtc_state); > > - if (!intel_get_shared_dpll(crtc_state, encoder)) { > + if (!intel_reserve_shared_dplls(state, crtc, encoder)) { > DRM_DEBUG_KMS("failed to find PLL for pipe %c\n", > pipe_name(crtc->pipe)); > return -EINVAL; > @@ -13195,27 +13197,20 @@ static void update_scanline_offset(const struct intel_crtc_state *crtc_state) > static void intel_modeset_clear_plls(struct intel_atomic_state *state) > { > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > - struct intel_crtc_state *old_crtc_state, *new_crtc_state; > + struct intel_crtc_state *new_crtc_state; > struct intel_crtc *crtc; > int i; > > if (!dev_priv->display.crtc_compute_clock) > return; > > - for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > - new_crtc_state, i) { > - struct intel_shared_dpll *old_dpll = > - old_crtc_state->shared_dpll; > - > + for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { > if (!needs_modeset(&new_crtc_state->base)) > continue; > > new_crtc_state->shared_dpll = NULL; > > - if (!old_dpll) > - continue; > - > - intel_release_shared_dpll(old_dpll, crtc, &state->base); > + intel_release_shared_dplls(state, crtc); > } > } > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > index bf66261c8bf0..3fbc975851fa 100644 > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > @@ -36,9 +36,10 @@ > * This file provides an abstraction over display PLLs. The function > * intel_shared_dpll_init() initializes the PLLs for the given platform. The > * users of a PLL are tracked and that tracking is integrated with the atomic > - * modest interface. During an atomic operation, a PLL can be requested for a > - * given CRTC and encoder configuration by calling intel_get_shared_dpll() and > - * a previously used PLL can be released with intel_release_shared_dpll(). > + * modset interface. During an atomic operation, required PLLs can be reserved > + * for a given CRTC and encoder configuration by calling > + * intel_reserve_shared_dplls() and previously reserved PLLs can be released > + * with intel_release_shared_dplls(). > * Changes to the users are first staged in the atomic state, and then made > * effective by calling intel_shared_dpll_swap_state() during the atomic > * commit phase. > @@ -309,6 +310,28 @@ intel_reference_shared_dpll(struct intel_shared_dpll *pll, > shared_dpll[id].crtc_mask |= 1 << crtc->pipe; > } > > +static void intel_unreference_shared_dpll(struct intel_atomic_state *state, > + const struct intel_crtc *crtc, > + const struct intel_shared_dpll *pll) > +{ > + struct intel_shared_dpll_state *shared_dpll; > + > + shared_dpll = intel_atomic_get_shared_dpll_state(&state->base); > + shared_dpll[pll->info->id].crtc_mask &= ~(1 << crtc->pipe); > +} > + > +static void intel_put_dpll(struct intel_atomic_state *state, > + struct intel_crtc *crtc) > +{ > + struct intel_crtc_state *crtc_state = > + intel_atomic_get_old_crtc_state(state, crtc); I'm wondering a bit why we're using the old crtc state here. We duplicated the state so shouldn't the new crtc state have the same dpll still at this point? Doesn't really matter I suppose. Not even sure which state would be more correct here. Anyways, the patch seems OK. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + > + if (!crtc_state->shared_dpll) > + return; > + > + intel_unreference_shared_dpll(state, crtc, crtc_state->shared_dpll); > +} > + > /** > * intel_shared_dpll_swap_state - make atomic DPLL configuration effective > * @state: atomic state > @@ -421,11 +444,12 @@ static void ibx_pch_dpll_disable(struct drm_i915_private *dev_priv, > udelay(200); > } > > -static struct intel_shared_dpll * > -ibx_get_dpll(struct intel_crtc_state *crtc_state, > - struct intel_encoder *encoder) > +static bool ibx_get_dpll(struct intel_atomic_state *state, > + struct intel_crtc *crtc, > + struct intel_encoder *encoder) > { > - struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > + struct intel_crtc_state *crtc_state = > + intel_atomic_get_new_crtc_state(state, crtc); > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > struct intel_shared_dpll *pll; > enum intel_dpll_id i; > @@ -445,12 +469,12 @@ ibx_get_dpll(struct intel_crtc_state *crtc_state, > } > > if (!pll) > - return NULL; > + return false; > > /* reference the pll */ > intel_reference_shared_dpll(pll, crtc_state); > > - return pll; > + return true; > } > > static void ibx_dump_hw_state(struct drm_i915_private *dev_priv, > @@ -821,10 +845,12 @@ hsw_ddi_dp_get_dpll(struct intel_crtc_state *crtc_state) > return pll; > } > > -static struct intel_shared_dpll * > -hsw_get_dpll(struct intel_crtc_state *crtc_state, > - struct intel_encoder *encoder) > +static bool hsw_get_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; > > memset(&crtc_state->dpll_hw_state, 0, > @@ -836,7 +862,7 @@ hsw_get_dpll(struct intel_crtc_state *crtc_state, > pll = hsw_ddi_dp_get_dpll(crtc_state); > } else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_ANALOG)) { > if (WARN_ON(crtc_state->port_clock / 2 != 135000)) > - return NULL; > + return false; > > crtc_state->dpll_hw_state.spll = > SPLL_PLL_ENABLE | SPLL_FREQ_1350MHz | SPLL_REF_MUXED_SSC; > @@ -844,15 +870,15 @@ hsw_get_dpll(struct intel_crtc_state *crtc_state, > pll = intel_find_shared_dpll(crtc_state, > DPLL_ID_SPLL, DPLL_ID_SPLL); > } else { > - return NULL; > + return false; > } > > if (!pll) > - return NULL; > + return false; > > intel_reference_shared_dpll(pll, crtc_state); > > - return pll; > + return true; > } > > static void hsw_dump_hw_state(struct drm_i915_private *dev_priv, > @@ -1385,10 +1411,12 @@ skl_ddi_dp_set_dpll_hw_state(struct intel_crtc_state *crtc_state) > return true; > } > > -static struct intel_shared_dpll * > -skl_get_dpll(struct intel_crtc_state *crtc_state, > - struct intel_encoder *encoder) > +static bool skl_get_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; > bool bret; > > @@ -1396,16 +1424,16 @@ skl_get_dpll(struct intel_crtc_state *crtc_state, > bret = skl_ddi_hdmi_pll_dividers(crtc_state); > if (!bret) { > DRM_DEBUG_KMS("Could not get HDMI pll dividers.\n"); > - return NULL; > + return false; > } > } else if (intel_crtc_has_dp_encoder(crtc_state)) { > bret = skl_ddi_dp_set_dpll_hw_state(crtc_state); > if (!bret) { > DRM_DEBUG_KMS("Could not set DP dpll HW state.\n"); > - return NULL; > + return false; > } > } else { > - return NULL; > + return false; > } > > if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP)) > @@ -1417,11 +1445,11 @@ skl_get_dpll(struct intel_crtc_state *crtc_state, > DPLL_ID_SKL_DPLL1, > DPLL_ID_SKL_DPLL3); > if (!pll) > - return NULL; > + return false; > > intel_reference_shared_dpll(pll, crtc_state); > > - return pll; > + return true; > } > > static void skl_dump_hw_state(struct drm_i915_private *dev_priv, > @@ -1827,22 +1855,23 @@ bxt_ddi_hdmi_set_dpll_hw_state(struct intel_crtc_state *crtc_state) > return bxt_ddi_set_dpll_hw_state(crtc_state, &clk_div); > } > > -static struct intel_shared_dpll * > -bxt_get_dpll(struct intel_crtc_state *crtc_state, > - struct intel_encoder *encoder) > +static bool bxt_get_dpll(struct intel_atomic_state *state, > + struct intel_crtc *crtc, > + struct intel_encoder *encoder) > { > - struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > + struct intel_crtc_state *crtc_state = > + intel_atomic_get_new_crtc_state(state, crtc); > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > struct intel_shared_dpll *pll; > enum intel_dpll_id id; > > if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI) && > !bxt_ddi_hdmi_set_dpll_hw_state(crtc_state)) > - return NULL; > + return false; > > if (intel_crtc_has_dp_encoder(crtc_state) && > !bxt_ddi_dp_set_dpll_hw_state(crtc_state)) > - return NULL; > + return false; > > /* 1:1 mapping between ports and PLLs */ > id = (enum intel_dpll_id) encoder->port; > @@ -1853,7 +1882,7 @@ bxt_get_dpll(struct intel_crtc_state *crtc_state, > > intel_reference_shared_dpll(pll, crtc_state); > > - return pll; > + return true; > } > > static void bxt_dump_hw_state(struct drm_i915_private *dev_priv, > @@ -1884,8 +1913,11 @@ static const struct intel_shared_dpll_funcs bxt_ddi_pll_funcs = { > struct intel_dpll_mgr { > const struct dpll_info *dpll_info; > > - struct intel_shared_dpll *(*get_dpll)(struct intel_crtc_state *crtc_state, > - struct intel_encoder *encoder); > + bool (*get_dplls)(struct intel_atomic_state *state, > + struct intel_crtc *crtc, > + struct intel_encoder *encoder); > + void (*put_dplls)(struct intel_atomic_state *state, > + struct intel_crtc *crtc); > > void (*dump_hw_state)(struct drm_i915_private *dev_priv, > const struct intel_dpll_hw_state *hw_state); > @@ -1899,7 +1931,8 @@ static const struct dpll_info pch_plls[] = { > > static const struct intel_dpll_mgr pch_pll_mgr = { > .dpll_info = pch_plls, > - .get_dpll = ibx_get_dpll, > + .get_dplls = ibx_get_dpll, > + .put_dplls = intel_put_dpll, > .dump_hw_state = ibx_dump_hw_state, > }; > > @@ -1915,7 +1948,8 @@ static const struct dpll_info hsw_plls[] = { > > static const struct intel_dpll_mgr hsw_pll_mgr = { > .dpll_info = hsw_plls, > - .get_dpll = hsw_get_dpll, > + .get_dplls = hsw_get_dpll, > + .put_dplls = intel_put_dpll, > .dump_hw_state = hsw_dump_hw_state, > }; > > @@ -1929,7 +1963,8 @@ static const struct dpll_info skl_plls[] = { > > static const struct intel_dpll_mgr skl_pll_mgr = { > .dpll_info = skl_plls, > - .get_dpll = skl_get_dpll, > + .get_dplls = skl_get_dpll, > + .put_dplls = intel_put_dpll, > .dump_hw_state = skl_dump_hw_state, > }; > > @@ -1942,7 +1977,8 @@ static const struct dpll_info bxt_plls[] = { > > static const struct intel_dpll_mgr bxt_pll_mgr = { > .dpll_info = bxt_plls, > - .get_dpll = bxt_get_dpll, > + .get_dplls = bxt_get_dpll, > + .put_dplls = intel_put_dpll, > .dump_hw_state = bxt_dump_hw_state, > }; > > @@ -2332,10 +2368,12 @@ cnl_ddi_dp_set_dpll_hw_state(struct intel_crtc_state *crtc_state) > return true; > } > > -static struct intel_shared_dpll * > -cnl_get_dpll(struct intel_crtc_state *crtc_state, > - struct intel_encoder *encoder) > +static bool cnl_get_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; > bool bret; > > @@ -2343,18 +2381,18 @@ cnl_get_dpll(struct intel_crtc_state *crtc_state, > bret = cnl_ddi_hdmi_pll_dividers(crtc_state); > if (!bret) { > DRM_DEBUG_KMS("Could not get HDMI pll dividers.\n"); > - return NULL; > + return false; > } > } else if (intel_crtc_has_dp_encoder(crtc_state)) { > bret = cnl_ddi_dp_set_dpll_hw_state(crtc_state); > if (!bret) { > DRM_DEBUG_KMS("Could not set DP dpll HW state.\n"); > - return NULL; > + return false; > } > } else { > DRM_DEBUG_KMS("Skip DPLL setup for output_types 0x%x\n", > crtc_state->output_types); > - return NULL; > + return false; > } > > pll = intel_find_shared_dpll(crtc_state, > @@ -2362,12 +2400,12 @@ cnl_get_dpll(struct intel_crtc_state *crtc_state, > DPLL_ID_SKL_DPLL2); > if (!pll) { > DRM_DEBUG_KMS("No PLL selected\n"); > - return NULL; > + return false; > } > > intel_reference_shared_dpll(pll, crtc_state); > > - return pll; > + return true; > } > > static void cnl_dump_hw_state(struct drm_i915_private *dev_priv, > @@ -2394,7 +2432,8 @@ static const struct dpll_info cnl_plls[] = { > > static const struct intel_dpll_mgr cnl_pll_mgr = { > .dpll_info = cnl_plls, > - .get_dpll = cnl_get_dpll, > + .get_dplls = cnl_get_dpll, > + .put_dplls = intel_put_dpll, > .dump_hw_state = cnl_dump_hw_state, > }; > > @@ -2792,11 +2831,13 @@ static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state) > return true; > } > > -static struct intel_shared_dpll * > -icl_get_dpll(struct intel_crtc_state *crtc_state, > - struct intel_encoder *encoder) > +static bool icl_get_dplls(struct intel_atomic_state *state, > + struct intel_crtc *crtc, > + struct intel_encoder *encoder) > { > - struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > + 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); > struct intel_digital_port *intel_dig_port; > struct intel_shared_dpll *pll; > enum port port = encoder->port; > @@ -2831,24 +2872,24 @@ icl_get_dpll(struct intel_crtc_state *crtc_state, > } > } else { > MISSING_CASE(port); > - return NULL; > + return false; > } > > if (!ret) { > DRM_DEBUG_KMS("Could not calculate PLL state.\n"); > - return NULL; > + return false; > } > > > pll = intel_find_shared_dpll(crtc_state, min, max); > if (!pll) { > DRM_DEBUG_KMS("No PLL selected\n"); > - return NULL; > + return false; > } > > intel_reference_shared_dpll(pll, crtc_state); > > - return pll; > + return true; > } > > static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv, > @@ -3223,7 +3264,8 @@ static const struct dpll_info icl_plls[] = { > > static const struct intel_dpll_mgr icl_pll_mgr = { > .dpll_info = icl_plls, > - .get_dpll = icl_get_dpll, > + .get_dplls = icl_get_dplls, > + .put_dplls = intel_put_dpll, > .dump_hw_state = icl_dump_hw_state, > }; > > @@ -3235,7 +3277,8 @@ static const struct dpll_info ehl_plls[] = { > > static const struct intel_dpll_mgr ehl_pll_mgr = { > .dpll_info = ehl_plls, > - .get_dpll = icl_get_dpll, > + .get_dplls = icl_get_dplls, > + .put_dplls = intel_put_dpll, > .dump_hw_state = icl_dump_hw_state, > }; > > @@ -3287,50 +3330,64 @@ void intel_shared_dpll_init(struct drm_device *dev) > } > > /** > - * intel_get_shared_dpll - get a shared DPLL for CRTC and encoder combination > - * @crtc_state: atomic state for the crtc > + * intel_reserve_shared_dplls - reserve DPLLs for CRTC and encoder combination > + * @state: atomic state > + * @crtc: CRTC to reserve DPLLs for > * @encoder: encoder > * > - * Find an appropriate DPLL for the given CRTC and encoder combination. A > - * reference from the @crtc_state to the returned pll is registered in the > - * atomic state. That configuration is made effective by calling > - * intel_shared_dpll_swap_state(). The reference should be released by calling > - * intel_release_shared_dpll(). > + * This function reserves all required DPLLs for the given CRTC and encoder > + * combination in the current atomic commit @state and the new @crtc atomic > + * state. > + * > + * The new configuration in the atomic commit @state is made effective by > + * calling intel_shared_dpll_swap_state(). > + * > + * The reserved DPLLs should be released by calling > + * intel_release_shared_dplls(). > * > * Returns: > - * A shared DPLL to be used by @crtc_state and @encoder. > + * True if all required DPLLs were successfully reserved. > */ > -struct intel_shared_dpll * > -intel_get_shared_dpll(struct intel_crtc_state *crtc_state, > - struct intel_encoder *encoder) > +bool intel_reserve_shared_dplls(struct intel_atomic_state *state, > + struct intel_crtc *crtc, > + struct intel_encoder *encoder) > { > - struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr; > > if (WARN_ON(!dpll_mgr)) > - return NULL; > + return false; > > - return dpll_mgr->get_dpll(crtc_state, encoder); > + return dpll_mgr->get_dplls(state, crtc, encoder); > } > > /** > - * intel_release_shared_dpll - end use of DPLL by CRTC in atomic state > - * @dpll: dpll in use by @crtc > - * @crtc: crtc > + * intel_release_shared_dplls - end use of DPLLs by CRTC in atomic state > * @state: atomic state > + * @crtc: crtc from which the DPLLs are to be released > * > - * This function releases the reference from @crtc to @dpll from the > - * atomic @state. The new configuration is made effective by calling > - * intel_shared_dpll_swap_state(). > + * This function releases all DPLLs reserved by intel_reserve_shared_dplls() > + * from the current atomic commit @state and the old @crtc atomic state. > + * > + * The new configuration in the atomic commit @state is made effective by > + * calling intel_shared_dpll_swap_state(). > */ > -void intel_release_shared_dpll(struct intel_shared_dpll *dpll, > - struct intel_crtc *crtc, > - struct drm_atomic_state *state) > +void intel_release_shared_dplls(struct intel_atomic_state *state, > + struct intel_crtc *crtc) > { > - struct intel_shared_dpll_state *shared_dpll_state; > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr; > + > + /* > + * FIXME: this function is called for every platform having a > + * compute_clock hook, even though the platform doesn't yet support > + * the shared DPLL framework and intel_reserve_shared_dplls() is not > + * called on those. > + */ > + if (!dpll_mgr) > + return; > > - shared_dpll_state = intel_atomic_get_shared_dpll_state(state); > - shared_dpll_state[dpll->info->id].crtc_mask &= ~(1 << crtc->pipe); > + dpll_mgr->put_dplls(state, crtc); > } > > /** > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > index d0570414f3d1..16ddab138574 100644 > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > @@ -39,6 +39,7 @@ > struct drm_atomic_state; > struct drm_device; > struct drm_i915_private; > +struct intel_atomic_state; > struct intel_crtc; > struct intel_crtc_state; > struct intel_encoder; > @@ -195,7 +196,7 @@ struct intel_dpll_hw_state { > * future state which would be applied by an atomic mode set (stored in > * a struct &intel_atomic_state). > * > - * See also intel_get_shared_dpll() and intel_release_shared_dpll(). > + * See also intel_reserve_shared_dplls() and intel_release_shared_dplls(). > */ > struct intel_shared_dpll_state { > /** > @@ -331,11 +332,11 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv, > bool state); > #define assert_shared_dpll_enabled(d, p) assert_shared_dpll(d, p, true) > #define assert_shared_dpll_disabled(d, p) assert_shared_dpll(d, p, false) > -struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc_state *state, > - struct intel_encoder *encoder); > -void intel_release_shared_dpll(struct intel_shared_dpll *dpll, > - struct intel_crtc *crtc, > - struct drm_atomic_state *state); > +bool intel_reserve_shared_dplls(struct intel_atomic_state *state, > + struct intel_crtc *crtc, > + struct intel_encoder *encoder); > +void intel_release_shared_dplls(struct intel_atomic_state *state, > + struct intel_crtc *crtc); > 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); > -- > 2.17.1
On Tue, Jun 25, 2019 at 04:53:01PM +0300, Ville Syrjälä wrote: > On Thu, Jun 20, 2019 at 05:05:53PM +0300, Imre Deak wrote: > > For consistency s/intel_get_shared_dpll()/intel_reserve_shared_dplls()/ > > to better match intel_release_shared_dplls(). Also, pass to the > > reserve/release and get_dplls/put_dplls hooks the intel_atomic_state and > > CRTC object, that way these functions can look up the old or new state > > as needed. > > > > Also release the PLLs from the atomic state via a new > > put_dplls->intel_unreference_shared_dpll() call chain for better > > symmetry with the reservation via the > > get_dplls->intel_reference_shared_dpll() call chain. > > > > Since nothing uses the PLL returned by intel_reserve_shared_dplls(), > > make it return only a bool. > > > > While at it also clarify the reserve/release function docbook headers > > making it clear that multiple DPLLs will be reserved/released and > > whether the new or old atomic CRTC state is affected. > > > > This refactoring is also a preparation for a follow-up change that needs > > to reserve multiple DPLLs. > > > > Kudos to Ville for the idea to pass intel_atomic_state around, to make > > things clearer locally where an object's old/new atomic state is > > required. > > > > No functional changes. > > > > v2: > > - Fix checkpatch issue: typo in code comment. > > > > 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/display/intel_display.c | 19 +- > > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 221 +++++++++++------- > > drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 13 +- > > 3 files changed, 153 insertions(+), 100 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index 468ca6d84bd8..688137524179 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -9504,6 +9504,8 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, > > struct intel_crtc_state *crtc_state) > > { > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > + struct intel_atomic_state *state = > > + to_intel_atomic_state(crtc_state->base.state); > > const struct intel_limit *limit; > > int refclk = 120000; > > > > @@ -9545,7 +9547,7 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, > > > > ironlake_compute_dpll(crtc, crtc_state, NULL); > > > > - if (!intel_get_shared_dpll(crtc_state, NULL)) { > > + if (!intel_reserve_shared_dplls(state, crtc, NULL)) { > > DRM_DEBUG_KMS("failed to find PLL for pipe %c\n", > > pipe_name(crtc->pipe)); > > return -EINVAL; > > @@ -9926,7 +9928,7 @@ static int haswell_crtc_compute_clock(struct intel_crtc *crtc, > > struct intel_encoder *encoder = > > intel_get_crtc_new_encoder(state, crtc_state); > > > > - if (!intel_get_shared_dpll(crtc_state, encoder)) { > > + if (!intel_reserve_shared_dplls(state, crtc, encoder)) { > > DRM_DEBUG_KMS("failed to find PLL for pipe %c\n", > > pipe_name(crtc->pipe)); > > return -EINVAL; > > @@ -13195,27 +13197,20 @@ static void update_scanline_offset(const struct intel_crtc_state *crtc_state) > > static void intel_modeset_clear_plls(struct intel_atomic_state *state) > > { > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > - struct intel_crtc_state *old_crtc_state, *new_crtc_state; > > + struct intel_crtc_state *new_crtc_state; > > struct intel_crtc *crtc; > > int i; > > > > if (!dev_priv->display.crtc_compute_clock) > > return; > > > > - for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > - new_crtc_state, i) { > > - struct intel_shared_dpll *old_dpll = > > - old_crtc_state->shared_dpll; > > - > > + for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { > > if (!needs_modeset(&new_crtc_state->base)) > > continue; > > > > new_crtc_state->shared_dpll = NULL; > > > > - if (!old_dpll) > > - continue; > > - > > - intel_release_shared_dpll(old_dpll, crtc, &state->base); > > + intel_release_shared_dplls(state, crtc); > > } > > } > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > index bf66261c8bf0..3fbc975851fa 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > @@ -36,9 +36,10 @@ > > * This file provides an abstraction over display PLLs. The function > > * intel_shared_dpll_init() initializes the PLLs for the given platform. The > > * users of a PLL are tracked and that tracking is integrated with the atomic > > - * modest interface. During an atomic operation, a PLL can be requested for a > > - * given CRTC and encoder configuration by calling intel_get_shared_dpll() and > > - * a previously used PLL can be released with intel_release_shared_dpll(). > > + * modset interface. During an atomic operation, required PLLs can be reserved > > + * for a given CRTC and encoder configuration by calling > > + * intel_reserve_shared_dplls() and previously reserved PLLs can be released > > + * with intel_release_shared_dplls(). > > * Changes to the users are first staged in the atomic state, and then made > > * effective by calling intel_shared_dpll_swap_state() during the atomic > > * commit phase. > > @@ -309,6 +310,28 @@ intel_reference_shared_dpll(struct intel_shared_dpll *pll, > > shared_dpll[id].crtc_mask |= 1 << crtc->pipe; > > } > > > > +static void intel_unreference_shared_dpll(struct intel_atomic_state *state, > > + const struct intel_crtc *crtc, > > + const struct intel_shared_dpll *pll) > > +{ > > + struct intel_shared_dpll_state *shared_dpll; > > + > > + shared_dpll = intel_atomic_get_shared_dpll_state(&state->base); > > + shared_dpll[pll->info->id].crtc_mask &= ~(1 << crtc->pipe); > > +} > > + > > +static void intel_put_dpll(struct intel_atomic_state *state, > > + struct intel_crtc *crtc) > > +{ > > + struct intel_crtc_state *crtc_state = > > + intel_atomic_get_old_crtc_state(state, crtc); > > I'm wondering a bit why we're using the old crtc state here. We > duplicated the state so shouldn't the new crtc state have the > same dpll still at this point? > > Doesn't really matter I suppose. Not even sure which state would > be more correct here. You are right, the new crtc state would be the better choice here. The two states are equal here yes, but in the upcoming icl_put_dplls() we should clear the new crtc_state->icl_port_dplls. Not clearing them happens to not cause a problem, since we reallocate all PLLs in icl_get_dplls(), but it's better to do the clearing anyway for consistency. I'll change that. > > Anyways, the patch seems OK. > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > + > > + if (!crtc_state->shared_dpll) > > + return; > > + > > + intel_unreference_shared_dpll(state, crtc, crtc_state->shared_dpll); > > +} > > + > > /** > > * intel_shared_dpll_swap_state - make atomic DPLL configuration effective > > * @state: atomic state > > @@ -421,11 +444,12 @@ static void ibx_pch_dpll_disable(struct drm_i915_private *dev_priv, > > udelay(200); > > } > > > > -static struct intel_shared_dpll * > > -ibx_get_dpll(struct intel_crtc_state *crtc_state, > > - struct intel_encoder *encoder) > > +static bool ibx_get_dpll(struct intel_atomic_state *state, > > + struct intel_crtc *crtc, > > + struct intel_encoder *encoder) > > { > > - struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > > + struct intel_crtc_state *crtc_state = > > + intel_atomic_get_new_crtc_state(state, crtc); > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > struct intel_shared_dpll *pll; > > enum intel_dpll_id i; > > @@ -445,12 +469,12 @@ ibx_get_dpll(struct intel_crtc_state *crtc_state, > > } > > > > if (!pll) > > - return NULL; > > + return false; > > > > /* reference the pll */ > > intel_reference_shared_dpll(pll, crtc_state); > > > > - return pll; > > + return true; > > } > > > > static void ibx_dump_hw_state(struct drm_i915_private *dev_priv, > > @@ -821,10 +845,12 @@ hsw_ddi_dp_get_dpll(struct intel_crtc_state *crtc_state) > > return pll; > > } > > > > -static struct intel_shared_dpll * > > -hsw_get_dpll(struct intel_crtc_state *crtc_state, > > - struct intel_encoder *encoder) > > +static bool hsw_get_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; > > > > memset(&crtc_state->dpll_hw_state, 0, > > @@ -836,7 +862,7 @@ hsw_get_dpll(struct intel_crtc_state *crtc_state, > > pll = hsw_ddi_dp_get_dpll(crtc_state); > > } else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_ANALOG)) { > > if (WARN_ON(crtc_state->port_clock / 2 != 135000)) > > - return NULL; > > + return false; > > > > crtc_state->dpll_hw_state.spll = > > SPLL_PLL_ENABLE | SPLL_FREQ_1350MHz | SPLL_REF_MUXED_SSC; > > @@ -844,15 +870,15 @@ hsw_get_dpll(struct intel_crtc_state *crtc_state, > > pll = intel_find_shared_dpll(crtc_state, > > DPLL_ID_SPLL, DPLL_ID_SPLL); > > } else { > > - return NULL; > > + return false; > > } > > > > if (!pll) > > - return NULL; > > + return false; > > > > intel_reference_shared_dpll(pll, crtc_state); > > > > - return pll; > > + return true; > > } > > > > static void hsw_dump_hw_state(struct drm_i915_private *dev_priv, > > @@ -1385,10 +1411,12 @@ skl_ddi_dp_set_dpll_hw_state(struct intel_crtc_state *crtc_state) > > return true; > > } > > > > -static struct intel_shared_dpll * > > -skl_get_dpll(struct intel_crtc_state *crtc_state, > > - struct intel_encoder *encoder) > > +static bool skl_get_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; > > bool bret; > > > > @@ -1396,16 +1424,16 @@ skl_get_dpll(struct intel_crtc_state *crtc_state, > > bret = skl_ddi_hdmi_pll_dividers(crtc_state); > > if (!bret) { > > DRM_DEBUG_KMS("Could not get HDMI pll dividers.\n"); > > - return NULL; > > + return false; > > } > > } else if (intel_crtc_has_dp_encoder(crtc_state)) { > > bret = skl_ddi_dp_set_dpll_hw_state(crtc_state); > > if (!bret) { > > DRM_DEBUG_KMS("Could not set DP dpll HW state.\n"); > > - return NULL; > > + return false; > > } > > } else { > > - return NULL; > > + return false; > > } > > > > if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP)) > > @@ -1417,11 +1445,11 @@ skl_get_dpll(struct intel_crtc_state *crtc_state, > > DPLL_ID_SKL_DPLL1, > > DPLL_ID_SKL_DPLL3); > > if (!pll) > > - return NULL; > > + return false; > > > > intel_reference_shared_dpll(pll, crtc_state); > > > > - return pll; > > + return true; > > } > > > > static void skl_dump_hw_state(struct drm_i915_private *dev_priv, > > @@ -1827,22 +1855,23 @@ bxt_ddi_hdmi_set_dpll_hw_state(struct intel_crtc_state *crtc_state) > > return bxt_ddi_set_dpll_hw_state(crtc_state, &clk_div); > > } > > > > -static struct intel_shared_dpll * > > -bxt_get_dpll(struct intel_crtc_state *crtc_state, > > - struct intel_encoder *encoder) > > +static bool bxt_get_dpll(struct intel_atomic_state *state, > > + struct intel_crtc *crtc, > > + struct intel_encoder *encoder) > > { > > - struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > > + struct intel_crtc_state *crtc_state = > > + intel_atomic_get_new_crtc_state(state, crtc); > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > struct intel_shared_dpll *pll; > > enum intel_dpll_id id; > > > > if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI) && > > !bxt_ddi_hdmi_set_dpll_hw_state(crtc_state)) > > - return NULL; > > + return false; > > > > if (intel_crtc_has_dp_encoder(crtc_state) && > > !bxt_ddi_dp_set_dpll_hw_state(crtc_state)) > > - return NULL; > > + return false; > > > > /* 1:1 mapping between ports and PLLs */ > > id = (enum intel_dpll_id) encoder->port; > > @@ -1853,7 +1882,7 @@ bxt_get_dpll(struct intel_crtc_state *crtc_state, > > > > intel_reference_shared_dpll(pll, crtc_state); > > > > - return pll; > > + return true; > > } > > > > static void bxt_dump_hw_state(struct drm_i915_private *dev_priv, > > @@ -1884,8 +1913,11 @@ static const struct intel_shared_dpll_funcs bxt_ddi_pll_funcs = { > > struct intel_dpll_mgr { > > const struct dpll_info *dpll_info; > > > > - struct intel_shared_dpll *(*get_dpll)(struct intel_crtc_state *crtc_state, > > - struct intel_encoder *encoder); > > + bool (*get_dplls)(struct intel_atomic_state *state, > > + struct intel_crtc *crtc, > > + struct intel_encoder *encoder); > > + void (*put_dplls)(struct intel_atomic_state *state, > > + struct intel_crtc *crtc); > > > > void (*dump_hw_state)(struct drm_i915_private *dev_priv, > > const struct intel_dpll_hw_state *hw_state); > > @@ -1899,7 +1931,8 @@ static const struct dpll_info pch_plls[] = { > > > > static const struct intel_dpll_mgr pch_pll_mgr = { > > .dpll_info = pch_plls, > > - .get_dpll = ibx_get_dpll, > > + .get_dplls = ibx_get_dpll, > > + .put_dplls = intel_put_dpll, > > .dump_hw_state = ibx_dump_hw_state, > > }; > > > > @@ -1915,7 +1948,8 @@ static const struct dpll_info hsw_plls[] = { > > > > static const struct intel_dpll_mgr hsw_pll_mgr = { > > .dpll_info = hsw_plls, > > - .get_dpll = hsw_get_dpll, > > + .get_dplls = hsw_get_dpll, > > + .put_dplls = intel_put_dpll, > > .dump_hw_state = hsw_dump_hw_state, > > }; > > > > @@ -1929,7 +1963,8 @@ static const struct dpll_info skl_plls[] = { > > > > static const struct intel_dpll_mgr skl_pll_mgr = { > > .dpll_info = skl_plls, > > - .get_dpll = skl_get_dpll, > > + .get_dplls = skl_get_dpll, > > + .put_dplls = intel_put_dpll, > > .dump_hw_state = skl_dump_hw_state, > > }; > > > > @@ -1942,7 +1977,8 @@ static const struct dpll_info bxt_plls[] = { > > > > static const struct intel_dpll_mgr bxt_pll_mgr = { > > .dpll_info = bxt_plls, > > - .get_dpll = bxt_get_dpll, > > + .get_dplls = bxt_get_dpll, > > + .put_dplls = intel_put_dpll, > > .dump_hw_state = bxt_dump_hw_state, > > }; > > > > @@ -2332,10 +2368,12 @@ cnl_ddi_dp_set_dpll_hw_state(struct intel_crtc_state *crtc_state) > > return true; > > } > > > > -static struct intel_shared_dpll * > > -cnl_get_dpll(struct intel_crtc_state *crtc_state, > > - struct intel_encoder *encoder) > > +static bool cnl_get_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; > > bool bret; > > > > @@ -2343,18 +2381,18 @@ cnl_get_dpll(struct intel_crtc_state *crtc_state, > > bret = cnl_ddi_hdmi_pll_dividers(crtc_state); > > if (!bret) { > > DRM_DEBUG_KMS("Could not get HDMI pll dividers.\n"); > > - return NULL; > > + return false; > > } > > } else if (intel_crtc_has_dp_encoder(crtc_state)) { > > bret = cnl_ddi_dp_set_dpll_hw_state(crtc_state); > > if (!bret) { > > DRM_DEBUG_KMS("Could not set DP dpll HW state.\n"); > > - return NULL; > > + return false; > > } > > } else { > > DRM_DEBUG_KMS("Skip DPLL setup for output_types 0x%x\n", > > crtc_state->output_types); > > - return NULL; > > + return false; > > } > > > > pll = intel_find_shared_dpll(crtc_state, > > @@ -2362,12 +2400,12 @@ cnl_get_dpll(struct intel_crtc_state *crtc_state, > > DPLL_ID_SKL_DPLL2); > > if (!pll) { > > DRM_DEBUG_KMS("No PLL selected\n"); > > - return NULL; > > + return false; > > } > > > > intel_reference_shared_dpll(pll, crtc_state); > > > > - return pll; > > + return true; > > } > > > > static void cnl_dump_hw_state(struct drm_i915_private *dev_priv, > > @@ -2394,7 +2432,8 @@ static const struct dpll_info cnl_plls[] = { > > > > static const struct intel_dpll_mgr cnl_pll_mgr = { > > .dpll_info = cnl_plls, > > - .get_dpll = cnl_get_dpll, > > + .get_dplls = cnl_get_dpll, > > + .put_dplls = intel_put_dpll, > > .dump_hw_state = cnl_dump_hw_state, > > }; > > > > @@ -2792,11 +2831,13 @@ static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state) > > return true; > > } > > > > -static struct intel_shared_dpll * > > -icl_get_dpll(struct intel_crtc_state *crtc_state, > > - struct intel_encoder *encoder) > > +static bool icl_get_dplls(struct intel_atomic_state *state, > > + struct intel_crtc *crtc, > > + struct intel_encoder *encoder) > > { > > - struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > > + 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); > > struct intel_digital_port *intel_dig_port; > > struct intel_shared_dpll *pll; > > enum port port = encoder->port; > > @@ -2831,24 +2872,24 @@ icl_get_dpll(struct intel_crtc_state *crtc_state, > > } > > } else { > > MISSING_CASE(port); > > - return NULL; > > + return false; > > } > > > > if (!ret) { > > DRM_DEBUG_KMS("Could not calculate PLL state.\n"); > > - return NULL; > > + return false; > > } > > > > > > pll = intel_find_shared_dpll(crtc_state, min, max); > > if (!pll) { > > DRM_DEBUG_KMS("No PLL selected\n"); > > - return NULL; > > + return false; > > } > > > > intel_reference_shared_dpll(pll, crtc_state); > > > > - return pll; > > + return true; > > } > > > > static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv, > > @@ -3223,7 +3264,8 @@ static const struct dpll_info icl_plls[] = { > > > > static const struct intel_dpll_mgr icl_pll_mgr = { > > .dpll_info = icl_plls, > > - .get_dpll = icl_get_dpll, > > + .get_dplls = icl_get_dplls, > > + .put_dplls = intel_put_dpll, > > .dump_hw_state = icl_dump_hw_state, > > }; > > > > @@ -3235,7 +3277,8 @@ static const struct dpll_info ehl_plls[] = { > > > > static const struct intel_dpll_mgr ehl_pll_mgr = { > > .dpll_info = ehl_plls, > > - .get_dpll = icl_get_dpll, > > + .get_dplls = icl_get_dplls, > > + .put_dplls = intel_put_dpll, > > .dump_hw_state = icl_dump_hw_state, > > }; > > > > @@ -3287,50 +3330,64 @@ void intel_shared_dpll_init(struct drm_device *dev) > > } > > > > /** > > - * intel_get_shared_dpll - get a shared DPLL for CRTC and encoder combination > > - * @crtc_state: atomic state for the crtc > > + * intel_reserve_shared_dplls - reserve DPLLs for CRTC and encoder combination > > + * @state: atomic state > > + * @crtc: CRTC to reserve DPLLs for > > * @encoder: encoder > > * > > - * Find an appropriate DPLL for the given CRTC and encoder combination. A > > - * reference from the @crtc_state to the returned pll is registered in the > > - * atomic state. That configuration is made effective by calling > > - * intel_shared_dpll_swap_state(). The reference should be released by calling > > - * intel_release_shared_dpll(). > > + * This function reserves all required DPLLs for the given CRTC and encoder > > + * combination in the current atomic commit @state and the new @crtc atomic > > + * state. > > + * > > + * The new configuration in the atomic commit @state is made effective by > > + * calling intel_shared_dpll_swap_state(). > > + * > > + * The reserved DPLLs should be released by calling > > + * intel_release_shared_dplls(). > > * > > * Returns: > > - * A shared DPLL to be used by @crtc_state and @encoder. > > + * True if all required DPLLs were successfully reserved. > > */ > > -struct intel_shared_dpll * > > -intel_get_shared_dpll(struct intel_crtc_state *crtc_state, > > - struct intel_encoder *encoder) > > +bool intel_reserve_shared_dplls(struct intel_atomic_state *state, > > + struct intel_crtc *crtc, > > + struct intel_encoder *encoder) > > { > > - struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr; > > > > if (WARN_ON(!dpll_mgr)) > > - return NULL; > > + return false; > > > > - return dpll_mgr->get_dpll(crtc_state, encoder); > > + return dpll_mgr->get_dplls(state, crtc, encoder); > > } > > > > /** > > - * intel_release_shared_dpll - end use of DPLL by CRTC in atomic state > > - * @dpll: dpll in use by @crtc > > - * @crtc: crtc > > + * intel_release_shared_dplls - end use of DPLLs by CRTC in atomic state > > * @state: atomic state > > + * @crtc: crtc from which the DPLLs are to be released > > * > > - * This function releases the reference from @crtc to @dpll from the > > - * atomic @state. The new configuration is made effective by calling > > - * intel_shared_dpll_swap_state(). > > + * This function releases all DPLLs reserved by intel_reserve_shared_dplls() > > + * from the current atomic commit @state and the old @crtc atomic state. > > + * > > + * The new configuration in the atomic commit @state is made effective by > > + * calling intel_shared_dpll_swap_state(). > > */ > > -void intel_release_shared_dpll(struct intel_shared_dpll *dpll, > > - struct intel_crtc *crtc, > > - struct drm_atomic_state *state) > > +void intel_release_shared_dplls(struct intel_atomic_state *state, > > + struct intel_crtc *crtc) > > { > > - struct intel_shared_dpll_state *shared_dpll_state; > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > + const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr; > > + > > + /* > > + * FIXME: this function is called for every platform having a > > + * compute_clock hook, even though the platform doesn't yet support > > + * the shared DPLL framework and intel_reserve_shared_dplls() is not > > + * called on those. > > + */ > > + if (!dpll_mgr) > > + return; > > > > - shared_dpll_state = intel_atomic_get_shared_dpll_state(state); > > - shared_dpll_state[dpll->info->id].crtc_mask &= ~(1 << crtc->pipe); > > + dpll_mgr->put_dplls(state, crtc); > > } > > > > /** > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > > index d0570414f3d1..16ddab138574 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > > @@ -39,6 +39,7 @@ > > struct drm_atomic_state; > > struct drm_device; > > struct drm_i915_private; > > +struct intel_atomic_state; > > struct intel_crtc; > > struct intel_crtc_state; > > struct intel_encoder; > > @@ -195,7 +196,7 @@ struct intel_dpll_hw_state { > > * future state which would be applied by an atomic mode set (stored in > > * a struct &intel_atomic_state). > > * > > - * See also intel_get_shared_dpll() and intel_release_shared_dpll(). > > + * See also intel_reserve_shared_dplls() and intel_release_shared_dplls(). > > */ > > struct intel_shared_dpll_state { > > /** > > @@ -331,11 +332,11 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv, > > bool state); > > #define assert_shared_dpll_enabled(d, p) assert_shared_dpll(d, p, true) > > #define assert_shared_dpll_disabled(d, p) assert_shared_dpll(d, p, false) > > -struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc_state *state, > > - struct intel_encoder *encoder); > > -void intel_release_shared_dpll(struct intel_shared_dpll *dpll, > > - struct intel_crtc *crtc, > > - struct drm_atomic_state *state); > > +bool intel_reserve_shared_dplls(struct intel_atomic_state *state, > > + struct intel_crtc *crtc, > > + struct intel_encoder *encoder); > > +void intel_release_shared_dplls(struct intel_atomic_state *state, > > + struct intel_crtc *crtc); > > 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); > > -- > > 2.17.1 > > -- > Ville Syrjälä > Intel
On Tue, Jun 25, 2019 at 09:57:38PM +0300, Imre Deak wrote: > On Tue, Jun 25, 2019 at 04:53:01PM +0300, Ville Syrjälä wrote: > > On Thu, Jun 20, 2019 at 05:05:53PM +0300, Imre Deak wrote: > > > For consistency s/intel_get_shared_dpll()/intel_reserve_shared_dplls()/ > > > to better match intel_release_shared_dplls(). Also, pass to the > > > reserve/release and get_dplls/put_dplls hooks the intel_atomic_state and > > > CRTC object, that way these functions can look up the old or new state > > > as needed. > > > > > > Also release the PLLs from the atomic state via a new > > > put_dplls->intel_unreference_shared_dpll() call chain for better > > > symmetry with the reservation via the > > > get_dplls->intel_reference_shared_dpll() call chain. > > > > > > Since nothing uses the PLL returned by intel_reserve_shared_dplls(), > > > make it return only a bool. > > > > > > While at it also clarify the reserve/release function docbook headers > > > making it clear that multiple DPLLs will be reserved/released and > > > whether the new or old atomic CRTC state is affected. > > > > > > This refactoring is also a preparation for a follow-up change that needs > > > to reserve multiple DPLLs. > > > > > > Kudos to Ville for the idea to pass intel_atomic_state around, to make > > > things clearer locally where an object's old/new atomic state is > > > required. > > > > > > No functional changes. > > > > > > v2: > > > - Fix checkpatch issue: typo in code comment. > > > > > > 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/display/intel_display.c | 19 +- > > > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 221 +++++++++++------- > > > drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 13 +- > > > 3 files changed, 153 insertions(+), 100 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > index 468ca6d84bd8..688137524179 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -9504,6 +9504,8 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, > > > struct intel_crtc_state *crtc_state) > > > { > > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > > + struct intel_atomic_state *state = > > > + to_intel_atomic_state(crtc_state->base.state); > > > const struct intel_limit *limit; > > > int refclk = 120000; > > > > > > @@ -9545,7 +9547,7 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, > > > > > > ironlake_compute_dpll(crtc, crtc_state, NULL); > > > > > > - if (!intel_get_shared_dpll(crtc_state, NULL)) { > > > + if (!intel_reserve_shared_dplls(state, crtc, NULL)) { > > > DRM_DEBUG_KMS("failed to find PLL for pipe %c\n", > > > pipe_name(crtc->pipe)); > > > return -EINVAL; > > > @@ -9926,7 +9928,7 @@ static int haswell_crtc_compute_clock(struct intel_crtc *crtc, > > > struct intel_encoder *encoder = > > > intel_get_crtc_new_encoder(state, crtc_state); > > > > > > - if (!intel_get_shared_dpll(crtc_state, encoder)) { > > > + if (!intel_reserve_shared_dplls(state, crtc, encoder)) { > > > DRM_DEBUG_KMS("failed to find PLL for pipe %c\n", > > > pipe_name(crtc->pipe)); > > > return -EINVAL; > > > @@ -13195,27 +13197,20 @@ static void update_scanline_offset(const struct intel_crtc_state *crtc_state) > > > static void intel_modeset_clear_plls(struct intel_atomic_state *state) > > > { > > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > - struct intel_crtc_state *old_crtc_state, *new_crtc_state; > > > + struct intel_crtc_state *new_crtc_state; > > > struct intel_crtc *crtc; > > > int i; > > > > > > if (!dev_priv->display.crtc_compute_clock) > > > return; > > > > > > - for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > > - new_crtc_state, i) { > > > - struct intel_shared_dpll *old_dpll = > > > - old_crtc_state->shared_dpll; > > > - > > > + for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { > > > if (!needs_modeset(&new_crtc_state->base)) > > > continue; > > > > > > new_crtc_state->shared_dpll = NULL; > > > > > > - if (!old_dpll) > > > - continue; > > > - > > > - intel_release_shared_dpll(old_dpll, crtc, &state->base); > > > + intel_release_shared_dplls(state, crtc); > > > } > > > } > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > > index bf66261c8bf0..3fbc975851fa 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > > @@ -36,9 +36,10 @@ > > > * This file provides an abstraction over display PLLs. The function > > > * intel_shared_dpll_init() initializes the PLLs for the given platform. The > > > * users of a PLL are tracked and that tracking is integrated with the atomic > > > - * modest interface. During an atomic operation, a PLL can be requested for a > > > - * given CRTC and encoder configuration by calling intel_get_shared_dpll() and > > > - * a previously used PLL can be released with intel_release_shared_dpll(). > > > + * modset interface. During an atomic operation, required PLLs can be reserved > > > + * for a given CRTC and encoder configuration by calling > > > + * intel_reserve_shared_dplls() and previously reserved PLLs can be released > > > + * with intel_release_shared_dplls(). > > > * Changes to the users are first staged in the atomic state, and then made > > > * effective by calling intel_shared_dpll_swap_state() during the atomic > > > * commit phase. > > > @@ -309,6 +310,28 @@ intel_reference_shared_dpll(struct intel_shared_dpll *pll, > > > shared_dpll[id].crtc_mask |= 1 << crtc->pipe; > > > } > > > > > > +static void intel_unreference_shared_dpll(struct intel_atomic_state *state, > > > + const struct intel_crtc *crtc, > > > + const struct intel_shared_dpll *pll) > > > +{ > > > + struct intel_shared_dpll_state *shared_dpll; > > > + > > > + shared_dpll = intel_atomic_get_shared_dpll_state(&state->base); > > > + shared_dpll[pll->info->id].crtc_mask &= ~(1 << crtc->pipe); > > > +} > > > + > > > +static void intel_put_dpll(struct intel_atomic_state *state, > > > + struct intel_crtc *crtc) > > > +{ > > > + struct intel_crtc_state *crtc_state = > > > + intel_atomic_get_old_crtc_state(state, crtc); > > > > I'm wondering a bit why we're using the old crtc state here. We > > duplicated the state so shouldn't the new crtc state have the > > same dpll still at this point? > > > > Doesn't really matter I suppose. Not even sure which state would > > be more correct here. > > You are right, the new crtc state would be the better choice here. The > two states are equal here yes, but in the upcoming icl_put_dplls() we > should clear the new crtc_state->icl_port_dplls. Not clearing them > happens to not cause a problem, since we reallocate all PLLs in > icl_get_dplls(), but it's better to do the clearing anyway for > consistency. > > I'll change that. Ah, now I remember why new crtc state wouldn't work here: when restoring the state via intel_release_load_detect_pipe()->drm_atomic_helper_commit_duplicated_state() the new and old ctrc states won't be equal. So we need to use the old crtc state as that contains the PLL we must release, while the new crtc state could have no PLL assigned to it (if the pipe was disabled before load-detect). So we should leave this one here as-is, and do the clearing of icl_port_dplls[] later from the new crtc state (while still looking up the PLLs to put in icl_put_dplls() from the old crtc state). > > > > > Anyways, the patch seems OK. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > + > > > + if (!crtc_state->shared_dpll) > > > + return; > > > + > > > + intel_unreference_shared_dpll(state, crtc, crtc_state->shared_dpll); > > > +} > > > + > > > /** > > > * intel_shared_dpll_swap_state - make atomic DPLL configuration effective > > > * @state: atomic state > > > @@ -421,11 +444,12 @@ static void ibx_pch_dpll_disable(struct drm_i915_private *dev_priv, > > > udelay(200); > > > } > > > > > > -static struct intel_shared_dpll * > > > -ibx_get_dpll(struct intel_crtc_state *crtc_state, > > > - struct intel_encoder *encoder) > > > +static bool ibx_get_dpll(struct intel_atomic_state *state, > > > + struct intel_crtc *crtc, > > > + struct intel_encoder *encoder) > > > { > > > - struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > > > + struct intel_crtc_state *crtc_state = > > > + intel_atomic_get_new_crtc_state(state, crtc); > > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > > struct intel_shared_dpll *pll; > > > enum intel_dpll_id i; > > > @@ -445,12 +469,12 @@ ibx_get_dpll(struct intel_crtc_state *crtc_state, > > > } > > > > > > if (!pll) > > > - return NULL; > > > + return false; > > > > > > /* reference the pll */ > > > intel_reference_shared_dpll(pll, crtc_state); > > > > > > - return pll; > > > + return true; > > > } > > > > > > static void ibx_dump_hw_state(struct drm_i915_private *dev_priv, > > > @@ -821,10 +845,12 @@ hsw_ddi_dp_get_dpll(struct intel_crtc_state *crtc_state) > > > return pll; > > > } > > > > > > -static struct intel_shared_dpll * > > > -hsw_get_dpll(struct intel_crtc_state *crtc_state, > > > - struct intel_encoder *encoder) > > > +static bool hsw_get_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; > > > > > > memset(&crtc_state->dpll_hw_state, 0, > > > @@ -836,7 +862,7 @@ hsw_get_dpll(struct intel_crtc_state *crtc_state, > > > pll = hsw_ddi_dp_get_dpll(crtc_state); > > > } else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_ANALOG)) { > > > if (WARN_ON(crtc_state->port_clock / 2 != 135000)) > > > - return NULL; > > > + return false; > > > > > > crtc_state->dpll_hw_state.spll = > > > SPLL_PLL_ENABLE | SPLL_FREQ_1350MHz | SPLL_REF_MUXED_SSC; > > > @@ -844,15 +870,15 @@ hsw_get_dpll(struct intel_crtc_state *crtc_state, > > > pll = intel_find_shared_dpll(crtc_state, > > > DPLL_ID_SPLL, DPLL_ID_SPLL); > > > } else { > > > - return NULL; > > > + return false; > > > } > > > > > > if (!pll) > > > - return NULL; > > > + return false; > > > > > > intel_reference_shared_dpll(pll, crtc_state); > > > > > > - return pll; > > > + return true; > > > } > > > > > > static void hsw_dump_hw_state(struct drm_i915_private *dev_priv, > > > @@ -1385,10 +1411,12 @@ skl_ddi_dp_set_dpll_hw_state(struct intel_crtc_state *crtc_state) > > > return true; > > > } > > > > > > -static struct intel_shared_dpll * > > > -skl_get_dpll(struct intel_crtc_state *crtc_state, > > > - struct intel_encoder *encoder) > > > +static bool skl_get_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; > > > bool bret; > > > > > > @@ -1396,16 +1424,16 @@ skl_get_dpll(struct intel_crtc_state *crtc_state, > > > bret = skl_ddi_hdmi_pll_dividers(crtc_state); > > > if (!bret) { > > > DRM_DEBUG_KMS("Could not get HDMI pll dividers.\n"); > > > - return NULL; > > > + return false; > > > } > > > } else if (intel_crtc_has_dp_encoder(crtc_state)) { > > > bret = skl_ddi_dp_set_dpll_hw_state(crtc_state); > > > if (!bret) { > > > DRM_DEBUG_KMS("Could not set DP dpll HW state.\n"); > > > - return NULL; > > > + return false; > > > } > > > } else { > > > - return NULL; > > > + return false; > > > } > > > > > > if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP)) > > > @@ -1417,11 +1445,11 @@ skl_get_dpll(struct intel_crtc_state *crtc_state, > > > DPLL_ID_SKL_DPLL1, > > > DPLL_ID_SKL_DPLL3); > > > if (!pll) > > > - return NULL; > > > + return false; > > > > > > intel_reference_shared_dpll(pll, crtc_state); > > > > > > - return pll; > > > + return true; > > > } > > > > > > static void skl_dump_hw_state(struct drm_i915_private *dev_priv, > > > @@ -1827,22 +1855,23 @@ bxt_ddi_hdmi_set_dpll_hw_state(struct intel_crtc_state *crtc_state) > > > return bxt_ddi_set_dpll_hw_state(crtc_state, &clk_div); > > > } > > > > > > -static struct intel_shared_dpll * > > > -bxt_get_dpll(struct intel_crtc_state *crtc_state, > > > - struct intel_encoder *encoder) > > > +static bool bxt_get_dpll(struct intel_atomic_state *state, > > > + struct intel_crtc *crtc, > > > + struct intel_encoder *encoder) > > > { > > > - struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > > > + struct intel_crtc_state *crtc_state = > > > + intel_atomic_get_new_crtc_state(state, crtc); > > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > > struct intel_shared_dpll *pll; > > > enum intel_dpll_id id; > > > > > > if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI) && > > > !bxt_ddi_hdmi_set_dpll_hw_state(crtc_state)) > > > - return NULL; > > > + return false; > > > > > > if (intel_crtc_has_dp_encoder(crtc_state) && > > > !bxt_ddi_dp_set_dpll_hw_state(crtc_state)) > > > - return NULL; > > > + return false; > > > > > > /* 1:1 mapping between ports and PLLs */ > > > id = (enum intel_dpll_id) encoder->port; > > > @@ -1853,7 +1882,7 @@ bxt_get_dpll(struct intel_crtc_state *crtc_state, > > > > > > intel_reference_shared_dpll(pll, crtc_state); > > > > > > - return pll; > > > + return true; > > > } > > > > > > static void bxt_dump_hw_state(struct drm_i915_private *dev_priv, > > > @@ -1884,8 +1913,11 @@ static const struct intel_shared_dpll_funcs bxt_ddi_pll_funcs = { > > > struct intel_dpll_mgr { > > > const struct dpll_info *dpll_info; > > > > > > - struct intel_shared_dpll *(*get_dpll)(struct intel_crtc_state *crtc_state, > > > - struct intel_encoder *encoder); > > > + bool (*get_dplls)(struct intel_atomic_state *state, > > > + struct intel_crtc *crtc, > > > + struct intel_encoder *encoder); > > > + void (*put_dplls)(struct intel_atomic_state *state, > > > + struct intel_crtc *crtc); > > > > > > void (*dump_hw_state)(struct drm_i915_private *dev_priv, > > > const struct intel_dpll_hw_state *hw_state); > > > @@ -1899,7 +1931,8 @@ static const struct dpll_info pch_plls[] = { > > > > > > static const struct intel_dpll_mgr pch_pll_mgr = { > > > .dpll_info = pch_plls, > > > - .get_dpll = ibx_get_dpll, > > > + .get_dplls = ibx_get_dpll, > > > + .put_dplls = intel_put_dpll, > > > .dump_hw_state = ibx_dump_hw_state, > > > }; > > > > > > @@ -1915,7 +1948,8 @@ static const struct dpll_info hsw_plls[] = { > > > > > > static const struct intel_dpll_mgr hsw_pll_mgr = { > > > .dpll_info = hsw_plls, > > > - .get_dpll = hsw_get_dpll, > > > + .get_dplls = hsw_get_dpll, > > > + .put_dplls = intel_put_dpll, > > > .dump_hw_state = hsw_dump_hw_state, > > > }; > > > > > > @@ -1929,7 +1963,8 @@ static const struct dpll_info skl_plls[] = { > > > > > > static const struct intel_dpll_mgr skl_pll_mgr = { > > > .dpll_info = skl_plls, > > > - .get_dpll = skl_get_dpll, > > > + .get_dplls = skl_get_dpll, > > > + .put_dplls = intel_put_dpll, > > > .dump_hw_state = skl_dump_hw_state, > > > }; > > > > > > @@ -1942,7 +1977,8 @@ static const struct dpll_info bxt_plls[] = { > > > > > > static const struct intel_dpll_mgr bxt_pll_mgr = { > > > .dpll_info = bxt_plls, > > > - .get_dpll = bxt_get_dpll, > > > + .get_dplls = bxt_get_dpll, > > > + .put_dplls = intel_put_dpll, > > > .dump_hw_state = bxt_dump_hw_state, > > > }; > > > > > > @@ -2332,10 +2368,12 @@ cnl_ddi_dp_set_dpll_hw_state(struct intel_crtc_state *crtc_state) > > > return true; > > > } > > > > > > -static struct intel_shared_dpll * > > > -cnl_get_dpll(struct intel_crtc_state *crtc_state, > > > - struct intel_encoder *encoder) > > > +static bool cnl_get_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; > > > bool bret; > > > > > > @@ -2343,18 +2381,18 @@ cnl_get_dpll(struct intel_crtc_state *crtc_state, > > > bret = cnl_ddi_hdmi_pll_dividers(crtc_state); > > > if (!bret) { > > > DRM_DEBUG_KMS("Could not get HDMI pll dividers.\n"); > > > - return NULL; > > > + return false; > > > } > > > } else if (intel_crtc_has_dp_encoder(crtc_state)) { > > > bret = cnl_ddi_dp_set_dpll_hw_state(crtc_state); > > > if (!bret) { > > > DRM_DEBUG_KMS("Could not set DP dpll HW state.\n"); > > > - return NULL; > > > + return false; > > > } > > > } else { > > > DRM_DEBUG_KMS("Skip DPLL setup for output_types 0x%x\n", > > > crtc_state->output_types); > > > - return NULL; > > > + return false; > > > } > > > > > > pll = intel_find_shared_dpll(crtc_state, > > > @@ -2362,12 +2400,12 @@ cnl_get_dpll(struct intel_crtc_state *crtc_state, > > > DPLL_ID_SKL_DPLL2); > > > if (!pll) { > > > DRM_DEBUG_KMS("No PLL selected\n"); > > > - return NULL; > > > + return false; > > > } > > > > > > intel_reference_shared_dpll(pll, crtc_state); > > > > > > - return pll; > > > + return true; > > > } > > > > > > static void cnl_dump_hw_state(struct drm_i915_private *dev_priv, > > > @@ -2394,7 +2432,8 @@ static const struct dpll_info cnl_plls[] = { > > > > > > static const struct intel_dpll_mgr cnl_pll_mgr = { > > > .dpll_info = cnl_plls, > > > - .get_dpll = cnl_get_dpll, > > > + .get_dplls = cnl_get_dpll, > > > + .put_dplls = intel_put_dpll, > > > .dump_hw_state = cnl_dump_hw_state, > > > }; > > > > > > @@ -2792,11 +2831,13 @@ static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state) > > > return true; > > > } > > > > > > -static struct intel_shared_dpll * > > > -icl_get_dpll(struct intel_crtc_state *crtc_state, > > > - struct intel_encoder *encoder) > > > +static bool icl_get_dplls(struct intel_atomic_state *state, > > > + struct intel_crtc *crtc, > > > + struct intel_encoder *encoder) > > > { > > > - struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > > > + 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); > > > struct intel_digital_port *intel_dig_port; > > > struct intel_shared_dpll *pll; > > > enum port port = encoder->port; > > > @@ -2831,24 +2872,24 @@ icl_get_dpll(struct intel_crtc_state *crtc_state, > > > } > > > } else { > > > MISSING_CASE(port); > > > - return NULL; > > > + return false; > > > } > > > > > > if (!ret) { > > > DRM_DEBUG_KMS("Could not calculate PLL state.\n"); > > > - return NULL; > > > + return false; > > > } > > > > > > > > > pll = intel_find_shared_dpll(crtc_state, min, max); > > > if (!pll) { > > > DRM_DEBUG_KMS("No PLL selected\n"); > > > - return NULL; > > > + return false; > > > } > > > > > > intel_reference_shared_dpll(pll, crtc_state); > > > > > > - return pll; > > > + return true; > > > } > > > > > > static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv, > > > @@ -3223,7 +3264,8 @@ static const struct dpll_info icl_plls[] = { > > > > > > static const struct intel_dpll_mgr icl_pll_mgr = { > > > .dpll_info = icl_plls, > > > - .get_dpll = icl_get_dpll, > > > + .get_dplls = icl_get_dplls, > > > + .put_dplls = intel_put_dpll, > > > .dump_hw_state = icl_dump_hw_state, > > > }; > > > > > > @@ -3235,7 +3277,8 @@ static const struct dpll_info ehl_plls[] = { > > > > > > static const struct intel_dpll_mgr ehl_pll_mgr = { > > > .dpll_info = ehl_plls, > > > - .get_dpll = icl_get_dpll, > > > + .get_dplls = icl_get_dplls, > > > + .put_dplls = intel_put_dpll, > > > .dump_hw_state = icl_dump_hw_state, > > > }; > > > > > > @@ -3287,50 +3330,64 @@ void intel_shared_dpll_init(struct drm_device *dev) > > > } > > > > > > /** > > > - * intel_get_shared_dpll - get a shared DPLL for CRTC and encoder combination > > > - * @crtc_state: atomic state for the crtc > > > + * intel_reserve_shared_dplls - reserve DPLLs for CRTC and encoder combination > > > + * @state: atomic state > > > + * @crtc: CRTC to reserve DPLLs for > > > * @encoder: encoder > > > * > > > - * Find an appropriate DPLL for the given CRTC and encoder combination. A > > > - * reference from the @crtc_state to the returned pll is registered in the > > > - * atomic state. That configuration is made effective by calling > > > - * intel_shared_dpll_swap_state(). The reference should be released by calling > > > - * intel_release_shared_dpll(). > > > + * This function reserves all required DPLLs for the given CRTC and encoder > > > + * combination in the current atomic commit @state and the new @crtc atomic > > > + * state. > > > + * > > > + * The new configuration in the atomic commit @state is made effective by > > > + * calling intel_shared_dpll_swap_state(). > > > + * > > > + * The reserved DPLLs should be released by calling > > > + * intel_release_shared_dplls(). > > > * > > > * Returns: > > > - * A shared DPLL to be used by @crtc_state and @encoder. > > > + * True if all required DPLLs were successfully reserved. > > > */ > > > -struct intel_shared_dpll * > > > -intel_get_shared_dpll(struct intel_crtc_state *crtc_state, > > > - struct intel_encoder *encoder) > > > +bool intel_reserve_shared_dplls(struct intel_atomic_state *state, > > > + struct intel_crtc *crtc, > > > + struct intel_encoder *encoder) > > > { > > > - struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr; > > > > > > if (WARN_ON(!dpll_mgr)) > > > - return NULL; > > > + return false; > > > > > > - return dpll_mgr->get_dpll(crtc_state, encoder); > > > + return dpll_mgr->get_dplls(state, crtc, encoder); > > > } > > > > > > /** > > > - * intel_release_shared_dpll - end use of DPLL by CRTC in atomic state > > > - * @dpll: dpll in use by @crtc > > > - * @crtc: crtc > > > + * intel_release_shared_dplls - end use of DPLLs by CRTC in atomic state > > > * @state: atomic state > > > + * @crtc: crtc from which the DPLLs are to be released > > > * > > > - * This function releases the reference from @crtc to @dpll from the > > > - * atomic @state. The new configuration is made effective by calling > > > - * intel_shared_dpll_swap_state(). > > > + * This function releases all DPLLs reserved by intel_reserve_shared_dplls() > > > + * from the current atomic commit @state and the old @crtc atomic state. > > > + * > > > + * The new configuration in the atomic commit @state is made effective by > > > + * calling intel_shared_dpll_swap_state(). > > > */ > > > -void intel_release_shared_dpll(struct intel_shared_dpll *dpll, > > > - struct intel_crtc *crtc, > > > - struct drm_atomic_state *state) > > > +void intel_release_shared_dplls(struct intel_atomic_state *state, > > > + struct intel_crtc *crtc) > > > { > > > - struct intel_shared_dpll_state *shared_dpll_state; > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > + const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr; > > > + > > > + /* > > > + * FIXME: this function is called for every platform having a > > > + * compute_clock hook, even though the platform doesn't yet support > > > + * the shared DPLL framework and intel_reserve_shared_dplls() is not > > > + * called on those. > > > + */ > > > + if (!dpll_mgr) > > > + return; > > > > > > - shared_dpll_state = intel_atomic_get_shared_dpll_state(state); > > > - shared_dpll_state[dpll->info->id].crtc_mask &= ~(1 << crtc->pipe); > > > + dpll_mgr->put_dplls(state, crtc); > > > } > > > > > > /** > > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > > > index d0570414f3d1..16ddab138574 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > > > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > > > @@ -39,6 +39,7 @@ > > > struct drm_atomic_state; > > > struct drm_device; > > > struct drm_i915_private; > > > +struct intel_atomic_state; > > > struct intel_crtc; > > > struct intel_crtc_state; > > > struct intel_encoder; > > > @@ -195,7 +196,7 @@ struct intel_dpll_hw_state { > > > * future state which would be applied by an atomic mode set (stored in > > > * a struct &intel_atomic_state). > > > * > > > - * See also intel_get_shared_dpll() and intel_release_shared_dpll(). > > > + * See also intel_reserve_shared_dplls() and intel_release_shared_dplls(). > > > */ > > > struct intel_shared_dpll_state { > > > /** > > > @@ -331,11 +332,11 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv, > > > bool state); > > > #define assert_shared_dpll_enabled(d, p) assert_shared_dpll(d, p, true) > > > #define assert_shared_dpll_disabled(d, p) assert_shared_dpll(d, p, false) > > > -struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc_state *state, > > > - struct intel_encoder *encoder); > > > -void intel_release_shared_dpll(struct intel_shared_dpll *dpll, > > > - struct intel_crtc *crtc, > > > - struct drm_atomic_state *state); > > > +bool intel_reserve_shared_dplls(struct intel_atomic_state *state, > > > + struct intel_crtc *crtc, > > > + struct intel_encoder *encoder); > > > +void intel_release_shared_dplls(struct intel_atomic_state *state, > > > + struct intel_crtc *crtc); > > > 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); > > > -- > > > 2.17.1 > > > > -- > > Ville Syrjälä > > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 468ca6d84bd8..688137524179 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -9504,6 +9504,8 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) { struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + struct intel_atomic_state *state = + to_intel_atomic_state(crtc_state->base.state); const struct intel_limit *limit; int refclk = 120000; @@ -9545,7 +9547,7 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, ironlake_compute_dpll(crtc, crtc_state, NULL); - if (!intel_get_shared_dpll(crtc_state, NULL)) { + if (!intel_reserve_shared_dplls(state, crtc, NULL)) { DRM_DEBUG_KMS("failed to find PLL for pipe %c\n", pipe_name(crtc->pipe)); return -EINVAL; @@ -9926,7 +9928,7 @@ static int haswell_crtc_compute_clock(struct intel_crtc *crtc, struct intel_encoder *encoder = intel_get_crtc_new_encoder(state, crtc_state); - if (!intel_get_shared_dpll(crtc_state, encoder)) { + if (!intel_reserve_shared_dplls(state, crtc, encoder)) { DRM_DEBUG_KMS("failed to find PLL for pipe %c\n", pipe_name(crtc->pipe)); return -EINVAL; @@ -13195,27 +13197,20 @@ static void update_scanline_offset(const struct intel_crtc_state *crtc_state) static void intel_modeset_clear_plls(struct intel_atomic_state *state) { struct drm_i915_private *dev_priv = to_i915(state->base.dev); - struct intel_crtc_state *old_crtc_state, *new_crtc_state; + struct intel_crtc_state *new_crtc_state; struct intel_crtc *crtc; int i; if (!dev_priv->display.crtc_compute_clock) return; - for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, - new_crtc_state, i) { - struct intel_shared_dpll *old_dpll = - old_crtc_state->shared_dpll; - + for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { if (!needs_modeset(&new_crtc_state->base)) continue; new_crtc_state->shared_dpll = NULL; - if (!old_dpll) - continue; - - intel_release_shared_dpll(old_dpll, crtc, &state->base); + intel_release_shared_dplls(state, crtc); } } diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c index bf66261c8bf0..3fbc975851fa 100644 --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c @@ -36,9 +36,10 @@ * This file provides an abstraction over display PLLs. The function * intel_shared_dpll_init() initializes the PLLs for the given platform. The * users of a PLL are tracked and that tracking is integrated with the atomic - * modest interface. During an atomic operation, a PLL can be requested for a - * given CRTC and encoder configuration by calling intel_get_shared_dpll() and - * a previously used PLL can be released with intel_release_shared_dpll(). + * modset interface. During an atomic operation, required PLLs can be reserved + * for a given CRTC and encoder configuration by calling + * intel_reserve_shared_dplls() and previously reserved PLLs can be released + * with intel_release_shared_dplls(). * Changes to the users are first staged in the atomic state, and then made * effective by calling intel_shared_dpll_swap_state() during the atomic * commit phase. @@ -309,6 +310,28 @@ intel_reference_shared_dpll(struct intel_shared_dpll *pll, shared_dpll[id].crtc_mask |= 1 << crtc->pipe; } +static void intel_unreference_shared_dpll(struct intel_atomic_state *state, + const struct intel_crtc *crtc, + const struct intel_shared_dpll *pll) +{ + struct intel_shared_dpll_state *shared_dpll; + + shared_dpll = intel_atomic_get_shared_dpll_state(&state->base); + shared_dpll[pll->info->id].crtc_mask &= ~(1 << crtc->pipe); +} + +static void intel_put_dpll(struct intel_atomic_state *state, + struct intel_crtc *crtc) +{ + struct intel_crtc_state *crtc_state = + intel_atomic_get_old_crtc_state(state, crtc); + + if (!crtc_state->shared_dpll) + return; + + intel_unreference_shared_dpll(state, crtc, crtc_state->shared_dpll); +} + /** * intel_shared_dpll_swap_state - make atomic DPLL configuration effective * @state: atomic state @@ -421,11 +444,12 @@ static void ibx_pch_dpll_disable(struct drm_i915_private *dev_priv, udelay(200); } -static struct intel_shared_dpll * -ibx_get_dpll(struct intel_crtc_state *crtc_state, - struct intel_encoder *encoder) +static bool ibx_get_dpll(struct intel_atomic_state *state, + struct intel_crtc *crtc, + struct intel_encoder *encoder) { - struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); + struct intel_crtc_state *crtc_state = + intel_atomic_get_new_crtc_state(state, crtc); struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); struct intel_shared_dpll *pll; enum intel_dpll_id i; @@ -445,12 +469,12 @@ ibx_get_dpll(struct intel_crtc_state *crtc_state, } if (!pll) - return NULL; + return false; /* reference the pll */ intel_reference_shared_dpll(pll, crtc_state); - return pll; + return true; } static void ibx_dump_hw_state(struct drm_i915_private *dev_priv, @@ -821,10 +845,12 @@ hsw_ddi_dp_get_dpll(struct intel_crtc_state *crtc_state) return pll; } -static struct intel_shared_dpll * -hsw_get_dpll(struct intel_crtc_state *crtc_state, - struct intel_encoder *encoder) +static bool hsw_get_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; memset(&crtc_state->dpll_hw_state, 0, @@ -836,7 +862,7 @@ hsw_get_dpll(struct intel_crtc_state *crtc_state, pll = hsw_ddi_dp_get_dpll(crtc_state); } else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_ANALOG)) { if (WARN_ON(crtc_state->port_clock / 2 != 135000)) - return NULL; + return false; crtc_state->dpll_hw_state.spll = SPLL_PLL_ENABLE | SPLL_FREQ_1350MHz | SPLL_REF_MUXED_SSC; @@ -844,15 +870,15 @@ hsw_get_dpll(struct intel_crtc_state *crtc_state, pll = intel_find_shared_dpll(crtc_state, DPLL_ID_SPLL, DPLL_ID_SPLL); } else { - return NULL; + return false; } if (!pll) - return NULL; + return false; intel_reference_shared_dpll(pll, crtc_state); - return pll; + return true; } static void hsw_dump_hw_state(struct drm_i915_private *dev_priv, @@ -1385,10 +1411,12 @@ skl_ddi_dp_set_dpll_hw_state(struct intel_crtc_state *crtc_state) return true; } -static struct intel_shared_dpll * -skl_get_dpll(struct intel_crtc_state *crtc_state, - struct intel_encoder *encoder) +static bool skl_get_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; bool bret; @@ -1396,16 +1424,16 @@ skl_get_dpll(struct intel_crtc_state *crtc_state, bret = skl_ddi_hdmi_pll_dividers(crtc_state); if (!bret) { DRM_DEBUG_KMS("Could not get HDMI pll dividers.\n"); - return NULL; + return false; } } else if (intel_crtc_has_dp_encoder(crtc_state)) { bret = skl_ddi_dp_set_dpll_hw_state(crtc_state); if (!bret) { DRM_DEBUG_KMS("Could not set DP dpll HW state.\n"); - return NULL; + return false; } } else { - return NULL; + return false; } if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP)) @@ -1417,11 +1445,11 @@ skl_get_dpll(struct intel_crtc_state *crtc_state, DPLL_ID_SKL_DPLL1, DPLL_ID_SKL_DPLL3); if (!pll) - return NULL; + return false; intel_reference_shared_dpll(pll, crtc_state); - return pll; + return true; } static void skl_dump_hw_state(struct drm_i915_private *dev_priv, @@ -1827,22 +1855,23 @@ bxt_ddi_hdmi_set_dpll_hw_state(struct intel_crtc_state *crtc_state) return bxt_ddi_set_dpll_hw_state(crtc_state, &clk_div); } -static struct intel_shared_dpll * -bxt_get_dpll(struct intel_crtc_state *crtc_state, - struct intel_encoder *encoder) +static bool bxt_get_dpll(struct intel_atomic_state *state, + struct intel_crtc *crtc, + struct intel_encoder *encoder) { - struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); + struct intel_crtc_state *crtc_state = + intel_atomic_get_new_crtc_state(state, crtc); struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); struct intel_shared_dpll *pll; enum intel_dpll_id id; if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI) && !bxt_ddi_hdmi_set_dpll_hw_state(crtc_state)) - return NULL; + return false; if (intel_crtc_has_dp_encoder(crtc_state) && !bxt_ddi_dp_set_dpll_hw_state(crtc_state)) - return NULL; + return false; /* 1:1 mapping between ports and PLLs */ id = (enum intel_dpll_id) encoder->port; @@ -1853,7 +1882,7 @@ bxt_get_dpll(struct intel_crtc_state *crtc_state, intel_reference_shared_dpll(pll, crtc_state); - return pll; + return true; } static void bxt_dump_hw_state(struct drm_i915_private *dev_priv, @@ -1884,8 +1913,11 @@ static const struct intel_shared_dpll_funcs bxt_ddi_pll_funcs = { struct intel_dpll_mgr { const struct dpll_info *dpll_info; - struct intel_shared_dpll *(*get_dpll)(struct intel_crtc_state *crtc_state, - struct intel_encoder *encoder); + bool (*get_dplls)(struct intel_atomic_state *state, + struct intel_crtc *crtc, + struct intel_encoder *encoder); + void (*put_dplls)(struct intel_atomic_state *state, + struct intel_crtc *crtc); void (*dump_hw_state)(struct drm_i915_private *dev_priv, const struct intel_dpll_hw_state *hw_state); @@ -1899,7 +1931,8 @@ static const struct dpll_info pch_plls[] = { static const struct intel_dpll_mgr pch_pll_mgr = { .dpll_info = pch_plls, - .get_dpll = ibx_get_dpll, + .get_dplls = ibx_get_dpll, + .put_dplls = intel_put_dpll, .dump_hw_state = ibx_dump_hw_state, }; @@ -1915,7 +1948,8 @@ static const struct dpll_info hsw_plls[] = { static const struct intel_dpll_mgr hsw_pll_mgr = { .dpll_info = hsw_plls, - .get_dpll = hsw_get_dpll, + .get_dplls = hsw_get_dpll, + .put_dplls = intel_put_dpll, .dump_hw_state = hsw_dump_hw_state, }; @@ -1929,7 +1963,8 @@ static const struct dpll_info skl_plls[] = { static const struct intel_dpll_mgr skl_pll_mgr = { .dpll_info = skl_plls, - .get_dpll = skl_get_dpll, + .get_dplls = skl_get_dpll, + .put_dplls = intel_put_dpll, .dump_hw_state = skl_dump_hw_state, }; @@ -1942,7 +1977,8 @@ static const struct dpll_info bxt_plls[] = { static const struct intel_dpll_mgr bxt_pll_mgr = { .dpll_info = bxt_plls, - .get_dpll = bxt_get_dpll, + .get_dplls = bxt_get_dpll, + .put_dplls = intel_put_dpll, .dump_hw_state = bxt_dump_hw_state, }; @@ -2332,10 +2368,12 @@ cnl_ddi_dp_set_dpll_hw_state(struct intel_crtc_state *crtc_state) return true; } -static struct intel_shared_dpll * -cnl_get_dpll(struct intel_crtc_state *crtc_state, - struct intel_encoder *encoder) +static bool cnl_get_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; bool bret; @@ -2343,18 +2381,18 @@ cnl_get_dpll(struct intel_crtc_state *crtc_state, bret = cnl_ddi_hdmi_pll_dividers(crtc_state); if (!bret) { DRM_DEBUG_KMS("Could not get HDMI pll dividers.\n"); - return NULL; + return false; } } else if (intel_crtc_has_dp_encoder(crtc_state)) { bret = cnl_ddi_dp_set_dpll_hw_state(crtc_state); if (!bret) { DRM_DEBUG_KMS("Could not set DP dpll HW state.\n"); - return NULL; + return false; } } else { DRM_DEBUG_KMS("Skip DPLL setup for output_types 0x%x\n", crtc_state->output_types); - return NULL; + return false; } pll = intel_find_shared_dpll(crtc_state, @@ -2362,12 +2400,12 @@ cnl_get_dpll(struct intel_crtc_state *crtc_state, DPLL_ID_SKL_DPLL2); if (!pll) { DRM_DEBUG_KMS("No PLL selected\n"); - return NULL; + return false; } intel_reference_shared_dpll(pll, crtc_state); - return pll; + return true; } static void cnl_dump_hw_state(struct drm_i915_private *dev_priv, @@ -2394,7 +2432,8 @@ static const struct dpll_info cnl_plls[] = { static const struct intel_dpll_mgr cnl_pll_mgr = { .dpll_info = cnl_plls, - .get_dpll = cnl_get_dpll, + .get_dplls = cnl_get_dpll, + .put_dplls = intel_put_dpll, .dump_hw_state = cnl_dump_hw_state, }; @@ -2792,11 +2831,13 @@ static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state) return true; } -static struct intel_shared_dpll * -icl_get_dpll(struct intel_crtc_state *crtc_state, - struct intel_encoder *encoder) +static bool icl_get_dplls(struct intel_atomic_state *state, + struct intel_crtc *crtc, + struct intel_encoder *encoder) { - struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); + 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); struct intel_digital_port *intel_dig_port; struct intel_shared_dpll *pll; enum port port = encoder->port; @@ -2831,24 +2872,24 @@ icl_get_dpll(struct intel_crtc_state *crtc_state, } } else { MISSING_CASE(port); - return NULL; + return false; } if (!ret) { DRM_DEBUG_KMS("Could not calculate PLL state.\n"); - return NULL; + return false; } pll = intel_find_shared_dpll(crtc_state, min, max); if (!pll) { DRM_DEBUG_KMS("No PLL selected\n"); - return NULL; + return false; } intel_reference_shared_dpll(pll, crtc_state); - return pll; + return true; } static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv, @@ -3223,7 +3264,8 @@ static const struct dpll_info icl_plls[] = { static const struct intel_dpll_mgr icl_pll_mgr = { .dpll_info = icl_plls, - .get_dpll = icl_get_dpll, + .get_dplls = icl_get_dplls, + .put_dplls = intel_put_dpll, .dump_hw_state = icl_dump_hw_state, }; @@ -3235,7 +3277,8 @@ static const struct dpll_info ehl_plls[] = { static const struct intel_dpll_mgr ehl_pll_mgr = { .dpll_info = ehl_plls, - .get_dpll = icl_get_dpll, + .get_dplls = icl_get_dplls, + .put_dplls = intel_put_dpll, .dump_hw_state = icl_dump_hw_state, }; @@ -3287,50 +3330,64 @@ void intel_shared_dpll_init(struct drm_device *dev) } /** - * intel_get_shared_dpll - get a shared DPLL for CRTC and encoder combination - * @crtc_state: atomic state for the crtc + * intel_reserve_shared_dplls - reserve DPLLs for CRTC and encoder combination + * @state: atomic state + * @crtc: CRTC to reserve DPLLs for * @encoder: encoder * - * Find an appropriate DPLL for the given CRTC and encoder combination. A - * reference from the @crtc_state to the returned pll is registered in the - * atomic state. That configuration is made effective by calling - * intel_shared_dpll_swap_state(). The reference should be released by calling - * intel_release_shared_dpll(). + * This function reserves all required DPLLs for the given CRTC and encoder + * combination in the current atomic commit @state and the new @crtc atomic + * state. + * + * The new configuration in the atomic commit @state is made effective by + * calling intel_shared_dpll_swap_state(). + * + * The reserved DPLLs should be released by calling + * intel_release_shared_dplls(). * * Returns: - * A shared DPLL to be used by @crtc_state and @encoder. + * True if all required DPLLs were successfully reserved. */ -struct intel_shared_dpll * -intel_get_shared_dpll(struct intel_crtc_state *crtc_state, - struct intel_encoder *encoder) +bool intel_reserve_shared_dplls(struct intel_atomic_state *state, + struct intel_crtc *crtc, + struct intel_encoder *encoder) { - struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); + struct drm_i915_private *dev_priv = to_i915(state->base.dev); const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr; if (WARN_ON(!dpll_mgr)) - return NULL; + return false; - return dpll_mgr->get_dpll(crtc_state, encoder); + return dpll_mgr->get_dplls(state, crtc, encoder); } /** - * intel_release_shared_dpll - end use of DPLL by CRTC in atomic state - * @dpll: dpll in use by @crtc - * @crtc: crtc + * intel_release_shared_dplls - end use of DPLLs by CRTC in atomic state * @state: atomic state + * @crtc: crtc from which the DPLLs are to be released * - * This function releases the reference from @crtc to @dpll from the - * atomic @state. The new configuration is made effective by calling - * intel_shared_dpll_swap_state(). + * This function releases all DPLLs reserved by intel_reserve_shared_dplls() + * from the current atomic commit @state and the old @crtc atomic state. + * + * The new configuration in the atomic commit @state is made effective by + * calling intel_shared_dpll_swap_state(). */ -void intel_release_shared_dpll(struct intel_shared_dpll *dpll, - struct intel_crtc *crtc, - struct drm_atomic_state *state) +void intel_release_shared_dplls(struct intel_atomic_state *state, + struct intel_crtc *crtc) { - struct intel_shared_dpll_state *shared_dpll_state; + struct drm_i915_private *dev_priv = to_i915(state->base.dev); + const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr; + + /* + * FIXME: this function is called for every platform having a + * compute_clock hook, even though the platform doesn't yet support + * the shared DPLL framework and intel_reserve_shared_dplls() is not + * called on those. + */ + if (!dpll_mgr) + return; - shared_dpll_state = intel_atomic_get_shared_dpll_state(state); - shared_dpll_state[dpll->info->id].crtc_mask &= ~(1 << crtc->pipe); + dpll_mgr->put_dplls(state, crtc); } /** diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h index d0570414f3d1..16ddab138574 100644 --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h @@ -39,6 +39,7 @@ struct drm_atomic_state; struct drm_device; struct drm_i915_private; +struct intel_atomic_state; struct intel_crtc; struct intel_crtc_state; struct intel_encoder; @@ -195,7 +196,7 @@ struct intel_dpll_hw_state { * future state which would be applied by an atomic mode set (stored in * a struct &intel_atomic_state). * - * See also intel_get_shared_dpll() and intel_release_shared_dpll(). + * See also intel_reserve_shared_dplls() and intel_release_shared_dplls(). */ struct intel_shared_dpll_state { /** @@ -331,11 +332,11 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv, bool state); #define assert_shared_dpll_enabled(d, p) assert_shared_dpll(d, p, true) #define assert_shared_dpll_disabled(d, p) assert_shared_dpll(d, p, false) -struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc_state *state, - struct intel_encoder *encoder); -void intel_release_shared_dpll(struct intel_shared_dpll *dpll, - struct intel_crtc *crtc, - struct drm_atomic_state *state); +bool intel_reserve_shared_dplls(struct intel_atomic_state *state, + struct intel_crtc *crtc, + struct intel_encoder *encoder); +void intel_release_shared_dplls(struct intel_atomic_state *state, + struct intel_crtc *crtc); 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);
For consistency s/intel_get_shared_dpll()/intel_reserve_shared_dplls()/ to better match intel_release_shared_dplls(). Also, pass to the reserve/release and get_dplls/put_dplls hooks the intel_atomic_state and CRTC object, that way these functions can look up the old or new state as needed. Also release the PLLs from the atomic state via a new put_dplls->intel_unreference_shared_dpll() call chain for better symmetry with the reservation via the get_dplls->intel_reference_shared_dpll() call chain. Since nothing uses the PLL returned by intel_reserve_shared_dplls(), make it return only a bool. While at it also clarify the reserve/release function docbook headers making it clear that multiple DPLLs will be reserved/released and whether the new or old atomic CRTC state is affected. This refactoring is also a preparation for a follow-up change that needs to reserve multiple DPLLs. Kudos to Ville for the idea to pass intel_atomic_state around, to make things clearer locally where an object's old/new atomic state is required. No functional changes. v2: - Fix checkpatch issue: typo in code comment. 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/display/intel_display.c | 19 +- drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 221 +++++++++++------- drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 13 +- 3 files changed, 153 insertions(+), 100 deletions(-)