Am 08.03.21 um 22:09 schrieb Kim, Jonathan:
[SNIP]
First of all rptr/wptr are not 32bit, their maximum is 20 or 19 bits IIRC (and
they are dw, so 4M or 2M bytes).
Thanks Christian. This makes sense now. I can see how rptrs advance by dword
sets in the iv decode helper.
My apologies, but I'm still a bit confused on the pseudo code below and have a
few questions before I give this another go ...
Then the purpose of the wait_event() is to wait for changes of the rptr, so
the matching wake_up() should be at the same place as calling
amdgpu_ih_set_rptr().
My original idea of the wrap around counter assumes that the counter is
updated in amdgpu_ih_process(). That isn't strictly necessary, but it could be
better even if it adds some overhead to IH processing.
If you want to implement it like below it should look something like this:
uint32_t rptr, wptr, tmp;
wptr = amdgpu_ih_get_wptr(adev, ih);
rmb();
rptr = READ_ONCE(ih->rptr);
if (rptr > wptr)
rptr += ih->ptr_mask + 1;
So I think you're trying to be safe by assuming the rptr still needs to
overtake the wptr and hasn't done so yet so this is looking ahead by saying the
rptr will have to wrap?
I really need to wake up before writing stuff like that. No, the problem
why you don't understand is that I mixed up rptr and wptr :)
wait_event(ig->processing, {
tmp = amdgpu_ih_get_wptr(adev, ih);
tmp |= wptr & ~ih->ptr_mask;
if (tmp < wptr)
tmp += ih->ptr_mask + 1;
wptr = tmp;
For short term use, I think the caller will try to guarantee that the
checkpoint wptr is checked during a time when interrupts won't be generated
anymore, but I guess for general use, we can't guarantee this and this is why
we're using a moving wptr checkpoint?
wptr >= rptr;
Assuming you mean 'return wptr >= rptr' will unblock if true, I can see this for wptr
== rptr, but why wptr > rptr? What happens if the caller waits when nothing is
wrapped but there are entries to be processed? (I'm assuming rptr < wptr can be true
with entries that still require processing). Won't the caller advance without waiting?
No as I wrote above I just completely mixed up things here. Let me try
once more:
wptr = amdgpu_ih_get_wptr(adev, ih);
rmb();
rptr = READ_ONCE(ih->rptr);
if (rptr > wptr)
wptr += ih->ptr_mask + 1;
wait_event(ig->processing, {
tmp = ih->rptr;
tmp |= rptr & ~ih->ptr_mask;
if (tmp < rptr)
tmp += ih->ptr_mask + 1;
rptr = tmp;
rptr >= wptr;
});
What I also wanted to note here is that using wait_event() makes things
much easier since it provides the necessary memory barriers.
})
I would put the condition of the wait_event into a separate function to make
it more readable and not use GCC extension, but essentially that's it.
The only problem this could have is that the wptr wraps around multiple
times between wake ups. To handle this as well we would need to increment
the wrap around counter in amdgpu_ih_process() as well, but this means
some extra overhead during IH processing. And then I would also use 64bit
counters to make the stuff easier to handle.
I'm still not fully clear on how 64-bit counters help. It already looks like
you're leveraging the ptr_mask limit of 19/20 bits to increment the unused top
bits of the rptr/wptr by adding the rb size.
When you detect the rptr wrap around only on the waiters side there is
the chance that you miss a wrap around and wait to long.
But I now think that is better than adding overhead to the processing side.
regards,
Christian.
Thanks,
Jon
Regards,
Christian.
Thanks,
Jon
Christian.
+
+ spin_begin();
+ while (true) {
+ bool rptr_wrapped = false, wptr_wrapped = false;
+ u32 rptr, wptr;
+
+ spin_cpu_relax();
+
+ /* Update wptr checkpoint/wrap count compare. */
+ wptr = amdgpu_ih_get_wptr(adev, ih);
+ if (wptr < prev_wptr) {
+ wptr_wrap++;
+ wptr_check = wptr | (wptr_wrap << 32);
+ wptr_wrapped = true;
+ }
+ prev_wptr = wptr;
+
+ /* Order wptr with rptr. */
+ rmb();
+
+ /* Update rptr/wrap count compare. */
+ rptr = READ_ONCE(ih->rptr);
+ if (rptr < prev_rptr) {
+ rptr_wrap++;
+ rptr_wrapped = true;
+ }
+ rptr_check = rptr | (rptr_wrap << 32);
+ prev_rptr = rptr;
+
+ /* Wrapping occurred so restart. */
+ if (rptr_wrapped || wptr_wrapped)
+ continue;
+
+ /* Exit on reaching or passing checkpoint. */
+ if (rptr_check >= wptr_check &&
+ rptr >= (wptr_check & ih->ptr_mask))
+ break;
+ }
+ 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
