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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Ander Conselvan De Oliveira <conselvan2@gmail.com>
Cc: intel-gfx@lists.freedesktop.org, Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH v2 07/14] drm/i915: Start moving the cdclk stuff into a distinct state structure
Date: Fri, 23 Dec 2016 14:27:52 +0200	[thread overview]
Message-ID: <20161223122752.GR31595@intel.com> (raw)
In-Reply-To: <1482484162.2633.20.camel@gmail.com>

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@linux.intel.com wrote:
> > > > 
> > > > From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
> > > A couple more comments beloew, but either way,
> > > 
> > > Reviewed-by: Ander Conselvan de Oliveira <conselvan2@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
> 
>   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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx