st/mesa: Override blend factors involving alpha if it doesn't exist.

Submitted by Kenneth Graunke on Aug. 25, 2018, 7:48 a.m.

Details

Message ID 20180825074843.20457-1-kenneth@whitecape.org
State New
Headers show
Series "st/mesa: Override blend factors involving alpha if it doesn't exist." ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Kenneth Graunke Aug. 25, 2018, 7:48 a.m.
When faking an RGB format with an RGBA format, there may be a channel
of data containing garbage.  st/mesa already overrides texture swizzles
to replace the A channel with ONE.  This patch makes it override blend
factors to achieve a similar effect.

It appears that st_update_blend is already called when the framebuffer
is changed, so it should be safe to look at the render target formats
without adding additional state dependencies.
---
 src/mesa/state_tracker/st_atom_blend.c | 35 ++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/mesa/state_tracker/st_atom_blend.c b/src/mesa/state_tracker/st_atom_blend.c
index 804de2f154f..3e6e4411107 100644
--- a/src/mesa/state_tracker/st_atom_blend.c
+++ b/src/mesa/state_tracker/st_atom_blend.c
@@ -41,6 +41,7 @@ 
 
 #include "framebuffer.h"
 #include "main/blend.h"
+#include "main/glformats.h"
 #include "main/macros.h"
 
 /**
@@ -142,6 +143,28 @@  blend_per_rt(const struct gl_context *ctx, unsigned num_cb)
    return GL_FALSE;
 }
 
+/**
+ * Modify blend function to force destination alpha to 1.0
+ *
+ * If \c function specifies a blend function that uses destination alpha,
+ * replace it with a function that hard-wires destination alpha to 1.0.  This
+ * is used when rendering to xRGB targets.
+ */
+static enum pipe_blendfactor
+fix_xrgb_alpha(enum pipe_blendfactor factor)
+{
+   switch (factor) {
+   case PIPE_BLENDFACTOR_DST_ALPHA:
+      return PIPE_BLENDFACTOR_ONE;
+
+   case PIPE_BLENDFACTOR_INV_DST_ALPHA:
+   case PIPE_BLENDFACTOR_SRC_ALPHA_SATURATE:
+      return PIPE_BLENDFACTOR_ZERO;
+   default:
+      return factor;
+   }
+}
+
 void
 st_update_blend( struct st_context *st )
 {
@@ -210,6 +233,18 @@  st_update_blend( struct st_context *st )
             blend->rt[i].alpha_dst_factor =
                translate_blend(ctx->Color.Blend[j].DstA);
          }
