[PATCH] drm/i915: Stash desired logical vco in a reliable place.

[PATCH] drm/i915: Stash desired logical vco in a reliable place.

[Rodrigo Vivi]

On intel_dp_compute_config() we calculate the needed vco
for eDP on gen9 and we stash 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.

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

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 83da50b13d81..be066afaefa7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1946,7 +1946,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 			break;
 		}
 
-		to_intel_atomic_state(pipe_config->base.state)->cdclk.logical.vco = vco;
+		dev_priv->cdclk.logical.vco = vco;
 	}
 
 	if (!HAS_DDI(dev_priv))
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Stash desired logical vco in a reliable place.

[Patchwork]

== Series Details ==

Series: drm/i915: Stash desired logical vco in a reliable place.
URL   : https://patchwork.freedesktop.org/series/42537/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4117 -> Patchwork_8864 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_8864 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8864, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42537/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8864:

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_chamelium@dp-edid-read:
      fi-kbl-7500u:       PASS -> FAIL

    
== Known issues ==

  Here are the changes found in Patchwork_8864 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)

    
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713


== Participating hosts (37 -> 34) ==

  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4117 -> Patchwork_8864

  CI_DRM_4117: 844dd95837ab995c37d1139d74ff55139987b437 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4454: 5baea95fc3abaedff3feb0f96ce29b995f0e301d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8864: 19d62881c5a3cb8b066a889d4a1d4d5f82829cc3 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4454: d0a0bca2194a673c4d9a70a2256837c59213c64b @ git://anongit.freedesktop.org/piglit


== Linux commits ==

19d62881c5a3 drm/i915: Stash desired logical vco in a reliable place.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8864/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Stash desired logical vco in a reliable place.

[Patchwork]

== Series Details ==

Series: drm/i915: Stash desired logical vco in a reliable place.
URL   : https://patchwork.freedesktop.org/series/42537/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4121 -> Patchwork_8871 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_8871 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8871, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42537/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8871:

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       PASS -> DMESG-FAIL

    
== Known issues ==

  Here are the changes found in Patchwork_8871 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-ivb-3520m:       PASS -> DMESG-WARN (fdo#106084)

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-ivb-3520m:       DMESG-WARN (fdo#106084) -> PASS

    
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#106084 https://bugs.freedesktop.org/show_bug.cgi?id=106084


== Participating hosts (40 -> 35) ==

  Missing    (5): fi-ctg-p8600 fi-glk-j4005 fi-kbl-7560u fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4121 -> Patchwork_8871

  CI_DRM_4121: d4f7520d80ab83ea9053ee080df09a23463b6966 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4456: 43761534c6482dc67b9c3d8eeecd425ef40b3c4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8871: 933e2d4d85cc49818ed47c556c6f6fb9553b8b25 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4456: 30b992bdc047073e1fe99b1ac622f026618a8081 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

933e2d4d85cc drm/i915: Stash desired logical vco in a reliable place.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8871/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Stash desired logical vco in a reliable place.

[Ville Syrjälä]

On Tue, May 01, 2018 at 05:26:37PM -0700, Rodrigo Vivi wrote:
> On intel_dp_compute_config() we calculate the needed vco
> for eDP on gen9 and we stash 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.

Hmm. Yeah, that's a problem. However we can't just overwrite
the current state like you propose in the middle of the atomic
check phase.

I guess what we could do is move the code from intel_dp_compute_config()
into skl_modeset_calc_cdclk(). That way we wouldn't have to add new
encoder hooks or anything like that. Something like this perhaps?

static int skl_dpll0_vco(struct intel_atomic_state *state)
{
	vco = state->cdclk.logical.vco;
	if (!vco)
		vco = dev_priv->skl_preferred_vco_freq;

	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
		if (!crtc_state->base.enable)
			continue;

		if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP))
			continue;
	
		<insert code from intel_dp_compute_config()>
	}

	return vco;
}

skl_modeset_calc_cdclk()
{
	...
-	vco = intel_state->cdclk.logical.vco;
-	if (!vco)
-		vco = dev_priv->skl_preferred_vco_freq;
+	vco = skl_dpll0_vco(intel_state);
	...
}

> 
> 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
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 83da50b13d81..be066afaefa7 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1946,7 +1946,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  			break;
>  		}
>  
> -		to_intel_atomic_state(pipe_config->base.state)->cdclk.logical.vco = vco;
> +		dev_priv->cdclk.logical.vco = vco;
>  	}
>  
>  	if (!HAS_DDI(dev_priv))
> -- 
> 2.13.6

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Stash desired logical vco in a reliable place.

[Patchwork]

== Series Details ==

