[Mesa-dev] [PATCH 3/6] i965/gen6-7: Implement stall and flushes required prior to switching pipelines.

[Francisco Jerez]

Kenneth Graunke <kenneth at whitecape.org> writes:

> On Saturday, January 2, 2016 10:48:02 PM PST Francisco Jerez wrote:
>> Switching the current pipeline while it's not completely idle or the
>> read and write caches aren't flushed can lead to corruption.  Fixes
>> misrendering of at least the following Khronos CTS test:
>> 
>>  ES31-CTS.shader_image_load_store.basic-allTargets-store-fs
>> 
>> The stall and flushes are no longer required on Gen8+.
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93323
>> ---
>>  src/mesa/drivers/dri/i965/brw_misc_state.c | 28 +++++++++++++++++++++++++++
> +
>>  1 file changed, 28 insertions(+)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c b/src/mesa/drivers/
> dri/i965/brw_misc_state.c
>> index 7d53d18..75540c1 100644
>> --- a/src/mesa/drivers/dri/i965/brw_misc_state.c
>> +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c
>> @@ -886,6 +886,34 @@ brw_emit_select_pipeline(struct brw_context *brw, enum 
> brw_pipeline pipeline)
>>  
>>           brw->ctx.NewDriverState |= BRW_NEW_CC_STATE;
>>        }
>> +
>> +   } else if (brw->gen >= 6) {
>> +      /* From "BXML » GT » MI » vol1a GPU Overview » [Instruction]
>> +       * PIPELINE_SELECT [DevBWR+]":
>
> Can we cite the public docs?
>

The public docs for PIPELINE_SELECT seemed rather inaccurate.  The IVB
version I have in front of me right now is missing this one workaround,
and the BDW version mentions it incorrectly.  Sigh...

>> +       *
>> +       *   Project: DEVSNB+
>> +       *
>> +       *   Software must ensure all the write caches are flushed through a
>> +       *   stalling PIPE_CONTROL command followed by another PIPE_CONTROL
>> +       *   command to invalidate read only caches prior to programming
>> +       *   MI_PIPELINE_SELECT command to change the Pipeline Select Mode.
>> +       */
>> +      const unsigned dc_flush =
>> +         brw->gen >= 7 ? PIPE_CONTROL_DATA_CACHE_INVALIDATE : 0;
>
> I was going to suggest doing a brw_emit_post_sync_nonzero_flush first
> on Sandybridge, but I forgot that we now just emit that at the start
> of every state upload.  Fairly moot anyway since we don't do GPGPU on
> Sandybridge anyway.
>
Hmm, that sounds very sensible to me, it would be rather fragile for
this function to rely on a flush with post-sync op having been done
previously, even if at this point this will only be called once at
context creation on SNB -- Although for the same reason it seems rather
fragile for brw_emit_pipe_control_flush() to assume that the workaround
has been applied already.  I'd be inclined to change
brw_emit_pipe_control_flush() to emit the post-sync op when needed on
SNB just like we do for other PIPE_CONTROL workarounds on Gen7 and Gen8.

>> +
>> +      brw_emit_pipe_control_flush(brw,
>> +                                  PIPE_CONTROL_RENDER_TARGET_FLUSH |
>> +                                  PIPE_CONTROL_DEPTH_CACHE_FLUSH |
>> +                                  dc_flush |
>> +                                  PIPE_CONTROL_NO_WRITE |
>> +                                  PIPE_CONTROL_CS_STALL);
>
> Why RENDER_TARGET_FLUSH, DEPTH_CACHE_FLUSH, DATA_CACHE_INVALIDATE,
> and NO_WRITE?  The cited workaround explains a CS Stall and the RO
> invalidations below, but I'm not seeing why the others are needed.
>
It also says that "software must ensure all the write caches are
flushed".

>> +
>> +      brw_emit_pipe_control_flush(brw,
>> +                                  PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE |
>> +                                  PIPE_CONTROL_CONST_CACHE_INVALIDATE |
>> +                                  PIPE_CONTROL_STATE_CACHE_INVALIDATE |
>> +                                  PIPE_CONTROL_INSTRUCTION_INVALIDATE |
>> +                                  PIPE_CONTROL_NO_WRITE);
>>     }
>>  
>>     /* Select the pipeline */
>> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20160103/5d6cd303/attachment.sig>