Re: [PATCH 2/6] drm/amdgpu: cleanup GPU recovery check a bit
Am 23.08.2018 um 04:54 schrieb Huang Rui: On Wed, Aug 22, 2018 at 12:04:53PM +0200, Christian König wrote: Check if we should call the function instead of providing the forced flag. Signed-off-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu.h| 3 ++- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 38 -- drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c| 4 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_job.c| 3 ++- drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c | 3 ++- 7 files changed, 36 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 19ef7711d944..340e40d03d54 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -1158,8 +1158,9 @@ int emu_soc_asic_init(struct amdgpu_device *adev); #define amdgpu_asic_need_full_reset(adev) (adev)->asic_funcs->need_full_reset((adev)) /* Common functions */ +bool amdgpu_device_should_recover_gpu(struct amdgpu_device *adev); int amdgpu_device_gpu_recover(struct amdgpu_device *adev, - struct amdgpu_job* job, bool force); + struct amdgpu_job* job); void amdgpu_device_pci_config_reset(struct amdgpu_device *adev); bool amdgpu_device_need_post(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 c23339d8ae2d..9f5e4be76d5e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -3244,32 +3244,44 @@ static int amdgpu_device_reset_sriov(struct amdgpu_device *adev, return r; } +/** + * 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_device_ip_check_soft_reset(adev)) { + DRM_INFO("Timeout, but no hardware hang detected.\n"); + return false; + } + + if (amdgpu_gpu_recovery == 0 || (amdgpu_gpu_recovery == -1 && +!amdgpu_sriov_vf(adev))) { + DRM_INFO("GPU recovery disabled.\n"); + return false; + } + + return true; +} + /** * amdgpu_device_gpu_recover - reset the asic and recover scheduler * * @adev: amdgpu device pointer * @job: which job trigger hang - * @force: forces reset regardless of amdgpu_gpu_recovery * * Attempt to reset the GPU if it has hung (all asics). * Returns 0 for success or an error on failure. */ int amdgpu_device_gpu_recover(struct amdgpu_device *adev, - struct amdgpu_job *job, bool force) + struct amdgpu_job *job) { In my view, actually, we don't need return as "int" for this function. Because, no calling is to check the return value. Yeah, that is also something I noticed. But for now this cleanup is only about the force flag and the check if the function should be called or not. Others looks good for me. Reviewed-by: Huang Rui Thanks, Christian. int i, r, resched; - if (!force && !amdgpu_device_ip_check_soft_reset(adev)) { - DRM_INFO("No hardware hang detected. Did some blocks stall?\n"); - return 0; - } - - if (!force && (amdgpu_gpu_recovery == 0 || - (amdgpu_gpu_recovery == -1 && !amdgpu_sriov_vf(adev { - DRM_INFO("GPU recovery disabled.\n"); - return 0; - } - dev_info(adev->dev, "GPU reset begin!\n"); mutex_lock(>lock_reset); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index e74d620d9699..68cccebb8463 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -702,7 +702,7 @@ static int amdgpu_debugfs_gpu_recover(struct seq_file *m, void *data) struct amdgpu_device *adev = dev->dev_private; seq_printf(m, "gpu recover\n"); - amdgpu_device_gpu_recover(adev, NULL, true); + amdgpu_device_gpu_recover(adev, NULL); return 0; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c index 1abf5b5bac9e..b927e8798534 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c @@ -105,8 +105,8 @@ static void amdgpu_irq_reset_work_func(struct work_struct *work) struct amdgpu_device *adev = container_of(work, struct amdgpu_device, reset_work); - if (!amdgpu_sriov_vf(adev)) -
Re: [PATCH 2/6] drm/amdgpu: cleanup GPU recovery check a bit
On Wed, Aug 22, 2018 at 12:04:53PM +0200, Christian König wrote: > Check if we should call the function instead of providing the forced > flag. > > Signed-off-by: Christian König > --- > drivers/gpu/drm/amd/amdgpu/amdgpu.h| 3 ++- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 38 > -- > drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c| 4 ++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_job.c| 3 ++- > drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 4 ++-- > drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c | 3 ++- > 7 files changed, 36 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > index 19ef7711d944..340e40d03d54 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > @@ -1158,8 +1158,9 @@ int emu_soc_asic_init(struct amdgpu_device *adev); > #define amdgpu_asic_need_full_reset(adev) > (adev)->asic_funcs->need_full_reset((adev)) > > /* Common functions */ > +bool amdgpu_device_should_recover_gpu(struct amdgpu_device *adev); > int amdgpu_device_gpu_recover(struct amdgpu_device *adev, > - struct amdgpu_job* job, bool force); > + struct amdgpu_job* job); > void amdgpu_device_pci_config_reset(struct amdgpu_device *adev); > bool amdgpu_device_need_post(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 c23339d8ae2d..9f5e4be76d5e 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -3244,32 +3244,44 @@ static int amdgpu_device_reset_sriov(struct > amdgpu_device *adev, > return r; > } > > +/** > + * 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_device_ip_check_soft_reset(adev)) { > + DRM_INFO("Timeout, but no hardware hang detected.\n"); > + return false; > + } > + > + if (amdgpu_gpu_recovery == 0 || (amdgpu_gpu_recovery == -1 && > + !amdgpu_sriov_vf(adev))) { > + DRM_INFO("GPU recovery disabled.\n"); > + return false; > + } > + > + return true; > +} > + > /** > * amdgpu_device_gpu_recover - reset the asic and recover scheduler > * > * @adev: amdgpu device pointer > * @job: which job trigger hang > - * @force: forces reset regardless of amdgpu_gpu_recovery > * > * Attempt to reset the GPU if it has hung (all asics). > * Returns 0 for success or an error on failure. > */ > int amdgpu_device_gpu_recover(struct amdgpu_device *adev, > - struct amdgpu_job *job, bool force) > + struct amdgpu_job *job) > { In my view, actually, we don't need return as "int" for this function. Because, no calling is to check the return value. Others looks good for me. Reviewed-by: Huang Rui > int i, r, resched; > > - if (!force && !amdgpu_device_ip_check_soft_reset(adev)) { > - DRM_INFO("No hardware hang detected. Did some blocks stall?\n"); > - return 0; > - } > - > - if (!force && (amdgpu_gpu_recovery == 0 || > - (amdgpu_gpu_recovery == -1 && > !amdgpu_sriov_vf(adev { > - DRM_INFO("GPU recovery disabled.\n"); > - return 0; > - } > - > dev_info(adev->dev, "GPU reset begin!\n"); > > mutex_lock(>lock_reset); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > index e74d620d9699..68cccebb8463 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > @@ -702,7 +702,7 @@ static int amdgpu_debugfs_gpu_recover(struct seq_file *m, > void *data) > struct amdgpu_device *adev = dev->dev_private; > > seq_printf(m, "gpu recover\n"); > - amdgpu_device_gpu_recover(adev, NULL, true); > + amdgpu_device_gpu_recover(adev, NULL); > > return 0; > } > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c > index 1abf5b5bac9e..b927e8798534 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c > @@ -105,8 +105,8 @@ static void amdgpu_irq_reset_work_func(struct work_struct > *work) > struct amdgpu_device *adev = container_of(work, struct amdgpu_device, > reset_work); > > - if (!amdgpu_sriov_vf(adev)) > - amdgpu_device_gpu_recover(adev, NULL, false); > + if
[PATCH 2/6] drm/amdgpu: cleanup GPU recovery check a bit
Check if we should call the function instead of providing the forced flag. Signed-off-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu.h| 3 ++- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 38 -- drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c| 4 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_job.c| 3 ++- drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c | 3 ++- 7 files changed, 36 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 19ef7711d944..340e40d03d54 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -1158,8 +1158,9 @@ int emu_soc_asic_init(struct amdgpu_device *adev); #define amdgpu_asic_need_full_reset(adev) (adev)->asic_funcs->need_full_reset((adev)) /* Common functions */ +bool amdgpu_device_should_recover_gpu(struct amdgpu_device *adev); int amdgpu_device_gpu_recover(struct amdgpu_device *adev, - struct amdgpu_job* job, bool force); + struct amdgpu_job* job); void amdgpu_device_pci_config_reset(struct amdgpu_device *adev); bool amdgpu_device_need_post(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 c23339d8ae2d..9f5e4be76d5e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -3244,32 +3244,44 @@ static int amdgpu_device_reset_sriov(struct amdgpu_device *adev, return r; } +/** + * 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_device_ip_check_soft_reset(adev)) { + DRM_INFO("Timeout, but no hardware hang detected.\n"); + return false; + } + + if (amdgpu_gpu_recovery == 0 || (amdgpu_gpu_recovery == -1 && +!amdgpu_sriov_vf(adev))) { + DRM_INFO("GPU recovery disabled.\n"); + return false; + } + + return true; +} + /** * amdgpu_device_gpu_recover - reset the asic and recover scheduler * * @adev: amdgpu device pointer * @job: which job trigger hang - * @force: forces reset regardless of amdgpu_gpu_recovery * * Attempt to reset the GPU if it has hung (all asics). * Returns 0 for success or an error on failure. */ int amdgpu_device_gpu_recover(struct amdgpu_device *adev, - struct amdgpu_job *job, bool force) + struct amdgpu_job *job) { int i, r, resched; - if (!force && !amdgpu_device_ip_check_soft_reset(adev)) { - DRM_INFO("No hardware hang detected. Did some blocks stall?\n"); - return 0; - } - - if (!force && (amdgpu_gpu_recovery == 0 || - (amdgpu_gpu_recovery == -1 && !amdgpu_sriov_vf(adev { - DRM_INFO("GPU recovery disabled.\n"); - return 0; - } - dev_info(adev->dev, "GPU reset begin!\n"); mutex_lock(>lock_reset); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index e74d620d9699..68cccebb8463 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -702,7 +702,7 @@ static int amdgpu_debugfs_gpu_recover(struct seq_file *m, void *data) struct amdgpu_device *adev = dev->dev_private; seq_printf(m, "gpu recover\n"); - amdgpu_device_gpu_recover(adev, NULL, true); + amdgpu_device_gpu_recover(adev, NULL); return 0; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c index 1abf5b5bac9e..b927e8798534 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c @@ -105,8 +105,8 @@ static void amdgpu_irq_reset_work_func(struct work_struct *work) struct amdgpu_device *adev = container_of(work, struct amdgpu_device, reset_work); - if (!amdgpu_sriov_vf(adev)) - amdgpu_device_gpu_recover(adev, NULL, false); + if (!amdgpu_sriov_vf(adev) && amdgpu_device_should_recover_gpu(adev)) + amdgpu_device_gpu_recover(adev, NULL); } /** diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c index 391e2f7c03aa..265ff90f4e01 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c @@ -37,7 +37,8 @@ static void amdgpu_job_timedout(struct drm_sched_job *s_job)