[PATCH 7/8] drm/amdgpu: reserve vram for memory training
From: "Tianci.Yin" memory training using specific fixed vram segment, reserve these segments before anyone may allocate it. Change-Id: I1436755813a565608a2857a683f535377620a637 Reviewed-by: Alex Deucher Signed-off-by: Tianci.Yin --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 96 + 1 file changed, 96 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 74a9bd94f10c..0819721af16e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -1667,6 +1667,93 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct amdgpu_device *adev) &adev->fw_vram_usage.va); } +/* + * Memoy training reservation functions + */ +/** + * amdgpu_ttm_training_reserve_vram_fini - free memory training reserved vram + * + * @adev: amdgpu_device pointer + * + * free memory training reserved vram if it has been reserved. + */ +static int amdgpu_ttm_training_reserve_vram_fini(struct amdgpu_device *adev) +{ + int ret = 0; + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; + + ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT; + if(ctx->c2p_bo) { + amdgpu_bo_free_kernel(&ctx->c2p_bo, NULL, NULL); + ctx->c2p_bo = NULL; + } + if(ctx->p2c_bo) { + amdgpu_bo_free_kernel(&ctx->p2c_bo, NULL, NULL); + ctx->p2c_bo = NULL; + } + + return ret; +} + +/** + * amdgpu_ttm_training_reserve_vram_init - create bo vram reservation from memory training + * + * @adev: amdgpu_device pointer + * + * create bo vram reservation from memory training. + */ +static int amdgpu_ttm_training_reserve_vram_init(struct amdgpu_device *adev) +{ + int ret = 0; + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; + + memset(ctx, 0, sizeof(*ctx)); + if(!adev->fw_vram_usage.mem_train_support) { + DRM_DEBUG("memory training does not support!\n"); + return 0; + } + + ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc; + ctx->p2c_train_data_offset = (adev->gmc.mc_vram_size - GDDR6_MEM_TRAINING_OFFSET); + ctx->train_data_size = GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES; + + DRM_DEBUG("train_data_size:%llx,p2c_train_data_offset:%llx,c2p_train_data_offset:%llx.\n", + ctx->train_data_size, + ctx->p2c_train_data_offset, + ctx->c2p_train_data_offset); + + ret = amdgpu_bo_create_kernel_at(adev, +ctx->p2c_train_data_offset, +ctx->train_data_size, +AMDGPU_GEM_DOMAIN_VRAM, +&ctx->p2c_bo, +NULL); + if(ret) { + DRM_ERROR("alloc p2c_bo failed(%d)!\n", ret); + ret = -ENOMEM; + goto err_out; + } + + ret = amdgpu_bo_create_kernel_at(adev, +ctx->c2p_train_data_offset, +ctx->train_data_size, +AMDGPU_GEM_DOMAIN_VRAM, +&ctx->c2p_bo, +NULL); + if(ret) { + DRM_ERROR("alloc c2p_bo failed(%d)!\n", ret); + ret = -ENOMEM; + goto err_out; + } + + ctx->init = PSP_MEM_TRAIN_RESERVE_SUCCESS; + return 0; + +err_out: + amdgpu_ttm_training_reserve_vram_fini(adev); + return ret; +} + /** * amdgpu_ttm_init - Init the memory management (ttm) as well as various * gtt/vram related fields. @@ -1740,6 +1827,14 @@ int amdgpu_ttm_init(struct amdgpu_device *adev) return r; } + /* +*The reserved vram for memory training must be pinned to the specified +*place on the VRAM, so reserve it early. +*/ + r = amdgpu_ttm_training_reserve_vram_init(adev); + if (r) + return r; + /* allocate memory as required for VGA * This is used for VGA emulation and pre-OS scanout buffers to * avoid display artifacts while transitioning between pre-OS @@ -1825,6 +1920,7 @@ void amdgpu_ttm_fini(struct amdgpu_device *adev) return; amdgpu_ttm_debugfs_fini(adev); + amdgpu_ttm_training_reserve_vram_fini(adev); amdgpu_ttm_fw_reserve_vram_fini(adev); if (adev->mman.aper_base_kaddr) iounmap(adev->mman.aper_base_kaddr); -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 7/8] drm/amdgpu: reserve vram for memory training
From: "Tianci.Yin" memory training using specific fixed vram segment, reserve these segments before anyone may allocate it. Change-Id: I1436755813a565608a2857a683f535377620a637 Reviewed-by: Alex Deucher Signed-off-by: Tianci.Yin --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 96 + 1 file changed, 96 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 9da6350a4ba2..42d0fcb98382 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -1667,6 +1667,93 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct amdgpu_device *adev) &adev->fw_vram_usage.va); } +/* + * Memoy training reservation functions + */ + +/** + * amdgpu_ttm_training_reserve_vram_fini - free memory training reserved vram + * + * @adev: amdgpu_device pointer + * + * free memory training reserved vram if it has been reserved. + */ +static int amdgpu_ttm_training_reserve_vram_fini(struct amdgpu_device *adev) +{ + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; + + ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT; + if (ctx->c2p_bo) { + amdgpu_bo_free_kernel(&ctx->c2p_bo, NULL, NULL); + ctx->c2p_bo = NULL; + } + if (ctx->p2c_bo) { + amdgpu_bo_free_kernel(&ctx->p2c_bo, NULL, NULL); + ctx->p2c_bo = NULL; + } + + return 0; +} + +/** + * amdgpu_ttm_training_reserve_vram_init - create bo vram reservation from memory training + * + * @adev: amdgpu_device pointer + * + * create bo vram reservation from memory training. + */ +static int amdgpu_ttm_training_reserve_vram_init(struct amdgpu_device *adev) +{ + int ret; + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; + + memset(ctx, 0, sizeof(*ctx)); + if (!adev->fw_vram_usage.mem_train_support) { + DRM_DEBUG("memory training does not support!\n"); + return 0; + } + + ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc; + ctx->p2c_train_data_offset = (adev->gmc.mc_vram_size - GDDR6_MEM_TRAINING_OFFSET); + ctx->train_data_size = GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES; + + DRM_DEBUG("train_data_size:%llx,p2c_train_data_offset:%llx,c2p_train_data_offset:%llx.\n", + ctx->train_data_size, + ctx->p2c_train_data_offset, + ctx->c2p_train_data_offset); + + ret = amdgpu_bo_create_kernel_at(adev, +ctx->p2c_train_data_offset, +ctx->train_data_size, +AMDGPU_GEM_DOMAIN_VRAM, +&ctx->p2c_bo, +NULL); + if (ret) { + DRM_ERROR("alloc p2c_bo failed(%d)!\n", ret); + ret = -ENOMEM; + goto err_out; + } + + ret = amdgpu_bo_create_kernel_at(adev, +ctx->c2p_train_data_offset, +ctx->train_data_size, +AMDGPU_GEM_DOMAIN_VRAM, +&ctx->c2p_bo, +NULL); + if (ret) { + DRM_ERROR("alloc c2p_bo failed(%d)!\n", ret); + ret = -ENOMEM; + goto err_out; + } + + ctx->init = PSP_MEM_TRAIN_RESERVE_SUCCESS; + return 0; + +err_out: + amdgpu_ttm_training_reserve_vram_fini(adev); + return ret; +} + /** * amdgpu_ttm_init - Init the memory management (ttm) as well as various * gtt/vram related fields. @@ -1740,6 +1827,14 @@ int amdgpu_ttm_init(struct amdgpu_device *adev) return r; } + /* +*The reserved vram for memory training must be pinned to the specified +*place on the VRAM, so reserve it early. +*/ + r = amdgpu_ttm_training_reserve_vram_init(adev); + if (r) + return r; + /* allocate memory as required for VGA * This is used for VGA emulation and pre-OS scanout buffers to * avoid display artifacts while transitioning between pre-OS @@ -1825,6 +1920,7 @@ void amdgpu_ttm_fini(struct amdgpu_device *adev) return; amdgpu_ttm_debugfs_fini(adev); + amdgpu_ttm_training_reserve_vram_fini(adev); amdgpu_ttm_fw_reserve_vram_fini(adev); if (adev->mman.aper_base_kaddr) iounmap(adev->mman.aper_base_kaddr); -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 7/8] drm/amdgpu: reserve vram for memory training(v3)
From: "Tianci.Yin" memory training using specific fixed vram segment, reserve these segments before anyone may allocate it. Change-Id: I1436755813a565608a2857a683f535377620a637 Reviewed-by: Alex Deucher Signed-off-by: Tianci.Yin --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 95 + 1 file changed, 95 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 2e85a5154f87..56782b3ed933 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -1667,6 +1667,92 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct amdgpu_device *adev) &adev->fw_vram_usage.va); } +/* + * Memoy training reservation functions + */ + +/** + * amdgpu_ttm_training_reserve_vram_fini - free memory training reserved vram + * + * @adev: amdgpu_device pointer + * + * free memory training reserved vram if it has been reserved. + */ +static int amdgpu_ttm_training_reserve_vram_fini(struct amdgpu_device *adev) +{ + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; + + ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT; + if (ctx->c2p_bo) { + amdgpu_bo_free_kernel(&ctx->c2p_bo, NULL, NULL); + ctx->c2p_bo = NULL; + } + + if (ctx->p2c_bo) { + amdgpu_bo_free_kernel(&ctx->p2c_bo, NULL, NULL); + ctx->p2c_bo = NULL; + } + + return 0; +} + +/** + * amdgpu_ttm_training_reserve_vram_init - create bo vram reservation from memory training + * + * @adev: amdgpu_device pointer + * + * create bo vram reservation from memory training. + */ +static int amdgpu_ttm_training_reserve_vram_init(struct amdgpu_device *adev) +{ + int ret; + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; + + memset(ctx, 0, sizeof(*ctx)); + if (!adev->fw_vram_usage.mem_train_support) { + DRM_DEBUG("memory training does not support!\n"); + return 0; + } + + ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc; + ctx->p2c_train_data_offset = (adev->gmc.mc_vram_size - GDDR6_MEM_TRAINING_OFFSET); + ctx->train_data_size = GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES; + + DRM_DEBUG("train_data_size:%llx,p2c_train_data_offset:%llx,c2p_train_data_offset:%llx.\n", + ctx->train_data_size, + ctx->p2c_train_data_offset, + ctx->c2p_train_data_offset); + + ret = amdgpu_bo_create_kernel_at(adev, +ctx->p2c_train_data_offset, +ctx->train_data_size, +AMDGPU_GEM_DOMAIN_VRAM, +&ctx->p2c_bo, +NULL); + if (ret) { + DRM_ERROR("alloc p2c_bo failed(%d)!\n", ret); + goto Err_out; + } + + ret = amdgpu_bo_create_kernel_at(adev, +ctx->c2p_train_data_offset, +ctx->train_data_size, +AMDGPU_GEM_DOMAIN_VRAM, +&ctx->c2p_bo, +NULL); + if (ret) { + DRM_ERROR("alloc c2p_bo failed(%d)!\n", ret); + goto Err_out; + } + + ctx->init = PSP_MEM_TRAIN_RESERVE_SUCCESS; + return 0; + +Err_out: + amdgpu_ttm_training_reserve_vram_fini(adev); + return ret; +} + /** * amdgpu_ttm_init - Init the memory management (ttm) as well as various * gtt/vram related fields. @@ -1740,6 +1826,14 @@ int amdgpu_ttm_init(struct amdgpu_device *adev) return r; } + /* +*The reserved vram for memory training must be pinned to the specified +*place on the VRAM, so reserve it early. +*/ + r = amdgpu_ttm_training_reserve_vram_init(adev); + if (r) + return r; + /* allocate memory as required for VGA * This is used for VGA emulation and pre-OS scanout buffers to * avoid display artifacts while transitioning between pre-OS @@ -1842,6 +1936,7 @@ void amdgpu_ttm_fini(struct amdgpu_device *adev) return; amdgpu_ttm_debugfs_fini(adev); + amdgpu_ttm_training_reserve_vram_fini(adev); amdgpu_ttm_fw_reserve_vram_fini(adev); if (adev->mman.aper_base_kaddr) iounmap(adev->mman.aper_base_kaddr); -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training
Am 12.10.19 um 01:23 schrieb Tuikov, Luben: > On 2019-10-10 11:50 p.m., Tianci Yin wrote: >> From: "Tianci.Yin" >> >> memory training using specific fixed vram segment, reserve these >> segments before anyone may allocate it. >> >> Change-Id: I1436755813a565608a2857a683f535377620a637 >> Reviewed-by: Alex Deucher >> Signed-off-by: Tianci.Yin >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 96 + >> 1 file changed, 96 insertions(+) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> index 9da6350a4ba2..42d0fcb98382 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> @@ -1667,6 +1667,93 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct >> amdgpu_device *adev) >>&adev->fw_vram_usage.va); >> } >> >> +/* >> + * Memoy training reservation functions >> + */ >> + >> +/** >> + * amdgpu_ttm_training_reserve_vram_fini - free memory training reserved >> vram >> + * >> + * @adev: amdgpu_device pointer >> + * >> + * free memory training reserved vram if it has been reserved. >> + */ >> +static int amdgpu_ttm_training_reserve_vram_fini(struct amdgpu_device *adev) >> +{ >> +struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; >> + >> +ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT; >> +if (ctx->c2p_bo) { >> +amdgpu_bo_free_kernel(&ctx->c2p_bo, NULL, NULL); >> +ctx->c2p_bo = NULL; >> +} > Generally it is a good idea to paragraph your code. > So an empty line between if-statements is a good idea. > However, there is no need in: > > ret = f(x); > if (ret) { > > } > > if (blah) { > > } > > The above are two (2) well-formed paragraphs. Additional to that amdgpu_bo_free_kernel() just like kfree() is NULL safe for the reason that you shouldn't need "if"s like that one. E.g. just write: amdgpu_bo_free_kernel(&ctx->c2p_bo...); and you are done. Regards, Christian. > >> +if (ctx->p2c_bo) { >> +amdgpu_bo_free_kernel(&ctx->p2c_bo, NULL, NULL); >> +ctx->p2c_bo = NULL; >> +} >> + >> +return 0; >> +} >> + >> +/** >> + * amdgpu_ttm_training_reserve_vram_init - create bo vram reservation from >> memory training >> + * >> + * @adev: amdgpu_device pointer >> + * >> + * create bo vram reservation from memory training. >> + */ >> +static int amdgpu_ttm_training_reserve_vram_init(struct amdgpu_device *adev) >> +{ >> +int ret; >> +struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; >> + >> +memset(ctx, 0, sizeof(*ctx)); >> +if (!adev->fw_vram_usage.mem_train_support) { >> +DRM_DEBUG("memory training does not support!\n"); >> +return 0; >> +} >> + >> +ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc; >> +ctx->p2c_train_data_offset = (adev->gmc.mc_vram_size - >> GDDR6_MEM_TRAINING_OFFSET); >> +ctx->train_data_size = GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES; >> + >> + >> DRM_DEBUG("train_data_size:%llx,p2c_train_data_offset:%llx,c2p_train_data_offset:%llx.\n", >> + ctx->train_data_size, >> + ctx->p2c_train_data_offset, >> + ctx->c2p_train_data_offset); >> + >> +ret = amdgpu_bo_create_kernel_at(adev, >> + ctx->p2c_train_data_offset, >> + ctx->train_data_size, >> + AMDGPU_GEM_DOMAIN_VRAM, >> + &ctx->p2c_bo, >> + NULL); >> +if (ret) { >> +DRM_ERROR("alloc p2c_bo failed(%d)!\n", ret); >> +ret = -ENOMEM; >> +goto err_out; >> +} > NAK! > Why are you re-writing the error code from "amdgpu_bo_create_kenrel_at()"? > Pass the error as is. > >> + >> +ret = amdgpu_bo_create_kernel_at(adev, >> + ctx->c2p_train_data_offset, >> + ctx->train_data_size, >> + AMDGPU_GEM_DOMAIN_VRAM, >> + &ctx->c2p_bo, >> + NULL); >> +if (ret) { >> +DRM_ERROR("alloc c2p_bo failed(%d)!\n", ret); >> +ret = -ENOMEM; >> +goto err_out; >> +} > NAK! > Why are you re-writing the error code from "amdgpu_bo_create_kenrel_at()"? > Pass the error as is. > >> + >> +ctx->init = PSP_MEM_TRAIN_RESERVE_SUCCESS; >> +return 0; >> + >> +err_out: > Yes... well "err_out" could be any identifier, including > a variable, as our variables follow snake-notation, all lowercase. > > Back at the turn of this century, Linux followed capitalized > goto labels to distinguish them from anything else around > in the kernel code: > > goto Bad_err; > ... > > return 0; > Bad_err: > return bad_gift; > } > > To distinguis
Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training
Thanks very much Christian! Rico From: Koenig, Christian Sent: Monday, October 14, 2019 16:26 To: Tuikov, Luben ; Yin, Tianci (Rico) ; amd-gfx@lists.freedesktop.org Cc: Deucher, Alexander Subject: Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training Am 12.10.19 um 01:23 schrieb Tuikov, Luben: > On 2019-10-10 11:50 p.m., Tianci Yin wrote: >> From: "Tianci.Yin" >> >> memory training using specific fixed vram segment, reserve these >> segments before anyone may allocate it. >> >> Change-Id: I1436755813a565608a2857a683f535377620a637 >> Reviewed-by: Alex Deucher >> Signed-off-by: Tianci.Yin >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 96 + >> 1 file changed, 96 insertions(+) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> index 9da6350a4ba2..42d0fcb98382 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> @@ -1667,6 +1667,93 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct >> amdgpu_device *adev) >> &adev->fw_vram_usage.va); >> } >> >> +/* >> + * Memoy training reservation functions >> + */ >> + >> +/** >> + * amdgpu_ttm_training_reserve_vram_fini - free memory training reserved >> vram >> + * >> + * @adev: amdgpu_device pointer >> + * >> + * free memory training reserved vram if it has been reserved. >> + */ >> +static int amdgpu_ttm_training_reserve_vram_fini(struct amdgpu_device *adev) >> +{ >> +struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; >> + >> +ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT; >> +if (ctx->c2p_bo) { >> +amdgpu_bo_free_kernel(&ctx->c2p_bo, NULL, NULL); >> +ctx->c2p_bo = NULL; >> +} > Generally it is a good idea to paragraph your code. > So an empty line between if-statements is a good idea. > However, there is no need in: > > ret = f(x); > if (ret) { > > } > > if (blah) { > > } > > The above are two (2) well-formed paragraphs. Additional to that amdgpu_bo_free_kernel() just like kfree() is NULL safe for the reason that you shouldn't need "if"s like that one. E.g. just write: amdgpu_bo_free_kernel(&ctx->c2p_bo...); and you are done. Regards, Christian. > >> +if (ctx->p2c_bo) { >> +amdgpu_bo_free_kernel(&ctx->p2c_bo, NULL, NULL); >> +ctx->p2c_bo = NULL; >> +} >> + >> +return 0; >> +} >> + >> +/** >> + * amdgpu_ttm_training_reserve_vram_init - create bo vram reservation from >> memory training >> + * >> + * @adev: amdgpu_device pointer >> + * >> + * create bo vram reservation from memory training. >> + */ >> +static int amdgpu_ttm_training_reserve_vram_init(struct amdgpu_device *adev) >> +{ >> +int ret; >> +struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; >> + >> +memset(ctx, 0, sizeof(*ctx)); >> +if (!adev->fw_vram_usage.mem_train_support) { >> +DRM_DEBUG("memory training does not support!\n"); >> +return 0; >> +} >> + >> +ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc; >> +ctx->p2c_train_data_offset = (adev->gmc.mc_vram_size - >> GDDR6_MEM_TRAINING_OFFSET); >> +ctx->train_data_size = GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES; >> + >> + >> DRM_DEBUG("train_data_size:%llx,p2c_train_data_offset:%llx,c2p_train_data_offset:%llx.\n", >> + ctx->train_data_size, >> + ctx->p2c_train_data_offset, >> + ctx->c2p_train_data_offset); >> + >> +ret = amdgpu_bo_create_kernel_at(adev, >> + ctx->p2c_train_data_offset, >> + ctx->train_data_size, >> + AMDGPU_GEM_DOMAIN_VRAM, >> + &ctx->p2c_bo, >> + NULL); >> +if (ret) { >> +DRM_ERROR("alloc p2c_bo failed(%d)!\n", ret); >> +ret = -ENOMEM; >> +goto err_out; >> +} > NAK! > Why are you re-writing the error code from "amdgpu_bo_create_kenrel_at()"? > Pass the error as is. > >> + >> +
Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training
On 2019-10-08 3:29 p.m., Alex Deucher wrote: > From: "Tianci.Yin" > > memory training using specific fixed vram segment, reserve these > segments before anyone may allocate it. > > Change-Id: I1436755813a565608a2857a683f535377620a637 > Reviewed-by: Alex Deucher > Signed-off-by: Tianci.Yin > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 96 + > 1 file changed, 96 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 74a9bd94f10c..0819721af16e 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -1667,6 +1667,93 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct > amdgpu_device *adev) > &adev->fw_vram_usage.va); > } > > +/* > + * Memoy training reservation functions > + */ Include an empty line between the two comments, just as you would a paragraph in written text. > +/** > + * amdgpu_ttm_training_reserve_vram_fini - free memory training reserved vram > + * > + * @adev: amdgpu_device pointer > + * > + * free memory training reserved vram if it has been reserved. > + */ > +static int amdgpu_ttm_training_reserve_vram_fini(struct amdgpu_device *adev) > +{ > + int ret = 0; > + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; > + > + ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT; > + if(ctx->c2p_bo) { Space after keywords, according to LKCS: if (...) > + amdgpu_bo_free_kernel(&ctx->c2p_bo, NULL, NULL); > + ctx->c2p_bo = NULL; > + } > + if(ctx->p2c_bo) { Space after keywords, according to LKCS: if (...) > + amdgpu_bo_free_kernel(&ctx->p2c_bo, NULL, NULL); > + ctx->p2c_bo = NULL; > + } > + > + return ret; > +} Get rid of "ret" variable altogether as it is not used in this function. > + > +/** > + * amdgpu_ttm_training_reserve_vram_init - create bo vram reservation from > memory training > + * > + * @adev: amdgpu_device pointer > + * > + * create bo vram reservation from memory training. > + */ > +static int amdgpu_ttm_training_reserve_vram_init(struct amdgpu_device *adev) > +{ > + int ret = 0; DO NOT preinitialize ret. > + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; > + > + memset(ctx, 0, sizeof(*ctx)); > + if(!adev->fw_vram_usage.mem_train_support) { Space after keywords: "if (". > + DRM_DEBUG("memory training does not support!\n"); > + return 0; > + } > + > + ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc; > + ctx->p2c_train_data_offset = (adev->gmc.mc_vram_size - > GDDR6_MEM_TRAINING_OFFSET); > + ctx->train_data_size = GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES; > + > + > DRM_DEBUG("train_data_size:%llx,p2c_train_data_offset:%llx,c2p_train_data_offset:%llx.\n", > + ctx->train_data_size, > + ctx->p2c_train_data_offset, > + ctx->c2p_train_data_offset); > + > + ret = amdgpu_bo_create_kernel_at(adev, Here is where you definitively set "ret" so DO NOT preinitialize it to 0, just to avoid "pesky compiler unininitialized variable warnings"--those are helpful to make the code more secure: a variable should be intentionally initialized in all paths. > + ctx->p2c_train_data_offset, > + ctx->train_data_size, > + AMDGPU_GEM_DOMAIN_VRAM, > + &ctx->p2c_bo, > + NULL); > + if(ret) { Space after keywords "if (". > + DRM_ERROR("alloc p2c_bo failed(%d)!\n", ret); > + ret = -ENOMEM; > + goto err_out; > + } > + > + ret = amdgpu_bo_create_kernel_at(adev, > + ctx->c2p_train_data_offset, > + ctx->train_data_size, > + AMDGPU_GEM_DOMAIN_VRAM, > + &ctx->c2p_bo, > + NULL); > + if(ret) { Space after keywords: "if (", according to LKCS. Regards, Luben > + DRM_ERROR("alloc c2p_bo failed(%d)!\n", ret); > + ret = -ENOMEM; > + goto err_out; > + } > + > + ctx->init = PSP_MEM_TRAIN_RESERVE_SUCCESS; > + return 0; > + > +err_out: > + amdgpu_ttm_training_reserve_vram_fini(adev); > + return ret; > +} > + > /** > * amdgpu_ttm_init - Init the memory management (ttm) as well as various > * gtt/vram related fields. > @@ -1740,6 +1827,14 @@ int amdgpu_ttm_init(struct amdgpu_device *adev) > return r; > } > > + /* > + *The reserved vram for memory training must be pinned to the specified > + *place on the VRAM, so reserve it early. > + */ > + r = amdgpu_ttm_training_reserve_vram_init(ade
Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training
Here is where you definitively set "ret" so DO NOT preinitialize it to 0, just to avoid "pesky compiler unininitialized variable warnings"--those are helpful to make the code more secure: a variable should be intentionally initialized in all paths. Rico: Still in confusion, pre-initialization can avoid "uninitialized variable", why should we can't pre-initialize? From: Tuikov, Luben Sent: Wednesday, October 9, 2019 11:44 To: Alex Deucher ; amd-gfx@lists.freedesktop.org Cc: Deucher, Alexander ; Yin, Tianci (Rico) Subject: Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training On 2019-10-08 3:29 p.m., Alex Deucher wrote: > From: "Tianci.Yin" > > memory training using specific fixed vram segment, reserve these > segments before anyone may allocate it. > > Change-Id: I1436755813a565608a2857a683f535377620a637 > Reviewed-by: Alex Deucher > Signed-off-by: Tianci.Yin > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 96 + > 1 file changed, 96 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 74a9bd94f10c..0819721af16e 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -1667,6 +1667,93 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct > amdgpu_device *adev) > &adev->fw_vram_usage.va); > } > > +/* > + * Memoy training reservation functions > + */ Include an empty line between the two comments, just as you would a paragraph in written text. > +/** > + * amdgpu_ttm_training_reserve_vram_fini - free memory training reserved vram > + * > + * @adev: amdgpu_device pointer > + * > + * free memory training reserved vram if it has been reserved. > + */ > +static int amdgpu_ttm_training_reserve_vram_fini(struct amdgpu_device *adev) > +{ > + int ret = 0; > + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; > + > + ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT; > + if(ctx->c2p_bo) { Space after keywords, according to LKCS: if (...) > + amdgpu_bo_free_kernel(&ctx->c2p_bo, NULL, NULL); > + ctx->c2p_bo = NULL; > + } > + if(ctx->p2c_bo) { Space after keywords, according to LKCS: if (...) > + amdgpu_bo_free_kernel(&ctx->p2c_bo, NULL, NULL); > + ctx->p2c_bo = NULL; > + } > + > + return ret; > +} Get rid of "ret" variable altogether as it is not used in this function. > + > +/** > + * amdgpu_ttm_training_reserve_vram_init - create bo vram reservation from > memory training > + * > + * @adev: amdgpu_device pointer > + * > + * create bo vram reservation from memory training. > + */ > +static int amdgpu_ttm_training_reserve_vram_init(struct amdgpu_device *adev) > +{ > + int ret = 0; DO NOT preinitialize ret. > + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; > + > + memset(ctx, 0, sizeof(*ctx)); > + if(!adev->fw_vram_usage.mem_train_support) { Space after keywords: "if (". > + DRM_DEBUG("memory training does not support!\n"); > + return 0; > + } > + > + ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc; > + ctx->p2c_train_data_offset = (adev->gmc.mc_vram_size - > GDDR6_MEM_TRAINING_OFFSET); > + ctx->train_data_size = GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES; > + > + > DRM_DEBUG("train_data_size:%llx,p2c_train_data_offset:%llx,c2p_train_data_offset:%llx.\n", > + ctx->train_data_size, > + ctx->p2c_train_data_offset, > + ctx->c2p_train_data_offset); > + > + ret = amdgpu_bo_create_kernel_at(adev, Here is where you definitively set "ret" so DO NOT preinitialize it to 0, just to avoid "pesky compiler unininitialized variable warnings"--those are helpful to make the code more secure: a variable should be intentionally initialized in all paths. > + ctx->p2c_train_data_offset, > + ctx->train_data_size, > + AMDGPU_GEM_DOMAIN_VRAM, > + &ctx->p2c_bo, > + NULL); > + if(ret) { Space after keywords "if (". > + DRM_ERROR("alloc p2c_bo failed(%d)!\n", ret); > + ret = -ENOMEM; > + goto err_out; > + } > + > + ret = amdgpu_bo_creat
Recall: [PATCH 7/8] drm/amdgpu: reserve vram for memory training
Yin, Tianci (Rico) would like to recall the message, "[PATCH 7/8] drm/amdgpu: reserve vram for memory training". ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Recall: [PATCH 7/8] drm/amdgpu: reserve vram for memory training
Yin, Tianci (Rico) would like to recall the message, "[PATCH 7/8] drm/amdgpu: reserve vram for memory training". ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training
Am 09.10.19 um 13:12 schrieb Yin, Tianci (Rico): Here is where you definitively set "ret" so DO NOT preinitialize it to 0, just to avoid "pesky compiler unininitialized variable warnings"--those are helpful to make the code more secure: a variable should be intentionally initialized in all paths. Rico: Still in confusion, pre-initialization can avoid "uninitialized variable", why should we can't pre-initialize? Because it avoids the uninitialized variable warning :) See we really want the warning because it means that we have a bug in the code somewhere. Regards, Christian. *From:* Tuikov, Luben *Sent:* Wednesday, October 9, 2019 11:44 *To:* Alex Deucher ; amd-gfx@lists.freedesktop.org *Cc:* Deucher, Alexander ; Yin, Tianci (Rico) *Subject:* Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training On 2019-10-08 3:29 p.m., Alex Deucher wrote: > From: "Tianci.Yin" > > memory training using specific fixed vram segment, reserve these > segments before anyone may allocate it. > > Change-Id: I1436755813a565608a2857a683f535377620a637 > Reviewed-by: Alex Deucher > Signed-off-by: Tianci.Yin > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 96 + > 1 file changed, 96 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 74a9bd94f10c..0819721af16e 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -1667,6 +1667,93 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct amdgpu_device *adev) > &adev->fw_vram_usage.va); > } > > +/* > + * Memoy training reservation functions > + */ Include an empty line between the two comments, just as you would a paragraph in written text. > +/** > + * amdgpu_ttm_training_reserve_vram_fini - free memory training reserved vram > + * > + * @adev: amdgpu_device pointer > + * > + * free memory training reserved vram if it has been reserved. > + */ > +static int amdgpu_ttm_training_reserve_vram_fini(struct amdgpu_device *adev) > +{ > + int ret = 0; > + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; > + > + ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT; > + if(ctx->c2p_bo) { Space after keywords, according to LKCS: if (...) > + amdgpu_bo_free_kernel(&ctx->c2p_bo, NULL, NULL); > + ctx->c2p_bo = NULL; > + } > + if(ctx->p2c_bo) { Space after keywords, according to LKCS: if (...) > + amdgpu_bo_free_kernel(&ctx->p2c_bo, NULL, NULL); > + ctx->p2c_bo = NULL; > + } > + > + return ret; > +} Get rid of "ret" variable altogether as it is not used in this function. > + > +/** > + * amdgpu_ttm_training_reserve_vram_init - create bo vram reservation from memory training > + * > + * @adev: amdgpu_device pointer > + * > + * create bo vram reservation from memory training. > + */ > +static int amdgpu_ttm_training_reserve_vram_init(struct amdgpu_device *adev) > +{ > + int ret = 0; DO NOT preinitialize ret. > + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; > + > + memset(ctx, 0, sizeof(*ctx)); > + if(!adev->fw_vram_usage.mem_train_support) { Space after keywords: "if (". > + DRM_DEBUG("memory training does not support!\n"); > + return 0; > + } > + > + ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc; > + ctx->p2c_train_data_offset = (adev->gmc.mc_vram_size - GDDR6_MEM_TRAINING_OFFSET); > + ctx->train_data_size = GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES; > + > + DRM_DEBUG("train_data_size:%llx,p2c_train_data_offset:%llx,c2p_train_data_offset:%llx.\n", > + ctx->train_data_size, > + ctx->p2c_train_data_offset, > + ctx->c2p_train_data_offset); > + > + ret = amdgpu_bo_create_kernel_at(adev, Here is where you definitively set "ret" so DO NOT preinitialize it to 0, just to avoid "pesky compiler unininitialized variable warnings"--those are helpful to make the code more secure: a variable should be intentionally initialized in all paths. > + ctx->p2c_train_data_offset, > + ctx->train_data_size, > + AMDGPU_GEM_DOMAIN_VRAM, > + &ctx->p2c_bo, > + NULL); > + if(ret) { Space after keywords "if (". > + DRM_ERROR("alloc p2c_bo failed(%d)!\n", ret); > + ret = -ENOMEM; > + goto err_out; > + } &
Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training
Oh, I see, I thought we should eliminate warning, but it's a wrong idea, actually we need it. Thanks! Rico From: Christian K?nig Sent: Wednesday, October 9, 2019 19:40 To: Yin, Tianci (Rico) ; Tuikov, Luben ; Alex Deucher ; amd-gfx@lists.freedesktop.org Cc: Deucher, Alexander Subject: Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training Am 09.10.19 um 13:12 schrieb Yin, Tianci (Rico): Here is where you definitively set "ret" so DO NOT preinitialize it to 0, just to avoid "pesky compiler unininitialized variable warnings"--those are helpful to make the code more secure: a variable should be intentionally initialized in all paths. Rico: Still in confusion, pre-initialization can avoid "uninitialized variable", why should we can't pre-initialize? Because it avoids the uninitialized variable warning :) See we really want the warning because it means that we have a bug in the code somewhere. Regards, Christian. From: Tuikov, Luben <mailto:luben.tui...@amd.com> Sent: Wednesday, October 9, 2019 11:44 To: Alex Deucher <mailto:alexdeuc...@gmail.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <mailto:amd-gfx@lists.freedesktop.org> Cc: Deucher, Alexander <mailto:alexander.deuc...@amd.com>; Yin, Tianci (Rico) <mailto:tianci....@amd.com> Subject: Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training On 2019-10-08 3:29 p.m., Alex Deucher wrote: > From: "Tianci.Yin" <mailto:tianci@amd.com> > > memory training using specific fixed vram segment, reserve these > segments before anyone may allocate it. > > Change-Id: I1436755813a565608a2857a683f535377620a637 > Reviewed-by: Alex Deucher > <mailto:alexander.deuc...@amd.com> > Signed-off-by: Tianci.Yin <mailto:tianci@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 96 + > 1 file changed, 96 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 74a9bd94f10c..0819721af16e 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -1667,6 +1667,93 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct > amdgpu_device *adev) > &adev->fw_vram_usage.va); > } > > +/* > + * Memoy training reservation functions > + */ Include an empty line between the two comments, just as you would a paragraph in written text. > +/** > + * amdgpu_ttm_training_reserve_vram_fini - free memory training reserved vram > + * > + * @adev: amdgpu_device pointer > + * > + * free memory training reserved vram if it has been reserved. > + */ > +static int amdgpu_ttm_training_reserve_vram_fini(struct amdgpu_device *adev) > +{ > + int ret = 0; > + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; > + > + ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT; > + if(ctx->c2p_bo) { Space after keywords, according to LKCS: if (...) > + amdgpu_bo_free_kernel(&ctx->c2p_bo, NULL, NULL); > + ctx->c2p_bo = NULL; > + } > + if(ctx->p2c_bo) { Space after keywords, according to LKCS: if (...) > + amdgpu_bo_free_kernel(&ctx->p2c_bo, NULL, NULL); > + ctx->p2c_bo = NULL; > + } > + > + return ret; > +} Get rid of "ret" variable altogether as it is not used in this function. > + > +/** > + * amdgpu_ttm_training_reserve_vram_init - create bo vram reservation from > memory training > + * > + * @adev: amdgpu_device pointer > + * > + * create bo vram reservation from memory training. > + */ > +static int amdgpu_ttm_training_reserve_vram_init(struct amdgpu_device *adev) > +{ > + int ret = 0; DO NOT preinitialize ret. > + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; > + > + memset(ctx, 0, sizeof(*ctx)); > + if(!adev->fw_vram_usage.mem_train_support) { Space after keywords: "if (". > + DRM_DEBUG("memory training does not support!\n"); > + return 0; > + } > + > + ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc; > + ctx->p2c_train_data_offset = (adev->gmc.mc_vram_size - > GDDR6_MEM_TRAINING_OFFSET); > + ctx->train_data_size = GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES; > + > + > DRM_DEBUG("train_data_size:%llx,p2c_train_data_offset:%llx,c2p_train_data_offset:%llx.\n", > + ctx->train_data_size, > + ctx->p2c_train_data_offset, > + ctx
Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training
On 2019-10-10 11:50 p.m., Tianci Yin wrote: > From: "Tianci.Yin" > > memory training using specific fixed vram segment, reserve these > segments before anyone may allocate it. > > Change-Id: I1436755813a565608a2857a683f535377620a637 > Reviewed-by: Alex Deucher > Signed-off-by: Tianci.Yin > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 96 + > 1 file changed, 96 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 9da6350a4ba2..42d0fcb98382 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -1667,6 +1667,93 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct > amdgpu_device *adev) > &adev->fw_vram_usage.va); > } > > +/* > + * Memoy training reservation functions > + */ > + > +/** > + * amdgpu_ttm_training_reserve_vram_fini - free memory training reserved vram > + * > + * @adev: amdgpu_device pointer > + * > + * free memory training reserved vram if it has been reserved. > + */ > +static int amdgpu_ttm_training_reserve_vram_fini(struct amdgpu_device *adev) > +{ > + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; > + > + ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT; > + if (ctx->c2p_bo) { > + amdgpu_bo_free_kernel(&ctx->c2p_bo, NULL, NULL); > + ctx->c2p_bo = NULL; > + } Generally it is a good idea to paragraph your code. So an empty line between if-statements is a good idea. However, there is no need in: ret = f(x); if (ret) { } if (blah) { } The above are two (2) well-formed paragraphs. > + if (ctx->p2c_bo) { > + amdgpu_bo_free_kernel(&ctx->p2c_bo, NULL, NULL); > + ctx->p2c_bo = NULL; > + } > + > + return 0; > +} > + > +/** > + * amdgpu_ttm_training_reserve_vram_init - create bo vram reservation from > memory training > + * > + * @adev: amdgpu_device pointer > + * > + * create bo vram reservation from memory training. > + */ > +static int amdgpu_ttm_training_reserve_vram_init(struct amdgpu_device *adev) > +{ > + int ret; > + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; > + > + memset(ctx, 0, sizeof(*ctx)); > + if (!adev->fw_vram_usage.mem_train_support) { > + DRM_DEBUG("memory training does not support!\n"); > + return 0; > + } > + > + ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc; > + ctx->p2c_train_data_offset = (adev->gmc.mc_vram_size - > GDDR6_MEM_TRAINING_OFFSET); > + ctx->train_data_size = GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES; > + > + > DRM_DEBUG("train_data_size:%llx,p2c_train_data_offset:%llx,c2p_train_data_offset:%llx.\n", > + ctx->train_data_size, > + ctx->p2c_train_data_offset, > + ctx->c2p_train_data_offset); > + > + ret = amdgpu_bo_create_kernel_at(adev, > + ctx->p2c_train_data_offset, > + ctx->train_data_size, > + AMDGPU_GEM_DOMAIN_VRAM, > + &ctx->p2c_bo, > + NULL); > + if (ret) { > + DRM_ERROR("alloc p2c_bo failed(%d)!\n", ret); > + ret = -ENOMEM; > + goto err_out; > + } NAK! Why are you re-writing the error code from "amdgpu_bo_create_kenrel_at()"? Pass the error as is. > + > + ret = amdgpu_bo_create_kernel_at(adev, > + ctx->c2p_train_data_offset, > + ctx->train_data_size, > + AMDGPU_GEM_DOMAIN_VRAM, > + &ctx->c2p_bo, > + NULL); > + if (ret) { > + DRM_ERROR("alloc c2p_bo failed(%d)!\n", ret); > + ret = -ENOMEM; > + goto err_out; > + } NAK! Why are you re-writing the error code from "amdgpu_bo_create_kenrel_at()"? Pass the error as is. > + > + ctx->init = PSP_MEM_TRAIN_RESERVE_SUCCESS; > + return 0; > + > +err_out: Yes... well "err_out" could be any identifier, including a variable, as our variables follow snake-notation, all lowercase. Back at the turn of this century, Linux followed capitalized goto labels to distinguish them from anything else around in the kernel code: goto Bad_err; ... return 0; Bad_err: return bad_gift; } To distinguish that a capitalized identifier is a goto label, "Bad_err" and all lower-case label is just another variable or function identifier, "bad_gift". Regards, Luben > + amdgpu_ttm_training_reserve_vram_fini(adev); > + return ret; > +} > + > /** > * amdgpu_ttm_init - Init the memory management (ttm) as well as various > * gtt/vram related fields. > @@ -1740,6 +1827,14 @@ int amdgpu_ttm
Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training
On Fri, Oct 11, 2019 at 7:23 PM Tuikov, Luben wrote: > > On 2019-10-10 11:50 p.m., Tianci Yin wrote: > > From: "Tianci.Yin" > > > > memory training using specific fixed vram segment, reserve these > > segments before anyone may allocate it. > > > > Change-Id: I1436755813a565608a2857a683f535377620a637 > > Reviewed-by: Alex Deucher > > Signed-off-by: Tianci.Yin > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 96 + > > 1 file changed, 96 insertions(+) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > > index 9da6350a4ba2..42d0fcb98382 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > > @@ -1667,6 +1667,93 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct > > amdgpu_device *adev) > > &adev->fw_vram_usage.va); > > } > > > > +/* > > + * Memoy training reservation functions > > + */ > > + > > +/** > > + * amdgpu_ttm_training_reserve_vram_fini - free memory training reserved > > vram > > + * > > + * @adev: amdgpu_device pointer > > + * > > + * free memory training reserved vram if it has been reserved. > > + */ > > +static int amdgpu_ttm_training_reserve_vram_fini(struct amdgpu_device > > *adev) > > +{ > > + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; > > + > > + ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT; > > + if (ctx->c2p_bo) { > > + amdgpu_bo_free_kernel(&ctx->c2p_bo, NULL, NULL); > > + ctx->c2p_bo = NULL; > > + } > > Generally it is a good idea to paragraph your code. > So an empty line between if-statements is a good idea. > However, there is no need in: > > ret = f(x); > if (ret) { > > } > > if (blah) { > > } > > The above are two (2) well-formed paragraphs. > > > + if (ctx->p2c_bo) { > > + amdgpu_bo_free_kernel(&ctx->p2c_bo, NULL, NULL); > > + ctx->p2c_bo = NULL; > > + } > > + > > + return 0; > > +} > > + > > +/** > > + * amdgpu_ttm_training_reserve_vram_init - create bo vram reservation from > > memory training > > + * > > + * @adev: amdgpu_device pointer > > + * > > + * create bo vram reservation from memory training. > > + */ > > +static int amdgpu_ttm_training_reserve_vram_init(struct amdgpu_device > > *adev) > > +{ > > + int ret; > > + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; > > + > > + memset(ctx, 0, sizeof(*ctx)); > > + if (!adev->fw_vram_usage.mem_train_support) { > > + DRM_DEBUG("memory training does not support!\n"); > > + return 0; > > + } > > + > > + ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc; > > + ctx->p2c_train_data_offset = (adev->gmc.mc_vram_size - > > GDDR6_MEM_TRAINING_OFFSET); > > + ctx->train_data_size = GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES; > > + > > + > > DRM_DEBUG("train_data_size:%llx,p2c_train_data_offset:%llx,c2p_train_data_offset:%llx.\n", > > + ctx->train_data_size, > > + ctx->p2c_train_data_offset, > > + ctx->c2p_train_data_offset); > > + > > + ret = amdgpu_bo_create_kernel_at(adev, > > + ctx->p2c_train_data_offset, > > + ctx->train_data_size, > > + AMDGPU_GEM_DOMAIN_VRAM, > > + &ctx->p2c_bo, > > + NULL); > > + if (ret) { > > + DRM_ERROR("alloc p2c_bo failed(%d)!\n", ret); > > + ret = -ENOMEM; > > + goto err_out; > > + } > > NAK! > Why are you re-writing the error code from "amdgpu_bo_create_kenrel_at()"? > Pass the error as is. > > > + > > + ret = amdgpu_bo_create_kernel_at(adev, > > + ctx->c2p_train_data_offset, > > + ctx->train_data_size, > > + AMDGPU_GEM_DOMAIN_VRAM, > > + &ctx->c2p_bo, > > + NULL); > > + if (ret) { > > + DRM_ERROR("alloc c2p_bo failed(%d)!\n", ret); > > + ret = -ENOMEM; > > + goto err_out; > > + } > > NAK! > Why are you re-writing the error code from "amdgpu_bo_create_kenrel_at()"? > Pass the error as is. > > > + > > + ctx->init = PSP_MEM_TRAIN_RESERVE_SUCCESS; > > + return 0; > > + > > +err_out: > > Yes... well "err_out" could be any identifier, including > a variable, as our variables follow snake-notation, all lowercase. > > Back at the turn of this century, Linux followed capitalized > goto labels to distinguish them from anything else around > in the kernel code: > > goto Bad_err; > ... > > return 0; > Bad_err: > return bad_gift; > } > > To distinguish that a capitalized identifier is a goto