[Mesa-dev] [PATCH 3/3] i965: adjust gen6+ timestamp pipe_control writes

[Daniel Vetter]

On Sun, Jul 01, 2012 at 08:10:49PM -0700, Kenneth Graunke wrote:
> On 06/26/2012 07:28 AM, Daniel Vetter wrote:
> > Similar treatment to the depth count pipe_control writes
> > - Add the CS_STALL workaround, timestamp writes are non-zero post-sync
> >   ops, too.
> > - Also ensure that we write the full 64bits by using the 5 dword long
> >   variant of pipe_control.
> > ---
> >  src/mesa/drivers/dri/i965/brw_queryobj.c |   32 ++++++++++++++++++++++++++---
> >  1 files changed, 28 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_queryobj.c b/src/mesa/drivers/dri/i965/brw_queryobj.c
> > index dae7af0..36de43e 100644
> > --- a/src/mesa/drivers/dri/i965/brw_queryobj.c
> > +++ b/src/mesa/drivers/dri/i965/brw_queryobj.c
> > @@ -131,14 +131,26 @@ brw_begin_query(struct gl_context *ctx, struct gl_query_object *q)
> >  				     4096, 4096);
> >  
> >        if (intel->gen >= 6) {
> > -	  BEGIN_BATCH(4);
> > -	  OUT_BATCH(_3DSTATE_PIPE_CONTROL);
> > +	  BEGIN_BATCH(9);
> > +
> > +          /* Workaround: A non-zero post-sync op (i.e. the DEPTH_COUNT write below
> > +           * needs a pipe control with CS_STALL set beforehand.
> > +           * Workaround: CS_STALL can't be set alone, we pick STALL_AT_SCOREBOARD
> > +           * like the kernel. */
> 
> It's a timestamp write here, not depth count. :)  I would actually
> prefer a comment along the lines of:
> 
> /* Workaround: A non-zero post-sync op (i.e. the timestamp write below)
>  * must be preceded by a CS stall.  See rationale in the comments for
>  * intel_emit_post_sync_nonzero_flush().
>  */
> 
> The workaround chain is pretty well explained in the comments on that
> function, and so may be more helpful than "like the kernel."

Yeah, too enthusiastic copy&pasting. I'll update.

> As for the substance of the patch (actually adding the CS stall), that
> seems reasonable to me.  Odd though.  Normally failing to do this
> results in GPU hangs.  Haven't seen any of those.  Still.

I've mostly added these in the vain hope that they fix some rare pipe
control failures on snb. Alas, no :(

Bikeshed below is good, too - I wasn't too happy with the _5 either.
-Daniel

> > +          OUT_BATCH(_3DSTATE_PIPE_CONTROL);
> > +          OUT_BATCH(PIPE_CONTROL_CS_STALL |
> > +                    PIPE_CONTROL_STALL_AT_SCOREBOARD);
> > +          OUT_BATCH(0);
> > +          OUT_BATCH(0);
> > +
> > +	  OUT_BATCH(_3DSTATE_PIPE_CONTROL_5);
> 
> OUT_BATCH(_3DSTATE_PIPE_CONTROL | (5 - 2))
> 
> >  	  OUT_BATCH(PIPE_CONTROL_WRITE_TIMESTAMP);
> >  	  OUT_RELOC(query->bo,
> >  		  I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION,
> >  		  PIPE_CONTROL_GLOBAL_GTT_WRITE |
> >  		  0);
> >  	  OUT_BATCH(0);
> > +	  OUT_BATCH(0);
> >  	  ADVANCE_BATCH();
> >        
> >        } else {
> > @@ -199,14 +211,26 @@ brw_end_query(struct gl_context *ctx, struct gl_query_object *q)
> >     switch (query->Base.Target) {
> >     case GL_TIME_ELAPSED_EXT:
> >        if (intel->gen >= 6) {
> > -	  BEGIN_BATCH(4);
> > -	  OUT_BATCH(_3DSTATE_PIPE_CONTROL);
> > +	  BEGIN_BATCH(9);
> > +
> > +          /* Workaround: A non-zero post-sync op (i.e. the DEPTH_COUNT write below
> > +           * needs a pipe control with CS_STALL set beforehand.
> > +           * Workaround: CS_STALL can't be set alone, we pick STALL_AT_SCOREBOARD
> > +           * like the kernel. */
> 
> ditto (comment)
> 
> > +          OUT_BATCH(_3DSTATE_PIPE_CONTROL);
> > +          OUT_BATCH(PIPE_CONTROL_CS_STALL |
> > +                    PIPE_CONTROL_STALL_AT_SCOREBOARD);
> > +          OUT_BATCH(0);
> > +          OUT_BATCH(0);
> > +
> > +	  OUT_BATCH(_3DSTATE_PIPE_CONTROL_5);
> 
> ditto (5 - 2)
> 
> >  	  OUT_BATCH(PIPE_CONTROL_WRITE_TIMESTAMP);
> >  	  OUT_RELOC(query->bo,
> >  		  I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION,
> >  		  PIPE_CONTROL_GLOBAL_GTT_WRITE |
> >  		  8);
> >  	  OUT_BATCH(0);
> > +	  OUT_BATCH(0);
> >  	  ADVANCE_BATCH();
> >        
> >        } else {
> 
> With those changes,
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48