Re: [Mesa-dev] [PATCH] radeonsi: Fix blending using destination alpha factor but non-alpha destination

2013-02-19 Thread Christian König 

Am 18.02.2013 20:11, schrieb Roland Scheidegger:

Am 18.02.2013 19:14, schrieb Michel Dänzer:

From: Michel Dänzer michel.daen...@amd.com

11 more little piglits.

NOTE: This is a candidate for the 9.1 branch.

Signed-off-by: Michel Dänzer michel.daen...@amd.com
---

Any ideas why this seems necessary with radeonsi but not with r600g?

Maybe the hw uses an implicit 1 if the format has no alpha (though I'm
not sure if it can always know with bgrx formats and the like).
I'm wondering if there should be a helper for those fixups. Looks to me
like quite some drivers need it (though well so far I think just
non-gallium i965 does this plus llvmpipe, but for some of the others I'm
skeptical if not doing it is really correct...).


I agree alpha blending with a buffer format that doesn't have alpha is a 
bit strange, that should be catched by the upper layers.





  src/gallium/drivers/radeonsi/si_state.c | 116 +---
  src/gallium/drivers/radeonsi/si_state.h |   3 +-
  2 files changed, 61 insertions(+), 58 deletions(-)

diff --git a/src/gallium/drivers/radeonsi/si_state.c 
b/src/gallium/drivers/radeonsi/si_state.c
index d20e3ff..144a29d 100644
--- a/src/gallium/drivers/radeonsi/si_state.c
+++ b/src/gallium/drivers/radeonsi/si_state.c
@@ -36,33 +36,6 @@
  #include si_state.h
  #include sid.h
  
-/*

- * inferred framebuffer and blender state
- */
-static void si_update_fb_blend_state(struct r600_context *rctx)
-{
-   struct si_pm4_state *pm4;
-   struct si_state_blend *blend = rctx-queued.named.blend;
-   uint32_t mask;
-
-   if (blend == NULL)
-   return;
-
-   pm4 = CALLOC_STRUCT(si_pm4_state);
-   if (pm4 == NULL)
-   return;
-
-   mask = (1ULL  ((unsigned)rctx-framebuffer.nr_cbufs * 4)) - 1;
-   mask = blend-cb_target_mask;
-   si_pm4_set_reg(pm4, R_028238_CB_TARGET_MASK, mask);
-
-   si_pm4_set_state(rctx, fb_blend, pm4);
-}
-
-/*
- * Blender functions
- */
-
  static uint32_t si_translate_blend_function(int blend_func)
  {
switch (blend_func) {
@@ -84,7 +57,7 @@ static uint32_t si_translate_blend_function(int blend_func)
return 0;
  }
  
-static uint32_t si_translate_blend_factor(int blend_fact)

+static uint32_t si_translate_blend_factor(int blend_fact, bool dst_alpha)
  {
switch (blend_fact) {
case PIPE_BLENDFACTOR_ONE:
@@ -94,7 +67,7 @@ static uint32_t si_translate_blend_factor(int blend_fact)
case PIPE_BLENDFACTOR_SRC_ALPHA:
return V_028780_BLEND_SRC_ALPHA;
case PIPE_BLENDFACTOR_DST_ALPHA:
-   return V_028780_BLEND_DST_ALPHA;
+   return dst_alpha ? V_028780_BLEND_DST_ALPHA : 
V_028780_BLEND_ONE;
case PIPE_BLENDFACTOR_DST_COLOR:
return V_028780_BLEND_DST_COLOR;
case PIPE_BLENDFACTOR_SRC_ALPHA_SATURATE:
@@ -110,7 +83,7 @@ static uint32_t si_translate_blend_factor(int blend_fact)
case PIPE_BLENDFACTOR_INV_SRC_ALPHA:
return V_028780_BLEND_ONE_MINUS_SRC_ALPHA;
case PIPE_BLENDFACTOR_INV_DST_ALPHA:
-   return V_028780_BLEND_ONE_MINUS_DST_ALPHA;
+   return dst_alpha ? V_028780_BLEND_ONE_MINUS_DST_ALPHA : 
V_028780_BLEND_ZERO;
case PIPE_BLENDFACTOR_INV_DST_COLOR:
return V_028780_BLEND_ONE_MINUS_DST_COLOR;
case PIPE_BLENDFACTOR_INV_CONST_COLOR:
@@ -133,30 +106,25 @@ static uint32_t si_translate_blend_factor(int blend_fact)
return 0;
  }

I think you might also need to patch up SRC_ALPHA_SATURATE (to zero).

Can't comment on the hw stuff but at least llvmpipe does the same
otherwise :-).


Why should we do so? SRC_ALPHA_SATURATE should still work fine, even 
when the destination buffer doesn't have an alpha component.


Christian.



Roland
___
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

Re: [Mesa-dev] [PATCH] radeonsi: Fix blending using destination alpha factor but non-alpha destination

2013-02-19 Thread Marek Olšák 
On Tue, Feb 19, 2013 at 10:33 AM, Christian König
deathsim...@vodafone.de wrote:
 Am 18.02.2013 20:11, schrieb Roland Scheidegger:

 Am 18.02.2013 19:14, schrieb Michel Dänzer:

 From: Michel Dänzer michel.daen...@amd.com

 11 more little piglits.

 NOTE: This is a candidate for the 9.1 branch.

 Signed-off-by: Michel Dänzer michel.daen...@amd.com
 ---

 Any ideas why this seems necessary with radeonsi but not with r600g?

 Maybe the hw uses an implicit 1 if the format has no alpha (though I'm
 not sure if it can always know with bgrx formats and the like).
 I'm wondering if there should be a helper for those fixups. Looks to me
 like quite some drivers need it (though well so far I think just
 non-gallium i965 does this plus llvmpipe, but for some of the others I'm
 skeptical if not doing it is really correct...).


 I agree alpha blending with a buffer format that doesn't have alpha is a bit
 strange, that should be catched by the upper layers.

I think it's better to do that in drivers instead.

r300g also uses a different blend state for RGBX and RGBA. The R300
blend state CSO actually contains 11 command buffers and the driver
switches between them when needed. Two of those command buffers
contain blend state for RGBX and RGBA. This approach of having
multiple command buffers per CSO has a much lower overhead than any
other solution I've seen (including rebuilding states on the fly and
having the state tracker figure it out).

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

Re: [Mesa-dev] [PATCH] radeonsi: Fix blending using destination alpha factor but non-alpha destination

2013-02-19 Thread Marek Olšák 
On Tue, Feb 19, 2013 at 11:02 AM, Michel Dänzer mic...@daenzer.net wrote:
 On Die, 2013-02-19 at 10:33 +0100, Christian König wrote:
 Am 18.02.2013 20:11, schrieb Roland Scheidegger:
  Am 18.02.2013 19:14, schrieb Michel Dänzer:
  From: Michel Dänzer michel.daen...@amd.com
 
  11 more little piglits.
 
  NOTE: This is a candidate for the 9.1 branch.
 
  Signed-off-by: Michel Dänzer michel.daen...@amd.com
  ---
 
  Any ideas why this seems necessary with radeonsi but not with r600g?
  Maybe the hw uses an implicit 1 if the format has no alpha (though I'm
  not sure if it can always know with bgrx formats and the like).
  I'm wondering if there should be a helper for those fixups. Looks to me
  like quite some drivers need it (though well so far I think just
  non-gallium i965 does this plus llvmpipe, but for some of the others I'm
  skeptical if not doing it is really correct...).

 I agree alpha blending with a buffer format that doesn't have alpha is a
 bit strange, that should be catched by the upper layers.

 If it was that simple. :\

 The problem is that AFAICT for formats such as R8G8B8X8, there's no
 other way to tell the hardware to always use 1 for the destination
 alpha. And I'm not sure we can just not support any such formats, I
 certainly don't think that would be a good idea.


  @@ -84,7 +57,7 @@ static uint32_t si_translate_blend_function(int 
  blend_func)
 return 0;
}
 
  -static uint32_t si_translate_blend_factor(int blend_fact)
  +static uint32_t si_translate_blend_factor(int blend_fact, bool dst_alpha)
{
 switch (blend_fact) {
 case PIPE_BLENDFACTOR_ONE:
  @@ -94,7 +67,7 @@ static uint32_t si_translate_blend_factor(int 
  blend_fact)
 case PIPE_BLENDFACTOR_SRC_ALPHA:
 return V_028780_BLEND_SRC_ALPHA;
 case PIPE_BLENDFACTOR_DST_ALPHA:
  -  return V_028780_BLEND_DST_ALPHA;
  +  return dst_alpha ? V_028780_BLEND_DST_ALPHA : 
  V_028780_BLEND_ONE;
 case PIPE_BLENDFACTOR_DST_COLOR:
 return V_028780_BLEND_DST_COLOR;
 case PIPE_BLENDFACTOR_SRC_ALPHA_SATURATE:
  @@ -110,7 +83,7 @@ static uint32_t si_translate_blend_factor(int 
  blend_fact)
 case PIPE_BLENDFACTOR_INV_SRC_ALPHA:
 return V_028780_BLEND_ONE_MINUS_SRC_ALPHA;
 case PIPE_BLENDFACTOR_INV_DST_ALPHA:
  -  return V_028780_BLEND_ONE_MINUS_DST_ALPHA;
  +  return dst_alpha ? V_028780_BLEND_ONE_MINUS_DST_ALPHA : 
  V_028780_BLEND_ZERO;
 case PIPE_BLENDFACTOR_INV_DST_COLOR:
 return V_028780_BLEND_ONE_MINUS_DST_COLOR;
 case PIPE_BLENDFACTOR_INV_CONST_COLOR:
  @@ -133,30 +106,25 @@ static uint32_t si_translate_blend_factor(int 
  blend_fact)
 return 0;
}
  I think you might also need to patch up SRC_ALPHA_SATURATE (to zero).
 
  Can't comment on the hw stuff but at least llvmpipe does the same
  otherwise :-).

 Why should we do so? SRC_ALPHA_SATURATE should still work fine, even
 when the destination buffer doesn't have an alpha component.

 I think Roland is right. When the destination has no alpha, the
 destination alpha value is supposed to be always 1, so
 SRC_ALPHA_SATURATE is always 0. But with a format as described above,
 the destination X8 channel may contain any value.


 Really, what I don't understand is why r600g doesn't seem affected by
 this... at least on my RS880 it's passing the piglit tests this change
 fixes with radeonsi. So maybe I'm just missing some magic bit for
 radeonsi.

RGB formats do fail fbo-blending-formats with r600g/redwood here.

However the alpha channel can sometimes contain 1 in memory even if
the format is RGBX. Off the top of my head, glClear, glTex[Sub]Image,
glCopyTex[Sub]Image always set alpha to 1. Blits do too if they use
RGBX as a source. One way to set alpha != 1 is to draw some geometry.

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

Re: [Mesa-dev] [PATCH] radeonsi: Fix blending using destination alpha factor but non-alpha destination

2013-02-19 Thread Michel Dänzer 
On Die, 2013-02-19 at 14:04 +0100, Marek Olšák wrote: 
 On Tue, Feb 19, 2013 at 11:02 AM, Michel Dänzer mic...@daenzer.net wrote:
 
  Really, what I don't understand is why r600g doesn't seem affected by
  this... at least on my RS880 it's passing the piglit tests this change
  fixes with radeonsi. So maybe I'm just missing some magic bit for
  radeonsi.
 
 RGB formats do fail fbo-blending-formats with r600g/redwood here.