+
+         const struct gl_renderbuffer *rb =
+            ctx->DrawBuffer->_ColorDrawBuffers[i];
+
+         if (rb && !_mesa_base_format_has_channel(rb->_BaseFormat,
+                                                  GL_TEXTURE_ALPHA_TYPE)) {
+            struct pipe_rt_blend_state *rt = &blend->rt[i];
+            rt->rgb_src_factor = fix_xrgb_alpha(rt->rgb_src_factor);
+            rt->rgb_dst_factor = fix_xrgb_alpha(rt->rgb_dst_factor);
+            rt->alpha_src_factor = fix_xrgb_alpha(rt->alpha_src_factor);
+            rt->alpha_dst_factor = fix_xrgb_alpha(rt->alpha_dst_factor);
+         }
       }
    }
    else {

Comments

You have to make sure to flip blend->independent_blend_enable to 1 in
that case, and that will only work on some (well, most, but not all)
hardware. Pre-something Tesla-era and I assume r600-era gpu's must
have all rt's configured the same way.

I'm also not 100% sure that it's OK to access the draw buffers in
here, but Marek would know for sure -- he restructured the atoms logic
a while back, and I haven't re-learned the new scheme.

  -ilia

On Sat, Aug 25, 2018 at 3:48 AM, Kenneth Graunke <kenneth@whitecape.org> wrote:
> When faking an RGB format with an RGBA format, there may be a channel
> of data containing garbage.  st/mesa already overrides texture swizzles
> to replace the A channel with ONE.  This patch makes it override blend
> factors to achieve a similar effect.
>
> It appears that st_update_blend is already called when the framebuffer
> is changed, so it should be safe to look at the render target formats
> without adding additional state dependencies.
> ---
>  src/mesa/state_tracker/st_atom_blend.c | 35 ++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/src/mesa/state_tracker/st_atom_blend.c b/src/mesa/state_tracker/st_atom_blend.c
> index 804de2f154f..3e6e4411107 100644
> --- a/src/mesa/state_tracker/st_atom_blend.c
> +++ b/src/mesa/state_tracker/st_atom_blend.c
> @@ -41,6 +41,7 @@
>
>  #include "framebuffer.h"
>  #include "main/blend.h"
> +#include "main/glformats.h"
>  #include "main/macros.h"
>
>  /**
> @@ -142,6 +143,28 @@ blend_per_rt(const struct gl_context *ctx, unsigned num_cb)
>     return GL_FALSE;
>  }
>
> +/**
> + * Modify blend function to force destination alpha to 1.0
> + *
> + * If \c function specifies a blend function that uses destination alpha,
> + * replace it with a function that hard-wires destination alpha to 1.0.  This
> + * is used when rendering to xRGB targets.
> + */
> +static enum pipe_blendfactor
> +fix_xrgb_alpha(enum pipe_blendfactor factor)
> +{
> +   switch (factor) {
> +   case PIPE_BLENDFACTOR_DST_ALPHA:
> +      return PIPE_BLENDFACTOR_ONE;
> +
> +   case PIPE_BLENDFACTOR_INV_DST_ALPHA:
> +   case PIPE_BLENDFACTOR_SRC_ALPHA_SATURATE:
> +      return PIPE_BLENDFACTOR_ZERO;
> +   default:
> +      return factor;
> +   }
> +}
> +
>  void
>  st_update_blend( struct st_context *st )
>  {
> @@ -210,6 +233,18 @@ st_update_blend( struct st_context *st )
>              blend->rt[i].alpha_dst_factor =
>                 translate_blend(ctx->Color.Blend[j].DstA);
>           }
> +
> +         const struct gl_renderbuffer *rb =
> +            ctx->DrawBuffer->_ColorDrawBuffers[i];
> +
> +         if (rb && !_mesa_base_format_has_channel(rb->_BaseFormat,
> +                                                  GL_TEXTURE_ALPHA_TYPE)) {
> +            struct pipe_rt_blend_state *rt = &blend->rt[i];
> +            rt->rgb_src_factor = fix_xrgb_alpha(rt->rgb_src_factor);
> +            rt->rgb_dst_factor = fix_xrgb_alpha(rt->rgb_dst_factor);
> +            rt->alpha_src_factor = fix_xrgb_alpha(rt->alpha_src_factor);
> +            rt->alpha_dst_factor = fix_xrgb_alpha(rt->alpha_dst_factor);
> +         }
>        }
>     }
>     else {
> --
> 2.18.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Yeah, this will be more complicated because it's per RT.

I suggest adding a PIPE_CAP for the hw capability to force DST_ALPHA
to 0, and applying this workaround only if the PIPE_CAP is 0.

Marek


On Sat, Aug 25, 2018 at 10:46 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> You have to make sure to flip blend->independent_blend_enable to 1 in
> that case, and that will only work on some (well, most, but not all)
> hardware. Pre-something Tesla-era and I assume r600-era gpu's must
> have all rt's configured the same way.
>
> I'm also not 100% sure that it's OK to access the draw buffers in
> here, but Marek would know for sure -- he restructured the atoms logic
> a while back, and I haven't re-learned the new scheme.
>
>   -ilia
>
> On Sat, Aug 25, 2018 at 3:48 AM, Kenneth Graunke <kenneth@whitecape.org> wrote:
>> When faking an RGB format with an RGBA format, there may be a channel
>> of data containing garbage.  st/mesa already overrides texture swizzles
>> to replace the A channel with ONE.  This patch makes it override blend
>> factors to achieve a similar effect.
>>
>> It appears that st_update_blend is already called when the framebuffer
>> is changed, so it should be safe to look at the render target formats
>> without adding additional state dependencies.
>> ---
>>  src/mesa/state_tracker/st_atom_blend.c | 35 ++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/src/mesa/state_tracker/st_atom_blend.c b/src/mesa/state_tracker/st_atom_blend.c
>> index 804de2f154f..3e6e4411107 100644
>> --- a/src/mesa/state_tracker/st_atom_blend.c
>> +++ b/src/mesa/state_tracker/st_atom_blend.c
>> @@ -41,6 +41,7 @@
>>
>>  #include "framebuffer.h"
>>  #include "main/blend.h"
>> +#include "main/glformats.h"
>>  #include "main/macros.h"
>>
>>  /**
>> @@ -142,6 +143,28 @@ blend_per_rt(const struct gl_context *ctx, unsigned num_cb)
>>     return GL_FALSE;
>>  }
>>
>> +/**
>> + * Modify blend function to force destination alpha to 1.0
>> + *
>> + * If \c function specifies a blend function that uses destination alpha,
>> + * replace it with a function that hard-wires destination alpha to 1.0.  This
>> + * is used when rendering to xRGB targets.
>> + */
>> +static enum pipe_blendfactor
>> +fix_xrgb_alpha(enum pipe_blendfactor factor)
>> +{
>> +   switch (factor) {
>> +   case PIPE_BLENDFACTOR_DST_ALPHA:
>> +      return PIPE_BLENDFACTOR_ONE;
>> +
>> +   case PIPE_BLENDFACTOR_INV_DST_ALPHA:
>> +   case PIPE_BLENDFACTOR_SRC_ALPHA_SATURATE:
>> +      return PIPE_BLENDFACTOR_ZERO;
>> +   default:
>> +      return factor;
>> +   }
>> +}
>> +
>>  void
>>  st_update_blend( struct st_context *st )
>>  {
>> @@ -210,6 +233,18 @@ st_update_blend( struct st_context *st )
>>              blend->rt[i].alpha_dst_factor =
>>                 translate_blend(ctx->Color.Blend[j].DstA);
>>           }
>> +
>> +         const struct gl_renderbuffer *rb =
>> +            ctx->DrawBuffer->_ColorDrawBuffers[i];
>> +
>> +         if (rb && !_mesa_base_format_has_channel(rb->_BaseFormat,
>> +                                                  GL_TEXTURE_ALPHA_TYPE)) {
>> +            struct pipe_rt_blend_state *rt = &blend->rt[i];
>> +            rt->rgb_src_factor = fix_xrgb_alpha(rt->rgb_src_factor);
>> +            rt->rgb_dst_factor = fix_xrgb_alpha(rt->rgb_dst_factor);
>> +            rt->alpha_src_factor = fix_xrgb_alpha(rt->alpha_src_factor);
>> +            rt->alpha_dst_factor = fix_xrgb_alpha(rt->alpha_dst_factor);
>> +         }
>>        }
>>     }
>>     else {
>> --
>> 2.18.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Monday, August 27, 2018 11:05:19 AM PDT Marek Olšák wrote:
> Yeah, this will be more complicated because it's per RT.
> 
> I suggest adding a PIPE_CAP for the hw capability to force DST_ALPHA
> to 0, and applying this workaround only if the PIPE_CAP is 0.
> 
> Marek

I was thinking of applying this hack based on PIPE_CAP_INDEP_BLEND_FUNC.

Ilia seemed to think that it was unsupportable on older Nouveau (without
independent alpha blending), but harmless either way on modern Nouveau.
(Supposedly Freedreno does this workaround in the driver...)

Would you prefer not to have this triggered?
On Mon, Aug 27, 2018 at 5:55 PM, Kenneth Graunke <kenneth@whitecape.org> wrote:
> On Monday, August 27, 2018 11:05:19 AM PDT Marek Olšák wrote:
>> Yeah, this will be more complicated because it's per RT.
>>
>> I suggest adding a PIPE_CAP for the hw capability to force DST_ALPHA
>> to 0, and applying this workaround only if the PIPE_CAP is 0.
>>
>> Marek
>
> I was thinking of applying this hack based on PIPE_CAP_INDEP_BLEND_FUNC.
>
> Ilia seemed to think that it was unsupportable on older Nouveau (without
> independent alpha blending), but harmless either way on modern Nouveau.
> (Supposedly Freedreno does this workaround in the driver...)
>
> Would you prefer not to have this triggered?

Yes. I think it would be unnecessary overhead for us.

Marek
On Monday, August 27, 2018 6:18:21 PM PDT Marek Olšák wrote:
> On Mon, Aug 27, 2018 at 5:55 PM, Kenneth Graunke <kenneth@whitecape.org> wrote:
> > On Monday, August 27, 2018 11:05:19 AM PDT Marek Olšák wrote:
> >> Yeah, this will be more complicated because it's per RT.
> >>
> >> I suggest adding a PIPE_CAP for the hw capability to force DST_ALPHA
> >> to 0, and applying this workaround only if the PIPE_CAP is 0.
> >>
> >> Marek
> >
> > I was thinking of applying this hack based on PIPE_CAP_INDEP_BLEND_FUNC.
> >
> > Ilia seemed to think that it was unsupportable on older Nouveau (without
> > independent alpha blending), but harmless either way on modern Nouveau.
> > (Supposedly Freedreno does this workaround in the driver...)
> >
> > Would you prefer not to have this triggered?
> 
> Yes. I think it would be unnecessary overhead for us.
> 
> Marek

Okay, cool, thanks for the feedback.  Let's shelve this for now.

I'm thinking of reworking how I handle RGBX -> RGBA stuff anyway,
and may need to handle it in the driver in the end.  I'll see how
things look and either handle it myself or come up with a version
of this patch which makes the hack optional.