[PATCH resend 0/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[PATCH resend 0/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Hans de Goede]

Hi All,

Here is a resend of my patch-set to deal with an
"transcoder A assertion failure (expected off, current on)"
error + WARN (and backtrace) seen on some Bay Trail devices with a DSI panel.

I've rebased it on the latest drm-intel-next, so this time around the CI
should be able to actually apply and test it, please review.

Regards,

Hans


Hans de Goede (2):
  drm/i915/display: Add a intel_pipe_is_enabled() helper
  drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use
    for non DP purposes

 drivers/gpu/drm/i915/display/intel_display.c | 20 ++++++++++++------
 drivers/gpu/drm/i915/display/intel_display.h |  2 ++
 drivers/gpu/drm/i915/display/intel_pps.c     | 22 ++++++++++++++++++++
 3 files changed, 38 insertions(+), 6 deletions(-)

-- 
2.30.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH resend 0/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Hans de Goede]

Hi All,

Here is a resend of my patch-set to deal with an
"transcoder A assertion failure (expected off, current on)"
error + WARN (and backtrace) seen on some Bay Trail devices with a DSI panel.

I've rebased it on the latest drm-intel-next, so this time around the CI
should be able to actually apply and test it, please review.

Regards,

Hans


Hans de Goede (2):
  drm/i915/display: Add a intel_pipe_is_enabled() helper
  drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use
    for non DP purposes

 drivers/gpu/drm/i915/display/intel_display.c | 20 ++++++++++++------
 drivers/gpu/drm/i915/display/intel_display.h |  2 ++
 drivers/gpu/drm/i915/display/intel_pps.c     | 22 ++++++++++++++++++++
 3 files changed, 38 insertions(+), 6 deletions(-)

-- 
2.30.1

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

[PATCH resend 1/2] drm/i915/display: Add a intel_pipe_is_enabled() helper

[Hans de Goede]

Factor the code to check if a pipe is currently enabled out of
assert_pipe() and put it in a new intel_pipe_is_enabled() helper,
so that it can be re-used without copy-pasting it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 20 ++++++++++++++------
 drivers/gpu/drm/i915/display/intel_display.h |  2 ++
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index e1060076ac83..9cb304aee285 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -442,17 +442,13 @@ void assert_panel_unlocked(struct drm_i915_private *dev_priv, enum pipe pipe)
 	     pipe_name(pipe));
 }
 
-void assert_pipe(struct drm_i915_private *dev_priv,
-		 enum transcoder cpu_transcoder, bool state)
+bool intel_pipe_is_enabled(struct drm_i915_private *dev_priv,
+			   enum transcoder cpu_transcoder)
 {
 	bool cur_state;
 	enum intel_display_power_domain power_domain;
 	intel_wakeref_t wakeref;
 
-	/* we keep both pipes enabled on 830 */
-	if (IS_I830(dev_priv))
-		state = true;
-
 	power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder);
 	wakeref = intel_display_power_get_if_enabled(dev_priv, power_domain);
 	if (wakeref) {
@@ -464,6 +460,18 @@ void assert_pipe(struct drm_i915_private *dev_priv,
 		cur_state = false;
 	}
 
+	return cur_state;
+}
+
+void assert_pipe(struct drm_i915_private *dev_priv,
+		 enum transcoder cpu_transcoder, bool state)
+{
+	bool cur_state = intel_pipe_is_enabled(dev_priv, cpu_transcoder);
+
+	/* we keep both pipes enabled on 830 */
+	if (IS_I830(dev_priv))
+		state = true;
+
 	I915_STATE_WARN(cur_state != state,
 			"transcoder %s assertion failure (expected %s, current %s)\n",
 			transcoder_name(cpu_transcoder),
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index 0e4c1481fa00..642cc87f3e38 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -533,6 +533,8 @@ void intel_enable_pipe(const struct intel_crtc_state *new_crtc_state);
 void intel_disable_pipe(const struct intel_crtc_state *old_crtc_state);
 void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
 void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
+bool intel_pipe_is_enabled(struct drm_i915_private *dev_priv,
+			   enum transcoder cpu_transcoder);
 enum pipe intel_crtc_pch_transcoder(struct intel_crtc *crtc);
 int vlv_get_hpll_vco(struct drm_i915_private *dev_priv);
 int vlv_get_cck_clock(struct drm_i915_private *dev_priv,
-- 
2.30.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH resend 1/2] drm/i915/display: Add a intel_pipe_is_enabled() helper

[Hans de Goede]

Factor the code to check if a pipe is currently enabled out of
assert_pipe() and put it in a new intel_pipe_is_enabled() helper,
so that it can be re-used without copy-pasting it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 20 ++++++++++++++------
 drivers/gpu/drm/i915/display/intel_display.h |  2 ++
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index e1060076ac83..9cb304aee285 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -442,17 +442,13 @@ void assert_panel_unlocked(struct drm_i915_private *dev_priv, enum pipe pipe)
 	     pipe_name(pipe));
 }
 
