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

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

[Francisco Jerez]

From: Peter Antoine <peter.antoine@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.

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)
v5:
- Changed the name of a function. (Chris Wilson)
v6:
- re-based
- Added Mesa table entry (skylake & broxton) (Francisco Jerez)
- Tidied up the readability defines (Francisco Jerez)
- NUMBER of entries defines wrong. (Jim Bish)
- Added comments to clear up the meaning of the tables (Jim Bish)

Signed-off-by: Peter Antoine <peter.antoine@intel.com>

v7 (Francisco Jerez):
- Don't write L3-specific MOCS_ESC/SCC values into the e/LLC control
  tables.  Prefix L3-specific defines consistently with L3_ and
  e/LLC-specific defines with LE_ to avoid this kind of confusion in
  the future.
- Change L3CC WT define back to RESERVED (matches my hardware
  documentation and the original patch, probably a misunderstanding
  of my own previous comment).
- Drop Android tables, define new minimal tables more suitable for the
  open source stack.
- Add comment that the MOCS tables are part of the kernel ABI.
- Move intel_logical_ring_begin() and _advance() calls one level down
  (Chris Wilson).
- Minor formatting and style fixes.

Signed-off-by: Francisco Jerez <currojerez@riseup.net>
---
 drivers/gpu/drm/i915/Makefile     |   1 +
 drivers/gpu/drm/i915/i915_reg.h   |   9 ++
 drivers/gpu/drm/i915/intel_lrc.c  |  11 +-
 drivers/gpu/drm/i915/intel_lrc.h  |   1 +
 drivers/gpu/drm/i915/intel_mocs.c | 324 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_mocs.h |  57 +++++++
 6 files changed, 401 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_mocs.c
 create mode 100644 drivers/gpu/drm/i915/intel_mocs.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index de21965..e52e012 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -36,6 +36,7 @@ i915-y += i915_cmd_parser.o \
 	  i915_trace_points.o \
 	  intel_hotplug.o \
 	  intel_lrc.o \
+	  intel_mocs.o \
 	  intel_ringbuffer.o \
 	  intel_uncore.o
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2a29bcc..9b17260 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7906,4 +7906,13 @@ enum skl_disp_power_wells {
 #define _PALETTE_A (dev_priv->info.display_mmio_offset + 0xa000)
 #define _PALETTE_B (dev_priv->info.display_mmio_offset + 0xa800)
 
+/* MOCS (Memory Object Control State) registers */
+#define GEN9_LNCFCMOCS0		(0xB020)	/* L3 Cache Control base */
+
+#define GEN9_GFX_MOCS_0		(0xc800)	/* Graphics MOCS base register*/
+#define GEN9_MFX0_MOCS_0	(0xc900)	/* Media 0 MOCS base register*/
+#define GEN9_MFX1_MOCS_0	(0xcA00)	/* Media 1 MOCS base register*/
+#define GEN9_VEBOX_MOCS_0	(0xcB00)	/* Video MOCS base register*/
+#define GEN9_BLT_MOCS_0		(0xcc00)	/* Blitter MOCS base register*/
+
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d4f8b43..466d17c 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -135,6 +135,7 @@
 #include <drm/drmP.h>
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
+#include "intel_mocs.h"
 
 #define GEN9_LR_CONTEXT_RENDER_SIZE (22 * PAGE_SIZE)
 #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
@@ -772,8 +773,7 @@ static int logical_ring_prepare(struct drm_i915_gem_request *req, int bytes)
  *
  * Return: non-zero if the ringbuffer is not ready to be written to.
  */
-static int intel_logical_ring_begin(struct drm_i915_gem_request *req,
-				    int num_dwords)
+int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords)
 {
 	struct drm_i915_private *dev_priv;
 	int ret;
@@ -1675,6 +1675,13 @@ static int gen8_init_rcs_context(struct drm_i915_gem_request *req)
 	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(req) != 0)
+		DRM_ERROR("MOCS failed to program: expect performance issues.");
+
 	return intel_lr_context_render_state_init(req);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index e0299fb..64f89f99 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -42,6 +42,7 @@ int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request);
 void intel_logical_ring_stop(struct intel_engine_cs *ring);
 void intel_logical_ring_cleanup(struct intel_engine_cs *ring);
 int intel_logical_rings_init(struct drm_device *dev);
+int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords);
 
 int logical_ring_flush_all_caches(struct drm_i915_gem_request *req);
 /**
diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
new file mode 100644
index 0000000..f7b93e9
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_mocs.c
@@ -0,0 +1,324 @@
+/*
+ * Copyright (c) 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions: *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include "intel_mocs.h"
+#include "intel_lrc.h"
+#include "intel_ringbuffer.h"
+
+/* structures required */
+struct drm_i915_mocs_entry {
+	u32	control_value;
+	u16	l3cc_value;
+};
+
+struct drm_i915_mocs_table {
+	u32					size;
+	const struct drm_i915_mocs_entry	*table;
+};
+
+/* Defines for the tables (XXX_MOCS_0 - XXX_MOCS_63) */
+#define LE_CACHEABILITY(value)	(value << 0)
+#define LE_TGT_CACHE(value)	(value << 2)
+#define LE_LRUM(value)		(value << 4)
+#define LE_AOM(value)		(value << 6)
+#define LE_RSC(value)		(value << 7)
+#define LE_SCC(value)		(value << 8)
+#define LE_PFM(value)		(value << 11)
+#define LE_SCF(value)		(value << 14)
+
+/* Defines for the tables (LNCFMOCS0 - LNCFMOCS31) - two entries per word */
+#define L3_ESC(value)		(value << 0)
+#define L3_SCC(value)		(value << 1)
+#define L3_CACHEABILITY(value)	(value << 4)
+
+/* Helper defines */
+#define GEN9_NUM_MOCS_ENTRIES	(62)  /* 62 out of 64 - 63 & 64 are reserved. */
+
+/* (e)LLC caching options */
+#define LE_PAGETABLE		(0)
+#define LE_UC			(1)
+#define LE_WT			(2)
+#define LE_WB			(3)
+
+/* L3 caching options */
+#define L3_DIRECT		(0)
+#define L3_UC			(1)
+#define L3_RESERVED		(2)
+#define L3_WB			(3)
+
+/* Target cache */
+#define ELLC			(0)
+#define LLC			(1)
+#define LLC_ELLC		(2)
+
+/*
+ * 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.
+ *
+ * These tables are intended to be kept reasonably consistent across
+ * platforms. However some of the fields are not applicable to all of
+ * them.
+ *
+ * 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.  These tables are part of the kernel ABI and
+ *       may only be updated incrementally by adding entries at the
+ *       end.
+ */
+static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
+	/* { 0x00000009, 0x0010 } */
+	{ (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
+	/* { 0x00000038, 0x0030 } */
+	{ (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
+	/* { 0x0000003b, 0x0030 } */
+	{ (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
+};
+
+/* NOTE: the LE_TGT_CACHE is not used on Broxton */
+static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
+	/* { 0x00000009, 0x0010 } */
+	{ (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
+	/* { 0x00000038, 0x0030 } */
+	{ (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
+	/* { 0x0000003b, 0x0030 } */
+	{ (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
+};
+
+/**
+ * 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;
+
+	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);
+	}
+
+	return result;
+}
+
+/**
+ * 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;
+	}
+
+	intel_logical_ring_emit(ringbuf,
+				MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES));
+
+	for (index = 0; index < table->size; index++) {
+		intel_logical_ring_emit(ringbuf, reg_base + index * 4);
+		intel_logical_ring_emit(ringbuf,
+					table->table[index].control_value);
+	}
+
+	/*
+	 * Ok, now set the unused entries to uncached. These entries
+	 * are officially undefined and no contract for the contents
+	 * and settings is given for these entries.
+	 *
+	 * Entry 0 in the table is uncached - so we are just writing
+	 * that value to all the used entries.
+	 */
+	for (; index < GEN9_NUM_MOCS_ENTRIES; index++) {
+		intel_logical_ring_emit(ringbuf, reg_base + index * 4);
+		intel_logical_ring_emit(ringbuf, table->table[0].control_value);
+	}
+
+	intel_logical_ring_emit(ringbuf, MI_NOOP);
+	intel_logical_ring_advance(ringbuf);
+
+	return 0;
+}
+
+/**
+ * emit_mocs_l3cc_table() - emit the mocs control table
+ * @req:	Request to set up the MOCS table for.
+ * @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 0 on success, otherwise the error status.
+ */
+static int emit_mocs_l3cc_table(struct drm_i915_gem_request *req,
+				const struct drm_i915_mocs_table *table)
+{
+	struct intel_ringbuffer *ringbuf = req->ringbuf;
+	unsigned int count;
+	unsigned int i;
+	u32 value;
+	u32 filler = (table->table[0].l3cc_value & 0xffff) |
+			((table->table[0].l3cc_value & 0xffff) << 16);
+	int ret;
+
+	ret = intel_logical_ring_begin(req, 2 + GEN9_NUM_MOCS_ENTRIES);
+	if (ret) {
+		DRM_DEBUG("intel_logical_ring_begin failed %d\n", ret);
+		return ret;
+	}
+
+	intel_logical_ring_emit(ringbuf,
+			MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES / 2));
+
+	for (i = 0, count = 0; i < table->size / 2; i++, count += 2) {
+		value = (table->table[count].l3cc_value & 0xffff) |
+			((table->table[count + 1].l3cc_value & 0xffff) << 16);
+
+		intel_logical_ring_emit(ringbuf, GEN9_LNCFCMOCS0 + i * 4);
+		intel_logical_ring_emit(ringbuf, value);
+	}
+
+	if (table->size & 0x01) {
+		/* Odd table size - 1 left over */
+		value = (table->table[count].l3cc_value & 0xffff) |
+			((table->table[0].l3cc_value & 0xffff) << 16);
+	} else
+		value = filler;
+
+	/*
+	 * Now set the rest of the table to uncached - use entry 0 as
+	 * this will be uncached. Leave the last pair uninitialised as
+	 * they are reserved by the hardware.
+	 */
+	for (; i < GEN9_NUM_MOCS_ENTRIES / 2; i++) {
+		intel_logical_ring_emit(ringbuf, GEN9_LNCFCMOCS0 + i * 4);
+		intel_logical_ring_emit(ringbuf, value);
+
+		value = filler;
+	}
+
+	intel_logical_ring_emit(ringbuf, MI_NOOP);
+	intel_logical_ring_advance(ringbuf);
+
+	return 0;
+}
+
+/**
+ * intel_rcs_context_init_mocs() - program the MOCS register.
+ *
+ * @req:	Request to set up the MOCS tables for.
+ *
+ * This function will emit a batch buffer with the values required for
+ * programming the MOCS register values for all the currently supported
+ * rings.
+ *
+ * These registers are partially stored in the RCS context, so they are
+ * emitted at the same time so that when a context is created these registers
+ * are set up. These registers have to be emitted into the start of the
+ * context as setting the ELSP will re-init some of these registers back
+ * to the hw values.
+ *
+ * @return 0 on success, otherwise the error status.
+ */
+int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req)
+{
+	struct drm_i915_mocs_table t;
+	int ret;
+
+	if (get_mocs_settings(req->ring->dev, &t)) {
+		/* Program the control registers */
+		ret = emit_mocs_control_table(req, &t, GEN9_GFX_MOCS_0);
+		if (ret)
+			return ret;
+
+		ret = emit_mocs_control_table(req, &t, GEN9_MFX0_MOCS_0);
+		if (ret)
+			return ret;
+
+		ret = emit_mocs_control_table(req, &t, GEN9_MFX1_MOCS_0);
+		if (ret)
+			return ret;
+
+		ret = emit_mocs_control_table(req, &t, GEN9_VEBOX_MOCS_0);
+		if (ret)
+			return ret;
+
+		ret = emit_mocs_control_table(req, &t, GEN9_BLT_MOCS_0);
+		if (ret)
+			return ret;
+
+		/* Now program the l3cc registers */
+		ret = emit_mocs_l3cc_table(req, &t);
+		if (ret)
+			return ret;
+
+		DRM_DEBUG("MOCS: Table set in context.\n");
+	} else {
+		DRM_DEBUG("MOCS: Table not supported on platform.\n");
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_mocs.h b/drivers/gpu/drm/i915/intel_mocs.h
new file mode 100644
index 0000000..76e45b1
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_mocs.h
@@ -0,0 +1,57 @@
+/*
+ * Copyright (c) 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef INTEL_MOCS_H
+#define INTEL_MOCS_H
+
+/**
+ * DOC: Memory Objects Control State (MOCS)
+ *
+ * Motivation:
+ * In previous Gens the MOCS settings was a value that was set by user land as
+ * part of the batch. In Gen9 this has changed to be a single table (per ring)
+ * that all batches now reference by index instead of programming the MOCS
+ * directly.
+ *
+ * The one wrinkle in this is that only PART of the MOCS tables are included
+ * in context (The GFX_MOCS_0 - GFX_MOCS_64 and the LNCFCMOCS0 - LNCFCMOCS32
+ * registers). The rest are not (the settings for the other rings).
+ *
+ * This table needs to be set at system start-up because the way the table
+ * interacts with the contexts and the GmmLib interface.
+ *
+ *
+ * Implementation:
+ *
+ * The tables (one per supported platform) are defined in intel_mocs.c
+ * and are programmed in the first batch after the context is loaded
+ * (with the hardware workarounds). This will then let the usual
+ * context handling keep the MOCS in step.
+ */
+
+#include <drm/drmP.h>
+#include "i915_drv.h"
+
+int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req);
+
+#endif
-- 
2.4.3

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

Re: [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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Francisco Jerez]

[-- Attachment #1.1.1: Type: text/plain, Size: 4405 bytes --]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Tue, Jul 07, 2015 at 10:13:01PM +0300, Francisco Jerez wrote:
>> From: Peter Antoine <peter.antoine@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.
>
Hah, yeah, they are definitely not optimal, it's just that I didn't feel
fully confident modifying the original message above Peter's Signed-off
line.  How about we replace that sentence with: "The set of MOCS
configuration entries introduced by this patch is intended to be minimal
but sufficient to cover the needs of current userspace, it's expected to
be extended in the future based on the userspace demand for additional
caching configurations."

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

Yeah, I also have the feeling that we may want to switch to some dynamic
set-up scheme in the future.  With only three of the available 62
entries in use right now it's likely to take a *long* time until we run
out of entries from the global tables though, when (if) that happens we
can always switch to dynamic set-up via some new IOCTL without
disrupting older userspace relying on the fixed MOCS defaults.

Implementing dynamic set-up is less straightforward than it may seem
though, because only the MOCS tables for some of the engines are context
saved and restored automatically by the hardware, so the settings
specified by one context would partially leak into other applications,
unless we context-switch the remaining engines manually from the kernel.

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

OK, I'll fix that and resend.

Thanks!

>
> Considering that's my only real critique, pretty good!
> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

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

[Chris Wilson]

On Wed, Jul 08, 2015 at 03:50:06PM +0300, Francisco Jerez wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > On Tue, Jul 07, 2015 at 10:13:01PM +0300, Francisco Jerez wrote:
> >> From: Peter Antoine <peter.antoine@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.
> >
> Hah, yeah, they are definitely not optimal, it's just that I didn't feel
> fully confident modifying the original message above Peter's Signed-off
> line.  How about we replace that sentence with: "The set of MOCS
> configuration entries introduced by this patch is intended to be minimal
> but sufficient to cover the needs of current userspace, it's expected to
> be extended in the future based on the userspace demand for additional
> caching configurations."

That's much clearer. However, one now wonders how it will change in the
future and whether we have sufficient future proofing. Do you mean that
there is a safe subset of the current tables with higher entries
reserved for future use (and perhaps a future param to tell userspace
when they can use them), or perhaps in the future we will allow
userspace to replace select MOCS tables.

Bikeshedding:

The set of MOCS configuration entries introduced by this patch is intended
to be minimal but sufficient to cover the needs of current userspace - i.e.
a good set of defaults. It is expected to be extended in the future to
provide further default values or to allow userspace to redefine its private
MOCS tables based on its demand for additional caching configurations.
In this setup, userspace should only utilize the first N entries, higher
entries are reserved for future use.

> >> +/**
> >> + * 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.
> >
> 
> Yeah, I also have the feeling that we may want to switch to some dynamic
> set-up scheme in the future.  With only three of the available 62
> entries in use right now it's likely to take a *long* time until we run
> out of entries from the global tables though, when (if) that happens we
> can always switch to dynamic set-up via some new IOCTL without
> disrupting older userspace relying on the fixed MOCS defaults.
> 
> Implementing dynamic set-up is less straightforward than it may seem
> though, because only the MOCS tables for some of the engines are context
> saved and restored automatically by the hardware, so the settings
> specified by one context would partially leak into other applications,
> unless we context-switch the remaining engines manually from the kernel.

Sounds like we need a set of common unalterable mocs tables for the
shared configs, and limit userspace to doing interesting things on
select engines. :(
-Chris

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

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

[Francisco Jerez]

[-- Attachment #1.1.1: Type: text/plain, Size: 4452 bytes --]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Wed, Jul 08, 2015 at 03:50:06PM +0300, Francisco Jerez wrote:
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > On Tue, Jul 07, 2015 at 10:13:01PM +0300, Francisco Jerez wrote:
>> >> From: Peter Antoine <peter.antoine@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.
>> >
>> Hah, yeah, they are definitely not optimal, it's just that I didn't feel
>> fully confident modifying the original message above Peter's Signed-off
>> line.  How about we replace that sentence with: "The set of MOCS
>> configuration entries introduced by this patch is intended to be minimal
>> but sufficient to cover the needs of current userspace, it's expected to
>> be extended in the future based on the userspace demand for additional
>> caching configurations."
>
> That's much clearer. However, one now wonders how it will change in the
> future and whether we have sufficient future proofing. Do you mean that
> there is a safe subset of the current tables with higher entries
> reserved for future use (and perhaps a future param to tell userspace
> when they can use them), or perhaps in the future we will allow
> userspace to replace select MOCS tables.
>
> Bikeshedding:
>
> The set of MOCS configuration entries introduced by this patch is intended
> to be minimal but sufficient to cover the needs of current userspace - i.e.
> a good set of defaults. It is expected to be extended in the future to
> provide further default values or to allow userspace to redefine its private
> MOCS tables based on its demand for additional caching configurations.
> In this setup, userspace should only utilize the first N entries, higher
> entries are reserved for future use.
>
Sounds good to me, I'll use your text and also add a comment on top of
the tables saying that currently undefined entries are implicitly
initialized to uncached for forwards compatibility, so that applications
written against newer kernel versions with extended MOCS tables
(i.e. providing more relaxed caching semantics for some of the undefined
entries) will still work when run on older kernels, if not blazingly
fast.

>> >> +/**
>> >> + * 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.
>> >
>> 
>> Yeah, I also have the feeling that we may want to switch to some dynamic
>> set-up scheme in the future.  With only three of the available 62
>> entries in use right now it's likely to take a *long* time until we run
>> out of entries from the global tables though, when (if) that happens we
>> can always switch to dynamic set-up via some new IOCTL without
>> disrupting older userspace relying on the fixed MOCS defaults.
>> 
>> Implementing dynamic set-up is less straightforward than it may seem
>> though, because only the MOCS tables for some of the engines are context
>> saved and restored automatically by the hardware, so the settings
>> specified by one context would partially leak into other applications,
>> unless we context-switch the remaining engines manually from the kernel.
>
> Sounds like we need a set of common unalterable mocs tables for the
> shared configs, and limit userspace to doing interesting things on
> select engines. :(
> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCHv8] drm/i915: Added Programming of the MOCS

[Francisco Jerez]

From: Peter Antoine <peter.antoine@intel.com>

This change adds the programming of the MOCS registers to the gen 9+
platforms. The set of MOCS configuration entries introduced by this
patch is intended to be minimal but sufficient to cover the needs of
current userspace - i.e. a good set of defaults. It is expected to be
extended in the future to provide further default values or to allow
userspace to redefine its private MOCS tables based on its demand for
additional caching configurations. In this setup, userspace should
only utilize the first N entries, higher entries are reserved for
future use.

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)
v5:
- Changed the name of a function. (Chris Wilson)
v6:
- re-based
- Added Mesa table entry (skylake & broxton) (Francisco Jerez)
- Tidied up the readability defines (Francisco Jerez)
- NUMBER of entries defines wrong. (Jim Bish)
- Added comments to clear up the meaning of the tables (Jim Bish)

Signed-off-by: Peter Antoine <peter.antoine@intel.com>

v7 (Francisco Jerez):
- Don't write L3-specific MOCS_ESC/SCC values into the e/LLC control
  tables.  Prefix L3-specific defines consistently with L3_ and
  e/LLC-specific defines with LE_ to avoid this kind of confusion in
  the future.
- Change L3CC WT define back to RESERVED (matches my hardware
  documentation and the original patch, probably a misunderstanding
  of my own previous comment).
- Drop Android tables, define new minimal tables more suitable for the
  open source stack.
- Add comment that the MOCS tables are part of the kernel ABI.
- Move intel_logical_ring_begin() and _advance() calls one level down
  (Chris Wilson).
- Minor formatting and style fixes.

v8 (Francisco Jerez):
- Add table size sanity check to emit_mocs_control/l3cc_table() (Chris
  Wilson).
- Add comment about undefined entries being implicitly set to uncached
  for forwards compatibility.

Signed-off-by: Francisco Jerez <currojerez@riseup.net>
---
 drivers/gpu/drm/i915/Makefile     |   1 +
 drivers/gpu/drm/i915/i915_reg.h   |   9 +
 drivers/gpu/drm/i915/intel_lrc.c  |  11 +-
 drivers/gpu/drm/i915/intel_lrc.h  |   1 +
 drivers/gpu/drm/i915/intel_mocs.c | 337 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_mocs.h |  57 +++++++
 6 files changed, 414 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_mocs.c
 create mode 100644 drivers/gpu/drm/i915/intel_mocs.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index de21965..e52e012 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -36,6 +36,7 @@ i915-y += i915_cmd_parser.o \
 	  i915_trace_points.o \
 	  intel_hotplug.o \
 	  intel_lrc.o \
+	  intel_mocs.o \
 	  intel_ringbuffer.o \
 	  intel_uncore.o
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2a29bcc..9b17260 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7906,4 +7906,13 @@ enum skl_disp_power_wells {
 #define _PALETTE_A (dev_priv->info.display_mmio_offset + 0xa000)
 #define _PALETTE_B (dev_priv->info.display_mmio_offset + 0xa800)
 
+/* MOCS (Memory Object Control State) registers */
+#define GEN9_LNCFCMOCS0		(0xB020)	/* L3 Cache Control base */
+
+#define GEN9_GFX_MOCS_0		(0xc800)	/* Graphics MOCS base register*/
+#define GEN9_MFX0_MOCS_0	(0xc900)	/* Media 0 MOCS base register*/
+#define GEN9_MFX1_MOCS_0	(0xcA00)	/* Media 1 MOCS base register*/
+#define GEN9_VEBOX_MOCS_0	(0xcB00)	/* Video MOCS base register*/
+#define GEN9_BLT_MOCS_0		(0xcc00)	/* Blitter MOCS base register*/
+
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d4f8b43..466d17c 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -135,6 +135,7 @@
 #include <drm/drmP.h>
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
+#include "intel_mocs.h"
 
 #define GEN9_LR_CONTEXT_RENDER_SIZE (22 * PAGE_SIZE)
 #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
@@ -772,8 +773,7 @@ static int logical_ring_prepare(struct drm_i915_gem_request *req, int bytes)
  *
  * Return: non-zero if the ringbuffer is not ready to be written to.
  */
-static int intel_logical_ring_begin(struct drm_i915_gem_request *req,
-				    int num_dwords)
+int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords)
 {
 	struct drm_i915_private *dev_priv;
 	int ret;
@@ -1675,6 +1675,13 @@ static int gen8_init_rcs_context(struct drm_i915_gem_request *req)
 	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(req) != 0)
+		DRM_ERROR("MOCS failed to program: expect performance issues.");
+
 	return intel_lr_context_render_state_init(req);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index e0299fb..64f89f99 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -42,6 +42,7 @@ int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request);
 void intel_logical_ring_stop(struct intel_engine_cs *ring);
 void intel_logical_ring_cleanup(struct intel_engine_cs *ring);
 int intel_logical_rings_init(struct drm_device *dev);
+int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords);
 
 int logical_ring_flush_all_caches(struct drm_i915_gem_request *req);
 /**
diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
new file mode 100644
index 0000000..130bcbe
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_mocs.c
@@ -0,0 +1,337 @@
+/*
+ * Copyright (c) 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions: *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include "intel_mocs.h"
+#include "intel_lrc.h"
+#include "intel_ringbuffer.h"
+
+/* structures required */
+struct drm_i915_mocs_entry {
+	u32	control_value;
+	u16	l3cc_value;
+};
+
+struct drm_i915_mocs_table {
+	u32					size;
+	const struct drm_i915_mocs_entry	*table;
+};
+
+/* Defines for the tables (XXX_MOCS_0 - XXX_MOCS_63) */
+#define LE_CACHEABILITY(value)	(value << 0)
+#define LE_TGT_CACHE(value)	(value << 2)
+#define LE_LRUM(value)		(value << 4)
+#define LE_AOM(value)		(value << 6)
+#define LE_RSC(value)		(value << 7)
+#define LE_SCC(value)		(value << 8)
+#define LE_PFM(value)		(value << 11)
+#define LE_SCF(value)		(value << 14)
+
+/* Defines for the tables (LNCFMOCS0 - LNCFMOCS31) - two entries per word */
+#define L3_ESC(value)		(value << 0)
+#define L3_SCC(value)		(value << 1)
+#define L3_CACHEABILITY(value)	(value << 4)
+
+/* Helper defines */
+#define GEN9_NUM_MOCS_ENTRIES	(62)  /* 62 out of 64 - 63 & 64 are reserved. */
+
+/* (e)LLC caching options */
+#define LE_PAGETABLE		(0)
+#define LE_UC			(1)
+#define LE_WT			(2)
+#define LE_WB			(3)
+
+/* L3 caching options */
+#define L3_DIRECT		(0)
+#define L3_UC			(1)
+#define L3_RESERVED		(2)
+#define L3_WB			(3)
+
+/* Target cache */
+#define ELLC			(0)
+#define LLC			(1)
+#define LLC_ELLC		(2)
+
+/*
+ * 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.
+ *
+ * These tables are intended to be kept reasonably consistent across
+ * platforms. However some of the fields are not applicable to all of
+ * them.
+ *
+ * Entries not part of the following tables are undefined as far as
+ * userspace is concerned and shouldn't be relied upon.  For the time
+ * being they will be implicitly initialized to the strictest caching
+ * configuration (uncached) to guarantee forwards compatibility with
+ * userspace programs written against more recent kernels providing
+ * additional MOCS entries.
+ *
+ * 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.  These tables are part of the kernel ABI and
+ *       may only be updated incrementally by adding entries at the
+ *       end.
+ */
+static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
+	/* { 0x00000009, 0x0010 } */
+	{ (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
+	/* { 0x00000038, 0x0030 } */
+	{ (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
+	/* { 0x0000003b, 0x0030 } */
+	{ (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
+};
+
+/* NOTE: the LE_TGT_CACHE is not used on Broxton */
+static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
+	/* { 0x00000009, 0x0010 } */
+	{ (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
+	/* { 0x00000038, 0x0030 } */
+	{ (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
+	/* { 0x0000003b, 0x0030 } */
+	{ (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
+};
+
+/**
+ * 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;
+
+	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);
+	}
+
+	return result;
+}
+
+/**
+ * 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;
+
+	if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
+		return -ENODEV;
+
+	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;
+	}
+
+	intel_logical_ring_emit(ringbuf,
+				MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES));
+
+	for (index = 0; index < table->size; index++) {
+		intel_logical_ring_emit(ringbuf, reg_base + index * 4);
+		intel_logical_ring_emit(ringbuf,
+					table->table[index].control_value);
+	}
+
+	/*
+	 * Ok, now set the unused entries to uncached. These entries
+	 * are officially undefined and no contract for the contents
+	 * and settings is given for these entries.
+	 *
+	 * Entry 0 in the table is uncached - so we are just writing
+	 * that value to all the used entries.
+	 */
+	for (; index < GEN9_NUM_MOCS_ENTRIES; index++) {
+		intel_logical_ring_emit(ringbuf, reg_base + index * 4);
+		intel_logical_ring_emit(ringbuf, table->table[0].control_value);
+	}
+
+	intel_logical_ring_emit(ringbuf, MI_NOOP);
+	intel_logical_ring_advance(ringbuf);
+
+	return 0;
+}
+
+/**
+ * emit_mocs_l3cc_table() - emit the mocs control table
+ * @req:	Request to set up the MOCS table for.
+ * @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 0 on success, otherwise the error status.
+ */
+static int emit_mocs_l3cc_table(struct drm_i915_gem_request *req,
+				const struct drm_i915_mocs_table *table)
+{
+	struct intel_ringbuffer *ringbuf = req->ringbuf;
+	unsigned int count;
+	unsigned int i;
+	u32 value;
+	u32 filler = (table->table[0].l3cc_value & 0xffff) |
+			((table->table[0].l3cc_value & 0xffff) << 16);
+	int ret;
+
+	if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
+		return -ENODEV;
+
+	ret = intel_logical_ring_begin(req, 2 + GEN9_NUM_MOCS_ENTRIES);
+	if (ret) {
+		DRM_DEBUG("intel_logical_ring_begin failed %d\n", ret);
+		return ret;
+	}
+
+	intel_logical_ring_emit(ringbuf,
+			MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES / 2));
+
+	for (i = 0, count = 0; i < table->size / 2; i++, count += 2) {
+		value = (table->table[count].l3cc_value & 0xffff) |
+			((table->table[count + 1].l3cc_value & 0xffff) << 16);
+
+		intel_logical_ring_emit(ringbuf, GEN9_LNCFCMOCS0 + i * 4);
+		intel_logical_ring_emit(ringbuf, value);
+	}
+
+	if (table->size & 0x01) {
+		/* Odd table size - 1 left over */
+		value = (table->table[count].l3cc_value & 0xffff) |
+			((table->table[0].l3cc_value & 0xffff) << 16);
+	} else
+		value = filler;
+
+	/*
+	 * Now set the rest of the table to uncached - use entry 0 as
+	 * this will be uncached. Leave the last pair uninitialised as
+	 * they are reserved by the hardware.
+	 */
+	for (; i < GEN9_NUM_MOCS_ENTRIES / 2; i++) {
+		intel_logical_ring_emit(ringbuf, GEN9_LNCFCMOCS0 + i * 4);
+		intel_logical_ring_emit(ringbuf, value);
+
+		value = filler;
+	}
+
+	intel_logical_ring_emit(ringbuf, MI_NOOP);
+	intel_logical_ring_advance(ringbuf);
+
+	return 0;
+}
+
+/**
+ * intel_rcs_context_init_mocs() - program the MOCS register.
+ *
+ * @req:	Request to set up the MOCS tables for.
+ *
+ * This function will emit a batch buffer with the values required for
+ * programming the MOCS register values for all the currently supported
+ * rings.
+ *
+ * These registers are partially stored in the RCS context, so they are
+ * emitted at the same time so that when a context is created these registers
+ * are set up. These registers have to be emitted into the start of the
+ * context as setting the ELSP will re-init some of these registers back
+ * to the hw values.
+ *
+ * @return 0 on success, otherwise the error status.
+ */
+int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req)
+{
+	struct drm_i915_mocs_table t;
+	int ret;
+
+	if (get_mocs_settings(req->ring->dev, &t)) {
+		/* Program the control registers */
+		ret = emit_mocs_control_table(req, &t, GEN9_GFX_MOCS_0);
+		if (ret)
+			return ret;
+
+		ret = emit_mocs_control_table(req, &t, GEN9_MFX0_MOCS_0);
+		if (ret)
+			return ret;
+
+		ret = emit_mocs_control_table(req, &t, GEN9_MFX1_MOCS_0);
+		if (ret)
+			return ret;
+
+		ret = emit_mocs_control_table(req, &t, GEN9_VEBOX_MOCS_0);
+		if (ret)
+			return ret;
+
+		ret = emit_mocs_control_table(req, &t, GEN9_BLT_MOCS_0);
+		if (ret)
+			return ret;
+
+		/* Now program the l3cc registers */
+		ret = emit_mocs_l3cc_table(req, &t);
+		if (ret)
+			return ret;
+
+		DRM_DEBUG("MOCS: Table set in context.\n");
+	} else {
+		DRM_DEBUG("MOCS: Table not supported on platform.\n");
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_mocs.h b/drivers/gpu/drm/i915/intel_mocs.h
new file mode 100644
index 0000000..76e45b1
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_mocs.h
@@ -0,0 +1,57 @@
+/*
+ * Copyright (c) 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef INTEL_MOCS_H
+#define INTEL_MOCS_H
+
+/**
+ * DOC: Memory Objects Control State (MOCS)
+ *
+ * Motivation:
+ * In previous Gens the MOCS settings was a value that was set by user land as
+ * part of the batch. In Gen9 this has changed to be a single table (per ring)
+ * that all batches now reference by index instead of programming the MOCS
+ * directly.
+ *
+ * The one wrinkle in this is that only PART of the MOCS tables are included
+ * in context (The GFX_MOCS_0 - GFX_MOCS_64 and the LNCFCMOCS0 - LNCFCMOCS32
+ * registers). The rest are not (the settings for the other rings).
+ *
+ * This table needs to be set at system start-up because the way the table
+ * interacts with the contexts and the GmmLib interface.
+ *
+ *
+ * Implementation:
+ *
+ * The tables (one per supported platform) are defined in intel_mocs.c
+ * and are programmed in the first batch after the context is loaded
+ * (with the hardware workarounds). This will then let the usual
+ * context handling keep the MOCS in step.
+ */
+
+#include <drm/drmP.h>
+#include "i915_drv.h"
+
+int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req);
+
+#endif
-- 
2.4.3

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

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

[Ville Syrjälä]

On Wed, Jul 08, 2015 at 05:51:22PM +0300, Francisco Jerez wrote:

Just a few style nits, didn't look at the actual contents...

<snip>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2a29bcc..9b17260 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7906,4 +7906,13 @@ enum skl_disp_power_wells {
>  #define _PALETTE_A (dev_priv->info.display_mmio_offset + 0xa000)
>  #define _PALETTE_B (dev_priv->info.display_mmio_offset + 0xa800)
>  
> +/* MOCS (Memory Object Control State) registers */
> +#define GEN9_LNCFCMOCS0		(0xB020)	/* L3 Cache Control base */
> +
> +#define GEN9_GFX_MOCS_0		(0xc800)	/* Graphics MOCS base register*/
> +#define GEN9_MFX0_MOCS_0	(0xc900)	/* Media 0 MOCS base register*/
> +#define GEN9_MFX1_MOCS_0	(0xcA00)	/* Media 1 MOCS base register*/
> +#define GEN9_VEBOX_MOCS_0	(0xcB00)	/* Video MOCS base register*/
> +#define GEN9_BLT_MOCS_0		(0xcc00)	/* Blitter MOCS base register*/

No need for all the parens.

<snip>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index e0299fb..64f89f99 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -42,6 +42,7 @@ int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request);
>  void intel_logical_ring_stop(struct intel_engine_cs *ring);
>  void intel_logical_ring_cleanup(struct intel_engine_cs *ring);
>  int intel_logical_rings_init(struct drm_device *dev);
> +int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords);
>  
>  int logical_ring_flush_all_caches(struct drm_i915_gem_request *req);
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> new file mode 100644
> index 0000000..130bcbe
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -0,0 +1,337 @@
> +/*
> + * Copyright (c) 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions: *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#include "intel_mocs.h"
> +#include "intel_lrc.h"
> +#include "intel_ringbuffer.h"
> +
> +/* structures required */
> +struct drm_i915_mocs_entry {
> +	u32	control_value;
> +	u16	l3cc_value;
> +};
> +
> +struct drm_i915_mocs_table {
> +	u32					size;
> +	const struct drm_i915_mocs_entry	*table;
> +};
> +
> +/* Defines for the tables (XXX_MOCS_0 - XXX_MOCS_63) */
> +#define LE_CACHEABILITY(value)	(value << 0)
> +#define LE_TGT_CACHE(value)	(value << 2)
> +#define LE_LRUM(value)		(value << 4)
> +#define LE_AOM(value)		(value << 6)
> +#define LE_RSC(value)		(value << 7)
> +#define LE_SCC(value)		(value << 8)
> +#define LE_PFM(value)		(value << 11)
> +#define LE_SCF(value)		(value << 14)
> +
> +/* Defines for the tables (LNCFMOCS0 - LNCFMOCS31) - two entries per word */
> +#define L3_ESC(value)		(value << 0)
> +#define L3_SCC(value)		(value << 1)
> +#define L3_CACHEABILITY(value)	(value << 4)

These should have parens arond (value) to avoid suprises if someone
passes in something more complicated than a single number.

> +
> +/* Helper defines */
> +#define GEN9_NUM_MOCS_ENTRIES	(62)  /* 62 out of 64 - 63 & 64 are reserved. */
> +
> +/* (e)LLC caching options */
> +#define LE_PAGETABLE		(0)
> +#define LE_UC			(1)
> +#define LE_WT			(2)
> +#define LE_WB			(3)
> +
> +/* L3 caching options */
> +#define L3_DIRECT		(0)
> +#define L3_UC			(1)
> +#define L3_RESERVED		(2)
> +#define L3_WB			(3)
> +
> +/* Target cache */
> +#define ELLC			(0)
> +#define LLC			(1)
> +#define LLC_ELLC		(2)

And these parens seem useless.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCHv9] drm/i915: Added Programming of the MOCS

[Francisco Jerez]

From: Peter Antoine <peter.antoine@intel.com>

This change adds the programming of the MOCS registers to the gen 9+
platforms. The set of MOCS configuration entries introduced by this
patch is intended to be minimal but sufficient to cover the needs of
current userspace - i.e. a good set of defaults. It is expected to be
extended in the future to provide further default values or to allow
userspace to redefine its private MOCS tables based on its demand for
additional caching configurations. In this setup, userspace should
only utilize the first N entries, higher entries are reserved for
future use.

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)
v5:
- Changed the name of a function. (Chris Wilson)
v6:
- re-based
- Added Mesa table entry (skylake & broxton) (Francisco Jerez)
- Tidied up the readability defines (Francisco Jerez)
- NUMBER of entries defines wrong. (Jim Bish)
- Added comments to clear up the meaning of the tables (Jim Bish)

Signed-off-by: Peter Antoine <peter.antoine@intel.com>

v7 (Francisco Jerez):
- Don't write L3-specific MOCS_ESC/SCC values into the e/LLC control
  tables.  Prefix L3-specific defines consistently with L3_ and
  e/LLC-specific defines with LE_ to avoid this kind of confusion in
  the future.
- Change L3CC WT define back to RESERVED (matches my hardware
  documentation and the original patch, probably a misunderstanding
  of my own previous comment).
- Drop Android tables, define new minimal tables more suitable for the
  open source stack.
- Add comment that the MOCS tables are part of the kernel ABI.
- Move intel_logical_ring_begin() and _advance() calls one level down
  (Chris Wilson).
- Minor formatting and style fixes.
v8 (Francisco Jerez):
- Add table size sanity check to emit_mocs_control/l3cc_table() (Chris
  Wilson).
- Add comment about undefined entries being implicitly set to uncached
  for forwards compatibility.
v9 (Francisco Jerez):
- Minor style fixes.

Signed-off-by: Francisco Jerez <currojerez@riseup.net>
---
 drivers/gpu/drm/i915/Makefile     |   1 +
 drivers/gpu/drm/i915/i915_reg.h   |   9 +
 drivers/gpu/drm/i915/intel_lrc.c  |  12 +-
 drivers/gpu/drm/i915/intel_lrc.h  |   1 +
 drivers/gpu/drm/i915/intel_mocs.c | 335 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_mocs.h |  57 +++++++
 6 files changed, 413 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_mocs.c
 create mode 100644 drivers/gpu/drm/i915/intel_mocs.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index de21965..e52e012 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -36,6 +36,7 @@ i915-y += i915_cmd_parser.o \
 	  i915_trace_points.o \
 	  intel_hotplug.o \
 	  intel_lrc.o \
+	  intel_mocs.o \
 	  intel_ringbuffer.o \
 	  intel_uncore.o
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2a29bcc..79996be 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7906,4 +7906,13 @@ enum skl_disp_power_wells {
 #define _PALETTE_A (dev_priv->info.display_mmio_offset + 0xa000)
 #define _PALETTE_B (dev_priv->info.display_mmio_offset + 0xa800)
 
+/* MOCS (Memory Object Control State) registers */
+#define GEN9_LNCFCMOCS0		0xb020	/* L3 Cache Control base */
+
+#define GEN9_GFX_MOCS_0		0xc800	/* Graphics MOCS base register*/
+#define GEN9_MFX0_MOCS_0	0xc900	/* Media 0 MOCS base register*/
+#define GEN9_MFX1_MOCS_0	0xca00	/* Media 1 MOCS base register*/
+#define GEN9_VEBOX_MOCS_0	0xcb00	/* Video MOCS base register*/
+#define GEN9_BLT_MOCS_0		0xcc00	/* Blitter MOCS base register*/
+
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d4f8b43..276a438 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -135,6 +135,7 @@
 #include <drm/drmP.h>
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
+#include "intel_mocs.h"
 
 #define GEN9_LR_CONTEXT_RENDER_SIZE (22 * PAGE_SIZE)
 #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
@@ -772,8 +773,7 @@ static int logical_ring_prepare(struct drm_i915_gem_request *req, int bytes)
  *
  * Return: non-zero if the ringbuffer is not ready to be written to.
  */
-static int intel_logical_ring_begin(struct drm_i915_gem_request *req,
-				    int num_dwords)
+int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords)
 {
 	struct drm_i915_private *dev_priv;
 	int ret;
@@ -1675,6 +1675,14 @@ static int gen8_init_rcs_context(struct drm_i915_gem_request *req)
 	if (ret)
 		return ret;
 
+	ret = intel_rcs_context_init_mocs(req);
+	/*
+	 * Failing to program the MOCS is non-fatal.The system will not
+	 * run at peak performance. So generate an error and carry on.
+	 */
+	if (ret)
+		DRM_ERROR("MOCS failed to program: expect performance issues.\n");
+
 	return intel_lr_context_render_state_init(req);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index e0299fb..64f89f99 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -42,6 +42,7 @@ int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request);
 void intel_logical_ring_stop(struct intel_engine_cs *ring);
 void intel_logical_ring_cleanup(struct intel_engine_cs *ring);
 int intel_logical_rings_init(struct drm_device *dev);
+int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords);
 
 int logical_ring_flush_all_caches(struct drm_i915_gem_request *req);
 /**
diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
new file mode 100644
index 0000000..6d3c6c0
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_mocs.c
@@ -0,0 +1,335 @@
+/*
+ * Copyright (c) 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions: *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include "intel_mocs.h"
+#include "intel_lrc.h"
+#include "intel_ringbuffer.h"
+
+/* structures required */
+struct drm_i915_mocs_entry {
+	u32 control_value;
+	u16 l3cc_value;
+};
+
+struct drm_i915_mocs_table {
+	u32 size;
+	const struct drm_i915_mocs_entry *table;
+};
+
+/* Defines for the tables (XXX_MOCS_0 - XXX_MOCS_63) */
+#define LE_CACHEABILITY(value)	((value) << 0)
+#define LE_TGT_CACHE(value)	((value) << 2)
+#define LE_LRUM(value)		((value) << 4)
+#define LE_AOM(value)		((value) << 6)
+#define LE_RSC(value)		((value) << 7)
+#define LE_SCC(value)		((value) << 8)
+#define LE_PFM(value)		((value) << 11)
+#define LE_SCF(value)		((value) << 14)
+
+/* Defines for the tables (LNCFMOCS0 - LNCFMOCS31) - two entries per word */
+#define L3_ESC(value)		((value) << 0)
+#define L3_SCC(value)		((value) << 1)
+#define L3_CACHEABILITY(value)	((value) << 4)
+
+/* Helper defines */
+#define GEN9_NUM_MOCS_ENTRIES	62  /* 62 out of 64 - 63 & 64 are reserved. */
+
+/* (e)LLC caching options */
+#define LE_PAGETABLE		0
+#define LE_UC			1
+#define LE_WT			2
+#define LE_WB			3
+
+/* L3 caching options */
+#define L3_DIRECT		0
+#define L3_UC			1
+#define L3_RESERVED		2
+#define L3_WB			3
+
+/* Target cache */
+#define ELLC			0
+#define LLC			1
+#define LLC_ELLC		2
+
+/*
+ * 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.
+ *
+ * These tables are intended to be kept reasonably consistent across
+ * platforms. However some of the fields are not applicable to all of
+ * them.
+ *
+ * Entries not part of the following tables are undefined as far as
+ * userspace is concerned and shouldn't be relied upon.  For the time
+ * being they will be implicitly initialized to the strictest caching
+ * configuration (uncached) to guarantee forwards compatibility with
+ * userspace programs written against more recent kernels providing
+ * additional MOCS entries.
+ *
+ * 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.  These tables are part of the kernel ABI and
+ *       may only be updated incrementally by adding entries at the
+ *       end.
+ */
+static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
+	/* { 0x00000009, 0x0010 } */
+	{ (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
+	/* { 0x00000038, 0x0030 } */
+	{ (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
+	/* { 0x0000003b, 0x0030 } */
+	{ (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
+};
+
+/* NOTE: the LE_TGT_CACHE is not used on Broxton */
+static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
+	/* { 0x00000009, 0x0010 } */
+	{ (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
+	/* { 0x00000038, 0x0030 } */
+	{ (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
+	/* { 0x0000003b, 0x0030 } */
+	{ (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
+	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
+	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
+};
+
+/**
+ * get_mocs_settings()
+ * @dev:        DRM device.
+ * @table:      Output table that will be made to point at appropriate
+ *              MOCS values for the device.
+ *
+ * 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.
+ *
+ * Return: true if there are applicable MOCS settings for the device.
+ */
+static bool get_mocs_settings(struct drm_device *dev,
+			      struct drm_i915_mocs_table *table)
+{
+	bool result = false;
+
+	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 {
+		WARN_ONCE(INTEL_INFO(dev)->gen >= 9,
+			  "Platform that should have a MOCS table does not.\n");
+	}
+
+	return result;
+}
+
+/**
+ * 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;
+
+	if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
+		return -ENODEV;
+
+	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;
+	}
+
+	intel_logical_ring_emit(ringbuf,
+				MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES));
+
+	for (index = 0; index < table->size; index++) {
+		intel_logical_ring_emit(ringbuf, reg_base + index * 4);
+		intel_logical_ring_emit(ringbuf,
+					table->table[index].control_value);
+	}
+
+	/*
+	 * Ok, now set the unused entries to uncached. These entries
+	 * are officially undefined and no contract for the contents
+	 * and settings is given for these entries.
+	 *
+	 * Entry 0 in the table is uncached - so we are just writing
+	 * that value to all the used entries.
+	 */
+	for (; index < GEN9_NUM_MOCS_ENTRIES; index++) {
+		intel_logical_ring_emit(ringbuf, reg_base + index * 4);
+		intel_logical_ring_emit(ringbuf, table->table[0].control_value);
+	}
+
+	intel_logical_ring_emit(ringbuf, MI_NOOP);
+	intel_logical_ring_advance(ringbuf);
+
+	return 0;
+}
+
+/**
+ * emit_mocs_l3cc_table() - emit the mocs control table
+ * @req:	Request to set up the MOCS table for.
+ * @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: 0 on success, otherwise the error status.
+ */
+static int emit_mocs_l3cc_table(struct drm_i915_gem_request *req,
+				const struct drm_i915_mocs_table *table)
+{
+	struct intel_ringbuffer *ringbuf = req->ringbuf;
+	unsigned int count;
+	unsigned int i;
+	u32 value;
+	u32 filler = (table->table[0].l3cc_value & 0xffff) |
+			((table->table[0].l3cc_value & 0xffff) << 16);
+	int ret;
+
+	if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
+		return -ENODEV;
+
+	ret = intel_logical_ring_begin(req, 2 + GEN9_NUM_MOCS_ENTRIES);
+	if (ret) {
+		DRM_DEBUG("intel_logical_ring_begin failed %d\n", ret);
+		return ret;
+	}
+
+	intel_logical_ring_emit(ringbuf,
+			MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES / 2));
+
+	for (i = 0, count = 0; i < table->size / 2; i++, count += 2) {
+		value = (table->table[count].l3cc_value & 0xffff) |
+			((table->table[count + 1].l3cc_value & 0xffff) << 16);
+
+		intel_logical_ring_emit(ringbuf, GEN9_LNCFCMOCS0 + i * 4);
+		intel_logical_ring_emit(ringbuf, value);
+	}
+
+	if (table->size & 0x01) {
+		/* Odd table size - 1 left over */
+		value = (table->table[count].l3cc_value & 0xffff) |
+			((table->table[0].l3cc_value & 0xffff) << 16);
+	} else
+		value = filler;
+
+	/*
+	 * Now set the rest of the table to uncached - use entry 0 as
+	 * this will be uncached. Leave the last pair uninitialised as
+	 * they are reserved by the hardware.
+	 */
+	for (; i < GEN9_NUM_MOCS_ENTRIES / 2; i++) {
+		intel_logical_ring_emit(ringbuf, GEN9_LNCFCMOCS0 + i * 4);
+		intel_logical_ring_emit(ringbuf, value);
+
+		value = filler;
+	}
+
+	intel_logical_ring_emit(ringbuf, MI_NOOP);
+	intel_logical_ring_advance(ringbuf);
+
+	return 0;
+}
+
+/**
+ * intel_rcs_context_init_mocs() - program the MOCS register.
+ * @req:	Request to set up the MOCS tables for.
+ *
+ * This function will emit a batch buffer with the values required for
+ * programming the MOCS register values for all the currently supported
+ * rings.
+ *
+ * These registers are partially stored in the RCS context, so they are
+ * emitted at the same time so that when a context is created these registers
+ * are set up. These registers have to be emitted into the start of the
+ * context as setting the ELSP will re-init some of these registers back
+ * to the hw values.
+ *
+ * Return: 0 on success, otherwise the error status.
+ */
+int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req)
+{
+	struct drm_i915_mocs_table t;
+	int ret;
+
+	if (get_mocs_settings(req->ring->dev, &t)) {
+		/* Program the control registers */
+		ret = emit_mocs_control_table(req, &t, GEN9_GFX_MOCS_0);
+		if (ret)
+			return ret;
+
+		ret = emit_mocs_control_table(req, &t, GEN9_MFX0_MOCS_0);
+		if (ret)
+			return ret;
+
+		ret = emit_mocs_control_table(req, &t, GEN9_MFX1_MOCS_0);
+		if (ret)
+			return ret;
+
+		ret = emit_mocs_control_table(req, &t, GEN9_VEBOX_MOCS_0);
+		if (ret)
+			return ret;
+
+		ret = emit_mocs_control_table(req, &t, GEN9_BLT_MOCS_0);
+		if (ret)
+			return ret;
+
+		/* Now program the l3cc registers */
+		ret = emit_mocs_l3cc_table(req, &t);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_mocs.h b/drivers/gpu/drm/i915/intel_mocs.h
new file mode 100644
index 0000000..76e45b1
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_mocs.h
@@ -0,0 +1,57 @@
+/*
+ * Copyright (c) 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef INTEL_MOCS_H
+#define INTEL_MOCS_H
+
+/**
+ * DOC: Memory Objects Control State (MOCS)
+ *
+ * Motivation:
+ * In previous Gens the MOCS settings was a value that was set by user land as
+ * part of the batch. In Gen9 this has changed to be a single table (per ring)
+ * that all batches now reference by index instead of programming the MOCS
+ * directly.
+ *
+ * The one wrinkle in this is that only PART of the MOCS tables are included
+ * in context (The GFX_MOCS_0 - GFX_MOCS_64 and the LNCFCMOCS0 - LNCFCMOCS32
+ * registers). The rest are not (the settings for the other rings).
+ *
+ * This table needs to be set at system start-up because the way the table
+ * interacts with the contexts and the GmmLib interface.
+ *
+ *
+ * Implementation:
+ *
+ * The tables (one per supported platform) are defined in intel_mocs.c
+ * and are programmed in the first batch after the context is loaded
+ * (with the hardware workarounds). This will then let the usual
+ * context handling keep the MOCS in step.
+ */
+
+#include <drm/drmP.h>
+#include "i915_drv.h"
+
+int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req);
+
+#endif
-- 
2.4.3

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

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

[Damien Lespiau]

On Fri, Jul 10, 2015 at 08:13:11PM +0300, Francisco Jerez wrote:
> From: Peter Antoine <peter.antoine@intel.com>
> 
> This change adds the programming of the MOCS registers to the gen 9+
> platforms. The set of MOCS configuration entries introduced by this
> patch is intended to be minimal but sufficient to cover the needs of
> current userspace - i.e. a good set of defaults. It is expected to be
> extended in the future to provide further default values or to allow
> userspace to redefine its private MOCS tables based on its demand for
> additional caching configurations. In this setup, userspace should
> only utilize the first N entries, higher entries are reserved for
> future use.
> 
> 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)
> v5:
> - Changed the name of a function. (Chris Wilson)
> v6:
> - re-based
> - Added Mesa table entry (skylake & broxton) (Francisco Jerez)
> - Tidied up the readability defines (Francisco Jerez)
> - NUMBER of entries defines wrong. (Jim Bish)
> - Added comments to clear up the meaning of the tables (Jim Bish)
> 
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> 
> v7 (Francisco Jerez):
> - Don't write L3-specific MOCS_ESC/SCC values into the e/LLC control
>   tables.  Prefix L3-specific defines consistently with L3_ and
>   e/LLC-specific defines with LE_ to avoid this kind of confusion in
>   the future.
> - Change L3CC WT define back to RESERVED (matches my hardware
>   documentation and the original patch, probably a misunderstanding
>   of my own previous comment).
> - Drop Android tables, define new minimal tables more suitable for the
>   open source stack.
> - Add comment that the MOCS tables are part of the kernel ABI.
> - Move intel_logical_ring_begin() and _advance() calls one level down
>   (Chris Wilson).
> - Minor formatting and style fixes.
> v8 (Francisco Jerez):
> - Add table size sanity check to emit_mocs_control/l3cc_table() (Chris
>   Wilson).
> - Add comment about undefined entries being implicitly set to uncached
>   for forwards compatibility.
> v9 (Francisco Jerez):
> - Minor style fixes.

What's happening here? are we ready to commit to this ABI?

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

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

[Francisco Jerez]

[-- Attachment #1.1.1: Type: text/plain, Size: 3147 bytes --]

Damien Lespiau <damien.lespiau@intel.com> writes:

> On Fri, Jul 10, 2015 at 08:13:11PM +0300, Francisco Jerez wrote:
>> From: Peter Antoine <peter.antoine@intel.com>
>> 
>> This change adds the programming of the MOCS registers to the gen 9+
>> platforms. The set of MOCS configuration entries introduced by this
>> patch is intended to be minimal but sufficient to cover the needs of
>> current userspace - i.e. a good set of defaults. It is expected to be
>> extended in the future to provide further default values or to allow
>> userspace to redefine its private MOCS tables based on its demand for
>> additional caching configurations. In this setup, userspace should
>> only utilize the first N entries, higher entries are reserved for
>> future use.
>> 
>> 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)
>> v5:
>> - Changed the name of a function. (Chris Wilson)
>> v6:
>> - re-based
>> - Added Mesa table entry (skylake & broxton) (Francisco Jerez)
>> - Tidied up the readability defines (Francisco Jerez)
>> - NUMBER of entries defines wrong. (Jim Bish)
>> - Added comments to clear up the meaning of the tables (Jim Bish)
>> 
>> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
>> 
>> v7 (Francisco Jerez):
>> - Don't write L3-specific MOCS_ESC/SCC values into the e/LLC control
>>   tables.  Prefix L3-specific defines consistently with L3_ and
>>   e/LLC-specific defines with LE_ to avoid this kind of confusion in
>>   the future.
>> - Change L3CC WT define back to RESERVED (matches my hardware
>>   documentation and the original patch, probably a misunderstanding
>>   of my own previous comment).
>> - Drop Android tables, define new minimal tables more suitable for the
>>   open source stack.
>> - Add comment that the MOCS tables are part of the kernel ABI.
>> - Move intel_logical_ring_begin() and _advance() calls one level down
>>   (Chris Wilson).
>> - Minor formatting and style fixes.
>> v8 (Francisco Jerez):
>> - Add table size sanity check to emit_mocs_control/l3cc_table() (Chris
>>   Wilson).
>> - Add comment about undefined entries being implicitly set to uncached
>>   for forwards compatibility.
>> v9 (Francisco Jerez):
>> - Minor style fixes.
>
> What's happening here? are we ready to commit to this ABI?

I'm for it.  I also sent a patch for userspace to switch to the new
tables [1] and already have an R-b on it.

[1] http://lists.freedesktop.org/archives/mesa-dev/2015-July/088310.html

>
> -- 
> Damien

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

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

[Daniel Vetter]

On Tue, Jul 14, 2015 at 05:47:37PM +0300, Francisco Jerez wrote:
> Damien Lespiau <damien.lespiau@intel.com> writes:
> 
> > On Fri, Jul 10, 2015 at 08:13:11PM +0300, Francisco Jerez wrote:
> >> From: Peter Antoine <peter.antoine@intel.com>
> >> 
> >> This change adds the programming of the MOCS registers to the gen 9+
> >> platforms. The set of MOCS configuration entries introduced by this
> >> patch is intended to be minimal but sufficient to cover the needs of
> >> current userspace - i.e. a good set of defaults. It is expected to be
> >> extended in the future to provide further default values or to allow
> >> userspace to redefine its private MOCS tables based on its demand for
> >> additional caching configurations. In this setup, userspace should
> >> only utilize the first N entries, higher entries are reserved for
> >> future use.
> >> 
> >> 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)
> >> v5:
> >> - Changed the name of a function. (Chris Wilson)
> >> v6:
> >> - re-based
> >> - Added Mesa table entry (skylake & broxton) (Francisco Jerez)
> >> - Tidied up the readability defines (Francisco Jerez)
> >> - NUMBER of entries defines wrong. (Jim Bish)
> >> - Added comments to clear up the meaning of the tables (Jim Bish)
> >> 
> >> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> >> 
> >> v7 (Francisco Jerez):
> >> - Don't write L3-specific MOCS_ESC/SCC values into the e/LLC control
> >>   tables.  Prefix L3-specific defines consistently with L3_ and
> >>   e/LLC-specific defines with LE_ to avoid this kind of confusion in
> >>   the future.
> >> - Change L3CC WT define back to RESERVED (matches my hardware
> >>   documentation and the original patch, probably a misunderstanding
> >>   of my own previous comment).
> >> - Drop Android tables, define new minimal tables more suitable for the
> >>   open source stack.
> >> - Add comment that the MOCS tables are part of the kernel ABI.
> >> - Move intel_logical_ring_begin() and _advance() calls one level down
> >>   (Chris Wilson).
> >> - Minor formatting and style fixes.
> >> v8 (Francisco Jerez):
> >> - Add table size sanity check to emit_mocs_control/l3cc_table() (Chris
> >>   Wilson).
> >> - Add comment about undefined entries being implicitly set to uncached
> >>   for forwards compatibility.
> >> v9 (Francisco Jerez):
> >> - Minor style fixes.
> >
> > What's happening here? are we ready to commit to this ABI?
> 
> I'm for it.  I also sent a patch for userspace to switch to the new
> tables [1] and already have an R-b on it.
> 
> [1] http://lists.freedesktop.org/archives/mesa-dev/2015-July/088310.html

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Siluvery, Arun]

On 07/07/2015 20:13, Francisco Jerez wrote:
> From: Peter Antoine <peter.antoine@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.
>
> 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)
> v5:
> - Changed the name of a function. (Chris Wilson)
> v6:
> - re-based
> - Added Mesa table entry (skylake & broxton) (Francisco Jerez)
> - Tidied up the readability defines (Francisco Jerez)
> - NUMBER of entries defines wrong. (Jim Bish)
> - Added comments to clear up the meaning of the tables (Jim Bish)
>
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
>
> v7 (Francisco Jerez):
> - Don't write L3-specific MOCS_ESC/SCC values into the e/LLC control
>    tables.  Prefix L3-specific defines consistently with L3_ and
>    e/LLC-specific defines with LE_ to avoid this kind of confusion in
>    the future.
> - Change L3CC WT define back to RESERVED (matches my hardware
>    documentation and the original patch, probably a misunderstanding
>    of my own previous comment).
> - Drop Android tables, define new minimal tables more suitable for the
>    open source stack.
> - Add comment that the MOCS tables are part of the kernel ABI.
> - Move intel_logical_ring_begin() and _advance() calls one level down
>    (Chris Wilson).
> - Minor formatting and style fixes.
>
> Signed-off-by: Francisco Jerez <currojerez@riseup.net>
> ---
>   drivers/gpu/drm/i915/Makefile     |   1 +
>   drivers/gpu/drm/i915/i915_reg.h   |   9 ++
>   drivers/gpu/drm/i915/intel_lrc.c  |  11 +-
>   drivers/gpu/drm/i915/intel_lrc.h  |   1 +
>   drivers/gpu/drm/i915/intel_mocs.c | 324 ++++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_mocs.h |  57 +++++++
>   6 files changed, 401 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/gpu/drm/i915/intel_mocs.c
>   create mode 100644 drivers/gpu/drm/i915/intel_mocs.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index de21965..e52e012 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -36,6 +36,7 @@ i915-y += i915_cmd_parser.o \
>   	  i915_trace_points.o \
>   	  intel_hotplug.o \
>   	  intel_lrc.o \
> +	  intel_mocs.o \
>   	  intel_ringbuffer.o \
>   	  intel_uncore.o
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2a29bcc..9b17260 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7906,4 +7906,13 @@ enum skl_disp_power_wells {
>   #define _PALETTE_A (dev_priv->info.display_mmio_offset + 0xa000)
>   #define _PALETTE_B (dev_priv->info.display_mmio_offset + 0xa800)
>
> +/* MOCS (Memory Object Control State) registers */
> +#define GEN9_LNCFCMOCS0		(0xB020)	/* L3 Cache Control base */
> +
> +#define GEN9_GFX_MOCS_0		(0xc800)	/* Graphics MOCS base register*/
> +#define GEN9_MFX0_MOCS_0	(0xc900)	/* Media 0 MOCS base register*/
> +#define GEN9_MFX1_MOCS_0	(0xcA00)	/* Media 1 MOCS base register*/
> +#define GEN9_VEBOX_MOCS_0	(0xcB00)	/* Video MOCS base register*/
> +#define GEN9_BLT_MOCS_0		(0xcc00)	/* Blitter MOCS base register*/
> +
>   #endif /* _I915_REG_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index d4f8b43..466d17c 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -135,6 +135,7 @@
>   #include <drm/drmP.h>
>   #include <drm/i915_drm.h>
>   #include "i915_drv.h"
> +#include "intel_mocs.h"
>
>   #define GEN9_LR_CONTEXT_RENDER_SIZE (22 * PAGE_SIZE)
>   #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
> @@ -772,8 +773,7 @@ static int logical_ring_prepare(struct drm_i915_gem_request *req, int bytes)
>    *
>    * Return: non-zero if the ringbuffer is not ready to be written to.
>    */
> -static int intel_logical_ring_begin(struct drm_i915_gem_request *req,
> -				    int num_dwords)
> +int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords)
>   {
>   	struct drm_i915_private *dev_priv;
>   	int ret;
> @@ -1675,6 +1675,13 @@ static int gen8_init_rcs_context(struct drm_i915_gem_request *req)
>   	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(req) != 0)
> +		DRM_ERROR("MOCS failed to program: expect performance issues.");

we don't see this type of constructs, to be consistent why not,
ret = intel_rcs_context_init_mocs(req);
if (ret)
	DRM_ERROR();

nitpick, comment says warning but DRM_ERROR is used.
'\n' missing in DRM_ERROR

> +
>   	return intel_lr_context_render_state_init(req);
>   }
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index e0299fb..64f89f99 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -42,6 +42,7 @@ int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request);
>   void intel_logical_ring_stop(struct intel_engine_cs *ring);
>   void intel_logical_ring_cleanup(struct intel_engine_cs *ring);
>   int intel_logical_rings_init(struct drm_device *dev);
> +int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords);
>
>   int logical_ring_flush_all_caches(struct drm_i915_gem_request *req);
>   /**
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> new file mode 100644
> index 0000000..f7b93e9
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -0,0 +1,324 @@
> +/*
> + * Copyright (c) 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions: *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#include "intel_mocs.h"
> +#include "intel_lrc.h"
> +#include "intel_ringbuffer.h"
> +
> +/* structures required */
> +struct drm_i915_mocs_entry {
> +	u32	control_value;
> +	u16	l3cc_value;
> +};
> +
> +struct drm_i915_mocs_table {
> +	u32					size;
> +	const struct drm_i915_mocs_entry	*table;
> +};
> +
too much spacing.

> +/* Defines for the tables (XXX_MOCS_0 - XXX_MOCS_63) */
> +#define LE_CACHEABILITY(value)	(value << 0)
> +#define LE_TGT_CACHE(value)	(value << 2)
> +#define LE_LRUM(value)		(value << 4)
> +#define LE_AOM(value)		(value << 6)
> +#define LE_RSC(value)		(value << 7)
> +#define LE_SCC(value)		(value << 8)
> +#define LE_PFM(value)		(value << 11)
> +#define LE_SCF(value)		(value << 14)
> +
> +/* Defines for the tables (LNCFMOCS0 - LNCFMOCS31) - two entries per word */
> +#define L3_ESC(value)		(value << 0)
> +#define L3_SCC(value)		(value << 1)
> +#define L3_CACHEABILITY(value)	(value << 4)
> +
> +/* Helper defines */
> +#define GEN9_NUM_MOCS_ENTRIES	(62)  /* 62 out of 64 - 63 & 64 are reserved. */
> +
> +/* (e)LLC caching options */
> +#define LE_PAGETABLE		(0)
> +#define LE_UC			(1)
> +#define LE_WT			(2)
> +#define LE_WB			(3)
> +
> +/* L3 caching options */
> +#define L3_DIRECT		(0)
> +#define L3_UC			(1)
> +#define L3_RESERVED		(2)
> +#define L3_WB			(3)
> +
> +/* Target cache */
> +#define ELLC			(0)
> +#define LLC			(1)
> +#define LLC_ELLC		(2)
> +
> +/*
> + * 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.
> + *
> + * These tables are intended to be kept reasonably consistent across
> + * platforms. However some of the fields are not applicable to all of
> + * them.
> + *
> + * 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.  These tables are part of the kernel ABI and
> + *       may only be updated incrementally by adding entries at the
> + *       end.
> + */
> +static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
> +	/* { 0x00000009, 0x0010 } */
> +	{ (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
> +	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
> +	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
> +	/* { 0x00000038, 0x0030 } */
> +	{ (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
> +	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
> +	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
> +	/* { 0x0000003b, 0x0030 } */
> +	{ (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
> +	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
> +	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
> +};
> +
> +/* NOTE: the LE_TGT_CACHE is not used on Broxton */
> +static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
> +	/* { 0x00000009, 0x0010 } */
> +	{ (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
> +	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
> +	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
> +	/* { 0x00000038, 0x0030 } */
> +	{ (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
> +	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
> +	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
> +	/* { 0x0000003b, 0x0030 } */
> +	{ (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
> +	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
> +	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
> +};
> +
> +/**
> + * 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)
> +{
you will get a warning from kernel 0-day that description is missing for 
arguments and return value, I think it is better to add now or new 
warning gets added to the current list of warnings.

> +	bool result = false;
> +
> +	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);
if we use this kernel on future platforms before MOCS support is added, 
this prints WARNING every time context is initialized, I faced similar 
issue with WA batch and the comment that I received was that WARNING is 
not very useful here hence changed it to DRM_ERROR but no strong 
opinion, perhaps WARN_ONCE if you want to retain the warning.

> +	}
> +
> +	return result;
> +}
> +
> +/**
> + * 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;
> +	}
> +
> +	intel_logical_ring_emit(ringbuf,
> +				MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES));
> +
> +	for (index = 0; index < table->size; index++) {
> +		intel_logical_ring_emit(ringbuf, reg_base + index * 4);
> +		intel_logical_ring_emit(ringbuf,
> +					table->table[index].control_value);
> +	}

intel_logical_ring_emit(ringbuf, MI_NOOP);
after the loop to make number of commands even.
Count also need to be updated.

> +
> +	/*
> +	 * Ok, now set the unused entries to uncached. These entries
> +	 * are officially undefined and no contract for the contents
> +	 * and settings is given for these entries.
> +	 *
> +	 * Entry 0 in the table is uncached - so we are just writing
> +	 * that value to all the used entries.
> +	 */
> +	for (; index < GEN9_NUM_MOCS_ENTRIES; index++) {
> +		intel_logical_ring_emit(ringbuf, reg_base + index * 4);
> +		intel_logical_ring_emit(ringbuf, table->table[0].control_value);
> +	}
> +
> +	intel_logical_ring_emit(ringbuf, MI_NOOP);
> +	intel_logical_ring_advance(ringbuf);
> +
> +	return 0;
> +}
> +
> +/**
> + * emit_mocs_l3cc_table() - emit the mocs control table
> + * @req:	Request to set up the MOCS table for.
> + * @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 0 on success, otherwise the error status.
> + */
> +static int emit_mocs_l3cc_table(struct drm_i915_gem_request *req,
> +				const struct drm_i915_mocs_table *table)
> +{
> +	struct intel_ringbuffer *ringbuf = req->ringbuf;
> +	unsigned int count;
> +	unsigned int i;
> +	u32 value;
> +	u32 filler = (table->table[0].l3cc_value & 0xffff) |
> +			((table->table[0].l3cc_value & 0xffff) << 16);
> +	int ret;
> +
> +	ret = intel_logical_ring_begin(req, 2 + GEN9_NUM_MOCS_ENTRIES);
> +	if (ret) {
> +		DRM_DEBUG("intel_logical_ring_begin failed %d\n", ret);
> +		return ret;
> +	}
> +
> +	intel_logical_ring_emit(ringbuf,
> +			MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES / 2));
> +
> +	for (i = 0, count = 0; i < table->size / 2; i++, count += 2) {
> +		value = (table->table[count].l3cc_value & 0xffff) |
> +			((table->table[count + 1].l3cc_value & 0xffff) << 16);
> +
> +		intel_logical_ring_emit(ringbuf, GEN9_LNCFCMOCS0 + i * 4);
> +		intel_logical_ring_emit(ringbuf, value);
> +	}
> +
same as above.

> +	if (table->size & 0x01) {
> +		/* Odd table size - 1 left over */
> +		value = (table->table[count].l3cc_value & 0xffff) |
> +			((table->table[0].l3cc_value & 0xffff) << 16);
> +	} else
> +		value = filler;
> +
> +	/*
> +	 * Now set the rest of the table to uncached - use entry 0 as
> +	 * this will be uncached. Leave the last pair uninitialised as
> +	 * they are reserved by the hardware.
> +	 */
> +	for (; i < GEN9_NUM_MOCS_ENTRIES / 2; i++) {
> +		intel_logical_ring_emit(ringbuf, GEN9_LNCFCMOCS0 + i * 4);
> +		intel_logical_ring_emit(ringbuf, value);
> +
> +		value = filler;
> +	}
> +
> +	intel_logical_ring_emit(ringbuf, MI_NOOP);
> +	intel_logical_ring_advance(ringbuf);
> +
> +	return 0;
> +}
> +
> +/**
> + * intel_rcs_context_init_mocs() - program the MOCS register.
> + *
> + * @req:	Request to set up the MOCS tables for.
> + *
> + * This function will emit a batch buffer with the values required for
> + * programming the MOCS register values for all the currently supported
> + * rings.
> + *
> + * These registers are partially stored in the RCS context, so they are
> + * emitted at the same time so that when a context is created these registers
> + * are set up. These registers have to be emitted into the start of the
> + * context as setting the ELSP will re-init some of these registers back
> + * to the hw values.
> + *
> + * @return 0 on success, otherwise the error status.
> + */
> +int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req)
> +{
> +	struct drm_i915_mocs_table t;
> +	int ret;
> +
> +	if (get_mocs_settings(req->ring->dev, &t)) {
can be changed to "ret = get_mocs_settings();" to be consistent and 
handle error case first; otherwise in this case non-zero is treated as 
success case and zero as failure.

any sanity check on the received table, if at all required?

> +		/* Program the control registers */
> +		ret = emit_mocs_control_table(req, &t, GEN9_GFX_MOCS_0);
> +		if (ret)
> +			return ret;
> +
> +		ret = emit_mocs_control_table(req, &t, GEN9_MFX0_MOCS_0);
> +		if (ret)
> +			return ret;
> +
> +		ret = emit_mocs_control_table(req, &t, GEN9_MFX1_MOCS_0);
> +		if (ret)
> +			return ret;
> +
> +		ret = emit_mocs_control_table(req, &t, GEN9_VEBOX_MOCS_0);
> +		if (ret)
> +			return ret;
> +
> +		ret = emit_mocs_control_table(req, &t, GEN9_BLT_MOCS_0);
> +		if (ret)
> +			return ret;
> +
> +		/* Now program the l3cc registers */
> +		ret = emit_mocs_l3cc_table(req, &t);
> +		if (ret)
> +			return ret;

In case if we fail before setting tables for all rings, is that a valid 
setup? I mean do we need to revert the entries in those rings that are 
successfully setup?

> +
> +		DRM_DEBUG("MOCS: Table set in context.\n");
> +	} else {
> +		DRM_DEBUG("MOCS: Table not supported on platform.\n");
DRM_ERROR may be?
and it returns 0 in this case also as success; perhaps we can remove 
printing any message in this as we already WARN/DRM_ERROR in 
get_mocs_settings() or move it here and remove it that function.

regards
Arun

> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_mocs.h b/drivers/gpu/drm/i915/intel_mocs.h
> new file mode 100644
> index 0000000..76e45b1
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_mocs.h
> @@ -0,0 +1,57 @@
> +/*
> + * Copyright (c) 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#ifndef INTEL_MOCS_H
> +#define INTEL_MOCS_H
> +
> +/**
> + * DOC: Memory Objects Control State (MOCS)
> + *
> + * Motivation:
> + * In previous Gens the MOCS settings was a value that was set by user land as
> + * part of the batch. In Gen9 this has changed to be a single table (per ring)
> + * that all batches now reference by index instead of programming the MOCS
> + * directly.
> + *
> + * The one wrinkle in this is that only PART of the MOCS tables are included
> + * in context (The GFX_MOCS_0 - GFX_MOCS_64 and the LNCFCMOCS0 - LNCFCMOCS32
> + * registers). The rest are not (the settings for the other rings).
> + *
> + * This table needs to be set at system start-up because the way the table
> + * interacts with the contexts and the GmmLib interface.
> + *
> + *
> + * Implementation:
> + *
> + * The tables (one per supported platform) are defined in intel_mocs.c
> + * and are programmed in the first batch after the context is loaded
> + * (with the hardware workarounds). This will then let the usual
> + * context handling keep the MOCS in step.
> + */
> +
> +#include <drm/drmP.h>
> +#include "i915_drv.h"
> +
> +int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req);
> +
> +#endif
>

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

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

[Francisco Jerez]

[-- Attachment #1.1.1: Type: text/plain, Size: 23997 bytes --]

"Siluvery, Arun" <arun.siluvery@linux.intel.com> writes:

> On 07/07/2015 20:13, Francisco Jerez wrote:
>> From: Peter Antoine <peter.antoine@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.
>>
>> 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)
>> v5:
>> - Changed the name of a function. (Chris Wilson)
>> v6:
>> - re-based
>> - Added Mesa table entry (skylake & broxton) (Francisco Jerez)
>> - Tidied up the readability defines (Francisco Jerez)
>> - NUMBER of entries defines wrong. (Jim Bish)
>> - Added comments to clear up the meaning of the tables (Jim Bish)
>>
>> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
>>
>> v7 (Francisco Jerez):
>> - Don't write L3-specific MOCS_ESC/SCC values into the e/LLC control
>>    tables.  Prefix L3-specific defines consistently with L3_ and
>>    e/LLC-specific defines with LE_ to avoid this kind of confusion in
>>    the future.
>> - Change L3CC WT define back to RESERVED (matches my hardware
>>    documentation and the original patch, probably a misunderstanding
>>    of my own previous comment).
>> - Drop Android tables, define new minimal tables more suitable for the
>>    open source stack.
>> - Add comment that the MOCS tables are part of the kernel ABI.
>> - Move intel_logical_ring_begin() and _advance() calls one level down
>>    (Chris Wilson).
>> - Minor formatting and style fixes.
>>
>> Signed-off-by: Francisco Jerez <currojerez@riseup.net>
>> ---
>>   drivers/gpu/drm/i915/Makefile     |   1 +
>>   drivers/gpu/drm/i915/i915_reg.h   |   9 ++
>>   drivers/gpu/drm/i915/intel_lrc.c  |  11 +-
>>   drivers/gpu/drm/i915/intel_lrc.h  |   1 +
>>   drivers/gpu/drm/i915/intel_mocs.c | 324 ++++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_mocs.h |  57 +++++++
>>   6 files changed, 401 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/gpu/drm/i915/intel_mocs.c
>>   create mode 100644 drivers/gpu/drm/i915/intel_mocs.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index de21965..e52e012 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -36,6 +36,7 @@ i915-y += i915_cmd_parser.o \
>>   	  i915_trace_points.o \
>>   	  intel_hotplug.o \
>>   	  intel_lrc.o \
>> +	  intel_mocs.o \
>>   	  intel_ringbuffer.o \
>>   	  intel_uncore.o
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 2a29bcc..9b17260 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7906,4 +7906,13 @@ enum skl_disp_power_wells {
>>   #define _PALETTE_A (dev_priv->info.display_mmio_offset + 0xa000)
>>   #define _PALETTE_B (dev_priv->info.display_mmio_offset + 0xa800)
>>
>> +/* MOCS (Memory Object Control State) registers */
>> +#define GEN9_LNCFCMOCS0		(0xB020)	/* L3 Cache Control base */
>> +
>> +#define GEN9_GFX_MOCS_0		(0xc800)	/* Graphics MOCS base register*/
>> +#define GEN9_MFX0_MOCS_0	(0xc900)	/* Media 0 MOCS base register*/
>> +#define GEN9_MFX1_MOCS_0	(0xcA00)	/* Media 1 MOCS base register*/
>> +#define GEN9_VEBOX_MOCS_0	(0xcB00)	/* Video MOCS base register*/
>> +#define GEN9_BLT_MOCS_0		(0xcc00)	/* Blitter MOCS base register*/
>> +
>>   #endif /* _I915_REG_H_ */
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index d4f8b43..466d17c 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -135,6 +135,7 @@
>>   #include <drm/drmP.h>
>>   #include <drm/i915_drm.h>
>>   #include "i915_drv.h"
>> +#include "intel_mocs.h"
>>
>>   #define GEN9_LR_CONTEXT_RENDER_SIZE (22 * PAGE_SIZE)
>>   #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
>> @@ -772,8 +773,7 @@ static int logical_ring_prepare(struct drm_i915_gem_request *req, int bytes)
>>    *
>>    * Return: non-zero if the ringbuffer is not ready to be written to.
>>    */
>> -static int intel_logical_ring_begin(struct drm_i915_gem_request *req,
>> -				    int num_dwords)
>> +int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords)
>>   {
>>   	struct drm_i915_private *dev_priv;
>>   	int ret;
>> @@ -1675,6 +1675,13 @@ static int gen8_init_rcs_context(struct drm_i915_gem_request *req)
>>   	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(req) != 0)
>> +		DRM_ERROR("MOCS failed to program: expect performance issues.");
>
> we don't see this type of constructs, to be consistent why not,
> ret = intel_rcs_context_init_mocs(req);
> if (ret)
> 	DRM_ERROR();
>
> nitpick, comment says warning but DRM_ERROR is used.
> '\n' missing in DRM_ERROR
>
Thanks, fixed.

>> +
>>   	return intel_lr_context_render_state_init(req);
>>   }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
>> index e0299fb..64f89f99 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.h
>> +++ b/drivers/gpu/drm/i915/intel_lrc.h
>> @@ -42,6 +42,7 @@ int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request);
>>   void intel_logical_ring_stop(struct intel_engine_cs *ring);
>>   void intel_logical_ring_cleanup(struct intel_engine_cs *ring);
>>   int intel_logical_rings_init(struct drm_device *dev);
>> +int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords);
>>
>>   int logical_ring_flush_all_caches(struct drm_i915_gem_request *req);
>>   /**
>> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
>> new file mode 100644
>> index 0000000..f7b93e9
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_mocs.c
>> @@ -0,0 +1,324 @@
>> +/*
>> + * Copyright (c) 2015 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions: *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
>> + * SOFTWARE.
>> + */
>> +
>> +#include "intel_mocs.h"
>> +#include "intel_lrc.h"
>> +#include "intel_ringbuffer.h"
>> +
>> +/* structures required */
>> +struct drm_i915_mocs_entry {
>> +	u32	control_value;
>> +	u16	l3cc_value;
>> +};
>> +
>> +struct drm_i915_mocs_table {
>> +	u32					size;
>> +	const struct drm_i915_mocs_entry	*table;
>> +};
>> +
> too much spacing.
>
>> +/* Defines for the tables (XXX_MOCS_0 - XXX_MOCS_63) */
>> +#define LE_CACHEABILITY(value)	(value << 0)
>> +#define LE_TGT_CACHE(value)	(value << 2)
>> +#define LE_LRUM(value)		(value << 4)
>> +#define LE_AOM(value)		(value << 6)
>> +#define LE_RSC(value)		(value << 7)
>> +#define LE_SCC(value)		(value << 8)
>> +#define LE_PFM(value)		(value << 11)
>> +#define LE_SCF(value)		(value << 14)
>> +
>> +/* Defines for the tables (LNCFMOCS0 - LNCFMOCS31) - two entries per word */
>> +#define L3_ESC(value)		(value << 0)
>> +#define L3_SCC(value)		(value << 1)
>> +#define L3_CACHEABILITY(value)	(value << 4)
>> +
>> +/* Helper defines */
>> +#define GEN9_NUM_MOCS_ENTRIES	(62)  /* 62 out of 64 - 63 & 64 are reserved. */
>> +
>> +/* (e)LLC caching options */
>> +#define LE_PAGETABLE		(0)
>> +#define LE_UC			(1)
>> +#define LE_WT			(2)
>> +#define LE_WB			(3)
>> +
>> +/* L3 caching options */
>> +#define L3_DIRECT		(0)
>> +#define L3_UC			(1)
>> +#define L3_RESERVED		(2)
>> +#define L3_WB			(3)
>> +
>> +/* Target cache */
>> +#define ELLC			(0)
>> +#define LLC			(1)
>> +#define LLC_ELLC		(2)
>> +
>> +/*
>> + * 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.
>> + *
>> + * These tables are intended to be kept reasonably consistent across
>> + * platforms. However some of the fields are not applicable to all of
>> + * them.
>> + *
>> + * 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.  These tables are part of the kernel ABI and
>> + *       may only be updated incrementally by adding entries at the
>> + *       end.
>> + */
>> +static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
>> +	/* { 0x00000009, 0x0010 } */
>> +	{ (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
>> +	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
>> +	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
>> +	/* { 0x00000038, 0x0030 } */
>> +	{ (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
>> +	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
>> +	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
>> +	/* { 0x0000003b, 0x0030 } */
>> +	{ (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
>> +	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
>> +	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
>> +};
>> +
>> +/* NOTE: the LE_TGT_CACHE is not used on Broxton */
>> +static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
>> +	/* { 0x00000009, 0x0010 } */
>> +	{ (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
>> +	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
>> +	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
>> +	/* { 0x00000038, 0x0030 } */
>> +	{ (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
>> +	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
>> +	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
>> +	/* { 0x0000003b, 0x0030 } */
>> +	{ (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
>> +	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
>> +	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
>> +};
>> +
>> +/**
>> + * 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)
>> +{
> you will get a warning from kernel 0-day that description is missing for 
> arguments and return value, I think it is better to add now or new 
> warning gets added to the current list of warnings.
>

Fixed.

>> +	bool result = false;
>> +
>> +	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);
> if we use this kernel on future platforms before MOCS support is added, 
> this prints WARNING every time context is initialized, I faced similar 
> issue with WA batch and the comment that I received was that WARNING is 
> not very useful here hence changed it to DRM_ERROR but no strong 
> opinion, perhaps WARN_ONCE if you want to retain the warning.
>
Right, I've changed it locally to use WARN_ONCE() instead.

>> +	}
>> +
>> +	return result;
>> +}
>> +
>> +/**
>> + * 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;
>> +	}
>> +
>> +	intel_logical_ring_emit(ringbuf,
>> +				MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES));
>> +
>> +	for (index = 0; index < table->size; index++) {
>> +		intel_logical_ring_emit(ringbuf, reg_base + index * 4);
>> +		intel_logical_ring_emit(ringbuf,
>> +					table->table[index].control_value);
>> +	}
>
> intel_logical_ring_emit(ringbuf, MI_NOOP);
> after the loop to make number of commands even.
> Count also need to be updated.
>

It would be illegal to emit a MI_NOOP here in the middle of another
command.  It's already being done a few lines later BTW.

>> +
>> +	/*
>> +	 * Ok, now set the unused entries to uncached. These entries
>> +	 * are officially undefined and no contract for the contents
>> +	 * and settings is given for these entries.
>> +	 *
>> +	 * Entry 0 in the table is uncached - so we are just writing
>> +	 * that value to all the used entries.
>> +	 */
>> +	for (; index < GEN9_NUM_MOCS_ENTRIES; index++) {
>> +		intel_logical_ring_emit(ringbuf, reg_base + index * 4);
>> +		intel_logical_ring_emit(ringbuf, table->table[0].control_value);
>> +	}
>> +
>> +	intel_logical_ring_emit(ringbuf, MI_NOOP);
>> +	intel_logical_ring_advance(ringbuf);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * emit_mocs_l3cc_table() - emit the mocs control table
>> + * @req:	Request to set up the MOCS table for.
>> + * @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 0 on success, otherwise the error status.
>> + */
>> +static int emit_mocs_l3cc_table(struct drm_i915_gem_request *req,
>> +				const struct drm_i915_mocs_table *table)
>> +{
>> +	struct intel_ringbuffer *ringbuf = req->ringbuf;
>> +	unsigned int count;
>> +	unsigned int i;
>> +	u32 value;
>> +	u32 filler = (table->table[0].l3cc_value & 0xffff) |
>> +			((table->table[0].l3cc_value & 0xffff) << 16);
>> +	int ret;
>> +
>> +	ret = intel_logical_ring_begin(req, 2 + GEN9_NUM_MOCS_ENTRIES);
>> +	if (ret) {
>> +		DRM_DEBUG("intel_logical_ring_begin failed %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	intel_logical_ring_emit(ringbuf,
>> +			MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES / 2));
>> +
>> +	for (i = 0, count = 0; i < table->size / 2; i++, count += 2) {
>> +		value = (table->table[count].l3cc_value & 0xffff) |
>> +			((table->table[count + 1].l3cc_value & 0xffff) << 16);
>> +
>> +		intel_logical_ring_emit(ringbuf, GEN9_LNCFCMOCS0 + i * 4);
>> +		intel_logical_ring_emit(ringbuf, value);
>> +	}
>> +
> same as above.
>
>> +	if (table->size & 0x01) {
>> +		/* Odd table size - 1 left over */
>> +		value = (table->table[count].l3cc_value & 0xffff) |
>> +			((table->table[0].l3cc_value & 0xffff) << 16);
>> +	} else
>> +		value = filler;
>> +
>> +	/*
>> +	 * Now set the rest of the table to uncached - use entry 0 as
>> +	 * this will be uncached. Leave the last pair uninitialised as
>> +	 * they are reserved by the hardware.
>> +	 */
>> +	for (; i < GEN9_NUM_MOCS_ENTRIES / 2; i++) {
>> +		intel_logical_ring_emit(ringbuf, GEN9_LNCFCMOCS0 + i * 4);
>> +		intel_logical_ring_emit(ringbuf, value);
>> +
>> +		value = filler;
>> +	}
>> +
>> +	intel_logical_ring_emit(ringbuf, MI_NOOP);
>> +	intel_logical_ring_advance(ringbuf);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * intel_rcs_context_init_mocs() - program the MOCS register.
>> + *
>> + * @req:	Request to set up the MOCS tables for.
>> + *
>> + * This function will emit a batch buffer with the values required for
>> + * programming the MOCS register values for all the currently supported
>> + * rings.
>> + *
>> + * These registers are partially stored in the RCS context, so they are
>> + * emitted at the same time so that when a context is created these registers
>> + * are set up. These registers have to be emitted into the start of the
>> + * context as setting the ELSP will re-init some of these registers back
>> + * to the hw values.
>> + *
>> + * @return 0 on success, otherwise the error status.
>> + */
>> +int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req)
>> +{
>> +	struct drm_i915_mocs_table t;
>> +	int ret;
>> +
>> +	if (get_mocs_settings(req->ring->dev, &t)) {
> can be changed to "ret = get_mocs_settings();" to be consistent and 
> handle error case first; otherwise in this case non-zero is treated as 
> success case and zero as failure.
>

Maybe.  The thing is there seems to be no error case here AFAICT.  It's
not an error for a pre-Gen9 platform not to have MOCS tables, and it's
also not an error for other hardware to have MOCS tables to program.
Apparently it was Peter's intention to make get_mocs_settings() return a
boolean with no failure implications reporting whether the platform has
applicable MOCS settings or not, which seems to make sense to me.

> any sanity check on the received table, if at all required?
>
>> +		/* Program the control registers */
>> +		ret = emit_mocs_control_table(req, &t, GEN9_GFX_MOCS_0);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = emit_mocs_control_table(req, &t, GEN9_MFX0_MOCS_0);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = emit_mocs_control_table(req, &t, GEN9_MFX1_MOCS_0);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = emit_mocs_control_table(req, &t, GEN9_VEBOX_MOCS_0);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = emit_mocs_control_table(req, &t, GEN9_BLT_MOCS_0);
>> +		if (ret)
>> +			return ret;
>> +
>> +		/* Now program the l3cc registers */
>> +		ret = emit_mocs_l3cc_table(req, &t);
>> +		if (ret)
>> +			return ret;
>
> In case if we fail before setting tables for all rings, is that a valid 
> setup? I mean do we need to revert the entries in those rings that are 
> successfully setup?
>
Strictly speaking no, it wouldn't be a valid setup, but reverting back
to the original entries wouldn't give a valid setup either, and most
likely it would fail too because it would need to use basically the same
path that just failed to re-program the original entries.

>> +
>> +		DRM_DEBUG("MOCS: Table set in context.\n");
>> +	} else {
>> +		DRM_DEBUG("MOCS: Table not supported on platform.\n");
> DRM_ERROR may be?
> and it returns 0 in this case also as success; perhaps we can remove 
> printing any message in this as we already WARN/DRM_ERROR in 
> get_mocs_settings() or move it here and remove it that function.

As explained earlier neither of these two conditions is an error, but
the debug messages do seem a bit redundant, I've removed them locally.

>
> regards
> Arun
>
Thanks.

>> +	}
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_mocs.h b/drivers/gpu/drm/i915/intel_mocs.h
>> new file mode 100644
>> index 0000000..76e45b1
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_mocs.h
>> @@ -0,0 +1,57 @@
>> +/*
>> + * Copyright (c) 2015 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
>> + * SOFTWARE.
>> + */
>> +
>> +#ifndef INTEL_MOCS_H
>> +#define INTEL_MOCS_H
>> +
>> +/**
>> + * DOC: Memory Objects Control State (MOCS)
>> + *
>> + * Motivation:
>> + * In previous Gens the MOCS settings was a value that was set by user land as
>> + * part of the batch. In Gen9 this has changed to be a single table (per ring)
>> + * that all batches now reference by index instead of programming the MOCS
>> + * directly.
>> + *
>> + * The one wrinkle in this is that only PART of the MOCS tables are included
>> + * in context (The GFX_MOCS_0 - GFX_MOCS_64 and the LNCFCMOCS0 - LNCFCMOCS32
>> + * registers). The rest are not (the settings for the other rings).
>> + *
>> + * This table needs to be set at system start-up because the way the table
>> + * interacts with the contexts and the GmmLib interface.
>> + *
>> + *
>> + * Implementation:
>> + *
>> + * The tables (one per supported platform) are defined in intel_mocs.c
>> + * and are programmed in the first batch after the context is loaded
>> + * (with the hardware workarounds). This will then let the usual
>> + * context handling keep the MOCS in step.
>> + */
>> +
>> +#include <drm/drmP.h>
>> +#include "i915_drv.h"
>> +
>> +int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req);
>> +
>> +#endif
>>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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