[PATCH] drm/amdgpu:fix exclusive mode game texture blank(v2)

[PATCH] drm/amdgpu:fix exclusive mode game texture blank(v2)

[Monk Liu]

this fix DOTA and other exclusive & full screen game
textrue blank bug, the root cause is that when no ctx
switch between two DMAframe, CE go too faster and step
to the next DMAframe, thus DE and CE are not working on
the same DAMframe.

Change-Id: I92714a1d434bb05e94220a2db9b4a6113b0c2efc
Signed-off-by: Monk Liu <Monk.Liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
index 38261a0..4863426 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -129,6 +129,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
 
 	unsigned i;
 	int r = 0;
+	unsigned extra_nop = 0;
 
 	if (num_ibs == 0)
 		return -EINVAL;
@@ -155,6 +156,10 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
 	alloc_size = amdgpu_ring_get_dma_frame_size(ring) +
 		num_ibs * amdgpu_ring_get_emit_ib_size(ring);
 
+	if (job && !job->vm_needs_flush && ring->type == AMDGPU_RING_TYPE_GFX) {
+		extra_nop = 128;
+	}
+
 	r = amdgpu_ring_alloc(ring, alloc_size);
 	if (r) {
 		dev_err(adev->dev, "scheduling IB failed (%d).\n", r);
@@ -165,6 +170,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
 		patch_offset = amdgpu_ring_init_cond_exec(ring);
 
 	if (vm) {
+		amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE go too fast than DE */
+
 		r = amdgpu_vm_flush(ring, job);
 		if (r) {
 			amdgpu_ring_undo(ring);
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu:fix exclusive mode game texture blank(v2)

[Zhang, Hawking]

+	if (job && !job->vm_needs_flush && ring->type == AMDGPU_RING_TYPE_GFX) {
+		extra_nop = 128;
+	}
+
Please remove the redundant braces here. With that fixed, the patch is
Reviewed-by: Hawking Zhang <Hawking.Zhang@amd.com>

Regards,
Hawking

-----Original Message-----
From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Monk Liu
Sent: Friday, September 30, 2016 13:00
To: amd-gfx@lists.freedesktop.org
Cc: Liu, Monk <Monk.Liu@amd.com>
Subject: [PATCH] drm/amdgpu:fix exclusive mode game texture blank(v2)

this fix DOTA and other exclusive & full screen game textrue blank bug, the root cause is that when no ctx switch between two DMAframe, CE go too faster and step to the next DMAframe, thus DE and CE are not working on the same DAMframe.

Change-Id: I92714a1d434bb05e94220a2db9b4a6113b0c2efc
Signed-off-by: Monk Liu <Monk.Liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
index 38261a0..4863426 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -129,6 +129,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
 
 	unsigned i;
 	int r = 0;
+	unsigned extra_nop = 0;
 
 	if (num_ibs == 0)
 		return -EINVAL;
@@ -155,6 +156,10 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
 	alloc_size = amdgpu_ring_get_dma_frame_size(ring) +
 		num_ibs * amdgpu_ring_get_emit_ib_size(ring);
 
+	if (job && !job->vm_needs_flush && ring->type == AMDGPU_RING_TYPE_GFX) {
+		extra_nop = 128;
+	}
+
 	r = amdgpu_ring_alloc(ring, alloc_size);
 	if (r) {
 		dev_err(adev->dev, "scheduling IB failed (%d).\n", r); @@ -165,6 +170,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
 		patch_offset = amdgpu_ring_init_cond_exec(ring);
 
 	if (vm) {
+		amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE go too fast 
+than DE */
+
 		r = amdgpu_vm_flush(ring, job);
 		if (r) {
 			amdgpu_ring_undo(ring);
--
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu:fix exclusive mode game texture blank(v2)

[Christian König]

NAK, please stop adding workarounds like this into common code.

If we need to add the extra NOPs between each command submission than 
just do this in one of the existing IP specific callbacks.

Additional to that this sounds more and more like the removal of the 
double sync wasn't such a good idea.

Christian.

Am 30.09.2016 um 07:00 schrieb Monk Liu:
> this fix DOTA and other exclusive & full screen game
> textrue blank bug, the root cause is that when no ctx
> switch between two DMAframe, CE go too faster and step
> to the next DMAframe, thus DE and CE are not working on
> the same DAMframe.
>
> Change-Id: I92714a1d434bb05e94220a2db9b4a6113b0c2efc
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index 38261a0..4863426 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -129,6 +129,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   
>   	unsigned i;
>   	int r = 0;
> +	unsigned extra_nop = 0;
>   
>   	if (num_ibs == 0)
>   		return -EINVAL;
> @@ -155,6 +156,10 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   	alloc_size = amdgpu_ring_get_dma_frame_size(ring) +
>   		num_ibs * amdgpu_ring_get_emit_ib_size(ring);
>   
> +	if (job && !job->vm_needs_flush && ring->type == AMDGPU_RING_TYPE_GFX) {
> +		extra_nop = 128;
> +	}
> +
>   	r = amdgpu_ring_alloc(ring, alloc_size);
>   	if (r) {
>   		dev_err(adev->dev, "scheduling IB failed (%d).\n", r);
> @@ -165,6 +170,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   		patch_offset = amdgpu_ring_init_cond_exec(ring);
>   
>   	if (vm) {
> +		amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE go too fast than DE */
> +
>   		r = amdgpu_vm_flush(ring, job);
>   		if (r) {
>   			amdgpu_ring_undo(ring);


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu:fix exclusive mode game texture blank(v2)

[Christian König]

Thinking more about this. I assume that windows doesn't run into this 
problem because of the larger padding for the GFX ring, doesn't they?

Well what do you think about just increasing the padding to what the 
windows KMD is using then?

Regards,
Christian.

Am 30.09.2016 um 11:06 schrieb Christian König:
> NAK, please stop adding workarounds like this into common code.
>
> If we need to add the extra NOPs between each command submission than 
> just do this in one of the existing IP specific callbacks.
>
> Additional to that this sounds more and more like the removal of the 
> double sync wasn't such a good idea.
>
> Christian.
>
> Am 30.09.2016 um 07:00 schrieb Monk Liu:
>> this fix DOTA and other exclusive & full screen game
>> textrue blank bug, the root cause is that when no ctx
>> switch between two DMAframe, CE go too faster and step
>> to the next DMAframe, thus DE and CE are not working on
>> the same DAMframe.
>>
>> Change-Id: I92714a1d434bb05e94220a2db9b4a6113b0c2efc
>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> index 38261a0..4863426 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> @@ -129,6 +129,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, 
>> unsigned num_ibs,
>>         unsigned i;
>>       int r = 0;
>> +    unsigned extra_nop = 0;
>>         if (num_ibs == 0)
>>           return -EINVAL;
>> @@ -155,6 +156,10 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, 
>> unsigned num_ibs,
>>       alloc_size = amdgpu_ring_get_dma_frame_size(ring) +
>>           num_ibs * amdgpu_ring_get_emit_ib_size(ring);
>>   +    if (job && !job->vm_needs_flush && ring->type == 
>> AMDGPU_RING_TYPE_GFX) {
>> +        extra_nop = 128;
>> +    }
>> +
>>       r = amdgpu_ring_alloc(ring, alloc_size);
>>       if (r) {
>>           dev_err(adev->dev, "scheduling IB failed (%d).\n", r);
>> @@ -165,6 +170,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, 
>> unsigned num_ibs,
>>           patch_offset = amdgpu_ring_init_cond_exec(ring);
>>         if (vm) {
>> +        amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE go 
>> too fast than DE */
>> +
>>           r = amdgpu_vm_flush(ring, job);
>>           if (r) {
>>               amdgpu_ring_undo(ring);
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu:fix exclusive mode game texture blank(v2)

[Liu, Monk]

What's your suggestion,

The thing is that when no vm_flush() needed we also have no 128dw nop inserted so the CE goes to fast to the next CS, since CE already dealing with SWITCH_BUFFERs, so CE and DE work in the different buffer and lead to corruption 
The fix for it is that with or without VM_FLUSH, we always need that 128dw nop inserted in head of each DMAframe , (windows use padding so it always have 128dw nop after each SWITCH_BUFFERS)

BR Monk

-----Original Message-----
From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Christian K?nig
Sent: Friday, September 30, 2016 5:07 PM
To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu:fix exclusive mode game texture blank(v2)

NAK, please stop adding workarounds like this into common code.

If we need to add the extra NOPs between each command submission than just do this in one of the existing IP specific callbacks.

Additional to that this sounds more and more like the removal of the double sync wasn't such a good idea.

Christian.

Am 30.09.2016 um 07:00 schrieb Monk Liu:
> this fix DOTA and other exclusive & full screen game textrue blank 
> bug, the root cause is that when no ctx switch between two DMAframe, 
> CE go too faster and step to the next DMAframe, thus DE and CE are not 
> working on the same DAMframe.
>
> Change-Id: I92714a1d434bb05e94220a2db9b4a6113b0c2efc
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index 38261a0..4863426 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -129,6 +129,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, 
> unsigned num_ibs,
>   
>   	unsigned i;
>   	int r = 0;
> +	unsigned extra_nop = 0;
>   
>   	if (num_ibs == 0)
>   		return -EINVAL;
> @@ -155,6 +156,10 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   	alloc_size = amdgpu_ring_get_dma_frame_size(ring) +
>   		num_ibs * amdgpu_ring_get_emit_ib_size(ring);
>   
> +	if (job && !job->vm_needs_flush && ring->type == AMDGPU_RING_TYPE_GFX) {
> +		extra_nop = 128;
> +	}
> +
>   	r = amdgpu_ring_alloc(ring, alloc_size);
>   	if (r) {
>   		dev_err(adev->dev, "scheduling IB failed (%d).\n", r); @@ -165,6 
> +170,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   		patch_offset = amdgpu_ring_init_cond_exec(ring);
>   
>   	if (vm) {
> +		amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE go too fast 
> +than DE */
> +
>   		r = amdgpu_vm_flush(ring, job);
>   		if (r) {
>   			amdgpu_ring_undo(ring);


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu:fix exclusive mode game texture blank(v2)

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Monk Liu
> Sent: Friday, September 30, 2016 1:00 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Liu, Monk
> Subject: [PATCH] drm/amdgpu:fix exclusive mode game texture blank(v2)
> 
> this fix DOTA and other exclusive & full screen game
> textrue blank bug, the root cause is that when no ctx
> switch between two DMAframe, CE go too faster and step
> to the next DMAframe, thus DE and CE are not working on
> the same DAMframe.
> 
> Change-Id: I92714a1d434bb05e94220a2db9b4a6113b0c2efc
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index 38261a0..4863426 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -129,6 +129,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring,
> unsigned num_ibs,
> 
>  	unsigned i;
>  	int r = 0;
> +	unsigned extra_nop = 0;
> 
>  	if (num_ibs == 0)
>  		return -EINVAL;
> @@ -155,6 +156,10 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring,
> unsigned num_ibs,
>  	alloc_size = amdgpu_ring_get_dma_frame_size(ring) +
>  		num_ibs * amdgpu_ring_get_emit_ib_size(ring);
> 
> +	if (job && !job->vm_needs_flush && ring->type ==
> AMDGPU_RING_TYPE_GFX) {
> +		extra_nop = 128;
> +	}

Don't you need to increase the alloc_size?  Also, can we add this to the asic specific gfx code?

> +
>  	r = amdgpu_ring_alloc(ring, alloc_size);
>  	if (r) {
>  		dev_err(adev->dev, "scheduling IB failed (%d).\n", r);
> @@ -165,6 +170,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring,
> unsigned num_ibs,
>  		patch_offset = amdgpu_ring_init_cond_exec(ring);
> 
>  	if (vm) {
> +		amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE go
> too fast than DE */
> +
>  		r = amdgpu_vm_flush(ring, job);
>  		if (r) {
>  			amdgpu_ring_undo(ring);
> --
> 1.9.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu:fix exclusive mode game texture blank(v2)

[Liu, Monk]

Already pushed to staging because Pro team are kinda hurry for this fix

The alloc size isn't need changed, cuz it always count vm_flush() in event no vm_flush needed

BR Monk

-----Original Message-----
From: Deucher, Alexander 
Sent: Friday, September 30, 2016 9:23 PM
To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Liu, Monk <Monk.Liu@amd.com>
Subject: RE: [PATCH] drm/amdgpu:fix exclusive mode game texture blank(v2)

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf 
> Of Monk Liu
> Sent: Friday, September 30, 2016 1:00 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Liu, Monk
> Subject: [PATCH] drm/amdgpu:fix exclusive mode game texture blank(v2)
> 
> this fix DOTA and other exclusive & full screen game textrue blank 
> bug, the root cause is that when no ctx switch between two DMAframe, 
> CE go too faster and step to the next DMAframe, thus DE and CE are not 
> working on the same DAMframe.
> 
> Change-Id: I92714a1d434bb05e94220a2db9b4a6113b0c2efc
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index 38261a0..4863426 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -129,6 +129,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, 
> unsigned num_ibs,
> 
>  	unsigned i;
>  	int r = 0;
> +	unsigned extra_nop = 0;
> 
>  	if (num_ibs == 0)
>  		return -EINVAL;
> @@ -155,6 +156,10 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, 
> unsigned num_ibs,
>  	alloc_size = amdgpu_ring_get_dma_frame_size(ring) +
>  		num_ibs * amdgpu_ring_get_emit_ib_size(ring);
> 
> +	if (job && !job->vm_needs_flush && ring->type ==
> AMDGPU_RING_TYPE_GFX) {
> +		extra_nop = 128;
> +	}

Don't you need to increase the alloc_size?  Also, can we add this to the asic specific gfx code?

> +
>  	r = amdgpu_ring_alloc(ring, alloc_size);
>  	if (r) {
>  		dev_err(adev->dev, "scheduling IB failed (%d).\n", r); @@ -165,6 
> +170,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned 
> num_ibs,
>  		patch_offset = amdgpu_ring_init_cond_exec(ring);
> 
>  	if (vm) {
> +		amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE go
> too fast than DE */
> +
>  		r = amdgpu_vm_flush(ring, job);
>  		if (r) {
>  			amdgpu_ring_undo(ring);
> --
> 1.9.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx