[PATCH 3/6] drm/amdgpu:re-write sriov_reinit_early/late

[Alex Deucher]

On Wed, May 3, 2017 at 5:10 AM, Liu, Monk <Monk.Liu at amd.com> wrote:
> It's correct and already working  on vega10/tonga for days,
> In fact the guilty context already works at my side

Need to use ARRAY_SIZE for the the loops rather than open coding it.
Beyond that, if it works for sr-iov, it's fine.  Maybe we can look at
unifying things for sr-iov and bare metal in this case in the future.
With the ARRAY_SIZE change:

Reviewed-by: Alex Deucher <alexander.deucher at amd.com>

Alex

>
> BR Monk
>
>
>
> -----Original Message-----
> From: Christian König [mailto:deathsimple at vodafone.de]
> Sent: Wednesday, May 03, 2017 5:02 PM
> To: Liu, Monk <Monk.Liu at amd.com>; amd-gfx at lists.freedesktop.org
> Subject: Re: [PATCH 3/6] drm/amdgpu:re-write sriov_reinit_early/late
>
> Am 03.05.2017 um 05:48 schrieb Monk Liu:
>> 1,this way we make those routines compatible with the sequence
>>    requirment for both Tonga and Vega10 2,ignore PSP hw init when
>> doing TDR, because for SR-IOV device the ucode won't get lost after VF
>> FLR, so no need to invoke PSP doing the ucode reloading again.
>>
>> Signed-off-by: Monk Liu <Monk.Liu at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 63 ++++++++++++++++++------------
>>   1 file changed, 39 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 5161c20..5573792 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -1718,19 +1718,27 @@ static int amdgpu_sriov_reinit_early(struct amdgpu_device *adev)
>>   {
>>       int i, r;
>>
>> -     for (i = 0; i < adev->num_ip_blocks; i++) {
>> -             if (!adev->ip_blocks[i].status.valid)
>> -                     continue;
>> -
>> -             if (adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_COMMON ||
>> -                             adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_GMC ||
>> -                             adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_IH)
>> -                     r = adev->ip_blocks[i].version->funcs->hw_init(adev);
>> +     static enum amd_ip_block_type ip_order[] = {
>> +             AMD_IP_BLOCK_TYPE_GMC,
>> +             AMD_IP_BLOCK_TYPE_COMMON,
>> +             AMD_IP_BLOCK_TYPE_GFXHUB,
>> +             AMD_IP_BLOCK_TYPE_MMHUB,
>> +             AMD_IP_BLOCK_TYPE_IH,
>> +     };
>> +
>> +     for (i = 0; i < sizeof(ip_order)/sizeof(ip_order[0]); i++) {
>
> You should use ARRAY_SIZE here instead.
>
>> +             int j;
>> +             struct amdgpu_ip_block *block;
>> +
>> +             for (j = 0; j < adev->num_ip_blocks; j++) {
>> +                     block = &adev->ip_blocks[j];
>> +
>> +                     if (block->version->type != ip_order[i] ||
>> +                             !block->status.valid)
>> +                             continue;
>>
>> -             if (r) {
>> -                     DRM_ERROR("resume of IP block <%s> failed %d\n",
>> -                               adev->ip_blocks[i].version->funcs->name, r);
>> -                     return r;
>> +                     r = block->version->funcs->hw_init(adev);
>> +                     DRM_INFO("RE-INIT: %s %s\n", block->version->funcs->name,
>> +r?"failed":"successed");
>>               }
>>       }
>>
>> @@ -1741,20 +1749,27 @@ static int amdgpu_sriov_reinit_late(struct amdgpu_device *adev)
>>   {
>>       int i, r;
>>
>> -     for (i = 0; i < adev->num_ip_blocks; i++) {
>> -             if (!adev->ip_blocks[i].status.valid)
>> -                     continue;
>> +     static enum amd_ip_block_type ip_order[] = {
>> +             AMD_IP_BLOCK_TYPE_SMC,
>> +             AMD_IP_BLOCK_TYPE_DCE,
>> +             AMD_IP_BLOCK_TYPE_GFX,
>> +             AMD_IP_BLOCK_TYPE_SDMA,
>> +             AMD_IP_BLOCK_TYPE_VCE,
>> +     };
>>
>> -             if (adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_COMMON ||
>> -                             adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_GMC ||
>> -                             adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_IH )
>> -                     continue;
>> +     for (i = 0; i < sizeof(ip_order)/sizeof(ip_order[0]); i++) {
>
> And here as well.
>
>> +             int j;
>> +             struct amdgpu_ip_block *block;
>>
>> -             r = adev->ip_blocks[i].version->funcs->hw_init(adev);
>> -             if (r) {
>> -                     DRM_ERROR("resume of IP block <%s> failed %d\n",
>> -                               adev->ip_blocks[i].version->funcs->name, r);
>> -                     return r;
>> +             for (j = 0; j < adev->num_ip_blocks; j++) {
>> +                     block = &adev->ip_blocks[j];
>> +
>> +                     if (block->version->type != ip_order[i] ||
>> +                             !block->status.valid)
>> +                             continue;
>> +
>> +                     r = block->version->funcs->hw_init(adev);
>> +                     DRM_INFO("RE-INIT: %s %s\n", block->version->funcs->name,
>> +r?"failed":"successed");
>
> This changes the order in which blocks are initialized which is probably not correct.
>
> Alex needs to take a look at this, but we clearly need to improve the handling here.
>
> Regards,
> Christian.
>
>>               }
>>       }
>>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx