[Mesa-dev] [PATCH 24/42] i965/blorp: move emission of sample combining into eu-emitter

[Pohjolainen, Topi]

On Mon, Jan 20, 2014 at 08:30:41PM -0800, Paul Berry wrote:
>    On 20 January 2014 19:36, Paul Berry <stereotype441 at gmail.com> wrote:
> 
>      On 20 December 2013 06:38, Topi Pohjolainen <topi.pohjolainen at intel.com>
>      wrote:
> 
>        diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.cpp
>        b/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.cpp
>        index b189aa2..dcfd82b 100644
>        --- a/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.cpp
>        +++ b/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.cpp
>        @@ -148,3 +148,15 @@
>        brw_blorp_eu_emitter::emit_render_target_write(const struct brw_reg
>        &src0,
>                         true /* eot */,
>                         use_header);
>         }
>        +
>        +void
>        +brw_blorp_eu_emitter::emit_combine(unsigned texture_data_type,
>        +                                   const struct brw_reg &dst,
>        +                                   const struct brw_reg &src_1,
>        +                                   const struct brw_reg &src_2)
>        +{
>        +   if (texture_data_type == BRW_REGISTER_TYPE_F)
>        +      brw_ADD(&func, dst, src_1, src_2);
>        +   else
>        +      brw_AVG(&func, dst, src_1, src_2);
>        +}
> 
>      It's a bit of an awkward split to have most of the algorithm for
>      combining samples in brw_blorp_blit_program::manual_blend_average(), but
>      the choice of whether to use ADD or AVG is here in
>      brw_blorp_eu_emitter::emit_combine().  How about if we replace
>      texture_data_type with a bool called "combine_using_add"?  That way
>      someone reading manual_blend_average() won't have to refer to
>      emit_combine() to understand what the algorithm does; and similarly
>      someone reading emt_combine() won't have to look at
>      manual_blend_average() to understand why we use ADD for floats and AVG
>      for ints.
> 
>    On further reflection, I think it would be even better to replace the
>    texture_data_type argument with an opcode argument--that way the caller
>    can pass in BRW_OPCODE_ADD or BRW_OPCODE_AVG.  Once we get to patch 42/42,
>    this function can be changed to just plumb the opcode straight through
>    into the fs_inst constructor.

I like that, version two on its way, thanks!