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

[Antoine, Peter]

On Thu, 2015-06-18 at 08:49 +0100, chris at chris-wilson.co.uk wrote:
> 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.

I don't get your point, the original seemed good to me.
Changing name to what you want as this needs to get in.

> 
> > > > +	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.
How does that make any difference?
We still need the if's to make it work on different platforms. So the
"result =" code still stays the same.

> -Chris
>