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.vers...@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 <e...@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.vers...@linux.intel.com
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
