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

2014-01-20 Thread Paul Berry 
On 20 December 2013 06:38, Topi Pohjolainen topi.pohjolai...@intel.comwrote:

 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.

With that change, this patch is:

Reviewed-by: Paul Berry stereotype...@gmail.com
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2014-01-20 Thread Paul Berry 
On 20 January 2014 19:36, Paul Berry stereotype...@gmail.com wrote:

 On 20 December 2013 06:38, Topi Pohjolainen topi.pohjolai...@intel.comwrote:

 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.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2014-01-20 Thread Pohjolainen, Topi 
On Mon, Jan 20, 2014 at 08:30:41PM -0800, Paul Berry wrote:
On 20 January 2014 19:36, Paul Berry stereotype...@gmail.com wrote:
 
  On 20 December 2013 06:38, Topi Pohjolainen topi.pohjolai...@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!
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2013-12-20 Thread Topi Pohjolainen 
Signed-off-by: Topi Pohjolainen topi.pohjolai...@intel.com
---
 src/mesa/drivers/dri/i965/brw_blorp_blit.cpp| 13 -
 src/mesa/drivers/dri/i965/brw_blorp_blit_eu.cpp | 12 
 src/mesa/drivers/dri/i965/brw_blorp_blit_eu.h   |  5 +
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp 
b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
index 65b5403..f651846 100644
--- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
+++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
@@ -1529,12 +1529,6 @@ brw_blorp_blit_program::manual_blend_average(unsigned 
num_samples)
 * For integer formats, we replace the add operations with average
 * operations and skip the final division.
 */
-   typedef struct brw_instruction *(*brw_op2_ptr)(struct brw_compile *,
-  struct brw_reg,
-  struct brw_reg,
-  struct brw_reg);
-   brw_op2_ptr combine_op =
-  key-texture_data_type == BRW_REGISTER_TYPE_F ? brw_ADD : brw_AVG;
unsigned stack_depth = 0;
for (unsigned i = 0; i  num_samples; ++i) {
   assert(stack_depth == _mesa_bitcount(i)); /* Loop invariant */
@@ -1576,9 +1570,10 @@ brw_blorp_blit_program::manual_blend_average(unsigned 
num_samples)
 
  /* TODO: should use a smaller loop bound for non_RGBA formats */
  for (int k = 0; k  4; ++k) {
-combine_op(func, offset(texture_data[stack_depth - 1], 2*k),
-   offset(vec8(texture_data[stack_depth - 1]), 2*k),
-   offset(vec8(texture_data[stack_depth]), 2*k));
+emit_combine(key-texture_data_type,
+ offset(texture_data[stack_depth - 1], 2*k),
+ offset(vec8(texture_data[stack_depth - 1]), 2*k),
+ offset(vec8(texture_data[stack_depth]), 2*k));
  }
   }
}
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);
+}
diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.h 
b/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.h
index 5f0c8cf..00624c9 100644
--- a/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.h
+++ b/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.h
@@ -52,6 +52,11 @@ protected:
  unsigned msg_length,
  bool use_header);
 
+   void emit_combine(unsigned texture_data_type,
+ const struct brw_reg dst,
+ const struct brw_reg src_1,
+ const struct brw_reg src_2);
+
void *mem_ctx;
struct brw_compile func;
 };
-- 
1.8.3.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev