[Intel-gfx] [RFC] drm/i915: Fix assert_plane() warning on bootup with external display

Thu Jun 21 15:46:35 UTC 2018

On Thu, Jun 21, 2018 at 01:00:48AM +0000, Shaikh, Azhar wrote:
> 
> 
> >-----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.

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