[Mesa-dev] [PATCH v02 26/37] i965: Port gen6+ 3DSTATE_WM to genxml.

[Rafael Antognolli]

On Wed, Apr 26, 2017 at 11:15:45PM -0700, Kenneth Graunke wrote:
> On Monday, April 24, 2017 3:19:21 PM PDT Rafael Antognolli wrote:
> [snip]
> > diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c b/src/mesa/drivers/dri/i965/genX_state_upload.c
> > index 1b9dedf..0f7a222 100644
> > --- a/src/mesa/drivers/dri/i965/genX_state_upload.c
> > +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
> > @@ -28,6 +28,7 @@
> >  
> >  #include "brw_context.h"
> >  #include "brw_state.h"
> > +#include "brw_wm.h"
> >  #include "brw_util.h"
> >  
> >  #include "intel_batchbuffer.h"
> > @@ -39,6 +40,8 @@
> >  #include "main/stencil.h"
> >  #include "main/transformfeedback.h"
> >  
> > +#include "compiler/brw_defines_common.h"
> > +
> >  UNUSED static void *
> >  emit_dwords(struct brw_context *brw, unsigned n)
> >  {
> > @@ -788,6 +791,189 @@ static const struct brw_tracked_state genX(sf_state) = {
> >     .emit = genX(upload_sf),
> >  };
> >  
> > +/* ---------------------------------------------------------------------- */
> > +
> > +static void
> > +genX(upload_wm)(struct brw_context *brw)
> > +{
> > +   struct gl_context *ctx = &brw->ctx;
> > +
> > +   /* BRW_NEW_FS_PROG_DATA */
> > +   const struct brw_wm_prog_data *wm_prog_data =
> > +      brw_wm_prog_data(brw->wm.base.prog_data);
> > +#if GEN_GEN < 8
> > +   bool writes_depth = wm_prog_data->computed_depth_mode != BRW_PSCDEPTH_OFF;
> > +   /* _NEW_BUFFERS */
> > +   const bool multisampled_fbo = _mesa_geometric_samples(ctx->DrawBuffer) > 1;
> > +#endif
> 
> I think you can declare these in the GEN_GEN < 8 block below and save
> yourself an #if...#endif block.

Hmmm... it looks like I can't do that with writes_depth, because it is
also used in the GEN_GEN < 7 block. Is it fine if I just leave it at the
top with UNUSED?

> > +
> > +#if GEN_GEN < 7
> > +   const struct brw_stage_state *stage_state = &brw->wm.base;
> > +   const bool enable_dual_src_blend = wm_prog_data->dual_src_blend &&
> > +                                      (ctx->Color.BlendEnabled & 1) &&
> > +                                      ctx->Color.Blend[0]._UsesDualSrc;
> 
> Let's get rid of enable_dual_src_blend and just put the expression in
> the wm.DualSourceBlendEnable assignment below.  Then this whole block
> is about constant packets, which reads nicely.
> 
> > +   const struct gen_device_info *devinfo = &brw->screen->devinfo;
> > +
> > +   /* We can't fold this into gen6_upload_wm_push_constants(), because
> > +    * according to the SNB PRM, vol 2 part 1 section 7.2.2
> > +    * (3DSTATE_CONSTANT_PS [DevSNB]):
> > +    *
> > +    *     "[DevSNB]: This packet must be followed by WM_STATE."
> > +    */
> > +   brw_batch_emit(brw, GENX(3DSTATE_CONSTANT_PS), wmcp) {
> > +      if (wm_prog_data->base.nr_params != 0) {
> > +         wmcp.Buffer0Valid = true;
> > +         /* Pointer to the WM constant buffer.  Covered by the set of
> > +          * state flags from gen6_upload_wm_push_constants.
> > +          */
> > +         wmcp.PointertoPSConstantBuffer0 = stage_state->push_const_offset;
> > +         wmcp.PSConstantBuffer0ReadLength = stage_state->push_const_size - 1;
> > +      }
> > +   }
> > +#endif
> > +
> > +   brw_batch_emit(brw, GENX(3DSTATE_WM), wm) {
> > +      wm.StatisticsEnable = true;
> > +      wm.LineAntialiasingRegionWidth = _10pixels;
> > +      wm.LineEndCapAntialiasingRegionWidth = _05pixels;
> > +
> > +#if GEN_GEN < 7
> > +      if (wm_prog_data->base.use_alt_mode)
> > +         wm.FloatingPointMode = Alternate;
> > +
> > +      wm.SamplerCount |= ALIGN(stage_state->sampler_count, 4) / 4;
> 
> You want =, not |=.  Also, let's use DIV_ROUND_UP:
> 
>       wm.SamplerCount = DIV_ROUND_UP(stage_state->sampler_count, 4);
> 
> > +      wm.BindingTableEntryCount = wm_prog_data->base.binding_table.size_bytes / 4;
> > +      wm.MaximumNumberofThreads = devinfo->max_wm_threads - 1;
> > +      wm._8PixelDispatchEnable = !!wm_prog_data->dispatch_8;
> > +      wm._16PixelDispatchEnable = !!wm_prog_data->dispatch_16;
> 
> I don't think you need !! - these are bools, they turn into 0 or 1
> automatically.
> 
> > +      wm.DispatchGRFStartRegisterForConstantSetupData0 =
> > +         wm_prog_data->base.dispatch_grf_start_reg;
> > +      wm.DispatchGRFStartRegisterForConstantSetupData2 =
> > +         wm_prog_data->dispatch_grf_start_reg_2;
> > +      wm.KernelStartPointer0 = stage_state->prog_offset;
> > +      wm.KernelStartPointer2 = stage_state->prog_offset +
> > +         wm_prog_data->prog_offset_2;
> > +      wm.DualSourceBlendEnable = enable_dual_src_blend;
> > +      wm.oMaskPresenttoRenderTarget = wm_prog_data->uses_omask;
> > +      wm.NumberofSFOutputAttributes = wm_prog_data->num_varying_inputs;
> > +
> > +      /* From the SNB PRM, volume 2 part 1, page 281:
> > +       * "If the PS kernel does not need the Position XY Offsets
> > +       * to compute a Position XY value, then this field should be
> > +       * programmed to POSOFFSET_NONE."
> > +       *
> > +       * "SW Recommendation: If the PS kernel needs the Position Offsets
> > +       * to compute a Position XY value, this field should match Position
> > +       * ZW Interpolation Mode to ensure a consistent position.xyzw
> > +       * computation."
> > +       * We only require XY sample offsets. So, this recommendation doesn't
> > +       * look useful at the moment. We might need this in future.
> > +       */
> > +      if (wm_prog_data->uses_pos_offset)
> > +         wm.PositionXYOffsetSelect = POSOFFSET_SAMPLE;
> > +      else
> > +         wm.PositionXYOffsetSelect = POSOFFSET_NONE;
> > +
> > +      if (wm_prog_data->base.total_scratch) {
> > +         wm.ScratchSpaceBasePointer =
> > +            render_bo(stage_state->scratch_bo,
> > +                      ffs(stage_state->per_thread_scratch) - 11);
> > +      }
> > +
> > +      wm.PixelShaderComputedDepth = writes_depth;
> > +#endif
> > +
> > +#if GEN_GEN >= 8
> > +      wm.PointRasterizationRule = RASTRULE_UPPER_RIGHT;
> > +#endif
> 
> Let's enable this unconditionally - the fact that we didn't on Gen6-7.5
> seems like a bug.  I sent a patch to fix that.
> 
> With those changes,
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>