Re: [PATCH] drm/i915: Select DPLL's via mask

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  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@linux.intel.com>

>  	}
>  
>  	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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx