[Mesa-dev] [PATCH v02 25/37] i965: Port gen7+ 3DSTATE_PS to genxml.

[Kenneth Graunke]

On Monday, April 24, 2017 3:19:20 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 cb3c2db..1b9dedf 100644
> --- a/src/mesa/drivers/dri/i965/genX_state_upload.c
> +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
> @@ -1230,6 +1230,135 @@ static const struct brw_tracked_state genX(sol_state) = {
>     .emit = genX(upload_sol),
>  };
>  
> +/* ---------------------------------------------------------------------- */
> +
> +static void
> +genX(upload_ps)(struct brw_context *brw)
> +{
> +   /* BRW_NEW_FS_PROG_DATA */
> +   const struct brw_wm_prog_data *prog_data =
> +      brw_wm_prog_data(brw->wm.base.prog_data);
> +   const struct brw_stage_state *stage_state = &brw->wm.base;
> +#if GEN_GEN < 8
> +   const struct gl_context *ctx = &brw->ctx;
> +   /* BRW_NEW_FS_PROG_DATA | _NEW_COLOR */
> +   const bool enable_dual_src_blend = prog_data->dual_src_blend &&
> +                                      (ctx->Color.BlendEnabled & 1) &&
> +                                      ctx->Color.Blend[0]._UsesDualSrc;
> +   const struct gen_device_info *devinfo = &brw->screen->devinfo;
> +#endif

Let's move the dual source blending down, i.e.

#if GEN_GEN < 8
      ps.DualSourceBlendEnable = prog_data->dual_src_blend && ...;
#endif

and mark ctx/devinfo with the UNUSED macro to shut up warnings rather
than #if'ing them out.

> +
> +   brw_batch_emit(brw, GENX(3DSTATE_PS), ps) {
> +      /* Initialize the execution mask with VMask.  Otherwise, derivatives are
> +       * incorrect for subspans where some of the pixels are unlit.  We believe
> +       * the bit just didn't take effect in previous generations.
> +       */
> +#if GEN_GEN >= 8
> +      ps.VectorMaskEnable = true;
> +#endif

Simpler,

      ps.VectorMaskEnable = GEN_GEN >= 8;

> +      ps.SamplerCount =
> +         DIV_ROUND_UP(CLAMP(stage_state->sampler_count, 0, 16), 4);
> +
> +      /* BRW_NEW_FS_PROG_DATA */
> +      ps.BindingTableEntryCount = prog_data->base.binding_table.size_bytes / 4;
> +
> +      if (prog_data->base.use_alt_mode)
> +         ps.FloatingPointMode = Alternate;
> +
> +      /* Haswell requires the sample mask to be set in this packet as well as
> +       * in 3DSTATE_SAMPLE_MASK; the values should match. */

Comment closer */ goes on its own line.

> +      /* _NEW_BUFFERS, _NEW_MULTISAMPLE */
> +#if GEN_IS_HASWELL
> +      ps.SampleMask = gen6_determine_sample_mask(brw);
> +#endif
> +
> +      /* 3DSTATE_PS expects the number of threads per PSD, which is always 64;
> +       * it implicitly scales for different GT levels (which have some # of
> +       * PSDs).
> +       *
> +       * In Gen8 the format is U8-2 whereas in Gen9 it is U8-1.
> +       */
> +#if GEN_GEN >= 9
> +      ps.MaximumNumberofThreadsPerPSD = 64 - 1;
> +#elif GEN_GEN >= 8
> +      ps.MaximumNumberofThreadsPerPSD = 64 - 2;
> +#else
> +      ps.MaximumNumberofThreads = devinfo->max_wm_threads - 1;
> +#endif
> +
> +      if (prog_data->base.nr_params > 0)
> +         ps.PushConstantEnable = true;
> +
> +#if GEN_GEN < 8
> +      /* From the IVB PRM, volume 2 part 1, page 287:
> +       * "This bit is inserted in the PS payload header and made available to
> +       * the DataPort (either via the message header or via header bypass) to
> +       * indicate that oMask data (one or two phases) is included in Render
> +       * Target Write messages. If present, the oMask data is used to mask off
> +       * samples."
> +       */
> +      ps.oMaskPresenttoRenderTarget = prog_data->uses_omask;
> +
> +      /* The hardware wedges if you have this bit set but don't turn on any dual
> +       * source blend factors.
> +       */
> +      ps.DualSourceBlendEnable = enable_dual_src_blend;
> +
> +      /* BRW_NEW_FS_PROG_DATA */
> +      ps.AttributeEnable = (prog_data->num_varying_inputs != 0);
> +#endif
> +
> +      /* From the documentation for this packet:
> +       * "If the PS kernel does not need the Position XY Offsets to
> +       *  compute a Position 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 (prog_data->uses_pos_offset)
> +         ps.PositionXYOffsetSelect = POSOFFSET_SAMPLE;
> +      else
> +         ps.PositionXYOffsetSelect = POSOFFSET_NONE;
> +
> +      ps.RenderTargetFastClearEnable = brw->wm.fast_clear_op;
> +      ps._8PixelDispatchEnable = !!prog_data->dispatch_8;
> +      ps._16PixelDispatchEnable = !!prog_data->dispatch_16;

I don't think you need !!, the prog_data fields are bool type which
always becomes 0 or 1 when casting to integer types.

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- 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/20170426/41a73842/attachment.sig>