[Mesa-dev,1/2] st/mesa: adjust blending modes if we don't have destination alpha

Submitted by Marek Olšák on April 29, 2015, 11:52 a.m.

Details

Message ID CAAxE2A7HW0DcHGgP3MD+EnR9A5VrxwKs_A0G4071FK-2OOBpjQ@mail.gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Marek Olšák April 29, 2015, 11:52 a.m.
On Wed, Apr 29, 2015 at 12:44 PM, Jose Fonseca <jfonseca@vmware.com> wrote:
> I think there are two different things here: one is the driver internally
> fakes BGRX with BGRA, and obviously it's the pipe driver that needs to fix
> up alpha channel blending to simulate it's one.
>
> The other is the state tracker is faking BGRX with BGRA, and in that case,
> it's the state tracker that needs to do the fix up (as the pipe has no way
> to know if that's intentional or not).

That's a fair point. On the other hand, I don't see why drivers can't
support BGRX even if they can force A=1 for texturing only.

>
> Or are you saying that state trackers should never attempt to fulfill BGRX
> formats with BGRA, and that every driver needs to workaround this
> internally?

Not necessarily.

>
> I don't fill too strongly about this, but it wouldn't sound consistent.
> Nowadays have a pipe cap for every single little thing out there.  I don't
> see a reason to treat render target formats differently. Otherwise something
> that can be easily done once in the state tracker now needs to be replicated
> in every driver.
>
>
> But you have a point about _NEW_BUFFERS.  It could be avoided.  We could
> introduce a new flag _NEW_BUFFERS_EMULATED, set when fbo's with emulated
> formats (e.g., backing BGRX with BGRA), so that drivers that support (or
> workaround) these things internally don't .  On the other hand, _NEW_BUFFERS
> probably doesn't change that frequently, blend state probably changes much
> more often.

A new CAP would work. There is an easier way to do it though, see the
attached patch (assuming blend_force_dst_alpha_to_one is set
correctly).

Marek

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 6bb4077..0d1542f 100644
--- a/src/mesa/state_tracker/st_atom_blend.c
+++ b/src/mesa/state_tracker/st_atom_blend.c
@@ -286,7 +286,7 @@  const struct st_tracked_state st_update_blend = {
    "st_update_blend",					/* name */
    {							/* dirty */
       (_NEW_COLOR | _NEW_MULTISAMPLE),  /* XXX _NEW_BLEND someday? */	/* mesa */
-      0,						/* st */
+      ST_NEW_BLEND,					/* st */
    },
    update_blend,					/* update */
 };
diff --git a/src/mesa/state_tracker/st_context.c b/src/mesa/state_tracker/st_context.c
index 5fe132a..d7ebe22 100644
--- a/src/mesa/state_tracker/st_context.c
+++ b/src/mesa/state_tracker/st_context.c
@@ -95,6 +95,10 @@  void st_invalidate_state(struct gl_context * ctx, GLuint new_state)
       st->dirty.st |= ST_NEW_VERTEX_PROGRAM;
    }
 
+   if (st->blend_force_dst_alpha_to_one && (new_state & _NEW_BUFFERS)) {
+      st->dirty.st |= ST_NEW_BLEND;
+   }
+
    st->dirty.mesa |= new_state;
    st->dirty.st |= ST_NEW_MESA;
 
diff --git a/src/mesa/state_tracker/st_context.h b/src/mesa/state_tracker/st_context.h
index 8a9504b..a9c861d 100644
--- a/src/mesa/state_tracker/st_context.h
+++ b/src/mesa/state_tracker/st_context.h
@@ -53,7 +53,7 @@  struct u_upload_mgr;
 #define ST_NEW_FRAGMENT_PROGRAM        (1 << 1)
 #define ST_NEW_VERTEX_PROGRAM          (1 << 2)
 #define ST_NEW_FRAMEBUFFER             (1 << 3)
-/* gap, re-use it */
+#define ST_NEW_BLEND                   (1 << 4)
 #define ST_NEW_GEOMETRY_PROGRAM        (1 << 5)
 #define ST_NEW_VERTEX_ARRAYS           (1 << 6)
 #define ST_NEW_RASTERIZER              (1 << 7)
@@ -89,6 +89,7 @@  struct st_context
    struct draw_stage *rastpos_stage;  /**< For glRasterPos */
    GLboolean clamp_frag_color_in_shader;
    GLboolean clamp_vert_color_in_shader;
+   GLboolean blend_force_dst_alpha_to_one;
    boolean has_stencil_export; /**< can do shader stencil export? */
    boolean has_time_elapsed;
    boolean has_shader_model3;

Comments

On 04/29/2015 05:52 AM, Marek Olšák wrote:
> On Wed, Apr 29, 2015 at 12:44 PM, Jose Fonseca <jfonseca@vmware.com> wrote:
>> I think there are two different things here: one is the driver internally
>> fakes BGRX with BGRA, and obviously it's the pipe driver that needs to fix
>> up alpha channel blending to simulate it's one.
>>
>> The other is the state tracker is faking BGRX with BGRA, and in that case,
>> it's the state tracker that needs to do the fix up (as the pipe has no way
>> to know if that's intentional or not).
>
> That's a fair point. On the other hand, I don't see why drivers can't
> support BGRX even if they can force A=1 for texturing only.

Yeah, the key thing is whether the driver advertises support for alpha 
"X" formats.  We've only been doing that for one case 
(PIPE_FORMAT_B8G8R8X8_UNORM) which directly corresponds to an SVGA3D_ 
format (SVGA3D_X8R8G8B8).

If the state tracker can always find a PIPE_FORMAT_RGBX format for a 
GL_RGB format then the driver can detect this particular blend / format 
combination and work things out itself.

But if the state tracker can't find a PIPE_FORMAT_RGBX format for GL_RGB 
and uses a PIPE_FORMAT_RGBA format, it's really up to the state tracker 
to fix-up the blend mode in this case.

The gallium interface has never specified a required set of surface 
formats.  Maybe we need to establish a policy for that first.


>> Or are you saying that state trackers should never attempt to fulfill BGRX
>> formats with BGRA, and that every driver needs to workaround this
>> internally?
>
> Not necessarily.
>
>>
>> I don't fill too strongly about this, but it wouldn't sound consistent.
>> Nowadays have a pipe cap for every single little thing out there.  I don't
>> see a reason to treat render target formats differently. Otherwise something
>> that can be easily done once in the state tracker now needs to be replicated
>> in every driver.
>>
>>
>> But you have a point about _NEW_BUFFERS.  It could be avoided.  We could
>> introduce a new flag _NEW_BUFFERS_EMULATED, set when fbo's with emulated
>> formats (e.g., backing BGRX with BGRA), so that drivers that support (or
>> workaround) these things internally don't .  On the other hand, _NEW_BUFFERS
>> probably doesn't change that frequently, blend state probably changes much
>> more often.
>
> A new CAP would work. There is an easier way to do it though, see the
> attached patch (assuming blend_force_dst_alpha_to_one is set
> correctly).

I guess we could do yet another new cap.

I'm probably not going to pursue this any further right now.  I'm going 
off-line for a few days and have other things to finish up.

In any case, this is an odd state combination that a real app would 
never hit.  Why would one use GL_DST_ALPHA if the dest surface doesn't 
have an alpha channel??  I am tempted to change the piglit 
fbo-blend-formats test to skip the GL_DST_ALPHA blend test if the 
surface doesn't have alpha.  As it is, we get failures for some formats 
when, in fact, all the other blend modes work fine.  It's just this odd 
case that's spoiling the party and given a false impression of brokenness.

-Brian
On 29/04/15 15:56, Brian Paul wrote:
> On 04/29/2015 05:52 AM, Marek Olšák wrote:
>> On Wed, Apr 29, 2015 at 12:44 PM, Jose Fonseca <jfonseca@vmware.com>
>> wrote:
>>> I think there are two different things here: one is the driver
>>> internally
>>> fakes BGRX with BGRA, and obviously it's the pipe driver that needs
>>> to fix
>>> up alpha channel blending to simulate it's one.
>>>
>>> The other is the state tracker is faking BGRX with BGRA, and in that
>>> case,
>>> it's the state tracker that needs to do the fix up (as the pipe has
>>> no way
>>> to know if that's intentional or not).
>>
>> That's a fair point. On the other hand, I don't see why drivers can't
>> support BGRX even if they can force A=1 for texturing only.
>
> Yeah, the key thing is whether the driver advertises support for alpha
> "X" formats.  We've only been doing that for one case
> (PIPE_FORMAT_B8G8R8X8_UNORM) which directly corresponds to an SVGA3D_
> format (SVGA3D_X8R8G8B8).
>
> If the state tracker can always find a PIPE_FORMAT_RGBX format for a
> GL_RGB format then the driver can detect this particular blend / format
> combination and work things out itself.
>
> But if the state tracker can't find a PIPE_FORMAT_RGBX format for GL_RGB
> and uses a PIPE_FORMAT_RGBA format, it's really up to the state tracker
> to fix-up the blend mode in this case.
>
> The gallium interface has never specified a required set of surface
> formats.  Maybe we need to establish a policy for that first.
>
>
>>> Or are you saying that state trackers should never attempt to fulfill
>>> BGRX
>>> formats with BGRA, and that every driver needs to workaround this
>>> internally?
>>
>> Not necessarily.
>>
>>>
>>> I don't fill too strongly about this, but it wouldn't sound consistent.
>>> Nowadays have a pipe cap for every single little thing out there.  I
>>> don't
>>> see a reason to treat render target formats differently. Otherwise
>>> something
>>> that can be easily done once in the state tracker now needs to be
>>> replicated
>>> in every driver.
>>>
>>>
>>> But you have a point about _NEW_BUFFERS.  It could be avoided.  We could
>>> introduce a new flag _NEW_BUFFERS_EMULATED, set when fbo's with emulated
>>> formats (e.g., backing BGRX with BGRA), so that drivers that support (or
>>> workaround) these things internally don't .  On the other hand,
>>> _NEW_BUFFERS
>>> probably doesn't change that frequently, blend state probably changes
>>> much
>>> more often.
>>
>> A new CAP would work. There is an easier way to do it though, see the
>> attached patch (assuming blend_force_dst_alpha_to_one is set
>> correctly).
>
> I guess we could do yet another new cap.

I think that the is_format_support suffices.  Like you said, if the 
driver doesn't support the RGBX format, the state tracker will emulate 
it with the RGBA.  So no new cap necessary, unless some driver actually 
doesn't want this automatic conversion.

Jose