On Thu, Sep 22, 2022 at 06:55:37PM +0300, Jani Nikula wrote:
> On Wed, 21 Sep 2022, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
>
>
> > + break;
> > +
> > dev_priv->display.dpll.shared_dplls[i].info = &dpll_info[i];
> > }
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Ville Syrjälä
Intel
