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.
>
> + /* 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@xxxxxxxxx>
> + break;
> +
> dev_priv->display.dpll.shared_dplls[i].info = &dpll_info[i];
> }
--
Jani Nikula, Intel Open Source Graphics Center
