[PATCH 01/10] drm/i915/dmc: use kernel types

[PATCH 01/10] drm/i915/dmc: use kernel types

[Lucas De Marchi]

Change all fields in intel_package_header and intel_dmc_header whose
meaning are 1-byte numbers to use u8.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 4527b9662330..b05e7a6aebc7 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -130,12 +130,12 @@ struct intel_fw_info {
 
 struct intel_package_header {
 	/* DMC container header length in dwords */
-	unsigned char header_len;
+	u8 header_len;
 
 	/* always value would be 0x01 */
-	unsigned char header_ver;
+	u8 header_ver;
 
-	unsigned char reserved[10];
+	u8 reserved[10];
 
 	/* Number of valid entries in the FWInfo array below */
 	u32 num_entries;
@@ -148,10 +148,10 @@ struct intel_dmc_header {
 	u32 signature;
 
 	/* DMC binary header length */
-	unsigned char header_len;
+	u8 header_len;
 
 	/* 0x01 */
-	unsigned char header_ver;
+	u8 header_ver;
 
 	/* Reserved */
 	u16 dmcc_ver;
-- 
2.21.0

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

[PATCH 02/10] drm/i915/dmc: extract fw_info and table walk from intel_package_header

[Lucas De Marchi]

Move fw_info out of struct intel_package_header to allow it to grow more
easily in future. To make a cleaner move, let's also extract a function to
search the header for the dmc_offset.

While reviewing this code I wondered why we continued the search even
after finding a suitable firmware. Add a comment to explain we will
continue to try to find a more specific firmware version, even if this
is not required by the spec.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 68 ++++++++++++++++++++++++--------
 1 file changed, 51 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index b05e7a6aebc7..01ae356e69cc 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -70,6 +70,7 @@ MODULE_FIRMWARE(SKL_CSR_PATH);
 MODULE_FIRMWARE(BXT_CSR_PATH);
 
 #define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
+#define PACKAGE_MAX_FW_INFO_ENTRIES	20
 
 struct intel_css_header {
 	/* 0x09 for DMC */
@@ -139,8 +140,6 @@ struct intel_package_header {
 
 	/* Number of valid entries in the FWInfo array below */
 	u32 num_entries;
-
-	struct intel_fw_info fw_info[20];
 } __packed;
 
 struct intel_dmc_header {
@@ -292,6 +291,46 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv)
 	gen9_set_dc_state_debugmask(dev_priv);
 }
 
+/*
+ * Search fw_info table for dmc_offset to find firmware binary: num_entries is
+ * already sanitized.
+ */
+static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
+			      unsigned int num_entries,
+			      const struct stepping_info *si)
+{
+	u32 dmc_offset = CSR_DEFAULT_FW_OFFSET;
+	unsigned int i;
+
+	for (i = 0; i < num_entries; i++) {
+		if (fw_info[i].substepping == '*' &&
+		    si->stepping == fw_info[i].stepping) {
+			dmc_offset = fw_info[i].offset;
+			break;
+		}
+
+		if (si->stepping == fw_info[i].stepping &&
+		    si->substepping == fw_info[i].substepping) {
+			dmc_offset = fw_info[i].offset;
+			break;
+		}
+
+		if (fw_info[i].stepping == '*' &&
+		    fw_info[i].substepping == '*') {
+			/*
+			 * In theory we should stop the search as generic
+			 * entries should alwas come after the more specific
+			 * ones, but let's continue to make sure to work even
+			 * with "broken" firmwares. If we don't find a more
+			 * specific one, then we use this entry
+			 */
+			dmc_offset = fw_info[i].offset;
+		}
+	}
+
+	return dmc_offset;
+}
+
 static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 			 const struct firmware *fw)
 {
@@ -300,7 +339,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 	struct intel_dmc_header *dmc_header;
 	struct intel_csr *csr = &dev_priv->csr;
 	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
-	u32 dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
+	u32 dmc_offset, num_entries, readcount = 0, nbytes;
 	u32 i;
 	u32 *dmc_payload;
 
@@ -342,27 +381,22 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 			  (package_header->header_len * 4));
 		return NULL;
 	}
+
 	readcount += sizeof(struct intel_package_header);
+	num_entries = package_header->num_entries;
+	if (WARN_ON(package_header->num_entries > PACKAGE_MAX_FW_INFO_ENTRIES))
+		num_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
 
-	/* Search for dmc_offset to find firware binary. */
-	for (i = 0; i < package_header->num_entries; i++) {
-		if (package_header->fw_info[i].substepping == '*' &&
-		    si->stepping == package_header->fw_info[i].stepping) {
-			dmc_offset = package_header->fw_info[i].offset;
-			break;
-		} else if (si->stepping == package_header->fw_info[i].stepping &&
-			   si->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;
-	}
+	dmc_offset = find_dmc_fw_offset((struct intel_fw_info *)
+					&fw->data[readcount], num_entries, si);
 	if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
 		DRM_ERROR("DMC firmware not supported for %c stepping\n",
 			  si->stepping);
 		return NULL;
 	}
+	/* we always have space for PACKAGE_MAX_FW_INFO_ENTRIES */
+	readcount += PACKAGE_MAX_FW_INFO_ENTRIES * sizeof(struct intel_fw_info);
+
 	/* Convert dmc_offset into number of bytes. By default it is in dwords*/
 	dmc_offset *= 4;
 	readcount += dmc_offset;
-- 
2.21.0

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

[PATCH 03/10] drm/i915/dmc: add support for package_header with version 2

[Lucas De Marchi]

The only meaninful change is that it supports up to 32 fw_info entries
rather than the previous max=20.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 35 ++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 01ae356e69cc..b9651fbe4c25 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -71,6 +71,7 @@ MODULE_FIRMWARE(BXT_CSR_PATH);
 
 #define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
 #define PACKAGE_MAX_FW_INFO_ENTRIES	20
+#define PACKAGE_V2_MAX_FW_INFO_ENTRIES	32
 
 struct intel_css_header {
 	/* 0x09 for DMC */
@@ -133,7 +134,7 @@ struct intel_package_header {
 	/* DMC container header length in dwords */
 	u8 header_len;
 
-	/* always value would be 0x01 */
+	/* 0x01, 0x02 */
 	u8 header_ver;
 
 	u8 reserved[10];
@@ -339,7 +340,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 	struct intel_dmc_header *dmc_header;
 	struct intel_csr *csr = &dev_priv->csr;
 	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
-	u32 dmc_offset, num_entries, readcount = 0, nbytes;
+	u32 dmc_offset, num_entries, max_entries, readcount = 0, nbytes;
 	u32 i;
 	u32 *dmc_payload;
 
@@ -374,18 +375,30 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 	/* Extract Package Header information*/
 	package_header = (struct intel_package_header *)
 		&fw->data[readcount];
-	if (sizeof(struct intel_package_header) !=
-	    (package_header->header_len * 4)) {
+
+	readcount += sizeof(struct intel_package_header);
+
+	if (package_header->header_ver == 1) {
+		max_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
+	} else if (package_header->header_ver == 2) {
+		max_entries = PACKAGE_V2_MAX_FW_INFO_ENTRIES;
+	} else {
+		DRM_ERROR("DMC firmware has unknown header version %u\n",
+			  package_header->header_ver);
+		return NULL;
+	}
+
+	if (package_header->header_len * 4 !=
+	    sizeof(struct intel_package_header) +
+	    max_entries * sizeof(struct intel_fw_info)) {
 		DRM_ERROR("DMC firmware has wrong package header length "
-			  "(%u bytes)\n",
-			  (package_header->header_len * 4));
+			  "(%u bytes)\n", package_header->header_len * 4);
 		return NULL;
 	}
 
-	readcount += sizeof(struct intel_package_header);
 	num_entries = package_header->num_entries;
-	if (WARN_ON(package_header->num_entries > PACKAGE_MAX_FW_INFO_ENTRIES))
-		num_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
+	if (WARN_ON(package_header->num_entries > max_entries))
+		num_entries = max_entries;
 
 	dmc_offset = find_dmc_fw_offset((struct intel_fw_info *)
 					&fw->data[readcount], num_entries, si);
@@ -394,9 +407,9 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 			  si->stepping);
 		return NULL;
 	}
-	/* we always have space for PACKAGE_MAX_FW_INFO_ENTRIES */
-	readcount += PACKAGE_MAX_FW_INFO_ENTRIES * sizeof(struct intel_fw_info);
 
+	/* we always have space for max_entries, even if not all are used */
+	readcount += max_entries * sizeof(struct intel_fw_info);
 	/* Convert dmc_offset into number of bytes. By default it is in dwords*/
 	dmc_offset *= 4;
 	readcount += dmc_offset;
-- 
2.21.0

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

[PATCH 04/10] drm/i915/dmc: extract function to parse css header

[Lucas De Marchi]

Let's start splitting the parse function, making all of them return the
number of bytes parsed - different versions of the firmware header may
require different sizes for the structures.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 49 +++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index b9651fbe4c25..7e53bf576892 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -332,29 +332,16 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
 	return dmc_offset;
 }
 
-static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
-			 const struct firmware *fw)
+/* Return number of bytes parsed or 0 on error */
+static u32 parse_csr_fw_css(struct intel_csr *csr,
+			    struct intel_css_header *css_header)
 {
-	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;
-	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
-	u32 dmc_offset, num_entries, max_entries, readcount = 0, nbytes;
-	u32 i;
-	u32 *dmc_payload;
-
-	if (!fw)
-		return NULL;
-
-	/* 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("DMC firmware has wrong CSS header length "
 			  "(%u bytes)\n",
 			  (css_header->header_len * 4));
-		return NULL;
+		return 0;
 	}
 
 	if (csr->required_version &&
@@ -365,12 +352,36 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 			 CSR_VERSION_MINOR(css_header->version),
 			 CSR_VERSION_MAJOR(csr->required_version),
 			 CSR_VERSION_MINOR(csr->required_version));
-		return NULL;
+		return 0;
 	}
 
 	csr->version = css_header->version;
 
-	readcount += sizeof(struct intel_css_header);
+	return sizeof(struct intel_css_header);
+}
+
+static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
+			 const struct firmware *fw)
+{
+	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;
+	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
+	u32 dmc_offset, num_entries, max_entries, readcount = 0, nbytes;
+	u32 i, r;
+	u32 *dmc_payload;
+
+	if (!fw)
+		return NULL;
+
+	/* Extract CSS Header information*/
+	css_header = (struct intel_css_header *)fw->data;
+	r = parse_csr_fw_css(csr, css_header);
+	if (!r)
+		return NULL;
+
+	readcount += r;
 
 	/* Extract Package Header information*/
 	package_header = (struct intel_package_header *)
-- 
2.21.0

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

[PATCH 05/10] drm/i915/dmc: extract function to parse package_header

[Lucas De Marchi]

Like parse_csr_fw_css() this parses the package_header from firmware and
saves the relevant fields in the csr struct. In this function we also
lookup the fw_info we are interested in.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 94 +++++++++++++++++++-------------
 1 file changed, 55 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 7e53bf576892..b19742becb98 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -332,6 +332,56 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
 	return dmc_offset;
 }
 
+static u32
+parse_csr_fw_package(struct intel_csr *csr,
+		     const struct intel_package_header *package_header,
+		     const struct stepping_info *si)
+{
+	u32 num_entries, max_entries, dmc_offset, r;
+	const struct intel_fw_info *fw_info;
+
+	if (package_header->header_ver == 1) {
+		max_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
+	} else if (package_header->header_ver == 2) {
+		max_entries = PACKAGE_V2_MAX_FW_INFO_ENTRIES;
+	} else {
+		DRM_ERROR("DMC firmware has unknown header version %u\n",
+			  package_header->header_ver);
+		return 0;
+	}
+
+	if (package_header->header_len * 4 !=
+	    sizeof(struct intel_package_header) +
+	    max_entries * sizeof(struct intel_fw_info)) {
+		DRM_ERROR("DMC firmware has wrong package header length "
+			  "(%u bytes)\n", package_header->header_len * 4);
+		return 0;
+	}
+
+	num_entries = package_header->num_entries;
+	if (WARN_ON(package_header->num_entries > max_entries))
+		num_entries = max_entries;
+
+	fw_info = (const struct intel_fw_info *)
+		((u8 *)package_header + sizeof(*package_header));
+	dmc_offset = find_dmc_fw_offset(fw_info, num_entries, si);
+	if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
+		DRM_ERROR("DMC firmware not supported for %c stepping\n",
+			  si->stepping);
+		return 0;
+	}
+
+	r = sizeof(struct intel_package_header);
+
+	/* we always have space for max_entries, even if not all are used */
+	r += max_entries * sizeof(struct intel_fw_info);
+
+	/* dmc_offset is in dwords */
+	r += dmc_offset * 4;
+
+	return r;
+}
+
 /* Return number of bytes parsed or 0 on error */
 static u32 parse_csr_fw_css(struct intel_csr *csr,
 			    struct intel_css_header *css_header)
@@ -368,7 +418,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 	struct intel_dmc_header *dmc_header;
 	struct intel_csr *csr = &dev_priv->csr;
 	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
-	u32 dmc_offset, num_entries, max_entries, readcount = 0, nbytes;
+	u32 readcount = 0, nbytes;
 	u32 i, r;
 	u32 *dmc_payload;
 
@@ -384,46 +434,12 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 	readcount += r;
 
 	/* Extract Package Header information*/
-	package_header = (struct intel_package_header *)
-		&fw->data[readcount];
-
-	readcount += sizeof(struct intel_package_header);
-
-	if (package_header->header_ver == 1) {
-		max_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
-	} else if (package_header->header_ver == 2) {
-		max_entries = PACKAGE_V2_MAX_FW_INFO_ENTRIES;
-	} else {
-		DRM_ERROR("DMC firmware has unknown header version %u\n",
-			  package_header->header_ver);
-		return NULL;
-	}
-
-	if (package_header->header_len * 4 !=
-	    sizeof(struct intel_package_header) +
-	    max_entries * sizeof(struct intel_fw_info)) {
-		DRM_ERROR("DMC firmware has wrong package header length "
-			  "(%u bytes)\n", package_header->header_len * 4);
-		return NULL;
-	}
-
-	num_entries = package_header->num_entries;
-	if (WARN_ON(package_header->num_entries > max_entries))
-		num_entries = max_entries;
-
-	dmc_offset = find_dmc_fw_offset((struct intel_fw_info *)
-					&fw->data[readcount], num_entries, si);
-	if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
-		DRM_ERROR("DMC firmware not supported for %c stepping\n",
-			  si->stepping);
+	package_header = (struct intel_package_header *)&fw->data[readcount];
+	r = parse_csr_fw_package(csr, package_header, si);
+	if (!r)
 		return NULL;
-	}
 
-	/* we always have space for max_entries, even if not all are used */
-	readcount += max_entries * sizeof(struct intel_fw_info);
-	/* Convert dmc_offset into number of bytes. By default it is in dwords*/
-	dmc_offset *= 4;
-	readcount += dmc_offset;
+	readcount += r;
 
 	/* Extract dmc_header information. */
 	dmc_header = (struct intel_dmc_header *)&fw->data[readcount];
-- 
2.21.0

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

[PATCH 06/10] drm/i915/dmc: extract function to parse dmc_header

[Lucas De Marchi]

Complete the extraction of functions to parse specific parts of the
firmware. The return of the function parse_csr_fw() is now redundant
since it already sets the dmc_payload field. Changing it is left for
later to avoid noise in the commit.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 102 ++++++++++++++++++-------------
 1 file changed, 60 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index b19742becb98..20dd4bd5feae 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -332,6 +332,61 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
 	return dmc_offset;
 }
 
+static u32 parse_csr_fw_dmc(struct intel_csr *csr,
+			    const struct intel_dmc_header *dmc_header)
+{
+	unsigned int i, payload_size;
+	u32 r;
+	u8 *payload;
+
+	if (sizeof(struct intel_dmc_header) != dmc_header->header_len) {
+		DRM_ERROR("DMC firmware has wrong dmc header length "
+			  "(%u bytes)\n",
+			  (dmc_header->header_len));
+		return 0;
+	}
+
+	/* Cache the dmc header info. */
+	if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) {
+		DRM_ERROR("DMC firmware has wrong mmio count %u\n",
+			  dmc_header->mmio_count);
+		return 0;
+	}
+
+	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("DMC firmware has wrong mmio address 0x%x\n",
+				  dmc_header->mmioaddr[i]);
+			return 0;
+		}
+		csr->mmioaddr[i] = _MMIO(dmc_header->mmioaddr[i]);
+		csr->mmiodata[i] = dmc_header->mmiodata[i];
+	}
+
+	/* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
+	payload_size = dmc_header->fw_size * 4;
+	if (payload_size > csr->max_fw_size) {
+		DRM_ERROR("DMC FW too big (%u bytes)\n", payload_size);
+		return 0;
+	}
+	csr->dmc_fw_size = dmc_header->fw_size;
+
+	csr->dmc_payload = kmalloc(payload_size, GFP_KERNEL);
+	if (!csr->dmc_payload) {
+		DRM_ERROR("Memory allocation failed for dmc payload\n");
+		return 0;
+	}
+
+	r = sizeof(struct intel_dmc_header);
+	payload = (u8 *)(dmc_header) + r;
+	memcpy(csr->dmc_payload, payload, payload_size);
+	r += payload_size;
+
+	return r;
+}
+
 static u32
 parse_csr_fw_package(struct intel_csr *csr,
 		     const struct intel_package_header *package_header,
@@ -418,9 +473,8 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 	struct intel_dmc_header *dmc_header;
 	struct intel_csr *csr = &dev_priv->csr;
 	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
-	u32 readcount = 0, nbytes;
-	u32 i, r;
-	u32 *dmc_payload;
+	u32 readcount = 0;
+	u32 r;
 
 	if (!fw)
 		return NULL;
@@ -443,47 +497,11 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 
 	/* 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("DMC firmware has wrong dmc header length "
-			  "(%u bytes)\n",
-			  (dmc_header->header_len));
-		return NULL;
-	}
-	readcount += sizeof(struct intel_dmc_header);
-
-	/* Cache the dmc header info. */
-	if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) {
-		DRM_ERROR("DMC firmware has wrong mmio count %u\n",
-			  dmc_header->mmio_count);
-		return NULL;
-	}
-	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("DMC firmware has wrong mmio address 0x%x\n",
-				  dmc_header->mmioaddr[i]);
-			return NULL;
-		}
-		csr->mmioaddr[i] = _MMIO(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("DMC FW too big (%u bytes)\n", nbytes);
-		return NULL;
-	}
-	csr->dmc_fw_size = dmc_header->fw_size;
-
-	dmc_payload = kmalloc(nbytes, GFP_KERNEL);
-	if (!dmc_payload) {
-		DRM_ERROR("Memory allocation failed for dmc payload\n");
+	r = parse_csr_fw_dmc(csr, dmc_header);
+	if (!r)
 		return NULL;
-	}
 
-	return memcpy(dmc_payload, &fw->data[readcount], nbytes);
+	return csr->dmc_payload;
 }
 
 static void intel_csr_runtime_pm_get(struct drm_i915_private *dev_priv)
-- 
2.21.0

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

[PATCH 07/10] drm/i915/dmc: add support to load dmc_header version 3

[Lucas De Marchi]

Main difference is that now there are up to 20 MMIOs that can be set and
a lot of noise due to the struct changing the fields in the middle.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |   4 +-
 drivers/gpu/drm/i915/intel_csr.c | 107 +++++++++++++++++++++++--------
 2 files changed, 83 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1ad3818d2676..04a6b59256fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -354,8 +354,8 @@ struct intel_csr {
 	u32 dmc_fw_size; /* dwords */
 	u32 version;
 	u32 mmio_count;
-	i915_reg_t mmioaddr[8];
-	u32 mmiodata[8];
+	i915_reg_t mmioaddr[20];
+	u32 mmiodata[20];
 	u32 dc_state;
 	u32 allowed_dc_mask;
 	intel_wakeref_t wakeref;
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 20dd4bd5feae..ad4ee55a8c5e 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -72,6 +72,8 @@ MODULE_FIRMWARE(BXT_CSR_PATH);
 #define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
 #define PACKAGE_MAX_FW_INFO_ENTRIES	20
 #define PACKAGE_V2_MAX_FW_INFO_ENTRIES	32
+#define DMC_V1_MAX_MMIO_COUNT		8
+#define DMC_V3_MAX_MMIO_COUNT		20
 
 struct intel_css_header {
 	/* 0x09 for DMC */
@@ -143,7 +145,7 @@ struct intel_package_header {
 	u32 num_entries;
 } __packed;
 
-struct intel_dmc_header {
+struct intel_dmc_header_base {
 	/* always value would be 0x40403E3E */
 	u32 signature;
 
@@ -164,22 +166,47 @@ struct intel_dmc_header {
 
 	/* Major Minor version */
 	u32 fw_version;
+} __packed;
+
+struct intel_dmc_header_v1 {
+	struct intel_dmc_header_base base;
 
 	/* Number of valid MMIO cycles present. */
 	u32 mmio_count;
 
 	/* MMIO address */
-	u32 mmioaddr[8];
+	u32 mmioaddr[DMC_V1_MAX_MMIO_COUNT];
 
 	/* MMIO data */
-	u32 mmiodata[8];
+	u32 mmiodata[DMC_V1_MAX_MMIO_COUNT];
 
 	/* FW filename  */
-	unsigned char dfile[32];
+	char dfile[32];
 
 	u32 reserved1[2];
 } __packed;
 
+struct intel_dmc_header_v3 {
+	struct intel_dmc_header_base base;
+
+	/* DMC RAM start MMIO address */
+	u32 start_mmioaddr;
+
+	u32 reserved[9];
+
+	/* FW filename */
+	char dfile[32];
+
+	/* Number of valid MMIO cycles present. */
+	u32 mmio_count;
+
+	/* MMIO address */
+	u32 mmioaddr[DMC_V3_MAX_MMIO_COUNT];
+
+	/* MMIO data */
+	u32 mmiodata[DMC_V3_MAX_MMIO_COUNT];
+} __packed;
+
 struct stepping_info {
 	char stepping;
 	char substepping;
@@ -333,37 +360,67 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
 }
 
 static u32 parse_csr_fw_dmc(struct intel_csr *csr,
-			    const struct intel_dmc_header *dmc_header)
+			    const struct intel_dmc_header_base *dmc_header)
 {
-	unsigned int i, payload_size;
-	u32 r;
+	unsigned int header_len_bytes, dmc_header_size, payload_size, i;
+	const u32 *mmioaddr, *mmiodata;
+	u32 mmio_count, mmio_count_max;
 	u8 *payload;
 
-	if (sizeof(struct intel_dmc_header) != dmc_header->header_len) {
+	BUILD_BUG_ON(ARRAY_SIZE(csr->mmioaddr) < DMC_V3_MAX_MMIO_COUNT ||
+		     ARRAY_SIZE(csr->mmioaddr) < DMC_V1_MAX_MMIO_COUNT);
+
+	/* Cope with small differences between v1 and v3 */
+	if (dmc_header->header_ver == 3) {
+		const struct intel_dmc_header_v3 *v3 =
+			(const struct intel_dmc_header_v3 *)dmc_header;
+
+		mmioaddr = v3->mmioaddr;
+		mmiodata = v3->mmiodata;
+		mmio_count = v3->mmio_count;
+		mmio_count_max = DMC_V3_MAX_MMIO_COUNT;
+		/* header_len is in dwords */
+		header_len_bytes = dmc_header->header_len * 4;
+		dmc_header_size = sizeof(*v3);
+	} else if (dmc_header->header_ver == 1) {
+		const struct intel_dmc_header_v1 *v1 =
+			(const struct intel_dmc_header_v1 *)dmc_header;
+
+		mmioaddr = v1->mmioaddr;
+		mmiodata = v1->mmiodata;
+		mmio_count = v1->mmio_count;
+		mmio_count_max = DMC_V1_MAX_MMIO_COUNT;
+		header_len_bytes = dmc_header->header_len;
+		dmc_header_size = sizeof(*v1);
+	} else {
+		DRM_ERROR("Unknown DMC fw header version: %u\n",
+			  dmc_header->header_ver);
+		return 0;
+	}
+
+	if (header_len_bytes != dmc_header_size) {
 		DRM_ERROR("DMC firmware has wrong dmc header length "
-			  "(%u bytes)\n",
-			  (dmc_header->header_len));
+			  "(%u bytes)\n", header_len_bytes);
 		return 0;
 	}
 
 	/* Cache the dmc header info. */
-	if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) {
-		DRM_ERROR("DMC firmware has wrong mmio count %u\n",
-			  dmc_header->mmio_count);
+	if (mmio_count > mmio_count_max) {
+		DRM_ERROR("DMC firmware has wrong mmio count %u\n", mmio_count);
 		return 0;
 	}
 
-	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) {
+	for (i = 0; i < mmio_count; i++) {
+		if (mmioaddr[i] < CSR_MMIO_START_RANGE ||
+		    mmioaddr[i] > CSR_MMIO_END_RANGE) {
 			DRM_ERROR("DMC firmware has wrong mmio address 0x%x\n",
-				  dmc_header->mmioaddr[i]);
+				  mmioaddr[i]);
 			return 0;
 		}
-		csr->mmioaddr[i] = _MMIO(dmc_header->mmioaddr[i]);
-		csr->mmiodata[i] = dmc_header->mmiodata[i];
+		csr->mmioaddr[i] = _MMIO(mmioaddr[i]);
+		csr->mmiodata[i] = mmiodata[i];
 	}
+	csr->mmio_count = mmio_count;
 
 	/* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
 	payload_size = dmc_header->fw_size * 4;
@@ -379,12 +436,10 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
 		return 0;
 	}
 
-	r = sizeof(struct intel_dmc_header);
-	payload = (u8 *)(dmc_header) + r;
+	payload = (u8 *)(dmc_header) + header_len_bytes;
 	memcpy(csr->dmc_payload, payload, payload_size);
-	r += payload_size;
 
-	return r;
+	return header_len_bytes + payload_size;
 }
 
 static u32
@@ -470,7 +525,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 {
 	struct intel_css_header *css_header;
 	struct intel_package_header *package_header;
-	struct intel_dmc_header *dmc_header;
+	struct intel_dmc_header_base *dmc_header;
 	struct intel_csr *csr = &dev_priv->csr;
 	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
 	u32 readcount = 0;
@@ -496,7 +551,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 	readcount += r;
 
 	/* Extract dmc_header information. */
-	dmc_header = (struct intel_dmc_header *)&fw->data[readcount];
+	dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount];
 	r = parse_csr_fw_dmc(csr, dmc_header);
 	if (!r)
 		return NULL;
-- 
2.21.0

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

[PATCH 08/10] drm/i915/dmc: remove redundant return in parse_csr_fw()

[Lucas De Marchi]

parse_csr_fw() is responsible to set up several fields in struct intel_csr,
including the payload. We don't need to assign it again.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index ad4ee55a8c5e..7ff08de83cc6 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -520,7 +520,7 @@ static u32 parse_csr_fw_css(struct intel_csr *csr,
 	return sizeof(struct intel_css_header);
 }
 
-static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
+static void parse_csr_fw(struct drm_i915_private *dev_priv,
 			 const struct firmware *fw)
 {
 	struct intel_css_header *css_header;
@@ -532,13 +532,13 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 	u32 r;
 
 	if (!fw)
-		return NULL;
+		return;
 
 	/* Extract CSS Header information*/
 	css_header = (struct intel_css_header *)fw->data;
 	r = parse_csr_fw_css(csr, css_header);
 	if (!r)
-		return NULL;
+		return;
 
 	readcount += r;
 
@@ -546,17 +546,13 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 	package_header = (struct intel_package_header *)&fw->data[readcount];
 	r = parse_csr_fw_package(csr, package_header, si);
 	if (!r)
-		return NULL;
+		return;
 
 	readcount += r;
 
 	/* Extract dmc_header information. */
 	dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount];
-	r = parse_csr_fw_dmc(csr, dmc_header);
-	if (!r)
-		return NULL;
-
-	return csr->dmc_payload;
+	parse_csr_fw_dmc(csr, dmc_header);
 }
 
 static void intel_csr_runtime_pm_get(struct drm_i915_private *dev_priv)
@@ -584,8 +580,7 @@ static void csr_load_work_fn(struct work_struct *work)
 	csr = &dev_priv->csr;
 
 	request_firmware(&fw, dev_priv->csr.fw_path, &dev_priv->drm.pdev->dev);
-	if (fw)
-		dev_priv->csr.dmc_payload = parse_csr_fw(dev_priv, fw);
+	parse_csr_fw(dev_priv, fw);
 
 	if (dev_priv->csr.dmc_payload) {
 		intel_csr_load_program(dev_priv);
-- 
2.21.0

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

[PATCH 09/10] drm/i915/dmc: protect against reading random memory

[Lucas De Marchi]

While loading the DMC firmware we were double checking the headers made
sense, but in no place we checked that we were actually reading memory
we were supposed to. This could be wrong in case the firmware file is
truncated.

Before parsing any part of the firmware, validate the input first.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 71 ++++++++++++++++++++++++--------
 1 file changed, 53 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 7ff08de83cc6..b7181ca6c8f5 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -360,7 +360,8 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
 }
 
 static u32 parse_csr_fw_dmc(struct intel_csr *csr,
-			    const struct intel_dmc_header_base *dmc_header)
+			    const struct intel_dmc_header_base *dmc_header,
+			    size_t rem_size)
 {
 	unsigned int header_len_bytes, dmc_header_size, payload_size, i;
 	const u32 *mmioaddr, *mmiodata;
@@ -375,6 +376,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
 		const struct intel_dmc_header_v3 *v3 =
 			(const struct intel_dmc_header_v3 *)dmc_header;
 
+		if (rem_size < sizeof(struct intel_dmc_header_v3))
+			goto error_truncated;
+
 		mmioaddr = v3->mmioaddr;
 		mmiodata = v3->mmiodata;
 		mmio_count = v3->mmio_count;
@@ -386,6 +390,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
 		const struct intel_dmc_header_v1 *v1 =
 			(const struct intel_dmc_header_v1 *)dmc_header;
 
+		if (rem_size < sizeof(struct intel_dmc_header_v1))
+			goto error_truncated;
+
 		mmioaddr = v1->mmioaddr;
 		mmiodata = v1->mmiodata;
 		mmio_count = v1->mmio_count;
@@ -404,6 +411,8 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
 		return 0;
 	}
 
+	rem_size -= header_len_bytes;
+
 	/* Cache the dmc header info. */
 	if (mmio_count > mmio_count_max) {
 		DRM_ERROR("DMC firmware has wrong mmio count %u\n", mmio_count);
@@ -424,6 +433,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
 
 	/* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
 	payload_size = dmc_header->fw_size * 4;
+	if (rem_size < payload_size)
+		goto error_truncated;
+
 	if (payload_size > csr->max_fw_size) {
 		DRM_ERROR("DMC FW too big (%u bytes)\n", payload_size);
 		return 0;
@@ -440,16 +452,25 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
 	memcpy(csr->dmc_payload, payload, payload_size);
 
 	return header_len_bytes + payload_size;
+
+error_truncated:
+	DRM_ERROR("Truncated DMC firmware, refusing.\n");
+	return 0;
 }
 
 static u32
 parse_csr_fw_package(struct intel_csr *csr,
 		     const struct intel_package_header *package_header,
-		     const struct stepping_info *si)
+		     const struct stepping_info *si,
+		     size_t rem_size)
 {
-	u32 num_entries, max_entries, dmc_offset, r;
+	u32 package_size = sizeof(struct intel_package_header);
+	u32 num_entries, max_entries, dmc_offset;
 	const struct intel_fw_info *fw_info;
 
+	if (rem_size < package_size)
+		goto error_truncated;
+
 	if (package_header->header_ver == 1) {
 		max_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
 	} else if (package_header->header_ver == 2) {
@@ -460,11 +481,17 @@ parse_csr_fw_package(struct intel_csr *csr,
 		return 0;
 	}
 
-	if (package_header->header_len * 4 !=
-	    sizeof(struct intel_package_header) +
-	    max_entries * sizeof(struct intel_fw_info)) {
+	/*
+	 * We should always have space for max_entries,
+	 * even if not all are used
+	 */
+	package_size += max_entries * sizeof(struct intel_fw_info);
+	if (rem_size < package_size)
+		goto error_truncated;
+
+	if (package_header->header_len * 4 != package_size) {
 		DRM_ERROR("DMC firmware has wrong package header length "
-			  "(%u bytes)\n", package_header->header_len * 4);
+			  "(%u bytes)\n", package_size);
 		return 0;
 	}
 
@@ -481,21 +508,24 @@ parse_csr_fw_package(struct intel_csr *csr,
 		return 0;
 	}
 
-	r = sizeof(struct intel_package_header);
-
-	/* we always have space for max_entries, even if not all are used */
-	r += max_entries * sizeof(struct intel_fw_info);
-
 	/* dmc_offset is in dwords */
-	r += dmc_offset * 4;
+	return package_size + dmc_offset * 4;
 
