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@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 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
