|
Thanks very much Christian!
Rico
From: Koenig, Christian <Christian.Koenig@xxxxxxx>
Sent: Monday, October 14, 2019 16:26 To: Tuikov, Luben <Luben.Tuikov@xxxxxxx>; Yin, Tianci (Rico) <Tianci.Yin@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx <amd-gfx@xxxxxxxxxxxxxxxxxxxxx> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx> Subject: Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training Am 12.10.19 um 01:23 schrieb Tuikov, Luben:
> On 2019-10-10 11:50 p.m., Tianci Yin wrote: >> From: "Tianci.Yin" <tianci.yin@xxxxxxx> >> >> memory training using specific fixed vram segment, reserve these >> segments before anyone may allocate it. >> >> Change-Id: I1436755813a565608a2857a683f535377620a637 >> Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx> >> Signed-off-by: Tianci.Yin <tianci.yin@xxxxxxx> >> --- >> 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 9da6350a4ba2..42d0fcb98382 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 >> + */ >> + >> +/** >> + * 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) >> +{ >> + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; >> + >> + ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT; >> + if (ctx->c2p_bo) { >> + amdgpu_bo_free_kernel(&ctx->c2p_bo, NULL, NULL); >> + ctx->c2p_bo = NULL; >> + } > Generally it is a good idea to paragraph your code. > So an empty line between if-statements is a good idea. > However, there is no need in: > > ret = f(x); > if (ret) { > <body of code> > } > > if (blah) { > <body of code> > } > > The above are two (2) well-formed paragraphs. Additional to that amdgpu_bo_free_kernel() just like kfree() is NULL safe for the reason that you shouldn't need "if"s like that one. E.g. just write: amdgpu_bo_free_kernel(&ctx->c2p_bo...); and you are done. Regards, Christian. > >> + if (ctx->p2c_bo) { >> + amdgpu_bo_free_kernel(&ctx->p2c_bo, NULL, NULL); >> + ctx->p2c_bo = NULL; >> + } >> + >> + return 0; >> +} >> + >> +/** >> + * 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; >> + struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx; >> + >> + memset(ctx, 0, sizeof(*ctx)); >> + if (!adev->fw_vram_usage.mem_train_support) { >> + 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, >> + ctx->p2c_train_data_offset, >> + ctx->train_data_size, >> + AMDGPU_GEM_DOMAIN_VRAM, >> + &ctx->p2c_bo, >> + NULL); >> + if (ret) { >> + DRM_ERROR("alloc p2c_bo failed(%d)!\n", ret); >> + ret = -ENOMEM; >> + goto err_out; >> + } > NAK! > Why are you re-writing the error code from "amdgpu_bo_create_kenrel_at()"? > Pass the error as is. > >> + >> + 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) { >> + DRM_ERROR("alloc c2p_bo failed(%d)!\n", ret); >> + ret = -ENOMEM; >> + goto err_out; >> + } > NAK! > Why are you re-writing the error code from "amdgpu_bo_create_kenrel_at()"? > Pass the error as is. > >> + >> + ctx->init = PSP_MEM_TRAIN_RESERVE_SUCCESS; >> + return 0; >> + >> +err_out: > Yes... well "err_out" could be any identifier, including > a variable, as our variables follow snake-notation, all lowercase. > > Back at the turn of this century, Linux followed capitalized > goto labels to distinguish them from anything else around > in the kernel code: > > goto Bad_err; > ... > > return 0; > Bad_err: > return bad_gift; > } > > To distinguish that a capitalized identifier is a goto label, > "Bad_err" and all lower-case label is just another variable > or function identifier, "bad_gift". > > Regards, > Luben > >> + 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); >> |
From 88b7a04f5c6b02ac3003f8b22b0b04bc7a8e08ea Mon Sep 17 00:00:00 2001
From: "Tianci.Yin" <tianci.yin@xxxxxxx>
Date: Mon, 30 Sep 2019 14:28:17 +0800
Subject: [PATCH] drm/amdgpu: reserve vram for memory training(v4)
memory training using specific fixed vram segment, reserve these
segments before anyone may allocate it.
Change-Id: I1436755813a565608a2857a683f535377620a637
Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx>
Signed-off-by: Tianci.Yin <tianci.yin@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 91 +++++++++++++++++++++++++
1 file changed, 91 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 2e85a5154f87..69e54308f080 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1667,6 +1667,88 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct amdgpu_device *adev)
&adev->fw_vram_usage.va);
}
+/*
+ * Memoy training reservation functions
+ */
+
+/**
+ * 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)
+{
+ struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx;
+
+ ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT;
+ amdgpu_bo_free_kernel(&ctx->c2p_bo, NULL, NULL);
+ ctx->c2p_bo = NULL;
+
+ amdgpu_bo_free_kernel(&ctx->p2c_bo, NULL, NULL);
+ ctx->p2c_bo = NULL;
+
+ return 0;
+}
+
+/**
+ * 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;
+ struct psp_memory_training_context *ctx = &adev->psp.mem_train_ctx;
+
+ memset(ctx, 0, sizeof(*ctx));
+ if (!adev->fw_vram_usage.mem_train_support) {
+ 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,
+ ctx->p2c_train_data_offset,
+ ctx->train_data_size,
+ AMDGPU_GEM_DOMAIN_VRAM,
+ &ctx->p2c_bo,
+ NULL);
+ if (ret) {
+ DRM_ERROR("alloc p2c_bo failed(%d)!\n", ret);
+ 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) {
+ DRM_ERROR("alloc c2p_bo failed(%d)!\n", ret);
+ 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 +1822,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
@@ -1842,6 +1932,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);
--
2.17.1
_______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
