[PATCH v5] drm/amdgpu: handle IH ring1 overflow

[PATCH v5] drm/amdgpu: handle IH ring1 overflow

[Philip Yang]

IH ring1 is used to process GPU retry fault, overflow is enabled to
drain retry fault because we want receive other interrupts while
handling retry fault to recover range. There is no overflow flag set
when wptr pass rptr. Use timestamp of rptr and wptr to handle overflow
and drain retry fault.

Add helper function amdgpu_ih_decode_iv_ts to get 48bit timestamp from
IV entry. drain retry fault is done if processed_timestamp is
equal to or larger than checkpoint timestamp.

Add function amdgpu_ih_process1 to process IH ring1 until timestamp of
rptr is larger then timestamp of next entry.

Helper amdgpu_ih_ts_after to compare time stamps with 48bit wrap around.

Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c  | 107 ++++++++++++++++++------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h  |  13 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c |   2 +-
 drivers/gpu/drm/amd/amdgpu/navi10_ih.c  |   1 +
 drivers/gpu/drm/amd/amdgpu/vega10_ih.c  |   1 +
 drivers/gpu/drm/amd/amdgpu/vega20_ih.c  |   1 +
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c    |   2 +-
 7 files changed, 99 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
index 0c7963dfacad..30b4e0e01444 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
@@ -164,52 +164,45 @@ void amdgpu_ih_ring_write(struct amdgpu_ih_ring *ih, const uint32_t *iv,
 	}
 }
 
+/* return true if time stamp t2 is after t1 with 48bit wrap around */
+static inline bool amdgpu_ih_ts_after(uint64_t t1, uint64_t t2)
+{
+	return ((int64_t)(t2 << 16) - (int64_t)(t1 << 16)) > 0LL;
+}
+
 /* Waiter helper that checks current rptr matches or passes checkpoint wptr */
-static bool amdgpu_ih_has_checkpoint_processed(struct amdgpu_device *adev,
+static bool amdgpu_ih_has_checkpoint_processed_ts(struct amdgpu_device *adev,
 					struct amdgpu_ih_ring *ih,
-					uint32_t checkpoint_wptr,
-					uint32_t *prev_rptr)
+					uint64_t checkpoint_ts)
 {
-	uint32_t cur_rptr = ih->rptr | (*prev_rptr & ~ih->ptr_mask);
-
-	/* rptr has wrapped. */
-	if (cur_rptr < *prev_rptr)
-		cur_rptr += ih->ptr_mask + 1;
-	*prev_rptr = cur_rptr;
-
-	/* check ring is empty to workaround missing wptr overflow flag */
-	return cur_rptr >= checkpoint_wptr ||
-	       (cur_rptr & ih->ptr_mask) == amdgpu_ih_get_wptr(adev, ih);
+	return !amdgpu_ih_ts_after(ih->processed_timestamp, checkpoint_ts);
 }
 
 /**
- * amdgpu_ih_wait_on_checkpoint_process - wait to process IVs up to checkpoint
+ * amdgpu_ih_wait_on_checkpoint_process_ts - wait to process IVs up to checkpoint
  *
  * @adev: amdgpu_device pointer
  * @ih: ih ring to process
  *
  * Used to ensure ring has processed IVs up to the checkpoint write pointer.
  */
-int amdgpu_ih_wait_on_checkpoint_process(struct amdgpu_device *adev,
+int amdgpu_ih_wait_on_checkpoint_process_ts(struct amdgpu_device *adev,
 					struct amdgpu_ih_ring *ih)
 {
-	uint32_t checkpoint_wptr, rptr;
+	uint32_t checkpoint_wptr;
+	uint64_t checkpoint_ts;
 
 	if (!ih->enabled || adev->shutdown)
 		return -ENODEV;
 
 	checkpoint_wptr = amdgpu_ih_get_wptr(adev, ih);
-	/* Order wptr with rptr. */
+	/* Order wptr with ring data. */
 	rmb();
-	rptr = READ_ONCE(ih->rptr);
-
-	/* wptr has wrapped. */
-	if (rptr > checkpoint_wptr)
-		checkpoint_wptr += ih->ptr_mask + 1;
+	checkpoint_ts = amdgpu_ih_decode_iv_ts(adev, ih, checkpoint_wptr, -1);
 
 	return wait_event_interruptible(ih->wait_process,
-				amdgpu_ih_has_checkpoint_processed(adev, ih,
-						checkpoint_wptr, &rptr));
+				amdgpu_ih_has_checkpoint_processed_ts(adev, ih,
+						checkpoint_ts));
 }
 
 /**
@@ -254,6 +247,56 @@ int amdgpu_ih_process(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih)
 	return IRQ_HANDLED;
 }
 
+/**
+ * amdgpu_ih_process1 - interrupt handler work for IH ring1
+ *
+ * @adev: amdgpu_device pointer
+ * @ih: ih ring to process
+ *
+ * Interrupt handler of IH ring1, walk the IH ring1.
+ * Returns irq process return code.
+ */
+int amdgpu_ih_process1(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih)
+{
+	uint64_t ts, ts_next;
+	unsigned int count;
+	u32 wptr;
+
+	if (!ih->enabled || adev->shutdown)
+		return IRQ_NONE;
+
+	wptr = amdgpu_ih_get_wptr(adev, ih);
+	if (ih->rptr == wptr)
+		return 0;
+
+restart_ih:
+	count = AMDGPU_IH_MAX_NUM_IVS;
+
+	ts = amdgpu_ih_decode_iv_ts(adev, ih, ih->rptr, 0);
+	ts_next = amdgpu_ih_decode_iv_ts(adev, ih, ih->rptr, 1);
+	while (amdgpu_ih_ts_after(ts, ts_next) && --count) {
+		amdgpu_irq_dispatch(adev, ih);
+		ih->rptr &= ih->ptr_mask;
+		ts = ts_next;
+		ts_next = amdgpu_ih_decode_iv_ts(adev, ih, ih->rptr, 1);
+	}
+	/*
+	 * Process the last timestamp updated entry or one more entry
+	 * if count = 0, ts is timestamp of the entry.
+	 */
+	amdgpu_irq_dispatch(adev, ih);
+	amdgpu_ih_set_rptr(adev, ih);
+	wake_up_all(&ih->wait_process);
+
+	wptr = amdgpu_ih_get_wptr(adev, ih);
+	/* Order reading of wptr vs. reading of IH ring data */
+	rmb();
+	if (amdgpu_ih_ts_after(ts, amdgpu_ih_decode_iv_ts(adev, ih, wptr, -1)))
+		goto restart_ih;
+
+	return IRQ_HANDLED;
+}
+
 /**
  * amdgpu_ih_decode_iv_helper - decode an interrupt vector
  *
@@ -298,4 +341,20 @@ void amdgpu_ih_decode_iv_helper(struct amdgpu_device *adev,
 
 	/* wptr/rptr are in bytes! */
 	ih->rptr += 32;
+	ih->processed_timestamp = entry->timestamp;
+}
+
+uint64_t amdgpu_ih_decode_iv_ts_helper(struct amdgpu_ih_ring *ih, u32 rptr,
+				       signed offset)
+{
+	uint32_t iv_size = 32;
+	uint32_t dw1, dw2;
+	uint32_t index;
+
+	rptr += iv_size * offset;
+	index = (rptr & ih->ptr_mask) >> 2;
+
+	dw1 = le32_to_cpu(ih->ring[index + 1]);
+	dw2 = le32_to_cpu(ih->ring[index + 2]);
+	return dw1 | ((u64)(dw2 & 0xffff) << 32);
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
index 0649b59830a5..d7e1ffeca38f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
@@ -68,6 +68,7 @@ struct amdgpu_ih_ring {
 
 	/* For waiting on IH processing at checkpoint. */
 	wait_queue_head_t wait_process;
+	uint64_t		processed_timestamp;
 };
 
 /* provided by the ih block */
@@ -76,12 +77,17 @@ struct amdgpu_ih_funcs {
 	u32 (*get_wptr)(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih);
 	void (*decode_iv)(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih,
 			  struct amdgpu_iv_entry *entry);
+	uint64_t (*decode_iv_ts)(struct amdgpu_ih_ring *ih, u32 rptr,
+				 signed offset);
 	void (*set_rptr)(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih);
 };
 
 #define amdgpu_ih_get_wptr(adev, ih) (adev)->irq.ih_funcs->get_wptr((adev), (ih))
 #define amdgpu_ih_decode_iv(adev, iv) \
 	(adev)->irq.ih_funcs->decode_iv((adev), (ih), (iv))
+#define amdgpu_ih_decode_iv_ts(adev, ih, rptr, offset) \
+	(WARN_ON_ONCE(!(adev)->irq.ih_funcs->decode_iv_ts) ? 0 : \
+	(adev)->irq.ih_funcs->decode_iv_ts((ih), (rptr), (offset)))
 #define amdgpu_ih_set_rptr(adev, ih) (adev)->irq.ih_funcs->set_rptr((adev), (ih))
 
 int amdgpu_ih_ring_init(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih,
@@ -89,10 +95,13 @@ int amdgpu_ih_ring_init(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih,
 void amdgpu_ih_ring_fini(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih);
 void amdgpu_ih_ring_write(struct amdgpu_ih_ring *ih, const uint32_t *iv,
 			  unsigned int num_dw);
-int amdgpu_ih_wait_on_checkpoint_process(struct amdgpu_device *adev,
-					struct amdgpu_ih_ring *ih);
+int amdgpu_ih_wait_on_checkpoint_process_ts(struct amdgpu_device *adev,
+					    struct amdgpu_ih_ring *ih);
 int amdgpu_ih_process(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih);
+int amdgpu_ih_process1(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih);
 void amdgpu_ih_decode_iv_helper(struct amdgpu_device *adev,
 				struct amdgpu_ih_ring *ih,
 				struct amdgpu_iv_entry *entry);
+uint64_t amdgpu_ih_decode_iv_ts_helper(struct amdgpu_ih_ring *ih, u32 rptr,
+				       signed offset);
 #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
index e9023687dc9a..891486cca94b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -224,7 +224,7 @@ static void amdgpu_irq_handle_ih1(struct work_struct *work)
 	struct amdgpu_device *adev = container_of(work, struct amdgpu_device,
 						  irq.ih1_work);
 
-	amdgpu_ih_process(adev, &adev->irq.ih1);
+	amdgpu_ih_process1(adev, &adev->irq.ih1);
 }
 
 /**
diff --git a/drivers/gpu/drm/amd/amdgpu/navi10_ih.c b/drivers/gpu/drm/amd/amdgpu/navi10_ih.c
index 38241cf0e1f1..8ce5b8ca1fd7 100644
--- a/drivers/gpu/drm/amd/amdgpu/navi10_ih.c
+++ b/drivers/gpu/drm/amd/amdgpu/navi10_ih.c
@@ -716,6 +716,7 @@ static const struct amd_ip_funcs navi10_ih_ip_funcs = {
 static const struct amdgpu_ih_funcs navi10_ih_funcs = {
 	.get_wptr = navi10_ih_get_wptr,
 	.decode_iv = amdgpu_ih_decode_iv_helper,
+	.decode_iv_ts = amdgpu_ih_decode_iv_ts_helper,
 	.set_rptr = navi10_ih_set_rptr
 };
 
diff --git a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
index a9ca6988009e..3070466f54e1 100644
--- a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
+++ b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
@@ -640,6 +640,7 @@ const struct amd_ip_funcs vega10_ih_ip_funcs = {
 static const struct amdgpu_ih_funcs vega10_ih_funcs = {
 	.get_wptr = vega10_ih_get_wptr,
 	.decode_iv = amdgpu_ih_decode_iv_helper,
+	.decode_iv_ts = amdgpu_ih_decode_iv_ts_helper,
 	.set_rptr = vega10_ih_set_rptr
 };
 
diff --git a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
index f51dfc38ac65..3b4eb8285943 100644
--- a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
+++ b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
@@ -688,6 +688,7 @@ const struct amd_ip_funcs vega20_ih_ip_funcs = {
 static const struct amdgpu_ih_funcs vega20_ih_funcs = {
 	.get_wptr = vega20_ih_get_wptr,
 	.decode_iv = amdgpu_ih_decode_iv_helper,
+	.decode_iv_ts = amdgpu_ih_decode_iv_ts_helper,
 	.set_rptr = vega20_ih_set_rptr
 };
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 10868d5b549f..663489ae56d7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -1974,7 +1974,7 @@ static void svm_range_drain_retry_fault(struct svm_range_list *svms)
 
 		pr_debug("drain retry fault gpu %d svms %p\n", i, svms);
 
-		amdgpu_ih_wait_on_checkpoint_process(pdd->dev->adev,
+		amdgpu_ih_wait_on_checkpoint_process_ts(pdd->dev->adev,
 						     &pdd->dev->adev->irq.ih1);
 		pr_debug("drain retry fault gpu %d svms 0x%p done\n", i, svms);
 	}
-- 
2.17.1

Re: [PATCH v5] drm/amdgpu: handle IH ring1 overflow

[Christian König]

Am 23.11.21 um 20:22 schrieb Philip Yang:
> IH ring1 is used to process GPU retry fault, overflow is enabled to
> drain retry fault because we want receive other interrupts while
> handling retry fault to recover range. There is no overflow flag set
> when wptr pass rptr. Use timestamp of rptr and wptr to handle overflow
> and drain retry fault.
>
> Add helper function amdgpu_ih_decode_iv_ts to get 48bit timestamp from
> IV entry. drain retry fault is done if processed_timestamp is
> equal to or larger than checkpoint timestamp.
>
> Add function amdgpu_ih_process1 to process IH ring1 until timestamp of
> rptr is larger then timestamp of next entry.
>
> Helper amdgpu_ih_ts_after to compare time stamps with 48bit wrap around.
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c  | 107 ++++++++++++++++++------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h  |  13 ++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c |   2 +-
>   drivers/gpu/drm/amd/amdgpu/navi10_ih.c  |   1 +
>   drivers/gpu/drm/amd/amdgpu/vega10_ih.c  |   1 +
>   drivers/gpu/drm/amd/amdgpu/vega20_ih.c  |   1 +
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c    |   2 +-
>   7 files changed, 99 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
> index 0c7963dfacad..30b4e0e01444 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
> @@ -164,52 +164,45 @@ void amdgpu_ih_ring_write(struct amdgpu_ih_ring *ih, const uint32_t *iv,
>   	}
>   }
>   
> +/* return true if time stamp t2 is after t1 with 48bit wrap around */
> +static inline bool amdgpu_ih_ts_after(uint64_t t1, uint64_t t2)
> +{
> +	return ((int64_t)(t2 << 16) - (int64_t)(t1 << 16)) > 0LL;
> +}
> +
>   /* Waiter helper that checks current rptr matches or passes checkpoint wptr */
> -static bool amdgpu_ih_has_checkpoint_processed(struct amdgpu_device *adev,
> +static bool amdgpu_ih_has_checkpoint_processed_ts(struct amdgpu_device *adev,
>   					struct amdgpu_ih_ring *ih,
> -					uint32_t checkpoint_wptr,
> -					uint32_t *prev_rptr)
> +					uint64_t checkpoint_ts)
>   {
> -	uint32_t cur_rptr = ih->rptr | (*prev_rptr & ~ih->ptr_mask);
> -
> -	/* rptr has wrapped. */
> -	if (cur_rptr < *prev_rptr)
> -		cur_rptr += ih->ptr_mask + 1;
> -	*prev_rptr = cur_rptr;
> -
> -	/* check ring is empty to workaround missing wptr overflow flag */
> -	return cur_rptr >= checkpoint_wptr ||
> -	       (cur_rptr & ih->ptr_mask) == amdgpu_ih_get_wptr(adev, ih);
> +	return !amdgpu_ih_ts_after(ih->processed_timestamp, checkpoint_ts);

I don't see much of a reason to keep this function, it only consists of 
a call to amdgpu_ih_ts_after() and is used only once.

>   }
>   
>   /**
> - * amdgpu_ih_wait_on_checkpoint_process - wait to process IVs up to checkpoint
> + * amdgpu_ih_wait_on_checkpoint_process_ts - wait to process IVs up to checkpoint
>    *
>    * @adev: amdgpu_device pointer
>    * @ih: ih ring to process
>    *
>    * Used to ensure ring has processed IVs up to the checkpoint write pointer.
>    */
> -int amdgpu_ih_wait_on_checkpoint_process(struct amdgpu_device *adev,
> +int amdgpu_ih_wait_on_checkpoint_process_ts(struct amdgpu_device *adev,
>   					struct amdgpu_ih_ring *ih)
>   {
> -	uint32_t checkpoint_wptr, rptr;
> +	uint32_t checkpoint_wptr;
> +	uint64_t checkpoint_ts;
>   
>   	if (!ih->enabled || adev->shutdown)
>   		return -ENODEV;
>   
>   	checkpoint_wptr = amdgpu_ih_get_wptr(adev, ih);
> -	/* Order wptr with rptr. */
> +	/* Order wptr with ring data. */
>   	rmb();
> -	rptr = READ_ONCE(ih->rptr);
> -
> -	/* wptr has wrapped. */
> -	if (rptr > checkpoint_wptr)
> -		checkpoint_wptr += ih->ptr_mask + 1;
> +	checkpoint_ts = amdgpu_ih_decode_iv_ts(adev, ih, checkpoint_wptr, -1);
>   
>   	return wait_event_interruptible(ih->wait_process,
> -				amdgpu_ih_has_checkpoint_processed(adev, ih,
> -						checkpoint_wptr, &rptr));
> +				amdgpu_ih_has_checkpoint_processed_ts(adev, ih,
> +						checkpoint_ts));
>   }
>   
>   /**
> @@ -254,6 +247,56 @@ int amdgpu_ih_process(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih)
>   	return IRQ_HANDLED;
>   }
>   
> +/**
> + * amdgpu_ih_process1 - interrupt handler work for IH ring1
> + *
> + * @adev: amdgpu_device pointer
> + * @ih: ih ring to process
> + *
> + * Interrupt handler of IH ring1, walk the IH ring1.
> + * Returns irq process return code.
> + */
> +int amdgpu_ih_process1(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih)

I don't think we need this new function any more.

The check if the timestamp goes backwards can now be done inside the 
page fault handler.

> +{
> +	uint64_t ts, ts_next;
> +	unsigned int count;
> +	u32 wptr;
> +
> +	if (!ih->enabled || adev->shutdown)
> +		return IRQ_NONE;
> +
> +	wptr = amdgpu_ih_get_wptr(adev, ih);
> +	if (ih->rptr == wptr)
> +		return 0;
> +
> +restart_ih:
> +	count = AMDGPU_IH_MAX_NUM_IVS;
> +
> +	ts = amdgpu_ih_decode_iv_ts(adev, ih, ih->rptr, 0);
> +	ts_next = amdgpu_ih_decode_iv_ts(adev, ih, ih->rptr, 1);
> +	while (amdgpu_ih_ts_after(ts, ts_next) && --count) {
> +		amdgpu_irq_dispatch(adev, ih);
> +		ih->rptr &= ih->ptr_mask;
> +		ts = ts_next;
> +		ts_next = amdgpu_ih_decode_iv_ts(adev, ih, ih->rptr, 1);
> +	}
> +	/*
> +	 * Process the last timestamp updated entry or one more entry
> +	 * if count = 0, ts is timestamp of the entry.
> +	 */
> +	amdgpu_irq_dispatch(adev, ih);
> +	amdgpu_ih_set_rptr(adev, ih);
> +	wake_up_all(&ih->wait_process);
> +
> +	wptr = amdgpu_ih_get_wptr(adev, ih);
> +	/* Order reading of wptr vs. reading of IH ring data */
> +	rmb();
> +	if (amdgpu_ih_ts_after(ts, amdgpu_ih_decode_iv_ts(adev, ih, wptr, -1)))
> +		goto restart_ih;
> +
> +	return IRQ_HANDLED;
> +}
> +
>   /**
>    * amdgpu_ih_decode_iv_helper - decode an interrupt vector
>    *
> @@ -298,4 +341,20 @@ void amdgpu_ih_decode_iv_helper(struct amdgpu_device *adev,
>   
>   	/* wptr/rptr are in bytes! */
>   	ih->rptr += 32;
> +	ih->processed_timestamp = entry->timestamp;
> +}
> +
> +uint64_t amdgpu_ih_decode_iv_ts_helper(struct amdgpu_ih_ring *ih, u32 rptr,
> +				       signed offset)
> +{
> +	uint32_t iv_size = 32;
> +	uint32_t dw1, dw2;
> +	uint32_t index;
> +
> +	rptr += iv_size * offset;
> +	index = (rptr & ih->ptr_mask) >> 2;
> +
> +	dw1 = le32_to_cpu(ih->ring[index + 1]);
> +	dw2 = le32_to_cpu(ih->ring[index + 2]);
> +	return dw1 | ((u64)(dw2 & 0xffff) << 32);
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
> index 0649b59830a5..d7e1ffeca38f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
> @@ -68,6 +68,7 @@ struct amdgpu_ih_ring {
>   
>   	/* For waiting on IH processing at checkpoint. */
>   	wait_queue_head_t wait_process;
> +	uint64_t		processed_timestamp;
>   };
>   
>   /* provided by the ih block */
> @@ -76,12 +77,17 @@ struct amdgpu_ih_funcs {
>   	u32 (*get_wptr)(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih);
>   	void (*decode_iv)(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih,
>   			  struct amdgpu_iv_entry *entry);
> +	uint64_t (*decode_iv_ts)(struct amdgpu_ih_ring *ih, u32 rptr,
> +				 signed offset);
>   	void (*set_rptr)(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih);
>   };
>   
>   #define amdgpu_ih_get_wptr(adev, ih) (adev)->irq.ih_funcs->get_wptr((adev), (ih))
>   #define amdgpu_ih_decode_iv(adev, iv) \
>   	(adev)->irq.ih_funcs->decode_iv((adev), (ih), (iv))
> +#define amdgpu_ih_decode_iv_ts(adev, ih, rptr, offset) \
> +	(WARN_ON_ONCE(!(adev)->irq.ih_funcs->decode_iv_ts) ? 0 : \

Please drop that WARN_ON_ONCE here.

Regards,
Christian.

> +	(adev)->irq.ih_funcs->decode_iv_ts((ih), (rptr), (offset)))
>   #define amdgpu_ih_set_rptr(adev, ih) (adev)->irq.ih_funcs->set_rptr((adev), (ih))
>   
>   int amdgpu_ih_ring_init(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih,
> @@ -89,10 +95,13 @@ int amdgpu_ih_ring_init(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih,
>   void amdgpu_ih_ring_fini(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih);
>   void amdgpu_ih_ring_write(struct amdgpu_ih_ring *ih, const uint32_t *iv,
>   			  unsigned int num_dw);
> -int amdgpu_ih_wait_on_checkpoint_process(struct amdgpu_device *adev,
> -					struct amdgpu_ih_ring *ih);
> +int amdgpu_ih_wait_on_checkpoint_process_ts(struct amdgpu_device *adev,
> +					    struct amdgpu_ih_ring *ih);
>   int amdgpu_ih_process(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih);
> +int amdgpu_ih_process1(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih);
>   void amdgpu_ih_decode_iv_helper(struct amdgpu_device *adev,
>   				struct amdgpu_ih_ring *ih,
>   				struct amdgpu_iv_entry *entry);
> +uint64_t amdgpu_ih_decode_iv_ts_helper(struct amdgpu_ih_ring *ih, u32 rptr,
> +				       signed offset);
>   #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> index e9023687dc9a..891486cca94b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> @@ -224,7 +224,7 @@ static void amdgpu_irq_handle_ih1(struct work_struct *work)
>   	struct amdgpu_device *adev = container_of(work, struct amdgpu_device,
>   						  irq.ih1_work);
>   
> -	amdgpu_ih_process(adev, &adev->irq.ih1);
> +	amdgpu_ih_process1(adev, &adev->irq.ih1);
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/navi10_ih.c b/drivers/gpu/drm/amd/amdgpu/navi10_ih.c
> index 38241cf0e1f1..8ce5b8ca1fd7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/navi10_ih.c
> +++ b/drivers/gpu/drm/amd/amdgpu/navi10_ih.c
> @@ -716,6 +716,7 @@ static const struct amd_ip_funcs navi10_ih_ip_funcs = {
>   static const struct amdgpu_ih_funcs navi10_ih_funcs = {
>   	.get_wptr = navi10_ih_get_wptr,
>   	.decode_iv = amdgpu_ih_decode_iv_helper,
> +	.decode_iv_ts = amdgpu_ih_decode_iv_ts_helper,
>   	.set_rptr = navi10_ih_set_rptr
>   };
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
> index a9ca6988009e..3070466f54e1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
> @@ -640,6 +640,7 @@ const struct amd_ip_funcs vega10_ih_ip_funcs = {
>   static const struct amdgpu_ih_funcs vega10_ih_funcs = {
>   	.get_wptr = vega10_ih_get_wptr,
>   	.decode_iv = amdgpu_ih_decode_iv_helper,
> +	.decode_iv_ts = amdgpu_ih_decode_iv_ts_helper,
>   	.set_rptr = vega10_ih_set_rptr
>   };
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
> index f51dfc38ac65..3b4eb8285943 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
> @@ -688,6 +688,7 @@ const struct amd_ip_funcs vega20_ih_ip_funcs = {
>   static const struct amdgpu_ih_funcs vega20_ih_funcs = {
>   	.get_wptr = vega20_ih_get_wptr,
>   	.decode_iv = amdgpu_ih_decode_iv_helper,
> +	.decode_iv_ts = amdgpu_ih_decode_iv_ts_helper,
>   	.set_rptr = vega20_ih_set_rptr
>   };
>   
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 10868d5b549f..663489ae56d7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -1974,7 +1974,7 @@ static void svm_range_drain_retry_fault(struct svm_range_list *svms)
>   
>   		pr_debug("drain retry fault gpu %d svms %p\n", i, svms);
>   
> -		amdgpu_ih_wait_on_checkpoint_process(pdd->dev->adev,
> +		amdgpu_ih_wait_on_checkpoint_process_ts(pdd->dev->adev,
>   						     &pdd->dev->adev->irq.ih1);
>   		pr_debug("drain retry fault gpu %d svms 0x%p done\n", i, svms);
>   	}

Re: [PATCH v5] drm/amdgpu: handle IH ring1 overflow

[philip yang]

[-- Attachment #1: Type: text/html, Size: 24518 bytes --]

Re: [PATCH v5] drm/amdgpu: handle IH ring1 overflow

[Christian König]

Am 24.11.21 um 16:23 schrieb philip yang:
> [SNIP]
>>>   +/**
>>> + * amdgpu_ih_process1 - interrupt handler work for IH ring1
>>> + *
>>> + * @adev: amdgpu_device pointer
>>> + * @ih: ih ring to process
>>> + *
>>> + * Interrupt handler of IH ring1, walk the IH ring1.
>>> + * Returns irq process return code.
>>> + */
>>> +int amdgpu_ih_process1(struct amdgpu_device *adev, struct 
>>> amdgpu_ih_ring *ih)
>>
>> I don't think we need this new function any more.
>>
>> The check if the timestamp goes backwards can now be done inside the 
>> page fault handler.
> Do you mean to merge this into the original ring0 interrupt handler? 
> Then we need add parameter (ih->overflow_enabled) and process two 
> different cases in ring0 interrupt handler, I think that is not good 
> to maintain later so I want to separate them.

What I mean is you don't need any different handling any more if we use 
the timestamp anyway.

Just keep the last processed timestamp in the page fault code and ignore 
faults when it starts to go backwards.

If IVs should be dropped or processed as much as possible is depending 
on quite a bunch of things and should probably not be handled in the IH 
code in general.

Regards,
Christian.

Re: [PATCH v5] drm/amdgpu: handle IH ring1 overflow

[Felix Kuehling]

Am 2021-11-24 um 10:23 a.m. schrieb philip yang:
>>>     #define amdgpu_ih_get_wptr(adev, ih)
>>> (adev)->irq.ih_funcs->get_wptr((adev), (ih))
>>>   #define amdgpu_ih_decode_iv(adev, iv) \
>>>       (adev)->irq.ih_funcs->decode_iv((adev), (ih), (iv))
>>> +#define amdgpu_ih_decode_iv_ts(adev, ih, rptr, offset) \
>>> +    (WARN_ON_ONCE(!(adev)->irq.ih_funcs->decode_iv_ts) ? 0 : \
>>
>> Please drop that WARN_ON_ONCE here.
>>
> Agree, will drop it.
>
I suggested this. We're assuming that this function will never be called
on hardware that doesn't support time stamps, and that all hardware with
time stamps will implement the decode_iv_ts function. But it's good to
get a log message if that assumption is ever broken, rather than just
silently getting wrong results.

Regards,
  Felix


> Regards,
>
> Philip
>

Re: [PATCH v5] drm/amdgpu: handle IH ring1 overflow

[philip yang]

[-- Attachment #1: Type: text/html, Size: 2630 bytes --]

Re: [PATCH v5] drm/amdgpu: handle IH ring1 overflow

[Christian König]

Am 24.11.21 um 21:20 schrieb Felix Kuehling:
> Am 2021-11-24 um 10:23 a.m. schrieb philip yang:
>>>>      #define amdgpu_ih_get_wptr(adev, ih)
>>>> (adev)->irq.ih_funcs->get_wptr((adev), (ih))
>>>>    #define amdgpu_ih_decode_iv(adev, iv) \
>>>>        (adev)->irq.ih_funcs->decode_iv((adev), (ih), (iv))
>>>> +#define amdgpu_ih_decode_iv_ts(adev, ih, rptr, offset) \
>>>> +    (WARN_ON_ONCE(!(adev)->irq.ih_funcs->decode_iv_ts) ? 0 : \
>>> Please drop that WARN_ON_ONCE here.
>>>
>> Agree, will drop it.
>>
> I suggested this. We're assuming that this function will never be called
> on hardware that doesn't support time stamps, and that all hardware with
> time stamps will implement the decode_iv_ts function. But it's good to
> get a log message if that assumption is ever broken, rather than just
> silently getting wrong results.

Well exactly that's the point, you won't get wrong results but a NULL 
pointer exception instead.

So we already have a backtrace in the logs.

Regards,
Christian.

>
> Regards,
>    Felix
>
>
>> Regards,
>>
>> Philip
>>

Re: [PATCH v5] drm/amdgpu: handle IH ring1 overflow

[Christian König]

Am 24.11.21 um 23:52 schrieb philip yang:
>
>
> On 2021-11-24 10:33 a.m., Christian König wrote:
>>
>>
>> Am 24.11.21 um 16:23 schrieb philip yang:
>>> [SNIP]
>>>>>   +/**
>>>>> + * amdgpu_ih_process1 - interrupt handler work for IH ring1
>>>>> + *
>>>>> + * @adev: amdgpu_device pointer
>>>>> + * @ih: ih ring to process
>>>>> + *
>>>>> + * Interrupt handler of IH ring1, walk the IH ring1.
>>>>> + * Returns irq process return code.
>>>>> + */
>>>>> +int amdgpu_ih_process1(struct amdgpu_device *adev, struct 
>>>>> amdgpu_ih_ring *ih)
>>>>
>>>> I don't think we need this new function any more.
>>>>
>>>> The check if the timestamp goes backwards can now be done inside 
>>>> the page fault handler.
>>> Do you mean to merge this into the original ring0 interrupt handler? 
>>> Then we need add parameter (ih->overflow_enabled) and process two 
>>> different cases in ring0 interrupt handler, I think that is not good 
>>> to maintain later so I want to separate them.
>>
>> What I mean is you don't need any different handling any more if we 
>> use the timestamp anyway.
>>
>> Just keep the last processed timestamp in the page fault code and 
>> ignore faults when it starts to go backwards.
>>
>> If IVs should be dropped or processed as much as possible is 
>> depending on quite a bunch of things and should probably not be 
>> handled in the IH code in general.
>
> Use ih->processed_timestamp updated in decode_iv, for both checkpoint 
> process and page fault handler to drop stale fault.
>
> interrupt handler don't need change.
>

Well that's what I wanted to say: I would rather change the interrupt 
handler for page faults than the general interrupt processing.

That we should drop older faults is page fault handling specific and 
should not affect other handlers.

Regards,
Christian.

> Thanks,
>
> Philip
>
>>
>> Regards,
>> Christian.

Re: [PATCH v5] drm/amdgpu: handle IH ring1 overflow

[philip yang]

[-- Attachment #1: Type: text/html, Size: 2880 bytes --]