[v4,1/8] drm/i915/skl: Add support to load SKL CSR firmware.

Submitted by Animesh Manna on April 16, 2015, 8:52 a.m.

Details

Message ID 1429174334-12089-2-git-send-email-animesh.manna@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Animesh Manna April 16, 2015, 8:52 a.m.
From: "A.Sunil Kamath" <sunil.kamath@intel.com>

Display Context Save and Restore support is needed for
various SKL Display C states like DC5, DC6.

This implementation is added based on first version of DMC CSR program
that we received from h/w team.

Here we are using request_firmware based design.
Finally this firmware should end up in linux-firmware tree.

For SKL platform its mandatory to ensure that we load this
csr program before enabling DC states like DC5/DC6.

As CSR program gets reset on various conditions, we should ensure
to load it during boot and in future change to be added to load
this system resume sequence too.

v1: Initial relese as RFC patch

v2: Design change as per Daniel, Damien and Shobit's review comments
request firmware method followed.

v3: Some optimization and functional changes.
Pulled register defines into drivers/gpu/drm/i915/i915_reg.h
Used kmemdup to allocate and duplicate firmware content.
Ensured to free allocated buffer.

v4: Modified as per review comments from Satheesh and Daniel
Removed temporary buffer.
Optimized number of writes by replacing I915_WRITE with I915_WRITE64.

v5:
Modified as per review comemnts from Damien.
- Changed name for functions and firmware.
- Introduced HAS_CSR.
- Reverted back previous change and used csr_buf with u8 size.
- Using cpu_to_be64 for endianness change.

Modified as per review comments from Imre.
- Modified registers and macro names to be a bit closer to bspec terminology
and the existing register naming in the driver.
- Early return for non SKL platforms in intel_load_csr_program function.
- Added locking around CSR program load function as it may be called
concurrently during system/runtime resume.
- Releasing the fw before loading the program for consistency
- Handled error path during f/w load.

v6: Modified as per review comments from Imre.
- Corrected out_freecsr sequence.

v7: Modified as per review comments from Imre.
Fail loading fw if fw->size%8!=0.

v8: Rebase to latest.

v9: Rebase on top of -nightly (Damien)

v10: Enabled support for dmc firmware ver 1.0.
According to ver 1.0 in a single binary package all the firmware's that are
required for different stepping's of the product will be stored. The package
contains the css header, followed by the package header and the actual dmc
firmwares. Package header contains the firmware/stepping mapping table and
the corresponding firmware offsets to the individual binaries, within the
package. Each individual program binary contains the header and the payload
sections whose size is specified in the header section. This changes are done
to extract the specific firmaware from the package. (Animesh)

v11: Modified as per review comemnts from Imre.
- Added code comment from bpec for header structure elements.
- Added __packed to avoid structure padding.
- Added helper functions for stepping and substepping info.
- Added code comment for CSR_MAX_FW_SIZE.
- Disabled BXT firmware loading, will be enabled with dmc 1.0 support.
- Changed skl_stepping_info based on bspec, earlier used from config DB.
- Removed duplicate call of cpu_to_be* from intel_csr_load_program function.
- Used cpu_to_be32 instead of cpu_to_be64 as firmware binary in dword aligned.
- Added sanity check for header length.
- Added sanity check for mmio address got from firmware binary.
- kmalloc done separately for dmc header and dmc firmware. (Animesh)

v12: Modified as per review comemnts from Imre.
- Corrected the typo error in skl stepping info structure.
- Added out-of-bound access for skl_stepping_info.
- Sanity check for mmio address modified.
- Sanity check added for stepping and substeppig.
- Modified the intel_dmc_info structure, cache only the required header info. (Animesh)

v13: clarify firmware load error message.
The reason for a firmware loading failure can be obscure if the driver
is built-in. Provide an explanation to the user about the likely reason for
the failure and how to resolve it. (Imre)

v14: Suggested by Jani.
- fix s/I915/CONFIG_DRM_I915/ typo
- add fw_path to the firmware object instead of using a static ptr (Jani)

v15:
1) Changed the firmware name as dmc_gen9.bin, everytime for a new firmware version a symbolic link
with same name will help not to build kernel again.
2) Changes done as per review comments from Imre.
- Error check removed for intel_csr_ucode_init.
- Moved csr-specific data structure to intel_csr.h and optimization done on structure definition.
- fw->data used directly for parsing the header info & memory allocation
only done separately for payload. (Animesh)

v16:
- No need for out_regs label in i915_driver_load(), so removed it.
- Changed the firmware name as skl_dmc_ver1.bin, followed naming convention <platform>_dmc_<api-version>.bin (Animesh)

Issue: VIZ-2569
Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/Makefile    |   3 +-
 drivers/gpu/drm/i915/i915_dma.c  |  11 +-
 drivers/gpu/drm/i915/i915_drv.c  |  20 +++
 drivers/gpu/drm/i915/i915_drv.h  |  17 ++
 drivers/gpu/drm/i915/intel_csr.c | 367 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |   5 +
 6 files changed, 420 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_csr.c

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index a69002e..5238deb 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -12,7 +12,8 @@  i915-y := i915_drv.o \
           i915_suspend.o \
 	  i915_sysfs.o \
 	  intel_pm.o \
-	  intel_runtime_pm.o
+	  intel_runtime_pm.o \
+	  intel_csr.o
 
 i915-$(CONFIG_COMPAT)   += i915_ioc32.o
 i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e44116f..a238889 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -816,6 +816,7 @@  int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	spin_lock_init(&dev_priv->mmio_flip_lock);
 	mutex_init(&dev_priv->dpio_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
+	mutex_init(&dev_priv->csr_lock);
 
 	intel_pm_setup(dev);
 
@@ -861,9 +862,12 @@  int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	intel_uncore_init(dev);
 
+	/* Load CSR Firmware for SKL */
+	intel_csr_ucode_init(dev);
+
 	ret = i915_gem_gtt_init(dev);
 	if (ret)
-		goto out_regs;
+		goto out_freecsr;
 
 	/* WARNING: Apparently we must kick fbdev drivers before vgacon,
 	 * otherwise the vga fbdev driver falls over. */
@@ -1033,7 +1037,8 @@  out_mtrrfree:
 	io_mapping_free(dev_priv->gtt.mappable);
 out_gtt:
 	i915_global_gtt_cleanup(dev);
-out_regs:
+out_freecsr:
+	intel_csr_ucode_fini(dev);
 	intel_uncore_fini(dev);
 	pci_iounmap(dev->pdev, dev_priv->regs);
 put_bridge:
@@ -1113,6 +1118,8 @@  int i915_driver_unload(struct drm_device *dev)
 	mutex_unlock(&dev->struct_mutex);
 	i915_gem_cleanup_stolen(dev);
 
+	intel_csr_ucode_fini(dev);
+
 	intel_teardown_gmbus(dev);
 	intel_teardown_mchbar(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c3fdbb0..acd0e2b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -556,6 +556,26 @@  void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
 	cancel_delayed_work_sync(&dev_priv->hotplug_reenable_work);
 }
 
+void i915_firmware_load_error_print(const char *fw_path, int err)
+{
+	DRM_ERROR("failed to load firmware %s (%d)\n", fw_path, err);
+
+	/*
+	 * If the reason is not known assume -ENOENT since that's the most
+	 * usual failure mode.
+	 */
+	if (!err)
+		err = -ENOENT;
+
+	if (!(IS_BUILTIN(CONFIG_DRM_I915) && err == -ENOENT))
+		return;
+
+	DRM_ERROR(
+	  "The driver is built-in, so to load the firmware you need to\n"
+	  "include it either in the kernel (see CONFIG_EXTRA_FIRMWARE) or\n"
+	  "in your initrd/initramfs image.\n");
+}
+
 static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 47be4a5..90e47a9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -667,6 +667,15 @@  struct intel_uncore {
 #define for_each_fw_domain(domain__, dev_priv__, i__) \
 	for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
 
+struct intel_csr {
+	const char *fw_path;
+	__be32 *dmc_payload;
+	uint32_t dmc_fw_size;
+	uint32_t mmio_count;
+	uint32_t mmioaddr[8];
+	uint32_t mmiodata[8];
+};
+
 #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
 	func(is_mobile) sep \
 	func(is_i85x) sep \
@@ -1573,6 +1582,11 @@  struct drm_i915_private {
 
 	struct i915_virtual_gpu vgpu;
 
+	struct intel_csr csr;
+
+	/* Display CSR-related protection */
+	struct mutex csr_lock;
+
 	struct intel_gmbus gmbus[GMBUS_NUM_PINS];
 
 	/** gmbus_mutex protects against concurrent usage of the single hw gmbus
@@ -2425,6 +2439,8 @@  struct drm_i915_cmd_table {
 #define HAS_RC6(dev)		(INTEL_INFO(dev)->gen >= 6)
 #define HAS_RC6p(dev)		(INTEL_INFO(dev)->gen == 6 || IS_IVYBRIDGE(dev))
 
+#define HAS_CSR(dev)	(IS_SKYLAKE(dev))
+
 #define INTEL_PCH_DEVICE_ID_MASK		0xff00
 #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
 #define INTEL_PCH_CPT_DEVICE_ID_TYPE		0x1c00
@@ -2515,6 +2531,7 @@  extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
 extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
 int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
 void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
+void i915_firmware_load_error_print(const char *fw_path, int err);
 
 /* i915_irq.c */
 void i915_queue_hangcheck(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
new file mode 100644
index 0000000..f5fa574
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -0,0 +1,367 @@ 
+/*
+ * Copyright © 2014 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 <linux/firmware.h>
+#include "i915_drv.h"
+#include "i915_reg.h"
+
+#define I915_CSR_SKL "i915/skl_dmc_ver1.bin"
+
+MODULE_FIRMWARE(I915_CSR_SKL);
+
+/*
+* SKL CSR registers for DC5 and DC6
+*/
+#define CSR_PROGRAM_BASE		0x80000
+#define CSR_SSP_BASE_ADDR_GEN9		0x00002FC0
+#define CSR_HTP_ADDR_SKL		0x00500034
+#define CSR_SSP_BASE			0x8F074
+#define CSR_HTP_SKL			0x8F004
+#define CSR_LAST_WRITE			0x8F034
+#define CSR_LAST_WRITE_VALUE		0xc003b400
+/* MMIO address range for CSR program (0x80000 - 0x82FFF) */
+#define CSR_MAX_FW_SIZE			0x2FFF
+#define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
+#define CSR_MMIO_START_RANGE	0x80000
+#define CSR_MMIO_END_RANGE		0x8FFFF
+
+struct intel_css_header {
+	/* 0x09 for DMC */
+	uint32_t module_type;
+
+	/* Includes the DMC specific header in dwords */
+	uint32_t header_len;
+
+	/* always value would be 0x10000 */
+	uint32_t header_ver;
+
+	/* Not used */
+	uint32_t module_id;
+
+	/* Not used */
+	uint32_t module_vendor;
+
+	/* in YYYYMMDD format */
+	uint32_t date;
+
+	/* Size in dwords (CSS_Headerlen + PackageHeaderLen + dmc FWsLen)/4 */
+	uint32_t size;
+
+	/* Not used */
+	uint32_t key_size;
+
+	/* Not used */
+	uint32_t modulus_size;
+
+	/* Not used */
+	uint32_t exponent_size;
+
+	/* Not used */
+	uint32_t reserved1[12];
+
+	/* Major Minor */
+	uint32_t version;
+
+	/* Not used */
+	uint32_t reserved2[8];
+
+	/* Not used */
+	uint32_t kernel_header_info;
+} __packed;
+
+struct intel_fw_info {
+	uint16_t reserved1;
+
+	/* Stepping (A, B, C, ..., *). * is a wildcard */
+	char stepping;
+
+	/* Sub-stepping (0, 1, ..., *). * is a wildcard */
+	char substepping;
+
+	uint32_t offset;
+	uint32_t reserved2;
+} __packed;
+
+struct intel_package_header {
+	/* DMC container header length in dwords */
+	unsigned char header_len;
+
+	/* always value would be 0x01 */
+	unsigned char header_ver;
+
+	unsigned char reserved[10];
+
+	/* Number of valid entries in the FWInfo array below */
+	uint32_t num_entries;
+
+	struct intel_fw_info fw_info[20];
+} __packed;
+
+struct intel_dmc_header {
+	/* always value would be 0x40403E3E */
+	uint32_t signature;
+
+	/* DMC binary header length */
+	unsigned char header_len;
+
+	/* 0x01 */
+	unsigned char header_ver;
+
+	/* Reserved */
+	uint16_t dmcc_ver;
+
+	/* Major, Minor */
+	uint32_t	project;
+
+	/* Firmware program size (excluding header) in dwords */
+	uint32_t	fw_size;
+
+	/* Major Minor version */
+	uint32_t fw_version;
+
+	/* Number of valid MMIO cycles present. */
+	uint32_t mmio_count;
+
+	/* MMIO address */
+	uint32_t mmioaddr[8];
+
+	/* MMIO data */
+	uint32_t mmiodata[8];
+
+	/* FW filename  */
+	unsigned char dfile[32];
+
+	uint32_t reserved1[2];
+} __packed;
+
+struct stepping_info {
+	char stepping;
+	char substepping;
+};
+
+static const struct stepping_info skl_stepping_info[] = {
+		{'A', '0'}, {'B', '0'}, {'C', '0'},
+		{'D', '0'}, {'E', '0'}, {'F', '0'},
+		{'G', '0'}, {'H', '0'}, {'I', '0'}
+};
+
+static char intel_get_stepping(struct drm_device *dev)
+{
+	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
+			ARRAY_SIZE(skl_stepping_info)))
+		return skl_stepping_info[dev->pdev->revision].stepping;
+	else
+		return -ENODATA;
+}
+
+static char intel_get_substepping(struct drm_device *dev)
+{
+	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
+			ARRAY_SIZE(skl_stepping_info)))
+		return skl_stepping_info[dev->pdev->revision].substepping;
+	else
+		return -ENODATA;
+}
+
+void intel_csr_load_program(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	__be32 *payload = dev_priv->csr.dmc_payload;
+	uint32_t i, fw_size;
+
+	if (!IS_GEN9(dev)) {
+		DRM_ERROR("No CSR support available for this platform\n");
+		return;
+	}
+
+	mutex_lock(&dev_priv->csr_lock);
+	fw_size = dev_priv->csr.dmc_fw_size;
+	for (i = 0; i < fw_size; i++)
+		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
+			(u32 __force)payload[i]);
+
+	for (i = 0; i < dev_priv->csr.mmio_count; i++) {
+		I915_WRITE(dev_priv->csr.mmioaddr[i],
+			dev_priv->csr.mmiodata[i]);
+	}
+	mutex_unlock(&dev_priv->csr_lock);
+}
+
+static void finish_csr_load(const struct firmware *fw, void *context)
+{
+	struct drm_i915_private *dev_priv = context;
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_css_header *css_header;
+	struct intel_package_header *package_header;
+	struct intel_dmc_header *dmc_header;
+	struct intel_csr *csr = &dev_priv->csr;
+	char stepping = intel_get_stepping(dev);
+	char substepping = intel_get_substepping(dev);
+	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
+	uint32_t i;
+	__be32 *dmc_payload;
+
+	if (!fw) {
+		i915_firmware_load_error_print(csr->fw_path, 0);
+		goto out;
+	}
+
+	if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
+		DRM_ERROR("Unknown stepping info, firmware loading failed\n");
+		goto out;
+	}
+
+	/* Extract CSS Header information*/
+	css_header = (struct intel_css_header *)fw->data;
+	if (sizeof(struct intel_css_header) !=
+		(css_header->header_len * 4)) {
+		DRM_ERROR("Firmware has wrong CSS header length %u bytes\n",
+			(css_header->header_len * 4));
+		goto out;
+	}
+	readcount += sizeof(struct intel_css_header);
+
+	/* Extract Package Header information*/
+	package_header = (struct intel_package_header *)
+					&fw->data[readcount];
+	if (sizeof(struct intel_package_header) !=
+		(package_header->header_len * 4)) {
+		DRM_ERROR("Firmware has wrong package header length %u bytes\n",
+			(package_header->header_len * 4));
+		goto out;
+	}
+	readcount += sizeof(struct intel_package_header);
+
+	/* Search for dmc_offset to find firware binary. */
+	for (i = 0; i < package_header->num_entries; i++) {
+		if (package_header->fw_info[i].substepping == '*' &&
+			stepping == package_header->fw_info[i].stepping) {
+			dmc_offset = package_header->fw_info[i].offset;
+			break;
+		} else if (stepping == package_header->fw_info[i].stepping &&
+			substepping == package_header->fw_info[i].substepping) {
+			dmc_offset = package_header->fw_info[i].offset;
+			break;
+		} else if (package_header->fw_info[i].stepping == '*' &&
+			package_header->fw_info[i].substepping == '*')
+			dmc_offset = package_header->fw_info[i].offset;
+	}
+	if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
+		DRM_ERROR("Firmware not supported for %c stepping\n", stepping);
+		goto out;
+	}
+	readcount += dmc_offset;
+
+	/* Extract dmc_header information. */
+	dmc_header = (struct intel_dmc_header *)&fw->data[readcount];
+	if (sizeof(struct intel_dmc_header) != (dmc_header->header_len)) {
+		DRM_ERROR("Firmware has wrong dmc header length %u bytes\n",
+				(dmc_header->header_len));
+		goto out;
+	}
+	readcount += sizeof(struct intel_dmc_header);
+
+	/* Cache the dmc header info. */
+	if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) {
+		DRM_ERROR("Firmware has wrong mmio count %u\n",
+						dmc_header->mmio_count);
+		goto out;
+	}
+	csr->mmio_count = dmc_header->mmio_count;
+	for (i = 0; i < dmc_header->mmio_count; i++) {
+		if (dmc_header->mmioaddr[i] < CSR_MMIO_START_RANGE &&
+			dmc_header->mmioaddr[i] > CSR_MMIO_END_RANGE) {
+			DRM_ERROR(" Firmware has wrong mmio address 0x%x\n",
+						dmc_header->mmioaddr[i]);
+			goto out;
+		}
+		csr->mmioaddr[i] = dmc_header->mmioaddr[i];
+		csr->mmiodata[i] = dmc_header->mmiodata[i];
+	}
+
+	/* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
+	nbytes = dmc_header->fw_size * 4;
+	if (nbytes > CSR_MAX_FW_SIZE) {
+		DRM_ERROR("CSR firmware too big (%u) bytes\n", nbytes);
+		goto out;
+	}
+	csr->dmc_fw_size = dmc_header->fw_size;
+
+	csr->dmc_payload = kmalloc(nbytes, GFP_KERNEL);
+	if (!csr->dmc_payload) {
+		DRM_ERROR("Memory allocation failed for dmc payload\n");
+		goto out;
+	}
+
+	dmc_payload = csr->dmc_payload;
+	for (i = 0; i < dmc_header->fw_size; i++) {
+		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
+		/*
+		 * The firmware payload is an array of 32 bit words stored in
+		 * little-endian format in the firmware image and programmed
+		 * as 32 bit big-endian format to memory.
+		 */
+		dmc_payload[i] = cpu_to_be32(*tmp);
+	}
+
+	/* load csr program during system boot, as needed for DC states */
+	intel_csr_load_program(dev);
+out:
+	release_firmware(fw);
+}
+
+void intel_csr_ucode_init(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_csr *csr = &dev_priv->csr;
+	int ret;
+
+	if (!HAS_CSR(dev))
+		return;
+
+	if (IS_SKYLAKE(dev))
+		csr->fw_path = I915_CSR_SKL;
+	else {
+		DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
+		return;
+	}
+
+	/* CSR supported for platform, load firmware */
+	ret = request_firmware_nowait(THIS_MODULE, true, csr->fw_path,
+				&dev_priv->dev->pdev->dev,
+				GFP_KERNEL, dev_priv,
+				finish_csr_load);
+	if (ret)
+		i915_firmware_load_error_print(csr->fw_path, ret);
+
+}
+
+void intel_csr_ucode_fini(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!HAS_CSR(dev))
+		return;
+
+	kfree(dev_priv->csr.dmc_payload);
+}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7a0aa24..f3a2d88 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1062,6 +1062,11 @@  void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
 unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
 				     struct drm_i915_gem_object *obj);
 
+/* intel_csr.c */
+void intel_csr_ucode_init(struct drm_device *dev);
+void intel_csr_load_program(struct drm_device *dev);
+void intel_csr_ucode_fini(struct drm_device *dev);
+
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
 bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,

Comments

On Thu, Apr 16, 2015 at 02:22:07PM +0530, Animesh Manna wrote:
> +#define I915_CSR_SKL "i915/skl_dmc_ver1.bin"

This needs to be skl_dmc_ver4.bin, because that's the version we made
public.
On to, 2015-04-16 at 14:22 +0530, Animesh Manna wrote:
> [...]
> +#include <linux/firmware.h>
> +#include "i915_drv.h"
> +#include "i915_reg.h"
> +
> +#define I915_CSR_SKL "i915/skl_dmc_ver1.bin"

The latest version on the FW download page is skl_dmc_ver4.bin as Damien
pointed out and skl_dmc_ver1.bin is not available there. So why did you
choose ver1? If ver4 is not compatible with the current driver code,
then ver1 must be made available too on the download page, otherwise we
should use here ver4.

The patch otherwise looks ok to me.

--Imre
On to, 2015-04-16 at 17:29 +0530, Animesh Manna wrote:
> + Jesse, Rodrigo
> 
> On 04/16/2015 02:51 PM, Imre Deak wrote:
> > On to, 2015-04-16 at 14:22 +0530, Animesh Manna wrote:
> >> [...]
> >> +#include <linux/firmware.h>
> >> +#include "i915_drv.h"
> >> +#include "i915_reg.h"
> >> +
> >> +#define I915_CSR_SKL "i915/skl_dmc_ver1.bin"
> > The latest version on the FW download page is skl_dmc_ver4.bin as Damien
> > pointed out and skl_dmc_ver1.bin is not available there. So why did you
> > choose ver1? If ver4 is not compatible with the current driver code,
> > then ver1 must be made available too on the download page, otherwise we
> > should use here ver4.
> >
> > The patch otherwise looks ok to me.
> >
> > --Imre
>
> Yes, 01.org will be updated with skl_dmc_ver1.bin. Thinking of  future
> scaling it is decided that we will follow the naming convention
> <platform>_dmc_<api-version>.bin and this will be a symbolic link of
> the actual firmware.
>
> Will be requesting firmware team to release firmware in
> <platform>_dmc_<api-version>_<minor-version>.bin Here in
> skl_dmc_ver4.bin api version is 1.0 and minor version is 4, so changed
> the file name to skl_dmc_ver1.bin.

