| Message ID | 1529973185-18846-1-git-send-email-azhar.shaikh@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 25, 2018 at 05:33:05PM -0700, Azhar Shaikh wrote: > On KBL, WHL RVPs, booting up with an external display connected, triggers > below warning, when the BiOS brings up the external display too. > 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, force all active planes to recompute their states > in probe. > > Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com> > --- > Changes in v2: > - Force all planes to recompute their states.(Ville Syrjälä) > - Update the commit message > > drivers/gpu/drm/i915/intel_display.c | 49 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3709fa1b6318..1511d58991cc 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15092,6 +15092,51 @@ static void intel_update_fdi_pll_freq(struct drm_i915_private *dev_priv) > DRM_DEBUG_DRIVER("FDI PLL freq=%d\n", dev_priv->fdi_pll_freq); > } > > +static void intel_initial_commit(struct drm_device *dev, > + struct intel_crtc *intel_crtc) > +{ > + struct drm_atomic_state *state = NULL; > + struct drm_crtc *crtc = &intel_crtc->base; > + struct drm_modeset_acquire_ctx ctx; > + struct drm_crtc_state *crtc_state; > + int ret; > + > + drm_modeset_acquire_init(&ctx, 0); > + > + state = drm_atomic_state_alloc(dev); > + if (!state) > + goto unlock; > + > + state->acquire_ctx = &ctx; > + I would put the for_each_crtc() loop here so that a single atomic commit will take care of all the active pipes. > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > + if (IS_ERR(crtc_state)) > + goto out; > + > + if (!crtc_state->active) > + goto out; > + > + ret = drm_atomic_add_affected_planes(state, crtc); > + if (ret) > + goto out; > + > + ret = drm_atomic_commit(state); > + if (ret == -EDEADLK) { > + drm_atomic_state_clear(state); > + drm_modeset_backoff(&ctx); > + } You're missing the EDEADLK retry loop entirely. > + > +out: > + if (state) { > + drm_atomic_state_put(state); > + state = NULL; > + } > + > +unlock: > + drm_modeset_drop_locks(&ctx); > + drm_modeset_acquire_fini(&ctx); We probably want to print an error of some sort if this fails. > +} > + > int intel_modeset_init(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = to_i915(dev); > @@ -15221,6 +15266,10 @@ int intel_modeset_init(struct drm_device *dev) > intel_find_initial_plane_obj(crtc, &plane_config); > } > > + /* Force all active planes to recompute their states */ > + for_each_intel_crtc(dev, crtc) > + intel_initial_commit(dev, crtc); > + > /* > * Make sure hardware watermarks really match the state we read out. > * Note that we need to do this after reconstructing the BIOS fb's > -- > 1.9.1
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Tuesday, June 26, 2018 6:01 AM >To: Shaikh, Azhar <azhar.shaikh@intel.com> >Cc: intel-gfx@lists.freedesktop.org; Navare, Manasi D ><manasi.d.navare@intel.com> >Subject: Re: [PATCH v2] drm/i915: Fix assert_plane() warning on bootup with >external display > >On Mon, Jun 25, 2018 at 05:33:05PM -0700, Azhar Shaikh wrote: >> On KBL, WHL RVPs, booting up with an external display connected, >> triggers below warning, when the BiOS brings up the external display too. >> 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, force all active planes to recompute their states >> in probe. >> >> Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com> >> --- >> Changes in v2: >> - Force all planes to recompute their states.(Ville Syrjälä) >> - Update the commit message >> >> drivers/gpu/drm/i915/intel_display.c | 49 >> ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 49 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index 3709fa1b6318..1511d58991cc 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -15092,6 +15092,51 @@ static void intel_update_fdi_pll_freq(struct >drm_i915_private *dev_priv) >> DRM_DEBUG_DRIVER("FDI PLL freq=%d\n", dev_priv->fdi_pll_freq); >} >> >> +static void intel_initial_commit(struct drm_device *dev, >> + struct intel_crtc *intel_crtc) >> +{ >> + struct drm_atomic_state *state = NULL; >> + struct drm_crtc *crtc = &intel_crtc->base; >> + struct drm_modeset_acquire_ctx ctx; >> + struct drm_crtc_state *crtc_state; >> + int ret; >> + >> + drm_modeset_acquire_init(&ctx, 0); >> + >> + state = drm_atomic_state_alloc(dev); >> + if (!state) >> + goto unlock; >> + >> + state->acquire_ctx = &ctx; >> + > >I would put the for_each_crtc() loop here so that a single atomic commit will >take care of all the active pipes. > Good point! Will do that. >> + crtc_state = drm_atomic_get_crtc_state(state, crtc); >> + if (IS_ERR(crtc_state)) >> + goto out; >> + >> + if (!crtc_state->active) >> + goto out; >> + >> + ret = drm_atomic_add_affected_planes(state, crtc); >> + if (ret) >> + goto out; >> + >> + ret = drm_atomic_commit(state); >> + if (ret == -EDEADLK) { >> + drm_atomic_state_clear(state); >> + drm_modeset_backoff(&ctx); >> + } > >You're missing the EDEADLK retry loop entirely. > Yeah :( will add it. >> + >> +out: >> + if (state) { >> + drm_atomic_state_put(state); >> + state = NULL; >> + } >> + >> +unlock: >> + drm_modeset_drop_locks(&ctx); >> + drm_modeset_acquire_fini(&ctx); > >We probably want to print an error of some sort if this fails. > Sure, will add an error statement. >> +} >> + >> int intel_modeset_init(struct drm_device *dev) { >> struct drm_i915_private *dev_priv = to_i915(dev); @@ -15221,6 >> +15266,10 @@ int intel_modeset_init(struct drm_device *dev) >> intel_find_initial_plane_obj(crtc, &plane_config); >> } >> >> + /* Force all active planes to recompute their states */ >> + for_each_intel_crtc(dev, crtc) >> + intel_initial_commit(dev, crtc); >> + >> /* >> * Make sure hardware watermarks really match the state we read >out. >> * Note that we need to do this after reconstructing the BIOS fb's >> -- >> 1.9.1 > >-- >Ville Syrjälä >Intel Regards, Azhar Shaikh
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3709fa1b6318..1511d58991cc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15092,6 +15092,51 @@ static void intel_update_fdi_pll_freq(struct drm_i915_private *dev_priv) DRM_DEBUG_DRIVER("FDI PLL freq=%d\n", dev_priv->fdi_pll_freq); } +static void intel_initial_commit(struct drm_device *dev, + struct intel_crtc *intel_crtc) +{ + struct drm_atomic_state *state = NULL; + struct drm_crtc *crtc = &intel_crtc->base; + struct drm_modeset_acquire_ctx ctx; + struct drm_crtc_state *crtc_state; + int ret; + + drm_modeset_acquire_init(&ctx, 0); + + state = drm_atomic_state_alloc(dev); + if (!state) + goto unlock; + + state->acquire_ctx = &ctx; + + crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(crtc_state)) + goto out; + + if (!crtc_state->active) + goto out; + + ret = drm_atomic_add_affected_planes(state, crtc); + if (ret) + goto out; + + ret = drm_atomic_commit(state); + if (ret == -EDEADLK) { + drm_atomic_state_clear(state); + drm_modeset_backoff(&ctx); + } + +out: + if (state) { + drm_atomic_state_put(state); + state = NULL; + } + +unlock: + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); +} + int intel_modeset_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = to_i915(dev); @@ -15221,6 +15266,10 @@ int intel_modeset_init(struct drm_device *dev) intel_find_initial_plane_obj(crtc, &plane_config); } + /* Force all active planes to recompute their states */ + for_each_intel_crtc(dev, crtc) + intel_initial_commit(dev, crtc); + /* * Make sure hardware watermarks really match the state we read out. * Note that we need to do this after reconstructing the BIOS fb's
On KBL, WHL RVPs, booting up with an external display connected, triggers below warning, when the BiOS brings up the external display too. 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, force all active planes to recompute their states in probe. Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com> --- Changes in v2: - Force all planes to recompute their states.(Ville Syrjälä) - Update the commit message drivers/gpu/drm/i915/intel_display.c | 49 ++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)