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

[PATCH v3 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.

v2: Rebase

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_csr.c | 59 ++++++++++++++++------------------------
 2 files changed, 25 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8624b4bdc242..37f5fcf1644d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -465,6 +465,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 4aa8f3d6b64c..ee277ec33996 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -287,7 +287,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;
@@ -302,38 +301,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_ICELAKE(dev_priv)) {
-		required_version = ICL_CSR_VERSION_REQUIRED;
-	} 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*/
@@ -474,20 +454,29 @@ 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_ICELAKE(dev_priv))
+		/* Bypass version check for firmware override. */
+		csr->required_version = 0;
+	} else if (IS_ICELAKE(dev_priv)) {
 		csr->fw_path = I915_CSR_ICL;
-	else if (IS_CANNONLAKE(dev_priv))
+		csr->required_version = ICL_CSR_VERSION_REQUIRED;
+	} 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 v3 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. While at it, extend the comment on why we return
with the reference held on errors.

We also need to move the module parameter override later to reuse the
max firmware size, which is independent of the override.

v2: Add comment on why we leak the wakeref on errors (Chris)

v3: Rebase

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v2
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  3 ++-
 drivers/gpu/drm/i915/intel_csr.c | 46 +++++++++++++++++++++++-----------------
 2 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 37f5fcf1644d..9e185f0a822a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -466,8 +466,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 ee277ec33996..b3a084c48a94 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,
 	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;
 
@@ -381,15 +380,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;
 	}
@@ -454,35 +445,50 @@ 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;
-	} else if (IS_ICELAKE(dev_priv)) {
+	/*
+	 * 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);
+
+	if (IS_ICELAKE(dev_priv)) {
 		csr->fw_path = I915_CSR_ICL;
 		csr->required_version = ICL_CSR_VERSION_REQUIRED;
+		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 v3 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.

v2: Use DRM_INFO for logging (Chris)

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
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 b3a084c48a94..81935da9cd6b 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -485,6 +485,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_INFO("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

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

[Chris Wilson]

Quoting Jani Nikula (2018-09-26 14:34:12)
> Having two separate if ladders gets increasingly hard to maintain. Put
> them together.
> 
> v2: Rebase
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1

Didn't spot anything wrong in the revisions, so keep the r-b on both
patches.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Wed, 26 Sep 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Jani Nikula (2018-09-26 14:34:12)
>> Having two separate if ladders gets increasingly hard to maintain. Put
>> them together.
>> 
>> v2: Rebase
>> 
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
>
> Didn't spot anything wrong in the revisions, so keep the r-b on both
> patches.

Thanks for the quick check; I'll push once the CI overlords give me the
green light.

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

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

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/3] drm/i915/csr: keep firmware name and required version together
URL   : https://patchwork.freedesktop.org/series/50215/
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:3718:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3719: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:3719:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3720: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

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

[Patchwork]

== Series Details ==

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

== Summary ==

= CI Bug Log - changes from CI_DRM_4882 -> Patchwork_10285 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@basic-flip-vs-modeset:
      fi-skl-6700hq:      PASS -> DMESG-WARN (fdo#105998)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)

    igt@kms_psr@primary_page_flip:
      fi-cnl-u:           PASS -> FAIL (fdo#107336)

    
    ==== Possible fixes ====

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

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

    igt@kms_psr@primary_page_flip:
      fi-kbl-r:           FAIL (fdo#107336) -> PASS

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718


== Participating hosts (46 -> 42) ==

  Additional (1): fi-snb-2520m 
  Missing    (5): fi-bsw-cyan fi-ilk-m540 fi-byt-squawks fi-icl-u2 fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4882 -> Patchwork_10285

  CI_DRM_4882: ac10240c56532bc7ecbd3c2ba786cf2778bfff8b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4650: a6e21812d100dce68450727e79fc09e0c0033683 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10285: 2a2502d0bae8e73022c3e2644f727ffe9e4b6eb6 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

2a2502d0bae8 drm/i915/csr: bypass firmware request on i915.dmc_firmware_path=""
00284cb89dc5 drm/i915/csr: keep max firmware size together with firmare name and version
a6b6fca90ce9 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_10285/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Wed, 26 Sep 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> On Wed, 26 Sep 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> Quoting Jani Nikula (2018-09-26 14:34:12)
>>> Having two separate if ladders gets increasingly hard to maintain. Put
>>> them together.
>>> 
>>> v2: Rebase
>>> 
>>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
>>
>> Didn't spot anything wrong in the revisions, so keep the r-b on both
>> patches.
>
> Thanks for the quick check; I'll push once the CI overlords give me the
> green light.

And pushed, thanks.

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