-	return r;
+error_truncated:
+	DRM_ERROR("Truncated DMC firmware, refusing.\n");
+	return 0;
 }
 
 /* Return number of bytes parsed or 0 on error */
 static u32 parse_csr_fw_css(struct intel_csr *csr,
-			    struct intel_css_header *css_header)
+			    struct intel_css_header *css_header,
+			    size_t rem_size)
 {
+	if (rem_size < sizeof(struct intel_css_header)) {
+		DRM_ERROR("Truncated DMC firmware, refusing.\n");
+		return 0;
+	}
+
 	if (sizeof(struct intel_css_header) !=
 	    (css_header->header_len * 4)) {
 		DRM_ERROR("DMC firmware has wrong CSS header length "
@@ -530,29 +560,34 @@ static void parse_csr_fw(struct drm_i915_private *dev_priv,
 	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
 	u32 readcount = 0;
 	u32 r;
+	size_t rem_size;
 
 	if (!fw)
 		return;
 
+	rem_size = fw->size;
+
 	/* Extract CSS Header information*/
 	css_header = (struct intel_css_header *)fw->data;
-	r = parse_csr_fw_css(csr, css_header);
+	r = parse_csr_fw_css(csr, css_header, rem_size);
 	if (!r)
 		return;
 
+	rem_size -= r;
 	readcount += r;
 
 	/* Extract Package Header information*/
 	package_header = (struct intel_package_header *)&fw->data[readcount];
-	r = parse_csr_fw_package(csr, package_header, si);
+	r = parse_csr_fw_package(csr, package_header, si, rem_size);
 	if (!r)
 		return;
 
+	rem_size -= r;
 	readcount += r;
 
 	/* Extract dmc_header information. */
 	dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount];
-	parse_csr_fw_dmc(csr, dmc_header);
+	parse_csr_fw_dmc(csr, dmc_header, rem_size);
 }
 
 static void intel_csr_runtime_pm_get(struct drm_i915_private *dev_priv)
-- 
2.21.0

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

[PATCH 10/10] drm/i915/dmc: protect against loading wrong firmware

[Lucas De Marchi]

In intel_package_header version 2 there's a new field in the
fw_info table that must be 0, otherwise it's not the correct DMC
firmware. Add a check for version 2 or later.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index b7181ca6c8f5..d4996dcf596c 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -120,7 +120,10 @@ struct intel_css_header {
 } __packed;
 
 struct intel_fw_info {
-	u16 reserved1;
+	u8 reserved1;
+
+	/* reserved on package_header version 1, must be 0 on version 2 */
+	u8 dmc_id;
 
 	/* Stepping (A, B, C, ..., *). * is a wildcard */
 	char stepping;
@@ -325,12 +328,16 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv)
  */
 static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
 			      unsigned int num_entries,
-			      const struct stepping_info *si)
+			      const struct stepping_info *si,
+			      u8 package_ver)
 {
 	u32 dmc_offset = CSR_DEFAULT_FW_OFFSET;
 	unsigned int i;
 
 	for (i = 0; i < num_entries; i++) {
+		if (package_ver > 1 && fw_info[i].dmc_id != 0)
+			continue;
+
 		if (fw_info[i].substepping == '*' &&
 		    si->stepping == fw_info[i].stepping) {
 			dmc_offset = fw_info[i].offset;
@@ -501,7 +508,8 @@ parse_csr_fw_package(struct intel_csr *csr,
 
 	fw_info = (const struct intel_fw_info *)
 		((u8 *)package_header + sizeof(*package_header));
-	dmc_offset = find_dmc_fw_offset(fw_info, num_entries, si);
+	dmc_offset = find_dmc_fw_offset(fw_info, num_entries, si,
+					package_header->header_ver);
 	if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
 		DRM_ERROR("DMC firmware not supported for %c stepping\n",
 			  si->stepping);
-- 
2.21.0

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

✓ Fi.CI.BAT: success for series starting with [01/10] drm/i915/dmc: use kernel types

[Patchwork]

== Series Details ==

Series: series starting with [01/10] drm/i915/dmc: use kernel types
URL   : https://patchwork.freedesktop.org/series/61016/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6132 -> Patchwork_13077
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/

Known issues
------------

  Here are the changes found in Patchwork_13077 that come from known issues:

### IGT changes ###

#### Possible fixes ####

  * igt@gem_ctx_create@basic-files:
    - {fi-icl-dsi}:       [INCOMPLETE][1] ([fdo#107713] / [fdo#109100]) -> [PASS][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/fi-icl-dsi/igt@gem_ctx_create@basic-files.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/fi-icl-dsi/igt@gem_ctx_create@basic-files.html

  * igt@gem_exec_reloc@basic-write-gtt-noreloc:
    - {fi-icl-u3}:        [DMESG-WARN][3] ([fdo#107724]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/fi-icl-u3/igt@gem_exec_reloc@basic-write-gtt-noreloc.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/fi-icl-u3/igt@gem_exec_reloc@basic-write-gtt-noreloc.html

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       [INCOMPLETE][5] ([fdo#107718]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html

  
#### Warnings ####

  * igt@i915_selftest@live_hangcheck:
    - fi-apl-guc:         [INCOMPLETE][7] ([fdo#103927] / [fdo#110624]) -> [DMESG-FAIL][8] ([fdo#110620])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/fi-apl-guc/igt@i915_selftest@live_hangcheck.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/fi-apl-guc/igt@i915_selftest@live_hangcheck.html

  * igt@runner@aborted:
    - fi-apl-guc:         [FAIL][9] ([fdo#110624]) -> [FAIL][10] ([fdo#110622])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/fi-apl-guc/igt@runner@aborted.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/fi-apl-guc/igt@runner@aborted.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#110620]: https://bugs.freedesktop.org/show_bug.cgi?id=110620
  [fdo#110622]: https://bugs.freedesktop.org/show_bug.cgi?id=110622
  [fdo#110624]: https://bugs.freedesktop.org/show_bug.cgi?id=110624


Participating hosts (54 -> 46)
------------------------------

  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_6132 -> Patchwork_13077

  CI_DRM_6132: 78850b480c542b2e10da5a93afac2e13307909cb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5010: 631f3ac2e78c8d6332afc693bf290ae23d8d5685 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13077: 596763e01370d2bf15d881cbd4a6d5284e57de68 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

596763e01370 drm/i915/dmc: protect against loading wrong firmware
183479a1083a drm/i915/dmc: protect against reading random memory
8ce0540c860f drm/i915/dmc: remove redundant return in parse_csr_fw()
e7353db5f5cf drm/i915/dmc: add support to load dmc_header version 3
92e339122cfb drm/i915/dmc: extract function to parse dmc_header
4884a500694f drm/i915/dmc: extract function to parse package_header
93cd911d1163 drm/i915/dmc: extract function to parse css header
d610d4134c2f drm/i915/dmc: add support for package_header with version 2
b9a53fe1ae60 drm/i915/dmc: extract fw_info and table walk from intel_package_header
5d4c27a796d9 drm/i915/dmc: use kernel types

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/10] drm/i915/dmc: use kernel types

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Thursday, May 23, 2019 1:24 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Jani Nikula <jani.nikula@linux.intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; De
>Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 01/10] drm/i915/dmc: use kernel types
>
>Change all fields in intel_package_header and intel_dmc_header whose meaning
>are 1-byte numbers to use u8.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Double checked with the spec.
Looks good.

Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>---
> drivers/gpu/drm/i915/intel_csr.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>index 4527b9662330..b05e7a6aebc7 100644
>--- a/drivers/gpu/drm/i915/intel_csr.c
>+++ b/drivers/gpu/drm/i915/intel_csr.c
>@@ -130,12 +130,12 @@ struct intel_fw_info {
>
> struct intel_package_header {
> 	/* DMC container header length in dwords */
>-	unsigned char header_len;
>+	u8 header_len;
>
> 	/* always value would be 0x01 */
>-	unsigned char header_ver;
>+	u8 header_ver;
>
>-	unsigned char reserved[10];
>+	u8 reserved[10];
>
> 	/* Number of valid entries in the FWInfo array below */
> 	u32 num_entries;
>@@ -148,10 +148,10 @@ struct intel_dmc_header {
> 	u32 signature;
>
> 	/* DMC binary header length */
>-	unsigned char header_len;
>+	u8 header_len;
>
> 	/* 0x01 */
>-	unsigned char header_ver;
>+	u8 header_ver;
>
> 	/* Reserved */
> 	u16 dmcc_ver;
>--
>2.21.0

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

Re: [PATCH 01/10] drm/i915/dmc: use kernel types

[Rodrigo Vivi]

On Thu, May 23, 2019 at 01:24:11AM -0700, Lucas De Marchi wrote:
> Change all fields in intel_package_header and intel_dmc_header whose
> meaning are 1-byte numbers to use u8.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_csr.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 4527b9662330..b05e7a6aebc7 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -130,12 +130,12 @@ struct intel_fw_info {
>  
>  struct intel_package_header {
>  	/* DMC container header length in dwords */
> -	unsigned char header_len;
> +	u8 header_len;
>  
>  	/* always value would be 0x01 */
> -	unsigned char header_ver;
> +	u8 header_ver;
>  
> -	unsigned char reserved[10];
> +	u8 reserved[10];
>  
>  	/* Number of valid entries in the FWInfo array below */
>  	u32 num_entries;
> @@ -148,10 +148,10 @@ struct intel_dmc_header {
>  	u32 signature;
>  
>  	/* DMC binary header length */
> -	unsigned char header_len;
> +	u8 header_len;
>  
>  	/* 0x01 */
> -	unsigned char header_ver;
> +	u8 header_ver;
>  
>  	/* Reserved */
>  	u16 dmcc_ver;
> -- 
> 2.21.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/10] drm/i915/dmc: extract fw_info and table walk from intel_package_header

[Rodrigo Vivi]

On Thu, May 23, 2019 at 01:24:12AM -0700, Lucas De Marchi wrote:
> Move fw_info out of struct intel_package_header to allow it to grow more
> easily in future. To make a cleaner move, let's also extract a function to
> search the header for the dmc_offset.
> 
> While reviewing this code I wondered why we continued the search even
> after finding a suitable firmware. Add a comment to explain we will
> continue to try to find a more specific firmware version, even if this
> is not required by the spec.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_csr.c | 68 ++++++++++++++++++++++++--------
>  1 file changed, 51 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index b05e7a6aebc7..01ae356e69cc 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -70,6 +70,7 @@ MODULE_FIRMWARE(SKL_CSR_PATH);
>  MODULE_FIRMWARE(BXT_CSR_PATH);
>  
>  #define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
> +#define PACKAGE_MAX_FW_INFO_ENTRIES	20
>  
>  struct intel_css_header {
>  	/* 0x09 for DMC */
> @@ -139,8 +140,6 @@ struct intel_package_header {
>  
>  	/* Number of valid entries in the FWInfo array below */
>  	u32 num_entries;
> -
> -	struct intel_fw_info fw_info[20];
>  } __packed;
>  
>  struct intel_dmc_header {
> @@ -292,6 +291,46 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv)
>  	gen9_set_dc_state_debugmask(dev_priv);
>  }
>  
> +/*
> + * Search fw_info table for dmc_offset to find firmware binary: num_entries is
> + * already sanitized.
> + */
> +static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
> +			      unsigned int num_entries,
> +			      const struct stepping_info *si)
> +{
> +	u32 dmc_offset = CSR_DEFAULT_FW_OFFSET;
> +	unsigned int i;
> +
> +	for (i = 0; i < num_entries; i++) {
> +		if (fw_info[i].substepping == '*' &&
> +		    si->stepping == fw_info[i].stepping) {
> +			dmc_offset = fw_info[i].offset;
> +			break;
> +		}
> +
> +		if (si->stepping == fw_info[i].stepping &&
> +		    si->substepping == fw_info[i].substepping) {
> +			dmc_offset = fw_info[i].offset;
> +			break;
> +		}
> +
> +		if (fw_info[i].stepping == '*' &&
> +		    fw_info[i].substepping == '*') {
> +			/*
> +			 * In theory we should stop the search as generic
> +			 * entries should alwas come after the more specific
> +			 * ones, but let's continue to make sure to work even
> +			 * with "broken" firmwares. If we don't find a more
> +			 * specific one, then we use this entry
> +			 */

good point.


Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>



> +			dmc_offset = fw_info[i].offset;
> +		}
> +	}
> +
> +	return dmc_offset;
> +}
> +
>  static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
>  			 const struct firmware *fw)
>  {
> @@ -300,7 +339,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
>  	struct intel_dmc_header *dmc_header;
>  	struct intel_csr *csr = &dev_priv->csr;
>  	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
> -	u32 dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
> +	u32 dmc_offset, num_entries, readcount = 0, nbytes;
>  	u32 i;
>  	u32 *dmc_payload;
>  
> @@ -342,27 +381,22 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
>  			  (package_header->header_len * 4));
>  		return NULL;
>  	}
> +
>  	readcount += sizeof(struct intel_package_header);
> +	num_entries = package_header->num_entries;
> +	if (WARN_ON(package_header->num_entries > PACKAGE_MAX_FW_INFO_ENTRIES))
> +		num_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
>  
> -	/* Search for dmc_offset to find firware binary. */
> -	for (i = 0; i < package_header->num_entries; i++) {
> -		if (package_header->fw_info[i].substepping == '*' &&
> -		    si->stepping == package_header->fw_info[i].stepping) {
> -			dmc_offset = package_header->fw_info[i].offset;
> -			break;
> -		} else if (si->stepping == package_header->fw_info[i].stepping &&
> -			   si->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;
> -	}
> +	dmc_offset = find_dmc_fw_offset((struct intel_fw_info *)
> +					&fw->data[readcount], num_entries, si);
>  	if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
>  		DRM_ERROR("DMC firmware not supported for %c stepping\n",
>  			  si->stepping);
>  		return NULL;
>  	}
> +	/* we always have space for PACKAGE_MAX_FW_INFO_ENTRIES */
> +	readcount += PACKAGE_MAX_FW_INFO_ENTRIES * sizeof(struct intel_fw_info);
> +
>  	/* Convert dmc_offset into number of bytes. By default it is in dwords*/
>  	dmc_offset *= 4;
>  	readcount += dmc_offset;
> -- 
> 2.21.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/10] drm/i915/dmc: add support for package_header with version 2

[Rodrigo Vivi]

On Thu, May 23, 2019 at 01:24:13AM -0700, Lucas De Marchi wrote:
> The only meaninful change is that it supports up to 32 fw_info entries
> rather than the previous max=20.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_csr.c | 35 ++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 01ae356e69cc..b9651fbe4c25 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -71,6 +71,7 @@ MODULE_FIRMWARE(BXT_CSR_PATH);
>  
>  #define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
>  #define PACKAGE_MAX_FW_INFO_ENTRIES	20
> +#define PACKAGE_V2_MAX_FW_INFO_ENTRIES	32

Looking to spec I could only find
"} FWInfo[20];"

Where did you get the 32?

>  
>  struct intel_css_header {
>  	/* 0x09 for DMC */
> @@ -133,7 +134,7 @@ struct intel_package_header {
>  	/* DMC container header length in dwords */
>  	u8 header_len;
>  
> -	/* always value would be 0x01 */
> +	/* 0x01, 0x02 */
>  	u8 header_ver;
>  
>  	u8 reserved[10];
> @@ -339,7 +340,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
>  	struct intel_dmc_header *dmc_header;
>  	struct intel_csr *csr = &dev_priv->csr;
>  	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
> -	u32 dmc_offset, num_entries, readcount = 0, nbytes;
> +	u32 dmc_offset, num_entries, max_entries, readcount = 0, nbytes;
>  	u32 i;
>  	u32 *dmc_payload;
>  
> @@ -374,18 +375,30 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
>  	/* Extract Package Header information*/
>  	package_header = (struct intel_package_header *)
>  		&fw->data[readcount];
> -	if (sizeof(struct intel_package_header) !=
> -	    (package_header->header_len * 4)) {
> +
> +	readcount += sizeof(struct intel_package_header);
> +
> +	if (package_header->header_ver == 1) {
> +		max_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
> +	} else if (package_header->header_ver == 2) {
> +		max_entries = PACKAGE_V2_MAX_FW_INFO_ENTRIES;
> +	} else {
> +		DRM_ERROR("DMC firmware has unknown header version %u\n",
> +			  package_header->header_ver);
> +		return NULL;
> +	}
> +
> +	if (package_header->header_len * 4 !=
> +	    sizeof(struct intel_package_header) +
> +	    max_entries * sizeof(struct intel_fw_info)) {
>  		DRM_ERROR("DMC firmware has wrong package header length "
> -			  "(%u bytes)\n",
> -			  (package_header->header_len * 4));
> +			  "(%u bytes)\n", package_header->header_len * 4);
>  		return NULL;
>  	}
>  
> -	readcount += sizeof(struct intel_package_header);
>  	num_entries = package_header->num_entries;
> -	if (WARN_ON(package_header->num_entries > PACKAGE_MAX_FW_INFO_ENTRIES))
> -		num_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
> +	if (WARN_ON(package_header->num_entries > max_entries))
> +		num_entries = max_entries;
>  
>  	dmc_offset = find_dmc_fw_offset((struct intel_fw_info *)
>  					&fw->data[readcount], num_entries, si);
> @@ -394,9 +407,9 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
>  			  si->stepping);
>  		return NULL;
>  	}
> -	/* we always have space for PACKAGE_MAX_FW_INFO_ENTRIES */
> -	readcount += PACKAGE_MAX_FW_INFO_ENTRIES * sizeof(struct intel_fw_info);
>  
> +	/* we always have space for max_entries, even if not all are used */
> +	readcount += max_entries * sizeof(struct intel_fw_info);
>  	/* Convert dmc_offset into number of bytes. By default it is in dwords*/
>  	dmc_offset *= 4;
>  	readcount += dmc_offset;
> -- 
> 2.21.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/10] drm/i915/dmc: extract function to parse css header

[Rodrigo Vivi]

On Thu, May 23, 2019 at 01:24:14AM -0700, Lucas De Marchi wrote:
> Let's start splitting the parse function, making all of them return the
> number of bytes parsed - different versions of the firmware header may
> require different sizes for the structures.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

>  drivers/gpu/drm/i915/intel_csr.c | 49 +++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index b9651fbe4c25..7e53bf576892 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -332,29 +332,16 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
>  	return dmc_offset;
>  }
>  
> -static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
> -			 const struct firmware *fw)
> +/* Return number of bytes parsed or 0 on error */
> +static u32 parse_csr_fw_css(struct intel_csr *csr,
> +			    struct intel_css_header *css_header)
>  {
> -	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;
> -	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
> -	u32 dmc_offset, num_entries, max_entries, readcount = 0, nbytes;
> -	u32 i;
> -	u32 *dmc_payload;
> -
> -	if (!fw)
> -		return NULL;
> -
> -	/* 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("DMC firmware has wrong CSS header length "
>  			  "(%u bytes)\n",
>  			  (css_header->header_len * 4));
> -		return NULL;
> +		return 0;
>  	}
>  
>  	if (csr->required_version &&
> @@ -365,12 +352,36 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
>  			 CSR_VERSION_MINOR(css_header->version),
>  			 CSR_VERSION_MAJOR(csr->required_version),
>  			 CSR_VERSION_MINOR(csr->required_version));
> -		return NULL;
> +		return 0;
>  	}
>  
>  	csr->version = css_header->version;
>  
> -	readcount += sizeof(struct intel_css_header);
> +	return sizeof(struct intel_css_header);
> +}
> +
> +static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
> +			 const struct firmware *fw)
> +{
> +	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;
> +	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
> +	u32 dmc_offset, num_entries, max_entries, readcount = 0, nbytes;
> +	u32 i, r;
> +	u32 *dmc_payload;
> +
> +	if (!fw)
> +		return NULL;
> +
> +	/* Extract CSS Header information*/
> +	css_header = (struct intel_css_header *)fw->data;
> +	r = parse_csr_fw_css(csr, css_header);
> +	if (!r)
> +		return NULL;
> +
> +	readcount += r;
>  
>  	/* Extract Package Header information*/
>  	package_header = (struct intel_package_header *)
> -- 
> 2.21.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/10] drm/i915/dmc: extract fw_info and table walk from intel_package_header

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Thursday, May 23, 2019 1:24 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Jani Nikula <jani.nikula@linux.intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; De
>Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 02/10] drm/i915/dmc: extract fw_info and table walk from
>intel_package_header
>
>Move fw_info out of struct intel_package_header to allow it to grow more easily
>in future. To make a cleaner move, let's also extract a function to search the
>header for the dmc_offset.
>
>While reviewing this code I wondered why we continued the search even after
>finding a suitable firmware. Add a comment to explain we will continue to try to
>find a more specific firmware version, even if this is not required by the spec.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>

>---
> drivers/gpu/drm/i915/intel_csr.c | 68 ++++++++++++++++++++++++--------
> 1 file changed, 51 insertions(+), 17 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>index b05e7a6aebc7..01ae356e69cc 100644
>--- a/drivers/gpu/drm/i915/intel_csr.c
>+++ b/drivers/gpu/drm/i915/intel_csr.c
>@@ -70,6 +70,7 @@ MODULE_FIRMWARE(SKL_CSR_PATH);
>MODULE_FIRMWARE(BXT_CSR_PATH);
>
> #define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
>+#define PACKAGE_MAX_FW_INFO_ENTRIES	20
>
> struct intel_css_header {
> 	/* 0x09 for DMC */
>@@ -139,8 +140,6 @@ struct intel_package_header {
>
> 	/* Number of valid entries in the FWInfo array below */
> 	u32 num_entries;
>-
>-	struct intel_fw_info fw_info[20];
> } __packed;
>
> struct intel_dmc_header {
>@@ -292,6 +291,46 @@ void intel_csr_load_program(struct drm_i915_private
>*dev_priv)
> 	gen9_set_dc_state_debugmask(dev_priv);
> }
>
>+/*
>+ * Search fw_info table for dmc_offset to find firmware binary:
>+num_entries is
>+ * already sanitized.
>+ */
>+static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
>+			      unsigned int num_entries,
>+			      const struct stepping_info *si) {
>+	u32 dmc_offset = CSR_DEFAULT_FW_OFFSET;
>+	unsigned int i;
>+
>+	for (i = 0; i < num_entries; i++) {
>+		if (fw_info[i].substepping == '*' &&
>+		    si->stepping == fw_info[i].stepping) {
>+			dmc_offset = fw_info[i].offset;
>+			break;
>+		}
>+
>+		if (si->stepping == fw_info[i].stepping &&
>+		    si->substepping == fw_info[i].substepping) {
>+			dmc_offset = fw_info[i].offset;
>+			break;
>+		}
>+
>+		if (fw_info[i].stepping == '*' &&
>+		    fw_info[i].substepping == '*') {
>+			/*
>+			 * In theory we should stop the search as generic
>+			 * entries should alwas come after the more specific
>+			 * ones, but let's continue to make sure to work even
>+			 * with "broken" firmwares. If we don't find a more
>+			 * specific one, then we use this entry
>+			 */
>+			dmc_offset = fw_info[i].offset;
>+		}
>+	}
>+
>+	return dmc_offset;
>+}
>+
> static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
> 			 const struct firmware *fw)
> {
>@@ -300,7 +339,7 @@ static u32 *parse_csr_fw(struct drm_i915_private
>*dev_priv,
> 	struct intel_dmc_header *dmc_header;
> 	struct intel_csr *csr = &dev_priv->csr;
> 	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
>-	u32 dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
>+	u32 dmc_offset, num_entries, readcount = 0, nbytes;
> 	u32 i;
> 	u32 *dmc_payload;
>
>@@ -342,27 +381,22 @@ static u32 *parse_csr_fw(struct drm_i915_private
>*dev_priv,
> 			  (package_header->header_len * 4));
> 		return NULL;
> 	}
>+
> 	readcount += sizeof(struct intel_package_header);
>+	num_entries = package_header->num_entries;
>+	if (WARN_ON(package_header->num_entries >
>PACKAGE_MAX_FW_INFO_ENTRIES))
>+		num_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
>
>-	/* Search for dmc_offset to find firware binary. */
>-	for (i = 0; i < package_header->num_entries; i++) {
>-		if (package_header->fw_info[i].substepping == '*' &&
>-		    si->stepping == package_header->fw_info[i].stepping) {
>-			dmc_offset = package_header->fw_info[i].offset;
>-			break;
>-		} else if (si->stepping == package_header->fw_info[i].stepping
>&&
>-			   si->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;
>-	}
>+	dmc_offset = find_dmc_fw_offset((struct intel_fw_info *)
>+					&fw->data[readcount], num_entries, si);
> 	if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
> 		DRM_ERROR("DMC firmware not supported for %c stepping\n",
> 			  si->stepping);
> 		return NULL;
> 	}
>+	/* we always have space for PACKAGE_MAX_FW_INFO_ENTRIES */
>+	readcount += PACKAGE_MAX_FW_INFO_ENTRIES * sizeof(struct
>+intel_fw_info);
>+
> 	/* Convert dmc_offset into number of bytes. By default it is in dwords*/
> 	dmc_offset *= 4;
> 	readcount += dmc_offset;
>--
>2.21.0

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

Re: [PATCH 03/10] drm/i915/dmc: add support for package_header with version 2

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Thursday, May 23, 2019 1:24 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Jani Nikula <jani.nikula@linux.intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; De
>Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 03/10] drm/i915/dmc: add support for package_header with
>version 2
>
>The only meaninful change is that it supports up to 32 fw_info entries rather than
>the previous max=20.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Confirmed with the spec.

Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>---
> drivers/gpu/drm/i915/intel_csr.c | 35 ++++++++++++++++++++++----------
> 1 file changed, 24 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>index 01ae356e69cc..b9651fbe4c25 100644
>--- a/drivers/gpu/drm/i915/intel_csr.c
>+++ b/drivers/gpu/drm/i915/intel_csr.c
>@@ -71,6 +71,7 @@ MODULE_FIRMWARE(BXT_CSR_PATH);
>
> #define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
> #define PACKAGE_MAX_FW_INFO_ENTRIES	20
>+#define PACKAGE_V2_MAX_FW_INFO_ENTRIES	32
>
> struct intel_css_header {
> 	/* 0x09 for DMC */
>@@ -133,7 +134,7 @@ struct intel_package_header {
> 	/* DMC container header length in dwords */
> 	u8 header_len;
>
>-	/* always value would be 0x01 */
>+	/* 0x01, 0x02 */
> 	u8 header_ver;
>
> 	u8 reserved[10];
>@@ -339,7 +340,7 @@ static u32 *parse_csr_fw(struct drm_i915_private
>*dev_priv,
> 	struct intel_dmc_header *dmc_header;
> 	struct intel_csr *csr = &dev_priv->csr;
> 	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
>-	u32 dmc_offset, num_entries, readcount = 0, nbytes;
>+	u32 dmc_offset, num_entries, max_entries, readcount = 0, nbytes;
> 	u32 i;
> 	u32 *dmc_payload;
>
>@@ -374,18 +375,30 @@ static u32 *parse_csr_fw(struct drm_i915_private
>*dev_priv,
> 	/* Extract Package Header information*/
> 	package_header = (struct intel_package_header *)
> 		&fw->data[readcount];
>-	if (sizeof(struct intel_package_header) !=
>-	    (package_header->header_len * 4)) {
>+
>+	readcount += sizeof(struct intel_package_header);
>+
>+	if (package_header->header_ver == 1) {
>+		max_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
>+	} else if (package_header->header_ver == 2) {
>+		max_entries = PACKAGE_V2_MAX_FW_INFO_ENTRIES;
>+	} else {
>+		DRM_ERROR("DMC firmware has unknown header version
>%u\n",
>+			  package_header->header_ver);
>+		return NULL;
>+	}
>+
>+	if (package_header->header_len * 4 !=
>+	    sizeof(struct intel_package_header) +
>+	    max_entries * sizeof(struct intel_fw_info)) {
> 		DRM_ERROR("DMC firmware has wrong package header length "
>-			  "(%u bytes)\n",
>-			  (package_header->header_len * 4));
>+			  "(%u bytes)\n", package_header->header_len * 4);
> 		return NULL;
> 	}
>
>-	readcount += sizeof(struct intel_package_header);
> 	num_entries = package_header->num_entries;
>-	if (WARN_ON(package_header->num_entries >
>PACKAGE_MAX_FW_INFO_ENTRIES))
>-		num_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
>+	if (WARN_ON(package_header->num_entries > max_entries))
>+		num_entries = max_entries;
>
> 	dmc_offset = find_dmc_fw_offset((struct intel_fw_info *)
> 					&fw->data[readcount], num_entries, si);
>@@ -394,9 +407,9 @@ static u32 *parse_csr_fw(struct drm_i915_private
>*dev_priv,
> 			  si->stepping);
> 		return NULL;
> 	}
>-	/* we always have space for PACKAGE_MAX_FW_INFO_ENTRIES */
>-	readcount += PACKAGE_MAX_FW_INFO_ENTRIES * sizeof(struct
>intel_fw_info);
>
>+	/* we always have space for max_entries, even if not all are used */
>+	readcount += max_entries * sizeof(struct intel_fw_info);
> 	/* Convert dmc_offset into number of bytes. By default it is in dwords*/
> 	dmc_offset *= 4;
> 	readcount += dmc_offset;
>--
>2.21.0

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

Re: [PATCH 04/10] drm/i915/dmc: extract function to parse css header

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Thursday, May 23, 2019 1:24 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Jani Nikula <jani.nikula@linux.intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; De
>Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 04/10] drm/i915/dmc: extract function to parse css header
>
>Let's start splitting the parse function, making all of them return the number of
>bytes parsed - different versions of the firmware header may require different
>sizes for the structures.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Looks good.

Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>---
> drivers/gpu/drm/i915/intel_csr.c | 49 +++++++++++++++++++-------------
> 1 file changed, 30 insertions(+), 19 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>index b9651fbe4c25..7e53bf576892 100644
>--- a/drivers/gpu/drm/i915/intel_csr.c
>+++ b/drivers/gpu/drm/i915/intel_csr.c
>@@ -332,29 +332,16 @@ static u32 find_dmc_fw_offset(const struct
>intel_fw_info *fw_info,
> 	return dmc_offset;
> }
>
>-static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
>-			 const struct firmware *fw)
>+/* Return number of bytes parsed or 0 on error */ static u32
>+parse_csr_fw_css(struct intel_csr *csr,
>+			    struct intel_css_header *css_header)
> {
>-	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;
>-	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
>-	u32 dmc_offset, num_entries, max_entries, readcount = 0, nbytes;
>-	u32 i;
>-	u32 *dmc_payload;
>-
>-	if (!fw)
>-		return NULL;
>-
>-	/* 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("DMC firmware has wrong CSS header length "
> 			  "(%u bytes)\n",
> 			  (css_header->header_len * 4));
>-		return NULL;
>+		return 0;
> 	}
>
> 	if (csr->required_version &&
>@@ -365,12 +352,36 @@ static u32 *parse_csr_fw(struct drm_i915_private
>*dev_priv,
> 			 CSR_VERSION_MINOR(css_header->version),
> 			 CSR_VERSION_MAJOR(csr->required_version),
> 			 CSR_VERSION_MINOR(csr->required_version));
>-		return NULL;
>+		return 0;
> 	}
>
> 	csr->version = css_header->version;
>
>-	readcount += sizeof(struct intel_css_header);
>+	return sizeof(struct intel_css_header); }
>+
>+static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
>+			 const struct firmware *fw)
>+{
>+	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;
>+	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
>+	u32 dmc_offset, num_entries, max_entries, readcount = 0, nbytes;
>+	u32 i, r;
>+	u32 *dmc_payload;
>+
>+	if (!fw)
>+		return NULL;
>+
>+	/* Extract CSS Header information*/
>+	css_header = (struct intel_css_header *)fw->data;
>+	r = parse_csr_fw_css(csr, css_header);
>+	if (!r)
>+		return NULL;
>+
>+	readcount += r;
>
> 	/* Extract Package Header information*/
> 	package_header = (struct intel_package_header *)
>--
>2.21.0

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

Re: [PATCH 03/10] drm/i915/dmc: add support for package_header with version 2

[Lucas De Marchi]

On Thu, May 23, 2019 at 10:43:39AM -0700, Rodrigo Vivi wrote:
>On Thu, May 23, 2019 at 01:24:13AM -0700, Lucas De Marchi wrote:
>> The only meaninful change is that it supports up to 32 fw_info entries
>> rather than the previous max=20.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_csr.c | 35 ++++++++++++++++++++++----------
>>  1 file changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>> index 01ae356e69cc..b9651fbe4c25 100644
>> --- a/drivers/gpu/drm/i915/intel_csr.c
>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>> @@ -71,6 +71,7 @@ MODULE_FIRMWARE(BXT_CSR_PATH);
>>
>>  #define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
>>  #define PACKAGE_MAX_FW_INFO_ENTRIES	20
>> +#define PACKAGE_V2_MAX_FW_INFO_ENTRIES	32
>
>Looking to spec I could only find
>"} FWInfo[20];"
>
>Where did you get the 32?

you are looking to the struct for header_ver == 1. Look to the one with
header_ver == 2.

Lucas De Marchi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/10] drm/i915/dmc: extract function to parse package_header

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Thursday, May 23, 2019 1:24 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Jani Nikula <jani.nikula@linux.intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; De
>Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 05/10] drm/i915/dmc: extract function to parse package_header
>
>Like parse_csr_fw_css() this parses the package_header from firmware and saves
>the relevant fields in the csr struct. In this function we also lookup the fw_info we
>are interested in.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Parsing is handled well.

Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>

>---
> drivers/gpu/drm/i915/intel_csr.c | 94 +++++++++++++++++++-------------
> 1 file changed, 55 insertions(+), 39 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>index 7e53bf576892..b19742becb98 100644
>--- a/drivers/gpu/drm/i915/intel_csr.c
>+++ b/drivers/gpu/drm/i915/intel_csr.c
>@@ -332,6 +332,56 @@ static u32 find_dmc_fw_offset(const struct
>intel_fw_info *fw_info,
> 	return dmc_offset;
> }
>
>+static u32
>+parse_csr_fw_package(struct intel_csr *csr,
>+		     const struct intel_package_header *package_header,
>+		     const struct stepping_info *si)
>+{
>+	u32 num_entries, max_entries, dmc_offset, r;
>+	const struct intel_fw_info *fw_info;
>+
>+	if (package_header->header_ver == 1) {
>+		max_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
>+	} else if (package_header->header_ver == 2) {
>+		max_entries = PACKAGE_V2_MAX_FW_INFO_ENTRIES;
>+	} else {
>+		DRM_ERROR("DMC firmware has unknown header version
>%u\n",
>+			  package_header->header_ver);
>+		return 0;
>+	}
>+
>+	if (package_header->header_len * 4 !=
>+	    sizeof(struct intel_package_header) +
>+	    max_entries * sizeof(struct intel_fw_info)) {
>+		DRM_ERROR("DMC firmware has wrong package header length "
>+			  "(%u bytes)\n", package_header->header_len * 4);
>+		return 0;
>+	}
>+
>+	num_entries = package_header->num_entries;
>+	if (WARN_ON(package_header->num_entries > max_entries))
>+		num_entries = max_entries;
>+
>+	fw_info = (const struct intel_fw_info *)
>+		((u8 *)package_header + sizeof(*package_header));
>+	dmc_offset = find_dmc_fw_offset(fw_info, num_entries, si);
>+	if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
>+		DRM_ERROR("DMC firmware not supported for %c stepping\n",
>+			  si->stepping);
>+		return 0;
>+	}
>+
>+	r = sizeof(struct intel_package_header);
>+
>+	/* we always have space for max_entries, even if not all are used */
>+	r += max_entries * sizeof(struct intel_fw_info);
>+
>+	/* dmc_offset is in dwords */
>+	r += dmc_offset * 4;
>+
>+	return r;
>+}
>+
> /* Return number of bytes parsed or 0 on error */  static u32
>parse_csr_fw_css(struct intel_csr *csr,
> 			    struct intel_css_header *css_header) @@ -368,7
>+418,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
> 	struct intel_dmc_header *dmc_header;
> 	struct intel_csr *csr = &dev_priv->csr;
> 	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
>-	u32 dmc_offset, num_entries, max_entries, readcount = 0, nbytes;
>+	u32 readcount = 0, nbytes;
> 	u32 i, r;
> 	u32 *dmc_payload;
>
>@@ -384,46 +434,12 @@ static u32 *parse_csr_fw(struct drm_i915_private
>*dev_priv,
> 	readcount += r;
>
> 	/* Extract Package Header information*/
>-	package_header = (struct intel_package_header *)
>-		&fw->data[readcount];
>-
>-	readcount += sizeof(struct intel_package_header);
>-
>-	if (package_header->header_ver == 1) {
>-		max_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
>-	} else if (package_header->header_ver == 2) {
>-		max_entries = PACKAGE_V2_MAX_FW_INFO_ENTRIES;
>-	} else {
>-		DRM_ERROR("DMC firmware has unknown header version
>%u\n",
>-			  package_header->header_ver);
>-		return NULL;
>-	}
>-
>-	if (package_header->header_len * 4 !=
>-	    sizeof(struct intel_package_header) +
>-	    max_entries * sizeof(struct intel_fw_info)) {
>-		DRM_ERROR("DMC firmware has wrong package header length "
>-			  "(%u bytes)\n", package_header->header_len * 4);
>-		return NULL;
>-	}
>-
>-	num_entries = package_header->num_entries;
>-	if (WARN_ON(package_header->num_entries > max_entries))
>-		num_entries = max_entries;
>-
>-	dmc_offset = find_dmc_fw_offset((struct intel_fw_info *)
>-					&fw->data[readcount], num_entries, si);
>-	if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
>-		DRM_ERROR("DMC firmware not supported for %c stepping\n",
>-			  si->stepping);
>+	package_header = (struct intel_package_header *)&fw-
>>data[readcount];
>+	r = parse_csr_fw_package(csr, package_header, si);
>+	if (!r)
> 		return NULL;
>-	}
>
>-	/* we always have space for max_entries, even if not all are used */
>-	readcount += max_entries * sizeof(struct intel_fw_info);
>-	/* Convert dmc_offset into number of bytes. By default it is in dwords*/
>-	dmc_offset *= 4;
>-	readcount += dmc_offset;
>+	readcount += r;
>
> 	/* Extract dmc_header information. */
> 	dmc_header = (struct intel_dmc_header *)&fw->data[readcount];
>--
>2.21.0

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

Re: [PATCH 06/10] drm/i915/dmc: extract function to parse dmc_header

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Thursday, May 23, 2019 1:24 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Jani Nikula <jani.nikula@linux.intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; De
>Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 06/10] drm/i915/dmc: extract function to parse dmc_header
>
>Complete the extraction of functions to parse specific parts of the firmware. The
>return of the function parse_csr_fw() is now redundant since it already sets the
>dmc_payload field. Changing it is left for later to avoid noise in the commit.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>

>---
> drivers/gpu/drm/i915/intel_csr.c | 102 ++++++++++++++++++-------------
> 1 file changed, 60 insertions(+), 42 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>index b19742becb98..20dd4bd5feae 100644
>--- a/drivers/gpu/drm/i915/intel_csr.c
>+++ b/drivers/gpu/drm/i915/intel_csr.c
>@@ -332,6 +332,61 @@ static u32 find_dmc_fw_offset(const struct
>intel_fw_info *fw_info,
> 	return dmc_offset;
> }
>
>+static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>+			    const struct intel_dmc_header *dmc_header) {
>+	unsigned int i, payload_size;
>+	u32 r;
>+	u8 *payload;
>+
>+	if (sizeof(struct intel_dmc_header) != dmc_header->header_len) {
>+		DRM_ERROR("DMC firmware has wrong dmc header length "
>+			  "(%u bytes)\n",
>+			  (dmc_header->header_len));
>+		return 0;
>+	}
>+
>+	/* Cache the dmc header info. */
>+	if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) {
>+		DRM_ERROR("DMC firmware has wrong mmio count %u\n",
>+			  dmc_header->mmio_count);
>+		return 0;
>+	}
>+
>+	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("DMC firmware has wrong mmio address
>0x%x\n",
>+				  dmc_header->mmioaddr[i]);
>+			return 0;
>+		}
>+		csr->mmioaddr[i] = _MMIO(dmc_header->mmioaddr[i]);
>+		csr->mmiodata[i] = dmc_header->mmiodata[i];
>+	}
>+
>+	/* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
>+	payload_size = dmc_header->fw_size * 4;
>+	if (payload_size > csr->max_fw_size) {
>+		DRM_ERROR("DMC FW too big (%u bytes)\n", payload_size);
>+		return 0;
>+	}
>+	csr->dmc_fw_size = dmc_header->fw_size;
>+
>+	csr->dmc_payload = kmalloc(payload_size, GFP_KERNEL);
>+	if (!csr->dmc_payload) {
>+		DRM_ERROR("Memory allocation failed for dmc payload\n");
>+		return 0;
>+	}
>+
>+	r = sizeof(struct intel_dmc_header);
>+	payload = (u8 *)(dmc_header) + r;
>+	memcpy(csr->dmc_payload, payload, payload_size);
>+	r += payload_size;
>+
>+	return r;
>+}
>+
> static u32
> parse_csr_fw_package(struct intel_csr *csr,
> 		     const struct intel_package_header *package_header, @@ -
>418,9 +473,8 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
> 	struct intel_dmc_header *dmc_header;
> 	struct intel_csr *csr = &dev_priv->csr;
> 	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
>-	u32 readcount = 0, nbytes;
>-	u32 i, r;
>-	u32 *dmc_payload;
>+	u32 readcount = 0;
>+	u32 r;
>
> 	if (!fw)
> 		return NULL;
>@@ -443,47 +497,11 @@ static u32 *parse_csr_fw(struct drm_i915_private
>*dev_priv,
>
> 	/* 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("DMC firmware has wrong dmc header length "
>-			  "(%u bytes)\n",
>-			  (dmc_header->header_len));
>-		return NULL;
>-	}
>-	readcount += sizeof(struct intel_dmc_header);
>-
>-	/* Cache the dmc header info. */
>-	if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) {
>-		DRM_ERROR("DMC firmware has wrong mmio count %u\n",
>-			  dmc_header->mmio_count);
>-		return NULL;
>-	}
>-	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("DMC firmware has wrong mmio address
>0x%x\n",
>-				  dmc_header->mmioaddr[i]);
>-			return NULL;
>-		}
>-		csr->mmioaddr[i] = _MMIO(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("DMC FW too big (%u bytes)\n", nbytes);
>-		return NULL;
>-	}
>-	csr->dmc_fw_size = dmc_header->fw_size;
>-
>-	dmc_payload = kmalloc(nbytes, GFP_KERNEL);
>-	if (!dmc_payload) {
>-		DRM_ERROR("Memory allocation failed for dmc payload\n");
>+	r = parse_csr_fw_dmc(csr, dmc_header);
>+	if (!r)
> 		return NULL;
>-	}
>
>-	return memcpy(dmc_payload, &fw->data[readcount], nbytes);
>+	return csr->dmc_payload;
> }
>
> static void intel_csr_runtime_pm_get(struct drm_i915_private *dev_priv)
>--
>2.21.0

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

Re: [PATCH 07/10] drm/i915/dmc: add support to load dmc_header version 3

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Thursday, May 23, 2019 1:24 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Jani Nikula <jani.nikula@linux.intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; De
>Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 07/10] drm/i915/dmc: add support to load dmc_header version 3
>
>Main difference is that now there are up to 20 MMIOs that can be set and a lot of
>noise due to the struct changing the fields in the middle.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Checked with spec. The header struct looks good. 

Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>---
> drivers/gpu/drm/i915/i915_drv.h  |   4 +-
> drivers/gpu/drm/i915/intel_csr.c | 107 +++++++++++++++++++++++--------
> 2 files changed, 83 insertions(+), 28 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index 1ad3818d2676..04a6b59256fd 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -354,8 +354,8 @@ struct intel_csr {
> 	u32 dmc_fw_size; /* dwords */
> 	u32 version;
> 	u32 mmio_count;
>-	i915_reg_t mmioaddr[8];
>-	u32 mmiodata[8];
>+	i915_reg_t mmioaddr[20];
>+	u32 mmiodata[20];
> 	u32 dc_state;
> 	u32 allowed_dc_mask;
> 	intel_wakeref_t wakeref;
>diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>index 20dd4bd5feae..ad4ee55a8c5e 100644
>--- a/drivers/gpu/drm/i915/intel_csr.c
>+++ b/drivers/gpu/drm/i915/intel_csr.c
>@@ -72,6 +72,8 @@ MODULE_FIRMWARE(BXT_CSR_PATH);
> #define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
> #define PACKAGE_MAX_FW_INFO_ENTRIES	20
> #define PACKAGE_V2_MAX_FW_INFO_ENTRIES	32
>+#define DMC_V1_MAX_MMIO_COUNT		8
>+#define DMC_V3_MAX_MMIO_COUNT		20
>
> struct intel_css_header {
> 	/* 0x09 for DMC */
>@@ -143,7 +145,7 @@ struct intel_package_header {
> 	u32 num_entries;
> } __packed;
>
>-struct intel_dmc_header {
>+struct intel_dmc_header_base {
> 	/* always value would be 0x40403E3E */
> 	u32 signature;
>
>@@ -164,22 +166,47 @@ struct intel_dmc_header {
>
> 	/* Major Minor version */
> 	u32 fw_version;
>+} __packed;
>+
>+struct intel_dmc_header_v1 {
>+	struct intel_dmc_header_base base;
>
> 	/* Number of valid MMIO cycles present. */
> 	u32 mmio_count;
>
> 	/* MMIO address */
>-	u32 mmioaddr[8];
>+	u32 mmioaddr[DMC_V1_MAX_MMIO_COUNT];
>
> 	/* MMIO data */
>-	u32 mmiodata[8];
>+	u32 mmiodata[DMC_V1_MAX_MMIO_COUNT];
>
> 	/* FW filename  */
>-	unsigned char dfile[32];
>+	char dfile[32];
>
> 	u32 reserved1[2];
> } __packed;
>
>+struct intel_dmc_header_v3 {
>+	struct intel_dmc_header_base base;
>+
>+	/* DMC RAM start MMIO address */
>+	u32 start_mmioaddr;
>+
>+	u32 reserved[9];
>+
>+	/* FW filename */
>+	char dfile[32];
>+
>+	/* Number of valid MMIO cycles present. */
>+	u32 mmio_count;
>+
>+	/* MMIO address */
>+	u32 mmioaddr[DMC_V3_MAX_MMIO_COUNT];
>+
>+	/* MMIO data */
>+	u32 mmiodata[DMC_V3_MAX_MMIO_COUNT];
>+} __packed;
>+
> struct stepping_info {
> 	char stepping;
> 	char substepping;
>@@ -333,37 +360,67 @@ static u32 find_dmc_fw_offset(const struct
>intel_fw_info *fw_info,  }
>
> static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>-			    const struct intel_dmc_header *dmc_header)
>+			    const struct intel_dmc_header_base *dmc_header)
> {
>-	unsigned int i, payload_size;
>-	u32 r;
>+	unsigned int header_len_bytes, dmc_header_size, payload_size, i;
>+	const u32 *mmioaddr, *mmiodata;
>+	u32 mmio_count, mmio_count_max;
> 	u8 *payload;
>
>-	if (sizeof(struct intel_dmc_header) != dmc_header->header_len) {
>+	BUILD_BUG_ON(ARRAY_SIZE(csr->mmioaddr) <
>DMC_V3_MAX_MMIO_COUNT ||
>+		     ARRAY_SIZE(csr->mmioaddr) <
>DMC_V1_MAX_MMIO_COUNT);
>+
>+	/* Cope with small differences between v1 and v3 */
>+	if (dmc_header->header_ver == 3) {
>+		const struct intel_dmc_header_v3 *v3 =
>+			(const struct intel_dmc_header_v3 *)dmc_header;
>+
>+		mmioaddr = v3->mmioaddr;
>+		mmiodata = v3->mmiodata;
>+		mmio_count = v3->mmio_count;
>+		mmio_count_max = DMC_V3_MAX_MMIO_COUNT;
>+		/* header_len is in dwords */
>+		header_len_bytes = dmc_header->header_len * 4;
>+		dmc_header_size = sizeof(*v3);
>+	} else if (dmc_header->header_ver == 1) {
>+		const struct intel_dmc_header_v1 *v1 =
>+			(const struct intel_dmc_header_v1 *)dmc_header;
>+
>+		mmioaddr = v1->mmioaddr;
>+		mmiodata = v1->mmiodata;
>+		mmio_count = v1->mmio_count;
>+		mmio_count_max = DMC_V1_MAX_MMIO_COUNT;
>+		header_len_bytes = dmc_header->header_len;
>+		dmc_header_size = sizeof(*v1);
>+	} else {
>+		DRM_ERROR("Unknown DMC fw header version: %u\n",
>+			  dmc_header->header_ver);
>+		return 0;
>+	}
>+
>+	if (header_len_bytes != dmc_header_size) {
> 		DRM_ERROR("DMC firmware has wrong dmc header length "
>-			  "(%u bytes)\n",
>-			  (dmc_header->header_len));
>+			  "(%u bytes)\n", header_len_bytes);
> 		return 0;
> 	}
>
> 	/* Cache the dmc header info. */
>-	if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) {
>-		DRM_ERROR("DMC firmware has wrong mmio count %u\n",
>-			  dmc_header->mmio_count);
>+	if (mmio_count > mmio_count_max) {
>+		DRM_ERROR("DMC firmware has wrong mmio count %u\n",
>mmio_count);
> 		return 0;
> 	}
>
>-	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) {
>+	for (i = 0; i < mmio_count; i++) {
>+		if (mmioaddr[i] < CSR_MMIO_START_RANGE ||
>+		    mmioaddr[i] > CSR_MMIO_END_RANGE) {
> 			DRM_ERROR("DMC firmware has wrong mmio address
>0x%x\n",
>-				  dmc_header->mmioaddr[i]);
>+				  mmioaddr[i]);
> 			return 0;
> 		}
>-		csr->mmioaddr[i] = _MMIO(dmc_header->mmioaddr[i]);
>-		csr->mmiodata[i] = dmc_header->mmiodata[i];
>+		csr->mmioaddr[i] = _MMIO(mmioaddr[i]);
>+		csr->mmiodata[i] = mmiodata[i];
> 	}
>+	csr->mmio_count = mmio_count;
>
> 	/* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
> 	payload_size = dmc_header->fw_size * 4; @@ -379,12 +436,10 @@
>static u32 parse_csr_fw_dmc(struct intel_csr *csr,
> 		return 0;
> 	}
>
>-	r = sizeof(struct intel_dmc_header);
>-	payload = (u8 *)(dmc_header) + r;
>+	payload = (u8 *)(dmc_header) + header_len_bytes;
> 	memcpy(csr->dmc_payload, payload, payload_size);
>-	r += payload_size;
>
>-	return r;
>+	return header_len_bytes + payload_size;
> }
>
> static u32
>@@ -470,7 +525,7 @@ static u32 *parse_csr_fw(struct drm_i915_private
>*dev_priv,  {
> 	struct intel_css_header *css_header;
> 	struct intel_package_header *package_header;
>-	struct intel_dmc_header *dmc_header;
>+	struct intel_dmc_header_base *dmc_header;
> 	struct intel_csr *csr = &dev_priv->csr;
> 	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
> 	u32 readcount = 0;
>@@ -496,7 +551,7 @@ static u32 *parse_csr_fw(struct drm_i915_private
>*dev_priv,
> 	readcount += r;
>
> 	/* Extract dmc_header information. */
>-	dmc_header = (struct intel_dmc_header *)&fw->data[readcount];
>+	dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount];
> 	r = parse_csr_fw_dmc(csr, dmc_header);
> 	if (!r)
> 		return NULL;
>--
>2.21.0

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

Re: [PATCH 08/10] drm/i915/dmc: remove redundant return in parse_csr_fw()

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Thursday, May 23, 2019 1:24 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Jani Nikula <jani.nikula@linux.intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; De
>Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 08/10] drm/i915/dmc: remove redundant return in
>parse_csr_fw()
>
>parse_csr_fw() is responsible to set up several fields in struct intel_csr, including
>the payload. We don't need to assign it again.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Changes look good.
Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>---
> drivers/gpu/drm/i915/intel_csr.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>index ad4ee55a8c5e..7ff08de83cc6 100644
>--- a/drivers/gpu/drm/i915/intel_csr.c
>+++ b/drivers/gpu/drm/i915/intel_csr.c
>@@ -520,7 +520,7 @@ static u32 parse_csr_fw_css(struct intel_csr *csr,
> 	return sizeof(struct intel_css_header);  }
>
>-static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
>+static void parse_csr_fw(struct drm_i915_private *dev_priv,
> 			 const struct firmware *fw)
> {
> 	struct intel_css_header *css_header;
>@@ -532,13 +532,13 @@ static u32 *parse_csr_fw(struct drm_i915_private
>*dev_priv,
> 	u32 r;
>
> 	if (!fw)
>-		return NULL;
>+		return;
>
> 	/* Extract CSS Header information*/
> 	css_header = (struct intel_css_header *)fw->data;
> 	r = parse_csr_fw_css(csr, css_header);
> 	if (!r)
>-		return NULL;
>+		return;
>
> 	readcount += r;
>
>@@ -546,17 +546,13 @@ static u32 *parse_csr_fw(struct drm_i915_private
>*dev_priv,
> 	package_header = (struct intel_package_header *)&fw-
>>data[readcount];
> 	r = parse_csr_fw_package(csr, package_header, si);
> 	if (!r)
>-		return NULL;
>+		return;
>
> 	readcount += r;
>
> 	/* Extract dmc_header information. */
> 	dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount];
>-	r = parse_csr_fw_dmc(csr, dmc_header);
>-	if (!r)
>-		return NULL;
>-
>-	return csr->dmc_payload;
>+	parse_csr_fw_dmc(csr, dmc_header);
> }
>
> static void intel_csr_runtime_pm_get(struct drm_i915_private *dev_priv) @@ -
>584,8 +580,7 @@ static void csr_load_work_fn(struct work_struct *work)
> 	csr = &dev_priv->csr;
>
> 	request_firmware(&fw, dev_priv->csr.fw_path, &dev_priv->drm.pdev-
>>dev);
>-	if (fw)
>-		dev_priv->csr.dmc_payload = parse_csr_fw(dev_priv, fw);
>+	parse_csr_fw(dev_priv, fw);
>
> 	if (dev_priv->csr.dmc_payload) {
> 		intel_csr_load_program(dev_priv);
>--
>2.21.0

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

Re: [PATCH 07/10] drm/i915/dmc: add support to load dmc_header version 3

[Rodrigo Vivi]

On Thu, May 23, 2019 at 01:24:17AM -0700, Lucas De Marchi wrote:
> Main difference is that now there are up to 20 MMIOs that can be set and
> a lot of noise due to the struct changing the fields in the middle.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |   4 +-
>  drivers/gpu/drm/i915/intel_csr.c | 107 +++++++++++++++++++++++--------
>  2 files changed, 83 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1ad3818d2676..04a6b59256fd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -354,8 +354,8 @@ struct intel_csr {
>  	u32 dmc_fw_size; /* dwords */
>  	u32 version;
>  	u32 mmio_count;
> -	i915_reg_t mmioaddr[8];
> -	u32 mmiodata[8];
> +	i915_reg_t mmioaddr[20];
> +	u32 mmiodata[20];
>  	u32 dc_state;
>  	u32 allowed_dc_mask;
>  	intel_wakeref_t wakeref;
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 20dd4bd5feae..ad4ee55a8c5e 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -72,6 +72,8 @@ MODULE_FIRMWARE(BXT_CSR_PATH);
>  #define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
>  #define PACKAGE_MAX_FW_INFO_ENTRIES	20
>  #define PACKAGE_V2_MAX_FW_INFO_ENTRIES	32
> +#define DMC_V1_MAX_MMIO_COUNT		8
> +#define DMC_V3_MAX_MMIO_COUNT		20
>  
>  struct intel_css_header {
>  	/* 0x09 for DMC */
> @@ -143,7 +145,7 @@ struct intel_package_header {
>  	u32 num_entries;
>  } __packed;
>  
> -struct intel_dmc_header {
> +struct intel_dmc_header_base {
>  	/* always value would be 0x40403E3E */
>  	u32 signature;
>  
> @@ -164,22 +166,47 @@ struct intel_dmc_header {
>  
>  	/* Major Minor version */
>  	u32 fw_version;
> +} __packed;
> +
> +struct intel_dmc_header_v1 {
> +	struct intel_dmc_header_base base;
>  
>  	/* Number of valid MMIO cycles present. */
>  	u32 mmio_count;
>  
>  	/* MMIO address */
> -	u32 mmioaddr[8];
> +	u32 mmioaddr[DMC_V1_MAX_MMIO_COUNT];
>  
>  	/* MMIO data */
> -	u32 mmiodata[8];
> +	u32 mmiodata[DMC_V1_MAX_MMIO_COUNT];
>  
>  	/* FW filename  */
> -	unsigned char dfile[32];
> +	char dfile[32];
>  
>  	u32 reserved1[2];
>  } __packed;
>  
> +struct intel_dmc_header_v3 {
> +	struct intel_dmc_header_base base;
> +
> +	/* DMC RAM start MMIO address */
> +	u32 start_mmioaddr;
> +
> +	u32 reserved[9];
> +
> +	/* FW filename */
> +	char dfile[32];
> +
> +	/* Number of valid MMIO cycles present. */
> +	u32 mmio_count;
> +
> +	/* MMIO address */
> +	u32 mmioaddr[DMC_V3_MAX_MMIO_COUNT];
> +
> +	/* MMIO data */
> +	u32 mmiodata[DMC_V3_MAX_MMIO_COUNT];
> +} __packed;
> +
>  struct stepping_info {
>  	char stepping;
>  	char substepping;
> @@ -333,37 +360,67 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
>  }
>  
>  static u32 parse_csr_fw_dmc(struct intel_csr *csr,
> -			    const struct intel_dmc_header *dmc_header)
> +			    const struct intel_dmc_header_base *dmc_header)
>  {
> -	unsigned int i, payload_size;
> -	u32 r;
> +	unsigned int header_len_bytes, dmc_header_size, payload_size, i;
> +	const u32 *mmioaddr, *mmiodata;
> +	u32 mmio_count, mmio_count_max;
>  	u8 *payload;
>  
> -	if (sizeof(struct intel_dmc_header) != dmc_header->header_len) {
> +	BUILD_BUG_ON(ARRAY_SIZE(csr->mmioaddr) < DMC_V3_MAX_MMIO_COUNT ||
> +		     ARRAY_SIZE(csr->mmioaddr) < DMC_V1_MAX_MMIO_COUNT);
> +
> +	/* Cope with small differences between v1 and v3 */
> +	if (dmc_header->header_ver == 3) {

I'm missing on this patch something like:

- /* 0x01, 0x02 */
+  /* 0x01, 0x02, 0x03 */
near header_ver definition

or maybe kill that comment at all...

The rest seems right so feel free to use
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +		const struct intel_dmc_header_v3 *v3 =
> +			(const struct intel_dmc_header_v3 *)dmc_header;
> +
> +		mmioaddr = v3->mmioaddr;
> +		mmiodata = v3->mmiodata;
> +		mmio_count = v3->mmio_count;
> +		mmio_count_max = DMC_V3_MAX_MMIO_COUNT;
> +		/* header_len is in dwords */
> +		header_len_bytes = dmc_header->header_len * 4;
> +		dmc_header_size = sizeof(*v3);
> +	} else if (dmc_header->header_ver == 1) {
> +		const struct intel_dmc_header_v1 *v1 =
> +			(const struct intel_dmc_header_v1 *)dmc_header;
> +
> +		mmioaddr = v1->mmioaddr;
> +		mmiodata = v1->mmiodata;
> +		mmio_count = v1->mmio_count;
> +		mmio_count_max = DMC_V1_MAX_MMIO_COUNT;
> +		header_len_bytes = dmc_header->header_len;
> +		dmc_header_size = sizeof(*v1);
> +	} else {
> +		DRM_ERROR("Unknown DMC fw header version: %u\n",
> +			  dmc_header->header_ver);
> +		return 0;
> +	}
> +
> +	if (header_len_bytes != dmc_header_size) {
>  		DRM_ERROR("DMC firmware has wrong dmc header length "
> -			  "(%u bytes)\n",
> -			  (dmc_header->header_len));
> +			  "(%u bytes)\n", header_len_bytes);
>  		return 0;
>  	}
>  
>  	/* Cache the dmc header info. */
> -	if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) {
> -		DRM_ERROR("DMC firmware has wrong mmio count %u\n",
> -			  dmc_header->mmio_count);
> +	if (mmio_count > mmio_count_max) {
> +		DRM_ERROR("DMC firmware has wrong mmio count %u\n", mmio_count);
>  		return 0;
>  	}
>  
> -	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) {
> +	for (i = 0; i < mmio_count; i++) {
> +		if (mmioaddr[i] < CSR_MMIO_START_RANGE ||
> +		    mmioaddr[i] > CSR_MMIO_END_RANGE) {
>  			DRM_ERROR("DMC firmware has wrong mmio address 0x%x\n",
> -				  dmc_header->mmioaddr[i]);
> +				  mmioaddr[i]);
>  			return 0;
>  		}
> -		csr->mmioaddr[i] = _MMIO(dmc_header->mmioaddr[i]);
> -		csr->mmiodata[i] = dmc_header->mmiodata[i];
> +		csr->mmioaddr[i] = _MMIO(mmioaddr[i]);
> +		csr->mmiodata[i] = mmiodata[i];
>  	}
> +	csr->mmio_count = mmio_count;
>  
>  	/* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
>  	payload_size = dmc_header->fw_size * 4;
> @@ -379,12 +436,10 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>  		return 0;
>  	}
>  
> -	r = sizeof(struct intel_dmc_header);
> -	payload = (u8 *)(dmc_header) + r;
> +	payload = (u8 *)(dmc_header) + header_len_bytes;
>  	memcpy(csr->dmc_payload, payload, payload_size);
> -	r += payload_size;
>  
> -	return r;
> +	return header_len_bytes + payload_size;
>  }
>  
>  static u32
> @@ -470,7 +525,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
>  {
>  	struct intel_css_header *css_header;
>  	struct intel_package_header *package_header;
> -	struct intel_dmc_header *dmc_header;
> +	struct intel_dmc_header_base *dmc_header;
>  	struct intel_csr *csr = &dev_priv->csr;
>  	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
>  	u32 readcount = 0;
> @@ -496,7 +551,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
>  	readcount += r;
>  
>  	/* Extract dmc_header information. */
> -	dmc_header = (struct intel_dmc_header *)&fw->data[readcount];
> +	dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount];
>  	r = parse_csr_fw_dmc(csr, dmc_header);
>  	if (!r)
>  		return NULL;
> -- 
> 2.21.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/10] drm/i915/dmc: protect against reading random memory

[Rodrigo Vivi]

On Thu, May 23, 2019 at 01:24:19AM -0700, Lucas De Marchi wrote:
> While loading the DMC firmware we were double checking the headers made
> sense, but in no place we checked that we were actually reading memory
> we were supposed to. This could be wrong in case the firmware file is
> truncated.
> 
> Before parsing any part of the firmware, validate the input first.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

I wonder if this patch should be the first one on the series
for easy backport if needed.
Maybe cc: Stable?

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_csr.c | 71 ++++++++++++++++++++++++--------
>  1 file changed, 53 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 7ff08de83cc6..b7181ca6c8f5 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -360,7 +360,8 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
>  }
>  
>  static u32 parse_csr_fw_dmc(struct intel_csr *csr,
> -			    const struct intel_dmc_header_base *dmc_header)
> +			    const struct intel_dmc_header_base *dmc_header,
> +			    size_t rem_size)
>  {
>  	unsigned int header_len_bytes, dmc_header_size, payload_size, i;
>  	const u32 *mmioaddr, *mmiodata;
> @@ -375,6 +376,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>  		const struct intel_dmc_header_v3 *v3 =
>  			(const struct intel_dmc_header_v3 *)dmc_header;
>  
> +		if (rem_size < sizeof(struct intel_dmc_header_v3))
> +			goto error_truncated;
> +
>  		mmioaddr = v3->mmioaddr;
>  		mmiodata = v3->mmiodata;
>  		mmio_count = v3->mmio_count;
> @@ -386,6 +390,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>  		const struct intel_dmc_header_v1 *v1 =
>  			(const struct intel_dmc_header_v1 *)dmc_header;
>  
> +		if (rem_size < sizeof(struct intel_dmc_header_v1))
> +			goto error_truncated;
> +
>  		mmioaddr = v1->mmioaddr;
>  		mmiodata = v1->mmiodata;
>  		mmio_count = v1->mmio_count;
> @@ -404,6 +411,8 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>  		return 0;
>  	}
>  
> +	rem_size -= header_len_bytes;
> +
>  	/* Cache the dmc header info. */
>  	if (mmio_count > mmio_count_max) {
>  		DRM_ERROR("DMC firmware has wrong mmio count %u\n", mmio_count);
> @@ -424,6 +433,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>  
>  	/* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
>  	payload_size = dmc_header->fw_size * 4;
> +	if (rem_size < payload_size)
> +		goto error_truncated;
> +
>  	if (payload_size > csr->max_fw_size) {
>  		DRM_ERROR("DMC FW too big (%u bytes)\n", payload_size);
>  		return 0;
> @@ -440,16 +452,25 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>  	memcpy(csr->dmc_payload, payload, payload_size);
>  
>  	return header_len_bytes + payload_size;
> +
> +error_truncated:
> +	DRM_ERROR("Truncated DMC firmware, refusing.\n");
> +	return 0;
>  }
>  
>  static u32
>  parse_csr_fw_package(struct intel_csr *csr,
>  		     const struct intel_package_header *package_header,
> -		     const struct stepping_info *si)
> +		     const struct stepping_info *si,
> +		     size_t rem_size)
>  {
> -	u32 num_entries, max_entries, dmc_offset, r;
> +	u32 package_size = sizeof(struct intel_package_header);
> +	u32 num_entries, max_entries, dmc_offset;
>  	const struct intel_fw_info *fw_info;
>  
> +	if (rem_size < package_size)
> +		goto error_truncated;
> +
>  	if (package_header->header_ver == 1) {
>  		max_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
>  	} else if (package_header->header_ver == 2) {
> @@ -460,11 +481,17 @@ parse_csr_fw_package(struct intel_csr *csr,
>  		return 0;
>  	}
>  
> -	if (package_header->header_len * 4 !=
> -	    sizeof(struct intel_package_header) +
> -	    max_entries * sizeof(struct intel_fw_info)) {
> +	/*
> +	 * We should always have space for max_entries,
> +	 * even if not all are used
> +	 */
> +	package_size += max_entries * sizeof(struct intel_fw_info);
> +	if (rem_size < package_size)
> +		goto error_truncated;
> +
> +	if (package_header->header_len * 4 != package_size) {
>  		DRM_ERROR("DMC firmware has wrong package header length "
> -			  "(%u bytes)\n", package_header->header_len * 4);
> +			  "(%u bytes)\n", package_size);
>  		return 0;
>  	}
>  
> @@ -481,21 +508,24 @@ parse_csr_fw_package(struct intel_csr *csr,
>  		return 0;
>  	}
>  
> -	r = sizeof(struct intel_package_header);
> -
> -	/* we always have space for max_entries, even if not all are used */
> -	r += max_entries * sizeof(struct intel_fw_info);
> -
>  	/* dmc_offset is in dwords */
> -	r += dmc_offset * 4;
> +	return package_size + dmc_offset * 4;
>  
> -	return r;
> +error_truncated:
> +	DRM_ERROR("Truncated DMC firmware, refusing.\n");
> +	return 0;
>  }
>  
>  /* Return number of bytes parsed or 0 on error */
>  static u32 parse_csr_fw_css(struct intel_csr *csr,
> -			    struct intel_css_header *css_header)
> +			    struct intel_css_header *css_header,
> +			    size_t rem_size)
>  {
> +	if (rem_size < sizeof(struct intel_css_header)) {
> +		DRM_ERROR("Truncated DMC firmware, refusing.\n");
> +		return 0;
> +	}
> +
>  	if (sizeof(struct intel_css_header) !=
>  	    (css_header->header_len * 4)) {
>  		DRM_ERROR("DMC firmware has wrong CSS header length "
> @@ -530,29 +560,34 @@ static void parse_csr_fw(struct drm_i915_private *dev_priv,
>  	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
>  	u32 readcount = 0;
>  	u32 r;
> +	size_t rem_size;
>  
>  	if (!fw)
>  		return;
>  
> +	rem_size = fw->size;
> +
>  	/* Extract CSS Header information*/
>  	css_header = (struct intel_css_header *)fw->data;
> -	r = parse_csr_fw_css(csr, css_header);
> +	r = parse_csr_fw_css(csr, css_header, rem_size);
>  	if (!r)
>  		return;
>  
> +	rem_size -= r;
>  	readcount += r;
>  
>  	/* Extract Package Header information*/
>  	package_header = (struct intel_package_header *)&fw->data[readcount];
> -	r = parse_csr_fw_package(csr, package_header, si);
> +	r = parse_csr_fw_package(csr, package_header, si, rem_size);
>  	if (!r)
>  		return;
>  
> +	rem_size -= r;
>  	readcount += r;
>  
>  	/* Extract dmc_header information. */
>  	dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount];
> -	parse_csr_fw_dmc(csr, dmc_header);
> +	parse_csr_fw_dmc(csr, dmc_header, rem_size);
>  }
>  
>  static void intel_csr_runtime_pm_get(struct drm_i915_private *dev_priv)
> -- 
> 2.21.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/10] drm/i915/dmc: protect against loading wrong firmware

[Rodrigo Vivi]

On Thu, May 23, 2019 at 01:24:20AM -0700, Lucas De Marchi wrote:
> In intel_package_header version 2 there's a new field in the
> fw_info table that must be 0, otherwise it's not the correct DMC
> firmware. Add a check for version 2 or later.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_csr.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index b7181ca6c8f5..d4996dcf596c 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -120,7 +120,10 @@ struct intel_css_header {
>  } __packed;
>  
>  struct intel_fw_info {
> -	u16 reserved1;
> +	u8 reserved1;
> +
> +	/* reserved on package_header version 1, must be 0 on version 2 */

2+?
> 1?
>= 2?

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +	u8 dmc_id;
>  
>  	/* Stepping (A, B, C, ..., *). * is a wildcard */
>  	char stepping;
> @@ -325,12 +328,16 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv)
>   */
>  static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
>  			      unsigned int num_entries,
> -			      const struct stepping_info *si)
> +			      const struct stepping_info *si,
> +			      u8 package_ver)
>  {
>  	u32 dmc_offset = CSR_DEFAULT_FW_OFFSET;
>  	unsigned int i;
>  
>  	for (i = 0; i < num_entries; i++) {
> +		if (package_ver > 1 && fw_info[i].dmc_id != 0)
> +			continue;
> +
>  		if (fw_info[i].substepping == '*' &&
>  		    si->stepping == fw_info[i].stepping) {
>  			dmc_offset = fw_info[i].offset;
> @@ -501,7 +508,8 @@ parse_csr_fw_package(struct intel_csr *csr,
>  
>  	fw_info = (const struct intel_fw_info *)
>  		((u8 *)package_header + sizeof(*package_header));
> -	dmc_offset = find_dmc_fw_offset(fw_info, num_entries, si);
> +	dmc_offset = find_dmc_fw_offset(fw_info, num_entries, si,
> +					package_header->header_ver);
>  	if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
>  		DRM_ERROR("DMC firmware not supported for %c stepping\n",
>  			  si->stepping);
> -- 
> 2.21.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/10] drm/i915/dmc: add support to load dmc_header version 3

[Lucas De Marchi]

On Thu, May 23, 2019 at 11:57:19AM -0700, Rodrigo Vivi wrote:
>On Thu, May 23, 2019 at 01:24:17AM -0700, Lucas De Marchi wrote:
>> Main difference is that now there are up to 20 MMIOs that can be set and
>> a lot of noise due to the struct changing the fields in the middle.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |   4 +-
>>  drivers/gpu/drm/i915/intel_csr.c | 107 +++++++++++++++++++++++--------
>>  2 files changed, 83 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 1ad3818d2676..04a6b59256fd 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -354,8 +354,8 @@ struct intel_csr {
>>  	u32 dmc_fw_size; /* dwords */
>>  	u32 version;
>>  	u32 mmio_count;
>> -	i915_reg_t mmioaddr[8];
>> -	u32 mmiodata[8];
>> +	i915_reg_t mmioaddr[20];
>> +	u32 mmiodata[20];
>>  	u32 dc_state;
>>  	u32 allowed_dc_mask;
>>  	intel_wakeref_t wakeref;
>> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>> index 20dd4bd5feae..ad4ee55a8c5e 100644
>> --- a/drivers/gpu/drm/i915/intel_csr.c
>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>> @@ -72,6 +72,8 @@ MODULE_FIRMWARE(BXT_CSR_PATH);
>>  #define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
>>  #define PACKAGE_MAX_FW_INFO_ENTRIES	20
>>  #define PACKAGE_V2_MAX_FW_INFO_ENTRIES	32
>> +#define DMC_V1_MAX_MMIO_COUNT		8
>> +#define DMC_V3_MAX_MMIO_COUNT		20
>>
>>  struct intel_css_header {
>>  	/* 0x09 for DMC */
>> @@ -143,7 +145,7 @@ struct intel_package_header {
>>  	u32 num_entries;
>>  } __packed;
>>
>> -struct intel_dmc_header {
>> +struct intel_dmc_header_base {
>>  	/* always value would be 0x40403E3E */
>>  	u32 signature;
>>
>> @@ -164,22 +166,47 @@ struct intel_dmc_header {
>>
>>  	/* Major Minor version */
>>  	u32 fw_version;
>> +} __packed;
>> +
>> +struct intel_dmc_header_v1 {
>> +	struct intel_dmc_header_base base;
>>
>>  	/* Number of valid MMIO cycles present. */
>>  	u32 mmio_count;
>>
>>  	/* MMIO address */
>> -	u32 mmioaddr[8];
>> +	u32 mmioaddr[DMC_V1_MAX_MMIO_COUNT];
>>
>>  	/* MMIO data */
>> -	u32 mmiodata[8];
>> +	u32 mmiodata[DMC_V1_MAX_MMIO_COUNT];
>>
>>  	/* FW filename  */
>> -	unsigned char dfile[32];
>> +	char dfile[32];
>>
>>  	u32 reserved1[2];
>>  } __packed;
>>
>> +struct intel_dmc_header_v3 {
>> +	struct intel_dmc_header_base base;
>> +
>> +	/* DMC RAM start MMIO address */
>> +	u32 start_mmioaddr;
>> +
>> +	u32 reserved[9];
>> +
>> +	/* FW filename */
>> +	char dfile[32];
>> +
>> +	/* Number of valid MMIO cycles present. */
>> +	u32 mmio_count;
>> +
>> +	/* MMIO address */
>> +	u32 mmioaddr[DMC_V3_MAX_MMIO_COUNT];
>> +
>> +	/* MMIO data */
>> +	u32 mmiodata[DMC_V3_MAX_MMIO_COUNT];
>> +} __packed;
>> +
>>  struct stepping_info {
>>  	char stepping;
>>  	char substepping;
>> @@ -333,37 +360,67 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
>>  }
>>
>>  static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>> -			    const struct intel_dmc_header *dmc_header)
>> +			    const struct intel_dmc_header_base *dmc_header)
>>  {
>> -	unsigned int i, payload_size;
>> -	u32 r;
>> +	unsigned int header_len_bytes, dmc_header_size, payload_size, i;
>> +	const u32 *mmioaddr, *mmiodata;
>> +	u32 mmio_count, mmio_count_max;
>>  	u8 *payload;
>>
>> -	if (sizeof(struct intel_dmc_header) != dmc_header->header_len) {
>> +	BUILD_BUG_ON(ARRAY_SIZE(csr->mmioaddr) < DMC_V3_MAX_MMIO_COUNT ||
>> +		     ARRAY_SIZE(csr->mmioaddr) < DMC_V1_MAX_MMIO_COUNT);
>> +
>> +	/* Cope with small differences between v1 and v3 */
>> +	if (dmc_header->header_ver == 3) {
>
>I'm missing on this patch something like:
>
>- /* 0x01, 0x02 */
>+  /* 0x01, 0x02, 0x03 */
>near header_ver definition
>
>or maybe kill that comment at all...

nah, that is another version and it's a bit confusing. But we have
*package* header version: 1 or 2
*dmc* header version: 1 or 3

Overall structure of the firmware:

.------------.
| CSS Header |
|------------|
| Package    |
| Header     |
|------------|
| 20 or 32   |         * 20 for package header v1
| fw_info    |--.-.-.    32 for package header v2
|------------|<-' | |
| DMC header |    | |
| DMC payload|    | |
|------------|<---' |
| DMC header |      |
| DMC payload|      |
|------------|      .
     ...            .
|------------|<-----'
| DMC header |
| DMC payload|
'------------'

here the dmc_header v3 will dictate how the *DMC header*
will be parsed, that can have either 8 or 20 MMIOs to be programmed,
besides the firmware to be loaded.

I guess I could add a patch on top with this sketch as a comment to
better explain how is the layout of the file.


Lucas De Marchi

>
>The rest seems right so feel free to use
>Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
>> +		const struct intel_dmc_header_v3 *v3 =
>> +			(const struct intel_dmc_header_v3 *)dmc_header;
>> +
>> +		mmioaddr = v3->mmioaddr;
>> +		mmiodata = v3->mmiodata;
>> +		mmio_count = v3->mmio_count;
>> +		mmio_count_max = DMC_V3_MAX_MMIO_COUNT;
>> +		/* header_len is in dwords */
>> +		header_len_bytes = dmc_header->header_len * 4;
>> +		dmc_header_size = sizeof(*v3);
>> +	} else if (dmc_header->header_ver == 1) {
>> +		const struct intel_dmc_header_v1 *v1 =
>> +			(const struct intel_dmc_header_v1 *)dmc_header;
>> +
>> +		mmioaddr = v1->mmioaddr;
>> +		mmiodata = v1->mmiodata;
>> +		mmio_count = v1->mmio_count;
>> +		mmio_count_max = DMC_V1_MAX_MMIO_COUNT;
>> +		header_len_bytes = dmc_header->header_len;
>> +		dmc_header_size = sizeof(*v1);
>> +	} else {
>> +		DRM_ERROR("Unknown DMC fw header version: %u\n",
>> +			  dmc_header->header_ver);
>> +		return 0;
>> +	}
>> +
>> +	if (header_len_bytes != dmc_header_size) {
>>  		DRM_ERROR("DMC firmware has wrong dmc header length "
>> -			  "(%u bytes)\n",
>> -			  (dmc_header->header_len));
>> +			  "(%u bytes)\n", header_len_bytes);
>>  		return 0;
>>  	}
>>
>>  	/* Cache the dmc header info. */
>> -	if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) {
>> -		DRM_ERROR("DMC firmware has wrong mmio count %u\n",
>> -			  dmc_header->mmio_count);
>> +	if (mmio_count > mmio_count_max) {
>> +		DRM_ERROR("DMC firmware has wrong mmio count %u\n", mmio_count);
>>  		return 0;
>>  	}
>>
>> -	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) {
>> +	for (i = 0; i < mmio_count; i++) {
>> +		if (mmioaddr[i] < CSR_MMIO_START_RANGE ||
>> +		    mmioaddr[i] > CSR_MMIO_END_RANGE) {
>>  			DRM_ERROR("DMC firmware has wrong mmio address 0x%x\n",
>> -				  dmc_header->mmioaddr[i]);
>> +				  mmioaddr[i]);
>>  			return 0;
>>  		}
>> -		csr->mmioaddr[i] = _MMIO(dmc_header->mmioaddr[i]);
>> -		csr->mmiodata[i] = dmc_header->mmiodata[i];
>> +		csr->mmioaddr[i] = _MMIO(mmioaddr[i]);
>> +		csr->mmiodata[i] = mmiodata[i];
>>  	}
>> +	csr->mmio_count = mmio_count;
>>
>>  	/* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
>>  	payload_size = dmc_header->fw_size * 4;
>> @@ -379,12 +436,10 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>>  		return 0;
>>  	}
>>
>> -	r = sizeof(struct intel_dmc_header);
>> -	payload = (u8 *)(dmc_header) + r;
>> +	payload = (u8 *)(dmc_header) + header_len_bytes;
>>  	memcpy(csr->dmc_payload, payload, payload_size);
>> -	r += payload_size;
>>
>> -	return r;
>> +	return header_len_bytes + payload_size;
>>  }
>>
>>  static u32
>> @@ -470,7 +525,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
>>  {
>>  	struct intel_css_header *css_header;
>>  	struct intel_package_header *package_header;
>> -	struct intel_dmc_header *dmc_header;
>> +	struct intel_dmc_header_base *dmc_header;
>>  	struct intel_csr *csr = &dev_priv->csr;
>>  	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
>>  	u32 readcount = 0;
>> @@ -496,7 +551,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
>>  	readcount += r;
>>
>>  	/* Extract dmc_header information. */
>> -	dmc_header = (struct intel_dmc_header *)&fw->data[readcount];
>> +	dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount];
>>  	r = parse_csr_fw_dmc(csr, dmc_header);
>>  	if (!r)
>>  		return NULL;
>> --
>> 2.21.0
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/10] drm/i915/dmc: protect against reading random memory

[Lucas De Marchi]

On Thu, May 23, 2019 at 11:58:46AM -0700, Rodrigo Vivi wrote:
>On Thu, May 23, 2019 at 01:24:19AM -0700, Lucas De Marchi wrote:
>> While loading the DMC firmware we were double checking the headers made
>> sense, but in no place we checked that we were actually reading memory
>> we were supposed to. This could be wrong in case the firmware file is
>> truncated.
>>
>> Before parsing any part of the firmware, validate the input first.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>
>I wonder if this patch should be the first one on the series
>for easy backport if needed.

yeah, too bad I only noticed after doing the first ones.
This patch as the first one will be very different, but I agree that is
what I need to do.

>Maybe cc: Stable?

yep. I will wait a little more on review and send a new version with
order swapped.

>
>Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

thanks
Lucas De Marchi

>
>> ---
>>  drivers/gpu/drm/i915/intel_csr.c | 71 ++++++++++++++++++++++++--------
>>  1 file changed, 53 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>> index 7ff08de83cc6..b7181ca6c8f5 100644
>> --- a/drivers/gpu/drm/i915/intel_csr.c
>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>> @@ -360,7 +360,8 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
>>  }
>>
>>  static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>> -			    const struct intel_dmc_header_base *dmc_header)
>> +			    const struct intel_dmc_header_base *dmc_header,
>> +			    size_t rem_size)
>>  {
>>  	unsigned int header_len_bytes, dmc_header_size, payload_size, i;
>>  	const u32 *mmioaddr, *mmiodata;
>> @@ -375,6 +376,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>>  		const struct intel_dmc_header_v3 *v3 =
>>  			(const struct intel_dmc_header_v3 *)dmc_header;
>>
>> +		if (rem_size < sizeof(struct intel_dmc_header_v3))
>> +			goto error_truncated;
>> +
>>  		mmioaddr = v3->mmioaddr;
>>  		mmiodata = v3->mmiodata;
>>  		mmio_count = v3->mmio_count;
>> @@ -386,6 +390,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>>  		const struct intel_dmc_header_v1 *v1 =
>>  			(const struct intel_dmc_header_v1 *)dmc_header;
>>
>> +		if (rem_size < sizeof(struct intel_dmc_header_v1))
>> +			goto error_truncated;
>> +
>>  		mmioaddr = v1->mmioaddr;
>>  		mmiodata = v1->mmiodata;
>>  		mmio_count = v1->mmio_count;
>> @@ -404,6 +411,8 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>>  		return 0;
>>  	}
>>
>> +	rem_size -= header_len_bytes;
>> +
>>  	/* Cache the dmc header info. */
>>  	if (mmio_count > mmio_count_max) {
>>  		DRM_ERROR("DMC firmware has wrong mmio count %u\n", mmio_count);
>> @@ -424,6 +433,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>>
>>  	/* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
>>  	payload_size = dmc_header->fw_size * 4;
>> +	if (rem_size < payload_size)
>> +		goto error_truncated;
>> +
>>  	if (payload_size > csr->max_fw_size) {
>>  		DRM_ERROR("DMC FW too big (%u bytes)\n", payload_size);
>>  		return 0;
>> @@ -440,16 +452,25 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
>>  	memcpy(csr->dmc_payload, payload, payload_size);
>>
>>  	return header_len_bytes + payload_size;
>> +
>> +error_truncated:
>> +	DRM_ERROR("Truncated DMC firmware, refusing.\n");
>> +	return 0;
>>  }
>>
>>  static u32
>>  parse_csr_fw_package(struct intel_csr *csr,
>>  		     const struct intel_package_header *package_header,
>> -		     const struct stepping_info *si)
>> +		     const struct stepping_info *si,
>> +		     size_t rem_size)
>>  {
>> -	u32 num_entries, max_entries, dmc_offset, r;
>> +	u32 package_size = sizeof(struct intel_package_header);
>> +	u32 num_entries, max_entries, dmc_offset;
>>  	const struct intel_fw_info *fw_info;
>>
>> +	if (rem_size < package_size)
>> +		goto error_truncated;
>> +
>>  	if (package_header->header_ver == 1) {
>>  		max_entries = PACKAGE_MAX_FW_INFO_ENTRIES;
>>  	} else if (package_header->header_ver == 2) {
>> @@ -460,11 +481,17 @@ parse_csr_fw_package(struct intel_csr *csr,
>>  		return 0;
>>  	}
>>
>> -	if (package_header->header_len * 4 !=
>> -	    sizeof(struct intel_package_header) +
>> -	    max_entries * sizeof(struct intel_fw_info)) {
>> +	/*
>> +	 * We should always have space for max_entries,
>> +	 * even if not all are used
>> +	 */
>> +	package_size += max_entries * sizeof(struct intel_fw_info);
>> +	if (rem_size < package_size)
>> +		goto error_truncated;
>> +
>> +	if (package_header->header_len * 4 != package_size) {
>>  		DRM_ERROR("DMC firmware has wrong package header length "
>> -			  "(%u bytes)\n", package_header->header_len * 4);
>> +			  "(%u bytes)\n", package_size);
>>  		return 0;
>>  	}
>>
>> @@ -481,21 +508,24 @@ parse_csr_fw_package(struct intel_csr *csr,
>>  		return 0;
>>  	}
>>
>> -	r = sizeof(struct intel_package_header);
>> -
>> -	/* we always have space for max_entries, even if not all are used */
>> -	r += max_entries * sizeof(struct intel_fw_info);
>> -
>>  	/* dmc_offset is in dwords */
>> -	r += dmc_offset * 4;
>> +	return package_size + dmc_offset * 4;
>>
>> -	return r;
>> +error_truncated:
>> +	DRM_ERROR("Truncated DMC firmware, refusing.\n");
>> +	return 0;
>>  }
>>
>>  /* Return number of bytes parsed or 0 on error */
>>  static u32 parse_csr_fw_css(struct intel_csr *csr,
>> -			    struct intel_css_header *css_header)
>> +			    struct intel_css_header *css_header,
>> +			    size_t rem_size)
>>  {
>> +	if (rem_size < sizeof(struct intel_css_header)) {
>> +		DRM_ERROR("Truncated DMC firmware, refusing.\n");
>> +		return 0;
>> +	}
>> +
>>  	if (sizeof(struct intel_css_header) !=
>>  	    (css_header->header_len * 4)) {
>>  		DRM_ERROR("DMC firmware has wrong CSS header length "
>> @@ -530,29 +560,34 @@ static void parse_csr_fw(struct drm_i915_private *dev_priv,
>>  	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
>>  	u32 readcount = 0;
>>  	u32 r;
>> +	size_t rem_size;
>>
>>  	if (!fw)
>>  		return;
>>
>> +	rem_size = fw->size;
>> +
>>  	/* Extract CSS Header information*/
>>  	css_header = (struct intel_css_header *)fw->data;
>> -	r = parse_csr_fw_css(csr, css_header);
>> +	r = parse_csr_fw_css(csr, css_header, rem_size);
>>  	if (!r)
>>  		return;
>>
>> +	rem_size -= r;
>>  	readcount += r;
>>
>>  	/* Extract Package Header information*/
>>  	package_header = (struct intel_package_header *)&fw->data[readcount];
>> -	r = parse_csr_fw_package(csr, package_header, si);
>> +	r = parse_csr_fw_package(csr, package_header, si, rem_size);
>>  	if (!r)
>>  		return;
>>
>> +	rem_size -= r;
>>  	readcount += r;
>>
>>  	/* Extract dmc_header information. */
>>  	dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount];
>> -	parse_csr_fw_dmc(csr, dmc_header);
>> +	parse_csr_fw_dmc(csr, dmc_header, rem_size);
>>  }
>>
>>  static void intel_csr_runtime_pm_get(struct drm_i915_private *dev_priv)
>> --
>> 2.21.0
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/10] drm/i915/dmc: add support to load dmc_header version 3

[Rodrigo Vivi]

On Thu, May 23, 2019 at 12:25:34PM -0700, Lucas De Marchi wrote:
> On Thu, May 23, 2019 at 11:57:19AM -0700, Rodrigo Vivi wrote:
> > On Thu, May 23, 2019 at 01:24:17AM -0700, Lucas De Marchi wrote:
> > > Main difference is that now there are up to 20 MMIOs that can be set and
> > > a lot of noise due to the struct changing the fields in the middle.
> > > 
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  |   4 +-
> > >  drivers/gpu/drm/i915/intel_csr.c | 107 +++++++++++++++++++++++--------
> > >  2 files changed, 83 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 1ad3818d2676..04a6b59256fd 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -354,8 +354,8 @@ struct intel_csr {
> > >  	u32 dmc_fw_size; /* dwords */
> > >  	u32 version;
> > >  	u32 mmio_count;
> > > -	i915_reg_t mmioaddr[8];
> > > -	u32 mmiodata[8];
> > > +	i915_reg_t mmioaddr[20];
> > > +	u32 mmiodata[20];
> > >  	u32 dc_state;
> > >  	u32 allowed_dc_mask;
> > >  	intel_wakeref_t wakeref;
> > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> > > index 20dd4bd5feae..ad4ee55a8c5e 100644
> > > --- a/drivers/gpu/drm/i915/intel_csr.c
> > > +++ b/drivers/gpu/drm/i915/intel_csr.c
> > > @@ -72,6 +72,8 @@ MODULE_FIRMWARE(BXT_CSR_PATH);
> > >  #define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
> > >  #define PACKAGE_MAX_FW_INFO_ENTRIES	20
> > >  #define PACKAGE_V2_MAX_FW_INFO_ENTRIES	32
> > > +#define DMC_V1_MAX_MMIO_COUNT		8
> > > +#define DMC_V3_MAX_MMIO_COUNT		20
> > > 
> > >  struct intel_css_header {
> > >  	/* 0x09 for DMC */
> > > @@ -143,7 +145,7 @@ struct intel_package_header {
> > >  	u32 num_entries;
> > >  } __packed;
> > > 
> > > -struct intel_dmc_header {
> > > +struct intel_dmc_header_base {
> > >  	/* always value would be 0x40403E3E */
> > >  	u32 signature;
> > > 
> > > @@ -164,22 +166,47 @@ struct intel_dmc_header {
> > > 
> > >  	/* Major Minor version */
> > >  	u32 fw_version;
> > > +} __packed;
> > > +
> > > +struct intel_dmc_header_v1 {
> > > +	struct intel_dmc_header_base base;
> > > 
> > >  	/* Number of valid MMIO cycles present. */
> > >  	u32 mmio_count;
> > > 
> > >  	/* MMIO address */
> > > -	u32 mmioaddr[8];
> > > +	u32 mmioaddr[DMC_V1_MAX_MMIO_COUNT];
> > > 
> > >  	/* MMIO data */
> > > -	u32 mmiodata[8];
> > > +	u32 mmiodata[DMC_V1_MAX_MMIO_COUNT];
> > > 
> > >  	/* FW filename  */
> > > -	unsigned char dfile[32];
> > > +	char dfile[32];
> > > 
> > >  	u32 reserved1[2];
> > >  } __packed;
> > > 
> > > +struct intel_dmc_header_v3 {
> > > +	struct intel_dmc_header_base base;
> > > +
> > > +	/* DMC RAM start MMIO address */
> > > +	u32 start_mmioaddr;
> > > +
> > > +	u32 reserved[9];
> > > +
> > > +	/* FW filename */
> > > +	char dfile[32];
> > > +
> > > +	/* Number of valid MMIO cycles present. */
> > > +	u32 mmio_count;
> > > +
> > > +	/* MMIO address */
> > > +	u32 mmioaddr[DMC_V3_MAX_MMIO_COUNT];
> > > +
> > > +	/* MMIO data */
> > > +	u32 mmiodata[DMC_V3_MAX_MMIO_COUNT];
> > > +} __packed;
> > > +
> > >  struct stepping_info {
> > >  	char stepping;
> > >  	char substepping;
> > > @@ -333,37 +360,67 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info,
> > >  }
> > > 
> > >  static u32 parse_csr_fw_dmc(struct intel_csr *csr,
> > > -			    const struct intel_dmc_header *dmc_header)
> > > +			    const struct intel_dmc_header_base *dmc_header)
> > >  {
> > > -	unsigned int i, payload_size;
> > > -	u32 r;
> > > +	unsigned int header_len_bytes, dmc_header_size, payload_size, i;
> > > +	const u32 *mmioaddr, *mmiodata;
> > > +	u32 mmio_count, mmio_count_max;
> > >  	u8 *payload;
> > > 
> > > -	if (sizeof(struct intel_dmc_header) != dmc_header->header_len) {
> > > +	BUILD_BUG_ON(ARRAY_SIZE(csr->mmioaddr) < DMC_V3_MAX_MMIO_COUNT ||
> > > +		     ARRAY_SIZE(csr->mmioaddr) < DMC_V1_MAX_MMIO_COUNT);
> > > +
> > > +	/* Cope with small differences between v1 and v3 */
> > > +	if (dmc_header->header_ver == 3) {
> > 
> > I'm missing on this patch something like:
> > 
> > - /* 0x01, 0x02 */
> > +  /* 0x01, 0x02, 0x03 */
> > near header_ver definition
> > 
> > or maybe kill that comment at all...
> 
> nah, that is another version and it's a bit confusing. But we have
> *package* header version: 1 or 2
> *dmc* header version: 1 or 3
> 
> Overall structure of the firmware:
> 
> .------------.
> | CSS Header |
> |------------|
> | Package    |
> | Header     |
> |------------|
> | 20 or 32   |         * 20 for package header v1
> | fw_info    |--.-.-.    32 for package header v2
> |------------|<-' | |
> | DMC header |    | |
> | DMC payload|    | |
> |------------|<---' |
> | DMC header |      |
> | DMC payload|      |
> |------------|      .
>     ...            .
> |------------|<-----'
> | DMC header |
> | DMC payload|
> '------------'
> 
> here the dmc_header v3 will dictate how the *DMC header*
> will be parsed, that can have either 8 or 20 MMIOs to be programmed,
> besides the firmware to be loaded.

Ahh! I got it now.
Thanks for detailing it out.

> 
> I guess I could add a patch on top with this sketch as a comment to
> better explain how is the layout of the file.

That is a good idea! :)

Thanks,
Rodrigo.

> 
> 
> Lucas De Marchi
> 
> > 
> > The rest seems right so feel free to use
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > > +		const struct intel_dmc_header_v3 *v3 =
> > > +			(const struct intel_dmc_header_v3 *)dmc_header;
> > > +
> > > +		mmioaddr = v3->mmioaddr;
> > > +		mmiodata = v3->mmiodata;
> > > +		mmio_count = v3->mmio_count;
> > > +		mmio_count_max = DMC_V3_MAX_MMIO_COUNT;
> > > +		/* header_len is in dwords */
> > > +		header_len_bytes = dmc_header->header_len * 4;
> > > +		dmc_header_size = sizeof(*v3);
> > > +	} else if (dmc_header->header_ver == 1) {
> > > +		const struct intel_dmc_header_v1 *v1 =
> > > +			(const struct intel_dmc_header_v1 *)dmc_header;
> > > +
> > > +		mmioaddr = v1->mmioaddr;
> > > +		mmiodata = v1->mmiodata;
> > > +		mmio_count = v1->mmio_count;
> > > +		mmio_count_max = DMC_V1_MAX_MMIO_COUNT;
> > > +		header_len_bytes = dmc_header->header_len;
> > > +		dmc_header_size = sizeof(*v1);
> > > +	} else {
> > > +		DRM_ERROR("Unknown DMC fw header version: %u\n",
> > > +			  dmc_header->header_ver);
> > > +		return 0;
> > > +	}
> > > +
> > > +	if (header_len_bytes != dmc_header_size) {
> > >  		DRM_ERROR("DMC firmware has wrong dmc header length "
> > > -			  "(%u bytes)\n",
> > > -			  (dmc_header->header_len));
> > > +			  "(%u bytes)\n", header_len_bytes);
> > >  		return 0;
> > >  	}
> > > 
> > >  	/* Cache the dmc header info. */
> > > -	if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) {
> > > -		DRM_ERROR("DMC firmware has wrong mmio count %u\n",
> > > -			  dmc_header->mmio_count);
> > > +	if (mmio_count > mmio_count_max) {
> > > +		DRM_ERROR("DMC firmware has wrong mmio count %u\n", mmio_count);
> > >  		return 0;
> > >  	}
> > > 
> > > -	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) {
> > > +	for (i = 0; i < mmio_count; i++) {
> > > +		if (mmioaddr[i] < CSR_MMIO_START_RANGE ||
> > > +		    mmioaddr[i] > CSR_MMIO_END_RANGE) {
> > >  			DRM_ERROR("DMC firmware has wrong mmio address 0x%x\n",
> > > -				  dmc_header->mmioaddr[i]);
> > > +				  mmioaddr[i]);
> > >  			return 0;
> > >  		}
> > > -		csr->mmioaddr[i] = _MMIO(dmc_header->mmioaddr[i]);
> > > -		csr->mmiodata[i] = dmc_header->mmiodata[i];
> > > +		csr->mmioaddr[i] = _MMIO(mmioaddr[i]);
> > > +		csr->mmiodata[i] = mmiodata[i];
> > >  	}
> > > +	csr->mmio_count = mmio_count;
> > > 
> > >  	/* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
> > >  	payload_size = dmc_header->fw_size * 4;
> > > @@ -379,12 +436,10 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr,
> > >  		return 0;
> > >  	}
> > > 
> > > -	r = sizeof(struct intel_dmc_header);
> > > -	payload = (u8 *)(dmc_header) + r;
> > > +	payload = (u8 *)(dmc_header) + header_len_bytes;
> > >  	memcpy(csr->dmc_payload, payload, payload_size);
> > > -	r += payload_size;
> > > 
> > > -	return r;
> > > +	return header_len_bytes + payload_size;
> > >  }
> > > 
> > >  static u32
> > > @@ -470,7 +525,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
> > >  {
> > >  	struct intel_css_header *css_header;
> > >  	struct intel_package_header *package_header;
> > > -	struct intel_dmc_header *dmc_header;
> > > +	struct intel_dmc_header_base *dmc_header;
> > >  	struct intel_csr *csr = &dev_priv->csr;
> > >  	const struct stepping_info *si = intel_get_stepping_info(dev_priv);
> > >  	u32 readcount = 0;
> > > @@ -496,7 +551,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
> > >  	readcount += r;
> > > 
> > >  	/* Extract dmc_header information. */
> > > -	dmc_header = (struct intel_dmc_header *)&fw->data[readcount];
> > > +	dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount];
> > >  	r = parse_csr_fw_dmc(csr, dmc_header);
> > >  	if (!r)
> > >  		return NULL;
> > > --
> > > 2.21.0
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [01/10] drm/i915/dmc: use kernel types

[Patchwork]

== Series Details ==

Series: series starting with [01/10] drm/i915/dmc: use kernel types
URL   : https://patchwork.freedesktop.org/series/61016/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6132_full -> Patchwork_13077_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_13077_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_rpm@legacy-planes:
    - shard-skl:          [PASS][1] -> [INCOMPLETE][2] ([fdo#107807])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-skl10/igt@i915_pm_rpm@legacy-planes.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-skl2/igt@i915_pm_rpm@legacy-planes.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-skl:          [PASS][3] -> [INCOMPLETE][4] ([fdo#110741])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-skl3/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-skl5/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_flip@2x-flip-vs-dpms-off-vs-modeset:
    - shard-hsw:          [PASS][5] -> [SKIP][6] ([fdo#109271]) +29 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-hsw7/igt@kms_flip@2x-flip-vs-dpms-off-vs-modeset.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-hsw5/igt@kms_flip@2x-flip-vs-dpms-off-vs-modeset.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-pwrite:
    - shard-iclb:         [PASS][7] -> [FAIL][8] ([fdo#103167]) +3 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-pwrite.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-iclb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-pwrite.html

  * igt@kms_lease@cursor_implicit_plane:
    - shard-snb:          [PASS][9] -> [SKIP][10] ([fdo#109271]) +2 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-snb1/igt@kms_lease@cursor_implicit_plane.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-snb2/igt@kms_lease@cursor_implicit_plane.html

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min:
    - shard-skl:          [PASS][11] -> [FAIL][12] ([fdo#108145])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-skl7/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-skl10/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [PASS][13] -> [FAIL][14] ([fdo#108145] / [fdo#110403])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-skl1/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-skl8/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr2_su@page_flip:
    - shard-iclb:         [PASS][15] -> [SKIP][16] ([fdo#109642])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-iclb2/igt@kms_psr2_su@page_flip.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-iclb6/igt@kms_psr2_su@page_flip.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [PASS][17] -> [FAIL][18] ([fdo#99912])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-kbl2/igt@kms_setmode@basic.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-kbl3/igt@kms_setmode@basic.html

  * igt@kms_vblank@pipe-c-ts-continuation-suspend:
    - shard-apl:          [PASS][19] -> [DMESG-WARN][20] ([fdo#108566]) +8 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-apl2/igt@kms_vblank@pipe-c-ts-continuation-suspend.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-apl7/igt@kms_vblank@pipe-c-ts-continuation-suspend.html

  
#### Possible fixes ####

  * igt@gem_eio@in-flight-suspend:
    - shard-apl:          [DMESG-WARN][21] ([fdo#108566]) -> [PASS][22] +4 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-apl1/igt@gem_eio@in-flight-suspend.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-apl2/igt@gem_eio@in-flight-suspend.html

  * igt@gem_softpin@noreloc-s3:
    - shard-skl:          [INCOMPLETE][23] ([fdo#104108]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-skl5/igt@gem_softpin@noreloc-s3.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-skl3/igt@gem_softpin@noreloc-s3.html

  * igt@gem_tiled_swapping@non-threaded:
    - shard-hsw:          [FAIL][25] ([fdo#108686]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-hsw1/igt@gem_tiled_swapping@non-threaded.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-hsw5/igt@gem_tiled_swapping@non-threaded.html

  * igt@kms_flip_tiling@flip-to-y-tiled:
    - shard-iclb:         [FAIL][27] ([fdo#107931] / [fdo#108134]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-iclb3/igt@kms_flip_tiling@flip-to-y-tiled.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-iclb2/igt@kms_flip_tiling@flip-to-y-tiled.html

  * igt@kms_flip_tiling@flip-x-tiled:
    - shard-skl:          [FAIL][29] ([fdo#108145] / [fdo#108303]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-skl5/igt@kms_flip_tiling@flip-x-tiled.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-skl8/igt@kms_flip_tiling@flip-x-tiled.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-move:
    - shard-iclb:         [FAIL][31] ([fdo#103167]) -> [PASS][32] +1 similar issue
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-iclb4/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-move.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-iclb2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-move.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-shrfb-draw-mmap-wc:
    - shard-hsw:          [SKIP][33] ([fdo#109271]) -> [PASS][34] +21 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-hsw5/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-shrfb-draw-mmap-wc.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-hsw7/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-shrfb-draw-mmap-wc.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [FAIL][35] ([fdo#108145]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-skl5/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-skl8/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-iclb:         [FAIL][37] ([fdo#103166]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-iclb4/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-iclb2/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_psr@psr2_primary_mmap_cpu:
    - shard-iclb:         [SKIP][39] ([fdo#109441]) -> [PASS][40] +2 similar issues
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-iclb3/igt@kms_psr@psr2_primary_mmap_cpu.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-iclb2/igt@kms_psr@psr2_primary_mmap_cpu.html

  
#### Warnings ####

  * igt@gem_mmap_gtt@forked-big-copy-xy:
    - shard-iclb:         [INCOMPLETE][41] ([fdo#107713] / [fdo#109100]) -> [TIMEOUT][42] ([fdo#109673])
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-iclb1/igt@gem_mmap_gtt@forked-big-copy-xy.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-iclb1/igt@gem_mmap_gtt@forked-big-copy-xy.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-move:
    - shard-skl:          [FAIL][43] ([fdo#108040]) -> [FAIL][44] ([fdo#103167])
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-skl3/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-move.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-skl5/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-move.html

  * igt@prime_vgem@busy-bsd1:
    - shard-snb:          [FAIL][45] -> [INCOMPLETE][46] ([fdo#105411])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6132/shard-snb2/igt@prime_vgem@busy-bsd1.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/shard-snb5/igt@prime_vgem@busy-bsd1.html

  
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107931]: https://bugs.freedesktop.org/show_bug.cgi?id=107931
  [fdo#108040]: https://bugs.freedesktop.org/show_bug.cgi?id=108040
  [fdo#108134]: https://bugs.freedesktop.org/show_bug.cgi?id=108134
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108303]: https://bugs.freedesktop.org/show_bug.cgi?id=108303
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#109673]: https://bugs.freedesktop.org/show_bug.cgi?id=109673
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#110741]: https://bugs.freedesktop.org/show_bug.cgi?id=110741
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * Linux: CI_DRM_6132 -> Patchwork_13077

  CI_DRM_6132: 78850b480c542b2e10da5a93afac2e13307909cb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5010: 631f3ac2e78c8d6332afc693bf290ae23d8d5685 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13077: 596763e01370d2bf15d881cbd4a6d5284e57de68 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13077/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx