All of lore.kernel.org
 help / color / mirror / Atom feed
* Fix hardware accelerated video playback with amdgpu on 32Bit system
@ 2017-03-29  9:18 Jan Burgmeier
  2017-03-29  9:18 ` [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems Jan Burgmeier
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Burgmeier @ 2017-03-29  9:18 UTC (permalink / raw)
  To: amd-gfx, dri-devel, linux-kernel; +Cc: airlied

Hi,

on 32Bit systems hardware accelerated video playback with amdgpu always errors
out with the following message:
	"[drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* IB va_start+ib_bytes is invalid."

Attached you find a patch witch fixes the problem. The patch was made against the staging-next branch.

The used hardware is:
HP T630 Thin Client
CPU: AMD Embedded G-Series GX-420GI Radeon R7E
GPU: Advanced Micro Devices, Inc. [AMD/ATI] Carrizo (rev 88)

Regards,
Jan Burgmeier

^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems
  2017-03-29  9:18 Fix hardware accelerated video playback with amdgpu on 32Bit system Jan Burgmeier
@ 2017-03-29  9:18 ` Jan Burgmeier
  2017-03-29 13:22     ` Christian König
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Burgmeier @ 2017-03-29  9:18 UTC (permalink / raw)
  To: amd-gfx, dri-devel, linux-kernel; +Cc: airlied

Signed-off-by: Jan Burgmeier <jan.burgmeier@unicon-software.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 99424cb8020b..583d22974e14 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -908,6 +908,7 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device *adev,
 			struct amdgpu_bo *aobj = NULL;
 			uint64_t offset;
 			uint8_t *kptr;
+			uint64_t it_last;
 
 			m = amdgpu_cs_find_mapping(parser, chunk_ib->va_start,
 						   &aobj);
@@ -916,8 +917,9 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device *adev,
 				return -EINVAL;
 			}
 
+			it_last = m->it.last;
 			if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
-			    (m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
+			    (it_last + 1) * AMDGPU_GPU_PAGE_SIZE) {
 				DRM_ERROR("IB va_start+ib_bytes is invalid\n");
 				return -EINVAL;
 			}
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems
@ 2017-03-29 13:22     ` Christian König
  0 siblings, 0 replies; 11+ messages in thread
From: Christian König @ 2017-03-29 13:22 UTC (permalink / raw)
  To: Jan Burgmeier, amd-gfx, dri-devel, linux-kernel; +Cc: airlied

