[v2] drm/amdgpu: fix scheduler timeout calc

Submitted by Flora Cui on June 27, 2019, 10:03 a.m.

Details

Message ID 1561629780-21649-1-git-send-email-flora.cui@amd.com
State New
Headers show
Series "drm/amdgpu: fix scheduler timeout calc" ( rev: 4 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Flora Cui June 27, 2019, 10:03 a.m.
scheduler timeout is in jiffies
v2: move timeout check to amdgpu_device_get_job_timeout_settings after
parsing the value

Change-Id: I26708c163db943ff8d930dd81bcab4b4b9d84eb2
Signed-off-by: Flora Cui <flora.cui@amd.com>

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.7.4

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index e74a175..cc29d70 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1300,7 +1300,9 @@  int amdgpu_device_get_job_timeout_settings(struct amdgpu_device *adev)
 	 * By default timeout for non compute jobs is 10000.
 	 * And there is no timeout enforced on compute jobs.
 	 */
-	adev->gfx_timeout = adev->sdma_timeout = adev->video_timeout = 10000;
+	adev->gfx_timeout = \
+		adev->sdma_timeout = \
+		adev->video_timeout = msecs_to_jiffies(10000);
 	adev->compute_timeout = MAX_SCHEDULE_TIMEOUT;
 
 	if (strnlen(input, AMDGPU_MAX_TIMEOUT_PARAM_LENTH)) {
@@ -1314,7 +1316,8 @@  int amdgpu_device_get_job_timeout_settings(struct amdgpu_device *adev)
 			if (timeout <= 0) {
 				index++;
 				continue;
-			}
+			} else
+				timeout = msecs_to_jiffies(timeout);
 
 			switch (index++) {
 			case 0:

Comments

Am 27.06.19 um 12:03 schrieb Cui, Flora:
> scheduler timeout is in jiffies
> v2: move timeout check to amdgpu_device_get_job_timeout_settings after
> parsing the value
>
> Change-Id: I26708c163db943ff8d930dd81bcab4b4b9d84eb2
> Signed-off-by: Flora Cui <flora.cui@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index e74a175..cc29d70 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -1300,7 +1300,9 @@ int amdgpu_device_get_job_timeout_settings(struct amdgpu_device *adev)
>   	 * By default timeout for non compute jobs is 10000.
>   	 * And there is no timeout enforced on compute jobs.
>   	 */
> -	adev->gfx_timeout = adev->sdma_timeout = adev->video_timeout = 10000;
> +	adev->gfx_timeout = \
> +		adev->sdma_timeout = \
> +		adev->video_timeout = msecs_to_jiffies(10000);

Of hand that looks very odd to me. This is not a macro so why the \ here?

>   	adev->compute_timeout = MAX_SCHEDULE_TIMEOUT;
>   
>   	if (strnlen(input, AMDGPU_MAX_TIMEOUT_PARAM_LENTH)) {
> @@ -1314,7 +1316,8 @@ int amdgpu_device_get_job_timeout_settings(struct amdgpu_device *adev)
>   			if (timeout <= 0) {
>   				index++;
>   				continue;
> -			}
> +			} else
> +				timeout = msecs_to_jiffies(timeout);

You can actually remove the "if (timeout <= 0)" as well, 
msecs_to_jiffies will do the right thing for negative values.

Christian.

>   
>   			switch (index++) {
>   			case 0:
在 6/27/2019 6:17 PM, Christian König 写道:
> Am 27.06.19 um 12:03 schrieb Cui, Flora:

>> scheduler timeout is in jiffies

>> v2: move timeout check to amdgpu_device_get_job_timeout_settings after

>> parsing the value

>>

>> Change-Id: I26708c163db943ff8d930dd81bcab4b4b9d84eb2

>> Signed-off-by: Flora Cui <flora.cui@amd.com>

>> ---

>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 7 +++++--

>>   1 file changed, 5 insertions(+), 2 deletions(-)

>>

>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 

>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

>> index e74a175..cc29d70 100644

>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

>> @@ -1300,7 +1300,9 @@ int 

>> amdgpu_device_get_job_timeout_settings(struct amdgpu_device *adev)

>>        * By default timeout for non compute jobs is 10000.

>>        * And there is no timeout enforced on compute jobs.

>>        */

>> -    adev->gfx_timeout = adev->sdma_timeout = adev->video_timeout = 

>> 10000;

>> +    adev->gfx_timeout = \

>> +        adev->sdma_timeout = \

>> +        adev->video_timeout = msecs_to_jiffies(10000);

>

> Of hand that looks very odd to me. This is not a macro so why the \ here?

will update in v3
>

>>       adev->compute_timeout = MAX_SCHEDULE_TIMEOUT;

>>         if (strnlen(input, AMDGPU_MAX_TIMEOUT_PARAM_LENTH)) {

>> @@ -1314,7 +1316,8 @@ int 

>> amdgpu_device_get_job_timeout_settings(struct amdgpu_device *adev)

>>               if (timeout <= 0) {

>>                   index++;

>>                   continue;

>> -            }

>> +            } else

>> +                timeout = msecs_to_jiffies(timeout);

>

> You can actually remove the "if (timeout <= 0)" as well, 

> msecs_to_jiffies will do the right thing for negative values.

IMHO check for timeout==0 is still needed. msecs_to_jiffies() would 
return 0 and that's not desired for scheduler timer.
>

> Christian.

>

>>                 switch (index++) {

>>               case 0:

>
Am 28.06.19 um 07:32 schrieb Cui, Flora:
> 在 6/27/2019 6:17 PM, Christian König 写道:

>> Am 27.06.19 um 12:03 schrieb Cui, Flora:

>>> scheduler timeout is in jiffies

>>> v2: move timeout check to amdgpu_device_get_job_timeout_settings after

>>> parsing the value

>>>

>>> Change-Id: I26708c163db943ff8d930dd81bcab4b4b9d84eb2

>>> Signed-off-by: Flora Cui <flora.cui@amd.com>

>>> ---

>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 7 +++++--

>>>    1 file changed, 5 insertions(+), 2 deletions(-)

>>>

>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

>>> index e74a175..cc29d70 100644

>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

>>> @@ -1300,7 +1300,9 @@ int

>>> amdgpu_device_get_job_timeout_settings(struct amdgpu_device *adev)

>>>         * By default timeout for non compute jobs is 10000.

>>>         * And there is no timeout enforced on compute jobs.

>>>         */

>>> -    adev->gfx_timeout = adev->sdma_timeout = adev->video_timeout =

>>> 10000;

>>> +    adev->gfx_timeout = \

>>> +        adev->sdma_timeout = \

>>> +        adev->video_timeout = msecs_to_jiffies(10000);

>> Of hand that looks very odd to me. This is not a macro so why the \ here?

> will update in v3

>>>        adev->compute_timeout = MAX_SCHEDULE_TIMEOUT;

>>>          if (strnlen(input, AMDGPU_MAX_TIMEOUT_PARAM_LENTH)) {

>>> @@ -1314,7 +1316,8 @@ int

>>> amdgpu_device_get_job_timeout_settings(struct amdgpu_device *adev)

>>>                if (timeout <= 0) {

>>>                    index++;

>>>                    continue;

>>> -            }

>>> +            } else

>>> +                timeout = msecs_to_jiffies(timeout);

>> You can actually remove the "if (timeout <= 0)" as well,

>> msecs_to_jiffies will do the right thing for negative values.

> IMHO check for timeout==0 is still needed. msecs_to_jiffies() would

> return 0 and that's not desired for scheduler timer.


Good point, so 0 would use the default value and negative values would 
use infinity.

That sounds like a good solution to me,
Christian.

>> Christian.

>>

>>>                  switch (index++) {

>>>                case 0: