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

[Andrey Grodzovsky]

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