<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Thanks very much Christian!</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Rico </div> <div id="appendonsend"></div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Koenig, Christian <Christian.Koenig@amd.com> Sent: Monday, October 14, 2019 16:26 To: Tuikov, Luben <Luben.Tuikov@amd.com>; Yin, Tianci (Rico) <Tianci.Yin@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> Cc: Deucher, Alexander <Alexander.Deucher@amd.com> Subject: Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">Am 12.10.19 um 01:23 schrieb Tuikov, Luben: > On 2019-10-10 11:50 p.m., Tianci Yin wrote: >> From: "Tianci.Yin" <tianci.yin@amd.com> >> >> memory training using specific fixed vram segment, reserve these >> segments before anyone may allocate it. >> >> Change-Id: I1436755813a565608a2857a683f535377620a637 >> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> >> Signed-off-by: Tianci.Yin <tianci.yin@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 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) { > <body of code> > } > > if (blah) { > <body of code> > } > > 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 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_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); >> </div> </div> </body> </html>