[PATCH v3 0/3] drm/i915: MST and wakeref leak fixes

[PATCH v3 0/3] drm/i915: MST and wakeref leak fixes

[Lyude Paul]

While trying to fix a problem with suspend/resume that I introduced in
the atomic VCPI helpers for MST, I also ran into some issues with i915
varying from "not that bad" to "oh wow that's very bad". Here are the
fixes for those issues.

This series was originally just one patch,
"drm/i915: Don't send MST hotplugs during resume"

Lyude Paul (3):
  drm/i915: Block fbdev HPD processing during suspend
  drm/i915: Don't send MST hotplugs during resume
  drm/i915: Don't send hotplug in intel_dp_check_mst_status()

 drivers/gpu/drm/i915/intel_dp.c    | 13 ++++++------
 drivers/gpu/drm/i915/intel_drv.h   | 10 +++++++++
 drivers/gpu/drm/i915/intel_fbdev.c | 33 +++++++++++++++++++++++++++++-
 3 files changed, 49 insertions(+), 7 deletions(-)

-- 
2.20.1

[PATCH v3 1/3] drm/i915: Block fbdev HPD processing during suspend

[Lyude Paul]

When resuming, we check whether or not any previously connected
MST topologies are still present and if so, attempt to resume them. If
this fails, we disable said MST topologies and fire off a hotplug event
so that userspace knows to reprobe.

However, sending a hotplug event involves calling
drm_fb_helper_hotplug_event(), which in turn results in fbcon doing a
connector reprobe in the caller's thread - something we can't do at the
point in which i915 calls drm_dp_mst_topology_mgr_resume() since
hotplugging hasn't been fully initialized yet.

This currently causes some rather subtle but fatal issues. For example,
on my T480s the laptop dock connected to it usually disappears during a
suspend cycle, and comes back up a short while after the system has been
resumed. This guarantees pretty much every suspend and resume cycle,
drm_dp_mst_topology_mgr_set_mst(mgr, false); will be caused and in turn,
a connector hotplug will occur. Now it's Rute Goldberg time: when the
connector hotplug occurs, i915 reprobes /all/ of the connectors,
including eDP. However, eDP probing requires that we power on the panel
VDD which in turn, grabs a wakeref to the appropriate power domain on
the GPU (on my T480s, this is the PORT_DDI_A_IO domain). This is where
things start breaking, since this all happens before
intel_power_domains_enable() is called we end up leaking the wakeref
that was acquired and never releasing it later. Come next suspend/resume
cycle, this causes us to fail to shut down the GPU properly, which
causes it not to resume properly and die a horrible complicated death.

(as a note: this only happens when there's both an eDP panel and MST
topology connected which is removed mid-suspend. One or the other seems
to always be OK).

We could try to fix the VDD wakeref leak, but this doesn't seem like
it's worth it at all since we aren't able to handle hotplug detection
while resuming anyway. So, let's go with a more robust solution inspired
by nouveau: block fbdev from handling hotplug events until we resume
fbdev. This allows us to still send sysfs hotplug events to be handled
later by user space while we're resuming, while also preventing us from
actually processing any hotplug events we receive until it's safe.

This fixes the wakeref leak observed on the T480s and as such, also
fixes suspend/resume with MST topologies connected on this machine.

Changes since v2:
* Don't call drm_fb_helper_hotplug_event() under lock, do it after lock
  (Chris Wilson)
* Don't call drm_fb_helper_hotplug_event() in
  intel_fbdev_output_poll_changed() under lock (Chris Wilson)
* Always set ifbdev->hpd_waiting (Chris Wilson)

Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: 0e32b39ceed6 ("drm/i915: add DP 1.2 MST support (v0.7)")
Cc: Todd Previte <tprevite@gmail.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v3.17+
---
 drivers/gpu/drm/i915/intel_drv.h   | 10 +++++++++
 drivers/gpu/drm/i915/intel_fbdev.c | 33 +++++++++++++++++++++++++++++-
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 90ba5436370e..9ec3d00fbd19 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -213,6 +213,16 @@ struct intel_fbdev {
 	unsigned long vma_flags;
 	async_cookie_t cookie;
 	int preferred_bpp;
+
+	/* Whether or not fbdev hpd processing is temporarily suspended */
+	bool hpd_suspended : 1;
+	/* Set when a hotplug was received while HPD processing was
+	 * suspended
+	 */
+	bool hpd_waiting : 1;
+
+	/* Protects hpd_suspended */
+	struct mutex hpd_lock;
 };
 
 struct intel_encoder {
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 8cf3efe88f02..376ffe842e26 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -681,6 +681,7 @@ int intel_fbdev_init(struct drm_device *dev)
 	if (ifbdev == NULL)
 		return -ENOMEM;
 
+	mutex_init(&ifbdev->hpd_lock);
 	drm_fb_helper_prepare(dev, &ifbdev->helper, &intel_fb_helper_funcs);
 
 	if (!intel_fbdev_init_bios(dev, ifbdev))
@@ -754,6 +755,26 @@ void intel_fbdev_fini(struct drm_i915_private *dev_priv)
 	intel_fbdev_destroy(ifbdev);
 }
 
+/* Suspends/resumes fbdev processing of incoming HPD events. When resuming HPD
+ * processing, fbdev will perform a full connector reprobe if a hotplug event
+ * was received while HPD was suspended.
+ */
+static void intel_fbdev_hpd_set_suspend(struct intel_fbdev *ifbdev, int state)
+{
+	bool send_hpd = false;
+
+	mutex_lock(&ifbdev->hpd_lock);
+	ifbdev->hpd_suspended = state == FBINFO_STATE_SUSPENDED;
+	send_hpd = !ifbdev->hpd_suspended && ifbdev->hpd_waiting;
+	ifbdev->hpd_waiting = false;
+	mutex_unlock(&ifbdev->hpd_lock);
+
+	if (send_hpd) {
+		DRM_DEBUG_KMS("Handling delayed fbcon HPD event\n");
+		drm_fb_helper_hotplug_event(&ifbdev->helper);
+	}
+}
+
 void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -775,6 +796,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
 		 */
 		if (state != FBINFO_STATE_RUNNING)
 			flush_work(&dev_priv->fbdev_suspend_work);
+
 		console_lock();
 	} else {
 		/*
@@ -802,17 +824,26 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
 
 	drm_fb_helper_set_suspend(&ifbdev->helper, state);
 	console_unlock();
+
+	intel_fbdev_hpd_set_suspend(ifbdev, state);
 }
 
 void intel_fbdev_output_poll_changed(struct drm_device *dev)
 {
 	struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
+	bool send_hpd;
 
 	if (!ifbdev)
 		return;
 
 	intel_fbdev_sync(ifbdev);
-	if (ifbdev->vma || ifbdev->helper.deferred_setup)
+
+	mutex_lock(&ifbdev->hpd_lock);
+	send_hpd = !ifbdev->hpd_suspended;
+	ifbdev->hpd_waiting = true;
+	mutex_unlock(&ifbdev->hpd_lock);
+
+	if (send_hpd && (ifbdev->vma || ifbdev->helper.deferred_setup))
 		drm_fb_helper_hotplug_event(&ifbdev->helper);
 }
 
-- 
2.20.1

[PATCH v3 1/3] drm/i915: Block fbdev HPD processing during suspend

[Lyude Paul]

When resuming, we check whether or not any previously connected
MST topologies are still present and if so, attempt to resume them. If
this fails, we disable said MST topologies and fire off a hotplug event
so that userspace knows to reprobe.

However, sending a hotplug event involves calling
drm_fb_helper_hotplug_event(), which in turn results in fbcon doing a
connector reprobe in the caller's thread - something we can't do at the
point in which i915 calls drm_dp_mst_topology_mgr_resume() since
hotplugging hasn't been fully initialized yet.

This currently causes some rather subtle but fatal issues. For example,
on my T480s the laptop dock connected to it usually disappears during a
suspend cycle, and comes back up a short while after the system has been
resumed. This guarantees pretty much every suspend and resume cycle,
drm_dp_mst_topology_mgr_set_mst(mgr, false); will be caused and in turn,
a connector hotplug will occur. Now it's Rute Goldberg time: when the
connector hotplug occurs, i915 reprobes /all/ of the connectors,
including eDP. However, eDP probing requires that we power on the panel
VDD which in turn, grabs a wakeref to the appropriate power domain on
the GPU (on my T480s, this is the PORT_DDI_A_IO domain). This is where
things start breaking, since this all happens before
intel_power_domains_enable() is called we end up leaking the wakeref
that was acquired and never releasing it later. Come next suspend/resume
cycle, this causes us to fail to shut down the GPU properly, which
causes it not to resume properly and die a horrible complicated death.

(as a note: this only happens when there's both an eDP panel and MST
topology connected which is removed mid-suspend. One or the other seems
to always be OK).

We could try to fix the VDD wakeref leak, but this doesn't seem like
it's worth it at all since we aren't able to handle hotplug detection
while resuming anyway. So, let's go with a more robust solution inspired
by nouveau: block fbdev from handling hotplug events until we resume
fbdev. This allows us to still send sysfs hotplug events to be handled
later by user space while we're resuming, while also preventing us from
actually processing any hotplug events we receive until it's safe.

This fixes the wakeref leak observed on the T480s and as such, also
fixes suspend/resume with MST topologies connected on this machine.

Changes since v2:
* Don't call drm_fb_helper_hotplug_event() under lock, do it after lock
  (Chris Wilson)
* Don't call drm_fb_helper_hotplug_event() in
  intel_fbdev_output_poll_changed() under lock (Chris Wilson)
* Always set ifbdev->hpd_waiting (Chris Wilson)

Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: 0e32b39ceed6 ("drm/i915: add DP 1.2 MST support (v0.7)")
Cc: Todd Previte <tprevite@gmail.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v3.17+
---
 drivers/gpu/drm/i915/intel_drv.h   | 10 +++++++++
 drivers/gpu/drm/i915/intel_fbdev.c | 33 +++++++++++++++++++++++++++++-
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 90ba5436370e..9ec3d00fbd19 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -213,6 +213,16 @@ struct intel_fbdev {
 	unsigned long vma_flags;
 	async_cookie_t cookie;
 	int preferred_bpp;
+
+	/* Whether or not fbdev hpd processing is temporarily suspended */
+	bool hpd_suspended : 1;
+	/* Set when a hotplug was received while HPD processing was
+	 * suspended
+	 */
+	bool hpd_waiting : 1;
+
+	/* Protects hpd_suspended */
+	struct mutex hpd_lock;
 };
 
 struct intel_encoder {
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 8cf3efe88f02..376ffe842e26 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -681,6 +681,7 @@ int intel_fbdev_init(struct drm_device *dev)
 	if (ifbdev == NULL)
 		return -ENOMEM;
 
+	mutex_init(&ifbdev->hpd_lock);
 	drm_fb_helper_prepare(dev, &ifbdev->helper, &intel_fb_helper_funcs);
 
 	if (!intel_fbdev_init_bios(dev, ifbdev))
@@ -754,6 +755,26 @@ void intel_fbdev_fini(struct drm_i915_private *dev_priv)
 	intel_fbdev_destroy(ifbdev);
 }
 
