[PATCH] drm/amdgpu: fix PSP autoload twice in FLR

[Liu, Cheng Zhe]

[AMD Official Use Only - Internal Distribution Only]

Thanks for the modification!

-----Original Message-----
From: Tuikov, Luben <Luben.Tuikov at amd.com> 
Sent: Wednesday, July 29, 2020 2:49 AM
To: Liu, Cheng Zhe <ChengZhe.Liu at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig at amd.com>; Deucher, Alexander <Alexander.Deucher at amd.com>; Xiao, Jack <Jack.Xiao at amd.com>; Zhang, Hawking <Hawking.Zhang at amd.com>; Xu, Feifei <Feifei.Xu at amd.com>; Wang, Kevin(Yang) <Kevin1.Wang at amd.com>; Yuan, Xiaojie <Xiaojie.Yuan at amd.com>
Subject: Re: [PATCH] drm/amdgpu: fix PSP autoload twice in FLR

On 2020-07-28 2:04 p.m., Luben Tuikov wrote:
> Thanks for removing the braces.
> 
> On 2020-07-27 10:29 p.m., Liu ChengZhe wrote:
>> the block->status.hw = false assignment will overwrite PSP's previous
> ^^^^^^
> You want to start a sentence here. Capitalize "The".
> Also don't use future tense in commit descriptions (and commit titles).
> Simply use present tense. Using future tense makes it confusing if 
> this is what the code used to do before this change or if the code is 
> doing this right now as someone is reading the commit in the future with "git log".
> 
>> hw status, which will cause PSP execute resume operation after hw init.
> 
> I've found it best to describe what's being done as if telling a story.
> Break it down into "tell what's happening" and "tell what what's fixed 
> and how it affects the rest of the system". Something like this:
> 
> 	Assigning false to block->status.hw overwrites PSP's previous
> 	hardware status, which causes the PSP to resume operation after
> 	hardware init.
> 
> 	Remove this assignment and let the PSP start when it is told to.
> 
> Check if the above rendition of your change is correct, and use it if so.

Double checking now, since "resume" is an op, you should capitalize it.

"... which causes the PSP to execute Resume operation right  after hardware init.

 Remove this assignment and let the PSP execute Resume operation when it  is told to do so."

Or something to that effect.

Regards,
Luben

> 
> Regards,
> Luben
> 
>>
>> v2: (R)remove the braces(.)
> 
> 
> 
>>
>> Signed-off-by: Liu ChengZhe <ChengZhe.Liu at amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 62ecac97fbd2..5d9affa1d35a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -2574,6 +2574,9 @@ static int amdgpu_device_ip_reinit_early_sriov(struct amdgpu_device *adev)
>>  		AMD_IP_BLOCK_TYPE_IH,
>>  	};
>>  
>> +	for (i = 0; i < adev->num_ip_blocks; i++)
>> +		adev->ip_blocks[i].status.hw = false;
>> +
>>  	for (i = 0; i < ARRAY_SIZE(ip_order); i++) {
>>  		int j;
>>  		struct amdgpu_ip_block *block;
>> @@ -2581,7 +2584,6 @@ static int amdgpu_device_ip_reinit_early_sriov(struct amdgpu_device *adev)
>>  		for (j = 0; j < adev->num_ip_blocks; j++) {
>>  			block = &adev->ip_blocks[j];
>>  
>> -			block->status.hw = false;
>>  			if (block->version->type != ip_order[i] ||
>>  				!block->status.valid)
>>  				continue;
>>
>