| Message ID | 20180906082126.15539-2-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915/csr: keep firmware name and required version together | expand |
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@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@chris-wilson.co.uk> -Chris
On Thu, 06 Sep 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > 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@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. I know... but decided to not open that can of worms. > >> } 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. That doesn't sit well with gen 11+ until we move the max size here too. >> } >> >> - /* >> - * 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@chris-wilson.co.uk> Will fix, thanks for the review. BR, Jani. > -Chris
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 */ 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) { 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. + */ + intel_display_power_get(dev_priv, POWER_DOMAIN_INIT); + + if (INTEL_GEN(dev_priv) >= 11) { + csr->max_fw_size = ICL_CSR_MAX_FW_SIZE; } 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; } 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; } 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; } 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; } 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; + } else { + MISSING_CASE(INTEL_REVID(dev_priv)); + return; } - /* - * 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; + } if (csr->fw_path == NULL) { DRM_DEBUG_KMS("No known CSR firmware for platform, disabling runtime PM\n");
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@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(-)