<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"> [AMD Official Use Only - Internal Distribution Only] <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);"> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> What a brilliant thought! Concise and easy for eyes!</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Thanks so much!</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 style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </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: Thursday, December 19, 2019 11:10 To: Yin, Tianci (Rico) <Tianci.Yin@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> Cc: 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>; Wang, Kevin(Yang) <Kevin1.Wang@amd.com> Subject: Re: [PATCH 1/2] drm/amdgpu: update the method to get fb_loc of memory training(V3) <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">On 2019-12-18 9:44 p.m., Tianci Yin wrote: > From: "Tianci.Yin" <tianci.yin@amd.com> > > The method of getting fb_loc changed from parsing VBIOS to > taking certain offset from top of VRAM > > Change-Id: I053b42fdb1d822722fa7980b2cd9f86b3fdce539 > Signed-off-by: Tianci.Yin <tianci.yin@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 3 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 2 +- > .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c | 38 ++----------------- > .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 13 ++++++- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 7 ++++ > drivers/gpu/drm/amd/include/atomfirmware.h | 14 ------- > 7 files changed, 26 insertions(+), 53 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > index a78a363b1d71..fa2cf8e7bc07 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > @@ -642,9 +642,8 @@ struct amdgpu_fw_vram_usage { > struct amdgpu_bo *reserved_bo; > void *va; > > - /* Offset on the top of VRAM, used as c2p write buffer. > + /* GDDR6 training support flag. > */ > - u64 mem_train_fb_loc; > bool mem_train_support; > }; > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c > index 9ba80d828876..fdd52d86a4d7 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c > @@ -2022,7 +2022,7 @@ int amdgpu_atombios_init(struct amdgpu_device *adev) > if (adev->is_atom_fw) { > amdgpu_atomfirmware_scratch_regs_init(adev); > amdgpu_atomfirmware_allocate_fb_scratch(adev); > - ret = amdgpu_atomfirmware_get_mem_train_fb_loc(adev); > + ret = amdgpu_atomfirmware_get_mem_train_info(adev); > if (ret) { > DRM_ERROR("Failed to get mem train fb location.\n"); > return ret; > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c > index ff4eb96bdfb5..58f9d8c3a17a 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c > @@ -525,16 +525,12 @@ static int gddr6_mem_train_support(struct amdgpu_device *adev) > return ret; > } > > -int amdgpu_atomfirmware_get_mem_train_fb_loc(struct amdgpu_device *adev) > +int amdgpu_atomfirmware_get_mem_train_info(struct amdgpu_device *adev) > { > struct atom_context *ctx = adev->mode_info.atom_context; > - unsigned char *bios = ctx->bios; > - struct vram_reserve_block *reserved_block; > - int index, block_number; > + int index; > uint8_t frev, crev; > uint16_t data_offset, size; > - uint32_t start_address_in_kb; > - uint64_t offset; > int ret; > > adev->fw_vram_usage.mem_train_support = false; > @@ -569,32 +565,6 @@ int amdgpu_atomfirmware_get_mem_train_fb_loc(struct amdgpu_device *adev) > return -EINVAL; > } > > - reserved_block = (struct vram_reserve_block *) > - (bios + data_offset + sizeof(struct atom_common_table_header)); > - block_number = ((unsigned int)size - sizeof(struct atom_common_table_header)) > - / sizeof(struct vram_reserve_block); > - reserved_block += (block_number > 0) ? block_number-1 : 0; > - DRM_DEBUG("block_number:0x%04x, last block: 0x%08xkb sz, %dkb fw, %dkb drv.\n", > - block_number, > - le32_to_cpu(reserved_block->start_address_in_kb), > - le16_to_cpu(reserved_block->used_by_firmware_in_kb), > - le16_to_cpu(reserved_block->used_by_driver_in_kb)); > - if (reserved_block->used_by_firmware_in_kb > 0) { > - start_address_in_kb = le32_to_cpu(reserved_block->start_address_in_kb); > - offset = (uint64_t)start_address_in_kb * ONE_KiB; > - if ((offset & (ONE_MiB - 1)) < (4 * ONE_KiB + 1) ) { > - offset -= ONE_MiB; > - } > - > - offset &= ~(ONE_MiB - 1); > - adev->fw_vram_usage.mem_train_fb_loc = offset; > - adev->fw_vram_usage.mem_train_support = true; > - DRM_DEBUG("mem_train_fb_loc:0x%09llx.\n", offset); > - ret = 0; > - } else { > - DRM_ERROR("used_by_firmware_in_kb is 0!\n"); > - ret = -EINVAL; > - } > - > - return ret; > + adev->fw_vram_usage.mem_train_support = true; > + return 0; > } > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h > index f871af5ea6f3..434fe2fa0089 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h > @@ -31,7 +31,7 @@ void amdgpu_atomfirmware_scratch_regs_init(struct amdgpu_device *adev); > int amdgpu_atomfirmware_allocate_fb_scratch(struct amdgpu_device *adev); > int amdgpu_atomfirmware_get_vram_info(struct amdgpu_device *adev, > int *vram_width, int *vram_type, int *vram_vendor); > -int amdgpu_atomfirmware_get_mem_train_fb_loc(struct amdgpu_device *adev); > +int amdgpu_atomfirmware_get_mem_train_info(struct amdgpu_device *adev); > int amdgpu_atomfirmware_get_clock_info(struct amdgpu_device *adev); > int amdgpu_atomfirmware_get_gfx_info(struct amdgpu_device *adev); > bool amdgpu_atomfirmware_mem_ecc_supported(struct amdgpu_device *adev); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 2ff63d0414c9..ec84acdd43a2 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -1687,6 +1687,17 @@ static int amdgpu_ttm_training_reserve_vram_fini(struct amdgpu_device *adev) > return 0; > } > > +static void amdgpu_ttm_training_get_c2p_offset(struct amdgpu_device *adev) > +{ > + u64 offset = adev->gmc.mc_vram_size; > + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; > + > + if ((offset & (ONE_MiB - 1)) < (4 * ONE_KiB + 1) ) > + offset -= ONE_MiB; > + > + ctx->c2p_train_data_offset = ALIGN(offset,ONE_MiB); > +} > + > /** > * amdgpu_ttm_training_reserve_vram_init - create bo vram reservation from memory training > * > @@ -1705,7 +1716,7 @@ static int amdgpu_ttm_training_reserve_vram_init(struct amdgpu_device *adev) > return 0; > } > > - ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc; > + amdgpu_ttm_training_get_c2p_offset(); > 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; Well, let's take a look... what have we here? Something like this: ctx->a = something; ctx->b = something; function(); ctx->c = something; Now, the problem is that that function sets a member of ctx, in a *hidden* way. We dont'want to hide this. That is, we want to be explicit inline. So, we want to do it like this: ctx->a = something; ctx->b = something; ctx->c = f(x); ctx->d = something; To become something like this: +static u64 amdgpu_ttm_training_get_c2p_offset(u64 vram_size) +{ + if ((vram_size & (ONE_MiB - 1)) < (4 * ONE_KiB + 1) ) + vram_size -= ONE_MiB; + + return ALIGN(vram_size, ONE_MiB); +} ... - ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc; + ctx->c2p_train_data_offset = amdgpu_ttm_training_get_c2p_offset(adev->gmc.mc_vram_size); 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; And when someone looks at this, they can read down, the assignments, explictly inline assignment operators. It's open to see. (And maybe the '=' equal chars would be column aligned. :-) Note, that the function above doesn't need to know about dev structs and anything like that. It only needs to know about numbers, since this is what it operates on. Input is a number. Output is a number. y = f(x). Minimal. If you compare to my previous email, we just took/lifted the statements which operate on 'a' and put them into a function. Literally: On 2019-12-18 3:14 p.m., Luben Tuikov wrote: > a = adev->gmc.mc_vram_size; > if ((a & (ONE_MiB - 1)) < (4 * ONE_KiB + 1)) > a -= ONE_MiB; > ctx->c2p_train_data_offset = ALIGN(a, ONE_MiB); Hope this helps. Regards, Luben P.S. The compiler we optimize away this function and the code flow and order, but it would be very readable to a human. > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h > index f1ebd424510c..19eb3e8456c7 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h > @@ -66,6 +66,13 @@ struct amdgpu_copy_mem { > unsigned long offset; > }; > > +/* Definitions for constance */ > +enum amdgpu_internal_constants > +{ > + ONE_KiB = 0x400, > + ONE_MiB = 0x100000, > +}; > + > extern const struct ttm_mem_type_manager_func amdgpu_gtt_mgr_func; > extern const struct ttm_mem_type_manager_func amdgpu_vram_mgr_func; > > diff --git a/drivers/gpu/drm/amd/include/atomfirmware.h b/drivers/gpu/drm/amd/include/atomfirmware.h > index dd7cbc00a0aa..70146518174c 100644 > --- a/drivers/gpu/drm/amd/include/atomfirmware.h > +++ b/drivers/gpu/drm/amd/include/atomfirmware.h > @@ -672,20 +672,6 @@ struct vram_usagebyfirmware_v2_1 > uint16_t used_by_driver_in_kb; > }; > > -/* This is part of vram_usagebyfirmware_v2_1 */ > -struct vram_reserve_block > -{ > - uint32_t start_address_in_kb; > - uint16_t used_by_firmware_in_kb; > - uint16_t used_by_driver_in_kb; > -}; > - > -/* Definitions for constance */ > -enum atomfirmware_internal_constants > -{ > - ONE_KiB = 0x400, > - ONE_MiB = 0x100000, > -}; > > /* > *************************************************************************** > </div> </div> </div> </body> </html>