[PATCH v2 0/3] drm/amdgpu: Fix suspend/resume issues with MST

[PATCH v2 0/3] drm/amdgpu: Fix suspend/resume issues with MST

[Lyude Paul]

Fix the suspend/issues that Jerry Zuo found in amdgpu, and add some
compiler warnings for drivers ignoring the return code of
drm_dp_mst_topology_mgr_resume() to help ensure we don't need to fix
this again in the future for someone else's driver.

Cc: Jerry Zuo <Jerry.Zuo@amd.com>

Lyude Paul (3):
  drm/amdgpu: Don't ignore rc from drm_dp_mst_topology_mgr_resume()
  drm/amdgpu: Don't fail resume process if resuming atomic state fails
  drm/dp_mst: Add __must_check to drm_dp_mst_topology_mgr_resume()

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 37 +++++++++++++------
 include/drm/drm_dp_mst_helper.h               |  3 +-
 2 files changed, 27 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] drm/amdgpu: Don't ignore rc from drm_dp_mst_topology_mgr_resume()

[Lyude Paul]

drm_dp_mst_topology_mgr_resume() returns whether or not it managed to
find the topology in question after a suspend resume cycle, and the
driver is supposed to check this value and disable MST accordingly if
it's gone-in addition to sending a hotplug in order to notify userspace
that something changed during suspend.

Currently, amdgpu just makes the mistake of ignoring the return code
from drm_dp_mst_topology_mgr_resume() which means that if a topology was
removed in suspend, amdgpu never notices and assumes it's still
connected which leads to all sorts of problems.

So, fix this by actually checking the rc from
drm_dp_mst_topology_mgr_resume(). Also, reformat the rest of the
function while we're at it to fix the over-indenting.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Cc: Jerry Zuo <Jerry.Zuo@amd.com>
Cc: <stable@vger.kernel.org> # v4.15+
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +++++++++++++------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 8a626d16e8e3..3f326a2c513b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -699,22 +699,36 @@ static void s3_handle_mst(struct drm_device *dev, bool suspend)
 {
 	struct amdgpu_dm_connector *aconnector;
 	struct drm_connector *connector;
+	struct drm_dp_mst_topology_mgr *mgr;
+	int ret;
+	bool need_hotplug = false;
 
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		   aconnector = to_amdgpu_dm_connector(connector);
-		   if (aconnector->dc_link->type == dc_connection_mst_branch &&
-				   !aconnector->mst_port) {
+	list_for_each_entry(connector, &dev->mode_config.connector_list,
+			    head) {
+		aconnector = to_amdgpu_dm_connector(connector);
+		if (aconnector->dc_link->type != dc_connection_mst_branch ||
+		    aconnector->mst_port)
+			continue;
+
+		mgr = &aconnector->mst_mgr;
 
-			   if (suspend)
-				   drm_dp_mst_topology_mgr_suspend(&aconnector->mst_mgr);
-			   else
-				   drm_dp_mst_topology_mgr_resume(&aconnector->mst_mgr);
-		   }
+		if (suspend) {
+			drm_dp_mst_topology_mgr_suspend(mgr);
+		} else {
+			ret = drm_dp_mst_topology_mgr_resume(mgr);
+			if (ret < 0) {
+				drm_dp_mst_topology_mgr_set_mst(mgr, false);
+				need_hotplug = true;
+			}
+		}
 	}
 
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
+	if (need_hotplug)
+		drm_kms_helper_hotplug_event(dev);
 }
 
 /**
-- 
2.20.1

[PATCH v2 2/3] drm/amdgpu: Don't fail resume process if resuming atomic state fails

[Lyude Paul]

This is an ugly one unfortunately. Currently, all DRM drivers supporting
atomic modesetting will save the state that userspace had set before
suspending, then attempt to restore that state on resume. This probably
worked very well at one point, like many other things, until DP MST came
into the picture. While it's easy to restore state on normal display
connectors that were disconnected during suspend regardless of their
state post-resume, this can't really be done with MST because of the
fact that setting up a downstream sink requires performing sideband
transactions between the source and the MST hub, sending out the ACT
packets, etc.

Because of this, there isn't really a guarantee that we can restore the
atomic state we had before suspend once we've resumed. This sucks pretty
bad, but so far I haven't run into any compositors that this actually
causes serious issues with. Most compositors will notice the hotplug we
send afterwards, and then reprobe state.

Since nouveau and i915 also don't fail the suspend/resume process due to
failing to restore the atomic state, let's make amdgpu match this
behavior. Better to resume the GPU properly, then to stop the process
half way because of a potentially unavoidable atomic commit failure.

Eventually, we'll have a real fix for this problem on the DRM level. But
we've got some more important low-hanging fruit to deal with first.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Cc: Jerry Zuo <Jerry.Zuo@amd.com>
Cc: <stable@vger.kernel.org> # v4.15+
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 3f326a2c513b..a3e65e457348 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -912,7 +912,6 @@ static int dm_resume(void *handle)
 	struct drm_plane_state *new_plane_state;
 	struct dm_plane_state *dm_new_plane_state;
 	enum dc_connection_type new_connection_type = dc_connection_none;
-	int ret;
 	int i;
 
 	/* power on hardware */
