All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/amdgpu: Initialize schedulers before using them
@ 2023-10-23  3:23 Luben Tuikov
  2023-10-23  5:49 ` Christian König
  0 siblings, 1 reply; 9+ messages in thread
From: Luben Tuikov @ 2023-10-23  3:23 UTC (permalink / raw)
  To: AMD Graphics
  Cc: Alex Deucher, Felix Kuehling, Luben Tuikov, Christian König

Initialize ring schedulers before using them, very early in the amdgpu boot,
at PCI probe time, specifically at frame-buffer dumb-create at fill-buffer.

This was discovered by using dynamic scheduler run-queues, which showed that
amdgpu was using a scheduler before calling drm_sched_init(), and the only
reason it was working was because sched_rq[] was statically allocated in the
scheduler structure. However, the scheduler structure had _not_ been
initialized.

When switching to dynamically allocated run-queues, this lack of
initialization was causing an oops and a blank screen at boot up. This patch
fixes this amdgpu bug.

This patch depends on the "drm/sched: Convert the GPU scheduler to variable
number of run-queues" patch, as that patch prevents subsequent scheduler
initialization if a scheduler has already been initialized.

Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <Alexander.Deucher@amd.com>
Cc: Felix Kuehling <Felix.Kuehling@amd.com>
Cc: AMD Graphics <amd-gfx@lists.freedesktop.org>
Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 4e51dce3aab5d6..575ef7e1e30fd4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -60,6 +60,7 @@
 #include "amdgpu_atomfirmware.h"
 #include "amdgpu_res_cursor.h"
 #include "bif/bif_4_1_d.h"
+#include "amdgpu_reset.h"
 
 MODULE_IMPORT_NS(DMA_BUF);
 
@@ -2059,6 +2060,19 @@ void amdgpu_ttm_set_buffer_funcs_status(struct amdgpu_device *adev, bool enable)
 
 		ring = adev->mman.buffer_funcs_ring;
 		sched = &ring->sched;
+
+		r = drm_sched_init(sched, &amdgpu_sched_ops,
+				   DRM_SCHED_PRIORITY_COUNT,
+				   ring->num_hw_submission, 0,
+				   adev->sdma_timeout, adev->reset_domain->wq,
+				   ring->sched_score, ring->name,
+				   adev->dev);
+		if (r) {
+			drm_err(adev, "%s: couldn't initialize ring:%s error:%d\n",
+				__func__, ring->name, r);
+			return;
+		}
+
 		r = drm_sched_entity_init(&adev->mman.high_pr,
 					  DRM_SCHED_PRIORITY_KERNEL, &sched,
 					  1, NULL);

base-commit: 05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1
prerequisite-patch-id: c52673df9b6fc9ee001d6261c7ac107b618912a0
-- 
2.42.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: Initialize schedulers before using them
  2023-10-23  3:23 [PATCH] drm/amdgpu: Initialize schedulers before using them Luben Tuikov
@ 2023-10-23  5:49 ` Christian König
  2023-10-24  2:55   ` Luben Tuikov
  0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2023-10-23  5:49 UTC (permalink / raw)
  To: Luben Tuikov, AMD Graphics
  Cc: Alex Deucher, Felix Kuehling, Christian König



Am 23.10.23 um 05:23 schrieb Luben Tuikov:
> Initialize ring schedulers before using them, very early in the amdgpu boot,
> at PCI probe time, specifically at frame-buffer dumb-create at fill-buffer.
>
> This was discovered by using dynamic scheduler run-queues, which showed that
> amdgpu was using a scheduler before calling drm_sched_init(), and the only
> reason it was working was because sched_rq[] was statically allocated in the
> scheduler structure. However, the scheduler structure had _not_ been
> initialized.
>
> When switching to dynamically allocated run-queues, this lack of
> initialization was causing an oops and a blank screen at boot up. This patch
> fixes this amdgpu bug.
>
> This patch depends on the "drm/sched: Convert the GPU scheduler to variable
> number of run-queues" patch, as that patch prevents subsequent scheduler
> initialization if a scheduler has already been initialized.
>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <Alexander.Deucher@amd.com>
> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
> Cc: AMD Graphics <amd-gfx@lists.freedesktop.org>
> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 4e51dce3aab5d6..575ef7e1e30fd4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -60,6 +60,7 @@
>   #include "amdgpu_atomfirmware.h"
>   #include "amdgpu_res_cursor.h"
>   #include "bif/bif_4_1_d.h"
> +#include "amdgpu_reset.h"
>   
>   MODULE_IMPORT_NS(DMA_BUF);
>   
> @@ -2059,6 +2060,19 @@ void amdgpu_ttm_set_buffer_funcs_status(struct amdgpu_device *adev, bool enable)
>   
>   		ring = adev->mman.buffer_funcs_ring;
>   		sched = &ring->sched;
> +
> +		r = drm_sched_init(sched, &amdgpu_sched_ops,
> +				   DRM_SCHED_PRIORITY_COUNT,
> +				   ring->num_hw_submission, 0,
> +				   adev->sdma_timeout, adev->reset_domain->wq,
> +				   ring->sched_score, ring->name,
> +				   adev->dev);
> +		if (r) {
> +			drm_err(adev, "%s: couldn't initialize ring:%s error:%d\n",
> +				__func__, ring->name, r);
> +			return;
> +		}

That doesn't look correct either.

amdgpu_ttm_set_buffer_funcs_status() should only be called with 
enable=true as argument *after* the copy ring is initialized and valid 
to use. One part of this ring initialization is to setup the scheduler.


> +
>   		r = drm_sched_entity_init(&adev->mman.high_pr,
>   					  DRM_SCHED_PRIORITY_KERNEL, &sched,
>   					  1, NULL);

That here looks totally incorrect and misplaced to me. 
amdgpu_ttm_set_buffer_funcs_status() should only enabled/disable using 
the copy functions and not really initialize the entity.

So the entity should only be created when enable=true and it should 
especially *not* re-created all the time without properly destroying it.

Can you look at the history of the code? I'm pretty sure that this was 
at some time correctly implemented.

Thanks,
Christian.

>
> base-commit: 05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1
> prerequisite-patch-id: c52673df9b6fc9ee001d6261c7ac107b618912a0


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: Initialize schedulers before using them
  2023-10-23  5:49 ` Christian König
@ 2023-10-24  2:55   ` Luben Tuikov
  2023-10-24  6:00     ` Christian König
  0 siblings, 1 reply; 9+ messages in thread
From: Luben Tuikov @ 2023-10-24  2:55 UTC (permalink / raw)
  To: Christian König, AMD Graphics
  Cc: Alex Deucher, Felix Kuehling, Christian König

On 2023-10-23 01:49, Christian König wrote:
> 
> 
> Am 23.10.23 um 05:23 schrieb Luben Tuikov:
>> Initialize ring schedulers before using them, very early in the amdgpu boot,
>> at PCI probe time, specifically at frame-buffer dumb-create at fill-buffer.
>>
>> This was discovered by using dynamic scheduler run-queues, which showed that
>> amdgpu was using a scheduler before calling drm_sched_init(), and the only
>> reason it was working was because sched_rq[] was statically allocated in the
>> scheduler structure. However, the scheduler structure had _not_ been
>> initialized.
>>
>> When switching to dynamically allocated run-queues, this lack of
>> initialization was causing an oops and a blank screen at boot up. This patch
>> fixes this amdgpu bug.
>>
>> This patch depends on the "drm/sched: Convert the GPU scheduler to variable
>> number of run-queues" patch, as that patch prevents subsequent scheduler
>> initialization if a scheduler has already been initialized.
>>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Alex Deucher <Alexander.Deucher@amd.com>
>> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>> Cc: AMD Graphics <amd-gfx@lists.freedesktop.org>
>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 4e51dce3aab5d6..575ef7e1e30fd4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -60,6 +60,7 @@
>>   #include "amdgpu_atomfirmware.h"
>>   #include "amdgpu_res_cursor.h"
>>   #include "bif/bif_4_1_d.h"
>> +#include "amdgpu_reset.h"
>>   
>>   MODULE_IMPORT_NS(DMA_BUF);
>>   
>> @@ -2059,6 +2060,19 @@ void amdgpu_ttm_set_buffer_funcs_status(struct amdgpu_device *adev, bool enable)
>>   
>>   		ring = adev->mman.buffer_funcs_ring;
>>   		sched = &ring->sched;
>> +
>> +		r = drm_sched_init(sched, &amdgpu_sched_ops,
>> +				   DRM_SCHED_PRIORITY_COUNT,
>> +				   ring->num_hw_submission, 0,
>> +				   adev->sdma_timeout, adev->reset_domain->wq,
>> +				   ring->sched_score, ring->name,
>> +				   adev->dev);
>> +		if (r) {
>> +			drm_err(adev, "%s: couldn't initialize ring:%s error:%d\n",
>> +				__func__, ring->name, r);
>> +			return;
>> +		}
> 
> That doesn't look correct either.
> 
> amdgpu_ttm_set_buffer_funcs_status() should only be called with 
> enable=true as argument *after* the copy ring is initialized and valid 
> to use. One part of this ring initialization is to setup the scheduler.

It's the only way to keep the functionality of amdgpu_fill_buffer()
from amdgpu_mode_dumb_create(), from drm_client_framebuffer_create(),
from ... without an oops and a blank screen at boot up.

Here is a stack of the oops:

Oct 20 22:12:34 fedora kernel: RIP: 0010:drm_sched_job_arm+0x1f/0x60 [gpu_sched]
Oct 20 22:12:34 fedora kernel: Code: 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 55 53 48 8b 6f 58 48 85 ed 74 3f 48 89 fb 48 89 ef e8 95 34 00 00 48 8b 45 10 <48> 8b 50 08 48 89 53 18 8b 45 24 89 43 54 b8 01 00 00 00 f0 48 0f
Oct 20 22:12:34 fedora kernel: RSP: 0018:ffffc90001613838 EFLAGS: 00010246
Oct 20 22:12:34 fedora kernel: RAX: 0000000000000000 RBX: ffff88812f33b400 RCX: 0000000000000004
Oct 20 22:12:34 fedora kernel: RDX: 0000000000000000 RSI: ffffc9000395145c RDI: ffff88812eacf850
Oct 20 22:12:34 fedora kernel: RBP: ffff88812eacf850 R08: 0000000000000004 R09: 0000000000030000
Oct 20 22:12:34 fedora kernel: R10: ffffffffc066b850 R11: ffffffffbc848ef1 R12: 0000000000000000
Oct 20 22:12:34 fedora kernel: R13: 0000000000000004 R14: 0000008003000000 R15: 0000000001000000
Oct 20 22:12:34 fedora kernel: FS:  00007f7be4866940(0000) GS:ffff88880ed00000(0000) knlGS:0000000000000000
Oct 20 22:12:34 fedora kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Oct 20 22:12:34 fedora kernel: CR2: 0000000000000008 CR3: 000000012cf22000 CR4: 00000000003506e0
Oct 20 22:12:34 fedora kernel: Call Trace:
Oct 20 22:12:34 fedora kernel:  <TASK>
Oct 20 22:12:34 fedora kernel:  ? __die+0x1f/0x70
Oct 20 22:12:34 fedora kernel:  ? page_fault_oops+0x149/0x440
Oct 20 22:12:34 fedora kernel:  ? drm_sched_fence_alloc+0x1a/0x40 [gpu_sched]
Oct 20 22:12:34 fedora kernel:  ? amdgpu_job_alloc_with_ib+0x34/0xb0 [amdgpu]
Oct 20 22:12:34 fedora kernel:  ? srso_return_thunk+0x5/0x10
Oct 20 22:12:34 fedora kernel:  ? do_user_addr_fault+0x65/0x650
Oct 20 22:12:34 fedora kernel:  ? drm_client_framebuffer_create+0xa3/0x280 [drm]
Oct 20 22:12:34 fedora kernel:  ? exc_page_fault+0x7b/0x180
Oct 20 22:12:34 fedora kernel:  ? asm_exc_page_fault+0x22/0x30
Oct 20 22:12:34 fedora kernel:  ? local_pci_probe+0x41/0x90
Oct 20 22:12:34 fedora kernel:  ? __pfx_sdma_v5_0_emit_fill_buffer+0x10/0x10 [amdgpu]
Oct 20 22:12:34 fedora kernel:  ? drm_sched_job_arm+0x1f/0x60 [gpu_sched]
Oct 20 22:12:34 fedora kernel:  ? drm_sched_job_arm+0x1b/0x60 [gpu_sched]
Oct 20 22:12:34 fedora kernel:  amdgpu_job_submit+0xf/0x70 [amdgpu]
Oct 20 22:12:34 fedora kernel:  amdgpu_fill_buffer+0x2b4/0x650 [amdgpu]
Oct 20 22:12:34 fedora kernel:  amdgpu_bo_create+0x401/0x4a0 [amdgpu]
Oct 20 22:12:34 fedora kernel:  ? srso_return_thunk+0x5/0x10
Oct 20 22:12:34 fedora kernel:  amdgpu_bo_create_user+0x24/0x40 [amdgpu]
Oct 20 22:12:34 fedora kernel:  amdgpu_mode_dumb_create+0xf8/0x1a0 [amdgpu]
Oct 20 22:12:34 fedora kernel:  ? drm_client_framebuffer_create+0x69/0x280 [drm]
Oct 20 22:12:34 fedora kernel:  ? __pfx_amdgpu_bo_user_destroy+0x10/0x10 [amdgpu]
Oct 20 22:12:34 fedora kernel:  drm_client_framebuffer_create+0xa3/0x280 [drm]
Oct 20 22:12:34 fedora kernel:  ? amdgpu_vm_bo_add+0x2a/0xb0 [amdgpu]
Oct 20 22:12:34 fedora kernel:  drm_fbdev_generic_helper_fb_probe+0x61/0x190 [drm_kms_helper]
Oct 20 22:12:34 fedora kernel:  __drm_fb_helper_initial_config_and_unlock+0x297/0x500 [drm_kms_helper]
Oct 20 22:12:34 fedora kernel:  drm_fbdev_generic_client_hotplug+0x66/0xb0 [drm_kms_helper]
Oct 20 22:12:34 fedora kernel:  drm_client_register+0x75/0xb0 [drm]
Oct 20 22:12:34 fedora kernel:  amdgpu_pci_probe+0x3ac/0x440 [amdgpu]
Oct 20 22:12:34 fedora kernel:  local_pci_probe+0x41/0x90
Oct 20 22:12:34 fedora kernel:  pci_device_probe+0xb3/0x210
Oct 20 22:12:34 fedora kernel:  really_probe+0x19e/0x3e0
Oct 20 22:12:34 fedora kernel:  ? __pfx___driver_attach+0x10/0x10
Oct 20 22:12:34 fedora kernel:  __driver_probe_device+0x78/0x160
Oct 20 22:12:34 fedora kernel:  driver_probe_device+0x1f/0x90
Oct 20 22:12:34 fedora kernel:  __driver_attach+0xce/0x1c0
Oct 20 22:12:34 fedora kernel:  bus_for_each_dev+0x63/0xa0
Oct 20 22:12:34 fedora kernel:  bus_add_driver+0x112/0x210
Oct 20 22:12:34 fedora kernel:  driver_register+0x55/0x100
Oct 20 22:12:34 fedora kernel:  ? __pfx_amdgpu_init+0x10/0x10 [amdgpu]
Oct 20 22:12:34 fedora kernel:  do_one_initcall+0x46/0x310
Oct 20 22:12:34 fedora kernel:  ? srso_return_thunk+0x5/0x10
Oct 20 22:12:34 fedora kernel:  ? kmalloc_trace+0x26/0x90
Oct 20 22:12:34 fedora kernel:  do_init_module+0x60/0x230
Oct 20 22:12:34 fedora kernel:  init_module_from_file+0x75/0xa0
Oct 20 22:12:34 fedora kernel:  idempotent_init_module+0xf9/0x270
Oct 20 22:12:34 fedora kernel:  __x64_sys_finit_module+0x5a/0xb0
Oct 20 22:12:34 fedora kernel:  do_syscall_64+0x3b/0x90
Oct 20 22:12:34 fedora kernel:  entry_SYSCALL_64_after_hwframe+0x6e/0xd8

It is at PCI probe time, when DRM probes for an fb dev.

> 
> 
>> +
>>   		r = drm_sched_entity_init(&adev->mman.high_pr,
>>   					  DRM_SCHED_PRIORITY_KERNEL, &sched,
>>   					  1, NULL);
> 
> That here looks totally incorrect and misplaced to me. 
> amdgpu_ttm_set_buffer_funcs_status() should only enabled/disable using 
> the copy functions and not really initialize the entity.
> 
> So the entity should only be created when enable=true and it should 
> especially *not* re-created all the time without properly destroying it.
> 
> Can you look at the history of the code? I'm pretty sure that this was 
> at some time correctly implemented.

Yeah, the drm_sched_entity_init() line above--which is not part of this
patch--comes from this commit:

commit c3aaca43fb07ce
Author: Mukul Joshi <mukul.joshi@amd.com>
Date:   Wed May 17 15:53:50 2023 -0400

    drm/amdgpu: Add a low priority scheduler for VRAM clearing
    
    Add a low priority DRM scheduler for VRAM clearing instead of using
    the exisiting high priority scheduler. Use the high priority scheduler
    for migrations and evictions.
    
    Signed-off-by: Mukul Joshi <mukul.joshi@amd.com>
    Acked-by: Felix Kuehling <Felix.Kuehling@amd.com>
    Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

The options are,

a) Revert c3aaca43fb07ce.

b) Let this patch in, so that it's not blocking the DRM dynamic sched_rq,
   and we can fix this properly in the future locally in amdgpu, as it is
   a driver issue, and it shouldn't be blocking the DRM dynamic sched_rq.
   If we had the dynamic sched_rq in before May 17 of this year, c3aaca43fb07ce
   wouldn't have been able to go in (due to oops). More details in the commit
   message of this patch above.

I'm writing this from a 6.6.0-rc6 + {DRM dynamic sched_rq patch, this patch}.

> 
> Thanks,
> Christian.
> 
>>
>> base-commit: 05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1
>> prerequisite-patch-id: c52673df9b6fc9ee001d6261c7ac107b618912a0
> 

-- 
Regards,
Luben


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: Initialize schedulers before using them
  2023-10-24  2:55   ` Luben Tuikov
@ 2023-10-24  6:00     ` Christian König
  2023-10-24 10:14       ` Christian König
  0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2023-10-24  6:00 UTC (permalink / raw)
  To: Luben Tuikov, AMD Graphics
  Cc: Alex Deucher, Felix Kuehling, Christian König

Am 24.10.23 um 04:55 schrieb Luben Tuikov:
> On 2023-10-23 01:49, Christian König wrote:
>>
>> Am 23.10.23 um 05:23 schrieb Luben Tuikov:
>>> Initialize ring schedulers before using them, very early in the amdgpu boot,
>>> at PCI probe time, specifically at frame-buffer dumb-create at fill-buffer.
>>>
>>> This was discovered by using dynamic scheduler run-queues, which showed that
>>> amdgpu was using a scheduler before calling drm_sched_init(), and the only
>>> reason it was working was because sched_rq[] was statically allocated in the
>>> scheduler structure. However, the scheduler structure had _not_ been
>>> initialized.
>>>
>>> When switching to dynamically allocated run-queues, this lack of
>>> initialization was causing an oops and a blank screen at boot up. This patch
>>> fixes this amdgpu bug.
>>>
>>> This patch depends on the "drm/sched: Convert the GPU scheduler to variable
>>> number of run-queues" patch, as that patch prevents subsequent scheduler
>>> initialization if a scheduler has already been initialized.
>>>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Alex Deucher <Alexander.Deucher@amd.com>
>>> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>>> Cc: AMD Graphics <amd-gfx@lists.freedesktop.org>
>>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 14 ++++++++++++++
>>>    1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> index 4e51dce3aab5d6..575ef7e1e30fd4 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> @@ -60,6 +60,7 @@
>>>    #include "amdgpu_atomfirmware.h"
>>>    #include "amdgpu_res_cursor.h"
>>>    #include "bif/bif_4_1_d.h"
>>> +#include "amdgpu_reset.h"
>>>    
>>>    MODULE_IMPORT_NS(DMA_BUF);
>>>    
>>> @@ -2059,6 +2060,19 @@ void amdgpu_ttm_set_buffer_funcs_status(struct amdgpu_device *adev, bool enable)
>>>    
>>>    		ring = adev->mman.buffer_funcs_ring;
>>>    		sched = &ring->sched;
>>> +
>>> +		r = drm_sched_init(sched, &amdgpu_sched_ops,
>>> +				   DRM_SCHED_PRIORITY_COUNT,
>>> +				   ring->num_hw_submission, 0,
>>> +				   adev->sdma_timeout, adev->reset_domain->wq,
>>> +				   ring->sched_score, ring->name,
>>> +				   adev->dev);
>>> +		if (r) {
>>> +			drm_err(adev, "%s: couldn't initialize ring:%s error:%d\n",
>>> +				__func__, ring->name, r);
>>> +			return;
>>> +		}
>> That doesn't look correct either.
>>
>> amdgpu_ttm_set_buffer_funcs_status() should only be called with
>> enable=true as argument *after* the copy ring is initialized and valid
>> to use. One part of this ring initialization is to setup the scheduler.
> It's the only way to keep the functionality of amdgpu_fill_buffer()
> from amdgpu_mode_dumb_create(), from drm_client_framebuffer_create(),
> from ... without an oops and a blank screen at boot up.
>
> Here is a stack of the oops:
>
> Oct 20 22:12:34 fedora kernel: RIP: 0010:drm_sched_job_arm+0x1f/0x60 [gpu_sched]
> Oct 20 22:12:34 fedora kernel: Code: 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 55 53 48 8b 6f 58 48 85 ed 74 3f 48 89 fb 48 89 ef e8 95 34 00 00 48 8b 45 10 <48> 8b 50 08 48 89 53 18 8b 45 24 89 43 54 b8 01 00 00 00 f0 48 0f
> Oct 20 22:12:34 fedora kernel: RSP: 0018:ffffc90001613838 EFLAGS: 00010246
> Oct 20 22:12:34 fedora kernel: RAX: 0000000000000000 RBX: ffff88812f33b400 RCX: 0000000000000004
> Oct 20 22:12:34 fedora kernel: RDX: 0000000000000000 RSI: ffffc9000395145c RDI: ffff88812eacf850
> Oct 20 22:12:34 fedora kernel: RBP: ffff88812eacf850 R08: 0000000000000004 R09: 0000000000030000
> Oct 20 22:12:34 fedora kernel: R10: ffffffffc066b850 R11: ffffffffbc848ef1 R12: 0000000000000000
> Oct 20 22:12:34 fedora kernel: R13: 0000000000000004 R14: 0000008003000000 R15: 0000000001000000
> Oct 20 22:12:34 fedora kernel: FS:  00007f7be4866940(0000) GS:ffff88880ed00000(0000) knlGS:0000000000000000
> Oct 20 22:12:34 fedora kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> Oct 20 22:12:34 fedora kernel: CR2: 0000000000000008 CR3: 000000012cf22000 CR4: 00000000003506e0
> Oct 20 22:12:34 fedora kernel: Call Trace:
> Oct 20 22:12:34 fedora kernel:  <TASK>
> Oct 20 22:12:34 fedora kernel:  ? __die+0x1f/0x70
> Oct 20 22:12:34 fedora kernel:  ? page_fault_oops+0x149/0x440
> Oct 20 22:12:34 fedora kernel:  ? drm_sched_fence_alloc+0x1a/0x40 [gpu_sched]
> Oct 20 22:12:34 fedora kernel:  ? amdgpu_job_alloc_with_ib+0x34/0xb0 [amdgpu]
> Oct 20 22:12:34 fedora kernel:  ? srso_return_thunk+0x5/0x10
> Oct 20 22:12:34 fedora kernel:  ? do_user_addr_fault+0x65/0x650
> Oct 20 22:12:34 fedora kernel:  ? drm_client_framebuffer_create+0xa3/0x280 [drm]
> Oct 20 22:12:34 fedora kernel:  ? exc_page_fault+0x7b/0x180
> Oct 20 22:12:34 fedora kernel:  ? asm_exc_page_fault+0x22/0x30
> Oct 20 22:12:34 fedora kernel:  ? local_pci_probe+0x41/0x90
> Oct 20 22:12:34 fedora kernel:  ? __pfx_sdma_v5_0_emit_fill_buffer+0x10/0x10 [amdgpu]
> Oct 20 22:12:34 fedora kernel:  ? drm_sched_job_arm+0x1f/0x60 [gpu_sched]
> Oct 20 22:12:34 fedora kernel:  ? drm_sched_job_arm+0x1b/0x60 [gpu_sched]
> Oct 20 22:12:34 fedora kernel:  amdgpu_job_submit+0xf/0x70 [amdgpu]
> Oct 20 22:12:34 fedora kernel:  amdgpu_fill_buffer+0x2b4/0x650 [amdgpu]
> Oct 20 22:12:34 fedora kernel:  amdgpu_bo_create+0x401/0x4a0 [amdgpu]
> Oct 20 22:12:34 fedora kernel:  ? srso_return_thunk+0x5/0x10
> Oct 20 22:12:34 fedora kernel:  amdgpu_bo_create_user+0x24/0x40 [amdgpu]
> Oct 20 22:12:34 fedora kernel:  amdgpu_mode_dumb_create+0xf8/0x1a0 [amdgpu]
> Oct 20 22:12:34 fedora kernel:  ? drm_client_framebuffer_create+0x69/0x280 [drm]
> Oct 20 22:12:34 fedora kernel:  ? __pfx_amdgpu_bo_user_destroy+0x10/0x10 [amdgpu]
> Oct 20 22:12:34 fedora kernel:  drm_client_framebuffer_create+0xa3/0x280 [drm]
> Oct 20 22:12:34 fedora kernel:  ? amdgpu_vm_bo_add+0x2a/0xb0 [amdgpu]
> Oct 20 22:12:34 fedora kernel:  drm_fbdev_generic_helper_fb_probe+0x61/0x190 [drm_kms_helper]
> Oct 20 22:12:34 fedora kernel:  __drm_fb_helper_initial_config_and_unlock+0x297/0x500 [drm_kms_helper]
> Oct 20 22:12:34 fedora kernel:  drm_fbdev_generic_client_hotplug+0x66/0xb0 [drm_kms_helper]
> Oct 20 22:12:34 fedora kernel:  drm_client_register+0x75/0xb0 [drm]
> Oct 20 22:12:34 fedora kernel:  amdgpu_pci_probe+0x3ac/0x440 [amdgpu]
> Oct 20 22:12:34 fedora kernel:  local_pci_probe+0x41/0x90
> Oct 20 22:12:34 fedora kernel:  pci_device_probe+0xb3/0x210
> Oct 20 22:12:34 fedora kernel:  really_probe+0x19e/0x3e0
> Oct 20 22:12:34 fedora kernel:  ? __pfx___driver_attach+0x10/0x10
> Oct 20 22:12:34 fedora kernel:  __driver_probe_device+0x78/0x160
> Oct 20 22:12:34 fedora kernel:  driver_probe_device+0x1f/0x90
> Oct 20 22:12:34 fedora kernel:  __driver_attach+0xce/0x1c0
> Oct 20 22:12:34 fedora kernel:  bus_for_each_dev+0x63/0xa0
> Oct 20 22:12:34 fedora kernel:  bus_add_driver+0x112/0x210
> Oct 20 22:12:34 fedora kernel:  driver_register+0x55/0x100
> Oct 20 22:12:34 fedora kernel:  ? __pfx_amdgpu_init+0x10/0x10 [amdgpu]
> Oct 20 22:12:34 fedora kernel:  do_one_initcall+0x46/0x310
> Oct 20 22:12:34 fedora kernel:  ? srso_return_thunk+0x5/0x10
> Oct 20 22:12:34 fedora kernel:  ? kmalloc_trace+0x26/0x90
> Oct 20 22:12:34 fedora kernel:  do_init_module+0x60/0x230
> Oct 20 22:12:34 fedora kernel:  init_module_from_file+0x75/0xa0
> Oct 20 22:12:34 fedora kernel:  idempotent_init_module+0xf9/0x270
> Oct 20 22:12:34 fedora kernel:  __x64_sys_finit_module+0x5a/0xb0
> Oct 20 22:12:34 fedora kernel:  do_syscall_64+0x3b/0x90
> Oct 20 22:12:34 fedora kernel:  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
>
> It is at PCI probe time, when DRM probes for an fb dev.
>
>>
>>> +
>>>    		r = drm_sched_entity_init(&adev->mman.high_pr,
>>>    					  DRM_SCHED_PRIORITY_KERNEL, &sched,
>>>    					  1, NULL);
>> That here looks totally incorrect and misplaced to me.
>> amdgpu_ttm_set_buffer_funcs_status() should only enabled/disable using
>> the copy functions and not really initialize the entity.
>>
>> So the entity should only be created when enable=true and it should
>> especially *not* re-created all the time without properly destroying it.
>>
>> Can you look at the history of the code? I'm pretty sure that this was
>> at some time correctly implemented.
> Yeah, the drm_sched_entity_init() line above--which is not part of this
> patch--comes from this commit:
>
> commit c3aaca43fb07ce
> Author: Mukul Joshi <mukul.joshi@amd.com>
> Date:   Wed May 17 15:53:50 2023 -0400
>
>      drm/amdgpu: Add a low priority scheduler for VRAM clearing
>      
>      Add a low priority DRM scheduler for VRAM clearing instead of using
>      the exisiting high priority scheduler. Use the high priority scheduler
>      for migrations and evictions.
>      
>      Signed-off-by: Mukul Joshi <mukul.joshi@amd.com>
>      Acked-by: Felix Kuehling <Felix.Kuehling@amd.com>
>      Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>
> The options are,
>
> a) Revert c3aaca43fb07ce.
>
> b) Let this patch in, so that it's not blocking the DRM dynamic sched_rq,
>     and we can fix this properly in the future locally in amdgpu, as it is
>     a driver issue, and it shouldn't be blocking the DRM dynamic sched_rq.
>     If we had the dynamic sched_rq in before May 17 of this year, c3aaca43fb07ce
>     wouldn't have been able to go in (due to oops). More details in the commit
>     message of this patch above.
>
> I'm writing this from a 6.6.0-rc6 + {DRM dynamic sched_rq patch, this patch}.

Let me take a closer look first, there seems to be quite a bunch of 
stuff wrong here. Alex, if you can some input would be helpful as well.

In general "drm/amdgpu: Add a low priority scheduler for VRAM clearing" 
shouldn't have added the entity init in this place, but this is just a 
minor issue.

The bigger problem is that we call drm_client_register() *before* the 
hardware is fully initialized. That is certainly illegal and can cause 
quite a bunch of other problems as well.

What saved us so far was the fact that once the scheduler is created we 
ended up with the right result, e.g. a cleared and allocated buffer. But 
that was pure coincident and not proper engineering.

Thanks for looking into this,
Christian.

>
>> Thanks,
>> Christian.
>>
>>> base-commit: 05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1
>>> prerequisite-patch-id: c52673df9b6fc9ee001d6261c7ac107b618912a0


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: Initialize schedulers before using them
  2023-10-24  6:00     ` Christian König
@ 2023-10-24 10:14       ` Christian König
  2023-10-24 14:46         ` Alex Deucher
  0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2023-10-24 10:14 UTC (permalink / raw)
  To: Luben Tuikov, AMD Graphics
  Cc: Alex Deucher, Felix Kuehling, Christian König

[SNIP]
> Let me take a closer look first

I think I've figured out why this isn't working as expected. It started 
with this patch here:

commit 5fd8518d187ed03403a4d4f7f56f52c00b11c148
Author: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
Date:   Mon Dec 6 14:59:35 2021 -0500

     drm/amdgpu: Move scheduler init to after XGMI is ready

     Before we initialize schedulers we must know which reset
     domain are we in - for single device there iis a single
     domain per device and so single wq per device. For XGMI
     the reset domain spans the entire XGMI hive and so the
     reset wq is per hive.

     Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
     Reviewed-by: Christian König <christian.koenig@amd.com>
     Link: https://www.spinics.net/lists/amd-gfx/msg74112.html

Andrey separated the scheduler initialization from the ring init because 
we need some of the rings for XGMI initialization which in turn in 
necessary to figure out the XGMI hive and so the reset domain for the 
scheduler.

The code inside amdgpu_ttm_set_buffer_funcs_status() is actually 
correct, the problem is that this is called as part of the hw init which 
comes earlier than the scheduler init.

@Alex, Ideas how to fix this? My best guess is that we should move the 
call to amdgpu_ttm_set_buffer_funcs_status() from the DMA specific code 
into the higher level handling in amdgpu_device.c

Regards,
Christian.



^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: Initialize schedulers before using them
  2023-10-24 10:14       ` Christian König
@ 2023-10-24 14:46         ` Alex Deucher
  2023-10-25  1:10           ` Luben Tuikov
  0 siblings, 1 reply; 9+ messages in thread
From: Alex Deucher @ 2023-10-24 14:46 UTC (permalink / raw)
  To: Christian König
  Cc: Alex Deucher, Felix Kuehling, Luben Tuikov, Christian König,
	AMD Graphics

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

On Tue, Oct 24, 2023 at 6:14 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> [SNIP]
> > Let me take a closer look first
>
> I think I've figured out why this isn't working as expected. It started
> with this patch here:
>
> commit 5fd8518d187ed03403a4d4f7f56f52c00b11c148
> Author: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Date:   Mon Dec 6 14:59:35 2021 -0500
>
>      drm/amdgpu: Move scheduler init to after XGMI is ready
>
>      Before we initialize schedulers we must know which reset
>      domain are we in - for single device there iis a single
>      domain per device and so single wq per device. For XGMI
>      the reset domain spans the entire XGMI hive and so the
>      reset wq is per hive.
>
>      Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>      Reviewed-by: Christian König <christian.koenig@amd.com>
>      Link: https://www.spinics.net/lists/amd-gfx/msg74112.html
>
> Andrey separated the scheduler initialization from the ring init because
> we need some of the rings for XGMI initialization which in turn in
> necessary to figure out the XGMI hive and so the reset domain for the
> scheduler.
>
> The code inside amdgpu_ttm_set_buffer_funcs_status() is actually
> correct, the problem is that this is called as part of the hw init which
> comes earlier than the scheduler init.
>
> @Alex, Ideas how to fix this? My best guess is that we should move the
> call to amdgpu_ttm_set_buffer_funcs_status() from the DMA specific code
> into the higher level handling in amdgpu_device.c

Yes, I think so, but there could be some tricky ordering issues with
respect to suspend and resume.  I think something like the attached
patch should do the trick.

Alex

[-- Attachment #2: 0001-drm-amdgpu-move-buffer-funcs-setting-up-a-level.patch --]
[-- Type: text/x-patch, Size: 13358 bytes --]

From e06ec86af03c8f730e8ba2ae8b668bb3727f455d Mon Sep 17 00:00:00 2001
From: Alex Deucher <alexander.deucher@amd.com>
Date: Tue, 24 Oct 2023 10:42:08 -0400
Subject: [PATCH] drm/amdgpu: move buffer funcs setting up a level

Rather than doing this in the IP code for the SDMA paging
engine, move it up to the core device level init level.
This should fix the scheduler init ordering.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c   | 23 ++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h   |  1 +
 drivers/gpu/drm/amd/amdgpu/cik_sdma.c      |  5 -----
 drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c     |  5 -----
 drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c     |  5 -----
 drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c     | 16 +--------------
 drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c     | 10 +---------
 drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c     | 10 +---------
 drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c     | 10 +---------
 drivers/gpu/drm/amd/amdgpu/si_dma.c        |  5 -----
 11 files changed, 40 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index cc047fe0b7ee..7b4120383f89 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2667,6 +2667,8 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev)
 	if (r)
 		goto init_failed;
 
+	amdgpu_sdma_set_buffer_funcs_helper(adev);
+
 	/* Don't init kfd if whole hive need to be reset during init */
 	if (!adev->gmc.xgmi.pending_reset) {
 		kgd2kfd_init_zone_device(adev);
@@ -3265,6 +3267,8 @@ int amdgpu_device_ip_suspend(struct amdgpu_device *adev)
 		amdgpu_virt_request_full_gpu(adev, false);
 	}
 
+	amdgpu_sdma_unset_buffer_funcs_helper(adev);
+
 	r = amdgpu_device_ip_suspend_phase1(adev);
 	if (r)
 		return r;
@@ -3454,6 +3458,8 @@ static int amdgpu_device_ip_resume(struct amdgpu_device *adev)
 
 	r = amdgpu_device_ip_resume_phase2(adev);
 
+	amdgpu_sdma_set_buffer_funcs_helper(adev);
+
 	return r;
 }
 
@@ -4241,6 +4247,8 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
 	/* disable ras feature must before hw fini */
 	amdgpu_ras_pre_fini(adev);
 
+	amdgpu_sdma_unset_buffer_funcs_helper(adev);
+
 	amdgpu_device_ip_fini_early(adev);
 
 	amdgpu_irq_fini_hw(adev);
@@ -4412,6 +4420,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
 
 	amdgpu_ras_suspend(adev);
 
+	amdgpu_sdma_unset_buffer_funcs_helper(adev);
+
 	amdgpu_device_ip_suspend_phase1(adev);
 
 	if (!adev->in_s0ix)
@@ -5183,6 +5193,8 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle,
 				if (r)
 					goto out;
 
+				amdgpu_sdma_set_buffer_funcs_helper(tmp_adev);
+
 				if (vram_lost)
 					amdgpu_device_fill_reset_magic(tmp_adev);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
index e8cbc4142d80..33f88fc9d92f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
@@ -292,6 +292,29 @@ int amdgpu_sdma_init_microcode(struct amdgpu_device *adev,
 	return err;
 }
 
+void amdgpu_sdma_set_buffer_funcs_helper(struct amdgpu_device *adev)
+{
+	struct amdgpu_ring *sdma;
+	int i;
+
+	for (i = 0; i < adev->sdma.num_instances; i++) {
+		if (adev->sdma.has_page_queue) {
+			sdma = &adev->sdma.instance[i].page;
+			if ((adev->mman.buffer_funcs_ring == sdma) &&
+			    sdma->sched.ready) {
+				amdgpu_ttm_set_buffer_funcs_status(adev, true);
+				break;
+			}
+		}
+		sdma = &adev->sdma.instance[i].ring;
+		if ((adev->mman.buffer_funcs_ring == sdma) &&
+		    sdma->sched.ready) {
+			amdgpu_ttm_set_buffer_funcs_status(adev, true);
+			break;
+		}
+	}
+}
+
 void amdgpu_sdma_unset_buffer_funcs_helper(struct amdgpu_device *adev)
 {
 	struct amdgpu_ring *sdma;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
index 513ac22120c1..33209593e974 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
@@ -169,6 +169,7 @@ int amdgpu_sdma_init_microcode(struct amdgpu_device *adev, u32 instance,
 			       bool duplicate);
 void amdgpu_sdma_destroy_inst_ctx(struct amdgpu_device *adev,
         bool duplicate);
+void amdgpu_sdma_set_buffer_funcs_helper(struct amdgpu_device *adev);
 void amdgpu_sdma_unset_buffer_funcs_helper(struct amdgpu_device *adev);
 int amdgpu_sdma_ras_sw_init(struct amdgpu_device *adev);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
index ee5dce6f6043..a3fccc4c1f43 100644
--- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
@@ -308,8 +308,6 @@ static void cik_sdma_gfx_stop(struct amdgpu_device *adev)
 	u32 rb_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32(mmSDMA0_GFX_RB_CNTL + sdma_offsets[i]);
 		rb_cntl &= ~SDMA0_GFX_RB_CNTL__RB_ENABLE_MASK;
@@ -498,9 +496,6 @@ static int cik_sdma_gfx_resume(struct amdgpu_device *adev)
 		r = amdgpu_ring_test_helper(ring);
 		if (r)
 			return r;
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
index b58a13bd75db..45377a175250 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
@@ -339,8 +339,6 @@ static void sdma_v2_4_gfx_stop(struct amdgpu_device *adev)
 	u32 rb_cntl, ib_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32(mmSDMA0_GFX_RB_CNTL + sdma_offsets[i]);
 		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_GFX_RB_CNTL, RB_ENABLE, 0);
@@ -474,9 +472,6 @@ static int sdma_v2_4_gfx_resume(struct amdgpu_device *adev)
 		r = amdgpu_ring_test_helper(ring);
 		if (r)
 			return r;
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
index c5ea32687eb5..2ad615be4bb3 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
@@ -513,8 +513,6 @@ static void sdma_v3_0_gfx_stop(struct amdgpu_device *adev)
 	u32 rb_cntl, ib_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32(mmSDMA0_GFX_RB_CNTL + sdma_offsets[i]);
 		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_GFX_RB_CNTL, RB_ENABLE, 0);
@@ -746,9 +744,6 @@ static int sdma_v3_0_gfx_resume(struct amdgpu_device *adev)
 		r = amdgpu_ring_test_helper(ring);
 		if (r)
 			return r;
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
index 683d51ae4bf1..3d68dd5523c6 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
@@ -877,8 +877,6 @@ static void sdma_v4_0_gfx_enable(struct amdgpu_device *adev, bool enable)
 	u32 rb_cntl, ib_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32_SDMA(i, mmSDMA0_GFX_RB_CNTL);
 		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_GFX_RB_CNTL, RB_ENABLE, enable ? 1 : 0);
@@ -913,8 +911,6 @@ static void sdma_v4_0_page_stop(struct amdgpu_device *adev)
 	u32 rb_cntl, ib_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32_SDMA(i, mmSDMA0_PAGE_RB_CNTL);
 		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_PAGE_RB_CNTL,
@@ -1402,13 +1398,7 @@ static int sdma_v4_0_start(struct amdgpu_device *adev)
 			r = amdgpu_ring_test_helper(page);
 			if (r)
 				return r;
-
-			if (adev->mman.buffer_funcs_ring == page)
-				amdgpu_ttm_set_buffer_funcs_status(adev, true);
 		}
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return r;
@@ -1921,11 +1911,8 @@ static int sdma_v4_0_hw_fini(void *handle)
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 	int i;
 
-	if (amdgpu_sriov_vf(adev)) {
-		/* disable the scheduler for SDMA */
-		amdgpu_sdma_unset_buffer_funcs_helper(adev);
+	if (amdgpu_sriov_vf(adev))
 		return 0;
-	}
 
 	if (amdgpu_ras_is_supported(adev, AMDGPU_RAS_BLOCK__SDMA)) {
 		for (i = 0; i < adev->sdma.num_instances; i++) {
@@ -1964,7 +1951,6 @@ static int sdma_v4_0_resume(void *handle)
 	if (adev->in_s0ix) {
 		sdma_v4_0_enable(adev, true);
 		sdma_v4_0_gfx_enable(adev, true);
-		amdgpu_ttm_set_buffer_funcs_status(adev, true);
 		return 0;
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
index c1ff5eda8961..3c485e5a531a 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
@@ -559,8 +559,6 @@ static void sdma_v5_0_gfx_stop(struct amdgpu_device *adev)
 	u32 rb_cntl, ib_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32_SOC15_IP(GC, sdma_v5_0_get_reg_offset(adev, i, mmSDMA0_GFX_RB_CNTL));
 		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_GFX_RB_CNTL, RB_ENABLE, 0);
@@ -825,9 +823,6 @@ static int sdma_v5_0_gfx_resume(struct amdgpu_device *adev)
 		r = amdgpu_ring_test_helper(ring);
 		if (r)
 			return r;
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return 0;
@@ -1426,11 +1421,8 @@ static int sdma_v5_0_hw_fini(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-	if (amdgpu_sriov_vf(adev)) {
-		/* disable the scheduler for SDMA */
-		amdgpu_sdma_unset_buffer_funcs_helper(adev);
+	if (amdgpu_sriov_vf(adev))
 		return 0;
-	}
 
 	sdma_v5_0_ctx_switch_enable(adev, false);
 	sdma_v5_0_enable(adev, false);
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
index 7d1e57189c8c..83c240f741b5 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
@@ -364,8 +364,6 @@ static void sdma_v5_2_gfx_stop(struct amdgpu_device *adev)
 	u32 rb_cntl, ib_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_GFX_RB_CNTL));
 		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_GFX_RB_CNTL, RB_ENABLE, 0);
@@ -625,9 +623,6 @@ static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev)
 		r = amdgpu_ring_test_helper(ring);
 		if (r)
 			return r;
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return 0;
@@ -1284,11 +1279,8 @@ static int sdma_v5_2_hw_fini(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-	if (amdgpu_sriov_vf(adev)) {
-		/* disable the scheduler for SDMA */
-		amdgpu_sdma_unset_buffer_funcs_helper(adev);
+	if (amdgpu_sriov_vf(adev))
 		return 0;
-	}
 
 	sdma_v5_2_ctx_switch_enable(adev, false);
 	sdma_v5_2_enable(adev, false);
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
index 7e4d5188cbfa..3c7ddd219de8 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
@@ -348,8 +348,6 @@ static void sdma_v6_0_gfx_stop(struct amdgpu_device *adev)
 	u32 rb_cntl, ib_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32_SOC15_IP(GC, sdma_v6_0_get_reg_offset(adev, i, regSDMA0_QUEUE0_RB_CNTL));
 		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_QUEUE0_RB_CNTL, RB_ENABLE, 0);
@@ -561,9 +559,6 @@ static int sdma_v6_0_gfx_resume(struct amdgpu_device *adev)
 		r = amdgpu_ring_test_helper(ring);
 		if (r)
 			return r;
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return 0;
@@ -1308,11 +1303,8 @@ static int sdma_v6_0_hw_fini(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-	if (amdgpu_sriov_vf(adev)) {
-		/* disable the scheduler for SDMA */
-		amdgpu_sdma_unset_buffer_funcs_helper(adev);
+	if (amdgpu_sriov_vf(adev))
 		return 0;
-	}
 
 	sdma_v6_0_ctxempty_int_enable(adev, false);
 	sdma_v6_0_enable(adev, false);
diff --git a/drivers/gpu/drm/amd/amdgpu/si_dma.c b/drivers/gpu/drm/amd/amdgpu/si_dma.c
index 42c4547f32ec..9aa0e11ee673 100644
--- a/drivers/gpu/drm/amd/amdgpu/si_dma.c
+++ b/drivers/gpu/drm/amd/amdgpu/si_dma.c
@@ -115,8 +115,6 @@ static void si_dma_stop(struct amdgpu_device *adev)
 	u32 rb_cntl;
 	unsigned i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		/* dma0 */
 		rb_cntl = RREG32(DMA_RB_CNTL + sdma_offsets[i]);
@@ -177,9 +175,6 @@ static int si_dma_start(struct amdgpu_device *adev)
 		r = amdgpu_ring_test_helper(ring);
 		if (r)
 			return r;
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return 0;
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: Initialize schedulers before using them
  2023-10-24 14:46         ` Alex Deucher
@ 2023-10-25  1:10           ` Luben Tuikov
  2023-10-25  4:24             ` [PATCH] drm/amdgpu: move buffer funcs setting up a level (v2) Luben Tuikov
  0 siblings, 1 reply; 9+ messages in thread
From: Luben Tuikov @ 2023-10-25  1:10 UTC (permalink / raw)
  To: Alex Deucher, Christian König
  Cc: Alex Deucher, Felix Kuehling, Christian König, AMD Graphics

On 2023-10-24 10:46, Alex Deucher wrote:
> On Tue, Oct 24, 2023 at 6:14 AM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>>
>> [SNIP]
>>> Let me take a closer look first
>>
>> I think I've figured out why this isn't working as expected. It started
>> with this patch here:
>>
>> commit 5fd8518d187ed03403a4d4f7f56f52c00b11c148
>> Author: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> Date:   Mon Dec 6 14:59:35 2021 -0500
>>
>>      drm/amdgpu: Move scheduler init to after XGMI is ready
>>
>>      Before we initialize schedulers we must know which reset
>>      domain are we in - for single device there iis a single
>>      domain per device and so single wq per device. For XGMI
>>      the reset domain spans the entire XGMI hive and so the
>>      reset wq is per hive.
>>
>>      Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>      Reviewed-by: Christian König <christian.koenig@amd.com>
>>      Link: https://www.spinics.net/lists/amd-gfx/msg74112.html
>>
>> Andrey separated the scheduler initialization from the ring init because
>> we need some of the rings for XGMI initialization which in turn in
>> necessary to figure out the XGMI hive and so the reset domain for the
>> scheduler.
>>
>> The code inside amdgpu_ttm_set_buffer_funcs_status() is actually
>> correct, the problem is that this is called as part of the hw init which
>> comes earlier than the scheduler init.
>>
>> @Alex, Ideas how to fix this? My best guess is that we should move the
>> call to amdgpu_ttm_set_buffer_funcs_status() from the DMA specific code
>> into the higher level handling in amdgpu_device.c
> 
> Yes, I think so, but there could be some tricky ordering issues with
> respect to suspend and resume.  I think something like the attached
> patch should do the trick.

This patch works. I've tested suspend and resume too.

Tested-by: Luben Tuikov <luben.tuikov@amd.com>

scripts/checkpatch.pl complains about extra parenthesis.

-- 
Regards,
Luben


^ permalink raw reply	[flat|nested] 9+ messages in thread
* [PATCH] drm/amdgpu: move buffer funcs setting up a level (v2)
  2023-10-25  1:10           ` Luben Tuikov
@ 2023-10-25  4:24             ` Luben Tuikov
  2023-10-25  5:40               ` Christian König
  0 siblings, 1 reply; 9+ messages in thread
From: Luben Tuikov @ 2023-10-25  4:24 UTC (permalink / raw)
  To: Alex Deucher, Christian König
  Cc: Alex Deucher, Felix Kuehling, Luben Tuikov, Christian König,
	AMD Graphics

From: Alex Deucher <alexander.deucher@amd.com>

Rather than doing this in the IP code for the SDMA paging
engine, move it up to the core device level init level.
This should fix the scheduler init ordering.

v2: Fix checkpatch parens complaint; long lines. (Luben)

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
Tested-by: Luben Tuikov <luben.tuikov@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 ++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c   | 21 +++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h   |  1 +
 drivers/gpu/drm/amd/amdgpu/cik_sdma.c      |  5 -----
 drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c     |  5 -----
 drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c     |  5 -----
 drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c     | 16 +---------------
 drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c     | 10 +---------
 drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c     | 10 +---------
 drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c     | 10 +---------
 drivers/gpu/drm/amd/amdgpu/si_dma.c        |  5 -----
 11 files changed, 38 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 7ec32b44df052f..47c1e60109c14c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2662,6 +2662,8 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev)
 	if (r)
 		goto init_failed;
 
+	amdgpu_sdma_set_buffer_funcs_helper(adev);
+
 	/* Don't init kfd if whole hive need to be reset during init */
 	if (!adev->gmc.xgmi.pending_reset) {
 		kgd2kfd_init_zone_device(adev);
@@ -3260,6 +3262,8 @@ int amdgpu_device_ip_suspend(struct amdgpu_device *adev)
 		amdgpu_virt_request_full_gpu(adev, false);
 	}
 
+	amdgpu_sdma_unset_buffer_funcs_helper(adev);
+
 	r = amdgpu_device_ip_suspend_phase1(adev);
 	if (r)
 		return r;
@@ -3449,6 +3453,8 @@ static int amdgpu_device_ip_resume(struct amdgpu_device *adev)
 
 	r = amdgpu_device_ip_resume_phase2(adev);
 
+	amdgpu_sdma_set_buffer_funcs_helper(adev);
+
 	return r;
 }
 
@@ -4236,6 +4242,8 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
 	/* disable ras feature must before hw fini */
 	amdgpu_ras_pre_fini(adev);
 
+	amdgpu_sdma_unset_buffer_funcs_helper(adev);
+
 	amdgpu_device_ip_fini_early(adev);
 
 	amdgpu_irq_fini_hw(adev);
@@ -4407,6 +4415,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
 
 	amdgpu_ras_suspend(adev);
 
+	amdgpu_sdma_unset_buffer_funcs_helper(adev);
+
 	amdgpu_device_ip_suspend_phase1(adev);
 
 	if (!adev->in_s0ix)
@@ -5178,6 +5188,8 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle,
 				if (r)
 					goto out;
 
+				amdgpu_sdma_set_buffer_funcs_helper(tmp_adev);
+
 				if (vram_lost)
 					amdgpu_device_fill_reset_magic(tmp_adev);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
index e8cbc4142d8021..c4d642b06f3c5b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
@@ -292,6 +292,27 @@ int amdgpu_sdma_init_microcode(struct amdgpu_device *adev,
 	return err;
 }
 
+void amdgpu_sdma_set_buffer_funcs_helper(struct amdgpu_device *adev)
+{
+	struct amdgpu_ring *sdma;
+	int i;
+
+	for (i = 0; i < adev->sdma.num_instances; i++) {
+		if (adev->sdma.has_page_queue) {
+			sdma = &adev->sdma.instance[i].page;
+			if (adev->mman.buffer_funcs_ring == sdma && sdma->sched.ready) {
+				amdgpu_ttm_set_buffer_funcs_status(adev, true);
+				break;
+			}
+		}
+		sdma = &adev->sdma.instance[i].ring;
+		if (adev->mman.buffer_funcs_ring == sdma && sdma->sched.ready) {
+			amdgpu_ttm_set_buffer_funcs_status(adev, true);
+			break;
+		}
+	}
+}
+
 void amdgpu_sdma_unset_buffer_funcs_helper(struct amdgpu_device *adev)
 {
 	struct amdgpu_ring *sdma;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
index 513ac22120c1fa..33209593e97461 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
@@ -169,6 +169,7 @@ int amdgpu_sdma_init_microcode(struct amdgpu_device *adev, u32 instance,
 			       bool duplicate);
 void amdgpu_sdma_destroy_inst_ctx(struct amdgpu_device *adev,
         bool duplicate);
+void amdgpu_sdma_set_buffer_funcs_helper(struct amdgpu_device *adev);
 void amdgpu_sdma_unset_buffer_funcs_helper(struct amdgpu_device *adev);
 int amdgpu_sdma_ras_sw_init(struct amdgpu_device *adev);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
index ee5dce6f604369..a3fccc4c1f4375 100644
--- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
@@ -308,8 +308,6 @@ static void cik_sdma_gfx_stop(struct amdgpu_device *adev)
 	u32 rb_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32(mmSDMA0_GFX_RB_CNTL + sdma_offsets[i]);
 		rb_cntl &= ~SDMA0_GFX_RB_CNTL__RB_ENABLE_MASK;
@@ -498,9 +496,6 @@ static int cik_sdma_gfx_resume(struct amdgpu_device *adev)
 		r = amdgpu_ring_test_helper(ring);
 		if (r)
 			return r;
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
index b58a13bd75db8f..45377a1752503b 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
@@ -339,8 +339,6 @@ static void sdma_v2_4_gfx_stop(struct amdgpu_device *adev)
 	u32 rb_cntl, ib_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32(mmSDMA0_GFX_RB_CNTL + sdma_offsets[i]);
 		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_GFX_RB_CNTL, RB_ENABLE, 0);
@@ -474,9 +472,6 @@ static int sdma_v2_4_gfx_resume(struct amdgpu_device *adev)
 		r = amdgpu_ring_test_helper(ring);
 		if (r)
 			return r;
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
index c5ea32687eb5e8..2ad615be4bb3d2 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
@@ -513,8 +513,6 @@ static void sdma_v3_0_gfx_stop(struct amdgpu_device *adev)
 	u32 rb_cntl, ib_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32(mmSDMA0_GFX_RB_CNTL + sdma_offsets[i]);
 		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_GFX_RB_CNTL, RB_ENABLE, 0);
@@ -746,9 +744,6 @@ static int sdma_v3_0_gfx_resume(struct amdgpu_device *adev)
 		r = amdgpu_ring_test_helper(ring);
 		if (r)
 			return r;
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
index 683d51ae4bf10c..3d68dd5523c65a 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
@@ -877,8 +877,6 @@ static void sdma_v4_0_gfx_enable(struct amdgpu_device *adev, bool enable)
 	u32 rb_cntl, ib_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32_SDMA(i, mmSDMA0_GFX_RB_CNTL);
 		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_GFX_RB_CNTL, RB_ENABLE, enable ? 1 : 0);
@@ -913,8 +911,6 @@ static void sdma_v4_0_page_stop(struct amdgpu_device *adev)
 	u32 rb_cntl, ib_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32_SDMA(i, mmSDMA0_PAGE_RB_CNTL);
 		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_PAGE_RB_CNTL,
@@ -1402,13 +1398,7 @@ static int sdma_v4_0_start(struct amdgpu_device *adev)
 			r = amdgpu_ring_test_helper(page);
 			if (r)
 				return r;
-
-			if (adev->mman.buffer_funcs_ring == page)
-				amdgpu_ttm_set_buffer_funcs_status(adev, true);
 		}
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return r;
@@ -1921,11 +1911,8 @@ static int sdma_v4_0_hw_fini(void *handle)
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 	int i;
 
-	if (amdgpu_sriov_vf(adev)) {
-		/* disable the scheduler for SDMA */
-		amdgpu_sdma_unset_buffer_funcs_helper(adev);
+	if (amdgpu_sriov_vf(adev))
 		return 0;
-	}
 
 	if (amdgpu_ras_is_supported(adev, AMDGPU_RAS_BLOCK__SDMA)) {
 		for (i = 0; i < adev->sdma.num_instances; i++) {
@@ -1964,7 +1951,6 @@ static int sdma_v4_0_resume(void *handle)
 	if (adev->in_s0ix) {
 		sdma_v4_0_enable(adev, true);
 		sdma_v4_0_gfx_enable(adev, true);
-		amdgpu_ttm_set_buffer_funcs_status(adev, true);
 		return 0;
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
index c1ff5eda89619f..3c485e5a531a0e 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
@@ -559,8 +559,6 @@ static void sdma_v5_0_gfx_stop(struct amdgpu_device *adev)
 	u32 rb_cntl, ib_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32_SOC15_IP(GC, sdma_v5_0_get_reg_offset(adev, i, mmSDMA0_GFX_RB_CNTL));
 		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_GFX_RB_CNTL, RB_ENABLE, 0);
@@ -825,9 +823,6 @@ static int sdma_v5_0_gfx_resume(struct amdgpu_device *adev)
 		r = amdgpu_ring_test_helper(ring);
 		if (r)
 			return r;
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return 0;
@@ -1426,11 +1421,8 @@ static int sdma_v5_0_hw_fini(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-	if (amdgpu_sriov_vf(adev)) {
-		/* disable the scheduler for SDMA */
-		amdgpu_sdma_unset_buffer_funcs_helper(adev);
+	if (amdgpu_sriov_vf(adev))
 		return 0;
-	}
 
 	sdma_v5_0_ctx_switch_enable(adev, false);
 	sdma_v5_0_enable(adev, false);
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
index 7d1e57189c8c69..83c240f741b519 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
@@ -364,8 +364,6 @@ static void sdma_v5_2_gfx_stop(struct amdgpu_device *adev)
 	u32 rb_cntl, ib_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_GFX_RB_CNTL));
 		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_GFX_RB_CNTL, RB_ENABLE, 0);
@@ -625,9 +623,6 @@ static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev)
 		r = amdgpu_ring_test_helper(ring);
 		if (r)
 			return r;
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return 0;
@@ -1284,11 +1279,8 @@ static int sdma_v5_2_hw_fini(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-	if (amdgpu_sriov_vf(adev)) {
-		/* disable the scheduler for SDMA */
-		amdgpu_sdma_unset_buffer_funcs_helper(adev);
+	if (amdgpu_sriov_vf(adev))
 		return 0;
-	}
 
 	sdma_v5_2_ctx_switch_enable(adev, false);
 	sdma_v5_2_enable(adev, false);
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
index 7e4d5188cbfa85..3c7ddd219de850 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
@@ -348,8 +348,6 @@ static void sdma_v6_0_gfx_stop(struct amdgpu_device *adev)
 	u32 rb_cntl, ib_cntl;
 	int i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		rb_cntl = RREG32_SOC15_IP(GC, sdma_v6_0_get_reg_offset(adev, i, regSDMA0_QUEUE0_RB_CNTL));
 		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_QUEUE0_RB_CNTL, RB_ENABLE, 0);
@@ -561,9 +559,6 @@ static int sdma_v6_0_gfx_resume(struct amdgpu_device *adev)
 		r = amdgpu_ring_test_helper(ring);
 		if (r)
 			return r;
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return 0;
@@ -1308,11 +1303,8 @@ static int sdma_v6_0_hw_fini(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-	if (amdgpu_sriov_vf(adev)) {
-		/* disable the scheduler for SDMA */
-		amdgpu_sdma_unset_buffer_funcs_helper(adev);
+	if (amdgpu_sriov_vf(adev))
 		return 0;
-	}
 
 	sdma_v6_0_ctxempty_int_enable(adev, false);
 	sdma_v6_0_enable(adev, false);
diff --git a/drivers/gpu/drm/amd/amdgpu/si_dma.c b/drivers/gpu/drm/amd/amdgpu/si_dma.c
index 42c4547f32ec9c..9aa0e11ee67327 100644
--- a/drivers/gpu/drm/amd/amdgpu/si_dma.c
+++ b/drivers/gpu/drm/amd/amdgpu/si_dma.c
@@ -115,8 +115,6 @@ static void si_dma_stop(struct amdgpu_device *adev)
 	u32 rb_cntl;
 	unsigned i;
 
-	amdgpu_sdma_unset_buffer_funcs_helper(adev);
-
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		/* dma0 */
 		rb_cntl = RREG32(DMA_RB_CNTL + sdma_offsets[i]);
@@ -177,9 +175,6 @@ static int si_dma_start(struct amdgpu_device *adev)
 		r = amdgpu_ring_test_helper(ring);
 		if (r)
 			return r;
-
-		if (adev->mman.buffer_funcs_ring == ring)
-			amdgpu_ttm_set_buffer_funcs_status(adev, true);
 	}
 
 	return 0;

base-commit: fefaa6c51e990dc8e5142729accef661f475a731
-- 
2.42.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: move buffer funcs setting up a level (v2)
  2023-10-25  4:24             ` [PATCH] drm/amdgpu: move buffer funcs setting up a level (v2) Luben Tuikov
@ 2023-10-25  5:40               ` Christian König
  0 siblings, 0 replies; 9+ messages in thread
From: Christian König @ 2023-10-25  5:40 UTC (permalink / raw)
  To: Luben Tuikov, Alex Deucher
  Cc: Alex Deucher, Felix Kuehling, Christian König, AMD Graphics

Am 25.10.23 um 06:24 schrieb Luben Tuikov:
> From: Alex Deucher <alexander.deucher@amd.com>
>
> Rather than doing this in the IP code for the SDMA paging
> engine, move it up to the core device level init level.
> This should fix the scheduler init ordering.
>
> v2: Fix checkpatch parens complaint; long lines. (Luben)
>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> Tested-by: Luben Tuikov <luben.tuikov@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 ++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c   | 21 +++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h   |  1 +
>   drivers/gpu/drm/amd/amdgpu/cik_sdma.c      |  5 -----
>   drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c     |  5 -----
>   drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c     |  5 -----
>   drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c     | 16 +---------------
>   drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c     | 10 +---------
>   drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c     | 10 +---------
>   drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c     | 10 +---------
>   drivers/gpu/drm/amd/amdgpu/si_dma.c        |  5 -----
>   11 files changed, 38 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 7ec32b44df052f..47c1e60109c14c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2662,6 +2662,8 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev)
>   	if (r)
>   		goto init_failed;
>   
> +	amdgpu_sdma_set_buffer_funcs_helper(adev);
> +
>   	/* Don't init kfd if whole hive need to be reset during init */
>   	if (!adev->gmc.xgmi.pending_reset) {
>   		kgd2kfd_init_zone_device(adev);
> @@ -3260,6 +3262,8 @@ int amdgpu_device_ip_suspend(struct amdgpu_device *adev)
>   		amdgpu_virt_request_full_gpu(adev, false);
>   	}
>   
> +	amdgpu_sdma_unset_buffer_funcs_helper(adev);
> +
>   	r = amdgpu_device_ip_suspend_phase1(adev);
>   	if (r)
>   		return r;
> @@ -3449,6 +3453,8 @@ static int amdgpu_device_ip_resume(struct amdgpu_device *adev)
>   
>   	r = amdgpu_device_ip_resume_phase2(adev);
>   
> +	amdgpu_sdma_set_buffer_funcs_helper(adev);
> +
>   	return r;
>   }
>   
> @@ -4236,6 +4242,8 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
>   	/* disable ras feature must before hw fini */
>   	amdgpu_ras_pre_fini(adev);
>   
> +	amdgpu_sdma_unset_buffer_funcs_helper(adev);
> +
>   	amdgpu_device_ip_fini_early(adev);
>   
>   	amdgpu_irq_fini_hw(adev);
> @@ -4407,6 +4415,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
>   
>   	amdgpu_ras_suspend(adev);
>   
> +	amdgpu_sdma_unset_buffer_funcs_helper(adev);
> +
>   	amdgpu_device_ip_suspend_phase1(adev);
>   
>   	if (!adev->in_s0ix)
> @@ -5178,6 +5188,8 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle,
>   				if (r)
>   					goto out;
>   
> +				amdgpu_sdma_set_buffer_funcs_helper(tmp_adev);
> +
>   				if (vram_lost)
>   					amdgpu_device_fill_reset_magic(tmp_adev);
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> index e8cbc4142d8021..c4d642b06f3c5b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> @@ -292,6 +292,27 @@ int amdgpu_sdma_init_microcode(struct amdgpu_device *adev,
>   	return err;
>   }
>   
> +void amdgpu_sdma_set_buffer_funcs_helper(struct amdgpu_device *adev)

 From the functionality and general idea that looks good to me.

But I think both the amdgpu_sdma_set_buffer_funcs_helper() as well the 
existing amdgpu_sdma_unset_buffer_funcs_helper() are just an unnecessary 
extra check when they are not used by the SDMA code.

I think we should just call amdgpu_ttm_set_buffer_funcs_status() 
directly instead.

Regards,
Christian.

> +{
> +	struct amdgpu_ring *sdma;
> +	int i;
> +
> +	for (i = 0; i < adev->sdma.num_instances; i++) {
> +		if (adev->sdma.has_page_queue) {
> +			sdma = &adev->sdma.instance[i].page;
> +			if (adev->mman.buffer_funcs_ring == sdma && sdma->sched.ready) {
> +				amdgpu_ttm_set_buffer_funcs_status(adev, true);
> +				break;
> +			}
> +		}
> +		sdma = &adev->sdma.instance[i].ring;
> +		if (adev->mman.buffer_funcs_ring == sdma && sdma->sched.ready) {
> +			amdgpu_ttm_set_buffer_funcs_status(adev, true);
> +			break;
> +		}
> +	}
> +}
> +
>   void amdgpu_sdma_unset_buffer_funcs_helper(struct amdgpu_device *adev)
>   {
>   	struct amdgpu_ring *sdma;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> index 513ac22120c1fa..33209593e97461 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> @@ -169,6 +169,7 @@ int amdgpu_sdma_init_microcode(struct amdgpu_device *adev, u32 instance,
>   			       bool duplicate);
>   void amdgpu_sdma_destroy_inst_ctx(struct amdgpu_device *adev,
>           bool duplicate);
> +void amdgpu_sdma_set_buffer_funcs_helper(struct amdgpu_device *adev);
>   void amdgpu_sdma_unset_buffer_funcs_helper(struct amdgpu_device *adev);
>   int amdgpu_sdma_ras_sw_init(struct amdgpu_device *adev);
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
> index ee5dce6f604369..a3fccc4c1f4375 100644
> --- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
> @@ -308,8 +308,6 @@ static void cik_sdma_gfx_stop(struct amdgpu_device *adev)
>   	u32 rb_cntl;
>   	int i;
>   
> -	amdgpu_sdma_unset_buffer_funcs_helper(adev);
> -
>   	for (i = 0; i < adev->sdma.num_instances; i++) {
>   		rb_cntl = RREG32(mmSDMA0_GFX_RB_CNTL + sdma_offsets[i]);
>   		rb_cntl &= ~SDMA0_GFX_RB_CNTL__RB_ENABLE_MASK;
> @@ -498,9 +496,6 @@ static int cik_sdma_gfx_resume(struct amdgpu_device *adev)
>   		r = amdgpu_ring_test_helper(ring);
>   		if (r)
>   			return r;
> -
> -		if (adev->mman.buffer_funcs_ring == ring)
> -			amdgpu_ttm_set_buffer_funcs_status(adev, true);
>   	}
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
> index b58a13bd75db8f..45377a1752503b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
> @@ -339,8 +339,6 @@ static void sdma_v2_4_gfx_stop(struct amdgpu_device *adev)
>   	u32 rb_cntl, ib_cntl;
>   	int i;
>   
> -	amdgpu_sdma_unset_buffer_funcs_helper(adev);
> -
>   	for (i = 0; i < adev->sdma.num_instances; i++) {
>   		rb_cntl = RREG32(mmSDMA0_GFX_RB_CNTL + sdma_offsets[i]);
>   		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_GFX_RB_CNTL, RB_ENABLE, 0);
> @@ -474,9 +472,6 @@ static int sdma_v2_4_gfx_resume(struct amdgpu_device *adev)
>   		r = amdgpu_ring_test_helper(ring);
>   		if (r)
>   			return r;
> -
> -		if (adev->mman.buffer_funcs_ring == ring)
> -			amdgpu_ttm_set_buffer_funcs_status(adev, true);
>   	}
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> index c5ea32687eb5e8..2ad615be4bb3d2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> @@ -513,8 +513,6 @@ static void sdma_v3_0_gfx_stop(struct amdgpu_device *adev)
>   	u32 rb_cntl, ib_cntl;
>   	int i;
>   
> -	amdgpu_sdma_unset_buffer_funcs_helper(adev);
> -
>   	for (i = 0; i < adev->sdma.num_instances; i++) {
>   		rb_cntl = RREG32(mmSDMA0_GFX_RB_CNTL + sdma_offsets[i]);
>   		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_GFX_RB_CNTL, RB_ENABLE, 0);
> @@ -746,9 +744,6 @@ static int sdma_v3_0_gfx_resume(struct amdgpu_device *adev)
>   		r = amdgpu_ring_test_helper(ring);
>   		if (r)
>   			return r;
> -
> -		if (adev->mman.buffer_funcs_ring == ring)
> -			amdgpu_ttm_set_buffer_funcs_status(adev, true);
>   	}
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> index 683d51ae4bf10c..3d68dd5523c65a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> @@ -877,8 +877,6 @@ static void sdma_v4_0_gfx_enable(struct amdgpu_device *adev, bool enable)
>   	u32 rb_cntl, ib_cntl;
>   	int i;
>   
> -	amdgpu_sdma_unset_buffer_funcs_helper(adev);
> -
>   	for (i = 0; i < adev->sdma.num_instances; i++) {
>   		rb_cntl = RREG32_SDMA(i, mmSDMA0_GFX_RB_CNTL);
>   		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_GFX_RB_CNTL, RB_ENABLE, enable ? 1 : 0);
> @@ -913,8 +911,6 @@ static void sdma_v4_0_page_stop(struct amdgpu_device *adev)
>   	u32 rb_cntl, ib_cntl;
>   	int i;
>   
> -	amdgpu_sdma_unset_buffer_funcs_helper(adev);
> -
>   	for (i = 0; i < adev->sdma.num_instances; i++) {
>   		rb_cntl = RREG32_SDMA(i, mmSDMA0_PAGE_RB_CNTL);
>   		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_PAGE_RB_CNTL,
> @@ -1402,13 +1398,7 @@ static int sdma_v4_0_start(struct amdgpu_device *adev)
>   			r = amdgpu_ring_test_helper(page);
>   			if (r)
>   				return r;
> -
> -			if (adev->mman.buffer_funcs_ring == page)
> -				amdgpu_ttm_set_buffer_funcs_status(adev, true);
>   		}
> -
> -		if (adev->mman.buffer_funcs_ring == ring)
> -			amdgpu_ttm_set_buffer_funcs_status(adev, true);
>   	}
>   
>   	return r;
> @@ -1921,11 +1911,8 @@ static int sdma_v4_0_hw_fini(void *handle)
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>   	int i;
>   
> -	if (amdgpu_sriov_vf(adev)) {
> -		/* disable the scheduler for SDMA */
> -		amdgpu_sdma_unset_buffer_funcs_helper(adev);
> +	if (amdgpu_sriov_vf(adev))
>   		return 0;
> -	}
>   
>   	if (amdgpu_ras_is_supported(adev, AMDGPU_RAS_BLOCK__SDMA)) {
>   		for (i = 0; i < adev->sdma.num_instances; i++) {
> @@ -1964,7 +1951,6 @@ static int sdma_v4_0_resume(void *handle)
>   	if (adev->in_s0ix) {
>   		sdma_v4_0_enable(adev, true);
>   		sdma_v4_0_gfx_enable(adev, true);
> -		amdgpu_ttm_set_buffer_funcs_status(adev, true);
>   		return 0;
>   	}
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> index c1ff5eda89619f..3c485e5a531a0e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> @@ -559,8 +559,6 @@ static void sdma_v5_0_gfx_stop(struct amdgpu_device *adev)
>   	u32 rb_cntl, ib_cntl;
>   	int i;
>   
> -	amdgpu_sdma_unset_buffer_funcs_helper(adev);
> -
>   	for (i = 0; i < adev->sdma.num_instances; i++) {
>   		rb_cntl = RREG32_SOC15_IP(GC, sdma_v5_0_get_reg_offset(adev, i, mmSDMA0_GFX_RB_CNTL));
>   		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_GFX_RB_CNTL, RB_ENABLE, 0);
> @@ -825,9 +823,6 @@ static int sdma_v5_0_gfx_resume(struct amdgpu_device *adev)
>   		r = amdgpu_ring_test_helper(ring);
>   		if (r)
>   			return r;
> -
> -		if (adev->mman.buffer_funcs_ring == ring)
> -			amdgpu_ttm_set_buffer_funcs_status(adev, true);
>   	}
>   
>   	return 0;
> @@ -1426,11 +1421,8 @@ static int sdma_v5_0_hw_fini(void *handle)
>   {
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>   
> -	if (amdgpu_sriov_vf(adev)) {
> -		/* disable the scheduler for SDMA */
> -		amdgpu_sdma_unset_buffer_funcs_helper(adev);
> +	if (amdgpu_sriov_vf(adev))
>   		return 0;
> -	}
>   
>   	sdma_v5_0_ctx_switch_enable(adev, false);
>   	sdma_v5_0_enable(adev, false);
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> index 7d1e57189c8c69..83c240f741b519 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> @@ -364,8 +364,6 @@ static void sdma_v5_2_gfx_stop(struct amdgpu_device *adev)
>   	u32 rb_cntl, ib_cntl;
>   	int i;
>   
> -	amdgpu_sdma_unset_buffer_funcs_helper(adev);
> -
>   	for (i = 0; i < adev->sdma.num_instances; i++) {
>   		rb_cntl = RREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_GFX_RB_CNTL));
>   		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_GFX_RB_CNTL, RB_ENABLE, 0);
> @@ -625,9 +623,6 @@ static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev)
>   		r = amdgpu_ring_test_helper(ring);
>   		if (r)
>   			return r;
> -
> -		if (adev->mman.buffer_funcs_ring == ring)
> -			amdgpu_ttm_set_buffer_funcs_status(adev, true);
>   	}
>   
>   	return 0;
> @@ -1284,11 +1279,8 @@ static int sdma_v5_2_hw_fini(void *handle)
>   {
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>   
> -	if (amdgpu_sriov_vf(adev)) {
> -		/* disable the scheduler for SDMA */
> -		amdgpu_sdma_unset_buffer_funcs_helper(adev);
> +	if (amdgpu_sriov_vf(adev))
>   		return 0;
> -	}
>   
>   	sdma_v5_2_ctx_switch_enable(adev, false);
>   	sdma_v5_2_enable(adev, false);
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> index 7e4d5188cbfa85..3c7ddd219de850 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> @@ -348,8 +348,6 @@ static void sdma_v6_0_gfx_stop(struct amdgpu_device *adev)
>   	u32 rb_cntl, ib_cntl;
>   	int i;
>   
> -	amdgpu_sdma_unset_buffer_funcs_helper(adev);
> -
>   	for (i = 0; i < adev->sdma.num_instances; i++) {
>   		rb_cntl = RREG32_SOC15_IP(GC, sdma_v6_0_get_reg_offset(adev, i, regSDMA0_QUEUE0_RB_CNTL));
>   		rb_cntl = REG_SET_FIELD(rb_cntl, SDMA0_QUEUE0_RB_CNTL, RB_ENABLE, 0);
> @@ -561,9 +559,6 @@ static int sdma_v6_0_gfx_resume(struct amdgpu_device *adev)
>   		r = amdgpu_ring_test_helper(ring);
>   		if (r)
>   			return r;
> -
> -		if (adev->mman.buffer_funcs_ring == ring)
> -			amdgpu_ttm_set_buffer_funcs_status(adev, true);
>   	}
>   
>   	return 0;
> @@ -1308,11 +1303,8 @@ static int sdma_v6_0_hw_fini(void *handle)
>   {
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>   
> -	if (amdgpu_sriov_vf(adev)) {
> -		/* disable the scheduler for SDMA */
> -		amdgpu_sdma_unset_buffer_funcs_helper(adev);
> +	if (amdgpu_sriov_vf(adev))
>   		return 0;
> -	}
>   
>   	sdma_v6_0_ctxempty_int_enable(adev, false);
>   	sdma_v6_0_enable(adev, false);
> diff --git a/drivers/gpu/drm/amd/amdgpu/si_dma.c b/drivers/gpu/drm/amd/amdgpu/si_dma.c
> index 42c4547f32ec9c..9aa0e11ee67327 100644
> --- a/drivers/gpu/drm/amd/amdgpu/si_dma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/si_dma.c
> @@ -115,8 +115,6 @@ static void si_dma_stop(struct amdgpu_device *adev)
>   	u32 rb_cntl;
>   	unsigned i;
>   
> -	amdgpu_sdma_unset_buffer_funcs_helper(adev);
> -
>   	for (i = 0; i < adev->sdma.num_instances; i++) {
>   		/* dma0 */
>   		rb_cntl = RREG32(DMA_RB_CNTL + sdma_offsets[i]);
> @@ -177,9 +175,6 @@ static int si_dma_start(struct amdgpu_device *adev)
>   		r = amdgpu_ring_test_helper(ring);
>   		if (r)
>   			return r;
> -
> -		if (adev->mman.buffer_funcs_ring == ring)
> -			amdgpu_ttm_set_buffer_funcs_status(adev, true);
>   	}
>   
>   	return 0;
>
> base-commit: fefaa6c51e990dc8e5142729accef661f475a731


^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-10-25  5:41 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-23  3:23 [PATCH] drm/amdgpu: Initialize schedulers before using them Luben Tuikov
2023-10-23  5:49 ` Christian König
2023-10-24  2:55   ` Luben Tuikov
2023-10-24  6:00     ` Christian König
2023-10-24 10:14       ` Christian König
2023-10-24 14:46         ` Alex Deucher
2023-10-25  1:10           ` Luben Tuikov
2023-10-25  4:24             ` [PATCH] drm/amdgpu: move buffer funcs setting up a level (v2) Luben Tuikov
2023-10-25  5:40               ` 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.