I just repeat here Damien, but I don't think there is a need to
complicate things with symlinks. IMO I915_CSR_SKL should point to the
exact version of the firmware, not just to a symlink with the API
version. The driver will be tested with that particular firmware version
and so the driver writers should be in full control of what version will
be loaded with a given driver version.

--Imre
+ Jesse, Rodrigo

On 04/16/2015 02:51 PM, Imre Deak wrote:
> On to, 2015-04-16 at 14:22 +0530, Animesh Manna wrote:
>> [...]
>> +#include <linux/firmware.h>
>> +#include "i915_drv.h"
>> +#include "i915_reg.h"
>> +
>> +#define I915_CSR_SKL "i915/skl_dmc_ver1.bin"
> The latest version on the FW download page is skl_dmc_ver4.bin as Damien
> pointed out and skl_dmc_ver1.bin is not available there. So why did you
> choose ver1? If ver4 is not compatible with the current driver code,
> then ver1 must be made available too on the download page, otherwise we
> should use here ver4.
>
> The patch otherwise looks ok to me.
>
> --Imre
>
Yes, 01.org will be updated with skl_dmc_ver1.bin. Thinking of  future scaling it is decided that we will follow the naming convention
<platform>_dmc_<api-version>.bin and this will be a symbolic link of the actual firmware.

