| Message ID | 20190815215859.10970-1-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gen11: Add Wa_1604278689:icl,ehl | expand |
Quoting Matt Roper (2019-08-15 22:58:59) > From the bspec: > > "SW must always program the FBC_RT_BASE_ADDR_REGISTER_* register > in Render Engine to a reserved value (0xFFFF_FFFF) such that the > programmed value doesn’t match the render target surface address > programmed. This would disable render engine from generating > modify messages to FBC unit in display." > > Bspec: 11388 > Bspec: 33451 > Cc: José Roberto de Souza <jose.souza@intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_workarounds.c | 6 ++++++ > drivers/gpu/drm/i915/i915_reg.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c > index 704ace01e7f5..29b50e2c0627 100644 > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > @@ -567,6 +567,12 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine, > /* allow headerless messages for preemptible GPGPU context */ > WA_SET_BIT_MASKED(GEN10_SAMPLER_MODE, > GEN11_SAMPLER_ENABLE_HEADLESS_MSG); > + > + /* Wa_1604278689:icl,ehl */ > + wa_write_masked_or(wal, IVB_FBC_RT_BASE_UPPER, > + 0, /* write-only register; skip validation */ > + 0xFFFFFFFF); > + wa_write(wal, IVB_FBC_RT_BASE, 0xFFFFFFFF); It's part of the context? -Chris
On Thu, Aug 15, 2019 at 11:19:36PM +0100, Chris Wilson wrote: > Quoting Matt Roper (2019-08-15 22:58:59) > > From the bspec: > > > > "SW must always program the FBC_RT_BASE_ADDR_REGISTER_* register > > in Render Engine to a reserved value (0xFFFF_FFFF) such that the > > programmed value doesn’t match the render target surface address > > programmed. This would disable render engine from generating > > modify messages to FBC unit in display." > > > > Bspec: 11388 > > Bspec: 33451 > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 6 ++++++ > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > 2 files changed, 7 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > index 704ace01e7f5..29b50e2c0627 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > @@ -567,6 +567,12 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine, > > /* allow headerless messages for preemptible GPGPU context */ > > WA_SET_BIT_MASKED(GEN10_SAMPLER_MODE, > > GEN11_SAMPLER_ENABLE_HEADLESS_MSG); > > + > > + /* Wa_1604278689:icl,ehl */ > > + wa_write_masked_or(wal, IVB_FBC_RT_BASE_UPPER, > > + 0, /* write-only register; skip validation */ > > + 0xFFFFFFFF); > > + wa_write(wal, IVB_FBC_RT_BASE, 0xFFFFFFFF); > > It's part of the context? > -Chris The register definitions say "This Register is saved and restored as part of Context" so I think so? But that does seem to be different than how we used to program this register back before commit b339088d8 ("drm/i915: Don't write IVB_FBC_RT_BASE") so maybe I'm misinterpreting? Matt
Quoting Patchwork (2019-08-16 00:52:20) > #### Possible regressions #### > > * igt@i915_selftest@live_hangcheck: > - fi-icl-u3: [PASS][1] -> [DMESG-FAIL][2] > [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6714/fi-icl-u3/igt@i915_selftest@live_hangcheck.html > [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14040/fi-icl-u3/igt@i915_selftest@live_hangcheck.html > - {fi-icl-dsi}: [PASS][3] -> [DMESG-FAIL][4] > [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6714/fi-icl-dsi/igt@i915_selftest@live_hangcheck.html > [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14040/fi-icl-dsi/igt@i915_selftest@live_hangcheck.html > - {fi-icl-u4}: [PASS][5] -> [DMESG-FAIL][6] > [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6714/fi-icl-u4/igt@i915_selftest@live_hangcheck.html > [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14040/fi-icl-u4/igt@i915_selftest@live_hangcheck.html All icl machines suffering a similar failure to reset an engine (not rcs!). We haven't seen that before, so it does look very suspicious. -Chris
On Fri, Aug 16, 2019 at 08:07:18AM +0100, Chris Wilson wrote: > Quoting Patchwork (2019-08-16 00:52:20) > > #### Possible regressions #### > > > > * igt@i915_selftest@live_hangcheck: > > - fi-icl-u3: [PASS][1] -> [DMESG-FAIL][2] > > [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6714/fi-icl-u3/igt@i915_selftest@live_hangcheck.html > > [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14040/fi-icl-u3/igt@i915_selftest@live_hangcheck.html > > - {fi-icl-dsi}: [PASS][3] -> [DMESG-FAIL][4] > > [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6714/fi-icl-dsi/igt@i915_selftest@live_hangcheck.html > > [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14040/fi-icl-dsi/igt@i915_selftest@live_hangcheck.html > > - {fi-icl-u4}: [PASS][5] -> [DMESG-FAIL][6] > > [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6714/fi-icl-u4/igt@i915_selftest@live_hangcheck.html > > [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14040/fi-icl-u4/igt@i915_selftest@live_hangcheck.html > > All icl machines suffering a similar failure to reset an engine (not > rcs!). We haven't seen that before, so it does look very suspicious. > -Chris Hmm. So for a render engine register that's saved/restored as part of the context, is there somewhere else I should be setting this value? My understanding was that the items added in *_ctx_workarounds_init only applied to the render engine (since __intel_engine_init_ctx_wa bails out for other engine classes), yet it seems it's the BCS engine that's failing to reset with this patch?. If I just I915_WRITE to this register, won't the value only apply to whichever context is currently executing on the RCS engine and be lost when other contexts switch in? Matt
Quoting Matt Roper (2019-08-16 17:29:09) > On Fri, Aug 16, 2019 at 08:07:18AM +0100, Chris Wilson wrote: > > Quoting Patchwork (2019-08-16 00:52:20) > > > #### Possible regressions #### > > > > > > * igt@i915_selftest@live_hangcheck: > > > - fi-icl-u3: [PASS][1] -> [DMESG-FAIL][2] > > > [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6714/fi-icl-u3/igt@i915_selftest@live_hangcheck.html > > > [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14040/fi-icl-u3/igt@i915_selftest@live_hangcheck.html > > > - {fi-icl-dsi}: [PASS][3] -> [DMESG-FAIL][4] > > > [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6714/fi-icl-dsi/igt@i915_selftest@live_hangcheck.html > > > [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14040/fi-icl-dsi/igt@i915_selftest@live_hangcheck.html > > > - {fi-icl-u4}: [PASS][5] -> [DMESG-FAIL][6] > > > [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6714/fi-icl-u4/igt@i915_selftest@live_hangcheck.html > > > [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14040/fi-icl-u4/igt@i915_selftest@live_hangcheck.html > > > > All icl machines suffering a similar failure to reset an engine (not > > rcs!). We haven't seen that before, so it does look very suspicious. > > -Chris > > Hmm. So for a render engine register that's saved/restored as part of > the context, is there somewhere else I should be setting this value? My > understanding was that the items added in *_ctx_workarounds_init only > applied to the render engine (since __intel_engine_init_ctx_wa bails out > for other engine classes), yet it seems it's the BCS engine that's > failing to reset with this patch?. If I just I915_WRITE to this > register, won't the value only apply to whichever context is currently > executing on the RCS engine and be lost when other contexts switch in? The magic is in the ordering. If you put it in the gt_workarounds, it gets applied before we record the default context image -- and so it gets copied into all subsequent contexts. The only reason why we still have ctx_workarounds is that some registers had to be written via CS, and it's easy for us to apply the rule "if the spec says it is a context register, put it in the ctx_workarounds". We also use that to determine whether to use a SRM or mmio verification. At the end of the day, whatever works :) -Chris
On Thu, Aug 15, 2019 at 02:58:59PM -0700, Matt Roper wrote: > From the bspec: > > "SW must always program the FBC_RT_BASE_ADDR_REGISTER_* register > in Render Engine to a reserved value (0xFFFF_FFFF) such that the > programmed value doesn’t match the render target surface address > programmed. This would disable render engine from generating > modify messages to FBC unit in display." This looks a bit peculiar. That magic value seems to imply that the RT_VALID bit no longer functions as intended. I filed a spec issue to get some clarification on this. > > Bspec: 11388 > Bspec: 33451 > Cc: José Roberto de Souza <jose.souza@intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_workarounds.c | 6 ++++++ > drivers/gpu/drm/i915/i915_reg.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c > index 704ace01e7f5..29b50e2c0627 100644 > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > @@ -567,6 +567,12 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine, > /* allow headerless messages for preemptible GPGPU context */ > WA_SET_BIT_MASKED(GEN10_SAMPLER_MODE, > GEN11_SAMPLER_ENABLE_HEADLESS_MSG); > + > + /* Wa_1604278689:icl,ehl */ > + wa_write_masked_or(wal, IVB_FBC_RT_BASE_UPPER, > + 0, /* write-only register; skip validation */ > + 0xFFFFFFFF); > + wa_write(wal, IVB_FBC_RT_BASE, 0xFFFFFFFF); > } > > static void > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index def6dbdc7e2e..14af1b1dc0d3 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3214,6 +3214,7 @@ enum i915_power_well_id { > > /* Framebuffer compression for Ivybridge */ > #define IVB_FBC_RT_BASE _MMIO(0x7020) > +#define IVB_FBC_RT_BASE_UPPER _MMIO(0x7024) That register seems to be BDW+ actually. > > #define IPS_CTL _MMIO(0x43408) > #define IPS_ENABLE (1 << 31) > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Aug 19, 2019 at 07:13:56PM +0300, Ville Syrjälä wrote: > On Thu, Aug 15, 2019 at 02:58:59PM -0700, Matt Roper wrote: > > From the bspec: > > > > "SW must always program the FBC_RT_BASE_ADDR_REGISTER_* register > > in Render Engine to a reserved value (0xFFFF_FFFF) such that the > > programmed value doesn’t match the render target surface address > > programmed. This would disable render engine from generating > > modify messages to FBC unit in display." > > This looks a bit peculiar. That magic value seems to imply that the > RT_VALID bit no longer functions as intended. I filed a spec issue to > get some clarification on this. Yeah, this worried me as well, although I figured their logic was that turning on the 'valid' bit was okay as long as the address comparisons always returned a mismatch. However CI starts failing with this workaround applied, and experimentation with trybot indicates that the failures go away when I add a "& ~RT_VALID" to the RT_BASE register value. I'll submit an updated version that turns that bit off in a bit. Matt > > > > > Bspec: 11388 > > Bspec: 33451 > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 6 ++++++ > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > 2 files changed, 7 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > index 704ace01e7f5..29b50e2c0627 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > @@ -567,6 +567,12 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine, > > /* allow headerless messages for preemptible GPGPU context */ > > WA_SET_BIT_MASKED(GEN10_SAMPLER_MODE, > > GEN11_SAMPLER_ENABLE_HEADLESS_MSG); > > + > > + /* Wa_1604278689:icl,ehl */ > > + wa_write_masked_or(wal, IVB_FBC_RT_BASE_UPPER, > > + 0, /* write-only register; skip validation */ > > + 0xFFFFFFFF); > > + wa_write(wal, IVB_FBC_RT_BASE, 0xFFFFFFFF); > > } > > > > static void > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index def6dbdc7e2e..14af1b1dc0d3 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -3214,6 +3214,7 @@ enum i915_power_well_id { > > > > /* Framebuffer compression for Ivybridge */ > > #define IVB_FBC_RT_BASE _MMIO(0x7020) > > +#define IVB_FBC_RT_BASE_UPPER _MMIO(0x7024) > > That register seems to be BDW+ actually. > > > > > #define IPS_CTL _MMIO(0x43408) > > #define IPS_ENABLE (1 << 31) > > -- > > 2.20.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c index 704ace01e7f5..29b50e2c0627 100644 --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c @@ -567,6 +567,12 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine, /* allow headerless messages for preemptible GPGPU context */ WA_SET_BIT_MASKED(GEN10_SAMPLER_MODE, GEN11_SAMPLER_ENABLE_HEADLESS_MSG); + + /* Wa_1604278689:icl,ehl */ + wa_write_masked_or(wal, IVB_FBC_RT_BASE_UPPER, + 0, /* write-only register; skip validation */ + 0xFFFFFFFF); + wa_write(wal, IVB_FBC_RT_BASE, 0xFFFFFFFF); } static void diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index def6dbdc7e2e..14af1b1dc0d3 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3214,6 +3214,7 @@ enum i915_power_well_id { /* Framebuffer compression for Ivybridge */ #define IVB_FBC_RT_BASE _MMIO(0x7020) +#define IVB_FBC_RT_BASE_UPPER _MMIO(0x7024) #define IPS_CTL _MMIO(0x43408) #define IPS_ENABLE (1 << 31)
From the bspec: "SW must always program the FBC_RT_BASE_ADDR_REGISTER_* register in Render Engine to a reserved value (0xFFFF_FFFF) such that the programmed value doesn’t match the render target surface address programmed. This would disable render engine from generating modify messages to FBC unit in display." Bspec: 11388 Bspec: 33451 Cc: José Roberto de Souza <jose.souza@intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/gt/intel_workarounds.c | 6 ++++++ drivers/gpu/drm/i915/i915_reg.h | 1 + 2 files changed, 7 insertions(+)