+/* Suspends/resumes fbdev processing of incoming HPD events. When resuming HPD
+ * processing, fbdev will perform a full connector reprobe if a hotplug event
+ * was received while HPD was suspended.
+ */
+static void intel_fbdev_hpd_set_suspend(struct intel_fbdev *ifbdev, int state)
+{
+	bool send_hpd = false;
+
+	mutex_lock(&ifbdev->hpd_lock);
+	ifbdev->hpd_suspended = state == FBINFO_STATE_SUSPENDED;
+	send_hpd = !ifbdev->hpd_suspended && ifbdev->hpd_waiting;
+	ifbdev->hpd_waiting = false;
+	mutex_unlock(&ifbdev->hpd_lock);
+
+	if (send_hpd) {
+		DRM_DEBUG_KMS("Handling delayed fbcon HPD event\n");
+		drm_fb_helper_hotplug_event(&ifbdev->helper);
+	}
+}
+
 void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -775,6 +796,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
 		 */
 		if (state != FBINFO_STATE_RUNNING)
 			flush_work(&dev_priv->fbdev_suspend_work);
+
 		console_lock();
 	} else {
 		/*
@@ -802,17 +824,26 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
 
 	drm_fb_helper_set_suspend(&ifbdev->helper, state);
 	console_unlock();
+
+	intel_fbdev_hpd_set_suspend(ifbdev, state);
 }
 
 void intel_fbdev_output_poll_changed(struct drm_device *dev)
 {
 	struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
+	bool send_hpd;
 
 	if (!ifbdev)
 		return;
 
 	intel_fbdev_sync(ifbdev);
-	if (ifbdev->vma || ifbdev->helper.deferred_setup)
+
+	mutex_lock(&ifbdev->hpd_lock);
+	send_hpd = !ifbdev->hpd_suspended;
+	ifbdev->hpd_waiting = true;
+	mutex_unlock(&ifbdev->hpd_lock);
+
+	if (send_hpd && (ifbdev->vma || ifbdev->helper.deferred_setup))
 		drm_fb_helper_hotplug_event(&ifbdev->helper);
 }
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 2/3] drm/i915: Don't send MST hotplugs during resume

[Lyude Paul]

We have a bad habit of calling drm_fb_helper_hotplug_event() far more
then we actually need to. MST appears to be one of these cases, where we
call drm_fb_helper_hotplug_event() if we fail to resume a connected MST
topology in intel_dp_mst_resume(). We don't actually need to do this at
all though since hotplug events are already sent from
drm_dp_connector_destroy_work() every time connectors are unregistered
from userspace's PoV. Additionally, extra calls to
drm_fb_helper_hotplug_event() also just mean more of a chance of doing a
connector probe somewhere we shouldn't.

So, don't send any hotplug events during resume if the MST topology
fails to come up. Just rely on the DP MST helpers to send them for us.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 681e88405ada..c2399acf177b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -7096,7 +7096,10 @@ void intel_dp_mst_resume(struct drm_i915_private *dev_priv)
 			continue;
 
 		ret = drm_dp_mst_topology_mgr_resume(&intel_dp->mst_mgr);
-		if (ret)
-			intel_dp_check_mst_status(intel_dp);
+		if (ret) {
+			intel_dp->is_mst = false;
+			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+							false);
+		}
 	}
 }
-- 
2.20.1

[PATCH v3 3/3] drm/i915: Don't send hotplug in intel_dp_check_mst_status()

[Lyude Paul]

This hotplug also isn't needed: drm_dp_mst_topology_mgr_set_mst()
already sends a hotplug on its own from drm_dp_destroy_connector_work()
after destroying connectors in the MST topology.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c2399acf177b..f9113c0cdfcd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4608,12 +4608,10 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
 
 			return ret;
 		} else {
-			struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 			DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
 			intel_dp->is_mst = false;
-			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
-			/* send a hotplug event */
-			drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
+			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+							intel_dp->is_mst);
 		}
 	}
 	return -EINVAL;
