[Mesa-dev] [Intel-gfx] [PATCH 2/3] iris: Create a composite context for both compute and render pipelines

[Jordan Justen]

On 2019-03-31 03:02:21, Chris Wilson wrote:
> Quoting Jordan Justen (2019-03-31 10:57:09)
> > On 2019-03-25 03:58:59, Chris Wilson wrote:
> > > iris currently uses two distinct GEM contexts to have distinct logical
> > > HW contexts for the compute and render pipelines. However, using two
> > > distinct GEM contexts implies that they are distinct timelines, yet as
> > > they are a single GL context that implies they belong to a single
> > > timeline from the client perspective. Currently, fences are occasionally
> > > inserted to order the two timelines. Using 2 GEM contexts, also implies
> > > that we keep 2 ppGTT for identical buffer state. If we can create a
> > > single GEM context, with the right capabilities, we can have a single
> > > VM, a single timeline, but 2 logical HW contexts for the 2 pipelines.
> > > 
> > > This is allowed through the new context interface that allows VM to be
> > > shared, timelines to be specified, and for the logical contexts to be
> > > constructed as the user desires.
> > > 
> > > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > > Cc: Kenneth Graunke <kenneth at whitecape.org>
> > > ---
> > >  src/gallium/drivers/iris/iris_batch.c   | 16 ++-----
> > >  src/gallium/drivers/iris/iris_batch.h   |  5 +--
> > >  src/gallium/drivers/iris/iris_context.c | 56 ++++++++++++++++++++++++-
> > >  3 files changed, 60 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/src/gallium/drivers/iris/iris_batch.c b/src/gallium/drivers/iris/iris_batch.c
> > > index 556422c38bc..40bcd939795 100644
> > > --- a/src/gallium/drivers/iris/iris_batch.c
> > > +++ b/src/gallium/drivers/iris/iris_batch.c
> > > @@ -164,24 +164,16 @@ iris_init_batch(struct iris_batch *batch,
> > >                  struct iris_vtable *vtbl,
> > >                  struct pipe_debug_callback *dbg,
> > >                  struct iris_batch *all_batches,
> > > -                enum iris_batch_name name,
> > > -                uint8_t engine,
> > > -                int priority)
> > > +                uint32_t hw_id,
> > > +                enum iris_batch_name name)
> > >  {
> > >     batch->screen = screen;
> > >     batch->vtbl = vtbl;
> > >     batch->dbg = dbg;
> > >     batch->name = name;
> > >  
> > > -   /* engine should be one of I915_EXEC_RENDER, I915_EXEC_BLT, etc. */
> > > -   assert((engine & ~I915_EXEC_RING_MASK) == 0);
> > > -   assert(util_bitcount(engine) == 1);
> > > -   batch->engine = engine;
> > > -
> > > -   batch->hw_ctx_id = iris_create_hw_context(screen->bufmgr);
> > > -   assert(batch->hw_ctx_id);
> > > -
> > > -   iris_hw_context_set_priority(screen->bufmgr, batch->hw_ctx_id, priority);
> > > +   batch->hw_ctx_id = hw_id;
> > > +   batch->engine = name;
> > 
> > I think this should fall-back to I915_EXEC_RENDER if the old style
> > contexts are created.
> 
> It uses the legacy uABI to map both name=COMPUTE to the render ring
> and name=RENDER to the render ring.

Oh, what legacy uABI sets that? I thought if engines weren't set then
I915_EXEC_RENDER would be needed for the two contexts to execute on
the rcs. But, after this patch, isn't name 0 and 1?

I guess I915_EXEC_DEFAULT is 0 and I915_EXEC_RENDER is 1. So, will
I915_EXEC_DEFAULT execute on rcs too? If so, I'm not sure I agree with
taking advantage of this coincidence without even a comment.

-Jordan

> Should we have more or either, or
> start using the many video engines, ctx->engines[] will be the only way
> to define the execbuf mapping. So name == engine, as used today, can
> remain invariant across the legacy I915_EXEC_RING API and future
> ctx->engines[] for the remaining forseeable future of GEM_EXECBUFFER2.