On Mon, 2019-05-06 at 14:32 -0500, Jason Ekstrand wrote:
> On Mon, May 6, 2019 at 9:01 AM Iago Toral Quiroga <ito...@igalia.com>
> wrote:
> > From: Samuel Iglesias Gonsálvez <sigles...@igalia.com>
> >
> >
> >
> > There are tests in CTS for alpha to coverage without a color
> > attachment
> >
> > that are failing. This happens because 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 the alpha component. 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)
> >
> >
> >
> > v4:
> >
> > - Make sure we take into account the array length of the shader
> > outputs,
> >
> > which we were no handling correctly either and make sure we
> > also
> >
> > create null render targets for any invalid array entries too.
> > (Jason)
> >
> >
> >
> > Fixes the following CTS tests:
> >
> > dEQP-
> > VK.pipeline.multisample.alpha_to_coverage_no_color_attachment.*
> >
> >
> >
> > Signed-off-by: Samuel Iglesias Gonsálvez <sigles...@igalia.com>
> >
> > Signed-off-by: Iago Toral Quiroga <ito...@igalia.com>
> >
> > ---
> >
> > src/intel/vulkan/anv_pipeline.c | 56 ++++++++++++++++++++++++-----
> > ----
> >
> > 1 file changed, 42 insertions(+), 14 deletions(-)
> >
> >
> >
> > diff --git a/src/intel/vulkan/anv_pipeline.c
> > b/src/intel/vulkan/anv_pipeline.c
> >
> > index 20eab548fb2..f15f0896266 100644
> >
> > --- a/src/intel/vulkan/anv_pipeline.c
> >
> > +++ b/src/intel/vulkan/anv_pipeline.c
> >
> > @@ -823,14 +823,24 @@ anv_pipeline_link_fs(const struct
> > brw_compiler *compiler,
> >
> > 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;
> >
> >
> >
> > const unsigned array_len =
> >
> > glsl_type_is_array(var->type) ? glsl_get_length(var-
> > >type) : 1;
> >
> > assert(rt + array_len <= max_rt);
> >
> >
> >
> > + /* Unused */
> >
> > + if (!(stage->key.wm.color_outputs_valid & BITFIELD_RANGE(rt,
> > array_len))) {
> >
> > + /* If this is the RT at location 0 and we have alpha to
> > coverage
> >
> > + * enabled we will have to create a null RT for it, so
> > mark it as
> >
> > + * used.
> >
> > + */
> >
> > + if (rt > 0 || !stage->key.wm.alpha_to_coverage)
> >
> > + continue;
> >
> > + }
> >
> > +
> >
> > for (unsigned i = 0; i < array_len; i++)
> >
> > rt_used[rt + i] = true;
> >
> > }
> >
> > @@ -841,11 +851,22 @@ anv_pipeline_link_fs(const struct
> > brw_compiler *compiler,
> >
> > continue;
> >
> >
> >
> > rt_to_bindings[i] = num_rts;
> >
> > - rt_bindings[rt_to_bindings[i]] = (struct
> > anv_pipeline_binding) {
> >
> > - .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,
> >
> > - .binding = 0,
> >
> > - .index = i,
> >
> > - };
> >
> > +
> >
> > + if (stage->key.wm.color_outputs_valid & (1 << i)) {
> >
> > + rt_bindings[rt_to_bindings[i]] = (struct
> > anv_pipeline_binding) {
> >
> > + .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,
> >
> > + .binding = 0,
> >
> > + .index = i,
> >
> > + };
> >
> > + } else {
> >
> > + /* Setup a null render target */
> >
> > + rt_bindings[rt_to_bindings[i]] = (struct
> > anv_pipeline_binding) {
> >
> > + .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,
> >
> > + .binding = 0,
> >
> > + .index = UINT32_MAX,
> >
> > + };
> >
> > + }
> >
> > +
> >
> > num_rts++;
> >
> > }
> >
> >
> >
> > @@ -855,14 +876,21 @@ anv_pipeline_link_fs(const struct
> > brw_compiler *compiler,
> >
> > continue;
> >
> >
> >
> > const unsigned rt = var->data.location - FRAG_RESULT_DATA0;
> >
> > + const unsigned array_len =
> >
> > + glsl_type_is_array(var->type) ? glsl_get_length(var-
> > >type) : 1;
> >
> > +
> >
> > 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);
> >
> > - continue;
> >
> > + !(stage->key.wm.color_outputs_valid & BITFIELD_RANGE(rt,
> > array_len))) {
> >
> > + /* Unused or out-of-bounds, throw it away, unless it is
> > the first
> >
> > + * RT and we have alpha to coverage enabled.
> >
> > + */
> >
> > + 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;
> >
> > + }
>
> I think we can simplify this yet further and just do
>
> if (rt >= MAX_RTS || !rt_used[rt])
>
> That way, all the logic stays in one place.
Right, that's much better.
> Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net>
thanks!
Iago
> > }
> >
> >
> >
> > /* Give it the new location */
> >
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
