[Mesa-dev] [PATCH] i965: Rewrite the HiZ op

[Eric Anholt]

Initial note: I'm very concerned about landing this much driver code
right before a release.  It's very nice code, and easy to read and
review, but I'm still more scared of it than the previous hacks with
known bugs in this timeframe.  For merging after 8.0, I'm fully on
board.

On Mon,  6 Feb 2012 00:33:35 -0800, Chad Versace <chad.versace at linux.intel.com> wrote:
> +    * Ideally, we would not need to flush for the resolve op. But, I suspect
> +    * that it's unsafe for CMD_PIPELINE_SELECT to occur multiple times in

I don't see any problem with that.

> +    * a single batch, and there is no safe way to ensure that other than by
> +    * fencing the resolve with flushes. Ideally, we would just detect if
> +    * a batch is in progress and do the right thing, but that would require
> +    * the ability to *safely* access brw_context::state::dirty::brw
> +    * outside of the brw_state_init() codepath.

What's brw_state_init()?  Did you mean brw_upload_state()?

What concern do you have with safely accessing the dirty bits?  The
context is single-threaded, so that's not an issue.  The batch flush
dirty bit setting happens outside of state upload, too.

> +   /* 3DSTATE_URB
> +    *
> +    * Assign the entire URB to the VS. Even though the VS disabled, URB space
> +    * is still needed because the cliiper loads the VUE's from the URB. From

"clipper"

> +    * the Sandybridge PRM, Volume 2, Part 1, Section 3DSTATE,
> +    * Dword 1.15:0 "VS Number of URB Entries":
> +    *     This field is always used (even if VS Function Enable is DISABLED).
> +
> +    * The warning below appears in the PRM (Section 3DSTATE_URB), but we can
> +    * safely ignore it because this batch contains only one draw call.
> +    *     Because of URB corruption caused by allocating a previous GS unit
> +    *     URB entry to the VS unit, software is required to send a “GS NULL
> +    *     Fence” (Send URB fence with VS URB size == 1 and GS URB size == 0)
> +    *     plus a dummy DRAW call before any case where VS will be taking over
> +    *     GS URB space.
> +    */

As noted above, I don't think we really need to flush the batch around
the hiz op.  But that currently is providing a sufficient guarantee for
this requirement: MI_BATCH_BUFFER_START is a full pipeline stall, and
all this requirement is about is getting enough of a pipeline stall, I
think.

> +   /* 3DSTATE_DEPTH_BUFFER */
> +   {
> +      uint32_t width = mt->level[level].width;
> +      uint32_t height = mt->level[level].height;
> +
> +      uint32_t tile_x;
> +      uint32_t tile_y;
> +      uint32_t offset;
> +      {
> +         /* Construct a dummy renderbuffer just to extract tile offsets. */
> +         struct intel_renderbuffer rb;
> +         rb.mt = mt;
> +         rb.mt_level = level;
> +         rb.mt_layer = layer;
> +         intel_renderbuffer_set_draw_offset(&rb);
> +         offset = intel_renderbuffer_tile_offsets(&rb, &tile_x, &tile_y);
> +      }

I'd remove the block indent here.

The actual body of the code in this patch is:

Reviewed-by: Eric Anholt <eric at anholt.net>

batch flushing mitigation can come later if we feel like it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120207/758e707e/attachment.pgp>