[Mesa-dev] radeonsi: don't set DATA_FORMAT if ADD_TID_ENABLE is set on VI

Submitted by Marek Olšák on Sept. 29, 2015, 2:54 p.m.

Details

Message ID 1443538463-23563-1-git-send-email-maraeo@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Marek Olšák Sept. 29, 2015, 2:54 p.m.
From: Marek Olšák <marek.olsak@amd.com>

This can cause incorrect address calculations and hangs.

Cc: mesa-stable@lists.freedesktop.org
---
 src/gallium/drivers/radeonsi/si_descriptors.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/radeonsi/si_descriptors.c b/src/gallium/drivers/radeonsi/si_descriptors.c
index b07ab3b..0ced6c3 100644
--- a/src/gallium/drivers/radeonsi/si_descriptors.c
+++ b/src/gallium/drivers/radeonsi/si_descriptors.c
@@ -619,11 +619,14 @@  void si_set_ring_buffer(struct pipe_context *ctx, uint shader, uint slot,
 			  S_008F0C_DST_SEL_Z(V_008F0C_SQ_SEL_Z) |
 			  S_008F0C_DST_SEL_W(V_008F0C_SQ_SEL_W) |
 			  S_008F0C_NUM_FORMAT(V_008F0C_BUF_NUM_FORMAT_FLOAT) |
-			  S_008F0C_DATA_FORMAT(V_008F0C_BUF_DATA_FORMAT_32) |
 			  S_008F0C_ELEMENT_SIZE(element_size) |
 			  S_008F0C_INDEX_STRIDE(index_stride) |
 			  S_008F0C_ADD_TID_ENABLE(add_tid);
 
+		/* If ADD_TID_ENABLE is set, DATA_FORMAT specifies STRIDE bits [14:17] */
+		if (sctx->b.chip_class >= VI && !add_tid)
+			desc[3] |= S_008F0C_DATA_FORMAT(V_008F0C_BUF_DATA_FORMAT_32);
+
 		pipe_resource_reference(&buffers->buffers[slot], buffer);
 		radeon_add_to_buffer_list(&sctx->b, &sctx->b.rings.gfx,
 				      (struct r600_resource*)buffer,

Comments

On 29.09.2015 23:54, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak@amd.com>
> 
> This can cause incorrect address calculations and hangs.
> 
> Cc: mesa-stable@lists.freedesktop.org

IIRC without "10.6 11.0" this only makes it a candidate for the 11.0
branch at this point[0]. Is that your intention?

[0] If so, maybe the rules should be changed such that no specific
    version means it's a candidate for all active stable branches?


> diff --git a/src/gallium/drivers/radeonsi/si_descriptors.c b/src/gallium/drivers/radeonsi/si_descriptors.c
> index b07ab3b..0ced6c3 100644
> --- a/src/gallium/drivers/radeonsi/si_descriptors.c
> +++ b/src/gallium/drivers/radeonsi/si_descriptors.c
> @@ -619,11 +619,14 @@ void si_set_ring_buffer(struct pipe_context *ctx, uint shader, uint slot,
>  			  S_008F0C_DST_SEL_Z(V_008F0C_SQ_SEL_Z) |
>  			  S_008F0C_DST_SEL_W(V_008F0C_SQ_SEL_W) |
>  			  S_008F0C_NUM_FORMAT(V_008F0C_BUF_NUM_FORMAT_FLOAT) |
> -			  S_008F0C_DATA_FORMAT(V_008F0C_BUF_DATA_FORMAT_32) |
>  			  S_008F0C_ELEMENT_SIZE(element_size) |
>  			  S_008F0C_INDEX_STRIDE(index_stride) |
>  			  S_008F0C_ADD_TID_ENABLE(add_tid);
>  
> +		/* If ADD_TID_ENABLE is set, DATA_FORMAT specifies STRIDE bits [14:17] */
> +		if (sctx->b.chip_class >= VI && !add_tid)
> +			desc[3] |= S_008F0C_DATA_FORMAT(V_008F0C_BUF_DATA_FORMAT_32);
> +
>  		pipe_resource_reference(&buffers->buffers[slot], buffer);
>  		radeon_add_to_buffer_list(&sctx->b, &sctx->b.rings.gfx,
>  				      (struct r600_resource*)buffer,
> 

Since the DATA_FORMAT behaviour only changed as of VI, I think this
should be:

		/* If ADD_TID_ENABLE is set, DATA_FORMAT specifies STRIDE bits [14:17] on VI */
		if (sctx->b.chip_class < VI || !add_tid)
			desc[3] |= S_008F0C_DATA_FORMAT(V_008F0C_BUF_DATA_FORMAT_32);
On Wed, Sep 30, 2015 at 5:51 AM, Michel Dänzer <michel@daenzer.net> wrote:
> On 29.09.2015 23:54, Marek Olšák wrote:
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> This can cause incorrect address calculations and hangs.
>>
>> Cc: mesa-stable@lists.freedesktop.org
>
> IIRC without "10.6 11.0" this only makes it a candidate for the 11.0
> branch at this point[0]. Is that your intention?

Yes, because VI is supported since 11.0.

>
> [0] If so, maybe the rules should be changed such that no specific
>     version means it's a candidate for all active stable branches?
>
>
>> diff --git a/src/gallium/drivers/radeonsi/si_descriptors.c b/src/gallium/drivers/radeonsi/si_descriptors.c
>> index b07ab3b..0ced6c3 100644
>> --- a/src/gallium/drivers/radeonsi/si_descriptors.c
>> +++ b/src/gallium/drivers/radeonsi/si_descriptors.c
>> @@ -619,11 +619,14 @@ void si_set_ring_buffer(struct pipe_context *ctx, uint shader, uint slot,
>>                         S_008F0C_DST_SEL_Z(V_008F0C_SQ_SEL_Z) |
>>                         S_008F0C_DST_SEL_W(V_008F0C_SQ_SEL_W) |
>>                         S_008F0C_NUM_FORMAT(V_008F0C_BUF_NUM_FORMAT_FLOAT) |
>> -                       S_008F0C_DATA_FORMAT(V_008F0C_BUF_DATA_FORMAT_32) |
>>                         S_008F0C_ELEMENT_SIZE(element_size) |
>>                         S_008F0C_INDEX_STRIDE(index_stride) |
>>                         S_008F0C_ADD_TID_ENABLE(add_tid);
>>
>> +             /* If ADD_TID_ENABLE is set, DATA_FORMAT specifies STRIDE bits [14:17] */
>> +             if (sctx->b.chip_class >= VI && !add_tid)
>> +                     desc[3] |= S_008F0C_DATA_FORMAT(V_008F0C_BUF_DATA_FORMAT_32);
>> +
>>               pipe_resource_reference(&buffers->buffers[slot], buffer);
>>               radeon_add_to_buffer_list(&sctx->b, &sctx->b.rings.gfx,
>>                                     (struct r600_resource*)buffer,
>>
>
> Since the DATA_FORMAT behaviour only changed as of VI, I think this
> should be:
>
>                 /* If ADD_TID_ENABLE is set, DATA_FORMAT specifies STRIDE bits [14:17] on VI */
>                 if (sctx->b.chip_class < VI || !add_tid)
>                         desc[3] |= S_008F0C_DATA_FORMAT(V_008F0C_BUF_DATA_FORMAT_32);

Thanks. I'll fix that.

Marek