Am 29.03.2017 um 11:18 schrieb Jan Burgmeier:
> Signed-off-by: Jan Burgmeier <jan.burgmeier@unicon-software.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 99424cb8020b..583d22974e14 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -908,6 +908,7 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device *adev,
>   			struct amdgpu_bo *aobj = NULL;
>   			uint64_t offset;
>   			uint8_t *kptr;
> +			uint64_t it_last;
>   
>   			m = amdgpu_cs_find_mapping(parser, chunk_ib->va_start,
>   						   &aobj);
> @@ -916,8 +917,9 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device *adev,
>   				return -EINVAL;
>   			}
>   
> +			it_last = m->it.last;
>   			if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
> -			    (m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
> +			    (it_last + 1) * AMDGPU_GPU_PAGE_SIZE) {

Nice catch, but just adding a u64 case should do here as well. E.g:

if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
     (u64)(m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {

With that fixed the patch is Reviewed-by: Christian König 
<christian.koenig@amd.com>.

Regards,
Christian.

>   				DRM_ERROR("IB va_start+ib_bytes is invalid\n");
>   				return -EINVAL;
>   			}

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems
@ 2017-03-29 13:22     ` Christian König
  0 siblings, 0 replies; 11+ messages in thread
From: Christian König @ 2017-03-29 13:22 UTC (permalink / raw)
  To: Jan Burgmeier, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: airlied-cv59FeDIM0c

Am 29.03.2017 um 11:18 schrieb Jan Burgmeier:
> Signed-off-by: Jan Burgmeier <jan.burgmeier@unicon-software.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 99424cb8020b..583d22974e14 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -908,6 +908,7 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device *adev,
>   			struct amdgpu_bo *aobj = NULL;
>   			uint64_t offset;
>   			uint8_t *kptr;
> +			uint64_t it_last;
>   
>   			m = amdgpu_cs_find_mapping(parser, chunk_ib->va_start,
>   						   &aobj);
> @@ -916,8 +917,9 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device *adev,
>   				return -EINVAL;
>   			}
>   
> +			it_last = m->it.last;
>   			if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
> -			    (m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
> +			    (it_last + 1) * AMDGPU_GPU_PAGE_SIZE) {

Nice catch, but just adding a u64 case should do here as well. E.g:

if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
     (u64)(m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {

With that fixed the patch is Reviewed-by: Christian König 
<christian.koenig@amd.com>.

Regards,
Christian.

>   				DRM_ERROR("IB va_start+ib_bytes is invalid\n");
>   				return -EINVAL;
>   			}


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

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems
@ 2017-03-29 14:54       ` Michel Dänzer
  0 siblings, 0 replies; 11+ messages in thread
From: Michel Dänzer @ 2017-03-29 14:54 UTC (permalink / raw)
  To: Christian König, Jan Burgmeier
  Cc: amd-gfx, dri-devel, linux-kernel, airlied

On 29/03/17 10:22 PM, Christian König wrote:
> Am 29.03.2017 um 11:18 schrieb Jan Burgmeier:
>> Signed-off-by: Jan Burgmeier <jan.burgmeier@unicon-software.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index 99424cb8020b..583d22974e14 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -908,6 +908,7 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>> *adev,
>>               struct amdgpu_bo *aobj = NULL;
>>               uint64_t offset;
>>               uint8_t *kptr;
>> +            uint64_t it_last;
>>                 m = amdgpu_cs_find_mapping(parser, chunk_ib->va_start,
>>                              &aobj);
>> @@ -916,8 +917,9 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>> *adev,
>>                   return -EINVAL;
>>               }
>>   +            it_last = m->it.last;
>>               if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>> -                (m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>> +                (it_last + 1) * AMDGPU_GPU_PAGE_SIZE) {
> 
> Nice catch, but just adding a u64 case should do here as well. E.g:
> 
> if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>     (u64)(m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {

That won't work correctly if m->it.last == 0xffffffff ? Or is that not
possible?


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems
@ 2017-03-29 14:54       ` Michel Dänzer
  0 siblings, 0 replies; 11+ messages in thread
From: Michel Dänzer @ 2017-03-29 14:54 UTC (permalink / raw)
  To: Christian König, Jan Burgmeier
  Cc: airlied-cv59FeDIM0c, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 29/03/17 10:22 PM, Christian König wrote:
> Am 29.03.2017 um 11:18 schrieb Jan Burgmeier:
>> Signed-off-by: Jan Burgmeier <jan.burgmeier@unicon-software.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index 99424cb8020b..583d22974e14 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -908,6 +908,7 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>> *adev,
>>               struct amdgpu_bo *aobj = NULL;
>>               uint64_t offset;
>>               uint8_t *kptr;
>> +            uint64_t it_last;
>>                 m = amdgpu_cs_find_mapping(parser, chunk_ib->va_start,
>>                              &aobj);
>> @@ -916,8 +917,9 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>> *adev,
>>                   return -EINVAL;
>>               }
>>   +            it_last = m->it.last;
>>               if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>> -                (m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>> +                (it_last + 1) * AMDGPU_GPU_PAGE_SIZE) {
> 
> Nice catch, but just adding a u64 case should do here as well. E.g:
> 
> if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>     (u64)(m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {

That won't work correctly if m->it.last == 0xffffffff ? Or is that not
possible?


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems
@ 2017-03-29 15:18         ` Christian König
  0 siblings, 0 replies; 11+ messages in thread
From: Christian König @ 2017-03-29 15:18 UTC (permalink / raw)
  To: Michel Dänzer, Jan Burgmeier
  Cc: amd-gfx, dri-devel, linux-kernel, airlied

Am 29.03.2017 um 16:54 schrieb Michel Dänzer:
> On 29/03/17 10:22 PM, Christian König wrote:
>> Am 29.03.2017 um 11:18 schrieb Jan Burgmeier:
>>> Signed-off-by: Jan Burgmeier <jan.burgmeier@unicon-software.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index 99424cb8020b..583d22974e14 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -908,6 +908,7 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>>> *adev,
>>>                struct amdgpu_bo *aobj = NULL;
>>>                uint64_t offset;
>>>                uint8_t *kptr;
>>> +            uint64_t it_last;
>>>                  m = amdgpu_cs_find_mapping(parser, chunk_ib->va_start,
>>>                               &aobj);
>>> @@ -916,8 +917,9 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>>> *adev,
>>>                    return -EINVAL;
>>>                }
>>>    +            it_last = m->it.last;
>>>                if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>>> -                (m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>> +                (it_last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>> Nice catch, but just adding a u64 case should do here as well. E.g:
>>
>> if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>>      (u64)(m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
> That won't work correctly if m->it.last == 0xffffffff ? Or is that not
> possible?
Hui, why? is it.last signed?

And even then m->it.last probably won't ever become 0xffffffff on a 
32bit system.

BTW: We need to fix using the 64bit R/B tree instead of the long sized 
tree for Vega10 here anyway.

Regards,
Christian.

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems
@ 2017-03-29 15:18         ` Christian König
  0 siblings, 0 replies; 11+ messages in thread
From: Christian König @ 2017-03-29 15:18 UTC (permalink / raw)
  To: Michel Dänzer, Jan Burgmeier
  Cc: airlied-cv59FeDIM0c, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

Am 29.03.2017 um 16:54 schrieb Michel Dänzer:
> On 29/03/17 10:22 PM, Christian König wrote:
>> Am 29.03.2017 um 11:18 schrieb Jan Burgmeier:
>>> Signed-off-by: Jan Burgmeier <jan.burgmeier@unicon-software.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index 99424cb8020b..583d22974e14 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -908,6 +908,7 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>>> *adev,
>>>                struct amdgpu_bo *aobj = NULL;
>>>                uint64_t offset;
>>>                uint8_t *kptr;
>>> +            uint64_t it_last;
>>>                  m = amdgpu_cs_find_mapping(parser, chunk_ib->va_start,
>>>                               &aobj);
>>> @@ -916,8 +917,9 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>>> *adev,
>>>                    return -EINVAL;
>>>                }
>>>    +            it_last = m->it.last;
>>>                if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>>> -                (m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>> +                (it_last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>> Nice catch, but just adding a u64 case should do here as well. E.g:
>>
>> if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>>      (u64)(m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
> That won't work correctly if m->it.last == 0xffffffff ? Or is that not
> possible?
Hui, why? is it.last signed?

And even then m->it.last probably won't ever become 0xffffffff on a 
32bit system.

BTW: We need to fix using the 64bit R/B tree instead of the long sized 
tree for Vega10 here anyway.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems
  2017-03-29 15:18         ` Christian König
  (?)
@ 2017-03-30  1:41         ` Michel Dänzer
  2017-03-30  8:36             ` Christian König
  -1 siblings, 1 reply; 11+ messages in thread
From: Michel Dänzer @ 2017-03-30  1:41 UTC (permalink / raw)
  To: Christian König, Jan Burgmeier
  Cc: airlied, dri-devel, amd-gfx, linux-kernel

On 30/03/17 12:18 AM, Christian König wrote:
> Am 29.03.2017 um 16:54 schrieb Michel Dänzer:
>> On 29/03/17 10:22 PM, Christian König wrote:
>>> Am 29.03.2017 um 11:18 schrieb Jan Burgmeier:
>>>> Signed-off-by: Jan Burgmeier <jan.burgmeier@unicon-software.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> index 99424cb8020b..583d22974e14 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> @@ -908,6 +908,7 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>>>> *adev,
>>>>                struct amdgpu_bo *aobj = NULL;
>>>>                uint64_t offset;
>>>>                uint8_t *kptr;
>>>> +            uint64_t it_last;
>>>>                  m = amdgpu_cs_find_mapping(parser, chunk_ib->va_start,
>>>>                               &aobj);
>>>> @@ -916,8 +917,9 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>>>> *adev,
>>>>                    return -EINVAL;
>>>>                }
>>>>    +            it_last = m->it.last;
>>>>                if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>>>> -                (m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>>> +                (it_last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>> Nice catch, but just adding a u64 case should do here as well. E.g:
>>>
>>> if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>>>      (u64)(m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>> That won't work correctly if m->it.last == 0xffffffff ? Or is that not
>> possible?
> Hui, why? is it.last signed?

No. If m->it.last == 0xffffffff, (m->it.last + 1) == 0, the u64 cast
won't change that. I thought that would be bad, but maybe not?


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems
@ 2017-03-30  8:36             ` Christian König
  0 siblings, 0 replies; 11+ messages in thread
From: Christian König @ 2017-03-30  8:36 UTC (permalink / raw)
  To: Michel Dänzer, Jan Burgmeier
  Cc: airlied, dri-devel, amd-gfx, linux-kernel

Am 30.03.2017 um 03:41 schrieb Michel Dänzer:
> On 30/03/17 12:18 AM, Christian König wrote:
>> Am 29.03.2017 um 16:54 schrieb Michel Dänzer:
>>> On 29/03/17 10:22 PM, Christian König wrote:
>>>> Am 29.03.2017 um 11:18 schrieb Jan Burgmeier:
>>>>> Signed-off-by: Jan Burgmeier <jan.burgmeier@unicon-software.com>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +++-
>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> index 99424cb8020b..583d22974e14 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> @@ -908,6 +908,7 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>>>>> *adev,
>>>>>                 struct amdgpu_bo *aobj = NULL;
>>>>>                 uint64_t offset;
>>>>>                 uint8_t *kptr;
>>>>> +            uint64_t it_last;
>>>>>                   m = amdgpu_cs_find_mapping(parser, chunk_ib->va_start,
>>>>>                                &aobj);
>>>>> @@ -916,8 +917,9 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>>>>> *adev,
>>>>>                     return -EINVAL;
>>>>>                 }
>>>>>     +            it_last = m->it.last;
>>>>>                 if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>>>>> -                (m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>>>> +                (it_last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>>> Nice catch, but just adding a u64 case should do here as well. E.g:
>>>>
>>>> if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>>>>       (u64)(m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>> That won't work correctly if m->it.last == 0xffffffff ? Or is that not
>>> possible?
>> Hui, why? is it.last signed?
> No. If m->it.last == 0xffffffff, (m->it.last + 1) == 0, the u64 cast
> won't change that. I thought that would be bad, but maybe not?

Ah, good catch. Yes the u64 cast needs to be inside the (), not outside.

Christian.

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems
@ 2017-03-30  8:36             ` Christian König
  0 siblings, 0 replies; 11+ messages in thread
From: Christian König @ 2017-03-30  8:36 UTC (permalink / raw)
  To: Michel Dänzer, Jan Burgmeier
  Cc: airlied-cv59FeDIM0c, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

Am 30.03.2017 um 03:41 schrieb Michel Dänzer:
> On 30/03/17 12:18 AM, Christian König wrote:
>> Am 29.03.2017 um 16:54 schrieb Michel Dänzer:
>>> On 29/03/17 10:22 PM, Christian König wrote:
>>>> Am 29.03.2017 um 11:18 schrieb Jan Burgmeier:
>>>>> Signed-off-by: Jan Burgmeier <jan.burgmeier@unicon-software.com>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +++-
>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> index 99424cb8020b..583d22974e14 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> @@ -908,6 +908,7 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>>>>> *adev,
>>>>>                 struct amdgpu_bo *aobj = NULL;
>>>>>                 uint64_t offset;
>>>>>                 uint8_t *kptr;
>>>>> +            uint64_t it_last;
>>>>>                   m = amdgpu_cs_find_mapping(parser, chunk_ib->va_start,
>>>>>                                &aobj);
>>>>> @@ -916,8 +917,9 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>>>>> *adev,
>>>>>                     return -EINVAL;
>>>>>                 }
>>>>>     +            it_last = m->it.last;
>>>>>                 if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>>>>> -                (m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>>>> +                (it_last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>>> Nice catch, but just adding a u64 case should do here as well. E.g:
>>>>
>>>> if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>>>>       (u64)(m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>> That won't work correctly if m->it.last == 0xffffffff ? Or is that not
>>> possible?
>> Hui, why? is it.last signed?
> No. If m->it.last == 0xffffffff, (m->it.last + 1) == 0, the u64 cast
> won't change that. I thought that would be bad, but maybe not?

Ah, good catch. Yes the u64 cast needs to be inside the (), not outside.

Christian.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-03-30  8:36 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-29  9:18 Fix hardware accelerated video playback with amdgpu on 32Bit system Jan Burgmeier
2017-03-29  9:18 ` [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems Jan Burgmeier
2017-03-29 13:22   ` Christian König
2017-03-29 13:22     ` Christian König
2017-03-29 14:54     ` Michel Dänzer
2017-03-29 14:54       ` Michel Dänzer
2017-03-29 15:18       ` Christian König
2017-03-29 15:18         ` Christian König
2017-03-30  1:41         ` Michel Dänzer
2017-03-30  8:36           ` Christian König
2017-03-30  8:36             ` Christian König

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.