On Wed, 21 Sep 2022, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> There's no good reason to keep around this PLL index == PLL ID
> footgun. Get rid of it.
>
> Both i915->shared_dplls[] and state->shared_dpll[] are indexed
> by the same thing now, which is just the index we get at
> initialization from dpll_mgr->dpll_info[]. The rest is all about
> PLL IDs now.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 64 +--
> .../gpu/drm/i915/display/intel_pch_refclk.c | 5 +-
> 2 files changed, 47 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 f900c4c73cc6..fb09029cc4aa 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -110,7 +110,7 @@ static void
> intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
> struct intel_shared_dpll_state *shared_dpll)
> {
> - enum intel_dpll_id i;
> + int i;
>
> /* Copy shared dpll state */
> for (i = 0; i < dev_priv->display.dpll.num_shared_dpll; i++) {
> @@ -137,6 +137,13 @@ intel_atomic_get_shared_dpll_state(struct
> drm_atomic_state *s)
> return state->shared_dpll;
> }
>
> +static int
> +intel_shared_dpll_idx(struct drm_i915_private *i915,
> + const struct intel_shared_dpll *pll)
> +{
> + return pll - &i915->display.dpll.shared_dplls[0];
> +}
I liked getting rid of this magic in the previous patch, and would not
like to have it brought back!
I'm thinking
static int
intel_shared_dpll_idx(struct drm_i915_private *i915, enum intel_dpll_id id)
which would loop over shared_dplls[] and return the index, similar to
the function below. Feels more robust.
Otherwise LGTM.
BR,
Jani.
> +
> /**
> * intel_get_shared_dpll_by_id - get a DPLL given its id
> * @dev_priv: i915 device instance
> @@ -149,7 +156,17 @@ struct intel_shared_dpll *
> intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
> enum intel_dpll_id id)
> {
> - return &dev_priv->display.dpll.shared_dplls[id];
> + int i;
> +
> + for (i = 0; i < dev_priv->display.dpll.num_shared_dpll; i++) {
> + struct intel_shared_dpll *pll =
> &dev_priv->display.dpll.shared_dplls[i];
> +
> + if (pll->info->id == id)
> + return pll;
> + }
> +
> + MISSING_CASE(id);
> + return NULL;
> }
>
> /* For ILK+ */
> @@ -305,16 +322,23 @@ intel_find_shared_dpll(struct intel_atomic_state *state,
> unsigned long dpll_mask)
> {
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> - struct intel_shared_dpll *pll, *unused_pll = NULL;
> struct intel_shared_dpll_state *shared_dpll;
> - enum intel_dpll_id i;
> + 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));
>
> - for_each_set_bit(i, &dpll_mask, I915_NUM_PLLS) {
> - pll = &dev_priv->display.dpll.shared_dplls[i];
> + for_each_set_bit(id, &dpll_mask, I915_NUM_PLLS) {
> + struct intel_shared_dpll *pll;
> + int i;
> +
> + pll = intel_get_shared_dpll_by_id(dev_priv, id);
> + if (!pll)
> + continue;
> +
> + i = intel_shared_dpll_idx(dev_priv, pll);
>
> /* Only want to check enabled timings first */
> if (shared_dpll[i].pipe_mask == 0) {
> @@ -355,27 +379,29 @@ intel_reference_shared_dpll(struct intel_atomic_state
> *state,
> {
> struct drm_i915_private *i915 = to_i915(state->base.dev);
> struct intel_shared_dpll_state *shared_dpll;
> - const enum intel_dpll_id id = pll->info->id;
> + int i = intel_shared_dpll_idx(i915, pll);
>
> shared_dpll = intel_atomic_get_shared_dpll_state(&state->base);
>
> - if (shared_dpll[id].pipe_mask == 0)
> - shared_dpll[id].hw_state = *pll_state;
> + if (shared_dpll[i].pipe_mask == 0)
> + shared_dpll[i].hw_state = *pll_state;
>
> drm_dbg(&i915->drm, "using %s for pipe %c\n", pll->info->name,
> pipe_name(crtc->pipe));
>
> - shared_dpll[id].pipe_mask |= BIT(crtc->pipe);
> + shared_dpll[i].pipe_mask |= BIT(crtc->pipe);
> }
>
> static void intel_unreference_shared_dpll(struct intel_atomic_state *state,
> const struct intel_crtc *crtc,
> const struct intel_shared_dpll *pll)
> {
> + struct drm_i915_private *i915 = to_i915(state->base.dev);
> struct intel_shared_dpll_state *shared_dpll;
> + int i = intel_shared_dpll_idx(i915, pll);
>
> shared_dpll = intel_atomic_get_shared_dp