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

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

[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().
V3: removed usage of separate wq, ensure ib tests is
    run before enabling clockgating.

Signed-off-by: Shirish S <shirish.s@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 17 ++++++-----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    |  3 +++
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 1762eb4..f225840 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1656,6 +1656,10 @@ static int amdgpu_device_ip_late_set_cg_state(struct amdgpu_device *adev)
 	if (amdgpu_emu_mode == 1)
 		return 0;
 
+	r = amdgpu_ib_ring_tests(adev);
+	if (r)
+		DRM_ERROR("ib ring test failed (%d).\n", r);
+
 	for (i = 0; i < adev->num_ip_blocks; i++) {
 		if (!adev->ip_blocks[i].status.valid)
 			continue;
@@ -1706,8 +1710,8 @@ static int amdgpu_device_ip_late_init(struct amdgpu_device *adev)
 		}
 	}
 
-	mod_delayed_work(system_wq, &adev->late_init_work,
-			msecs_to_jiffies(AMDGPU_RESUME_MS));
+	queue_delayed_work(system_wq, &adev->late_init_work,
+			   msecs_to_jiffies(AMDGPU_RESUME_MS));
 
 	amdgpu_device_fill_reset_magic(adev);
 
@@ -2374,10 +2378,6 @@ 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);
-
 	if (amdgpu_sriov_vf(adev))
 		amdgpu_virt_init_data_exchange(adev);
 
@@ -2640,11 +2640,6 @@ int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon)
 
 	amdgpu_fence_driver_resume(adev);
 
-	if (resume) {
-		r = amdgpu_ib_ring_tests(adev);
-		if (r)
-			DRM_ERROR("ib ring test failed (%d).\n", r);
-	}
 
 	r = amdgpu_device_ip_late_init(adev);
 	if (r)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 487d39e..83d7160 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -279,6 +279,9 @@ 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 are run on ring */
+	flush_delayed_work(&adev->late_init_work);
+
 	switch (info->query) {
 	case AMDGPU_INFO_ACCEL_WORKING:
 		ui32 = adev->accel_working;
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Christian König]

Am 16.04.2018 um 09:17 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().
> V3: removed usage of separate wq, ensure ib tests is
>      run before enabling clockgating.
>
> Signed-off-by: Shirish S <shirish.s@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 17 ++++++-----------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    |  3 +++
>   2 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 1762eb4..f225840 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1656,6 +1656,10 @@ static int amdgpu_device_ip_late_set_cg_state(struct amdgpu_device *adev)
>   	if (amdgpu_emu_mode == 1)
>   		return 0;
>   
> +	r = amdgpu_ib_ring_tests(adev);
> +	if (r)
> +		DRM_ERROR("ib ring test failed (%d).\n", r);
> +
>   	for (i = 0; i < adev->num_ip_blocks; i++) {
>   		if (!adev->ip_blocks[i].status.valid)
>   			continue;
> @@ -1706,8 +1710,8 @@ static int amdgpu_device_ip_late_init(struct amdgpu_device *adev)
>   		}
>   	}
>   
> -	mod_delayed_work(system_wq, &adev->late_init_work,
> -			msecs_to_jiffies(AMDGPU_RESUME_MS));
> +	queue_delayed_work(system_wq, &adev->late_init_work,
> +			   msecs_to_jiffies(AMDGPU_RESUME_MS));
>   
>   	amdgpu_device_fill_reset_magic(adev);
>   
> @@ -2374,10 +2378,6 @@ 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);
> -
>   	if (amdgpu_sriov_vf(adev))
>   		amdgpu_virt_init_data_exchange(adev);
>   
> @@ -2640,11 +2640,6 @@ int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon)
>   
>   	amdgpu_fence_driver_resume(adev);
>   
> -	if (resume) {
> -		r = amdgpu_ib_ring_tests(adev);
> -		if (r)
> -			DRM_ERROR("ib ring test failed (%d).\n", r);
> -	}
>   
>   	r = amdgpu_device_ip_late_init(adev);
>   	if (r)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 487d39e..83d7160 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -279,6 +279,9 @@ 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 are run on ring */
> +	flush_delayed_work(&adev->late_init_work);
> +
>   	switch (info->query) {
>   	case AMDGPU_INFO_ACCEL_WORKING:
>   		ui32 = adev->accel_working;

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Zhang, Jerry (Junwei)]

