> -----Original Message-----
> From: Ben Widawsky [mailto:b...@bwidawsk.net]
> Sent: Monday, February 08, 2016 5:41 PM
> To: Kibey, Sameer
> Cc: mesa-dev@lists.freedesktop.org; Sharp, Sarah A; Widawsky, Benjamin
> Subject: Re: [Mesa-dev] [PATCH v2] workarounds: Update workaround
> names and platforms
>
> On Fri, Feb 05, 2016 at 01:59:23PM -0800, Sameer Kibey wrote:
> > Update the format in which workarounds are documented in the source
> > code. This allows mesa to be parsed by the list-workarounds utility in
> > intel-gpu-tools.
> >
> > Signed-off-by: Sameer Kibey <sameer.ki...@intel.com>
> > ---
> > changed byt to vlv for consistency.
> > src/mesa/drivers/dri/i965/brw_binding_tables.c | 2 +-
> > src/mesa/drivers/dri/i965/brw_blorp.cpp | 2 ++
> > src/mesa/drivers/dri/i965/brw_defines.h | 3 ++-
> > src/mesa/drivers/dri/i965/brw_eu_emit.c | 3 ++-
> > src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 9 ++++++---
> > src/mesa/drivers/dri/i965/brw_pipe_control.c | 4 +++-
> > src/mesa/drivers/dri/i965/gen6_queryobj.c | 2 +-
> > src/mesa/drivers/dri/i965/gen8_depth_state.c | 3 ++-
> > src/mesa/drivers/dri/i965/intel_batchbuffer.c | 2 +-
> > 9 files changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_binding_tables.c
> > b/src/mesa/drivers/dri/i965/brw_binding_tables.c
> > index f3a0310..bcf6422 100644
> > --- a/src/mesa/drivers/dri/i965/brw_binding_tables.c
> > +++ b/src/mesa/drivers/dri/i965/brw_binding_tables.c
> > @@ -54,7 +54,7 @@ static uint32_t
> > reserve_hw_bt_space(struct brw_context *brw, unsigned bytes) {
> > /* From the Broadwell PRM, Volume 16, "Workarounds",
> > - * WaStateBindingTableOverfetch:
> > + * WaStateBindingTableOverfetch:hsw,bdw,chv,bxt
> > * "HW over-fetches two cache lines of binding table indices. When
> > * using the resource streamer, SW needs to pad binding table pointer
> > * updates with an additional two cache lines."
> > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.cpp
> > b/src/mesa/drivers/dri/i965/brw_blorp.cpp
> > index 1bc6d15..dd01ea8 100644
> > --- a/src/mesa/drivers/dri/i965/brw_blorp.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_blorp.cpp
> > @@ -304,6 +304,8 @@ brw_hiz_op_params::brw_hiz_op_params(struct
> intel_mipmap_tree *mt,
> > * aligned to an 8x4 pixel block relative to the upper left corner
> > * of the depth buffer [...]
> > *
> > + * WaHizAmbiguate8x4Aligned:hsw
> > + *
> > * For hiz resolves, the rectangle must also be 8x4 aligned. Item
> > * WaHizAmbiguate8x4Aligned from the Haswell workarounds page and
> the
> > * Ivybridge simulator require the alignment.
> > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
> > b/src/mesa/drivers/dri/i965/brw_defines.h
> > index 01e0c99..5410a1d 100644
> > --- a/src/mesa/drivers/dri/i965/brw_defines.h
> > +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> > @@ -1756,7 +1756,8 @@ enum brw_message_target {
> > /* Dataport special binding table indices: */
> > #define BRW_BTI_STATELESS 255
> > #define GEN7_BTI_SLM 254
> > -/* Note that on Gen8+ BTI 255 was redefined to be IA-coherent
> > according to the
> > +/* WaForceEnableNonCoherent:bdw,chv,skl,kbl
> > + * Note that on Gen8+ BTI 255 was redefined to be IA-coherent
> > +according to the
> > * hardware spec, however because the DRM sets bit 4 of HDC_CHICKEN0
> on BDW,
> > * CHV and at least some pre-production steppings of SKL due to
> > * WaForceEnableNonCoherent, HDC memory access may have been
> > overridden by the diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > index 35d8039..918d69e 100644
> > --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > @@ -1885,7 +1885,8 @@ void brw_CMP(struct brw_codegen *p,
> > brw_set_src0(p, insn, src0);
> > brw_set_src1(p, insn, src1);
> >
> > - /* Item WaCMPInstNullDstForcesThreadSwitch in the Haswell Bspec
> workarounds
> > + /* WaCMPInstNullDstForcesThreadSwitch:ivb,hsw,vlv
> > + * Item WaCMPInstNullDstForcesThreadSwitch in the Haswell Bspec
> > + workarounds
> > * page says:
> > * "Any CMP instruction with a null destination must use a {switch}."
> > *
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > index 1916a99..24d4a9d 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > @@ -1836,7 +1836,8 @@ fs_generator::generate_code(const cfg_t *cfg,
> int dispatch_width)
> > brw_F16TO32(p, dst, src[0]);
> > break;
> > case BRW_OPCODE_CMP:
> > - /* The Ivybridge/BayTrail WaCMPInstFlagDepClearedEarly
> workaround says
> > + /* WaCMPInstFlagDepClearedEarly:ivb,hsw,vlv
> > + * The Ivybridge/BayTrail WaCMPInstFlagDepClearedEarly
> > + workaround says
> > * that when the destination is a GRF that the dependency-clear
> > bit on
> > * the flag register is cleared early.
> > *
> > @@ -1928,7 +1929,8 @@ fs_generator::generate_code(const cfg_t *cfg,
> > int dispatch_width)
> >
> > case BRW_OPCODE_BFI1:
> > assert(devinfo->gen >= 7);
> > - /* The Haswell WaForceSIMD8ForBFIInstruction workaround says that
> we
> > + /* WaForceSIMD8ForBFIInstruction:hsw
> > + * The Haswell WaForceSIMD8ForBFIInstruction workaround says
> > + that we
> > * should
> > *
> > * "Force BFI instructions to be executed always in SIMD8."
> > @@ -1947,7 +1949,8 @@ fs_generator::generate_code(const cfg_t *cfg,
> int dispatch_width)
> > case BRW_OPCODE_BFI2:
> > assert(devinfo->gen >= 7);
> > brw_set_default_access_mode(p, BRW_ALIGN_16);
> > - /* The Haswell WaForceSIMD8ForBFIInstruction workaround says that
> we
> > + /* WaForceSIMD8ForBFIInstruction:hsw
> > + * The Haswell WaForceSIMD8ForBFIInstruction workaround says
> > + that we
> > * should
> > *
> > * "Force BFI instructions to be executed always in SIMD8."
> > diff --git a/src/mesa/drivers/dri/i965/brw_pipe_control.c
> > b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> > index 6c636d2..822473c 100644
> > --- a/src/mesa/drivers/dri/i965/brw_pipe_control.c
> > +++ b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> > @@ -60,7 +60,9 @@ gen8_add_cs_stall_workaround_bits(uint32_t *flags)
> > *flags |= PIPE_CONTROL_STALL_AT_SCOREBOARD; }
> >
> > -/* Implement the WaCsStallAtEveryFourthPipecontrol workaround on
> IVB, BYT:
> > +/* WaCsStallAtEveryFourthPipecontrol:ivb,vlv
> > + *
> > + * Implement the WaCsStallAtEveryFourthPipecontrol workaround on
> IVB, BYT:
> > *
> > * "Every 4th PIPE_CONTROL command, not counting the PIPE_CONTROL
> with
> > * only read-cache-invalidate bit(s) set, must have a CS_STALL bit set."
> > diff --git a/src/mesa/drivers/dri/i965/gen6_queryobj.c
> > b/src/mesa/drivers/dri/i965/gen6_queryobj.c
> > index d508c4c..9171df0 100644
> > --- a/src/mesa/drivers/dri/i965/gen6_queryobj.c
> > +++ b/src/mesa/drivers/dri/i965/gen6_queryobj.c
> > @@ -238,7 +238,7 @@ gen6_queryobj_get_results(struct gl_context *ctx,
> >
> > case GL_FRAGMENT_SHADER_INVOCATIONS_ARB:
> > query->Base.Result = (results[1] - results[0]);
> > - /* Implement the "WaDividePSInvocationCountBy4:HSW,BDW"
> workaround:
> > + /* Implement the WaDividePSInvocationCountBy4:hsw,bdw,chv
> workaround:
> > * "Invocation counter is 4 times actual. WA: SW to divide HW
> > reported
> > * PS Invocations value by 4."
> > *
>
> As an example of rewording like I mention below, you could entirely drop the
> "Implement..." sentence, and just keep the actual wa prose. Then slap the
> label at the bottom.
Will do that.
> > diff --git a/src/mesa/drivers/dri/i965/gen8_depth_state.c
> > b/src/mesa/drivers/dri/i965/gen8_depth_state.c
> > index 93100a0..c5ac7e4 100644
> > --- a/src/mesa/drivers/dri/i965/gen8_depth_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen8_depth_state.c
> > @@ -193,7 +193,8 @@ gen8_emit_depth_stencil_hiz(struct brw_context
> *brw,
> > case GL_TEXTURE_1D_ARRAY:
> > case GL_TEXTURE_1D:
> > if (brw->gen >= 9) {
> > - /* WaDisable1DDepthStencil. Skylake+ doesn't support 1D depth
> > + /* WaDisable1DDepthStencil:skl,bxt,kbl
> > + * Skylake+ doesn't support 1D depth
> > * textures but it does allow pretending it's a 2D texture
> > * instead.
> > */
>
> Please rewrap this comment.
Will do.
> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > index f778074..6fc5ebd 100644
> > --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > @@ -228,7 +228,7 @@ brw_finish_batch(struct brw_context *brw)
> > * From the example in the docs, it seems to expect a regular pipe
> control
> > * flush here as well. We may have done it already, but meh.
> > *
> > - * See also WaAvoidRCZCounterRollover.
> > + * See also WaAvoidRCZCounterRollover:hsw
>
> Just get rid of "See also"
>
> > */
> > brw_emit_mi_flush(brw);
> > BEGIN_BATCH(2);
>
> Okay, I had a chat with Ken to make sure he's on board with this, since he's
> been one of the primary developers who looks over it. The change itself is
> somewhat trivial, but I wanted to have the conversation about whether this
> will actually be useful. I think we're of similar minds in that we hope it's
> useful, and it doesn't make much new clutter (and hopefully it doesn't get
> stale).
>
> One thing they do in the kernel, which is like is that have the workaround
> label listed last in the commit message (as an example, you can see
> intel_fbc_work_fn()). This makes it easy to spot without the tool. I think
> it's
> weird to have the workaround label with platforms in the prose of a commit
> message. It's fine if you do some rewording of the comments to make things
> work a little better with this scheme, but that's up to you.
>
> /* blahblahblah
> * workarounds suck
> * and we hate to do them.
> *
> * WaWTFMate:ilk,snb
> */
>
> I can modify this for you on merge, if you don't want to resubmit. It's up to
> you. Just let me know.
No problem, I will resubmit based on your feedback.
> --
> Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