-- 
2.20.1

Re: [PATCH v3 1/3] drm/i915: Block fbdev HPD processing during suspend

[Chris Wilson]

Quoting Lyude Paul (2019-01-29 19:09:59)
> When resuming, we check whether or not any previously connected
> MST topologies are still present and if so, attempt to resume them. If
> this fails, we disable said MST topologies and fire off a hotplug event
> so that userspace knows to reprobe.
> 
> However, sending a hotplug event involves calling
> drm_fb_helper_hotplug_event(), which in turn results in fbcon doing a
> connector reprobe in the caller's thread - something we can't do at the
> point in which i915 calls drm_dp_mst_topology_mgr_resume() since
> hotplugging hasn't been fully initialized yet.
> 
> This currently causes some rather subtle but fatal issues. For example,
> on my T480s the laptop dock connected to it usually disappears during a
> suspend cycle, and comes back up a short while after the system has been
> resumed. This guarantees pretty much every suspend and resume cycle,
> drm_dp_mst_topology_mgr_set_mst(mgr, false); will be caused and in turn,
> a connector hotplug will occur. Now it's Rute Goldberg time: when the
> connector hotplug occurs, i915 reprobes /all/ of the connectors,
> including eDP. However, eDP probing requires that we power on the panel
> VDD which in turn, grabs a wakeref to the appropriate power domain on
> the GPU (on my T480s, this is the PORT_DDI_A_IO domain). This is where
> things start breaking, since this all happens before
> intel_power_domains_enable() is called we end up leaking the wakeref
> that was acquired and never releasing it later. Come next suspend/resume
> cycle, this causes us to fail to shut down the GPU properly, which
> causes it not to resume properly and die a horrible complicated death.
> 
> (as a note: this only happens when there's both an eDP panel and MST
> topology connected which is removed mid-suspend. One or the other seems
> to always be OK).
> 
> We could try to fix the VDD wakeref leak, but this doesn't seem like
> it's worth it at all since we aren't able to handle hotplug detection
> while resuming anyway. So, let's go with a more robust solution inspired
> by nouveau: block fbdev from handling hotplug events until we resume
> fbdev. This allows us to still send sysfs hotplug events to be handled
> later by user space while we're resuming, while also preventing us from
> actually processing any hotplug events we receive until it's safe.
> 
> This fixes the wakeref leak observed on the T480s and as such, also
> fixes suspend/resume with MST topologies connected on this machine.
> 
> Changes since v2:
> * Don't call drm_fb_helper_hotplug_event() under lock, do it after lock
>   (Chris Wilson)
> * Don't call drm_fb_helper_hotplug_event() in
>   intel_fbdev_output_poll_changed() under lock (Chris Wilson)
> * Always set ifbdev->hpd_waiting (Chris Wilson)
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Fixes: 0e32b39ceed6 ("drm/i915: add DP 1.2 MST support (v0.7)")
> Cc: Todd Previte <tprevite@gmail.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v3.17+

I suspect the locking is overkill, but certainly easier to reason than
trying to remember all the ins and outs of fbdev with its dubious async
initialisation.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

✗ Fi.CI.CHECKPATCH: warning for drm/i915: MST and wakeref leak fixes (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: MST and wakeref leak fixes (rev2)
URL   : https://patchwork.freedesktop.org/series/55868/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
13be9527c910 drm/i915: Block fbdev HPD processing during suspend
-:77: WARNING:BOOL_BITFIELD: Avoid using bool as bitfield.  Prefer bool bitfields as unsigned int or u<8|16|32>
#77: FILE: drivers/gpu/drm/i915/intel_drv.h:218:
+	bool hpd_suspended : 1;

-:81: WARNING:BOOL_BITFIELD: Avoid using bool as bitfield.  Prefer bool bitfields as unsigned int or u<8|16|32>
#81: FILE: drivers/gpu/drm/i915/intel_drv.h:222:
+	bool hpd_waiting : 1;

total: 0 errors, 2 warnings, 0 checks, 83 lines checked
649fabd15d65 drm/i915: Don't send MST hotplugs during resume
d2dcf0a4f100 drm/i915: Don't send hotplug in intel_dp_check_mst_status()

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: MST and wakeref leak fixes (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: MST and wakeref leak fixes (rev2)
URL   : https://patchwork.freedesktop.org/series/55868/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5501 -> Patchwork_12080
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/55868/revisions/2/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12080 that come from known issues:

### IGT changes ###

#### Possible fixes ####

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-kbl-7567u:       DMESG-WARN [fdo#105602] / [fdo#108529] -> PASS +1

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7567u:       DMESG-FAIL [fdo#105079] -> PASS

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS +1

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c:
    - fi-kbl-7567u:       {SKIP} [fdo#109271] -> PASS +33

  * igt@pm_rpm@module-reload:
    - fi-kbl-7567u:       DMESG-WARN [fdo#108529] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#105079]: https://bugs.freedesktop.org/show_bug.cgi?id=105079
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#108529]: https://bugs.freedesktop.org/show_bug.cgi?id=108529
  [fdo#108915]: https://bugs.freedesktop.org/show_bug.cgi?id=108915
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271


Participating hosts (46 -> 39)
------------------------------

  Additional (1): fi-icl-y 
  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-pnv-d510 fi-bdw-samus 


Build changes
-------------

    * Linux: CI_DRM_5501 -> Patchwork_12080

  CI_DRM_5501: 5aac0b69f3a3085b9b8a9924cb19cb94f7860dbe @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4798: 998e0a4aedf10fb5f7c271018cd80d874668bf55 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12080: d2dcf0a4f1004a999f325171740fffd50cbdf31a @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

d2dcf0a4f100 drm/i915: Don't send hotplug in intel_dp_check_mst_status()
649fabd15d65 drm/i915: Don't send MST hotplugs during resume
13be9527c910 drm/i915: Block fbdev HPD processing during suspend

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12080/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: MST and wakeref leak fixes (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: MST and wakeref leak fixes (rev2)
URL   : https://patchwork.freedesktop.org/series/55868/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5501_full -> Patchwork_12080_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_12080_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_eio@reset-stress:
    - shard-hsw:          PASS -> INCOMPLETE [fdo#103540]

  * igt@gem_workarounds@suspend-resume-fd:
    - shard-apl:          PASS -> DMESG-WARN [fdo#108566]

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
    - shard-snb:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_color@pipe-a-legacy-gamma:
    - shard-apl:          PASS -> FAIL [fdo#104782] / [fdo#108145]

  * igt@kms_cursor_crc@cursor-128x128-random:
    - shard-apl:          PASS -> FAIL [fdo#103232] +2

  * igt@kms_flip@2x-flip-vs-expired-vblank:
    - shard-glk:          PASS -> FAIL [fdo#105363]

  * igt@kms_plane@pixel-format-pipe-c-planes-source-clamping:
    - shard-apl:          PASS -> FAIL [fdo#108948]

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-none:
    - shard-apl:          PASS -> FAIL [fdo#103166] +1

  
#### Possible fixes ####

  * igt@kms_color@pipe-b-ctm-max:
    - shard-apl:          FAIL [fdo#108147] -> PASS

  * igt@kms_cursor_crc@cursor-256x256-random:
    - shard-glk:          FAIL [fdo#103232] -> PASS +1

  * igt@kms_cursor_crc@cursor-256x256-suspend:
    - shard-apl:          FAIL [fdo#103191] / [fdo#103232] -> PASS

  * igt@kms_cursor_crc@cursor-256x85-random:
    - shard-apl:          FAIL [fdo#103232] -> PASS +2

  * igt@kms_cursor_legacy@2x-long-nonblocking-modeset-vs-cursor-atomic:
    - shard-glk:          FAIL [fdo#105454] / [fdo#106509] -> PASS

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-glk:          FAIL [fdo#102887] / [fdo#105363] -> PASS

  * igt@kms_setmode@basic:
    - shard-apl:          FAIL [fdo#99912] -> PASS

  * igt@prime_busy@hang-bsd:
    - shard-hsw:          FAIL [fdo#108807] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105454]: https://bugs.freedesktop.org/show_bug.cgi?id=105454
  [fdo#106509]: https://bugs.freedesktop.org/show_bug.cgi?id=106509
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108807]: https://bugs.freedesktop.org/show_bug.cgi?id=108807
  [fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (7 -> 5)
------------------------------

  Missing    (2): shard-skl shard-iclb 


Build changes
-------------

    * Linux: CI_DRM_5501 -> Patchwork_12080

  CI_DRM_5501: 5aac0b69f3a3085b9b8a9924cb19cb94f7860dbe @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4798: 998e0a4aedf10fb5f7c271018cd80d874668bf55 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12080: d2dcf0a4f1004a999f325171740fffd50cbdf31a @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12080/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/3] drm/i915: Block fbdev HPD processing during suspend

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 0e32b39ceed6 drm/i915: add DP 1.2 MST support (v0.7).

The bot has tested the following trees: v4.20.5, v4.19.18, v4.14.96, v4.9.153, v4.4.172, v3.18.133.

v4.20.5: Build OK!
v4.19.18: Build OK!
v4.14.96: Failed to apply! Possible dependencies:
    df9e6521749a ("drm/i915/fbdev: Enable late fbdev initial configuration")

v4.9.153: Failed to apply! Possible dependencies:
    1c777c5d1dcd ("drm/i915/hsw: Fix GPU hang during resume from S3-devices state")
    275f039db56f ("drm/i915: Move user fault tracking to a separate list")
    3594a3e21f1f ("drm/i915: Remove superfluous locking around userfault_list")
    4f256d8219f2 ("drm/i915: Fix fbdev unload sequence")
    7c108fd8feac ("drm/i915: Move fence cancellation to runtime suspend")
    8baa1f04b9ed ("drm/i915: Update debugfs describe_obj() to show fault-mappable")
    96d776345277 ("drm/i915: Use a radixtree for random access to the object's backing storage")
    a4f5ea64f0a8 ("drm/i915: Refactor object page API")
    ad88d7fc6c03 ("drm/i915/fbdev: Serialise early hotplug events with async fbdev config")
    bf9e8429ab97 ("drm/i915: Make various init functions take dev_priv")
    eef57324d926 ("drm/i915: setup bridge for HDMI LPE audio driver")
    f8a7fde45610 ("drm/i915: Defer active reference until required")
    fbbd37b36fa5 ("drm/i915: Move object release to a freelist + worker")

v4.4.172: Failed to apply! Possible dependencies:
    61642ff03523 ("drm/i915: Inspect subunit states on hangcheck")
    c45eb4fed12d ("drm/i915/fbdev: Check for the framebuffer before use")
    ca82580c9cea ("drm/i915: Do not call API requiring struct_mutex where it is not available")
    cbdc12a9fc9d ("drm/i915: make A0 wa's applied to A1")
    e28e404c3e93 ("drm/i915: tidy up a few leftovers")
    e2f80391478a ("drm/i915: Rename local struct intel_engine_cs variables")
    e87a005d90c3 ("drm/i915: add helpers for platform specific revision id range checks")
    ed54c1a1d11c ("drm/i915: abolish separate per-ring default_context pointers")
    ef712bb4b700 ("drm/i915: remove parens around revision ids")
    fac5e23e3c38 ("drm/i915: Mass convert dev->dev_private to to_i915(dev)")
    fffda3f4fb49 ("drm/i915/bxt: add revision id for A1 stepping and use it")

v3.18.133: Failed to apply! Possible dependencies:
    0794aed30285 ("drm/i915: Fix context object leak for legacy contexts")
    20e28fba48f2 ("drm/i915: Be consistent on printing seqnos")
    24955f2412fa ("drm/i915: Clarify mmio_flip_lock locking")
    26ff27621080 ("drm/i915: Add kerneldoc for intel_pipe_update_{start, end}")
    3a8a946efbe0 ("drm/i915: Remove redundant flip_work->flip_queued_ring")
    481a3d43b94f ("drm/i915: Include active flag when describing objects in debugfs")
    493018dcb1c7 ("drm/i915: Implement a framework for batch buffer pools")
    536f5b5e86b2 ("drm/i915: Make mmio flip wait for seqno in the work function")
    6259cead57eb ("drm/i915: Remove 'outstanding_lazy_seqno'")
    9362c7c576d3 ("drm/i915: Use vblank evade mechanism in mmio_flip")
    97b2a6a10a1a ("drm/i915: Replace last_[rwf]_seqno with last_[rwf]_req")
    9eba5d4a1d79 ("drm/i915: Ensure OLS & PLR are always in sync")
    abfe262ae762 ("drm/i915: Add reference count to request structure")
    b47161858ba1 ("drm/i915: Implement inter-engine read-read optimisations")
    c45eb4fed12d ("drm/i915/fbdev: Check for the framebuffer before use")
    dcb4c12a6877 ("drm/i915/bdw: Pin the context backing objects to GGTT on-demand")
    e2f80391478a ("drm/i915: Rename local struct intel_engine_cs variables")
    f06cc1b9401c ("drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request'")
    fac5e23e3c38 ("drm/i915: Mass convert dev->dev_private to to_i915(dev)")


How should we proceed with this patch?

--
Thanks,
Sasha
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/3] drm/i915: Don't send MST hotplugs during resume

[Ville Syrjälä]

On Tue, Jan 29, 2019 at 02:10:00PM -0500, Lyude Paul wrote:
> We have a bad habit of calling drm_fb_helper_hotplug_event() far more
> then we actually need to. MST appears to be one of these cases, where we
> call drm_fb_helper_hotplug_event() if we fail to resume a connected MST
> topology in intel_dp_mst_resume(). We don't actually need to do this at
> all though since hotplug events are already sent from
> drm_dp_connector_destroy_work() every time connectors are unregistered
> from userspace's PoV. Additionally, extra calls to
> drm_fb_helper_hotplug_event() also just mean more of a chance of doing a
> connector probe somewhere we shouldn't.
> 
> So, don't send any hotplug events during resume if the MST topology
> fails to come up. Just rely on the DP MST helpers to send them for us.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 681e88405ada..c2399acf177b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -7096,7 +7096,10 @@ void intel_dp_mst_resume(struct drm_i915_private *dev_priv)
>  			continue;
>  
>  		ret = drm_dp_mst_topology_mgr_resume(&intel_dp->mst_mgr);
> -		if (ret)
> -			intel_dp_check_mst_status(intel_dp);
> +		if (ret) {
> +			intel_dp->is_mst = false;
> +			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> +							false);

drm_dp_mst_topology_mgr_set_mst()
-> drm_dp_mst_topology_put_mstb()
   -> drm_dp_destroy_mst_branch_device()
      -> drm_dp_mst_topology_put_port()
         -> drm_dp_destroy_port()
	    -> drm_dp_destroy_connector_work()
	       -> drm_kms_helper_hotplug_event()

That of course requires that no one is hanging on to the
kref(s). The lifetime of the references isn't really clear
to me, but I'll take your word that it works.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		}
>  	}
>  }
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v3 3/3] drm/i915: Don't send hotplug in intel_dp_check_mst_status()

[Ville Syrjälä]

On Tue, Jan 29, 2019 at 02:10:01PM -0500, Lyude Paul wrote:
> This hotplug also isn't needed: drm_dp_mst_topology_mgr_set_mst()
> already sends a hotplug on its own from drm_dp_destroy_connector_work()
> after destroying connectors in the MST topology.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c2399acf177b..f9113c0cdfcd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4608,12 +4608,10 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  
>  			return ret;
>  		} else {
> -			struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  			DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
>  			intel_dp->is_mst = false;
> -			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> -			/* send a hotplug event */
> -			drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
> +			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> +							intel_dp->is_mst);
>  		}
>  	}
>  	return -EINVAL;
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 1/3] drm/i915: Block fbdev HPD processing during suspend

[Lyude Paul]

On Wed, 2019-01-30 at 14:46 +0000, Sasha Levin wrote:
> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: 0e32b39ceed6 drm/i915: add DP 1.2 MST support (v0.7).
> 
> The bot has tested the following trees: v4.20.5, v4.19.18, v4.14.96,
> v4.9.153, v4.4.172, v3.18.133.
> 
> v4.20.5: Build OK!
> v4.19.18: Build OK!

Just apply this to 4.20 and 4.19 then
> v4.14.96: Failed to apply! Possible dependencies:
>     df9e6521749a ("drm/i915/fbdev: Enable late fbdev initial configuration")
> 
> v4.9.153: Failed to apply! Possible dependencies:
>     1c777c5d1dcd ("drm/i915/hsw: Fix GPU hang during resume from S3-devices
> state")
>     275f039db56f ("drm/i915: Move user fault tracking to a separate list")
>     3594a3e21f1f ("drm/i915: Remove superfluous locking around
> userfault_list")
>     4f256d8219f2 ("drm/i915: Fix fbdev unload sequence")
>     7c108fd8feac ("drm/i915: Move fence cancellation to runtime suspend")
>     8baa1f04b9ed ("drm/i915: Update debugfs describe_obj() to show fault-
> mappable")
>     96d776345277 ("drm/i915: Use a radixtree for random access to the
> object's backing storage")
>     a4f5ea64f0a8 ("drm/i915: Refactor object page API")
>     ad88d7fc6c03 ("drm/i915/fbdev: Serialise early hotplug events with async
> fbdev config")
>     bf9e8429ab97 ("drm/i915: Make various init functions take dev_priv")
>     eef57324d926 ("drm/i915: setup bridge for HDMI LPE audio driver")
>     f8a7fde45610 ("drm/i915: Defer active reference until required")
>     fbbd37b36fa5 ("drm/i915: Move object release to a freelist + worker")
> 
> v4.4.172: Failed to apply! Possible dependencies:
>     61642ff03523 ("drm/i915: Inspect subunit states on hangcheck")
>     c45eb4fed12d ("drm/i915/fbdev: Check for the framebuffer before use")
>     ca82580c9cea ("drm/i915: Do not call API requiring struct_mutex where it
> is not available")
>     cbdc12a9fc9d ("drm/i915: make A0 wa's applied to A1")
>     e28e404c3e93 ("drm/i915: tidy up a few leftovers")
>     e2f80391478a ("drm/i915: Rename local struct intel_engine_cs variables")
>     e87a005d90c3 ("drm/i915: add helpers for platform specific revision id
> range checks")
>     ed54c1a1d11c ("drm/i915: abolish separate per-ring default_context
> pointers")
>     ef712bb4b700 ("drm/i915: remove parens around revision ids")
>     fac5e23e3c38 ("drm/i915: Mass convert dev->dev_private to to_i915(dev)")
>     fffda3f4fb49 ("drm/i915/bxt: add revision id for A1 stepping and use
> it")
> 
> v3.18.133: Failed to apply! Possible dependencies:
>     0794aed30285 ("drm/i915: Fix context object leak for legacy contexts")
>     20e28fba48f2 ("drm/i915: Be consistent on printing seqnos")
>     24955f2412fa ("drm/i915: Clarify mmio_flip_lock locking")
>     26ff27621080 ("drm/i915: Add kerneldoc for intel_pipe_update_{start,
> end}")
>     3a8a946efbe0 ("drm/i915: Remove redundant flip_work->flip_queued_ring")
>     481a3d43b94f ("drm/i915: Include active flag when describing objects in
> debugfs")
>     493018dcb1c7 ("drm/i915: Implement a framework for batch buffer pools")
>     536f5b5e86b2 ("drm/i915: Make mmio flip wait for seqno in the work
> function")
>     6259cead57eb ("drm/i915: Remove 'outstanding_lazy_seqno'")
>     9362c7c576d3 ("drm/i915: Use vblank evade mechanism in mmio_flip")
>     97b2a6a10a1a ("drm/i915: Replace last_[rwf]_seqno with last_[rwf]_req")
>     9eba5d4a1d79 ("drm/i915: Ensure OLS & PLR are always in sync")
>     abfe262ae762 ("drm/i915: Add reference count to request structure")
>     b47161858ba1 ("drm/i915: Implement inter-engine read-read
> optimisations")
>     c45eb4fed12d ("drm/i915/fbdev: Check for the framebuffer before use")
>     dcb4c12a6877 ("drm/i915/bdw: Pin the context backing objects to GGTT on-
> demand")
>     e2f80391478a ("drm/i915: Rename local struct intel_engine_cs variables")
>     f06cc1b9401c ("drm/i915: Convert 'flip_queued_seqno' into
> 'flip_queued_request'")
>     fac5e23e3c38 ("drm/i915: Mass convert dev->dev_private to to_i915(dev)")
> 
> 
> How should we proceed with this patch?
> 
> --
> Thanks,
> Sasha
-- 
Cheers,
	Lyude Paul