[Intel-gfx] [PATCH 51/53] drm/i915/bdw: Document Logical Rings, LR contexts and Execlists

[Mateo Lozano, Oscar]

---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47


> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Tuesday, June 17, 2014 10:40 AM
> To: Mateo Lozano, Oscar
> Cc: Daniel Vetter; Chris Wilson; intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 51/53] drm/i915/bdw: Document Logical
> Rings, LR contexts and Execlists
> 
> On Tue, Jun 17, 2014 at 08:22:55AM +0000, Mateo Lozano, Oscar wrote:
> >
> > > -----Original Message-----
> > > From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of
> > > Daniel Vetter
> > > Sent: Monday, June 16, 2014 6:56 PM
> > > To: Mateo Lozano, Oscar
> > > Cc: Chris Wilson; intel-gfx at lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH 51/53] drm/i915/bdw: Document
> > > Logical Rings, LR contexts and Execlists
> > >
> > > On Mon, Jun 16, 2014 at 03:24:26PM +0000, Mateo Lozano, Oscar
> wrote:
> > > > > -----Original Message-----
> > > > > From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> > > > > Sent: Friday, June 13, 2014 5:51 PM
> > > > > To: Mateo Lozano, Oscar
> > > > > Cc: intel-gfx at lists.freedesktop.org
> > > > > Subject: Re: [Intel-gfx] [PATCH 51/53] drm/i915/bdw: Document
> > > > > Logical Rings, LR contexts and Execlists
> > > > >
> > > > > On Fri, Jun 13, 2014 at 04:38:09PM +0100, oscar.mateo at intel.com
> > > wrote:
> > > > > > +/**
> > > > > > + * intel_execlists_ctx_id() - get the Execlists Context ID
> > > > > > + * @ctx_obj: Logical Ring Context backing object.
> > > > > > + *
> > > > > > + * Do not confuse with ctx->id! Unfortunately we have a name
> > > > > > +overload
> > > > > > + * here: the old context ID we pass to userspace as a handler
> > > > > > +so that
> > > > > > + * they can refer to a context, and the new context ID we
> > > > > > +pass to the
> > > > > > + * ELSP so that the GPU can inform us of the context status
> > > > > > +via
> > > > > > + * interrupts.
> > > > > > + *
> > > > > > + * Return: 20-bits globally unique context ID.
> > > > > > + */
> > > > >
> > > > > Use tag for the ctx id we pass around in hw?
> > > > > -Chris
> > > >
> > > > I also tried other names, like "submission id", but it confuses
> > > > people when they search for in the BSpec. Maybe changing ctx->id
> > > > to ctx->tag, and leaving id for the hardware?
> > >
> > > I think Chris' idea was to reuse the id from the idr for the hw tag.
> > > But I guess that fails because our idr is global.
> > >
> > > Or I'm totally confused.
> > >
> > > I'd vote for hw_ctx_id or something.
> > > -Daniel
> >
> > In the first version of the series I tried to reuse the id from the
> > idr, but that was a bad idea because the id we pass to the hw has to
> > be globally unique, while our idr is per file_priv. What I did is
> > adding an id field to the file_priv and then generating the hw ctx id
> > by using some bits from ctx->id, some from file_priv->id and finally
> > some from
> > ring->id (since we multiplex several hw contexts inside our struct
> > intel_context). But the ELSP context descriptor only allows 20 bits
> > for the id, so I had to limit the maximum number of contexts, files or
> > rings artificially (ugly).
> 
> Considerations like this should be somewhere in the commit message.
> Especially when it's all stuff you've discovered before review started and
> hence doesn't have a public record anywhere.

Nope, I discovered this *after* review started: v1 used the Frankenstein-style context id, v2 didn´t. 

The comment appears in the commit message for " drm/i915/bdw: Implement context switching (somewhat) ":

    v3: Use LRCA[31:12] as hwCtxId[19:0]. This guarantees that the HW context
    ID we submit to the ELSP is globally unique and != 0 (Bspec requirements
    of the software use-only bits of the Context ID in the Context Descriptor
    Format) without the hassle of the previous submission Id construction.

> > Another proposal: s/ctx->id/ctx->handle. After all, our ctx->id software
> construct is just a userspace handle...
> 
> Not sure either is clearer really. As long as there's a clear disdinction
> between the hw id and the userspace handle I'm ok, maybe augmented with
> some comments to explain the struct fields in the header.
> -Daniel

I´ll go with s/ctx->id/ctx->handle and the comments then (IMHO, it´s clear enough).