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

[Chris Wilson]

On Tue, Jul 07, 2015 at 10:13:01PM +0300, Francisco Jerez wrote:
> From: Peter Antoine <peter.antoine at intel.com>
> 
> 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.

If they were optimal why weren't they defaults? I'm not feeling very
satisfied by this explanation.

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

This looks easy enough to extend to get_mocs_settings(ctx, &table) and
use CONTEXT_SET_PARAM to load a user defined mocs table. These defaults
have the smell of policy in the era where even CPU PAT are becoming user
defined (with per-process definitions). Is it worth shifting these to
userspace? Having sane/safe defaults is good definitely.

The changelog should be more explicit on what you mean by optimal and
why other avenues are not pursued.

> +/**
> + * emit_mocs_control_table() - emit the mocs control table
> + * @req:	Request to set up the MOCS table for.
> + * @table:	The values to program into the control regs.
> + * @reg_base:	The base for the engine that needs to be programmed.
> + *
> + * This function simply emits a MI_LOAD_REGISTER_IMM command for the
> + * given table starting at the given address.
> + *
> + * @return 0 on success, otherwise the error status.
> + */
> +static int emit_mocs_control_table(struct drm_i915_gem_request *req,
> +				   const struct drm_i915_mocs_table *table,
> +				   u32 reg_base)
> +{
> +	struct intel_ringbuffer *ringbuf = req->ringbuf;
> +	unsigned int index;
> +	int ret;
> +
> +	ret = intel_logical_ring_begin(req, 2 + 2 * GEN9_NUM_MOCS_ENTRIES);
> +	if (ret) {
> +		DRM_DEBUG("intel_logical_ring_begin failed %d\n", ret);
> +		return ret;
> +	}

Much happier now. I don't have to jump back and forth to check you have
reserved the correct amount of space.

One last thing to do would be

if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
	return -ENODEV;

It's nice here as we have the two loops and this sanity check helps
explain the relationship between those loops and the ring emission. But
equally you may feel that doing that check in get_mocs_table() (or just
after) is sufficient. It needs to be done somewhere though.

(If you go with adding the sanity check here, it should also be done in 
emit_mocs_l3cc_table.)

Considering that's my only real critique, pretty good!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre