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

[Jani Nikula <jani.nikula@intel.com>]

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

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx