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

[Chris Wilson]

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.

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 :)

> +/*
> + * 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.

> +/**
> + * 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?

> +
> +	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. :)
> +	}
> +
> +	return result;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre