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

[Chris Wilson <chris@chris-wilson.co.uk>]

On Thu, Jun 18, 2015 at 01:29:45PM +0100, Peter Antoine wrote:
> @@ -1379,6 +1380,13 @@ static int gen8_init_rcs_context(struct intel_engine_cs *ring,
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Failing to program the MOCS is non-fatal.The system will not
> +	 * run at peak performance. So generate a warning and carry on.
> +	 */
> +	if (intel_rcs_context_init_mocs(ring, ctx) != 0)
> +		DRM_ERROR("MOCS failed to program: expect performance issues.");

You said to expect display corruption as well if this failed.
Fortunately, if this fails, we have severe driver issues...

> +/**
> + * emit_mocs_l3cc_table() - emit the mocs control table
> + * @ringbuf:	DRM device.
> + * @table:	The values to program into the control regs.
> + *
> + * This function simply emits a MI_LOAD_REGISTER_IMM command for the
> + * given table starting at the given address. This register set is  programmed
> + * in pairs.
> + *
> + * Return: Nothing.
> + */
> +static void emit_mocs_l3cc_table(struct intel_ringbuffer *ringbuf,
> +			 struct drm_i915_mocs_table *table) {
> +	unsigned int count;
> +	unsigned int i;
> +	u32 value;
> +	u32 filler = (table->table[0].l3cc_value & 0xffff) |
> +			((table->table[0].l3cc_value & 0xffff) << 16);

l3cc_value is only u16, & 0xffff is just noise, without & you don't need
the parantheses.

> +int intel_rcs_context_init_mocs(struct intel_engine_cs *ring,
> +				struct intel_context *ctx)
> +{
> +	int ret = 0;
> +
> +	struct drm_i915_mocs_table t;
> +	struct drm_device *dev = ring->dev;
> +	struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf;
> +
> +	if (get_mocs_settings(dev, &t)) {
> +		u32 table_size;
> +
> +		/*
> +		 * OK. For each supported ring:
> +		 *  number of mocs entries * 2 dwords for each control_value
> +		 *  plus number of mocs entries /2 dwords for l3cc values.
> +		 *
> +		 *  Plus 1 for the load command and 1 for the NOOP per ring
> +		 *  and the l3cc programming.
		 *
		 * With 5 rings and 63 mocs entries, this gives 715
		 * dwords.
> +		 */

> +		table_size = GEN9_NUM_MOCS_RINGS *
> +				((2 * GEN9_NUM_MOCS_ENTRIES) + 2) +
> +				GEN9_NUM_MOCS_ENTRIES + 2;

If you pushed the ring_begin into each function, not only would it be
easier to verify, you then don't need an explanation that starts with
"This looks like a mistake". Validation of ring_begin/ring_advance is by
review, so it has to be easy to review.

> +		ret = intel_logical_ring_begin(ringbuf, ctx, table_size);
> +		if (ret) {
> +			DRM_DEBUG("intel_logical_ring_begin failed %d\n", ret);
> +			return ret;
> +		}


-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx