From: Imre Deak Date: Fri, 28 Jun 2019 14:36:28 +0000 (+0300) Subject: drm/i915: Sanitize the shared DPLL reserve/release interface X-Git-Url: http://git.lede-project.org./?a=commitdiff_plain;h=866955fa452e1f3d2164bd731d71348d4978e8bc;p=openwrt%2Fstaging%2Fblogic.git drm/i915: Sanitize the shared DPLL reserve/release interface 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. v3: - Rebase on drm-tip. Cc: Ville Syrjälä Cc: Daniel Vetter Cc: Maarten Lankhorst Signed-off-by: Imre Deak Reviewed-by: Ville Syrjälä Link: https://patchwork.freedesktop.org/patch/msgid/20190628143635.22066-17-imre.deak@intel.com --- diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 2b6732e0d02c..b27cdaad611f 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -9493,6 +9493,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; @@ -9534,7 +9536,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; @@ -9915,7 +9917,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; @@ -13171,27 +13173,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)) 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);