[Mesa-dev] [PATCH 12/18] i965: Convert CC state on gen4-5 to genxml.

Kenneth Graunke kenneth at whitecape.org
Mon Jun 19 22:25:17 UTC 2017 

On Monday, June 19, 2017 11:13:05 AM PDT Rafael Antognolli wrote:
> On Sat, Jun 17, 2017 at 11:31:51AM -0700, Kenneth Graunke wrote:
> > On Friday, June 16, 2017 4:31:25 PM PDT Rafael Antognolli wrote:
> > > Use set_blend_entry_bits and set_depth_stencil_bits to fill most of the
> > > color calc struct, and then manually update the rest.
> > > 
> > > Signed-off-by: Rafael Antognolli <rafael.antognolli at intel.com>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_cc.c            | 174 --------------------------
> > >  src/mesa/drivers/dri/i965/brw_state.h         |   1 -
> > >  src/mesa/drivers/dri/i965/brw_structs.h       |  92 --------------
> > >  src/mesa/drivers/dri/i965/brw_util.h          |   1 -
> > >  src/mesa/drivers/dri/i965/genX_state_upload.c |  99 ++++++++++++---
> > >  5 files changed, 81 insertions(+), 286 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_cc.c b/src/mesa/drivers/dri/i965/brw_cc.c
> > > index cdaa696..503ec83 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_cc.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_cc.c
[snip]
> > > @@ -2552,6 +2576,9 @@ set_blend_entry_bits(struct brw_context *brw, BLEND_ENTRY_GENXML *entry, int i,
> > >      */
> > >     const bool integer = ctx->DrawBuffer->_IntegerBuffers & (0x1 << i);
> > >  
> > > +   const unsigned blend_enabled = GEN_GEN >= 6 ?
> > > +      ctx->Color.BlendEnabled & (1 << i) : ctx->Color.BlendEnabled;
> > > +
> > :
> > I think always using ctx->Color.BlendEnabled & (1 << i) should be fine.
> > That corresponds to the enable bit for blend entry 0, which is the
> > only one we're handling here.  (Gen4-5 only support a single entry.)
> 
> I think I tried doing that, but I remember having some test failing, and that
> was because BlendEnabled was set to something like 0x2, and this test would
> catch it. But it could have been some other mistake on my side.
> 
> I'll double check this and see if it works like this.

Okay.  We can always leave it as is and revisit it later, if you prefer.
It's simpler, and I thought it would be safe, but maybe it isn't.

> > >     /* _NEW_COLOR */
> > >     if (ctx->Color.ColorLogicOpEnabled) {
> > >        GLenum rb_type = rb ? _mesa_get_format_datatype(rb->Format)
> > > @@ -2567,8 +2594,8 @@ set_blend_entry_bits(struct brw_context *brw, BLEND_ENTRY_GENXML *entry, int i,
> > >           entry->LogicOpFunction =
> > >              intel_translate_logic_op(ctx->Color.LogicOp);
> > >        }
> > > -   } else if (ctx->Color.BlendEnabled & (1 << i) && !integer &&
> > > -              !ctx->Color._AdvancedBlendMode) {
> > > +   } else if (blend_enabled && !ctx->Color._AdvancedBlendMode
> > > +              && (GEN_GEN <= 5 || !integer)) {
> > 
> > The GEN_GEN <= 5 || !integer seems bizarre, and I wonder whether it's
> > correct.  However, you're just preserving the existing behavior, so
> > that's fine - we may want to revisit it in the future.
> > 
> > >        GLenum eqRGB = ctx->Color.Blend[i].EquationRGB;
> > >        GLenum eqA = ctx->Color.Blend[i].EquationA;
> > >        GLenum srcRGB = ctx->Color.Blend[i].SrcRGB;
> > > @@ -2994,17 +3021,40 @@ static const struct brw_tracked_state genX(multisample_state) = {
> > >  
> > >  /* ---------------------------------------------------------------------- */
> > >  
> > > -#if GEN_GEN >= 6
> > >  static void
> > >  genX(upload_color_calc_state)(struct brw_context *brw)
> > >  {
> > >     struct gl_context *ctx = &brw->ctx;
> > >  
> > >     brw_state_emit(brw, GENX(COLOR_CALC_STATE), 64, &brw->cc.state_offset, cc) {
> > > +#if GEN_GEN <= 5
> > > +      cc.IndependentAlphaBlendEnable =
> > > +         set_blend_entry_bits(brw, &cc, 0, false);
> > > +      set_depth_stencil_bits(brw, &cc);
> > > +
> > > +      if (ctx->Color.AlphaEnabled &&
> > > +          ctx->DrawBuffer->_NumColorDrawBuffers <= 1) {
> > > +         cc.AlphaTestEnable = true;
> > > +         cc.AlphaTestFunction =
> > > +            intel_translate_compare_func(ctx->Color.AlphaFunc);
> > > +      }
> > 
> > We should probably make this consistent across generations:
> > 
> >    if (ctx->Color.AlphaEnabled &&
> >        (GEN_GEN >= 6 || ctx->DrawBuffer->_NumColorDrawBuffers <= 1) {
> >       cc.AlphaTestEnable = true;
> >       cc.AlphaTestFunction =
> >          intel_translate_compare_func(ctx->Color.AlphaFunc);
> >       cc.AlphaTestFormat = ALPHATEST_UNORM8;
> >       UNCLAMPED_FLOAT_TO_UBYTE(cc.AlphaReferenceValueAsUNORM8,
> >                                ctx->Color.AlphaRef);
> >    }
> 
> Hmm... COLOR_CALC_STATE on gen >= 6 doesn't have the fields AlphaTestEnable
> and AlphaTestFunction. Instead, they are inside the BLEND_STATE. Are you
> suggesting that I use some kind of polymorphism here too, to set these fields
> on both the BLEND_STATE and COLOR_CALC_STATE, depending on the generation?

No, definitely not suggesting anything complicated.  I must have misread
something.

I guess what I'm suggesting is...

> > > +#else
> > >        /* _NEW_COLOR */
> > > -      cc.AlphaTestFormat = ALPHATEST_UNORM8;
> > > -      UNCLAMPED_FLOAT_TO_UBYTE(cc.AlphaReferenceValueAsUNORM8,
> > > -                               ctx->Color.AlphaRef);

...we should continue setting these unconditionally.

ALPHATEST_UNORM8 is actually 0, so whether or not we execute that line has
no effect at all.

It should be fine to program the ref value regardless of whether alpha
testing is enabled or not.  The GPU will ignore the ref if testing is off.

> > > +      cc.BlendConstantColorRed = ctx->Color.BlendColorUnclamped[0];
> > > +      cc.BlendConstantColorGreen = ctx->Color.BlendColorUnclamped[1];
> > > +      cc.BlendConstantColorBlue = ctx->Color.BlendColorUnclamped[2];
> > > +      cc.BlendConstantColorAlpha = ctx->Color.BlendColorUnclamped[3];
> > >  
> > >  #if GEN_GEN < 9
> > >        /* _NEW_STENCIL */
> > > @@ -3013,34 +3063,47 @@ genX(upload_color_calc_state)(struct brw_context *brw)
> > >           _mesa_get_stencil_ref(ctx, ctx->Stencil._BackFace);
> > >  #endif
> > >  
> > > +#endif
> > > +
> > >        /* _NEW_COLOR */
> > > -      cc.BlendConstantColorRed = ctx->Color.BlendColorUnclamped[0];
> > > -      cc.BlendConstantColorGreen = ctx->Color.BlendColorUnclamped[1];
> > > -      cc.BlendConstantColorBlue = ctx->Color.BlendColorUnclamped[2];
> > > -      cc.BlendConstantColorAlpha = ctx->Color.BlendColorUnclamped[3];
> > > +      if (GEN_GEN >= 6 ||
> > > +          (ctx->Color.AlphaEnabled &&
> > > +           ctx->DrawBuffer->_NumColorDrawBuffers <= 1)) {
> > > +         cc.AlphaTestFormat = ALPHATEST_UNORM8;
> > > +         UNCLAMPED_FLOAT_TO_UBYTE(cc.AlphaReferenceValueAsUNORM8,
> > > +                                  ctx->Color.AlphaRef);
> > > +      }

...which means we don't need to duplicate these conditions.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170619/73e71ad6/attachment-0001.sig>

More information about the mesa-dev mailing list