drm/amdgpu: fix UBSAN: Undefined behaviour for amdgpu_fence.c

Submitted by Liu, Leo on June 25, 2018, 7:17 p.m.

Details

Message ID 20180625191729.4371-1-leo.liu@amd.com
State New
Headers show
Series "drm/amdgpu: fix UBSAN: Undefined behaviour for amdgpu_fence.c" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Liu, Leo June 25, 2018, 7:17 p.m.
[    3.866656] index 2 is out of range for type 'amdgpu_uvd_inst [2]'
[    3.866667] CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.16.0-rc7+ #3
[    3.866677] Hardware name: Gigabyte Technology Co., Ltd. GA-990FXA-UD7/GA-990FXA-UD7, BIOS F9 06/08/2012
[    3.866693] Workqueue: events work_for_cpu_fn
[    3.866702] Call Trace:
[    3.866710]  dump_stack+0x85/0xc5
[    3.866719]  ubsan_epilogue+0x9/0x40
[    3.866727]  __ubsan_handle_out_of_bounds+0x89/0x90
[    3.866737]  ? rcu_read_lock_sched_held+0x58/0x60
[    3.866746]  ? __kmalloc+0x26c/0x2d0
[    3.866846]  amdgpu_fence_driver_start_ring+0x259/0x280 [amdgpu]
[    3.866896]  amdgpu_ring_init+0x12c/0x710 [amdgpu]
[    3.866906]  ? sprintf+0x42/0x50
[    3.866956]  amdgpu_gfx_kiq_init_ring+0x1bc/0x3a0 [amdgpu]
[    3.867009]  gfx_v8_0_sw_init+0x1ad3/0x2360 [amdgpu]
[    3.867062]  ? smu7_init+0xec/0x160 [amdgpu]
[    3.867109]  amdgpu_device_init+0x112c/0x1dc0 [amdgpu]

ring->me might be set as 2 with amdgpu_gfx_kiq_init_ring.

Signed-off-by: Leo Liu <leo.liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 39ec6b8890a1..6067dae1552f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -375,8 +375,17 @@  int amdgpu_fence_driver_start_ring(struct amdgpu_ring *ring,
 {
 	struct amdgpu_device *adev = ring->adev;
 	uint64_t index;
+	bool uvd_ring = false;
+	int i;
+
+	for (i = 0; i < AMDGPU_MAX_UVD_INSTANCES; ++i) {
+		if (ring == &adev->uvd.inst[i].ring) {
+			uvd_ring = true;
+			break;
+		}
+	}
 
-	if (ring != &adev->uvd.inst[ring->me].ring) {
+	if (!uvd_ring) {
 		ring->fence_drv.cpu_addr = &adev->wb.wb[ring->fence_offs];
 		ring->fence_drv.gpu_addr = adev->wb.gpu_addr + (ring->fence_offs * 4);
 	} else {

Comments

On Mon, Jun 25, 2018 at 3:17 PM, Leo Liu <leo.liu@amd.com> wrote:
> [    3.866656] index 2 is out of range for type 'amdgpu_uvd_inst [2]'
> [    3.866667] CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.16.0-rc7+ #3
> [    3.866677] Hardware name: Gigabyte Technology Co., Ltd. GA-990FXA-UD7/GA-990FXA-UD7, BIOS F9 06/08/2012
> [    3.866693] Workqueue: events work_for_cpu_fn
> [    3.866702] Call Trace:
> [    3.866710]  dump_stack+0x85/0xc5
> [    3.866719]  ubsan_epilogue+0x9/0x40
> [    3.866727]  __ubsan_handle_out_of_bounds+0x89/0x90
> [    3.866737]  ? rcu_read_lock_sched_held+0x58/0x60
> [    3.866746]  ? __kmalloc+0x26c/0x2d0
> [    3.866846]  amdgpu_fence_driver_start_ring+0x259/0x280 [amdgpu]
> [    3.866896]  amdgpu_ring_init+0x12c/0x710 [amdgpu]
> [    3.866906]  ? sprintf+0x42/0x50
> [    3.866956]  amdgpu_gfx_kiq_init_ring+0x1bc/0x3a0 [amdgpu]
> [    3.867009]  gfx_v8_0_sw_init+0x1ad3/0x2360 [amdgpu]
> [    3.867062]  ? smu7_init+0xec/0x160 [amdgpu]
> [    3.867109]  amdgpu_device_init+0x112c/0x1dc0 [amdgpu]
>
> ring->me might be set as 2 with amdgpu_gfx_kiq_init_ring.
>
> Signed-off-by: Leo Liu <leo.liu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index 39ec6b8890a1..6067dae1552f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> @@ -375,8 +375,17 @@ int amdgpu_fence_driver_start_ring(struct amdgpu_ring *ring,
>  {
>         struct amdgpu_device *adev = ring->adev;
>         uint64_t index;
> +       bool uvd_ring = false;
> +       int i;
> +
> +       for (i = 0; i < AMDGPU_MAX_UVD_INSTANCES; ++i) {
> +               if (ring == &adev->uvd.inst[i].ring) {
> +                       uvd_ring = true;
> +                       break;
> +               }
> +       }
>
> -       if (ring != &adev->uvd.inst[ring->me].ring) {
> +       if (!uvd_ring) {

I think we can just simplify this to:
if (ring->funcs->type != AMDGPU_RING_TYPE_UVD)

Alex

>                 ring->fence_drv.cpu_addr = &adev->wb.wb[ring->fence_offs];
>                 ring->fence_drv.gpu_addr = adev->wb.gpu_addr + (ring->fence_offs * 4);
>         } else {
> --
> 2.17.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 2018-06-25 04:02 PM, Alex Deucher wrote:
> On Mon, Jun 25, 2018 at 3:17 PM, Leo Liu <leo.liu@amd.com> wrote:
>> [    3.866656] index 2 is out of range for type 'amdgpu_uvd_inst [2]'
>> [    3.866667] CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.16.0-rc7+ #3
>> [    3.866677] Hardware name: Gigabyte Technology Co., Ltd. GA-990FXA-UD7/GA-990FXA-UD7, BIOS F9 06/08/2012
>> [    3.866693] Workqueue: events work_for_cpu_fn
>> [    3.866702] Call Trace:
>> [    3.866710]  dump_stack+0x85/0xc5
>> [    3.866719]  ubsan_epilogue+0x9/0x40
>> [    3.866727]  __ubsan_handle_out_of_bounds+0x89/0x90
>> [    3.866737]  ? rcu_read_lock_sched_held+0x58/0x60
>> [    3.866746]  ? __kmalloc+0x26c/0x2d0
>> [    3.866846]  amdgpu_fence_driver_start_ring+0x259/0x280 [amdgpu]
>> [    3.866896]  amdgpu_ring_init+0x12c/0x710 [amdgpu]
>> [    3.866906]  ? sprintf+0x42/0x50
>> [    3.866956]  amdgpu_gfx_kiq_init_ring+0x1bc/0x3a0 [amdgpu]
>> [    3.867009]  gfx_v8_0_sw_init+0x1ad3/0x2360 [amdgpu]
>> [    3.867062]  ? smu7_init+0xec/0x160 [amdgpu]
>> [    3.867109]  amdgpu_device_init+0x112c/0x1dc0 [amdgpu]
>>
>> ring->me might be set as 2 with amdgpu_gfx_kiq_init_ring.
>>
>> Signed-off-by: Leo Liu <leo.liu@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>> index 39ec6b8890a1..6067dae1552f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>> @@ -375,8 +375,17 @@ int amdgpu_fence_driver_start_ring(struct amdgpu_ring *ring,
>>   {
>>          struct amdgpu_device *adev = ring->adev;
>>          uint64_t index;
>> +       bool uvd_ring = false;
>> +       int i;
>> +
>> +       for (i = 0; i < AMDGPU_MAX_UVD_INSTANCES; ++i) {

>> +               if (ring == &adev->uvd.inst[i].ring) {
>> +                       uvd_ring = true;
>> +                       break;
>> +               }
>> +       }
>>
>> -       if (ring != &adev->uvd.inst[ring->me].ring) {
>> +       if (!uvd_ring) {
> I think we can just simplify this to:
> if (ring->funcs->type != AMDGPU_RING_TYPE_UVD)
>
> Alex
Need add UVD_ENC also:
       if ( ring->funcs->type != AMDGPU_RING_TYPE_UVD &&
             ring->funcs->type != AMDGPU_RING_TYPE_UVD_ENC ) {

After this, this patch is Reviewed-by: James Zhu <James.Zhu@amd.com>

James
>>                  ring->fence_drv.cpu_addr = &adev->wb.wb[ring->fence_offs];
>>                  ring->fence_drv.gpu_addr = adev->wb.gpu_addr + (ring->fence_offs * 4);
>>          } else {
>> --
>> 2.17.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Mon, Jun 25, 2018 at 4:26 PM, James Zhu <jamesz@amd.com> wrote:
>
>
> On 2018-06-25 04:02 PM, Alex Deucher wrote:
>>
>> On Mon, Jun 25, 2018 at 3:17 PM, Leo Liu <leo.liu@amd.com> wrote:
>>>
>>> [    3.866656] index 2 is out of range for type 'amdgpu_uvd_inst [2]'
>>> [    3.866667] CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.16.0-rc7+
>>> #3
>>> [    3.866677] Hardware name: Gigabyte Technology Co., Ltd.
>>> GA-990FXA-UD7/GA-990FXA-UD7, BIOS F9 06/08/2012
>>> [    3.866693] Workqueue: events work_for_cpu_fn
>>> [    3.866702] Call Trace:
>>> [    3.866710]  dump_stack+0x85/0xc5
>>> [    3.866719]  ubsan_epilogue+0x9/0x40
>>> [    3.866727]  __ubsan_handle_out_of_bounds+0x89/0x90
>>> [    3.866737]  ? rcu_read_lock_sched_held+0x58/0x60
>>> [    3.866746]  ? __kmalloc+0x26c/0x2d0
>>> [    3.866846]  amdgpu_fence_driver_start_ring+0x259/0x280 [amdgpu]
>>> [    3.866896]  amdgpu_ring_init+0x12c/0x710 [amdgpu]
>>> [    3.866906]  ? sprintf+0x42/0x50
>>> [    3.866956]  amdgpu_gfx_kiq_init_ring+0x1bc/0x3a0 [amdgpu]
>>> [    3.867009]  gfx_v8_0_sw_init+0x1ad3/0x2360 [amdgpu]
>>> [    3.867062]  ? smu7_init+0xec/0x160 [amdgpu]
>>> [    3.867109]  amdgpu_device_init+0x112c/0x1dc0 [amdgpu]
>>>
>>> ring->me might be set as 2 with amdgpu_gfx_kiq_init_ring.
>>>
>>> Signed-off-by: Leo Liu <leo.liu@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 11 ++++++++++-
>>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> index 39ec6b8890a1..6067dae1552f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> @@ -375,8 +375,17 @@ int amdgpu_fence_driver_start_ring(struct
>>> amdgpu_ring *ring,
>>>   {
>>>          struct amdgpu_device *adev = ring->adev;
>>>          uint64_t index;
>>> +       bool uvd_ring = false;
>>> +       int i;
>>> +
>>> +       for (i = 0; i < AMDGPU_MAX_UVD_INSTANCES; ++i) {
>
>
>>> +               if (ring == &adev->uvd.inst[i].ring) {
>>> +                       uvd_ring = true;
>>> +                       break;
>>> +               }
>>> +       }
>>>
>>> -       if (ring != &adev->uvd.inst[ring->me].ring) {
>>> +       if (!uvd_ring) {
>>
>> I think we can just simplify this to:
>> if (ring->funcs->type != AMDGPU_RING_TYPE_UVD)
>>
>> Alex
>
> Need add UVD_ENC also:
>       if ( ring->funcs->type != AMDGPU_RING_TYPE_UVD &&
>             ring->funcs->type != AMDGPU_RING_TYPE_UVD_ENC ) {
>

The old code never checked against the enc rings.  Are you sure they
have the same limitation?

Alex

> After this, this patch is Reviewed-by: James Zhu <James.Zhu@amd.com>
>
> James
>
>>>                  ring->fence_drv.cpu_addr =
>>> &adev->wb.wb[ring->fence_offs];
>>>                  ring->fence_drv.gpu_addr = adev->wb.gpu_addr +
>>> (ring->fence_offs * 4);
>>>          } else {
>>> --
>>> 2.17.1
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
On 2018-06-25 04:32 PM, Alex Deucher wrote:
> On Mon, Jun 25, 2018 at 4:26 PM, James Zhu <jamesz@amd.com> wrote:
>>
>> On 2018-06-25 04:02 PM, Alex Deucher wrote:
>>> On Mon, Jun 25, 2018 at 3:17 PM, Leo Liu <leo.liu@amd.com> wrote:
>>>> [    3.866656] index 2 is out of range for type 'amdgpu_uvd_inst [2]'
>>>> [    3.866667] CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.16.0-rc7+
>>>> #3
>>>> [    3.866677] Hardware name: Gigabyte Technology Co., Ltd.
>>>> GA-990FXA-UD7/GA-990FXA-UD7, BIOS F9 06/08/2012
>>>> [    3.866693] Workqueue: events work_for_cpu_fn
>>>> [    3.866702] Call Trace:
>>>> [    3.866710]  dump_stack+0x85/0xc5
>>>> [    3.866719]  ubsan_epilogue+0x9/0x40
>>>> [    3.866727]  __ubsan_handle_out_of_bounds+0x89/0x90
>>>> [    3.866737]  ? rcu_read_lock_sched_held+0x58/0x60
>>>> [    3.866746]  ? __kmalloc+0x26c/0x2d0
>>>> [    3.866846]  amdgpu_fence_driver_start_ring+0x259/0x280 [amdgpu]
>>>> [    3.866896]  amdgpu_ring_init+0x12c/0x710 [amdgpu]
>>>> [    3.866906]  ? sprintf+0x42/0x50
>>>> [    3.866956]  amdgpu_gfx_kiq_init_ring+0x1bc/0x3a0 [amdgpu]
>>>> [    3.867009]  gfx_v8_0_sw_init+0x1ad3/0x2360 [amdgpu]
>>>> [    3.867062]  ? smu7_init+0xec/0x160 [amdgpu]
>>>> [    3.867109]  amdgpu_device_init+0x112c/0x1dc0 [amdgpu]
>>>>
>>>> ring->me might be set as 2 with amdgpu_gfx_kiq_init_ring.
>>>>
>>>> Signed-off-by: Leo Liu <leo.liu@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 11 ++++++++++-
>>>>    1 file changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>> index 39ec6b8890a1..6067dae1552f 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>> @@ -375,8 +375,17 @@ int amdgpu_fence_driver_start_ring(struct
>>>> amdgpu_ring *ring,
>>>>    {
>>>>           struct amdgpu_device *adev = ring->adev;
>>>>           uint64_t index;
>>>> +       bool uvd_ring = false;
>>>> +       int i;
>>>> +
>>>> +       for (i = 0; i < AMDGPU_MAX_UVD_INSTANCES; ++i) {
>>
>>>> +               if (ring == &adev->uvd.inst[i].ring) {
>>>> +                       uvd_ring = true;
>>>> +                       break;
>>>> +               }
>>>> +       }
>>>>
>>>> -       if (ring != &adev->uvd.inst[ring->me].ring) {
>>>> +       if (!uvd_ring) {
>>> I think we can just simplify this to:
>>> if (ring->funcs->type != AMDGPU_RING_TYPE_UVD)
>>>
>>> Alex
>> Need add UVD_ENC also:
>>        if ( ring->funcs->type != AMDGPU_RING_TYPE_UVD &&
>>              ring->funcs->type != AMDGPU_RING_TYPE_UVD_ENC ) {
>>
> The old code never checked against the enc rings.  Are you sure they
> have the same limitation?
I am wrong. thanks! James
>
> Alex
>
>> After this, this patch is Reviewed-by: James Zhu <James.Zhu@amd.com>
>>
>> James
>>
>>>>                   ring->fence_drv.cpu_addr =
>>>> &adev->wb.wb[ring->fence_offs];
>>>>                   ring->fence_drv.gpu_addr = adev->wb.gpu_addr +
>>>> (ring->fence_offs * 4);
>>>>           } else {
>>>> --
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
On Mon, Jun 25, 2018 at 4:37 PM, James Zhu <jamesz@amd.com> wrote:
>
>
> On 2018-06-25 04:32 PM, Alex Deucher wrote:
>>
>> On Mon, Jun 25, 2018 at 4:26 PM, James Zhu <jamesz@amd.com> wrote:
>>>
>>>
>>> On 2018-06-25 04:02 PM, Alex Deucher wrote:
>>>>
>>>> On Mon, Jun 25, 2018 at 3:17 PM, Leo Liu <leo.liu@amd.com> wrote:
>>>>>
>>>>> [    3.866656] index 2 is out of range for type 'amdgpu_uvd_inst [2]'
>>>>> [    3.866667] CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.16.0-rc7+
>>>>> #3
>>>>> [    3.866677] Hardware name: Gigabyte Technology Co., Ltd.
>>>>> GA-990FXA-UD7/GA-990FXA-UD7, BIOS F9 06/08/2012
>>>>> [    3.866693] Workqueue: events work_for_cpu_fn
>>>>> [    3.866702] Call Trace:
>>>>> [    3.866710]  dump_stack+0x85/0xc5
>>>>> [    3.866719]  ubsan_epilogue+0x9/0x40
>>>>> [    3.866727]  __ubsan_handle_out_of_bounds+0x89/0x90
>>>>> [    3.866737]  ? rcu_read_lock_sched_held+0x58/0x60
>>>>> [    3.866746]  ? __kmalloc+0x26c/0x2d0
>>>>> [    3.866846]  amdgpu_fence_driver_start_ring+0x259/0x280 [amdgpu]
>>>>> [    3.866896]  amdgpu_ring_init+0x12c/0x710 [amdgpu]
>>>>> [    3.866906]  ? sprintf+0x42/0x50
>>>>> [    3.866956]  amdgpu_gfx_kiq_init_ring+0x1bc/0x3a0 [amdgpu]
>>>>> [    3.867009]  gfx_v8_0_sw_init+0x1ad3/0x2360 [amdgpu]
>>>>> [    3.867062]  ? smu7_init+0xec/0x160 [amdgpu]
>>>>> [    3.867109]  amdgpu_device_init+0x112c/0x1dc0 [amdgpu]
>>>>>
>>>>> ring->me might be set as 2 with amdgpu_gfx_kiq_init_ring.
>>>>>
>>>>> Signed-off-by: Leo Liu <leo.liu@amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 11 ++++++++++-
>>>>>    1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>> index 39ec6b8890a1..6067dae1552f 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>> @@ -375,8 +375,17 @@ int amdgpu_fence_driver_start_ring(struct
>>>>> amdgpu_ring *ring,
>>>>>    {
>>>>>           struct amdgpu_device *adev = ring->adev;
>>>>>           uint64_t index;
>>>>> +       bool uvd_ring = false;
>>>>> +       int i;
>>>>> +
>>>>> +       for (i = 0; i < AMDGPU_MAX_UVD_INSTANCES; ++i) {
>>>
>>>
>>>>> +               if (ring == &adev->uvd.inst[i].ring) {
>>>>> +                       uvd_ring = true;
>>>>> +                       break;
>>>>> +               }
>>>>> +       }
>>>>>
>>>>> -       if (ring != &adev->uvd.inst[ring->me].ring) {
>>>>> +       if (!uvd_ring) {
>>>>
>>>> I think we can just simplify this to:
>>>> if (ring->funcs->type != AMDGPU_RING_TYPE_UVD)
>>>>
>>>> Alex
>>>
>>> Need add UVD_ENC also:
>>>        if ( ring->funcs->type != AMDGPU_RING_TYPE_UVD &&
>>>              ring->funcs->type != AMDGPU_RING_TYPE_UVD_ENC ) {
>>>
>> The old code never checked against the enc rings.  Are you sure they
>> have the same limitation?
>
> I am wrong. thanks! James

Actually does the uvd decode ring still have this limitation or is
this just some leftover from a hw limitation on an older asic?  If
it's still required, the current code in
amdgpu_fence_driver_start_ring() won't work for multiple instances.
The fences for the rings will overwrite each other.

Alex

>
>>
>> Alex
>>
>>> After this, this patch is Reviewed-by: James Zhu <James.Zhu@amd.com>
>>>
>>> James
>>>
>>>>>                   ring->fence_drv.cpu_addr =
>>>>> &adev->wb.wb[ring->fence_offs];
>>>>>                   ring->fence_drv.gpu_addr = adev->wb.gpu_addr +
>>>>> (ring->fence_offs * 4);
>>>>>           } else {
>>>>> --
>>>>> 2.17.1
>>>>>
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>>
>
On 06/25/2018 04:42 PM, Alex Deucher wrote:
> On Mon, Jun 25, 2018 at 4:37 PM, James Zhu <jamesz@amd.com> wrote:
>>
>> On 2018-06-25 04:32 PM, Alex Deucher wrote:
>>> On Mon, Jun 25, 2018 at 4:26 PM, James Zhu <jamesz@amd.com> wrote:
>>>>
>>>> On 2018-06-25 04:02 PM, Alex Deucher wrote:
>>>>> On Mon, Jun 25, 2018 at 3:17 PM, Leo Liu <leo.liu@amd.com> wrote:
>>>>>> [    3.866656] index 2 is out of range for type 'amdgpu_uvd_inst [2]'
>>>>>> [    3.866667] CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.16.0-rc7+
>>>>>> #3
>>>>>> [    3.866677] Hardware name: Gigabyte Technology Co., Ltd.
>>>>>> GA-990FXA-UD7/GA-990FXA-UD7, BIOS F9 06/08/2012
>>>>>> [    3.866693] Workqueue: events work_for_cpu_fn
>>>>>> [    3.866702] Call Trace:
>>>>>> [    3.866710]  dump_stack+0x85/0xc5
>>>>>> [    3.866719]  ubsan_epilogue+0x9/0x40
>>>>>> [    3.866727]  __ubsan_handle_out_of_bounds+0x89/0x90
>>>>>> [    3.866737]  ? rcu_read_lock_sched_held+0x58/0x60
>>>>>> [    3.866746]  ? __kmalloc+0x26c/0x2d0
>>>>>> [    3.866846]  amdgpu_fence_driver_start_ring+0x259/0x280 [amdgpu]
>>>>>> [    3.866896]  amdgpu_ring_init+0x12c/0x710 [amdgpu]
>>>>>> [    3.866906]  ? sprintf+0x42/0x50
>>>>>> [    3.866956]  amdgpu_gfx_kiq_init_ring+0x1bc/0x3a0 [amdgpu]
>>>>>> [    3.867009]  gfx_v8_0_sw_init+0x1ad3/0x2360 [amdgpu]
>>>>>> [    3.867062]  ? smu7_init+0xec/0x160 [amdgpu]
>>>>>> [    3.867109]  amdgpu_device_init+0x112c/0x1dc0 [amdgpu]
>>>>>>
>>>>>> ring->me might be set as 2 with amdgpu_gfx_kiq_init_ring.
>>>>>>
>>>>>> Signed-off-by: Leo Liu <leo.liu@amd.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 11 ++++++++++-
>>>>>>     1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>> index 39ec6b8890a1..6067dae1552f 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>> @@ -375,8 +375,17 @@ int amdgpu_fence_driver_start_ring(struct
>>>>>> amdgpu_ring *ring,
>>>>>>     {
>>>>>>            struct amdgpu_device *adev = ring->adev;
>>>>>>            uint64_t index;
>>>>>> +       bool uvd_ring = false;
>>>>>> +       int i;
>>>>>> +
>>>>>> +       for (i = 0; i < AMDGPU_MAX_UVD_INSTANCES; ++i) {
>>>>
>>>>>> +               if (ring == &adev->uvd.inst[i].ring) {
>>>>>> +                       uvd_ring = true;
>>>>>> +                       break;
>>>>>> +               }
>>>>>> +       }
>>>>>>
>>>>>> -       if (ring != &adev->uvd.inst[ring->me].ring) {
>>>>>> +       if (!uvd_ring) {
>>>>> I think we can just simplify this to:
>>>>> if (ring->funcs->type != AMDGPU_RING_TYPE_UVD)
>>>>>
>>>>> Alex
>>>> Need add UVD_ENC also:
>>>>         if ( ring->funcs->type != AMDGPU_RING_TYPE_UVD &&
>>>>               ring->funcs->type != AMDGPU_RING_TYPE_UVD_ENC ) {
>>>>
>>> The old code never checked against the enc rings.  Are you sure they
>>> have the same limitation?
>> I am wrong. thanks! James
> Actually does the uvd decode ring still have this limitation or is
> this just some leftover from a hw limitation on an older asic?
The code here inherited from old ASICs, and not be revisited since there 
is no problem in functional wise.


>   If
> it's still required, the current code in
> amdgpu_fence_driver_start_ring() won't work for multiple instances.
> The fences for the rings will overwrite each other.
IIRC Vega20 with 2 instances have separated runtime environment, that 
should have fence separated as well.

Regards,
Leo



>
> Alex
>
>>> Alex
>>>
>>>> After this, this patch is Reviewed-by: James Zhu <James.Zhu@amd.com>
>>>>
>>>> James
>>>>
>>>>>>                    ring->fence_drv.cpu_addr =
>>>>>> &adev->wb.wb[ring->fence_offs];
>>>>>>                    ring->fence_drv.gpu_addr = adev->wb.gpu_addr +
>>>>>> (ring->fence_offs * 4);
>>>>>>            } else {
>>>>>> --
>>>>>> 2.17.1
>>>>>>
>>>>>> _______________________________________________
>>>>>> amd-gfx mailing list
>>>>>> amd-gfx@lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>
On Tue, Jun 26, 2018 at 8:29 AM, Leo Liu <leo.liu@amd.com> wrote:
>
>
> On 06/25/2018 04:42 PM, Alex Deucher wrote:
>>
>> On Mon, Jun 25, 2018 at 4:37 PM, James Zhu <jamesz@amd.com> wrote:
>>>
>>>
>>> On 2018-06-25 04:32 PM, Alex Deucher wrote:
>>>>
>>>> On Mon, Jun 25, 2018 at 4:26 PM, James Zhu <jamesz@amd.com> wrote:
>>>>>
>>>>>
>>>>> On 2018-06-25 04:02 PM, Alex Deucher wrote:
>>>>>>
>>>>>> On Mon, Jun 25, 2018 at 3:17 PM, Leo Liu <leo.liu@amd.com> wrote:
>>>>>>>
>>>>>>> [    3.866656] index 2 is out of range for type 'amdgpu_uvd_inst [2]'
>>>>>>> [    3.866667] CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted
>>>>>>> 4.16.0-rc7+
>>>>>>> #3
>>>>>>> [    3.866677] Hardware name: Gigabyte Technology Co., Ltd.
>>>>>>> GA-990FXA-UD7/GA-990FXA-UD7, BIOS F9 06/08/2012
>>>>>>> [    3.866693] Workqueue: events work_for_cpu_fn
>>>>>>> [    3.866702] Call Trace:
>>>>>>> [    3.866710]  dump_stack+0x85/0xc5
>>>>>>> [    3.866719]  ubsan_epilogue+0x9/0x40
>>>>>>> [    3.866727]  __ubsan_handle_out_of_bounds+0x89/0x90
>>>>>>> [    3.866737]  ? rcu_read_lock_sched_held+0x58/0x60
>>>>>>> [    3.866746]  ? __kmalloc+0x26c/0x2d0
>>>>>>> [    3.866846]  amdgpu_fence_driver_start_ring+0x259/0x280 [amdgpu]
>>>>>>> [    3.866896]  amdgpu_ring_init+0x12c/0x710 [amdgpu]
>>>>>>> [    3.866906]  ? sprintf+0x42/0x50
>>>>>>> [    3.866956]  amdgpu_gfx_kiq_init_ring+0x1bc/0x3a0 [amdgpu]
>>>>>>> [    3.867009]  gfx_v8_0_sw_init+0x1ad3/0x2360 [amdgpu]
>>>>>>> [    3.867062]  ? smu7_init+0xec/0x160 [amdgpu]
>>>>>>> [    3.867109]  amdgpu_device_init+0x112c/0x1dc0 [amdgpu]
>>>>>>>
>>>>>>> ring->me might be set as 2 with amdgpu_gfx_kiq_init_ring.
>>>>>>>
>>>>>>> Signed-off-by: Leo Liu <leo.liu@amd.com>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 11 ++++++++++-
>>>>>>>     1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>> index 39ec6b8890a1..6067dae1552f 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>> @@ -375,8 +375,17 @@ int amdgpu_fence_driver_start_ring(struct
>>>>>>> amdgpu_ring *ring,
>>>>>>>     {
>>>>>>>            struct amdgpu_device *adev = ring->adev;
>>>>>>>            uint64_t index;
>>>>>>> +       bool uvd_ring = false;
>>>>>>> +       int i;
>>>>>>> +
>>>>>>> +       for (i = 0; i < AMDGPU_MAX_UVD_INSTANCES; ++i) {
>>>>>
>>>>>
>>>>>>> +               if (ring == &adev->uvd.inst[i].ring) {
>>>>>>> +                       uvd_ring = true;
>>>>>>> +                       break;
>>>>>>> +               }
>>>>>>> +       }
>>>>>>>
>>>>>>> -       if (ring != &adev->uvd.inst[ring->me].ring) {
>>>>>>> +       if (!uvd_ring) {
>>>>>>
>>>>>> I think we can just simplify this to:
>>>>>> if (ring->funcs->type != AMDGPU_RING_TYPE_UVD)
>>>>>>
>>>>>> Alex
>>>>>
>>>>> Need add UVD_ENC also:
>>>>>         if ( ring->funcs->type != AMDGPU_RING_TYPE_UVD &&
>>>>>               ring->funcs->type != AMDGPU_RING_TYPE_UVD_ENC ) {
>>>>>
>>>> The old code never checked against the enc rings.  Are you sure they
>>>> have the same limitation?
>>>
>>> I am wrong. thanks! James
>>
>> Actually does the uvd decode ring still have this limitation or is
>> this just some leftover from a hw limitation on an older asic?
>
> The code here inherited from old ASICs, and not be revisited since there is
> no problem in functional wise.
>
>
>>   If
>> it's still required, the current code in
>> amdgpu_fence_driver_start_ring() won't work for multiple instances.
>> The fences for the rings will overwrite each other.
>
> IIRC Vega20 with 2 instances have separated runtime environment, that should
> have fence separated as well.

Ok.  For some reason I was thinking the instances shared the same copy
of the firmware.  Anyway, the patch as is is:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
We can optimize for newer asics as a later improvement.

Alex