[PATCH 38/73] drm/amd/display: Fix warnings on S3 resume

[Leo Li]

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

>>
>> 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
>