[PATCH] drm/amdgpu: add ih waiter on process until checkpoint

[Christian König]

Am 05.03.21 um 21:11 schrieb Jonathan Kim:
> Add IH function to allow caller to wait until ring entries are processed
> until the checkpoint write pointer.
>
> This will be primarily used by HMM to drain pending page fault interrupts
> before memory unmap to prevent HMM from handling stale interrupts.
>
> v2: Update title and description to clarify use.
> Add rptr/wptr wrap counter checks to guarantee ring entries are processed
> until the checkpoint.

First of all as I said please use a wait_event, busy waiting is a clear NAK.

>
> Signed-off-by: Jonathan Kim <jonathan.kim at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c | 68 +++++++++++++++++++++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h |  2 +
>   2 files changed, 69 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..954518b4fb79 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,72 @@ 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)
> +{
> +	u64 rptr_check, wptr_check, rptr_wrap = 0, wptr_wrap = 0;
> +	u32 prev_rptr, prev_wptr;
> +
> +	if (!ih->enabled || adev->shutdown)
> +		return -ENODEV;
> +
> +	prev_wptr = amdgpu_ih_get_wptr(adev, ih);
> +	/* Order wptr with rptr. */
> +	rmb();
> +	prev_rptr = READ_ONCE(ih->rptr);
> +	rptr_check = prev_rptr | (rptr_wrap << 32);
> +	wptr_check = prev_wptr | (wptr_wrap << 32);

Hui what? That check doesn't even make remotely sense to me.

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,