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

Mon Jun 18 21:40:38 UTC 2018

>-----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?
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