>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Friday, June 7, 2019 2:12 AM
>To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>Cc: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; Srivatsa, Anusha
><anusha.srivatsa@xxxxxxxxx>; De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>
>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@xxxxxxxxx>
Reviewed-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
>---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
