On Friday, April 19, 2019 4:16:01 AM PDT Chris Wilson wrote:
> Broadwater and the rest of gen4 do support being able to saving and
> reloading context specific registers between contexts, providing isolation
> of the basic GPU state (as programmable by userspace). This allows
> userspace to assume that the GPU retains their state from one batch to the
> next, minimising the amount of state it needs to reload and manually save
> across batches.
>
> v2: CONSTANT_BUFFER woes
>
> Running through piglit turned up an interesting issue, a GPU hang inside
> the context load. The context image includes the CONSTANT_BUFFER command
> that loads an address into a on-gpu buffer, and the context load was
> executing that immediately. However, since it was reading from the GTT
> there is no guarantee that the GTT retains the same configuration as
> when the context was saved, resulting in stray reads and a GPU hang.
>
> Having tried issuing a CONSTANT_BUFFER (to disable the command) from the
> ring before saving the context to no avail, we resort to patching out
> the instruction inside the context image before loading.
>
> This does impose that gen4 always reissues CONSTANT_BUFFER commands on
> each batch, but due to the use of a shared GTT that was and will remain
> a requirement.
>
> v3: ECOSPKD to the rescue
>
> Ville found the magic bit in the ECOSPKD to disable saving and restoring
> the CONSTANT_BUFFER from the context image, thereby completely avoiding
> the GPU hangs from chasing invalid pointers. This appears to be the
> default behaviour for gen5, and so we just need to tweak gen4 to match.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Kenneth Graunke <kenneth@xxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 3 +++
> drivers/gpu/drm/i915/intel_engine_cs.c | 2 +-
> drivers/gpu/drm/i915/intel_ringbuffer.c | 14 ++++++++++++++
> 3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b74824f0b5b1..5815703ac35f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2665,6 +2665,9 @@ enum i915_power_well_id {
> # define MODE_IDLE (1 << 9)
> # define STOP_RING (1 << 8)
>
> +#define ECOSPKD _MMIO(0x21d0)
> +# define CONSTANT_BUFFER_SR_DISABLE BIT(4)
> +
The name of this register is ECOSKPD (Scratch Pad, or SK PD).
The G45 PRM says it's DevBW-C1+. I can't recall if earlier ones
shipped or not. If so, we might be in trouble. But I've seen a
lot of DevBW-A/B warnings that I'm pretty sure I've safely ignored...
With the typo fixed, this patch is:
Reviewed-by: Kenneth Graunke <kenneth@xxxxxxxxxxxxx>
> #define GEN6_GT_MODE _MMIO(0x20d0)
> #define GEN7_GT_MODE _MMIO(0x7008)
> #define GEN6_WIZ_HASHING(hi, lo) (((hi) << 9) | ((lo) << 7))
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index fc8be2fcb4e6..f9db2e0bca12 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -211,6 +211,7 @@ __intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
> return round_up(GEN6_CXT_TOTAL_SIZE(cxt_size) * 64,
> PAGE_SIZE);
> case 5:
> + case 4:
> /*
> * There is a discrepancy here between the size reported
> * by the register and the size of the context layout
> @@ -227,7 +228,6 @@ __intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
> cxt_size * 64,
> cxt_size - 1);
> return round_up(cxt_size * 64, PAGE_SIZE);
> - case 4:
> case 3:
> case 2:
> /* For the special day when i810 gets merged. */
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 2d2e33cd3fae..26b276ed00b3 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -832,6 +832,20 @@ static int init_render_ring(struct intel_engine_cs *engine)
> {
> struct drm_i915_private *dev_priv = engine->i915;
>
> + /*
> + * Disable CONSTANT_BUFFER before it is loaded from the context
> + * image. For as it is loaded, it is executed and the stored
> + * address may no longer be valid, leading to a GPU hang.
> + *
> + * This imposes the requirement that userspace reload their
> + * CONSTANT_BUFFER on every batch, fortunately a requirement
> + * they are already accustomed to from before contexts were
> + * enabled.
> + */
> + if (IS_GEN(dev_priv, 4))
> + I915_WRITE(ECOSPKD,
> + _MASKED_BIT_ENABLE(CONSTANT_BUFFER_SR_DISABLE));
> +
> /* WaTimedSingleVertexDispatch:cl,bw,ctg,elk,ilk,snb */
> if (IS_GEN_RANGE(dev_priv, 4, 6))
> I915_WRITE(MI_MODE, _MASKED_BIT_ENABLE(VS_TIMER_DISPATCH));
>
Attachment:
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
