[Intel-gfx] [PATCH v2 07/14] drm/i915: Start moving the cdclk stuff into a distinct state structure

Fri Dec 23 13:07:25 UTC 2016

On Fri, 2016-12-23 at 14:27 +0200, Ville Syrjälä wrote:
> On Fri, Dec 23, 2016 at 11:09:22AM +0200, Ander Conselvan De Oliveira wrote:
> > 
> > On Thu, 2016-12-22 at 16:33 +0200, Ville Syrjälä wrote:
> > > 
> > > On Thu, Dec 22, 2016 at 04:14:40PM +0200, Ander Conselvan De Oliveira
> > > wrote:
> > > > 
> > > > 
> > > > On Mon, 2016-12-19 at 19:28 +0200, ville.syrjala at linux.intel.com wrote:
> > > > > 
> > > > > 
> > > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > 
> > > > > Introduce intel_cdclk state which for now will track the cdclk
> > > > > frequency, the vco frequency and the reference frequency (not sure we
> > > > > want the last one, but I put it there anyway). We'll also make the
> > > > > .get_cdclk() function fill out this state structure rather than
> > > > > just returning the current cdclk frequency.
> > > > > 
> > > > > One immediate benefit is that calling .get_cdclk() will no longer
> > > > > clobber state stored under dev_priv unless ex[plicitly told to do
> > > > Typo: ex[plicity
> > > > 
> > > > > 
> > > > > 
> > > > > so. Previously it clobbered the vco and reference clocks stored
> > > > > there on some platforms.
> > > > > 
> > > > > We'll expand the use of this structure to actually precomputing the
> > > > > state and whatnot later.
> > > > > 
> > > > > v2: Constify intel_cdclk_state_compare()
> > > > > 
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > A couple more comments beloew, but either way,
> > > > 
> > > > Reviewed-by: Ander Conselvan de Oliveira <conselvan2 at gmail.com>
> > > > 
> > > > > 
> > > > > 
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_debugfs.c     |   2 +-
> > > > >  drivers/gpu/drm/i915/i915_drv.h         |  14 +-
> > > > >  drivers/gpu/drm/i915/intel_audio.c      |   2 +-
> > > > >  drivers/gpu/drm/i915/intel_cdclk.c      | 359 ++++++++++++++++++-----
> > > > > ---------
> > > > >  drivers/gpu/drm/i915/intel_display.c    |  18 +-
> > > > >  drivers/gpu/drm/i915/intel_dp.c         |   2 +-
> > > > >  drivers/gpu/drm/i915/intel_drv.h        |   3 +
> > > > >  drivers/gpu/drm/i915/intel_fbc.c        |   2 +-
> > > > >  drivers/gpu/drm/i915/intel_panel.c      |   4 +-
> > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c |   5 +-
> > > > >  10 files changed, 240 insertions(+), 171 deletions(-)
> > > > > 
> > [...]
> > 
> > @@ -629,12 +668,13 @@ static int skl_calc_cdclk(int max_pixclk, int vco)
> > > 
> > > > 
> > > > > 
> > > > >  }
> > > > >  
> > > > > -static void skl_dpll0_update(struct drm_i915_private *dev_priv)
> > > > > +static void skl_dpll0_update(struct drm_i915_private *dev_priv,
> > > > > +			     struct intel_cdclk_state *cdclk_state)
> > > > >  {
> > > > >  	u32 val;
> > > > >  
> > > > > -	dev_priv->cdclk_pll.ref = 24000;
> > > > > -	dev_priv->cdclk_pll.vco = 0;
> > > > > +	cdclk_state->ref = 24000;
> > > > > +	cdclk_state->vco = 0;
> > > > >  
> > > > >  	val = I915_READ(LCPLL1_CTL);
> > > > >  	if ((val & LCPLL_PLL_ENABLE) == 0)
> > > > > @@ -656,11 +696,11 @@ static void skl_dpll0_update(struct
> > > > > drm_i915_private *dev_priv)
> > > > >  	case DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1350,
> > > > > SKL_DPLL0):
> > > > >  	case DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1620,
> > > > > SKL_DPLL0):
> > > > >  	case DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_2700,
> > > > > SKL_DPLL0):
> > > > > -		dev_priv->cdclk_pll.vco = 8100000;
> > > > > +		cdclk_state->vco = 8100000;
> > > > >  		break;
> > > > >  	case DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1080,
> > > > > SKL_DPLL0):
> > > > >  	case DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_2160,
> > > > > SKL_DPLL0):
> > > > > -		dev_priv->cdclk_pll.vco = 8640000;
> > > > > +		cdclk_state->vco = 8640000;
> > > > >  		break;
> > > > >  	default:
> > > > >  		MISSING_CASE(val &
> > > > > DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0));
> > > > This function is a bit funny now, since it sets up the cdclk state based
> > > > on
> > > > current pll0 programming, instead of looking at an atomic state.
> > > We should probably rename it to somehting more descriptive.
> > > skl_get_dpll0()
> > > maybe. Or maybe skl_dpll0_get_config to match the .get_config()
> > > nomenclature used for eg. encoders.
> > Hmm, I think the problem is that this have a mix of get_hw_state() and
> > get_config(). We actually have skl_ddi_dpll0_get_hw_state(), but it wouldn't
> > be
> > trivial to make that code reusable for the cdclk stuff.
> > 
> > Maybe it should be two separate functions, one that gets the hardware state
> > and
> > another for choosing the vco.
> This function only exists to read the hw state.

Then get_hw_state() wouldn't be a bad name. The only difference between this and
the pll get_hw_state() hook is that the latter doesn't set a vco value, since
that is not directly programmed.

> 
> > 
> > Then
> > 
> >   vco = skl_cdclk_vco(skl_dpll0_get_hw_state());
> > 
> > directly in skl_get_cdclk() ? I'm not really sure, maybe just leave the way
> > it
> > is for now.
> > 
> > > 
> > >  Though if we go there then
> > > .get_cdclk() might need to be renamed to .cdclk_get_config or something.
> > .cdclk_get_hw_state() ?
> .get_hw_state() is the thing which tells us if something is enabled,
> .get_config() is the thing that actually reads out the full state. Or at
> least that's how it used to be.

For encoders, get_hw_state() may also read what pipe it is in. For crtcs,
get_pipe_config() does both. PLLs get_hw_state() read the whole config too. I
guess there's room for making these more consistent. I'd go for is_enabled() for
something that is only supposed to say if something is enabled. :)

Ander