-void assert_pipe(struct drm_i915_private *dev_priv,
-		 enum transcoder cpu_transcoder, bool state)
+bool intel_pipe_is_enabled(struct drm_i915_private *dev_priv,
+			   enum transcoder cpu_transcoder)
 {
 	bool cur_state;
 	enum intel_display_power_domain power_domain;
 	intel_wakeref_t wakeref;
 
-	/* we keep both pipes enabled on 830 */
-	if (IS_I830(dev_priv))
-		state = true;
-
 	power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder);
 	wakeref = intel_display_power_get_if_enabled(dev_priv, power_domain);
 	if (wakeref) {
@@ -464,6 +460,18 @@ void assert_pipe(struct drm_i915_private *dev_priv,
 		cur_state = false;
 	}
 
+	return cur_state;
+}
+
+void assert_pipe(struct drm_i915_private *dev_priv,
+		 enum transcoder cpu_transcoder, bool state)
+{
+	bool cur_state = intel_pipe_is_enabled(dev_priv, cpu_transcoder);
+
+	/* we keep both pipes enabled on 830 */
+	if (IS_I830(dev_priv))
+		state = true;
+
 	I915_STATE_WARN(cur_state != state,
 			"transcoder %s assertion failure (expected %s, current %s)\n",
 			transcoder_name(cpu_transcoder),
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index 0e4c1481fa00..642cc87f3e38 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -533,6 +533,8 @@ void intel_enable_pipe(const struct intel_crtc_state *new_crtc_state);
 void intel_disable_pipe(const struct intel_crtc_state *old_crtc_state);
 void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
 void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
+bool intel_pipe_is_enabled(struct drm_i915_private *dev_priv,
+			   enum transcoder cpu_transcoder);
 enum pipe intel_crtc_pch_transcoder(struct intel_crtc *crtc);
 int vlv_get_hpll_vco(struct drm_i915_private *dev_priv);
 int vlv_get_cck_clock(struct drm_i915_private *dev_priv,
-- 
2.30.1

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

Re: [PATCH resend 1/2] drm/i915/display: Add a intel_pipe_is_enabled() helper

[Jani Nikula]

On Tue, 02 Mar 2021, Hans de Goede <hdegoede@redhat.com> wrote:
> Factor the code to check if a pipe is currently enabled out of
> assert_pipe() and put it in a new intel_pipe_is_enabled() helper,
> so that it can be re-used without copy-pasting it.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Does what it says on the box.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 20 ++++++++++++++------
>  drivers/gpu/drm/i915/display/intel_display.h |  2 ++
>  2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index e1060076ac83..9cb304aee285 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -442,17 +442,13 @@ void assert_panel_unlocked(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	     pipe_name(pipe));
>  }
>  
> -void assert_pipe(struct drm_i915_private *dev_priv,
> -		 enum transcoder cpu_transcoder, bool state)
> +bool intel_pipe_is_enabled(struct drm_i915_private *dev_priv,
> +			   enum transcoder cpu_transcoder)
>  {
>  	bool cur_state;
>  	enum intel_display_power_domain power_domain;
>  	intel_wakeref_t wakeref;
>  
> -	/* we keep both pipes enabled on 830 */
> -	if (IS_I830(dev_priv))
> -		state = true;
> -
>  	power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder);
>  	wakeref = intel_display_power_get_if_enabled(dev_priv, power_domain);
>  	if (wakeref) {
> @@ -464,6 +460,18 @@ void assert_pipe(struct drm_i915_private *dev_priv,
>  		cur_state = false;
>  	}
>  
> +	return cur_state;
> +}
> +
> +void assert_pipe(struct drm_i915_private *dev_priv,
> +		 enum transcoder cpu_transcoder, bool state)
> +{
> +	bool cur_state = intel_pipe_is_enabled(dev_priv, cpu_transcoder);
> +
> +	/* we keep both pipes enabled on 830 */
> +	if (IS_I830(dev_priv))
> +		state = true;
> +
>  	I915_STATE_WARN(cur_state != state,
>  			"transcoder %s assertion failure (expected %s, current %s)\n",
>  			transcoder_name(cpu_transcoder),
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index 0e4c1481fa00..642cc87f3e38 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -533,6 +533,8 @@ void intel_enable_pipe(const struct intel_crtc_state *new_crtc_state);
>  void intel_disable_pipe(const struct intel_crtc_state *old_crtc_state);
>  void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
>  void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
> +bool intel_pipe_is_enabled(struct drm_i915_private *dev_priv,
> +			   enum transcoder cpu_transcoder);
>  enum pipe intel_crtc_pch_transcoder(struct intel_crtc *crtc);
>  int vlv_get_hpll_vco(struct drm_i915_private *dev_priv);
>  int vlv_get_cck_clock(struct drm_i915_private *dev_priv,

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH resend 1/2] drm/i915/display: Add a intel_pipe_is_enabled() helper

[Jani Nikula]

On Tue, 02 Mar 2021, Hans de Goede <hdegoede@redhat.com> wrote:
> Factor the code to check if a pipe is currently enabled out of
> assert_pipe() and put it in a new intel_pipe_is_enabled() helper,
> so that it can be re-used without copy-pasting it.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Does what it says on the box.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 20 ++++++++++++++------
>  drivers/gpu/drm/i915/display/intel_display.h |  2 ++
>  2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index e1060076ac83..9cb304aee285 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -442,17 +442,13 @@ void assert_panel_unlocked(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	     pipe_name(pipe));
>  }
>  
> -void assert_pipe(struct drm_i915_private *dev_priv,
> -		 enum transcoder cpu_transcoder, bool state)
> +bool intel_pipe_is_enabled(struct drm_i915_private *dev_priv,
> +			   enum transcoder cpu_transcoder)
>  {
>  	bool cur_state;
>  	enum intel_display_power_domain power_domain;
>  	intel_wakeref_t wakeref;
>  
> -	/* we keep both pipes enabled on 830 */
> -	if (IS_I830(dev_priv))
> -		state = true;
> -
>  	power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder);
>  	wakeref = intel_display_power_get_if_enabled(dev_priv, power_domain);
>  	if (wakeref) {
> @@ -464,6 +460,18 @@ void assert_pipe(struct drm_i915_private *dev_priv,
>  		cur_state = false;
>  	}
>  
> +	return cur_state;
> +}
> +
> +void assert_pipe(struct drm_i915_private *dev_priv,
> +		 enum transcoder cpu_transcoder, bool state)
> +{
> +	bool cur_state = intel_pipe_is_enabled(dev_priv, cpu_transcoder);
> +
> +	/* we keep both pipes enabled on 830 */
> +	if (IS_I830(dev_priv))
> +		state = true;
> +
>  	I915_STATE_WARN(cur_state != state,
>  			"transcoder %s assertion failure (expected %s, current %s)\n",
>  			transcoder_name(cpu_transcoder),
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index 0e4c1481fa00..642cc87f3e38 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -533,6 +533,8 @@ void intel_enable_pipe(const struct intel_crtc_state *new_crtc_state);
>  void intel_disable_pipe(const struct intel_crtc_state *old_crtc_state);
>  void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
>  void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
> +bool intel_pipe_is_enabled(struct drm_i915_private *dev_priv,
> +			   enum transcoder cpu_transcoder);
>  enum pipe intel_crtc_pch_transcoder(struct intel_crtc *crtc);
>  int vlv_get_hpll_vco(struct drm_i915_private *dev_priv);
>  int vlv_get_cck_clock(struct drm_i915_private *dev_priv,

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Hans de Goede]

As explained by a long comment block, on VLV intel_setup_outputs()
sometimes thinks there might be an eDP panel connected while there is none.
In this case intel_setup_outputs() will call intel_dp_init() to check.

In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
even though this might be in use for non DP purposes. When this is the case
then the assert_pipe() in vlv_force_pll_on() will fail when called from
vlv_power_sequencer_kick().

This happens on a Voyo winpad A15, leading to the following WARN/backtrace:

[    8.661531] ------------[ cut here ]------------
[    8.661590] transcoder A assertion failure (expected off, current on)
[    8.661647] WARNING: CPU: 2 PID: 243 at drivers/gpu/drm/i915/display/intel_display.c:1288 assert_pipe+0x125/0xc20 [i915]
[    8.661822] Modules linked in: i915(E+) mmc_block crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_algo_bit drm_kms_helper cec drm drm_privacy_screen_helper video(E) sdhci_acpi sdhci i2c_hid pwm_lpss_platform pwm_lpss mmc_core i2c_dev fuse
[    8.661944] CPU: 2 PID: 243 Comm: systemd-udevd Tainted: G            E     5.11.0-rc5+ #228
[    8.661954] Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 5.6.5 11/20/2014
[    8.661961] RIP: 0010:assert_pipe+0x125/0xc20 [i915]
[    8.662050] Code: c7 c2 e5 39 4a c0 74 c9 48 c7 c6 53 3b 4a c0 83 fb 06 77 0a 89 db 48 8b 34 dd 80 38 45 c0 48 c7 c7 c8 ff 47 c0 e8 13 6c 8f df <0f> 0b e9 1d ff ff ff 89 db 48 8b 34 dd 80 38 45 c0 eb a0 48 c7 c2
[    8.662058] RSP: 0018:ffffa939c0557690 EFLAGS: 00010286
[    8.662071] RAX: 0000000000000039 RBX: 0000000000000000 RCX: ffff89c67bd19058
[    8.662078] RDX: 00000000ffffffd8 RSI: 0000000000000027 RDI: ffff89c67bd19050
[    8.662085] RBP: ffff89c64a3c0000 R08: 0000000000000001 R09: 0000000000000001
[    8.662091] R10: ffffa939c05574c0 R11: ffffffffa0961248 R12: 0000000000000009
[    8.662098] R13: 0000000000000000 R14: 00000000e0000000 R15: ffff89c64a3c0000
[    8.662105] FS:  00007fe824e42380(0000) GS:ffff89c67bd00000(0000) knlGS:0000000000000000
[    8.662113] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    8.662120] CR2: 00007fffdc770558 CR3: 0000000106ab8000 CR4: 00000000001006e0
[    8.662127] Call Trace:
[    8.662148]  assert_pipe+0xa9e/0xc20 [i915]
[    8.662252]  vlv_force_pll_on+0xfb/0x1b0 [i915]
[    8.662344]  intel_dp_sync_state+0xd92/0x2e70 [i915]
[    8.662448]  intel_dp_sync_state+0x1908/0x2e70 [i915]
[    8.662541]  intel_dp_sync_state+0x1a3e/0x2e70 [i915]
[    8.662620]  ? recalibrate_cpu_khz+0x10/0x10
[    8.662633]  ? ktime_get_with_offset+0xad/0x160
[    8.662658]  intel_dp_sync_state+0x1f21/0x2e70 [i915]
[    8.662788]  intel_dp_encoder_suspend+0x41f/0x14b0 [i915]
[    8.662875]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
[    8.662940]  ? __mutex_lock+0x7e/0x7a0
[    8.662950]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
[    8.662982]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
[    8.663025]  intel_dp_encoder_suspend+0xdf3/0x14b0 [i915]
[    8.663112]  ? find_held_lock+0x2b/0x80
[    8.663132]  drm_dp_dpcd_access+0x62/0xf0 [drm_kms_helper]
[    8.663181]  drm_dp_dpcd_read+0xb6/0xf0 [drm_kms_helper]
[    8.663223]  drm_dp_read_dpcd_caps+0x20/0x110 [drm_kms_helper]
[    8.663262]  intel_dp_init_connector+0x79e/0x1010 [i915]
[    8.663366]  intel_dp_init+0x251/0x480 [i915]
[    8.663453]  intel_modeset_init_nogem+0x1998/0x1b70 [i915]
[    8.663540]  ? intel_pcode_init+0x3b6b/0x5d60 [i915]
[    8.663625]  i915_driver_probe+0x5d5/0xcb0 [i915]
[    8.663734]  ? drm_privacy_screen_get+0x163/0x1a0 [drm_privacy_screen_helper]
[    8.663759]  i915_params_free+0x11a/0x200 [i915]
[    8.663830]  ? __pm_runtime_resume+0x58/0x90
[    8.663849]  local_pci_probe+0x42/0x80
[    8.663869]  pci_device_probe+0xd9/0x190
[    8.663892]  really_probe+0xf2/0x440
[    8.663915]  driver_probe_device+0xe1/0x150
[    8.663930]  device_driver_attach+0xa8/0xb0
[    8.663948]  __driver_attach+0x8c/0x150
[    8.663957]  ? device_driver_attach+0xb0/0xb0
[    8.663966]  ? device_driver_attach+0xb0/0xb0
[    8.663979]  bus_for_each_dev+0x67/0x90
[    8.663998]  bus_add_driver+0x12e/0x1f0
[    8.664015]  driver_register+0x8b/0xe0
[    8.664025]  ? 0xffffffffc055a000
[    8.664039]  init_module+0x62/0x7c [i915]
[    8.664127]  do_one_initcall+0x5b/0x2d0
[    8.664143]  ? rcu_read_lock_sched_held+0x3f/0x80
[    8.664155]  ? kmem_cache_alloc_trace+0x292/0x2c0
[    8.664178]  do_init_module+0x5c/0x260
[    8.664194]  __do_sys_init_module+0x13d/0x1a0
[    8.664247]  do_syscall_64+0x33/0x40
[    8.664260]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[    8.664272] RIP: 0033:0x7fe825d9a6be
[    8.664284] Code: 48 8b 0d bd 27 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8a 27 0c 00 f7 d8 64 89 01 48
[    8.664293] RSP: 002b:00007fffdc778028 EFLAGS: 00000246 ORIG_RAX: 00000000000000af
[    8.664307] RAX: ffffffffffffffda RBX: 00005573f0770cf0 RCX: 00007fe825d9a6be
[    8.664314] RDX: 00007fe825ed035a RSI: 000000000052b0a0 RDI: 00005573f112ae10
[    8.664321] RBP: 00005573f112ae10 R08: 00005573f112ae10 R09: 00007fffdc774f70
[    8.664328] R10: 00005573f0759010 R11: 0000000000000246 R12: 00007fe825ed035a
[    8.664334] R13: 00005573f077e1e0 R14: 0000000000000007 R15: 00005573f077f2d0
[    8.664379] irq event stamp: 126913
[    8.664385] hardirqs last  enabled at (126919): [<ffffffff9f162af9>] console_unlock+0x4e9/0x600
[    8.664397] hardirqs last disabled at (126924): [<ffffffff9f162a6c>] console_unlock+0x45c/0x600
[    8.664406] softirqs last  enabled at (126624): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
[    8.664416] softirqs last disabled at (126619): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
[    8.664426] ---[ end trace 5049606d4dbfaebc ]---

Add a check for the combination of the DPLL not being enabled (indicating
that DP is not active on the pipe), while the pipe is enabled; and when
both conditions are true don't use the pipe for pps. This fixes the above
WARN/backtrace. After this the attempt to detect the non existing eDP
panel on port B results in the following 2 info messages:

[    8.461967] i915 0000:00:02.0: [drm] Pipe A is used for non DP, not using it for pps
[    8.675304] i915 0000:00:02.0: [drm] failed to retrieve link info, disabling eDP

Indicating that everything is working as it should.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/display/intel_pps.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
index f20ba71f4307..757e82825919 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -121,6 +121,7 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
 {
 	struct intel_encoder *encoder;
 	unsigned int pipes = (1 << PIPE_A) | (1 << PIPE_B);
+	enum pipe pipe;
 
 	/*
 	 * We don't have power sequencer currently.
@@ -146,6 +147,27 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
 		}
 	}
 
+	/*
+	 * If the DPLL is not enabled and the pipe is enabled then the pipe is
+	 * in use for non DP uses. In this case we *must* not use it for pps.
+	 * This may happen when PIPE A is used for a DSI panel, yet the VLV code
+	 * in intel_setup_outputs() thinks port B may be used for eDP and calls
+	 * intel_dp_init() to check.
+	 */
+	for (pipe = PIPE_A; pipe <= PIPE_B; pipe++) {
+		if (!(pipes & (1 << pipe)))
+			continue;
+
+		if (intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE)
+			continue;
+
+		if (intel_pipe_is_enabled(dev_priv, (enum transcoder)pipe)) {
+			drm_info(&dev_priv->drm, "Pipe %c is used for non DP, not using it for pps\n",
+				 pipe_name(pipe));
+			pipes &= ~(1 << pipe);
+		}
+	}
+
 	if (pipes == 0)
 		return INVALID_PIPE;
 
-- 
2.30.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Hans de Goede]

As explained by a long comment block, on VLV intel_setup_outputs()
sometimes thinks there might be an eDP panel connected while there is none.
In this case intel_setup_outputs() will call intel_dp_init() to check.

In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
even though this might be in use for non DP purposes. When this is the case
then the assert_pipe() in vlv_force_pll_on() will fail when called from
vlv_power_sequencer_kick().

This happens on a Voyo winpad A15, leading to the following WARN/backtrace:

[    8.661531] ------------[ cut here ]------------
[    8.661590] transcoder A assertion failure (expected off, current on)
[    8.661647] WARNING: CPU: 2 PID: 243 at drivers/gpu/drm/i915/display/intel_display.c:1288 assert_pipe+0x125/0xc20 [i915]
[    8.661822] Modules linked in: i915(E+) mmc_block crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_algo_bit drm_kms_helper cec drm drm_privacy_screen_helper video(E) sdhci_acpi sdhci i2c_hid pwm_lpss_platform pwm_lpss mmc_core i2c_dev fuse
[    8.661944] CPU: 2 PID: 243 Comm: systemd-udevd Tainted: G            E     5.11.0-rc5+ #228
[    8.661954] Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 5.6.5 11/20/2014
[    8.661961] RIP: 0010:assert_pipe+0x125/0xc20 [i915]
[    8.662050] Code: c7 c2 e5 39 4a c0 74 c9 48 c7 c6 53 3b 4a c0 83 fb 06 77 0a 89 db 48 8b 34 dd 80 38 45 c0 48 c7 c7 c8 ff 47 c0 e8 13 6c 8f df <0f> 0b e9 1d ff ff ff 89 db 48 8b 34 dd 80 38 45 c0 eb a0 48 c7 c2
[    8.662058] RSP: 0018:ffffa939c0557690 EFLAGS: 00010286
[    8.662071] RAX: 0000000000000039 RBX: 0000000000000000 RCX: ffff89c67bd19058
[    8.662078] RDX: 00000000ffffffd8 RSI: 0000000000000027 RDI: ffff89c67bd19050
[    8.662085] RBP: ffff89c64a3c0000 R08: 0000000000000001 R09: 0000000000000001
[    8.662091] R10: ffffa939c05574c0 R11: ffffffffa0961248 R12: 0000000000000009
[    8.662098] R13: 0000000000000000 R14: 00000000e0000000 R15: ffff89c64a3c0000
[    8.662105] FS:  00007fe824e42380(0000) GS:ffff89c67bd00000(0000) knlGS:0000000000000000
[    8.662113] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    8.662120] CR2: 00007fffdc770558 CR3: 0000000106ab8000 CR4: 00000000001006e0
[    8.662127] Call Trace:
[    8.662148]  assert_pipe+0xa9e/0xc20 [i915]
[    8.662252]  vlv_force_pll_on+0xfb/0x1b0 [i915]
[    8.662344]  intel_dp_sync_state+0xd92/0x2e70 [i915]
[    8.662448]  intel_dp_sync_state+0x1908/0x2e70 [i915]
[    8.662541]  intel_dp_sync_state+0x1a3e/0x2e70 [i915]
[    8.662620]  ? recalibrate_cpu_khz+0x10/0x10
[    8.662633]  ? ktime_get_with_offset+0xad/0x160
[    8.662658]  intel_dp_sync_state+0x1f21/0x2e70 [i915]
[    8.662788]  intel_dp_encoder_suspend+0x41f/0x14b0 [i915]
[    8.662875]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
[    8.662940]  ? __mutex_lock+0x7e/0x7a0
[    8.662950]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
[    8.662982]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
[    8.663025]  intel_dp_encoder_suspend+0xdf3/0x14b0 [i915]
[    8.663112]  ? find_held_lock+0x2b/0x80
[    8.663132]  drm_dp_dpcd_access+0x62/0xf0 [drm_kms_helper]
[    8.663181]  drm_dp_dpcd_read+0xb6/0xf0 [drm_kms_helper]
[    8.663223]  drm_dp_read_dpcd_caps+0x20/0x110 [drm_kms_helper]
[    8.663262]  intel_dp_init_connector+0x79e/0x1010 [i915]
[    8.663366]  intel_dp_init+0x251/0x480 [i915]
[    8.663453]  intel_modeset_init_nogem+0x1998/0x1b70 [i915]
[    8.663540]  ? intel_pcode_init+0x3b6b/0x5d60 [i915]
[    8.663625]  i915_driver_probe+0x5d5/0xcb0 [i915]
[    8.663734]  ? drm_privacy_screen_get+0x163/0x1a0 [drm_privacy_screen_helper]
[    8.663759]  i915_params_free+0x11a/0x200 [i915]
[    8.663830]  ? __pm_runtime_resume+0x58/0x90
[    8.663849]  local_pci_probe+0x42/0x80
[    8.663869]  pci_device_probe+0xd9/0x190
[    8.663892]  really_probe+0xf2/0x440
[    8.663915]  driver_probe_device+0xe1/0x150
[    8.663930]  device_driver_attach+0xa8/0xb0
[    8.663948]  __driver_attach+0x8c/0x150
[    8.663957]  ? device_driver_attach+0xb0/0xb0
[    8.663966]  ? device_driver_attach+0xb0/0xb0
[    8.663979]  bus_for_each_dev+0x67/0x90
[    8.663998]  bus_add_driver+0x12e/0x1f0
[    8.664015]  driver_register+0x8b/0xe0
[    8.664025]  ? 0xffffffffc055a000
[    8.664039]  init_module+0x62/0x7c [i915]
[    8.664127]  do_one_initcall+0x5b/0x2d0
[    8.664143]  ? rcu_read_lock_sched_held+0x3f/0x80
[    8.664155]  ? kmem_cache_alloc_trace+0x292/0x2c0
[    8.664178]  do_init_module+0x5c/0x260
[    8.664194]  __do_sys_init_module+0x13d/0x1a0
[    8.664247]  do_syscall_64+0x33/0x40
[    8.664260]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[    8.664272] RIP: 0033:0x7fe825d9a6be
[    8.664284] Code: 48 8b 0d bd 27 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8a 27 0c 00 f7 d8 64 89 01 48
[    8.664293] RSP: 002b:00007fffdc778028 EFLAGS: 00000246 ORIG_RAX: 00000000000000af
[    8.664307] RAX: ffffffffffffffda RBX: 00005573f0770cf0 RCX: 00007fe825d9a6be
[    8.664314] RDX: 00007fe825ed035a RSI: 000000000052b0a0 RDI: 00005573f112ae10
[    8.664321] RBP: 00005573f112ae10 R08: 00005573f112ae10 R09: 00007fffdc774f70
[    8.664328] R10: 00005573f0759010 R11: 0000000000000246 R12: 00007fe825ed035a
[    8.664334] R13: 00005573f077e1e0 R14: 0000000000000007 R15: 00005573f077f2d0
[    8.664379] irq event stamp: 126913
[    8.664385] hardirqs last  enabled at (126919): [<ffffffff9f162af9>] console_unlock+0x4e9/0x600
[    8.664397] hardirqs last disabled at (126924): [<ffffffff9f162a6c>] console_unlock+0x45c/0x600
[    8.664406] softirqs last  enabled at (126624): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
[    8.664416] softirqs last disabled at (126619): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
[    8.664426] ---[ end trace 5049606d4dbfaebc ]---

Add a check for the combination of the DPLL not being enabled (indicating
that DP is not active on the pipe), while the pipe is enabled; and when
both conditions are true don't use the pipe for pps. This fixes the above
WARN/backtrace. After this the attempt to detect the non existing eDP
panel on port B results in the following 2 info messages:

[    8.461967] i915 0000:00:02.0: [drm] Pipe A is used for non DP, not using it for pps
[    8.675304] i915 0000:00:02.0: [drm] failed to retrieve link info, disabling eDP

Indicating that everything is working as it should.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/display/intel_pps.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
index f20ba71f4307..757e82825919 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -121,6 +121,7 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
 {
 	struct intel_encoder *encoder;
 	unsigned int pipes = (1 << PIPE_A) | (1 << PIPE_B);
+	enum pipe pipe;
 
 	/*
 	 * We don't have power sequencer currently.
@@ -146,6 +147,27 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
 		}
 	}
 
+	/*
+	 * If the DPLL is not enabled and the pipe is enabled then the pipe is
+	 * in use for non DP uses. In this case we *must* not use it for pps.
+	 * This may happen when PIPE A is used for a DSI panel, yet the VLV code
+	 * in intel_setup_outputs() thinks port B may be used for eDP and calls
+	 * intel_dp_init() to check.
+	 */
+	for (pipe = PIPE_A; pipe <= PIPE_B; pipe++) {
+		if (!(pipes & (1 << pipe)))
+			continue;
+
+		if (intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE)
+			continue;
+
+		if (intel_pipe_is_enabled(dev_priv, (enum transcoder)pipe)) {
+			drm_info(&dev_priv->drm, "Pipe %c is used for non DP, not using it for pps\n",
+				 pipe_name(pipe));
+			pipes &= ~(1 << pipe);
+		}
+	}
+
 	if (pipes == 0)
 		return INVALID_PIPE;
 
-- 
2.30.1

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

Re: [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Jani Nikula]

On Tue, 02 Mar 2021, Hans de Goede <hdegoede@redhat.com> wrote:
> As explained by a long comment block, on VLV intel_setup_outputs()
> sometimes thinks there might be an eDP panel connected while there is none.
> In this case intel_setup_outputs() will call intel_dp_init() to check.
>
> In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
> even though this might be in use for non DP purposes. When this is the case
> then the assert_pipe() in vlv_force_pll_on() will fail when called from
> vlv_power_sequencer_kick().
>
> This happens on a Voyo winpad A15, leading to the following WARN/backtrace:
>
> [    8.661531] ------------[ cut here ]------------
> [    8.661590] transcoder A assertion failure (expected off, current on)
> [    8.661647] WARNING: CPU: 2 PID: 243 at drivers/gpu/drm/i915/display/intel_display.c:1288 assert_pipe+0x125/0xc20 [i915]
> [    8.661822] Modules linked in: i915(E+) mmc_block crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_algo_bit drm_kms_helper cec drm drm_privacy_screen_helper video(E) sdhci_acpi sdhci i2c_hid pwm_lpss_platform pwm_lpss mmc_core i2c_dev fuse
> [    8.661944] CPU: 2 PID: 243 Comm: systemd-udevd Tainted: G            E     5.11.0-rc5+ #228
> [    8.661954] Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 5.6.5 11/20/2014
> [    8.661961] RIP: 0010:assert_pipe+0x125/0xc20 [i915]
> [    8.662050] Code: c7 c2 e5 39 4a c0 74 c9 48 c7 c6 53 3b 4a c0 83 fb 06 77 0a 89 db 48 8b 34 dd 80 38 45 c0 48 c7 c7 c8 ff 47 c0 e8 13 6c 8f df <0f> 0b e9 1d ff ff ff 89 db 48 8b 34 dd 80 38 45 c0 eb a0 48 c7 c2
> [    8.662058] RSP: 0018:ffffa939c0557690 EFLAGS: 00010286
> [    8.662071] RAX: 0000000000000039 RBX: 0000000000000000 RCX: ffff89c67bd19058
> [    8.662078] RDX: 00000000ffffffd8 RSI: 0000000000000027 RDI: ffff89c67bd19050
> [    8.662085] RBP: ffff89c64a3c0000 R08: 0000000000000001 R09: 0000000000000001
> [    8.662091] R10: ffffa939c05574c0 R11: ffffffffa0961248 R12: 0000000000000009
> [    8.662098] R13: 0000000000000000 R14: 00000000e0000000 R15: ffff89c64a3c0000
> [    8.662105] FS:  00007fe824e42380(0000) GS:ffff89c67bd00000(0000) knlGS:0000000000000000
> [    8.662113] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    8.662120] CR2: 00007fffdc770558 CR3: 0000000106ab8000 CR4: 00000000001006e0
> [    8.662127] Call Trace:
> [    8.662148]  assert_pipe+0xa9e/0xc20 [i915]
> [    8.662252]  vlv_force_pll_on+0xfb/0x1b0 [i915]
> [    8.662344]  intel_dp_sync_state+0xd92/0x2e70 [i915]
> [    8.662448]  intel_dp_sync_state+0x1908/0x2e70 [i915]
> [    8.662541]  intel_dp_sync_state+0x1a3e/0x2e70 [i915]
> [    8.662620]  ? recalibrate_cpu_khz+0x10/0x10
> [    8.662633]  ? ktime_get_with_offset+0xad/0x160
> [    8.662658]  intel_dp_sync_state+0x1f21/0x2e70 [i915]
> [    8.662788]  intel_dp_encoder_suspend+0x41f/0x14b0 [i915]
> [    8.662875]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
> [    8.662940]  ? __mutex_lock+0x7e/0x7a0
> [    8.662950]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
> [    8.662982]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
> [    8.663025]  intel_dp_encoder_suspend+0xdf3/0x14b0 [i915]
> [    8.663112]  ? find_held_lock+0x2b/0x80
> [    8.663132]  drm_dp_dpcd_access+0x62/0xf0 [drm_kms_helper]
> [    8.663181]  drm_dp_dpcd_read+0xb6/0xf0 [drm_kms_helper]
> [    8.663223]  drm_dp_read_dpcd_caps+0x20/0x110 [drm_kms_helper]
> [    8.663262]  intel_dp_init_connector+0x79e/0x1010 [i915]
> [    8.663366]  intel_dp_init+0x251/0x480 [i915]
> [    8.663453]  intel_modeset_init_nogem+0x1998/0x1b70 [i915]
> [    8.663540]  ? intel_pcode_init+0x3b6b/0x5d60 [i915]
> [    8.663625]  i915_driver_probe+0x5d5/0xcb0 [i915]
> [    8.663734]  ? drm_privacy_screen_get+0x163/0x1a0 [drm_privacy_screen_helper]
> [    8.663759]  i915_params_free+0x11a/0x200 [i915]
> [    8.663830]  ? __pm_runtime_resume+0x58/0x90
> [    8.663849]  local_pci_probe+0x42/0x80
> [    8.663869]  pci_device_probe+0xd9/0x190
> [    8.663892]  really_probe+0xf2/0x440
> [    8.663915]  driver_probe_device+0xe1/0x150
> [    8.663930]  device_driver_attach+0xa8/0xb0
> [    8.663948]  __driver_attach+0x8c/0x150
> [    8.663957]  ? device_driver_attach+0xb0/0xb0
> [    8.663966]  ? device_driver_attach+0xb0/0xb0
> [    8.663979]  bus_for_each_dev+0x67/0x90
> [    8.663998]  bus_add_driver+0x12e/0x1f0
> [    8.664015]  driver_register+0x8b/0xe0
> [    8.664025]  ? 0xffffffffc055a000
> [    8.664039]  init_module+0x62/0x7c [i915]
> [    8.664127]  do_one_initcall+0x5b/0x2d0
> [    8.664143]  ? rcu_read_lock_sched_held+0x3f/0x80
> [    8.664155]  ? kmem_cache_alloc_trace+0x292/0x2c0
> [    8.664178]  do_init_module+0x5c/0x260
> [    8.664194]  __do_sys_init_module+0x13d/0x1a0
> [    8.664247]  do_syscall_64+0x33/0x40
> [    8.664260]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [    8.664272] RIP: 0033:0x7fe825d9a6be
> [    8.664284] Code: 48 8b 0d bd 27 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8a 27 0c 00 f7 d8 64 89 01 48
> [    8.664293] RSP: 002b:00007fffdc778028 EFLAGS: 00000246 ORIG_RAX: 00000000000000af
> [    8.664307] RAX: ffffffffffffffda RBX: 00005573f0770cf0 RCX: 00007fe825d9a6be
> [    8.664314] RDX: 00007fe825ed035a RSI: 000000000052b0a0 RDI: 00005573f112ae10
> [    8.664321] RBP: 00005573f112ae10 R08: 00005573f112ae10 R09: 00007fffdc774f70
> [    8.664328] R10: 00005573f0759010 R11: 0000000000000246 R12: 00007fe825ed035a
> [    8.664334] R13: 00005573f077e1e0 R14: 0000000000000007 R15: 00005573f077f2d0
> [    8.664379] irq event stamp: 126913
> [    8.664385] hardirqs last  enabled at (126919): [<ffffffff9f162af9>] console_unlock+0x4e9/0x600
> [    8.664397] hardirqs last disabled at (126924): [<ffffffff9f162a6c>] console_unlock+0x45c/0x600
> [    8.664406] softirqs last  enabled at (126624): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
> [    8.664416] softirqs last disabled at (126619): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
> [    8.664426] ---[ end trace 5049606d4dbfaebc ]---
>
> Add a check for the combination of the DPLL not being enabled (indicating
> that DP is not active on the pipe), while the pipe is enabled; and when
> both conditions are true don't use the pipe for pps. This fixes the above
> WARN/backtrace. After this the attempt to detect the non existing eDP
> panel on port B results in the following 2 info messages:
>
> [    8.461967] i915 0000:00:02.0: [drm] Pipe A is used for non DP, not using it for pps
> [    8.675304] i915 0000:00:02.0: [drm] failed to retrieve link info, disabling eDP
>
> Indicating that everything is working as it should.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/display/intel_pps.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index f20ba71f4307..757e82825919 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -121,6 +121,7 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_encoder *encoder;
>  	unsigned int pipes = (1 << PIPE_A) | (1 << PIPE_B);
> +	enum pipe pipe;
>  
>  	/*
>  	 * We don't have power sequencer currently.
> @@ -146,6 +147,27 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>  		}
>  	}
>  
> +	/*
> +	 * If the DPLL is not enabled and the pipe is enabled then the pipe is
> +	 * in use for non DP uses. In this case we *must* not use it for pps.
> +	 * This may happen when PIPE A is used for a DSI panel, yet the VLV code
> +	 * in intel_setup_outputs() thinks port B may be used for eDP and calls
> +	 * intel_dp_init() to check.
> +	 */
> +	for (pipe = PIPE_A; pipe <= PIPE_B; pipe++) {
> +		if (!(pipes & (1 << pipe)))
> +			continue;

This should be:

	for_each_pipe_masked(dev_priv, pipe, pipes) {

Other than that, I think this looks sound, but I'd like to get Ville's
ack on it too.

BR,
Jani.

> +
> +		if (intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE)
> +			continue;
> +
> +		if (intel_pipe_is_enabled(dev_priv, (enum transcoder)pipe)) {
> +			drm_info(&dev_priv->drm, "Pipe %c is used for non DP, not using it for pps\n",
> +				 pipe_name(pipe));
> +			pipes &= ~(1 << pipe);
> +		}
> +	}
> +
>  	if (pipes == 0)
>  		return INVALID_PIPE;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Jani Nikula]

On Tue, 02 Mar 2021, Hans de Goede <hdegoede@redhat.com> wrote:
> As explained by a long comment block, on VLV intel_setup_outputs()
> sometimes thinks there might be an eDP panel connected while there is none.
> In this case intel_setup_outputs() will call intel_dp_init() to check.
>
> In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
> even though this might be in use for non DP purposes. When this is the case
> then the assert_pipe() in vlv_force_pll_on() will fail when called from
> vlv_power_sequencer_kick().
>
> This happens on a Voyo winpad A15, leading to the following WARN/backtrace:
>
> [    8.661531] ------------[ cut here ]------------
> [    8.661590] transcoder A assertion failure (expected off, current on)
> [    8.661647] WARNING: CPU: 2 PID: 243 at drivers/gpu/drm/i915/display/intel_display.c:1288 assert_pipe+0x125/0xc20 [i915]
> [    8.661822] Modules linked in: i915(E+) mmc_block crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_algo_bit drm_kms_helper cec drm drm_privacy_screen_helper video(E) sdhci_acpi sdhci i2c_hid pwm_lpss_platform pwm_lpss mmc_core i2c_dev fuse
> [    8.661944] CPU: 2 PID: 243 Comm: systemd-udevd Tainted: G            E     5.11.0-rc5+ #228
> [    8.661954] Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 5.6.5 11/20/2014
> [    8.661961] RIP: 0010:assert_pipe+0x125/0xc20 [i915]
> [    8.662050] Code: c7 c2 e5 39 4a c0 74 c9 48 c7 c6 53 3b 4a c0 83 fb 06 77 0a 89 db 48 8b 34 dd 80 38 45 c0 48 c7 c7 c8 ff 47 c0 e8 13 6c 8f df <0f> 0b e9 1d ff ff ff 89 db 48 8b 34 dd 80 38 45 c0 eb a0 48 c7 c2
> [    8.662058] RSP: 0018:ffffa939c0557690 EFLAGS: 00010286
> [    8.662071] RAX: 0000000000000039 RBX: 0000000000000000 RCX: ffff89c67bd19058
> [    8.662078] RDX: 00000000ffffffd8 RSI: 0000000000000027 RDI: ffff89c67bd19050
> [    8.662085] RBP: ffff89c64a3c0000 R08: 0000000000000001 R09: 0000000000000001
> [    8.662091] R10: ffffa939c05574c0 R11: ffffffffa0961248 R12: 0000000000000009
> [    8.662098] R13: 0000000000000000 R14: 00000000e0000000 R15: ffff89c64a3c0000
> [    8.662105] FS:  00007fe824e42380(0000) GS:ffff89c67bd00000(0000) knlGS:0000000000000000
> [    8.662113] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    8.662120] CR2: 00007fffdc770558 CR3: 0000000106ab8000 CR4: 00000000001006e0
> [    8.662127] Call Trace:
> [    8.662148]  assert_pipe+0xa9e/0xc20 [i915]
> [    8.662252]  vlv_force_pll_on+0xfb/0x1b0 [i915]
> [    8.662344]  intel_dp_sync_state+0xd92/0x2e70 [i915]
> [    8.662448]  intel_dp_sync_state+0x1908/0x2e70 [i915]
> [    8.662541]  intel_dp_sync_state+0x1a3e/0x2e70 [i915]
> [    8.662620]  ? recalibrate_cpu_khz+0x10/0x10
> [    8.662633]  ? ktime_get_with_offset+0xad/0x160
> [    8.662658]  intel_dp_sync_state+0x1f21/0x2e70 [i915]
> [    8.662788]  intel_dp_encoder_suspend+0x41f/0x14b0 [i915]
> [    8.662875]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
> [    8.662940]  ? __mutex_lock+0x7e/0x7a0
> [    8.662950]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
> [    8.662982]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
> [    8.663025]  intel_dp_encoder_suspend+0xdf3/0x14b0 [i915]
> [    8.663112]  ? find_held_lock+0x2b/0x80
> [    8.663132]  drm_dp_dpcd_access+0x62/0xf0 [drm_kms_helper]
> [    8.663181]  drm_dp_dpcd_read+0xb6/0xf0 [drm_kms_helper]
> [    8.663223]  drm_dp_read_dpcd_caps+0x20/0x110 [drm_kms_helper]
> [    8.663262]  intel_dp_init_connector+0x79e/0x1010 [i915]
> [    8.663366]  intel_dp_init+0x251/0x480 [i915]
> [    8.663453]  intel_modeset_init_nogem+0x1998/0x1b70 [i915]
> [    8.663540]  ? intel_pcode_init+0x3b6b/0x5d60 [i915]
> [    8.663625]  i915_driver_probe+0x5d5/0xcb0 [i915]
> [    8.663734]  ? drm_privacy_screen_get+0x163/0x1a0 [drm_privacy_screen_helper]
> [    8.663759]  i915_params_free+0x11a/0x200 [i915]
> [    8.663830]  ? __pm_runtime_resume+0x58/0x90
> [    8.663849]  local_pci_probe+0x42/0x80
> [    8.663869]  pci_device_probe+0xd9/0x190
> [    8.663892]  really_probe+0xf2/0x440
> [    8.663915]  driver_probe_device+0xe1/0x150
> [    8.663930]  device_driver_attach+0xa8/0xb0
> [    8.663948]  __driver_attach+0x8c/0x150
> [    8.663957]  ? device_driver_attach+0xb0/0xb0
> [    8.663966]  ? device_driver_attach+0xb0/0xb0
> [    8.663979]  bus_for_each_dev+0x67/0x90
> [    8.663998]  bus_add_driver+0x12e/0x1f0
> [    8.664015]  driver_register+0x8b/0xe0
> [    8.664025]  ? 0xffffffffc055a000
> [    8.664039]  init_module+0x62/0x7c [i915]
> [    8.664127]  do_one_initcall+0x5b/0x2d0
> [    8.664143]  ? rcu_read_lock_sched_held+0x3f/0x80
> [    8.664155]  ? kmem_cache_alloc_trace+0x292/0x2c0
> [    8.664178]  do_init_module+0x5c/0x260
> [    8.664194]  __do_sys_init_module+0x13d/0x1a0
> [    8.664247]  do_syscall_64+0x33/0x40
> [    8.664260]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [    8.664272] RIP: 0033:0x7fe825d9a6be
> [    8.664284] Code: 48 8b 0d bd 27 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8a 27 0c 00 f7 d8 64 89 01 48
> [    8.664293] RSP: 002b:00007fffdc778028 EFLAGS: 00000246 ORIG_RAX: 00000000000000af
> [    8.664307] RAX: ffffffffffffffda RBX: 00005573f0770cf0 RCX: 00007fe825d9a6be
> [    8.664314] RDX: 00007fe825ed035a RSI: 000000000052b0a0 RDI: 00005573f112ae10
> [    8.664321] RBP: 00005573f112ae10 R08: 00005573f112ae10 R09: 00007fffdc774f70
> [    8.664328] R10: 00005573f0759010 R11: 0000000000000246 R12: 00007fe825ed035a
> [    8.664334] R13: 00005573f077e1e0 R14: 0000000000000007 R15: 00005573f077f2d0
> [    8.664379] irq event stamp: 126913
> [    8.664385] hardirqs last  enabled at (126919): [<ffffffff9f162af9>] console_unlock+0x4e9/0x600
> [    8.664397] hardirqs last disabled at (126924): [<ffffffff9f162a6c>] console_unlock+0x45c/0x600
> [    8.664406] softirqs last  enabled at (126624): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
> [    8.664416] softirqs last disabled at (126619): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
> [    8.664426] ---[ end trace 5049606d4dbfaebc ]---
>
> Add a check for the combination of the DPLL not being enabled (indicating
> that DP is not active on the pipe), while the pipe is enabled; and when
> both conditions are true don't use the pipe for pps. This fixes the above
> WARN/backtrace. After this the attempt to detect the non existing eDP
> panel on port B results in the following 2 info messages:
>
> [    8.461967] i915 0000:00:02.0: [drm] Pipe A is used for non DP, not using it for pps
> [    8.675304] i915 0000:00:02.0: [drm] failed to retrieve link info, disabling eDP
>
> Indicating that everything is working as it should.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/display/intel_pps.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index f20ba71f4307..757e82825919 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -121,6 +121,7 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_encoder *encoder;
>  	unsigned int pipes = (1 << PIPE_A) | (1 << PIPE_B);
> +	enum pipe pipe;
>  
>  	/*
>  	 * We don't have power sequencer currently.
> @@ -146,6 +147,27 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>  		}
>  	}
>  
> +	/*
> +	 * If the DPLL is not enabled and the pipe is enabled then the pipe is
> +	 * in use for non DP uses. In this case we *must* not use it for pps.
> +	 * This may happen when PIPE A is used for a DSI panel, yet the VLV code
> +	 * in intel_setup_outputs() thinks port B may be used for eDP and calls
> +	 * intel_dp_init() to check.
> +	 */
> +	for (pipe = PIPE_A; pipe <= PIPE_B; pipe++) {
> +		if (!(pipes & (1 << pipe)))
> +			continue;

This should be:

	for_each_pipe_masked(dev_priv, pipe, pipes) {

Other than that, I think this looks sound, but I'd like to get Ville's
ack on it too.

BR,
Jani.

> +
> +		if (intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE)
> +			continue;
> +
> +		if (intel_pipe_is_enabled(dev_priv, (enum transcoder)pipe)) {
> +			drm_info(&dev_priv->drm, "Pipe %c is used for non DP, not using it for pps\n",
> +				 pipe_name(pipe));
> +			pipes &= ~(1 << pipe);
> +		}
> +	}
> +
>  	if (pipes == 0)
>  		return INVALID_PIPE;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Ville Syrjälä]

On Tue, Mar 02, 2021 at 01:00:40PM +0100, Hans de Goede wrote:
> As explained by a long comment block, on VLV intel_setup_outputs()
> sometimes thinks there might be an eDP panel connected while there is none.
> In this case intel_setup_outputs() will call intel_dp_init() to check.
> 
> In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
> even though this might be in use for non DP purposes. When this is the case
> then the assert_pipe() in vlv_force_pll_on() will fail when called from
> vlv_power_sequencer_kick().

The idea is that you *can* select a PPS from a pipe used for a non-DP
port since those don't care about the PPS stuff. So this doesn't seem
correct.

a) I would like to see the VBT for this machine
b) I wonder if the DSI PLL is sufficient for getting the PPS going?
c) If we do need the normal DPLL is there any harm to DSI in enabling it?

> 
> This happens on a Voyo winpad A15, leading to the following WARN/backtrace:
> 
> [    8.661531] ------------[ cut here ]------------
> [    8.661590] transcoder A assertion failure (expected off, current on)
> [    8.661647] WARNING: CPU: 2 PID: 243 at drivers/gpu/drm/i915/display/intel_display.c:1288 assert_pipe+0x125/0xc20 [i915]
> [    8.661822] Modules linked in: i915(E+) mmc_block crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_algo_bit drm_kms_helper cec drm drm_privacy_screen_helper video(E) sdhci_acpi sdhci i2c_hid pwm_lpss_platform pwm_lpss mmc_core i2c_dev fuse
> [    8.661944] CPU: 2 PID: 243 Comm: systemd-udevd Tainted: G            E     5.11.0-rc5+ #228
> [    8.661954] Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 5.6.5 11/20/2014
> [    8.661961] RIP: 0010:assert_pipe+0x125/0xc20 [i915]
> [    8.662050] Code: c7 c2 e5 39 4a c0 74 c9 48 c7 c6 53 3b 4a c0 83 fb 06 77 0a 89 db 48 8b 34 dd 80 38 45 c0 48 c7 c7 c8 ff 47 c0 e8 13 6c 8f df <0f> 0b e9 1d ff ff ff 89 db 48 8b 34 dd 80 38 45 c0 eb a0 48 c7 c2
> [    8.662058] RSP: 0018:ffffa939c0557690 EFLAGS: 00010286
> [    8.662071] RAX: 0000000000000039 RBX: 0000000000000000 RCX: ffff89c67bd19058
> [    8.662078] RDX: 00000000ffffffd8 RSI: 0000000000000027 RDI: ffff89c67bd19050
> [    8.662085] RBP: ffff89c64a3c0000 R08: 0000000000000001 R09: 0000000000000001
> [    8.662091] R10: ffffa939c05574c0 R11: ffffffffa0961248 R12: 0000000000000009
> [    8.662098] R13: 0000000000000000 R14: 00000000e0000000 R15: ffff89c64a3c0000
> [    8.662105] FS:  00007fe824e42380(0000) GS:ffff89c67bd00000(0000) knlGS:0000000000000000
> [    8.662113] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    8.662120] CR2: 00007fffdc770558 CR3: 0000000106ab8000 CR4: 00000000001006e0
> [    8.662127] Call Trace:
> [    8.662148]  assert_pipe+0xa9e/0xc20 [i915]
> [    8.662252]  vlv_force_pll_on+0xfb/0x1b0 [i915]
> [    8.662344]  intel_dp_sync_state+0xd92/0x2e70 [i915]
> [    8.662448]  intel_dp_sync_state+0x1908/0x2e70 [i915]
> [    8.662541]  intel_dp_sync_state+0x1a3e/0x2e70 [i915]
> [    8.662620]  ? recalibrate_cpu_khz+0x10/0x10
> [    8.662633]  ? ktime_get_with_offset+0xad/0x160
> [    8.662658]  intel_dp_sync_state+0x1f21/0x2e70 [i915]
> [    8.662788]  intel_dp_encoder_suspend+0x41f/0x14b0 [i915]
> [    8.662875]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
> [    8.662940]  ? __mutex_lock+0x7e/0x7a0
> [    8.662950]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
> [    8.662982]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
> [    8.663025]  intel_dp_encoder_suspend+0xdf3/0x14b0 [i915]
> [    8.663112]  ? find_held_lock+0x2b/0x80
> [    8.663132]  drm_dp_dpcd_access+0x62/0xf0 [drm_kms_helper]
> [    8.663181]  drm_dp_dpcd_read+0xb6/0xf0 [drm_kms_helper]
> [    8.663223]  drm_dp_read_dpcd_caps+0x20/0x110 [drm_kms_helper]
> [    8.663262]  intel_dp_init_connector+0x79e/0x1010 [i915]
> [    8.663366]  intel_dp_init+0x251/0x480 [i915]
> [    8.663453]  intel_modeset_init_nogem+0x1998/0x1b70 [i915]
> [    8.663540]  ? intel_pcode_init+0x3b6b/0x5d60 [i915]
> [    8.663625]  i915_driver_probe+0x5d5/0xcb0 [i915]
> [    8.663734]  ? drm_privacy_screen_get+0x163/0x1a0 [drm_privacy_screen_helper]
> [    8.663759]  i915_params_free+0x11a/0x200 [i915]
> [    8.663830]  ? __pm_runtime_resume+0x58/0x90
> [    8.663849]  local_pci_probe+0x42/0x80
> [    8.663869]  pci_device_probe+0xd9/0x190
> [    8.663892]  really_probe+0xf2/0x440
> [    8.663915]  driver_probe_device+0xe1/0x150
> [    8.663930]  device_driver_attach+0xa8/0xb0
> [    8.663948]  __driver_attach+0x8c/0x150
> [    8.663957]  ? device_driver_attach+0xb0/0xb0
> [    8.663966]  ? device_driver_attach+0xb0/0xb0
> [    8.663979]  bus_for_each_dev+0x67/0x90
> [    8.663998]  bus_add_driver+0x12e/0x1f0
> [    8.664015]  driver_register+0x8b/0xe0
> [    8.664025]  ? 0xffffffffc055a000
> [    8.664039]  init_module+0x62/0x7c [i915]
> [    8.664127]  do_one_initcall+0x5b/0x2d0
> [    8.664143]  ? rcu_read_lock_sched_held+0x3f/0x80
> [    8.664155]  ? kmem_cache_alloc_trace+0x292/0x2c0
> [    8.664178]  do_init_module+0x5c/0x260
> [    8.664194]  __do_sys_init_module+0x13d/0x1a0
> [    8.664247]  do_syscall_64+0x33/0x40
> [    8.664260]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [    8.664272] RIP: 0033:0x7fe825d9a6be
> [    8.664284] Code: 48 8b 0d bd 27 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8a 27 0c 00 f7 d8 64 89 01 48
> [    8.664293] RSP: 002b:00007fffdc778028 EFLAGS: 00000246 ORIG_RAX: 00000000000000af
> [    8.664307] RAX: ffffffffffffffda RBX: 00005573f0770cf0 RCX: 00007fe825d9a6be
> [    8.664314] RDX: 00007fe825ed035a RSI: 000000000052b0a0 RDI: 00005573f112ae10
> [    8.664321] RBP: 00005573f112ae10 R08: 00005573f112ae10 R09: 00007fffdc774f70
> [    8.664328] R10: 00005573f0759010 R11: 0000000000000246 R12: 00007fe825ed035a
> [    8.664334] R13: 00005573f077e1e0 R14: 0000000000000007 R15: 00005573f077f2d0
> [    8.664379] irq event stamp: 126913
> [    8.664385] hardirqs last  enabled at (126919): [<ffffffff9f162af9>] console_unlock+0x4e9/0x600
> [    8.664397] hardirqs last disabled at (126924): [<ffffffff9f162a6c>] console_unlock+0x45c/0x600
> [    8.664406] softirqs last  enabled at (126624): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
> [    8.664416] softirqs last disabled at (126619): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
> [    8.664426] ---[ end trace 5049606d4dbfaebc ]---
> 
> Add a check for the combination of the DPLL not being enabled (indicating
> that DP is not active on the pipe), while the pipe is enabled; and when
> both conditions are true don't use the pipe for pps. This fixes the above
> WARN/backtrace. After this the attempt to detect the non existing eDP
> panel on port B results in the following 2 info messages:
> 
> [    8.461967] i915 0000:00:02.0: [drm] Pipe A is used for non DP, not using it for pps
> [    8.675304] i915 0000:00:02.0: [drm] failed to retrieve link info, disabling eDP
> 
> Indicating that everything is working as it should.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/display/intel_pps.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index f20ba71f4307..757e82825919 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -121,6 +121,7 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_encoder *encoder;
>  	unsigned int pipes = (1 << PIPE_A) | (1 << PIPE_B);
> +	enum pipe pipe;
>  
>  	/*
>  	 * We don't have power sequencer currently.
> @@ -146,6 +147,27 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>  		}
>  	}
>  
> +	/*
> +	 * If the DPLL is not enabled and the pipe is enabled then the pipe is
> +	 * in use for non DP uses. In this case we *must* not use it for pps.
> +	 * This may happen when PIPE A is used for a DSI panel, yet the VLV code
> +	 * in intel_setup_outputs() thinks port B may be used for eDP and calls
> +	 * intel_dp_init() to check.
> +	 */
> +	for (pipe = PIPE_A; pipe <= PIPE_B; pipe++) {
> +		if (!(pipes & (1 << pipe)))
> +			continue;
> +
> +		if (intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE)
> +			continue;
> +
> +		if (intel_pipe_is_enabled(dev_priv, (enum transcoder)pipe)) {
> +			drm_info(&dev_priv->drm, "Pipe %c is used for non DP, not using it for pps\n",
> +				 pipe_name(pipe));
> +			pipes &= ~(1 << pipe);
> +		}
> +	}
> +
>  	if (pipes == 0)
>  		return INVALID_PIPE;
>  
> -- 
> 2.30.1

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

