[PATCH] drm/amdgpu: Move IB pool init and fini.

[Koenig, Christian]

Am 07.03.19 um 17:59 schrieb Grodzovsky, Andrey:
> On 3/7/19 6:29 AM, Christian König wrote:
>> Am 06.03.19 um 22:25 schrieb Andrey Grodzovsky:
>>> Problem:
>>> Using SDMA for TLB invalidation in certain ASICs exposed a problem
>>> of IB pool not being ready while SDMA already up on Init and already
>>> shutt down while SDMA still running on Fini. This caused
>>> IB allocation failure. Temproary fix was commited into a
>>> bringup branch but this is the generic fix.
>>>
>>> Fix:
>>> Init IB pool rigth after GMC is ready but before SDMA is ready.
>>> Do th opposite for Fini.
>>>
>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 20 +++++++++++---------
>>>    1 file changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 00def57..c05a551 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -1686,6 +1686,13 @@ static int amdgpu_device_ip_init(struct
>>> amdgpu_device *adev)
>>>            }
>>>        }
>>>    +    r = amdgpu_ib_pool_init(adev);
>>> +    if (r) {
>>> +        dev_err(adev->dev, "IB initialization failed (%d).\n", r);
>>> +        amdgpu_vf_error_put(adev, AMDGIM_ERROR_VF_IB_INIT_FAIL, 0, r);
>>> +        goto init_failed;
>>> +    }
>>> +
>>>        r = amdgpu_ucode_create_bo(adev); /* create ucode bo when
>>> sw_init complete*/
>>>        if (r)
>>>            goto init_failed;
>>> @@ -1888,7 +1895,8 @@ static int amdgpu_device_ip_fini(struct
>>> amdgpu_device *adev)
>>>        for (i = 0; i < adev->num_ip_blocks; i++) {
>>>            if (!adev->ip_blocks[i].status.hw)
>>>                continue;
>>> -        if (adev->ip_blocks[i].version->type ==
>>> AMD_IP_BLOCK_TYPE_SMC) {
>>> +        if (adev->ip_blocks[i].version->type ==
>>> AMD_IP_BLOCK_TYPE_SMC ||
>>> +            adev->ip_blocks[i].version->type ==
>>> AMD_IP_BLOCK_TYPE_SDMA) {
>> Why is that necessary?
>
> I assumed SDMA gw fini will call tlb flush but after testing with this
> removed it didn't so ok.
>
>>>                r = adev->ip_blocks[i].version->funcs->hw_fini((void
>>> *)adev);
>>>                /* XXX handle errors */
>>>                if (r) {
>>> @@ -1900,6 +1908,8 @@ static int amdgpu_device_ip_fini(struct
>>> amdgpu_device *adev)
>>>            }
>>>        }
>>>    +    amdgpu_ib_pool_fini(adev);
>> Thinking more about it this should probably be done right before GMC
>> SW fini.
>>
>> IIRC we already have an extra case for this somewhere.
>>
>> Christian.
>
> Why do you think that the proper place ? Seemed to work fine where it was.

IIRC we already freed up a couple of other BOs at this place, e.g. the 
ucode etc..

And you moved the ip_pool_init directly before the ucode allocation, so 
that seems to be a good choice for freeing it up again.

Christian.

>
> Anyway, i moved it and resent v2.
>
> Andrey
>
>
>>> +
>>>        for (i = adev->num_ip_blocks - 1; i >= 0; i--) {
>>>            if (!adev->ip_blocks[i].status.hw)
>>>                continue;
>>> @@ -2651,13 +2661,6 @@ int amdgpu_device_init(struct amdgpu_device
>>> *adev,
>>>        /* Get a log2 for easy divisions. */
>>>        adev->mm_stats.log2_max_MBps = ilog2(max(1u, max_MBps));
>>>    -    r = amdgpu_ib_pool_init(adev);
>>> -    if (r) {
>>> -        dev_err(adev->dev, "IB initialization failed (%d).\n", r);
>>> -        amdgpu_vf_error_put(adev, AMDGIM_ERROR_VF_IB_INIT_FAIL, 0, r);
>>> -        goto failed;
>>> -    }
>>> -
>>>        amdgpu_fbdev_init(adev);
>>>          r = amdgpu_pm_sysfs_init(adev);
>>> @@ -2735,7 +2738,6 @@ void amdgpu_device_fini(struct amdgpu_device
>>> *adev)
>>>            else
>>>                drm_atomic_helper_shutdown(adev->ddev);
>>>        }
>>> -    amdgpu_ib_pool_fini(adev);
>>>        amdgpu_fence_driver_fini(adev);
>>>        amdgpu_pm_sysfs_fini(adev);
>>>        amdgpu_fbdev_fini(adev);