Will be requesting firmware team to release firmware in <platform>_dmc_<api-version>_<minor-version>.bin
Here in skl_dmc_ver4.bin api version is 1.0 and minor version is 4, so changed the file name to skl_dmc_ver1.bin.

Regards,
Animesh
On 4/16/2015 4:55 PM, Imre Deak wrote:
> On to, 2015-04-16 at 17:29 +0530, Animesh Manna wrote:
>> + Jesse, Rodrigo
>>
>> On 04/16/2015 02:51 PM, Imre Deak wrote:
>>> On to, 2015-04-16 at 14:22 +0530, Animesh Manna wrote:
>>>> [...]
>>>> +#include <linux/firmware.h>
>>>> +#include "i915_drv.h"
>>>> +#include "i915_reg.h"
>>>> +
>>>> +#define I915_CSR_SKL "i915/skl_dmc_ver1.bin"
>>> The latest version on the FW download page is skl_dmc_ver4.bin as Damien
>>> pointed out and skl_dmc_ver1.bin is not available there. So why did you
>>> choose ver1? If ver4 is not compatible with the current driver code,
>>> then ver1 must be made available too on the download page, otherwise we
>>> should use here ver4.
>>>
>>> The patch otherwise looks ok to me.
>>>
>>> --Imre
>> Yes, 01.org will be updated with skl_dmc_ver1.bin. Thinking of  future
>> scaling it is decided that we will follow the naming convention
>> <platform>_dmc_<api-version>.bin and this will be a symbolic link of
>> the actual firmware.
>>
>> Will be requesting firmware team to release firmware in
>> <platform>_dmc_<api-version>_<minor-version>.bin Here in
>> skl_dmc_ver4.bin api version is 1.0 and minor version is 4, so changed
>> the file name to skl_dmc_ver1.bin.
> I just repeat here Damien, but I don't think there is a need to
> complicate things with symlinks. IMO I915_CSR_SKL should point to the
> exact version of the firmware, not just to a symlink with the API
> version. The driver will be tested with that particular firmware version
> and so the driver writers should be in full control of what version will
> be loaded with a given driver version.
>
> --Imre