Re: [Intel-gfx] [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Ville Syrjälä]

On Tue, Mar 02, 2021 at 01:00:40PM +0100, Hans de Goede wrote:
> As explained by a long comment block, on VLV intel_setup_outputs()
> sometimes thinks there might be an eDP panel connected while there is none.
> In this case intel_setup_outputs() will call intel_dp_init() to check.
> 
> In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
> even though this might be in use for non DP purposes. When this is the case
> then the assert_pipe() in vlv_force_pll_on() will fail when called from
> vlv_power_sequencer_kick().

The idea is that you *can* select a PPS from a pipe used for a non-DP
port since those don't care about the PPS stuff. So this doesn't seem
correct.

a) I would like to see the VBT for this machine
b) I wonder if the DSI PLL is sufficient for getting the PPS going?
c) If we do need the normal DPLL is there any harm to DSI in enabling it?

> 
> This happens on a Voyo winpad A15, leading to the following WARN/backtrace:
> 
> [    8.661531] ------------[ cut here ]------------
> [    8.661590] transcoder A assertion failure (expected off, current on)
> [    8.661647] WARNING: CPU: 2 PID: 243 at drivers/gpu/drm/i915/display/intel_display.c:1288 assert_pipe+0x125/0xc20 [i915]
> [    8.661822] Modules linked in: i915(E+) mmc_block crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_algo_bit drm_kms_helper cec drm drm_privacy_screen_helper video(E) sdhci_acpi sdhci i2c_hid pwm_lpss_platform pwm_lpss mmc_core i2c_dev fuse
> [    8.661944] CPU: 2 PID: 243 Comm: systemd-udevd Tainted: G            E     5.11.0-rc5+ #228
> [    8.661954] Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 5.6.5 11/20/2014
> [    8.661961] RIP: 0010:assert_pipe+0x125/0xc20 [i915]
> [    8.662050] Code: c7 c2 e5 39 4a c0 74 c9 48 c7 c6 53 3b 4a c0 83 fb 06 77 0a 89 db 48 8b 34 dd 80 38 45 c0 48 c7 c7 c8 ff 47 c0 e8 13 6c 8f df <0f> 0b e9 1d ff ff ff 89 db 48 8b 34 dd 80 38 45 c0 eb a0 48 c7 c2
> [    8.662058] RSP: 0018:ffffa939c0557690 EFLAGS: 00010286
> [    8.662071] RAX: 0000000000000039 RBX: 0000000000000000 RCX: ffff89c67bd19058
> [    8.662078] RDX: 00000000ffffffd8 RSI: 0000000000000027 RDI: ffff89c67bd19050
> [    8.662085] RBP: ffff89c64a3c0000 R08: 0000000000000001 R09: 0000000000000001
> [    8.662091] R10: ffffa939c05574c0 R11: ffffffffa0961248 R12: 0000000000000009
> [    8.662098] R13: 0000000000000000 R14: 00000000e0000000 R15: ffff89c64a3c0000
> [    8.662105] FS:  00007fe824e42380(0000) GS:ffff89c67bd00000(0000) knlGS:0000000000000000
> [    8.662113] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    8.662120] CR2: 00007fffdc770558 CR3: 0000000106ab8000 CR4: 00000000001006e0
> [    8.662127] Call Trace:
> [    8.662148]  assert_pipe+0xa9e/0xc20 [i915]
> [    8.662252]  vlv_force_pll_on+0xfb/0x1b0 [i915]
> [    8.662344]  intel_dp_sync_state+0xd92/0x2e70 [i915]
> [    8.662448]  intel_dp_sync_state+0x1908/0x2e70 [i915]
> [    8.662541]  intel_dp_sync_state+0x1a3e/0x2e70 [i915]
> [    8.662620]  ? recalibrate_cpu_khz+0x10/0x10
> [    8.662633]  ? ktime_get_with_offset+0xad/0x160
> [    8.662658]  intel_dp_sync_state+0x1f21/0x2e70 [i915]
> [    8.662788]  intel_dp_encoder_suspend+0x41f/0x14b0 [i915]
> [    8.662875]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
> [    8.662940]  ? __mutex_lock+0x7e/0x7a0
> [    8.662950]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
> [    8.662982]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
> [    8.663025]  intel_dp_encoder_suspend+0xdf3/0x14b0 [i915]
> [    8.663112]  ? find_held_lock+0x2b/0x80
> [    8.663132]  drm_dp_dpcd_access+0x62/0xf0 [drm_kms_helper]
> [    8.663181]  drm_dp_dpcd_read+0xb6/0xf0 [drm_kms_helper]
> [    8.663223]  drm_dp_read_dpcd_caps+0x20/0x110 [drm_kms_helper]
> [    8.663262]  intel_dp_init_connector+0x79e/0x1010 [i915]
> [    8.663366]  intel_dp_init+0x251/0x480 [i915]
> [    8.663453]  intel_modeset_init_nogem+0x1998/0x1b70 [i915]
> [    8.663540]  ? intel_pcode_init+0x3b6b/0x5d60 [i915]
> [    8.663625]  i915_driver_probe+0x5d5/0xcb0 [i915]
> [    8.663734]  ? drm_privacy_screen_get+0x163/0x1a0 [drm_privacy_screen_helper]
> [    8.663759]  i915_params_free+0x11a/0x200 [i915]
> [    8.663830]  ? __pm_runtime_resume+0x58/0x90
> [    8.663849]  local_pci_probe+0x42/0x80
> [    8.663869]  pci_device_probe+0xd9/0x190
> [    8.663892]  really_probe+0xf2/0x440
> [    8.663915]  driver_probe_device+0xe1/0x150
> [    8.663930]  device_driver_attach+0xa8/0xb0
> [    8.663948]  __driver_attach+0x8c/0x150
> [    8.663957]  ? device_driver_attach+0xb0/0xb0
> [    8.663966]  ? device_driver_attach+0xb0/0xb0
> [    8.663979]  bus_for_each_dev+0x67/0x90
> [    8.663998]  bus_add_driver+0x12e/0x1f0
> [    8.664015]  driver_register+0x8b/0xe0
> [    8.664025]  ? 0xffffffffc055a000
> [    8.664039]  init_module+0x62/0x7c [i915]
> [    8.664127]  do_one_initcall+0x5b/0x2d0
> [    8.664143]  ? rcu_read_lock_sched_held+0x3f/0x80
> [    8.664155]  ? kmem_cache_alloc_trace+0x292/0x2c0
> [    8.664178]  do_init_module+0x5c/0x260
> [    8.664194]  __do_sys_init_module+0x13d/0x1a0
> [    8.664247]  do_syscall_64+0x33/0x40
> [    8.664260]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [    8.664272] RIP: 0033:0x7fe825d9a6be
> [    8.664284] Code: 48 8b 0d bd 27 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8a 27 0c 00 f7 d8 64 89 01 48
> [    8.664293] RSP: 002b:00007fffdc778028 EFLAGS: 00000246 ORIG_RAX: 00000000000000af
> [    8.664307] RAX: ffffffffffffffda RBX: 00005573f0770cf0 RCX: 00007fe825d9a6be
> [    8.664314] RDX: 00007fe825ed035a RSI: 000000000052b0a0 RDI: 00005573f112ae10
> [    8.664321] RBP: 00005573f112ae10 R08: 00005573f112ae10 R09: 00007fffdc774f70
> [    8.664328] R10: 00005573f0759010 R11: 0000000000000246 R12: 00007fe825ed035a
> [    8.664334] R13: 00005573f077e1e0 R14: 0000000000000007 R15: 00005573f077f2d0
> [    8.664379] irq event stamp: 126913
> [    8.664385] hardirqs last  enabled at (126919): [<ffffffff9f162af9>] console_unlock+0x4e9/0x600
> [    8.664397] hardirqs last disabled at (126924): [<ffffffff9f162a6c>] console_unlock+0x45c/0x600
> [    8.664406] softirqs last  enabled at (126624): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
> [    8.664416] softirqs last disabled at (126619): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
> [    8.664426] ---[ end trace 5049606d4dbfaebc ]---
> 
> Add a check for the combination of the DPLL not being enabled (indicating
> that DP is not active on the pipe), while the pipe is enabled; and when
> both conditions are true don't use the pipe for pps. This fixes the above
> WARN/backtrace. After this the attempt to detect the non existing eDP
> panel on port B results in the following 2 info messages:
> 
> [    8.461967] i915 0000:00:02.0: [drm] Pipe A is used for non DP, not using it for pps
> [    8.675304] i915 0000:00:02.0: [drm] failed to retrieve link info, disabling eDP
> 
> Indicating that everything is working as it should.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/display/intel_pps.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index f20ba71f4307..757e82825919 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -121,6 +121,7 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_encoder *encoder;
>  	unsigned int pipes = (1 << PIPE_A) | (1 << PIPE_B);
> +	enum pipe pipe;
>  
>  	/*
>  	 * We don't have power sequencer currently.
> @@ -146,6 +147,27 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>  		}
>  	}
>  
> +	/*
> +	 * If the DPLL is not enabled and the pipe is enabled then the pipe is
> +	 * in use for non DP uses. In this case we *must* not use it for pps.
> +	 * This may happen when PIPE A is used for a DSI panel, yet the VLV code
> +	 * in intel_setup_outputs() thinks port B may be used for eDP and calls
> +	 * intel_dp_init() to check.
> +	 */
> +	for (pipe = PIPE_A; pipe <= PIPE_B; pipe++) {
> +		if (!(pipes & (1 << pipe)))
> +			continue;
> +
> +		if (intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE)
> +			continue;
> +
> +		if (intel_pipe_is_enabled(dev_priv, (enum transcoder)pipe)) {
> +			drm_info(&dev_priv->drm, "Pipe %c is used for non DP, not using it for pps\n",
> +				 pipe_name(pipe));
> +			pipes &= ~(1 << pipe);
> +		}
> +	}
> +
>  	if (pipes == 0)
>  		return INVALID_PIPE;
>  
> -- 
> 2.30.1

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

