| Message ID | 20180502175255.5344-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 02, 2018 at 10:52:55AM -0700, Rodrigo Vivi wrote: > On intel_dp_compute_config() we were calculating the needed vco > for eDP on gen9 and we stashing it in > intel_atomic_state.cdclk.logical.vco > > However few moments later on intel_modeset_checks() we fully > replace entire intel_atomic_state.cdclk.logical with > dev_priv->cdclk.logical fully overwriting the logical desired > vco for eDP on gen9. > > So, with wrong VCO value we end up with wrong desired cdclk, but > also it will raise a lot of WARNs: On gen9, when we read > CDCLK_CTL to verify if we configured properly the desired > frequency the CD Frequency Select bits [27:26] == 10b can mean > 337.5 or 308.57 MHz depending on the VCO. So if we have wrong > VCO value stashed we will believe the frequency selection didn't > stick and start to raise WARNs of cdclk mismatch. > > [ 42.857519] [drm:intel_dump_cdclk_state [i915]] Changing CDCLK to 308571 kHz, VCO 8640000 kHz, ref 24000 kHz, bypass 24000 kHz, voltage level 0 > [ 42.897269] cdclk state doesn't match! > [ 42.901052] WARNING: CPU: 5 PID: 1116 at drivers/gpu/drm/i915/intel_cdclk.c:2084 intel_set_cdclk+0x5d/0x110 [i915] > [ 42.938004] RIP: 0010:intel_set_cdclk+0x5d/0x110 [i915] > [ 43.155253] WARNING: CPU: 5 PID: 1116 at drivers/gpu/drm/i915/intel_cdclk.c:2084 intel_set_cdclk+0x5d/0x110 [i915] > [ 43.170277] [drm:intel_dump_cdclk_state [i915]] [hw state] 337500 kHz, VCO 8100000 kHz, ref 24000 kHz, bypass 24000 kHz, voltage level 0 > [ 43.182566] [drm:intel_dump_cdclk_state [i915]] [sw state] 308571 kHz, VCO 8640000 kHz, ref 24000 kHz, bypass 24000 kHz, voltage level 0 > > v2: Move the entire eDP's vco logical adjustment to inside > the skl_modeset_calc_cdclk as suggested by Ville. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_cdclk.c | 41 ++++++++++++++++++++++++++++++++++---- > drivers/gpu/drm/i915/intel_dp.c | 20 ------------------- > 2 files changed, 37 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c > index 32d24c69da3c..704ddb4d3ca7 100644 > --- a/drivers/gpu/drm/i915/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > @@ -2302,9 +2302,44 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) > return 0; > } > > +static int skl_dpll0_vco(struct intel_atomic_state *intel_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(intel_state->base.dev); > + struct intel_crtc *crtc; > + struct intel_crtc_state *crtc_state; > + int vco, i; > + > + vco = intel_state->cdclk.logical.vco; > + if (!vco) > + vco = dev_priv->skl_preferred_vco_freq; > + > + for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) { > + if (!crtc_state->base.enable) > + continue; > + > + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP)) > + continue; > + > + /* > + * DPLL0 VCO may need to be adjusted to get the correct > + * clock for eDP. This will affect cdclk as well. > + */ > + switch (crtc_state->port_clock / 2) { > + case 108000: > + case 216000: > + vco = 8640000; > + break; > + default: > + vco = 8100000; > + break; > + } > + } > + > + return vco; > +} Yep. I think this should do the right thing. For the times when the pipe driving eDP isn't part of the atomic state we wil simply preserve the current vco setting. That means other pipes can't actually make a mess of the DPLL0 VCO while eDP depends on it. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + > static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) > { > - struct drm_i915_private *dev_priv = to_i915(state->dev); > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > int min_cdclk, cdclk, vco; > > @@ -2312,9 +2347,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) > if (min_cdclk < 0) > return min_cdclk; > > - vco = intel_state->cdclk.logical.vco; > - if (!vco) > - vco = dev_priv->skl_preferred_vco_freq; > + vco = skl_dpll0_vco(intel_state); > > /* > * FIXME should also account for plane ratio > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 83da50b13d81..dde92e4af5d3 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1929,26 +1929,6 @@ intel_dp_compute_config(struct intel_encoder *encoder, > reduce_m_n); > } > > - /* > - * DPLL0 VCO may need to be adjusted to get the correct > - * clock for eDP. This will affect cdclk as well. > - */ > - if (intel_dp_is_edp(intel_dp) && IS_GEN9_BC(dev_priv)) { > - int vco; > - > - switch (pipe_config->port_clock / 2) { > - case 108000: > - case 216000: > - vco = 8640000; > - break; > - default: > - vco = 8100000; > - break; > - } > - > - to_intel_atomic_state(pipe_config->base.state)->cdclk.logical.vco = vco; > - } > - > if (!HAS_DDI(dev_priv)) > intel_dp_set_clock(encoder, pipe_config); > > -- > 2.13.6
On Wed, May 02, 2018 at 10:12:51PM +0300, Ville Syrjälä wrote: > On Wed, May 02, 2018 at 10:52:55AM -0700, Rodrigo Vivi wrote: > > On intel_dp_compute_config() we were calculating the needed vco > > for eDP on gen9 and we stashing it in > > intel_atomic_state.cdclk.logical.vco > > > > However few moments later on intel_modeset_checks() we fully > > replace entire intel_atomic_state.cdclk.logical with > > dev_priv->cdclk.logical fully overwriting the logical desired > > vco for eDP on gen9. > > > > So, with wrong VCO value we end up with wrong desired cdclk, but > > also it will raise a lot of WARNs: On gen9, when we read > > CDCLK_CTL to verify if we configured properly the desired > > frequency the CD Frequency Select bits [27:26] == 10b can mean > > 337.5 or 308.57 MHz depending on the VCO. So if we have wrong > > VCO value stashed we will believe the frequency selection didn't > > stick and start to raise WARNs of cdclk mismatch. > > > > [ 42.857519] [drm:intel_dump_cdclk_state [i915]] Changing CDCLK to 308571 kHz, VCO 8640000 kHz, ref 24000 kHz, bypass 24000 kHz, voltage level 0 > > [ 42.897269] cdclk state doesn't match! > > [ 42.901052] WARNING: CPU: 5 PID: 1116 at drivers/gpu/drm/i915/intel_cdclk.c:2084 intel_set_cdclk+0x5d/0x110 [i915] > > [ 42.938004] RIP: 0010:intel_set_cdclk+0x5d/0x110 [i915] > > [ 43.155253] WARNING: CPU: 5 PID: 1116 at drivers/gpu/drm/i915/intel_cdclk.c:2084 intel_set_cdclk+0x5d/0x110 [i915] > > [ 43.170277] [drm:intel_dump_cdclk_state [i915]] [hw state] 337500 kHz, VCO 8100000 kHz, ref 24000 kHz, bypass 24000 kHz, voltage level 0 > > [ 43.182566] [drm:intel_dump_cdclk_state [i915]] [sw state] 308571 kHz, VCO 8640000 kHz, ref 24000 kHz, bypass 24000 kHz, voltage level 0 > > > > v2: Move the entire eDP's vco logical adjustment to inside > > the skl_modeset_calc_cdclk as suggested by Ville. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_cdclk.c | 41 ++++++++++++++++++++++++++++++++++---- > > drivers/gpu/drm/i915/intel_dp.c | 20 ------------------- > > 2 files changed, 37 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c > > index 32d24c69da3c..704ddb4d3ca7 100644 > > --- a/drivers/gpu/drm/i915/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > > @@ -2302,9 +2302,44 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) > > return 0; > > } > > > > +static int skl_dpll0_vco(struct intel_atomic_state *intel_state) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(intel_state->base.dev); > > + struct intel_crtc *crtc; > > + struct intel_crtc_state *crtc_state; > > + int vco, i; > > + > > + vco = intel_state->cdclk.logical.vco; > > + if (!vco) > > + vco = dev_priv->skl_preferred_vco_freq; > > + > > + for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) { > > + if (!crtc_state->base.enable) > > + continue; > > + > > + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP)) > > + continue; > > + > > + /* > > + * DPLL0 VCO may need to be adjusted to get the correct > > + * clock for eDP. This will affect cdclk as well. > > + */ > > + switch (crtc_state->port_clock / 2) { > > + case 108000: > > + case 216000: > > + vco = 8640000; > > + break; > > + default: > > + vco = 8100000; > > + break; > > + } > > + } > > + > > + return vco; > > +} > > Yep. I think this should do the right thing. For the times when the pipe > driving eDP isn't part of the atomic state we wil simply preserve the > current vco setting. That means other pipes can't actually make a mess > of the DPLL0 VCO while eDP depends on it. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks for the suggestions. So, on setting the examples of what *not* to do: I forgot to include Fixes: and cc stable. Luckily I remembered before pushing. So, Fixes: bb0f4aab0e76 ("drm/i915: Track full cdclk state for the logical and actual cdclk frequencies") Cc: <stable@vger.kernel.org> # v4.12+ seems right for you? > > > + > > static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) > > { > > - struct drm_i915_private *dev_priv = to_i915(state->dev); > > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > > int min_cdclk, cdclk, vco; > > > > @@ -2312,9 +2347,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) > > if (min_cdclk < 0) > > return min_cdclk; > > > > - vco = intel_state->cdclk.logical.vco; > > - if (!vco) > > - vco = dev_priv->skl_preferred_vco_freq; > > + vco = skl_dpll0_vco(intel_state); > > > > /* > > * FIXME should also account for plane ratio > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 83da50b13d81..dde92e4af5d3 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1929,26 +1929,6 @@ intel_dp_compute_config(struct intel_encoder *encoder, > > reduce_m_n); > > } > > > > - /* > > - * DPLL0 VCO may need to be adjusted to get the correct > > - * clock for eDP. This will affect cdclk as well. > > - */ > > - if (intel_dp_is_edp(intel_dp) && IS_GEN9_BC(dev_priv)) { > > - int vco; > > - > > - switch (pipe_config->port_clock / 2) { > > - case 108000: > > - case 216000: > > - vco = 8640000; > > - break; > > - default: > > - vco = 8100000; > > - break; > > - } > > - > > - to_intel_atomic_state(pipe_config->base.state)->cdclk.logical.vco = vco; > > - } > > - > > if (!HAS_DDI(dev_priv)) > > intel_dp_set_clock(encoder, pipe_config); > > > > -- > > 2.13.6 > > -- > Ville Syrjälä > Intel
On Thu, May 03, 2018 at 06:07:45AM -0700, Rodrigo Vivi wrote: > On Wed, May 02, 2018 at 10:12:51PM +0300, Ville Syrjälä wrote: > > On Wed, May 02, 2018 at 10:52:55AM -0700, Rodrigo Vivi wrote: > > > On intel_dp_compute_config() we were calculating the needed vco > > > for eDP on gen9 and we stashing it in > > > intel_atomic_state.cdclk.logical.vco > > > > > > However few moments later on intel_modeset_checks() we fully > > > replace entire intel_atomic_state.cdclk.logical with > > > dev_priv->cdclk.logical fully overwriting the logical desired > > > vco for eDP on gen9. > > > > > > So, with wrong VCO value we end up with wrong desired cdclk, but > > > also it will raise a lot of WARNs: On gen9, when we read > > > CDCLK_CTL to verify if we configured properly the desired > > > frequency the CD Frequency Select bits [27:26] == 10b can mean > > > 337.5 or 308.57 MHz depending on the VCO. So if we have wrong > > > VCO value stashed we will believe the frequency selection didn't > > > stick and start to raise WARNs of cdclk mismatch. > > > > > > [ 42.857519] [drm:intel_dump_cdclk_state [i915]] Changing CDCLK to 308571 kHz, VCO 8640000 kHz, ref 24000 kHz, bypass 24000 kHz, voltage level 0 > > > [ 42.897269] cdclk state doesn't match! > > > [ 42.901052] WARNING: CPU: 5 PID: 1116 at drivers/gpu/drm/i915/intel_cdclk.c:2084 intel_set_cdclk+0x5d/0x110 [i915] > > > [ 42.938004] RIP: 0010:intel_set_cdclk+0x5d/0x110 [i915] > > > [ 43.155253] WARNING: CPU: 5 PID: 1116 at drivers/gpu/drm/i915/intel_cdclk.c:2084 intel_set_cdclk+0x5d/0x110 [i915] > > > [ 43.170277] [drm:intel_dump_cdclk_state [i915]] [hw state] 337500 kHz, VCO 8100000 kHz, ref 24000 kHz, bypass 24000 kHz, voltage level 0 > > > [ 43.182566] [drm:intel_dump_cdclk_state [i915]] [sw state] 308571 kHz, VCO 8640000 kHz, ref 24000 kHz, bypass 24000 kHz, voltage level 0 > > > > > > v2: Move the entire eDP's vco logical adjustment to inside > > > the skl_modeset_calc_cdclk as suggested by Ville. > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_cdclk.c | 41 ++++++++++++++++++++++++++++++++++---- > > > drivers/gpu/drm/i915/intel_dp.c | 20 ------------------- > > > 2 files changed, 37 insertions(+), 24 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c > > > index 32d24c69da3c..704ddb4d3ca7 100644 > > > --- a/drivers/gpu/drm/i915/intel_cdclk.c > > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > > > @@ -2302,9 +2302,44 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) > > > return 0; > > > } > > > > > > +static int skl_dpll0_vco(struct intel_atomic_state *intel_state) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(intel_state->base.dev); > > > + struct intel_crtc *crtc; > > > + struct intel_crtc_state *crtc_state; > > > + int vco, i; > > > + > > > + vco = intel_state->cdclk.logical.vco; > > > + if (!vco) > > > + vco = dev_priv->skl_preferred_vco_freq; > > > + > > > + for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) { > > > + if (!crtc_state->base.enable) > > > + continue; > > > + > > > + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP)) > > > + continue; > > > + > > > + /* > > > + * DPLL0 VCO may need to be adjusted to get the correct > > > + * clock for eDP. This will affect cdclk as well. > > > + */ > > > + switch (crtc_state->port_clock / 2) { > > > + case 108000: > > > + case 216000: > > > + vco = 8640000; > > > + break; > > > + default: > > > + vco = 8100000; > > > + break; > > > + } > > > + } > > > + > > > + return vco; > > > +} > > > > Yep. I think this should do the right thing. For the times when the pipe > > driving eDP isn't part of the atomic state we wil simply preserve the > > current vco setting. That means other pipes can't actually make a mess > > of the DPLL0 VCO while eDP depends on it. > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Thanks for the suggestions. > > So, on setting the examples of what *not* to do: I forgot to include > Fixes: and cc stable. > > Luckily I remembered before pushing. So, > > Fixes: bb0f4aab0e76 ("drm/i915: Track full cdclk state for the logical and actual cdclk frequencies") > Cc: <stable@vger.kernel.org> # v4.12+ > > seems right for you? Yeah, that looks like the one to blame. > > > > > > + > > > static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) > > > { > > > - struct drm_i915_private *dev_priv = to_i915(state->dev); > > > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > > > int min_cdclk, cdclk, vco; > > > > > > @@ -2312,9 +2347,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) > > > if (min_cdclk < 0) > > > return min_cdclk; > > > > > > - vco = intel_state->cdclk.logical.vco; > > > - if (!vco) > > > - vco = dev_priv->skl_preferred_vco_freq; > > > + vco = skl_dpll0_vco(intel_state); > > > > > > /* > > > * FIXME should also account for plane ratio > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > index 83da50b13d81..dde92e4af5d3 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -1929,26 +1929,6 @@ intel_dp_compute_config(struct intel_encoder *encoder, > > > reduce_m_n); > > > } > > > > > > - /* > > > - * DPLL0 VCO may need to be adjusted to get the correct > > > - * clock for eDP. This will affect cdclk as well. > > > - */ > > > - if (intel_dp_is_edp(intel_dp) && IS_GEN9_BC(dev_priv)) { > > > - int vco; > > > - > > > - switch (pipe_config->port_clock / 2) { > > > - case 108000: > > > - case 216000: > > > - vco = 8640000; > > > - break; > > > - default: > > > - vco = 8100000; > > > - break; > > > - } > > > - > > > - to_intel_atomic_state(pipe_config->base.state)->cdclk.logical.vco = vco; > > > - } > > > - > > > if (!HAS_DDI(dev_priv)) > > > intel_dp_set_clock(encoder, pipe_config); > > > > > > -- > > > 2.13.6 > > > > -- > > Ville Syrjälä > > Intel
On Thu, May 03, 2018 at 04:17:24PM +0300, Ville Syrjälä wrote: > On Thu, May 03, 2018 at 06:07:45AM -0700, Rodrigo Vivi wrote: > > On Wed, May 02, 2018 at 10:12:51PM +0300, Ville Syrjälä wrote: > > > On Wed, May 02, 2018 at 10:52:55AM -0700, Rodrigo Vivi wrote: > > > > On intel_dp_compute_config() we were calculating the needed vco > > > > for eDP on gen9 and we stashing it in > > > > intel_atomic_state.cdclk.logical.vco > > > > > > > > However few moments later on intel_modeset_checks() we fully > > > > replace entire intel_atomic_state.cdclk.logical with > > > > dev_priv->cdclk.logical fully overwriting the logical desired > > > > vco for eDP on gen9. > > > > > > > > So, with wrong VCO value we end up with wrong desired cdclk, but > > > > also it will raise a lot of WARNs: On gen9, when we read > > > > CDCLK_CTL to verify if we configured properly the desired > > > > frequency the CD Frequency Select bits [27:26] == 10b can mean > > > > 337.5 or 308.57 MHz depending on the VCO. So if we have wrong > > > > VCO value stashed we will believe the frequency selection didn't > > > > stick and start to raise WARNs of cdclk mismatch. > > > > > > > > [ 42.857519] [drm:intel_dump_cdclk_state [i915]] Changing CDCLK to 308571 kHz, VCO 8640000 kHz, ref 24000 kHz, bypass 24000 kHz, voltage level 0 > > > > [ 42.897269] cdclk state doesn't match! > > > > [ 42.901052] WARNING: CPU: 5 PID: 1116 at drivers/gpu/drm/i915/intel_cdclk.c:2084 intel_set_cdclk+0x5d/0x110 [i915] > > > > [ 42.938004] RIP: 0010:intel_set_cdclk+0x5d/0x110 [i915] > > > > [ 43.155253] WARNING: CPU: 5 PID: 1116 at drivers/gpu/drm/i915/intel_cdclk.c:2084 intel_set_cdclk+0x5d/0x110 [i915] > > > > [ 43.170277] [drm:intel_dump_cdclk_state [i915]] [hw state] 337500 kHz, VCO 8100000 kHz, ref 24000 kHz, bypass 24000 kHz, voltage level 0 > > > > [ 43.182566] [drm:intel_dump_cdclk_state [i915]] [sw state] 308571 kHz, VCO 8640000 kHz, ref 24000 kHz, bypass 24000 kHz, voltage level 0 > > > > > > > > v2: Move the entire eDP's vco logical adjustment to inside > > > > the skl_modeset_calc_cdclk as suggested by Ville. > > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_cdclk.c | 41 ++++++++++++++++++++++++++++++++++---- > > > > drivers/gpu/drm/i915/intel_dp.c | 20 ------------------- > > > > 2 files changed, 37 insertions(+), 24 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c > > > > index 32d24c69da3c..704ddb4d3ca7 100644 > > > > --- a/drivers/gpu/drm/i915/intel_cdclk.c > > > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > > > > @@ -2302,9 +2302,44 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) > > > > return 0; > > > > } > > > > > > > > +static int skl_dpll0_vco(struct intel_atomic_state *intel_state) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(intel_state->base.dev); > > > > + struct intel_crtc *crtc; > > > > + struct intel_crtc_state *crtc_state; > > > > + int vco, i; > > > > + > > > > + vco = intel_state->cdclk.logical.vco; > > > > + if (!vco) > > > > + vco = dev_priv->skl_preferred_vco_freq; > > > > + > > > > + for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) { > > > > + if (!crtc_state->base.enable) > > > > + continue; > > > > + > > > > + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP)) > > > > + continue; > > > > + > > > > + /* > > > > + * DPLL0 VCO may need to be adjusted to get the correct > > > > + * clock for eDP. This will affect cdclk as well. > > > > + */ > > > > + switch (crtc_state->port_clock / 2) { > > > > + case 108000: > > > > + case 216000: > > > > + vco = 8640000; > > > > + break; > > > > + default: > > > > + vco = 8100000; > > > > + break; > > > > + } > > > > + } > > > > + > > > > + return vco; > > > > +} > > > > > > Yep. I think this should do the right thing. For the times when the pipe > > > driving eDP isn't part of the atomic state we wil simply preserve the > > > current vco setting. That means other pipes can't actually make a mess > > > of the DPLL0 VCO while eDP depends on it. > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Thanks for the suggestions. > > > > So, on setting the examples of what *not* to do: I forgot to include > > Fixes: and cc stable. > > > > Luckily I remembered before pushing. So, > > > > Fixes: bb0f4aab0e76 ("drm/i915: Track full cdclk state for the logical and actual cdclk frequencies") > > Cc: <stable@vger.kernel.org> # v4.12+ > > > > seems right for you? > > Yeah, that looks like the one to blame. pushed, thanks > > > > > > > > > > + > > > > static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) > > > > { > > > > - struct drm_i915_private *dev_priv = to_i915(state->dev); > > > > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > > > > int min_cdclk, cdclk, vco; > > > > > > > > @@ -2312,9 +2347,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) > > > > if (min_cdclk < 0) > > > > return min_cdclk; > > > > > > > > - vco = intel_state->cdclk.logical.vco; > > > > - if (!vco) > > > > - vco = dev_priv->skl_preferred_vco_freq; > > > > + vco = skl_dpll0_vco(intel_state); > > > > > > > > /* > > > > * FIXME should also account for plane ratio > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > > index 83da50b13d81..dde92e4af5d3 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -1929,26 +1929,6 @@ intel_dp_compute_config(struct intel_encoder *encoder, > > > > reduce_m_n); > > > > } > > > > > > > > - /* > > > > - * DPLL0 VCO may need to be adjusted to get the correct > > > > - * clock for eDP. This will affect cdclk as well. > > > > - */ > > > > - if (intel_dp_is_edp(intel_dp) && IS_GEN9_BC(dev_priv)) { > > > > - int vco; > > > > - > > > > - switch (pipe_config->port_clock / 2) { > > > > - case 108000: > > > > - case 216000: > > > > - vco = 8640000; > > > > - break; > > > > - default: > > > > - vco = 8100000; > > > > - break; > > > > - } > > > > - > > > > - to_intel_atomic_state(pipe_config->base.state)->cdclk.logical.vco = vco; > > > > - } > > > > - > > > > if (!HAS_DDI(dev_priv)) > > > > intel_dp_set_clock(encoder, pipe_config); > > > > > > > > -- > > > > 2.13.6 > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c index 32d24c69da3c..704ddb4d3ca7 100644 --- a/drivers/gpu/drm/i915/intel_cdclk.c +++ b/drivers/gpu/drm/i915/intel_cdclk.c @@ -2302,9 +2302,44 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) return 0; } +static int skl_dpll0_vco(struct intel_atomic_state *intel_state) +{ + struct drm_i915_private *dev_priv = to_i915(intel_state->base.dev); + struct intel_crtc *crtc; + struct intel_crtc_state *crtc_state; + int vco, i; + + vco = intel_state->cdclk.logical.vco; + if (!vco) + vco = dev_priv->skl_preferred_vco_freq; + + for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) { + if (!crtc_state->base.enable) + continue; + + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP)) + continue; + + /* + * DPLL0 VCO may need to be adjusted to get the correct + * clock for eDP. This will affect cdclk as well. + */ + switch (crtc_state->port_clock / 2) { + case 108000: + case 216000: + vco = 8640000; + break; + default: + vco = 8100000; + break; + } + } + + return vco; +} + static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) { - struct drm_i915_private *dev_priv = to_i915(state->dev); struct intel_atomic_state *intel_state = to_intel_atomic_state(state); int min_cdclk, cdclk, vco; @@ -2312,9 +2347,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) if (min_cdclk < 0) return min_cdclk; - vco = intel_state->cdclk.logical.vco; - if (!vco) - vco = dev_priv->skl_preferred_vco_freq; + vco = skl_dpll0_vco(intel_state); /* * FIXME should also account for plane ratio diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 83da50b13d81..dde92e4af5d3 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1929,26 +1929,6 @@ intel_dp_compute_config(struct intel_encoder *encoder, reduce_m_n); } - /* - * DPLL0 VCO may need to be adjusted to get the correct - * clock for eDP. This will affect cdclk as well. - */ - if (intel_dp_is_edp(intel_dp) && IS_GEN9_BC(dev_priv)) { - int vco; - - switch (pipe_config->port_clock / 2) { - case 108000: - case 216000: - vco = 8640000; - break; - default: - vco = 8100000; - break; - } - - to_intel_atomic_state(pipe_config->base.state)->cdclk.logical.vco = vco; - } - if (!HAS_DDI(dev_priv)) intel_dp_set_clock(encoder, pipe_config);
On intel_dp_compute_config() we were calculating the needed vco for eDP on gen9 and we stashing it in intel_atomic_state.cdclk.logical.vco However few moments later on intel_modeset_checks() we fully replace entire intel_atomic_state.cdclk.logical with dev_priv->cdclk.logical fully overwriting the logical desired vco for eDP on gen9. So, with wrong VCO value we end up with wrong desired cdclk, but also it will raise a lot of WARNs: On gen9, when we read CDCLK_CTL to verify if we configured properly the desired frequency the CD Frequency Select bits [27:26] == 10b can mean 337.5 or 308.57 MHz depending on the VCO. So if we have wrong VCO value stashed we will believe the frequency selection didn't stick and start to raise WARNs of cdclk mismatch. [ 42.857519] [drm:intel_dump_cdclk_state [i915]] Changing CDCLK to 308571 kHz, VCO 8640000 kHz, ref 24000 kHz, bypass 24000 kHz, voltage level 0 [ 42.897269] cdclk state doesn't match! [ 42.901052] WARNING: CPU: 5 PID: 1116 at drivers/gpu/drm/i915/intel_cdclk.c:2084 intel_set_cdclk+0x5d/0x110 [i915] [ 42.938004] RIP: 0010:intel_set_cdclk+0x5d/0x110 [i915] [ 43.155253] WARNING: CPU: 5 PID: 1116 at drivers/gpu/drm/i915/intel_cdclk.c:2084 intel_set_cdclk+0x5d/0x110 [i915] [ 43.170277] [drm:intel_dump_cdclk_state [i915]] [hw state] 337500 kHz, VCO 8100000 kHz, ref 24000 kHz, bypass 24000 kHz, voltage level 0 [ 43.182566] [drm:intel_dump_cdclk_state [i915]] [sw state] 308571 kHz, VCO 8640000 kHz, ref 24000 kHz, bypass 24000 kHz, voltage level 0 v2: Move the entire eDP's vco logical adjustment to inside the skl_modeset_calc_cdclk as suggested by Ville. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_cdclk.c | 41 ++++++++++++++++++++++++++++++++++---- drivers/gpu/drm/i915/intel_dp.c | 20 ------------------- 2 files changed, 37 insertions(+), 24 deletions(-)