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

["Wang, Chao-kai (Stylon)" <Stylon.Wang@xxxxxxx>]

[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 |  Twitter |  amd.com 

 

---

From: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
Sent: November 11, 2020 12:55 AM
To: Wentland, Harry <Harry.Wentland@xxxxxxx>; Wang, Chao-kai (Stylon) <Stylon.Wang@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>
Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@xxxxxxx>
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@xxxxxxx>
Sent: Tuesday, November 10, 2020 11:42 AM
To: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Wang, Chao-kai (Stylon) <Stylon.Wang@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>
Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@xxxxxxx>
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@xxxxxxx>
Sent: Tuesday, November 10, 2020 9:27 AM
To: Wang, Chao-kai (Stylon) <Stylon.Wang@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>
Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@xxxxxxx>; Deucher, Alexander <Alexander.Deucher@xxxxxxx>
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@xxxxxxx>

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

Acked-by: Harry Wentland <harry.wentland@xxxxxxx>

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

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx