All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] drm/i915: Fix assert_plane() warning on bootup with external display
@ 2018-06-18  5:18 Azhar Shaikh
  2018-06-18  5:46 ` ✗ Fi.CI.BAT: failure for " Patchwork
  2018-06-18  8:18 ` [RFC] " Jani Nikula
  0 siblings, 2 replies; 8+ messages in thread
From: Azhar Shaikh @ 2018-06-18  5:18 UTC (permalink / raw)
  To: intel-gfx

On KBL, WHL RVPs, booting up with an external display connected, triggers
below warning. This warning is not seen during hotplug.

[    3.615226] ------------[ cut here ]------------
[    3.619829] plane 1A assertion failure (expected on, current off)
[    3.632039] WARNING: CPU: 2 PID: 354 at drivers/gpu/drm/i915/intel_display.c:1294 assert_plane+0x71/0xbb
[    3.633920] iwlwifi 0000:00:14.3: loaded firmware version 38.c0e03d94.0 op_mode iwlmvm
[    3.647157] Modules linked in: iwlwifi cfg80211 btusb btrtl btbcm btintel bluetooth ecdh_generic
[    3.647163] CPU: 2 PID: 354 Comm: frecon Not tainted 4.17.0-rc7-50176-g655af12d39c2 #3
[    3.647165] Hardware name: Intel Corporation CoffeeLake Client Platform/WhiskeyLake U DDR4 ERB, BIOS CNLSFWR1.R00.X140.B00.1804040304 04/04/2018
[    3.684509] RIP: 0010:assert_plane+0x71/0xbb
[    3.764451] Call Trace:
[    3.766888]  intel_atomic_commit_tail+0xa97/0xb77
[    3.771569]  intel_atomic_commit+0x26a/0x279
[    3.771572]  drm_atomic_helper_set_config+0x5c/0x76
[    3.780670]  __drm_mode_set_config_internal+0x66/0x109
[    3.780672]  drm_mode_setcrtc+0x4c9/0x5cc
[    3.780674]  ? drm_mode_getcrtc+0x162/0x162
[    3.789774]  ? drm_mode_getcrtc+0x162/0x162
[    3.798108]  drm_ioctl_kernel+0x8d/0xe4
[    3.801926]  drm_ioctl+0x27d/0x368
[    3.805311]  ? drm_mode_getcrtc+0x162/0x162
[    3.805314]  ? selinux_file_ioctl+0x14e/0x199
[    3.805317]  vfs_ioctl+0x21/0x2f
[    3.813812]  do_vfs_ioctl+0x491/0x4b4
[    3.813813]  ? security_file_ioctl+0x37/0x4b
[    3.813816]  ksys_ioctl+0x55/0x75
[    3.820672]  __x64_sys_ioctl+0x1a/0x1e
[    3.820674]  do_syscall_64+0x51/0x5f
[    3.820678]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[    3.828221] RIP: 0033:0x7b5e04953967
[    3.835504] RSP: 002b:00007fff2eafb6f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[    3.835505] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007b5e04953967
[    3.835505] RDX: 00007fff2eafb730 RSI: 00000000c06864a2 RDI: 000000000000000f
[    3.835506] RBP: 00007fff2eafb720 R08: 0000000000000000 R09: 0000000000000000
[    3.835507] R10: 0000000000000070 R11: 0000000000000246 R12: 000000000000000f
[    3.879988] R13: 000056bc9dd7d210 R14: 00007fff2eafb730 R15: 00000000c06864a2
[    3.887081] Code: 48 c7 c7 06 71 a5 be 84 c0 48 c7 c2 06 fd a3 be 48 89 f9 48 0f 44 ca 84 db 48 0f 45 d7 48 c7 c7 df d3 a4 be 31 c0 e8 af a0 c0 ff <0f> 0b eb 2b 48 c7 c7 06 fd a3 be 84 c0 48 c7 c2 06 71 a5 be 48
[    3.905845] WARNING: CPU: 2 PID: 354 at drivers/gpu/drm/i915/intel_display.c:1294 assert_plane+0x71/0xbb
[    3.920964] ---[ end trace dac692f4ac46391a ]---

The warning is seen when mode_setcrtc() is called for pipeB
during bootup and before we get a mode_setcrtc() for pipeA,
while doing update_crtcs() in intel_atomic_commit_tail().
Now since, plane1A is still active after commit, update_crtcs()
is done for pipeA and eventually update_plane() for plane1A.

intel_plane_state->ctl for plane1A is not updated since set_modecrtc() is
called for pipeB. So intel_plane_state->ctl for plane 1A will be 0x0.
So doing an update_plane() for plane1A, will result in clearing
PLANE_CTL_ENABLE bit, and hence the warning.

To fix this warning, prior to updating the PLANE_CTL register with
intel_plane_state->ctl value, read PLANE_CTL register, OR it with
intel_plane_state->ctl and then write it to PLANE_CTL register
instead of just relying on intel_plane_state->ctl value.

Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 344c0e709b19..b491b1fbdea1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -254,6 +254,7 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
 	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
 	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
 	unsigned long irqflags;
+	u32 val;
 
 	/* Sizes are 0 based */
 	src_w--;
@@ -322,6 +323,8 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
 		I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x);
 	}
 
+	val = I915_READ_FW(PLANE_CTL(pipe, plane_id));
+	plane_ctl |= val;
 	I915_WRITE_FW(PLANE_CTL(pipe, plane_id), plane_ctl);
 	I915_WRITE_FW(PLANE_SURF(pipe, plane_id),
 		      intel_plane_ggtt_offset(plane_state) + surf_addr);
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* ✗ Fi.CI.BAT: failure for drm/i915: Fix assert_plane() warning on bootup with external display
  2018-06-18  5:18 [RFC] drm/i915: Fix assert_plane() warning on bootup with external display Azhar Shaikh
@ 2018-06-18  5:46 ` Patchwork
  2018-06-18  8:18 ` [RFC] " Jani Nikula
  1 sibling, 0 replies; 8+ messages in thread
From: Patchwork @ 2018-06-18  5:46 UTC (permalink / raw)
  To: Azhar Shaikh; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Fix assert_plane() warning on bootup with external display
URL   : https://patchwork.freedesktop.org/series/44909/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4331 -> Patchwork_9344 =

== Summary - FAILURE ==

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

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

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@pm_backlight@basic-brightness:
      fi-cnl-psr:         PASS -> DMESG-WARN

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_gttfill@basic:
      fi-byt-n2820:       PASS -> FAIL (fdo#106744)

    igt@kms_flip@basic-flip-vs-dpms:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106097) +1

    igt@kms_flip@basic-flip-vs-modeset:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000)

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

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         NOTRUN -> INCOMPLETE (fdo#103927)

    
    ==== Possible fixes ====

    igt@gem_ringfill@basic-default-hang:
      fi-elk-e7500:       DMESG-WARN -> PASS

    igt@kms_chamelium@dp-crc-fast:
      fi-kbl-7500u:       DMESG-FAIL (fdo#103841) -> PASS

    igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
      fi-glk-j4005:       FAIL (fdo#106765) -> PASS

    
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
  fdo#106744 https://bugs.freedesktop.org/show_bug.cgi?id=106744
  fdo#106765 https://bugs.freedesktop.org/show_bug.cgi?id=106765


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

  Additional (1): fi-bxt-dsi 
  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-bwr-2160 fi-ilk-650 fi-ctg-p8600 


== Build changes ==

    * Linux: CI_DRM_4331 -> Patchwork_9344

  CI_DRM_4331: e47233f783ee68a864c2a10216735aca62d642da @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4522: 077c6f7c3786334c5e5c34888ab446fdb4347331 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9344: ff8437bc0ea867a61d3c5ee8c73a0eec6335df4d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

ff8437bc0ea8 drm/i915: Fix assert_plane() warning on bootup with external display

== Logs ==

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] drm/i915: Fix assert_plane() warning on bootup with external display
  2018-06-18  5:18 [RFC] drm/i915: Fix assert_plane() warning on bootup with external display Azhar Shaikh
  2018-06-18  5:46 ` ✗ Fi.CI.BAT: failure for " Patchwork
@ 2018-06-18  8:18 ` Jani Nikula
  2018-06-18 11:57   ` Ville Syrjälä
  1 sibling, 1 reply; 8+ messages in thread
From: Jani Nikula @ 2018-06-18  8:18 UTC (permalink / raw)
  To: Azhar Shaikh, intel-gfx

On Sun, 17 Jun 2018, Azhar Shaikh <azhar.shaikh@intel.com> wrote:
> On KBL, WHL RVPs, booting up with an external display connected, triggers
> below warning. This warning is not seen during hotplug.
>
> [    3.615226] ------------[ cut here ]------------
> [    3.619829] plane 1A assertion failure (expected on, current off)
> [    3.632039] WARNING: CPU: 2 PID: 354 at drivers/gpu/drm/i915/intel_display.c:1294 assert_plane+0x71/0xbb
> [    3.633920] iwlwifi 0000:00:14.3: loaded firmware version 38.c0e03d94.0 op_mode iwlmvm
> [    3.647157] Modules linked in: iwlwifi cfg80211 btusb btrtl btbcm btintel bluetooth ecdh_generic
> [    3.647163] CPU: 2 PID: 354 Comm: frecon Not tainted 4.17.0-rc7-50176-g655af12d39c2 #3
> [    3.647165] Hardware name: Intel Corporation CoffeeLake Client Platform/WhiskeyLake U DDR4 ERB, BIOS CNLSFWR1.R00.X140.B00.1804040304 04/04/2018
> [    3.684509] RIP: 0010:assert_plane+0x71/0xbb
> [    3.764451] Call Trace:
> [    3.766888]  intel_atomic_commit_tail+0xa97/0xb77
> [    3.771569]  intel_atomic_commit+0x26a/0x279
> [    3.771572]  drm_atomic_helper_set_config+0x5c/0x76
> [    3.780670]  __drm_mode_set_config_internal+0x66/0x109
> [    3.780672]  drm_mode_setcrtc+0x4c9/0x5cc
> [    3.780674]  ? drm_mode_getcrtc+0x162/0x162
> [    3.789774]  ? drm_mode_getcrtc+0x162/0x162
> [    3.798108]  drm_ioctl_kernel+0x8d/0xe4
> [    3.801926]  drm_ioctl+0x27d/0x368
> [    3.805311]  ? drm_mode_getcrtc+0x162/0x162
> [    3.805314]  ? selinux_file_ioctl+0x14e/0x199
> [    3.805317]  vfs_ioctl+0x21/0x2f
> [    3.813812]  do_vfs_ioctl+0x491/0x4b4
> [    3.813813]  ? security_file_ioctl+0x37/0x4b
> [    3.813816]  ksys_ioctl+0x55/0x75
> [    3.820672]  __x64_sys_ioctl+0x1a/0x1e
> [    3.820674]  do_syscall_64+0x51/0x5f
> [    3.820678]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [    3.828221] RIP: 0033:0x7b5e04953967
> [    3.835504] RSP: 002b:00007fff2eafb6f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> [    3.835505] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007b5e04953967
> [    3.835505] RDX: 00007fff2eafb730 RSI: 00000000c06864a2 RDI: 000000000000000f
> [    3.835506] RBP: 00007fff2eafb720 R08: 0000000000000000 R09: 0000000000000000
> [    3.835507] R10: 0000000000000070 R11: 0000000000000246 R12: 000000000000000f
> [    3.879988] R13: 000056bc9dd7d210 R14: 00007fff2eafb730 R15: 00000000c06864a2
> [    3.887081] Code: 48 c7 c7 06 71 a5 be 84 c0 48 c7 c2 06 fd a3 be 48 89 f9 48 0f 44 ca 84 db 48 0f 45 d7 48 c7 c7 df d3 a4 be 31 c0 e8 af a0 c0 ff <0f> 0b eb 2b 48 c7 c7 06 fd a3 be 84 c0 48 c7 c2 06 71 a5 be 48
> [    3.905845] WARNING: CPU: 2 PID: 354 at drivers/gpu/drm/i915/intel_display.c:1294 assert_plane+0x71/0xbb
> [    3.920964] ---[ end trace dac692f4ac46391a ]---
>
> The warning is seen when mode_setcrtc() is called for pipeB
> during bootup and before we get a mode_setcrtc() for pipeA,
> while doing update_crtcs() in intel_atomic_commit_tail().
> Now since, plane1A is still active after commit, update_crtcs()
> is done for pipeA and eventually update_plane() for plane1A.
>
> intel_plane_state->ctl for plane1A is not updated since set_modecrtc() is
> called for pipeB. So intel_plane_state->ctl for plane 1A will be 0x0.
> So doing an update_plane() for plane1A, will result in clearing
> PLANE_CTL_ENABLE bit, and hence the warning.
>
> To fix this warning, prior to updating the PLANE_CTL register with
> intel_plane_state->ctl value, read PLANE_CTL register, OR it with
> intel_plane_state->ctl and then write it to PLANE_CTL register
> instead of just relying on intel_plane_state->ctl value.
>
> Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 344c0e709b19..b491b1fbdea1 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -254,6 +254,7 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
>  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
>  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
>  	unsigned long irqflags;
> +	u32 val;
>  
>  	/* Sizes are 0 based */
>  	src_w--;
> @@ -322,6 +323,8 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
>  		I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x);
>  	}
>  
> +	val = I915_READ_FW(PLANE_CTL(pipe, plane_id));
> +	plane_ctl |= val;

You get the warning backtrace from our state checker, the purpose of
which is to ensure that our software state and hardware state
match. This change defeats the purpose of the state checker by relying
on the hardware state using a read-modify-write. The states will be out
of sync and we won't even know.

The real fix, I think at least before I grab another cup of desperately
needed coffee, is to fix the state readout during probe.

BR,
Jani.

>  	I915_WRITE_FW(PLANE_CTL(pipe, plane_id), plane_ctl);
>  	I915_WRITE_FW(PLANE_SURF(pipe, plane_id),
>  		      intel_plane_ggtt_offset(plane_state) + surf_addr);

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] drm/i915: Fix assert_plane() warning on bootup with external display
  2018-06-18  8:18 ` [RFC] " Jani Nikula
@ 2018-06-18 11:57   ` Ville Syrjälä
  2018-06-18 21:40     ` Shaikh, Azhar
  0 siblings, 1 reply; 8+ messages in thread
From: Ville Syrjälä @ 2018-06-18 11:57 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

On Mon, Jun 18, 2018 at 11:18:52AM +0300, Jani Nikula wrote:
> On Sun, 17 Jun 2018, Azhar Shaikh <azhar.shaikh@intel.com> wrote:
> > On KBL, WHL RVPs, booting up with an external display connected, triggers
> > below warning. This warning is not seen during hotplug.
> >
> > [    3.615226] ------------[ cut here ]------------
> > [    3.619829] plane 1A assertion failure (expected on, current off)
> > [    3.632039] WARNING: CPU: 2 PID: 354 at drivers/gpu/drm/i915/intel_display.c:1294 assert_plane+0x71/0xbb
> > [    3.633920] iwlwifi 0000:00:14.3: loaded firmware version 38.c0e03d94.0 op_mode iwlmvm
> > [    3.647157] Modules linked in: iwlwifi cfg80211 btusb btrtl btbcm btintel bluetooth ecdh_generic
> > [    3.647163] CPU: 2 PID: 354 Comm: frecon Not tainted 4.17.0-rc7-50176-g655af12d39c2 #3
> > [    3.647165] Hardware name: Intel Corporation CoffeeLake Client Platform/WhiskeyLake U DDR4 ERB, BIOS CNLSFWR1.R00.X140.B00.1804040304 04/04/2018
> > [    3.684509] RIP: 0010:assert_plane+0x71/0xbb
> > [    3.764451] Call Trace:
> > [    3.766888]  intel_atomic_commit_tail+0xa97/0xb77
> > [    3.771569]  intel_atomic_commit+0x26a/0x279
> > [    3.771572]  drm_atomic_helper_set_config+0x5c/0x76
> > [    3.780670]  __drm_mode_set_config_internal+0x66/0x109
> > [    3.780672]  drm_mode_setcrtc+0x4c9/0x5cc
> > [    3.780674]  ? drm_mode_getcrtc+0x162/0x162
> > [    3.789774]  ? drm_mode_getcrtc+0x162/0x162
> > [    3.798108]  drm_ioctl_kernel+0x8d/0xe4
> > [    3.801926]  drm_ioctl+0x27d/0x368
> > [    3.805311]  ? drm_mode_getcrtc+0x162/0x162
> > [    3.805314]  ? selinux_file_ioctl+0x14e/0x199
> > [    3.805317]  vfs_ioctl+0x21/0x2f
> > [    3.813812]  do_vfs_ioctl+0x491/0x4b4
> > [    3.813813]  ? security_file_ioctl+0x37/0x4b
> > [    3.813816]  ksys_ioctl+0x55/0x75
> > [    3.820672]  __x64_sys_ioctl+0x1a/0x1e
> > [    3.820674]  do_syscall_64+0x51/0x5f
> > [    3.820678]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > [    3.828221] RIP: 0033:0x7b5e04953967
> > [    3.835504] RSP: 002b:00007fff2eafb6f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> > [    3.835505] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007b5e04953967
> > [    3.835505] RDX: 00007fff2eafb730 RSI: 00000000c06864a2 RDI: 000000000000000f
> > [    3.835506] RBP: 00007fff2eafb720 R08: 0000000000000000 R09: 0000000000000000
> > [    3.835507] R10: 0000000000000070 R11: 0000000000000246 R12: 000000000000000f
> > [    3.879988] R13: 000056bc9dd7d210 R14: 00007fff2eafb730 R15: 00000000c06864a2
> > [    3.887081] Code: 48 c7 c7 06 71 a5 be 84 c0 48 c7 c2 06 fd a3 be 48 89 f9 48 0f 44 ca 84 db 48 0f 45 d7 48 c7 c7 df d3 a4 be 31 c0 e8 af a0 c0 ff <0f> 0b eb 2b 48 c7 c7 06 fd a3 be 84 c0 48 c7 c2 06 71 a5 be 48
> > [    3.905845] WARNING: CPU: 2 PID: 354 at drivers/gpu/drm/i915/intel_display.c:1294 assert_plane+0x71/0xbb
> > [    3.920964] ---[ end trace dac692f4ac46391a ]---
> >
> > The warning is seen when mode_setcrtc() is called for pipeB
> > during bootup and before we get a mode_setcrtc() for pipeA,
> > while doing update_crtcs() in intel_atomic_commit_tail().
> > Now since, plane1A is still active after commit, update_crtcs()
> > is done for pipeA and eventually update_plane() for plane1A.
> >
> > intel_plane_state->ctl for plane1A is not updated since set_modecrtc() is
> > called for pipeB. So intel_plane_state->ctl for plane 1A will be 0x0.
> > So doing an update_plane() for plane1A, will result in clearing
> > PLANE_CTL_ENABLE bit, and hence the warning.
> >
> > To fix this warning, prior to updating the PLANE_CTL register with
> > intel_plane_state->ctl value, read PLANE_CTL register, OR it with
> > intel_plane_state->ctl and then write it to PLANE_CTL register
> > instead of just relying on intel_plane_state->ctl value.
> >
> > Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 344c0e709b19..b491b1fbdea1 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -254,6 +254,7 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
> >  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> >  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
> >  	unsigned long irqflags;
> > +	u32 val;
> >  
> >  	/* Sizes are 0 based */
> >  	src_w--;
> > @@ -322,6 +323,8 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
> >  		I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x);
> >  	}
> >  
> > +	val = I915_READ_FW(PLANE_CTL(pipe, plane_id));
> > +	plane_ctl |= val;
> 
> You get the warning backtrace from our state checker, the purpose of
> which is to ensure that our software state and hardware state
> match. This change defeats the purpose of the state checker by relying
> on the hardware state using a read-modify-write. The states will be out
> of sync and we won't even know.
> 
> The real fix, I think at least before I grab another cup of desperately
> needed coffee, is to fix the state readout during probe.

Full plane state readout would require a bit of work. Would be nice to
have indeed, but probably quicker to just force all planes to recompute
their states. I'm thinking that should be happening already, but maybe
I'm mistaken.

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] drm/i915: Fix assert_plane() warning on bootup with external display
  2018-06-18 11:57   ` Ville Syrjälä
@ 2018-06-18 21:40     ` Shaikh, Azhar
  2018-06-19 11:59       ` Ville Syrjälä
  0 siblings, 1 reply; 8+ messages in thread
From: Shaikh, Azhar @ 2018-06-18 21:40 UTC (permalink / raw)
  To: Ville Syrjälä, Jani Nikula; +Cc: intel-gfx



>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Monday, June 18, 2018 4:57 AM
>To: Jani Nikula <jani.nikula@linux.intel.com>
>Cc: Shaikh, Azhar <azhar.shaikh@intel.com>; intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [RFC] drm/i915: Fix assert_plane() warning on bootup
>with external display
>
>On Mon, Jun 18, 2018 at 11:18:52AM +0300, Jani Nikula wrote:
>> On Sun, 17 Jun 2018, Azhar Shaikh <azhar.shaikh@intel.com> wrote:
>> > On KBL, WHL RVPs, booting up with an external display connected,
>> > triggers below warning. This warning is not seen during hotplug.
>> >
>> > [    3.615226] ------------[ cut here ]------------
>> > [    3.619829] plane 1A assertion failure (expected on, current off)
>> > [    3.632039] WARNING: CPU: 2 PID: 354 at
>drivers/gpu/drm/i915/intel_display.c:1294 assert_plane+0x71/0xbb
>> > [    3.633920] iwlwifi 0000:00:14.3: loaded firmware version 38.c0e03d94.0
>op_mode iwlmvm
>> > [    3.647157] Modules linked in: iwlwifi cfg80211 btusb btrtl btbcm btintel
>bluetooth ecdh_generic
>> > [    3.647163] CPU: 2 PID: 354 Comm: frecon Not tainted 4.17.0-rc7-50176-
>g655af12d39c2 #3
>> > [    3.647165] Hardware name: Intel Corporation CoffeeLake Client
>Platform/WhiskeyLake U DDR4 ERB, BIOS
>CNLSFWR1.R00.X140.B00.1804040304 04/04/2018
>> > [    3.684509] RIP: 0010:assert_plane+0x71/0xbb
>> > [    3.764451] Call Trace:
>> > [    3.766888]  intel_atomic_commit_tail+0xa97/0xb77
>> > [    3.771569]  intel_atomic_commit+0x26a/0x279
>> > [    3.771572]  drm_atomic_helper_set_config+0x5c/0x76
>> > [    3.780670]  __drm_mode_set_config_internal+0x66/0x109
>> > [    3.780672]  drm_mode_setcrtc+0x4c9/0x5cc
>> > [    3.780674]  ? drm_mode_getcrtc+0x162/0x162
>> > [    3.789774]  ? drm_mode_getcrtc+0x162/0x162
>> > [    3.798108]  drm_ioctl_kernel+0x8d/0xe4
>> > [    3.801926]  drm_ioctl+0x27d/0x368
>> > [    3.805311]  ? drm_mode_getcrtc+0x162/0x162
>> > [    3.805314]  ? selinux_file_ioctl+0x14e/0x199
>> > [    3.805317]  vfs_ioctl+0x21/0x2f
>> > [    3.813812]  do_vfs_ioctl+0x491/0x4b4
>> > [    3.813813]  ? security_file_ioctl+0x37/0x4b
>> > [    3.813816]  ksys_ioctl+0x55/0x75
>> > [    3.820672]  __x64_sys_ioctl+0x1a/0x1e
>> > [    3.820674]  do_syscall_64+0x51/0x5f
>> > [    3.820678]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> > [    3.828221] RIP: 0033:0x7b5e04953967
>> > [    3.835504] RSP: 002b:00007fff2eafb6f8 EFLAGS: 00000246 ORIG_RAX:
>0000000000000010
>> > [    3.835505] RAX: ffffffffffffffda RBX: 0000000000000002 RCX:
>00007b5e04953967
>> > [    3.835505] RDX: 00007fff2eafb730 RSI: 00000000c06864a2 RDI:
>000000000000000f
>> > [    3.835506] RBP: 00007fff2eafb720 R08: 0000000000000000 R09:
>0000000000000000
>> > [    3.835507] R10: 0000000000000070 R11: 0000000000000246 R12:
>000000000000000f
>> > [    3.879988] R13: 000056bc9dd7d210 R14: 00007fff2eafb730 R15:
>00000000c06864a2
>> > [    3.887081] Code: 48 c7 c7 06 71 a5 be 84 c0 48 c7 c2 06 fd a3 be 48 89 f9 48
>0f 44 ca 84 db 48 0f 45 d7 48 c7 c7 df d3 a4 be 31 c0 e8 af a0 c0 ff <0f> 0b eb 2b
>48 c7 c7 06 fd a3 be 84 c0 48 c7 c2 06 71 a5 be 48
>> > [    3.905845] WARNING: CPU: 2 PID: 354 at
>drivers/gpu/drm/i915/intel_display.c:1294 assert_plane+0x71/0xbb
>> > [    3.920964] ---[ end trace dac692f4ac46391a ]---
>> >
>> > The warning is seen when mode_setcrtc() is called for pipeB during
>> > bootup and before we get a mode_setcrtc() for pipeA, while doing
>> > update_crtcs() in intel_atomic_commit_tail().
>> > Now since, plane1A is still active after commit, update_crtcs() is
>> > done for pipeA and eventually update_plane() for plane1A.
>> >
>> > intel_plane_state->ctl for plane1A is not updated since
>> > set_modecrtc() is called for pipeB. So intel_plane_state->ctl for plane 1A
>will be 0x0.
>> > So doing an update_plane() for plane1A, will result in clearing
>> > PLANE_CTL_ENABLE bit, and hence the warning.
>> >
>> > To fix this warning, prior to updating the PLANE_CTL register with
>> > intel_plane_state->ctl value, read PLANE_CTL register, OR it with
>> > intel_plane_state->ctl and then write it to PLANE_CTL register
>> > instead of just relying on intel_plane_state->ctl value.
>> >
>> > Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_sprite.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
>> > b/drivers/gpu/drm/i915/intel_sprite.c
>> > index 344c0e709b19..b491b1fbdea1 100644
>> > --- a/drivers/gpu/drm/i915/intel_sprite.c
>> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> > @@ -254,6 +254,7 @@ void intel_pipe_update_end(struct
>intel_crtc_state *new_crtc_state)
>> >  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
>> >  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
>> >  	unsigned long irqflags;
>> > +	u32 val;
>> >
>> >  	/* Sizes are 0 based */
>> >  	src_w--;
>> > @@ -322,6 +323,8 @@ void intel_pipe_update_end(struct
>intel_crtc_state *new_crtc_state)
>> >  		I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) |
>crtc_x);
>> >  	}
>> >
>> > +	val = I915_READ_FW(PLANE_CTL(pipe, plane_id));
>> > +	plane_ctl |= val;
>>
>> You get the warning backtrace from our state checker, the purpose of
>> which is to ensure that our software state and hardware state match.
>> This change defeats the purpose of the state checker by relying on the
>> hardware state using a read-modify-write. The states will be out of
>> sync and we won't even know.
>>
>> The real fix, I think at least before I grab another cup of
>> desperately needed coffee, is to fix the state readout during probe.
>
>Full plane state readout would require a bit of work. Would be nice to have
>indeed, but probably quicker to just force all planes to recompute their states.
>I'm thinking that should be happening already, but maybe I'm mistaken.
>

> force all planes to recompute their states

All planes on all pipes need to recompute or only the pipe which is in modeset?
Also recompute the sw state, is that right?

In this case pipeB is the only one which is doing a modeset and after commit, pipeB is not active. Its only pipeA is active when the warning is seen.



>--
>Ville Syrjälä
>Intel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] drm/i915: Fix assert_plane() warning on bootup with external display
  2018-06-18 21:40     ` Shaikh, Azhar
@ 2018-06-19 11:59       ` Ville Syrjälä
  2018-06-21  1:00         ` Shaikh, Azhar
  0 siblings, 1 reply; 8+ messages in thread
From: Ville Syrjälä @ 2018-06-19 11:59 UTC (permalink / raw)
  To: Shaikh, Azhar; +Cc: intel-gfx

On Mon, Jun 18, 2018 at 09:40:38PM +0000, Shaikh, Azhar wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Monday, June 18, 2018 4:57 AM
> >To: Jani Nikula <jani.nikula@linux.intel.com>
> >Cc: Shaikh, Azhar <azhar.shaikh@intel.com>; intel-gfx@lists.freedesktop.org
> >Subject: Re: [Intel-gfx] [RFC] drm/i915: Fix assert_plane() warning on bootup
> >with external display
> >
> >On Mon, Jun 18, 2018 at 11:18:52AM +0300, Jani Nikula wrote:
> >> On Sun, 17 Jun 2018, Azhar Shaikh <azhar.shaikh@intel.com> wrote:
> >> > On KBL, WHL RVPs, booting up with an external display connected,
> >> > triggers below warning. This warning is not seen during hotplug.
> >> >
> >> > [    3.615226] ------------[ cut here ]------------
> >> > [    3.619829] plane 1A assertion failure (expected on, current off)
> >> > [    3.632039] WARNING: CPU: 2 PID: 354 at
> >drivers/gpu/drm/i915/intel_display.c:1294 assert_plane+0x71/0xbb
> >> > [    3.633920] iwlwifi 0000:00:14.3: loaded firmware version 38.c0e03d94.0
> >op_mode iwlmvm
> >> > [    3.647157] Modules linked in: iwlwifi cfg80211 btusb btrtl btbcm btintel
> >bluetooth ecdh_generic
> >> > [    3.647163] CPU: 2 PID: 354 Comm: frecon Not tainted 4.17.0-rc7-50176-
> >g655af12d39c2 #3
> >> > [    3.647165] Hardware name: Intel Corporation CoffeeLake Client
> >Platform/WhiskeyLake U DDR4 ERB, BIOS
> >CNLSFWR1.R00.X140.B00.1804040304 04/04/2018
> >> > [    3.684509] RIP: 0010:assert_plane+0x71/0xbb
> >> > [    3.764451] Call Trace:
> >> > [    3.766888]  intel_atomic_commit_tail+0xa97/0xb77
> >> > [    3.771569]  intel_atomic_commit+0x26a/0x279
> >> > [    3.771572]  drm_atomic_helper_set_config+0x5c/0x76
> >> > [    3.780670]  __drm_mode_set_config_internal+0x66/0x109
> >> > [    3.780672]  drm_mode_setcrtc+0x4c9/0x5cc
> >> > [    3.780674]  ? drm_mode_getcrtc+0x162/0x162
> >> > [    3.789774]  ? drm_mode_getcrtc+0x162/0x162
> >> > [    3.798108]  drm_ioctl_kernel+0x8d/0xe4
> >> > [    3.801926]  drm_ioctl+0x27d/0x368
> >> > [    3.805311]  ? drm_mode_getcrtc+0x162/0x162
> >> > [    3.805314]  ? selinux_file_ioctl+0x14e/0x199
> >> > [    3.805317]  vfs_ioctl+0x21/0x2f
> >> > [    3.813812]  do_vfs_ioctl+0x491/0x4b4
> >> > [    3.813813]  ? security_file_ioctl+0x37/0x4b
> >> > [    3.813816]  ksys_ioctl+0x55/0x75
> >> > [    3.820672]  __x64_sys_ioctl+0x1a/0x1e
> >> > [    3.820674]  do_syscall_64+0x51/0x5f
> >> > [    3.820678]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >> > [    3.828221] RIP: 0033:0x7b5e04953967
> >> > [    3.835504] RSP: 002b:00007fff2eafb6f8 EFLAGS: 00000246 ORIG_RAX:
> >0000000000000010
> >> > [    3.835505] RAX: ffffffffffffffda RBX: 0000000000000002 RCX:
> >00007b5e04953967
> >> > [    3.835505] RDX: 00007fff2eafb730 RSI: 00000000c06864a2 RDI:
> >000000000000000f
> >> > [    3.835506] RBP: 00007fff2eafb720 R08: 0000000000000000 R09:
> >0000000000000000
> >> > [    3.835507] R10: 0000000000000070 R11: 0000000000000246 R12:
> >000000000000000f
> >> > [    3.879988] R13: 000056bc9dd7d210 R14: 00007fff2eafb730 R15:
> >00000000c06864a2
> >> > [    3.887081] Code: 48 c7 c7 06 71 a5 be 84 c0 48 c7 c2 06 fd a3 be 48 89 f9 48
> >0f 44 ca 84 db 48 0f 45 d7 48 c7 c7 df d3 a4 be 31 c0 e8 af a0 c0 ff <0f> 0b eb 2b
> >48 c7 c7 06 fd a3 be 84 c0 48 c7 c2 06 71 a5 be 48
> >> > [    3.905845] WARNING: CPU: 2 PID: 354 at
> >drivers/gpu/drm/i915/intel_display.c:1294 assert_plane+0x71/0xbb
> >> > [    3.920964] ---[ end trace dac692f4ac46391a ]---
> >> >
> >> > The warning is seen when mode_setcrtc() is called for pipeB during
> >> > bootup and before we get a mode_setcrtc() for pipeA, while doing
> >> > update_crtcs() in intel_atomic_commit_tail().
> >> > Now since, plane1A is still active after commit, update_crtcs() is
> >> > done for pipeA and eventually update_plane() for plane1A.
> >> >
> >> > intel_plane_state->ctl for plane1A is not updated since
> >> > set_modecrtc() is called for pipeB. So intel_plane_state->ctl for plane 1A
> >will be 0x0.
> >> > So doing an update_plane() for plane1A, will result in clearing
> >> > PLANE_CTL_ENABLE bit, and hence the warning.
> >> >
> >> > To fix this warning, prior to updating the PLANE_CTL register with
> >> > intel_plane_state->ctl value, read PLANE_CTL register, OR it with
> >> > intel_plane_state->ctl and then write it to PLANE_CTL register
> >> > instead of just relying on intel_plane_state->ctl value.
> >> >
> >> > Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_sprite.c | 3 +++
> >> >  1 file changed, 3 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> >> > b/drivers/gpu/drm/i915/intel_sprite.c
> >> > index 344c0e709b19..b491b1fbdea1 100644
> >> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> >> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >> > @@ -254,6 +254,7 @@ void intel_pipe_update_end(struct
> >intel_crtc_state *new_crtc_state)
> >> >  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> >> >  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
> >> >  	unsigned long irqflags;
> >> > +	u32 val;
> >> >
> >> >  	/* Sizes are 0 based */
> >> >  	src_w--;
> >> > @@ -322,6 +323,8 @@ void intel_pipe_update_end(struct
> >intel_crtc_state *new_crtc_state)
> >> >  		I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) |
> >crtc_x);
> >> >  	}
> >> >
> >> > +	val = I915_READ_FW(PLANE_CTL(pipe, plane_id));
> >> > +	plane_ctl |= val;
> >>
> >> You get the warning backtrace from our state checker, the purpose of
> >> which is to ensure that our software state and hardware state match.
> >> This change defeats the purpose of the state checker by relying on the
> >> hardware state using a read-modify-write. The states will be out of
> >> sync and we won't even know.
> >>
> >> The real fix, I think at least before I grab another cup of
> >> desperately needed coffee, is to fix the state readout during probe.
> >
> >Full plane state readout would require a bit of work. Would be nice to have
> >indeed, but probably quicker to just force all planes to recompute their states.
> >I'm thinking that should be happening already, but maybe I'm mistaken.
> >
> 
> > force all planes to recompute their states
> 
> All planes on all pipes need to recompute or only the pipe which is in modeset?

Any active pipe taking part in the commit that hasn't had its state
fully recomputed yet.

Hmm. I915_MODE_FLAG_INHERITED should force this on the first commit.
But apparently that's not happening in your case. We need to figure
out why that is.

> Also recompute the sw state, is that right?

What else is there?

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] drm/i915: Fix assert_plane() warning on bootup with external display
  2018-06-19 11:59       ` Ville Syrjälä
@ 2018-06-21  1:00         ` Shaikh, Azhar
  2018-06-21 15:46           ` Ville Syrjälä
  0 siblings, 1 reply; 8+ messages in thread
From: Shaikh, Azhar @ 2018-06-21  1:00 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx



>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Tuesday, June 19, 2018 5:00 AM
>To: Shaikh, Azhar <azhar.shaikh@intel.com>
>Cc: Jani Nikula <jani.nikula@linux.intel.com>; intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [RFC] drm/i915: Fix assert_plane() warning on bootup
>with external display
>
>On Mon, Jun 18, 2018 at 09:40:38PM +0000, Shaikh, Azhar wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >Sent: Monday, June 18, 2018 4:57 AM
>> >To: Jani Nikula <jani.nikula@linux.intel.com>
>> >Cc: Shaikh, Azhar <azhar.shaikh@intel.com>;
>> >intel-gfx@lists.freedesktop.org
>> >Subject: Re: [Intel-gfx] [RFC] drm/i915: Fix assert_plane() warning
>> >on bootup with external display
>> >
>> >On Mon, Jun 18, 2018 at 11:18:52AM +0300, Jani Nikula wrote:
>> >> On Sun, 17 Jun 2018, Azhar Shaikh <azhar.shaikh@intel.com> wrote:
>> >> > On KBL, WHL RVPs, booting up with an external display connected,
>> >> > triggers below warning. This warning is not seen during hotplug.
>> >> >
>> >> > [    3.615226] ------------[ cut here ]------------
>> >> > [    3.619829] plane 1A assertion failure (expected on, current off)
>> >> > [    3.632039] WARNING: CPU: 2 PID: 354 at
>> >drivers/gpu/drm/i915/intel_display.c:1294 assert_plane+0x71/0xbb
>> >> > [    3.633920] iwlwifi 0000:00:14.3: loaded firmware version
>38.c0e03d94.0
>> >op_mode iwlmvm
>> >> > [    3.647157] Modules linked in: iwlwifi cfg80211 btusb btrtl btbcm
>btintel
>> >bluetooth ecdh_generic
>> >> > [    3.647163] CPU: 2 PID: 354 Comm: frecon Not tainted 4.17.0-rc7-
>50176-
>> >g655af12d39c2 #3
>> >> > [    3.647165] Hardware name: Intel Corporation CoffeeLake Client
>> >Platform/WhiskeyLake U DDR4 ERB, BIOS
>> >CNLSFWR1.R00.X140.B00.1804040304 04/04/2018
>> >> > [    3.684509] RIP: 0010:assert_plane+0x71/0xbb
>> >> > [    3.764451] Call Trace:
>> >> > [    3.766888]  intel_atomic_commit_tail+0xa97/0xb77
>> >> > [    3.771569]  intel_atomic_commit+0x26a/0x279
>> >> > [    3.771572]  drm_atomic_helper_set_config+0x5c/0x76
>> >> > [    3.780670]  __drm_mode_set_config_internal+0x66/0x109
>> >> > [    3.780672]  drm_mode_setcrtc+0x4c9/0x5cc
>> >> > [    3.780674]  ? drm_mode_getcrtc+0x162/0x162
>> >> > [    3.789774]  ? drm_mode_getcrtc+0x162/0x162
>> >> > [    3.798108]  drm_ioctl_kernel+0x8d/0xe4
>> >> > [    3.801926]  drm_ioctl+0x27d/0x368
>> >> > [    3.805311]  ? drm_mode_getcrtc+0x162/0x162
>> >> > [    3.805314]  ? selinux_file_ioctl+0x14e/0x199
>> >> > [    3.805317]  vfs_ioctl+0x21/0x2f
>> >> > [    3.813812]  do_vfs_ioctl+0x491/0x4b4
>> >> > [    3.813813]  ? security_file_ioctl+0x37/0x4b
>> >> > [    3.813816]  ksys_ioctl+0x55/0x75
>> >> > [    3.820672]  __x64_sys_ioctl+0x1a/0x1e
>> >> > [    3.820674]  do_syscall_64+0x51/0x5f
>> >> > [    3.820678]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> >> > [    3.828221] RIP: 0033:0x7b5e04953967
>> >> > [    3.835504] RSP: 002b:00007fff2eafb6f8 EFLAGS: 00000246 ORIG_RAX:
>> >0000000000000010
>> >> > [    3.835505] RAX: ffffffffffffffda RBX: 0000000000000002 RCX:
>> >00007b5e04953967
>> >> > [    3.835505] RDX: 00007fff2eafb730 RSI: 00000000c06864a2 RDI:
>> >000000000000000f
>> >> > [    3.835506] RBP: 00007fff2eafb720 R08: 0000000000000000 R09:
>> >0000000000000000
>> >> > [    3.835507] R10: 0000000000000070 R11: 0000000000000246 R12:
>> >000000000000000f
>> >> > [    3.879988] R13: 000056bc9dd7d210 R14: 00007fff2eafb730 R15:
>> >00000000c06864a2
>> >> > [    3.887081] Code: 48 c7 c7 06 71 a5 be 84 c0 48 c7 c2 06 fd a3 be 48 89
>f9 48
>> >0f 44 ca 84 db 48 0f 45 d7 48 c7 c7 df d3 a4 be 31 c0 e8 af a0 c0 ff
>> ><0f> 0b eb 2b
>> >48 c7 c7 06 fd a3 be 84 c0 48 c7 c2 06 71 a5 be 48
>> >> > [    3.905845] WARNING: CPU: 2 PID: 354 at
>> >drivers/gpu/drm/i915/intel_display.c:1294 assert_plane+0x71/0xbb
>> >> > [    3.920964] ---[ end trace dac692f4ac46391a ]---
>> >> >
>> >> > The warning is seen when mode_setcrtc() is called for pipeB
>> >> > during bootup and before we get a mode_setcrtc() for pipeA, while
>> >> > doing
>> >> > update_crtcs() in intel_atomic_commit_tail().
>> >> > Now since, plane1A is still active after commit, update_crtcs()
>> >> > is done for pipeA and eventually update_plane() for plane1A.
>> >> >
>> >> > intel_plane_state->ctl for plane1A is not updated since
>> >> > set_modecrtc() is called for pipeB. So intel_plane_state->ctl for
>> >> > plane 1A
>> >will be 0x0.
>> >> > So doing an update_plane() for plane1A, will result in clearing
>> >> > PLANE_CTL_ENABLE bit, and hence the warning.
>> >> >
>> >> > To fix this warning, prior to updating the PLANE_CTL register
>> >> > with intel_plane_state->ctl value, read PLANE_CTL register, OR it
>> >> > with intel_plane_state->ctl and then write it to PLANE_CTL
>> >> > register instead of just relying on intel_plane_state->ctl value.
>> >> >
>> >> > Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
>> >> > ---
>> >> >  drivers/gpu/drm/i915/intel_sprite.c | 3 +++
>> >> >  1 file changed, 3 insertions(+)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
>> >> > b/drivers/gpu/drm/i915/intel_sprite.c
>> >> > index 344c0e709b19..b491b1fbdea1 100644
>> >> > --- a/drivers/gpu/drm/i915/intel_sprite.c
>> >> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> >> > @@ -254,6 +254,7 @@ void intel_pipe_update_end(struct
>> >intel_crtc_state *new_crtc_state)
>> >> >  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
>> >> >  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
>> >> >  	unsigned long irqflags;
>> >> > +	u32 val;
>> >> >
>> >> >  	/* Sizes are 0 based */
>> >> >  	src_w--;
>> >> > @@ -322,6 +323,8 @@ void intel_pipe_update_end(struct
>> >intel_crtc_state *new_crtc_state)
>> >> >  		I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) |
>> >crtc_x);
>> >> >  	}
>> >> >
>> >> > +	val = I915_READ_FW(PLANE_CTL(pipe, plane_id));
>> >> > +	plane_ctl |= val;
>> >>
>> >> You get the warning backtrace from our state checker, the purpose
>> >> of which is to ensure that our software state and hardware state match.
>> >> This change defeats the purpose of the state checker by relying on
>> >> the hardware state using a read-modify-write. The states will be
>> >> out of sync and we won't even know.
>> >>
>> >> The real fix, I think at least before I grab another cup of
>> >> desperately needed coffee, is to fix the state readout during probe.
>> >
>> >Full plane state readout would require a bit of work. Would be nice
>> >to have indeed, but probably quicker to just force all planes to recompute
>their states.
>> >I'm thinking that should be happening already, but maybe I'm mistaken.
>> >
>>
>> > force all planes to recompute their states
>>
>> All planes on all pipes need to recompute or only the pipe which is in
>modeset?
>
>Any active pipe taking part in the commit that hasn't had its state fully
>recomputed yet.
>

For now I tried readout of PLANE_CTL and PLANE_COLOR_CTL, updating the new_intel_plane_state with these values in intel_pre_plane_update_plane() before calling skl_update_plane(). With this change eDP is corrupted and external display fails to come up.

If I call skl_plane_ctl() before calling skl_update_plane(), I do not see any corruption, no warning and everything "seems" to works fine. So do I need to call skl_plane_ctl() + other values which need to be recomputed from readout HW state and then update the intel_plane_state()?

Also what all values do we need to recompute from HW readout, other than PLANE_CTL and PLANE_COLOR_CTL?

>Hmm. I915_MODE_FLAG_INHERITED should force this on the first commit.
>But apparently that's not happening in your case. We need to figure out why
>that is.
>
>> Also recompute the sw state, is that right?
>
>What else is there?
>
>--
>Ville Syrjälä
>Intel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] drm/i915: Fix assert_plane() warning on bootup with external display
  2018-06-21  1:00         ` Shaikh, Azhar
@ 2018-06-21 15:46           ` Ville Syrjälä
  0 siblings, 0 replies; 8+ messages in thread
From: Ville Syrjälä @ 2018-06-21 15:46 UTC (permalink / raw)
  To: Shaikh, Azhar; +Cc: intel-gfx

On Thu, Jun 21, 2018 at 01:00:48AM +0000, Shaikh, Azhar wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Tuesday, June 19, 2018 5:00 AM
> >To: Shaikh, Azhar <azhar.shaikh@intel.com>
> >Cc: Jani Nikula <jani.nikula@linux.intel.com>; intel-gfx@lists.freedesktop.org
> >Subject: Re: [Intel-gfx] [RFC] drm/i915: Fix assert_plane() warning on bootup
> >with external display
> >
> >On Mon, Jun 18, 2018 at 09:40:38PM +0000, Shaikh, Azhar wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >> >Sent: Monday, June 18, 2018 4:57 AM
> >> >To: Jani Nikula <jani.nikula@linux.intel.com>
> >> >Cc: Shaikh, Azhar <azhar.shaikh@intel.com>;
> >> >intel-gfx@lists.freedesktop.org
> >> >Subject: Re: [Intel-gfx] [RFC] drm/i915: Fix assert_plane() warning
> >> >on bootup with external display
> >> >
> >> >On Mon, Jun 18, 2018 at 11:18:52AM +0300, Jani Nikula wrote:
> >> >> On Sun, 17 Jun 2018, Azhar Shaikh <azhar.shaikh@intel.com> wrote:
> >> >> > On KBL, WHL RVPs, booting up with an external display connected,
> >> >> > triggers below warning. This warning is not seen during hotplug.
> >> >> >
> >> >> > [    3.615226] ------------[ cut here ]------------
> >> >> > [    3.619829] plane 1A assertion failure (expected on, current off)
> >> >> > [    3.632039] WARNING: CPU: 2 PID: 354 at
> >> >drivers/gpu/drm/i915/intel_display.c:1294 assert_plane+0x71/0xbb
> >> >> > [    3.633920] iwlwifi 0000:00:14.3: loaded firmware version
> >38.c0e03d94.0
> >> >op_mode iwlmvm
> >> >> > [    3.647157] Modules linked in: iwlwifi cfg80211 btusb btrtl btbcm
> >btintel
> >> >bluetooth ecdh_generic
> >> >> > [    3.647163] CPU: 2 PID: 354 Comm: frecon Not tainted 4.17.0-rc7-
> >50176-
> >> >g655af12d39c2 #3
> >> >> > [    3.647165] Hardware name: Intel Corporation CoffeeLake Client
> >> >Platform/WhiskeyLake U DDR4 ERB, BIOS
> >> >CNLSFWR1.R00.X140.B00.1804040304 04/04/2018
> >> >> > [    3.684509] RIP: 0010:assert_plane+0x71/0xbb
> >> >> > [    3.764451] Call Trace:
> >> >> > [    3.766888]  intel_atomic_commit_tail+0xa97/0xb77
> >> >> > [    3.771569]  intel_atomic_commit+0x26a/0x279
> >> >> > [    3.771572]  drm_atomic_helper_set_config+0x5c/0x76
> >> >> > [    3.780670]  __drm_mode_set_config_internal+0x66/0x109
> >> >> > [    3.780672]  drm_mode_setcrtc+0x4c9/0x5cc
> >> >> > [    3.780674]  ? drm_mode_getcrtc+0x162/0x162
> >> >> > [    3.789774]  ? drm_mode_getcrtc+0x162/0x162
> >> >> > [    3.798108]  drm_ioctl_kernel+0x8d/0xe4
> >> >> > [    3.801926]  drm_ioctl+0x27d/0x368
> >> >> > [    3.805311]  ? drm_mode_getcrtc+0x162/0x162
> >> >> > [    3.805314]  ? selinux_file_ioctl+0x14e/0x199
> >> >> > [    3.805317]  vfs_ioctl+0x21/0x2f
> >> >> > [    3.813812]  do_vfs_ioctl+0x491/0x4b4
> >> >> > [    3.813813]  ? security_file_ioctl+0x37/0x4b
> >> >> > [    3.813816]  ksys_ioctl+0x55/0x75
> >> >> > [    3.820672]  __x64_sys_ioctl+0x1a/0x1e
> >> >> > [    3.820674]  do_syscall_64+0x51/0x5f
> >> >> > [    3.820678]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >> >> > [    3.828221] RIP: 0033:0x7b5e04953967
> >> >> > [    3.835504] RSP: 002b:00007fff2eafb6f8 EFLAGS: 00000246 ORIG_RAX:
> >> >0000000000000010
> >> >> > [    3.835505] RAX: ffffffffffffffda RBX: 0000000000000002 RCX:
> >> >00007b5e04953967
> >> >> > [    3.835505] RDX: 00007fff2eafb730 RSI: 00000000c06864a2 RDI:
> >> >000000000000000f
> >> >> > [    3.835506] RBP: 00007fff2eafb720 R08: 0000000000000000 R09:
> >> >0000000000000000
> >> >> > [    3.835507] R10: 0000000000000070 R11: 0000000000000246 R12:
> >> >000000000000000f
> >> >> > [    3.879988] R13: 000056bc9dd7d210 R14: 00007fff2eafb730 R15:
> >> >00000000c06864a2
> >> >> > [    3.887081] Code: 48 c7 c7 06 71 a5 be 84 c0 48 c7 c2 06 fd a3 be 48 89
> >f9 48
> >> >0f 44 ca 84 db 48 0f 45 d7 48 c7 c7 df d3 a4 be 31 c0 e8 af a0 c0 ff
> >> ><0f> 0b eb 2b
> >> >48 c7 c7 06 fd a3 be 84 c0 48 c7 c2 06 71 a5 be 48
> >> >> > [    3.905845] WARNING: CPU: 2 PID: 354 at
> >> >drivers/gpu/drm/i915/intel_display.c:1294 assert_plane+0x71/0xbb
> >> >> > [    3.920964] ---[ end trace dac692f4ac46391a ]---
> >> >> >
> >> >> > The warning is seen when mode_setcrtc() is called for pipeB
> >> >> > during bootup and before we get a mode_setcrtc() for pipeA, while
> >> >> > doing
> >> >> > update_crtcs() in intel_atomic_commit_tail().
> >> >> > Now since, plane1A is still active after commit, update_crtcs()
> >> >> > is done for pipeA and eventually update_plane() for plane1A.
> >> >> >
> >> >> > intel_plane_state->ctl for plane1A is not updated since
> >> >> > set_modecrtc() is called for pipeB. So intel_plane_state->ctl for
> >> >> > plane 1A
> >> >will be 0x0.
> >> >> > So doing an update_plane() for plane1A, will result in clearing
> >> >> > PLANE_CTL_ENABLE bit, and hence the warning.
> >> >> >
> >> >> > To fix this warning, prior to updating the PLANE_CTL register
> >> >> > with intel_plane_state->ctl value, read PLANE_CTL register, OR it
> >> >> > with intel_plane_state->ctl and then write it to PLANE_CTL
> >> >> > register instead of just relying on intel_plane_state->ctl value.
> >> >> >
> >> >> > Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
> >> >> > ---
> >> >> >  drivers/gpu/drm/i915/intel_sprite.c | 3 +++
> >> >> >  1 file changed, 3 insertions(+)
> >> >> >
> >> >> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> >> >> > b/drivers/gpu/drm/i915/intel_sprite.c
> >> >> > index 344c0e709b19..b491b1fbdea1 100644
> >> >> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> >> >> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >> >> > @@ -254,6 +254,7 @@ void intel_pipe_update_end(struct
> >> >intel_crtc_state *new_crtc_state)
> >> >> >  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> >> >> >  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
> >> >> >  	unsigned long irqflags;
> >> >> > +	u32 val;
> >> >> >
> >> >> >  	/* Sizes are 0 based */
> >> >> >  	src_w--;
> >> >> > @@ -322,6 +323,8 @@ void intel_pipe_update_end(struct
> >> >intel_crtc_state *new_crtc_state)
> >> >> >  		I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) |
> >> >crtc_x);
> >> >> >  	}
> >> >> >
> >> >> > +	val = I915_READ_FW(PLANE_CTL(pipe, plane_id));
> >> >> > +	plane_ctl |= val;
> >> >>
> >> >> You get the warning backtrace from our state checker, the purpose
> >> >> of which is to ensure that our software state and hardware state match.
> >> >> This change defeats the purpose of the state checker by relying on
> >> >> the hardware state using a read-modify-write. The states will be
> >> >> out of sync and we won't even know.
> >> >>
> >> >> The real fix, I think at least before I grab another cup of
> >> >> desperately needed coffee, is to fix the state readout during probe.
> >> >
> >> >Full plane state readout would require a bit of work. Would be nice
> >> >to have indeed, but probably quicker to just force all planes to recompute
> >their states.
> >> >I'm thinking that should be happening already, but maybe I'm mistaken.
> >> >
> >>
> >> > force all planes to recompute their states
> >>
> >> All planes on all pipes need to recompute or only the pipe which is in
> >modeset?
> >
> >Any active pipe taking part in the commit that hasn't had its state fully
> >recomputed yet.
> >
> 
> For now I tried readout of PLANE_CTL and PLANE_COLOR_CTL, updating the new_intel_plane_state with these values in intel_pre_plane_update_plane() before calling skl_update_plane(). With this change eDP is corrupted and external display fails to come up.

The readout should go to the initial_plane_config() thing, which is
immplemented in a sort of ass backwards way. Ideally we'd want to do a
full plane state readout first, then reconstruct the fb based on that
state. Currently we do it basically the other way around.

One thing I was also thinking is that eventually we might want to create
some kind of placeholder fb for every enabled plane, even if aren't going
to take over that fb later. That way we could use the normal atomic paths
to disable all planes we don't want enabled.

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-06-21 15:46 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-18  5:18 [RFC] drm/i915: Fix assert_plane() warning on bootup with external display Azhar Shaikh
2018-06-18  5:46 ` ✗ Fi.CI.BAT: failure for " Patchwork
2018-06-18  8:18 ` [RFC] " Jani Nikula
2018-06-18 11:57   ` Ville Syrjälä
2018-06-18 21:40     ` Shaikh, Azhar
2018-06-19 11:59       ` Ville Syrjälä
2018-06-21  1:00         ` Shaikh, Azhar
2018-06-21 15:46           ` Ville Syrjälä

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.