* [PATCH 1/2] drm/amdgpu: sdma v5_2 ring bo mem leak @ 2020-06-18 7:53 Wenhui Sheng 2020-06-18 7:53 ` [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 Wenhui Sheng 0 siblings, 1 reply; 9+ messages in thread From: Wenhui Sheng @ 2020-06-18 7:53 UTC (permalink / raw) To: amd-gfx; +Cc: Wenhui Sheng invoke amdgpu_ring_fini for each sdma instance Signed-off-by: Wenhui Sheng <Wenhui.Sheng@amd.com> --- drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c index b523091dcde8..74ac929dd217 100644 --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c @@ -1244,6 +1244,10 @@ static int sdma_v5_2_sw_init(void *handle) static int sdma_v5_2_sw_fini(void *handle) { struct amdgpu_device *adev = (struct amdgpu_device *)handle; + int i; + + for (i = 0; i < adev->sdma.num_instances; i++) + amdgpu_ring_fini(&adev->sdma.instance[i].ring); sdma_v5_2_destroy_inst_ctx(adev); -- 2.17.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 2020-06-18 7:53 [PATCH 1/2] drm/amdgpu: sdma v5_2 ring bo mem leak Wenhui Sheng @ 2020-06-18 7:53 ` Wenhui Sheng 2020-06-18 8:25 ` Wang, Kevin(Yang) ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Wenhui Sheng @ 2020-06-18 7:53 UTC (permalink / raw) To: amd-gfx; +Cc: Wenhui Sheng sdma v5_0 fw isn't released when module exit Signed-off-by: Wenhui Sheng <Wenhui.Sheng@amd.com> --- drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c index 58d2a80af450..6751ad69ed90 100644 --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c @@ -1299,8 +1299,12 @@ static int sdma_v5_0_sw_fini(void *handle) struct amdgpu_device *adev = (struct amdgpu_device *)handle; int i; - for (i = 0; i < adev->sdma.num_instances; i++) + for (i = 0; i < adev->sdma.num_instances; i++) { + if (adev->sdma.instance[i].fw != NULL) + release_firmware(adev->sdma.instance[i].fw); + amdgpu_ring_fini(&adev->sdma.instance[i].ring); + } return 0; } -- 2.17.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 2020-06-18 7:53 ` [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 Wenhui Sheng @ 2020-06-18 8:25 ` Wang, Kevin(Yang) 2020-06-18 8:31 ` Sheng, Wenhui 2020-06-19 8:29 ` Zhang, Hawking 2020-06-22 18:20 ` Christian König 2 siblings, 1 reply; 9+ messages in thread From: Wang, Kevin(Yang) @ 2020-06-18 8:25 UTC (permalink / raw) To: Sheng, Wenhui, amd-gfx [-- Attachment #1.1: Type: text/plain, Size: 1933 bytes --] [AMD Official Use Only - Internal Distribution Only] ________________________________ From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Wenhui Sheng <Wenhui.Sheng@amd.com> Sent: Thursday, June 18, 2020 3:53 PM To: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> Cc: Sheng, Wenhui <Wenhui.Sheng@amd.com> Subject: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 sdma v5_0 fw isn't released when module exit Signed-off-by: Wenhui Sheng <Wenhui.Sheng@amd.com> --- drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c index 58d2a80af450..6751ad69ed90 100644 --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c @@ -1299,8 +1299,12 @@ static int sdma_v5_0_sw_fini(void *handle) struct amdgpu_device *adev = (struct amdgpu_device *)handle; int i; - for (i = 0; i < adev->sdma.num_instances; i++) + for (i = 0; i < adev->sdma.num_instances; i++) { + if (adev->sdma.instance[i].fw != NULL) + release_firmware(adev->sdma.instance[i].fw); [kevin]: the kernel api "release_firmware()" will check argument (is NULL pointer). i think the patch don't need to double check it. + amdgpu_ring_fini(&adev->sdma.instance[i].ring); + } return 0; } -- 2.17.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7CKevin1.Wang%40amd.com%7Cd0b8b72e0be443a7343d08d8135cb093%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637280636699912801&sdata=HrKVAmRO0HKFZBRG6oWq3thNlGBd0T9bMramDU1ijys%3D&reserved=0 [-- Attachment #1.2: Type: text/html, Size: 4427 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 2020-06-18 8:25 ` Wang, Kevin(Yang) @ 2020-06-18 8:31 ` Sheng, Wenhui 2020-06-18 8:47 ` Wang, Kevin(Yang) 0 siblings, 1 reply; 9+ messages in thread From: Sheng, Wenhui @ 2020-06-18 8:31 UTC (permalink / raw) To: Wang, Kevin(Yang), amd-gfx [-- Attachment #1.1: Type: text/plain, Size: 2724 bytes --] [AMD Official Use Only - Internal Distribution Only] [AMD Official Use Only - Internal Distribution Only] Although we know that release_firmware has null check, but the code is not maintained by ourselves, so I think it's much more safe to add null check before invoke release_firmware. Brs Wenhui From: Wang, Kevin(Yang) <Kevin1.Wang@amd.com> Sent: Thursday, June 18, 2020 4:25 PM To: Sheng, Wenhui <Wenhui.Sheng@amd.com>; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 [AMD Official Use Only - Internal Distribution Only] ________________________________ From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org<mailto:amd-gfx-bounces@lists.freedesktop.org>> on behalf of Wenhui Sheng <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>> Sent: Thursday, June 18, 2020 3:53 PM To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>> Cc: Sheng, Wenhui <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>> Subject: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 sdma v5_0 fw isn't released when module exit Signed-off-by: Wenhui Sheng <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>> --- drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c index 58d2a80af450..6751ad69ed90 100644 --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c @@ -1299,8 +1299,12 @@ static int sdma_v5_0_sw_fini(void *handle) struct amdgpu_device *adev = (struct amdgpu_device *)handle; int i; - for (i = 0; i < adev->sdma.num_instances; i++) + for (i = 0; i < adev->sdma.num_instances; i++) { + if (adev->sdma.instance[i].fw != NULL) + release_firmware(adev->sdma.instance[i].fw); [kevin]: the kernel api "release_firmware()" will check argument (is NULL pointer). i think the patch don't need to double check it. + amdgpu_ring_fini(&adev->sdma.instance[i].ring); + } return 0; } -- 2.17.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7CKevin1.Wang%40amd.com%7Cd0b8b72e0be443a7343d08d8135cb093%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637280636699912801&sdata=HrKVAmRO0HKFZBRG6oWq3thNlGBd0T9bMramDU1ijys%3D&reserved=0 [-- Attachment #1.2: Type: text/html, Size: 7920 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 2020-06-18 8:31 ` Sheng, Wenhui @ 2020-06-18 8:47 ` Wang, Kevin(Yang) 2020-06-18 9:21 ` Sheng, Wenhui 0 siblings, 1 reply; 9+ messages in thread From: Wang, Kevin(Yang) @ 2020-06-18 8:47 UTC (permalink / raw) To: Sheng, Wenhui, amd-gfx [-- Attachment #1.1: Type: text/plain, Size: 3386 bytes --] [AMD Official Use Only - Internal Distribution Only] afte the API firmware_realease created, the api logic never changed. (the first commit by Linus) and you can use below command to reference the api usage case in our drm driver. $ grep -nR "release_firmware" -A 5 -B 5 drivers/gpu/drm/ | vim - Best Regards, Kevin ________________________________ From: Sheng, Wenhui <Wenhui.Sheng@amd.com> Sent: Thursday, June 18, 2020 4:31 PM To: Wang, Kevin(Yang) <Kevin1.Wang@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> Subject: RE: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 [AMD Official Use Only - Internal Distribution Only] [AMD Official Use Only - Internal Distribution Only] Although we know that release_firmware has null check, but the code is not maintained by ourselves, so I think it’s much more safe to add null check before invoke release_firmware. Brs Wenhui From: Wang, Kevin(Yang) <Kevin1.Wang@amd.com> Sent: Thursday, June 18, 2020 4:25 PM To: Sheng, Wenhui <Wenhui.Sheng@amd.com>; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 [AMD Official Use Only - Internal Distribution Only] ________________________________ From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org<mailto:amd-gfx-bounces@lists.freedesktop.org>> on behalf of Wenhui Sheng <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>> Sent: Thursday, June 18, 2020 3:53 PM To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>> Cc: Sheng, Wenhui <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>> Subject: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 sdma v5_0 fw isn't released when module exit Signed-off-by: Wenhui Sheng <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>> --- drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c index 58d2a80af450..6751ad69ed90 100644 --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c @@ -1299,8 +1299,12 @@ static int sdma_v5_0_sw_fini(void *handle) struct amdgpu_device *adev = (struct amdgpu_device *)handle; int i; - for (i = 0; i < adev->sdma.num_instances; i++) + for (i = 0; i < adev->sdma.num_instances; i++) { + if (adev->sdma.instance[i].fw != NULL) + release_firmware(adev->sdma.instance[i].fw); [kevin]: the kernel api "release_firmware()" will check argument (is NULL pointer). i think the patch don't need to double check it. + amdgpu_ring_fini(&adev->sdma.instance[i].ring); + } return 0; } -- 2.17.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7CKevin1.Wang%40amd.com%7Cd0b8b72e0be443a7343d08d8135cb093%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637280636699912801&sdata=HrKVAmRO0HKFZBRG6oWq3thNlGBd0T9bMramDU1ijys%3D&reserved=0 [-- Attachment #1.2: Type: text/html, Size: 9714 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 2020-06-18 8:47 ` Wang, Kevin(Yang) @ 2020-06-18 9:21 ` Sheng, Wenhui 2020-06-18 9:29 ` Wang, Kevin(Yang) 0 siblings, 1 reply; 9+ messages in thread From: Sheng, Wenhui @ 2020-06-18 9:21 UTC (permalink / raw) To: Wang, Kevin(Yang), amd-gfx [-- Attachment #1.1: Type: text/plain, Size: 4209 bytes --] [AMD Official Use Only - Internal Distribution Only] [AMD Official Use Only - Internal Distribution Only] Hmmmm.... 1. There are some places in amdgpu driver using release_firmware like this way 2. This case is a little different, we use it in loop and maybe not all sdma instances' fw is initialized, just like we did in v5_2 Brs Wenhui From: Wang, Kevin(Yang) <Kevin1.Wang@amd.com> Sent: Thursday, June 18, 2020 4:48 PM To: Sheng, Wenhui <Wenhui.Sheng@amd.com>; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 [AMD Official Use Only - Internal Distribution Only] afte the API firmware_realease created, the api logic never changed. (the first commit by Linus) and you can use below command to reference the api usage case in our drm driver. $ grep -nR "release_firmware" -A 5 -B 5 drivers/gpu/drm/ | vim - Best Regards, Kevin ________________________________ From: Sheng, Wenhui <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>> Sent: Thursday, June 18, 2020 4:31 PM To: Wang, Kevin(Yang) <Kevin1.Wang@amd.com<mailto:Kevin1.Wang@amd.com>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>> Subject: RE: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 [AMD Official Use Only - Internal Distribution Only] [AMD Official Use Only - Internal Distribution Only] Although we know that release_firmware has null check, but the code is not maintained by ourselves, so I think it's much more safe to add null check before invoke release_firmware. Brs Wenhui From: Wang, Kevin(Yang) <Kevin1.Wang@amd.com<mailto:Kevin1.Wang@amd.com>> Sent: Thursday, June 18, 2020 4:25 PM To: Sheng, Wenhui <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> Subject: Re: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 [AMD Official Use Only - Internal Distribution Only] ________________________________ From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org<mailto:amd-gfx-bounces@lists.freedesktop.org>> on behalf of Wenhui Sheng <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>> Sent: Thursday, June 18, 2020 3:53 PM To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>> Cc: Sheng, Wenhui <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>> Subject: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 sdma v5_0 fw isn't released when module exit Signed-off-by: Wenhui Sheng <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>> --- drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c index 58d2a80af450..6751ad69ed90 100644 --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c @@ -1299,8 +1299,12 @@ static int sdma_v5_0_sw_fini(void *handle) struct amdgpu_device *adev = (struct amdgpu_device *)handle; int i; - for (i = 0; i < adev->sdma.num_instances; i++) + for (i = 0; i < adev->sdma.num_instances; i++) { + if (adev->sdma.instance[i].fw != NULL) + release_firmware(adev->sdma.instance[i].fw); [kevin]: the kernel api "release_firmware()" will check argument (is NULL pointer). i think the patch don't need to double check it. + amdgpu_ring_fini(&adev->sdma.instance[i].ring); + } return 0; } -- 2.17.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7CKevin1.Wang%40amd.com%7Cd0b8b72e0be443a7343d08d8135cb093%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637280636699912801&sdata=HrKVAmRO0HKFZBRG6oWq3thNlGBd0T9bMramDU1ijys%3D&reserved=0 [-- Attachment #1.2: Type: text/html, Size: 14330 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 2020-06-18 9:21 ` Sheng, Wenhui @ 2020-06-18 9:29 ` Wang, Kevin(Yang) 0 siblings, 0 replies; 9+ messages in thread From: Wang, Kevin(Yang) @ 2020-06-18 9:29 UTC (permalink / raw) To: Sheng, Wenhui, amd-gfx [-- Attachment #1.1: Type: text/plain, Size: 6057 bytes --] [AMD Official Use Only - Internal Distribution Only] before this patch, the driver has same code logic at "drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c" related patch: drm/amdgpu: fix sdma3 ucode mem leak commit 14d83e78c578a6c45163fb399ee760fe0d314bad Author: Monk Liu <Monk.Liu@amd.com> Date: Mon May 30 15:15:32 2016 +0800 drm/amdgpu: fix sdma3 ucode mem leak Signed-off-by: Monk Liu <Monk.Liu@amd.com> Reviewed-by: Christian König <christian.koenig@amd.com> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c index 33605d4ac2d9..532ea88da66a 100644 --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c @@ -236,6 +236,15 @@ static void sdma_v3_0_init_golden_registers(struct amdgpu_device *adev) } } +static void sdma_v3_0_free_microcode(struct amdgpu_device *adev) +{ + int i; + for (i = 0; i < adev->sdma.num_instances; i++) { + release_firmware(adev->sdma.instance[i].fw); + adev->sdma.instance[i].fw = NULL; + } +} + /** * sdma_v3_0_init_microcode - load ucode images from disk * @@ -1256,6 +1265,7 @@ static int sdma_v3_0_sw_fini(void *handle) for (i = 0; i < adev->sdma.num_instances; i++) amdgpu_ring_fini(&adev->sdma.instance[i].ring); + sdma_v3_0_free_microcode(adev); return 0; } Best Regards, Kevin ________________________________ From: Sheng, Wenhui <Wenhui.Sheng@amd.com> Sent: Thursday, June 18, 2020 5:21 PM To: Wang, Kevin(Yang) <Kevin1.Wang@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> Subject: RE: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 [AMD Official Use Only - Internal Distribution Only] [AMD Official Use Only - Internal Distribution Only] Hmmmm…. 1. There are some places in amdgpu driver using release_firmware like this way 2. This case is a little different, we use it in loop and maybe not all sdma instances’ fw is initialized, just like we did in v5_2 Brs Wenhui From: Wang, Kevin(Yang) <Kevin1.Wang@amd.com> Sent: Thursday, June 18, 2020 4:48 PM To: Sheng, Wenhui <Wenhui.Sheng@amd.com>; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 [AMD Official Use Only - Internal Distribution Only] afte the API firmware_realease created, the api logic never changed. (the first commit by Linus) and you can use below command to reference the api usage case in our drm driver. $ grep -nR "release_firmware" -A 5 -B 5 drivers/gpu/drm/ | vim - Best Regards, Kevin ________________________________ From: Sheng, Wenhui <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>> Sent: Thursday, June 18, 2020 4:31 PM To: Wang, Kevin(Yang) <Kevin1.Wang@amd.com<mailto:Kevin1.Wang@amd.com>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>> Subject: RE: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 [AMD Official Use Only - Internal Distribution Only] [AMD Official Use Only - Internal Distribution Only] Although we know that release_firmware has null check, but the code is not maintained by ourselves, so I think it’s much more safe to add null check before invoke release_firmware. Brs Wenhui From: Wang, Kevin(Yang) <Kevin1.Wang@amd.com<mailto:Kevin1.Wang@amd.com>> Sent: Thursday, June 18, 2020 4:25 PM To: Sheng, Wenhui <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> Subject: Re: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 [AMD Official Use Only - Internal Distribution Only] ________________________________ From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org<mailto:amd-gfx-bounces@lists.freedesktop.org>> on behalf of Wenhui Sheng <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>> Sent: Thursday, June 18, 2020 3:53 PM To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>> Cc: Sheng, Wenhui <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>> Subject: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 sdma v5_0 fw isn't released when module exit Signed-off-by: Wenhui Sheng <Wenhui.Sheng@amd.com<mailto:Wenhui.Sheng@amd.com>> --- drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c index 58d2a80af450..6751ad69ed90 100644 --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c @@ -1299,8 +1299,12 @@ static int sdma_v5_0_sw_fini(void *handle) struct amdgpu_device *adev = (struct amdgpu_device *)handle; int i; - for (i = 0; i < adev->sdma.num_instances; i++) + for (i = 0; i < adev->sdma.num_instances; i++) { + if (adev->sdma.instance[i].fw != NULL) + release_firmware(adev->sdma.instance[i].fw); [kevin]: the kernel api "release_firmware()" will check argument (is NULL pointer). i think the patch don't need to double check it. + amdgpu_ring_fini(&adev->sdma.instance[i].ring); + } return 0; } -- 2.17.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7CKevin1.Wang%40amd.com%7Cd0b8b72e0be443a7343d08d8135cb093%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637280636699912801&sdata=HrKVAmRO0HKFZBRG6oWq3thNlGBd0T9bMramDU1ijys%3D&reserved=0 [-- Attachment #1.2: Type: text/html, Size: 17716 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 2020-06-18 7:53 ` [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 Wenhui Sheng 2020-06-18 8:25 ` Wang, Kevin(Yang) @ 2020-06-19 8:29 ` Zhang, Hawking 2020-06-22 18:20 ` Christian König 2 siblings, 0 replies; 9+ messages in thread From: Zhang, Hawking @ 2020-06-19 8:29 UTC (permalink / raw) To: Sheng, Wenhui, amd-gfx; +Cc: Sheng, Wenhui [AMD Public Use] Series is: Reviewed-by: Hawking Zhang <Hawking.Zhang@amd.com> Regards, Hawking -----Original Message----- From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Wenhui Sheng Sent: Thursday, June 18, 2020 15:53 To: amd-gfx@lists.freedesktop.org Cc: Sheng, Wenhui <Wenhui.Sheng@amd.com> Subject: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 sdma v5_0 fw isn't released when module exit Signed-off-by: Wenhui Sheng <Wenhui.Sheng@amd.com> --- drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c index 58d2a80af450..6751ad69ed90 100644 --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c @@ -1299,8 +1299,12 @@ static int sdma_v5_0_sw_fini(void *handle) struct amdgpu_device *adev = (struct amdgpu_device *)handle; int i; - for (i = 0; i < adev->sdma.num_instances; i++) + for (i = 0; i < adev->sdma.num_instances; i++) { + if (adev->sdma.instance[i].fw != NULL) + release_firmware(adev->sdma.instance[i].fw); + amdgpu_ring_fini(&adev->sdma.instance[i].ring); + } return 0; } -- 2.17.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Chawking.zhang%40amd.com%7Cd0b8b72e0be443a7343d08d8135cb093%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637280636689938431&sdata=2g%2BOLo5Hg6gxO2pF%2B2uDnshodSAmLaSh9RZQsKrnFUI%3D&reserved=0 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 2020-06-18 7:53 ` [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 Wenhui Sheng 2020-06-18 8:25 ` Wang, Kevin(Yang) 2020-06-19 8:29 ` Zhang, Hawking @ 2020-06-22 18:20 ` Christian König 2 siblings, 0 replies; 9+ messages in thread From: Christian König @ 2020-06-22 18:20 UTC (permalink / raw) To: Wenhui Sheng, amd-gfx Am 18.06.20 um 09:53 schrieb Wenhui Sheng: > sdma v5_0 fw isn't released when module exit > > Signed-off-by: Wenhui Sheng <Wenhui.Sheng@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c > index 58d2a80af450..6751ad69ed90 100644 > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c > @@ -1299,8 +1299,12 @@ static int sdma_v5_0_sw_fini(void *handle) > struct amdgpu_device *adev = (struct amdgpu_device *)handle; > int i; > > - for (i = 0; i < adev->sdma.num_instances; i++) > + for (i = 0; i < adev->sdma.num_instances; i++) { > + if (adev->sdma.instance[i].fw != NULL) > + release_firmware(adev->sdma.instance[i].fw); Please drop the extra NULL check here. Kernel APIs like kfree() etc.. are usually NULL tolerant and we have automated scripts complaining about this as bad coding style if you duplicate the check in the driver. Apart from that the series looks good to me. Regards, Christian. > + > amdgpu_ring_fini(&adev->sdma.instance[i].ring); > + } > > return 0; > } _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-06-22 18:20 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-18 7:53 [PATCH 1/2] drm/amdgpu: sdma v5_2 ring bo mem leak Wenhui Sheng 2020-06-18 7:53 ` [PATCH 2/2] drm/amdgpu: add fw release for sdma v5_0 Wenhui Sheng 2020-06-18 8:25 ` Wang, Kevin(Yang) 2020-06-18 8:31 ` Sheng, Wenhui 2020-06-18 8:47 ` Wang, Kevin(Yang) 2020-06-18 9:21 ` Sheng, Wenhui 2020-06-18 9:29 ` Wang, Kevin(Yang) 2020-06-19 8:29 ` Zhang, Hawking 2020-06-22 18:20 ` Christian König
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.