<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">Am 09.10.19 um 13:12 schrieb Yin, Tianci (Rico): </div> <blockquote type="cite" cite="mid:SN1PR12MB2544F1BE7D37DEC4721AF95D95950@SN1PR12MB2544.namprd12.prod.outlook.com"> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> <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> </blockquote> 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. <blockquote type="cite" cite="mid:SN1PR12MB2544F1BE7D37DEC4721AF95D95950@SN1PR12MB2544.namprd12.prod.outlook.com"> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Tuikov, Luben <a class="moz-txt-link-rfc2396E" href="mailto:Luben.Tuikov@amd.com"><Luben.Tuikov@amd.com></a> Sent: Wednesday, October 9, 2019 11:44 To: Alex Deucher <a class="moz-txt-link-rfc2396E" href="mailto:alexdeucher@gmail.com"><alexdeucher@gmail.com></a>; <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> <a class="moz-txt-link-rfc2396E" href="mailto:amd-gfx@lists.freedesktop.org"><amd-gfx@lists.freedesktop.org></a> Cc: Deucher, Alexander <a class="moz-txt-link-rfc2396E" href="mailto:Alexander.Deucher@amd.com"><Alexander.Deucher@amd.com></a>; Yin, Tianci (Rico) <a class="moz-txt-link-rfc2396E" href="mailto:Tianci.Yin@amd.com"><Tianci.Yin@amd.com></a> 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" <a class="moz-txt-link-rfc2396E" href="mailto:tianci.yin@amd.com"><tianci.yin@amd.com></a> > > memory training using specific fixed vram segment, reserve these > segments before anyone may allocate it. > > Change-Id: I1436755813a565608a2857a683f535377620a637 > Reviewed-by: Alex Deucher <a class="moz-txt-link-rfc2396E" href="mailto:alexander.deucher@amd.com"><alexander.deucher@amd.com></a> > Signed-off-by: Tianci.Yin <a class="moz-txt-link-rfc2396E" href="mailto:tianci.yin@amd.com"><tianci.yin@amd.com></a> > --- > 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> <fieldset class="mimeAttachmentHeader"></fieldset> <pre class="moz-quote-pre" wrap="">_______________________________________________ amd-gfx mailing list <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a></pre> </blockquote> </body> </html>