On Tue, Oct 17, 2017 at 04:23:07PM -0700, Rodrigo Vivi wrote:
> On Tue, Oct 17, 2017 at 08:36:14PM +0000, Ville Syrjälä wrote:
> > On Tue, Oct 17, 2017 at 09:02:05PM +0300, Ville Syrjälä wrote:
> > > On Tue, Oct 17, 2017 at 10:45:22AM -0700, Rodrigo Vivi wrote:
> > > > On Tue, Oct 17, 2017 at 05:23:20PM +0000, Ville Syrjälä wrote:
> > > > > On Tue, Oct 17, 2017 at 09:47:05AM -0700, Rodrigo Vivi wrote:
> > > > > > On Tue, Oct 17, 2017 at 03:44:21PM +0000, Ville Syrjälä wrote:
> > > > > > > On Tue, Oct 03, 2017 at 12:06:08AM -0700, Rodrigo Vivi wrote:
> > > > > > > > From: "Kahola, Mika" <mika.kah...@intel.com>
> > > > > > > >
> > > > > > > > Display Voltage and Frequency Switching (DVFS) is used to
> > > > > > > > adjust the
> > > > > > > > display voltage to match the display clock frequencies. If
> > > > > > > > voltage is
> > > > > > > > set too low, it will break functionality. If voltage is set too
> > > > > > > > high,
> > > > > > > > it will waste power. Voltage level is selected based on CD
> > > > > > > > clock and
> > > > > > > > DDI clock.
> > > > > > > >
> > > > > > > > The sequence before frequency change is the following and it
> > > > > > > > requests
> > > > > > > > the power controller to raise voltage to maximum
> > > > > > > >
> > > > > > > > - Ensure any previous GT Driver Mailbox transaction is complete.
> > > > > > > > - Write GT Driver Mailbox Data Low = 0x3.
> > > > > > > > - Write GT Driver Mailbox Data High = 0x0.
> > > > > > > > - Write GT Driver Mailbox Interface = 0x80000007.
> > > > > > > > - Poll GT Driver Mailbox Interface for Run/Busy indication
> > > > > > > > cleared (bit 31 = 0).
> > > > > > > > - Read GT Driver Mailbox Data Low, if bit 0 is 0x1, continue,
> > > > > > > > else restart the sequence.
> > > > > > > > Timeout after 3ms
> > > > > > > >
> > > > > > > > The sequence after frequency change is the following and it
> > > > > > > > requests
> > > > > > > > the port controller to raise voltage to the requested level.
> > > > > > > >
> > > > > > > > - Write GT Driver Mailbox Data Low
> > > > > > > > * For level 0, write 0x0
> > > > > > > > * For level 1, write 0x1
> > > > > > > > * For level 2, write 0x2
> > > > > > > > * For level 3, write 0x3
> > > > > > > > - Write GT Driver Mailbox Data High = 0x0.
> > > > > > > > - Write GT Driver Mailbox Interface = 0x80000007.
> > > > > > > >
> > > > > > > > For Cannonlake, the level 3 is not used and it aliases to level
> > > > > > > > 2.
> > > > > > > >
> > > > > > > > v2: reuse Paulo's work on cdclk. This patch depends on Paulo's
> > > > > > > > patch
> > > > > > > > [PATCH 02/12] drm/i915/cnl: extract
> > > > > > > > cnl_dvfs_{pre,post}_change
> > > > > > > > v3: (By Rodrigo): Remove duplicated commend and fix typo on
> > > > > > > > Paulo's name.
> > > > > > > > v4: (By Rodrigo): Rebase on top "Unify and export gen9+
> > > > > > > > port_clock calculation"
> > > > > > > > The port clock calculation here was only addressing DP, so
> > > > > > > > let's reuse
> > > > > > > > the current port calculation that is already in place
> > > > > > > > without any duplication.
> > > > > > > > Alos fix portclk <= 594000 instead of portclk < 594000.
> > > > > > > >
> > > > > > > > Cc: Paulo Zanoni <paulo.r.zan...@intel.com>
> > > > > > > > Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > > > > > > > Signed-off-by: Kahola, Mika <mika.kah...@intel.com>
> > > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.v...@intel.com>
> > > > > > > > ---
> > > > > > > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 30
> > > > > > > > ++++++++++++++++++++++--------
> > > > > > > > 1 file changed, 22 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > > > index a2a3d93d67bd..6030fbafa580 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > > > @@ -1966,10 +1966,23 @@ static const struct intel_dpll_mgr
> > > > > > > > bxt_pll_mgr = {
> > > > > > > > .dump_hw_state = bxt_dump_hw_state,
> > > > > > > > };
> > > > > > > >
> > > > > > > > +static int cnl_get_dvfs_level(int cdclk, int portclk)
> > > > > > > > +{
> > > > > > > > + if (cdclk == 168000 && portclk <= 594000)
> > > > > > > > + return 0;
> > > > > > > > + else if (cdclk == 336000 && portclk <= 594000)
> > > > > > > > + return 1;
> > > > > > > > + else
> > > > > > > > + return 2;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > static void cnl_ddi_pll_enable(struct drm_i915_private
> > > > > > > > *dev_priv,
> > > > > > > > struct intel_shared_dpll *pll)
> > > > > > > > {
> > > > > > > > uint32_t val;
> > > > > > > > + int ret;
> > > > > > > > + int level;
> > > > > > > > + int cdclk, portclk;
> > > > > > > >
> > > > > > > > /* 1. Enable DPLL power in DPLL_ENABLE. */
> > > > > > > > val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > > > > > > @@ -2006,11 +2019,9 @@ static void cnl_ddi_pll_enable(struct
> > > > > > > > drm_i915_private *dev_priv,
> > > > > > > > /*
> > > > > > > > * 5. If the frequency will result in a change to the
> > > > > > > > voltage
> > > > > > > > * requirement, follow the Display Voltage Frequency
> > > > > > > > Switching
> > > > > > > > - * Sequence Before Frequency Change
> > > > > > > > - *
> > > > > > > > - * FIXME: (DVFS) is used to adjust the display voltage
> > > > > > > > to match the
> > > > > > > > - * display clock frequencies
> > > > > > > > + * (DVFS) Sequence Before Frequency Change
> > > > > > > > */
> > > > > > > > + ret = cnl_dvfs_pre_change(dev_priv);
> > > > > > > >
> > > > > > > > /* 6. Enable DPLL in DPLL_ENABLE. */
> > > > > > > > val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > > > > > > @@ -2028,11 +2039,14 @@ static void cnl_ddi_pll_enable(struct
> > > > > > > > drm_i915_private *dev_priv,
> > > > > > > > /*
> > > > > > > > * 8. If the frequency will result in a change to the
> > > > > > > > voltage
> > > > > > > > * requirement, follow the Display Voltage Frequency
> > > > > > > > Switching
> > > > > > > > - * Sequence After Frequency Change
> > > > > > > > - *
> > > > > > > > - * FIXME: (DVFS) is used to adjust the display voltage
> > > > > > > > to match the
> > > > > > > > - * display clock frequencies
> > > > > > > > + * (DVFS) Sequence After Frequency Change
> > > > > > > > */
> > > > > > > > + if (ret == 0) {
> > > > > > > > + cdclk = dev_priv->cdclk.hw.cdclk;
> > > > > > > > + portclk = intel_ddi_port_clock(dev_priv,
> > > > > > > > pll->id);
> > > > > > > > + level = cnl_get_dvfs_level(cdclk, portclk);
> > > > > > > > + cnl_dvfs_post_change(dev_priv, level);
> > > > > > >
> > > > > > > This isn't how I imagined we'd handle this. What I was after is
> > > > > > > pre-computing the correct "dfvfs level" (or what I'd probably
> > > > > > > just call
> > > > > > > "voltage" or something like that), and then we'd just let the
> > > > > > > normal
> > > > > > > cdclk programming sequence do its thing.
> > > > > > >
> > > > > > > Is it even safe to do this with other pipes/ports enabled? I would
> > > > > > > have assumed no, but maybe I'm mistaken?
> > > > > >
> > > > > > Well, this is how the spec is written. So I assume it is safe to
> > > > > > change
> > > > > > that at that point.
> > > > > >
> > > > > > Also we enable CDCLK during boot while we have no way to compute the
> > > > > > link rate. So I don't see a better way of hadling that.
> > > > >
> > > > > We do a full state readout, which should tell us what voltage we need.
> > > > > And if the cdclk isn't even enabled, then obviously we can't have any
> > > > > pipes running either, so we can safely enable it with any frequency
> > > > > even before the full state readout.
> > > > >
> > > > > > So, Art is it ok in the way that I'm proposing here:
> > > > > >
> > > > > > on core display init:
> > > > > >
> > > > > > - dvfs-pre;
> > > > > > - enable cdclk;
> > > > > > - dvfs-post considering cdclk only and link rate 0;
> > > > > >
> > > > > > on pll init:
> > > > > >
> > > > > > - consider cdclk and link rate and if desired level is different
> > > > > > from what is currently set: dvfs-pre
> > > > > > - enable pll
> > > > > > - consider cdclk and link rate and if desired level is different
> > > > > > from what is currently set: dvfs-post with new level.
> > > > > >
> > > > > > This is how I read the spec, but please let me know what I'm
> > > > > > missing.
> > > > >
> > > > > Hmm. OK, so maybe we can change the voltage while things are up and
> > > > > running.
> > > > >
> > > > > That said, the voltage level is a global thing, so whenever we change
> > > > > it
> > > > > we must consider the state of all ports, and the cdclk. This looks
> > > > > like
> > > > > it only considers the current port. Thus if we first enable a port
> > > > > that
> > > > > needs high voltage, and later a port that can make due with a lower
> > > > > voltage we'll end up with something not working.
> > > >
> > > > ouch! this is true! So, what link rate should we consider on that dvfs
> > > > spec?
> > > > so, always the highest avaiblable at that time?
> > >
> > > Yes.
> > >
> > > >
> > > > So we cache somewhere the highest on atomic check? and then during
> > > > these dvfs sequences we just check for the highest?
> > >
> > > IIRC I pastebinned a sketch for this at some point. Hmm. Found it, and
> > > pushed it here:
> > > git://github.com/vsyrjala/linux.git dvfs_voltage_2
> > >
> > > I tried to also pimp it a little bit to not force a modeset on all
> > > pipes if just the voltage changes.
> > >
> > > I *think* this should work, after someone fills out the code for
> > > DDI .compute_config() and .get_config(). But totally untested of course
> > > so YMMV.
> >
> > Except I forgot about the lack of a pcode command to read out the current
> > voltage setting :( So I had to add some extra kludges to the state readout
> > on top. Branch updated.
>
> hmmm... thanks!
>
> ok, we need to cache something somewhere.
>
> But for me it seems a bit hard to associate with the port clock on the pll
> still.
All that should be needed with my branch is set the crtc state
min_voltage based on port_clock. No need to think about PLLS.
>
> So I wonder if we should cache the link_rate directly into dpll_hw_state,
> on same places we set cfgcr0 values...
I wouldn't comlicate this by involving PLLs.
> so whenever we need to figure out the minimal level we do a
> for_each_pll
> max_link_clock = max(max_link_clock, pll->hw_state->link_clock)
> cnl_calc_voltage(cdclk, max_link_clock)a
>
> or actually move this for inside cnl_calc_voltage(cdclk).
>
> and also I don't believe we need the full modeset to change the voltage level.
That shouldn't happen with the latest stuff I pushed to the branch.
> I'd like to stay as close as possible to the spec sequence.
That would mean more code paths to do the same thing, which means
they'll accumulate different bugs. Not something I would cherish.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
