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