Re: [Intel-gfx] [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Hans de Goede]

Hi,

On 3/2/21 3:51 PM, Ville Syrjälä wrote:
> On Tue, Mar 02, 2021 at 01:00:40PM +0100, Hans de Goede wrote:
>> As explained by a long comment block, on VLV intel_setup_outputs()
>> sometimes thinks there might be an eDP panel connected while there is none.
>> In this case intel_setup_outputs() will call intel_dp_init() to check.
>>
>> In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
>> even though this might be in use for non DP purposes. When this is the case
>> then the assert_pipe() in vlv_force_pll_on() will fail when called from
>> vlv_power_sequencer_kick().
> 
> The idea is that you *can* select a PPS from a pipe used for a non-DP
> port since those don't care about the PPS stuff. So this doesn't seem
> correct.

They may not care about the PPS stuff, but as the WARN / backtrace
shows if the DPLL_VCO_ENABLE bit is not already set for the pipe, while
the pipe is "otherwise" in use then vlv_force_pll_on() becomes unhappy
triggering the WARN.

> a) I would like to see the VBT for this machine

https://fedorapeople.org/~jwrdegoede/voyo-winpad-a15-vbt

> b) I wonder if the DSI PLL is sufficient for getting the PPS going?

I have no idea, I just noticed the WARN / backtrace and this seemed
like a reasonably way to deal with it. With that said I'm fine with fixing
this a different way.

> c) If we do need the normal DPLL is there any harm to DSI in enabling it?

I would assume this increases power-consumption and DSI panels are almost
always used in battery powered devices.

Also this would impact all BYT/CHT devices, possible triggering unwanted
side-effects. Where as the proposed fix below is much more narrowly targeted
at the problem. It might not be the most pretty fix but AFAICT it has a low
risk of causing regressions.

Regards,

Hans



> 
>>
>> This happens on a Voyo winpad A15, leading to the following WARN/backtrace:
>>
>> [    8.661531] ------------[ cut here ]------------
>> [    8.661590] transcoder A assertion failure (expected off, current on)
>> [    8.661647] WARNING: CPU: 2 PID: 243 at drivers/gpu/drm/i915/display/intel_display.c:1288 assert_pipe+0x125/0xc20 [i915]
>> [    8.661822] Modules linked in: i915(E+) mmc_block crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_algo_bit drm_kms_helper cec drm drm_privacy_screen_helper video(E) sdhci_acpi sdhci i2c_hid pwm_lpss_platform pwm_lpss mmc_core i2c_dev fuse
>> [    8.661944] CPU: 2 PID: 243 Comm: systemd-udevd Tainted: G            E     5.11.0-rc5+ #228
>> [    8.661954] Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 5.6.5 11/20/2014
>> [    8.661961] RIP: 0010:assert_pipe+0x125/0xc20 [i915]
>> [    8.662050] Code: c7 c2 e5 39 4a c0 74 c9 48 c7 c6 53 3b 4a c0 83 fb 06 77 0a 89 db 48 8b 34 dd 80 38 45 c0 48 c7 c7 c8 ff 47 c0 e8 13 6c 8f df <0f> 0b e9 1d ff ff ff 89 db 48 8b 34 dd 80 38 45 c0 eb a0 48 c7 c2
>> [    8.662058] RSP: 0018:ffffa939c0557690 EFLAGS: 00010286
>> [    8.662071] RAX: 0000000000000039 RBX: 0000000000000000 RCX: ffff89c67bd19058
>> [    8.662078] RDX: 00000000ffffffd8 RSI: 0000000000000027 RDI: ffff89c67bd19050
>> [    8.662085] RBP: ffff89c64a3c0000 R08: 0000000000000001 R09: 0000000000000001
>> [    8.662091] R10: ffffa939c05574c0 R11: ffffffffa0961248 R12: 0000000000000009
>> [    8.662098] R13: 0000000000000000 R14: 00000000e0000000 R15: ffff89c64a3c0000
>> [    8.662105] FS:  00007fe824e42380(0000) GS:ffff89c67bd00000(0000) knlGS:0000000000000000
>> [    8.662113] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [    8.662120] CR2: 00007fffdc770558 CR3: 0000000106ab8000 CR4: 00000000001006e0
>> [    8.662127] Call Trace:
>> [    8.662148]  assert_pipe+0xa9e/0xc20 [i915]
>> [    8.662252]  vlv_force_pll_on+0xfb/0x1b0 [i915]
>> [    8.662344]  intel_dp_sync_state+0xd92/0x2e70 [i915]
>> [    8.662448]  intel_dp_sync_state+0x1908/0x2e70 [i915]
>> [    8.662541]  intel_dp_sync_state+0x1a3e/0x2e70 [i915]
>> [    8.662620]  ? recalibrate_cpu_khz+0x10/0x10
>> [    8.662633]  ? ktime_get_with_offset+0xad/0x160
>> [    8.662658]  intel_dp_sync_state+0x1f21/0x2e70 [i915]
>> [    8.662788]  intel_dp_encoder_suspend+0x41f/0x14b0 [i915]
>> [    8.662875]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
>> [    8.662940]  ? __mutex_lock+0x7e/0x7a0
>> [    8.662950]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
>> [    8.662982]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
>> [    8.663025]  intel_dp_encoder_suspend+0xdf3/0x14b0 [i915]
>> [    8.663112]  ? find_held_lock+0x2b/0x80
>> [    8.663132]  drm_dp_dpcd_access+0x62/0xf0 [drm_kms_helper]
>> [    8.663181]  drm_dp_dpcd_read+0xb6/0xf0 [drm_kms_helper]
>> [    8.663223]  drm_dp_read_dpcd_caps+0x20/0x110 [drm_kms_helper]
>> [    8.663262]  intel_dp_init_connector+0x79e/0x1010 [i915]
>> [    8.663366]  intel_dp_init+0x251/0x480 [i915]
>> [    8.663453]  intel_modeset_init_nogem+0x1998/0x1b70 [i915]
>> [    8.663540]  ? intel_pcode_init+0x3b6b/0x5d60 [i915]
>> [    8.663625]  i915_driver_probe+0x5d5/0xcb0 [i915]
>> [    8.663734]  ? drm_privacy_screen_get+0x163/0x1a0 [drm_privacy_screen_helper]
>> [    8.663759]  i915_params_free+0x11a/0x200 [i915]
>> [    8.663830]  ? __pm_runtime_resume+0x58/0x90
>> [    8.663849]  local_pci_probe+0x42/0x80
>> [    8.663869]  pci_device_probe+0xd9/0x190
>> [    8.663892]  really_probe+0xf2/0x440
>> [    8.663915]  driver_probe_device+0xe1/0x150
>> [    8.663930]  device_driver_attach+0xa8/0xb0
>> [    8.663948]  __driver_attach+0x8c/0x150
>> [    8.663957]  ? device_driver_attach+0xb0/0xb0
>> [    8.663966]  ? device_driver_attach+0xb0/0xb0
>> [    8.663979]  bus_for_each_dev+0x67/0x90
>> [    8.663998]  bus_add_driver+0x12e/0x1f0
>> [    8.664015]  driver_register+0x8b/0xe0
>> [    8.664025]  ? 0xffffffffc055a000
>> [    8.664039]  init_module+0x62/0x7c [i915]
>> [    8.664127]  do_one_initcall+0x5b/0x2d0
>> [    8.664143]  ? rcu_read_lock_sched_held+0x3f/0x80
>> [    8.664155]  ? kmem_cache_alloc_trace+0x292/0x2c0
>> [    8.664178]  do_init_module+0x5c/0x260
>> [    8.664194]  __do_sys_init_module+0x13d/0x1a0
>> [    8.664247]  do_syscall_64+0x33/0x40
>> [    8.664260]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [    8.664272] RIP: 0033:0x7fe825d9a6be
>> [    8.664284] Code: 48 8b 0d bd 27 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8a 27 0c 00 f7 d8 64 89 01 48
>> [    8.664293] RSP: 002b:00007fffdc778028 EFLAGS: 00000246 ORIG_RAX: 00000000000000af
>> [    8.664307] RAX: ffffffffffffffda RBX: 00005573f0770cf0 RCX: 00007fe825d9a6be
>> [    8.664314] RDX: 00007fe825ed035a RSI: 000000000052b0a0 RDI: 00005573f112ae10
>> [    8.664321] RBP: 00005573f112ae10 R08: 00005573f112ae10 R09: 00007fffdc774f70
>> [    8.664328] R10: 00005573f0759010 R11: 0000000000000246 R12: 00007fe825ed035a
>> [    8.664334] R13: 00005573f077e1e0 R14: 0000000000000007 R15: 00005573f077f2d0
>> [    8.664379] irq event stamp: 126913
>> [    8.664385] hardirqs last  enabled at (126919): [<ffffffff9f162af9>] console_unlock+0x4e9/0x600
>> [    8.664397] hardirqs last disabled at (126924): [<ffffffff9f162a6c>] console_unlock+0x45c/0x600
>> [    8.664406] softirqs last  enabled at (126624): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
>> [    8.664416] softirqs last disabled at (126619): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
>> [    8.664426] ---[ end trace 5049606d4dbfaebc ]---
>>
>> Add a check for the combination of the DPLL not being enabled (indicating
>> that DP is not active on the pipe), while the pipe is enabled; and when
>> both conditions are true don't use the pipe for pps. This fixes the above
>> WARN/backtrace. After this the attempt to detect the non existing eDP
>> panel on port B results in the following 2 info messages:
>>
>> [    8.461967] i915 0000:00:02.0: [drm] Pipe A is used for non DP, not using it for pps
>> [    8.675304] i915 0000:00:02.0: [drm] failed to retrieve link info, disabling eDP
>>
>> Indicating that everything is working as it should.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_pps.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
>> index f20ba71f4307..757e82825919 100644
>> --- a/drivers/gpu/drm/i915/display/intel_pps.c
>> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
>> @@ -121,6 +121,7 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>>  {
>>  	struct intel_encoder *encoder;
>>  	unsigned int pipes = (1 << PIPE_A) | (1 << PIPE_B);
>> +	enum pipe pipe;
>>  
>>  	/*
>>  	 * We don't have power sequencer currently.
>> @@ -146,6 +147,27 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>>  		}
>>  	}
>>  
>> +	/*
>> +	 * If the DPLL is not enabled and the pipe is enabled then the pipe is
>> +	 * in use for non DP uses. In this case we *must* not use it for pps.
>> +	 * This may happen when PIPE A is used for a DSI panel, yet the VLV code
>> +	 * in intel_setup_outputs() thinks port B may be used for eDP and calls
>> +	 * intel_dp_init() to check.
>> +	 */
>> +	for (pipe = PIPE_A; pipe <= PIPE_B; pipe++) {
>> +		if (!(pipes & (1 << pipe)))
>> +			continue;
>> +
>> +		if (intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE)
>> +			continue;
>> +
>> +		if (intel_pipe_is_enabled(dev_priv, (enum transcoder)pipe)) {
>> +			drm_info(&dev_priv->drm, "Pipe %c is used for non DP, not using it for pps\n",
>> +				 pipe_name(pipe));
>> +			pipes &= ~(1 << pipe);
>> +		}
>> +	}
>> +
>>  	if (pipes == 0)
>>  		return INVALID_PIPE;
>>  
>> -- 
>> 2.30.1
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Hans de Goede]

Hi,

On 3/2/21 3:51 PM, Ville Syrjälä wrote:
> On Tue, Mar 02, 2021 at 01:00:40PM +0100, Hans de Goede wrote:
>> As explained by a long comment block, on VLV intel_setup_outputs()
>> sometimes thinks there might be an eDP panel connected while there is none.
>> In this case intel_setup_outputs() will call intel_dp_init() to check.
>>
>> In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
>> even though this might be in use for non DP purposes. When this is the case
>> then the assert_pipe() in vlv_force_pll_on() will fail when called from
>> vlv_power_sequencer_kick().
> 
> The idea is that you *can* select a PPS from a pipe used for a non-DP
> port since those don't care about the PPS stuff. So this doesn't seem
> correct.

They may not care about the PPS stuff, but as the WARN / backtrace
shows if the DPLL_VCO_ENABLE bit is not already set for the pipe, while
the pipe is "otherwise" in use then vlv_force_pll_on() becomes unhappy
triggering the WARN.

> a) I would like to see the VBT for this machine

https://fedorapeople.org/~jwrdegoede/voyo-winpad-a15-vbt

> b) I wonder if the DSI PLL is sufficient for getting the PPS going?

I have no idea, I just noticed the WARN / backtrace and this seemed
like a reasonably way to deal with it. With that said I'm fine with fixing
this a different way.

> c) If we do need the normal DPLL is there any harm to DSI in enabling it?

I would assume this increases power-consumption and DSI panels are almost
always used in battery powered devices.

Also this would impact all BYT/CHT devices, possible triggering unwanted
side-effects. Where as the proposed fix below is much more narrowly targeted
at the problem. It might not be the most pretty fix but AFAICT it has a low
risk of causing regressions.

Regards,

Hans



> 
>>
>> This happens on a Voyo winpad A15, leading to the following WARN/backtrace:
>>
>> [    8.661531] ------------[ cut here ]------------
>> [    8.661590] transcoder A assertion failure (expected off, current on)
>> [    8.661647] WARNING: CPU: 2 PID: 243 at drivers/gpu/drm/i915/display/intel_display.c:1288 assert_pipe+0x125/0xc20 [i915]
>> [    8.661822] Modules linked in: i915(E+) mmc_block crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_algo_bit drm_kms_helper cec drm drm_privacy_screen_helper video(E) sdhci_acpi sdhci i2c_hid pwm_lpss_platform pwm_lpss mmc_core i2c_dev fuse
>> [    8.661944] CPU: 2 PID: 243 Comm: systemd-udevd Tainted: G            E     5.11.0-rc5+ #228
>> [    8.661954] Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 5.6.5 11/20/2014
>> [    8.661961] RIP: 0010:assert_pipe+0x125/0xc20 [i915]
>> [    8.662050] Code: c7 c2 e5 39 4a c0 74 c9 48 c7 c6 53 3b 4a c0 83 fb 06 77 0a 89 db 48 8b 34 dd 80 38 45 c0 48 c7 c7 c8 ff 47 c0 e8 13 6c 8f df <0f> 0b e9 1d ff ff ff 89 db 48 8b 34 dd 80 38 45 c0 eb a0 48 c7 c2
>> [    8.662058] RSP: 0018:ffffa939c0557690 EFLAGS: 00010286
>> [    8.662071] RAX: 0000000000000039 RBX: 0000000000000000 RCX: ffff89c67bd19058
>> [    8.662078] RDX: 00000000ffffffd8 RSI: 0000000000000027 RDI: ffff89c67bd19050
>> [    8.662085] RBP: ffff89c64a3c0000 R08: 0000000000000001 R09: 0000000000000001
>> [    8.662091] R10: ffffa939c05574c0 R11: ffffffffa0961248 R12: 0000000000000009
>> [    8.662098] R13: 0000000000000000 R14: 00000000e0000000 R15: ffff89c64a3c0000
>> [    8.662105] FS:  00007fe824e42380(0000) GS:ffff89c67bd00000(0000) knlGS:0000000000000000
>> [    8.662113] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [    8.662120] CR2: 00007fffdc770558 CR3: 0000000106ab8000 CR4: 00000000001006e0
>> [    8.662127] Call Trace:
>> [    8.662148]  assert_pipe+0xa9e/0xc20 [i915]
>> [    8.662252]  vlv_force_pll_on+0xfb/0x1b0 [i915]
>> [    8.662344]  intel_dp_sync_state+0xd92/0x2e70 [i915]
>> [    8.662448]  intel_dp_sync_state+0x1908/0x2e70 [i915]
>> [    8.662541]  intel_dp_sync_state+0x1a3e/0x2e70 [i915]
>> [    8.662620]  ? recalibrate_cpu_khz+0x10/0x10
>> [    8.662633]  ? ktime_get_with_offset+0xad/0x160
>> [    8.662658]  intel_dp_sync_state+0x1f21/0x2e70 [i915]
>> [    8.662788]  intel_dp_encoder_suspend+0x41f/0x14b0 [i915]
>> [    8.662875]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
>> [    8.662940]  ? __mutex_lock+0x7e/0x7a0
>> [    8.662950]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
>> [    8.662982]  ? drm_dp_dpcd_access+0x50/0xf0 [drm_kms_helper]
>> [    8.663025]  intel_dp_encoder_suspend+0xdf3/0x14b0 [i915]
>> [    8.663112]  ? find_held_lock+0x2b/0x80
>> [    8.663132]  drm_dp_dpcd_access+0x62/0xf0 [drm_kms_helper]
>> [    8.663181]  drm_dp_dpcd_read+0xb6/0xf0 [drm_kms_helper]
>> [    8.663223]  drm_dp_read_dpcd_caps+0x20/0x110 [drm_kms_helper]
>> [    8.663262]  intel_dp_init_connector+0x79e/0x1010 [i915]
>> [    8.663366]  intel_dp_init+0x251/0x480 [i915]
>> [    8.663453]  intel_modeset_init_nogem+0x1998/0x1b70 [i915]
>> [    8.663540]  ? intel_pcode_init+0x3b6b/0x5d60 [i915]
>> [    8.663625]  i915_driver_probe+0x5d5/0xcb0 [i915]
>> [    8.663734]  ? drm_privacy_screen_get+0x163/0x1a0 [drm_privacy_screen_helper]
>> [    8.663759]  i915_params_free+0x11a/0x200 [i915]
>> [    8.663830]  ? __pm_runtime_resume+0x58/0x90
>> [    8.663849]  local_pci_probe+0x42/0x80
>> [    8.663869]  pci_device_probe+0xd9/0x190
>> [    8.663892]  really_probe+0xf2/0x440
>> [    8.663915]  driver_probe_device+0xe1/0x150
>> [    8.663930]  device_driver_attach+0xa8/0xb0
>> [    8.663948]  __driver_attach+0x8c/0x150
>> [    8.663957]  ? device_driver_attach+0xb0/0xb0
>> [    8.663966]  ? device_driver_attach+0xb0/0xb0
>> [    8.663979]  bus_for_each_dev+0x67/0x90
>> [    8.663998]  bus_add_driver+0x12e/0x1f0
>> [    8.664015]  driver_register+0x8b/0xe0
>> [    8.664025]  ? 0xffffffffc055a000
>> [    8.664039]  init_module+0x62/0x7c [i915]
>> [    8.664127]  do_one_initcall+0x5b/0x2d0
>> [    8.664143]  ? rcu_read_lock_sched_held+0x3f/0x80
>> [    8.664155]  ? kmem_cache_alloc_trace+0x292/0x2c0
>> [    8.664178]  do_init_module+0x5c/0x260
>> [    8.664194]  __do_sys_init_module+0x13d/0x1a0
>> [    8.664247]  do_syscall_64+0x33/0x40
>> [    8.664260]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [    8.664272] RIP: 0033:0x7fe825d9a6be
>> [    8.664284] Code: 48 8b 0d bd 27 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8a 27 0c 00 f7 d8 64 89 01 48
>> [    8.664293] RSP: 002b:00007fffdc778028 EFLAGS: 00000246 ORIG_RAX: 00000000000000af
>> [    8.664307] RAX: ffffffffffffffda RBX: 00005573f0770cf0 RCX: 00007fe825d9a6be
>> [    8.664314] RDX: 00007fe825ed035a RSI: 000000000052b0a0 RDI: 00005573f112ae10
>> [    8.664321] RBP: 00005573f112ae10 R08: 00005573f112ae10 R09: 00007fffdc774f70
>> [    8.664328] R10: 00005573f0759010 R11: 0000000000000246 R12: 00007fe825ed035a
>> [    8.664334] R13: 00005573f077e1e0 R14: 0000000000000007 R15: 00005573f077f2d0
>> [    8.664379] irq event stamp: 126913
>> [    8.664385] hardirqs last  enabled at (126919): [<ffffffff9f162af9>] console_unlock+0x4e9/0x600
>> [    8.664397] hardirqs last disabled at (126924): [<ffffffff9f162a6c>] console_unlock+0x45c/0x600
>> [    8.664406] softirqs last  enabled at (126624): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
>> [    8.664416] softirqs last disabled at (126619): [<ffffffff9fe01112>] asm_call_irq_on_stack+0x12/0x20
>> [    8.664426] ---[ end trace 5049606d4dbfaebc ]---
>>
>> Add a check for the combination of the DPLL not being enabled (indicating
>> that DP is not active on the pipe), while the pipe is enabled; and when
>> both conditions are true don't use the pipe for pps. This fixes the above
>> WARN/backtrace. After this the attempt to detect the non existing eDP
>> panel on port B results in the following 2 info messages:
>>
>> [    8.461967] i915 0000:00:02.0: [drm] Pipe A is used for non DP, not using it for pps
>> [    8.675304] i915 0000:00:02.0: [drm] failed to retrieve link info, disabling eDP
>>
>> Indicating that everything is working as it should.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_pps.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
>> index f20ba71f4307..757e82825919 100644
>> --- a/drivers/gpu/drm/i915/display/intel_pps.c
>> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
>> @@ -121,6 +121,7 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>>  {
>>  	struct intel_encoder *encoder;
>>  	unsigned int pipes = (1 << PIPE_A) | (1 << PIPE_B);
>> +	enum pipe pipe;
>>  
>>  	/*
>>  	 * We don't have power sequencer currently.
>> @@ -146,6 +147,27 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>>  		}
>>  	}
>>  
>> +	/*
>> +	 * If the DPLL is not enabled and the pipe is enabled then the pipe is
>> +	 * in use for non DP uses. In this case we *must* not use it for pps.
>> +	 * This may happen when PIPE A is used for a DSI panel, yet the VLV code
>> +	 * in intel_setup_outputs() thinks port B may be used for eDP and calls
>> +	 * intel_dp_init() to check.
>> +	 */
>> +	for (pipe = PIPE_A; pipe <= PIPE_B; pipe++) {
>> +		if (!(pipes & (1 << pipe)))
>> +			continue;
>> +
>> +		if (intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE)
>> +			continue;
>> +
>> +		if (intel_pipe_is_enabled(dev_priv, (enum transcoder)pipe)) {
>> +			drm_info(&dev_priv->drm, "Pipe %c is used for non DP, not using it for pps\n",
>> +				 pipe_name(pipe));
>> +			pipes &= ~(1 << pipe);
>> +		}
>> +	}
>> +
>>  	if (pipes == 0)
>>  		return INVALID_PIPE;
>>  
>> -- 
>> 2.30.1
> 

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

Re: [Intel-gfx] [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Ville Syrjälä]

On Tue, Mar 23, 2021 at 11:39:09AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 3/2/21 3:51 PM, Ville Syrjälä wrote:
> > On Tue, Mar 02, 2021 at 01:00:40PM +0100, Hans de Goede wrote:
> >> As explained by a long comment block, on VLV intel_setup_outputs()
> >> sometimes thinks there might be an eDP panel connected while there is none.
> >> In this case intel_setup_outputs() will call intel_dp_init() to check.
> >>
> >> In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
> >> even though this might be in use for non DP purposes. When this is the case
> >> then the assert_pipe() in vlv_force_pll_on() will fail when called from
> >> vlv_power_sequencer_kick().
> > 
> > The idea is that you *can* select a PPS from a pipe used for a non-DP
> > port since those don't care about the PPS stuff. So this doesn't seem
> > correct.
> 
> They may not care about the PPS stuff, but as the WARN / backtrace
> shows if the DPLL_VCO_ENABLE bit is not already set for the pipe, while
> the pipe is "otherwise" in use then vlv_force_pll_on() becomes unhappy
> triggering the WARN.
> 
> > a) I would like to see the VBT for this machine
> 
> https://fedorapeople.org/~jwrdegoede/voyo-winpad-a15-vbt
> 
> > b) I wonder if the DSI PLL is sufficient for getting the PPS going?
> 
> I have no idea, I just noticed the WARN / backtrace and this seemed
> like a reasonably way to deal with it. With that said I'm fine with fixing
> this a different way.
> 
> > c) If we do need the normal DPLL is there any harm to DSI in enabling it?
> 
> I would assume this increases power-consumption and DSI panels are almost
> always used in battery powered devices.

This is just used while probing the panel, so power consumption is
not a concern.

> 
> Also this would impact all BYT/CHT devices, possible triggering unwanted
> side-effects. Where as the proposed fix below is much more narrowly targeted
> at the problem. It might not be the most pretty fix but AFAICT it has a low
> risk of causing regressions.

It rather significantly changes the logic of the workaround, potentially
causing us to not find a free PPS at all. Eg. if you were to boot with
a VLV with pipe A -> eDP B + eDP C inactive + pipe B -> VGA then your
change would cause us to not find the free pipe B PPS for probing eDP C,
and in the end we'd get a WARN and fall back to pipe A PPS which would
clobber the actually in use pipe A PPS.

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

Re: [Intel-gfx] [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Ville Syrjälä]

On Tue, Mar 23, 2021 at 11:39:09AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 3/2/21 3:51 PM, Ville Syrjälä wrote:
> > On Tue, Mar 02, 2021 at 01:00:40PM +0100, Hans de Goede wrote:
> >> As explained by a long comment block, on VLV intel_setup_outputs()
> >> sometimes thinks there might be an eDP panel connected while there is none.
> >> In this case intel_setup_outputs() will call intel_dp_init() to check.
> >>
> >> In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
> >> even though this might be in use for non DP purposes. When this is the case
> >> then the assert_pipe() in vlv_force_pll_on() will fail when called from
> >> vlv_power_sequencer_kick().
> > 
> > The idea is that you *can* select a PPS from a pipe used for a non-DP
> > port since those don't care about the PPS stuff. So this doesn't seem
> > correct.
> 
> They may not care about the PPS stuff, but as the WARN / backtrace
> shows if the DPLL_VCO_ENABLE bit is not already set for the pipe, while
> the pipe is "otherwise" in use then vlv_force_pll_on() becomes unhappy
> triggering the WARN.
> 
> > a) I would like to see the VBT for this machine
> 
> https://fedorapeople.org/~jwrdegoede/voyo-winpad-a15-vbt
> 
> > b) I wonder if the DSI PLL is sufficient for getting the PPS going?
> 
> I have no idea, I just noticed the WARN / backtrace and this seemed
> like a reasonably way to deal with it. With that said I'm fine with fixing
> this a different way.
> 
> > c) If we do need the normal DPLL is there any harm to DSI in enabling it?
> 
> I would assume this increases power-consumption and DSI panels are almost
> always used in battery powered devices.

This is just used while probing the panel, so power consumption is
not a concern.

> 
> Also this would impact all BYT/CHT devices, possible triggering unwanted
> side-effects. Where as the proposed fix below is much more narrowly targeted
> at the problem. It might not be the most pretty fix but AFAICT it has a low
> risk of causing regressions.

It rather significantly changes the logic of the workaround, potentially
causing us to not find a free PPS at all. Eg. if you were to boot with
a VLV with pipe A -> eDP B + eDP C inactive + pipe B -> VGA then your
change would cause us to not find the free pipe B PPS for probing eDP C,
and in the end we'd get a WARN and fall back to pipe A PPS which would
clobber the actually in use pipe A PPS.

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

Re: [Intel-gfx] [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Hans de Goede]

Hi,

On 3/24/21 3:02 PM, Ville Syrjälä wrote:
> On Tue, Mar 23, 2021 at 11:39:09AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 3/2/21 3:51 PM, Ville Syrjälä wrote:
>>> On Tue, Mar 02, 2021 at 01:00:40PM +0100, Hans de Goede wrote:
>>>> As explained by a long comment block, on VLV intel_setup_outputs()
>>>> sometimes thinks there might be an eDP panel connected while there is none.
>>>> In this case intel_setup_outputs() will call intel_dp_init() to check.
>>>>
>>>> In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
>>>> even though this might be in use for non DP purposes. When this is the case
>>>> then the assert_pipe() in vlv_force_pll_on() will fail when called from
>>>> vlv_power_sequencer_kick().
>>>
>>> The idea is that you *can* select a PPS from a pipe used for a non-DP
>>> port since those don't care about the PPS stuff. So this doesn't seem
>>> correct.
>>
>> They may not care about the PPS stuff, but as the WARN / backtrace
>> shows if the DPLL_VCO_ENABLE bit is not already set for the pipe, while
>> the pipe is "otherwise" in use then vlv_force_pll_on() becomes unhappy
>> triggering the WARN.DPLL_VCO_ENABLE bit is not
>>
>>> a) I would like to see the VBT for this machine
>>
>> https://fedorapeople.org/~jwrdegoede/voyo-winpad-a15-vbt
>>
>>> b) I wonder if the DSI PLL is sufficient for getting the PPS going?
>>
>> I have no idea, I just noticed the WARN / backtrace and this seemed
>> like a reasonably way to deal with it. With that said I'm fine with fixing
>> this a different way.
>>
>>> c) If we do need the normal DPLL is there any harm to DSI in enabling it?
>>
>> I would assume this increases power-consumption and DSI panels are almost
>> always used in battery powered devices.
> 
> This is just used while probing the panel, so power consumption is
> not a concern.