Okay.


 However the alpha channel can sometimes contain 1 in memory even if
 the format is RGBX. Off the top of my head, glClear, glTex[Sub]Image,
 glCopyTex[Sub]Image always set alpha to 1.

Well, but they shouldn't for these formats. :) The memory corresponding
to X* channels should remain unchanged. I'm working on a separate patch
for that for radeonsi.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast |  Debian, X and DRI developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: Fix blending using destination alpha factor but non-alpha destination

2013-02-19 Thread Marek Olšák 
On Tue, Feb 19, 2013 at 3:28 PM, Michel Dänzer mic...@daenzer.net wrote:
 On Die, 2013-02-19 at 14:04 +0100, Marek Olšák wrote:
 On Tue, Feb 19, 2013 at 11:02 AM, Michel Dänzer mic...@daenzer.net wrote:
 
  Really, what I don't understand is why r600g doesn't seem affected by
  this... at least on my RS880 it's passing the piglit tests this change
  fixes with radeonsi. So maybe I'm just missing some magic bit for
  radeonsi.

 RGB formats do fail fbo-blending-formats with r600g/redwood here.

 Okay.


 However the alpha channel can sometimes contain 1 in memory even if
 the format is RGBX. Off the top of my head, glClear, glTex[Sub]Image,
 glCopyTex[Sub]Image always set alpha to 1.

 Well, but they shouldn't for these formats. :) The memory corresponding
 to X* channels should remain unchanged. I'm working on a separate patch
 for that for radeonsi.

I think the only way you could do that is to set the colormask to RGB.
Doesn't it have a negative effect on performance if some channels are
masked out?

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

Re: [Mesa-dev] [PATCH] radeonsi: Fix blending using destination alpha factor but non-alpha destination

2013-02-19 Thread Michel Dänzer 
On Die, 2013-02-19 at 15:48 +0100, Marek Olšák wrote: 
 On Tue, Feb 19, 2013 at 3:28 PM, Michel Dänzer mic...@daenzer.net wrote:
  On Die, 2013-02-19 at 14:04 +0100, Marek Olšák wrote:
  On Tue, Feb 19, 2013 at 11:02 AM, Michel Dänzer mic...@daenzer.net wrote:
  
   Really, what I don't understand is why r600g doesn't seem affected by
   this... at least on my RS880 it's passing the piglit tests this change
   fixes with radeonsi. So maybe I'm just missing some magic bit for
   radeonsi.
 
  RGB formats do fail fbo-blending-formats with r600g/redwood here.
 
  Okay.
 
 
  However the alpha channel can sometimes contain 1 in memory even if
  the format is RGBX. Off the top of my head, glClear, glTex[Sub]Image,
  glCopyTex[Sub]Image always set alpha to 1.
 
  Well, but they shouldn't for these formats. :) The memory corresponding
  to X* channels should remain unchanged. I'm working on a separate patch
  for that for radeonsi.
 
 I think the only way you could do that is to set the colormask to RGB.

Exactly.

 Doesn't it have a negative effect on performance if some channels are
 masked out?

It might, but I don't see that we really have a choice. If the app /
state tracker doesn't want to preserve those bits, it should use a
non-X* format.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast |  Debian, X and DRI developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: Fix blending using destination alpha factor but non-alpha destination

2013-02-18 Thread Roland Scheidegger 
Am 18.02.2013 19:14, schrieb Michel Dänzer:
 From: Michel Dänzer michel.daen...@amd.com
 
 11 more little piglits.
 
 NOTE: This is a candidate for the 9.1 branch.
 
 Signed-off-by: Michel Dänzer michel.daen...@amd.com
 ---
 
 Any ideas why this seems necessary with radeonsi but not with r600g?
