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
Signed-off-by: Jan Burgmeier <[email protected]>
---
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
Am 29.03.2017 um 11:18 schrieb Jan Burgmeier:
> Signed-off-by: Jan Burgmeier <[email protected]>
> ---
> 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
<[email protected]>.
Regards,
Christian.
> DRM_ERROR("IB va_start+ib_bytes is invalid\n");
> return -EINVAL;
> }
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>> ---
>>> 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.
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 <[email protected]>
>>>> ---
>>>> 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
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 <[email protected]>
>>>>> ---
>>>>> 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.