[PATCH] drm/amdgpu: defer test IBs on the rings at boot (V2)

[Alex Deucher]

On Fri, Apr 13, 2018 at 9:25 AM, Christian König
<christian.koenig at amd.com> wrote:
> Am 13.04.2018 um 10:31 schrieb Shirish S:
>>
>> amdgpu_ib_ring_tests() runs test IB's on rings at boot
>> contributes to ~500 ms of amdgpu driver's boot time.
>>
>> This patch defers it and ensures that its executed
>> in amdgpu_info_ioctl() if it wasn't scheduled.
>>
>> V2: Use queue_delayed_work() & flush_delayed_work().
>>
>> Signed-off-by: Shirish S <shirish.s at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  2 ++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 20 +++++++++++++++++---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    |  4 ++++
>>   3 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index 5734871..ae8f722 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -1611,6 +1611,8 @@ struct amdgpu_device {
>>         /* delayed work_func for deferring clockgating during resume */
>>         struct delayed_work     late_init_work;
>> +       /* delayed work_func to defer testing IB's on rings during boot */
>> +       struct delayed_work     late_init_test_ib_work;
>
>
> That still has the chance of running the late init in parallel with the IB
> tests and that really doesn't looks like a good idea to me.

Yeah, at least on older chips we run into problems if we power or
clock gate some engines while they are in use.  Even on engines that
support dynamic gating, you usually have to set it up while the engine
is idle.  Make sure the IB tests run before we enable gating.

Alex

>
> Is there any issue with putting the IB test into the late init work handler
> as well?
>
>
>>         struct amdgpu_virt      virt;
>>         /* firmware VRAM reservation */
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 1762eb4..ee84058 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -63,6 +63,7 @@ MODULE_FIRMWARE("amdgpu/vega12_gpu_info.bin");
>>   MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");
>>     #define AMDGPU_RESUME_MS            2000
>> +#define AMDGPU_IB_TEST_SCHED_MS                2000
>>     static const char *amdgpu_asic_name[] = {
>>         "TAHITI",
>> @@ -2105,6 +2106,16 @@ bool amdgpu_device_asic_has_dc_support(enum
>> amd_asic_type asic_type)
>>         }
>>   }
>>   +static void amdgpu_device_late_init_test_ib_func_handler(struct
>> work_struct *work)
>> +{
>> +       struct amdgpu_device *adev =
>> +               container_of(work, struct amdgpu_device,
>> late_init_test_ib_work.work);
>> +       int r = amdgpu_ib_ring_tests(adev);
>> +
>> +       if (r)
>> +               DRM_ERROR("ib ring test failed (%d).\n", r);
>> +}
>> +
>>   /**
>>    * amdgpu_device_has_dc_support - check if dc is supported
>>    *
>> @@ -2212,6 +2223,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>         INIT_LIST_HEAD(&adev->ring_lru_list);
>>         spin_lock_init(&adev->ring_lru_list_lock);
>>   +     INIT_DELAYED_WORK(&adev->late_init_test_ib_work,
>> +                         amdgpu_device_late_init_test_ib_func_handler);
>>         INIT_DELAYED_WORK(&adev->late_init_work,
>>                           amdgpu_device_ip_late_init_func_handler);
>>   @@ -2374,9 +2387,9 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>                 goto failed;
>>         }
>>   -     r = amdgpu_ib_ring_tests(adev);
>> -       if (r)
>> -               DRM_ERROR("ib ring test failed (%d).\n", r);
>> +       /* Schedule amdgpu_ib_ring_tests() */
>> +       queue_delayed_work(system_wq, &adev->late_init_test_ib_work,
>> +                       msecs_to_jiffies(AMDGPU_IB_TEST_SCHED_MS));
>>         if (amdgpu_sriov_vf(adev))
>>                 amdgpu_virt_init_data_exchange(adev);
>> @@ -2469,6 +2482,7 @@ void amdgpu_device_fini(struct amdgpu_device *adev)
>>         }
>>         adev->accel_working = false;
>>         cancel_delayed_work_sync(&adev->late_init_work);
>> +       cancel_delayed_work_sync(&adev->late_init_test_ib_work);
>>         /* free i2c buses */
>>         if (!amdgpu_device_has_dc_support(adev))
>>                 amdgpu_i2c_fini(adev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index 487d39e..6fa326b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -279,6 +279,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev,
>> void *data, struct drm_file
>>         if (!info->return_size || !info->return_pointer)
>>                 return -EINVAL;
>>   +     /* Ensure IB tests on ring are executed */
>> +       if (delayed_work_pending(&adev->late_init_test_ib_work))
>> +               flush_delayed_work(&adev->late_init_test_ib_work);
>> +
>
>
> You just need to call flush_delayed_work() here without the if.
>
> Regards,
> Christian.
>
>>         switch (info->query) {
>>         case AMDGPU_INFO_ACCEL_WORKING:
>>                 ui32 = adev->accel_working;
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx