[PATCH 1/8] drm/amdgpu: update amdgpu_discovery to handle revision

[PATCH 1/8] drm/amdgpu: update amdgpu_discovery to handle revision

[Tianci Yin]

From: "Tianci.Yin" <tianci.yin@amd.com>

update amdgpu_discovery to get IP revision.

Change-Id: If8152103d03b58e1dc0f32db63625e290f5f08a0
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 4 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index 1481899f86c1..db2dab3a6dff 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -333,7 +333,7 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
 }
 
 int amdgpu_discovery_get_ip_version(struct amdgpu_device *adev, int hw_id,
-				    int *major, int *minor)
+				    int *major, int *minor, int *revision)
 {
 	struct binary_header *bhdr;
 	struct ip_discovery_header *ihdr;
@@ -369,6 +369,8 @@ int amdgpu_discovery_get_ip_version(struct amdgpu_device *adev, int hw_id,
 					*major = ip->major;
 				if (minor)
 					*minor = ip->minor;
+				if (revision)
+					*revision = ip->revision;
 				return 0;
 			}
 			ip_offset += sizeof(*ip) + 4 * (ip->num_base_address - 1);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h
index 85b8c4d4d576..ada30cfd9d35 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h
@@ -28,7 +28,7 @@ int amdgpu_discovery_init(struct amdgpu_device *adev);
 void amdgpu_discovery_fini(struct amdgpu_device *adev);
 int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev);
 int amdgpu_discovery_get_ip_version(struct amdgpu_device *adev, int hw_id,
-                                    int *major, int *minor);
+                                    int *major, int *minor, int *revision);
 int amdgpu_discovery_get_gfx_info(struct amdgpu_device *adev);
 
 #endif /* __AMDGPU_DISCOVERY__ */
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/8] drm/amdgpu: add a generic fb accessing helper function

[Tianci Yin]

From: "Tianci.Yin" <tianci.yin@amd.com>

add a generic helper function for accessing framebuffer via MMIO

Change-Id: I4baa0aa53c93a94c2eff98c6211a61f369239982
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    | 34 +++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 13 +------
 3 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 51ccf175cda0..1102e6bae5d5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -991,6 +991,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 void amdgpu_device_fini(struct amdgpu_device *adev);
 int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
 
+int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
+			      uint32_t *buf, size_t size, bool write);
 uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
 			uint32_t acc_flags);
 void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 598158e95ec1..fb21ec1f8a61 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -154,6 +154,40 @@ bool amdgpu_device_is_px(struct drm_device *dev)
 	return false;
 }
 
+/**
+ * VRAM access helper functions.
+ *
+ * amdgpu_device_vram_access - read/write a buffer in vram
+ *
+ * @adev: amdgpu_device pointer
+ * @pos: offset of the buffer in vram
+ * @buf: virtual address of the buffer in system memory
+ * @size: read/write size, sizeof(@buf) must > @size
+ * @write: true - write to vram, otherwise - read from vram
+ *
+ * Returns 0 on success or an -error on failure.
+ */
+int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
+			      uint32_t *buf, size_t size, bool write)
+{
+	uint64_t end = pos + size;
+	unsigned long flags;
+
+	while (pos < end) {
+		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
+		WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x80000000);
+		WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
+		if (write)
+			WREG32_NO_KIQ(mmMM_DATA, *buf++);
+		else
+			*buf++ = RREG32_NO_KIQ(mmMM_DATA);
+		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
+		pos += 4;
+	}
+
+	return 0;
+}
+
 /*
  * MMIO register access helper functions.
  */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index db2dab3a6dff..324c2d605f54 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -134,21 +134,10 @@ static int hw_id_map[MAX_HWIP] = {
 
 static int amdgpu_discovery_read_binary(struct amdgpu_device *adev, uint8_t *binary)
 {
-	uint32_t *p = (uint32_t *)binary;
 	uint64_t vram_size = (uint64_t)RREG32(mmRCC_CONFIG_MEMSIZE) << 20;
 	uint64_t pos = vram_size - BINARY_MAX_SIZE;
-	unsigned long flags;
-
-	while (pos < vram_size) {
-		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
-		WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x80000000);
-		WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
-		*p++ = RREG32_NO_KIQ(mmMM_DATA);
-		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
-		pos += 4;
-	}
 
-	return 0;
+	return amdgpu_device_vram_access(adev, pos, (uint32_t *)binary, BINARY_MAX_SIZE, false);
 }
 
 static uint16_t amdgpu_discovery_calculate_checksum(uint8_t *data, uint32_t size)
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/8] drm/amdgpu: add a generic fb accessing helper function

[Tuikov, Luben]

On 2019-10-10 11:50 p.m., Tianci Yin wrote:
> From: "Tianci.Yin" <tianci.yin@amd.com>
> 
> add a generic helper function for accessing framebuffer via MMIO
> 
> Change-Id: I4baa0aa53c93a94c2eff98c6211a61f369239982
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  2 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    | 34 +++++++++++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 13 +------
>  3 files changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 51ccf175cda0..1102e6bae5d5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -991,6 +991,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>  void amdgpu_device_fini(struct amdgpu_device *adev);
>  int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
>  
> +int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> +			      uint32_t *buf, size_t size, bool write);
>  uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
>  			uint32_t acc_flags);
>  void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 598158e95ec1..fb21ec1f8a61 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -154,6 +154,40 @@ bool amdgpu_device_is_px(struct drm_device *dev)
>  	return false;
>  }
>  
> +/**
> + * VRAM access helper functions.
> + *
> + * amdgpu_device_vram_access - read/write a buffer in vram
> + *
> + * @adev: amdgpu_device pointer
> + * @pos: offset of the buffer in vram
> + * @buf: virtual address of the buffer in system memory
> + * @size: read/write size, sizeof(@buf) must > @size
> + * @write: true - write to vram, otherwise - read from vram
> + *
> + * Returns 0 on success or an -error on failure.

Really? Where?
This function seems to return 0.
Traditionally read/write functions
return the number of bytes read/written or error.
You do neither. Just define it void.

> + */
> +int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> +			      uint32_t *buf, size_t size, bool write)
> +{
> +	uint64_t end = pos + size;

NAK! to preinitializing automatic variables.

> +	unsigned long flags;
> +
> +	while (pos < end) {
> +		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> +		WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x80000000);
> +		WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
> +		if (write)
> +			WREG32_NO_KIQ(mmMM_DATA, *buf++);
> +		else
> +			*buf++ = RREG32_NO_KIQ(mmMM_DATA);
> +		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> +		pos += 4;
> +	}

NAK! to this this:

while (pos < end) {     <---+
	<body of loop>      |--> Part of the loop definition
	pos += 4;       <---+
}

Instead of breaking up the loop definition like this,
use a for-loop, and DO NOT preinitialize the "end" variable,
and also protect from overflow, all like this:

last = size - 1;
for (last += pos; pos <= last; pos += 4) {
	<body of loop>
}

I mentioned the why of this in my previous review of the same
topic patchset over the same while loop fiasco.

Regards,
Luben

> +
> +	return 0;
> +}
> +
>  /*
>   * MMIO register access helper functions.
>   */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index db2dab3a6dff..324c2d605f54 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -134,21 +134,10 @@ static int hw_id_map[MAX_HWIP] = {
>  
>  static int amdgpu_discovery_read_binary(struct amdgpu_device *adev, uint8_t *binary)
>  {
> -	uint32_t *p = (uint32_t *)binary;
>  	uint64_t vram_size = (uint64_t)RREG32(mmRCC_CONFIG_MEMSIZE) << 20;
>  	uint64_t pos = vram_size - BINARY_MAX_SIZE;
> -	unsigned long flags;
> -
> -	while (pos < vram_size) {
> -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> -		WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x80000000);
> -		WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
> -		*p++ = RREG32_NO_KIQ(mmMM_DATA);
> -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> -		pos += 4;
> -	}
>  
> -	return 0;
> +	return amdgpu_device_vram_access(adev, pos, (uint32_t *)binary, BINARY_MAX_SIZE, false);
>  }
>  
>  static uint16_t amdgpu_discovery_calculate_checksum(uint8_t *data, uint32_t size)
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/8] drm/amdgpu: introduce psp_v11_0_is_sos_alive interface

[Tianci Yin]

From: "Tianci.Yin" <tianci.yin@amd.com>

introduce psp_v11_0_is_sos_alive func for common use.

Change-Id: Iee0a6dd924d6a4b164eb751c0bec49fcb7d79483
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
index 04318cfd50a8..2ba0f68ced10 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
@@ -206,18 +206,26 @@ static int psp_v11_0_init_microcode(struct psp_context *psp)
 	return err;
 }
 
