[PATCH 1/3] drm/i915/csr: keep firmware name and required version together

[PATCH 1/3] drm/i915/csr: keep firmware name and required version together

[Jani Nikula]

Having two separate if ladders gets increasingly hard to maintain. Put
them together.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_csr.c | 54 ++++++++++++++++------------------------
 2 files changed, 23 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 767615ecdea5..368066010f94 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -464,6 +464,7 @@ struct drm_i915_display_funcs {
 struct intel_csr {
 	struct work_struct work;
 	const char *fw_path;
+	uint32_t required_version;
 	uint32_t *dmc_payload;
 	uint32_t dmc_fw_size;
 	uint32_t version;
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 14cf4c367e36..9a60bb9cc443 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -284,7 +284,6 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
 	uint32_t max_fw_size = 0;
 	uint32_t i;
 	uint32_t *dmc_payload;
-	uint32_t required_version;
 
 	if (!fw)
 		return NULL;
@@ -299,36 +298,19 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
 		return NULL;
 	}
 
-	csr->version = css_header->version;
-
-	if (csr->fw_path == i915_modparams.dmc_firmware_path) {
-		/* Bypass version check for firmware override. */
-		required_version = csr->version;
-	} else if (IS_CANNONLAKE(dev_priv)) {
-		required_version = CNL_CSR_VERSION_REQUIRED;
-	} else if (IS_GEMINILAKE(dev_priv)) {
-		required_version = GLK_CSR_VERSION_REQUIRED;
-	} else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
-		required_version = KBL_CSR_VERSION_REQUIRED;
-	} else if (IS_SKYLAKE(dev_priv)) {
-		required_version = SKL_CSR_VERSION_REQUIRED;
-	} else if (IS_BROXTON(dev_priv)) {
-		required_version = BXT_CSR_VERSION_REQUIRED;
-	} else {
-		MISSING_CASE(INTEL_REVID(dev_priv));
-		required_version = 0;
-	}
-
-	if (csr->version != required_version) {
+	if (csr->required_version &&
+	    css_header->version != csr->required_version) {
 		DRM_INFO("Refusing to load DMC firmware v%u.%u,"
 			 " please use v%u.%u\n",
-			 CSR_VERSION_MAJOR(csr->version),
-			 CSR_VERSION_MINOR(csr->version),
-			 CSR_VERSION_MAJOR(required_version),
-			 CSR_VERSION_MINOR(required_version));
+			 CSR_VERSION_MAJOR(css_header->version),
+			 CSR_VERSION_MINOR(css_header->version),
+			 CSR_VERSION_MAJOR(csr->required_version),
+			 CSR_VERSION_MINOR(csr->required_version));
 		return NULL;
 	}
 
+	csr->version = css_header->version;
+
 	readcount += sizeof(struct intel_css_header);
 
 	/* Extract Package Header information*/
@@ -469,18 +451,26 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv)
 	if (!HAS_CSR(dev_priv))
 		return;
 
-	if (i915_modparams.dmc_firmware_path)
+	if (i915_modparams.dmc_firmware_path) {
 		csr->fw_path = i915_modparams.dmc_firmware_path;
-	else if (IS_CANNONLAKE(dev_priv))
+		/* Bypass version check for firmware override. */
+		csr->required_version = 0;
+	} else if (IS_CANNONLAKE(dev_priv)) {
 		csr->fw_path = I915_CSR_CNL;
-	else if (IS_GEMINILAKE(dev_priv))
+		csr->required_version = CNL_CSR_VERSION_REQUIRED;
+	} else if (IS_GEMINILAKE(dev_priv)) {
 		csr->fw_path = I915_CSR_GLK;
-	else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv))
+		csr->required_version = GLK_CSR_VERSION_REQUIRED;
+	} else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
 		csr->fw_path = I915_CSR_KBL;
-	else if (IS_SKYLAKE(dev_priv))
+		csr->required_version = KBL_CSR_VERSION_REQUIRED;
+	} else if (IS_SKYLAKE(dev_priv)) {
 		csr->fw_path = I915_CSR_SKL;
-	else if (IS_BROXTON(dev_priv))
+		csr->required_version = SKL_CSR_VERSION_REQUIRED;
+	} else if (IS_BROXTON(dev_priv)) {
 		csr->fw_path = I915_CSR_BXT;
+		csr->required_version = BXT_CSR_VERSION_REQUIRED;
+	}
 
 	/*
 	 * Obtain a runtime pm reference, until CSR is loaded,
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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 */
 	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");
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915/csr: bypass firmware request on i915.dmc_firmware_path=""

[Jani Nikula]

With i915.dmc_firmware_path="" it's obvious the intention is to disable
CSR firmware loading. Bypass the firmware request altogether in this
case, with more obvious debug logging.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 956ac8bbf5e4..dbaeffddb5e7 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -476,6 +476,12 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv)
 	}
 
 	if (i915_modparams.dmc_firmware_path) {
+		if (strlen(i915_modparams.dmc_firmware_path) == 0) {
+			csr->fw_path = NULL;
+			DRM_DEBUG_KMS("Disabling CSR firmare and runtime PM\n");
+			return;
+		}
+
 		csr->fw_path = i915_modparams.dmc_firmware_path;
 		/* Bypass version check for firmware override. */
 		csr->required_version = 0;
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for series starting with [1/3] drm/i915/csr: keep firmware name and required version together

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/csr: keep firmware name and required version together
URL   : https://patchwork.freedesktop.org/series/49256/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/csr: keep firmware name and required version together
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3688:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3689:16: warning: expression using sizeof(void)

Commit: drm/i915/csr: keep max firmware size together with firmare name and version
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3689:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3690:16: warning: expression using sizeof(void)

Commit: drm/i915/csr: bypass firmware request on i915.dmc_firmware_path=""
Okay!

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/csr: keep firmware name and required version together

[Chris Wilson]

Quoting Jani Nikula (2018-09-06 09:21:24)
> Having two separate if ladders gets increasingly hard to maintain. Put
> them together.

Does it even have to be an if-ladder? Something like
struct platform_requirements {
	unsigned long platform_mask;
	u32 required_version;
	const char *path;
} [];

How does that work by patch 3?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/csr: bypass firmware request on i915.dmc_firmware_path=""

[Chris Wilson]

Quoting Jani Nikula (2018-09-06 09:21:26)
> With i915.dmc_firmware_path="" it's obvious the intention is to disable
> CSR firmware loading. Bypass the firmware request altogether in this
> case, with more obvious debug logging.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_csr.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 956ac8bbf5e4..dbaeffddb5e7 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -476,6 +476,12 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv)
>         }
>  
>         if (i915_modparams.dmc_firmware_path) {
> +               if (strlen(i915_modparams.dmc_firmware_path) == 0) {
> +                       csr->fw_path = NULL;
> +                       DRM_DEBUG_KMS("Disabling CSR firmare and runtime PM\n");

In response to a user parameter, could even be DRM_INFO().
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915/csr: keep firmware name and required version together

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/csr: keep firmware name and required version together
URL   : https://patchwork.freedesktop.org/series/49256/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4778 -> Patchwork_10107 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_10107 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10107, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/49256/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10107:

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_psr@primary_page_flip:
      fi-kbl-r:           PASS -> FAIL

    
== Known issues ==

  Here are the changes found in Patchwork_10107 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         PASS -> DMESG-FAIL (fdo#107164)

    igt@drv_selftest@live_hangcheck:
      fi-kbl-guc:         PASS -> DMESG-FAIL (fdo#106560, fdo#107837)

    igt@gem_exec_suspend@basic-s3:
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)

    igt@kms_psr@primary_page_flip:
      fi-icl-u:           NOTRUN -> FAIL (fdo#107383) +3

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         PASS -> FAIL (fdo#104008)

    
    ==== Possible fixes ====

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     FAIL (fdo#103167) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-byt-clapper:     FAIL (fdo#107362, fdo#103191) -> PASS +1

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107383 https://bugs.freedesktop.org/show_bug.cgi?id=107383
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107837 https://bugs.freedesktop.org/show_bug.cgi?id=107837


== Participating hosts (53 -> 49) ==

  Additional (1): fi-icl-u 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4778 -> Patchwork_10107

  CI_DRM_4778: 456cfc52e9f12423c6e597f677d8acb05851c3e3 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4631: 8884101aa01aedee01b2c3d0ac075473384551b7 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10107: 2475ce20b41d4d37edacba420d5ae1f475f37807 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

2475ce20b41d drm/i915/csr: bypass firmware request on i915.dmc_firmware_path=""
6f9a15361f00 drm/i915/csr: keep max firmware size together with firmare name and version
c232905796cd drm/i915/csr: keep firmware name and required version together

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10107/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/csr: keep firmware name and required version together

[Jani Nikula]

On Thu, 06 Sep 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Jani Nikula (2018-09-06 09:21:24)
>> Having two separate if ladders gets increasingly hard to maintain. Put
>> them together.
>
> Does it even have to be an if-ladder? Something like
> struct platform_requirements {
> 	unsigned long platform_mask;
> 	u32 required_version;
> 	const char *path;
> } [];
>
> How does that work by patch 3?

I expect this to evolve to the same kind of thing as the PCH detection
and checks where you can't get away with a simple platform mask, and you
have to resort to an if ladder. Or you need separate match functions,
which get unwieldy without lambda functions.

Also, I think we may need to have several firmware blobs per platform
that get requested in sequence, to allow for more graceful updates. In
any case, I think grouping this stuff together is a good first step.


BR,
Jani.


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

Re: [PATCH 1/3] drm/i915/csr: keep firmware name and required version together

[Chris Wilson]

Quoting Jani Nikula (2018-09-06 09:21:24)
> Having two separate if ladders gets increasingly hard to maintain. Put
> them together.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_csr.c | 54 ++++++++++++++++------------------------
>  2 files changed, 23 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 767615ecdea5..368066010f94 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -464,6 +464,7 @@ struct drm_i915_display_funcs {
>  struct intel_csr {
>         struct work_struct work;
>         const char *fw_path;
> +       uint32_t required_version;
>         uint32_t *dmc_payload;
>         uint32_t dmc_fw_size;
>         uint32_t version;
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 14cf4c367e36..9a60bb9cc443 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -284,7 +284,6 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
>         uint32_t max_fw_size = 0;
>         uint32_t i;
>         uint32_t *dmc_payload;
> -       uint32_t required_version;
>  
>         if (!fw)
>                 return NULL;
> @@ -299,36 +298,19 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
>                 return NULL;
>         }
>  
> -       csr->version = css_header->version;
> -
> -       if (csr->fw_path == i915_modparams.dmc_firmware_path) {
> -               /* Bypass version check for firmware override. */
> -               required_version = csr->version;
> -       } else if (IS_CANNONLAKE(dev_priv)) {
> -               required_version = CNL_CSR_VERSION_REQUIRED;
> -       } else if (IS_GEMINILAKE(dev_priv)) {
> -               required_version = GLK_CSR_VERSION_REQUIRED;
> -       } else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
> -               required_version = KBL_CSR_VERSION_REQUIRED;
> -       } else if (IS_SKYLAKE(dev_priv)) {
> -               required_version = SKL_CSR_VERSION_REQUIRED;
> -       } else if (IS_BROXTON(dev_priv)) {
> -               required_version = BXT_CSR_VERSION_REQUIRED;
> -       } else {
> -               MISSING_CASE(INTEL_REVID(dev_priv));
> -               required_version = 0;
> -       }
> -
> -       if (csr->version != required_version) {
> +       if (csr->required_version &&
> +           css_header->version != csr->required_version) {
>                 DRM_INFO("Refusing to load DMC firmware v%u.%u,"
>                          " please use v%u.%u\n",
> -                        CSR_VERSION_MAJOR(csr->version),
> -                        CSR_VERSION_MINOR(csr->version),
> -                        CSR_VERSION_MAJOR(required_version),
> -                        CSR_VERSION_MINOR(required_version));
> +                        CSR_VERSION_MAJOR(css_header->version),
> +                        CSR_VERSION_MINOR(css_header->version),
> +                        CSR_VERSION_MAJOR(csr->required_version),
> +                        CSR_VERSION_MINOR(csr->required_version));
>                 return NULL;
>         }
>  
> +       csr->version = css_header->version;

Ok, so far.

> +
>         readcount += sizeof(struct intel_css_header);
>  
>         /* Extract Package Header information*/
> @@ -469,18 +451,26 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv)
>         if (!HAS_CSR(dev_priv))
>                 return;
>  
> -       if (i915_modparams.dmc_firmware_path)
> +       if (i915_modparams.dmc_firmware_path) {
>                 csr->fw_path = i915_modparams.dmc_firmware_path;
> -       else if (IS_CANNONLAKE(dev_priv))
> +               /* Bypass version check for firmware override. */
> +               csr->required_version = 0;
Ok.

> +       } else if (IS_CANNONLAKE(dev_priv)) {
>                 csr->fw_path = I915_CSR_CNL;
> -       else if (IS_GEMINILAKE(dev_priv))
> +               csr->required_version = CNL_CSR_VERSION_REQUIRED;
Ok.

> +       } else if (IS_GEMINILAKE(dev_priv)) {
>                 csr->fw_path = I915_CSR_GLK;
> -       else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv))
> +               csr->required_version = GLK_CSR_VERSION_REQUIRED;
Ok.

> +       } else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
>                 csr->fw_path = I915_CSR_KBL;
> -       else if (IS_SKYLAKE(dev_priv))
> +               csr->required_version = KBL_CSR_VERSION_REQUIRED;
Ok.

> +       } else if (IS_SKYLAKE(dev_priv)) {
>                 csr->fw_path = I915_CSR_SKL;
> -       else if (IS_BROXTON(dev_priv))
> +               csr->required_version = SKL_CSR_VERSION_REQUIRED;
Ok.

> +       } else if (IS_BROXTON(dev_priv)) {
>                 csr->fw_path = I915_CSR_BXT;
> +               csr->required_version = BXT_CSR_VERSION_REQUIRED;
Ok.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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

Re: [PATCH 3/3] drm/i915/csr: bypass firmware request on i915.dmc_firmware_path=""

[Chris Wilson]

Quoting Chris Wilson (2018-09-06 10:08:37)
> Quoting Jani Nikula (2018-09-06 09:21:26)
> > With i915.dmc_firmware_path="" it's obvious the intention is to disable
> > CSR firmware loading. Bypass the firmware request altogether in this
> > case, with more obvious debug logging.
> > 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_csr.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> > index 956ac8bbf5e4..dbaeffddb5e7 100644
> > --- a/drivers/gpu/drm/i915/intel_csr.c
> > +++ b/drivers/gpu/drm/i915/intel_csr.c
> > @@ -476,6 +476,12 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv)
> >         }
> >  
> >         if (i915_modparams.dmc_firmware_path) {
> > +               if (strlen(i915_modparams.dmc_firmware_path) == 0) {
> > +                       csr->fw_path = NULL;
> > +                       DRM_DEBUG_KMS("Disabling CSR firmare and runtime PM\n");
> 
> In response to a user parameter, could even be DRM_INFO().

Either way (but my preference here is for DRM_INFO),
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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