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@xxxxxxxxxxxxxxx wrote:
> > > >
> > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxxxx>
> > > A couple more comments beloew, but either way,
> > >
> > > Reviewed-by: Ander Conselvan de Oliveira <conselvan2@xxxxxxxxx>
> > >
> > > >
> > > > ---
> > > > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