+static bool psp_v11_0_is_sos_alive(struct psp_context *psp)
+{
+	struct amdgpu_device *adev = psp->adev;
+	uint32_t sol_reg;
+
+	sol_reg = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_81);
+
+	return sol_reg != 0x0;
+}
+
 static int psp_v11_0_bootloader_load_kdb(struct psp_context *psp)
 {
 	int ret;
 	uint32_t psp_gfxdrv_command_reg = 0;
 	struct amdgpu_device *adev = psp->adev;
-	uint32_t sol_reg;
 
 	/* Check tOS sign of life register to confirm sys driver and sOS
 	 * are already been loaded.
 	 */
-	sol_reg = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_81);
-	if (sol_reg) {
+	if (psp_v11_0_is_sos_alive(psp)) {
 		psp->sos_fw_version = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_58);
 		dev_info(adev->dev, "sos fw version = 0x%x.\n", psp->sos_fw_version);
 		return 0;
@@ -253,13 +261,11 @@ static int psp_v11_0_bootloader_load_sysdrv(struct psp_context *psp)
 	int ret;
 	uint32_t psp_gfxdrv_command_reg = 0;
 	struct amdgpu_device *adev = psp->adev;
-	uint32_t sol_reg;
 
 	/* Check sOS sign of life register to confirm sys driver and sOS
 	 * are already been loaded.
 	 */
-	sol_reg = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_81);
-	if (sol_reg) {
+	if (psp_v11_0_is_sos_alive(psp)) {
 		psp->sos_fw_version = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_58);
 		dev_info(adev->dev, "sos fw version = 0x%x.\n", psp->sos_fw_version);
 		return 0;
@@ -297,13 +303,11 @@ static int psp_v11_0_bootloader_load_sos(struct psp_context *psp)
 	int ret;
 	unsigned int psp_gfxdrv_command_reg = 0;
 	struct amdgpu_device *adev = psp->adev;
-	uint32_t sol_reg;
 
 	/* Check sOS sign of life register to confirm sys driver and sOS
 	 * are already been loaded.
 	 */
-	sol_reg = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_81);
-	if (sol_reg)
+	if (psp_v11_0_is_sos_alive(psp))
 		return 0;
 
 	/* Wait for bootloader to signify that is ready having bit 31 of C2PMSG_35 set to 1 */
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/8] drm/amdgpu: update atomfirmware header with memory training related members

[Tianci Yin]

From: "Tianci.Yin" <tianci.yin@amd.com>

add new vram_reserve_block structure and atomfirmware_internal_constants enumeration

Change-Id: I6ba642ecd7ad94250162ae5c322ed8d85de9c35a
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
---
 drivers/gpu/drm/amd/include/atomfirmware.h | 28 +++++++++++++++++-----
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/include/atomfirmware.h b/drivers/gpu/drm/amd/include/atomfirmware.h
index e88541d67aa0..463c18e99d78 100644
--- a/drivers/gpu/drm/amd/include/atomfirmware.h
+++ b/drivers/gpu/drm/amd/include/atomfirmware.h
@@ -492,12 +492,13 @@ struct atom_firmware_info_v3_1
 /* Total 32bit cap indication */
 enum atombios_firmware_capability
 {
-  ATOM_FIRMWARE_CAP_FIRMWARE_POSTED = 0x00000001,
-  ATOM_FIRMWARE_CAP_GPU_VIRTUALIZATION  = 0x00000002,
-  ATOM_FIRMWARE_CAP_WMI_SUPPORT  = 0x00000040,
-  ATOM_FIRMWARE_CAP_HWEMU_ENABLE  = 0x00000080,
-  ATOM_FIRMWARE_CAP_HWEMU_UMC_CFG = 0x00000100,
-  ATOM_FIRMWARE_CAP_SRAM_ECC      = 0x00000200,
+	ATOM_FIRMWARE_CAP_FIRMWARE_POSTED = 0x00000001,
+	ATOM_FIRMWARE_CAP_GPU_VIRTUALIZATION  = 0x00000002,
+	ATOM_FIRMWARE_CAP_WMI_SUPPORT  = 0x00000040,
+	ATOM_FIRMWARE_CAP_HWEMU_ENABLE  = 0x00000080,
+	ATOM_FIRMWARE_CAP_HWEMU_UMC_CFG = 0x00000100,
+	ATOM_FIRMWARE_CAP_SRAM_ECC      = 0x00000200,
+	ATOM_FIRMWARE_CAP_ENABLE_2STAGE_BIST_TRAINING  = 0x00000400,
 };
 
 enum atom_cooling_solution_id{
@@ -671,6 +672,21 @@ 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_K	= 0x400,
+	ONE_MEG	= 0x100000,
+	ONE_G	= 0x40000000,
+};
 
 /* 
   ***************************************************************************
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/8] drm/amdgpu: update atomfirmware header with memory training related members

[Tuikov, Luben]

On 2019-10-10 11:50 p.m., Tianci Yin wrote:
> From: "Tianci.Yin" <tianci.yin@amd.com>
> 
> add new vram_reserve_block structure and atomfirmware_internal_constants enumeration
> 
> Change-Id: I6ba642ecd7ad94250162ae5c322ed8d85de9c35a
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
> ---
>  drivers/gpu/drm/amd/include/atomfirmware.h | 28 +++++++++++++++++-----
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/include/atomfirmware.h b/drivers/gpu/drm/amd/include/atomfirmware.h
> index e88541d67aa0..463c18e99d78 100644
> --- a/drivers/gpu/drm/amd/include/atomfirmware.h
> +++ b/drivers/gpu/drm/amd/include/atomfirmware.h
> @@ -492,12 +492,13 @@ struct atom_firmware_info_v3_1
>  /* Total 32bit cap indication */
>  enum atombios_firmware_capability
>  {
> -  ATOM_FIRMWARE_CAP_FIRMWARE_POSTED = 0x00000001,
> -  ATOM_FIRMWARE_CAP_GPU_VIRTUALIZATION  = 0x00000002,
> -  ATOM_FIRMWARE_CAP_WMI_SUPPORT  = 0x00000040,
> -  ATOM_FIRMWARE_CAP_HWEMU_ENABLE  = 0x00000080,
> -  ATOM_FIRMWARE_CAP_HWEMU_UMC_CFG = 0x00000100,
> -  ATOM_FIRMWARE_CAP_SRAM_ECC      = 0x00000200,
> +	ATOM_FIRMWARE_CAP_FIRMWARE_POSTED = 0x00000001,
> +	ATOM_FIRMWARE_CAP_GPU_VIRTUALIZATION  = 0x00000002,
> +	ATOM_FIRMWARE_CAP_WMI_SUPPORT  = 0x00000040,
> +	ATOM_FIRMWARE_CAP_HWEMU_ENABLE  = 0x00000080,
> +	ATOM_FIRMWARE_CAP_HWEMU_UMC_CFG = 0x00000100,
> +	ATOM_FIRMWARE_CAP_SRAM_ECC      = 0x00000200,
> +	ATOM_FIRMWARE_CAP_ENABLE_2STAGE_BIST_TRAINING  = 0x00000400,
>  };
>  
>  enum atom_cooling_solution_id{
> @@ -671,6 +672,21 @@ 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_K	= 0x400,
> +	ONE_MEG	= 0x100000,
> +	ONE_G	= 0x40000000,

So... this is pronounced in English as "One Gee", and even though
I like it much much better due to what is actually says (no, it is not
"one gigabyte"), I'd rather you called this "ONE_GiB".

You do not have "One G" anywhere in this code. :-)

To fit international standardization and the move Linux constants
have been going to, name them this:

ONE_KiB
ONE_MiB
ONE_GiB

This means what it says and also that they are power of 2.

ONE_GB (10^9) is less than ONE_GiB (2^30).

Regards,
Luben

> +};
>  
>  /* 
>    ***************************************************************************
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/8] drm/amdgpu: update atomfirmware header with memory training related members

[Alex Deucher]

On Fri, Oct 11, 2019 at 6:53 PM Tuikov, Luben <Luben.Tuikov@amd.com> wrote:
>
> On 2019-10-10 11:50 p.m., Tianci Yin wrote:
> > From: "Tianci.Yin" <tianci.yin@amd.com>
> >
> > add new vram_reserve_block structure and atomfirmware_internal_constants enumeration
> >
> > Change-Id: I6ba642ecd7ad94250162ae5c322ed8d85de9c35a
> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> > Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
> > ---
> >  drivers/gpu/drm/amd/include/atomfirmware.h | 28 +++++++++++++++++-----
> >  1 file changed, 22 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/include/atomfirmware.h b/drivers/gpu/drm/amd/include/atomfirmware.h
> > index e88541d67aa0..463c18e99d78 100644
> > --- a/drivers/gpu/drm/amd/include/atomfirmware.h
> > +++ b/drivers/gpu/drm/amd/include/atomfirmware.h
> > @@ -492,12 +492,13 @@ struct atom_firmware_info_v3_1
> >  /* Total 32bit cap indication */
> >  enum atombios_firmware_capability
> >  {
> > -  ATOM_FIRMWARE_CAP_FIRMWARE_POSTED = 0x00000001,
> > -  ATOM_FIRMWARE_CAP_GPU_VIRTUALIZATION  = 0x00000002,
> > -  ATOM_FIRMWARE_CAP_WMI_SUPPORT  = 0x00000040,
> > -  ATOM_FIRMWARE_CAP_HWEMU_ENABLE  = 0x00000080,
> > -  ATOM_FIRMWARE_CAP_HWEMU_UMC_CFG = 0x00000100,
> > -  ATOM_FIRMWARE_CAP_SRAM_ECC      = 0x00000200,
> > +     ATOM_FIRMWARE_CAP_FIRMWARE_POSTED = 0x00000001,
> > +     ATOM_FIRMWARE_CAP_GPU_VIRTUALIZATION  = 0x00000002,
> > +     ATOM_FIRMWARE_CAP_WMI_SUPPORT  = 0x00000040,
> > +     ATOM_FIRMWARE_CAP_HWEMU_ENABLE  = 0x00000080,
> > +     ATOM_FIRMWARE_CAP_HWEMU_UMC_CFG = 0x00000100,
> > +     ATOM_FIRMWARE_CAP_SRAM_ECC      = 0x00000200,
> > +     ATOM_FIRMWARE_CAP_ENABLE_2STAGE_BIST_TRAINING  = 0x00000400,
> >  };
> >
> >  enum atom_cooling_solution_id{
> > @@ -671,6 +672,21 @@ 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_K   = 0x400,
> > +     ONE_MEG = 0x100000,
> > +     ONE_G   = 0x40000000,
>
> So... this is pronounced in English as "One Gee", and even though
> I like it much much better due to what is actually says (no, it is not
> "one gigabyte"), I'd rather you called this "ONE_GiB".
>
> You do not have "One G" anywhere in this code. :-)
>
> To fit international standardization and the move Linux constants
> have been going to, name them this:
>
> ONE_KiB
> ONE_MiB
> ONE_GiB
>
> This means what it says and also that they are power of 2.
>
> ONE_GB (10^9) is less than ONE_GiB (2^30).
>

