On 11/14/2017 11:00 AM, Leo Li wrote:


On 2017-11-10 01:40 PM, Andrey Grodzovsky wrote:


On 11/10/2017 01:38 PM, Andrey Grodzovsky wrote:


On 11/09/2017 03:05 PM, Harry Wentland wrote:
From: "Leo (Sunpeng) Li" <sunpeng...@amd.com>

This is a followup to the following revert:

Rex Zhu    Revert "drm/amd/display: Match actual state during S3
            resume."

Three things needed to be addressed:

1. Potential memory leak on dc_state creation in atomic_check during
    s3 resume
2. Warnings are now seen in dmesg during S3 resume
3. Since dc_state is now created in atomic_check, what the reverted
    patch was addressing needs to be reevaluated.

This change addresses the above:

1. Since the suspend procedure calls drm_atomic_state_clear, our hook
    for releasing the dc_state is called. This frees it before
    atomic_check creates it during resume. The leak does not occur.

2. The dc_crtc/plane_state references kept by the atomic states need to
    be released before calling atomic_check, which warns if they are
    non-null. This is because atomic_check is responsible for creating
    the dc_*_states. This is a special case for S3 resume, since the
    atomic state duplication that occurs during suspend also copies a
    reference to the dc_*_states.

3. See 2. comments are also updated to reflect this.

Change-Id: I6e342bf8134f0e5dc32888a8d894c2cd20d28296
Signed-off-by: Leo (Sunpeng) Li <sunpeng...@amd.com>
Reviewed-by: Harry Wentland <harry.wentl...@amd.com>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++++++++++
  1 file changed, 28 insertions(+)

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 1c7f22146bc9..bdef1ed0dfac 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -643,6 +643,11 @@ int amdgpu_dm_display_resume(struct amdgpu_device *adev)
      struct drm_connector *connector;
      struct drm_crtc *crtc;
      struct drm_crtc_state *new_crtc_state;
+    struct dm_crtc_state *dm_new_crtc_state;
+    struct drm_plane *plane;
+    struct drm_plane_state *new_plane_state;
+    struct dm_plane_state *dm_new_plane_state;
+
      int ret = 0;
      int i;
@@ -685,6 +690,29 @@ int amdgpu_dm_display_resume(struct amdgpu_device *adev) for_each_new_crtc_in_state(adev->dm.cached_state, crtc, new_crtc_state, i)
          new_crtc_state->active_changed = true;
  +    /*
+ * atomic_check is expected to create the dc states. We need to release
+     * them here, since they were duplicated as part of the suspend
+     * procedure.
+     */
+ for_each_new_crtc_in_state(adev->dm.cached_state, crtc, new_crtc_state, i) {
+        dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+        if (dm_new_crtc_state->stream) {
+ WARN_ON(kref_read(&dm_new_crtc_state->stream->refcount) > 1);
+ dc_stream_release(dm_new_crtc_state->stream);
+            dm_new_crtc_state->stream = NULL;
+        }
+    }
+
+ for_each_new_plane_in_state(adev->dm.cached_state, plane, new_plane_state, i) {
+        dm_new_plane_state = to_dm_plane_state(new_plane_state);
+        if (dm_new_plane_state->dc_state) {
+ WARN_ON(kref_read(&dm_new_plane_state->dc_state->refcount) > 1);
+ dc_plane_state_release(dm_new_plane_state->dc_state);
+            dm_new_plane_state->dc_state = NULL;
+        }
+    }

I guess the warnings you are referring to are in dm_update_crtcs_state and dm_update_planes_state, but I don't understand why you need to explicitly release them in amdgpu_dm_resume, any changed plane/crtc states will be removed during atomic check anyway and only after that new one will be added,
I find strange that this doesn't work.

P.S I tried to reproduce this to see the warnings with latest amd-staging-drm-next (776fb8c)on CZ but the system becomes unimpressive after going to suspend, might wanna check this on your side.

UNRESPONSIVE (it's unimpressive in any case :) )

Andrey


It's just a special case for s3 resume. The suspend helper first duplicates the existing atomic state (which in turn, duplicates references to dc states), then calls the disable_all helper. This means that during resume, the old state will have everything disabled, while the new state is the duplicated state that we're trying to restore.

Our atomic check isn't very happy with this. From it's perspective, we're clearly trying to enable a CRTC. It doesn't expect the new drm_*_states to have associated dc_*_states yet. This is why the releases are necessary within amdgpu_dm_resume; to make things consistent for atomic_check.

Leo

I see, this change Reviewed-by: Andrey Grodzovsky <andrey.grodzov...@amd.com>

Thanks,
Andrey


Thanks,
Andrey

+
      ret = drm_atomic_helper_resume(ddev, adev->dm.cached_state);
        drm_atomic_state_put(adev->dm.cached_state);

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


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

Reply via email to