[v2] drm/i915/guc: Consolidate firmware major-minor to one place

Submitted by Tvrtko Ursulin on Aug. 10, 2016, 3:16 p.m.

Details

Message ID 1470842206-35685-1-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show
Series "drm/i915/guc: Consolidate firmware major-minor to one place" ( rev: 2 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Tvrtko Ursulin Aug. 10, 2016, 3:16 p.m.
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Currently to change the firmware one has to update the exported
module firmware string and the major-minor versions used for
verification after load. Consolidate that to a single place
defining correct major and minor versions per platform.

v2: Rebased for KBL.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Dave Gordon <david.s.gordon@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Peter Antoine <peter.antoine@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Reviewed-by: Dave Gordon <david.s.gordon@intel.com> (v1)
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 3763e30cc165..bfcf6b5d29ad 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -59,13 +59,25 @@ 
  *
  */
 
-#define I915_SKL_GUC_UCODE "i915/skl_guc_ver6_1.bin"
+#define SKL_FW_MAJOR 6
+#define SKL_FW_MINOR 1
+
+#define BXT_FW_MAJOR 8
+#define BXT_FW_MINOR 7
+
+#define KBL_FW_MAJOR 9
+#define KBL_FW_MINOR 14
+
+#define GUC_FW_PATH(platform, major, minor) \
+       "i915/" __stringify(platform) "_guc_ver" __stringify(major) "_" __stringify(minor) ".bin"
+
+#define I915_SKL_GUC_UCODE GUC_FW_PATH(skl, SKL_FW_MAJOR, SKL_FW_MINOR)
 MODULE_FIRMWARE(I915_SKL_GUC_UCODE);
 
-#define I915_BXT_GUC_UCODE "i915/bxt_guc_ver8_7.bin"
+#define I915_BXT_GUC_UCODE GUC_FW_PATH(bxt, BXT_FW_MAJOR, BXT_FW_MINOR)
 MODULE_FIRMWARE(I915_BXT_GUC_UCODE);
 
-#define I915_KBL_GUC_UCODE "i915/kbl_guc_ver9_14.bin"
+#define I915_KBL_GUC_UCODE GUC_FW_PATH(kbl, KBL_FW_MAJOR, KBL_FW_MINOR)
 MODULE_FIRMWARE(I915_KBL_GUC_UCODE);
 
 /* User-friendly representation of an enum */
@@ -697,16 +709,16 @@  void intel_guc_init(struct drm_device *dev)
 		fw_path = NULL;
 	} else if (IS_SKYLAKE(dev)) {
 		fw_path = I915_SKL_GUC_UCODE;
-		guc_fw->guc_fw_major_wanted = 6;
-		guc_fw->guc_fw_minor_wanted = 1;
+		guc_fw->guc_fw_major_wanted = SKL_FW_MAJOR;
+		guc_fw->guc_fw_minor_wanted = SKL_FW_MINOR;
 	} else if (IS_BROXTON(dev)) {
 		fw_path = I915_BXT_GUC_UCODE;
-		guc_fw->guc_fw_major_wanted = 8;
-		guc_fw->guc_fw_minor_wanted = 7;
+		guc_fw->guc_fw_major_wanted = BXT_FW_MAJOR;
+		guc_fw->guc_fw_minor_wanted = BXT_FW_MINOR;
 	} else if (IS_KABYLAKE(dev)) {
 		fw_path = I915_KBL_GUC_UCODE;
-		guc_fw->guc_fw_major_wanted = 9;
-		guc_fw->guc_fw_minor_wanted = 14;
+		guc_fw->guc_fw_major_wanted = KBL_FW_MAJOR;
+		guc_fw->guc_fw_minor_wanted = KBL_FW_MINOR;
 	} else {
 		fw_path = "";	/* unknown device */
 	}

Comments

On 10/08/16 16:16, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Currently to change the firmware one has to update the exported
> module firmware string and the major-minor versions used for
> verification after load. Consolidate that to a single place
> defining correct major and minor versions per platform.
>
> v2: Rebased for KBL.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Peter Antoine <peter.antoine@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Reviewed-by: Dave Gordon <david.s.gordon@intel.com> (v1)

You can carry the R-b over to v2 too :)
But wouldn't it be even nicer to unify with the HuC and DMC loaders too?

.Dave.

> ---
>  drivers/gpu/drm/i915/intel_guc_loader.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 3763e30cc165..bfcf6b5d29ad 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -59,13 +59,25 @@
>   *
>   */
>
> -#define I915_SKL_GUC_UCODE "i915/skl_guc_ver6_1.bin"
> +#define SKL_FW_MAJOR 6
> +#define SKL_FW_MINOR 1
> +
> +#define BXT_FW_MAJOR 8
> +#define BXT_FW_MINOR 7
> +
> +#define KBL_FW_MAJOR 9
> +#define KBL_FW_MINOR 14
> +
> +#define GUC_FW_PATH(platform, major, minor) \
> +       "i915/" __stringify(platform) "_guc_ver" __stringify(major) "_" __stringify(minor) ".bin"
> +
> +#define I915_SKL_GUC_UCODE GUC_FW_PATH(skl, SKL_FW_MAJOR, SKL_FW_MINOR)
>  MODULE_FIRMWARE(I915_SKL_GUC_UCODE);
>
> -#define I915_BXT_GUC_UCODE "i915/bxt_guc_ver8_7.bin"
> +#define I915_BXT_GUC_UCODE GUC_FW_PATH(bxt, BXT_FW_MAJOR, BXT_FW_MINOR)
>  MODULE_FIRMWARE(I915_BXT_GUC_UCODE);
>
> -#define I915_KBL_GUC_UCODE "i915/kbl_guc_ver9_14.bin"
> +#define I915_KBL_GUC_UCODE GUC_FW_PATH(kbl, KBL_FW_MAJOR, KBL_FW_MINOR)
>  MODULE_FIRMWARE(I915_KBL_GUC_UCODE);
>
>  /* User-friendly representation of an enum */
> @@ -697,16 +709,16 @@ void intel_guc_init(struct drm_device *dev)
>  		fw_path = NULL;
>  	} else if (IS_SKYLAKE(dev)) {
>  		fw_path = I915_SKL_GUC_UCODE;
> -		guc_fw->guc_fw_major_wanted = 6;
> -		guc_fw->guc_fw_minor_wanted = 1;
> +		guc_fw->guc_fw_major_wanted = SKL_FW_MAJOR;
> +		guc_fw->guc_fw_minor_wanted = SKL_FW_MINOR;
>  	} else if (IS_BROXTON(dev)) {
>  		fw_path = I915_BXT_GUC_UCODE;
> -		guc_fw->guc_fw_major_wanted = 8;
> -		guc_fw->guc_fw_minor_wanted = 7;
> +		guc_fw->guc_fw_major_wanted = BXT_FW_MAJOR;
> +		guc_fw->guc_fw_minor_wanted = BXT_FW_MINOR;
>  	} else if (IS_KABYLAKE(dev)) {
>  		fw_path = I915_KBL_GUC_UCODE;
> -		guc_fw->guc_fw_major_wanted = 9;
> -		guc_fw->guc_fw_minor_wanted = 14;
> +		guc_fw->guc_fw_major_wanted = KBL_FW_MAJOR;
> +		guc_fw->guc_fw_minor_wanted = KBL_FW_MINOR;
>  	} else {
>  		fw_path = "";	/* unknown device */
>  	}
>
Should we not get the HuC patches merged before we amend them?

Peter.

-----Original Message-----
From: Gordon, David S 

Sent: Wednesday, August 10, 2016 4:25 PM
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>; Intel-gfx@lists.freedesktop.org
Cc: Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Antoine, Peter <peter.antoine@intel.com>; Thierry, Michel <michel.thierry@intel.com>
Subject: Re: [PATCH v2] drm/i915/guc: Consolidate firmware major-minor to one place

