Re: [PATCH] drm/amdgpu: add ih call to process until checkpoint

["Christian König" <christian.koenig@amd.com>]

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@amd.com>
>>>>>>>> Signed-off-by: Jonathan Kim <jonathan.kim@amd.com>
>>>>>>>> ---
>>>>>>>>      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@lists.freedesktop.org
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cfelix.kuehling%40amd.com%7C84d85e54bdcb4593e07f08d8d994be77%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637498580167313193%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=RvRHB9l4O%2BpbpogUFKUmnMGkqKnecwQCYRHrkxICDqU%3D&amp;reserved=0
>>>>>
>>>>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx