diff mbox series

[v5,19/27] drm/amdgpu: Finilise device fences on device remove.

Message ID 20210428151207.1212258-20-andrey.grodzovsky@amd.com (mailing list archive)
State New, archived
Headers show
Series RFC Support hot device unplug in amdgpu | expand

Commit Message

Andrey Grodzovsky April 28, 2021, 3:11 p.m. UTC
Make sure all fecens dependent on HW present are force signaled
when handling device removal. This helpes later to scope all HW
accesing code such as IOCTLs in drm_dev_enter/exit and use
drm_dev_unplug as synchronization point past which we know HW
will not be accessed anymore outside of pci remove driver callback.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  2 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 98 ++++++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  6 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 12 +--
 4 files changed, 103 insertions(+), 15 deletions(-)

Comments

Andrey Grodzovsky April 28, 2021, 3:20 p.m. UTC | #1
On 2021-04-28 11:11 a.m., Andrey Grodzovsky wrote:
> Make sure all fecens dependent on HW present are force signaled
> when handling device removal. This helpes later to scope all HW
> accesing code such as IOCTLs in drm_dev_enter/exit and use
> drm_dev_unplug as synchronization point past which we know HW
> will not be accessed anymore outside of pci remove driver callback.
> 
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  2 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 98 ++++++++++++++++++++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  6 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 12 +--
>   4 files changed, 103 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 0db0ba4fba89..df6c5ed676b1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1374,6 +1374,8 @@ void amdgpu_pci_resume(struct pci_dev *pdev);
>   bool amdgpu_device_cache_pci_state(struct pci_dev *pdev);
>   bool amdgpu_device_load_pci_state(struct pci_dev *pdev);
>   
> +void amdgpu_finilize_device_fences(struct drm_device *dev);
> +
>   #include "amdgpu_object.h"
>   
>   static inline bool amdgpu_is_tmz(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 33e8e9e1d1fe..55afc11c17e6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3692,15 +3692,12 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
>   		amdgpu_virt_fini_data_exchange(adev);
>   	}
>   
> -	/* disable all interrupts */
> -	amdgpu_irq_disable_all(adev);
>   	if (adev->mode_info.mode_config_initialized){
>   		if (!amdgpu_device_has_dc_support(adev))
>   			drm_helper_force_disable_all(adev_to_drm(adev));
>   		else
>   			drm_atomic_helper_shutdown(adev_to_drm(adev));
>   	}
> -	amdgpu_fence_driver_fini_hw(adev);
>   
>   	if (adev->pm_sysfs_en)
>   		amdgpu_pm_sysfs_fini(adev);
> @@ -4567,14 +4564,19 @@ static bool amdgpu_device_lock_adev(struct amdgpu_device *adev,
>   	return true;
>   }
>   
> -static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
> +static void amdgpu_device_unlock_adev_imp(struct amdgpu_device *adev, bool skip_in_gpu_reset)
>   {
>   	amdgpu_vf_error_trans_all(adev);
>   	adev->mp1_state = PP_MP1_STATE_NONE;
> -	atomic_set(&adev->in_gpu_reset, 0);
> +	!skip_in_gpu_reset ? atomic_set(&adev->in_gpu_reset, 0) : 0;
>   	up_write(&adev->reset_sem);
>   }
>   
> +static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
> +{
> +	amdgpu_device_unlock_adev_imp(adev, false);
> +}
> +
>   /*
>    * to lockup a list of amdgpu devices in a hive safely, if not a hive
>    * with multiple nodes, it will be similar as amdgpu_device_lock_adev.
> @@ -5321,3 +5323,89 @@ bool amdgpu_device_load_pci_state(struct pci_dev *pdev)
>   }
>   
>   
> +static void amdgpu_finilize_schedulded_fences(struct amdgpu_ctx_mgr *mgr)
> +{
> +	struct amdgpu_ctx *ctx;
> +	struct idr *idp;
> +	uint32_t id, i, j;
> +
> +	idp = &mgr->ctx_handles;
> +
> +	idr_for_each_entry(idp, ctx, id) {
> +		for (i = 0; i < AMDGPU_HW_IP_NUM; ++i) {
> +			for (j = 0; j < amdgpu_ctx_num_entities[i]; ++j) {
> +				struct drm_sched_entity *entity;
> +
> +				if (!ctx->entities[i][j])
> +					continue;
> +
> +				entity = &ctx->entities[i][j]->entity;
> +				drm_sched_entity_kill_jobs(entity);
> +			}
> +		}
> +	}
> +}
> +
> +/**
> + * amdgpu_finilize_device_fences() - Finilize all device fences
> + * @pdev: pointer to PCI device
> + *
> + * Will disable and finilise ISRs and will signal all fences
> + * that might hang if HW is gone
> + */
> +void amdgpu_finilize_device_fences(struct drm_device *dev)
> +{
> +	struct amdgpu_device *adev = drm_to_adev(dev);
> +	struct drm_file *file;
> +
> +	/*
> +	 *  Block TDRs from further execution by setting adev->in_gpu_reset
> +	 *  instead of holding full reset lock in order to not deadlock
> +	 *  further ahead against any thread locking the reset lock when we
> +	 *  wait for it's completion
> +	 */
> +	while (!amdgpu_device_lock_adev(adev, NULL))
> +		amdgpu_cancel_all_tdr(adev);
> +	amdgpu_device_unlock_adev_imp(adev, true);
> +
> +
> +	/* disable all HW interrupts */
> +	amdgpu_irq_disable_all(adev);
> +
> +	/* stop and flush all in flight HW interrupts handlers */
> +	disable_irq(pci_irq_vector(adev->pdev, 0));
> +
> +	/*
> +	 * Stop SW GPU schedulers and force completion on all HW fences. Since
> +	 * in the prev. step all ISRs were disabled and completed the
> +	 * HW fence array is idle (no insertions or extractions) and so it's
> +	 * safe to iterate it bellow.
> +	 * After this step all HW fences in the system are signaled. As a result
> +	 * also all the scheduler 'finished' fences are also signaled.
> +	 */
> +	amdgpu_fence_driver_fini_hw(adev);
> +
> +	/*
> +	 * Reject any further jobs to any scheduler entity queue. After this
> +	 * step no new insertions and because schedulers are stopped also no
> +	 * new extractions.
> +	 */
> +	down_read(&adev->sched_fence_completion_sem);
> +	adev->stop_job_submissions = true;
> +	up_read(&adev->sched_fence_completion_sem);

FYI: Typo here - down_write and up_write obviously.

Andrey

> +
> +	/*
> +	 * Complete all scheduler 'scheduled' fences currently pending.
> +	 * It's OK if new contexts and sched entities are concurrently
> +	 * still created as they will fail in pushing jobs to SW queues
> +	 * and their schedule fences will be signaled with error
> +	 */
> +	mutex_lock(&adev->ddev.filelist_mutex);
> +	list_for_each_entry(file, &adev->ddev.filelist, lhead) {
> +		struct amdgpu_fpriv *fpriv = file->driver_priv;
> +		amdgpu_finilize_schedulded_fences(&fpriv->ctx_mgr);
> +	}
> +	mutex_unlock(&adev->ddev.filelist_mutex);
> +}
> +
> +
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index f799c40d7e72..8a19b8dd02ee 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -1249,6 +1249,12 @@ amdgpu_pci_remove(struct pci_dev *pdev)
>   {
>   	struct drm_device *dev = pci_get_drvdata(pdev);
>   
> +	/*
> +	 * Force completion of all device related fences that might hang us when
> +	 * synchronizing SRCU in the following step.
> +	 */
> +	amdgpu_finilize_device_fences(dev);
> +
>   	drm_dev_unplug(dev);
>   	amdgpu_driver_unload_kms(dev);
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index 2670201e78d3..af592b28cd35 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> @@ -526,7 +526,7 @@ int amdgpu_fence_driver_init(struct amdgpu_device *adev)
>    */
>   void amdgpu_fence_driver_fini_hw(struct amdgpu_device *adev)
>   {
> -	int i, r;
> +	int i;
>   
>   	for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
>   		struct amdgpu_ring *ring = adev->rings[i];
> @@ -535,18 +535,10 @@ void amdgpu_fence_driver_fini_hw(struct amdgpu_device *adev)
>   			continue;
>   
>   		/* Stop any new job submissions from sched before flushing the ring */
> -		/* TODO Handle amdgpu_job_submit_direct and amdgpu_amdkfd_submit_ib */
>   		if (!ring->no_scheduler)
>   			drm_sched_fini(&ring->sched);
>   
> -		/* You can't wait for HW to signal if it's gone */
> -		if (!drm_dev_is_unplugged(&adev->ddev))
> -			r = amdgpu_fence_wait_empty(ring);
> -		else
> -			r = -ENODEV;
> -		/* no need to trigger GPU reset as we are unloading */
> -		if (r)
> -			amdgpu_fence_driver_force_completion(ring);
> +		amdgpu_fence_driver_force_completion(ring);
>   
>   		if (ring->fence_drv.irq_src)
>   			amdgpu_irq_put(adev, ring->fence_drv.irq_src,
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 0db0ba4fba89..df6c5ed676b1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1374,6 +1374,8 @@  void amdgpu_pci_resume(struct pci_dev *pdev);
 bool amdgpu_device_cache_pci_state(struct pci_dev *pdev);
 bool amdgpu_device_load_pci_state(struct pci_dev *pdev);
 
+void amdgpu_finilize_device_fences(struct drm_device *dev);
+
 #include "amdgpu_object.h"
 
 static inline bool amdgpu_is_tmz(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 33e8e9e1d1fe..55afc11c17e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3692,15 +3692,12 @@  void amdgpu_device_fini_hw(struct amdgpu_device *adev)
 		amdgpu_virt_fini_data_exchange(adev);
 	}
 
-	/* disable all interrupts */
-	amdgpu_irq_disable_all(adev);
 	if (adev->mode_info.mode_config_initialized){
 		if (!amdgpu_device_has_dc_support(adev))
 			drm_helper_force_disable_all(adev_to_drm(adev));
 		else
 			drm_atomic_helper_shutdown(adev_to_drm(adev));
 	}
-	amdgpu_fence_driver_fini_hw(adev);
 
 	if (adev->pm_sysfs_en)
 		amdgpu_pm_sysfs_fini(adev);
@@ -4567,14 +4564,19 @@  static bool amdgpu_device_lock_adev(struct amdgpu_device *adev,
 	return true;
 }
 
-static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
+static void amdgpu_device_unlock_adev_imp(struct amdgpu_device *adev, bool skip_in_gpu_reset)
 {
 	amdgpu_vf_error_trans_all(adev);
 	adev->mp1_state = PP_MP1_STATE_NONE;
-	atomic_set(&adev->in_gpu_reset, 0);
+	!skip_in_gpu_reset ? atomic_set(&adev->in_gpu_reset, 0) : 0;
 	up_write(&adev->reset_sem);
 }
 
+static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
+{
+	amdgpu_device_unlock_adev_imp(adev, false);
+}
+
 /*
  * to lockup a list of amdgpu devices in a hive safely, if not a hive
  * with multiple nodes, it will be similar as amdgpu_device_lock_adev.
@@ -5321,3 +5323,89 @@  bool amdgpu_device_load_pci_state(struct pci_dev *pdev)
 }
 
 
+static void amdgpu_finilize_schedulded_fences(struct amdgpu_ctx_mgr *mgr)
+{
+	struct amdgpu_ctx *ctx;
+	struct idr *idp;
+	uint32_t id, i, j;
+
+	idp = &mgr->ctx_handles;
+
+	idr_for_each_entry(idp, ctx, id) {
+		for (i = 0; i < AMDGPU_HW_IP_NUM; ++i) {
+			for (j = 0; j < amdgpu_ctx_num_entities[i]; ++j) {
+				struct drm_sched_entity *entity;
+
+				if (!ctx->entities[i][j])
+					continue;
+
+				entity = &ctx->entities[i][j]->entity;
+				drm_sched_entity_kill_jobs(entity);
+			}
+		}
+	}
+}
+
+/**
+ * amdgpu_finilize_device_fences() - Finilize all device fences
+ * @pdev: pointer to PCI device
+ *
+ * Will disable and finilise ISRs and will signal all fences
+ * that might hang if HW is gone
+ */
+void amdgpu_finilize_device_fences(struct drm_device *dev)
+{
+	struct amdgpu_device *adev = drm_to_adev(dev);
+	struct drm_file *file;
+
+	/*
+	 *  Block TDRs from further execution by setting adev->in_gpu_reset
+	 *  instead of holding full reset lock in order to not deadlock
+	 *  further ahead against any thread locking the reset lock when we
+	 *  wait for it's completion
+	 */
+	while (!amdgpu_device_lock_adev(adev, NULL))
+		amdgpu_cancel_all_tdr(adev);
+	amdgpu_device_unlock_adev_imp(adev, true);
+
+
+	/* disable all HW interrupts */
+	amdgpu_irq_disable_all(adev);
+
+	/* stop and flush all in flight HW interrupts handlers */
+	disable_irq(pci_irq_vector(adev->pdev, 0));
+
+	/*
+	 * Stop SW GPU schedulers and force completion on all HW fences. Since
+	 * in the prev. step all ISRs were disabled and completed the
+	 * HW fence array is idle (no insertions or extractions) and so it's
+	 * safe to iterate it bellow.
+	 * After this step all HW fences in the system are signaled. As a result
+	 * also all the scheduler 'finished' fences are also signaled.
+	 */
+	amdgpu_fence_driver_fini_hw(adev);
+
+	/*
+	 * Reject any further jobs to any scheduler entity queue. After this
+	 * step no new insertions and because schedulers are stopped also no
+	 * new extractions.
+	 */
+	down_read(&adev->sched_fence_completion_sem);
+	adev->stop_job_submissions = true;
+	up_read(&adev->sched_fence_completion_sem);
+
+	/*
+	 * Complete all scheduler 'scheduled' fences currently pending.
+	 * It's OK if new contexts and sched entities are concurrently
+	 * still created as they will fail in pushing jobs to SW queues
+	 * and their schedule fences will be signaled with error
+	 */
+	mutex_lock(&adev->ddev.filelist_mutex);
+	list_for_each_entry(file, &adev->ddev.filelist, lhead) {
+		struct amdgpu_fpriv *fpriv = file->driver_priv;
+		amdgpu_finilize_schedulded_fences(&fpriv->ctx_mgr);
+	}
+	mutex_unlock(&adev->ddev.filelist_mutex);
+}
+
+
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index f799c40d7e72..8a19b8dd02ee 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1249,6 +1249,12 @@  amdgpu_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
+	/*
+	 * Force completion of all device related fences that might hang us when
+	 * synchronizing SRCU in the following step.
+	 */
+	amdgpu_finilize_device_fences(dev);
+
 	drm_dev_unplug(dev);
 	amdgpu_driver_unload_kms(dev);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 2670201e78d3..af592b28cd35 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -526,7 +526,7 @@  int amdgpu_fence_driver_init(struct amdgpu_device *adev)
  */
 void amdgpu_fence_driver_fini_hw(struct amdgpu_device *adev)
 {
-	int i, r;
+	int i;
 
 	for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
 		struct amdgpu_ring *ring = adev->rings[i];
@@ -535,18 +535,10 @@  void amdgpu_fence_driver_fini_hw(struct amdgpu_device *adev)
 			continue;
 
 		/* Stop any new job submissions from sched before flushing the ring */
-		/* TODO Handle amdgpu_job_submit_direct and amdgpu_amdkfd_submit_ib */
 		if (!ring->no_scheduler)
 			drm_sched_fini(&ring->sched);
 
-		/* You can't wait for HW to signal if it's gone */
-		if (!drm_dev_is_unplugged(&adev->ddev))
-			r = amdgpu_fence_wait_empty(ring);
-		else
-			r = -ENODEV;
-		/* no need to trigger GPU reset as we are unloading */
-		if (r)
-			amdgpu_fence_driver_force_completion(ring);
+		amdgpu_fence_driver_force_completion(ring);
 
 		if (ring->fence_drv.irq_src)
 			amdgpu_irq_put(adev, ring->fence_drv.irq_src,