Am 25.02.21 um 19:33 schrieb Felix Kuehling:
[SNIP]
This in turn can lead to starvation of the work handler and so a life
lock as well.
I won't touch rptr or wptr at all for this.
Not sure what's your idea here, using ih->lock. Is it to completely
drain all IRQs until the IH ring is completely empty?
Correct.
That can
live-lock, if the GPU produces IRQs faster than the kernel can process
them. Therefore I was looking at rptr and wptr to drain only IRQs that
were already in the queue when the drain call was made. That also
ensures that the wait time is bounded and should be short (unless the
ring size is huge).
Correct as well, but the problem here is that Jonathan's
implementation is not even remotely correct.
See when you look at the rptr and wptr you can't be sure that they
haven't wrapped around between two looks.
What you could do is look at both the rptr as well as the original
wptr, but that is tricky.
The code in Jon's patch was suggested by me. I check for wrapping by
comparing rptr with the rptr from the previous loop iteration. Comparing
rptr with wptr you can never be sure whether rptr has already passed
wptr, or whether rptr has to wrap first.
Exactly that's my concern as well.
I could see a compromise where we sleep and wake up the waiting threads when
1. the IH ring is empty
2. the IH rptr wraps
That should be good enough to ensure a quick response in the common case
(no interrupt storm), and a reasonable response in the interrupt storm case.
But then it's still not clear what's the correct condition for checking
that the interrupts the caller cares about have been drained. Looking at
just rptr and wptr is always ambiguous. Maybe we could use timestamps of
the last processed interrupt? Those 48-bit time stamps wrap much less
frequently. The idea is this:
* At the start get the timestamp of the last written IH ring entry
(checkpoint)
* Wait until the last_processed IH timestamp passes the checkpoint:
sign_extend(last_processed - checkpoint) >= 0
Yeah that could work. Alternatively we could just have a rptr wrap
around counter.
This way you do something like this:
1. get the wrap around counter.
2. get wptr
3. get rptr
4. compare the wrap around counter with the old one, if it has changed
start over with #1
5. Use wrap around counter and rptr/wptr to come up with 64bit values.
6. Compare wptr with rptr/wrap around counter until we are sure the IHs
are processed.
Regards,
Christian.
Regards,
Felix
Regards,
Christian.
Regards,
Felix
Regards,
Christian.
Regards,
Felix
Suggested-by: Felix Kuehling <felix.kuehling@xxxxxxx>
Signed-off-by: Jonathan Kim <jonathan.kim@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c | 46
+++++++++++++++++++++++++-
drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h | 2 ++
2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
index dc852af4f3b7..cae50af9559d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
@@ -22,7 +22,7 @@
*/
#include <linux/dma-mapping.h>
-
+#include <linux/processor.h>
#include "amdgpu.h"
#include "amdgpu_ih.h"
@@ -160,6 +160,50 @@ void amdgpu_ih_ring_write(struct
amdgpu_ih_ring *ih, const uint32_t *iv,
}
}
+/**
+ * amdgpu_ih_wait_on_checkpoint_process - 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,
+struct amdgpu_ih_ring *ih)
+{
+u32 prev_rptr, cur_rptr, checkpoint_wptr;
+
+if (!ih->enabled || adev->shutdown)
+return -ENODEV;
+
+cur_rptr = READ_ONCE(ih->rptr);
+/* Order read of current rptr with checktpoint wptr. */
+mb();
+checkpoint_wptr = amdgpu_ih_get_wptr(adev, ih);
+
+/* allow rptr to wrap around */
+if (cur_rptr > checkpoint_wptr) {
+spin_begin();
+do {
+spin_cpu_relax();
+prev_rptr = cur_rptr;
+cur_rptr = READ_ONCE(ih->rptr);
+} while (cur_rptr >= prev_rptr);
+spin_end();
That's a certain NAK since it busy waits for IH processing. We need
some
event to trigger here.
The function is meant to be just a waiter up to the checkpoint.
There's a need to guarantee that "stale" interrupts have been
processed on check before doing other stuff after call.
The description could be improved to clarify that.
Would busy waiting only on a locked ring help? I assume an unlocked
ring means nothing to process so no need to wait and we can exit
early. Or is it better to just to process the entries up to the
checkpoint (maybe adjust amdgpu_ih_process for this need like adding
a bool arg to skip restart or something)?
Thanks,
Jon
+}
+
+/* wait for rptr to catch up to or pass checkpoint. */
+spin_begin();
+do {
+spin_cpu_relax();
+prev_rptr = cur_rptr;
+cur_rptr = READ_ONCE(ih->rptr);
+} while (cur_rptr >= prev_rptr && cur_rptr < checkpoint_wptr);
Same of course here.
Christian.
+spin_end();
+
+return 0;
+}
+
/**
* amdgpu_ih_process - interrupt handler
*
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
index 6ed4a85fc7c3..6817f0a812d2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
@@ -87,6 +87,8 @@ 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_process(struct amdgpu_device *adev, struct
amdgpu_ih_ring *ih);
void amdgpu_ih_decode_iv_helper(struct amdgpu_device *adev,
struct amdgpu_ih_ring *ih,
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Cfelix.kuehling%40amd.com%7C84d85e54bdcb4593e07f08d8d994be77%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637498580167313193%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=RvRHB9l4O%2BpbpogUFKUmnMGkqKnecwQCYRHrkxICDqU%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
