Hi Jason, On mié, 2014-09-17 at 11:39 -0700, Jason Ekstrand wrote: > I haven't tested this yet, just looked it over. I've got a couple of > inline comments below. One general comment though: I'm currently > working on a bunch of compiler stuff that reworks the way we do FB > writes. In particular, it reworks things to use GRF registers instead > of the MRF. It probably wouldn't be too bad for me to rebase on top > of this or to rebase your patch on top of what I'm doing. I just > thought I'd warn you about the conflict.
Thanks for the warning, I suppose it should be easy for me to rebase this patch on top of your work if you happen to land yours sooner. > On Wed, Sep 17, 2014 at 4:36 AM, Iago Toral Quiroga > <ito...@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 > --- > 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_generator.cpp | 14 +++++++-- > src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 41 > +++++++++++++++++++------- > 4 files changed, 45 insertions(+), 14 deletions(-) > > I tested this on SandyBridge and IvyBridge. No piglit > regressions in these > platforms, but would be nice if someone could test this in > later platforms too. > > I only noticed these two tests for dual source blending in > piglit though: > tests/spec/ext_framebuffer_multisample/alpha-to-one-dual-src-blend.cpp > > tests/spec/ext_framebuffer_multisample/alpha-to-coverage-dual-src-blend.cpp > > The first one fails, in both platforms with and without my > patch. The second one > passes in both platforms, with and without my patch. > > I also tested this with a seprate test program to verify that > it worked, at > least, in a simple case. > > diff --git a/src/mesa/drivers/dri/i965/brw_eu.h > b/src/mesa/drivers/dri/i965/brw_eu.h > index e6c26e3..5908ba5 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 39f94e9..ffdbe6d 100644 > --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c > +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c > @@ -2251,6 +2251,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; > @@ -2290,7 +2291,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_generator.cpp > b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp > index 1bc10f5..a4b84aa 100644 > --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp > +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp > @@ -121,9 +121,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; > @@ -131,6 +134,9 @@ 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, > base_reg, > @@ -140,6 +146,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); > @@ -254,6 +261,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 5d2e7c8..21dc5bc 100644 > --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp > +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp > @@ -3064,12 +3064,6 @@ fs_visitor::emit_fb_writes() > int reg_width = dispatch_width / 8; > bool src0_alpha_to_render_target = false; > > - if (do_dual_src) { > - no16("GL_ARB_blend_func_extended not yet supported in > SIMD16."); > - if (dispatch_width == 16) > - do_dual_src = false; > - } > - > /* From the Sandy Bridge PRM, volume 4, page 198: > * > * "Dispatched Pixel Enables. One bit per pixel > indicating > @@ -3109,11 +3103,22 @@ fs_visitor::emit_fb_writes() > nr += 1; > } > > - /* Reserve space for color. It'll be filled in per MRT > below. */ > + /* Reserve space for color. It'll be filled in per MRT > below. > + * > + * 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 color data in dual source mode, whether this is a > SIMD8 or SIMD16 > + * program, always requires 8 MRF registers. > + */ > int color_mrf = nr; > - nr += 4 * reg_width; > if (do_dual_src) > - nr += 4; > + nr += 8; > + else > + nr += 4 * reg_width; > if (src0_alpha_to_render_target) > nr += reg_width; > > @@ -3173,13 +3178,29 @@ fs_visitor::emit_fb_writes() > inst->target = 0; > inst->base_mrf = base_mrf; > inst->mlen = nr - base_mrf; > - inst->eot = true; > + inst->eot = dispatch_width == 8; > inst->header_present = header_present; > if ((brw->gen >= 8 || brw->is_haswell) && > prog_data->uses_kill) { > inst->predicate = BRW_PREDICATE_NORMAL; > inst->flag_subreg = 1; > } > > > It appears as if you don't copy the second half of the colors into the > message before doing the second send. Is there a reason for this? > It's quite possible that the piglit tests are only using solid colors > in which case they might not catch this. > Yes, I noticed that after I sent the patch. It is a mistake, I'll send a second version with this fixed. At least the standalone program I was using to test this was using solid colors, and I suppose piglit tests do the same.. maybe we should add one more test or something. Iago > + /* SIMD16 dual source blending requires to send two > SIMD8 dual source > + * messages, so here goes the second one. > + */ > + if (dispatch_width == 16) { > + fs_inst *inst = emit(FS_OPCODE_FB_WRITE); > + inst->target = 0; > + inst->base_mrf = base_mrf; > + inst->mlen = nr - base_mrf; > + inst->eot = true; > + inst->header_present = header_present; > + if ((brw->gen >= 8 || brw->is_haswell) && > prog_data->uses_kill) { > + inst->predicate = BRW_PREDICATE_NORMAL; > + inst->flag_subreg = 1; > + } > + } > + > prog_data->dual_src_blend = true; > this->current_annotation = NULL; > return; > -- > 1.9.1 > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
