| Message ID | 20171003070614.18396-8-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 03, 2017 at 12:06:08AM -0700, Rodrigo Vivi wrote: > From: "Kahola, Mika" <mika.kahola@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.zanoni@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Kahola, Mika <mika.kahola@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Looks good, dowble checked with the voltage levels from the spec. Reviewed-by: Manasi Navare <manasi.d.navare@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); > + } > > /* > * 9. turn on the clock for the DDI and map the DPLL to the DDI > -- > 2.13.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Oct 03, 2017 at 12:06:08AM -0700, Rodrigo Vivi wrote: > From: "Kahola, Mika" <mika.kahola@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.zanoni@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Kahola, Mika <mika.kahola@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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? > + } > > /* > * 9. turn on the clock for the DDI and map the DPLL to the DDI > -- > 2.13.5
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.kahola@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.zanoni@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Kahola, Mika <mika.kahola@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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. 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. Maybe I'm missing the link rate for eDP during core display init like we are currently kind of missing for SKL? > > > > + } > > > > /* > > * 9. turn on the clock for the DDI and map the DPLL to the DDI > > -- > > 2.13.5 > > -- > Ville Syrjälä > Intel OTC
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.kahola@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.zanoni@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Kahola, Mika <mika.kahola@intel.com> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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. We migth also end up changing the voltage multiple times if we enable multiple ports with one atomic operation. If things went via the normal cdclk path we'd just do the voltage change the once. Additonally I don't like that the code is getting the current clock by means of readout in the middle the modeset. With atomic the design is pretty much to pass all state in explicitly, and readout is only used to take over the from the BIOS, and to verify that we programmed the hardware correctly. > > Maybe I'm missing the link rate for eDP during core display init like we are currently kind of missing for SKL? Hmm. What exactly are we missing?
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.kahola@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.zanoni@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Kahola, Mika <mika.kahola@intel.com> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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? So we cache somewhere the highest on atomic check? and then during these dvfs sequences we just check for the highest? > > We migth also end up changing the voltage multiple times if we enable > multiple ports with one atomic operation. If things went via the normal > cdclk path we'd just do the voltage change the once. > > Additonally I don't like that the code is getting the current clock by > means of readout in the middle the modeset. With atomic the design is > pretty much to pass all state in explicitly, and readout is only used > to take over the from the BIOS, and to verify that we programmed the > hardware correctly. hmm... I thought the same recently when I was checking that skl Wa... So, we probably want to cache out link rates specifically on atomic cache? Or should we just move the functions to parse cfgcr1 on hw_state that was created during check phase? > > > > > Maybe I'm missing the link rate for eDP during core display init like we are currently kind of missing for SKL? > > Hmm. What exactly are we missing? I just sent a wa for skl... when doing that I saw that we are not following the spec for initialize the display in many aspects... but specially the dpll0 link rate... We consider the vco for eDP but not the accurate link rate... and then we update the dpll0 link rate during modeset. Apparently HW never expects we change dpll0 link rate on runtime like we are currently doing. But probably a discussion to move to that other patch... or not since all involve a link rate and possibly a cache of the value. > > -- > Ville Syrjälä > Intel OTC
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.kahola@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.zanoni@intel.com> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Signed-off-by: Kahola, Mika <mika.kahola@intel.com> > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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. > > > > > We migth also end up changing the voltage multiple times if we enable > > multiple ports with one atomic operation. If things went via the normal > > cdclk path we'd just do the voltage change the once. > > > > Additonally I don't like that the code is getting the current clock by > > means of readout in the middle the modeset. With atomic the design is > > pretty much to pass all state in explicitly, and readout is only used > > to take over the from the BIOS, and to verify that we programmed the > > hardware correctly. > > hmm... > I thought the same recently when I was checking that skl Wa... > So, we probably want to cache out link rates specifically on atomic cache? > Or should we just move the functions to parse cfgcr1 on hw_state that was > created during check phase? > > > > > > > > > Maybe I'm missing the link rate for eDP during core display init like we are currently kind of missing for SKL? > > > > Hmm. What exactly are we missing? > > I just sent a wa for skl... when doing that I saw that we are not following > the spec for initialize the display in many aspects... Not sure which part we're not following. > but specially the dpll0 > link rate... We consider the vco for eDP but not the accurate link rate... > and then we update the dpll0 link rate during modeset. Apparently HW never > expects we change dpll0 link rate on runtime like we are currently doing. We don't change it, except perhaps once if our initial guess was wrong. And that could only happen when eDP wasn't enabled during boot. After we've determined the link freq needed by eDP we should be sticking to the same DPLL0 VCO frequency. > > But probably a discussion to move to that other patch... or not since all > involve a link rate and possibly a cache of the value. > > > > > -- > > Ville Syrjälä > > Intel OTC
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.kahola@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.zanoni@intel.com> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Signed-off-by: Kahola, Mika <mika.kahola@intel.com> > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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.
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.kahola@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.zanoni@intel.com> > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > Signed-off-by: Kahola, Mika <mika.kahola@intel.com> > > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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. So I wonder if we should cache the link_rate directly into dpll_hw_state, on same places we set cfgcr0 values... 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) 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. I'd like to stay as close as possible to the spec sequence. we just need to check the minimal required with the one read from pcode or when we are writing or to be more atomic when calculating the future link rates we would also update the minimal level. same when disabling any pll we would iterate on all plls that are on and check new max link rate and consequently new min level. and set pll_hw_state->link_rate=0. > > -- > Ville Syrjälä > Intel OTC
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.kahola@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.zanoni@intel.com> > > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Signed-off-by: Kahola, Mika <mika.kahola@intel.com> > > > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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.
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); + } /* * 9. turn on the clock for the DDI and map the DPLL to the DDI