[Mesa-dev] [PATCH 16/19] i965: Manipulate state batches for HiZ operation meta-ops

[Chad Versace]

On 09/26/2011 01:15 PM, Eric Anholt wrote:
> On Fri, 23 Sep 2011 17:37:46 -0700, Chad Versace<chad at chad-versace.us>  wrote:
>
> That's a pretty terse commit message :)
>
>> Signed-off-by: Chad Versace<chad at chad-versace.us>
>> ---
>>   src/mesa/drivers/dri/i965/brw_draw.c          |    3 ++
>>   src/mesa/drivers/dri/i965/gen6_clip_state.c   |   17 +++++++++++++++
>>   src/mesa/drivers/dri/i965/gen6_depthstencil.c |   22 +++++++++++++++++-
>>   src/mesa/drivers/dri/i965/gen6_sf_state.c     |   16 ++++++++++++-
>>   src/mesa/drivers/dri/i965/gen6_wm_state.c     |   28 +++++++++++++++++++++---
>>   5 files changed, 78 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/gen6_depthstencil.c b/src/mesa/drivers/dri/i965/gen6_depthstencil.c
>> index 5d14147..59f3fe4 100644
>> --- a/src/mesa/drivers/dri/i965/gen6_depthstencil.c
>> +++ b/src/mesa/drivers/dri/i965/gen6_depthstencil.c
>> @@ -77,10 +77,28 @@ gen6_prepare_depth_stencil_state(struct brw_context *brw)
>>      }
>>
>>      /* _NEW_DEPTH */
>> -   if (ctx->Depth.Test) {
>> -      ds->ds2.depth_test_enable = 1;
>> +   if (ctx->Depth.Test || brw->hiz.op != 0) {
>>         ds->ds2.depth_test_func = intel_translate_compare_func(ctx->Depth.Func);
>>         ds->ds2.depth_write_enable = ctx->Depth.Mask;
>> +
>> +      /* See the following sections of the Sandy Bridge PRM, Volume 1, Part2:
>> +       *   - 7.5.3.1 Depth Buffer Clear
>> +       *   - 7.5.3.2 Depth Buffer Resolve
>> +       *   - 7.5.3.3 Hierarchical Depth Buffer Resolve
>> +       */
>> +      switch (brw->hiz.op) {
>> +      case BRW_HIZ_OP_NONE:
>> +      case BRW_HIZ_OP_DEPTH_RESOLVE:
>> +         ds->ds2.depth_test_enable = 1;
>> +         break;
>> +      case BRW_HIZ_OP_DEPTH_CLEAR:
>> +      case BRW_HIZ_OP_HIZ_RESOLVE:
>> +         ds->ds2.depth_test_enable = 0;
>> +         break;
>> +      default:
>> +         assert(0);
>> +         break;
>> +      }
>
> Isn't the metaop already ensuring that the depth test is
> enabled/disabled as appropriate?

Yes. I wrote this commit long before writing the meta-ops, and I failed to thoroughly
clean this commit up afterwards.

>
>> diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c b/src/mesa/drivers/dri/i965/gen6_sf_state.c
>> index 5cbfe78..bdd02ed 100644
>> --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
>> +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
>> @@ -149,8 +149,20 @@ upload_sf_state(struct brw_context *brw)
>>         num_outputs<<  GEN6_SF_NUM_OUTPUTS_SHIFT |
>>         urb_entry_read_length<<  GEN6_SF_URB_ENTRY_READ_LENGTH_SHIFT |
>>         urb_entry_read_offset<<  GEN6_SF_URB_ENTRY_READ_OFFSET_SHIFT;
>> -   dw2 = GEN6_SF_VIEWPORT_TRANSFORM_ENABLE |
>> -      GEN6_SF_STATISTICS_ENABLE;
>> +
>> +   dw2 = GEN6_SF_STATISTICS_ENABLE;
>> +
>> +   /* Enable viewport transform only if no HiZ operation is progress
>> +    *
>> +    * From page 11 of the SandyBridge PRM, Volume 2, Part 1, Section 1.3, "3D
>> +    * Primitives Overview":
>> +    *     RECTLIST: Viewport Mapping must be DISABLED (as is typical with the
>> +    *     use of screen- space coordinates).
>> +    */
>> +   if (brw->hiz.op == 0) {
>> +      dw2 |= GEN6_SF_VIEWPORT_TRANSFORM_ENABLE;
>> +   }
>
>> diff --git a/src/mesa/drivers/dri/i965/gen6_wm_state.c b/src/mesa/drivers/dri/i965/gen6_wm_state.c
>> index 07e9995..bbc421f 100644
>> --- a/src/mesa/drivers/dri/i965/gen6_wm_state.c
>> +++ b/src/mesa/drivers/dri/i965/gen6_wm_state.c
>> @@ -144,6 +144,23 @@ upload_wm_state(struct brw_context *brw)
>>      dw4 |= (brw->wm.prog_data->first_curbe_grf_16<<
>>   	GEN6_WM_DISPATCH_START_GRF_SHIFT_2);
>>
>> +   switch (brw->hiz.op) {
>> +   case BRW_HIZ_OP_NONE:
>> +      break;
>> +   case BRW_HIZ_OP_DEPTH_CLEAR:
>> +      dw4 |= GEN6_WM_DEPTH_CLEAR;
>> +      break;
>> +   case BRW_HIZ_OP_DEPTH_RESOLVE:
>> +      dw4 |= GEN6_WM_DEPTH_RESOLVE;
>> +      break;
>> +   case BRW_HIZ_OP_HIZ_RESOLVE:
>> +      dw4 |= GEN6_WM_HIERARCHICAL_DEPTH_RESOLVE;
>> +      break;
>> +   default:
>> +      assert(0);
>> +      break;
>> +   }
>> +
>>      dw5 |= (brw->wm_max_threads - 1)<<  GEN6_WM_MAX_THREADS_SHIFT;
>>
>>      /* CACHE_NEW_WM_PROG */
>> @@ -166,15 +183,18 @@ upload_wm_state(struct brw_context *brw)
>>      /* BRW_NEW_FRAGMENT_PROGRAM */
>>      if (fp->program.Base.InputsRead&  (1<<  FRAG_ATTRIB_WPOS))
>>         dw5 |= GEN6_WM_USES_SOURCE_DEPTH | GEN6_WM_USES_SOURCE_W;
>> -   if (fp->program.Base.OutputsWritten&  BITFIELD64_BIT(FRAG_RESULT_DEPTH))
>> +   if ((fp->program.Base.OutputsWritten&  BITFIELD64_BIT(FRAG_RESULT_DEPTH))&&
>> +       !brw->hiz.op)
>>         dw5 |= GEN6_WM_COMPUTED_DEPTH;
>>
>>      /* _NEW_COLOR */
>> -   if (fp->program.UsesKill || ctx->Color.AlphaEnabled)
>> +   if ((fp->program.UsesKill || ctx->Color.AlphaEnabled)&&
>> +       !brw->hiz.op)
>>         dw5 |= GEN6_WM_KILL_ENABLE;
>>
>> -   if (brw_color_buffer_write_enabled(brw) ||
>> -       dw5&  (GEN6_WM_KILL_ENABLE | GEN6_WM_COMPUTED_DEPTH)) {
>> +   if ((brw_color_buffer_write_enabled(brw) ||
>> +        (dw5&  (GEN6_WM_KILL_ENABLE | GEN6_WM_COMPUTED_DEPTH)))&&
>> +       !brw->hiz.op) {
>>         dw5 |= GEN6_WM_DISPATCH_ENABLE;
>
> It looks to me like if the meta code just set up an empty fragment
> shader with its VS, none of the hiz.op checks here would be needed.  I
> think that would be cleaner.

I agree. I'll test that idea and, if it works, this commit can be deleted.

-- 
Chad Versace
chad at chad-versace.us