On Wed, Mar 08, 2017 at 02:17:59AM -0800, Kenneth Graunke wrote: > On Thursday, March 2, 2017 4:36:08 PM PST Nanley Chery wrote: > > On Mon, Feb 06, 2017 at 03:55:49PM -0800, Kenneth Graunke wrote: > > > If a HiZ op is the first thing in the batch, we should make sure > > > to select the render pipeline and emit state base address before > > > proceeding. > > > > > > I believe 3DSTATE_WM_HZ_OP creates 3DPRIMITIVEs internally, and > > > dispatching those on the GPGPU pipeline seems a bit sketchy. I'm > > > > Yes, it does seem like we currently allow HZ_OPs within a GPGPU > > pipeline. This patch should fix that problem. > > > > > not actually sure that STATE_BASE_ADDRESS is necessary, as the > > > depth related commands use graphics addresses, not ones relative > > > to the base address...but we're likely to set it as part of the > > > next operation anyway, so we should just do it right away. > > > > > > > I agree, re-emitting STATE_BASE_ADDRESS doesn't seem necessary. I think > > we should drop this part of the patch and add it back in later if we get > > some data that it's necessary. Leaving it there may be distracting to > > some readers and the BDW PRM warns that it's an expensive command: > > > > Execution of this command causes a full pipeline flush, thus its > > use should be minimized for higher performance. > > I think it should be basically free, actually. We track a boolean, > brw->batch.state_base_address_emitted, to avoid emitting it multiple > times per batch. > > Let's say the first thing in a fresh batch is a HiZ op, followed by > normal drawing. Previously, we'd do: > > 1. HiZ op commands > 2. STATE_BASE_ADDRESS (triggered by normal rendering upload) > 3. rest of normal drawing commands > > Now we'd do: > > 1. STATE_BASE_ADDRESS (triggered by HiZ op) > 2. HiZ op commands > 3. normal drawing commands (second SBA is skipped) > > In other words...we're just moving it a bit earlier. I suppose there > could be a batch containing only HiZ ops, at which point we'd pay for > a single STATE_BASE_ADDRESS...but that seems really unlikely. >
Sorry for not stating it up front, but the special case you've mentioned is exactly what I'd like not to hurt unnecessarily. > > > Cc: "17.0" <mesa-sta...@lists.freedesktop.org> > > > Signed-off-by: Kenneth Graunke <kenn...@whitecape.org> > > > --- > > > src/mesa/drivers/dri/i965/gen8_depth_state.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/src/mesa/drivers/dri/i965/gen8_depth_state.c > > > b/src/mesa/drivers/dri/i965/gen8_depth_state.c > > > index a7e61354fd5..620b32df8bb 100644 > > > --- a/src/mesa/drivers/dri/i965/gen8_depth_state.c > > > +++ b/src/mesa/drivers/dri/i965/gen8_depth_state.c > > > @@ -404,6 +404,9 @@ gen8_hiz_exec(struct brw_context *brw, struct > > > intel_mipmap_tree *mt, > > > if (op == BLORP_HIZ_OP_NONE) > > > return; > > > > > > > It would be helpful if you included the rationale here as a code > > comment. Something like the first two sentences of your commit message > > should work. > > I can do that. > > > > + brw_select_pipeline(brw, BRW_RENDER_PIPELINE); > > > > According to Vol07 of the BDW+ PRMs, > > > > The previously active pipeline needs to be flushed via the > > MI_FLUSH command immediately before switching to a different > > pipeline via use of the PIPELINE_SELECT command. > > > > However it doesn't look like MI_FLUSH is present after HSW. So there > > shouldn't be any additional work to do here. > > Flushes are definitely required when switching the pipeline, but I > believe that brw_emit_select_pipeline() does that work. > > FWIW, MI_FLUSH was replaced by PIPE_CONTROL many generations ago. > I believe the validation team stopped testing MI_FLUSH on Sandybridge. > Thanks for letting me know about the MI_FLUSH replacement. Do you know what bits must be set to perform an equivalent operation? From the looks of it, brw_emit_select_pipeline() actually avoids emitting PIPE_CONTROL BDW+. -Nanley > --Ken _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
