Re: [Intel-gfx] [PATCH v7] drm/i915/display: disable HPD workers before display driver unregister

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jun 14, 2022 at 05:06:40PM +0200, Andrzej Hajda wrote:
> On 10.06.2022 20:37, Ville Syrjälä wrote:
> > On Fri, Jun 10, 2022 at 06:00:24PM +0200, Andrzej Hajda wrote:
> >> Handling HPD during driver removal is pointless, and can cause different
> >> use-after-free/concurrency issues:
> >> 1. Setup of deferred fbdev after fbdev unregistration.
> >> 2. Access to DP-AUX after DP-AUX removal.
> >>
> >> Below stacktraces of both cases observed on CI:
> >>
> >> [272.634530] general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b6b: 0000 [#1] PREEMPT SMP NOPTI
> >> [272.634536] CPU: 0 PID: 6030 Comm: i915_selftest Tainted: G     U            5.18.0-rc5-CI_DRM_11603-g12dccf4f5eef+ #1
> >> [272.634541] Hardware name: Intel Corporation Raptor Lake Client Platform/RPL-S ADP-S DDR5 UDIMM CRB, BIOS RPLSFWI1.R00.2397.A01.2109300731 09/30/2021
> >> [272.634545] RIP: 0010:fb_do_apertures_overlap.part.14+0x26/0x60
> >> ...
> >> [272.634582] Call Trace:
> >> [272.634583]  <TASK>
> >> [272.634585]  do_remove_conflicting_framebuffers+0x59/0xa0
> >> [272.634589]  remove_conflicting_framebuffers+0x2d/0xc0
> >> [272.634592]  remove_conflicting_pci_framebuffers+0xc8/0x110
> >> [272.634595]  drm_aperture_remove_conflicting_pci_framebuffers+0x52/0x70
> >> [272.634604]  i915_driver_probe+0x63a/0xdd0 [i915]
> >>
> >> [283.405824] cpu_latency_qos_update_request called for unknown object
> >> [283.405866] WARNING: CPU: 2 PID: 240 at kernel/power/qos.c:296 cpu_latency_qos_update_request+0x2d/0x100
> >> [283.405912] CPU: 2 PID: 240 Comm: kworker/u64:9 Not tainted 5.18.0-rc6-Patchwork_103738v3-g1672d1c43e43+ #1
> >> [283.405915] Hardware name: Intel Corporation Raptor Lake Client Platform/RPL-S ADP-S DDR5 UDIMM CRB, BIOS RPLSFWI1.R00.2397.A01.2109300731 09/30/2021
> >> [283.405916] Workqueue: i915-dp i915_digport_work_func [i915]
> >> [283.406020] RIP: 0010:cpu_latency_qos_update_request+0x2d/0x100
> >> ...
> >> [283.406040] Call Trace:
> >> [283.406041]  <TASK>
> >> [283.406044]  intel_dp_aux_xfer+0x60e/0x8e0 [i915]
> >> [283.406131]  ? finish_swait+0x80/0x80
> >> [283.406139]  intel_dp_aux_transfer+0xc5/0x2b0 [i915]
> >> [283.406218]  drm_dp_dpcd_access+0x79/0x130 [drm_display_helper]
> >> [283.406227]  drm_dp_dpcd_read+0xe2/0xf0 [drm_display_helper]
> >> [283.406233]  intel_dp_hpd_pulse+0x134/0x570 [i915]
> >> [283.406308]  ? __down_killable+0x70/0x140
> >> [283.406313]  i915_digport_work_func+0xba/0x150 [i915]
> >>
> >> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> >> ---
> >> Hi All,
> >>
> >> I am not sure about changes in shutdown path, any comments welcome.
> >> I suspect suspend path have also some common bits, but I am little
> >> bit afraid of touching it.
> >>
> >> Changes:
> >> v1 - v6:
> >>      - chasing the bug appearing only on public CI.
> >> v7:
> >>      - shutdown path adjusted (suggested by Jani)
> >>
> >> Regards
> >> Andrzej
> >> ---
> >>   drivers/gpu/drm/i915/display/intel_display.c | 11 ++++-------
> >>   drivers/gpu/drm/i915/i915_driver.c           |  5 ++---
> >>   2 files changed, 6 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >> index 186b37925d23f2..f9952ee8289fb2 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> @@ -10490,13 +10490,6 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
> >>   	 */
> >>   	intel_hpd_poll_fini(i915);
> >>   
> >> -	/*
> >> -	 * MST topology needs to be suspended so we don't have any calls to
> >> -	 * fbdev after it's finalized. MST will be destroyed later as part of
> >> -	 * drm_mode_config_cleanup()
> >> -	 */
> >> -	intel_dp_mst_suspend(i915);
> >> -
> >>   	/* poll work can call into fbdev, hence clean that up afterwards */
> >>   	intel_fbdev_fini(i915);
> >>   
> >> @@ -10588,6 +10581,10 @@ void intel_display_driver_unregister(struct drm_i915_private *i915)
> >>   	if (!HAS_DISPLAY(i915))
> >>   		return;
> >>   
> >> +	intel_dp_mst_suspend(i915);
> >> +	intel_hpd_cancel_work(i915);
> >> +	drm_kms_helper_poll_disable(&i915->drm);
> >> +
> >>   	intel_fbdev_unregister(i915);
> >>   	intel_audio_deinit(i915);
> >>   
> >> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> >> index d26dcca7e654aa..82cdccf072e2bc 100644
> >> --- a/drivers/gpu/drm/i915/i915_driver.c
> >> +++ b/drivers/gpu/drm/i915/i915_driver.c
> >> @@ -1070,15 +1070,14 @@ void i915_driver_shutdown(struct drm_i915_private *i915)
> >>   	i915_gem_suspend(i915);
> >>   
> >>   	if (HAS_DISPLAY(i915)) {
> >> +		intel_dp_mst_suspend(i915);
> >> +		intel_hpd_cancel_work(i915);
> >>   		drm_kms_helper_poll_disable(&i915->drm);
> >>   
> >>   		drm_atomic_helper_shutdown(&i915->drm);
> > 
> > You can't suspend MST before this since this is what actually turns the
> > displays off.
> > 
> > The real chicken and egg sitaation is due to MST sideband depending
> > on HPD_IRQs to work, but we want to stop the rest of hotplug processing
> > before we shut down the displays to make sure fbdev/etc. doesn't light
> > them back up.
> > 
> > If we didn't have MST sidband we could just turn off hotplug interrupts
> > ahead of time and flush the works, but with MST we need to keep the
> > interrupts alive. So I suspect we need some kind of flag to indicate
> > that at least full hotplug handling should not happen even though the
> > hotplug interrupts are still enabled.
> 
> 
> Thanks for explanation.
> As usual, reality is more complicated than expectations :)
> I was wondering about HPD during removal/shutdown/suspend.
> - HPD-plug should be ignored.
> - HPD-unplug probably should be handled - to avoid possible attempts to 
> communicate to non-exisitng sinks.
> So maybe at the beginning of removal/shutdown and maybe suspend we could 
> perform kind of cold HPD-unplug? What do you think?

Talking to non-existing sinks is perfectly normal. Happens every time
you pulle the cable out.

-- 
Ville Syrjälä
Intel