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.li at 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.li at amd.com>
>>>> Reviewed-by: Harry Wentland <Harry.Wentland at 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.grodzovsky at 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 at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
