On Tue, Oct 08, 2019 at 09:12:52AM -0700, Matt Roper wrote:
> This slightly simplifies the EHL DPLL4 handling and also gives us more
> flexibility in the future in case we need to skip the use of specific
> PLL's (e.g., due to hardware workarounds and such).
>
> Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 42 +++++++++----------
> 1 file changed, 20 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index 5e9e84c94a15..14e040658b12 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -247,8 +247,7 @@ static struct intel_shared_dpll *
> intel_find_shared_dpll(struct intel_atomic_state *state,
> const struct intel_crtc *crtc,
> const struct intel_dpll_hw_state *pll_state,
> - enum intel_dpll_id range_min,
> - enum intel_dpll_id range_max)
> + unsigned long dpll_mask)
I don't like seeing lone longs hanging around since they
change meaning between 32bit and 64bit builds. Always makes
me suspicious.
> {
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> struct intel_shared_dpll *pll, *unused_pll = NULL;
> @@ -257,7 +256,9 @@ intel_find_shared_dpll(struct intel_atomic_state *state,
>
> shared_dpll = intel_atomic_get_shared_dpll_state(&state->base);
>
> - for (i = range_min; i <= range_max; i++) {
> + WARN_ON(dpll_mask & ~(BIT(I915_NUM_PLLS) - 1));
> +
> + for_each_set_bit(i, &dpll_mask, I915_NUM_PLLS) {
But I guess this guy demands one :(
> pll = &dev_priv->shared_dplls[i];
>
> /* Only want to check enabled timings first */
> @@ -464,8 +465,8 @@ static bool ibx_get_dpll(struct intel_atomic_state *state,
> } else {
> pll = intel_find_shared_dpll(state, crtc,
> &crtc_state->dpll_hw_state,
> - DPLL_ID_PCH_PLL_A,
> - DPLL_ID_PCH_PLL_B);
> + GENMASK(DPLL_ID_PCH_PLL_B,
> + DPLL_ID_PCH_PLL_A));
I wonder if it wouldn't be better to always do BIT(A)|BIT(B)...
so that we won't get bitten if someone ever inserts new DPLL
IDs in the middle of the enum?
Anyways, seems at least as good as the current thing:
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> }
>
> if (!pll)
> @@ -814,7 +815,7 @@ hsw_ddi_hdmi_get_dpll(struct intel_atomic_state *state,
>
> pll = intel_find_shared_dpll(state, crtc,
> &crtc_state->dpll_hw_state,
> - DPLL_ID_WRPLL1, DPLL_ID_WRPLL2);
> + GENMASK(DPLL_ID_WRPLL2, DPLL_ID_WRPLL1));
>
> if (!pll)
> return NULL;
> @@ -877,7 +878,7 @@ static bool hsw_get_dpll(struct intel_atomic_state *state,
>
> pll = intel_find_shared_dpll(state, crtc,
> &crtc_state->dpll_hw_state,
> - DPLL_ID_SPLL, DPLL_ID_SPLL);
> + BIT(DPLL_ID_SPLL));
> } else {
> return false;
> }
> @@ -1447,13 +1448,12 @@ static bool skl_get_dpll(struct intel_atomic_state *state,
> if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP))
> pll = intel_find_shared_dpll(state, crtc,
> &crtc_state->dpll_hw_state,
> - DPLL_ID_SKL_DPLL0,
> - DPLL_ID_SKL_DPLL0);
> + BIT(DPLL_ID_SKL_DPLL0));
> else
> pll = intel_find_shared_dpll(state, crtc,
> &crtc_state->dpll_hw_state,
> - DPLL_ID_SKL_DPLL1,
> - DPLL_ID_SKL_DPLL3);
> + GENMASK(DPLL_ID_SKL_DPLL3,
> + DPLL_ID_SKL_DPLL1));
> if (!pll)
> return false;
>
> @@ -2401,8 +2401,8 @@ static bool cnl_get_dpll(struct intel_atomic_state *state,
>
> pll = intel_find_shared_dpll(state, crtc,
> &crtc_state->dpll_hw_state,
> - DPLL_ID_SKL_DPLL0,
> - DPLL_ID_SKL_DPLL2);
> + GENMASK(DPLL_ID_SKL_DPLL2,
> + DPLL_ID_SKL_DPLL0));
> if (!pll) {
> DRM_DEBUG_KMS("No PLL selected\n");
> return false;
> @@ -2975,7 +2975,7 @@ static bool icl_get_combo_phy_dpll(struct intel_atomic_state *state,
> &crtc_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> enum port port = encoder->port;
> - bool has_dpll4 = false;
> + unsigned dpll_mask;
>
> if (!icl_calc_dpll_state(crtc_state, encoder, &port_dpll->hw_state)) {
> DRM_DEBUG_KMS("Could not calculate combo PHY PLL state.\n");
> @@ -2984,13 +2984,13 @@ static bool icl_get_combo_phy_dpll(struct intel_atomic_state *state,
> }
>
> if (IS_ELKHARTLAKE(dev_priv) && port != PORT_A)
> - has_dpll4 = true;
> + dpll_mask = GENMASK(DPLL_ID_EHL_DPLL4, DPLL_ID_ICL_DPLL0);
> + else
> + dpll_mask = GENMASK(DPLL_ID_ICL_DPLL1, DPLL_ID_ICL_DPLL0);
>
> port_dpll->pll = intel_find_shared_dpll(state, crtc,
> &port_dpll->hw_state,
> - DPLL_ID_ICL_DPLL0,
> - has_dpll4 ? DPLL_ID_EHL_DPLL4
> - : DPLL_ID_ICL_DPLL1);
> + dpll_mask);
> if (!port_dpll->pll) {
> DRM_DEBUG_KMS("No combo PHY PLL found for [ENCODER:%d:%s]\n",
> encoder->base.base.id, encoder->base.name);
> @@ -3023,8 +3023,7 @@ static bool icl_get_tc_phy_dplls(struct intel_atomic_state *state,
>
> port_dpll->pll = intel_find_shared_dpll(state, crtc,
> &port_dpll->hw_state,
> - DPLL_ID_ICL_TBTPLL,
> - DPLL_ID_ICL_TBTPLL);
> + BIT(DPLL_ID_ICL_TBTPLL));
> if (!port_dpll->pll) {
> DRM_DEBUG_KMS("No TBT-ALT PLL found\n");
> return false;
> @@ -3043,8 +3042,7 @@ static bool icl_get_tc_phy_dplls(struct intel_atomic_state *state,
> encoder->port));
> port_dpll->pll = intel_find_shared_dpll(state, crtc,
> &port_dpll->hw_state,
> - dpll_id,
> - dpll_id);
> + BIT(dpll_id));
> if (!port_dpll->pll) {
> DRM_DEBUG_KMS("No MG PHY PLL found\n");
> goto err_unreference_tbt_pll;
> --
> 2.21.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
