[Mesa-dev] [PATCH v3] workarounds: Update workaround names and platforms
Matt Turner
mattst88 at gmail.com
Tue Feb 9 18:35:45 UTC 2016
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.
> 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.
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.
> */
> #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?
> */
> 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.
> */
> 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?
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.
More information about the mesa-dev
mailing list