[Intel-gfx] [PATCH 2/3] drm/i915/csr: keep max firmware size together with firmare name and version

[Chris Wilson]

Quoting Jani Nikula (2018-09-06 09:21:25)
> Move max firmware size to the same if ladder with firmware name and
> required version. This allows us to detect the missing max size for a
> platform without actually loading the firmware, and makes the whole
> thing easier to maintain.
> 
> We need to move the power get earlier to allow for early return in the
> missing platform case. We also need to move the module parameter
> override later to reuse the max firmware size, which is independent of
> the override. Note how this works with gen 11+ which don't have a
> specified firmware blob yet, but do have a maximum size.
> 
> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  3 ++-
>  drivers/gpu/drm/i915/intel_csr.c | 41 +++++++++++++++++++++-------------------
>  2 files changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 368066010f94..62444f4c3c8e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -465,8 +465,9 @@ struct intel_csr {
>         struct work_struct work;
>         const char *fw_path;
>         uint32_t required_version;
> +       uint32_t max_fw_size; /* bytes */
>         uint32_t *dmc_payload;
> -       uint32_t dmc_fw_size;
> +       uint32_t dmc_fw_size; /* dwords */

Appreciated.

>         uint32_t version;
>         uint32_t mmio_count;
>         i915_reg_t mmioaddr[8];
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 9a60bb9cc443..956ac8bbf5e4 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -281,7 +281,6 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
>         struct intel_csr *csr = &dev_priv->csr;
>         const struct stepping_info *si = intel_get_stepping_info(dev_priv);
>         uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
> -       uint32_t max_fw_size = 0;
>         uint32_t i;
>         uint32_t *dmc_payload;
>  
> @@ -378,15 +377,7 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
>  
>         /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
>         nbytes = dmc_header->fw_size * 4;
> -       if (INTEL_GEN(dev_priv) >= 11)
> -               max_fw_size = ICL_CSR_MAX_FW_SIZE;
> -       else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> -               max_fw_size = GLK_CSR_MAX_FW_SIZE;
> -       else if (IS_GEN9(dev_priv))
> -               max_fw_size = BXT_CSR_MAX_FW_SIZE;
> -       else
> -               MISSING_CASE(INTEL_REVID(dev_priv));
> -       if (nbytes > max_fw_size) {
> +       if (nbytes > csr->max_fw_size) {

Ok.
>                 DRM_ERROR("DMC FW too big (%u bytes)\n", nbytes);
>                 return NULL;
>         }
> @@ -451,32 +442,44 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv)
>         if (!HAS_CSR(dev_priv))
>                 return;
>  
> -       if (i915_modparams.dmc_firmware_path) {
> -               csr->fw_path = i915_modparams.dmc_firmware_path;
> -               /* Bypass version check for firmware override. */
> -               csr->required_version = 0;
> +       /*
> +        * Obtain a runtime pm reference, until CSR is loaded,
> +        * to avoid entering runtime-suspend.

	   * On error, we return with the rpm wakeref held to prevent
	   * runtime suspend as runtime suspend *requires* a working
	   * CSR for whatever reason.
> +        */
> +       intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);

A reminder that the leak is by design. Bonus points for integrating with
the wakeref tracking so that we demonstrate we aren't just leaking for
the fun of it.

> +       if (INTEL_GEN(dev_priv) >= 11) {
> +               csr->max_fw_size = ICL_CSR_MAX_FW_SIZE;

Ok.

>         } else if (IS_CANNONLAKE(dev_priv)) {
>                 csr->fw_path = I915_CSR_CNL;
>                 csr->required_version = CNL_CSR_VERSION_REQUIRED;
> +               csr->max_fw_size = GLK_CSR_MAX_FW_SIZE;
Ok.

>         } else if (IS_GEMINILAKE(dev_priv)) {
>                 csr->fw_path = I915_CSR_GLK;
>                 csr->required_version = GLK_CSR_VERSION_REQUIRED;
> +               csr->max_fw_size = GLK_CSR_MAX_FW_SIZE;
Ok.

>         } else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
>                 csr->fw_path = I915_CSR_KBL;
>                 csr->required_version = KBL_CSR_VERSION_REQUIRED;
> +               csr->max_fw_size = BXT_CSR_MAX_FW_SIZE;
Ok.

>         } else if (IS_SKYLAKE(dev_priv)) {
>                 csr->fw_path = I915_CSR_SKL;
>                 csr->required_version = SKL_CSR_VERSION_REQUIRED;
> +               csr->max_fw_size = BXT_CSR_MAX_FW_SIZE;
Ok, but interesting naming scheme.

>         } else if (IS_BROXTON(dev_priv)) {
>                 csr->fw_path = I915_CSR_BXT;
>                 csr->required_version = BXT_CSR_VERSION_REQUIRED;
> +               csr->max_fw_size = BXT_CSR_MAX_FW_SIZE;

Ok.

> +       } else {
> +               MISSING_CASE(INTEL_REVID(dev_priv));
> +               return;

Hmm. Feels like this should be retrofitted to patch 1, but whatever.
>         }
>  
> -       /*
> -        * Obtain a runtime pm reference, until CSR is loaded,
> -        * to avoid entering runtime-suspend.
> -        */
> -       intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> +       if (i915_modparams.dmc_firmware_path) {
> +               csr->fw_path = i915_modparams.dmc_firmware_path;
> +               /* Bypass version check for firmware override. */
> +               csr->required_version = 0;
> +       }

Ok.

Please add the comment describing the intentional leak,
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris