[Intel-gfx] [RFC] drm/i915: Fix assert_plane() warning on bootup with external display
Shaikh, Azhar
azhar.shaikh at intel.com
Thu Jun 21 01:00:48 UTC 2018
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>Sent: Tuesday, June 19, 2018 5:00 AM
>To: Shaikh, Azhar <azhar.shaikh at intel.com>
>Cc: Jani Nikula <jani.nikula at linux.intel.com>; intel-gfx at 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 at linux.intel.com]
>> >Sent: Monday, June 18, 2018 4:57 AM
>> >To: Jani Nikula <jani.nikula at linux.intel.com>
>> >Cc: Shaikh, Azhar <azhar.shaikh at intel.com>;
>> >intel-gfx at 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 at 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 at 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
More information about the Intel-gfx
mailing list