Sorry I misinterpreted what you wrote, I interpreted it as have the DSI
code enable it to avoid this problem. I see now that that is now what
you meant.

>> Also this would impact all BYT/CHT devices, possible triggering unwanted
>> side-effects. Where as the proposed fix below is much more narrowly targeted
>> at the problem. It might not be the most pretty fix but AFAICT it has a low
>> risk of causing regressions.
> 
> It rather significantly changes the logic of the workaround, potentially
> causing us to not find a free PPS at all. Eg. if you were to boot with
> a VLV with pipe A -> eDP B + eDP C inactive + pipe B -> VGA then your
> change would cause us to not find the free pipe B PPS for probing eDP C,
> and in the end we'd get a WARN and fall back to pipe A PPS which would
> clobber the actually in use pipe A PPS.

I would welcome, and will happily test, another fix for this. ATM we
have a WARN triggering on actual hardware (and not just in a hypothetical
example) and I would like to see that WARN fixed. If you can come up with
a better fix I would be happy to test.

Regards,

Hans


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Hans de Goede]

Hi,

On 3/24/21 3:02 PM, Ville Syrjälä wrote:
> On Tue, Mar 23, 2021 at 11:39:09AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 3/2/21 3:51 PM, Ville Syrjälä wrote:
>>> On Tue, Mar 02, 2021 at 01:00:40PM +0100, Hans de Goede wrote:
>>>> As explained by a long comment block, on VLV intel_setup_outputs()
>>>> sometimes thinks there might be an eDP panel connected while there is none.
>>>> In this case intel_setup_outputs() will call intel_dp_init() to check.
>>>>
>>>> In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
>>>> even though this might be in use for non DP purposes. When this is the case
>>>> then the assert_pipe() in vlv_force_pll_on() will fail when called from
>>>> vlv_power_sequencer_kick().
>>>
>>> The idea is that you *can* select a PPS from a pipe used for a non-DP
>>> port since those don't care about the PPS stuff. So this doesn't seem
>>> correct.
>>
>> They may not care about the PPS stuff, but as the WARN / backtrace
>> shows if the DPLL_VCO_ENABLE bit is not already set for the pipe, while
>> the pipe is "otherwise" in use then vlv_force_pll_on() becomes unhappy
>> triggering the WARN.DPLL_VCO_ENABLE bit is not
>>
>>> a) I would like to see the VBT for this machine
>>
>> https://fedorapeople.org/~jwrdegoede/voyo-winpad-a15-vbt
>>
>>> b) I wonder if the DSI PLL is sufficient for getting the PPS going?
>>
>> I have no idea, I just noticed the WARN / backtrace and this seemed
>> like a reasonably way to deal with it. With that said I'm fine with fixing
>> this a different way.
>>
>>> c) If we do need the normal DPLL is there any harm to DSI in enabling it?
>>
>> I would assume this increases power-consumption and DSI panels are almost
>> always used in battery powered devices.
> 
> This is just used while probing the panel, so power consumption is
> not a concern.

Sorry I misinterpreted what you wrote, I interpreted it as have the DSI
code enable it to avoid this problem. I see now that that is now what
you meant.

>> Also this would impact all BYT/CHT devices, possible triggering unwanted
>> side-effects. Where as the proposed fix below is much more narrowly targeted
>> at the problem. It might not be the most pretty fix but AFAICT it has a low
>> risk of causing regressions.
> 
> It rather significantly changes the logic of the workaround, potentially
> causing us to not find a free PPS at all. Eg. if you were to boot with
> a VLV with pipe A -> eDP B + eDP C inactive + pipe B -> VGA then your
> change would cause us to not find the free pipe B PPS for probing eDP C,
> and in the end we'd get a WARN and fall back to pipe A PPS which would
> clobber the actually in use pipe A PPS.

I would welcome, and will happily test, another fix for this. ATM we
have a WARN triggering on actual hardware (and not just in a hypothetical
example) and I would like to see that WARN fixed. If you can come up with
a better fix I would be happy to test.

Regards,

Hans


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

Re: [Intel-gfx] [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Ville Syrjälä]

On Wed, Mar 24, 2021 at 03:10:59PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 3/24/21 3:02 PM, Ville Syrjälä wrote:
> > On Tue, Mar 23, 2021 at 11:39:09AM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 3/2/21 3:51 PM, Ville Syrjälä wrote:
> >>> On Tue, Mar 02, 2021 at 01:00:40PM +0100, Hans de Goede wrote:
> >>>> As explained by a long comment block, on VLV intel_setup_outputs()
> >>>> sometimes thinks there might be an eDP panel connected while there is none.
> >>>> In this case intel_setup_outputs() will call intel_dp_init() to check.
> >>>>
> >>>> In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
> >>>> even though this might be in use for non DP purposes. When this is the case
> >>>> then the assert_pipe() in vlv_force_pll_on() will fail when called from
> >>>> vlv_power_sequencer_kick().
> >>>
> >>> The idea is that you *can* select a PPS from a pipe used for a non-DP
> >>> port since those don't care about the PPS stuff. So this doesn't seem
> >>> correct.
> >>
> >> They may not care about the PPS stuff, but as the WARN / backtrace
> >> shows if the DPLL_VCO_ENABLE bit is not already set for the pipe, while
> >> the pipe is "otherwise" in use then vlv_force_pll_on() becomes unhappy
> >> triggering the WARN.DPLL_VCO_ENABLE bit is not
> >>
> >>> a) I would like to see the VBT for this machine
> >>
> >> https://fedorapeople.org/~jwrdegoede/voyo-winpad-a15-vbt
> >>
> >>> b) I wonder if the DSI PLL is sufficient for getting the PPS going?
> >>
> >> I have no idea, I just noticed the WARN / backtrace and this seemed
> >> like a reasonably way to deal with it. With that said I'm fine with fixing
> >> this a different way.
> >>
> >>> c) If we do need the normal DPLL is there any harm to DSI in enabling it?
> >>
> >> I would assume this increases power-consumption and DSI panels are almost
> >> always used in battery powered devices.
> > 
> > This is just used while probing the panel, so power consumption is
> > not a concern.
> 
> Sorry I misinterpreted what you wrote, I interpreted it as have the DSI
> code enable it to avoid this problem. I see now that that is now what
> you meant.
> 
> >> Also this would impact all BYT/CHT devices, possible triggering unwanted
> >> side-effects. Where as the proposed fix below is much more narrowly targeted
> >> at the problem. It might not be the most pretty fix but AFAICT it has a low
> >> risk of causing regressions.
> > 
> > It rather significantly changes the logic of the workaround, potentially
> > causing us to not find a free PPS at all. Eg. if you were to boot with
> > a VLV with pipe A -> eDP B + eDP C inactive + pipe B -> VGA then your
> > change would cause us to not find the free pipe B PPS for probing eDP C,
> > and in the end we'd get a WARN and fall back to pipe A PPS which would
> > clobber the actually in use pipe A PPS.
> 
> I would welcome, and will happily test, another fix for this. ATM we
> have a WARN triggering on actual hardware (and not just in a hypothetical
> example) and I would like to see that WARN fixed. If you can come up with
> a better fix I would be happy to test.

Well, I think there are a couple things we want to experiment wiht:

a) Just skip the asserts and see if enabling the DPLL/poking the PPS
   perturbs the DSI output in any way.

