[PATCH 2/3] drm/amdgpu: add psp ecc support

[Huang Rui]

On Mon, Jul 31, 2017 at 04:10:10PM +0800, Zhang, Jerry wrote:
> 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 <Jerry.Zhang at amd.com>
> >> ---
> >>   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.

Not only asic support, but also VBIOS enablement. Yes, we would better
initialize it in sw init and use a boot param to configure it at least
until we confirmed the method to get ecc_mode from VBIOS. At same time, it
should add a comment to like below in sw_init:

/* FIXME: will find a way to get ecc mode via VBIOS */

> 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?
> 

Yes, make sense. But we could introduce the config wrapper when we need add
new configration in future. Currently, it looks a little superfluous.
Anyway, I am also fine if you want to introduce it now, no matter.

Thanks,
Rui