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 <deathsim...@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.ol...@amd.com> >>> >>> This can cause incorrect address calculations and hangs. >>> >>> v2: do it properly >>> >>> Cc: mesa-sta...@lists.freedesktop.org >>> Tested-and-Reviewed-by: Christian König <christian.koe...@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.daen...@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.koe...@amd.com> > > Regards, > Christian. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