@@ -985,13 +984,13 @@ static int dm_resume(void *handle)
 		}
 	}
 
-	ret = drm_atomic_helper_resume(ddev, dm->cached_state);
+	drm_atomic_helper_resume(ddev, dm->cached_state);
 
 	dm->cached_state = NULL;
 
 	amdgpu_dm_irq_resume_late(adev);
 
-	return ret;
+	return 0;
 }
 
 /**
-- 
2.20.1

[PATCH v2 3/3] drm/dp_mst: Add __must_check to drm_dp_mst_topology_mgr_resume()

[Lyude Paul]

Since I've had to fix two cases of drivers not checking the return code
from this function, let's make the compiler complain so this doesn't
come up again in the future.

Changes since v1:
* Remove unneeded __must_check in function declaration - danvet

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Jerry Zuo <Jerry.Zuo@amd.com>
Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
---
 include/drm/drm_dp_mst_helper.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 371cc2816477..4355b55d0081 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -614,7 +614,8 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
 			      struct drm_dp_mst_topology_mgr *mgr);
 
 void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
-int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
+int __must_check
+drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
 struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
 								    struct drm_dp_mst_topology_mgr *mgr);
 int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
-- 
2.20.1

Re: [PATCH v2 0/3] drm/amdgpu: Fix suspend/resume issues with MST

[Alex Deucher]

On Tue, Jan 8, 2019 at 4:11 PM Lyude Paul <lyude@redhat.com> wrote:
>
> Fix the suspend/issues that Jerry Zuo found in amdgpu, and add some
> compiler warnings for drivers ignoring the return code of
> drm_dp_mst_topology_mgr_resume() to help ensure we don't need to fix
> this again in the future for someone else's driver.
>
> Cc: Jerry Zuo <Jerry.Zuo@amd.com>
>
> Lyude Paul (3):
>   drm/amdgpu: Don't ignore rc from drm_dp_mst_topology_mgr_resume()
>   drm/amdgpu: Don't fail resume process if resuming atomic state fails
>   drm/dp_mst: Add __must_check to drm_dp_mst_topology_mgr_resume()

I can take these through the amdgpu tree or you can commit them to
drm-misc (assuming you have commit rights).  Either way works for me.

Thanks,

Alex

>
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 37 +++++++++++++------
>  include/drm/drm_dp_mst_helper.h               |  3 +-
>  2 files changed, 27 insertions(+), 13 deletions(-)
>
> --
> 2.20.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 0/3] drm/amdgpu: Fix suspend/resume issues with MST

[Lyude Paul]

On Tue, 2019-01-08 at 16:17 -0500, Alex Deucher wrote:
> On Tue, Jan 8, 2019 at 4:11 PM Lyude Paul <lyude@redhat.com> wrote:
> > Fix the suspend/issues that Jerry Zuo found in amdgpu, and add some
> > compiler warnings for drivers ignoring the return code of
> > drm_dp_mst_topology_mgr_resume() to help ensure we don't need to fix
> > this again in the future for someone else's driver.
> > 
> > Cc: Jerry Zuo <Jerry.Zuo@amd.com>
> > 
> > Lyude Paul (3):
> >   drm/amdgpu: Don't ignore rc from drm_dp_mst_topology_mgr_resume()
> >   drm/amdgpu: Don't fail resume process if resuming atomic state fails
> >   drm/dp_mst: Add __must_check to drm_dp_mst_topology_mgr_resume()
> 
> I can take these through the amdgpu tree or you can commit them to
> drm-misc (assuming you have commit rights).  Either way works for me.
I don't mind just pushing these to drm-misc since you seem OK with it. Thanks!
> 
> Thanks,
> 
> Alex
> 
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 37 +++++++++++++------
> >  include/drm/drm_dp_mst_helper.h               |  3 +-
> >  2 files changed, 27 insertions(+), 13 deletions(-)
> > 
> > --
> > 2.20.1
> > 
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
-- 
Cheers,
	Lyude Paul