Maybe the hw uses an implicit 1 if the format has no alpha (though I'm
not sure if it can always know with bgrx formats and the like).
I'm wondering if there should be a helper for those fixups. Looks to me
like quite some drivers need it (though well so far I think just
non-gallium i965 does this plus llvmpipe, but for some of the others I'm
skeptical if not doing it is really correct...).

 
  src/gallium/drivers/radeonsi/si_state.c | 116 
 +---
  src/gallium/drivers/radeonsi/si_state.h |   3 +-
  2 files changed, 61 insertions(+), 58 deletions(-)
 
 diff --git a/src/gallium/drivers/radeonsi/si_state.c 
 b/src/gallium/drivers/radeonsi/si_state.c
 index d20e3ff..144a29d 100644
 --- a/src/gallium/drivers/radeonsi/si_state.c
 +++ b/src/gallium/drivers/radeonsi/si_state.c
 @@ -36,33 +36,6 @@
  #include si_state.h
  #include sid.h
  
 -/*
 - * inferred framebuffer and blender state
 - */
 -static void si_update_fb_blend_state(struct r600_context *rctx)
 -{
 - struct si_pm4_state *pm4;
 - struct si_state_blend *blend = rctx-queued.named.blend;
 - uint32_t mask;
 -
 - if (blend == NULL)
 - return;
 -
 - pm4 = CALLOC_STRUCT(si_pm4_state);
 - if (pm4 == NULL)
 - return;
 -
 - mask = (1ULL  ((unsigned)rctx-framebuffer.nr_cbufs * 4)) - 1;
 - mask = blend-cb_target_mask;
 - si_pm4_set_reg(pm4, R_028238_CB_TARGET_MASK, mask);
 -
 - si_pm4_set_state(rctx, fb_blend, pm4);
 -}
 -
 -/*
 - * Blender functions
 - */
 -
  static uint32_t si_translate_blend_function(int blend_func)
  {
   switch (blend_func) {
 @@ -84,7 +57,7 @@ static uint32_t si_translate_blend_function(int blend_func)
   return 0;
  }
  
 -static uint32_t si_translate_blend_factor(int blend_fact)
 +static uint32_t si_translate_blend_factor(int blend_fact, bool dst_alpha)
  {
   switch (blend_fact) {
   case PIPE_BLENDFACTOR_ONE:
 @@ -94,7 +67,7 @@ static uint32_t si_translate_blend_factor(int blend_fact)
   case PIPE_BLENDFACTOR_SRC_ALPHA:
   return V_028780_BLEND_SRC_ALPHA;
   case PIPE_BLENDFACTOR_DST_ALPHA:
 - return V_028780_BLEND_DST_ALPHA;
 + return dst_alpha ? V_028780_BLEND_DST_ALPHA : 
 V_028780_BLEND_ONE;
   case PIPE_BLENDFACTOR_DST_COLOR:
   return V_028780_BLEND_DST_COLOR;
   case PIPE_BLENDFACTOR_SRC_ALPHA_SATURATE:
 @@ -110,7 +83,7 @@ static uint32_t si_translate_blend_factor(int blend_fact)
   case PIPE_BLENDFACTOR_INV_SRC_ALPHA:
   return V_028780_BLEND_ONE_MINUS_SRC_ALPHA;
   case PIPE_BLENDFACTOR_INV_DST_ALPHA:
 - return V_028780_BLEND_ONE_MINUS_DST_ALPHA;
 + return dst_alpha ? V_028780_BLEND_ONE_MINUS_DST_ALPHA : 
 V_028780_BLEND_ZERO;
   case PIPE_BLENDFACTOR_INV_DST_COLOR:
   return V_028780_BLEND_ONE_MINUS_DST_COLOR;
   case PIPE_BLENDFACTOR_INV_CONST_COLOR:
 @@ -133,30 +106,25 @@ static uint32_t si_translate_blend_factor(int 
 blend_fact)
   return 0;
  }

I think you might also need to patch up SRC_ALPHA_SATURATE (to zero).

Can't comment on the hw stuff but at least llvmpipe does the same
otherwise :-).

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