[Mesa-dev] [PATCH 03/10] i965: Create a helper function for emitting PIPE_CONTROL flushes.

[Kenneth Graunke]

On 01/09/2014 07:52 PM, Eric Anholt wrote:
> Kenneth Graunke <kenneth at whitecape.org> writes:
> 
>> These days, we need to emit PIPE_CONTROL flushes all over the place.
>> Being able to do that via a single function call seems convenient.
>>
>> Broadwell will also increase the length of these packets by 1; with the
>> refactoring, we should have to do this in substantially fewer places.
>>
>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> 
>> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> index d9b6c15..d2f0e90 100644
>> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> @@ -475,6 +475,32 @@ emit:
>>  }
>>  
>>  /**
>> + * Emit a PIPE_CONTROL with various flushing flags.
>> + *
>> + * The caller is responsible for deciding what flags are appropriate for the
>> + * given generation.
>> + */
>> +void
>> +brw_emit_pipe_control_flush(struct brw_context *brw, uint32_t flags)
>> +{
>> +   if (brw->gen >= 6) {
>> +      BEGIN_BATCH(4);
>> +      OUT_BATCH(_3DSTATE_PIPE_CONTROL | (4 - 2));
>> +      OUT_BATCH(flags);
>> +      OUT_BATCH(0);
>> +      OUT_BATCH(0);
>> +      ADVANCE_BATCH();
>> +   } else {
>> +      BEGIN_BATCH(4);
>> +      OUT_BATCH(_3DSTATE_PIPE_CONTROL | flags | (4 - 2));
>> +      OUT_BATCH(0);
>> +      OUT_BATCH(0);
>> +      OUT_BATCH(0);
>> +      ADVANCE_BATCH();
>> +   }
>> +}
>> +
>> +/**
>>   * Restriction [DevSNB, DevIVB]:
>>   *
>>   * Prior to changing Depth/Stencil Buffer state (i.e. any combination of
>> @@ -491,26 +517,9 @@ intel_emit_depth_stall_flushes(struct brw_context *brw)
>>  {
>>     assert(brw->gen >= 6 && brw->gen <= 7);
>>  
>> -   BEGIN_BATCH(4);
>> -   OUT_BATCH(_3DSTATE_PIPE_CONTROL | (4 - 2));
>> -   OUT_BATCH(PIPE_CONTROL_DEPTH_STALL);
>> -   OUT_BATCH(0); /* address */
>> -   OUT_BATCH(0); /* write data */
>> -   ADVANCE_BATCH()
>> -
>> -   BEGIN_BATCH(4);
>> -   OUT_BATCH(_3DSTATE_PIPE_CONTROL | (4 - 2));
>> -   OUT_BATCH(PIPE_CONTROL_DEPTH_CACHE_FLUSH);
>> -   OUT_BATCH(0); /* address */
>> -   OUT_BATCH(0); /* write data */
>> -   ADVANCE_BATCH();
>> -
>> -   BEGIN_BATCH(4);
>> -   OUT_BATCH(_3DSTATE_PIPE_CONTROL | (4 - 2));
>> -   OUT_BATCH(PIPE_CONTROL_DEPTH_STALL);
>> -   OUT_BATCH(0); /* address */
>> -   OUT_BATCH(0); /* write data */
>> -   ADVANCE_BATCH();
>> +   brw_emit_pipe_control_flush(brw, PIPE_CONTROL_DEPTH_STALL);
>> +   brw_emit_pipe_control_flush(brw, PIPE_CONTROL_DEPTH_CACHE_FLUSH);
>> +   brw_emit_pipe_control_flush(brw, PIPE_CONTROL_DEPTH_STALL);
>>  }
>>  
>>  /**
>> @@ -608,13 +617,8 @@ intel_emit_post_sync_nonzero_flush(struct brw_context *brw)
>>     if (!brw->batch.need_workaround_flush)
>>        return;
>>  
>> -   BEGIN_BATCH(4);
>> -   OUT_BATCH(_3DSTATE_PIPE_CONTROL | (4 - 2));
>> -   OUT_BATCH(PIPE_CONTROL_CS_STALL |
>> -	     PIPE_CONTROL_STALL_AT_SCOREBOARD);
>> -   OUT_BATCH(0); /* address */
>> -   OUT_BATCH(0); /* write data */
>> -   ADVANCE_BATCH();
>> +   brw_emit_pipe_control_flush(brw, PIPE_CONTROL_CS_STALL |
>> +                               PIPE_CONTROL_STALL_AT_SCOREBOARD);
>>  
>>     BEGIN_BATCH(4);
>>     OUT_BATCH(_3DSTATE_PIPE_CONTROL | (4 - 2));
>> @@ -636,46 +640,22 @@ intel_emit_post_sync_nonzero_flush(struct brw_context *brw)
>>  void
>>  intel_batchbuffer_emit_mi_flush(struct brw_context *brw)
>>  {
>> -   if (brw->gen >= 6) {
>> -      if (brw->batch.ring == BLT_RING) {
>> -	 BEGIN_BATCH_BLT(4);
>> -	 OUT_BATCH(MI_FLUSH_DW);
>> -	 OUT_BATCH(0);
>> -	 OUT_BATCH(0);
>> -	 OUT_BATCH(0);
>> -	 ADVANCE_BATCH();
>> -      } else {
>> -	 if (brw->gen == 6) {
>> -	    /* Hardware workaround: SNB B-Spec says:
>> -	     *
>> -	     * [Dev-SNB{W/A}]: Before a PIPE_CONTROL with Write Cache
>> -	     * Flush Enable =1, a PIPE_CONTROL with any non-zero
>> -	     * post-sync-op is required.
>> -	     */
>> -	    intel_emit_post_sync_nonzero_flush(brw);
>> -	 }
>> -
>> -	 BEGIN_BATCH(4);
>> -	 OUT_BATCH(_3DSTATE_PIPE_CONTROL | (4 - 2));
>> -	 OUT_BATCH(PIPE_CONTROL_INSTRUCTION_FLUSH |
>> -		   PIPE_CONTROL_WRITE_FLUSH |
>> -		   PIPE_CONTROL_DEPTH_CACHE_FLUSH |
>> -                   PIPE_CONTROL_VF_CACHE_INVALIDATE |
>> -		   PIPE_CONTROL_TC_FLUSH |
>> -		   PIPE_CONTROL_NO_WRITE |
>> -                   PIPE_CONTROL_CS_STALL);
>> -	 OUT_BATCH(0); /* write address */
>> -	 OUT_BATCH(0); /* write data */
>> -	 ADVANCE_BATCH();
>> -      }
>> -   } else {
>> -      BEGIN_BATCH(4);
>> -      OUT_BATCH(_3DSTATE_PIPE_CONTROL | (4 - 2) |
>> -		PIPE_CONTROL_WRITE_FLUSH |
>> -		PIPE_CONTROL_NO_WRITE);
>> -      OUT_BATCH(0); /* write address */
>> -      OUT_BATCH(0); /* write data */
>> -      OUT_BATCH(0); /* write data */
>> +   if (unlikely(brw->batch.ring == BLT_RING) && brw->gen >= 6) {
>> +      BEGIN_BATCH_BLT(4);
>> +      OUT_BATCH(MI_FLUSH_DW);
>> +      OUT_BATCH(0);
>> +      OUT_BATCH(0);
>> +      OUT_BATCH(0);
>>        ADVANCE_BATCH();
> 
> This shouldn't be marked unlikely.  You should use unlikely for "this
> path should be never executed in a performance-sensitive way", not just
> "I bet this will be used a bit less frequently than the alternative."
> (the compiler does more than just try to hint branch prediction using
> this information)

Removed in v2.

> 
>> +   } else {
>> +      int flags = PIPE_CONTROL_NO_WRITE | PIPE_CONTROL_WRITE_FLUSH;
>> +      if (brw->gen >= 6) {
>> +         flags |= PIPE_CONTROL_INSTRUCTION_FLUSH |
>> +                  PIPE_CONTROL_DEPTH_CACHE_FLUSH |
>> +                  PIPE_CONTROL_VF_CACHE_INVALIDATE |
>> +                  PIPE_CONTROL_TC_FLUSH |
>> +                  PIPE_CONTROL_CS_STALL;
>> +      }
> 
> Lost the emit_post_sync_nonzero_flush() from the previous code.

Yikes, thanks for catching that.  Added back in v2.

>> +      brw_emit_pipe_control_flush(brw, flags);
>>     }
>>  }
>