[PATCH] drm/amdgpu: add ih call to process until checkpoint
Felix Kuehling
felix.kuehling at amd.com
Thu Feb 25 15:35:29 UTC 2021
Am 2021-02-25 um 8:53 a.m. schrieb Christian König:
>
>
> Am 25.02.21 um 04:15 schrieb Felix Kuehling:
>> On 2021-02-24 10:54 a.m., Kim, Jonathan wrote:
>>> [AMD Official Use Only - Internal Distribution Only]
>>>
>>>> -----Original Message-----
>>>> From: Koenig, Christian <Christian.Koenig at amd.com>
>>>> Sent: Wednesday, February 24, 2021 4:17 AM
>>>> To: Kim, Jonathan <Jonathan.Kim at amd.com>; amd-
>>>> gfx at lists.freedesktop.org
>>>> Cc: Yang, Philip <Philip.Yang at amd.com>; Kuehling, Felix
>>>> <Felix.Kuehling at amd.com>
>>>> Subject: Re: [PATCH] drm/amdgpu: add ih call to process until
>>>> checkpoint
>>>>
>>>> Am 23.02.21 um 22:10 schrieb Jonathan Kim:
>>>>> Add IH function to allow caller to process ring entries until the
>>>>> checkpoint write pointer.
>>>> This needs a better description of what this will be used for.
>>> Felix or Philip could elaborate better for HMM needs.
>>> Debugging tools requires this but it's in experimental mode at the
>>> moment so probably not the best place to describe here.
>>
>> On the HMM side we're planning to use this to drain pending page
>> fault interrupts before we unmap memory. That should address phantom
>> VM faults after memory is unmapped.
>
> Thought so. I suggest to use a wait_event() here which on the waiter
> side checks ih->lock and add a wake_up_all() at the end of
> amdgpu_ih_process.
Right. I thought about that and it should be easy to add. The reason to
suggest busy waiting first is, that interrupt processing is supposed to
be fast. The IRQ handler itself doesn't sleep. So I'd expect the wait
time to be short enough that sleeping and scheduling is not worth it.
> 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? 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).
Regards,
Felix
>
> Regards,
> Christian.
>
>>
>> Regards,
>> Felix
>>
>>
>>>
>>>>> Suggested-by: Felix Kuehling <felix.kuehling at amd.com>
>>>>> Signed-off-by: Jonathan Kim <jonathan.kim at 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 at lists.freedesktop.org
>> 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
>>
>
More information about the amd-gfx
mailing list