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

Submitted by Marek Olšák on Sept. 30, 2015, 7:11 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Marek Olšák Sept. 30, 2015, 7:11 p.m.
From: Marek Olšák <marek.olsak@amd.com>

This can cause incorrect address calculations and hangs.

v2: do it properly

Cc: mesa-stable@lists.freedesktop.org
Tested-and-Reviewed-by: Christian König <christian.koenig@amd.com>
---
 src/gallium/drivers/radeonsi/si_descriptors.c | 9 ++++++++-
 1 file changed, 8 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..b56219a 100644
--- a/src/gallium/drivers/radeonsi/si_descriptors.c
+++ b/src/gallium/drivers/radeonsi/si_descriptors.c
@@ -619,11 +619,18 @@  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 on VI, DATA_FORMAT specifies
+		 * STRIDE bits [14:17]
+		 */
+		if (sctx->b.chip_class >= VI && add_tid)
+			desc[3] |= S_008F0C_DATA_FORMAT(stride >> 14);
+		else
+			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 01.10.2015 04:11, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak@amd.com>
> 
> This can cause incorrect address calculations and hangs.
> 
> v2: do it properly
> 
> Cc: mesa-stable@lists.freedesktop.org
> Tested-and-Reviewed-by: Christian König <christian.koenig@amd.com>
> ---
>  src/gallium/drivers/radeonsi/si_descriptors.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/gallium/drivers/radeonsi/si_descriptors.c b/src/gallium/drivers/radeonsi/si_descriptors.c
> index b07ab3b..b56219a 100644
> --- a/src/gallium/drivers/radeonsi/si_descriptors.c
> +++ b/src/gallium/drivers/radeonsi/si_descriptors.c
> @@ -619,11 +619,18 @@ 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 on VI, DATA_FORMAT specifies
> +		 * STRIDE bits [14:17]
> +		 */
> +		if (sctx->b.chip_class >= VI && add_tid)
> +			desc[3] |= S_008F0C_DATA_FORMAT(stride >> 14);
> +		else
> +			desc[3] |= S_008F0C_DATA_FORMAT(V_008F0C_BUF_DATA_FORMAT_32);

The beginning of the function has:

	/* The stride field in the resource descriptor has 14 bits */
	assert(stride < (1 << 14));

So the if-branch is dead code in a non-release build. Would be nice if
that could be reconciled somehow, but I'm fine with doing it in a
follow-up change. Either way, this change is

Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>
On 01.10.2015 05:44, Michel Dänzer wrote:
> On 01.10.2015 04:11, Marek Olšák wrote:
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> This can cause incorrect address calculations and hangs.
>>
>> v2: do it properly
>>
>> Cc: mesa-stable@lists.freedesktop.org
>> Tested-and-Reviewed-by: Christian König <christian.koenig@amd.com>
>> ---
>>   src/gallium/drivers/radeonsi/si_descriptors.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/drivers/radeonsi/si_descriptors.c b/src/gallium/drivers/radeonsi/si_descriptors.c
>> index b07ab3b..b56219a 100644
>> --- a/src/gallium/drivers/radeonsi/si_descriptors.c
>> +++ b/src/gallium/drivers/radeonsi/si_descriptors.c
>> @@ -619,11 +619,18 @@ 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 on VI, DATA_FORMAT specifies
>> +		 * STRIDE bits [14:17]
>> +		 */
>> +		if (sctx->b.chip_class >= VI && add_tid)
>> +			desc[3] |= S_008F0C_DATA_FORMAT(stride >> 14);
>> +		else
>> +			desc[3] |= S_008F0C_DATA_FORMAT(V_008F0C_BUF_DATA_FORMAT_32);
> The beginning of the function has:
>
> 	/* The stride field in the resource descriptor has 14 bits */
> 	assert(stride < (1 << 14));
>
> So the if-branch is dead code in a non-release build. Would be nice if
> that could be reconciled somehow, but I'm fine with doing it in a
> follow-up change. Either way, this change is
>
> Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>

Yeah, agree. Might be nice if someone can come up with a test for this, 
but I don't think this is absolutely necessary.

For now the patch is Reviewed-by: Christian König <christian.koenig@amd.com>

Regards,
Christian.
After some testing, I realized this patch causes problems. DATA_FORMAT
with ADD_TID=1 means STRIDE[14:17] only if the instruction is untyped
MUBUF, but Mesa only uses ADD_TID with typed MUBUF (tbuffer in
particular).

Marek

On Thu, Oct 1, 2015 at 10:27 AM, Christian König
<deathsimple@vodafone.de> wrote:
> On 01.10.2015 05:44, Michel Dänzer wrote:
>>
>> On 01.10.2015 04:11, Marek Olšák wrote:
>>>
>>> From: Marek Olšák <marek.olsak@amd.com>
>>>
>>> This can cause incorrect address calculations and hangs.
>>>
>>> v2: do it properly
>>>
>>> Cc: mesa-stable@lists.freedesktop.org
>>> Tested-and-Reviewed-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   src/gallium/drivers/radeonsi/si_descriptors.c | 9 ++++++++-
>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/gallium/drivers/radeonsi/si_descriptors.c
>>> b/src/gallium/drivers/radeonsi/si_descriptors.c
>>> index b07ab3b..b56219a 100644
>>> --- a/src/gallium/drivers/radeonsi/si_descriptors.c
>>> +++ b/src/gallium/drivers/radeonsi/si_descriptors.c
>>> @@ -619,11 +619,18 @@ 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 on VI, DATA_FORMAT specifies
>>> +                * STRIDE bits [14:17]
>>> +                */
>>> +               if (sctx->b.chip_class >= VI && add_tid)
>>> +                       desc[3] |= S_008F0C_DATA_FORMAT(stride >> 14);
>>> +               else
>>> +                       desc[3] |=
>>> S_008F0C_DATA_FORMAT(V_008F0C_BUF_DATA_FORMAT_32);
>>
>> The beginning of the function has:
>>
>>         /* The stride field in the resource descriptor has 14 bits */
>>         assert(stride < (1 << 14));
>>
>> So the if-branch is dead code in a non-release build. Would be nice if
>> that could be reconciled somehow, but I'm fine with doing it in a
>> follow-up change. Either way, this change is
>>
>> Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>
>
>
> Yeah, agree. Might be nice if someone can come up with a test for this, but
> I don't think this is absolutely necessary.
>
> For now the patch is Reviewed-by: Christian König <christian.koenig@amd.com>
>
> Regards,
> Christian.