On Thu, 06 Sep 2018, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@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.
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@xxxxxxxxxxxxxxxxxx>
Will fix, thanks for the review.
BR,
Jani.
> -Chris
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
