On Thu, Sep 22, 2022 at 06:46:30PM +0300, Jani Nikula wrote:
> On Wed, 21 Sep 2022, Ville Syrjala <ville.syrj...@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
> >
> > 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ä <ville.syrj...@linux.intel.com>
> > ---
> > 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.
Dunno if it's more robust, but I guess it does decouple us
a bit more from the array storage of the actual plls. We
could even do this exactly like eg. drm_crtc, ie. introduce
pll->index for the atomic state indexing but could use a
linked list to keep the actual plls. Though that would
again mean more kmallocs() that can fail, so I don't think
I'll go quite that far, at least not yet. Though I suppose
I could introduce pll->index already...
--
Ville Syrjälä
Intel