--- a/drivers/gpu/drm/i915/display/intel_dpll.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll.c
@@ -1467,7 +1467,7 @@ void vlv_enable_pll(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum pipe pipe = crtc->pipe;
 
-	assert_pipe_disabled(dev_priv, pipe_config->cpu_transcoder);
+	//assert_pipe_disabled(dev_priv, pipe_config->cpu_transcoder);
 
 	/* PLL is protected by panel, make sure we can write it */
 	assert_panel_unlocked(dev_priv, pipe);
@@ -1800,7 +1800,7 @@ void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
 	u32 val;
 
 	/* Make sure the pipe isn't still relying on us */
-	assert_pipe_disabled(dev_priv, (enum transcoder)pipe);
+	//assert_pipe_disabled(dev_priv, (enum transcoder)pipe);
 
 	val = DPLL_INTEGRATED_REF_CLK_VLV |
 		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -110,6 +110,8 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 	intel_de_write(dev_priv, intel_dp->output_reg, DP & ~DP_PORT_EN);
 	intel_de_posting_read(dev_priv, intel_dp->output_reg);
 
+	msleep(1000); // just to make sure we keep angering DSI for a bit longer
+
 	if (!pll_enabled) {
 		vlv_force_pll_off(dev_priv, pipe);
 

b) Don't enable the DPLL at all and see if the DSI PLL is capable of
   clocking the PPS. My gut feeling says this will not work and we
   should see the PPS state machine not make progress, but not sure.

--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -77,7 +77,7 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 	else
 		DP |= DP_PIPE_SEL(pipe);
 
-	pll_enabled = intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE;
+	pll_enabled = true;//intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE;
 
 	/*
 	 * The DPLL for the pipe must be enabled for this to work.

I do have DSI VLV machine at the office, so I can also try to poke it a
bit next time I'm at the office.

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

Re: [Intel-gfx] [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Ville Syrjälä]

On Wed, Mar 24, 2021 at 03:10:59PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 3/24/21 3:02 PM, Ville Syrjälä wrote:
> > On Tue, Mar 23, 2021 at 11:39:09AM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 3/2/21 3:51 PM, Ville Syrjälä wrote:
> >>> On Tue, Mar 02, 2021 at 01:00:40PM +0100, Hans de Goede wrote:
> >>>> As explained by a long comment block, on VLV intel_setup_outputs()
> >>>> sometimes thinks there might be an eDP panel connected while there is none.
> >>>> In this case intel_setup_outputs() will call intel_dp_init() to check.
> >>>>
> >>>> In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
> >>>> even though this might be in use for non DP purposes. When this is the case
> >>>> then the assert_pipe() in vlv_force_pll_on() will fail when called from
> >>>> vlv_power_sequencer_kick().
> >>>
> >>> The idea is that you *can* select a PPS from a pipe used for a non-DP
> >>> port since those don't care about the PPS stuff. So this doesn't seem
> >>> correct.
> >>
> >> They may not care about the PPS stuff, but as the WARN / backtrace
> >> shows if the DPLL_VCO_ENABLE bit is not already set for the pipe, while
> >> the pipe is "otherwise" in use then vlv_force_pll_on() becomes unhappy
> >> triggering the WARN.DPLL_VCO_ENABLE bit is not
> >>
> >>> a) I would like to see the VBT for this machine
> >>
> >> https://fedorapeople.org/~jwrdegoede/voyo-winpad-a15-vbt
> >>
> >>> b) I wonder if the DSI PLL is sufficient for getting the PPS going?
> >>
> >> I have no idea, I just noticed the WARN / backtrace and this seemed
> >> like a reasonably way to deal with it. With that said I'm fine with fixing
> >> this a different way.
> >>
> >>> c) If we do need the normal DPLL is there any harm to DSI in enabling it?
> >>
> >> I would assume this increases power-consumption and DSI panels are almost
> >> always used in battery powered devices.
> > 
> > This is just used while probing the panel, so power consumption is
> > not a concern.
> 
> Sorry I misinterpreted what you wrote, I interpreted it as have the DSI
> code enable it to avoid this problem. I see now that that is now what
> you meant.
> 
> >> Also this would impact all BYT/CHT devices, possible triggering unwanted
> >> side-effects. Where as the proposed fix below is much more narrowly targeted
> >> at the problem. It might not be the most pretty fix but AFAICT it has a low
> >> risk of causing regressions.
> > 
> > It rather significantly changes the logic of the workaround, potentially
> > causing us to not find a free PPS at all. Eg. if you were to boot with
> > a VLV with pipe A -> eDP B + eDP C inactive + pipe B -> VGA then your
> > change would cause us to not find the free pipe B PPS for probing eDP C,
> > and in the end we'd get a WARN and fall back to pipe A PPS which would
> > clobber the actually in use pipe A PPS.
> 
> I would welcome, and will happily test, another fix for this. ATM we
> have a WARN triggering on actual hardware (and not just in a hypothetical
> example) and I would like to see that WARN fixed. If you can come up with
> a better fix I would be happy to test.

Well, I think there are a couple things we want to experiment wiht:

a) Just skip the asserts and see if enabling the DPLL/poking the PPS
   perturbs the DSI output in any way.

--- a/drivers/gpu/drm/i915/display/intel_dpll.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll.c
@@ -1467,7 +1467,7 @@ void vlv_enable_pll(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum pipe pipe = crtc->pipe;
 
-	assert_pipe_disabled(dev_priv, pipe_config->cpu_transcoder);
+	//assert_pipe_disabled(dev_priv, pipe_config->cpu_transcoder);
 
 	/* PLL is protected by panel, make sure we can write it */
 	assert_panel_unlocked(dev_priv, pipe);
@@ -1800,7 +1800,7 @@ void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
 	u32 val;
 
 	/* Make sure the pipe isn't still relying on us */
-	assert_pipe_disabled(dev_priv, (enum transcoder)pipe);
+	//assert_pipe_disabled(dev_priv, (enum transcoder)pipe);
 
 	val = DPLL_INTEGRATED_REF_CLK_VLV |
 		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -110,6 +110,8 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 	intel_de_write(dev_priv, intel_dp->output_reg, DP & ~DP_PORT_EN);
 	intel_de_posting_read(dev_priv, intel_dp->output_reg);
 
+	msleep(1000); // just to make sure we keep angering DSI for a bit longer
+
 	if (!pll_enabled) {
 		vlv_force_pll_off(dev_priv, pipe);
 

b) Don't enable the DPLL at all and see if the DSI PLL is capable of
   clocking the PPS. My gut feeling says this will not work and we
   should see the PPS state machine not make progress, but not sure.

--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -77,7 +77,7 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 	else
 		DP |= DP_PIPE_SEL(pipe);
 
-	pll_enabled = intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE;
+	pll_enabled = true;//intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE;
 
 	/*
 	 * The DPLL for the pipe must be enabled for this to work.

I do have DSI VLV machine at the office, so I can also try to poke it a
bit next time I'm at the office.

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

Re: [Intel-gfx] [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Ville Syrjälä]

On Wed, Mar 24, 2021 at 04:37:14PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 24, 2021 at 03:10:59PM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 3/24/21 3:02 PM, Ville Syrjälä wrote:
> > > On Tue, Mar 23, 2021 at 11:39:09AM +0100, Hans de Goede wrote:
> > >> Hi,
> > >>
> > >> On 3/2/21 3:51 PM, Ville Syrjälä wrote:
> > >>> On Tue, Mar 02, 2021 at 01:00:40PM +0100, Hans de Goede wrote:
> > >>>> As explained by a long comment block, on VLV intel_setup_outputs()
> > >>>> sometimes thinks there might be an eDP panel connected while there is none.
> > >>>> In this case intel_setup_outputs() will call intel_dp_init() to check.
> > >>>>
> > >>>> In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
> > >>>> even though this might be in use for non DP purposes. When this is the case
> > >>>> then the assert_pipe() in vlv_force_pll_on() will fail when called from
> > >>>> vlv_power_sequencer_kick().
> > >>>
> > >>> The idea is that you *can* select a PPS from a pipe used for a non-DP
> > >>> port since those don't care about the PPS stuff. So this doesn't seem
> > >>> correct.
> > >>
> > >> They may not care about the PPS stuff, but as the WARN / backtrace
> > >> shows if the DPLL_VCO_ENABLE bit is not already set for the pipe, while
> > >> the pipe is "otherwise" in use then vlv_force_pll_on() becomes unhappy
> > >> triggering the WARN.DPLL_VCO_ENABLE bit is not
> > >>
> > >>> a) I would like to see the VBT for this machine
> > >>
> > >> https://fedorapeople.org/~jwrdegoede/voyo-winpad-a15-vbt
> > >>
> > >>> b) I wonder if the DSI PLL is sufficient for getting the PPS going?
> > >>
> > >> I have no idea, I just noticed the WARN / backtrace and this seemed
> > >> like a reasonably way to deal with it. With that said I'm fine with fixing
> > >> this a different way.
> > >>
> > >>> c) If we do need the normal DPLL is there any harm to DSI in enabling it?
> > >>
> > >> I would assume this increases power-consumption and DSI panels are almost
> > >> always used in battery powered devices.
> > > 
> > > This is just used while probing the panel, so power consumption is
> > > not a concern.
> > 
> > Sorry I misinterpreted what you wrote, I interpreted it as have the DSI
> > code enable it to avoid this problem. I see now that that is now what
> > you meant.
> > 
> > >> Also this would impact all BYT/CHT devices, possible triggering unwanted
> > >> side-effects. Where as the proposed fix below is much more narrowly targeted
> > >> at the problem. It might not be the most pretty fix but AFAICT it has a low
> > >> risk of causing regressions.
> > > 
> > > It rather significantly changes the logic of the workaround, potentially
> > > causing us to not find a free PPS at all. Eg. if you were to boot with
> > > a VLV with pipe A -> eDP B + eDP C inactive + pipe B -> VGA then your
> > > change would cause us to not find the free pipe B PPS for probing eDP C,
> > > and in the end we'd get a WARN and fall back to pipe A PPS which would
> > > clobber the actually in use pipe A PPS.
> > 
> > I would welcome, and will happily test, another fix for this. ATM we
> > have a WARN triggering on actual hardware (and not just in a hypothetical
> > example) and I would like to see that WARN fixed. If you can come up with
> > a better fix I would be happy to test.
> 
> Well, I think there are a couple things we want to experiment wiht:
> 
> a) Just skip the asserts and see if enabling the DPLL/poking the PPS
>    perturbs the DSI output in any way.
> 
> --- a/drivers/gpu/drm/i915/display/intel_dpll.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll.c
> @@ -1467,7 +1467,7 @@ void vlv_enable_pll(struct intel_crtc *crtc,
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	enum pipe pipe = crtc->pipe;
>  
> -	assert_pipe_disabled(dev_priv, pipe_config->cpu_transcoder);
> +	//assert_pipe_disabled(dev_priv, pipe_config->cpu_transcoder);
>  
>  	/* PLL is protected by panel, make sure we can write it */
>  	assert_panel_unlocked(dev_priv, pipe);
> @@ -1800,7 +1800,7 @@ void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	u32 val;
>  
>  	/* Make sure the pipe isn't still relying on us */
> -	assert_pipe_disabled(dev_priv, (enum transcoder)pipe);
> +	//assert_pipe_disabled(dev_priv, (enum transcoder)pipe);
>  
>  	val = DPLL_INTEGRATED_REF_CLK_VLV |
>  		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -110,6 +110,8 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
>  	intel_de_write(dev_priv, intel_dp->output_reg, DP & ~DP_PORT_EN);
>  	intel_de_posting_read(dev_priv, intel_dp->output_reg);
>  
> +	msleep(1000); // just to make sure we keep angering DSI for a bit longer
> +
>  	if (!pll_enabled) {
>  		vlv_force_pll_off(dev_priv, pipe);
>  
> 
> b) Don't enable the DPLL at all and see if the DSI PLL is capable of
>    clocking the PPS. My gut feeling says this will not work and we
>    should see the PPS state machine not make progress, but not sure.
> 
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -77,7 +77,7 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
>  	else
>  		DP |= DP_PIPE_SEL(pipe);
>  
> -	pll_enabled = intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE;
> +	pll_enabled = true;//intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE;
>  
>  	/*
>  	 * The DPLL for the pipe must be enabled for this to work.
> 
> I do have DSI VLV machine at the office, so I can also try to poke it a
> bit next time I'm at the office.

Finally managed to frob the VLV DSI a bit.

I pushed a couple of ideas here:
git://github.com/vsyrjala/linux.git vlv_pps_vs_dsi

The first patch just skips the DPLL forcing if the pipe
is already on. At least the power sequencer didn't get totally
upset about it, but it can cause some visual issues for the DSI
output; the screen just goes all gray while the DP port is 
temporarily enabled, though I guess that might also happen
with HDMI/CRT outputs, haven't actually double checked that
so not sure.

The other more complicated patch tries to avoid touching
the PPS on any pipe currently driving DSI. Not sure if it's
worth the extra complexity for this niche use case if the
simple approach of just not forcing the DPLL works well
enough.

The third approach I didn't even impelement would be just declare
DSI and eDP mutually exclusive. But in theory the display engine
at least supports such mixed configurations, not so sure about
the pin muxing on SoC though.

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

Re: [Intel-gfx] [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Ville Syrjälä]

On Wed, Mar 24, 2021 at 04:37:14PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 24, 2021 at 03:10:59PM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 3/24/21 3:02 PM, Ville Syrjälä wrote:
> > > On Tue, Mar 23, 2021 at 11:39:09AM +0100, Hans de Goede wrote:
> > >> Hi,
> > >>
> > >> On 3/2/21 3:51 PM, Ville Syrjälä wrote:
> > >>> On Tue, Mar 02, 2021 at 01:00:40PM +0100, Hans de Goede wrote:
> > >>>> As explained by a long comment block, on VLV intel_setup_outputs()
> > >>>> sometimes thinks there might be an eDP panel connected while there is none.
> > >>>> In this case intel_setup_outputs() will call intel_dp_init() to check.
> > >>>>
> > >>>> In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
> > >>>> even though this might be in use for non DP purposes. When this is the case
> > >>>> then the assert_pipe() in vlv_force_pll_on() will fail when called from
> > >>>> vlv_power_sequencer_kick().
> > >>>
> > >>> The idea is that you *can* select a PPS from a pipe used for a non-DP
> > >>> port since those don't care about the PPS stuff. So this doesn't seem
> > >>> correct.
> > >>
> > >> They may not care about the PPS stuff, but as the WARN / backtrace
> > >> shows if the DPLL_VCO_ENABLE bit is not already set for the pipe, while
> > >> the pipe is "otherwise" in use then vlv_force_pll_on() becomes unhappy
> > >> triggering the WARN.DPLL_VCO_ENABLE bit is not
> > >>
> > >>> a) I would like to see the VBT for this machine
> > >>
> > >> https://fedorapeople.org/~jwrdegoede/voyo-winpad-a15-vbt
> > >>
> > >>> b) I wonder if the DSI PLL is sufficient for getting the PPS going?
> > >>
> > >> I have no idea, I just noticed the WARN / backtrace and this seemed
> > >> like a reasonably way to deal with it. With that said I'm fine with fixing
> > >> this a different way.
> > >>
> > >>> c) If we do need the normal DPLL is there any harm to DSI in enabling it?
> > >>
> > >> I would assume this increases power-consumption and DSI panels are almost
> > >> always used in battery powered devices.
> > > 
> > > This is just used while probing the panel, so power consumption is
> > > not a concern.
> > 
> > Sorry I misinterpreted what you wrote, I interpreted it as have the DSI
> > code enable it to avoid this problem. I see now that that is now what
> > you meant.
> > 
> > >> Also this would impact all BYT/CHT devices, possible triggering unwanted
> > >> side-effects. Where as the proposed fix below is much more narrowly targeted
> > >> at the problem. It might not be the most pretty fix but AFAICT it has a low
> > >> risk of causing regressions.
> > > 
> > > It rather significantly changes the logic of the workaround, potentially
> > > causing us to not find a free PPS at all. Eg. if you were to boot with
> > > a VLV with pipe A -> eDP B + eDP C inactive + pipe B -> VGA then your
> > > change would cause us to not find the free pipe B PPS for probing eDP C,
> > > and in the end we'd get a WARN and fall back to pipe A PPS which would
> > > clobber the actually in use pipe A PPS.
> > 
> > I would welcome, and will happily test, another fix for this. ATM we
> > have a WARN triggering on actual hardware (and not just in a hypothetical
> > example) and I would like to see that WARN fixed. If you can come up with
> > a better fix I would be happy to test.
> 
> Well, I think there are a couple things we want to experiment wiht:
> 
> a) Just skip the asserts and see if enabling the DPLL/poking the PPS
>    perturbs the DSI output in any way.
> 
> --- a/drivers/gpu/drm/i915/display/intel_dpll.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll.c
> @@ -1467,7 +1467,7 @@ void vlv_enable_pll(struct intel_crtc *crtc,
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	enum pipe pipe = crtc->pipe;
>  
> -	assert_pipe_disabled(dev_priv, pipe_config->cpu_transcoder);
> +	//assert_pipe_disabled(dev_priv, pipe_config->cpu_transcoder);
>  
>  	/* PLL is protected by panel, make sure we can write it */
>  	assert_panel_unlocked(dev_priv, pipe);
> @@ -1800,7 +1800,7 @@ void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	u32 val;
>  
>  	/* Make sure the pipe isn't still relying on us */
> -	assert_pipe_disabled(dev_priv, (enum transcoder)pipe);
> +	//assert_pipe_disabled(dev_priv, (enum transcoder)pipe);
>  
>  	val = DPLL_INTEGRATED_REF_CLK_VLV |
>  		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -110,6 +110,8 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
>  	intel_de_write(dev_priv, intel_dp->output_reg, DP & ~DP_PORT_EN);
>  	intel_de_posting_read(dev_priv, intel_dp->output_reg);
>  
> +	msleep(1000); // just to make sure we keep angering DSI for a bit longer
> +
>  	if (!pll_enabled) {
>  		vlv_force_pll_off(dev_priv, pipe);
>  
> 
> b) Don't enable the DPLL at all and see if the DSI PLL is capable of
>    clocking the PPS. My gut feeling says this will not work and we
>    should see the PPS state machine not make progress, but not sure.
> 
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -77,7 +77,7 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
>  	else
>  		DP |= DP_PIPE_SEL(pipe);
>  
> -	pll_enabled = intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE;
> +	pll_enabled = true;//intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE;
>  
>  	/*
>  	 * The DPLL for the pipe must be enabled for this to work.
> 
> I do have DSI VLV machine at the office, so I can also try to poke it a
> bit next time I'm at the office.

Finally managed to frob the VLV DSI a bit.

I pushed a couple of ideas here:
git://github.com/vsyrjala/linux.git vlv_pps_vs_dsi

The first patch just skips the DPLL forcing if the pipe
is already on. At least the power sequencer didn't get totally
upset about it, but it can cause some visual issues for the DSI
output; the screen just goes all gray while the DP port is 
temporarily enabled, though I guess that might also happen
with HDMI/CRT outputs, haven't actually double checked that
so not sure.

The other more complicated patch tries to avoid touching
the PPS on any pipe currently driving DSI. Not sure if it's
worth the extra complexity for this niche use case if the
simple approach of just not forcing the DPLL works well
enough.

The third approach I didn't even impelement would be just declare
DSI and eDP mutually exclusive. But in theory the display engine
at least supports such mixed configurations, not so sure about
the pin muxing on SoC though.

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

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Patchwork]

== Series Details ==

Series: drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes
URL   : https://patchwork.freedesktop.org/series/87542/
State : warning

== Summary ==

$ dim sparse --fast origin/drm-tip
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.
-
+drivers/gpu/drm/i915/gt/intel_reset.c:1323:5: warning: context imbalance in 'intel_gt_reset_trylock' - different lock contexts for basic block
+drivers/gpu/drm/i915/gvt/mmio.c:295:23: warning: memcpy with byte count of 279040
+drivers/gpu/drm/i915/i915_perf.c:1437:15: warning: memset with byte count of 16777216
+drivers/gpu/drm/i915/i915_perf.c:1491:15: warning: memset with byte count of 16777216
+drivers/gpu/drm/i915/intel_wakeref.c:137:19: warning: context imbalance in 'wakeref_auto_timeout' - unexpected unlock
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'fwtable_read16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'fwtable_read32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'fwtable_read64' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'fwtable_read8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'fwtable_write16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'fwtable_write32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'fwtable_write8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen11_fwtable_read16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen11_fwtable_read32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen11_fwtable_read64' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen11_fwtable_read8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen11_fwtable_write16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen11_fwtable_write32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen11_fwtable_write8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen12_fwtable_read16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen12_fwtable_read32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen12_fwtable_read64' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen12_fwtable_read8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen12_fwtable_write16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen12_fwtable_write32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen12_fwtable_write8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen6_read16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen6_read32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen6_read64' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen6_read8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen6_write16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen6_write32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen6_write8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen8_write16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen8_write32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen8_write8' - different lock contexts for basic block


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

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Patchwork]

[-- Attachment #1.1: Type: text/plain, Size: 4458 bytes --]

== Series Details ==

Series: drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes
URL   : https://patchwork.freedesktop.org/series/87542/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_9820 -> Patchwork_19744
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19744/index.html

Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_cs_nop@sync-fork-compute0:
    - fi-snb-2600:        NOTRUN -> [SKIP][1] ([fdo#109271]) +17 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19744/fi-snb-2600/igt@amdgpu/amd_cs_nop@sync-fork-compute0.html

  * igt@amdgpu/amd_cs_nop@sync-gfx0:
    - fi-bsw-n3050:       NOTRUN -> [SKIP][2] ([fdo#109271]) +17 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19744/fi-bsw-n3050/igt@amdgpu/amd_cs_nop@sync-gfx0.html

  * igt@i915_selftest@live@execlists:
    - fi-bsw-kefka:       [PASS][3] -> [INCOMPLETE][4] ([i915#2940])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9820/fi-bsw-kefka/igt@i915_selftest@live@execlists.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19744/fi-bsw-kefka/igt@i915_selftest@live@execlists.html

  * igt@prime_vgem@basic-read:
    - fi-tgl-y:           [PASS][5] -> [DMESG-WARN][6] ([i915#402]) +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9820/fi-tgl-y/igt@prime_vgem@basic-read.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19744/fi-tgl-y/igt@prime_vgem@basic-read.html

  * igt@runner@aborted:
    - fi-bsw-kefka:       NOTRUN -> [FAIL][7] ([i915#1436])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19744/fi-bsw-kefka/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@gem_exec_create@basic:
    - fi-tgl-y:           [DMESG-WARN][8] ([i915#402]) -> [PASS][9] +1 similar issue
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9820/fi-tgl-y/igt@gem_exec_create@basic.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19744/fi-tgl-y/igt@gem_exec_create@basic.html

  * igt@gem_tiled_blits@basic:
    - fi-kbl-8809g:       [TIMEOUT][10] -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9820/fi-kbl-8809g/igt@gem_tiled_blits@basic.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19744/fi-kbl-8809g/igt@gem_tiled_blits@basic.html

  * igt@i915_selftest@live@execlists:
    - fi-bsw-n3050:       [INCOMPLETE][12] ([i915#2940]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9820/fi-bsw-n3050/igt@i915_selftest@live@execlists.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19744/fi-bsw-n3050/igt@i915_selftest@live@execlists.html

  * igt@i915_selftest@live@hangcheck:
    - fi-snb-2600:        [INCOMPLETE][14] ([i915#2782]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9820/fi-snb-2600/igt@i915_selftest@live@hangcheck.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19744/fi-snb-2600/igt@i915_selftest@live@hangcheck.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1436]: https://gitlab.freedesktop.org/drm/intel/issues/1436
  [i915#2782]: https://gitlab.freedesktop.org/drm/intel/issues/2782
  [i915#2940]: https://gitlab.freedesktop.org/drm/intel/issues/2940
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402


Participating hosts (45 -> 41)
------------------------------

  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-bsw-cyan fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_9820 -> Patchwork_19744

  CI-20190529: 20190529
  CI_DRM_9820: 9761fb66bc43d298a90c7dd353d376d0802e6e79 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6017: dc9f6d13f4ddea5286ed4723627b79db1e2210d2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_19744: 428b36184c012f3566040d6a61888dd549d509cd @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

428b36184c01 drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes
e0ab76cefc03 drm/i915/display: Add a intel_pipe_is_enabled() helper

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19744/index.html

[-- Attachment #1.2: Type: text/html, Size: 5493 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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