On 06/26/2012 07:28 AM, Daniel Vetter wrote:
> ... and the hardware seems to take the lenght of the pipe control
> command to indicate whether the write is 64bit or 32bit. Which makes
> sense for immediate writes.
>
> I've discovered this by writing a pattern into the query object bo and
> noticing that the high 32bits are left intact, even on those pipe
> control writes that seemingly worked.
I can't find any documentation or other justification for this, but if
you think it's useful, adding the extra 0 shouldn't hurt anything.
Minor bikeshed below.
> ---
> src/mesa/drivers/dri/i965/brw_queryobj.c | 10 ++++++----
> src/mesa/drivers/dri/intel/intel_reg.h | 1 +
> 2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_queryobj.c
> b/src/mesa/drivers/dri/i965/brw_queryobj.c
> index d7870d1..dae7af0 100644
> --- a/src/mesa/drivers/dri/i965/brw_queryobj.c
> +++ b/src/mesa/drivers/dri/i965/brw_queryobj.c
> @@ -333,7 +333,7 @@ brw_emit_query_begin(struct brw_context *brw)
> return;
>
> if (intel->gen >= 6) {
> - BEGIN_BATCH(12);
> + BEGIN_BATCH(13);
>
> /* Workaround: A non-zero post-sync op (i.e. the DEPTH_COUNT write
> below
> * needs a pipe control with CS_STALL set beforehand.
> @@ -346,13 +346,14 @@ brw_emit_query_begin(struct brw_context *brw)
> OUT_BATCH(0);
>
> /* The actual DEPTH_COUNT write. */
> - OUT_BATCH(_3DSTATE_PIPE_CONTROL);
> + OUT_BATCH(_3DSTATE_PIPE_CONTROL_5);
I would really prefer this to be
OUT_BATCH(_3DSTATE_PIPE_CONTROL | (5 - 2));
since this is what we do with every other state packet command. I'm not
a fan of PIPE_CONTROL being the only command with a length baked into
the define. (That said, I'm not suggesting removing the baked length
right now...but doing this will result in | 2 | 3 which is fine, since 3
includes all the bits for 2 anyway.)
Otherwise I have to wonder what a PIPE_CONTROL_5 is.
> OUT_BATCH(PIPE_CONTROL_WRITE_DEPTH_COUNT);
> OUT_RELOC(brw->query.bo,
> I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION,
> PIPE_CONTROL_GLOBAL_GTT_WRITE |
> ((brw->query.index * 2) * sizeof(uint64_t)));
> OUT_BATCH(0);
> + OUT_BATCH(0);
>
> /* We need to emit depth stall to get the right value for the depth
> * count. As a workaround this needs a preceeding pipe control with a
> @@ -403,7 +404,7 @@ brw_emit_query_end(struct brw_context *brw)
> return;
>
> if (intel->gen >= 6) {
> - BEGIN_BATCH(12);
> + BEGIN_BATCH(13);
>
> /* Workaround: A non-zero post-sync op (i.e. the DEPTH_COUNT write
> below
> * needs a pipe control with CS_STALL set beforehand.
> @@ -416,13 +417,14 @@ brw_emit_query_end(struct brw_context *brw)
> OUT_BATCH(0);
>
> /* The actual DEPTH_COUNT write. */
> - OUT_BATCH(_3DSTATE_PIPE_CONTROL);
> + OUT_BATCH(_3DSTATE_PIPE_CONTROL_5);
ditto
> OUT_BATCH(PIPE_CONTROL_WRITE_DEPTH_COUNT);
> OUT_RELOC(brw->query.bo,
> I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION,
> PIPE_CONTROL_GLOBAL_GTT_WRITE |
> ((brw->query.index * 2 + 1) * sizeof(uint64_t)));
> OUT_BATCH(0);
> + OUT_BATCH(0);
>
> /* We need to emit depth stall to get the right value for the depth
> * count. As a workaround this needs a preceeding pipe control with a
> diff --git a/src/mesa/drivers/dri/intel/intel_reg.h
> b/src/mesa/drivers/dri/intel/intel_reg.h
> index e2a6ee2..04f7a8d 100644
> --- a/src/mesa/drivers/dri/intel/intel_reg.h
> +++ b/src/mesa/drivers/dri/intel/intel_reg.h
> @@ -59,6 +59,7 @@
> * additional flushing control.
> */
> #define _3DSTATE_PIPE_CONTROL (CMD_3D | (3 << 27) | (2 << 24)
> | 2)
> +#define _3DSTATE_PIPE_CONTROL_5 (CMD_3D | (3 << 27) | (2 << 24)
> | 3)
> #define PIPE_CONTROL_CS_STALL (1 << 20)
> #define PIPE_CONTROL_GLOBAL_SNAPSHOT_COUNT_RESET (1 << 19)
> #define PIPE_CONTROL_TLB_INVALIDATE (1 << 18)
>
and then no need to add this.
With that change, this is:
Acked-by: Kenneth Graunke <kenn...@whitecape.org>
Though, again, I'm not sure why it'd be necessary.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
