dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/4] drm/i915/display: stop HPD workers before display driver unregister
@ 2022-07-22 12:51 Andrzej Hajda
  2022-07-22 12:51 ` [PATCH v6 1/4] drm/i915/hpd: postpone HPD cancel work after last user suspension Andrzej Hajda
                   ` (4 more replies)
  0 siblings, 5 replies; 18+ messages in thread
From: Andrzej Hajda @ 2022-07-22 12:51 UTC (permalink / raw)
  To: Jani Nikula, Ville Syrjälä, Arun R Murthy
  Cc: Tvrtko Ursulin, Andrzej Hajda, intel-gfx, dri-devel, Rodrigo Vivi

Hi Jani, Ville, Arun,

This patchset is replacement of patch
"drm/i915/display: disable HPD workers before display driver unregister" [1].
Ive decided to split patch into two parts - fbdev and MST, there are different
issues.
Ive also dropped shutdown path, as it has slightly different requirements,
and more importantly I am not able to test properly.

v2 (thx Arun for review):
  - reword of commit message (Arun)
  - intel_fbdev_hpd_set_suspend replaced with intel_fbdev_set_suspend (Arun)
v3:
  - new patch adding suspended flag, to handle
    https://gitlab.freedesktop.org/drm/intel/-/issues/5950
v4:
  - check suspend flag also in i915_digport_work_func
v5:
  - added patch blocking FB creation in case HPD is supended,
  - added R-B from Arun to patch 3, thx
v6:
  - finally, after getting direct access to bat-rpls-2, I have found the source of last WARN,
    intel_fbdev_hpd_set_suspend was not called in case of deferred setup, fixed in patch 2.

[1]: https://patchwork.freedesktop.org/series/103811/

Regards
Andrzej


Andrzej Hajda (4):
  drm/i915/hpd: postpone HPD cancel work after last user suspension
  drm/i915/fbdev: suspend HPD before fbdev unregistration
  drm/i915/display: add hotplug.suspended flag
  drm/i915/fbdev: do not create fbdev if HPD is suspended

 drivers/gpu/drm/i915/display/intel_display.c |  3 +++
 drivers/gpu/drm/i915/display/intel_fbdev.c   | 12 ++++++++++--
 drivers/gpu/drm/i915/display/intel_hotplug.c | 11 ++++++++++-
 drivers/gpu/drm/i915/display/intel_hotplug.h |  2 +-
 drivers/gpu/drm/i915/i915_driver.c           |  4 ++--
 drivers/gpu/drm/i915/i915_drv.h              |  2 ++
 drivers/gpu/drm/i915/i915_irq.c              |  1 -
 7 files changed, 28 insertions(+), 7 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v6 1/4] drm/i915/hpd: postpone HPD cancel work after last user suspension
  2022-07-22 12:51 [PATCH v6 0/4] drm/i915/display: stop HPD workers before display driver unregister Andrzej Hajda
@ 2022-07-22 12:51 ` Andrzej Hajda
  2022-08-22 17:08   ` Imre Deak
  2022-07-22 12:51 ` [PATCH v6 2/4] drm/i915/fbdev: suspend HPD before fbdev unregistration Andrzej Hajda
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Andrzej Hajda @ 2022-07-22 12:51 UTC (permalink / raw)
  To: Jani Nikula, Ville Syrjälä, Arun R Murthy
  Cc: Tvrtko Ursulin, Andrzej Hajda, intel-gfx, dri-devel, Rodrigo Vivi

i915->hotplug.dig_port_work can be queued from intel_hpd_irq_handler
called by IRQ handler or by intel_hpd_trigger_irq called from dp_mst.
Since dp_mst is suspended after irq handler uninstall, a cleaner approach
is to cancel hpd work after intel_dp_mst_suspend, otherwise we risk
use-after-free.

It should fix following WARNINGS:
[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]

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4586
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5558
Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 3 +++
 drivers/gpu/drm/i915/i915_irq.c              | 1 -
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index a0f84cbe974fc3..f1c765ac7ab8aa 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -9021,6 +9021,9 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
 	 */
 	intel_dp_mst_suspend(i915);
 
+	/* MST is the last user of HPD work */
+	intel_hpd_cancel_work(i915);
+
 	/* poll work can call into fbdev, hence clean that up afterwards */
 	intel_fbdev_fini(i915);
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 73cebc6aa65072..db14787aef95dd 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4597,7 +4597,6 @@ void intel_irq_uninstall(struct drm_i915_private *dev_priv)
 
 	free_irq(irq, dev_priv);
 
-	intel_hpd_cancel_work(dev_priv);
 	dev_priv->runtime_pm.irqs_enabled = false;
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v6 2/4] drm/i915/fbdev: suspend HPD before fbdev unregistration
  2022-07-22 12:51 [PATCH v6 0/4] drm/i915/display: stop HPD workers before display driver unregister Andrzej Hajda
  2022-07-22 12:51 ` [PATCH v6 1/4] drm/i915/hpd: postpone HPD cancel work after last user suspension Andrzej Hajda
@ 2022-07-22 12:51 ` Andrzej Hajda
  2022-08-22 17:10   ` [Intel-gfx] " Imre Deak
  2022-07-22 12:51 ` [PATCH v6 3/4] drm/i915/display: add hotplug.suspended flag Andrzej Hajda
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Andrzej Hajda @ 2022-07-22 12:51 UTC (permalink / raw)
  To: Jani Nikula, Ville Syrjälä, Arun R Murthy
  Cc: Tvrtko Ursulin, Andrzej Hajda, intel-gfx, dri-devel, Rodrigo Vivi

HPD event after fbdev unregistration can cause registration of deferred
fbdev which will not be unregistered later, causing use-after-free.
To avoid it HPD handling should be suspended before fbdev unregistration.

It should fix following GPF:
[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]

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5329
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5510
Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbdev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index 221336178991f0..94ddc0f34fde64 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -573,7 +573,8 @@ void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
 	if (!ifbdev)
 		return;
 
-	cancel_work_sync(&dev_priv->fbdev_suspend_work);
+	intel_fbdev_set_suspend(&dev_priv->drm, FBINFO_STATE_SUSPENDED, true);
+
 	if (!current_is_async())
 		intel_fbdev_sync(ifbdev);
 
@@ -618,7 +619,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
 	struct fb_info *info;
 
 	if (!ifbdev || !ifbdev->vma)
-		return;
+		goto unlock;
 
 	info = ifbdev->helper.fbdev;
 
@@ -661,6 +662,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
 	drm_fb_helper_set_suspend(&ifbdev->helper, state);
 	console_unlock();
 
+unlock:
 	intel_fbdev_hpd_set_suspend(dev_priv, state);
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v6 3/4] drm/i915/display: add hotplug.suspended flag
  2022-07-22 12:51 [PATCH v6 0/4] drm/i915/display: stop HPD workers before display driver unregister Andrzej Hajda
  2022-07-22 12:51 ` [PATCH v6 1/4] drm/i915/hpd: postpone HPD cancel work after last user suspension Andrzej Hajda
  2022-07-22 12:51 ` [PATCH v6 2/4] drm/i915/fbdev: suspend HPD before fbdev unregistration Andrzej Hajda
@ 2022-07-22 12:51 ` Andrzej Hajda
  2022-08-22 17:27   ` [Intel-gfx] " Imre Deak
  2022-07-22 12:51 ` [PATCH v6 4/4] drm/i915/fbdev: do not create fbdev if HPD is suspended Andrzej Hajda
  2022-08-02 12:24 ` [Intel-gfx] [PATCH v6 0/4] drm/i915/display: stop HPD workers before display driver unregister Gwan-gyeong Mun
  4 siblings, 1 reply; 18+ messages in thread
From: Andrzej Hajda @ 2022-07-22 12:51 UTC (permalink / raw)
  To: Jani Nikula, Ville Syrjälä, Arun R Murthy
  Cc: Tvrtko Ursulin, Andrzej Hajda, intel-gfx, dri-devel, Rodrigo Vivi

HPD events during driver removal can be generated by hardware and
software frameworks - drm_dp_mst, the former we can avoid by disabling
interrupts, the latter can be triggered by any drm_dp_mst transaction,
and this is too late. Introducing suspended flag allows to solve this
chicken-egg problem.

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5950
Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c |  2 +-
 drivers/gpu/drm/i915/display/intel_hotplug.c | 11 ++++++++++-
 drivers/gpu/drm/i915/display/intel_hotplug.h |  2 +-
 drivers/gpu/drm/i915/i915_driver.c           |  4 ++--
 drivers/gpu/drm/i915/i915_drv.h              |  2 ++
 5 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index f1c765ac7ab8aa..cd6139bb36151b 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -9022,7 +9022,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
 	intel_dp_mst_suspend(i915);
 
 	/* MST is the last user of HPD work */
-	intel_hpd_cancel_work(i915);
+	intel_hpd_suspend(i915);
 
 	/* poll work can call into fbdev, hence clean that up afterwards */
 	intel_fbdev_fini(i915);
diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c
index 5f8b4f481cff9a..e1d384cb99df6b 100644
--- a/drivers/gpu/drm/i915/display/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
@@ -303,6 +303,8 @@ static void i915_digport_work_func(struct work_struct *work)
 	u32 old_bits = 0;
 
 	spin_lock_irq(&dev_priv->irq_lock);
+	if (dev_priv->hotplug.suspended)
+		return spin_unlock_irq(&dev_priv->irq_lock);
 	long_port_mask = dev_priv->hotplug.long_port_mask;
 	dev_priv->hotplug.long_port_mask = 0;
 	short_port_mask = dev_priv->hotplug.short_port_mask;
@@ -353,6 +355,8 @@ void intel_hpd_trigger_irq(struct intel_digital_port *dig_port)
 	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
 
 	spin_lock_irq(&i915->irq_lock);
+	if (i915->hotplug.suspended)
+		return spin_unlock_irq(&i915->irq_lock);
 	i915->hotplug.short_port_mask |= BIT(dig_port->base.port);
 	spin_unlock_irq(&i915->irq_lock);
 
@@ -475,6 +479,9 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
 
 	spin_lock(&dev_priv->irq_lock);
 
+	if (dev_priv->hotplug.suspended)
+		return spin_unlock(&dev_priv->irq_lock);
+
 	/*
 	 * Determine whether ->hpd_pulse() exists for each pin, and
 	 * whether we have a short or a long pulse. This is needed
@@ -603,6 +610,7 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
 	 * just to make the assert_spin_locked checks happy.
 	 */
 	spin_lock_irq(&dev_priv->irq_lock);
+	dev_priv->hotplug.suspended = false;
 	intel_hpd_irq_setup(dev_priv);
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
@@ -721,13 +729,14 @@ void intel_hpd_init_work(struct drm_i915_private *dev_priv)
 			  intel_hpd_irq_storm_reenable_work);
 }
 
-void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
+void intel_hpd_suspend(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_DISPLAY(dev_priv))
 		return;
 
 	spin_lock_irq(&dev_priv->irq_lock);
 
+	dev_priv->hotplug.suspended = true;
 	dev_priv->hotplug.long_port_mask = 0;
 	dev_priv->hotplug.short_port_mask = 0;
 	dev_priv->hotplug.event_bits = 0;
diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.h b/drivers/gpu/drm/i915/display/intel_hotplug.h
index b87e95d606e668..54bddc4dd63421 100644
--- a/drivers/gpu/drm/i915/display/intel_hotplug.h
+++ b/drivers/gpu/drm/i915/display/intel_hotplug.h
@@ -23,7 +23,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
 void intel_hpd_trigger_irq(struct intel_digital_port *dig_port);
 void intel_hpd_init(struct drm_i915_private *dev_priv);
 void intel_hpd_init_work(struct drm_i915_private *dev_priv);
-void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
+void intel_hpd_suspend(struct drm_i915_private *dev_priv);
 enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
 				   enum port port);
 bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin);
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index deb8a8b76965a1..57a063a306e3a4 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1092,7 +1092,7 @@ void i915_driver_shutdown(struct drm_i915_private *i915)
 	intel_dp_mst_suspend(i915);
 
 	intel_runtime_pm_disable_interrupts(i915);
-	intel_hpd_cancel_work(i915);
+	intel_hpd_suspend(i915);
 
 	intel_suspend_encoders(i915);
 	intel_shutdown_encoders(i915);
@@ -1161,7 +1161,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 	intel_dp_mst_suspend(dev_priv);
 
 	intel_runtime_pm_disable_interrupts(dev_priv);
-	intel_hpd_cancel_work(dev_priv);
+	intel_hpd_suspend(dev_priv);
 
 	intel_suspend_encoders(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d25647be25d18b..dc1562b95d7ade 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -106,6 +106,8 @@ struct vlv_s0ix_state;
 #define HPD_STORM_DEFAULT_THRESHOLD 50
 
 struct i915_hotplug {
+	bool suspended;
+
 	struct delayed_work hotplug_work;
 
 	const u32 *hpd, *pch_hpd;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v6 4/4] drm/i915/fbdev: do not create fbdev if HPD is suspended
  2022-07-22 12:51 [PATCH v6 0/4] drm/i915/display: stop HPD workers before display driver unregister Andrzej Hajda
                   ` (2 preceding siblings ...)
  2022-07-22 12:51 ` [PATCH v6 3/4] drm/i915/display: add hotplug.suspended flag Andrzej Hajda
@ 2022-07-22 12:51 ` Andrzej Hajda
  2022-07-26  6:50   ` Murthy, Arun R
  2022-08-22 17:28   ` Imre Deak
  2022-08-02 12:24 ` [Intel-gfx] [PATCH v6 0/4] drm/i915/display: stop HPD workers before display driver unregister Gwan-gyeong Mun
  4 siblings, 2 replies; 18+ messages in thread
From: Andrzej Hajda @ 2022-07-22 12:51 UTC (permalink / raw)
  To: Jani Nikula, Ville Syrjälä, Arun R Murthy
  Cc: Tvrtko Ursulin, Andrzej Hajda, intel-gfx, dri-devel, Rodrigo Vivi

In case of deferred FB setup core can try to create new
framebuffer. Disallow it if hpd_suspended flag is set.

Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index 94ddc0f34fde64..fb8dbd532b9e05 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -210,6 +210,12 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	struct drm_i915_gem_object *obj;
 	int ret;
 
+	mutex_lock(&ifbdev->hpd_lock);
+	ret = ifbdev->hpd_suspended ? -EAGAIN : 0;
+	mutex_unlock(&ifbdev->hpd_lock);
+	if (ret)
+		return ret;
+
 	if (intel_fb &&
 	    (sizes->fb_width > intel_fb->base.width ||
 	     sizes->fb_height > intel_fb->base.height)) {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* RE: [PATCH v6 4/4] drm/i915/fbdev: do not create fbdev if HPD is suspended
  2022-07-22 12:51 ` [PATCH v6 4/4] drm/i915/fbdev: do not create fbdev if HPD is suspended Andrzej Hajda
@ 2022-07-26  6:50   ` Murthy, Arun R
  2022-08-22 17:28   ` Imre Deak
  1 sibling, 0 replies; 18+ messages in thread
From: Murthy, Arun R @ 2022-07-26  6:50 UTC (permalink / raw)
  To: Hajda, Andrzej, Jani Nikula, Ville Syrjälä
  Cc: Tvrtko Ursulin, intel-gfx, dri-devel, Vivi, Rodrigo

> -----Original Message-----
> From: Hajda, Andrzej <andrzej.hajda@intel.com>
> Sent: Friday, July 22, 2022 6:22 PM
> To: Jani Nikula <jani.nikula@linux.intel.com>; Ville Syrjälä
> <ville.syrjala@linux.intel.com>; Murthy, Arun R <arun.r.murthy@intel.com>
> Cc: Hajda, Andrzej <andrzej.hajda@intel.com>; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>; Daniel Vetter
> <daniel@ffwll.ch>; intel-gfx@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org
> Subject: [PATCH v6 4/4] drm/i915/fbdev: do not create fbdev if HPD is
> suspended
> 
> In case of deferred FB setup core can try to create new framebuffer. Disallow
> it if hpd_suspended flag is set.
> 
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> ---
Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>

Thanks and Regards,
Arun R Murthy
--------------------

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Intel-gfx] [PATCH v6 0/4] drm/i915/display: stop HPD workers before display driver unregister
  2022-07-22 12:51 [PATCH v6 0/4] drm/i915/display: stop HPD workers before display driver unregister Andrzej Hajda
                   ` (3 preceding siblings ...)
  2022-07-22 12:51 ` [PATCH v6 4/4] drm/i915/fbdev: do not create fbdev if HPD is suspended Andrzej Hajda
@ 2022-08-02 12:24 ` Gwan-gyeong Mun
  2022-08-11  8:33   ` Andrzej Hajda
  4 siblings, 1 reply; 18+ messages in thread
From: Gwan-gyeong Mun @ 2022-08-02 12:24 UTC (permalink / raw)
  To: Jani Nikula, Ville Syrjälä, Deak, Imre
  Cc: intel-gfx, Arun R Murthy, Andrzej Hajda, dri-devel, Rodrigo Vivi

Hi Jani, Ville and Imre,

If there are no problems after reviewing this patch series, could you 
please merge it?

Many thanks,
G.G.

On 7/22/22 3:51 PM, Andrzej Hajda wrote:
> Hi Jani, Ville, Arun,
> 
> This patchset is replacement of patch
> "drm/i915/display: disable HPD workers before display driver unregister" [1].
> Ive decided to split patch into two parts - fbdev and MST, there are different
> issues.
> Ive also dropped shutdown path, as it has slightly different requirements,
> and more importantly I am not able to test properly.
> 
> v2 (thx Arun for review):
>    - reword of commit message (Arun)
>    - intel_fbdev_hpd_set_suspend replaced with intel_fbdev_set_suspend (Arun)
> v3:
>    - new patch adding suspended flag, to handle
>      https://gitlab.freedesktop.org/drm/intel/-/issues/5950
> v4:
>    - check suspend flag also in i915_digport_work_func
> v5:
>    - added patch blocking FB creation in case HPD is supended,
>    - added R-B from Arun to patch 3, thx
> v6:
>    - finally, after getting direct access to bat-rpls-2, I have found the source of last WARN,
>      intel_fbdev_hpd_set_suspend was not called in case of deferred setup, fixed in patch 2.
> 
> [1]: https://patchwork.freedesktop.org/series/103811/
> 
> Regards
> Andrzej
> 
> 
> Andrzej Hajda (4):
>    drm/i915/hpd: postpone HPD cancel work after last user suspension
>    drm/i915/fbdev: suspend HPD before fbdev unregistration
>    drm/i915/display: add hotplug.suspended flag
>    drm/i915/fbdev: do not create fbdev if HPD is suspended
> 
>   drivers/gpu/drm/i915/display/intel_display.c |  3 +++
>   drivers/gpu/drm/i915/display/intel_fbdev.c   | 12 ++++++++++--
>   drivers/gpu/drm/i915/display/intel_hotplug.c | 11 ++++++++++-
>   drivers/gpu/drm/i915/display/intel_hotplug.h |  2 +-
>   drivers/gpu/drm/i915/i915_driver.c           |  4 ++--
>   drivers/gpu/drm/i915/i915_drv.h              |  2 ++
>   drivers/gpu/drm/i915/i915_irq.c              |  1 -
>   7 files changed, 28 insertions(+), 7 deletions(-)
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Intel-gfx] [PATCH v6 0/4] drm/i915/display: stop HPD workers before display driver unregister
  2022-08-02 12:24 ` [Intel-gfx] [PATCH v6 0/4] drm/i915/display: stop HPD workers before display driver unregister Gwan-gyeong Mun
@ 2022-08-11  8:33   ` Andrzej Hajda
  0 siblings, 0 replies; 18+ messages in thread
From: Andrzej Hajda @ 2022-08-11  8:33 UTC (permalink / raw)
  To: Gwan-gyeong Mun, Jani Nikula, Ville Syrjälä, Deak, Imre
  Cc: intel-gfx, dri-devel, Rodrigo Vivi

Hi Imre, Jani, Ville,

Since one of CI test machines is back (bat-rpls-2) tests are regularly 
aborted on this machine due to bugs this patchset resolves [1], 
reviewing/merging these patches would allow to cure the situation on CI.

[1]: https://intel-gfx-ci.01.org/tree/drm-tip/bat-rpls-2.html

Regards
Andrzej

On 02.08.2022 14:24, Gwan-gyeong Mun wrote:
> Hi Jani, Ville and Imre,
> 
> If there are no problems after reviewing this patch series, could you 
> please merge it?
> 
> Many thanks,
> G.G.
> 
> On 7/22/22 3:51 PM, Andrzej Hajda wrote:
>> Hi Jani, Ville, Arun,
>>
>> This patchset is replacement of patch
>> "drm/i915/display: disable HPD workers before display driver 
>> unregister" [1].
>> Ive decided to split patch into two parts - fbdev and MST, there are 
>> different
>> issues.
>> Ive also dropped shutdown path, as it has slightly different 
>> requirements,
>> and more importantly I am not able to test properly.
>>
>> v2 (thx Arun for review):
>>    - reword of commit message (Arun)
>>    - intel_fbdev_hpd_set_suspend replaced with intel_fbdev_set_suspend 
>> (Arun)
>> v3:
>>    - new patch adding suspended flag, to handle
>>      https://gitlab.freedesktop.org/drm/intel/-/issues/5950
>> v4:
>>    - check suspend flag also in i915_digport_work_func
>> v5:
>>    - added patch blocking FB creation in case HPD is supended,
>>    - added R-B from Arun to patch 3, thx
>> v6:
>>    - finally, after getting direct access to bat-rpls-2, I have found 
>> the source of last WARN,
>>      intel_fbdev_hpd_set_suspend was not called in case of deferred 
>> setup, fixed in patch 2.
>>
>> [1]: https://patchwork.freedesktop.org/series/103811/
>>
>> Regards
>> Andrzej
>>
>>
>> Andrzej Hajda (4):
>>    drm/i915/hpd: postpone HPD cancel work after last user suspension
>>    drm/i915/fbdev: suspend HPD before fbdev unregistration
>>    drm/i915/display: add hotplug.suspended flag
>>    drm/i915/fbdev: do not create fbdev if HPD is suspended
>>
>>   drivers/gpu/drm/i915/display/intel_display.c |  3 +++
>>   drivers/gpu/drm/i915/display/intel_fbdev.c   | 12 ++++++++++--
>>   drivers/gpu/drm/i915/display/intel_hotplug.c | 11 ++++++++++-
>>   drivers/gpu/drm/i915/display/intel_hotplug.h |  2 +-
>>   drivers/gpu/drm/i915/i915_driver.c           |  4 ++--
>>   drivers/gpu/drm/i915/i915_drv.h              |  2 ++
>>   drivers/gpu/drm/i915/i915_irq.c              |  1 -
>>   7 files changed, 28 insertions(+), 7 deletions(-)
>>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v6 1/4] drm/i915/hpd: postpone HPD cancel work after last user suspension
  2022-07-22 12:51 ` [PATCH v6 1/4] drm/i915/hpd: postpone HPD cancel work after last user suspension Andrzej Hajda
@ 2022-08-22 17:08   ` Imre Deak
  2022-08-23  7:41     ` Jani Nikula
  0 siblings, 1 reply; 18+ messages in thread
From: Imre Deak @ 2022-08-22 17:08 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: Tvrtko Ursulin, intel-gfx, dri-devel, Rodrigo Vivi, Arun R Murthy

On Fri, Jul 22, 2022 at 02:51:40PM +0200, Andrzej Hajda wrote:
> i915->hotplug.dig_port_work can be queued from intel_hpd_irq_handler
> called by IRQ handler or by intel_hpd_trigger_irq called from dp_mst.
> Since dp_mst is suspended after irq handler uninstall, a cleaner approach
> is to cancel hpd work after intel_dp_mst_suspend, otherwise we risk
> use-after-free.
> 
> It should fix following WARNINGS:
> [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]
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4586
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5558
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 3 +++
>  drivers/gpu/drm/i915/i915_irq.c              | 1 -
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index a0f84cbe974fc3..f1c765ac7ab8aa 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -9021,6 +9021,9 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
>  	 */
>  	intel_dp_mst_suspend(i915);
>  
> +	/* MST is the last user of HPD work */
> +	intel_hpd_cancel_work(i915);
> +

MST still requires AUX and short HPD interrupts and during shutdown and
suspend the order is suspend-MST -> disable-IRQs. So I think it makes
more sense to move intel_dp_mst_suspend() to i915_driver_remove() before
intel_irq_uninstall().

>  	/* poll work can call into fbdev, hence clean that up afterwards */
>  	intel_fbdev_fini(i915);
>  
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 73cebc6aa65072..db14787aef95dd 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4597,7 +4597,6 @@ void intel_irq_uninstall(struct drm_i915_private *dev_priv)
>  
>  	free_irq(irq, dev_priv);
>  
> -	intel_hpd_cancel_work(dev_priv);
>  	dev_priv->runtime_pm.irqs_enabled = false;
>  }
>  
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Intel-gfx] [PATCH v6 2/4] drm/i915/fbdev: suspend HPD before fbdev unregistration
  2022-07-22 12:51 ` [PATCH v6 2/4] drm/i915/fbdev: suspend HPD before fbdev unregistration Andrzej Hajda
@ 2022-08-22 17:10   ` Imre Deak
  0 siblings, 0 replies; 18+ messages in thread
From: Imre Deak @ 2022-08-22 17:10 UTC (permalink / raw)
  To: Andrzej Hajda; +Cc: intel-gfx, dri-devel, Rodrigo Vivi, Arun R Murthy

On Fri, Jul 22, 2022 at 02:51:41PM +0200, Andrzej Hajda wrote:
> HPD event after fbdev unregistration can cause registration of deferred
> fbdev which will not be unregistered later, causing use-after-free.
> To avoid it HPD handling should be suspended before fbdev unregistration.
> 
> It should fix following GPF:
> [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]
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5329
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5510
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fbdev.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 221336178991f0..94ddc0f34fde64 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -573,7 +573,8 @@ void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
>  	if (!ifbdev)
>  		return;
>  
> -	cancel_work_sync(&dev_priv->fbdev_suspend_work);
> +	intel_fbdev_set_suspend(&dev_priv->drm, FBINFO_STATE_SUSPENDED, true);
> +
>  	if (!current_is_async())
>  		intel_fbdev_sync(ifbdev);
>  
> @@ -618,7 +619,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
>  	struct fb_info *info;
>  
>  	if (!ifbdev || !ifbdev->vma)
> -		return;
> +		goto unlock;

goto set_suspend; 

Reviewed-by: Imre Deak <imre.deak@intel.com>

>  
>  	info = ifbdev->helper.fbdev;
>  
> @@ -661,6 +662,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
>  	drm_fb_helper_set_suspend(&ifbdev->helper, state);
>  	console_unlock();
>  
> +unlock:
>  	intel_fbdev_hpd_set_suspend(dev_priv, state);
>  }
>  
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Intel-gfx] [PATCH v6 3/4] drm/i915/display: add hotplug.suspended flag
  2022-07-22 12:51 ` [PATCH v6 3/4] drm/i915/display: add hotplug.suspended flag Andrzej Hajda
@ 2022-08-22 17:27   ` Imre Deak
  2022-08-23 21:48     ` Andrzej Hajda
  0 siblings, 1 reply; 18+ messages in thread
From: Imre Deak @ 2022-08-22 17:27 UTC (permalink / raw)
  To: Andrzej Hajda; +Cc: intel-gfx, dri-devel, Rodrigo Vivi, Arun R Murthy

On Fri, Jul 22, 2022 at 02:51:42PM +0200, Andrzej Hajda wrote:
> HPD events during driver removal can be generated by hardware and
> software frameworks - drm_dp_mst, the former we can avoid by disabling
> interrupts, the latter can be triggered by any drm_dp_mst transaction,
> and this is too late. Introducing suspended flag allows to solve this
> chicken-egg problem.

intel_hpd_cancel_work() is always called after suspending MST and
disabling IRQs (with the order I suggested in patch 1). If both of these
have disabled the corresponding functionality (MST, IRQs) properly with
all their MST/IRQ scheduled works guaranteed to not get rescheduled,
then it's not clear how could either intel_hpd_trigger_irq() or an IRQ
work run. So the problematic sequence would need a better explanation.

There's also already
dev_priv->runtime_pm.irqs_enabled
showing if hotplug interrupts are disabled (along with all other IRQs).

> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5950
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  2 +-
>  drivers/gpu/drm/i915/display/intel_hotplug.c | 11 ++++++++++-
>  drivers/gpu/drm/i915/display/intel_hotplug.h |  2 +-
>  drivers/gpu/drm/i915/i915_driver.c           |  4 ++--
>  drivers/gpu/drm/i915/i915_drv.h              |  2 ++
>  5 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index f1c765ac7ab8aa..cd6139bb36151b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -9022,7 +9022,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
>  	intel_dp_mst_suspend(i915);
>  
>  	/* MST is the last user of HPD work */
> -	intel_hpd_cancel_work(i915);
> +	intel_hpd_suspend(i915);
>  
>  	/* poll work can call into fbdev, hence clean that up afterwards */
>  	intel_fbdev_fini(i915);
> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c
> index 5f8b4f481cff9a..e1d384cb99df6b 100644
> --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> @@ -303,6 +303,8 @@ static void i915_digport_work_func(struct work_struct *work)
>  	u32 old_bits = 0;
>  
>  	spin_lock_irq(&dev_priv->irq_lock);
> +	if (dev_priv->hotplug.suspended)
> +		return spin_unlock_irq(&dev_priv->irq_lock);
>  	long_port_mask = dev_priv->hotplug.long_port_mask;
>  	dev_priv->hotplug.long_port_mask = 0;
>  	short_port_mask = dev_priv->hotplug.short_port_mask;
> @@ -353,6 +355,8 @@ void intel_hpd_trigger_irq(struct intel_digital_port *dig_port)
>  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
>  
>  	spin_lock_irq(&i915->irq_lock);
> +	if (i915->hotplug.suspended)
> +		return spin_unlock_irq(&i915->irq_lock);
>  	i915->hotplug.short_port_mask |= BIT(dig_port->base.port);
>  	spin_unlock_irq(&i915->irq_lock);
>  
> @@ -475,6 +479,9 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
>  
>  	spin_lock(&dev_priv->irq_lock);
>  
> +	if (dev_priv->hotplug.suspended)
> +		return spin_unlock(&dev_priv->irq_lock);
> +
>  	/*
>  	 * Determine whether ->hpd_pulse() exists for each pin, and
>  	 * whether we have a short or a long pulse. This is needed
> @@ -603,6 +610,7 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
>  	 * just to make the assert_spin_locked checks happy.
>  	 */
>  	spin_lock_irq(&dev_priv->irq_lock);
> +	dev_priv->hotplug.suspended = false;
>  	intel_hpd_irq_setup(dev_priv);
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
> @@ -721,13 +729,14 @@ void intel_hpd_init_work(struct drm_i915_private *dev_priv)
>  			  intel_hpd_irq_storm_reenable_work);
>  }
>  
> -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
> +void intel_hpd_suspend(struct drm_i915_private *dev_priv)
>  {
>  	if (!HAS_DISPLAY(dev_priv))
>  		return;
>  
>  	spin_lock_irq(&dev_priv->irq_lock);
>  
> +	dev_priv->hotplug.suspended = true;
>  	dev_priv->hotplug.long_port_mask = 0;
>  	dev_priv->hotplug.short_port_mask = 0;
>  	dev_priv->hotplug.event_bits = 0;
> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.h b/drivers/gpu/drm/i915/display/intel_hotplug.h
> index b87e95d606e668..54bddc4dd63421 100644
> --- a/drivers/gpu/drm/i915/display/intel_hotplug.h
> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.h
> @@ -23,7 +23,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
>  void intel_hpd_trigger_irq(struct intel_digital_port *dig_port);
>  void intel_hpd_init(struct drm_i915_private *dev_priv);
>  void intel_hpd_init_work(struct drm_i915_private *dev_priv);
> -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
> +void intel_hpd_suspend(struct drm_i915_private *dev_priv);
>  enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
>  				   enum port port);
>  bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin);
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index deb8a8b76965a1..57a063a306e3a4 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1092,7 +1092,7 @@ void i915_driver_shutdown(struct drm_i915_private *i915)
>  	intel_dp_mst_suspend(i915);
>  
>  	intel_runtime_pm_disable_interrupts(i915);
> -	intel_hpd_cancel_work(i915);
> +	intel_hpd_suspend(i915);
>  
>  	intel_suspend_encoders(i915);
>  	intel_shutdown_encoders(i915);
> @@ -1161,7 +1161,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>  	intel_dp_mst_suspend(dev_priv);
>  
>  	intel_runtime_pm_disable_interrupts(dev_priv);
> -	intel_hpd_cancel_work(dev_priv);
> +	intel_hpd_suspend(dev_priv);
>  
>  	intel_suspend_encoders(dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d25647be25d18b..dc1562b95d7ade 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -106,6 +106,8 @@ struct vlv_s0ix_state;
>  #define HPD_STORM_DEFAULT_THRESHOLD 50
>  
>  struct i915_hotplug {
> +	bool suspended;
> +
>  	struct delayed_work hotplug_work;
>  
>  	const u32 *hpd, *pch_hpd;
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v6 4/4] drm/i915/fbdev: do not create fbdev if HPD is suspended
  2022-07-22 12:51 ` [PATCH v6 4/4] drm/i915/fbdev: do not create fbdev if HPD is suspended Andrzej Hajda
  2022-07-26  6:50   ` Murthy, Arun R
@ 2022-08-22 17:28   ` Imre Deak
  1 sibling, 0 replies; 18+ messages in thread
From: Imre Deak @ 2022-08-22 17:28 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: Tvrtko Ursulin, intel-gfx, dri-devel, Rodrigo Vivi, Arun R Murthy

On Fri, Jul 22, 2022 at 02:51:43PM +0200, Andrzej Hajda wrote:
> In case of deferred FB setup core can try to create new
> framebuffer. Disallow it if hpd_suspended flag is set.
> 
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_fbdev.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 94ddc0f34fde64..fb8dbd532b9e05 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -210,6 +210,12 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	struct drm_i915_gem_object *obj;
>  	int ret;
>  
> +	mutex_lock(&ifbdev->hpd_lock);
> +	ret = ifbdev->hpd_suspended ? -EAGAIN : 0;
> +	mutex_unlock(&ifbdev->hpd_lock);
> +	if (ret)
> +		return ret;
> +
>  	if (intel_fb &&
>  	    (sizes->fb_width > intel_fb->base.width ||
>  	     sizes->fb_height > intel_fb->base.height)) {
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v6 1/4] drm/i915/hpd: postpone HPD cancel work after last user suspension
  2022-08-22 17:08   ` Imre Deak
@ 2022-08-23  7:41     ` Jani Nikula
  2022-08-23  9:10       ` Imre Deak
  0 siblings, 1 reply; 18+ messages in thread
From: Jani Nikula @ 2022-08-23  7:41 UTC (permalink / raw)
  To: imre.deak, Andrzej Hajda
  Cc: Tvrtko Ursulin, intel-gfx, dri-devel, Rodrigo Vivi, Arun R Murthy

On Mon, 22 Aug 2022, Imre Deak <imre.deak@intel.com> wrote:
> On Fri, Jul 22, 2022 at 02:51:40PM +0200, Andrzej Hajda wrote:
>> i915->hotplug.dig_port_work can be queued from intel_hpd_irq_handler
>> called by IRQ handler or by intel_hpd_trigger_irq called from dp_mst.
>> Since dp_mst is suspended after irq handler uninstall, a cleaner approach
>> is to cancel hpd work after intel_dp_mst_suspend, otherwise we risk
>> use-after-free.
>> 
>> It should fix following WARNINGS:
>> [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]
>> 
>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4586
>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5558
>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
>> Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display.c | 3 +++
>>  drivers/gpu/drm/i915/i915_irq.c              | 1 -
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index a0f84cbe974fc3..f1c765ac7ab8aa 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -9021,6 +9021,9 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
>>  	 */
>>  	intel_dp_mst_suspend(i915);
>>  
>> +	/* MST is the last user of HPD work */
>> +	intel_hpd_cancel_work(i915);
>> +
>
> MST still requires AUX and short HPD interrupts and during shutdown and
> suspend the order is suspend-MST -> disable-IRQs. So I think it makes
> more sense to move intel_dp_mst_suspend() to i915_driver_remove() before
> intel_irq_uninstall().

The high level i915_driver_remove() code should only call high level
display functions, not something like intel_dp_mst_suspend() directly.

BR,
Jani.

>
>>  	/* poll work can call into fbdev, hence clean that up afterwards */
>>  	intel_fbdev_fini(i915);
>>  
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 73cebc6aa65072..db14787aef95dd 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -4597,7 +4597,6 @@ void intel_irq_uninstall(struct drm_i915_private *dev_priv)
>>  
>>  	free_irq(irq, dev_priv);
>>  
>> -	intel_hpd_cancel_work(dev_priv);
>>  	dev_priv->runtime_pm.irqs_enabled = false;
>>  }
>>  
>> -- 
>> 2.25.1
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v6 1/4] drm/i915/hpd: postpone HPD cancel work after last user suspension
  2022-08-23  7:41     ` Jani Nikula
@ 2022-08-23  9:10       ` Imre Deak
  0 siblings, 0 replies; 18+ messages in thread
From: Imre Deak @ 2022-08-23  9:10 UTC (permalink / raw)
  To: Jani Nikula
  Cc: Tvrtko Ursulin, intel-gfx, dri-devel, Andrzej Hajda,
	Rodrigo Vivi, Arun R Murthy

On Tue, Aug 23, 2022 at 10:41:22AM +0300, Jani Nikula wrote:
> On Mon, 22 Aug 2022, Imre Deak <imre.deak@intel.com> wrote:
> > On Fri, Jul 22, 2022 at 02:51:40PM +0200, Andrzej Hajda wrote:
> >> i915->hotplug.dig_port_work can be queued from intel_hpd_irq_handler
> >> called by IRQ handler or by intel_hpd_trigger_irq called from dp_mst.
> >> Since dp_mst is suspended after irq handler uninstall, a cleaner approach
> >> is to cancel hpd work after intel_dp_mst_suspend, otherwise we risk
> >> use-after-free.
> >> 
> >> It should fix following WARNINGS:
> >> [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]
> >> 
> >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4586
> >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5558
> >> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> >> Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_display.c | 3 +++
> >>  drivers/gpu/drm/i915/i915_irq.c              | 1 -
> >>  2 files changed, 3 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >> index a0f84cbe974fc3..f1c765ac7ab8aa 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> @@ -9021,6 +9021,9 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
> >>  	 */
> >>  	intel_dp_mst_suspend(i915);
> >>  
> >> +	/* MST is the last user of HPD work */
> >> +	intel_hpd_cancel_work(i915);
> >> +
> >
> > MST still requires AUX and short HPD interrupts and during shutdown and
> > suspend the order is suspend-MST -> disable-IRQs. So I think it makes
> > more sense to move intel_dp_mst_suspend() to i915_driver_remove() before
> > intel_irq_uninstall().
> 
> The high level i915_driver_remove() code should only call high level
> display functions, not something like intel_dp_mst_suspend() directly.

Ok, calling it at the end of intel_modeset_driver_remove() should be
still ok.

> BR,
> Jani.
> 
> >
> >>  	/* poll work can call into fbdev, hence clean that up afterwards */
> >>  	intel_fbdev_fini(i915);
> >>  
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> index 73cebc6aa65072..db14787aef95dd 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -4597,7 +4597,6 @@ void intel_irq_uninstall(struct drm_i915_private *dev_priv)
> >>  
> >>  	free_irq(irq, dev_priv);
> >>  
> >> -	intel_hpd_cancel_work(dev_priv);
> >>  	dev_priv->runtime_pm.irqs_enabled = false;
> >>  }
> >>  
> >> -- 
> >> 2.25.1
> >> 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Intel-gfx] [PATCH v6 3/4] drm/i915/display: add hotplug.suspended flag
  2022-08-22 17:27   ` [Intel-gfx] " Imre Deak
@ 2022-08-23 21:48     ` Andrzej Hajda
  2022-08-24 11:22       ` Imre Deak
  0 siblings, 1 reply; 18+ messages in thread
From: Andrzej Hajda @ 2022-08-23 21:48 UTC (permalink / raw)
  To: imre.deak; +Cc: intel-gfx, dri-devel, Rodrigo Vivi, Arun R Murthy



On 22.08.2022 19:27, Imre Deak wrote:
> On Fri, Jul 22, 2022 at 02:51:42PM +0200, Andrzej Hajda wrote:
>> HPD events during driver removal can be generated by hardware and
>> software frameworks - drm_dp_mst, the former we can avoid by disabling
>> interrupts, the latter can be triggered by any drm_dp_mst transaction,
>> and this is too late. Introducing suspended flag allows to solve this
>> chicken-egg problem.
> intel_hpd_cancel_work() is always called after suspending MST and
> disabling IRQs (with the order I suggested in patch 1). If both of these
> have disabled the corresponding functionality (MST, IRQs) properly with
> all their MST/IRQ scheduled works guaranteed to not get rescheduled,
> then it's not clear how could either intel_hpd_trigger_irq() or an IRQ
> work run. So the problematic sequence would need a better explanation.

I am not familiar with MST but as I understand from earlier discussion 
MST framework can be called during driver removal code even after 
intel_dp_mst_suspend. And since MST transfer can timeout it can trigger 
drm_dp_mst_wait_tx_reply --> mgr->cbs->poll_hpd_irq(mgr) --> 
intel_dp_mst_poll_hpd_irq --> intel_hpd_trigger_irq.

So actually this patch supersedes the 1st patch.

>
> There's also already
> dev_priv->runtime_pm.irqs_enabled
> showing if hotplug interrupts are disabled (along with all other IRQs).

So it is slightly different, this patch introduces flag indicating if 
HPD is enabled, we can have IRQs not related to HPD, and HPD events not 
related to IRQs.

Regards
Andrzej

>
>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5950
>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
>> Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display.c |  2 +-
>>   drivers/gpu/drm/i915/display/intel_hotplug.c | 11 ++++++++++-
>>   drivers/gpu/drm/i915/display/intel_hotplug.h |  2 +-
>>   drivers/gpu/drm/i915/i915_driver.c           |  4 ++--
>>   drivers/gpu/drm/i915/i915_drv.h              |  2 ++
>>   5 files changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index f1c765ac7ab8aa..cd6139bb36151b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -9022,7 +9022,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
>>   	intel_dp_mst_suspend(i915);
>>   
>>   	/* MST is the last user of HPD work */
>> -	intel_hpd_cancel_work(i915);
>> +	intel_hpd_suspend(i915);
>>   
>>   	/* poll work can call into fbdev, hence clean that up afterwards */
>>   	intel_fbdev_fini(i915);
>> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c
>> index 5f8b4f481cff9a..e1d384cb99df6b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
>> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
>> @@ -303,6 +303,8 @@ static void i915_digport_work_func(struct work_struct *work)
>>   	u32 old_bits = 0;
>>   
>>   	spin_lock_irq(&dev_priv->irq_lock);
>> +	if (dev_priv->hotplug.suspended)
>> +		return spin_unlock_irq(&dev_priv->irq_lock);
>>   	long_port_mask = dev_priv->hotplug.long_port_mask;
>>   	dev_priv->hotplug.long_port_mask = 0;
>>   	short_port_mask = dev_priv->hotplug.short_port_mask;
>> @@ -353,6 +355,8 @@ void intel_hpd_trigger_irq(struct intel_digital_port *dig_port)
>>   	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
>>   
>>   	spin_lock_irq(&i915->irq_lock);
>> +	if (i915->hotplug.suspended)
>> +		return spin_unlock_irq(&i915->irq_lock);
>>   	i915->hotplug.short_port_mask |= BIT(dig_port->base.port);
>>   	spin_unlock_irq(&i915->irq_lock);
>>   
>> @@ -475,6 +479,9 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
>>   
>>   	spin_lock(&dev_priv->irq_lock);
>>   
>> +	if (dev_priv->hotplug.suspended)
>> +		return spin_unlock(&dev_priv->irq_lock);
>> +
>>   	/*
>>   	 * Determine whether ->hpd_pulse() exists for each pin, and
>>   	 * whether we have a short or a long pulse. This is needed
>> @@ -603,6 +610,7 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
>>   	 * just to make the assert_spin_locked checks happy.
>>   	 */
>>   	spin_lock_irq(&dev_priv->irq_lock);
>> +	dev_priv->hotplug.suspended = false;
>>   	intel_hpd_irq_setup(dev_priv);
>>   	spin_unlock_irq(&dev_priv->irq_lock);
>>   }
>> @@ -721,13 +729,14 @@ void intel_hpd_init_work(struct drm_i915_private *dev_priv)
>>   			  intel_hpd_irq_storm_reenable_work);
>>   }
>>   
>> -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
>> +void intel_hpd_suspend(struct drm_i915_private *dev_priv)
>>   {
>>   	if (!HAS_DISPLAY(dev_priv))
>>   		return;
>>   
>>   	spin_lock_irq(&dev_priv->irq_lock);
>>   
>> +	dev_priv->hotplug.suspended = true;
>>   	dev_priv->hotplug.long_port_mask = 0;
>>   	dev_priv->hotplug.short_port_mask = 0;
>>   	dev_priv->hotplug.event_bits = 0;
>> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.h b/drivers/gpu/drm/i915/display/intel_hotplug.h
>> index b87e95d606e668..54bddc4dd63421 100644
>> --- a/drivers/gpu/drm/i915/display/intel_hotplug.h
>> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.h
>> @@ -23,7 +23,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
>>   void intel_hpd_trigger_irq(struct intel_digital_port *dig_port);
>>   void intel_hpd_init(struct drm_i915_private *dev_priv);
>>   void intel_hpd_init_work(struct drm_i915_private *dev_priv);
>> -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
>> +void intel_hpd_suspend(struct drm_i915_private *dev_priv);
>>   enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
>>   				   enum port port);
>>   bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin);
>> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
>> index deb8a8b76965a1..57a063a306e3a4 100644
>> --- a/drivers/gpu/drm/i915/i915_driver.c
>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>> @@ -1092,7 +1092,7 @@ void i915_driver_shutdown(struct drm_i915_private *i915)
>>   	intel_dp_mst_suspend(i915);
>>   
>>   	intel_runtime_pm_disable_interrupts(i915);
>> -	intel_hpd_cancel_work(i915);
>> +	intel_hpd_suspend(i915);
>>   
>>   	intel_suspend_encoders(i915);
>>   	intel_shutdown_encoders(i915);
>> @@ -1161,7 +1161,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>>   	intel_dp_mst_suspend(dev_priv);
>>   
>>   	intel_runtime_pm_disable_interrupts(dev_priv);
>> -	intel_hpd_cancel_work(dev_priv);
>> +	intel_hpd_suspend(dev_priv);
>>   
>>   	intel_suspend_encoders(dev_priv);
>>   
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index d25647be25d18b..dc1562b95d7ade 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -106,6 +106,8 @@ struct vlv_s0ix_state;
>>   #define HPD_STORM_DEFAULT_THRESHOLD 50
>>   
>>   struct i915_hotplug {
>> +	bool suspended;
>> +
>>   	struct delayed_work hotplug_work;
>>   
>>   	const u32 *hpd, *pch_hpd;
>> -- 
>> 2.25.1
>>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Intel-gfx] [PATCH v6 3/4] drm/i915/display: add hotplug.suspended flag
  2022-08-23 21:48     ` Andrzej Hajda
@ 2022-08-24 11:22       ` Imre Deak
  2022-08-25 11:24         ` Andrzej Hajda
  0 siblings, 1 reply; 18+ messages in thread
From: Imre Deak @ 2022-08-24 11:22 UTC (permalink / raw)
  To: Andrzej Hajda; +Cc: intel-gfx, dri-devel, Rodrigo Vivi, Arun R Murthy

On Tue, Aug 23, 2022 at 11:48:01PM +0200, Andrzej Hajda wrote:
> 
> 
> On 22.08.2022 19:27, Imre Deak wrote:
> > On Fri, Jul 22, 2022 at 02:51:42PM +0200, Andrzej Hajda wrote:
> > > HPD events during driver removal can be generated by hardware and
> > > software frameworks - drm_dp_mst, the former we can avoid by disabling
> > > interrupts, the latter can be triggered by any drm_dp_mst transaction,
> > > and this is too late. Introducing suspended flag allows to solve this
> > > chicken-egg problem.
> > intel_hpd_cancel_work() is always called after suspending MST and
> > disabling IRQs (with the order I suggested in patch 1). If both of these
> > have disabled the corresponding functionality (MST, IRQs) properly with
> > all their MST/IRQ scheduled works guaranteed to not get rescheduled,
> > then it's not clear how could either intel_hpd_trigger_irq() or an IRQ
> > work run. So the problematic sequence would need a better explanation.
> 
> I am not familiar with MST but as I understand from earlier discussion MST
> framework can be called during driver removal code even after
> intel_dp_mst_suspend.

Not sure how that happens, but it looks wrong. One way I can imagine is
that connector detection re-enables MST, which should be prevented then.

> And since MST transfer can timeout it can trigger
> drm_dp_mst_wait_tx_reply --> mgr->cbs->poll_hpd_irq(mgr) -->
> intel_dp_mst_poll_hpd_irq --> intel_hpd_trigger_irq.
> 
> So actually this patch supersedes the 1st patch.
> 
> > 
> > There's also already
> > dev_priv->runtime_pm.irqs_enabled
> > showing if hotplug interrupts are disabled (along with all other IRQs).
> 
> So it is slightly different, this patch introduces flag indicating if HPD is
> enabled, we can have IRQs not related to HPD, and HPD events not related to
> IRQs.

In its current form I can't see a difference. What would make sense is
to add a flag that prevents connector detection (which would
incorrectly enable MST for instace), but leave the handling of other
interrupts enabled. That flag would be set already before suspending
MST.

> Regards
> Andrzej
> 
> > 
> > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5950
> > > Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> > > Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/display/intel_display.c |  2 +-
> > >   drivers/gpu/drm/i915/display/intel_hotplug.c | 11 ++++++++++-
> > >   drivers/gpu/drm/i915/display/intel_hotplug.h |  2 +-
> > >   drivers/gpu/drm/i915/i915_driver.c           |  4 ++--
> > >   drivers/gpu/drm/i915/i915_drv.h              |  2 ++
> > >   5 files changed, 16 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index f1c765ac7ab8aa..cd6139bb36151b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -9022,7 +9022,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
> > >   	intel_dp_mst_suspend(i915);
> > >   	/* MST is the last user of HPD work */
> > > -	intel_hpd_cancel_work(i915);
> > > +	intel_hpd_suspend(i915);
> > >   	/* poll work can call into fbdev, hence clean that up afterwards */
> > >   	intel_fbdev_fini(i915);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > > index 5f8b4f481cff9a..e1d384cb99df6b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > > @@ -303,6 +303,8 @@ static void i915_digport_work_func(struct work_struct *work)
> > >   	u32 old_bits = 0;
> > >   	spin_lock_irq(&dev_priv->irq_lock);
> > > +	if (dev_priv->hotplug.suspended)
> > > +		return spin_unlock_irq(&dev_priv->irq_lock);
> > >   	long_port_mask = dev_priv->hotplug.long_port_mask;
> > >   	dev_priv->hotplug.long_port_mask = 0;
> > >   	short_port_mask = dev_priv->hotplug.short_port_mask;
> > > @@ -353,6 +355,8 @@ void intel_hpd_trigger_irq(struct intel_digital_port *dig_port)
> > >   	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > >   	spin_lock_irq(&i915->irq_lock);
> > > +	if (i915->hotplug.suspended)
> > > +		return spin_unlock_irq(&i915->irq_lock);
> > >   	i915->hotplug.short_port_mask |= BIT(dig_port->base.port);
> > >   	spin_unlock_irq(&i915->irq_lock);
> > > @@ -475,6 +479,9 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> > >   	spin_lock(&dev_priv->irq_lock);
> > > +	if (dev_priv->hotplug.suspended)
> > > +		return spin_unlock(&dev_priv->irq_lock);
> > > +
> > >   	/*
> > >   	 * Determine whether ->hpd_pulse() exists for each pin, and
> > >   	 * whether we have a short or a long pulse. This is needed
> > > @@ -603,6 +610,7 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
> > >   	 * just to make the assert_spin_locked checks happy.
> > >   	 */
> > >   	spin_lock_irq(&dev_priv->irq_lock);
> > > +	dev_priv->hotplug.suspended = false;
> > >   	intel_hpd_irq_setup(dev_priv);
> > >   	spin_unlock_irq(&dev_priv->irq_lock);
> > >   }
> > > @@ -721,13 +729,14 @@ void intel_hpd_init_work(struct drm_i915_private *dev_priv)
> > >   			  intel_hpd_irq_storm_reenable_work);
> > >   }
> > > -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
> > > +void intel_hpd_suspend(struct drm_i915_private *dev_priv)
> > >   {
> > >   	if (!HAS_DISPLAY(dev_priv))
> > >   		return;
> > >   	spin_lock_irq(&dev_priv->irq_lock);
> > > +	dev_priv->hotplug.suspended = true;
> > >   	dev_priv->hotplug.long_port_mask = 0;
> > >   	dev_priv->hotplug.short_port_mask = 0;
> > >   	dev_priv->hotplug.event_bits = 0;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.h b/drivers/gpu/drm/i915/display/intel_hotplug.h
> > > index b87e95d606e668..54bddc4dd63421 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_hotplug.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.h
> > > @@ -23,7 +23,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> > >   void intel_hpd_trigger_irq(struct intel_digital_port *dig_port);
> > >   void intel_hpd_init(struct drm_i915_private *dev_priv);
> > >   void intel_hpd_init_work(struct drm_i915_private *dev_priv);
> > > -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
> > > +void intel_hpd_suspend(struct drm_i915_private *dev_priv);
> > >   enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
> > >   				   enum port port);
> > >   bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin);
> > > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > > index deb8a8b76965a1..57a063a306e3a4 100644
> > > --- a/drivers/gpu/drm/i915/i915_driver.c
> > > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > > @@ -1092,7 +1092,7 @@ void i915_driver_shutdown(struct drm_i915_private *i915)
> > >   	intel_dp_mst_suspend(i915);
> > >   	intel_runtime_pm_disable_interrupts(i915);
> > > -	intel_hpd_cancel_work(i915);
> > > +	intel_hpd_suspend(i915);
> > >   	intel_suspend_encoders(i915);
> > >   	intel_shutdown_encoders(i915);
> > > @@ -1161,7 +1161,7 @@ static int i915_drm_suspend(struct drm_device *dev)
> > >   	intel_dp_mst_suspend(dev_priv);
> > >   	intel_runtime_pm_disable_interrupts(dev_priv);
> > > -	intel_hpd_cancel_work(dev_priv);
> > > +	intel_hpd_suspend(dev_priv);
> > >   	intel_suspend_encoders(dev_priv);
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index d25647be25d18b..dc1562b95d7ade 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -106,6 +106,8 @@ struct vlv_s0ix_state;
> > >   #define HPD_STORM_DEFAULT_THRESHOLD 50
> > >   struct i915_hotplug {
> > > +	bool suspended;
> > > +
> > >   	struct delayed_work hotplug_work;
> > >   	const u32 *hpd, *pch_hpd;
> > > -- 
> > > 2.25.1
> > > 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Intel-gfx] [PATCH v6 3/4] drm/i915/display: add hotplug.suspended flag
  2022-08-24 11:22       ` Imre Deak
@ 2022-08-25 11:24         ` Andrzej Hajda
  2022-08-25 14:57           ` Imre Deak
  0 siblings, 1 reply; 18+ messages in thread
From: Andrzej Hajda @ 2022-08-25 11:24 UTC (permalink / raw)
  To: imre.deak; +Cc: intel-gfx, dri-devel, Rodrigo Vivi

On 24.08.2022 13:22, Imre Deak wrote:
> On Tue, Aug 23, 2022 at 11:48:01PM +0200, Andrzej Hajda wrote:
>>
>>
>> On 22.08.2022 19:27, Imre Deak wrote:
>>> On Fri, Jul 22, 2022 at 02:51:42PM +0200, Andrzej Hajda wrote:
>>>> HPD events during driver removal can be generated by hardware and
>>>> software frameworks - drm_dp_mst, the former we can avoid by disabling
>>>> interrupts, the latter can be triggered by any drm_dp_mst transaction,
>>>> and this is too late. Introducing suspended flag allows to solve this
>>>> chicken-egg problem.
>>> intel_hpd_cancel_work() is always called after suspending MST and
>>> disabling IRQs (with the order I suggested in patch 1). If both of these
>>> have disabled the corresponding functionality (MST, IRQs) properly with
>>> all their MST/IRQ scheduled works guaranteed to not get rescheduled,
>>> then it's not clear how could either intel_hpd_trigger_irq() or an IRQ
>>> work run. So the problematic sequence would need a better explanation.
>>
>> I am not familiar with MST but as I understand from earlier discussion MST
>> framework can be called during driver removal code even after
>> intel_dp_mst_suspend.
> 
> Not sure how that happens, but it looks wrong. One way I can imagine is
> that connector detection re-enables MST, which should be prevented then.

I am not MST expert and atm I have no access to the machine on which I 
could look for real prove.
As I understand intel_dp_mst_suspend prevents only downstream devices to 
initiate transactions, it does not prevent transactions initiated from 
i915 driver.
My guesses for such transactions are:
- any ioctl/sysfs/drm_property access initiated by user which can 
involve MST tranaction (set brightness, read EDID, reading capabilities, 
statuses, ....), unless they are already blocked, how?
- maybe some mode_config disabling code (for example 
intel_mst_disable_dp) - intel_mode_config_cleanup is called after 
intel_dp_mst_suspend.

And since MST transfer can timeout it can trigger
drm_dp_mst_wait_tx_reply --> mgr->cbs->poll_hpd_irq(mgr) -->
intel_dp_mst_poll_hpd_irq --> intel_hpd_trigger_irq.

If such situation happens after intel_dp_mst_suspend 
i915->hotplug.dig_port_work will be queued, and we have risk of 
use-after-free.

If this analysis looks incorrect I can send patches 1, 2, 4 with your 
comments addressed. CI probably will verify this anyway.

Regards
Andrzej


> 
>> And since MST transfer can timeout it can trigger
>> drm_dp_mst_wait_tx_reply --> mgr->cbs->poll_hpd_irq(mgr) -->
>> intel_dp_mst_poll_hpd_irq --> intel_hpd_trigger_irq.
>>
>> So actually this patch supersedes the 1st patch.
>>
>>>
>>> There's also already
>>> dev_priv->runtime_pm.irqs_enabled
>>> showing if hotplug interrupts are disabled (along with all other IRQs).
>>
>> So it is slightly different, this patch introduces flag indicating if HPD is
>> enabled, we can have IRQs not related to HPD, and HPD events not related to
>> IRQs.
> 
> In its current form I can't see a difference. What would make sense is
> to add a flag that prevents connector detection (which would
> incorrectly enable MST for instace), but leave the handling of other
> interrupts enabled. That flag would be set already before suspending
> MST.
> 
>> Regards
>> Andrzej
>>
>>>
>>>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5950
>>>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
>>>> Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/display/intel_display.c |  2 +-
>>>>    drivers/gpu/drm/i915/display/intel_hotplug.c | 11 ++++++++++-
>>>>    drivers/gpu/drm/i915/display/intel_hotplug.h |  2 +-
>>>>    drivers/gpu/drm/i915/i915_driver.c           |  4 ++--
>>>>    drivers/gpu/drm/i915/i915_drv.h              |  2 ++
>>>>    5 files changed, 16 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>>> index f1c765ac7ab8aa..cd6139bb36151b 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>>> @@ -9022,7 +9022,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
>>>>    	intel_dp_mst_suspend(i915);
>>>>    	/* MST is the last user of HPD work */
>>>> -	intel_hpd_cancel_work(i915);
>>>> +	intel_hpd_suspend(i915);
>>>>    	/* poll work can call into fbdev, hence clean that up afterwards */
>>>>    	intel_fbdev_fini(i915);
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c
>>>> index 5f8b4f481cff9a..e1d384cb99df6b 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
>>>> @@ -303,6 +303,8 @@ static void i915_digport_work_func(struct work_struct *work)
>>>>    	u32 old_bits = 0;
>>>>    	spin_lock_irq(&dev_priv->irq_lock);
>>>> +	if (dev_priv->hotplug.suspended)
>>>> +		return spin_unlock_irq(&dev_priv->irq_lock);
>>>>    	long_port_mask = dev_priv->hotplug.long_port_mask;
>>>>    	dev_priv->hotplug.long_port_mask = 0;
>>>>    	short_port_mask = dev_priv->hotplug.short_port_mask;
>>>> @@ -353,6 +355,8 @@ void intel_hpd_trigger_irq(struct intel_digital_port *dig_port)
>>>>    	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
>>>>    	spin_lock_irq(&i915->irq_lock);
>>>> +	if (i915->hotplug.suspended)
>>>> +		return spin_unlock_irq(&i915->irq_lock);
>>>>    	i915->hotplug.short_port_mask |= BIT(dig_port->base.port);
>>>>    	spin_unlock_irq(&i915->irq_lock);
>>>> @@ -475,6 +479,9 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
>>>>    	spin_lock(&dev_priv->irq_lock);
>>>> +	if (dev_priv->hotplug.suspended)
>>>> +		return spin_unlock(&dev_priv->irq_lock);
>>>> +
>>>>    	/*
>>>>    	 * Determine whether ->hpd_pulse() exists for each pin, and
>>>>    	 * whether we have a short or a long pulse. This is needed
>>>> @@ -603,6 +610,7 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
>>>>    	 * just to make the assert_spin_locked checks happy.
>>>>    	 */
>>>>    	spin_lock_irq(&dev_priv->irq_lock);
>>>> +	dev_priv->hotplug.suspended = false;
>>>>    	intel_hpd_irq_setup(dev_priv);
>>>>    	spin_unlock_irq(&dev_priv->irq_lock);
>>>>    }
>>>> @@ -721,13 +729,14 @@ void intel_hpd_init_work(struct drm_i915_private *dev_priv)
>>>>    			  intel_hpd_irq_storm_reenable_work);
>>>>    }
>>>> -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
>>>> +void intel_hpd_suspend(struct drm_i915_private *dev_priv)
>>>>    {
>>>>    	if (!HAS_DISPLAY(dev_priv))
>>>>    		return;
>>>>    	spin_lock_irq(&dev_priv->irq_lock);
>>>> +	dev_priv->hotplug.suspended = true;
>>>>    	dev_priv->hotplug.long_port_mask = 0;
>>>>    	dev_priv->hotplug.short_port_mask = 0;
>>>>    	dev_priv->hotplug.event_bits = 0;
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.h b/drivers/gpu/drm/i915/display/intel_hotplug.h
>>>> index b87e95d606e668..54bddc4dd63421 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_hotplug.h
>>>> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.h
>>>> @@ -23,7 +23,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
>>>>    void intel_hpd_trigger_irq(struct intel_digital_port *dig_port);
>>>>    void intel_hpd_init(struct drm_i915_private *dev_priv);
>>>>    void intel_hpd_init_work(struct drm_i915_private *dev_priv);
>>>> -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
>>>> +void intel_hpd_suspend(struct drm_i915_private *dev_priv);
>>>>    enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
>>>>    				   enum port port);
>>>>    bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin);
>>>> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
>>>> index deb8a8b76965a1..57a063a306e3a4 100644
>>>> --- a/drivers/gpu/drm/i915/i915_driver.c
>>>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>>>> @@ -1092,7 +1092,7 @@ void i915_driver_shutdown(struct drm_i915_private *i915)
>>>>    	intel_dp_mst_suspend(i915);
>>>>    	intel_runtime_pm_disable_interrupts(i915);
>>>> -	intel_hpd_cancel_work(i915);
>>>> +	intel_hpd_suspend(i915);
>>>>    	intel_suspend_encoders(i915);
>>>>    	intel_shutdown_encoders(i915);
>>>> @@ -1161,7 +1161,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>>>>    	intel_dp_mst_suspend(dev_priv);
>>>>    	intel_runtime_pm_disable_interrupts(dev_priv);
>>>> -	intel_hpd_cancel_work(dev_priv);
>>>> +	intel_hpd_suspend(dev_priv);
>>>>    	intel_suspend_encoders(dev_priv);
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index d25647be25d18b..dc1562b95d7ade 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -106,6 +106,8 @@ struct vlv_s0ix_state;
>>>>    #define HPD_STORM_DEFAULT_THRESHOLD 50
>>>>    struct i915_hotplug {
>>>> +	bool suspended;
>>>> +
>>>>    	struct delayed_work hotplug_work;
>>>>    	const u32 *hpd, *pch_hpd;
>>>> -- 
>>>> 2.25.1
>>>>
>>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Intel-gfx] [PATCH v6 3/4] drm/i915/display: add hotplug.suspended flag
  2022-08-25 11:24         ` Andrzej Hajda
