Re: [Intel-gfx] [PATCH 4/4] drm/i915: Decouple I915_NUM_PLLS from PLL IDs

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 21 Sep 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Stop assuming the size of PLL ID based bitmask is restricted
> to I915_NUM_PLLS bits. This is the last thing coupling the
> two things together and thus artificially limiting PLL IDs.
>
> We could just pass any arbitrary (large enough) size to
> for_each_set_bit() and be done with it, but the WARN
> requiring the caller to not pass in a bogus bitmask seems
> potentially useful to keep around. So let's just calculate
> the full bitmask on the spot.
>
> And while at it let's assert that the PLL IDs will fit
> into the bitmask we use for them.
>
> TODO: could also get rid of I915_NUM_PLLS entirely and just
> dynamically allocate i915->shared_dplls[] and state->shared_dpll[].
> But that would involve error handling in the modeset init path. Uff.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 24 +++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index fb09029cc4aa..ee04b63d2696 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -315,6 +315,21 @@ void intel_disable_shared_dpll(const struct intel_crtc_state *crtc_state)
>  	mutex_unlock(&dev_priv->display.dpll.lock);
>  }
>  
> +static unsigned long
> +intel_dpll_mask_all(struct drm_i915_private *i915)
> +{
> +	unsigned long dpll_mask = 0;
> +	int i;
> +
> +	for (i = 0; i < i915->display.dpll.num_shared_dpll; i++) {
> +		struct intel_shared_dpll *pll = &i915->display.dpll.shared_dplls[i];
> +
> +		dpll_mask |= BIT(pll->info->id);
> +	}
> +
> +	return dpll_mask;
> +}
> +
>  static struct intel_shared_dpll *
>  intel_find_shared_dpll(struct intel_atomic_state *state,
>  		       const struct intel_crtc *crtc,
> @@ -322,15 +337,16 @@ intel_find_shared_dpll(struct intel_atomic_state *state,
>  		       unsigned long dpll_mask)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	unsigned long dpll_mask_all = intel_dpll_mask_all(dev_priv);
>  	struct intel_shared_dpll_state *shared_dpll;
>  	struct intel_shared_dpll *unused_pll = NULL;
>  	enum intel_dpll_id id;
>  
>  	shared_dpll = intel_atomic_get_shared_dpll_state(&state->base);
>  
> -	drm_WARN_ON(&dev_priv->drm, dpll_mask & ~(BIT(I915_NUM_PLLS) - 1));
> +	drm_WARN_ON(&dev_priv->drm, dpll_mask & ~dpll_mask_all);
>  
> -	for_each_set_bit(id, &dpll_mask, I915_NUM_PLLS) {
> +	for_each_set_bit(id, &dpll_mask, fls(dpll_mask_all)) {
>  		struct intel_shared_dpll *pll;
>  		int i;
>  
> @@ -4234,6 +4250,10 @@ void intel_shared_dpll_init(struct drm_i915_private *dev_priv)
>  				i >= ARRAY_SIZE(dev_priv->display.dpll.shared_dplls)))
>  			break;

Would be nice to add

	unsigned long dpll_mask;

        drm_WARN_ON(&dev_priv->drm, dpll_mask & BIT(pll->info->id));

	dpll_mask |= BIT(pll->info->id);

to check for collisions.

>  
> +		/* must fit into unsigned long bitmask on 32bit */
> +		if (drm_WARN_ON(&dev_priv->drm, dpll_info[i].id >= 32))

BITS_PER_TYPE(dpll_mask) instead of 32? Of course would only hit this
when actually running a 32-bit build.

Regardless,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> +			break;
> +
>  		dev_priv->display.dpll.shared_dplls[i].info = &dpll_info[i];
>  	}

-- 
Jani Nikula, Intel Open Source Graphics Center