[Intel-gfx] [PATCH 1/3] drm/i915: Enable render context support for Ironlake (gen5)

[Chris Wilson]

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 at chris-wilson.co.uk>
> > ---
> >  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