Re: [PATCH 2/7] drm/i915/skl: Refuse to load outdated dmc firmware

Imre Deak
2015-10-29T15:39:35Z

On ti, 2015-10-27 at 14:47 +0200, Mika Kuoppala wrote:
> There is known issue on GT interrupt delivery with DC6 and
> firmwares <1.21. There is a suspicion that this causes
> spurious gpu hangs on driver init and with some workloads,
> as upgrading the firmware to 1.21 makes these problems
> disappear.
>
> As of now the current version included in distribution
> firmware packages is very like to be 1.19. Play it safe and
> refuse to load a firmware version that may affect gpu
> side stability.
>
> With < 1.23 there is a palette and dmc ram corruption issue
> so blacklist anything below that.
>
> v2: Refuse to load fw instead of notifying the user
> v3: Rebase on header version changes
> v4: Refuse to load anything less than 1.23
> v5: Give enough information for user for finding correct fw (Chris)
> v6: better url and formatting (Chris)
> v7: move error log for each fail path (Mika)
> bail out earlier in load path (Imre)
>
> Cc: Animesh Manna <animesh.manna@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Cc: Arun Siluvery <arun.siluvery@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> References: https://01.org/linuxgraphics/downloads/skldmcver121
> References: https://01.org/linuxgraphics/downloads/skylake-dmc-1.23
> Testcase: igt/gem_exec_nop
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
> drivers/gpu/drm/i915/intel_csr.c | 37 +++++++++++++++++++++++++++----------
> 1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index e620e85..701c685 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -47,6 +47,9 @@
> MODULE_FIRMWARE(I915_CSR_SKL);
> MODULE_FIRMWARE(I915_CSR_BXT);
>
> +#define SKL_REQUIRED_FW_MAJOR 1
> +#define SKL_REQUIRED_FW_MINOR 23
> +
> /*
> * SKL CSR registers for DC5 and DC6
> */
> @@ -303,10 +306,8 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> uint32_t *dmc_payload;
> bool fw_loaded = false;
>
> - if (!fw) {
> - i915_firmware_load_error_print(csr->fw_path, 0);
> + if (!fw)
> goto out;
> - }
>
> if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
> DRM_ERROR("Unknown stepping info, firmware loading failed\n");
> @@ -324,6 +325,19 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>
> csr->version = css_header->version;
>
> + if (IS_SKYLAKE(dev) &&
> + (CSR_VERSION_MAJOR(csr->version) < SKL_REQUIRED_FW_MAJOR ||
> + CSR_VERSION_MINOR(csr->version) < SKL_REQUIRED_FW_MINOR)) {

This would also reject 2.22 too for example, isn't that a problem?
If so, after fixing that:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> + DRM_INFO("Refusing to load old Skylake DMC firmware v%u.%u,"
> + " please upgrade to v%u.%u or later"
> + " [https://01.org/linuxgraphics/intel-linux-graphics-firmwares].\n",
> + CSR_VERSION_MAJOR(csr->version),
> + CSR_VERSION_MINOR(csr->version),
> + SKL_REQUIRED_FW_MAJOR,
> + SKL_REQUIRED_FW_MINOR);
> + goto out;
> + }
> +
> readcount += sizeof(struct intel_css_header);
>
> /* Extract Package Header information*/
> @@ -405,17 +419,20 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> intel_csr_load_program(dev);
> fw_loaded = true;
>
> - DRM_INFO("Finished loading %s (v%u.%u)\n",
> - dev_priv->csr.fw_path,
> - CSR_VERSION_MAJOR(csr->version),
> - CSR_VERSION_MINOR(csr->version));
> -
> out:
> - if (fw_loaded)
> + if (fw_loaded) {
> intel_runtime_pm_put(dev_priv);
> - else
> +
> + DRM_INFO("Finished loading %s (v%u.%u)\n",
> + dev_priv->csr.fw_path,
> + CSR_VERSION_MAJOR(csr->version),
> + CSR_VERSION_MINOR(csr->version));
> + } else {
> intel_csr_load_status_set(dev_priv, FW_FAILED);
>
> + i915_firmware_load_error_print(csr->fw_path, 0);
> + }
> +
> release_firmware(fw);
> }