[Mesa-dev] [PATCH] i965/fs: Implement SIMD16 dual source blending.
Jason Ekstrand
jason at jlekstrand.net
Thu Mar 5 21:39:58 PST 2015
This looks fine to me. I just kicked off a build on our test farm and,
assuming that looks good (I'll send another e-mail in the morning if it
does),
Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>
I ran shader-db on the change and I was kind of surprised to see that it
doesn't really do anything.
GAINED: shaders/dolphin/smg.1.shader_test FS SIMD16
total instructions in shared programs: 5769629 -> 5769629 (0.00%)
instructions in affected programs: 0 -> 0
helped: 0
HURT: 0
GAINED: 1
LOST: 0
Perhaps shader-db doesn't account for some other GL state required for
dual-source because I doubt only one shader uses it. Ken?
--Jason
On Thu, Mar 5, 2015 at 3:21 AM, Iago Toral Quiroga <itoral at igalia.com>
wrote:
> From the SNB PRM, volume 4, part 1, page 193:
>
> "The dual source render target messages only have SIMD8 forms due to
> maximum message length limitations. SIMD16 pixel shaders must send two of
> these messages to cover all of the pixels. Each message contains two
> colors
> (4 channels each) for each pixel in the message payload."
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82831
> ---
> I sent this patch for review some months ago, but it was bad timing because
> it was when Jason was doing a large rewrite of the visitor code handling
> FB writes, so the patch became immediately obsolete. This is the up-to-date
> version.
>
> If anyone wants to test this, I sent this patch to piglit with a test that
> can be used to check for correct SIMD16 implementation specifically:
> http://lists.freedesktop.org/archives/piglit/2015-March/015015.html
>
> src/mesa/drivers/dri/i965/brw_eu.h | 1 +
> src/mesa/drivers/dri/i965/brw_eu_emit.c | 3 +-
> src/mesa/drivers/dri/i965/brw_fs.h | 6 +-
> src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 15 ++++-
> src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 77
> +++++++++++++++++++++-----
> 5 files changed, 83 insertions(+), 19 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_eu.h
> b/src/mesa/drivers/dri/i965/brw_eu.h
> index 736c54b..d9ad5bd 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu.h
> +++ b/src/mesa/drivers/dri/i965/brw_eu.h
> @@ -266,6 +266,7 @@ void brw_fb_WRITE(struct brw_compile *p,
> unsigned msg_length,
> unsigned response_length,
> bool eot,
> + bool last_render_target,
> bool header_present);
>
> void brw_SAMPLE(struct brw_compile *p,
> diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> index 1d6fd67..74cf138 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> @@ -2292,6 +2292,7 @@ void brw_fb_WRITE(struct brw_compile *p,
> unsigned msg_length,
> unsigned response_length,
> bool eot,
> + bool last_render_target,
> bool header_present)
> {
> struct brw_context *brw = p->brw;
> @@ -2333,7 +2334,7 @@ void brw_fb_WRITE(struct brw_compile *p,
> msg_type,
> msg_length,
> header_present,
> - eot, /* last render target write */
> + last_render_target,
> response_length,
> eot,
> 0 /* send_commit_msg */);
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h
> b/src/mesa/drivers/dri/i965/brw_fs.h
> index 70098d8..5a4f66c 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -369,10 +369,12 @@ public:
> bool optimize_frontfacing_ternary(nir_alu_instr *instr,
> const fs_reg &result);
>
> - int setup_color_payload(fs_reg *dst, fs_reg color, unsigned
> components);
> + int setup_color_payload(fs_reg *dst, fs_reg color, unsigned components,
> + bool use_2nd_half);
> void emit_alpha_test();
> fs_inst *emit_single_fb_write(fs_reg color1, fs_reg color2,
> - fs_reg src0_alpha, unsigned components);
> + fs_reg src0_alpha, unsigned components,
> + bool use_2nd_half = false);
> void emit_fb_writes();
> void emit_urb_writes();
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> index cbe6191..db70df5 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> @@ -217,9 +217,12 @@ fs_generator::fire_fb_write(fs_inst *inst,
>
> if (inst->opcode == FS_OPCODE_REP_FB_WRITE)
> msg_control =
> BRW_DATAPORT_RENDER_TARGET_WRITE_SIMD16_SINGLE_SOURCE_REPLICATED;
> - else if (prog_data->dual_src_blend)
> - msg_control =
> BRW_DATAPORT_RENDER_TARGET_WRITE_SIMD8_DUAL_SOURCE_SUBSPAN01;
> - else if (dispatch_width == 16)
> + else if (prog_data->dual_src_blend) {
> + if (dispatch_width == 8 || !inst->eot)
> + msg_control =
> BRW_DATAPORT_RENDER_TARGET_WRITE_SIMD8_DUAL_SOURCE_SUBSPAN01;
> + else
> + msg_control =
> BRW_DATAPORT_RENDER_TARGET_WRITE_SIMD8_DUAL_SOURCE_SUBSPAN23;
> + } else if (dispatch_width == 16)
> msg_control = BRW_DATAPORT_RENDER_TARGET_WRITE_SIMD16_SINGLE_SOURCE;
> else
> msg_control =
> BRW_DATAPORT_RENDER_TARGET_WRITE_SIMD8_SINGLE_SOURCE_SUBSPAN01;
> @@ -227,6 +230,10 @@ fs_generator::fire_fb_write(fs_inst *inst,
> uint32_t surf_index =
> prog_data->binding_table.render_target_start + inst->target;
>
> + bool last_render_target = inst->eot ||
> + (prog_data->dual_src_blend && dispatch_width
> == 16);
> +
> +
> brw_fb_WRITE(p,
> dispatch_width,
> payload,
> @@ -236,6 +243,7 @@ fs_generator::fire_fb_write(fs_inst *inst,
> nr,
> 0,
> inst->eot,
> + last_render_target,
> inst->header_present);
>
> brw_mark_surface_used(&prog_data->base, surf_index);
> @@ -373,6 +381,7 @@ fs_generator::generate_blorp_fb_write(fs_inst *inst)
> inst->mlen,
> 0,
> true,
> + true,
> inst->header_present);
> }
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> index 57c4d66..d3b8345 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> @@ -3366,7 +3366,8 @@ fs_visitor::emit_interpolation_setup_gen6()
> }
>
> int
> -fs_visitor::setup_color_payload(fs_reg *dst, fs_reg color, unsigned
> components)
> +fs_visitor::setup_color_payload(fs_reg *dst, fs_reg color, unsigned
> components,
> + bool use_2nd_half)
> {
> brw_wm_prog_key *key = (brw_wm_prog_key*) this->key;
> fs_inst *inst;
> @@ -3384,7 +3385,7 @@ fs_visitor::setup_color_payload(fs_reg *dst, fs_reg
> color, unsigned components)
> colors_enabled = (1 << components) - 1;
> }
>
> - if (dispatch_width == 8 || brw->gen >= 6) {
> + if (dispatch_width == 8 || (brw->gen >= 6 && !do_dual_src)) {
> /* SIMD8 write looks like:
> * m + 0: r0
> * m + 1: r1
> @@ -3415,6 +3416,33 @@ fs_visitor::setup_color_payload(fs_reg *dst, fs_reg
> color, unsigned components)
> len++;
> }
> return len;
> + } else if (brw->gen >= 6 && do_dual_src) {
> + /* SIMD16 dual source blending for gen6+.
> + *
> + * From the SNB PRM, volume 4, part 1, page 193:
> + *
> + * "The dual source render target messages only have SIMD8 forms
> due to
> + * maximum message length limitations. SIMD16 pixel shaders must
> send two
> + * of these messages to cover all of the pixels. Each message
> contains
> + * two colors (4 channels each) for each pixel in the message
> payload."
> + *
> + * So in SIMD16 dual source blending we will send 2 SIMD8 messages,
> + * each one will call this function twice (one for each color
> involved),
> + * so in each pass we only write 4 registers. Notice that the second
> + * SIMD8 message needs to read color data from the 2nd half of the
> color
> + * registers, so it needs to call this with use_2nd_half = true.
> + */
> + for (unsigned i = 0; i < 4; ++i) {
> + if (colors_enabled & (1 << i)) {
> + dst[i] = fs_reg(GRF, alloc.allocate(1), color.type);
> + inst = emit(MOV(dst[i], half(offset(color, i),
> + use_2nd_half ? 1 : 0)));
> + inst->saturate = key->clamp_fragment_color;
> + if (use_2nd_half)
> + inst->force_sechalf = true;
> + }
> + }
> + return 4;
> } else {
> /* pre-gen6 SIMD16 single source DP write looks like:
> * m + 0: r0
> @@ -3498,7 +3526,8 @@ fs_visitor::emit_alpha_test()
>
> fs_inst *
> fs_visitor::emit_single_fb_write(fs_reg color0, fs_reg color1,
> - fs_reg src0_alpha, unsigned components)
> + fs_reg src0_alpha, unsigned components,
> + bool use_2nd_half)
> {
> assert(stage == MESA_SHADER_FRAGMENT);
> brw_wm_prog_data *prog_data = (brw_wm_prog_data*) this->prog_data;
> @@ -3558,7 +3587,8 @@ fs_visitor::emit_single_fb_write(fs_reg color0,
> fs_reg color1,
> * alpha out the pipeline to our null renderbuffer to support
> * alpha-testing, alpha-to-coverage, and so on.
> */
> - length += setup_color_payload(sources + length, this->outputs[0],
> 0);
> + length += setup_color_payload(sources + length, this->outputs[0], 0,
> + false);
> } else if (color1.file == BAD_FILE) {
> if (src0_alpha.file != BAD_FILE) {
> sources[length] = fs_reg(GRF, alloc.allocate(reg_size),
> @@ -3568,10 +3598,13 @@ fs_visitor::emit_single_fb_write(fs_reg color0,
> fs_reg color1,
> length++;
> }
>
> - length += setup_color_payload(sources + length, color0, components);
> + length += setup_color_payload(sources + length, color0, components,
> + false);
> } else {
> - length += setup_color_payload(sources + length, color0, components);
> - length += setup_color_payload(sources + length, color1, components);
> + length += setup_color_payload(sources + length, color0, components,
> + use_2nd_half);
> + length += setup_color_payload(sources + length, color1, components,
> + use_2nd_half);
> }
>
> if (source_depth_to_render_target) {
> @@ -3640,12 +3673,6 @@ fs_visitor::emit_fb_writes()
> brw_wm_prog_data *prog_data = (brw_wm_prog_data*) this->prog_data;
> brw_wm_prog_key *key = (brw_wm_prog_key*) this->key;
>
> - if (do_dual_src) {
> - no16("GL_ARB_blend_func_extended not yet supported in SIMD16.");
> - if (dispatch_width == 16)
> - do_dual_src = false;
> - }
> -
> fs_inst *inst;
> if (do_dual_src) {
> if (INTEL_DEBUG & DEBUG_SHADER_TIME)
> @@ -3656,6 +3683,30 @@ fs_visitor::emit_fb_writes()
> inst = emit_single_fb_write(this->outputs[0], this->dual_src_output,
> reg_undef, 4);
> inst->target = 0;
> +
> + /* SIMD16 dual source blending requires to send two SIMD8 dual
> source
> + * messages, where each message contains color data for 8 pixels.
> Color
> + * data for the first group of pixels is stored in the "lower" half
> of
> + * the color registers, so in SIMD16, the previous message did:
> + * m + 0: r0
> + * m + 1: g0
> + * m + 2: b0
> + * m + 3: a0
> + *
> + * Here goes the second message, which packs color data for the
> + * remaining 8 pixels. Color data for these pixels is stored in the
> + * "upper" half of the color registers, so we need to do:
> + * m + 0: r1
> + * m + 1: g1
> + * m + 2: b1
> + * m + 3: a1
> + */
> + if (dispatch_width == 16) {
> + inst = emit_single_fb_write(this->outputs[0],
> this->dual_src_output,
> + reg_undef, 4, true);
> + inst->target = 0;
> + }
> +
> prog_data->dual_src_blend = true;
> } else if (key->nr_color_regions > 0) {
> for (int target = 0; target < key->nr_color_regions; target++) {
> --
> 1.9.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150305/04ba3cdd/attachment-0001.html>
More information about the mesa-dev
mailing list