Anuj Phogat <anuj.pho...@gmail.com> writes:
> On Fri, Jul 22, 2016 at 8:58 PM, Francisco Jerez <curroje...@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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> Patch is:
> Reviewed-by: Anuj Phogat <anuj.pho...@gmail.com>
Thanks.
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
