[PATCH 0/9] Add support for new DMC headers

[PATCH 0/9] Add support for new DMC headers

[Lucas De Marchi]

This is v2 of https://patchwork.freedesktop.org/series/61016/
Last patch as moved as the first and sent separately so it can be
backported. That one is already applied.

Rodrigo and Anusha gave their r-b on all these, but given the rebase, the
patches changed considerably (and the end result is not even the same).
So I didn't add them here, except for the first that is a trivial one.

Lucas De Marchi (9):
  drm/i915/dmc: use kernel types
  drm/i915/dmc: extract fw_info and table walk from intel_package_header
  drm/i915/dmc: add support for package_header with version 2
  drm/i915/dmc: extract function to parse css header
  drm/i915/dmc: extract function to parse package_header
  drm/i915/dmc: extract function to parse dmc_header
  drm/i915/dmc: add support to load dmc_header version 3
  drm/i915/dmc: remove redundant return in parse_csr_fw()
  drm/i915/dmc: protect against loading wrong firmware

 drivers/gpu/drm/i915/i915_drv.h  |   4 +-
 drivers/gpu/drm/i915/intel_csr.c | 407 ++++++++++++++++++++++---------
 2 files changed, 290 insertions(+), 121 deletions(-)

-- 
2.21.0

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

[PATCH 1/9] 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>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
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 bf0eebd385b9..3d2beecd8d0d 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 2/9] 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 | 72 ++++++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 3d2beecd8d0d..99fa4db95e46 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 always 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;
 	size_t fsize;
@@ -349,27 +388,26 @@ 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;
-	}
+	fsize += PACKAGE_MAX_FW_INFO_ENTRIES * sizeof(struct intel_fw_info);
+	if (fsize > fw->size)
+		goto error_truncated;
+
+	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 3/9] 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 | 38 ++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 99fa4db95e46..ba72c29acbcc 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;
 	size_t fsize;
@@ -381,20 +382,32 @@ 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;
 
-	fsize += PACKAGE_MAX_FW_INFO_ENTRIES * sizeof(struct intel_fw_info);
+	fsize += max_entries * sizeof(struct intel_fw_info);
 	if (fsize > fw->size)
 		goto error_truncated;
 
@@ -405,8 +418,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;
-- 
2.21.0

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

[PATCH 4/9] 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.

v2: rework remaining bytes calculation on new protection for amount of
    bytes read

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

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index ba72c29acbcc..79732075f008 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -332,36 +332,22 @@ 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,
+			    size_t rem_size)
 {
-	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;
-	size_t fsize;
-
-	if (!fw)
-		return NULL;
-
-	fsize = sizeof(struct intel_css_header) +
-		sizeof(struct intel_package_header) +
-		sizeof(struct intel_dmc_header);
-	if (fsize > fw->size)
-		goto error_truncated;
+	if (rem_size < sizeof(struct intel_css_header)) {
+		DRM_ERROR("Truncated DMC firmware, refusing.\n");
+		return 0;
+	}
 
-	/* 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 &&
@@ -372,12 +358,42 @@ 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;
+	size_t fsize;
+
+	if (!fw)
+		return NULL;
+
+	/* Extract CSS Header information */
+	css_header = (struct intel_css_header *)fw->data;
+	r = parse_csr_fw_css(csr, css_header, fw->size);
+	if (!r)
+		return NULL;
+
+	readcount += r;
+	fsize = readcount +
+		sizeof(struct intel_package_header) +
+		sizeof(struct intel_dmc_header);
+	if (fsize > fw->size)
+		goto error_truncated;
 
 	/* 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 5/9] 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 | 117 +++++++++++++++++--------------
 1 file changed, 66 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 79732075f008..db5772616a4f 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -332,6 +332,64 @@ 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,
+		     size_t rem_size)
+{
+	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) {
+		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;
+	}
+
+	/*
+	 * 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_size);
+		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;
+	}
+
+	/* dmc_offset is in dwords */
+	return package_size + dmc_offset * 4;
+
+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,
@@ -374,7 +432,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;
 	size_t fsize;
@@ -389,59 +447,16 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 		return NULL;
 
 	readcount += r;
-	fsize = readcount +
-		sizeof(struct intel_package_header) +
-		sizeof(struct intel_dmc_header);
-	if (fsize > fw->size)
-		goto error_truncated;
-
-	/* 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;
-
-	fsize += max_entries * sizeof(struct intel_fw_info);
-	if (fsize > fw->size)
-		goto error_truncated;
 
-	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);
+	/* Extract Package Header information */
+	package_header = (struct intel_package_header *)&fw->data[readcount];
+	r = parse_csr_fw_package(csr, package_header, si, fw->size - readcount);
+	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;
-	fsize += dmc_offset;
+	readcount += r;
+	fsize = readcount +
+		sizeof(struct intel_dmc_header);
 	if (fsize > fw->size)
 		goto error_truncated;
 
-- 
2.21.0

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

[PATCH 6/9] 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 | 130 ++++++++++++++++++-------------
 1 file changed, 74 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index db5772616a4f..1fb42fd44519 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -332,6 +332,74 @@ 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,
+			    size_t rem_size)
+{
+	unsigned int i, payload_size;
+	u32 r;
+	u8 *payload;
+
+	if (rem_size < sizeof(struct intel_dmc_header))
+		goto error_truncated;
+
+	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];
+	}
+
+	rem_size -= dmc_header->header_len;
+
+	/* 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;
+	}
+	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;
+
+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,
@@ -432,10 +500,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;
-	size_t fsize;
+	u32 readcount = 0;
+	u32 r;
 
 	if (!fw)
 		return NULL;
@@ -455,62 +521,14 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv,
 		return NULL;
 
 	readcount += r;
-	fsize = readcount +
-		sizeof(struct intel_dmc_header);
-	if (fsize > fw->size)
-		goto error_truncated;
 
-	/* Extract dmc_header information. */
+	/* 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;
-	fsize += nbytes;
-	if (fsize > fw->size)
-		goto error_truncated;
-
-	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, fw->size - readcount);
+	if (!r)
 		return NULL;
-	}
 
-	return memcpy(dmc_payload, &fw->data[readcount], nbytes);
-
-error_truncated:
-	DRM_ERROR("Truncated DMC firmware, rejecting.\n");
-	return NULL;
+	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 7/9] 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 | 121 ++++++++++++++++++++++++-------
 2 files changed, 95 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index dfe4b11ee423..fc0a770fef15 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -352,8 +352,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 1fb42fd44519..0b1978a2f87d 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,43 +360,83 @@ 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,
 			    size_t rem_size)
 {
-	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 (rem_size < sizeof(struct intel_dmc_header))
+	BUILD_BUG_ON(ARRAY_SIZE(csr->mmioaddr) < DMC_V3_MAX_MMIO_COUNT ||
+		     ARRAY_SIZE(csr->mmioaddr) < DMC_V1_MAX_MMIO_COUNT);
+
+	/*
+	 * Check if we can access common fields, we will checkc again below
+	 * after we have read the version
+	 */
+	if (rem_size < sizeof(struct intel_dmc_header_base))
 		goto error_truncated;
 
-	if (sizeof(struct intel_dmc_header) != dmc_header->header_len) {
+	/* 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;
+
+		if (rem_size < sizeof(struct intel_dmc_header_v3))
+			goto error_truncated;
+
+		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;
+
+		if (rem_size < sizeof(struct intel_dmc_header_v1))
+			goto error_truncated;
+
+		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;
 
-	rem_size -= dmc_header->header_len;
+	rem_size -= header_len_bytes;
 
 	/* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
 	payload_size = dmc_header->fw_size * 4;
@@ -388,12 +455,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;
 
 error_truncated:
 	DRM_ERROR("Truncated DMC firmware, refusing.\n");
@@ -497,7 +562,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;
@@ -523,7 +588,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, fw->size - readcount);
 	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 8/9] 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 0b1978a2f87d..7608e4e2950d 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -557,7 +557,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;
@@ -569,13 +569,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, fw->size);
 	if (!r)
-		return NULL;
+		return;
 
 	readcount += r;
 
@@ -583,17 +583,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, fw->size - readcount);
 	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, fw->size - readcount);
-	if (!r)
-		return NULL;
-
-	return csr->dmc_payload;
+	parse_csr_fw_dmc(csr, dmc_header, fw->size - readcount);
 }
 
 static void intel_csr_runtime_pm_get(struct drm_i915_private *dev_priv)
@@ -621,8 +617,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 9/9] 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 7608e4e2950d..864531aae1a5 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;
@@ -508,7 +515,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 Add support for new DMC headers

[Patchwork]

== Series Details ==

Series: Add support for new DMC headers
URL   : https://patchwork.freedesktop.org/series/61761/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6216 -> Patchwork_13201
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_close_race@basic-threads:
    - fi-icl-dsi:         [PASS][1] -> [INCOMPLETE][2] ([fdo#107713])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/fi-icl-dsi/igt@gem_close_race@basic-threads.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/fi-icl-dsi/igt@gem_close_race@basic-threads.html

  * igt@gem_tiled_pread_basic:
    - fi-icl-u3:          [PASS][3] -> [DMESG-WARN][4] ([fdo#107724])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/fi-icl-u3/igt@gem_tiled_pread_basic.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/fi-icl-u3/igt@gem_tiled_pread_basic.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-icl-u3:          [PASS][5] -> [INCOMPLETE][6] ([fdo#107713])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/fi-icl-u3/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/fi-icl-u3/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

  
#### Possible fixes ####

  * igt@gem_close_race@basic-process:
    - fi-icl-u3:          [DMESG-WARN][7] ([fdo#107724]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/fi-icl-u3/igt@gem_close_race@basic-process.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/fi-icl-u3/igt@gem_close_race@basic-process.html

  * igt@gem_ctx_create@basic-files:
    - fi-icl-u2:          [INCOMPLETE][9] ([fdo#107713] / [fdo#109100]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/fi-icl-u2/igt@gem_ctx_create@basic-files.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/fi-icl-u2/igt@gem_ctx_create@basic-files.html
    - fi-icl-y:           [INCOMPLETE][11] ([fdo#107713] / [fdo#109100]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/fi-icl-y/igt@gem_ctx_create@basic-files.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/fi-icl-y/igt@gem_ctx_create@basic-files.html

  * igt@gem_exec_basic@basic-all:
    - fi-cml-u2:          [INCOMPLETE][13] ([fdo#110566]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/fi-cml-u2/igt@gem_exec_basic@basic-all.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/fi-cml-u2/igt@gem_exec_basic@basic-all.html

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

  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108044]: https://bugs.freedesktop.org/show_bug.cgi?id=108044
  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#110566]: https://bugs.freedesktop.org/show_bug.cgi?id=110566


Participating hosts (54 -> 47)
------------------------------

  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-hsw-peppy fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


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

  * Linux: CI_DRM_6216 -> Patchwork_13201

  CI_DRM_6216: e58a2b1f565ec5d77c69724d2f43a7de6f7953b3 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5047: 3f4b1a49ed5e1c77ea42970d4d3156c997f66050 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13201: 768c3b01e168330056b40713f46ea998e18b3a2c @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

768c3b01e168 drm/i915/dmc: protect against loading wrong firmware
d8baaeb55131 drm/i915/dmc: remove redundant return in parse_csr_fw()
912af5910248 drm/i915/dmc: add support to load dmc_header version 3
0c4b45196f4a drm/i915/dmc: extract function to parse dmc_header
11f96e9969b0 drm/i915/dmc: extract function to parse package_header
baa55de8db95 drm/i915/dmc: extract function to parse css header
71c4555f11ca drm/i915/dmc: add support for package_header with version 2
80a7692a85f3 drm/i915/dmc: extract fw_info and table walk from intel_package_header
455b1de4efc3 drm/i915/dmc: use kernel types

== Logs ==

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

✓ Fi.CI.IGT: success for Add support for new DMC headers

[Patchwork]

== Series Details ==

Series: Add support for new DMC headers
URL   : https://patchwork.freedesktop.org/series/61761/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6216_full -> Patchwork_13201_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_eio@in-flight-suspend:
    - shard-kbl:          [PASS][1] -> [DMESG-WARN][2] ([fdo#108566]) +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-kbl3/igt@gem_eio@in-flight-suspend.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-kbl6/igt@gem_eio@in-flight-suspend.html

  * igt@gem_persistent_relocs@forked-interruptible-thrash-inactive:
    - shard-apl:          [PASS][3] -> [INCOMPLETE][4] ([fdo#103927])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-apl4/igt@gem_persistent_relocs@forked-interruptible-thrash-inactive.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-apl8/igt@gem_persistent_relocs@forked-interruptible-thrash-inactive.html

  * igt@gem_softpin@noreloc-s3:
    - shard-skl:          [PASS][5] -> [INCOMPLETE][6] ([fdo#104108])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-skl6/igt@gem_softpin@noreloc-s3.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-skl7/igt@gem_softpin@noreloc-s3.html

  * igt@i915_suspend@debugfs-reader:
    - shard-apl:          [PASS][7] -> [DMESG-WARN][8] ([fdo#108566]) +5 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-apl4/igt@i915_suspend@debugfs-reader.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-apl4/igt@i915_suspend@debugfs-reader.html

  * igt@kms_cursor_crc@pipe-b-cursor-suspend:
    - shard-skl:          [PASS][9] -> [INCOMPLETE][10] ([fdo#110741])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-skl1/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-skl6/igt@kms_cursor_crc@pipe-b-cursor-suspend.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw:
    - shard-iclb:         [PASS][11] -> [FAIL][12] ([fdo#103167]) +3 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-iclb5/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-iclb1/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html

  * igt@kms_frontbuffer_tracking@psr-suspend:
    - shard-skl:          [PASS][13] -> [INCOMPLETE][14] ([fdo#104108] / [fdo#106978])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-skl8/igt@kms_frontbuffer_tracking@psr-suspend.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-skl10/igt@kms_frontbuffer_tracking@psr-suspend.html

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min:
    - shard-skl:          [PASS][15] -> [FAIL][16] ([fdo#108145])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-skl1/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-skl3/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-iclb:         [PASS][17] -> [FAIL][18] ([fdo#103166])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-iclb3/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-iclb8/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_psr@psr2_sprite_blt:
    - shard-iclb:         [PASS][19] -> [SKIP][20] ([fdo#109441])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-iclb2/igt@kms_psr@psr2_sprite_blt.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-iclb6/igt@kms_psr@psr2_sprite_blt.html

  
#### Possible fixes ####

  * igt@gem_ctx_engines@execute-one:
    - shard-skl:          [DMESG-WARN][21] -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-skl1/igt@gem_ctx_engines@execute-one.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-skl6/igt@gem_ctx_engines@execute-one.html

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-iclb:         [INCOMPLETE][23] ([fdo#107713] / [fdo#109100]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-iclb7/igt@gem_ctx_isolation@rcs0-s3.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-iclb7/igt@gem_ctx_isolation@rcs0-s3.html

  * igt@gem_workarounds@suspend-resume-fd:
    - shard-skl:          [INCOMPLETE][25] ([fdo#104108]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-skl1/igt@gem_workarounds@suspend-resume-fd.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-skl1/igt@gem_workarounds@suspend-resume-fd.html

  * igt@kms_cursor_crc@pipe-b-cursor-64x64-sliding:
    - shard-skl:          [FAIL][27] ([fdo#103232]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-skl9/igt@kms_cursor_crc@pipe-b-cursor-64x64-sliding.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-skl8/igt@kms_cursor_crc@pipe-b-cursor-64x64-sliding.html

  * igt@kms_frontbuffer_tracking@fbc-stridechange:
    - shard-iclb:         [FAIL][29] ([fdo#103167]) -> [PASS][30] +5 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-stridechange.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-stridechange.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-apl:          [DMESG-WARN][31] ([fdo#108566]) -> [PASS][32] +2 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-apl1/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-apl3/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          [FAIL][33] ([fdo#108145] / [fdo#110403]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-skl9/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-skl8/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html

  * igt@kms_psr@psr2_primary_blt:
    - shard-iclb:         [SKIP][35] ([fdo#109441]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-iclb6/igt@kms_psr@psr2_primary_blt.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-iclb2/igt@kms_psr@psr2_primary_blt.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [FAIL][37] ([fdo#99912]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-kbl7/igt@kms_setmode@basic.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-kbl3/igt@kms_setmode@basic.html

  * igt@kms_vblank@pipe-c-query-idle-hang:
    - shard-iclb:         [INCOMPLETE][39] ([fdo#107713]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-iclb1/igt@kms_vblank@pipe-c-query-idle-hang.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13201/shard-iclb7/igt@kms_vblank@pipe-c-query-idle-hang.html

  
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#106978]: https://bugs.freedesktop.org/show_bug.cgi?id=106978
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [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 (9 -> 9)
------------------------------

  No changes in participating hosts


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

  * Linux: CI_DRM_6216 -> Patchwork_13201

  CI_DRM_6216: e58a2b1f565ec5d77c69724d2f43a7de6f7953b3 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5047: 3f4b1a49ed5e1c77ea42970d4d3156c997f66050 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13201: 768c3b01e168330056b40713f46ea998e18b3a2c @ 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_13201/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Friday, June 7, 2019 2:12 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 2/9] 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 | 72 ++++++++++++++++++++++++--------
> 1 file changed, 55 insertions(+), 17 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>index 3d2beecd8d0d..99fa4db95e46 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 always 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;
> 	size_t fsize;
>@@ -349,27 +388,26 @@ 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;
>-	}
>+	fsize += PACKAGE_MAX_FW_INFO_ENTRIES * sizeof(struct
>intel_fw_info);
>+	if (fsize > fw->size)
>+		goto error_truncated;
>+
>+	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 3/9] drm/i915/dmc: add support for package_header with version 2

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Friday, June 7, 2019 2:12 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 3/9] 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>

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

>---
> drivers/gpu/drm/i915/intel_csr.c | 38 ++++++++++++++++++++++----------
> 1 file changed, 26 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>index 99fa4db95e46..ba72c29acbcc 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;
> 	size_t fsize;
>@@ -381,20 +382,32 @@ 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;
>
>-	fsize += PACKAGE_MAX_FW_INFO_ENTRIES * sizeof(struct
>intel_fw_info);
>+	fsize += max_entries * sizeof(struct intel_fw_info);
> 	if (fsize > fw->size)
> 		goto error_truncated;
>
>@@ -405,8 +418,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;
>--
>2.21.0

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

Re: [PATCH 4/9] drm/i915/dmc: extract function to parse css header

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Friday, June 7, 2019 2:12 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 4/9] 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.
>
>v2: rework remaining bytes calculation on new protection for amount of
>    bytes read

Looks good.
Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>---
> drivers/gpu/drm/i915/intel_csr.c | 66 ++++++++++++++++++++------------
> 1 file changed, 41 insertions(+), 25 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>index ba72c29acbcc..79732075f008 100644
>--- a/drivers/gpu/drm/i915/intel_csr.c
>+++ b/drivers/gpu/drm/i915/intel_csr.c
>@@ -332,36 +332,22 @@ 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,
>+			    size_t rem_size)
> {
>-	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;
>-	size_t fsize;
>-
>-	if (!fw)
>-		return NULL;
>-
>-	fsize = sizeof(struct intel_css_header) +
>-		sizeof(struct intel_package_header) +
>-		sizeof(struct intel_dmc_header);
>-	if (fsize > fw->size)
>-		goto error_truncated;
>+	if (rem_size < sizeof(struct intel_css_header)) {
>+		DRM_ERROR("Truncated DMC firmware, refusing.\n");
>+		return 0;
>+	}
>
>-	/* 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 &&
>@@ -372,12 +358,42 @@ 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;
>+	size_t fsize;
>+
>+	if (!fw)
>+		return NULL;
>+
>+	/* Extract CSS Header information */
>+	css_header = (struct intel_css_header *)fw->data;
>+	r = parse_csr_fw_css(csr, css_header, fw->size);
>+	if (!r)
>+		return NULL;
>+
>+	readcount += r;
>+	fsize = readcount +
>+		sizeof(struct intel_package_header) +
>+		sizeof(struct intel_dmc_header);
>+	if (fsize > fw->size)
>+		goto error_truncated;
>
> 	/* 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 9/9] drm/i915/dmc: protect against loading wrong firmware

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Friday, June 7, 2019 2:13 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 9/9] drm/i915/dmc: protect against loading wrong firmware
>
>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 7608e4e2950d..864531aae1a5 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;

Wont we need a message here? "Wrong DMC version, not loading v_x.0x" or something?.." 

Anusha 
> 		if (fw_info[i].substepping == '*' &&
> 		    si->stepping == fw_info[i].stepping) {
> 			dmc_offset = fw_info[i].offset;
>@@ -508,7 +515,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 8/9] drm/i915/dmc: remove redundant return in parse_csr_fw()

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Friday, June 7, 2019 2:12 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 8/9] 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>

Looks 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 0b1978a2f87d..7608e4e2950d 100644
>--- a/drivers/gpu/drm/i915/intel_csr.c
>+++ b/drivers/gpu/drm/i915/intel_csr.c
>@@ -557,7 +557,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;
>@@ -569,13 +569,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, fw->size);
> 	if (!r)
>-		return NULL;
>+		return;
>
> 	readcount += r;
>
>@@ -583,17 +583,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, fw->size - readcount);
> 	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, fw->size - readcount);
>-	if (!r)
>-		return NULL;
>-
>-	return csr->dmc_payload;
>+	parse_csr_fw_dmc(csr, dmc_header, fw->size - readcount);
> }
>
> static void intel_csr_runtime_pm_get(struct drm_i915_private *dev_priv) @@ -
>621,8 +617,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 7/9] drm/i915/dmc: add support to load dmc_header version 3

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Friday, June 7, 2019 2:12 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 7/9] 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.

Changes look good.
Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>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 | 121 ++++++++++++++++++++++++-------
> 2 files changed, 95 insertions(+), 30 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index dfe4b11ee423..fc0a770fef15 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -352,8 +352,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 1fb42fd44519..0b1978a2f87d 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,43 +360,83 @@ 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,
> 			    size_t rem_size)
> {
>-	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 (rem_size < sizeof(struct intel_dmc_header))
>+	BUILD_BUG_ON(ARRAY_SIZE(csr->mmioaddr) <
>DMC_V3_MAX_MMIO_COUNT ||
>+		     ARRAY_SIZE(csr->mmioaddr) <
>DMC_V1_MAX_MMIO_COUNT);
>+
>+	/*
>+	 * Check if we can access common fields, we will checkc again below
>+	 * after we have read the version
>+	 */
>+	if (rem_size < sizeof(struct intel_dmc_header_base))
> 		goto error_truncated;
>
>-	if (sizeof(struct intel_dmc_header) != dmc_header->header_len) {
>+	/* 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;
>+
>+		if (rem_size < sizeof(struct intel_dmc_header_v3))
>+			goto error_truncated;
>+
>+		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;
>+
>+		if (rem_size < sizeof(struct intel_dmc_header_v1))
>+			goto error_truncated;
>+
>+		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;
>
>-	rem_size -= dmc_header->header_len;
>+	rem_size -= header_len_bytes;
>
> 	/* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
> 	payload_size = dmc_header->fw_size * 4; @@ -388,12 +455,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;
>
> error_truncated:
> 	DRM_ERROR("Truncated DMC firmware, refusing.\n"); @@ -497,7
>+562,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;
>@@ -523,7 +588,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, fw->size - readcount);
> 	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 5/9] drm/i915/dmc: extract function to parse package_header

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Friday, June 7, 2019 2:12 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 5/9] 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.
>
Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>---
> drivers/gpu/drm/i915/intel_csr.c | 117 +++++++++++++++++--------------
> 1 file changed, 66 insertions(+), 51 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>index 79732075f008..db5772616a4f 100644
>--- a/drivers/gpu/drm/i915/intel_csr.c
>+++ b/drivers/gpu/drm/i915/intel_csr.c
>@@ -332,6 +332,64 @@ 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,
>+		     size_t rem_size)
>+{
>+	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) {
>+		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;
>+	}
>+
>+	/*
>+	 * 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_size);
>+		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;
>+	}
>+
>+	/* dmc_offset is in dwords */
>+	return package_size + dmc_offset * 4;
>+
>+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, @@ -374,7
>+432,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;
> 	size_t fsize;
>@@ -389,59 +447,16 @@ static u32 *parse_csr_fw(struct drm_i915_private
>*dev_priv,
> 		return NULL;
>
> 	readcount += r;
>-	fsize = readcount +
>-		sizeof(struct intel_package_header) +
>-		sizeof(struct intel_dmc_header);
>-	if (fsize > fw->size)
>-		goto error_truncated;
>-
>-	/* 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;
>-
>-	fsize += max_entries * sizeof(struct intel_fw_info);
>-	if (fsize > fw->size)
>-		goto error_truncated;
>
>-	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);
>+	/* Extract Package Header information */
>+	package_header = (struct intel_package_header *)&fw-
>>data[readcount];
>+	r = parse_csr_fw_package(csr, package_header, si, fw->size - readcount);
>+	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;
>-	fsize += dmc_offset;
>+	readcount += r;
>+	fsize = readcount +
>+		sizeof(struct intel_dmc_header);
> 	if (fsize > fw->size)
> 		goto error_truncated;
>
>--
>2.21.0

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

Re: [PATCH 6/9] drm/i915/dmc: extract function to parse dmc_header

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Friday, June 7, 2019 2:12 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>
>Subject: [PATCH 6/9] 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.
>
Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>---
> drivers/gpu/drm/i915/intel_csr.c | 130 ++++++++++++++++++-------------
> 1 file changed, 74 insertions(+), 56 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>index db5772616a4f..1fb42fd44519 100644
>--- a/drivers/gpu/drm/i915/intel_csr.c
>+++ b/drivers/gpu/drm/i915/intel_csr.c
>@@ -332,6 +332,74 @@ 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,
>+			    size_t rem_size)
>+{
>+	unsigned int i, payload_size;
>+	u32 r;
>+	u8 *payload;
>+
>+	if (rem_size < sizeof(struct intel_dmc_header))
>+		goto error_truncated;
>+
>+	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];
>+	}
>+
>+	rem_size -= dmc_header->header_len;
>+
>+	/* 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;
>+	}
>+	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;
>+
>+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, @@ -
>432,10 +500,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;
>-	size_t fsize;
>+	u32 readcount = 0;
>+	u32 r;
>
> 	if (!fw)
> 		return NULL;
>@@ -455,62 +521,14 @@ static u32 *parse_csr_fw(struct drm_i915_private
>*dev_priv,
> 		return NULL;
>
> 	readcount += r;
>-	fsize = readcount +
>-		sizeof(struct intel_dmc_header);
>-	if (fsize > fw->size)
>-		goto error_truncated;
>
>-	/* Extract dmc_header information. */
>+	/* 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;
>-	fsize += nbytes;
>-	if (fsize > fw->size)
>-		goto error_truncated;
>-
>-	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, fw->size - readcount);
>+	if (!r)
> 		return NULL;
>-	}
>
>-	return memcpy(dmc_payload, &fw->data[readcount], nbytes);
>-
>-error_truncated:
>-	DRM_ERROR("Truncated DMC firmware, rejecting.\n");
>-	return NULL;
>+	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 9/9] drm/i915/dmc: protect against loading wrong firmware

[Lucas De Marchi]

On Fri, Jun 14, 2019 at 12:44:50PM -0700, Anusha Srivatsa wrote:
>
>
>>-----Original Message-----
>>From: De Marchi, Lucas
>>Sent: Friday, June 7, 2019 2:13 AM
>>To: intel-gfx@lists.freedesktop.org
>>Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Srivatsa, Anusha
>><anusha.srivatsa@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>
>>Subject: [PATCH 9/9] drm/i915/dmc: protect against loading wrong firmware
>>
>>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 7608e4e2950d..864531aae1a5 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;
>
>Wont we need a message here? "Wrong DMC version, not loading v_x.0x" or something?.."

I don't think so. It's normal to have other blobs inside the firmware
that we don't care about. At most I could put a debug, but I don't think
we really care.

As long as we find one we should be fine. And if we don't, then we will
complain loudly later in this function with a DRM_ERROR().
dmc_offset remaining as CSR_DEFAULT_FW_OFFSET has a double meaning here:
either we didn't find any entry or we found one that had the offset set
to this value.

Lucas De Marchi

>
>Anusha
>> 		if (fw_info[i].substepping == '*' &&
>> 		    si->stepping == fw_info[i].stepping) {
>> 			dmc_offset = fw_info[i].offset;
>>@@ -508,7 +515,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 9/9] drm/i915/dmc: protect against loading wrong firmware

[Srivatsa, Anusha]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Friday, June 14, 2019 2:37 PM
>To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>
>Subject: Re: [PATCH 9/9] drm/i915/dmc: protect against loading wrong firmware
>
>On Fri, Jun 14, 2019 at 12:44:50PM -0700, Anusha Srivatsa wrote:
>>
>>
>>>-----Original Message-----
>>>From: De Marchi, Lucas
>>>Sent: Friday, June 7, 2019 2:13 AM
>>>To: intel-gfx@lists.freedesktop.org
>>>Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Srivatsa, Anusha
>>><anusha.srivatsa@intel.com>; De Marchi, Lucas
>>><lucas.demarchi@intel.com>
>>>Subject: [PATCH 9/9] drm/i915/dmc: protect against loading wrong
>>>firmware
>>>
>>>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 7608e4e2950d..864531aae1a5 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;
>>
>>Wont we need a message here? "Wrong DMC version, not loading v_x.0x" or
>something?.."
>
>I don't think so. It's normal to have other blobs inside the firmware that we don't
>care about. At most I could put a debug, but I don't think we really care.
>
>As long as we find one we should be fine. And if we don't, then we will complain
>loudly later in this function with a DRM_ERROR().
>dmc_offset remaining as CSR_DEFAULT_FW_OFFSET has a double meaning here:
>either we didn't find any entry or we found one that had the offset set to this
>value.

Ok. Makes sense.

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

>Lucas De Marchi
>
>>
>>Anusha
>>> 		if (fw_info[i].substepping == '*' &&
>>> 		    si->stepping == fw_info[i].stepping) {
>>> 			dmc_offset = fw_info[i].offset;
>>>@@ -508,7 +515,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