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

[Christian König]

Am 02.03.20 um 10:39 schrieb 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)

Ah, sorry! My incorrect thinking, kernel copies also don't use the CP 
but rather the SDMA. So the change is completely irrelevant for the copies.

Acked-by: Christian König <christian.koenig at amd.com> for the patch.

Regards,
Christian.

>
> 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);
>>    	}
>>