[Intel-gfx] [PATCH v4] drm/i915 : Added Programming of the MOCS

[chris at chris-wilson.co.uk]

On Thu, Jun 18, 2015 at 07:36:41AM +0000, Antoine, Peter wrote:
> 
> On Wed, 2015-06-17 at 17:33 +0100, Chris Wilson wrote:
> > On Wed, Jun 17, 2015 at 04:19:22PM +0100, Peter Antoine wrote:
> > > This change adds the programming of the MOCS registers to the gen 9+
> > > platforms. This change set programs the MOCS register values to a set
> > > of values that are defined to be optimal.
> > > 
> > > It creates a fixed register set that is programmed across the different
> > > engines so that all engines have the same table. This is done as the
> > > main RCS context only holds the registers for itself and the shared
> > > L3 values. By trying to keep the registers consistent across the
> > > different engines it should make the programming for the registers
> > > consistent.
> > > 
> > > v2:
> > > -'static const' for private data structures and style changes.(Matt Turner)
> > > v3:
> > > - Make the tables "slightly" more readable. (Damien Lespiau)
> > > - Updated tables fix performance regression.
> > > v4:
> > > - Code formatting. (Chris Wilson)
> > > - re-privatised mocs code. (Daniel Vetter)
> > 
> > Being really picky now, but reading your comments impressed upon me
> > the importance of reinforcing one particular point...
> > 
> > >  
> > > +	/*
> > > +	 * Failing to program the MOCS is non-fatal.The system will not
> > > +	 * run at peak performance. So generate a warning and carry on.
> > > +	 */
> > > +	if (gen9_program_mocs(ring, ctx) != 0)
> > 
> > I think this is better as intel_rcs_context_init_mocs(). Too me it is
> > important that you emphasize this is to be run once during very early
> > initialisation to setup the first context prior to anything else. i.e.
> > All subsequent execution state must be derived from this. Renaming it as
> > intel_rcs_context_init_mocs():
> > 
> > 1 - indicates you have written it to handle all generation, this is
> >     important as you are otherwise passing in gen8 into a gen9 function.
> > 
> > 2 - it is only called during RCS->init_context() and must not be called
> >     at any other time - this avoids the issue of modifying registers
> >     used by other rings at runtime, which is the trap you lead me into
> >     last time.
> No problem with that.But adding rcs to the original name suggests that it
> is only setting up the rcs engine and not all the engines. If any of the
> other context engines have there context extended then we may need to call
> the function from other ring initialise functions.

"intel_rcs_context" is the object
"init_mocs" is the verb, with "init" being a fairly well defined phase
of context operatinons.

My suggestion is that is only run during RCS context init. The comments
tell us that it affects all rings - and so we must emphasize that the
RCS context init *must* be run before the other rings are enabled for
submission.

If we have contexts being initialised on other rings, then one would not
think of calling intel_rcs_context_init* but instead think of how we
would need to interact with concurrent engine initialisation. Being
specifc here should stop someone simply calling the function and hoping
for the best.

> I'll change it to intel_context_emit_mocs() as this does say what it does
> on the tin, it only emits the mocs to the context and does not program them.

That misses the point I am trying to make.

> > > +	if (IS_SKYLAKE(dev)) {
> > > +		table->size  = ARRAY_SIZE(skylake_mocs_table);
> > > +		table->table = skylake_mocs_table;
> > > +		result = true;
> > > +	} else if (IS_BROXTON(dev)) {
> > > +		table->size  = ARRAY_SIZE(broxton_mocs_table);
> > > +		table->table = broxton_mocs_table;
> > > +		result = true;
> > > +	} else {
> > > +		/* Platform that should have a MOCS table does not */
> > > +		WARN_ON(INTEL_INFO(dev)->gen >= 9);
> > 
> > result = false; here would be fewer lines of code today and tomorrow. :)
> Fail safe return value. Makes not difference here, but golden in larger
> functions.

Actually I don't see why you can't encode the ARRAY_SIZE into the static
const tables, then the return value is just the appropriate table. If
you don't set a default value, then you get a compiler warning telling
you missed adding it your new code.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre