[Intel-gfx] [PATCH] drm/i915: Select DPLL's via mask

[Lucas De Marchi]

On Tue, Oct 08, 2019 at 07:28:45PM +0300, Ville Syrjälä wrote:
>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 at intel.com>
>> Signed-off-by: Matt Roper <matthew.d.roper at 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 :(

We are generally safe when the limit is much lower than the word sizes
we care about. I915_NUM_PLLS is currently 9 and I don't think it will go
near 32.

>> @@ -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;

for consistency this should be unsigned long too.

Lucas De Marchi

>>
>>  	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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>-- 
>Ville Syrjälä
>Intel