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 <kenn...@whitecape.org>
--
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
