Re: [PATCH 38/73] drm/amd/display: Fix warnings on S3 resume
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"
This is a followup to the following revert:
Rex ZhuRevert "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
Reviewed-by: Harry Wentland
---
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
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-
Re: [PATCH 38/73] drm/amd/display: Fix warnings on S3 resume
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"
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
Reviewed-by: Harry Wentland
---
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
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.or
Re: [PATCH 38/73] drm/amd/display: Fix warnings on S3 resume
On 11/10/2017 01:38 PM, Andrey Grodzovsky wrote:
On 11/09/2017 03:05 PM, Harry Wentland wrote:
From: "Leo (Sunpeng) Li"
This is a followup to the following revert:
Rex ZhuRevert "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
Reviewed-by: Harry Wentland
---
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
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
Re: [PATCH 38/73] drm/amd/display: Fix warnings on S3 resume
On 11/09/2017 03:05 PM, Harry Wentland wrote:
From: "Leo (Sunpeng) Li"
This is a followup to the following revert:
Rex ZhuRevert "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
Reviewed-by: Harry Wentland
---
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.
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
