Quoting Ville Syrjälä (2017-11-23 17:46:23)
> On Thu, Nov 23, 2017 at 04:27:48PM +0000, Chris Wilson wrote:
> > Ironlake does 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.
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_engine_cs.c | 2 ++
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++++++
> > 2 files changed, 9 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index fede62daf3e1..88ef00faf576 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -175,6 +175,8 @@ __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:
> > + cxt_size = I915_READ(CXT_SIZE);
> > + return round_up(cxt_size * 64, PAGE_SIZE);
>
> I don't think this is correct. It misses the non-pipelined 3D state,
> and the ring stuff at the start which IIRC at least SNB still
> saved even though it's not used in ring buffer mode. So I think
> this needs a 0xb added to the CXT_SIZE value.
/me scratches his head
This was described as the full size of the context; including the
extended state required for the power context.
In the past we know that it is less that one page, which is our minimum
allocation.
> But even that doesn't really match the docs. The context image layout
> is shown to be 0x3b cachelines long, but 0xb+0x2d only gets us to
> 0x38. So it looks like CXT_SIZE is off by two for some reason. But
> it does't really matter I suppose since we round it up to
> a page anyway.
Right. I started with just using 1 page and ignoring the register all
together.
> I don't think I ever looked at the context dump to see how big it
> really is on ILK. IIRC I did that on SNB and IVB, and maybe HSW.
>
> > case 4:
> > case 3:
> > case 2:
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index e2085820b586..e649b564b165 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -1403,11 +1403,14 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
> > /* These flags are for resource streamer on HSW+ */
> > flags |= HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN;
> > else
> > + /* We need to save the extended state for powersaving modes */
> > flags |= MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN;
> >
> > len = 4;
> > if (IS_GEN7(i915))
> > len += 2 + (num_rings ? 4*num_rings + 6 : 0);
> > + if (IS_GEN5(i915))
> > + len += 2;
> >
> > cs = intel_ring_begin(rq, len);
> > if (IS_ERR(cs))
> > @@ -1430,6 +1433,8 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
> > GEN6_PSMI_SLEEP_MSG_DISABLE);
> > }
> > }
> > + } else if (IS_GEN5(i915)) {
> > + *cs++ = MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN;
>
> Hmm. These are documented for steppings A/B. I wonder what is the
> first production stepping for ILK. Latest stepping mentioned in the
> w/a db is C2.
I was going by Ben's notes that we still wanted these for undetermined
reasons based around the powerctx programming example, iirc. Certainly,
the bspec I found only had them for a/b, but it should be a no-op for us
in any case as we don't use sync flush.
> Oh, actually w/a db has WaIlkEnableDisableSuspendFlush listed a
> "forever". Not sure which source to believe here.
I think it's safe and cheap enough to include them. But probably worth a
note that it's more paranoia than anything.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
