Re: [PATCH] drm/amdgpu: move kiq_reg_write_reg_wait() out of amdgpu_virt.c

[Christian König <christian.koenig@xxxxxxx>]

Am 08.01.24 um 22:38 schrieb Alex Deucher:
> It's used for more than just SR-IOV now, so move it to
> amdgpu_gmc.c and rename it to better match the functionality and
> update the comments in the code paths to better document
> when each path is used and why.  No functional change.
>
> Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
> Cc: Shaoyun.Liu@xxxxxxx
> Cc: Christian.Koenig@xxxxxxx

I'm wondering if having an amdgpu_kiq.c wouldn't be better, but for now 
the patch is Acked-by: Christian König <Christian.koenig@xxxxxxx>

Regards,
Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c  | 53 ++++++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h  |  4 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 53 ------------------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h |  4 --
>   drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c   |  9 ++--
>   drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c   |  9 ++--
>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c    | 12 +++---
>   7 files changed, 74 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index d2f273d77e59..331cf6384b12 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -746,6 +746,59 @@ int amdgpu_gmc_flush_gpu_tlb_pasid(struct amdgpu_device *adev, uint16_t pasid,
>   	return r;
>   }
>   
> +void amdgpu_gmc_fw_reg_write_reg_wait(struct amdgpu_device *adev,
> +				      uint32_t reg0, uint32_t reg1,
> +				      uint32_t ref, uint32_t mask,
> +				      uint32_t xcc_inst)
> +{
> +	struct amdgpu_kiq *kiq = &adev->gfx.kiq[xcc_inst];
> +	struct amdgpu_ring *ring = &kiq->ring;
> +	signed long r, cnt = 0;
> +	unsigned long flags;
> +	uint32_t seq;
> +
> +	if (adev->mes.ring.sched.ready) {
> +		amdgpu_mes_reg_write_reg_wait(adev, reg0, reg1,
> +					      ref, mask);
> +		return;
> +	}
> +
> +	spin_lock_irqsave(&kiq->ring_lock, flags);
> +	amdgpu_ring_alloc(ring, 32);
> +	amdgpu_ring_emit_reg_write_reg_wait(ring, reg0, reg1,
> +					    ref, mask);
> +	r = amdgpu_fence_emit_polling(ring, &seq, MAX_KIQ_REG_WAIT);
> +	if (r)
> +		goto failed_undo;
> +
> +	amdgpu_ring_commit(ring);
> +	spin_unlock_irqrestore(&kiq->ring_lock, flags);
> +
> +	r = amdgpu_fence_wait_polling(ring, seq, MAX_KIQ_REG_WAIT);
> +
> +	/* don't wait anymore for IRQ context */
> +	if (r < 1 && in_interrupt())
> +		goto failed_kiq;
> +
> +	might_sleep();
> +	while (r < 1 && cnt++ < MAX_KIQ_REG_TRY) {
> +
> +		msleep(MAX_KIQ_REG_BAILOUT_INTERVAL);
> +		r = amdgpu_fence_wait_polling(ring, seq, MAX_KIQ_REG_WAIT);
> +	}
> +
> +	if (cnt > MAX_KIQ_REG_TRY)
> +		goto failed_kiq;
> +
> +	return;
> +
> +failed_undo:
> +	amdgpu_ring_undo(ring);
> +	spin_unlock_irqrestore(&kiq->ring_lock, flags);
> +failed_kiq:
> +	dev_err(adev->dev, "failed to write reg %x wait reg %x\n", reg0, reg1);
> +}
> +
>   /**
>    * amdgpu_gmc_tmz_set -- check and set if a device supports TMZ
>    * @adev: amdgpu_device pointer
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> index e699d1ca8deb..17f40ea1104b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> @@ -417,6 +417,10 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>   int amdgpu_gmc_flush_gpu_tlb_pasid(struct amdgpu_device *adev, uint16_t pasid,
>   				   uint32_t flush_type, bool all_hub,
>   				   uint32_t inst);
> +void amdgpu_gmc_fw_reg_write_reg_wait(struct amdgpu_device *adev,
> +				      uint32_t reg0, uint32_t reg1,
> +				      uint32_t ref, uint32_t mask,
> +				      uint32_t xcc_inst);
>   
>   extern void amdgpu_gmc_tmz_set(struct amdgpu_device *adev);
>   extern void amdgpu_gmc_noretry_set(struct amdgpu_device *adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index 0dcff2889e25..f5c66e0038b5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -71,59 +71,6 @@ void amdgpu_virt_init_setting(struct amdgpu_device *adev)
>   		amdgpu_num_kcq = 2;
>   }
>   
> -void amdgpu_virt_kiq_reg_write_reg_wait(struct amdgpu_device *adev,
> -					uint32_t reg0, uint32_t reg1,
> -					uint32_t ref, uint32_t mask,
> -					uint32_t xcc_inst)
> -{
> -	struct amdgpu_kiq *kiq = &adev->gfx.kiq[xcc_inst];
> -	struct amdgpu_ring *ring = &kiq->ring;
> -	signed long r, cnt = 0;
> -	unsigned long flags;
> -	uint32_t seq;
> -
> -	if (adev->mes.ring.sched.ready) {
> -		amdgpu_mes_reg_write_reg_wait(adev, reg0, reg1,
> -					      ref, mask);
> -		return;
> -	}
> -
> -	spin_lock_irqsave(&kiq->ring_lock, flags);
> -	amdgpu_ring_alloc(ring, 32);
> -	amdgpu_ring_emit_reg_write_reg_wait(ring, reg0, reg1,
> -					    ref, mask);
> -	r = amdgpu_fence_emit_polling(ring, &seq, MAX_KIQ_REG_WAIT);
> -	if (r)
> -		goto failed_undo;
> -
> -	amdgpu_ring_commit(ring);
> -	spin_unlock_irqrestore(&kiq->ring_lock, flags);
> -
> -	r = amdgpu_fence_wait_polling(ring, seq, MAX_KIQ_REG_WAIT);
> -
> -	/* don't wait anymore for IRQ context */
> -	if (r < 1 && in_interrupt())
> -		goto failed_kiq;
> -
> -	might_sleep();
> -	while (r < 1 && cnt++ < MAX_KIQ_REG_TRY) {
> -
> -		msleep(MAX_KIQ_REG_BAILOUT_INTERVAL);
> -		r = amdgpu_fence_wait_polling(ring, seq, MAX_KIQ_REG_WAIT);
> -	}
> -
> -	if (cnt > MAX_KIQ_REG_TRY)
> -		goto failed_kiq;
> -
> -	return;
> -
> -failed_undo:
> -	amdgpu_ring_undo(ring);
> -	spin_unlock_irqrestore(&kiq->ring_lock, flags);
> -failed_kiq:
> -	dev_err(adev->dev, "failed to write reg %x wait reg %x\n", reg0, reg1);
> -}
> -
>   /**
>    * amdgpu_virt_request_full_gpu() - request full gpu access
>    * @adev:	amdgpu device.
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> index d4207e44141f..1b49c007ff62 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> @@ -332,10 +332,6 @@ static inline bool is_virtual_machine(void)
>   	((adev)->virt.gim_feature & AMDGIM_FEATURE_VCN_RB_DECOUPLE)
>   bool amdgpu_virt_mmio_blocked(struct amdgpu_device *adev);
>   void amdgpu_virt_init_setting(struct amdgpu_device *adev);
> -void amdgpu_virt_kiq_reg_write_reg_wait(struct amdgpu_device *adev,
> -					uint32_t reg0, uint32_t rreg1,
> -					uint32_t ref, uint32_t mask,
> -					uint32_t xcc_inst);
>   int amdgpu_virt_request_full_gpu(struct amdgpu_device *adev, bool init);
>   int amdgpu_virt_release_full_gpu(struct amdgpu_device *adev, bool init);
>   int amdgpu_virt_reset_gpu(struct amdgpu_device *adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> index 6c5185608854..db89d13bd80d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> @@ -262,16 +262,17 @@ static void gmc_v10_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>   	/* flush hdp cache */
>   	adev->hdp.funcs->flush_hdp(adev, NULL);
>   
> -	/* For SRIOV run time, driver shouldn't access the register through MMIO
> -	 * Directly use kiq to do the vm invalidation instead
> +	/* This is necessary for SRIOV as well as for GFXOFF to function
> +	 * properly under bare metal
>   	 */
>   	if (adev->gfx.kiq[0].ring.sched.ready && !adev->enable_mes &&
>   	    (amdgpu_sriov_runtime(adev) || !amdgpu_sriov_vf(adev))) {
> -		amdgpu_virt_kiq_reg_write_reg_wait(adev, req, ack, inv_req,
> -				1 << vmid, GET_INST(GC, 0));
> +		amdgpu_gmc_fw_reg_write_reg_wait(adev, req, ack, inv_req,
> +						 1 << vmid, GET_INST(GC, 0));
>   		return;
>   	}
>   
> +	/* This path is needed before KIQ/MES/GFXOFF are set up */
>   	hub_ip = (vmhub == AMDGPU_GFXHUB(0)) ? GC_HWIP : MMHUB_HWIP;
>   
>   	spin_lock(&adev->gmc.invalidate_lock);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> index c9c653cfc765..6c68135cac9f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> @@ -223,16 +223,17 @@ static void gmc_v11_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>   	/* flush hdp cache */
>   	adev->hdp.funcs->flush_hdp(adev, NULL);
>   
> -	/* For SRIOV run time, driver shouldn't access the register through MMIO
> -	 * Directly use kiq to do the vm invalidation instead
> +	/* This is necessary for SRIOV as well as for GFXOFF to function
> +	 * properly under bare metal
>   	 */
>   	if ((adev->gfx.kiq[0].ring.sched.ready || adev->mes.ring.sched.ready) &&
>   	    (amdgpu_sriov_runtime(adev) || !amdgpu_sriov_vf(adev))) {
> -		amdgpu_virt_kiq_reg_write_reg_wait(adev, req, ack, inv_req,
> -				1 << vmid, GET_INST(GC, 0));
> +		amdgpu_gmc_fw_reg_write_reg_wait(adev, req, ack, inv_req,
> +						 1 << vmid, GET_INST(GC, 0));
>   		return;
>   	}
>   
> +	/* This path is needed before KIQ/MES/GFXOFF are set up */
>   	hub_ip = (vmhub == AMDGPU_GFXHUB(0)) ? GC_HWIP : MMHUB_HWIP;
>   
>   	spin_lock(&adev->gmc.invalidate_lock);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index f9039d64ff2d..9bff72356a37 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -829,23 +829,25 @@ static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>   	req = hub->vm_inv_eng0_req + hub->eng_distance * eng;
>   	ack = hub->vm_inv_eng0_ack + hub->eng_distance * eng;
>   
> -	/* This is necessary for a HW workaround under SRIOV as well
> -	 * as GFXOFF under bare metal
> -	 */
>   	if (vmhub >= AMDGPU_MMHUB0(0))
>   		inst = GET_INST(GC, 0);
>   	else
>   		inst = vmhub;
> +
> +	/* This is necessary for SRIOV as well as for GFXOFF to function
> +	 * properly under bare metal
> +	 */
>   	if (adev->gfx.kiq[inst].ring.sched.ready &&
>   	    (amdgpu_sriov_runtime(adev) || !amdgpu_sriov_vf(adev))) {
>   		uint32_t req = hub->vm_inv_eng0_req + hub->eng_distance * eng;
>   		uint32_t ack = hub->vm_inv_eng0_ack + hub->eng_distance * eng;
>   
> -		amdgpu_virt_kiq_reg_write_reg_wait(adev, req, ack, inv_req,
> -						   1 << vmid, inst);
> +		amdgpu_gmc_fw_reg_write_reg_wait(adev, req, ack, inv_req,
> +						 1 << vmid, inst);
>   		return;
>   	}
>   
> +	/* This path is needed before KIQ/MES/GFXOFF are set up */
>   	spin_lock(&adev->gmc.invalidate_lock);
>   
>   	/*