Re: [Mesa-dev] [PATCH v3] workarounds: Update workaround names and platforms
On Wed, Feb 10, 2016 at 12:57 PM, Ian Romanick wrote: > On 02/10/2016 12:35 PM, Matt Turner wrote: >> On Tue, Feb 9, 2016 at 10:04 PM, Ben Widawsky >> wrote: >>> On Tue, Feb 09, 2016 at 10:35:45AM -0800, Matt Turner wrote: On Tue, Feb 9, 2016 at 9:44 AM, 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. 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 > > Paraphrasing what Matt says below... It hurts the existing situation > because it gives us a false sense of security. Thanks. Yeah, that is a good way of putting it, and it reminds me of the time when Fulsim wrongly told me that there was a problem $here, but didn't say anything about the actual problem $there. Providing wrong information is significantly worse than providing no information. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3] workarounds: Update workaround names and platforms
On 02/10/2016 12:35 PM, Matt Turner wrote:
> On Tue, Feb 9, 2016 at 10:04 PM, Ben Widawsky
> wrote:
>> On Tue, Feb 09, 2016 at 10:35:45AM -0800, Matt Turner wrote:
>>> On Tue, Feb 9, 2016 at 9:44 AM, 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.
>>>
>>> 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
Paraphrasing what Matt says below... It hurts the existing situation
because it gives us a false sense of security.
>> primary benefit is it gives us internally a way to easily compare the
>> workarounds for different drivers.
>
> That's part of the concern. I don't have any idea how this information
> is going to be used. If I've understood correctly, it's going to be
> fed into the workarounds database so that we can compare what we
> implement vs the Windows driver.
>
> That sounds nice, but we use that database (perhaps inappropriately)
> to learn about hardware bugs. What happens when we start feeding bad
> information into that database? (See this patch)
>
> I get that this is just futzing with comments. But can't we do
> something better that actually captures what the code is doing? I'm
> thinking a brw_wa bitfield struct, created with the screen and
> initialized with an expression that corresponds to the data we want to
> encode. For example, for WaCMPInstFlagDepClearedEarly:ivb,hsw,vlv we'd
> have
>
>WaCMPInstFlagDepClearedEarly = brw->gen == 7;
>
> and then the code that implements the workaround would simply be
> wrapped in "if (Wa->WaCMPInstFlagDepClearedEarly)"
>
> I've heard people say that "this is what the kernel does" and that
> they already have scripts, but that doesn't address the problem of the
> comments not matching the code. I grepped for "Wa" in
> drivers/gpu/drm/i915 and literally the first workaround I looked at
> (WaProgramMiArbOnOffAroundMiSetContext in i915_gem_context.c) seems to
> suffer from the problem I'm talking about -- it reads:
>
> /* WaProgramMiArbOnOffAroundMiSetContext:ivb,vlv,hsw,bdw,chv */
> if (INTEL_INFO(ring->dev)->gen >= 7) {
>
> which first makes me wonder if it's actually only Gen7 and 8 that the
> workaround applies to (which the comment would lead you to believe).
> So I checked the workarounds database, and it actually says it's just
> HSW, VLV, and IVB, but then it lists source files and it appears in
> files beyond Gen8. So in four pieces of data, we have three
> conflicting answers.
>
> Can a kernel developer weigh in? Someone please convince me that all
> of this isn't just cargo-culted.
>
> It's certainly valuable to be able to determine the status of
> workarounds in the driver. I just don't see how this can accomplish
> that goal.
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3] workarounds: Update workaround names and platforms
On Tue, Feb 9, 2016 at 10:04 PM, Ben Widawsky
wrote:
> On Tue, Feb 09, 2016 at 10:35:45AM -0800, Matt Turner wrote:
>> On Tue, Feb 9, 2016 at 9:44 AM, 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.
>>
>> 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.
That's part of the concern. I don't have any idea how this information
is going to be used. If I've understood correctly, it's going to be
fed into the workarounds database so that we can compare what we
implement vs the Windows driver.
That sounds nice, but we use that database (perhaps inappropriately)
to learn about hardware bugs. What happens when we start feeding bad
information into that database? (See this patch)
I get that this is just futzing with comments. But can't we do
something better that actually captures what the code is doing? I'm
thinking a brw_wa bitfield struct, created with the screen and
initialized with an expression that corresponds to the data we want to
encode. For example, for WaCMPInstFlagDepClearedEarly:ivb,hsw,vlv we'd
have
WaCMPInstFlagDepClearedEarly = brw->gen == 7;
and then the code that implements the workaround would simply be
wrapped in "if (Wa->WaCMPInstFlagDepClearedEarly)"
I've heard people say that "this is what the kernel does" and that
they already have scripts, but that doesn't address the problem of the
comments not matching the code. I grepped for "Wa" in
drivers/gpu/drm/i915 and literally the first workaround I looked at
(WaProgramMiArbOnOffAroundMiSetContext in i915_gem_context.c) seems to
suffer from the problem I'm talking about -- it reads:
/* WaProgramMiArbOnOffAroundMiSetContext:ivb,vlv,hsw,bdw,chv */
if (INTEL_INFO(ring->dev)->gen >= 7) {
which first makes me wonder if it's actually only Gen7 and 8 that the
workaround applies to (which the comment would lead you to believe).
So I checked the workarounds database, and it actually says it's just
HSW, VLV, and IVB, but then it lists source files and it appears in
files beyond Gen8. So in four pieces of data, we have three
conflicting answers.
Can a kernel developer weigh in? Someone please convince me that all
of this isn't just cargo-culted.
It's certainly valuable to be able to determine the status of
workarounds in the driver. I just don't see how this can accomplish
that goal.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3] workarounds: Update workaround names and platforms
> -Original Message-
> From: Widawsky, Benjamin
> Sent: Tuesday, February 09, 2016 10:04 PM
> To: Matt Turner
> Cc: Kibey, Sameer; mesa-dev@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
> 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
> > > ---
> > > 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 ha
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 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.
> > Signed-off-by: Sameer Kibey
> > ---
> > 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.
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/b
Re: [Mesa-dev] [PATCH v3] workarounds: Update workaround names and platforms
On Tue, Feb 9, 2016 at 9:44 AM, 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.
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
> ---
> 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
