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

["Zhang, Jerry (Junwei)" <Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>]

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