On 07/31/2017 01:57 PM, Huang Rui wrote:
On Fri, Jul 28, 2017 at 05:11:18PM +0800, Junwei Zhang wrote:
Signed-off-by: Junwei Zhang <[email protected]>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 17 ++++++++++++++---
  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 22 ++++++++++++++++++++++
  2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 1aa41af..b04cc80 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -256,6 +256,10 @@ static int psp_hw_start(struct psp_context *psp)
  {
        int ret;

+       ret = psp_bootloader_set_ecc_mode(psp);
+       if (ret)
+               return ret;
+
        ret = psp_bootloader_load_sysdrv(psp);
        if (ret)
                return ret;
@@ -365,9 +369,16 @@ static int psp_load_fw(struct amdgpu_device *adev)
        if (ret)
                goto failed_mem;

-       ret = psp_hw_start(psp);
-       if (ret)
-               goto failed_mem;
+       if (psp_bootloader_is_sos_running(psp) &&
+                       psp->config.ecc_mode != PSP_ECC_MODE__NONE) {

It need set a default value to config psp->ecc_mode, otherwise, it is
always "0" in this implementation.

For ASIC support ECC, it's set in psp_sw_init(), like vega10 case.
For ASIC not support ECC yet, it always "0", indicating PSP_ECC_MODE__NONE,
aka not enabled by default.

It's expected result, I think.


+               if (psp_ring_create(psp, PSP_RING_TYPE__KM))
+                       goto failed_mem;
+               if (psp_tmr_load(psp))
+                       goto failed_mem;
+       } else {
+               if (psp_hw_start(psp))
+                       goto failed_mem;
+       }

        ret = psp_np_fw_load(psp);
        if (ret)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
index 3776186..8ec9194 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
@@ -63,6 +63,19 @@ enum psp_bootloader_command_list
        PSP_BL__DEFAULT_ECC = 0x30003,
  };

+enum psp_ecc_mode
+{
+       PSP_ECC_MODE__NONE = 0,
+       PSP_ECC_MODE__OFF = 1,
+       PSP_ECC_MODE__ON = 2,
+       PSP_ECC_MODE__PARTIALON = 3,
+};
+
+struct psp_config
+{
+       enum psp_ecc_mode               ecc_mode;
+};
+
  struct psp_context
  {
        struct amdgpu_device            *adev;
@@ -70,6 +83,8 @@ struct psp_context
        struct psp_gfx_cmd_resp         *cmd;

        int (*init_microcode)(struct psp_context *psp);
+       int (*bootloader_set_ecc_mode)(struct psp_context *psp);
+       bool (*bootloader_is_sos_running)(struct psp_context *psp);
        int (*bootloader_load_sysdrv)(struct psp_context *psp);
        int (*bootloader_load_sos)(struct psp_context *psp);
        int (*prep_cmd_buf)(struct amdgpu_firmware_info *ucode,
@@ -123,6 +138,9 @@ struct psp_context
        struct amdgpu_bo                *cmd_buf_bo;
        uint64_t                        cmd_buf_mc_addr;
        struct psp_gfx_cmd_resp         *cmd_buf_mem;
+
+       /* psp config */
+       struct psp_config               config;

At current, we don't need a psp_config wrapper here. Use "enum ecc_mode"
directly to make code more simple.

I considered it twice when implemented the code.
IMO, it's good way to collect all config info in a structure like gfx config. It's not only used for ECC, but an initial step for psp_config.

How do you think about it?

Jerry


  };

  struct amdgpu_psp_funcs {
@@ -140,6 +158,10 @@ struct amdgpu_psp_funcs {
                (psp)->compare_sram_data((psp), (ucode), (type))
  #define psp_init_microcode(psp) \
                ((psp)->init_microcode ? (psp)->init_microcode((psp)) : 0)
+#define psp_bootloader_set_ecc_mode(psp) \
+               ((psp)->bootloader_set_ecc_mode ? 
(psp)->bootloader_set_ecc_mode((psp)) : 0)
+#define psp_bootloader_is_sos_running(psp) \
+               ((psp)->bootloader_is_sos_running ? 
(psp)->bootloader_is_sos_running((psp)) : 0)
  #define psp_bootloader_load_sysdrv(psp) \
                ((psp)->bootloader_load_sysdrv ? 
(psp)->bootloader_load_sysdrv((psp)) : 0)
  #define psp_bootloader_load_sos(psp) \
--
1.9.1

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Reply via email to