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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx