<html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 2021-11-24 4:37 a.m., Christian König wrote: </div> <blockquote type="cite" cite="mid:3d17fd21-751d-6f87-95b9-d120db7f4a0e@gmail.com">Am 23.11.21 um 20:22 schrieb Philip Yang: <blockquote type="cite">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 <a class="moz-txt-link-rfc2396E" href="mailto:Philip.Yang@amd.com"><Philip.Yang@amd.com></a> --- 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); </blockquote> 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. </blockquote> Will remove this function. <blockquote type="cite" cite="mid:3d17fd21-751d-6f87-95b9-d120db7f4a0e@gmail.com"> <blockquote type="cite"> } /** - * 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, </blockquote> </blockquote> I will change this to wait_event_interruptible_timeout, found wait event may never returns if gpu reset, rptr=wptr=0, ih->processed_timestamp is not updated. Timeout value 1 second is long enough to drain fault. <blockquote type="cite" cite="mid:3d17fd21-751d-6f87-95b9-d120db7f4a0e@gmail.com"> <blockquote type="cite">- 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) </blockquote> 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. </blockquote> 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. <blockquote type="cite" cite="mid:3d17fd21-751d-6f87-95b9-d120db7f4a0e@gmail.com"> <blockquote type="cite">+{ + 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 : \ </blockquote> Please drop that WARN_ON_ONCE here. </blockquote> Agree, will drop it. Regards, Philip <blockquote type="cite" cite="mid:3d17fd21-751d-6f87-95b9-d120db7f4a0e@gmail.com">Regards, Christian. <blockquote type="cite">+ (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); } </blockquote> </blockquote> </body> </html>