@ 2022-08-25 14:57           ` Imre Deak
  0 siblings, 0 replies; 18+ messages in thread
From: Imre Deak @ 2022-08-25 14:57 UTC (permalink / raw)
  To: Andrzej Hajda; +Cc: intel-gfx, dri-devel, Rodrigo Vivi

On Thu, Aug 25, 2022 at 01:24:04PM +0200, Andrzej Hajda wrote:
> On 24.08.2022 13:22, Imre Deak wrote:
> > On Tue, Aug 23, 2022 at 11:48:01PM +0200, Andrzej Hajda wrote:
> > > 
> > > 
> > > On 22.08.2022 19:27, Imre Deak wrote:
> > > > On Fri, Jul 22, 2022 at 02:51:42PM +0200, Andrzej Hajda wrote:
> > > > > HPD events during driver removal can be generated by hardware and
> > > > > software frameworks - drm_dp_mst, the former we can avoid by disabling
> > > > > interrupts, the latter can be triggered by any drm_dp_mst transaction,
> > > > > and this is too late. Introducing suspended flag allows to solve this
> > > > > chicken-egg problem.
> > > > intel_hpd_cancel_work() is always called after suspending MST and
> > > > disabling IRQs (with the order I suggested in patch 1). If both of these
> > > > have disabled the corresponding functionality (MST, IRQs) properly with
> > > > all their MST/IRQ scheduled works guaranteed to not get rescheduled,
> > > > then it's not clear how could either intel_hpd_trigger_irq() or an IRQ
> > > > work run. So the problematic sequence would need a better explanation.
> > > 
> > > I am not familiar with MST but as I understand from earlier discussion MST
> > > framework can be called during driver removal code even after
> > > intel_dp_mst_suspend.
> > 
> > Not sure how that happens, but it looks wrong. One way I can imagine is
> > that connector detection re-enables MST, which should be prevented then.
> 
> I am not MST expert and atm I have no access to the machine on which I could
> look for real prove.
> As I understand intel_dp_mst_suspend prevents only downstream devices to
> initiate transactions, it does not prevent transactions initiated from i915
> driver.
> My guesses for such transactions are:
> - any ioctl/sysfs/drm_property access initiated by user which can involve
> MST tranaction (set brightness, read EDID, reading capabilities, statuses,
> ....), unless they are already blocked, how?

In theory all these should be blocked already, after
i915_driver_unregister() has returned; for the above cases in particular
via drm_connector_unregister_all().

> - maybe some mode_config disabling code (for example intel_mst_disable_dp) -

This should be called already via i915_driver_unregister() ->
intel_display_driver_unregister() -> drm_atomic_helper_shutdown().

> intel_mode_config_cleanup is called after intel_dp_mst_suspend.

This should only free the allocated objects etc, but not do any
transactions.

> And since MST transfer can timeout it can trigger
> drm_dp_mst_wait_tx_reply --> mgr->cbs->poll_hpd_irq(mgr) -->
> intel_dp_mst_poll_hpd_irq --> intel_hpd_trigger_irq.
> 
> If such situation happens after intel_dp_mst_suspend
> i915->hotplug.dig_port_work will be queued, and we have risk of
> use-after-free.
> 
> If this analysis looks incorrect I can send patches 1, 2, 4 with your
> comments addressed. CI probably will verify this anyway.

Ok. I suppose blocking detection based on a new flag would mean avoiding
scheduling and flushing hotplug.hotplug_work already as the first step
in intel_display_driver_unregister() and somewhere early during system
suspend.

> Regards
> Andrzej
> 
> 
> > 
> > > And since MST transfer can timeout it can trigger
> > > drm_dp_mst_wait_tx_reply --> mgr->cbs->poll_hpd_irq(mgr) -->
> > > intel_dp_mst_poll_hpd_irq --> intel_hpd_trigger_irq.
> > > 
> > > So actually this patch supersedes the 1st patch.
> > > 
> > > > 
> > > > There's also already
> > > > dev_priv->runtime_pm.irqs_enabled
> > > > showing if hotplug interrupts are disabled (along with all other IRQs).
> > > 
> > > So it is slightly different, this patch introduces flag indicating if HPD is
> > > enabled, we can have IRQs not related to HPD, and HPD events not related to
> > > IRQs.
> > 
> > In its current form I can't see a difference. What would make sense is
> > to add a flag that prevents connector detection (which would
> > incorrectly enable MST for instace), but leave the handling of other
> > interrupts enabled. That flag would be set already before suspending
> > MST.
> > 
> > > Regards
> > > Andrzej
> > > 
> > > > 
> > > > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5950
> > > > > Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> > > > > Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > > > ---
> > > > >    drivers/gpu/drm/i915/display/intel_display.c |  2 +-
> > > > >    drivers/gpu/drm/i915/display/intel_hotplug.c | 11 ++++++++++-
> > > > >    drivers/gpu/drm/i915/display/intel_hotplug.h |  2 +-
> > > > >    drivers/gpu/drm/i915/i915_driver.c           |  4 ++--
> > > > >    drivers/gpu/drm/i915/i915_drv.h              |  2 ++
> > > > >    5 files changed, 16 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index f1c765ac7ab8aa..cd6139bb36151b 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -9022,7 +9022,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
> > > > >    	intel_dp_mst_suspend(i915);
> > > > >    	/* MST is the last user of HPD work */
> > > > > -	intel_hpd_cancel_work(i915);
> > > > > +	intel_hpd_suspend(i915);
> > > > >    	/* poll work can call into fbdev, hence clean that up afterwards */
> > > > >    	intel_fbdev_fini(i915);
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > > > > index 5f8b4f481cff9a..e1d384cb99df6b 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > > > > @@ -303,6 +303,8 @@ static void i915_digport_work_func(struct work_struct *work)
> > > > >    	u32 old_bits = 0;
> > > > >    	spin_lock_irq(&dev_priv->irq_lock);
> > > > > +	if (dev_priv->hotplug.suspended)
> > > > > +		return spin_unlock_irq(&dev_priv->irq_lock);
> > > > >    	long_port_mask = dev_priv->hotplug.long_port_mask;
> > > > >    	dev_priv->hotplug.long_port_mask = 0;
> > > > >    	short_port_mask = dev_priv->hotplug.short_port_mask;
> > > > > @@ -353,6 +355,8 @@ void intel_hpd_trigger_irq(struct intel_digital_port *dig_port)
> > > > >    	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > >    	spin_lock_irq(&i915->irq_lock);
> > > > > +	if (i915->hotplug.suspended)
> > > > > +		return spin_unlock_irq(&i915->irq_lock);
> > > > >    	i915->hotplug.short_port_mask |= BIT(dig_port->base.port);
> > > > >    	spin_unlock_irq(&i915->irq_lock);
> > > > > @@ -475,6 +479,9 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> > > > >    	spin_lock(&dev_priv->irq_lock);
> > > > > +	if (dev_priv->hotplug.suspended)
> > > > > +		return spin_unlock(&dev_priv->irq_lock);
> > > > > +
> > > > >    	/*
> > > > >    	 * Determine whether ->hpd_pulse() exists for each pin, and
> > > > >    	 * whether we have a short or a long pulse. This is needed
> > > > > @@ -603,6 +610,7 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
> > > > >    	 * just to make the assert_spin_locked checks happy.
> > > > >    	 */
> > > > >    	spin_lock_irq(&dev_priv->irq_lock);
> > > > > +	dev_priv->hotplug.suspended = false;
> > > > >    	intel_hpd_irq_setup(dev_priv);
> > > > >    	spin_unlock_irq(&dev_priv->irq_lock);
> > > > >    }
> > > > > @@ -721,13 +729,14 @@ void intel_hpd_init_work(struct drm_i915_private *dev_priv)
> > > > >    			  intel_hpd_irq_storm_reenable_work);
> > > > >    }
> > > > > -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
> > > > > +void intel_hpd_suspend(struct drm_i915_private *dev_priv)
> > > > >    {
> > > > >    	if (!HAS_DISPLAY(dev_priv))
> > > > >    		return;
> > > > >    	spin_lock_irq(&dev_priv->irq_lock);
> > > > > +	dev_priv->hotplug.suspended = true;
> > > > >    	dev_priv->hotplug.long_port_mask = 0;
> > > > >    	dev_priv->hotplug.short_port_mask = 0;
> > > > >    	dev_priv->hotplug.event_bits = 0;
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.h b/drivers/gpu/drm/i915/display/intel_hotplug.h
> > > > > index b87e95d606e668..54bddc4dd63421 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_hotplug.h
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.h
> > > > > @@ -23,7 +23,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> > > > >    void intel_hpd_trigger_irq(struct intel_digital_port *dig_port);
> > > > >    void intel_hpd_init(struct drm_i915_private *dev_priv);
> > > > >    void intel_hpd_init_work(struct drm_i915_private *dev_priv);
> > > > > -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
> > > > > +void intel_hpd_suspend(struct drm_i915_private *dev_priv);
> > > > >    enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
> > > > >    				   enum port port);
> > > > >    bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin);
> > > > > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > > > > index deb8a8b76965a1..57a063a306e3a4 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_driver.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > > > > @@ -1092,7 +1092,7 @@ void i915_driver_shutdown(struct drm_i915_private *i915)
> > > > >    	intel_dp_mst_suspend(i915);
> > > > >    	intel_runtime_pm_disable_interrupts(i915);
> > > > > -	intel_hpd_cancel_work(i915);
> > > > > +	intel_hpd_suspend(i915);
> > > > >    	intel_suspend_encoders(i915);
> > > > >    	intel_shutdown_encoders(i915);
> > > > > @@ -1161,7 +1161,7 @@ static int i915_drm_suspend(struct drm_device *dev)
> > > > >    	intel_dp_mst_suspend(dev_priv);
> > > > >    	intel_runtime_pm_disable_interrupts(dev_priv);
> > > > > -	intel_hpd_cancel_work(dev_priv);
> > > > > +	intel_hpd_suspend(dev_priv);
> > > > >    	intel_suspend_encoders(dev_priv);
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index d25647be25d18b..dc1562b95d7ade 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -106,6 +106,8 @@ struct vlv_s0ix_state;
> > > > >    #define HPD_STORM_DEFAULT_THRESHOLD 50
> > > > >    struct i915_hotplug {
> > > > > +	bool suspended;
> > > > > +
> > > > >    	struct delayed_work hotplug_work;
> > > > >    	const u32 *hpd, *pch_hpd;
> > > > > -- 
> > > > > 2.25.1
> > > > > 
> > > 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2022-08-25 14:57 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-22 12:51 [PATCH v6 0/4] drm/i915/display: stop HPD workers before display driver unregister Andrzej Hajda
2022-07-22 12:51 ` [PATCH v6 1/4] drm/i915/hpd: postpone HPD cancel work after last user suspension Andrzej Hajda
2022-08-22 17:08   ` Imre Deak
2022-08-23  7:41     ` Jani Nikula
2022-08-23  9:10       ` Imre Deak
2022-07-22 12:51 ` [PATCH v6 2/4] drm/i915/fbdev: suspend HPD before fbdev unregistration Andrzej Hajda
2022-08-22 17:10   ` [Intel-gfx] " Imre Deak
2022-07-22 12:51 ` [PATCH v6 3/4] drm/i915/display: add hotplug.suspended flag Andrzej Hajda
2022-08-22 17:27   ` [Intel-gfx] " Imre Deak
2022-08-23 21:48     ` Andrzej Hajda
2022-08-24 11:22       ` Imre Deak
2022-08-25 11:24         ` Andrzej Hajda
2022-08-25 14:57           ` Imre Deak
2022-07-22 12:51 ` [PATCH v6 4/4] drm/i915/fbdev: do not create fbdev if HPD is suspended Andrzej Hajda
2022-07-26  6:50   ` Murthy, Arun R
2022-08-22 17:28   ` Imre Deak
2022-08-02 12:24 ` [Intel-gfx] [PATCH v6 0/4] drm/i915/display: stop HPD workers before display driver unregister Gwan-gyeong Mun
2022-08-11  8:33   ` Andrzej Hajda

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).