Hi Imre,

Agree, if we want to support only latest firmware for a particular platform we do not need symlink
but during discussion found that if user want to rollback the newest firmware for same API version
which can be achievable by using symlink and introducing api-version and minor-version. Anyways I am
ok with both the approaches.

Adding Jessi also,

Please suggest to proceed further.

Regards,
Animesh

>
On Thu, 2015-04-16 at 19:53 +0530, Animesh Manna wrote:
> 
> On 4/16/2015 4:55 PM, Imre Deak wrote:
> > On to, 2015-04-16 at 17:29 +0530, Animesh Manna wrote:
> >> + Jesse, Rodrigo
> >>
> >> On 04/16/2015 02:51 PM, Imre Deak wrote:
> >>> On to, 2015-04-16 at 14:22 +0530, Animesh Manna wrote:
> >>>> [...]
> >>>> +#include <linux/firmware.h>
> >>>> +#include "i915_drv.h"
> >>>> +#include "i915_reg.h"
> >>>> +
> >>>> +#define I915_CSR_SKL "i915/skl_dmc_ver1.bin"
> >>> The latest version on the FW download page is skl_dmc_ver4.bin as Damien
> >>> pointed out and skl_dmc_ver1.bin is not available there. So why did you
> >>> choose ver1? If ver4 is not compatible with the current driver code,
> >>> then ver1 must be made available too on the download page, otherwise we
> >>> should use here ver4.
> >>>
> >>> The patch otherwise looks ok to me.
> >>>
> >>> --Imre
> >> Yes, 01.org will be updated with skl_dmc_ver1.bin. Thinking of  future
> >> scaling it is decided that we will follow the naming convention
> >> <platform>_dmc_<api-version>.bin and this will be a symbolic link of
> >> the actual firmware.
> >>
> >> Will be requesting firmware team to release firmware in
> >> <platform>_dmc_<api-version>_<minor-version>.bin Here in
> >> skl_dmc_ver4.bin api version is 1.0 and minor version is 4, so changed
> >> the file name to skl_dmc_ver1.bin.
> > I just repeat here Damien, but I don't think there is a need to
> > complicate things with symlinks. IMO I915_CSR_SKL should point to the
> > exact version of the firmware, not just to a symlink with the API
> > version. The driver will be tested with that particular firmware version
> > and so the driver writers should be in full control of what version will
> > be loaded with a given driver version.
>
> Agree, if we want to support only latest firmware for a particular
> platform we do not need symlink but during discussion found that if
> user want to rollback the newest firmware for same API version which
> can be achievable by using symlink and introducing api-version and
> minor-version. Anyways I am ok with both the approaches.

The only reason I can see for rolling back would be a regression. Fixing
that would need to go through the usual bug reporting flow and eventual
firmware and driver update. A symlink could anyway be created - as a
kind of hack - to force loading a different firmware version, but making
this the regular way would only allow for confusion about which firmware
version is actually loaded.

--Imre
On to, 2015-04-16 at 14:22 +0530, Animesh Manna wrote:
> [...]
> +
> +#define I915_CSR_SKL "i915/skl_dmc_ver1.bin"

Animesh, does this firmware loading patchset work with skl_dmc_ver4.bin?
If so please re-send this patch with the above pointing to
i915/skl_dmc_ver4.bin (since that is the published firmware version).
Note that we can do this change now regardless of whether we decide
later to use a symlink or not.

--Imre