<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">
<p style="font-family:Arial;font-size:10pt;color:#0078D7;margin:15pt;" align="Left">
[AMD Official Use Only - Internal Distribution Only]<br>
</p>
<br>
<div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
Hi Luben,</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
Thank you very much for your suggestion and detailed explanation!</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
Patch has been refined, please help review.</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
Rico<br>
</div>
<div id="appendonsend"></div>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> Tuikov, Luben <Luben.Tuikov@amd.com><br>
<b>Sent:</b> Thursday, December 19, 2019 4:14<br>
<b>To:</b> Yin, Tianci (Rico) <Tianci.Yin@amd.com>; Wang, Kevin(Yang) <Kevin1.Wang@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org><br>
<b>Cc:</b> Koenig, Christian <Christian.Koenig@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Yuan, Xiaojie <Xiaojie.Yuan@amd.com>; Long, Gang <Gang.Long@amd.com><br>
<b>Subject:</b> Re: [PATCH 1/2] drm/amdgpu: update the method to get fb_loc of memory training(V2)</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText">On 2019-12-18 4:14 a.m., Yin, Tianci (Rico) wrote:<br>
> Hi Kevin,<br>
> <br>
> You mean like this? It's a bit lengthy.<br>
> - ctx->c2p_train_data_offset &= ~(ONE_MiB - 1);<br>
> + ctx->c2p_train_data_offset = ALIGN(ctx->c2p_train_data_offset, ONE_MiB);<br>
> <br>
> - ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc;<br>
> + ctx->c2p_train_data_offset = adev->gmc.mc_vram_size;<br>
> + if ((ctx->c2p_train_data_offset & (ONE_MiB - 1)) < (4 * ONE_KiB + 1) ) {<br>
> + ctx->c2p_train_data_offset -= ONE_MiB;<br>
> + }<br>
> + ctx->c2p_train_data_offset &= ~(ONE_MiB - 1);<br>
<br>
Using the macro ALIGN() is a good practice.<br>
Usually when calculating a quantity, such as the one above, you'd<br>
use a working variable, say 'a', and after you're done with<br>
the calculation, you'd then assign it to the variable which<br>
needs it. Something like this:<br>
<br>
a = adev->gmc.mc_vram_size;<br>
if ((a & (ONE_MiB - 1)) < (4 * ONE_KiB + 1))<br>
a -= ONE_MiB;<br>
ctx->c2p_train_data_offset = ALIGN(a, ONE_MiB);<br>
<br>
The easiest way to see this is, imagine, if all this calculation<br>
was offloaded to a dedicated function, f(), to do:<br>
<br>
ctx->c2p_train_data_offset = f(adev->gmc.mc_vram_size);<br>
<br>
Now, by using the working variable 'a', you've shown this<br>
abstraction just the same. (By using the working variable 'a',<br>
you've shown to the reader,that this calculation is abstracted,<br>
and could be relocated to a function.)<br>
<br>
Regards,<br>
Luben<br>
P.S. The compiler is probably already doing this, and not working<br>
directly on the ctx->c2p_train_data_offs, but assigns a final<br>
result, as explicitly shown above. The above is to make it easy<br>
for humans to read and understand the code. Hope this helps.<br>
<br>
> <br>
> *[kevin]:*<br>
> *i'd like to use the marco ALIGN() to simple above code.*<br>
> *anyway, the patch Reviewed-by: Kevin Wang <kevin1.wang@amd.com>*<br>
> <br>
> ctx->p2c_train_data_offset = (adev->gmc.mc_vram_size - GDDR6_MEM_TRAINING_OFFSET);<br>
> ctx->train_data_size = GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES;<br>
> <br>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h<br>
> index f1ebd424510c..19eb3e8456c7 100644<br>
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h<br>
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h<br>
> @@ -66,6 +66,13 @@ struct amdgpu_copy_mem {<br>
> unsigned long offset;<br>
> };<br>
> <br>
> +/* Definitions for constance */<br>
> +enum amdgpu_internal_constants<br>
> +{<br>
> + ONE_KiB = 0x400,<br>
> + ONE_MiB = 0x100000,<br>
> +};<br>
> +<br>
> extern const struct ttm_mem_type_manager_func amdgpu_gtt_mgr_func;<br>
> extern const struct ttm_mem_type_manager_func amdgpu_vram_mgr_func;<br>
> <br>
> diff --git a/drivers/gpu/drm/amd/include/atomfirmware.h b/drivers/gpu/drm/amd/include/atomfirmware.h<br>
> index dd7cbc00a0aa..70146518174c 100644<br>
> --- a/drivers/gpu/drm/amd/include/atomfirmware.h<br>
> +++ b/drivers/gpu/drm/amd/include/atomfirmware.h<br>
> @@ -672,20 +672,6 @@ struct vram_usagebyfirmware_v2_1<br>
> uint16_t used_by_driver_in_kb;<br>
> };<br>
> <br>
> -/* This is part of vram_usagebyfirmware_v2_1 */<br>
> -struct vram_reserve_block<br>
> -{<br>
> - uint32_t start_address_in_kb;<br>
> - uint16_t used_by_firmware_in_kb;<br>
> - uint16_t used_by_driver_in_kb;<br>
> -};<br>
> -<br>
> -/* Definitions for constance */<br>
> -enum atomfirmware_internal_constants<br>
> -{<br>
> - ONE_KiB = 0x400,<br>
> - ONE_MiB = 0x100000,<br>
> -};<br>
> <br>
> /*<br>
> ***************************************************************************<br>
> -- <br>
> 2.17.1<br>
> <br>
<br>
</div>
</span></font></div>
</div>
</body>
</html>