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

2022-06-15 Thread Ville Syrjälä 
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:  [#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]  
> >> [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]  
> >> [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 
> >> ---
> >> 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(>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(>drm);
> >>   
> >>drm_atomic_helper_shutdown(>drm);
> > 
> > You can't suspend MST before this since this is what actually turns the
> > displays

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

2022-06-14 Thread Andrzej Hajda 

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:  [#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]  
[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]  
[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 
---
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(>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(>drm);
  
  		drm_atomic_helper_shutdown(>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.

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

2022-06-10 Thread Ville Syrjälä 
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:  [#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]  
> [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]  
> [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 
> ---
> 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(>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(>drm);
>  
>   drm_atomic_helper_shutdown(>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