On Tue, Dec 8, 2015 at 4:44 PM, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On Tuesday, December 08, 2015 01:46:23 PM Jason Ekstrand wrote:
>> ---
>> src/mesa/drivers/dri/i965/brw_fs.h | 2 +-
>> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 106
>> ++++++++++++++-----------------
>> src/mesa/drivers/dri/i965/brw_nir.c | 45 ++++++++++---
>> 3 files changed, 84 insertions(+), 69 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h
>> b/src/mesa/drivers/dri/i965/brw_fs.h
>> index bca4589..b55589f 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.h
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
>> @@ -278,7 +278,7 @@ public:
>> unsigned stream_id);
>> void emit_gs_thread_end();
>> void emit_gs_input_load(const fs_reg &dst, const nir_src &vertex_src,
>> - const fs_reg &indirect_offset, unsigned
>> imm_offset,
>> + unsigned base_offset, const nir_src &offset_src,
>> unsigned num_components);
>> void emit_cs_terminate();
>> fs_reg *emit_cs_local_invocation_id_setup();
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> index 3754622..18c9d65 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> @@ -1603,29 +1603,31 @@ fs_visitor::emit_gs_vertex(const nir_src
>> &vertex_count_nir_src,
>> void
>> fs_visitor::emit_gs_input_load(const fs_reg &dst,
>> const nir_src &vertex_src,
>> - const fs_reg &indirect_offset,
>> - unsigned imm_offset,
>> + unsigned base_offset,
>> + const nir_src &offset_src,
>> unsigned num_components)
>> {
>> struct brw_gs_prog_data *gs_prog_data = (struct brw_gs_prog_data *)
>> prog_data;
>>
>> + nir_const_value *vertex_const = nir_src_as_const_value(vertex_src);
>> + nir_const_value *offset_const = nir_src_as_const_value(offset_src);
>> + const unsigned push_reg_count = gs_prog_data->base.urb_read_length * 8;
>> +
>> /* Offset 0 is the VUE header, which contains VARYING_SLOT_LAYER [.y],
>> * VARYING_SLOT_VIEWPORT [.z], and VARYING_SLOT_PSIZ [.w]. Only
>> * gl_PointSize is available as a GS input, however, so it must be that.
>> */
>> - const bool is_point_size =
>> - indirect_offset.file == BAD_FILE && imm_offset == 0;
>> -
>> - nir_const_value *vertex_const = nir_src_as_const_value(vertex_src);
>> - const unsigned push_reg_count = gs_prog_data->base.urb_read_length * 8;
>> + const bool is_point_size = (base_offset == 0);
>>
>> - if (indirect_offset.file == BAD_FILE && vertex_const != NULL &&
>> - 4 * imm_offset < push_reg_count) {
>> - imm_offset = 4 * imm_offset + vertex_const->u[0] * push_reg_count;
>> + if (offset_const != NULL && vertex_const != NULL &&
>> + 4 * offset_const->u[0] < push_reg_count) {
>
> This needs to be
>
> 4 * (base_offset + offset_const->u[0]) < push_reg_count
>
> otherwise we use the push model for inputs that aren't actually pushed,
> which breaks around 1500 piglit tests with INTEL_SCALAR_GS=1.
>
>> + int imm_offset = (base_offset + offset_const->u[0]) * 4 +
>> + vertex_const->u[0] * push_reg_count;
>> /* This input was pushed into registers. */
>> if (is_point_size) {
>> /* gl_PointSize comes in .w */
>> - bld.MOV(dst, fs_reg(ATTR, imm_offset + 3, dst.type));
>> + assert(imm_offset == 0);
>> + bld.MOV(dst, fs_reg(ATTR, 3, dst.type));
>
> This is wrong. imm_offset contains the offset derived from the vertex
> index, and gl_PointSize exists for multiple vertices. So it won't be 0.
>
> Please just remove this change, putting it back to:
>
> bld.MOV(dst, fs_reg(ATTR, imm_offset + 3, dst.type));
>
> That fixes assertion failures in 3 Piglit tests.
Fixed both locally. Thanks for the careful review and for watching
out for scalar GS!
> With that fixed, patches 1-8 and 11-12 are:
> Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
Thanks!
--Jason
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
