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