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

[Chad Versace]

On 02/07/2012 01:10 PM, Eric Anholt wrote:
> 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.

I agree that this is a scary piece of code to commit the day of the release. Ken,
Ian, Paul, and I agreed that we should give QA time to digest it and it should
likely land in 8.0.1.

> 
> 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.

I was unsure if it was safe for CMD_PIPELINE_SELECT to occur multiple times
in a batch, so I chose to err on the side of safety and emit a new batch to
eliminate my cause of uncertainty.

> 
>> +    * 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()?

Oops. I meant 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"

Typo fixed.

> 
>> +    * 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.

I do want to eliminate the unnecessary flushing, and I would like to do
that and the requisite dirty bit rework in follow-on work. Paul and I
brainstormed on a way to cleanly do this, and I think all of us should
discuss it in-person one day.

Thanks for taking the time to read the patch.

----
Chad Versace
chad.versace at linux.intel.com