On 10/08/16 16:16, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

>

> Currently to change the firmware one has to update the exported module 

> firmware string and the major-minor versions used for verification 

> after load. Consolidate that to a single place defining correct major 

> and minor versions per platform.

>

> v2: Rebased for KBL.

>

> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

> Cc: Dave Gordon <david.s.gordon@intel.com>

> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

> Cc: Peter Antoine <peter.antoine@intel.com>

> Cc: Michel Thierry <michel.thierry@intel.com>

> Reviewed-by: Dave Gordon <david.s.gordon@intel.com> (v1)


You can carry the R-b over to v2 too :)
But wouldn't it be even nicer to unify with the HuC and DMC loaders too?

.Dave.

> ---

>  drivers/gpu/drm/i915/intel_guc_loader.c | 30 

> +++++++++++++++++++++---------

>  1 file changed, 21 insertions(+), 9 deletions(-)

>

> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c 

> b/drivers/gpu/drm/i915/intel_guc_loader.c

> index 3763e30cc165..bfcf6b5d29ad 100644

> --- a/drivers/gpu/drm/i915/intel_guc_loader.c

> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c

> @@ -59,13 +59,25 @@

>   *

>   */

>

> -#define I915_SKL_GUC_UCODE "i915/skl_guc_ver6_1.bin"

> +#define SKL_FW_MAJOR 6

> +#define SKL_FW_MINOR 1

> +

> +#define BXT_FW_MAJOR 8

> +#define BXT_FW_MINOR 7

> +

> +#define KBL_FW_MAJOR 9

> +#define KBL_FW_MINOR 14

> +

> +#define GUC_FW_PATH(platform, major, minor) \

> +       "i915/" __stringify(platform) "_guc_ver" __stringify(major) "_" __stringify(minor) ".bin"

> +

> +#define I915_SKL_GUC_UCODE GUC_FW_PATH(skl, SKL_FW_MAJOR, 

> +SKL_FW_MINOR)

>  MODULE_FIRMWARE(I915_SKL_GUC_UCODE);

>

> -#define I915_BXT_GUC_UCODE "i915/bxt_guc_ver8_7.bin"

> +#define I915_BXT_GUC_UCODE GUC_FW_PATH(bxt, BXT_FW_MAJOR, 

> +BXT_FW_MINOR)

>  MODULE_FIRMWARE(I915_BXT_GUC_UCODE);

>

> -#define I915_KBL_GUC_UCODE "i915/kbl_guc_ver9_14.bin"

> +#define I915_KBL_GUC_UCODE GUC_FW_PATH(kbl, KBL_FW_MAJOR, 

> +KBL_FW_MINOR)

>  MODULE_FIRMWARE(I915_KBL_GUC_UCODE);

>

>  /* User-friendly representation of an enum */ @@ -697,16 +709,16 @@ 

> void intel_guc_init(struct drm_device *dev)

>  		fw_path = NULL;

>  	} else if (IS_SKYLAKE(dev)) {

>  		fw_path = I915_SKL_GUC_UCODE;

> -		guc_fw->guc_fw_major_wanted = 6;

> -		guc_fw->guc_fw_minor_wanted = 1;

> +		guc_fw->guc_fw_major_wanted = SKL_FW_MAJOR;

> +		guc_fw->guc_fw_minor_wanted = SKL_FW_MINOR;

>  	} else if (IS_BROXTON(dev)) {

>  		fw_path = I915_BXT_GUC_UCODE;

> -		guc_fw->guc_fw_major_wanted = 8;

> -		guc_fw->guc_fw_minor_wanted = 7;

> +		guc_fw->guc_fw_major_wanted = BXT_FW_MAJOR;

> +		guc_fw->guc_fw_minor_wanted = BXT_FW_MINOR;

>  	} else if (IS_KABYLAKE(dev)) {

>  		fw_path = I915_KBL_GUC_UCODE;

> -		guc_fw->guc_fw_major_wanted = 9;

> -		guc_fw->guc_fw_minor_wanted = 14;

> +		guc_fw->guc_fw_major_wanted = KBL_FW_MAJOR;

> +		guc_fw->guc_fw_minor_wanted = KBL_FW_MINOR;

>  	} else {

>  		fw_path = "";	/* unknown device */

>  	}

>
On 10/08/16 17:58, Patchwork wrote:
> == Series Details ==
>
> Series: drm/i915/guc: Consolidate firmware major-minor to one place (rev2)
> URL   : https://patchwork.freedesktop.org/series/9318/
> State : warning
>
> == Summary ==
>
> Series 9318v2 drm/i915/guc: Consolidate firmware major-minor to one place
> http://patchwork.freedesktop.org/api/1.0/series/9318/revisions/2/mbox
>
> Test drv_module_reload_basic:
>                  skip       -> PASS       (fi-skl-i5-6260u)
> Test kms_cursor_legacy:
>          Subgroup basic-flip-vs-cursor-legacy:
>                  fail       -> PASS       (fi-hsw-i7-4770k)
>                  fail       -> PASS       (ro-bdw-i7-5557U)
>                  fail       -> PASS       (ro-bdw-i5-5250u)
>                  fail       -> PASS       (ro-skl3-i5-6260u)
> Test kms_pipe_crc_basic:
>          Subgroup suspend-read-crc-pipe-b:
>                  pass       -> DMESG-WARN (ro-bdw-i7-5600u)


https://bugs.freedesktop.org/show_bug.cgi?id=94992

[  522.498109] WARNING: CPU: 3 PID: 7786 at 
drivers/gpu/drm/i915/intel_display.c:13657 
intel_atomic_commit_tail+0x10fe/0x1110 [i915]
[  522.498110] pipe A vblank wait timed out

>                  dmesg-warn -> SKIP       (ro-bdw-i7-5557U)
>          Subgroup suspend-read-crc-pipe-c:
>                  skip       -> DMESG-WARN (ro-bdw-i5-5250u)

https://bugs.freedesktop.org/show_bug.cgi?id=96614

[ 482.345981] [drm:intel_dp_link_training_clock_recovery [i915]] *ERROR* 
failed to enable link training
[ 482.431185] [drm:intel_dp_start_link_train [i915]] *ERROR* failed to 
start channel equalization

>
> fi-hsw-i7-4770k  total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22
> fi-skl-i5-6260u  total:244  pass:224  dwarn:4   dfail:0   fail:2   skip:14
> fi-skl-i7-6700k  total:244  pass:208  dwarn:4   dfail:2   fail:2   skip:28
> fi-snb-i7-2600   total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42
> ro-bdw-i5-5250u  total:240  pass:219  dwarn:2   dfail:0   fail:1   skip:18
> ro-bdw-i7-5557U  total:240  pass:220  dwarn:1   dfail:0   fail:0   skip:19
> ro-bdw-i7-5600u  total:240  pass:206  dwarn:1   dfail:0   fail:1   skip:32
> ro-bsw-n3050     total:240  pass:194  dwarn:0   dfail:0   fail:4   skip:42
> ro-byt-n2820     total:240  pass:197  dwarn:0   dfail:0   fail:3   skip:40
> ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26
> ro-hsw-i7-4770r  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26
> ro-ilk1-i5-650   total:235  pass:173  dwarn:0   dfail:0   fail:2   skip:60
> ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35
> ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31
> ro-skl3-i5-6260u total:240  pass:223  dwarn:0   dfail:0   fail:3   skip:14
>
> Results at /archive/results/CI_IGT_test/RO_Patchwork_1824/
>
> 3aec82c drm-intel-nightly: 2016y-08m-10d-15h-08m-03s UTC integration manifest
> e3c7d5e drm/i915/guc: Consolidate firmware major-minor to one place

Merged to dinq, thanks for the review!

Regards,

Tvrtko