| Message ID | 20190523082420.10352-2-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] drm/i915/dmc: use kernel types | expand |
On Thu, May 23, 2019 at 01:24:12AM -0700, Lucas De Marchi wrote: > Move fw_info out of struct intel_package_header to allow it to grow more > easily in future. To make a cleaner move, let's also extract a function to > search the header for the dmc_offset. > > While reviewing this code I wondered why we continued the search even > after finding a suitable firmware. Add a comment to explain we will > continue to try to find a more specific firmware version, even if this > is not required by the spec. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/intel_csr.c | 68 ++++++++++++++++++++++++-------- > 1 file changed, 51 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index b05e7a6aebc7..01ae356e69cc 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -70,6 +70,7 @@ MODULE_FIRMWARE(SKL_CSR_PATH); > MODULE_FIRMWARE(BXT_CSR_PATH); > > #define CSR_DEFAULT_FW_OFFSET 0xFFFFFFFF > +#define PACKAGE_MAX_FW_INFO_ENTRIES 20 > > struct intel_css_header { > /* 0x09 for DMC */ > @@ -139,8 +140,6 @@ struct intel_package_header { > > /* Number of valid entries in the FWInfo array below */ > u32 num_entries; > - > - struct intel_fw_info fw_info[20]; > } __packed; > > struct intel_dmc_header { > @@ -292,6 +291,46 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv) > gen9_set_dc_state_debugmask(dev_priv); > } > > +/* > + * Search fw_info table for dmc_offset to find firmware binary: num_entries is > + * already sanitized. > + */ > +static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info, > + unsigned int num_entries, > + const struct stepping_info *si) > +{ > + u32 dmc_offset = CSR_DEFAULT_FW_OFFSET; > + unsigned int i; > + > + for (i = 0; i < num_entries; i++) { > + if (fw_info[i].substepping == '*' && > + si->stepping == fw_info[i].stepping) { > + dmc_offset = fw_info[i].offset; > + break; > + } > + > + if (si->stepping == fw_info[i].stepping && > + si->substepping == fw_info[i].substepping) { > + dmc_offset = fw_info[i].offset; > + break; > + } > + > + if (fw_info[i].stepping == '*' && > + fw_info[i].substepping == '*') { > + /* > + * In theory we should stop the search as generic > + * entries should alwas come after the more specific > + * ones, but let's continue to make sure to work even > + * with "broken" firmwares. If we don't find a more > + * specific one, then we use this entry > + */ good point. Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > + dmc_offset = fw_info[i].offset; > + } > + } > + > + return dmc_offset; > +} > + > static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > const struct firmware *fw) > { > @@ -300,7 +339,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > struct intel_dmc_header *dmc_header; > struct intel_csr *csr = &dev_priv->csr; > const struct stepping_info *si = intel_get_stepping_info(dev_priv); > - u32 dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes; > + u32 dmc_offset, num_entries, readcount = 0, nbytes; > u32 i; > u32 *dmc_payload; > > @@ -342,27 +381,22 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > (package_header->header_len * 4)); > return NULL; > } > + > readcount += sizeof(struct intel_package_header); > + num_entries = package_header->num_entries; > + if (WARN_ON(package_header->num_entries > PACKAGE_MAX_FW_INFO_ENTRIES)) > + num_entries = PACKAGE_MAX_FW_INFO_ENTRIES; > > - /* Search for dmc_offset to find firware binary. */ > - for (i = 0; i < package_header->num_entries; i++) { > - if (package_header->fw_info[i].substepping == '*' && > - si->stepping == package_header->fw_info[i].stepping) { > - dmc_offset = package_header->fw_info[i].offset; > - break; > - } else if (si->stepping == package_header->fw_info[i].stepping && > - si->substepping == package_header->fw_info[i].substepping) { > - dmc_offset = package_header->fw_info[i].offset; > - break; > - } else if (package_header->fw_info[i].stepping == '*' && > - package_header->fw_info[i].substepping == '*') > - dmc_offset = package_header->fw_info[i].offset; > - } > + dmc_offset = find_dmc_fw_offset((struct intel_fw_info *) > + &fw->data[readcount], num_entries, si); > if (dmc_offset == CSR_DEFAULT_FW_OFFSET) { > DRM_ERROR("DMC firmware not supported for %c stepping\n", > si->stepping); > return NULL; > } > + /* we always have space for PACKAGE_MAX_FW_INFO_ENTRIES */ > + readcount += PACKAGE_MAX_FW_INFO_ENTRIES * sizeof(struct intel_fw_info); > + > /* Convert dmc_offset into number of bytes. By default it is in dwords*/ > dmc_offset *= 4; > readcount += dmc_offset; > -- > 2.21.0 >
>-----Original Message----- >From: De Marchi, Lucas >Sent: Thursday, May 23, 2019 1:24 AM >To: intel-gfx@lists.freedesktop.org >Cc: Jani Nikula <jani.nikula@linux.intel.com>; Srivatsa, Anusha ><anusha.srivatsa@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; De >Marchi, Lucas <lucas.demarchi@intel.com> >Subject: [PATCH 02/10] drm/i915/dmc: extract fw_info and table walk from >intel_package_header > >Move fw_info out of struct intel_package_header to allow it to grow more easily >in future. To make a cleaner move, let's also extract a function to search the >header for the dmc_offset. > >While reviewing this code I wondered why we continued the search even after >finding a suitable firmware. Add a comment to explain we will continue to try to >find a more specific firmware version, even if this is not required by the spec. > >Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com> >--- > drivers/gpu/drm/i915/intel_csr.c | 68 ++++++++++++++++++++++++-------- > 1 file changed, 51 insertions(+), 17 deletions(-) > >diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c >index b05e7a6aebc7..01ae356e69cc 100644 >--- a/drivers/gpu/drm/i915/intel_csr.c >+++ b/drivers/gpu/drm/i915/intel_csr.c >@@ -70,6 +70,7 @@ MODULE_FIRMWARE(SKL_CSR_PATH); >MODULE_FIRMWARE(BXT_CSR_PATH); > > #define CSR_DEFAULT_FW_OFFSET 0xFFFFFFFF >+#define PACKAGE_MAX_FW_INFO_ENTRIES 20 > > struct intel_css_header { > /* 0x09 for DMC */ >@@ -139,8 +140,6 @@ struct intel_package_header { > > /* Number of valid entries in the FWInfo array below */ > u32 num_entries; >- >- struct intel_fw_info fw_info[20]; > } __packed; > > struct intel_dmc_header { >@@ -292,6 +291,46 @@ void intel_csr_load_program(struct drm_i915_private >*dev_priv) > gen9_set_dc_state_debugmask(dev_priv); > } > >+/* >+ * Search fw_info table for dmc_offset to find firmware binary: >+num_entries is >+ * already sanitized. >+ */ >+static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info, >+ unsigned int num_entries, >+ const struct stepping_info *si) { >+ u32 dmc_offset = CSR_DEFAULT_FW_OFFSET; >+ unsigned int i; >+ >+ for (i = 0; i < num_entries; i++) { >+ if (fw_info[i].substepping == '*' && >+ si->stepping == fw_info[i].stepping) { >+ dmc_offset = fw_info[i].offset; >+ break; >+ } >+ >+ if (si->stepping == fw_info[i].stepping && >+ si->substepping == fw_info[i].substepping) { >+ dmc_offset = fw_info[i].offset; >+ break; >+ } >+ >+ if (fw_info[i].stepping == '*' && >+ fw_info[i].substepping == '*') { >+ /* >+ * In theory we should stop the search as generic >+ * entries should alwas come after the more specific >+ * ones, but let's continue to make sure to work even >+ * with "broken" firmwares. If we don't find a more >+ * specific one, then we use this entry >+ */ >+ dmc_offset = fw_info[i].offset; >+ } >+ } >+ >+ return dmc_offset; >+} >+ > static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > const struct firmware *fw) > { >@@ -300,7 +339,7 @@ static u32 *parse_csr_fw(struct drm_i915_private >*dev_priv, > struct intel_dmc_header *dmc_header; > struct intel_csr *csr = &dev_priv->csr; > const struct stepping_info *si = intel_get_stepping_info(dev_priv); >- u32 dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes; >+ u32 dmc_offset, num_entries, readcount = 0, nbytes; > u32 i; > u32 *dmc_payload; > >@@ -342,27 +381,22 @@ static u32 *parse_csr_fw(struct drm_i915_private >*dev_priv, > (package_header->header_len * 4)); > return NULL; > } >+ > readcount += sizeof(struct intel_package_header); >+ num_entries = package_header->num_entries; >+ if (WARN_ON(package_header->num_entries > >PACKAGE_MAX_FW_INFO_ENTRIES)) >+ num_entries = PACKAGE_MAX_FW_INFO_ENTRIES; > >- /* Search for dmc_offset to find firware binary. */ >- for (i = 0; i < package_header->num_entries; i++) { >- if (package_header->fw_info[i].substepping == '*' && >- si->stepping == package_header->fw_info[i].stepping) { >- dmc_offset = package_header->fw_info[i].offset; >- break; >- } else if (si->stepping == package_header->fw_info[i].stepping >&& >- si->substepping == package_header- >>fw_info[i].substepping) { >- dmc_offset = package_header->fw_info[i].offset; >- break; >- } else if (package_header->fw_info[i].stepping == '*' && >- package_header->fw_info[i].substepping == '*') >- dmc_offset = package_header->fw_info[i].offset; >- } >+ dmc_offset = find_dmc_fw_offset((struct intel_fw_info *) >+ &fw->data[readcount], num_entries, si); > if (dmc_offset == CSR_DEFAULT_FW_OFFSET) { > DRM_ERROR("DMC firmware not supported for %c stepping\n", > si->stepping); > return NULL; > } >+ /* we always have space for PACKAGE_MAX_FW_INFO_ENTRIES */ >+ readcount += PACKAGE_MAX_FW_INFO_ENTRIES * sizeof(struct >+intel_fw_info); >+ > /* Convert dmc_offset into number of bytes. By default it is in dwords*/ > dmc_offset *= 4; > readcount += dmc_offset; >-- >2.21.0
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;
Move fw_info out of struct intel_package_header to allow it to grow more easily in future. To make a cleaner move, let's also extract a function to search the header for the dmc_offset. While reviewing this code I wondered why we continued the search even after finding a suitable firmware. Add a comment to explain we will continue to try to find a more specific firmware version, even if this is not required by the spec. Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/intel_csr.c | 68 ++++++++++++++++++++++++-------- 1 file changed, 51 insertions(+), 17 deletions(-)