Re: [PATCH v4 4/6] drm/amdgpu: handle gpu_recovery per device

["Christian König" <christian.koenig@amd.com>]

On 11/9/26 18:13, Pierre-Eric Pelloux-Prayer wrote:
> Instead of having a module wide parameter, store gpu_recovery
> in amdgpu_device. Its initial value is based on the boot param
> and it can be overridden by writing to the existing
> amdgpu_gpu_recover debugfs file, so a single device can have
> auto-recovery disabled without affecting every other GPU in the
> system.
> 
> amdgpu_device_should_recover_gpu() now only consider this param.
> 
> Assisted-by: LLM
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h           | 11 ++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    | 92 +++++++++----------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c     | 21 ++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c       |  4 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c       |  6 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c      |  4 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c     |  4 +-
>  drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c         |  2 +-
>  drivers/gpu/drm/amd/amdgpu/soc15.c            |  2 +-
>  .../drm/amd/amdkfd/kfd_device_queue_manager.c |  6 +-
>  10 files changed, 81 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 5a32b54b1e9b..a486ac746a5b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -982,6 +982,9 @@ struct amdgpu_device {
>  
>  	struct amdgpu_uma_carveout_info uma_info;
>  
> +	/* Per device gpu_recovery setting. 0 means disabled. */
> +	unsigned int gpu_recovery;
> +
>  	/* KFD
>  	 * Must be last --ends in a flexible-array member.
>  	 */
> @@ -1195,7 +1198,6 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>  
>  /* Common functions */
>  bool amdgpu_device_has_job_running(struct amdgpu_device *adev);
> -bool amdgpu_device_should_recover_gpu(struct amdgpu_device *adev);
>  int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>  			      struct amdgpu_job *job,
>  			      struct amdgpu_reset_context *reset_context);
> @@ -1323,9 +1325,14 @@ int amdgpu_device_set_cg_state(struct amdgpu_device *adev,
>  int amdgpu_device_set_pg_state(struct amdgpu_device *adev,
>  			       enum amd_powergating_state state);
>  
> +static inline bool amdgpu_device_should_recover_gpu(struct amdgpu_device *adev)
> +{
> +	return adev->gpu_recovery != 0;
> +}
> +
>  static inline bool amdgpu_device_has_timeouts_enabled(struct amdgpu_device *adev)
>  {
> -	return amdgpu_gpu_recovery != 0 &&
> +	return amdgpu_device_should_recover_gpu(adev) &&
>  		adev->gfx_timeout != MAX_SCHEDULE_TIMEOUT &&
>  		adev->compute_timeout != MAX_SCHEDULE_TIMEOUT &&
>  		adev->sdma_timeout != MAX_SCHEDULE_TIMEOUT &&
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 730e9e93d078..a812551b353f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3762,6 +3762,45 @@ amdgpu_device_should_register_switcheroo(struct amdgpu_device *adev, bool px)
>  		       apple_gmux_detect(NULL, NULL)));
>  }
>  
> +static void amdgpu_device_init_gpu_recovery(struct amdgpu_device *adev)
> +{
> +	adev->gpu_recovery = 0;
> +
> +	if (amdgpu_gpu_recovery == 0)
> +		return;
> +
> +	/* Skip soft reset check in fatal error mode */
> +	if (!amdgpu_ras_is_poison_mode_supported(adev) ||
> +	    amdgpu_sriov_vf(adev)) {
> +		adev->gpu_recovery = adev->gpu_recovery > 0 ?
> +			adev->gpu_recovery :-1;
> +	} else if (amdgpu_gpu_recovery == -1) {
> +		switch (adev->asic_type) {
> +#ifdef CONFIG_DRM_AMDGPU_SI
> +		case CHIP_VERDE:
> +		case CHIP_TAHITI:
> +		case CHIP_PITCAIRN:
> +		case CHIP_OLAND:
> +		case CHIP_HAINAN:
> +#endif
> +#ifdef CONFIG_DRM_AMDGPU_CIK
> +		case CHIP_KAVERI:
> +		case CHIP_KABINI:
> +		case CHIP_MULLINS:
> +#endif
> +		case CHIP_CARRIZO:
> +		case CHIP_STONEY:
> +		case CHIP_CYAN_SKILLFISH:
> +			return;


Hui? We have recovery disabled by default for those devices?

> +		default:
> +			adev->gpu_recovery = 1;
> +			break;
> +		}
> +	} else {
> +		adev->gpu_recovery = amdgpu_gpu_recovery;
> +	}
> +}
> +
>  /**
>   * amdgpu_device_init - initialize the driver
>   *
> @@ -4109,6 +4148,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>  		}
>  	}
>  
> +	amdgpu_device_init_gpu_recovery(adev);
> +
>  fence_driver_init:
>  	/* Fence driver */
>  	r = amdgpu_fence_driver_sw_init(adev);
> @@ -4894,57 +4935,6 @@ bool amdgpu_device_has_job_running(struct amdgpu_device *adev)
>  	return false;
>  }
>  
> -/**
> - * amdgpu_device_should_recover_gpu - check if we should try GPU recovery
> - *
> - * @adev: amdgpu_device pointer
> - *
> - * Check amdgpu_gpu_recovery and SRIOV status to see if we should try to recover
> - * a hung GPU.
> - */
> -bool amdgpu_device_should_recover_gpu(struct amdgpu_device *adev)
> -{
> -
> -	if (amdgpu_gpu_recovery == 0)
> -		goto disabled;
> -
> -	/* Skip soft reset check in fatal error mode */
> -	if (!amdgpu_ras_is_poison_mode_supported(adev))
> -		return true;
> -
> -	if (amdgpu_sriov_vf(adev))
> -		return true;
> -
> -	if (amdgpu_gpu_recovery == -1) {
> -		switch (adev->asic_type) {
> -#ifdef CONFIG_DRM_AMDGPU_SI
> -		case CHIP_VERDE:
> -		case CHIP_TAHITI:
> -		case CHIP_PITCAIRN:
> -		case CHIP_OLAND:
> -		case CHIP_HAINAN:
> -#endif
> -#ifdef CONFIG_DRM_AMDGPU_CIK
> -		case CHIP_KAVERI:
> -		case CHIP_KABINI:
> -		case CHIP_MULLINS:
> -#endif
> -		case CHIP_CARRIZO:
> -		case CHIP_STONEY:
> -		case CHIP_CYAN_SKILLFISH:
> -			goto disabled;
> -		default:
> -			break;
> -		}
> -	}
> -
> -	return true;
> -
> -disabled:
> -		dev_info(adev->dev, "GPU recovery disabled.\n");
> -		return false;
> -}
> -
>  int amdgpu_device_mode1_reset(struct amdgpu_device *adev)
>  {
>  	u32 i;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index bff2d38f3571..a8e23f4e8113 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 = !!val;
> +
> +	return 0;
> +}
> +
>  DEFINE_SHOW_ATTRIBUTE(amdgpu_debugfs_fence_info);
> -DEFINE_DEBUGFS_ATTRIBUTE(amdgpu_debugfs_gpu_recover_fops, gpu_recover_get, NULL,
> +DEFINE_DEBUGFS_ATTRIBUTE(amdgpu_debugfs_gpu_recover_fops, gpu_recover_get, gpu_recover_set,
>  			 "%lld\n");
>  
>  static void amdgpu_debugfs_reset_work(struct work_struct *work)
> @@ -1042,7 +1055,7 @@ void amdgpu_debugfs_fence_init(struct amdgpu_device *adev)
>  	if (!amdgpu_sriov_vf(adev)) {
>  
>  		INIT_WORK(&adev->reset_work, amdgpu_debugfs_reset_work);
> -		debugfs_create_file("amdgpu_gpu_recover", 0444, root, adev,
> +		debugfs_create_file("amdgpu_gpu_recover", 0644, root, adev,
>  				    &amdgpu_debugfs_gpu_recover_fops);

You can also use debugfs_create_bool() here and drop the extra recover fops.

Apart from that looks good to me,
Christian.

>  	}
>  #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 0ae6e990c310..28f9048f806c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -2299,7 +2299,7 @@ static int amdgpu_gfx_sysfs_reset_mask_init(struct amdgpu_device *adev)
>  {
>  	int r = 0;
>  
> -	if (!amdgpu_gpu_recovery)
> +	if (!amdgpu_device_should_recover_gpu(adev))
>  		return r;
>  
>  	if (adev->gfx.num_gfx_rings) {
> @@ -2319,7 +2319,7 @@ static int amdgpu_gfx_sysfs_reset_mask_init(struct amdgpu_device *adev)
>  
>  static void amdgpu_gfx_sysfs_reset_mask_fini(struct amdgpu_device *adev)
>  {
> -	if (!amdgpu_gpu_recovery)
> +	if (!amdgpu_device_should_recover_gpu(adev))
>  		return;
>  
>  	if (adev->gfx.num_gfx_rings)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 4d363bcfb6a5..16391cc2b9a5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -111,7 +111,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct drm_sched_job *s_job)
>  	if (!amdgpu_sriov_vf(adev))
>  		amdgpu_job_core_dump(adev, job);
>  
> -	if (amdgpu_gpu_recovery &&
> +	if (amdgpu_device_should_recover_gpu(adev) &&
>  	    amdgpu_ring_is_reset_type_supported(ring, AMDGPU_RESET_TYPE_SOFT_RECOVERY) &&
>  	    amdgpu_ring_soft_recovery(ring, job->vmid, s_job->s_fence->parent)) {
>  		dev_err(adev->dev, "ring %s timeout, but soft recovered\n",
> @@ -130,7 +130,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct drm_sched_job *s_job)
>  	}
>  
>  	/* attempt a per ring reset */
> -	if (amdgpu_gpu_recovery &&
> +	if (amdgpu_device_should_recover_gpu(adev) &&
>  	    amdgpu_ring_is_reset_type_supported(ring, AMDGPU_RESET_TYPE_PER_QUEUE) &&
>  	    ring->funcs->reset) {
>  		dev_err(adev->dev, "Starting %s ring reset\n",
> @@ -152,7 +152,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct drm_sched_job *s_job)
>  	}
>  
>  	/* Attempt an IP block soft reset, if supported. */
> -	if (amdgpu_gpu_recovery &&
> +	if (amdgpu_device_should_recover_gpu(adev) &&
>  	    amdgpu_ring_is_reset_type_supported(ring, AMDGPU_RESET_TYPE_IP_BLOCK_SOFT_RESET)) {
>  		r = amdgpu_device_ip_soft_reset(ring, job->hw_fence);
>  		if (!r) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> index 87ecbd429f95..8da7c8e91e25 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> @@ -454,7 +454,7 @@ int amdgpu_sdma_sysfs_reset_mask_init(struct amdgpu_device *adev)
>  {
>  	int r = 0;
>  
> -	if (!amdgpu_gpu_recovery)
> +	if (!amdgpu_device_should_recover_gpu(adev))
>  		return r;
>  
>  	if (adev->sdma.num_instances) {
> @@ -468,7 +468,7 @@ int amdgpu_sdma_sysfs_reset_mask_init(struct amdgpu_device *adev)
>  
>  void amdgpu_sdma_sysfs_reset_mask_fini(struct amdgpu_device *adev)
>  {
> -	if (!amdgpu_gpu_recovery)
> +	if (!amdgpu_device_should_recover_gpu(adev))
>  		return;
>  
>  	if (adev->dev->kobj.sd) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 1a2aa85c504e..64fb04ff2098 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -101,7 +101,7 @@ static void amdgpu_userq_mgr_reset_work(struct work_struct *work)
>  	 * If GPU recovery feature is disabled system-wide,
>  	 * skip all reset detection logic
>  	 */
> -	if (!amdgpu_gpu_recovery)
> +	if (!amdgpu_device_should_recover_gpu(adev))
>  		return;
>  
>  	memset(&reset_context, 0, sizeof(reset_context));
> @@ -137,7 +137,7 @@ static void amdgpu_userq_hang_detect_work(struct work_struct *work)
>  	 * If GPU recovery feature is disabled system-wide,
>  	 * skip all reset detection logic
>  	 */
> -	if (!amdgpu_gpu_recovery)
> +	if (!amdgpu_device_should_recover_gpu(adev))
>  		return;
>  
>  	if (queue->vm && queue->vm->pasid) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c b/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c
> index c3293e5a658c..907f18b195ba 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c
> @@ -550,7 +550,7 @@ static int xgpu_vi_mailbox_rcv_irq(struct amdgpu_device *adev,
>  	int r;
>  
>  	/* trigger gpu-reset by hypervisor only if TDR disabled */
> -	if (!amdgpu_gpu_recovery) {
> +	if (!amdgpu_device_should_recover_gpu(adev)) {
>  		/* see what event we get */
>  		r = xgpu_vi_mailbox_rcv_msg(adev, IDH_FLR_NOTIFICATION);
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
> index b2024d05aa5f..b35622e65981 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
> @@ -570,7 +570,7 @@ soc15_asic_reset_method(struct amdgpu_device *adev)
>  		 */
>  		if (adev->pcie_reset_ctx.in_link_reset)
>  			return AMD_RESET_METHOD_LINK;
> -		if (amdgpu_gpu_recovery == 4 || amdgpu_gpu_recovery == 5)
> +		if (adev->gpu_recovery == 4 || adev->gpu_recovery == 5)
>  			return AMD_RESET_METHOD_MODE2;
>  		else if (!(adev->flags & AMD_IS_APU))
>  			return AMD_RESET_METHOD_MODE1;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> index ce67e56efa8c..18fa7c8cc210 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -336,7 +336,7 @@ static int remove_queue_mes_on_reset_option(struct device_queue_manager *dqm, st
>  
>  	/* remove_hw_queue failure indicates a queue hang. reset the queue */
>  	r = reset_queues_mes(dqm, q);
> -	if (r && amdgpu_gpu_recovery) {
> +	if (r && amdgpu_device_should_recover_gpu(adev)) {
>  		dev_err(adev->dev, "failed to remove queue from MES, doorbell=0x%x\n",
>  			q->properties.doorbell_off);
>  		dev_err(adev->dev, "MES might be in unrecoverable state, issue a GPU reset\n");
> @@ -494,7 +494,7 @@ static int reset_queues_mes(struct device_queue_manager *dqm, struct queue *q)
>  	if (WARN_ON(dqm->detect_hang_count > 0))
>  		return 0;
>  
> -	if (!amdgpu_gpu_recovery) {
> +	if (!amdgpu_device_should_recover_gpu(adev)) {
>  		r = -ENOTRECOVERABLE;
>  		goto fail;
>  	}
> @@ -2806,7 +2806,7 @@ static int reset_queues_on_hws_hang(struct device_queue_manager *dqm, bool is_sd
>  
>  		return -EPERM;
>  	}
> -	if (!amdgpu_gpu_recovery)
> +	if (!amdgpu_device_should_recover_gpu(adev))
>  		return -ENOTRECOVERABLE;
>  
>  	return is_sdma ? reset_hung_queues_sdma(dqm) : reset_hung_queues(dqm);