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

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

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.

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.

> 
> 3 - init_mocs() vs mocs_init() because it is the context_init calling
>     the mocs setup.
> 
> > +		DRM_ERROR("MOCS failed to program: expect performance issues.");
> 
> I guess asking for a new DRM_NOTICE() is too much :)
I searched the code for NOTICE and DRM_NOTICE and could not find it. So changed
it from an WARN_ON to and ERROR.
> 
> > +/*
> > + * MOCS tables
> > + *
> > + * These are the MOCS tables that are programmed across all the rings.
> > + * The control value is programmed to all the rings that support the
> > + * MOCS registers. While the l3cc_values are only programmed to the
> > + * LNCFCMOCS0 - LNCFCMOCS32 registers.
> > + *
> > + * NOTE: These tables MUST start with being uncached and the length MUST be
> > + *       less than 63 as the last two registers are reserved by the hardware.
> > + */
> > +static struct drm_i915_mocs_entry skylake_mocs_table[] = {
> 
> static const struct... and propagate that const.
Yup, with make it static const.
> 
> > +/**
> > + * get_mocs_settings
> > + *
> > + * This function will return the values of the MOCS table that needs to
> > + * be programmed for the platform. It will return the values that need
> > + * to be programmed and if they need to be programmed.
> > + *
> > + * If the return values is false then the registers do not need programming.
> > + */
> > +static bool get_mocs_settings(struct drm_device *dev,
> > +			      struct drm_i915_mocs_table *table) {
> > +	bool	result = false;
> 
> A tab?
Ok.
> 
> > +
> > +	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.
> > +	}
> > +
> > +	return result;
> -Chris
> 


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