On Fri, May 3, 2019 at 6:52 AM Iago Toral Quiroga <[email protected]> wrote:
> From: Samuel Iglesias Gonsálvez <[email protected]> > > There are tests in CTS for alpha to coverage without a color attachment > that are failing. This happens because when we remove the shader color > outputs when we don't have a valid color attachment for them, but when > alpha to coverage is enabled we still want to preserve the the output > at location 0 since we need its alpha component for alpha to coverage. > In that case we will also need to create a null render target for RT 0. > > v2: > - We already create a null rt when we don't have any, so reuse that > for this case (Jason) > - Simplify the code a bit (Iago) > > v3: > - Take alpha to coverage from the key and don't tie this to depth-only > rendering only, we want the same behavior if we have multiple render > targets but the one at location 0 is not used. (Jason). > - Rewrite commit message (Iago) > > Fixes the following CTS tests: > dEQP-VK.pipeline.multisample.alpha_to_coverage_no_color_attachment.* > > Signed-off-by: Samuel Iglesias Gonsálvez <[email protected]> > Signed-off-by: Iago Toral Quiroga <[email protected]> > --- > src/intel/vulkan/anv_pipeline.c | 48 +++++++++++++++++++++++++-------- > 1 file changed, 37 insertions(+), 11 deletions(-) > > diff --git a/src/intel/vulkan/anv_pipeline.c > b/src/intel/vulkan/anv_pipeline.c > index 20eab548fb2..f379dd2752e 100644 > --- a/src/intel/vulkan/anv_pipeline.c > +++ b/src/intel/vulkan/anv_pipeline.c > @@ -818,15 +818,28 @@ anv_pipeline_link_fs(const struct brw_compiler > *compiler, > memset(rt_used, 0, sizeof(rt_used)); > > /* Flag used render targets */ > + bool needs_null_rt_for_alpha_to_coverage = false; > nir_foreach_variable_safe(var, &stage->nir->outputs) { > if (var->data.location < FRAG_RESULT_DATA0) > continue; > > const unsigned rt = var->data.location - FRAG_RESULT_DATA0; > - /* Unused or out-of-bounds */ > - if (rt >= MAX_RTS || !(stage->key.wm.color_outputs_valid & (1 << > rt))) > + /* Out-of-bounds */ > + if (rt >= MAX_RTS) > continue; > > + /* Unused */ > + if (!(stage->key.wm.color_outputs_valid & (1 << rt))) { > While we're here, I realized reading this code today that we're only checking one bit for the color attachment whereas we really should be comparing against BITFIELD_RANGE(rt, array_size) here. > + /* If this is the RT at location 0 and we have alpha to coverage > + * enabled, we'll have to create a null render target and it must > + * be at index 0. > + */ > + if (rt == 0 && stage->key.wm.alpha_to_coverage) > + needs_null_rt_for_alpha_to_coverage = true; > Why do we need all this needs_null_rt_for_alpha_to_coverage buisiness? Why not just let it fall through and set rt_used to true and then.... > + > + continue; > + } > + > const unsigned array_len = > glsl_type_is_array(var->type) ? glsl_get_length(var->type) : 1; > assert(rt + array_len <= max_rt); > @@ -835,7 +848,12 @@ anv_pipeline_link_fs(const struct brw_compiler > *compiler, > rt_used[rt + i] = true; > } > > - /* Set new, compacted, location */ > + /* Make sure we leave the first RT slot available for alpha to coverage > + * if we don't have a valid RT 0. > + */ > + if (needs_null_rt_for_alpha_to_coverage) > + num_rts = 1; > + > for (unsigned i = 0; i < max_rt; i++) { > if (!rt_used[i]) > continue; > Down here just do if (stage->key.wm.color_outputs_valid & (1 << i)) { /* Set up a color attachment */ } else { /* Set up a null attachment */ } num_rts++; This would also fix a bug that I think we have today if you have an array in the shader that only has some of it's inputs valid. I think today you'd end up crashing when we go to bind the non-existent color attachments. > @@ -857,11 +875,15 @@ anv_pipeline_link_fs(const struct brw_compiler > *compiler, > const unsigned rt = var->data.location - FRAG_RESULT_DATA0; > if (rt >= MAX_RTS || > !(stage->key.wm.color_outputs_valid & (1 << rt))) { > - /* Unused or out-of-bounds, throw it away */ > - deleted_output = true; > - var->data.mode = nir_var_function_temp; > - exec_node_remove(&var->node); > - exec_list_push_tail(&impl->locals, &var->node); > + /* Unused or out-of-bounds, throw it away, unless it is the first > + * RT and we have alpha to coverage. > + */ > + if (rt != 0 || !stage->key.wm.alpha_to_coverage) { > + deleted_output = true; > + var->data.mode = nir_var_function_temp; > + exec_node_remove(&var->node); > + exec_list_push_tail(&impl->locals, &var->node); > + } > continue; > } > > @@ -873,14 +895,18 @@ anv_pipeline_link_fs(const struct brw_compiler > *compiler, > if (deleted_output) > nir_fixup_deref_modes(stage->nir); > > - if (num_rts == 0) { > - /* If we have no render targets, we need a null render target */ > + /* If we have no render targets or we need to create one for alpha to > + * coverage, we need a null render target. > + */ > + if (num_rts == 0 || needs_null_rt_for_alpha_to_coverage) { > rt_bindings[0] = (struct anv_pipeline_binding) { > .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS, > .binding = 0, > .index = UINT32_MAX, > }; > - num_rts = 1; > + > + if (num_rts == 0) > + num_rts = 1; > } > > /* Now that we've determined the actual number of render targets, > adjust > -- > 2.17.1 > >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
