<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);"> Here is where you definitively set "ret" so DO NOT preinitialize it to 0, </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 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.</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: Still in confusion, pre-initialization can avoid "uninitialized variable", why should we can't pre-initialize? </div> <div id="appendonsend"></div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Tuikov, Luben <Luben.Tuikov@amd.com> Sent: Wednesday, October 9, 2019 11:44 To: Alex Deucher <alexdeucher@gmail.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Yin, Tianci (Rico) <Tianci.Yin@amd.com> Subject: Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">On 2019-10-08 3:29 p.m., Alex Deucher 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 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(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>