[PATCH 1/3] drm/amdgpu: fix IB test MCBP bug

[Liu, Monk]

> -		if (!(ib->flags & AMDGPU_IB_FLAG_CE))
> +		if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)

Kernel copies also don't use a VMID, so I think that this won't work correctly.

Do you have the job available as well? That would probably be better to test for since only IB tests don't use a job.

[ML] 
The @job is not passed to the gfx_v10_0_ring_emit_ib_gfx() routine so I tend to modify as less as possible
Besides, for kernel copies: I believe they also do not support MCBP (can you point me the code in source of those kernel copies ?) , so use VMID to test is fine (To support MCBP on gfx ring you need a set of additional preamble IB co work with your workload iB and kernel copies apparently don't have those stuffs)

Thanks 
_____________________________________
Monk Liu|GPU Virtualization Team |AMD


-----Original Message-----
From: Christian König <ckoenig.leichtzumerken at gmail.com> 
Sent: Monday, March 2, 2020 5:35 PM
To: Liu, Monk <Monk.Liu at amd.com>; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH 1/3] drm/amdgpu: fix IB test MCBP bug

Am 02.03.20 um 10:22 schrieb Monk Liu:
> 1)for gfx IB test we shouldn't insert DE meta data
>
> 2)we should make sure IB test finished before we send event 3 to 
> hypervisor otherwise the IDLE from event 3 will preempt IB test, which 
> is not designed as a compatible structure for MCBP
>
> Signed-off-by: Monk Liu <Monk.Liu at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 ++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    | 3 ---
>   drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c     | 2 +-
>   drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c      | 2 +-
>   drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c      | 2 +-
>   5 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 351096a..572eb6e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3195,6 +3195,12 @@ void amdgpu_device_fini(struct amdgpu_device *adev)
>   	flush_delayed_work(&adev->delayed_init_work);
>   	adev->shutdown = true;
>   
> +	/* make sure IB test finished before entering exclusive mode
> +	 * to avoid preemption on IB test
> +	 * */
> +	if (amdgpu_sriov_vf(adev))
> +		amdgpu_virt_request_full_gpu(adev, false);
> +
>   	/* disable all interrupts */
>   	amdgpu_irq_disable_all(adev);
>   	if (adev->mode_info.mode_config_initialized){
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 0f35639..0b1511a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -88,9 +88,6 @@ void amdgpu_driver_unload_kms(struct drm_device *dev)
>   	if (adev->rmmio == NULL)
>   		goto done_free;
>   
> -	if (amdgpu_sriov_vf(adev))
> -		amdgpu_virt_request_full_gpu(adev, false);
> -
>   	if (adev->runpm) {
>   		pm_runtime_get_sync(dev->dev);
>   		pm_runtime_forbid(dev->dev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> index 94ca9ff..0555989 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> @@ -4432,7 +4432,7 @@ static void gfx_v10_0_ring_emit_ib_gfx(struct amdgpu_ring *ring,
>   		if (flags & AMDGPU_IB_PREEMPTED)
>   			control |= INDIRECT_BUFFER_PRE_RESUME(1);
>   
> -		if (!(ib->flags & AMDGPU_IB_FLAG_CE))
> +		if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)
>   			gfx_v10_0_ring_emit_de_meta(ring,
>   				    (!amdgpu_sriov_vf(ring->adev) && flags & AMDGPU_IB_PREEMPTED) ? true : false);
>   	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> index 393a132..b14f46a3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> @@ -6116,7 +6116,7 @@ static void gfx_v8_0_ring_emit_ib_gfx(struct amdgpu_ring *ring,
>   	if (amdgpu_sriov_vf(ring->adev) && (ib->flags & AMDGPU_IB_FLAG_PREEMPT)) {
>   		control |= INDIRECT_BUFFER_PRE_ENB(1);
>   
> -		if (!(ib->flags & AMDGPU_IB_FLAG_CE))
> +		if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)

Kernel copies also don't use a VMID, so I think that this won't work correctly.

Do you have the job available as well? That would probably be better to test for since only IB tests don't use a job.

Christian.

>   			gfx_v8_0_ring_emit_de_meta(ring);
>   	}
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 0156479..d8d256e6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -4985,7 +4985,7 @@ static void gfx_v9_0_ring_emit_ib_gfx(struct amdgpu_ring *ring,
>   	if (amdgpu_sriov_vf(ring->adev) && (ib->flags & AMDGPU_IB_FLAG_PREEMPT)) {
>   		control |= INDIRECT_BUFFER_PRE_ENB(1);
>   
> -		if (!(ib->flags & AMDGPU_IB_FLAG_CE))
> +		if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)
>   			gfx_v9_0_ring_emit_de_meta(ring);
>   	}
>