Series: drm/i915: Stash desired logical vco in a reliable place.
URL   : https://patchwork.freedesktop.org/series/42537/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4121 -> Patchwork_8875 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_8875 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8875, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42537/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8875:

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       PASS -> DMESG-FAIL

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-elk-e7500:       PASS -> DMESG-FAIL

    
    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_8875 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-skl-guc:         PASS -> FAIL (fdo#103191)

    
    ==== Possible fixes ====

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      fi-gdg-551:         FAIL (fdo#102575) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-ivb-3520m:       DMESG-WARN (fdo#106084) -> PASS

    
  fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#106084 https://bugs.freedesktop.org/show_bug.cgi?id=106084


== Participating hosts (40 -> 37) ==

  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4121 -> Patchwork_8875

  CI_DRM_4121: d4f7520d80ab83ea9053ee080df09a23463b6966 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4456: 43761534c6482dc67b9c3d8eeecd425ef40b3c4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8875: 5806758f04c585f394ed485dd592ffdf2b7e380e @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4456: 30b992bdc047073e1fe99b1ac622f026618a8081 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

5806758f04c5 drm/i915: Stash desired logical vco in a reliable place.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8875/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Adjust eDP's logical vco in a reliable place.

[Rodrigo Vivi]

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;
+}
+
 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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Stash desired logical vco in a reliable place. (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Stash desired logical vco in a reliable place. (rev2)
URL   : https://patchwork.freedesktop.org/series/42537/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4123 -> Patchwork_8878 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8878 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8878, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42537/revisions/2/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8878:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_8878 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-hsw-4770r:       PASS -> FAIL (fdo#100368)

    
    ==== Possible fixes ====

    igt@kms_chamelium@dp-edid-read:
      fi-kbl-7500u:       FAIL (fdo#103841) -> PASS

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-cfl-s3:          FAIL (fdo#100368, fdo#103928) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-ivb-3520m:       DMESG-WARN (fdo#106084) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#106084 https://bugs.freedesktop.org/show_bug.cgi?id=106084


== Participating hosts (40 -> 37) ==

  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4123 -> Patchwork_8878

  CI_DRM_4123: cbb6a0aa933f3323a8deb331aca503b7388abc06 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4456: 43761534c6482dc67b9c3d8eeecd425ef40b3c4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8878: f1f1c4d8d875ec8ac2eaa6e9034e81213965269a @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4456: 30b992bdc047073e1fe99b1ac622f026618a8081 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

f1f1c4d8d875 drm/i915: Adjust eDP's logical vco in a reliable place.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8878/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Adjust eDP's logical vco in a reliable place.

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Stash desired logical vco in a reliable place. (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Stash desired logical vco in a reliable place. (rev2)
URL   : https://patchwork.freedesktop.org/series/42537/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4123_full -> Patchwork_8878_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8878_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8878_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42537/revisions/2/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8878_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-bsd1:
      shard-kbl:          PASS -> SKIP

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_8878_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@absolute-wf_vblank-interruptible:
      shard-glk:          PASS -> FAIL (fdo#106087)

    igt@kms_flip@plain-flip-fb-recreate-interruptible:
      shard-glk:          PASS -> FAIL (fdo#100368) +1

    igt@kms_setmode@basic:
      shard-hsw:          PASS -> FAIL (fdo#99912)

    igt@kms_sysfs_edid_timing:
      shard-apl:          PASS -> WARN (fdo#100047)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s3:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@kms_atomic_interruptible@universal-setplane-primary:
      shard-kbl:          DMESG-WARN (fdo#105602, fdo#103558) -> PASS +40

    igt@kms_flip@dpms-vs-vblank-race-interruptible:
      shard-glk:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@flip-vs-expired-vblank:
      shard-glk:          FAIL (fdo#105363) -> PASS

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#102887) -> PASS

    igt@kms_flip@flip-vs-wf_vblank-interruptible:
      shard-glk:          FAIL (fdo#100368) -> PASS +1

    igt@kms_flip@wf_vblank-ts-check-interruptible:
      shard-hsw:          FAIL (fdo#103928) -> PASS

    
  fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#106087 https://bugs.freedesktop.org/show_bug.cgi?id=106087
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (7 -> 7) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4123 -> Patchwork_8878

  CI_DRM_4123: cbb6a0aa933f3323a8deb331aca503b7388abc06 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4456: 43761534c6482dc67b9c3d8eeecd425ef40b3c4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8878: f1f1c4d8d875ec8ac2eaa6e9034e81213965269a @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4456: 30b992bdc047073e1fe99b1ac622f026618a8081 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8878/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Adjust eDP's logical vco in a reliable place.

[Rodrigo Vivi]

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

Re: [PATCH] drm/i915: Adjust eDP's logical vco in a reliable place.

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Adjust eDP's logical vco in a reliable place.

[Rodrigo Vivi]

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