Re: [PATCH v4] drm/i915 : Added Programming of the MOCS

["Antoine, Peter" <peter.antoine@intel.com>]

On Thu, 2015-06-18 at 10:10 +0100, chris@chris-wilson.co.uk wrote:
> On Thu, Jun 18, 2015 at 08:45:10AM +0000, Antoine, Peter wrote:
> > On Thu, 2015-06-18 at 08:49 +0100, chris@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.
> 
> My point is that it is not a generic function and must be called at a
> certain phase of context construction and lrc initialisation. I am
> trying to suggest a name that encapsulates that to avoid possible
> misuse.
> 
> > > > > > +	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?
> 
> In terms of code construction, simpler and able to the mark all the data
> structures as const.
Going to leave the code as is. It's clean and readable. It's a simple
internal demux function and all the structures that are referenced are
const
-Peter.
> -Chris
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx