[Mesa-dev] [PATCH 01/21] i965/fs: Get rid of fs_visitor::do_dual_src.

[Francisco Jerez]

Anuj Phogat <anuj.phogat at gmail.com> writes:

> On Fri, Jul 22, 2016 at 8:58 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>> This boolean flag was being used for two different things:
>>
>>  - To set the brw_wm_prog_data::dual_src_blend flag.  Instead we can
>>    just set it based on whether the dual_src_output register is valid,
>>    which will be the case if the shader writes the secondary blending
>>    color.
>>
>>  - To decide whether to call emit_single_fb_write() once, or in a loop
>>    that would iterate only once, which seems pretty useless.
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.h           |  1 -
>>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp     |  2 --
>>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 37 +++++++++++-----------------
>>  3 files changed, 14 insertions(+), 26 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
>> index fc1e1c4..46b15b4 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.h
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
>> @@ -318,7 +318,6 @@ public:
>>     fs_reg sample_mask;
>>     fs_reg outputs[VARYING_SLOT_MAX];
>>     fs_reg dual_src_output;
>> -   bool do_dual_src;
>>     int first_non_payload_grf;
>>     /** Either BRW_MAX_GRF or GEN7_MRF_HACK_START */
>>     unsigned max_grf;
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> index 50d73eb..2872b2d 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> @@ -103,12 +103,10 @@ fs_visitor::nir_setup_outputs()
>>           if (key->force_dual_color_blend &&
>>               var->data.location == FRAG_RESULT_DATA1) {
>>              this->dual_src_output = reg;
>> -            this->do_dual_src = true;
>>           } else if (var->data.index > 0) {
>>              assert(var->data.location == FRAG_RESULT_DATA0);
>>              assert(var->data.index == 1);
>>              this->dual_src_output = reg;
>> -            this->do_dual_src = true;
>>           } else if (var->data.location == FRAG_RESULT_COLOR) {
>>              /* Writing gl_FragColor outputs to all color regions. */
>>              for (unsigned int i = 0; i < MAX2(key->nr_color_regions, 1); i++) {
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> index 6d84374..808d8af 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> @@ -437,33 +437,25 @@ fs_visitor::emit_fb_writes()
>>                             "in SIMD16+ mode.\n");
>>     }
>>
>> -   if (do_dual_src) {
>> -      const fs_builder abld = bld.annotate("FB dual-source write");
>> +   for (int target = 0; target < key->nr_color_regions; target++) {
>> +      /* Skip over outputs that weren't written. */
>> +      if (this->outputs[target].file == BAD_FILE)
>> +         continue;
>>
>> -      inst = emit_single_fb_write(abld, this->outputs[0],
>> -                                  this->dual_src_output, reg_undef, 4);
>> -      inst->target = 0;
>> -
>> -      prog_data->dual_src_blend = true;
>> -   } else {
>> -      for (int target = 0; target < key->nr_color_regions; target++) {
>> -         /* Skip over outputs that weren't written. */
>> -         if (this->outputs[target].file == BAD_FILE)
>> -            continue;
>> +      const fs_builder abld = bld.annotate(
>> +         ralloc_asprintf(this->mem_ctx, "FB write target %d", target));
>>
>> -         const fs_builder abld = bld.annotate(
>> -            ralloc_asprintf(this->mem_ctx, "FB write target %d", target));
>> +      fs_reg src0_alpha;
>> +      if (devinfo->gen >= 6 && key->replicate_alpha && target != 0)
>> +         src0_alpha = offset(outputs[0], bld, 3);
>>
>> -         fs_reg src0_alpha;
>> -         if (devinfo->gen >= 6 && key->replicate_alpha && target != 0)
>> -            src0_alpha = offset(outputs[0], bld, 3);
>> -
>> -         inst = emit_single_fb_write(abld, this->outputs[target], reg_undef,
>> -                                     src0_alpha, 4);
>> -         inst->target = target;
>> -      }
>> +      inst = emit_single_fb_write(abld, this->outputs[target],
>> +                                  this->dual_src_output, src0_alpha, 4);
>> +      inst->target = target;
>>     }
>>
>> +   prog_data->dual_src_blend = (this->dual_src_output.file != BAD_FILE);
>> +
> It'll be nice to add this assert here:
> assert(!prog_data->dual_src_blend ||  key->nr_color_regions == 1);
>
Heh, part of my purpose with this was to make the code above less wrong
for dual source blending in combination with multiple render targets --
Though it could be argued that the code is still kind of broken for that
case because the hardware doesn't support sending a src0 alpha payload
in the dual source RT write message, and because there is still a single
dual_src_output register instead of a per-target array of registers, so
I've added the assert locally.

>>     if (inst == NULL) {
>>        /* Even if there's no color buffers enabled, we still need to send
>>         * alpha out the pipeline to our null renderbuffer to support
>> @@ -914,7 +906,6 @@ fs_visitor::init()
>>     this->promoted_constants = 0,
>>
>>     this->spilled_any_registers = false;
>> -   this->do_dual_src = false;
>>  }
>>
>>  fs_visitor::~fs_visitor()
>> --
>> 2.9.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> Patch is:
> Reviewed-by: Anuj Phogat <anuj.phogat at gmail.com>

Thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160726/be6222e8/attachment.sig>