From 5e584a9db7f2900a8ed13575098940b79c84d98b Mon Sep 17 00:00:00 2001 From: Iago Toral Quiroga Date: Wed, 20 Sep 2017 09:22:51 +0200 Subject: [PATCH] i965: skip reading unused slots at the begining of the URB for the FS We can start reading the URB at the first offset that contains varyings that are actually read in the URB. We still need to make sure that we read at least one varying to honor hardware requirements. This helps alleviate a problem introduced with 99df02ca26f61 for separate shader objects: without separate shader objects we assign locations sequentially, however, since that commit we have changed the method for SSO so that the VUE slot assigned depends on the number of builtin slots plus the location assigned to the varying. This fixed layout is intended to help SSO programs by avoiding on-the-fly recompiles when swapping out shaders, however, it also means that if a varying uses a large location number close to the maximum allowed by the SF/FS units (31), then the offset introduced by the number of builtin slots can push the location outside the range and trigger an assertion. This problem is affecting at least the following CTS tests for enhanced layouts: KHR-GL45.enhanced_layouts.varying_array_components KHR-GL45.enhanced_layouts.varying_array_locations KHR-GL45.enhanced_layouts.varying_components KHR-GL45.enhanced_layouts.varying_locations which use SSO and the the location layout qualifier to select such location numbers explicitly. This change helps these tests because for SSO we always have to include things such as VARYING_SLOT_CLIP_DIST{0,1} even if the fragment shader is very unlikely to read them, so by doing this we free builtin slots from the fixed VUE layout and we avoid the tests to crash in this scenario. Of course, this is not a proper fix, we'd still run into problems if someone tries to use an explicit max location and read gl_ViewportIndex, gl_LayerID or gl_CullDistancein in the FS, but that would be a much less common bug and we can probably wait to see if anyone actually runs into that situation in a real world scenario before making the decision that more aggresive changes are required to support this without reverting 99df02ca26f61. v2: - Add a debug message when we skip clip distances (Ilia) - we also need to account for this when we compute the urb setup for the fragment shader stage, so add a compiler util to compute the first slot that we need to read from the URB instead of replicating the logic in both places. v3: - Make the util more generic so it can account for all unused slots at the beginning of the URB, that will make it more useful (Ken). - Drop the debug message, it was not what Ilia was asking for. Suggested-by: Kenneth Graunke Reviewed-by: Kenneth Graunke --- src/intel/compiler/brw_compiler.h | 29 +++++++++++++++++++ src/intel/compiler/brw_fs.cpp | 7 ++--- src/mesa/drivers/dri/i965/genX_state_upload.c | 16 ++++------ 3 files changed, 38 insertions(+), 14 deletions(-) diff --git a/src/intel/compiler/brw_compiler.h b/src/intel/compiler/brw_compiler.h index 6753a8daf08..038f3f95512 100644 --- a/src/intel/compiler/brw_compiler.h +++ b/src/intel/compiler/brw_compiler.h @@ -1222,6 +1222,35 @@ brw_stage_has_packed_dispatch(const struct gen_device_info *devinfo, } } +/** + * Computes the first varying slot in the URB produced by the previous stage + * that is used in the next stage. We do this by testing the varying slots in + * the previous stage's vue map against the inputs read in the next stage. + * + * Note that: + * + * - Each URB offset contains two varying slots and we can only skip a + * full offset if both slots are unused, so the value we return here is always + * rounded down to the closest multiple of two. + * + * - gl_Layer and gl_ViewportIndex don't have their own varying slots, they are + * part of the vue header, so if these are read we can't skip anything. + */ +static inline int +brw_compute_first_urb_slot_required(uint64_t inputs_read, + const struct brw_vue_map *prev_stage_vue_map) +{ + if ((inputs_read & (VARYING_BIT_LAYER | VARYING_BIT_VIEWPORT)) == 0) { + for (int i = 0; i < prev_stage_vue_map->num_slots; i++) { + int varying = prev_stage_vue_map->slot_to_varying[i]; + if (varying > 0 && (inputs_read & BITFIELD64_BIT(varying)) != 0) + return ROUND_DOWN_TO(i, 2); + } + } + + return 0; +} + #ifdef __cplusplus } /* extern "C" */ #endif diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp index e33cb0e1186..a40b910c1a0 100644 --- a/src/intel/compiler/brw_fs.cpp +++ b/src/intel/compiler/brw_fs.cpp @@ -1481,9 +1481,6 @@ fs_visitor::calculate_urb_setup() } } } else { - bool include_vue_header = - nir->info.inputs_read & (VARYING_BIT_LAYER | VARYING_BIT_VIEWPORT); - /* We have enough input varyings that the SF/SBE pipeline stage can't * arbitrarily rearrange them to suit our whim; we have to put them * in an order that matches the output of the previous pipeline stage @@ -1493,8 +1490,10 @@ fs_visitor::calculate_urb_setup() brw_compute_vue_map(devinfo, &prev_stage_vue_map, key->input_slots_valid, nir->info.separate_shader); + int first_slot = - include_vue_header ? 0 : 2 * BRW_SF_URB_ENTRY_READ_OFFSET; + brw_compute_first_urb_slot_required(nir->info.inputs_read, + &prev_stage_vue_map); assert(prev_stage_vue_map.num_slots <= first_slot + 32); for (int slot = first_slot; slot < prev_stage_vue_map.num_slots; diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c b/src/mesa/drivers/dri/i965/genX_state_upload.c index 2a99376e3c2..ecf5a9ae68d 100644 --- a/src/mesa/drivers/dri/i965/genX_state_upload.c +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c @@ -1029,17 +1029,13 @@ genX(calculate_attr_overrides)(const struct brw_context *brw, *point_sprite_enables = 0; - /* If the fragment shader reads VARYING_SLOT_LAYER, then we need to pass in - * the full vertex header. Otherwise, we can program the SF to start - * reading at an offset of 1 (2 varying slots) to skip unnecessary data: - * - VARYING_SLOT_PSIZ and BRW_VARYING_SLOT_NDC on gen4-5 - * - VARYING_SLOT_{PSIZ,LAYER} and VARYING_SLOT_POS on gen6+ - */ - - bool fs_needs_vue_header = fp->info.inputs_read & - (VARYING_BIT_LAYER | VARYING_BIT_VIEWPORT); + int first_slot = + brw_compute_first_urb_slot_required(fp->info.inputs_read, + &brw->vue_map_geom_out); - *urb_entry_read_offset = fs_needs_vue_header ? 0 : 1; + /* Each URB offset packs two varying slots */ + assert(first_slot % 2 == 0); + *urb_entry_read_offset = first_slot / 2; /* From the Ivybridge PRM, Vol 2 Part 1, 3DSTATE_SBE, * description of dw10 Point Sprite Texture Coordinate Enable: -- 2.30.2