Re: [PATCH v3 2/5] drm/amdgpu: add gpu_recovery_enabled flag to amdgpu_device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/amdgpu: gpu_recovery_enabled left false in amdgpu_device_init()
- [Medium] drm/amdgpu: per-device gpu_recovery_enabled ignored in userq and KFD resets
--

commit 86b9554fae60bb7deeb057b1bff4576f147cba44
Author: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

drm/amdgpu: add gpu_recovery_enabled flag to amdgpu_device

This commit adds a per-device gpu_recovery_enabled boolean to struct
amdgpu_device and exposes it via the amdgpu_gpu_recover debugfs file to
control automatic GPU recovery independently of the global
amdgpu.gpu_recovery module parameter. It also updates
amdgpu_device_should_recover_gpu() to check this flag.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 92cc0fa7ba33b..3658e46141ada 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4149,6 +4149,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>  		}
>  	}
>  
> +	adev->gpu_recovery_enabled = true;
> +
>  fence_driver_init:
>  	/* Fence driver */
>  	r = amdgpu_fence_driver_sw_init(adev);

[Severity: Medium]
Does placing the initialization of adev->gpu_recovery_enabled before the
fence_driver_init label leave it false when amdgpu_emu_mode == 1?

Earlier in amdgpu_device_init(), the emulation mode path jumps directly to
fence_driver_init:

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:amdgpu_device_init() {
	...
	if (amdgpu_emu_mode == 1) {
		/* post the asic on emulation mode */
		emu_soc_asic_init(adev);
		goto fence_driver_init;
	}
	...
}

Because struct amdgpu_device is zero-allocated in amdgpu_pci_probe() via
devm_drm_dev_alloc(), jumping to fence_driver_init bypasses setting
adev->gpu_recovery_enabled = true. As a result,
amdgpu_device_should_recover_gpu() will treat GPU recovery as disabled
during amdgpu_device_ip_init() -> amdgpu_get_soft_full_reset_mask() and on
runtime hangs. Subsequent commits in this series do not adjust this
placement.

Should adev->gpu_recovery_enabled = true be moved after the
fence_driver_init label (or before the goto fence_driver_init)?

[ ... ]

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index bff2d38f3571a..c54ff34572b00 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> @@ -980,9 +980,13 @@ static int amdgpu_debugfs_fence_info_show(struct seq_file *m, void *unused)
>  }
>  
>  /*
> - * amdgpu_debugfs_gpu_recover - manually trigger a gpu reset & recover
> + * amdgpu_debugfs_gpu_recover - manually trigger a gpu reset & recover,
> + * and control whether this device is allowed to auto-recover from a hang.
>   *
> - * Manually trigger a gpu reset at the next fence wait.
> + * Read triggers a gpu reset at the next fence wait.
> + *
> + * Write 0/1 to disable/enable auto GPU recovery for this device
> + * (equivalent to amdgpu.gpu_recovery=0, but scoped to this device only).
>   */
>  static int gpu_recover_get(void *data, u64 *val)
>  {
> @@ -1006,8 +1010,17 @@ static int gpu_recover_get(void *data, u64 *val)
>  	return 0;
>  }
>  
> +static int gpu_recover_set(void *data, u64 val)
> +{
> +	struct amdgpu_device *adev = (struct amdgpu_device *)data;
> +
> +	adev->gpu_recovery_enabled = !!val;
> +
> +	return 0;
> +}

[Severity: Medium]
The comment states that writing 0 to amdgpu_gpu_recover is equivalent to
amdgpu.gpu_recovery=0 scoped to this device, and later commits in this
series ("drm/amdgpu: honor gpu_recovery_enabled in amdgpu_job_timedout"
and "drm/amdgpu: add wedge event implementation") update
amdgpu_job_timedout() to check adev->gpu_recovery_enabled.

However, several other hang recovery and timeout paths only check the
global amdgpu_gpu_recovery variable and do not consult
adev->gpu_recovery_enabled or amdgpu_device_should_recover_gpu(adev):

1. Usermode queue hang detection and reset in
drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c:

amdgpu_userq_hang_detect_work() {
	...
	if (!amdgpu_gpu_recovery)
		return;
	...
}

amdgpu_userq_mgr_reset_work() {
	...
	if (!amdgpu_gpu_recovery)
		return;
	...
	amdgpu_device_gpu_recover(adev, NULL, &reset_context);
}

2. KFD queue and HWS hang resets in
drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c within
remove_queue_mes_on_reset_option(), reset_queues_mes(), and
reset_queues_on_hws_hang(), which only check if (!amdgpu_gpu_recovery).

3. Timeout enablement checks in drivers/gpu/drm/amd/amdgpu/amdgpu.h:

static inline bool amdgpu_device_has_timeouts_enabled(struct amdgpu_device *adev)
{
	return amdgpu_gpu_recovery != 0 &&
		adev->gfx_timeout != MAX_SCHEDULE_TIMEOUT &&
	...
}

When a user writes 0 to amdgpu_gpu_recover while amdgpu.gpu_recovery
remains at its default non-zero value, should these userq, KFD, and
timeout helpers also check adev->gpu_recovery_enabled so that per-device
recovery is fully disabled?

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