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