On 11/04/2011 03:01 PM, Eric Anholt wrote:
> Before, I was tracking the ir_variable * found for gl_FragColor or
> gl_FragData[]. Instead, when visiting those variables, set up an
> array of per-render-target fs_regs to copy the output data from. This
> cleans up the color emit path, while making handling of multiple
> user-defined out variables easier.
>
> Note that brw_type_for_base_type() now returns a sensible value for
> arrays. This probably obsoletes a bunch of type overrides we have
> laying around.
> ---
> src/mesa/drivers/dri/i965/brw_fs.h | 8 +-
> src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 80 ++++++++++++++-----------
> src/mesa/drivers/dri/i965/brw_shader.cpp | 1 +
> 3 files changed, 50 insertions(+), 39 deletions(-)
I like these changes. However, it feels like you're really doing three
separate changes in one patch. I went ahead and split it up, and will
post those shortly.
A few more comments inline.
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h
> b/src/mesa/drivers/dri/i965/brw_fs.h
> index e2ad649..854a935 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
[snip]
> @@ -77,9 +69,32 @@ fs_visitor::visit(ir_variable *ir)
> assert(reg);
> hash_table_insert(this->variable_ht, reg, ir);
> return;
> - }
> + } else if (ir->mode == ir_var_out) {
> + reg = new(this->mem_ctx) fs_reg(this, ir->type);
> +
> + if (strcmp(ir->name, "gl_FragColor") == 0) {
> + for (int i = 0; i < c->key.nr_color_regions; i++) {
> + this->outputs[i] = *reg;
> + }
> + } else if (strcmp(ir->name, "gl_FragData") == 0) {
> + for (int i = 0; i < c->key.nr_color_regions; i++) {
> + this->outputs[i] = *reg;
> + this->outputs[i].reg_offset += 4 * i;
> + }
> + } else if (strcmp(ir->name, "gl_FragDepth") == 0) {
> + this->frag_depth = ir;
> + } else {
> + assert(ir->location >= FRAG_RESULT_DATA0 &&
> + ir->location < FRAG_RESULT_DATA0 + BRW_MAX_DRAW_BUFFERS);
>
> - if (ir->mode == ir_var_uniform) {
> + /* General color output. */
> + for (unsigned int i = 0; i < MAX2(1, ir->type->length); i++) {
> + int output = ir->location - FRAG_RESULT_DATA0 + i;
> + this->outputs[output] = *reg;
> + this->outputs[output].reg_offset += 4 * i;
> + }
> + }
> + } else if (ir->mode == ir_var_uniform) {
> int param_index = c->prog_data.nr_params;
>
> if (c->dispatch_width == 16) {
I agree with Ian here---using ir->location ought to be cleaner. The
code for gl_FragData and generic FS outputs is exactly the same...
I've gone ahead and made this change too, I'll post that shortly.
[snip]
> @@ -1988,13 +1999,12 @@ fs_visitor::emit_fb_writes()
> }
>
> if (c->key.nr_color_regions == 0) {
> - if (c->key.alpha_test && (this->frag_color || this->frag_data)) {
> + if (c->key.alpha_test) {
> /* If the alpha test is enabled but there's no color buffer,
> * we still need to send alpha out the pipeline to our null
> * renderbuffer.
> */
> - color.reg_offset += 3;
> - emit_color_write(3, color_mrf, color);
> + emit_color_write(0, 3, color_mrf);
> }
I'm a little concerned about removing reg_offset += 3. This seems like
a change in behavior (assigning all four instead of just alpha). Was it
wrong before/is this a bug fix? Or, is this just simpler code (since
the other three channels are ignored anyway)?
--Kenneth
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