On 04/16/2018 03:17 PM, Shirish S wrote:
> 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().
> V3: removed usage of separate wq, ensure ib tests is
>      run before enabling clockgating.
>
> Signed-off-by: Shirish S <shirish.s@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 17 ++++++-----------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    |  3 +++
>   2 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 1762eb4..f225840 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1656,6 +1656,10 @@ static int amdgpu_device_ip_late_set_cg_state(struct amdgpu_device *adev)
>   	if (amdgpu_emu_mode == 1)
>   		return 0;
>
> +	r = amdgpu_ib_ring_tests(adev);
> +	if (r)
> +		DRM_ERROR("ib ring test failed (%d).\n", r);
> +

Just confirm:
IIRC, ib may not run immediately via scheduler thread.
Is there any possible that the actual ib ring test interferes cg enablement?

Jerry

>   	for (i = 0; i < adev->num_ip_blocks; i++) {
>   		if (!adev->ip_blocks[i].status.valid)
>   			continue;
> @@ -1706,8 +1710,8 @@ static int amdgpu_device_ip_late_init(struct amdgpu_device *adev)
>   		}
>   	}
>
> -	mod_delayed_work(system_wq, &adev->late_init_work,
> -			msecs_to_jiffies(AMDGPU_RESUME_MS));
> +	queue_delayed_work(system_wq, &adev->late_init_work,
> +			   msecs_to_jiffies(AMDGPU_RESUME_MS));
>
>   	amdgpu_device_fill_reset_magic(adev);
>
> @@ -2374,10 +2378,6 @@ 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);
> -
>   	if (amdgpu_sriov_vf(adev))
>   		amdgpu_virt_init_data_exchange(adev);
>
> @@ -2640,11 +2640,6 @@ int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon)
>
>   	amdgpu_fence_driver_resume(adev);
>
> -	if (resume) {
> -		r = amdgpu_ib_ring_tests(adev);
> -		if (r)
> -			DRM_ERROR("ib ring test failed (%d).\n", r);
> -	}
>
>   	r = amdgpu_device_ip_late_init(adev);
>   	if (r)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 487d39e..83d7160 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -279,6 +279,9 @@ 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 are run on ring */
> +	flush_delayed_work(&adev->late_init_work);
> +
>   	switch (info->query) {
>   	case AMDGPU_INFO_ACCEL_WORKING:
>   		ui32 = adev->accel_working;
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Christian König]

Am 16.04.2018 um 11:04 schrieb Zhang, Jerry (Junwei):
> On 04/16/2018 03:17 PM, Shirish S wrote:
>> 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().
>> V3: removed usage of separate wq, ensure ib tests is
>>      run before enabling clockgating.
>>
>> Signed-off-by: Shirish S <shirish.s@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 17 ++++++-----------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    |  3 +++
>>   2 files changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 1762eb4..f225840 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -1656,6 +1656,10 @@ static int 
>> amdgpu_device_ip_late_set_cg_state(struct amdgpu_device *adev)
>>       if (amdgpu_emu_mode == 1)
>>           return 0;
>>
>> +    r = amdgpu_ib_ring_tests(adev);
>> +    if (r)
>> +        DRM_ERROR("ib ring test failed (%d).\n", r);
>> +
>
> Just confirm:
> IIRC, ib may not run immediately via scheduler thread.
> Is there any possible that the actual ib ring test interferes cg 
> enablement?

No, the IB tests are run directly on the hardware ring and not through 
the scheduler.

But even when we run them through the scheduler we wait for the to 
complete, so that shouldn't be an issue.

Christian.

>
> Jerry
>
>>       for (i = 0; i < adev->num_ip_blocks; i++) {
>>           if (!adev->ip_blocks[i].status.valid)
>>               continue;
>> @@ -1706,8 +1710,8 @@ static int amdgpu_device_ip_late_init(struct 
>> amdgpu_device *adev)
>>           }
>>       }
>>
>> -    mod_delayed_work(system_wq, &adev->late_init_work,
>> -            msecs_to_jiffies(AMDGPU_RESUME_MS));
>> +    queue_delayed_work(system_wq, &adev->late_init_work,
>> +               msecs_to_jiffies(AMDGPU_RESUME_MS));
>>
>>       amdgpu_device_fill_reset_magic(adev);
>>
>> @@ -2374,10 +2378,6 @@ 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);
>> -
>>       if (amdgpu_sriov_vf(adev))
>>           amdgpu_virt_init_data_exchange(adev);
>>
>> @@ -2640,11 +2640,6 @@ int amdgpu_device_resume(struct drm_device 
>> *dev, bool resume, bool fbcon)
>>
>>       amdgpu_fence_driver_resume(adev);
>>
>> -    if (resume) {
>> -        r = amdgpu_ib_ring_tests(adev);
>> -        if (r)
>> -            DRM_ERROR("ib ring test failed (%d).\n", r);
>> -    }
>>
>>       r = amdgpu_device_ip_late_init(adev);
>>       if (r)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index 487d39e..83d7160 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -279,6 +279,9 @@ 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 are run on ring */
>> +    flush_delayed_work(&adev->late_init_work);
>> +
>>       switch (info->query) {
>>       case AMDGPU_INFO_ACCEL_WORKING:
>>           ui32 = adev->accel_working;
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Zhang, Jerry (Junwei)]

On 04/16/2018 05:19 PM, Christian König wrote:
> Am 16.04.2018 um 11:04 schrieb Zhang, Jerry (Junwei):
>> On 04/16/2018 03:17 PM, Shirish S wrote:
>>> 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().
>>> V3: removed usage of separate wq, ensure ib tests is
>>>      run before enabling clockgating.
>>>
>>> Signed-off-by: Shirish S <shirish.s@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 17 ++++++-----------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    |  3 +++
>>>   2 files changed, 9 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 1762eb4..f225840 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -1656,6 +1656,10 @@ static int amdgpu_device_ip_late_set_cg_state(struct
>>> amdgpu_device *adev)
>>>       if (amdgpu_emu_mode == 1)
>>>           return 0;
>>>
>>> +    r = amdgpu_ib_ring_tests(adev);
>>> +    if (r)
>>> +        DRM_ERROR("ib ring test failed (%d).\n", r);
>>> +
>>
>> Just confirm:
>> IIRC, ib may not run immediately via scheduler thread.
>> Is there any possible that the actual ib ring test interferes cg enablement?
>
> No, the IB tests are run directly on the hardware ring and not through the
> scheduler.
>
> But even when we run them through the scheduler we wait for the to complete, so
> that shouldn't be an issue.

Thanks for explanation.
That's fine.

Jerry

>
> Christian.
>
>>
>> Jerry
>>
>>>       for (i = 0; i < adev->num_ip_blocks; i++) {
>>>           if (!adev->ip_blocks[i].status.valid)
>>>               continue;
>>> @@ -1706,8 +1710,8 @@ static int amdgpu_device_ip_late_init(struct
>>> amdgpu_device *adev)
>>>           }
>>>       }
>>>
>>> -    mod_delayed_work(system_wq, &adev->late_init_work,
>>> -            msecs_to_jiffies(AMDGPU_RESUME_MS));
>>> +    queue_delayed_work(system_wq, &adev->late_init_work,
>>> +               msecs_to_jiffies(AMDGPU_RESUME_MS));
>>>
>>>       amdgpu_device_fill_reset_magic(adev);
>>>
>>> @@ -2374,10 +2378,6 @@ 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);
>>> -
>>>       if (amdgpu_sriov_vf(adev))
>>>           amdgpu_virt_init_data_exchange(adev);
>>>
>>> @@ -2640,11 +2640,6 @@ int amdgpu_device_resume(struct drm_device *dev, bool
>>> resume, bool fbcon)
>>>
>>>       amdgpu_fence_driver_resume(adev);
>>>
>>> -    if (resume) {
>>> -        r = amdgpu_ib_ring_tests(adev);
>>> -        if (r)
>>> -            DRM_ERROR("ib ring test failed (%d).\n", r);
>>> -    }
>>>
>>>       r = amdgpu_device_ip_late_init(adev);
>>>       if (r)
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> index 487d39e..83d7160 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> @@ -279,6 +279,9 @@ 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 are run on ring */
>>> +    flush_delayed_work(&adev->late_init_work);
>>> +
>>>       switch (info->query) {
>>>       case AMDGPU_INFO_ACCEL_WORKING:
>>>           ui32 = adev->accel_working;
>>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx