[PATCH Review 1/1] drm/amdgpu: Support setting recover method

[Christian König]

Am 11.04.24 um 13:49 schrieb Christian König:
> Am 11.04.24 um 13:30 schrieb Yang, Stanley:
>> [AMD Official Use Only - General]
>>
>>> -----Original Message-----
>>> From: Christian König <ckoenig.leichtzumerken at gmail.com>
>>> Sent: Thursday, April 11, 2024 7:17 PM
>>> To: Yang, Stanley <Stanley.Yang at amd.com>; amd-gfx at lists.freedesktop.org
>>> Subject: Re: [PATCH Review 1/1] drm/amdgpu: Support setting recover 
>>> method
>>>
>>> Am 11.04.24 um 13:11 schrieb Stanley.Yang:
>>>> Don't modify amdgpu gpu recover get operation, add amdgpu gpu recover
>>>> set operation to select reset method, only support mode1 and mode2
>>>> currently.
>>> Well I don't think setting this from userspace is valid.
>>>
>>> The reset method to use is determined by the hardware and 
>>> environment (e.g.
>>> SRIOV, passthrough, whatever) and can't be chosen simply.
>> [Stanley]: Agree, the setting is invalid for some devices not 
>> supported setting method and devices still reset with default method,
>> but it's valid for those devices supported setting reset method, user 
>> can conduct combination testing like mode1 test then mode2 test without
>> re-modprobe driver.
>
> Well and the user could also shoot himself into the foot.
>
> I really don't think that this is a valuable functionality.

To make clear what I mean: What you could do is to make the module 
parameter writeable.

In this case the hardware code still decides which reset method to use 
based on the module parameter in the moment the reset is requested.

That would also avoid re-loading the driver.

Regards,
Christian.

>
> Regards,
> Christian.
>
>>
>> Regards,
>> Stanley
>>> Regards,
>>> Christian.
>>>
>>>> Signed-off-by: Stanley.Yang <Stanley.Yang at amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  3 ++
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  1 +
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 37
>>> +++++++++++++++++++---
>>>>    3 files changed, 37 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> index 9c62552bec34..c82976b2b977 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> @@ -1151,6 +1151,9 @@ struct amdgpu_device {
>>>>      bool                            debug_largebar;
>>>>      bool debug_disable_soft_recovery;
>>>>      bool                            debug_use_vram_fw_buf;
>>>> +
>>>> +   /* Used to set gpu reset method */
>>>> +   int                             recover_method;
>>>>    };
>>>>
>>>>    static inline uint32_t amdgpu_ip_version(const struct amdgpu_device
>>>> *adev, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> index 3204b8f6edeb..8411a793be18 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> @@ -3908,6 +3908,7 @@ int amdgpu_device_init(struct amdgpu_device
>>> *adev,
>>>>      else
>>>>              adev->asic_type = flags & AMD_ASIC_MASK;
>>>>
>>>> +   adev->recover_method = AMD_RESET_METHOD_NONE;
>>>>      adev->usec_timeout = AMDGPU_MAX_USEC_TIMEOUT;
>>>>      if (amdgpu_emu_mode == 1)
>>>>              adev->usec_timeout *= 10;
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>> index 10832b470448..e388a50d11d9 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>> @@ -965,9 +965,37 @@ 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;
>>>> +   struct drm_device *dev = adev_to_drm(adev);
>>>> +   int r;
>>>> +
>>>> +   /* TODO: support mode1 and mode2 currently */
>>>> +   if (val == AMD_RESET_METHOD_MODE1 ||
>>>> +           val == AMD_RESET_METHOD_MODE2)
>>>> +           adev->recover_method = val;
>>>> +   else
>>>> +           adev->recover_method = AMD_RESET_METHOD_NONE;
>>>> +
>>>> +   r = pm_runtime_get_sync(dev->dev);
>>>> +   if (r < 0) {
>>>> +           pm_runtime_put_autosuspend(dev->dev);
>>>> +           return 0;
>>>> +   }
>>>> +
>>>> +   if (amdgpu_reset_domain_schedule(adev->reset_domain, &adev-
>>>> reset_work))
>>>> +           flush_work(&adev->reset_work);
>>>> +
>>>> +   pm_runtime_mark_last_busy(dev->dev);
>>>> +   pm_runtime_put_autosuspend(dev->dev);
>>>> +
>>>> +   return 0;
>>>> +}
>>>> +
>>>>    DEFINE_SHOW_ATTRIBUTE(amdgpu_debugfs_fence_info);
>>>> -DEFINE_DEBUGFS_ATTRIBUTE(amdgpu_debugfs_gpu_recover_fops,
>>> gpu_recover_get, NULL,
>>>> -                    "%lld\n");
>>>> +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)
>>>>    {
>>>> @@ -978,9 +1006,10 @@ static void amdgpu_debugfs_reset_work(struct
>>>> work_struct *work)
>>>>
>>>>      memset(&reset_context, 0, sizeof(reset_context));
>>>>
>>>> -   reset_context.method = AMD_RESET_METHOD_NONE;
>>>> +   reset_context.method = adev->recover_method;
>>>>      reset_context.reset_req_dev = adev;
>>>>      set_bit(AMDGPU_NEED_FULL_RESET, &reset_context.flags);
>>>> +   adev->recover_method = AMD_RESET_METHOD_NONE;
>>>>
>>>>      amdgpu_device_gpu_recover(adev, NULL, &reset_context);
>>>>    }
>>>> @@ -999,7 +1028,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", 0666, root, 
>>>> adev,
>>>> &amdgpu_debugfs_gpu_recover_fops);
>>>>      }
>>>>    #endif
>