[PATCH] drm/amd/display: Fix memory leaks in S3 resume

[Wang, Chao-kai (Stylon)]

[AMD Official Use Only - Internal Distribution Only]

Hi Harry,

Checked Lee Starnes's patch. It does not fix the issue but does preserve the tradition of assigning the number of probed modes to aconnector->num_modes, which appears elsewhere in AMD code. I observed in some cases the memory leak is not reproducing (or not detected) even without mine or Lee's fix. Maybe that's why Lee thinks it may fix the leak.

Another way to fix the issue is to not wipe out the probed list by calling INIT_LIST_HEAD() in amdgpu_dm_connector_ddc_get_modes(). But I am not entirely sure what other impact will that do.

I am checking out how everyone is handling resume from S3 so would like some second opinions.


Regards
Regards

Stylon Wang

MTS Software Development Eng.  |  AMD
Display Solution Team

O +(886) 2-3789-3667 ext. 23667  C +(886) 921-897-142

----------------------------------------------------------------------------------------------------------------------------------

6F, 3, YuanCyu St (NanKang Software Park) Taipei, Taiwan

Facebook<https://www.facebook.com/AMD> |  Twitter<https://twitter.com/AMD> |  amd.com<http://www.amd.com/>



________________________________
From: Deucher, Alexander <Alexander.Deucher at amd.com>
Sent: November 11, 2020 12:55 AM
To: Wentland, Harry <Harry.Wentland at amd.com>; Wang, Chao-kai (Stylon) <Stylon.Wang at amd.com>; amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>
Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas at amd.com>
Subject: Re: [PATCH] drm/amd/display: Fix memory leaks in S3 resume


[AMD Official Use Only - Internal Distribution Only]

Ah, sorry, I missed that part of the patch.

Alex

________________________________
From: Wentland, Harry <Harry.Wentland at amd.com>
Sent: Tuesday, November 10, 2020 11:42 AM
To: Deucher, Alexander <Alexander.Deucher at amd.com>; Wang, Chao-kai (Stylon) <Stylon.Wang at amd.com>; amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>
Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas at amd.com>
Subject: Re: [PATCH] drm/amd/display: Fix memory leaks in S3 resume

It's missing the "drm_connector_list_update" call which I assume is important.

Stylon, can you review Lee Starnes's patch? Is the drm_connector_list_update call maybe not needed?

Thanks,
Harry

On 2020-11-10 11:26 a.m., Deucher, Alexander wrote:

[AMD Official Use Only - Internal Distribution Only]

Lee Starnes just sent the exact same patch yesterday.  Please review that one:
https://patchwork.freedesktop.org/patch/399497/

Alex

________________________________
From: Wentland, Harry <Harry.Wentland at amd.com><mailto:Harry.Wentland at amd.com>
Sent: Tuesday, November 10, 2020 9:27 AM
To: Wang, Chao-kai (Stylon) <Stylon.Wang at amd.com><mailto:Stylon.Wang at amd.com>; amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org> <amd-gfx at lists.freedesktop.org><mailto:amd-gfx at lists.freedesktop.org>
Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas at amd.com><mailto:Nicholas.Kazlauskas at amd.com>; Deucher, Alexander <Alexander.Deucher at amd.com><mailto:Alexander.Deucher at amd.com>
Subject: Re: [PATCH] drm/amd/display: Fix memory leaks in S3 resume

On 2020-11-10 2:49 a.m., Stylon Wang wrote:
> EDID parsing in S3 resume pushes new display modes
> to probed_modes list but doesn't consolidate to actual
> mode list. This creates a race condition when
> amdgpu_dm_connector_ddc_get_modes() re-initializes the
> list head without walking the list and results in  memory leak.
>
> Signed-off-by: Stylon Wang <stylon.wang at amd.com><mailto:stylon.wang at amd.com>

Looks reasonable to me but haven't had a chance to understand whether
this is the best solution.

Acked-by: Harry Wentland <harry.wentland at amd.com><mailto:harry.wentland at amd.com>

Harry

> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> 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 0b6adf23d316..715e0bd489f8 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2337,7 +2337,8 @@ void amdgpu_dm_update_connector_after_detect(
>
>                        drm_connector_update_edid_property(connector,
>                                                           aconnector->edid);
> -                     drm_add_edid_modes(connector, aconnector->edid);
> +                     aconnector->num_modes = drm_add_edid_modes(connector, aconnector->edid);
> +                     drm_connector_list_update(connector);
>
>                        if (aconnector->dc_link->aux_mode)
>                                drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux,
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20201111/6452e3cc/attachment-0001.htm>