[PATCH 1/2] drm/amdgpu: update the method to get fb_loc of memory training(V2)

[Yin, Tianci (Rico)]

[AMD Official Use Only - Internal Distribution Only]

Hi Luben,

Thank you very much for your suggestion and detailed explanation!

Patch has been refined, please help review.

Rico
________________________________
From: Tuikov, Luben <Luben.Tuikov at amd.com>
Sent: Thursday, December 19, 2019 4:14
To: Yin, Tianci (Rico) <Tianci.Yin at amd.com>; Wang, Kevin(Yang) <Kevin1.Wang at amd.com>; amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>
Cc: Koenig, Christian <Christian.Koenig at amd.com>; Deucher, Alexander <Alexander.Deucher at amd.com>; Zhang, Hawking <Hawking.Zhang at amd.com>; Xu, Feifei <Feifei.Xu at amd.com>; Yuan, Xiaojie <Xiaojie.Yuan at amd.com>; Long, Gang <Gang.Long at amd.com>
Subject: Re: [PATCH 1/2] drm/amdgpu: update the method to get fb_loc of memory training(V2)

On 2019-12-18 4:14 a.m., Yin, Tianci (Rico) wrote:
> Hi Kevin,
>
> You mean like this? It's a bit lengthy.
> - ctx->c2p_train_data_offset &= ~(ONE_MiB - 1);
> + ctx->c2p_train_data_offset = ALIGN(ctx->c2p_train_data_offset, ONE_MiB);
>
> -       ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc;
> +       ctx->c2p_train_data_offset = adev->gmc.mc_vram_size;
> +       if ((ctx->c2p_train_data_offset & (ONE_MiB - 1)) < (4 * ONE_KiB + 1) ) {
> +               ctx->c2p_train_data_offset -= ONE_MiB;
> +       }
> +       ctx->c2p_train_data_offset &= ~(ONE_MiB - 1);

Using the macro ALIGN() is a good practice.
Usually when calculating a quantity, such as the one above, you'd
use a working variable, say 'a', and after you're done with
the calculation, you'd then assign it to the variable which
needs it. Something like this:

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);

The easiest way to see this is, imagine, if all this calculation
was offloaded to a dedicated function, f(), to do:

ctx->c2p_train_data_offset = f(adev->gmc.mc_vram_size);

Now, by using the working variable 'a', you've shown this
abstraction just the same. (By using the working variable 'a',
you've shown to the reader,that this calculation is abstracted,
and could be relocated to a function.)

Regards,
Luben
P.S. The compiler is probably already doing this, and not working
directly on the ctx->c2p_train_data_offs, but assigns a final
result, as explicitly shown above. The above is to make it easy
for humans to read and understand the code. Hope this helps.

>
> *[kevin]:*
> *i'd like to use the marco ALIGN() to simple above code.*
> *anyway, the patch Reviewed-by: Kevin Wang <kevin1.wang at amd.com>*
>
>          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;
>
> 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,
> -};
>
>  /*
>    ***************************************************************************
> --
> 2.17.1
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20191219/ad641497/attachment.htm>