Re: [Intel-gfx] [PATCH 3/4] drm/i915: Stop requiring PLL index == PLL ID

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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