[Intel-gfx] [PATCH] drm/i915/guc: Don't make assumptions while getting the lrca offset

[Chris Wilson]

Quoting Michel Thierry (2017-07-12 01:14:46)
> On 7/11/2017 5:07 PM, Chris Wilson wrote:
> > Quoting Chris Wilson (2017-07-12 01:00:02)
> >> Also feel free to try and fixup the other confusion in intel_lrc.c.
> >> Perhaps something like:
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >> index b0738d2..f498aa6 100644
> >> --- a/drivers/gpu/drm/i915/intel_lrc.c
> >> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> >> @@ -2031,8 +2031,9 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> >>   
> >>          context_size = round_up(engine->context_size, I915_GTT_PAGE_SIZE);
> >>   
> >> -       /* One extra page as the sharing data between driver and GuC */
> >> -       context_size += PAGE_SIZE * LRC_PPHWSP_PN;
> >> +       /* Add an extra page at the start for sharing data with the GuC */
> >> +       BUILD_BUG_ON(LRC_PPHWSP_PN != 1);
> >> +       context_size += PAGE_SIZE;
> >>   
> >>          ctx_obj = i915_gem_object_create(ctx->i915, context_size);
> >>          if (IS_ERR(ctx_obj)) {
> > 
> > Or better
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index b0738d2..0b7fb38 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -2031,8 +2031,8 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> >   
> >          context_size = round_up(engine->context_size, I915_GTT_PAGE_SIZE);
> >   
> > -       /* One extra page as the sharing data between driver and GuC */
> > -       context_size += PAGE_SIZE * LRC_PPHWSP_PN;
> > +       /* Add an extra page at the start for sharing data with the GuC */
> > +       context_size += LRC_GUCSHR_PAGES * PAGE_SIZE;

One more thought about this, and with a little coaxing LRC_PPHWSP_PN may
be the most appropriate constant to use here.

/* Before the actual start of the context image, we insert a few pages
 * for our own use and for sharing with the GuC.
 */
context_size += LRC_HEADER_PAGES * PAGE_SIZE;

> >          ctx_obj = i915_gem_object_create(ctx->i915, context_size);
> >          if (IS_ERR(ctx_obj)) {
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> > index 52b3a1f..848ca21 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.h
> > +++ b/drivers/gpu/drm/i915/intel_lrc.h
> > @@ -72,7 +72,8 @@ int logical_xcs_ring_init(struct intel_engine_cs *engine);
> >   
> >   /* One extra page is added before LRC for GuC as shared data */
> >   #define LRC_GUCSHR_PN  (0)
> > -#define LRC_PPHWSP_PN  (LRC_GUCSHR_PN + 1)
> > +#define LRC_GUCSHR_PAGES 1
> > +#define LRC_PPHWSP_PN  (LRC_GUCSHR_PN + LRC_GUCSHR_PAGES)
> >   #define LRC_STATE_PN   (LRC_PPHWSP_PN + 1)

but

/* We allocate a header at the start of the context image for our own
 * use, therefore the actual location of the logical state is offset
 * from the start of the VMA. The layout is
 *
 * | [guc]          | [hwsp] [logical state] |
 * |<- our header ->|<- context image      ->|
 *               
 */

/* One extra page is used for sharing with the GuC */
#define LRC_GUCSHR_PN  (0)
#define LRC_GUCSHR_PAGES 1

/* At the start of the context image is its per-process HWS page */
#define LRC_PPHWSP_PN  (LRC_GUCSHR_PN + LRC_GUCSHR_PAGES)

/* Finally we have the logical state for the context */
#define LRC_STATE_PN   (LRC_PPHWSP_PN + 1)

/* Currently we include the PPHWSP in __intel_engine_context_size() so
 * the size of the header is synonymous with the start of the PPHWSP.
 */
#define LRC_HEADER_PAGES LRC_PPHWSP_PN

> >   
> >   struct drm_i915_private;
> > 
> > Nowhere do we use LRC_GUCSHR_PN. Having it just gives us the mirage of
> > flexibility...
> 
> LRC_GUCSHR_PN is not used because people assumed it is always page 0 of 
> the kernel ctx :S
> 
> One more thing to fix, use LRC_GUCSHR_PN in all the places where this 
> _shared_ GuC page is used (guc_ggtt_offset(ctx->engine[RCS].state))

That may prove very handy.
-Chris