[Mesa-dev] [PATCH v3] workarounds: Update workaround names and platforms
Kibey, Sameer
sameer.kibey at intel.com
Wed Feb 10 18:35:51 UTC 2016
> -----Original Message-----
> From: Widawsky, Benjamin
> Sent: Tuesday, February 09, 2016 10:04 PM
> To: Matt Turner
> Cc: Kibey, Sameer; mesa-dev at lists.freedesktop.org; Kenneth Graunke
> Subject: Re: [Mesa-dev] [PATCH v3] workarounds: Update workaround
> names and platforms
>
> On Tue, Feb 09, 2016 at 10:35:45AM -0800, Matt Turner wrote:
> > On Tue, Feb 9, 2016 at 9:44 AM, Sameer Kibey <sameer.kibey at intel.com>
> 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.
> >
> > I don't know that I find this valuable.
> >
> > Ben touched on one concern -- keeping it updated. But I have another,
> > and that's whether the information is accurate, or useful at all.
> >
> >
>
> I think the bottom line is it really doesn't hurt the existing situation. The
> primary benefit is it gives us internally a way to easily compare the
> workarounds for different drivers.
Agree with Ben. Also this patch is the first step to get Mesa integrated into the Workaround database tool. If we can get Mesa into the WA database tool, that will allow quick comparing of Mesa WAs with the Windows driver, so that we can answer the question - what workarounds is Linux missing that Windows driver has? In some cases, it may help debug issues faster.
> > > Signed-off-by: Sameer Kibey <sameer.kibey at intel.com>
> > > ---
> > > src/mesa/drivers/dri/i965/brw_binding_tables.c | 3 ++-
> > > src/mesa/drivers/dri/i965/brw_blorp.cpp | 2 ++
> > > src/mesa/drivers/dri/i965/brw_defines.h | 2 ++
> > > src/mesa/drivers/dri/i965/brw_eu_emit.c | 2 ++
> > > src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 6 ++++++
> > > src/mesa/drivers/dri/i965/brw_pipe_control.c | 2 ++
> > > src/mesa/drivers/dri/i965/gen6_queryobj.c | 5 +++--
> > > src/mesa/drivers/dri/i965/gen8_depth_state.c | 7 ++++---
> > > src/mesa/drivers/dri/i965/intel_batchbuffer.c | 2 +-
> > > 9 files changed, 24 insertions(+), 7 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..6dd35dd 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_binding_tables.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_binding_tables.c
> > > @@ -54,13 +54,14 @@ static uint32_t
> > > reserve_hw_bt_space(struct brw_context *brw, unsigned bytes) {
> > > /* From the Broadwell PRM, Volume 16, "Workarounds",
> > > - * WaStateBindingTableOverfetch:
> > > * "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."
> > > *
> > > * Cache lines are 64 bytes, so we subtract 128 bytes from the size of
> > > * the binding table pool buffer.
> > > + *
> > > + * WaStateBindingTableOverfetch:hsw,bdw,chv,bxt
> > > */
> > > if (brw->hw_bt_pool.next_offset + bytes >= brw->hw_bt_pool.bo-
> >size - 128) {
> > > gen7_reset_hw_bt_pool_offsets(brw);
> > > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.cpp
> > > b/src/mesa/drivers/dri/i965/brw_blorp.cpp
> > > index 1bc6d15..f798e29 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_blorp.cpp
> > > +++ b/src/mesa/drivers/dri/i965/brw_blorp.cpp
> > > @@ -318,6 +318,8 @@ brw_hiz_op_params::brw_hiz_op_params(struct
> intel_mipmap_tree *mt,
> > > * SURFACE_STATE.Surface_Horizontal_Alignment should be 4 for Z24
> surfaces,
> > > * not 8. But commit 1f112cc increased the alignment from 4 to 8, which
> > > * prevents the clobbering.
> > > + *
> > > + * WaHizAmbiguate8x4Aligned:hsw
> > > */
> > > depth.width = ALIGN(depth.width, 8);
> > > depth.height = ALIGN(depth.height, 4); diff --git
> > > a/src/mesa/drivers/dri/i965/brw_defines.h
> > > b/src/mesa/drivers/dri/i965/brw_defines.h
> > > index 01e0c99..2146172 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_defines.h
> > > +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> > > @@ -1762,6 +1762,8 @@ enum brw_message_target {
> > > * WaForceEnableNonCoherent, HDC memory access may have been
> overridden by the
> > > * kernel to be non-coherent (matching the behavior of the same BTI on
> > > * pre-Gen8 hardware) and BTI 255 may actually be an alias for BTI 253.
> > > + *
> > > + * WaForceEnableNonCoherent:bdw,chv,skl,kbl
> >
> > This comment is just explaining that 255 on Gen8+ doesn't mean the
> > same thing as it did before. It passingly mentions
> > WaForceEnableNonCoherent, but that's a kernel driver workaround --
> > nothing in Mesa.
Ok, I will remove this WA as it does not apply to Mesa.
> > Moreover, when I look that up in the workarounds database it says it
> > applies to all BDW, CHV, KBL, but only SKL until D0. I'm not sure if
> > we even care about D0. Was that publicly released? Mentioning that it
> > applies to "skl" doesn't tell the whole story and might lead to us
> > applying it when it's not needed.
Right now the intel-gpu-tools utility to list workarounds which will parse this string, does not support parsing stepping info. Even the kernel code does not list the stepping info when documenting workarounds in this format. We can consider adding stepping, but it will require changes to the i-g-t list-workarounds script and also changes to 100+ workarounds listed in the kernel.
> There is definitely a huge gap regarding what the actual definition of a
> workaround is. My definition is actually very different than most
> workarounds, which consist of "set this bit". A workaround to me is like,
> geometry stage is broken, so we have to do it in software.... but whatever.
>
> I think your concerns about the accuracy are valid. I believe the kernel does
> attempt to capture stepping info, and we do not. We (I, anyway) make an
> effort to expunge workarounds for platforms that don't ship, so it's hopefully
> not even a thing for us.
>
> >
> > > */
> > > #define GEN8_BTI_STATELESS_IA_COHERENT 255
> > > #define GEN8_BTI_STATELESS_NON_COHERENT 253 diff --git
> > > a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > index 35d8039..7a6179a 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > @@ -1891,6 +1891,8 @@ void brw_CMP(struct brw_codegen *p,
> > > *
> > > * It also applies to other Gen7 platforms (IVB, BYT) even though it isn't
> > > * mentioned on their work-arounds pages.
> > > + *
> > > + * WaCMPInstNullDstForcesThreadSwitch:ivb,hsw,vlv
> >
> > Not related to your patch, but the ivbgt2 Bug database has not worked
> > for me... ever? Try the link in the workarounds database for this one.
> > Does it work for you? Maybe you've made contacts during this project
> > that might know something?
I don't have access to the ivbgt2 bug database. But the link shows up these two names as points of contact for this project:
Hantelmann, Angela +1 916 356 1546 FM5-2-C4/D4
Goh, Karen Liang Mei +60 4 253 6563 PG12-L2-G2
> > > */
> > > if (devinfo->gen == 7) {
> > > if (dest.file == BRW_ARCHITECTURE_REGISTER_FILE && diff --git
> > > a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > > b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > > index 1916a99..3d19605 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > > +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > > @@ -1846,6 +1846,8 @@ fs_generator::generate_code(const cfg_t *cfg,
> int dispatch_width)
> > > * We choose to split into CMP(8) instructions since disabling
> > > * coissuing would affect CMP instructions not otherwise affected by
> > > * the errata.
> > > + *
> > > + * WaCMPInstFlagDepClearedEarly:ivb,hsw,vlv
> >
> > Again, "ivb,hsw,vlv" is misleading (unless I'm just misunderstanding)
> > because it only applies to HSW Mobile A1, which we certainly don't
> > care about.
Ok, I will remove "hsw" from the list for this WA and resubmit.
> > > */
> > > if (dispatch_width == 16 && devinfo->gen == 7 &&
> > > !devinfo->is_haswell) {
> >
> > And in fact, you can see right here that we don't implement it for HSW.
> >
> > > if (dst.file == BRW_GENERAL_REGISTER_FILE) { @@ -1932,6
> > > +1934,8 @@ fs_generator::generate_code(const cfg_t *cfg, int
> dispatch_width)
> > > * should
> > > *
> > > * "Force BFI instructions to be executed always in SIMD8."
> > > + *
> > > + * WaForceSIMD8ForBFIInstruction:hsw
> > > */
> > > if (dispatch_width == 16 && devinfo->is_haswell) {
> > > brw_set_default_exec_size(p, BRW_EXECUTE_8); @@ -1954,6
> > > +1958,8 @@ fs_generator::generate_code(const cfg_t *cfg, int
> dispatch_width)
> > > *
> > > * Otherwise we would be able to emit compressed instructions like
> we
> > > * do for the other three-source instructions.
> > > + *
> > > + * WaForceSIMD8ForBFIInstruction:hsw
> > > */
> > > if (dispatch_width == 16 &&
> > > (devinfo->is_haswell ||
> > > !devinfo->supports_simd16_3src)) { diff --git
> > > a/src/mesa/drivers/dri/i965/brw_pipe_control.c
> > > b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> > > index b41e28e..51b20b2 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_pipe_control.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> > > @@ -67,6 +67,8 @@ gen8_add_cs_stall_workaround_bits(uint32_t
> *flags)
> > > *
> > > * Note that the kernel does CS stalls between batches, so we only need
> > > * to count them within a batch.
> > > + *
> > > + * WaCsStallAtEveryFourthPipecontrol:ivb,vlv
> >
> > Now I'm confused, because this one also applies to HSW Mobile until B0
> > (which we again don't care about), but it's not listed here. How can
> > that be?
Mesa doesn't implement the WaCsStallAtEveryFourthPipecontrol for HSW as per below line, so does not make sense to list HSW for this WA.
if (brw->gen == 7 && !brw->is_haswell) {
> Sameer, can you address some of Matt's concerns about the accuracy of the
> workarounds? I realize you don't know the mesa codebase well, but feel free
> to ask questions on IRC if you have them.
>
> >
> >
> > The Windows driver has a big list defining Wa* names in terms of some
> > conditional that matches the equivalent of devinfo->gen >= xx. Other
> > than doing something like that where the code itself defines what
> > workarounds we implement, I don't see how this can be successful.
>
> Yeah. It would be good if this wasn't the end of how we handle
> workarounds, I hope it's the beginning of coming up with a good way to
> manage the situation.
> I'm open to disagreement on the following, but: in my head, a QA or
> integration person should be able to easily figure out if our driver
> implements some workaround. Most importantly I'll reiterate that I don't
> think this actually makes the situation any worse than it is now, and I have
> some pretty high hopes that we'll be able to utilize some internal tools that
> do make this useful (especially when you consider this work is going to list
> kernel workarounds as well).
>
> So I guess like I said in my last comment - I hope this pans out, but even if it
> doesn't, the patch Sameer is hoping to land here is really innocuous.
>
>
> --
> Ben Widawsky, Intel Open Source Technology Center
More information about the mesa-dev
mailing list