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

[Ville Syrjälä]

On Thu, Sep 22, 2022 at 06:55:37PM +0300, Jani Nikula wrote:
> On Wed, 21 Sep 2022, Ville Syrjala <ville.syrjala at linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala at 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 at 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.

Yeah, I guess we can do that. Doing it only at init time might be
nicer but then I'd have to duplicate some code... What a conundrum.

> 
> >  
> > +		/* 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.

I'd prefer to catch it in 64bit builds too. This is the reason
I hate 'long'.

> 
> Regardless,
> 
> Reviewed-by: Jani Nikula <jani.nikula at intel.com>
> 
> 
> > +			break;
> > +
> >  		dev_priv->display.dpll.shared_dplls[i].info = &dpll_info[i];
> >  	}
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Ville Syrjälä
Intel