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

Submitted by Liu, Monk on Sept. 30, 2016, 5 a.m.

Details

Message ID 1475211608-17679-1-git-send-email-Monk.Liu@amd.com
State New
Headers show
Series "drm/amdgpu:fix exclusive mode game texture blank(v2)" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Liu, Monk Sept. 30, 2016, 5 a.m.
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(+)

Patch hide | download patch | download mbox

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

Comments

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