This file isn't owned by us.  We are just syncing with their latest
updates.  We try and stick to what they have pretty closely to avoid
conflicts in future updates.

Alex

> Regards,
> Luben
>
> > +};
> >
> >  /*
> >    ***************************************************************************
> >
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 5/8] drm/amdgpu/atomfirmware: add memory training related helper functions

[Tianci Yin]

From: "Tianci.Yin" <tianci.yin@amd.com>

parse firmware to get memory training capability and fb location.

Change-Id: I147c1d48e255e0191be4beb1ad6b637da607bf75
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |   7 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c  |   5 +
 .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c  | 133 ++++++++++++++++++
 .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h  |   1 +
 4 files changed, 146 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 1102e6bae5d5..e3d715c31ac9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -291,6 +291,9 @@ struct amdgpu_ip_block_version {
 	const struct amd_ip_funcs *funcs;
 };
 
+#define hw_revision(major, minor, revision) \
+	((((uint32_t) major) << 16) | ((uint32_t) minor << 8) | ((uint32_t) revision))
+
 struct amdgpu_ip_block {
 	struct amdgpu_ip_block_status status;
 	const struct amdgpu_ip_block_version *version;
@@ -633,6 +636,10 @@ struct amdgpu_fw_vram_usage {
 	u64 size;
 	struct amdgpu_bo *reserved_bo;
 	void *va;
+
+	/*offset on the top of vram, used as c2p write buffer*/
+	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 1c9d40f97a9b..72232fccf61a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
@@ -2038,6 +2038,11 @@ 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);
+		if (ret) {
+			DRM_ERROR("Failed to get mem train fb location.\n");
+			return ret;
+		}
 	} else {
 		amdgpu_atombios_scratch_regs_init(adev);
 		amdgpu_atombios_allocate_fb_scratch(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
index 39fd8ae5a822..1ebf5e9a9b7b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
@@ -27,6 +27,7 @@
 #include "amdgpu_atomfirmware.h"
 #include "atom.h"
 #include "atombios.h"
+#include "soc15_hw_ip.h"
 
 bool amdgpu_atomfirmware_gpu_supports_virtualization(struct amdgpu_device *adev)
 {
@@ -462,3 +463,135 @@ int amdgpu_atomfirmware_get_gfx_info(struct amdgpu_device *adev)
 	}
 	return -EINVAL;
 }
+
+/*
+ * Check if VBIOS supports GDDR6 training data save/restore
+ */
+static bool gddr6_mem_train_vbios_support(struct amdgpu_device *adev)
+{
+	uint16_t data_offset;
+	int index;
+
+	index = get_index_into_master_table(atom_master_list_of_data_tables_v2_1,
+					    firmwareinfo);
+	if (amdgpu_atom_parse_data_header(adev->mode_info.atom_context, index, NULL,
+					  NULL, NULL, &data_offset)) {
+		struct atom_firmware_info_v3_1 *firmware_info =
+			(struct atom_firmware_info_v3_1 *)(adev->mode_info.atom_context->bios +
+							   data_offset);
+
+		DRM_DEBUG("atom firmware capability:0x%08x.\n",
+			  le32_to_cpu(firmware_info->firmware_capability));
+
+		if (le32_to_cpu(firmware_info->firmware_capability) &
+		    ATOM_FIRMWARE_CAP_ENABLE_2STAGE_BIST_TRAINING)
+			return true;
+	}
+
+	return false;
+}
+
+static int gddr6_mem_train_support(struct amdgpu_device *adev)
+{
+	int ret;
+	bool vbios_support;
+	uint32_t major, minor, revision, hw_v;
+
+	if (!amdgpu_sriov_vf(adev) &&
+	    gddr6_mem_train_vbios_support(adev)) {
+		vbios_support = true;
+	} else {
+		vbios_support = false;
+	}
+	amdgpu_discovery_get_ip_version(adev, MP0_HWID, &major, &minor, &revision);
+	hw_v = hw_revision(major, minor, revision);
+	/*
+	 * treat 0 revision as a special case since register for MP0 and MMHUB is missing
+	 * for some Navi10 A0, preventing driver from discovering the hwip information since
+	 * none of the functions will be initialized, it should not cause any problems
+	 */
+	switch (hw_v) {
+	case hw_revision(11, 0, 0):
+	case hw_revision(11, 0, 5):
+		ret = vbios_support;
+		break;
+	default:
+		if (vbios_support) {
+			DRM_ERROR("memory training vbios supports but psp hw(%08x)"
+				  " doesn't support!\n", hw_v);
+			ret = -1;
+		} else {
+			ret = 0;
+		}
+		break;
+	}
+
+	DRM_DEBUG("mp0 hw_v %08x, ret:%d.\n", hw_v, ret);
+	return ret;
+}
+
+int amdgpu_atomfirmware_get_mem_train_fb_loc(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;
+	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;
+	ret = gddr6_mem_train_support(adev);
+	if (ret == -1)
+		return -EINVAL;
+	else if (ret == 0)
+		return 0;
+
+	index = get_index_into_master_table(atom_master_list_of_data_tables_v2_1,
+					    vram_usagebyfirmware);
+	ret = amdgpu_atom_parse_data_header(ctx, index, &size, &frev, &crev,
+					    &data_offset);
+	if (ret == 0) {
+		DRM_ERROR("parse data header failed.\n");
+		return -EINVAL;
+	}
+
+	DRM_DEBUG("atom firmware common table header size:0x%04x, frev:0x%02x,"
+		  " crev:0x%02x, data_offset:0x%04x.\n", size, frev, crev, data_offset);
+	/* only support 2.1+ */
+	if (((uint16_t)frev << 8 | crev) < 0x0201) {
+		DRM_ERROR("frev:0x%02x, crev:0x%02x < 2.1 !\n", frev, crev);
+		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_K;
+		if ((offset & (ONE_MEG - 1)) < (4 * ONE_K + 1) ) {
+			offset -= ONE_MEG;
+		}
+
+		offset &= ~(ONE_MEG - 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;
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h
index 53449fc7baf4..f871af5ea6f3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h
@@ -31,6 +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_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);
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/8] drm/amdgpu/atomfirmware: add memory training related helper functions

[Tuikov, Luben]

On 2019-10-10 11:50 p.m., Tianci Yin wrote:
> From: "Tianci.Yin" <tianci.yin@amd.com>
> 
> parse firmware to get memory training capability and fb location.
> 
> Change-Id: I147c1d48e255e0191be4beb1ad6b637da607bf75
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h           |   7 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c  |   5 +
>  .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c  | 133 ++++++++++++++++++
>  .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h  |   1 +
>  4 files changed, 146 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 1102e6bae5d5..e3d715c31ac9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -291,6 +291,9 @@ struct amdgpu_ip_block_version {
>  	const struct amd_ip_funcs *funcs;
>  };
>  
> +#define hw_revision(major, minor, revision) \
> +	((((uint32_t) major) << 16) | ((uint32_t) minor << 8) | ((uint32_t) revision))
> +

Last century, compilers and preprocessors weren't that smart and
should a variable exist of the same name as the replacement token in
a substituion macro, and the macro was used in that function, then
they got confused.

And also, you should surround the substition tokens in parenthesis
in the RHS of the macro expression!

For this reason we used tokens which one would never find in normal
C code:

#define HW_REV(_Major, _Minor, _Rev) \
	((((uint32_t) (_Major)) << 16) | ((uint32_t) (_Minor) << 8) | ((uint32_t) (_Rev)))

It became a habit and as it happens to all habits... a style.

>  struct amdgpu_ip_block {
>  	struct amdgpu_ip_block_status status;
>  	const struct amdgpu_ip_block_version *version;
> @@ -633,6 +636,10 @@ struct amdgpu_fw_vram_usage {
>  	u64 size;
>  	struct amdgpu_bo *reserved_bo;
>  	void *va;
> +
> +	/*offset on the top of vram, used as c2p write buffer*/
> +	u64 mem_train_fb_loc;
> +	bool mem_train_support;
>  };

We try to make comments pleasantly readable:

	/* Offset on the top of VRAM, used as c2p write buffer.
	 */
	u64  mem_train_fb_loc;
	bool mem_train_support;
}

Regards,
Luben

>  
>  /*
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> index 1c9d40f97a9b..72232fccf61a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> @@ -2038,6 +2038,11 @@ 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);
> +		if (ret) {
> +			DRM_ERROR("Failed to get mem train fb location.\n");
> +			return ret;
> +		}
>  	} else {
>  		amdgpu_atombios_scratch_regs_init(adev);
>  		amdgpu_atombios_allocate_fb_scratch(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> index 39fd8ae5a822..1ebf5e9a9b7b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> @@ -27,6 +27,7 @@
>  #include "amdgpu_atomfirmware.h"
>  #include "atom.h"
>  #include "atombios.h"
> +#include "soc15_hw_ip.h"
>  
>  bool amdgpu_atomfirmware_gpu_supports_virtualization(struct amdgpu_device *adev)
>  {
> @@ -462,3 +463,135 @@ int amdgpu_atomfirmware_get_gfx_info(struct amdgpu_device *adev)
>  	}
>  	return -EINVAL;
>  }
> +
> +/*
> + * Check if VBIOS supports GDDR6 training data save/restore
> + */
> +static bool gddr6_mem_train_vbios_support(struct amdgpu_device *adev)
> +{
> +	uint16_t data_offset;
> +	int index;
> +
> +	index = get_index_into_master_table(atom_master_list_of_data_tables_v2_1,
> +					    firmwareinfo);
> +	if (amdgpu_atom_parse_data_header(adev->mode_info.atom_context, index, NULL,
> +					  NULL, NULL, &data_offset)) {
> +		struct atom_firmware_info_v3_1 *firmware_info =
> +			(struct atom_firmware_info_v3_1 *)(adev->mode_info.atom_context->bios +
> +							   data_offset);
> +
> +		DRM_DEBUG("atom firmware capability:0x%08x.\n",
> +			  le32_to_cpu(firmware_info->firmware_capability));
> +
> +		if (le32_to_cpu(firmware_info->firmware_capability) &
> +		    ATOM_FIRMWARE_CAP_ENABLE_2STAGE_BIST_TRAINING)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int gddr6_mem_train_support(struct amdgpu_device *adev)
> +{
> +	int ret;
> +	bool vbios_support;
> +	uint32_t major, minor, revision, hw_v;
> +
> +	if (!amdgpu_sriov_vf(adev) &&
> +	    gddr6_mem_train_vbios_support(adev)) {
> +		vbios_support = true;
> +	} else {
> +		vbios_support = false;
> +	}
> +	amdgpu_discovery_get_ip_version(adev, MP0_HWID, &major, &minor, &revision);
> +	hw_v = hw_revision(major, minor, revision);
> +	/*
> +	 * treat 0 revision as a special case since register for MP0 and MMHUB is missing
> +	 * for some Navi10 A0, preventing driver from discovering the hwip information since
> +	 * none of the functions will be initialized, it should not cause any problems
> +	 */
> +	switch (hw_v) {
> +	case hw_revision(11, 0, 0):
> +	case hw_revision(11, 0, 5):
> +		ret = vbios_support;
> +		break;
> +	default:
> +		if (vbios_support) {
> +			DRM_ERROR("memory training vbios supports but psp hw(%08x)"
> +				  " doesn't support!\n", hw_v);
> +			ret = -1;
> +		} else {
> +			ret = 0;
> +		}
> +		break;
> +	}
> +
> +	DRM_DEBUG("mp0 hw_v %08x, ret:%d.\n", hw_v, ret);
> +	return ret;
> +}
> +
> +int amdgpu_atomfirmware_get_mem_train_fb_loc(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;
> +	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;
> +	ret = gddr6_mem_train_support(adev);
> +	if (ret == -1)
> +		return -EINVAL;
> +	else if (ret == 0)
> +		return 0;
> +
> +	index = get_index_into_master_table(atom_master_list_of_data_tables_v2_1,
> +					    vram_usagebyfirmware);
> +	ret = amdgpu_atom_parse_data_header(ctx, index, &size, &frev, &crev,
> +					    &data_offset);
> +	if (ret == 0) {
> +		DRM_ERROR("parse data header failed.\n");
> +		return -EINVAL;
> +	}
> +
> +	DRM_DEBUG("atom firmware common table header size:0x%04x, frev:0x%02x,"
> +		  " crev:0x%02x, data_offset:0x%04x.\n", size, frev, crev, data_offset);
> +	/* only support 2.1+ */
> +	if (((uint16_t)frev << 8 | crev) < 0x0201) {
> +		DRM_ERROR("frev:0x%02x, crev:0x%02x < 2.1 !\n", frev, crev);
> +		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_K;
> +		if ((offset & (ONE_MEG - 1)) < (4 * ONE_K + 1) ) {
> +			offset -= ONE_MEG;
> +		}
> +
> +		offset &= ~(ONE_MEG - 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;
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h
> index 53449fc7baf4..f871af5ea6f3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h
> @@ -31,6 +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_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);
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 6/8] drm/amdgpu: add psp memory training callbacks and macro

[Tianci Yin]

From: "Tianci.Yin" <tianci.yin@amd.com>

add interface for memory training.

Change-Id: Ibb6d1d24eb651df796bc2bb3419a44937af60242
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 18 ++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 55 +++++++++++++++++++++++++
 2 files changed, 73 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 37ffed5e2171..b996b5bc5804 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -88,6 +88,17 @@ static int psp_sw_init(void *handle)
 		return ret;
 	}
 
+	ret = psp_mem_training_init(psp);
+	if (ret) {
+		DRM_ERROR("Failed to initliaze memory training!\n");
+		return ret;
+	}
+	ret = psp_mem_training(psp, PSP_MEM_TRAIN_COLD_BOOT);
+	if (ret) {
+		DRM_ERROR("Failed to process memory training!\n");
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -95,6 +106,7 @@ static int psp_sw_fini(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
+	psp_mem_training_fini(&adev->psp);
 	release_firmware(adev->psp.sos_fw);
 	adev->psp.sos_fw = NULL;
 	release_firmware(adev->psp.asd_fw);
@@ -1608,6 +1620,12 @@ static int psp_resume(void *handle)
 
 	DRM_INFO("PSP is resuming...\n");
 
+	ret = psp_mem_training(psp, PSP_MEM_TRAIN_RESUME);
+	if (ret) {
+		DRM_ERROR("Failed to process memory training!\n");
+		return ret;
+	}
+
 	mutex_lock(&adev->firmware.mutex);
 
 	ret = psp_hw_start(psp);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
index 7dd9ae7dbbe4..c6f17d6310d6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
@@ -49,6 +49,8 @@ enum psp_bootloader_cmd {
 	PSP_BL__LOAD_SYSDRV		= 0x10000,
 	PSP_BL__LOAD_SOSDRV		= 0x20000,
 	PSP_BL__LOAD_KEY_DATABASE	= 0x80000,
+	PSP_BL__DRAM_LONG_TRAIN		= 0x100000,
+	PSP_BL__DRAM_SHORT_TRAIN	= 0x200000,
 };
 
 enum psp_ring_type
@@ -111,6 +113,9 @@ struct psp_funcs
 			struct ta_ras_trigger_error_input *info);
 	int (*ras_cure_posion)(struct psp_context *psp, uint64_t *mode_ptr);
 	int (*rlc_autoload_start)(struct psp_context *psp);
+	int (*mem_training_init)(struct psp_context *psp);
+	int (*mem_training_fini)(struct psp_context *psp);
+	int (*mem_training)(struct psp_context *psp, uint32_t ops);
 };
 
 #define AMDGPU_XGMI_MAX_CONNECTED_NODES		64
@@ -161,6 +166,49 @@ struct psp_dtm_context {
 	void			*dtm_shared_buf;
 };
 
+#define MEM_TRAIN_SYSTEM_SIGNATURE		0x54534942
+#define GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES	0x1000
+#define GDDR6_MEM_TRAINING_OFFSET		0x8000
+
+enum psp_memory_training_init_flag {
+	PSP_MEM_TRAIN_NOT_SUPPORT	= 0x0,
+	PSP_MEM_TRAIN_SUPPORT		= 0x1,
+	PSP_MEM_TRAIN_INIT_FAILED	= 0x2,
+	PSP_MEM_TRAIN_RESERVE_SUCCESS	= 0x4,
+	PSP_MEM_TRAIN_INIT_SUCCESS	= 0x8,
+};
+
+enum psp_memory_training_ops {
+	PSP_MEM_TRAIN_SEND_LONG_MSG	= 0x1,
+	PSP_MEM_TRAIN_SAVE		= 0x2,
+	PSP_MEM_TRAIN_RESTORE		= 0x4,
+	PSP_MEM_TRAIN_SEND_SHORT_MSG	= 0x8,
+	PSP_MEM_TRAIN_COLD_BOOT		= PSP_MEM_TRAIN_SEND_LONG_MSG,
+	PSP_MEM_TRAIN_RESUME		= PSP_MEM_TRAIN_SEND_SHORT_MSG,
+};
+
+struct psp_memory_training_context {
+	/*training data size*/
+	u64 train_data_size;
+	/*
+	 * sys_cache
+	 * cpu virtual address
+	 * system memory buffer that used to store the training data.
+	 */
+	void *sys_cache;
+
+	/*vram offset of the p2c training data*/
+	u64 p2c_train_data_offset;
+	struct amdgpu_bo *p2c_bo;
+
+	/*vram offset of the c2p training data*/
+	u64 c2p_train_data_offset;
+	struct amdgpu_bo *c2p_bo;
+
+	enum psp_memory_training_init_flag init;
+	u32 training_cnt;
+};
+
 struct psp_context
 {
 	struct amdgpu_device            *adev;
@@ -239,6 +287,7 @@ struct psp_context
 	struct psp_hdcp_context 	hdcp_context;
 	struct psp_dtm_context		dtm_context;
 	struct mutex			mutex;
+	struct psp_memory_training_context mem_train_ctx;
 };
 
 struct amdgpu_psp_funcs {
@@ -281,6 +330,12 @@ struct amdgpu_psp_funcs {
 		(psp)->funcs->xgmi_set_topology_info((psp), (num_device), (topology)) : -EINVAL)
 #define psp_rlc_autoload(psp) \
 		((psp)->funcs->rlc_autoload_start ? (psp)->funcs->rlc_autoload_start((psp)) : 0)
+#define psp_mem_training_init(psp) \
+	((psp)->funcs->mem_training_init ? (psp)->funcs->mem_training_init((psp)) : 0)
+#define psp_mem_training_fini(psp) \
+	((psp)->funcs->mem_training_fini ? (psp)->funcs->mem_training_fini((psp)) : 0)
+#define psp_mem_training(psp, ops) \
+	((psp)->funcs->mem_training ? (psp)->funcs->mem_training((psp), (ops)) : 0)
 
 #define amdgpu_psp_check_fw_loading_status(adev, i) (adev)->firmware.funcs->check_fw_loading_status((adev), (i))
 
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 7/8] drm/amdgpu: reserve vram for memory training

[Tianci Yin]

From: "Tianci.Yin" <tianci.yin@amd.com>

memory training using specific fixed vram segment, reserve these
segments before anyone may allocate it.

Change-Id: I1436755813a565608a2857a683f535377620a637
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
---
 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;
+	}
+	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;
+	}
+
+	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;
+	}
+
+	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);
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training

[Tuikov, Luben]

On 2019-10-10 11:50 p.m., Tianci Yin wrote:
> From: "Tianci.Yin" <tianci.yin@amd.com>
> 
> memory training using specific fixed vram segment, reserve these
> segments before anyone may allocate it.
> 
> Change-Id: I1436755813a565608a2857a683f535377620a637
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
> ---
>  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.

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training

[Alex Deucher]

On Fri, Oct 11, 2019 at 7:23 PM Tuikov, Luben <Luben.Tuikov@amd.com> wrote:
>
> On 2019-10-10 11:50 p.m., Tianci Yin wrote:
> > From: "Tianci.Yin" <tianci.yin@amd.com>
> >
> > memory training using specific fixed vram segment, reserve these
> > segments before anyone may allocate it.
> >
> > Change-Id: I1436755813a565608a2857a683f535377620a637
> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> > Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
> > ---
> >  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.
>
> > +     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".

I wouldn't worry too much about this.  Most kernel code uses lower
case labels now.  They are easier on the eyes.

Alex

>
> 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);
> >
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training

[Koenig, Christian]

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@amd.com>
>>
>> memory training using specific fixed vram segment, reserve these
>> segments before anyone may allocate it.
>>
>> Change-Id: I1436755813a565608a2857a683f535377620a637
>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>> Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
>> ---
>>   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);
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training

[Yin, Tianci (Rico)]

[-- Attachment #1.1: Type: text/plain, Size: 7163 bytes --]

Thanks very much Christian!

Rico
________________________________
From: Koenig, Christian <Christian.Koenig-5C7GfCeVMHo@public.gmane.org>
Sent: Monday, October 14, 2019 16:26
To: Tuikov, Luben <Luben.Tuikov-5C7GfCeVMHo@public.gmane.org>; Yin, Tianci (Rico) <Tianci.Yin@amd.com>; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
Cc: Deucher, Alexander <Alexander.Deucher-5C7GfCeVMHo@public.gmane.org>
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-5C7GfCeVMHo@public.gmane.org>
>>
>> memory training using specific fixed vram segment, reserve these
>> segments before anyone may allocate it.
>>
>> Change-Id: I1436755813a565608a2857a683f535377620a637
>> Reviewed-by: Alex Deucher <alexander.deucher-5C7GfCeVMHo@public.gmane.org>
>> Signed-off-by: Tianci.Yin <tianci.yin-5C7GfCeVMHo@public.gmane.org>
>> ---
>>   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);
>>


[-- Attachment #1.2: Type: text/html, Size: 14788 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-drm-amdgpu-reserve-vram-for-memory-training-v4.patch --]
[-- Type: text/x-patch; name="0001-drm-amdgpu-reserve-vram-for-memory-training-v4.patch", Size: 3986 bytes --]

From 88b7a04f5c6b02ac3003f8b22b0b04bc7a8e08ea Mon Sep 17 00:00:00 2001
From: "Tianci.Yin" <tianci.yin@amd.com>
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@amd.com>
Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
---
 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


[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 8/8] drm/amdgpu/psp: add psp memory training implementation

[Tianci Yin]

From: "Tianci.Yin" <tianci.yin@amd.com>

add memory training implementation code to save resume time.

Change-Id: I625794a780b11d824ab57ef39cc33b872c6dc6c9
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |   1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |   9 ++
 drivers/gpu/drm/amd/amdgpu/psp_v11_0.c  | 171 ++++++++++++++++++++++++
 3 files changed, 181 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index e3d715c31ac9..03c5c18bb51e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -151,6 +151,7 @@ extern uint amdgpu_sdma_phase_quantum;
 extern char *amdgpu_disable_cu;
 extern char *amdgpu_virtual_display;
 extern uint amdgpu_pp_feature_mask;
+extern uint amdgpu_force_long_training;
 extern int amdgpu_job_hang_limit;
 extern int amdgpu_lbpw;
 extern int amdgpu_compute_multipipe;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 5985bd79216d..f03dcc5d185d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -127,6 +127,7 @@ char *amdgpu_disable_cu = NULL;
 char *amdgpu_virtual_display = NULL;
 /* OverDrive(bit 14) disabled by default*/
 uint amdgpu_pp_feature_mask = 0xffffbfff;
+uint amdgpu_force_long_training = 0;
 int amdgpu_job_hang_limit = 0;
 int amdgpu_lbpw = -1;
 int amdgpu_compute_multipipe = -1;
@@ -390,6 +391,14 @@ module_param_named(sched_hw_submission, amdgpu_sched_hw_submission, int, 0444);
 MODULE_PARM_DESC(ppfeaturemask, "all power features enabled (default))");
 module_param_named(ppfeaturemask, amdgpu_pp_feature_mask, uint, 0444);
 
+/**
+ * DOC: forcelongtraining (uint)
+ * Force long memory training in resume.
+ * The default is zero, indicates short training in resume.
+ */
+MODULE_PARM_DESC(forcelongtraining, "force memory long training");
+module_param_named(forcelongtraining, amdgpu_force_long_training, uint, 0444);
+
 /**
  * DOC: pcie_gen_cap (uint)
  * Override PCIE gen speed capabilities. See the CAIL flags in drivers/gpu/drm/amd/include/amd_pcie.h.
diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
index 2ba0f68ced10..074f23c846cb 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
@@ -902,6 +902,174 @@ static int psp_v11_0_rlc_autoload_start(struct psp_context *psp)
 	return psp_rlc_autoload_start(psp);
 }
 
+static int psp_v11_0_memory_training_send_msg(struct psp_context *psp, int msg)
+{
+	int ret = 0;
+	int i = 0;
+	uint32_t data_32 = 0;
+	struct amdgpu_device *adev = psp->adev;
+
+	data_32 = (psp->mem_train_ctx.c2p_train_data_offset >> 20);
+	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, data_32);
+	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, msg);
+
+	/*max 5s*/
+	while (i < 50) {
+		ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+				   0x80000000, 0x80000000, false);
+		if (ret == 0)
+			break;
+		i++;
+	}
+	DRM_DEBUG("%s training %s, cost %d * %dms.\n",
+		  (msg == PSP_BL__DRAM_SHORT_TRAIN) ? "short" : "long",
+		  (ret == 0) ? "succeed" : "failed",
+		  i, adev->usec_timeout/1000);
+	return ret;
+}
+
+static int psp_v11_0_memory_training_fini(struct psp_context *psp)
+{
+	int ret = 0;
+	struct psp_memory_training_context *ctx = &psp->mem_train_ctx;
+
+	ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT;
+	if(ctx->sys_cache) {
+		kfree(ctx->sys_cache);
+		ctx->sys_cache = NULL;
+	}
+
+	return ret;
+}
+
+static int psp_v11_0_memory_training_init(struct psp_context *psp)
+{
+	int ret = 0;
+	struct psp_memory_training_context *ctx = &psp->mem_train_ctx;
+
+	if(ctx->init != PSP_MEM_TRAIN_RESERVE_SUCCESS) {
+		DRM_DEBUG("memory training does not support!\n");
+		return 0;
+	}
+
+	ctx->sys_cache = kzalloc(ctx->train_data_size, GFP_KERNEL);
+	if(ctx->sys_cache == NULL) {
+		DRM_ERROR("alloc mem_train_ctx.sys_cache failed(%d)!\n", ret);
+		ret = -ENOMEM;
+		goto err_out;
+	}
+
+	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);
+	ctx->init = PSP_MEM_TRAIN_INIT_SUCCESS;
+	return 0;
+
+err_out:
+	psp_v11_0_memory_training_fini(psp);
+	return ret;
+}
+
+/*
+ * save and restore proces
+ */
+static int psp_v11_0_memory_training(struct psp_context *psp, uint32_t ops)
+{
+	int ret = 0;
+	uint32_t p2c_header[4];
+	struct psp_memory_training_context *ctx = &psp->mem_train_ctx;
+	uint32_t *pcache = (uint32_t*)ctx->sys_cache;
+
+	if (ctx->init == PSP_MEM_TRAIN_NOT_SUPPORT) {
+		DRM_DEBUG("Memory training does not support.\n");
+		return 0;
+	} else if (ctx->init != PSP_MEM_TRAIN_INIT_SUCCESS) {
+		DRM_ERROR("Please check initialization failure.\n");
+		return -EINVAL;
+	}
+
+	if (psp_v11_0_is_sos_alive(psp)) {
+		DRM_DEBUG("sos is alive, skip memory training.\n");
+		return 0;
+	}
+
+	ret = amdgpu_device_vram_access(psp->adev, ctx->p2c_train_data_offset, p2c_header, sizeof(p2c_header), false);
+	if (ret) {
+		DRM_ERROR("read p2c header failed.\n");
+		return ret;
+	}
+	DRM_DEBUG("sys_cache[%08x,%08x,%08x,%08x] p2c_header[%08x,%08x,%08x,%08x]\n",
+		  pcache[0], pcache[1], pcache[2], pcache[3],
+		  p2c_header[0], p2c_header[1], p2c_header[2], p2c_header[3]);
+
+	if (ops & PSP_MEM_TRAIN_SEND_SHORT_MSG) {
+		DRM_DEBUG("short training depend on restore.\n");
+		ops |= PSP_MEM_TRAIN_RESTORE;
+	}
+
+	if ((ops & PSP_MEM_TRAIN_RESTORE) &&
+	    pcache[0] != MEM_TRAIN_SYSTEM_SIGNATURE) {
+		DRM_DEBUG("sys_cache[0] is invalid, restore depend on save.\n");
+		ops |= PSP_MEM_TRAIN_SAVE;
+	}
+
+	if (p2c_header[0] == MEM_TRAIN_SYSTEM_SIGNATURE &&
+	    !(pcache[0] == MEM_TRAIN_SYSTEM_SIGNATURE &&
+	      pcache[3] == p2c_header[3])) {
+		DRM_DEBUG("sys_cache is invalid or out-of-date, need save training data to sys_cache.\n");
+		ops |= PSP_MEM_TRAIN_SAVE;
+	}
+
+	if ((ops & PSP_MEM_TRAIN_SAVE) &&
+	    p2c_header[0] != MEM_TRAIN_SYSTEM_SIGNATURE) {
+		DRM_DEBUG("p2c_header[0] is invalid, save depend on long training.\n");
+		ops |= PSP_MEM_TRAIN_SEND_LONG_MSG;
+	}
+
+	if (ops & PSP_MEM_TRAIN_SEND_LONG_MSG) {
+		ops &= ~PSP_MEM_TRAIN_SEND_SHORT_MSG;
+		ops |= PSP_MEM_TRAIN_SAVE;
+	}
+
+	DRM_DEBUG("mem training ops:%x.\n", ops);
+
+	if (ops & PSP_MEM_TRAIN_SEND_LONG_MSG) {
+		ret = psp_v11_0_memory_training_send_msg(psp, PSP_BL__DRAM_LONG_TRAIN);
+		if (ret) {
+			DRM_ERROR("send long training msg failed.\n");
+			return ret;
+		}
+	}
+
+	if (ops & PSP_MEM_TRAIN_SAVE) {
+		ret = amdgpu_device_vram_access(psp->adev, ctx->p2c_train_data_offset, ctx->sys_cache, ctx->train_data_size, false);
+		if (ret) {
+			DRM_ERROR("read training data from vram failed.\n");
+			return ret;
+		}
+	}
+
+	if (ops & PSP_MEM_TRAIN_RESTORE) {
+		ret = amdgpu_device_vram_access(psp->adev, ctx->c2p_train_data_offset, ctx->sys_cache, ctx->train_data_size, true);
+		if (ret) {
+			DRM_ERROR("write training data to vram failed.\n");
+			return ret;
+		}
+	}
+
+	if (ops & PSP_MEM_TRAIN_SEND_SHORT_MSG) {
+		ret = psp_v11_0_memory_training_send_msg(psp, (amdgpu_force_long_training > 0) ?
+							 PSP_BL__DRAM_LONG_TRAIN : PSP_BL__DRAM_SHORT_TRAIN);
+		if (ret) {
+			DRM_ERROR("send training msg failed.\n");
+			return ret;
+		}
+	}
+	ctx->training_cnt++;
+	return ret;
+}
+
 static const struct psp_funcs psp_v11_0_funcs = {
 	.init_microcode = psp_v11_0_init_microcode,
 	.bootloader_load_kdb = psp_v11_0_bootloader_load_kdb,
@@ -922,6 +1090,9 @@ static const struct psp_funcs psp_v11_0_funcs = {
 	.ras_trigger_error = psp_v11_0_ras_trigger_error,
 	.ras_cure_posion = psp_v11_0_ras_cure_posion,
 	.rlc_autoload_start = psp_v11_0_rlc_autoload_start,
+	.mem_training_init = psp_v11_0_memory_training_init,
+	.mem_training_fini = psp_v11_0_memory_training_fini,
+	.mem_training = psp_v11_0_memory_training,
 };
 
 void psp_v11_0_set_psp_funcs(struct psp_context *psp)
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 8/8] drm/amdgpu/psp: add psp memory training implementation

[Tuikov, Luben]

On 2019-10-10 11:50 p.m., Tianci Yin wrote:
> From: "Tianci.Yin" <tianci.yin@amd.com>
> 
> add memory training implementation code to save resume time.
> 
> Change-Id: I625794a780b11d824ab57ef39cc33b872c6dc6c9
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h     |   1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |   9 ++
>  drivers/gpu/drm/amd/amdgpu/psp_v11_0.c  | 171 ++++++++++++++++++++++++
>  3 files changed, 181 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index e3d715c31ac9..03c5c18bb51e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -151,6 +151,7 @@ extern uint amdgpu_sdma_phase_quantum;
>  extern char *amdgpu_disable_cu;
>  extern char *amdgpu_virtual_display;
>  extern uint amdgpu_pp_feature_mask;
> +extern uint amdgpu_force_long_training;
>  extern int amdgpu_job_hang_limit;
>  extern int amdgpu_lbpw;
>  extern int amdgpu_compute_multipipe;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 5985bd79216d..f03dcc5d185d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -127,6 +127,7 @@ char *amdgpu_disable_cu = NULL;
>  char *amdgpu_virtual_display = NULL;
>  /* OverDrive(bit 14) disabled by default*/
>  uint amdgpu_pp_feature_mask = 0xffffbfff;
> +uint amdgpu_force_long_training = 0;
>  int amdgpu_job_hang_limit = 0;
>  int amdgpu_lbpw = -1;
>  int amdgpu_compute_multipipe = -1;
> @@ -390,6 +391,14 @@ module_param_named(sched_hw_submission, amdgpu_sched_hw_submission, int, 0444);
>  MODULE_PARM_DESC(ppfeaturemask, "all power features enabled (default))");
>  module_param_named(ppfeaturemask, amdgpu_pp_feature_mask, uint, 0444);
>  
> +/**
> + * DOC: forcelongtraining (uint)
> + * Force long memory training in resume.
> + * The default is zero, indicates short training in resume.
> + */
> +MODULE_PARM_DESC(forcelongtraining, "force memory long training");
> +module_param_named(forcelongtraining, amdgpu_force_long_training, uint, 0444);
> +
>  /**
>   * DOC: pcie_gen_cap (uint)
>   * Override PCIE gen speed capabilities. See the CAIL flags in drivers/gpu/drm/amd/include/amd_pcie.h.
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> index 2ba0f68ced10..074f23c846cb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> @@ -902,6 +902,174 @@ static int psp_v11_0_rlc_autoload_start(struct psp_context *psp)
>  	return psp_rlc_autoload_start(psp);
>  }
>  
> +static int psp_v11_0_memory_training_send_msg(struct psp_context *psp, int msg)
> +{
> +	int ret = 0;
> +	int i = 0;
> +	uint32_t data_32 = 0;
> +	struct amdgpu_device *adev = psp->adev;

NAK! to preinitializing variables.

> +
> +	data_32 = (psp->mem_train_ctx.c2p_train_data_offset >> 20);
> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, data_32);
> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, msg);
> +
> +	/*max 5s*/
> +	while (i < 50) {
> +		ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
> +				   0x80000000, 0x80000000, false);

Hmm, none of the loop body depends on "i"?
So, conceivably one could do this one time instead of 50?
Interesting... So perhaps the "50" couuld be a macro explaining
what kind of constant it is...

> +		if (ret == 0)
> +			break;
> +		i++;
> +	}

NAK to broken up loop definition. Use a for-loop:

for (i = 0; i < 50; i++) {
	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
			   0x80000000, 0x80000000, false);
	if (ret)
		break;
}

This makes the code more secure as well as the loop-body relocatable
into a function, etc. As well as obvious that the body
of the loop does NOT depend on "i" and therefore "50" is completely
arbitratry.

> +	DRM_DEBUG("%s training %s, cost %d * %dms.\n",
> +		  (msg == PSP_BL__DRAM_SHORT_TRAIN) ? "short" : "long",
> +		  (ret == 0) ? "succeed" : "failed",
> +		  i, adev->usec_timeout/1000);
> +	return ret;
> +}
> +
> +static int psp_v11_0_memory_training_fini(struct psp_context *psp)
> +{
> +	int ret = 0;

NAK!

> +	struct psp_memory_training_context *ctx = &psp->mem_train_ctx;
> +
> +	ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT;
> +	if(ctx->sys_cache) {

Space after keyword: "if (...".

> +		kfree(ctx->sys_cache);
> +		ctx->sys_cache = NULL;
> +	}
> +
> +	return ret;

NAK!
This function should be "void"! Like all "free" functions.

> +}
> +
> +static int psp_v11_0_memory_training_init(struct psp_context *psp)
> +{
> +	int ret = 0;

NAK due to pre-initializing "ret".

> +	struct psp_memory_training_context *ctx = &psp->mem_train_ctx;
> +
> +	if(ctx->init != PSP_MEM_TRAIN_RESERVE_SUCCESS) {
> +		DRM_DEBUG("memory training does not support!\n");

Perhaps you mean:
		DRM_DEBUG("Memory training is not supported!\n");

> +		return 0;
> +	}> +
> +	ctx->sys_cache = kzalloc(ctx->train_data_size, GFP_KERNEL);
> +	if(ctx->sys_cache == NULL) {

Space after keyword "if (...".

> +		DRM_ERROR("alloc mem_train_ctx.sys_cache failed(%d)!\n", ret);
> +		ret = -ENOMEM;
> +		goto err_out;
> +	}
> +
> +	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);
> +	ctx->init = PSP_MEM_TRAIN_INIT_SUCCESS;
> +	return 0;
> +
> +err_out:
> +	psp_v11_0_memory_training_fini(psp);
> +	return ret;
> +}
> +
> +/*
> + * save and restore proces
> + */
> +static int psp_v11_0_memory_training(struct psp_context *psp, uint32_t ops)
> +{
> +	int ret = 0;

NAK to pre-initializing return value of function which you don't know
apriori if it will succeed or fail.

Simply, don't pre-initialize automatic variables.

> +	uint32_t p2c_header[4];
> +	struct psp_memory_training_context *ctx = &psp->mem_train_ctx;
> +	uint32_t *pcache = (uint32_t*)ctx->sys_cache;
> +
> +	if (ctx->init == PSP_MEM_TRAIN_NOT_SUPPORT) {
> +		DRM_DEBUG("Memory training does not support.\n");

"Memory training is not supported!"

> +		return 0;
> +	} else if (ctx->init != PSP_MEM_TRAIN_INIT_SUCCESS) {
> +		DRM_ERROR("Please check initialization failure.\n");

"Memory training initialization failure."

> +		return -EINVAL;
> +	}
> +
> +	if (psp_v11_0_is_sos_alive(psp)) {
> +		DRM_DEBUG("sos is alive, skip memory training.\n");
> +		return 0;
> +	}
> +
> +	ret = amdgpu_device_vram_access(psp->adev, ctx->p2c_train_data_offset, p2c_header, sizeof(p2c_header), false);
> +	if (ret) {
> +		DRM_ERROR("read p2c header failed.\n");

"P2C header read failed."

> +		return ret;
> +	}
> +	DRM_DEBUG("sys_cache[%08x,%08x,%08x,%08x] p2c_header[%08x,%08x,%08x,%08x]\n",
> +		  pcache[0], pcache[1], pcache[2], pcache[3],
> +		  p2c_header[0], p2c_header[1], p2c_header[2], p2c_header[3]);
> +
> +	if (ops & PSP_MEM_TRAIN_SEND_SHORT_MSG) {
> +		DRM_DEBUG("short training depend on restore.\n");

Change "depend" --> "depends".
also feel free to start your sentences with a capital letter.

"Short training depends on restore."

Regards,
Luben

> +		ops |= PSP_MEM_TRAIN_RESTORE;
> +	}
> +
> +	if ((ops & PSP_MEM_TRAIN_RESTORE) &&
> +	    pcache[0] != MEM_TRAIN_SYSTEM_SIGNATURE) {
> +		DRM_DEBUG("sys_cache[0] is invalid, restore depend on save.\n");
> +		ops |= PSP_MEM_TRAIN_SAVE;
> +	}
> +
> +	if (p2c_header[0] == MEM_TRAIN_SYSTEM_SIGNATURE &&
> +	    !(pcache[0] == MEM_TRAIN_SYSTEM_SIGNATURE &&
> +	      pcache[3] == p2c_header[3])) {
> +		DRM_DEBUG("sys_cache is invalid or out-of-date, need save training data to sys_cache.\n");
> +		ops |= PSP_MEM_TRAIN_SAVE;
> +	}
> +
> +	if ((ops & PSP_MEM_TRAIN_SAVE) &&
> +	    p2c_header[0] != MEM_TRAIN_SYSTEM_SIGNATURE) {
> +		DRM_DEBUG("p2c_header[0] is invalid, save depend on long training.\n");
> +		ops |= PSP_MEM_TRAIN_SEND_LONG_MSG;
> +	}
> +
> +	if (ops & PSP_MEM_TRAIN_SEND_LONG_MSG) {
> +		ops &= ~PSP_MEM_TRAIN_SEND_SHORT_MSG;
> +		ops |= PSP_MEM_TRAIN_SAVE;
> +	}
> +
> +	DRM_DEBUG("mem training ops:%x.\n", ops);
> +
> +	if (ops & PSP_MEM_TRAIN_SEND_LONG_MSG) {
> +		ret = psp_v11_0_memory_training_send_msg(psp, PSP_BL__DRAM_LONG_TRAIN);
> +		if (ret) {
> +			DRM_ERROR("send long training msg failed.\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (ops & PSP_MEM_TRAIN_SAVE) {
> +		ret = amdgpu_device_vram_access(psp->adev, ctx->p2c_train_data_offset, ctx->sys_cache, ctx->train_data_size, false);
> +		if (ret) {
> +			DRM_ERROR("read training data from vram failed.\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (ops & PSP_MEM_TRAIN_RESTORE) {
> +		ret = amdgpu_device_vram_access(psp->adev, ctx->c2p_train_data_offset, ctx->sys_cache, ctx->train_data_size, true);
> +		if (ret) {
> +			DRM_ERROR("write training data to vram failed.\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (ops & PSP_MEM_TRAIN_SEND_SHORT_MSG) {
> +		ret = psp_v11_0_memory_training_send_msg(psp, (amdgpu_force_long_training > 0) ?
> +							 PSP_BL__DRAM_LONG_TRAIN : PSP_BL__DRAM_SHORT_TRAIN);
> +		if (ret) {
> +			DRM_ERROR("send training msg failed.\n");
> +			return ret;
> +		}
> +	}
> +	ctx->training_cnt++;
> +	return ret;
> +}
> +
>  static const struct psp_funcs psp_v11_0_funcs = {
>  	.init_microcode = psp_v11_0_init_microcode,
>  	.bootloader_load_kdb = psp_v11_0_bootloader_load_kdb,
> @@ -922,6 +1090,9 @@ static const struct psp_funcs psp_v11_0_funcs = {
>  	.ras_trigger_error = psp_v11_0_ras_trigger_error,
>  	.ras_cure_posion = psp_v11_0_ras_cure_posion,
>  	.rlc_autoload_start = psp_v11_0_rlc_autoload_start,
> +	.mem_training_init = psp_v11_0_memory_training_init,
> +	.mem_training_fini = psp_v11_0_memory_training_fini,
> +	.mem_training = psp_v11_0_memory_training,
>  };
>  
>  void psp_v11_0_set_psp_funcs(struct psp_context *psp)
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 0/8] low latency memory training for navi

[Alex Deucher]

This patch set improves the latency for memory training
on navi parts.

Tianci.Yin (8):
  drm/amdgpu: update amdgpu_discovery to handle revision
  drm/amdgpu: add a generic fb accessing helper function
  drm/amdgpu: introduce psp_v11_0_is_sos_alive interface
  drm/amdgpu: update atomfirmware header with memory training related
    members
  drm/amdgpu/atomfirmware: add memory training related helper functions
  drm/amdgpu: add psp memory training callbacks and macro
  drm/amdgpu: reserve vram for memory training
  drm/amdgpu/psp: add psp memory training implementation

 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  10 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c  |   5 +
 .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c  | 130 ++++++++++++
 .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h  |   1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    |  42 ++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c |  17 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h |   2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       |   9 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c       |  18 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h       |  55 +++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       |  96 +++++++++
 drivers/gpu/drm/amd/amdgpu/psp_v11_0.c        | 193 +++++++++++++++++-
 drivers/gpu/drm/amd/include/atomfirmware.h    |  15 ++
 13 files changed, 570 insertions(+), 23 deletions(-)

-- 
2.20.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 5/8] drm/amdgpu/atomfirmware: add memory training related helper functions

[Alex Deucher]

From: "Tianci.Yin" <tianci.yin@amd.com>

parse firmware to get memory training capability and fb location.

Change-Id: I147c1d48e255e0191be4beb1ad6b637da607bf75
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |   7 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c  |   5 +
 .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c  | 130 ++++++++++++++++++
 .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h  |   1 +
 4 files changed, 143 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 0d60c2e6c592..eeb6b6282fce 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -288,6 +288,9 @@ struct amdgpu_ip_block_version {
 	const struct amd_ip_funcs *funcs;
 };
 
+#define hw_revision(major, minor, revision) \
+	((((uint32_t) major) << 16) | ((uint32_t) minor << 8) | ((uint32_t) revision))
+
 struct amdgpu_ip_block {
 	struct amdgpu_ip_block_status status;
 	const struct amdgpu_ip_block_version *version;
@@ -630,6 +633,10 @@ struct amdgpu_fw_vram_usage {
 	u64 size;
 	struct amdgpu_bo *reserved_bo;
 	void *va;
+
+	/*offset on the top of vram, used as c2p write buffer*/
+	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 1c9d40f97a9b..5f5a2d3fff9b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
@@ -2038,6 +2038,11 @@ 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);
+		if(ret) {
+			DRM_ERROR("Failed to get mem train fb location.\n");
+			return ret;
+		}
 	} else {
 		amdgpu_atombios_scratch_regs_init(adev);
 		amdgpu_atombios_allocate_fb_scratch(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
index 39fd8ae5a822..dfaebd929332 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
@@ -27,6 +27,7 @@
 #include "amdgpu_atomfirmware.h"
 #include "atom.h"
 #include "atombios.h"
+#include "soc15_hw_ip.h"
 
 bool amdgpu_atomfirmware_gpu_supports_virtualization(struct amdgpu_device *adev)
 {
@@ -462,3 +463,132 @@ int amdgpu_atomfirmware_get_gfx_info(struct amdgpu_device *adev)
 	}
 	return -EINVAL;
 }
+
+/*
+ * Check if VBIOS supports GDDR6 training data save/restore
+ */
+static bool gddr6_mem_train_vbios_support(struct amdgpu_device *adev)
+{
+	uint16_t data_offset;
+	int index;
+
+	index = get_index_into_master_table(atom_master_list_of_data_tables_v2_1,
+					    firmwareinfo);
+	if (amdgpu_atom_parse_data_header(adev->mode_info.atom_context, index, NULL,
+					  NULL, NULL, &data_offset)) {
+		struct atom_firmware_info_v3_1 *firmware_info =
+			(struct atom_firmware_info_v3_1 *)(adev->mode_info.atom_context->bios +
+							   data_offset);
+
+		DRM_DEBUG("atom firmware capability:0x%08x.\n",
+			  le32_to_cpu(firmware_info->firmware_capability));
+
+		if (le32_to_cpu(firmware_info->firmware_capability) &
+		    ATOM_FIRMWARE_CAP_ENABLE_2STAGE_BIST_TRAINING)
+			return true;
+	}
+
+	return false;
+}
+
+static int gddr6_mem_train_support(struct amdgpu_device *adev)
+{
+	int ret = 0;
+	bool vbios_support = false;
+	uint32_t major, minor, revision, hw_v;
+
+	if (!amdgpu_sriov_vf(adev) &&
+	    gddr6_mem_train_vbios_support(adev)) {
+		vbios_support = true;
+	}
+
+	amdgpu_discovery_get_ip_version(adev, MP0_HWID, &major, &minor, &revision);
+	hw_v = hw_revision(major, minor, revision);
+	/*
+	 * treat 0 revision as a special case since register for MP0 and MMHUB is missing
+	 * for some Navi10 A0, preventing driver from discovering the hwip information since
+	 * none of the functions will be initialized, it should not cause any problems
+	 */
+	switch(hw_v) {
+	case hw_revision(11, 0, 0):
+	case hw_revision(11, 0, 5):
+		ret = vbios_support;
+		break;
+	default:
+		if (vbios_support) {
+			DRM_ERROR("memory training vbios supports but psp hw(%08x)"
+				  " doesn't support!\n", hw_v);
+			ret = -1;
+		}
+		break;
+	}
+
+	DRM_DEBUG("mp0 hw_v %08x, ret:%d.\n", hw_v, ret);
+	return ret;
+}
+
+int amdgpu_atomfirmware_get_mem_train_fb_loc(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;
+	uint8_t frev, crev;
+	uint16_t data_offset, size;
+	uint32_t start_address_in_kb = 0;
+	uint64_t offset;
+	int ret = 0;
+
+	adev->fw_vram_usage.mem_train_support = false;
+	ret = gddr6_mem_train_support(adev);
+	if (ret == -1)
+		return -EINVAL;
+	else if (ret == 0)
+		return 0;
+
+	index = get_index_into_master_table(atom_master_list_of_data_tables_v2_1,
+					    vram_usagebyfirmware);
+	ret = amdgpu_atom_parse_data_header(ctx, index, &size, &frev, &crev,
+					    &data_offset);
+	if (!ret) {
+		DRM_ERROR("parse data header failed.\n");
+		return -EINVAL;
+	}
+
+	DRM_DEBUG("atom firmware common table header size:0x%04x, frev:0x%02x,"
+		  " crev:0x%02x, data_offset:0x%04x.\n", size, frev, crev, data_offset);
+	/* only support 2.1+ */
+	if (((uint16_t)frev << 8 | crev) < 0x0201) {
+		DRM_ERROR("frev:0x%02x, crev:0x%02x < 2.1 !\n", frev, crev);
+		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_K;
+		if ((offset & (ONE_MEG - 1)) < (4 * ONE_K + 1) ) {
+			offset -= ONE_MEG;
+		}
+
+		offset &= ~(ONE_MEG - 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;
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h
index 53449fc7baf4..f871af5ea6f3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h
@@ -31,6 +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_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);
-- 
2.20.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/8] drm/amdgpu/atomfirmware: add memory training related helper functions

[Tuikov, Luben]

On 2019-10-08 3:29 p.m., Alex Deucher wrote:
> From: "Tianci.Yin" <tianci.yin@amd.com>
> 
> parse firmware to get memory training capability and fb location.
> 
> Change-Id: I147c1d48e255e0191be4beb1ad6b637da607bf75
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Tianci.Yin <tianci.yin@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h           |   7 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c  |   5 +
>  .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c  | 130 ++++++++++++++++++
>  .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h  |   1 +
>  4 files changed, 143 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 0d60c2e6c592..eeb6b6282fce 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -288,6 +288,9 @@ struct amdgpu_ip_block_version {
>  	const struct amd_ip_funcs *funcs;
>  };
>  
> +#define hw_revision(major, minor, revision) \
> +	((((uint32_t) major) << 16) | ((uint32_t) minor << 8) | ((uint32_t) revision))
> +
>  struct amdgpu_ip_block {
>  	struct amdgpu_ip_block_status status;
>  	const struct amdgpu_ip_block_version *version;
> @@ -630,6 +633,10 @@ struct amdgpu_fw_vram_usage {
>  	u64 size;
>  	struct amdgpu_bo *reserved_bo;
>  	void *va;
> +
> +	/*offset on the top of vram, used as c2p write buffer*/
> +	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 1c9d40f97a9b..5f5a2d3fff9b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> @@ -2038,6 +2038,11 @@ 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);
> +		if(ret) {

Space after a keyword: "if (ret)" according to LKCS.

> +			DRM_ERROR("Failed to get mem train fb location.\n");
> +			return ret;
> +		}
>  	} else {
>  		amdgpu_atombios_scratch_regs_init(adev);
>  		amdgpu_atombios_allocate_fb_scratch(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> index 39fd8ae5a822..dfaebd929332 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> @@ -27,6 +27,7 @@
>  #include "amdgpu_atomfirmware.h"
>  #include "atom.h"
>  #include "atombios.h"
> +#include "soc15_hw_ip.h"
>  
>  bool amdgpu_atomfirmware_gpu_supports_virtualization(struct amdgpu_device *adev)
>  {
> @@ -462,3 +463,132 @@ int amdgpu_atomfirmware_get_gfx_info(struct amdgpu_device *adev)
>  	}
>  	return -EINVAL;
>  }
> +
> +/*
> + * Check if VBIOS supports GDDR6 training data save/restore
> + */
> +static bool gddr6_mem_train_vbios_support(struct amdgpu_device *adev)
> +{
> +	uint16_t data_offset;
> +	int index;
> +
> +	index = get_index_into_master_table(atom_master_list_of_data_tables_v2_1,
> +					    firmwareinfo);
> +	if (amdgpu_atom_parse_data_header(adev->mode_info.atom_context, index, NULL,
> +					  NULL, NULL, &data_offset)) {
> +		struct atom_firmware_info_v3_1 *firmware_info =
> +			(struct atom_firmware_info_v3_1 *)(adev->mode_info.atom_context->bios +
> +							   data_offset);
> +
> +		DRM_DEBUG("atom firmware capability:0x%08x.\n",
> +			  le32_to_cpu(firmware_info->firmware_capability));
> +
> +		if (le32_to_cpu(firmware_info->firmware_capability) &
> +		    ATOM_FIRMWARE_CAP_ENABLE_2STAGE_BIST_TRAINING)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int gddr6_mem_train_support(struct amdgpu_device *adev)
> +{
> +	int ret = 0;

int ret;
Don't preinitialize. Instead, explicitly set on all contingencies.
This makes the code more secure. See below.

> +	bool vbios_support = false;
> +	uint32_t major, minor, revision, hw_v;
> +
> +	if (!amdgpu_sriov_vf(adev) &&
> +	    gddr6_mem_train_vbios_support(adev)) {
> +		vbios_support = true;
> +	}
> +
> +	amdgpu_discovery_get_ip_version(adev, MP0_HWID, &major, &minor, &revision);
> +	hw_v = hw_revision(major, minor, revision);
> +	/*
> +	 * treat 0 revision as a special case since register for MP0 and MMHUB is missing
> +	 * for some Navi10 A0, preventing driver from discovering the hwip information since
> +	 * none of the functions will be initialized, it should not cause any problems
> +	 */
> +	switch(hw_v) {

Space after keyword: "switch (hw_v) {" according to LKCS.

> +	case hw_revision(11, 0, 0):
> +	case hw_revision(11, 0, 5):
> +		ret = vbios_support;
> +		break;
> +	default:
> +		if (vbios_support) {
> +			DRM_ERROR("memory training vbios supports but psp hw(%08x)"
> +				  " doesn't support!\n", hw_v);
> +			ret = -1;
> +		}

} else {
	ret = 0;
}
break;

> +		break;
> +	}
> +
> +	DRM_DEBUG("mp0 hw_v %08x, ret:%d.\n", hw_v, ret);
> +	return ret;
> +}
> +
> +int amdgpu_atomfirmware_get_mem_train_fb_loc(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;
> +	uint8_t frev, crev;
> +	uint16_t data_offset, size;
> +	uint32_t start_address_in_kb = 0;
> +	uint64_t offset;
> +	int ret = 0;
> +
> +	adev->fw_vram_usage.mem_train_support = false;
> +	ret = gddr6_mem_train_support(adev);
> +	if (ret == -1)
> +		return -EINVAL;
> +	else if (ret == 0)
> +		return 0;
> +
> +	index = get_index_into_master_table(atom_master_list_of_data_tables_v2_1,
> +					    vram_usagebyfirmware);
> +	ret = amdgpu_atom_parse_data_header(ctx, index, &size, &frev, &crev,
> +					    &data_offset);
> +	if (!ret) {

if (ret == 0)
Since it is an integer and not a pointer--compare to 0 integer.

Regards,
Luben

> +		DRM_ERROR("parse data header failed.\n");
> +		return -EINVAL;
> +	}
> +
> +	DRM_DEBUG("atom firmware common table header size:0x%04x, frev:0x%02x,"
> +		  " crev:0x%02x, data_offset:0x%04x.\n", size, frev, crev, data_offset);
> +	/* only support 2.1+ */
> +	if (((uint16_t)frev << 8 | crev) < 0x0201) {
> +		DRM_ERROR("frev:0x%02x, crev:0x%02x < 2.1 !\n", frev, crev);
> +		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_K;
> +		if ((offset & (ONE_MEG - 1)) < (4 * ONE_K + 1) ) {
> +			offset -= ONE_MEG;
> +		}
> +
> +		offset &= ~(ONE_MEG - 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;
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h
> index 53449fc7baf4..f871af5ea6f3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.h
> @@ -31,6 +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_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);
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx