[PATCH v3 1/3] drm/amdgpu: don't disable ttm buffer funcs on reset

[PATCH v3 1/3] drm/amdgpu: don't disable ttm buffer funcs on reset

[Pierre-Eric Pelloux-Prayer]

Access to hw is already suspended so there are no reasons to
disable ttm buffer funcs.

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 964efec0d335..b7751dde2894 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3162,8 +3162,6 @@ static int amdgpu_device_ip_suspend(struct amdgpu_device *adev)
 		amdgpu_virt_request_full_gpu(adev, false);
 	}
 
-	amdgpu_ttm_disable_buffer_funcs(adev);
-
 	r = amdgpu_device_ip_suspend_phase1(adev);
 	if (r)
 		return r;
-- 
2.43.0

[PATCH v3 2/3] drm/amdgpu: delay ttm buffer func enablement on xgmi

[Pierre-Eric Pelloux-Prayer]

When amdgpu_init_minimal_xgmi is used, SDMA engines init
is delayed so amdgpu_ttm_enable_buffer_funcs must be
called later.

Without this, the check for num_buffer_funcs_scheds will
fail and using ttm buffer funcs later will fail.

Given that amdgpu_ttm_enable_buffer_funcs is a no-op if
amdgpu_in_reset() returns true, the call has to occur
after the reset lock is dropped.

Fixes: 3a5da695c8d1 ("drm/amdgpu: only use working sdma schedulers for ttm")
Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 +++++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c   | 3 +++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index b7751dde2894..1ec3162cb175 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2474,7 +2474,11 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev)
 	if (r)
 		goto init_failed;
 
-	amdgpu_ttm_enable_buffer_funcs(adev);
+	/* If SDMA is not brought up during hwini, the ttm buffer funcs enablement
+	 * is delayed after reset-on-init completes.
+	 */
+	if (amdgpu_ip_member_of_hwini(adev, AMD_IP_BLOCK_TYPE_SDMA))
+		amdgpu_ttm_enable_buffer_funcs(adev);
 
 	/* Don't init kfd if whole hive need to be reset during init */
 	if (adev->init_lvl->level != AMDGPU_INIT_LEVEL_MINIMAL_XGMI) {
@@ -5088,8 +5092,6 @@ int amdgpu_device_reinit_after_reset(struct amdgpu_reset_context *reset_context)
 				if (r)
 					goto out;
 
-				amdgpu_ttm_enable_buffer_funcs(tmp_adev);
-
 				r = amdgpu_device_ip_resume_phase3(tmp_adev);
 				if (r)
 					goto out;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
index 2725230aa5e3..45e31b3daf06 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
@@ -1380,6 +1380,9 @@ static void amdgpu_xgmi_reset_on_init_work(struct work_struct *work)
 	amdgpu_device_unlock_reset_domain(tmp_adev->reset_domain);
 
 	list_for_each_entry(tmp_adev, &hive->device_list, gmc.xgmi.head) {
+		/* Enable ttm buffers funcs after the reset lock has been dropped. */
+		amdgpu_ttm_enable_buffer_funcs(tmp_adev);
+
 		r = amdgpu_ras_init_badpage_info(tmp_adev);
 		if (r && r != -EHWPOISON)
 			dev_err(tmp_adev->dev,
-- 
2.43.0

[PATCH v3 3/3] drm/amdgpu: remove useless enable/disable of ttm buffer funcs

[Pierre-Eric Pelloux-Prayer]

amdgpu_ttm_disable_buffer_funcs and amdgpu_ttm_enable_buffer_funcs should
each only be called once per amdgpu_device: at startup and teardown.

Therefore the other calls are redundant and can be safely dropped.

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 -----
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    | 1 -
 2 files changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 1ec3162cb175..e966e6f7d2fa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3379,8 +3379,6 @@ static int amdgpu_device_ip_resume(struct amdgpu_device *adev)
 
 	r = amdgpu_device_ip_resume_phase2(adev);
 
-	amdgpu_ttm_enable_buffer_funcs(adev);
-
 	if (r)
 		return r;
 
@@ -4511,8 +4509,6 @@ int amdgpu_device_suspend(struct drm_device *dev, bool notify_clients)
 	if (r)
 		goto unwind_userq;
 
-	amdgpu_ttm_disable_buffer_funcs(adev);
-
 	amdgpu_fence_driver_hw_fini(adev);
 
 	r = amdgpu_device_ip_suspend_phase2(adev);
@@ -4525,7 +4521,6 @@ int amdgpu_device_suspend(struct drm_device *dev, bool notify_clients)
 	return 0;
 
 unwind_evict:
-	amdgpu_ttm_enable_buffer_funcs(adev);
 	amdgpu_fence_driver_hw_init(adev);
 
 unwind_userq:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 5bcb6a966941..ba99edbfe3a6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -2137,7 +2137,6 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
 	}
 
 	/* Change the size here instead of the init above so only lpfn is affected */
-	amdgpu_ttm_disable_buffer_funcs(adev);
 #ifdef CONFIG_64BIT
 #ifdef CONFIG_X86
 	if (adev->gmc.xgmi.connected_to_cpu)
-- 
2.43.0

Re: [PATCH v3 2/3] drm/amdgpu: delay ttm buffer func enablement on xgmi

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] `amdgpu_ttm_enable_buffer_funcs` is called asynchronously without locks, racing with device teardown.

Pre-existing issues:
- [High] Iterating over `hive->device_list` without holding `hive->hive_lock` causes a data race and potential use-after-free.
--

--- Patch [2]: [PATCH v3 2/3] drm/amdgpu: delay ttm buffer func enablement on xgmi ---
commit 9b07616fd6cac22b50c5fa10e4c312c149b74bbd
Author: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

drm/amdgpu: delay ttm buffer func enablement on xgmi

This commit delays SDMA engine init and ttm buffer func enablement until
after the reset lock is dropped for XGMI devices.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
> index 2725230aa5e39..45e31b3daf065 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
> @@ -1380,6 +1380,9 @@ static void amdgpu_xgmi_reset_on_init_work(struct work_struct *work)
>  	amdgpu_device_unlock_reset_domain(tmp_adev->reset_domain);
>  
>  	list_for_each_entry(tmp_adev, &hive->device_list, gmc.xgmi.head) {

[Severity: High]
This is a pre-existing issue, but does iterating over hive->device_list
without holding hive->hive_lock cause a data race and potential
use-after-free?

If a device is hot-unplugged or unbound while this async work is
running, couldn't another thread mutate the list (e.g., via
amdgpu_xgmi_remove_device), causing list corruption or accessing a freed
device?

> +		/* Enable ttm buffers funcs after the reset lock has been dropped. */
> +		amdgpu_ttm_enable_buffer_funcs(tmp_adev);

[Severity: High]
Does calling amdgpu_ttm_enable_buffer_funcs() asynchronously without
device locks introduce a race condition with device teardown?

If a device is unbound while reset_on_init_work is queued, the teardown
path calls amdgpu_device_fini_hw() -> amdgpu_ttm_disable_buffer_funcs().
Since buffer_funcs_enabled is still false, it returns early.

If this async work runs afterward, it calls
amdgpu_ttm_enable_buffer_funcs() which allocates memory and sets
buffer_funcs_enabled to true:

amdgpu_ttm_enable_buffer_funcs() {
    ...
	adev->mman.clear_entities = kcalloc(num_clear_entities,
						sizeof(struct amdgpu_ttm_buffer_entity),
						GFP_KERNEL);
    ...
}

Will this cause memory allocations to leak because teardown has already
completed, or potentially execute on an already-freed tmp_adev?

> +
>  		r = amdgpu_ras_init_badpage_info(tmp_adev);
>  		if (r && r != -EHWPOISON)
>  			dev_err(tmp_adev->dev,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810120218.23000-1-pierre-eric.pelloux-prayer@amd.com?part=2

Re: [PATCH v3 3/3] drm/amdgpu: remove useless enable/disable of ttm buffer funcs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing `amdgpu_ttm_disable_buffer_funcs` during system suspend leaves hardware-accelerated memory moves enabled, causing TTM to submit SDMA jobs while the SDMA hardware is suspended or fence interrupts are disabled.
--

--- Patch [3]: [PATCH v3 3/3] drm/amdgpu: remove useless enable/disable of ttm buffer funcs ---
commit fbef8cb928db8ff229937f5e75fbde6b525226c0
Author: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

drm/amdgpu: remove useless enable/disable of ttm buffer funcs

This commit removes calls to amdgpu_ttm_disable_buffer_funcs and
amdgpu_ttm_enable_buffer_funcs, indicating they are redundant outside of
device startup and teardown.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 1ec3162cb1750..e966e6f7d2fa6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3379,8 +3379,6 @@ static int amdgpu_device_ip_resume(struct amdgpu_device *adev)
>  
>  	r = amdgpu_device_ip_resume_phase2(adev);
>  
> -	amdgpu_ttm_enable_buffer_funcs(adev);
> -
>  	if (r)
>  		return r;

[Severity: High]
Does removing this call in amdgpu_device_ip_resume() cause BO moves triggered
in earlier phases to hang? If enablement is not explicitly delayed to this
safe point, TTM might attempt to use SDMA before it is fully ready, such as
during SR-IOV late reinit where DCE resumes before SDMA.

[ ... ]

> @@ -4511,8 +4509,6 @@ int amdgpu_device_suspend(struct drm_device *dev, bool notify_clients)
>  	if (r)
>  		goto unwind_userq;
>  
> -	amdgpu_ttm_disable_buffer_funcs(adev);
> -
>  	amdgpu_fence_driver_hw_fini(adev);

[Severity: High]
Does removing amdgpu_ttm_disable_buffer_funcs() in amdgpu_device_suspend()
leave hardware-accelerated memory moves enabled while the hardware is being
suspended? 

During system suspend, resume, or GPU reset, BO memory moves can still occur.
If TTM buffer functions are not disabled, TTM might submit BO moves to the
SDMA hardware while it is suspended, or while fence interrupts are disabled
by the subsequent call to amdgpu_fence_driver_hw_fini(). Could this lead to
deadlocks from fence timeouts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810120218.23000-1-pierre-eric.pelloux-prayer@amd.com?part=3