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

[Srivatsa, Anusha]

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

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