* [Intel-gfx] [PATCH v3 0/2] drm/i915/dmc: Make firmware loading backwards-compatible
@ 2022-12-29 19:07 Gustavo Sousa
2022-12-29 19:07 ` [Intel-gfx] [PATCH v3 1/2] drm/i915/dmc: Do not require specific versions Gustavo Sousa
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Gustavo Sousa @ 2022-12-29 19:07 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula, Lucas De Marchi, Rodrigo Vivi
This patch series changes DMC loading to be backwards-compatible by
removing version checking and preparing the driver to load from blobs
unversioned filenames.
Next DMC releases should start using using unversioned filenames and the
code should be changed to use them when that happens. It's likely that
old platforms will not have new releases, so we keep on using the
versioned paths for a while.
This version differs from v2 in that we are keeping the current paths
and will update them as new DMC releases are issued. In the future we
could move everybody to use the new convention after making sure that
required changes in linux-firmware are well spread. After that, the
fallback mechanism introduced by this series could be removed.
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
Gustavo Sousa (2):
drm/i915/dmc: Do not require specific versions
drm/i915/dmc: Prepare to use unversioned paths
drivers/gpu/drm/i915/display/intel_dmc.c | 97 +++++++++++-------------
drivers/gpu/drm/i915/display/intel_dmc.h | 1 -
2 files changed, 46 insertions(+), 52 deletions(-)
--
2.39.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Intel-gfx] [PATCH v3 1/2] drm/i915/dmc: Do not require specific versions
2022-12-29 19:07 [Intel-gfx] [PATCH v3 0/2] drm/i915/dmc: Make firmware loading backwards-compatible Gustavo Sousa
@ 2022-12-29 19:07 ` Gustavo Sousa
2022-12-30 8:32 ` Rodrigo Vivi
2022-12-29 19:07 ` [Intel-gfx] [PATCH v3 2/2] drm/i915/dmc: Prepare to use unversioned paths Gustavo Sousa
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Gustavo Sousa @ 2022-12-29 19:07 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula, Lucas De Marchi, Rodrigo Vivi
Currently there is no DMC version requirement with respect to how i915
interacts with it and new versions of the firmware serve as drop-in
replacements of older ones. As such, do not require specific versions.
References: https://lore.kernel.org/r/Y3081s7c5ksutpMW@intel.com
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
drivers/gpu/drm/i915/display/intel_dmc.c | 35 ------------------------
drivers/gpu/drm/i915/display/intel_dmc.h | 1 -
2 files changed, 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index 905b5dcdca14..4124b3d37110 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -53,51 +53,40 @@
#define DISPLAY_VER12_DMC_MAX_FW_SIZE ICL_DMC_MAX_FW_SIZE
#define DG2_DMC_PATH DMC_PATH(dg2, 2, 08)
-#define DG2_DMC_VERSION_REQUIRED DMC_VERSION(2, 8)
MODULE_FIRMWARE(DG2_DMC_PATH);
#define ADLP_DMC_PATH DMC_PATH(adlp, 2, 16)
-#define ADLP_DMC_VERSION_REQUIRED DMC_VERSION(2, 16)
MODULE_FIRMWARE(ADLP_DMC_PATH);
#define ADLS_DMC_PATH DMC_PATH(adls, 2, 01)
-#define ADLS_DMC_VERSION_REQUIRED DMC_VERSION(2, 1)
MODULE_FIRMWARE(ADLS_DMC_PATH);
#define DG1_DMC_PATH DMC_PATH(dg1, 2, 02)
-#define DG1_DMC_VERSION_REQUIRED DMC_VERSION(2, 2)
MODULE_FIRMWARE(DG1_DMC_PATH);
#define RKL_DMC_PATH DMC_PATH(rkl, 2, 03)
-#define RKL_DMC_VERSION_REQUIRED DMC_VERSION(2, 3)
MODULE_FIRMWARE(RKL_DMC_PATH);
#define TGL_DMC_PATH DMC_PATH(tgl, 2, 12)
-#define TGL_DMC_VERSION_REQUIRED DMC_VERSION(2, 12)
MODULE_FIRMWARE(TGL_DMC_PATH);
#define ICL_DMC_PATH DMC_PATH(icl, 1, 09)
-#define ICL_DMC_VERSION_REQUIRED DMC_VERSION(1, 9)
#define ICL_DMC_MAX_FW_SIZE 0x6000
MODULE_FIRMWARE(ICL_DMC_PATH);
#define GLK_DMC_PATH DMC_PATH(glk, 1, 04)
-#define GLK_DMC_VERSION_REQUIRED DMC_VERSION(1, 4)
#define GLK_DMC_MAX_FW_SIZE 0x4000
MODULE_FIRMWARE(GLK_DMC_PATH);
#define KBL_DMC_PATH DMC_PATH(kbl, 1, 04)
-#define KBL_DMC_VERSION_REQUIRED DMC_VERSION(1, 4)
#define KBL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
MODULE_FIRMWARE(KBL_DMC_PATH);
#define SKL_DMC_PATH DMC_PATH(skl, 1, 27)
-#define SKL_DMC_VERSION_REQUIRED DMC_VERSION(1, 27)
#define SKL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
MODULE_FIRMWARE(SKL_DMC_PATH);
#define BXT_DMC_PATH DMC_PATH(bxt, 1, 07)
-#define BXT_DMC_VERSION_REQUIRED DMC_VERSION(1, 7)
#define BXT_DMC_MAX_FW_SIZE 0x3000
MODULE_FIRMWARE(BXT_DMC_PATH);
@@ -765,17 +754,6 @@ static u32 parse_dmc_fw_css(struct intel_dmc *dmc,
return 0;
}
- if (dmc->required_version &&
- css_header->version != dmc->required_version) {
- drm_info(&i915->drm, "Refusing to load DMC firmware v%u.%u,"
- " please use v%u.%u\n",
- DMC_VERSION_MAJOR(css_header->version),
- DMC_VERSION_MINOR(css_header->version),
- DMC_VERSION_MAJOR(dmc->required_version),
- DMC_VERSION_MINOR(dmc->required_version));
- return 0;
- }
-
dmc->version = css_header->version;
return sizeof(struct intel_css_header);
@@ -903,49 +881,38 @@ void intel_dmc_ucode_init(struct drm_i915_private *dev_priv)
if (IS_DG2(dev_priv)) {
dmc->fw_path = DG2_DMC_PATH;
- dmc->required_version = DG2_DMC_VERSION_REQUIRED;
dmc->max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
} else if (IS_ALDERLAKE_P(dev_priv)) {
dmc->fw_path = ADLP_DMC_PATH;
- dmc->required_version = ADLP_DMC_VERSION_REQUIRED;
dmc->max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
} else if (IS_ALDERLAKE_S(dev_priv)) {
dmc->fw_path = ADLS_DMC_PATH;
- dmc->required_version = ADLS_DMC_VERSION_REQUIRED;
dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
} else if (IS_DG1(dev_priv)) {
dmc->fw_path = DG1_DMC_PATH;
- dmc->required_version = DG1_DMC_VERSION_REQUIRED;
dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
} else if (IS_ROCKETLAKE(dev_priv)) {
dmc->fw_path = RKL_DMC_PATH;
- dmc->required_version = RKL_DMC_VERSION_REQUIRED;
dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
} else if (IS_TIGERLAKE(dev_priv)) {
dmc->fw_path = TGL_DMC_PATH;
- dmc->required_version = TGL_DMC_VERSION_REQUIRED;
dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
} else if (DISPLAY_VER(dev_priv) == 11) {
dmc->fw_path = ICL_DMC_PATH;
- dmc->required_version = ICL_DMC_VERSION_REQUIRED;
dmc->max_fw_size = ICL_DMC_MAX_FW_SIZE;
} else if (IS_GEMINILAKE(dev_priv)) {
dmc->fw_path = GLK_DMC_PATH;
- dmc->required_version = GLK_DMC_VERSION_REQUIRED;
dmc->max_fw_size = GLK_DMC_MAX_FW_SIZE;
} else if (IS_KABYLAKE(dev_priv) ||
IS_COFFEELAKE(dev_priv) ||
IS_COMETLAKE(dev_priv)) {
dmc->fw_path = KBL_DMC_PATH;
- dmc->required_version = KBL_DMC_VERSION_REQUIRED;
dmc->max_fw_size = KBL_DMC_MAX_FW_SIZE;
} else if (IS_SKYLAKE(dev_priv)) {
dmc->fw_path = SKL_DMC_PATH;
- dmc->required_version = SKL_DMC_VERSION_REQUIRED;
dmc->max_fw_size = SKL_DMC_MAX_FW_SIZE;
} else if (IS_BROXTON(dev_priv)) {
dmc->fw_path = BXT_DMC_PATH;
- dmc->required_version = BXT_DMC_VERSION_REQUIRED;
dmc->max_fw_size = BXT_DMC_MAX_FW_SIZE;
}
@@ -958,8 +925,6 @@ void intel_dmc_ucode_init(struct drm_i915_private *dev_priv)
}
dmc->fw_path = dev_priv->params.dmc_firmware_path;
- /* Bypass version check for firmware override. */
- dmc->required_version = 0;
}
if (!dmc->fw_path) {
diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
index 67e03315ef99..435eab9b016b 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.h
+++ b/drivers/gpu/drm/i915/display/intel_dmc.h
@@ -25,7 +25,6 @@ enum {
struct intel_dmc {
struct work_struct work;
const char *fw_path;
- u32 required_version;
u32 max_fw_size; /* bytes */
u32 version;
struct dmc_fw_info {
--
2.39.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Intel-gfx] [PATCH v3 2/2] drm/i915/dmc: Prepare to use unversioned paths
2022-12-29 19:07 [Intel-gfx] [PATCH v3 0/2] drm/i915/dmc: Make firmware loading backwards-compatible Gustavo Sousa
2022-12-29 19:07 ` [Intel-gfx] [PATCH v3 1/2] drm/i915/dmc: Do not require specific versions Gustavo Sousa
@ 2022-12-29 19:07 ` Gustavo Sousa
2022-12-30 8:47 ` Rodrigo Vivi
2022-12-29 19:24 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/dmc: Make firmware loading backwards-compatible (rev3) Patchwork
2022-12-29 19:51 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
3 siblings, 1 reply; 12+ messages in thread
From: Gustavo Sousa @ 2022-12-29 19:07 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula, Lucas De Marchi, Rodrigo Vivi
New DMC releases in linux-firmware will stop using version number in
blob filenames. This new convention provides the following benefits:
1. It simplifies code maintenance, as new DMC releases for a platform
using the new convention will always use the same filename for the
blob.
2. It allows DMC to be loaded even if the target system does not have
the most recent firmware installed.
Prepare the driver by:
- Using the new convention for DMC_PATH() and renaming the currently
used one to make it clear it is for the legacy scheme.
- Implementing a fallback mechanism for future transitions from
versioned to unversioned paths so that we do not cause a regression
for systems not having the most up-to-date linux-firmware files.
v2:
- Keep using request_firmware() instead of firmware_request_nowarn().
(Jani)
v3:
- Keep current DMC paths instead of directly using unversioned ones,
so that we do not disturb initrd generation.
(Lucas, Rodrigo)
References: https://lore.kernel.org/r/Y3081s7c5ksutpMW@intel.com
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
drivers/gpu/drm/i915/display/intel_dmc.c | 62 ++++++++++++++++++------
1 file changed, 46 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index 4124b3d37110..12f05b2d33a3 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -42,51 +42,59 @@
#define DMC_VERSION_MAJOR(version) ((version) >> 16)
#define DMC_VERSION_MINOR(version) ((version) & 0xffff)
-#define DMC_PATH(platform, major, minor) \
- "i915/" \
- __stringify(platform) "_dmc_ver" \
- __stringify(major) "_" \
+#define DMC_PATH(platform) \
+ "i915/" __stringify(platform) "_dmc.bin"
+
+/*
+ * New DMC additions should not use this. This is used solely to remain
+ * compatible with systems that have not yet updated DMC blobs to use
+ * unversioned file names.
+ */
+#define DMC_LEGACY_PATH(platform, major, minor) \
+ "i915/" \
+ __stringify(platform) "_dmc_ver" \
+ __stringify(major) "_" \
__stringify(minor) ".bin"
#define DISPLAY_VER13_DMC_MAX_FW_SIZE 0x20000
#define DISPLAY_VER12_DMC_MAX_FW_SIZE ICL_DMC_MAX_FW_SIZE
-#define DG2_DMC_PATH DMC_PATH(dg2, 2, 08)
+#define DG2_DMC_PATH DMC_LEGACY_PATH(dg2, 2, 08)
MODULE_FIRMWARE(DG2_DMC_PATH);
-#define ADLP_DMC_PATH DMC_PATH(adlp, 2, 16)
+#define ADLP_DMC_PATH DMC_LEGACY_PATH(adlp, 2, 16)
MODULE_FIRMWARE(ADLP_DMC_PATH);
-#define ADLS_DMC_PATH DMC_PATH(adls, 2, 01)
+#define ADLS_DMC_PATH DMC_LEGACY_PATH(adls, 2, 01)
MODULE_FIRMWARE(ADLS_DMC_PATH);
-#define DG1_DMC_PATH DMC_PATH(dg1, 2, 02)
+#define DG1_DMC_PATH DMC_LEGACY_PATH(dg1, 2, 02)
MODULE_FIRMWARE(DG1_DMC_PATH);
-#define RKL_DMC_PATH DMC_PATH(rkl, 2, 03)
+#define RKL_DMC_PATH DMC_LEGACY_PATH(rkl, 2, 03)
MODULE_FIRMWARE(RKL_DMC_PATH);
-#define TGL_DMC_PATH DMC_PATH(tgl, 2, 12)
+#define TGL_DMC_PATH DMC_LEGACY_PATH(tgl, 2, 12)
MODULE_FIRMWARE(TGL_DMC_PATH);
-#define ICL_DMC_PATH DMC_PATH(icl, 1, 09)
+#define ICL_DMC_PATH DMC_LEGACY_PATH(icl, 1, 09)
#define ICL_DMC_MAX_FW_SIZE 0x6000
MODULE_FIRMWARE(ICL_DMC_PATH);
-#define GLK_DMC_PATH DMC_PATH(glk, 1, 04)
+#define GLK_DMC_PATH DMC_LEGACY_PATH(glk, 1, 04)
#define GLK_DMC_MAX_FW_SIZE 0x4000
MODULE_FIRMWARE(GLK_DMC_PATH);
-#define KBL_DMC_PATH DMC_PATH(kbl, 1, 04)
+#define KBL_DMC_PATH DMC_LEGACY_PATH(kbl, 1, 04)
#define KBL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
MODULE_FIRMWARE(KBL_DMC_PATH);
-#define SKL_DMC_PATH DMC_PATH(skl, 1, 27)
+#define SKL_DMC_PATH DMC_LEGACY_PATH(skl, 1, 27)
#define SKL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
MODULE_FIRMWARE(SKL_DMC_PATH);
-#define BXT_DMC_PATH DMC_PATH(bxt, 1, 07)
+#define BXT_DMC_PATH DMC_LEGACY_PATH(bxt, 1, 07)
#define BXT_DMC_MAX_FW_SIZE 0x3000
MODULE_FIRMWARE(BXT_DMC_PATH);
@@ -821,16 +829,38 @@ static void intel_dmc_runtime_pm_put(struct drm_i915_private *dev_priv)
intel_display_power_put(dev_priv, POWER_DOMAIN_INIT, wakeref);
}
+static const char *dmc_fallback_path(struct drm_i915_private *i915)
+{
+ /* No fallback paths for now. */
+ return NULL;
+}
+
static void dmc_load_work_fn(struct work_struct *work)
{
struct drm_i915_private *dev_priv;
struct intel_dmc *dmc;
const struct firmware *fw = NULL;
+ const char *fallback_path;
+ int err;
dev_priv = container_of(work, typeof(*dev_priv), display.dmc.work);
dmc = &dev_priv->display.dmc;
- request_firmware(&fw, dev_priv->display.dmc.fw_path, dev_priv->drm.dev);
+ err = request_firmware(&fw, dev_priv->display.dmc.fw_path, dev_priv->drm.dev);
+
+ if (err == -ENOENT && !dev_priv->params.dmc_firmware_path) {
+ fallback_path = dmc_fallback_path(dev_priv);
+ if (fallback_path) {
+ drm_dbg_kms(&dev_priv->drm,
+ "%s not found, falling back to %s\n",
+ dmc->fw_path,
+ fallback_path);
+ err = request_firmware(&fw, fallback_path, dev_priv->drm.dev);
+ if (err == 0)
+ dev_priv->display.dmc.fw_path = fallback_path;
+ }
+ }
+
parse_dmc_fw(dev_priv, fw);
if (intel_dmc_has_payload(dev_priv)) {
--
2.39.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/dmc: Make firmware loading backwards-compatible (rev3)
2022-12-29 19:07 [Intel-gfx] [PATCH v3 0/2] drm/i915/dmc: Make firmware loading backwards-compatible Gustavo Sousa
2022-12-29 19:07 ` [Intel-gfx] [PATCH v3 1/2] drm/i915/dmc: Do not require specific versions Gustavo Sousa
2022-12-29 19:07 ` [Intel-gfx] [PATCH v3 2/2] drm/i915/dmc: Prepare to use unversioned paths Gustavo Sousa
@ 2022-12-29 19:24 ` Patchwork
2022-12-29 19:51 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
3 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2022-12-29 19:24 UTC (permalink / raw)
To: Gustavo Sousa; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/dmc: Make firmware loading backwards-compatible (rev3)
URL : https://patchwork.freedesktop.org/series/112116/
State : warning
== Summary ==
Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/dmc: Make firmware loading backwards-compatible (rev3)
2022-12-29 19:07 [Intel-gfx] [PATCH v3 0/2] drm/i915/dmc: Make firmware loading backwards-compatible Gustavo Sousa
` (2 preceding siblings ...)
2022-12-29 19:24 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/dmc: Make firmware loading backwards-compatible (rev3) Patchwork
@ 2022-12-29 19:51 ` Patchwork
3 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2022-12-29 19:51 UTC (permalink / raw)
To: Gustavo Sousa; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 9727 bytes --]
== Series Details ==
Series: drm/i915/dmc: Make firmware loading backwards-compatible (rev3)
URL : https://patchwork.freedesktop.org/series/112116/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_12528 -> Patchwork_112116v3
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_112116v3 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_112116v3, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/index.html
Participating hosts (40 -> 43)
------------------------------
Additional (3): fi-bsw-kefka bat-dg2-9 bat-atsm-1
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_112116v3:
### IGT changes ###
#### Possible regressions ####
* igt@gem_exec_suspend@basic-s0@smem:
- fi-bsw-kefka: NOTRUN -> [INCOMPLETE][1]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/fi-bsw-kefka/igt@gem_exec_suspend@basic-s0@smem.html
#### Suppressed ####
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@i915_module_load@load:
- {bat-dg2-8}: [PASS][2] -> [FAIL][3]
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12528/bat-dg2-8/igt@i915_module_load@load.html
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/bat-dg2-8/igt@i915_module_load@load.html
* igt@i915_suspend@basic-s3-without-i915:
- {bat-adln-1}: [PASS][4] -> [INCOMPLETE][5]
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12528/bat-adln-1/igt@i915_suspend@basic-s3-without-i915.html
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/bat-adln-1/igt@i915_suspend@basic-s3-without-i915.html
Known issues
------------
Here are the changes found in Patchwork_112116v3 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_lmem_swapping@parallel-random-engines:
- bat-adlp-4: NOTRUN -> [SKIP][6] ([i915#4613]) +3 similar issues
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/bat-adlp-4/igt@gem_lmem_swapping@parallel-random-engines.html
* igt@i915_pm_rps@basic-api:
- bat-adlp-4: NOTRUN -> [SKIP][7] ([i915#6621])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/bat-adlp-4/igt@i915_pm_rps@basic-api.html
* igt@kms_chamelium@common-hpd-after-suspend:
- bat-adlp-4: NOTRUN -> [SKIP][8] ([fdo#111827])
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/bat-adlp-4/igt@kms_chamelium@common-hpd-after-suspend.html
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-bsw-kefka: NOTRUN -> [SKIP][9] ([fdo#109271] / [fdo#111827]) +7 similar issues
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/fi-bsw-kefka/igt@kms_chamelium@hdmi-hpd-fast.html
* igt@kms_setmode@basic-clone-single-crtc:
- fi-snb-2600: NOTRUN -> [SKIP][10] ([fdo#109271])
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/fi-snb-2600/igt@kms_setmode@basic-clone-single-crtc.html
* igt@prime_vgem@basic-fence-flip:
- fi-bsw-kefka: NOTRUN -> [SKIP][11] ([fdo#109271]) +17 similar issues
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/fi-bsw-kefka/igt@prime_vgem@basic-fence-flip.html
* igt@prime_vgem@basic-userptr:
- bat-adlp-4: NOTRUN -> [SKIP][12] ([fdo#109295] / [i915#3301] / [i915#3708])
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/bat-adlp-4/igt@prime_vgem@basic-userptr.html
* igt@prime_vgem@basic-write:
- bat-adlp-4: NOTRUN -> [SKIP][13] ([fdo#109295] / [i915#3291] / [i915#3708]) +2 similar issues
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/bat-adlp-4/igt@prime_vgem@basic-write.html
#### Possible fixes ####
* igt@i915_pm_rpm@basic-rte:
- bat-adlp-4: [DMESG-WARN][14] ([i915#7077]) -> [PASS][15]
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12528/bat-adlp-4/igt@i915_pm_rpm@basic-rte.html
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/bat-adlp-4/igt@i915_pm_rpm@basic-rte.html
* igt@i915_selftest@live@gt_heartbeat:
- fi-cfl-8109u: [DMESG-FAIL][16] ([i915#5334]) -> [PASS][17]
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12528/fi-cfl-8109u/igt@i915_selftest@live@gt_heartbeat.html
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/fi-cfl-8109u/igt@i915_selftest@live@gt_heartbeat.html
* igt@i915_selftest@live@reset:
- {bat-rpls-2}: [DMESG-FAIL][18] ([i915#4983]) -> [PASS][19]
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12528/bat-rpls-2/igt@i915_selftest@live@reset.html
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/bat-rpls-2/igt@i915_selftest@live@reset.html
* igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-2:
- {bat-dg2-11}: [FAIL][20] ([i915#7336]) -> [PASS][21]
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12528/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-2.html
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-2.html
* igt@kms_pipe_crc_basic@suspend-read-crc@pipe-c-dp-1:
- {bat-adlp-9}: [DMESG-WARN][22] ([i915#2867]) -> [PASS][23]
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12528/bat-adlp-9/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-c-dp-1.html
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/bat-adlp-9/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-c-dp-1.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
[fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
[i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
[i915#1836]: https://gitlab.freedesktop.org/drm/intel/issues/1836
[i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
[i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
[i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
[i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
[i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
[i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
[i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
[i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
[i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
[i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
[i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
[i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
[i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
[i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
[i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
[i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
[i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
[i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
[i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
[i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
[i915#6077]: https://gitlab.freedesktop.org/drm/intel/issues/6077
[i915#6078]: https://gitlab.freedesktop.org/drm/intel/issues/6078
[i915#6093]: https://gitlab.freedesktop.org/drm/intel/issues/6093
[i915#6094]: https://gitlab.freedesktop.org/drm/intel/issues/6094
[i915#6166]: https://gitlab.freedesktop.org/drm/intel/issues/6166
[i915#6311]: https://gitlab.freedesktop.org/drm/intel/issues/6311
[i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
[i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
[i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645
[i915#7077]: https://gitlab.freedesktop.org/drm/intel/issues/7077
[i915#7336]: https://gitlab.freedesktop.org/drm/intel/issues/7336
[i915#7357]: https://gitlab.freedesktop.org/drm/intel/issues/7357
[i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
[i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
Build changes
-------------
* Linux: CI_DRM_12528 -> Patchwork_112116v3
CI-20190529: 20190529
CI_DRM_12528: 7e9f060b6f2ad746710306da06ba9c4a53876357 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7104: fe5def13049225967770eaaf19ec01ef80e2adc5 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_112116v3: 7e9f060b6f2ad746710306da06ba9c4a53876357 @ git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
99e635b61c40 drm/i915/dmc: Prepare to use unversioned paths
41f0cba27e13 drm/i915/dmc: Do not require specific versions
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112116v3/index.html
[-- Attachment #2: Type: text/html, Size: 8995 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/dmc: Do not require specific versions
2022-12-29 19:07 ` [Intel-gfx] [PATCH v3 1/2] drm/i915/dmc: Do not require specific versions Gustavo Sousa
@ 2022-12-30 8:32 ` Rodrigo Vivi
2022-12-30 12:42 ` Gustavo Sousa
0 siblings, 1 reply; 12+ messages in thread
From: Rodrigo Vivi @ 2022-12-30 8:32 UTC (permalink / raw)
To: Gustavo Sousa; +Cc: Jani Nikula, intel-gfx, Lucas De Marchi
On Thu, Dec 29, 2022 at 04:07:39PM -0300, Gustavo Sousa wrote:
> Currently there is no DMC version requirement with respect to how i915
> interacts with it and new versions of the firmware serve as drop-in
> replacements of older ones. As such, do not require specific versions.
>
> References: https://lore.kernel.org/r/Y3081s7c5ksutpMW@intel.com
I don't believe this link is a good reference as justification
of this patch.
Probably https://docs.kernel.org/next/driver-api/firmware/firmware-usage-guidelines.html
is a better link?
Also, in the commit message we should be more clear that i915
interacts with the Hardware and not with any FW ABI/API, so the API
is fixed within the platform, hence no need to get this so-tied
version requirement.
with the commit msg changed you can count on my reviewed-by,
the patch looks good to me.
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_dmc.c | 35 ------------------------
> drivers/gpu/drm/i915/display/intel_dmc.h | 1 -
> 2 files changed, 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
> index 905b5dcdca14..4124b3d37110 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> @@ -53,51 +53,40 @@
> #define DISPLAY_VER12_DMC_MAX_FW_SIZE ICL_DMC_MAX_FW_SIZE
>
> #define DG2_DMC_PATH DMC_PATH(dg2, 2, 08)
> -#define DG2_DMC_VERSION_REQUIRED DMC_VERSION(2, 8)
> MODULE_FIRMWARE(DG2_DMC_PATH);
>
> #define ADLP_DMC_PATH DMC_PATH(adlp, 2, 16)
> -#define ADLP_DMC_VERSION_REQUIRED DMC_VERSION(2, 16)
> MODULE_FIRMWARE(ADLP_DMC_PATH);
>
> #define ADLS_DMC_PATH DMC_PATH(adls, 2, 01)
> -#define ADLS_DMC_VERSION_REQUIRED DMC_VERSION(2, 1)
> MODULE_FIRMWARE(ADLS_DMC_PATH);
>
> #define DG1_DMC_PATH DMC_PATH(dg1, 2, 02)
> -#define DG1_DMC_VERSION_REQUIRED DMC_VERSION(2, 2)
> MODULE_FIRMWARE(DG1_DMC_PATH);
>
> #define RKL_DMC_PATH DMC_PATH(rkl, 2, 03)
> -#define RKL_DMC_VERSION_REQUIRED DMC_VERSION(2, 3)
> MODULE_FIRMWARE(RKL_DMC_PATH);
>
> #define TGL_DMC_PATH DMC_PATH(tgl, 2, 12)
> -#define TGL_DMC_VERSION_REQUIRED DMC_VERSION(2, 12)
> MODULE_FIRMWARE(TGL_DMC_PATH);
>
> #define ICL_DMC_PATH DMC_PATH(icl, 1, 09)
> -#define ICL_DMC_VERSION_REQUIRED DMC_VERSION(1, 9)
> #define ICL_DMC_MAX_FW_SIZE 0x6000
> MODULE_FIRMWARE(ICL_DMC_PATH);
>
> #define GLK_DMC_PATH DMC_PATH(glk, 1, 04)
> -#define GLK_DMC_VERSION_REQUIRED DMC_VERSION(1, 4)
> #define GLK_DMC_MAX_FW_SIZE 0x4000
> MODULE_FIRMWARE(GLK_DMC_PATH);
>
> #define KBL_DMC_PATH DMC_PATH(kbl, 1, 04)
> -#define KBL_DMC_VERSION_REQUIRED DMC_VERSION(1, 4)
> #define KBL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
> MODULE_FIRMWARE(KBL_DMC_PATH);
>
> #define SKL_DMC_PATH DMC_PATH(skl, 1, 27)
> -#define SKL_DMC_VERSION_REQUIRED DMC_VERSION(1, 27)
> #define SKL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
> MODULE_FIRMWARE(SKL_DMC_PATH);
>
> #define BXT_DMC_PATH DMC_PATH(bxt, 1, 07)
> -#define BXT_DMC_VERSION_REQUIRED DMC_VERSION(1, 7)
> #define BXT_DMC_MAX_FW_SIZE 0x3000
> MODULE_FIRMWARE(BXT_DMC_PATH);
>
> @@ -765,17 +754,6 @@ static u32 parse_dmc_fw_css(struct intel_dmc *dmc,
> return 0;
> }
>
> - if (dmc->required_version &&
> - css_header->version != dmc->required_version) {
> - drm_info(&i915->drm, "Refusing to load DMC firmware v%u.%u,"
> - " please use v%u.%u\n",
> - DMC_VERSION_MAJOR(css_header->version),
> - DMC_VERSION_MINOR(css_header->version),
> - DMC_VERSION_MAJOR(dmc->required_version),
> - DMC_VERSION_MINOR(dmc->required_version));
> - return 0;
> - }
> -
> dmc->version = css_header->version;
>
> return sizeof(struct intel_css_header);
> @@ -903,49 +881,38 @@ void intel_dmc_ucode_init(struct drm_i915_private *dev_priv)
>
> if (IS_DG2(dev_priv)) {
> dmc->fw_path = DG2_DMC_PATH;
> - dmc->required_version = DG2_DMC_VERSION_REQUIRED;
> dmc->max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
> } else if (IS_ALDERLAKE_P(dev_priv)) {
> dmc->fw_path = ADLP_DMC_PATH;
> - dmc->required_version = ADLP_DMC_VERSION_REQUIRED;
> dmc->max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
> } else if (IS_ALDERLAKE_S(dev_priv)) {
> dmc->fw_path = ADLS_DMC_PATH;
> - dmc->required_version = ADLS_DMC_VERSION_REQUIRED;
> dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> } else if (IS_DG1(dev_priv)) {
> dmc->fw_path = DG1_DMC_PATH;
> - dmc->required_version = DG1_DMC_VERSION_REQUIRED;
> dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> } else if (IS_ROCKETLAKE(dev_priv)) {
> dmc->fw_path = RKL_DMC_PATH;
> - dmc->required_version = RKL_DMC_VERSION_REQUIRED;
> dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> } else if (IS_TIGERLAKE(dev_priv)) {
> dmc->fw_path = TGL_DMC_PATH;
> - dmc->required_version = TGL_DMC_VERSION_REQUIRED;
> dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> } else if (DISPLAY_VER(dev_priv) == 11) {
> dmc->fw_path = ICL_DMC_PATH;
> - dmc->required_version = ICL_DMC_VERSION_REQUIRED;
> dmc->max_fw_size = ICL_DMC_MAX_FW_SIZE;
> } else if (IS_GEMINILAKE(dev_priv)) {
> dmc->fw_path = GLK_DMC_PATH;
> - dmc->required_version = GLK_DMC_VERSION_REQUIRED;
> dmc->max_fw_size = GLK_DMC_MAX_FW_SIZE;
> } else if (IS_KABYLAKE(dev_priv) ||
> IS_COFFEELAKE(dev_priv) ||
> IS_COMETLAKE(dev_priv)) {
> dmc->fw_path = KBL_DMC_PATH;
> - dmc->required_version = KBL_DMC_VERSION_REQUIRED;
> dmc->max_fw_size = KBL_DMC_MAX_FW_SIZE;
> } else if (IS_SKYLAKE(dev_priv)) {
> dmc->fw_path = SKL_DMC_PATH;
> - dmc->required_version = SKL_DMC_VERSION_REQUIRED;
> dmc->max_fw_size = SKL_DMC_MAX_FW_SIZE;
> } else if (IS_BROXTON(dev_priv)) {
> dmc->fw_path = BXT_DMC_PATH;
> - dmc->required_version = BXT_DMC_VERSION_REQUIRED;
> dmc->max_fw_size = BXT_DMC_MAX_FW_SIZE;
> }
>
> @@ -958,8 +925,6 @@ void intel_dmc_ucode_init(struct drm_i915_private *dev_priv)
> }
>
> dmc->fw_path = dev_priv->params.dmc_firmware_path;
> - /* Bypass version check for firmware override. */
> - dmc->required_version = 0;
> }
>
> if (!dmc->fw_path) {
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
> index 67e03315ef99..435eab9b016b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc.h
> +++ b/drivers/gpu/drm/i915/display/intel_dmc.h
> @@ -25,7 +25,6 @@ enum {
> struct intel_dmc {
> struct work_struct work;
> const char *fw_path;
> - u32 required_version;
> u32 max_fw_size; /* bytes */
> u32 version;
> struct dmc_fw_info {
> --
> 2.39.0
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH v3 2/2] drm/i915/dmc: Prepare to use unversioned paths
2022-12-29 19:07 ` [Intel-gfx] [PATCH v3 2/2] drm/i915/dmc: Prepare to use unversioned paths Gustavo Sousa
@ 2022-12-30 8:47 ` Rodrigo Vivi
2022-12-30 13:36 ` Gustavo Sousa
0 siblings, 1 reply; 12+ messages in thread
From: Rodrigo Vivi @ 2022-12-30 8:47 UTC (permalink / raw)
To: Gustavo Sousa; +Cc: Jani Nikula, intel-gfx, Lucas De Marchi
On Thu, Dec 29, 2022 at 04:07:40PM -0300, Gustavo Sousa wrote:
> New DMC releases in linux-firmware will stop using version number in
> blob filenames. This new convention provides the following benefits:
>
> 1. It simplifies code maintenance, as new DMC releases for a platform
> using the new convention will always use the same filename for the
> blob.
>
> 2. It allows DMC to be loaded even if the target system does not have
> the most recent firmware installed.
>
> Prepare the driver by:
>
> - Using the new convention for DMC_PATH() and renaming the currently
> used one to make it clear it is for the legacy scheme.
>
> - Implementing a fallback mechanism for future transitions from
> versioned to unversioned paths so that we do not cause a regression
> for systems not having the most up-to-date linux-firmware files.
>
> v2:
> - Keep using request_firmware() instead of firmware_request_nowarn().
> (Jani)
> v3:
> - Keep current DMC paths instead of directly using unversioned ones,
> so that we do not disturb initrd generation.
> (Lucas, Rodrigo)
>
> References: https://lore.kernel.org/r/Y3081s7c5ksutpMW@intel.com
I also don't believe this link is a good reference here...
Regarding the patch, I liked the approach in general.
The patch is really neat, but I believe we will need to split it:
1. Add the new DMC_PATH and DMC_LEGACY_PATH only with the introduction
of the mtl_dmc.bin
2. And the fallback function, add only if/when we get a real fallback.
Oh, and I just realized that when doing the new _dmc.bin path we also
need to make sure that we read the fw_version from the header and
print as a drm_info msg somewhere.
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_dmc.c | 62 ++++++++++++++++++------
> 1 file changed, 46 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
> index 4124b3d37110..12f05b2d33a3 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> @@ -42,51 +42,59 @@
> #define DMC_VERSION_MAJOR(version) ((version) >> 16)
> #define DMC_VERSION_MINOR(version) ((version) & 0xffff)
>
> -#define DMC_PATH(platform, major, minor) \
> - "i915/" \
> - __stringify(platform) "_dmc_ver" \
> - __stringify(major) "_" \
> +#define DMC_PATH(platform) \
> + "i915/" __stringify(platform) "_dmc.bin"
> +
> +/*
> + * New DMC additions should not use this. This is used solely to remain
> + * compatible with systems that have not yet updated DMC blobs to use
> + * unversioned file names.
> + */
> +#define DMC_LEGACY_PATH(platform, major, minor) \
> + "i915/" \
> + __stringify(platform) "_dmc_ver" \
> + __stringify(major) "_" \
> __stringify(minor) ".bin"
>
> #define DISPLAY_VER13_DMC_MAX_FW_SIZE 0x20000
>
> #define DISPLAY_VER12_DMC_MAX_FW_SIZE ICL_DMC_MAX_FW_SIZE
>
> -#define DG2_DMC_PATH DMC_PATH(dg2, 2, 08)
> +#define DG2_DMC_PATH DMC_LEGACY_PATH(dg2, 2, 08)
> MODULE_FIRMWARE(DG2_DMC_PATH);
>
> -#define ADLP_DMC_PATH DMC_PATH(adlp, 2, 16)
> +#define ADLP_DMC_PATH DMC_LEGACY_PATH(adlp, 2, 16)
> MODULE_FIRMWARE(ADLP_DMC_PATH);
>
> -#define ADLS_DMC_PATH DMC_PATH(adls, 2, 01)
> +#define ADLS_DMC_PATH DMC_LEGACY_PATH(adls, 2, 01)
> MODULE_FIRMWARE(ADLS_DMC_PATH);
>
> -#define DG1_DMC_PATH DMC_PATH(dg1, 2, 02)
> +#define DG1_DMC_PATH DMC_LEGACY_PATH(dg1, 2, 02)
> MODULE_FIRMWARE(DG1_DMC_PATH);
>
> -#define RKL_DMC_PATH DMC_PATH(rkl, 2, 03)
> +#define RKL_DMC_PATH DMC_LEGACY_PATH(rkl, 2, 03)
> MODULE_FIRMWARE(RKL_DMC_PATH);
>
> -#define TGL_DMC_PATH DMC_PATH(tgl, 2, 12)
> +#define TGL_DMC_PATH DMC_LEGACY_PATH(tgl, 2, 12)
> MODULE_FIRMWARE(TGL_DMC_PATH);
>
> -#define ICL_DMC_PATH DMC_PATH(icl, 1, 09)
> +#define ICL_DMC_PATH DMC_LEGACY_PATH(icl, 1, 09)
> #define ICL_DMC_MAX_FW_SIZE 0x6000
> MODULE_FIRMWARE(ICL_DMC_PATH);
>
> -#define GLK_DMC_PATH DMC_PATH(glk, 1, 04)
> +#define GLK_DMC_PATH DMC_LEGACY_PATH(glk, 1, 04)
> #define GLK_DMC_MAX_FW_SIZE 0x4000
> MODULE_FIRMWARE(GLK_DMC_PATH);
>
> -#define KBL_DMC_PATH DMC_PATH(kbl, 1, 04)
> +#define KBL_DMC_PATH DMC_LEGACY_PATH(kbl, 1, 04)
> #define KBL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
> MODULE_FIRMWARE(KBL_DMC_PATH);
>
> -#define SKL_DMC_PATH DMC_PATH(skl, 1, 27)
> +#define SKL_DMC_PATH DMC_LEGACY_PATH(skl, 1, 27)
> #define SKL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
> MODULE_FIRMWARE(SKL_DMC_PATH);
>
> -#define BXT_DMC_PATH DMC_PATH(bxt, 1, 07)
> +#define BXT_DMC_PATH DMC_LEGACY_PATH(bxt, 1, 07)
> #define BXT_DMC_MAX_FW_SIZE 0x3000
> MODULE_FIRMWARE(BXT_DMC_PATH);
>
> @@ -821,16 +829,38 @@ static void intel_dmc_runtime_pm_put(struct drm_i915_private *dev_priv)
> intel_display_power_put(dev_priv, POWER_DOMAIN_INIT, wakeref);
> }
>
> +static const char *dmc_fallback_path(struct drm_i915_private *i915)
> +{
> + /* No fallback paths for now. */
> + return NULL;
> +}
> +
> static void dmc_load_work_fn(struct work_struct *work)
> {
> struct drm_i915_private *dev_priv;
> struct intel_dmc *dmc;
> const struct firmware *fw = NULL;
> + const char *fallback_path;
> + int err;
>
> dev_priv = container_of(work, typeof(*dev_priv), display.dmc.work);
> dmc = &dev_priv->display.dmc;
>
> - request_firmware(&fw, dev_priv->display.dmc.fw_path, dev_priv->drm.dev);
> + err = request_firmware(&fw, dev_priv->display.dmc.fw_path, dev_priv->drm.dev);
> +
> + if (err == -ENOENT && !dev_priv->params.dmc_firmware_path) {
> + fallback_path = dmc_fallback_path(dev_priv);
> + if (fallback_path) {
> + drm_dbg_kms(&dev_priv->drm,
> + "%s not found, falling back to %s\n",
> + dmc->fw_path,
> + fallback_path);
> + err = request_firmware(&fw, fallback_path, dev_priv->drm.dev);
> + if (err == 0)
> + dev_priv->display.dmc.fw_path = fallback_path;
> + }
> + }
> +
> parse_dmc_fw(dev_priv, fw);
>
> if (intel_dmc_has_payload(dev_priv)) {
> --
> 2.39.0
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/dmc: Do not require specific versions
2022-12-30 8:32 ` Rodrigo Vivi
@ 2022-12-30 12:42 ` Gustavo Sousa
2022-12-30 16:52 ` Rodrigo Vivi
0 siblings, 1 reply; 12+ messages in thread
From: Gustavo Sousa @ 2022-12-30 12:42 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: Jani Nikula, intel-gfx, Lucas De Marchi
On Fri, Dec 30, 2022 at 03:32:41AM -0500, Rodrigo Vivi wrote:
> On Thu, Dec 29, 2022 at 04:07:39PM -0300, Gustavo Sousa wrote:
> > Currently there is no DMC version requirement with respect to how i915
> > interacts with it and new versions of the firmware serve as drop-in
> > replacements of older ones. As such, do not require specific versions.
> >
> > References: https://lore.kernel.org/r/Y3081s7c5ksutpMW@intel.com
>
> I don't believe this link is a good reference as justification
> of this patch.
>
> Probably https://docs.kernel.org/next/driver-api/firmware/firmware-usage-guidelines.html
> is a better link?
Yep. I agree this would be better. Is there an "archive" version of this page?
Just to make sure we link to the exact version of that guide at the time of
writing this patch.
>
> Also, in the commit message we should be more clear that i915
> interacts with the Hardware and not with any FW ABI/API, so the API
> is fixed within the platform, hence no need to get this so-tied
> version requirement.
Thanks! I'll grab this paragraph and adapt it into the commit message if you
allow me =)
>
> with the commit msg changed you can count on my reviewed-by,
> the patch looks good to me.
>
> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_dmc.c | 35 ------------------------
> > drivers/gpu/drm/i915/display/intel_dmc.h | 1 -
> > 2 files changed, 36 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
> > index 905b5dcdca14..4124b3d37110 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> > @@ -53,51 +53,40 @@
> > #define DISPLAY_VER12_DMC_MAX_FW_SIZE ICL_DMC_MAX_FW_SIZE
> >
> > #define DG2_DMC_PATH DMC_PATH(dg2, 2, 08)
> > -#define DG2_DMC_VERSION_REQUIRED DMC_VERSION(2, 8)
> > MODULE_FIRMWARE(DG2_DMC_PATH);
> >
> > #define ADLP_DMC_PATH DMC_PATH(adlp, 2, 16)
> > -#define ADLP_DMC_VERSION_REQUIRED DMC_VERSION(2, 16)
> > MODULE_FIRMWARE(ADLP_DMC_PATH);
> >
> > #define ADLS_DMC_PATH DMC_PATH(adls, 2, 01)
> > -#define ADLS_DMC_VERSION_REQUIRED DMC_VERSION(2, 1)
> > MODULE_FIRMWARE(ADLS_DMC_PATH);
> >
> > #define DG1_DMC_PATH DMC_PATH(dg1, 2, 02)
> > -#define DG1_DMC_VERSION_REQUIRED DMC_VERSION(2, 2)
> > MODULE_FIRMWARE(DG1_DMC_PATH);
> >
> > #define RKL_DMC_PATH DMC_PATH(rkl, 2, 03)
> > -#define RKL_DMC_VERSION_REQUIRED DMC_VERSION(2, 3)
> > MODULE_FIRMWARE(RKL_DMC_PATH);
> >
> > #define TGL_DMC_PATH DMC_PATH(tgl, 2, 12)
> > -#define TGL_DMC_VERSION_REQUIRED DMC_VERSION(2, 12)
> > MODULE_FIRMWARE(TGL_DMC_PATH);
> >
> > #define ICL_DMC_PATH DMC_PATH(icl, 1, 09)
> > -#define ICL_DMC_VERSION_REQUIRED DMC_VERSION(1, 9)
> > #define ICL_DMC_MAX_FW_SIZE 0x6000
> > MODULE_FIRMWARE(ICL_DMC_PATH);
> >
> > #define GLK_DMC_PATH DMC_PATH(glk, 1, 04)
> > -#define GLK_DMC_VERSION_REQUIRED DMC_VERSION(1, 4)
> > #define GLK_DMC_MAX_FW_SIZE 0x4000
> > MODULE_FIRMWARE(GLK_DMC_PATH);
> >
> > #define KBL_DMC_PATH DMC_PATH(kbl, 1, 04)
> > -#define KBL_DMC_VERSION_REQUIRED DMC_VERSION(1, 4)
> > #define KBL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
> > MODULE_FIRMWARE(KBL_DMC_PATH);
> >
> > #define SKL_DMC_PATH DMC_PATH(skl, 1, 27)
> > -#define SKL_DMC_VERSION_REQUIRED DMC_VERSION(1, 27)
> > #define SKL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
> > MODULE_FIRMWARE(SKL_DMC_PATH);
> >
> > #define BXT_DMC_PATH DMC_PATH(bxt, 1, 07)
> > -#define BXT_DMC_VERSION_REQUIRED DMC_VERSION(1, 7)
> > #define BXT_DMC_MAX_FW_SIZE 0x3000
> > MODULE_FIRMWARE(BXT_DMC_PATH);
> >
> > @@ -765,17 +754,6 @@ static u32 parse_dmc_fw_css(struct intel_dmc *dmc,
> > return 0;
> > }
> >
> > - if (dmc->required_version &&
> > - css_header->version != dmc->required_version) {
> > - drm_info(&i915->drm, "Refusing to load DMC firmware v%u.%u,"
> > - " please use v%u.%u\n",
> > - DMC_VERSION_MAJOR(css_header->version),
> > - DMC_VERSION_MINOR(css_header->version),
> > - DMC_VERSION_MAJOR(dmc->required_version),
> > - DMC_VERSION_MINOR(dmc->required_version));
> > - return 0;
> > - }
> > -
> > dmc->version = css_header->version;
> >
> > return sizeof(struct intel_css_header);
> > @@ -903,49 +881,38 @@ void intel_dmc_ucode_init(struct drm_i915_private *dev_priv)
> >
> > if (IS_DG2(dev_priv)) {
> > dmc->fw_path = DG2_DMC_PATH;
> > - dmc->required_version = DG2_DMC_VERSION_REQUIRED;
> > dmc->max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
> > } else if (IS_ALDERLAKE_P(dev_priv)) {
> > dmc->fw_path = ADLP_DMC_PATH;
> > - dmc->required_version = ADLP_DMC_VERSION_REQUIRED;
> > dmc->max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
> > } else if (IS_ALDERLAKE_S(dev_priv)) {
> > dmc->fw_path = ADLS_DMC_PATH;
> > - dmc->required_version = ADLS_DMC_VERSION_REQUIRED;
> > dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> > } else if (IS_DG1(dev_priv)) {
> > dmc->fw_path = DG1_DMC_PATH;
> > - dmc->required_version = DG1_DMC_VERSION_REQUIRED;
> > dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> > } else if (IS_ROCKETLAKE(dev_priv)) {
> > dmc->fw_path = RKL_DMC_PATH;
> > - dmc->required_version = RKL_DMC_VERSION_REQUIRED;
> > dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> > } else if (IS_TIGERLAKE(dev_priv)) {
> > dmc->fw_path = TGL_DMC_PATH;
> > - dmc->required_version = TGL_DMC_VERSION_REQUIRED;
> > dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> > } else if (DISPLAY_VER(dev_priv) == 11) {
> > dmc->fw_path = ICL_DMC_PATH;
> > - dmc->required_version = ICL_DMC_VERSION_REQUIRED;
> > dmc->max_fw_size = ICL_DMC_MAX_FW_SIZE;
> > } else if (IS_GEMINILAKE(dev_priv)) {
> > dmc->fw_path = GLK_DMC_PATH;
> > - dmc->required_version = GLK_DMC_VERSION_REQUIRED;
> > dmc->max_fw_size = GLK_DMC_MAX_FW_SIZE;
> > } else if (IS_KABYLAKE(dev_priv) ||
> > IS_COFFEELAKE(dev_priv) ||
> > IS_COMETLAKE(dev_priv)) {
> > dmc->fw_path = KBL_DMC_PATH;
> > - dmc->required_version = KBL_DMC_VERSION_REQUIRED;
> > dmc->max_fw_size = KBL_DMC_MAX_FW_SIZE;
> > } else if (IS_SKYLAKE(dev_priv)) {
> > dmc->fw_path = SKL_DMC_PATH;
> > - dmc->required_version = SKL_DMC_VERSION_REQUIRED;
> > dmc->max_fw_size = SKL_DMC_MAX_FW_SIZE;
> > } else if (IS_BROXTON(dev_priv)) {
> > dmc->fw_path = BXT_DMC_PATH;
> > - dmc->required_version = BXT_DMC_VERSION_REQUIRED;
> > dmc->max_fw_size = BXT_DMC_MAX_FW_SIZE;
> > }
> >
> > @@ -958,8 +925,6 @@ void intel_dmc_ucode_init(struct drm_i915_private *dev_priv)
> > }
> >
> > dmc->fw_path = dev_priv->params.dmc_firmware_path;
> > - /* Bypass version check for firmware override. */
> > - dmc->required_version = 0;
> > }
> >
> > if (!dmc->fw_path) {
> > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
> > index 67e03315ef99..435eab9b016b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dmc.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dmc.h
> > @@ -25,7 +25,6 @@ enum {
> > struct intel_dmc {
> > struct work_struct work;
> > const char *fw_path;
> > - u32 required_version;
> > u32 max_fw_size; /* bytes */
> > u32 version;
> > struct dmc_fw_info {
> > --
> > 2.39.0
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH v3 2/2] drm/i915/dmc: Prepare to use unversioned paths
2022-12-30 8:47 ` Rodrigo Vivi
@ 2022-12-30 13:36 ` Gustavo Sousa
2022-12-30 16:56 ` Rodrigo Vivi
0 siblings, 1 reply; 12+ messages in thread
From: Gustavo Sousa @ 2022-12-30 13:36 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: Jani Nikula, intel-gfx, Lucas De Marchi
On Fri, Dec 30, 2022 at 03:47:43AM -0500, Rodrigo Vivi wrote:
> On Thu, Dec 29, 2022 at 04:07:40PM -0300, Gustavo Sousa wrote:
> > New DMC releases in linux-firmware will stop using version number in
> > blob filenames. This new convention provides the following benefits:
> >
> > 1. It simplifies code maintenance, as new DMC releases for a platform
> > using the new convention will always use the same filename for the
> > blob.
> >
> > 2. It allows DMC to be loaded even if the target system does not have
> > the most recent firmware installed.
> >
> > Prepare the driver by:
> >
> > - Using the new convention for DMC_PATH() and renaming the currently
> > used one to make it clear it is for the legacy scheme.
> >
> > - Implementing a fallback mechanism for future transitions from
> > versioned to unversioned paths so that we do not cause a regression
> > for systems not having the most up-to-date linux-firmware files.
> >
> > v2:
> > - Keep using request_firmware() instead of firmware_request_nowarn().
> > (Jani)
> > v3:
> > - Keep current DMC paths instead of directly using unversioned ones,
> > so that we do not disturb initrd generation.
> > (Lucas, Rodrigo)
> >
> > References: https://lore.kernel.org/r/Y3081s7c5ksutpMW@intel.com
>
> I also don't believe this link is a good reference here...
Noted.
>
> Regarding the patch, I liked the approach in general.
>
> The patch is really neat, but I believe we will need to split it:
>
> 1. Add the new DMC_PATH and DMC_LEGACY_PATH only with the introduction
> of the mtl_dmc.bin
>
> 2. And the fallback function, add only if/when we get a real fallback.
Okay. For future reference, how should that be implemented with respect to the
organization of the patches? I see two ways of doing it and have a personal
preference for the first one:
a) The future series would have first a patch adding the necessary functionality
and a second one using it.
b) The addition of the functionality would be incorporated in the same patch
using it.
For example, for (2), (a) would be a series two patches, the first adding the
fallback mechanism and the second one changing ADLP to use unversioned paths;
and (b) would be all of that in a single patch.
I looks to me that approach (b) has a potential issue. For example, let's say we
in the future we decide to revert that specific firmware update but we already
have other platforms also using the fallback - a clean revert is not possible
there and we would need to make sure that the fallback mechanism is kept.
That's why I like (a) more and I think (b) would be more appropriate for cases
where the functionality and it's user(s) have almost like a "1:1" relationship
(not strictly speaking, read that as "having a somewhat static and restricted
set of users").
>
> Oh, and I just realized that when doing the new _dmc.bin path we also
> need to make sure that we read the fw_version from the header and
> print as a drm_info msg somewhere.
I think the version is already being read by parse_dmc_fw_css() and printed at
the end of dmc_load_work_fn() if the blob was loaded and parsed succesfully.
>
> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_dmc.c | 62 ++++++++++++++++++------
> > 1 file changed, 46 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
> > index 4124b3d37110..12f05b2d33a3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> > @@ -42,51 +42,59 @@
> > #define DMC_VERSION_MAJOR(version) ((version) >> 16)
> > #define DMC_VERSION_MINOR(version) ((version) & 0xffff)
> >
> > -#define DMC_PATH(platform, major, minor) \
> > - "i915/" \
> > - __stringify(platform) "_dmc_ver" \
> > - __stringify(major) "_" \
> > +#define DMC_PATH(platform) \
> > + "i915/" __stringify(platform) "_dmc.bin"
> > +
> > +/*
> > + * New DMC additions should not use this. This is used solely to remain
> > + * compatible with systems that have not yet updated DMC blobs to use
> > + * unversioned file names.
> > + */
> > +#define DMC_LEGACY_PATH(platform, major, minor) \
> > + "i915/" \
> > + __stringify(platform) "_dmc_ver" \
> > + __stringify(major) "_" \
> > __stringify(minor) ".bin"
> >
> > #define DISPLAY_VER13_DMC_MAX_FW_SIZE 0x20000
> >
> > #define DISPLAY_VER12_DMC_MAX_FW_SIZE ICL_DMC_MAX_FW_SIZE
> >
> > -#define DG2_DMC_PATH DMC_PATH(dg2, 2, 08)
> > +#define DG2_DMC_PATH DMC_LEGACY_PATH(dg2, 2, 08)
> > MODULE_FIRMWARE(DG2_DMC_PATH);
> >
> > -#define ADLP_DMC_PATH DMC_PATH(adlp, 2, 16)
> > +#define ADLP_DMC_PATH DMC_LEGACY_PATH(adlp, 2, 16)
> > MODULE_FIRMWARE(ADLP_DMC_PATH);
> >
> > -#define ADLS_DMC_PATH DMC_PATH(adls, 2, 01)
> > +#define ADLS_DMC_PATH DMC_LEGACY_PATH(adls, 2, 01)
> > MODULE_FIRMWARE(ADLS_DMC_PATH);
> >
> > -#define DG1_DMC_PATH DMC_PATH(dg1, 2, 02)
> > +#define DG1_DMC_PATH DMC_LEGACY_PATH(dg1, 2, 02)
> > MODULE_FIRMWARE(DG1_DMC_PATH);
> >
> > -#define RKL_DMC_PATH DMC_PATH(rkl, 2, 03)
> > +#define RKL_DMC_PATH DMC_LEGACY_PATH(rkl, 2, 03)
> > MODULE_FIRMWARE(RKL_DMC_PATH);
> >
> > -#define TGL_DMC_PATH DMC_PATH(tgl, 2, 12)
> > +#define TGL_DMC_PATH DMC_LEGACY_PATH(tgl, 2, 12)
> > MODULE_FIRMWARE(TGL_DMC_PATH);
> >
> > -#define ICL_DMC_PATH DMC_PATH(icl, 1, 09)
> > +#define ICL_DMC_PATH DMC_LEGACY_PATH(icl, 1, 09)
> > #define ICL_DMC_MAX_FW_SIZE 0x6000
> > MODULE_FIRMWARE(ICL_DMC_PATH);
> >
> > -#define GLK_DMC_PATH DMC_PATH(glk, 1, 04)
> > +#define GLK_DMC_PATH DMC_LEGACY_PATH(glk, 1, 04)
> > #define GLK_DMC_MAX_FW_SIZE 0x4000
> > MODULE_FIRMWARE(GLK_DMC_PATH);
> >
> > -#define KBL_DMC_PATH DMC_PATH(kbl, 1, 04)
> > +#define KBL_DMC_PATH DMC_LEGACY_PATH(kbl, 1, 04)
> > #define KBL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
> > MODULE_FIRMWARE(KBL_DMC_PATH);
> >
> > -#define SKL_DMC_PATH DMC_PATH(skl, 1, 27)
> > +#define SKL_DMC_PATH DMC_LEGACY_PATH(skl, 1, 27)
> > #define SKL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
> > MODULE_FIRMWARE(SKL_DMC_PATH);
> >
> > -#define BXT_DMC_PATH DMC_PATH(bxt, 1, 07)
> > +#define BXT_DMC_PATH DMC_LEGACY_PATH(bxt, 1, 07)
> > #define BXT_DMC_MAX_FW_SIZE 0x3000
> > MODULE_FIRMWARE(BXT_DMC_PATH);
> >
> > @@ -821,16 +829,38 @@ static void intel_dmc_runtime_pm_put(struct drm_i915_private *dev_priv)
> > intel_display_power_put(dev_priv, POWER_DOMAIN_INIT, wakeref);
> > }
> >
> > +static const char *dmc_fallback_path(struct drm_i915_private *i915)
> > +{
> > + /* No fallback paths for now. */
> > + return NULL;
> > +}
> > +
> > static void dmc_load_work_fn(struct work_struct *work)
> > {
> > struct drm_i915_private *dev_priv;
> > struct intel_dmc *dmc;
> > const struct firmware *fw = NULL;
> > + const char *fallback_path;
> > + int err;
> >
> > dev_priv = container_of(work, typeof(*dev_priv), display.dmc.work);
> > dmc = &dev_priv->display.dmc;
> >
> > - request_firmware(&fw, dev_priv->display.dmc.fw_path, dev_priv->drm.dev);
> > + err = request_firmware(&fw, dev_priv->display.dmc.fw_path, dev_priv->drm.dev);
> > +
> > + if (err == -ENOENT && !dev_priv->params.dmc_firmware_path) {
> > + fallback_path = dmc_fallback_path(dev_priv);
> > + if (fallback_path) {
> > + drm_dbg_kms(&dev_priv->drm,
> > + "%s not found, falling back to %s\n",
> > + dmc->fw_path,
> > + fallback_path);
> > + err = request_firmware(&fw, fallback_path, dev_priv->drm.dev);
> > + if (err == 0)
> > + dev_priv->display.dmc.fw_path = fallback_path;
> > + }
> > + }
> > +
> > parse_dmc_fw(dev_priv, fw);
> >
> > if (intel_dmc_has_payload(dev_priv)) {
> > --
> > 2.39.0
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/dmc: Do not require specific versions
2022-12-30 12:42 ` Gustavo Sousa
@ 2022-12-30 16:52 ` Rodrigo Vivi
2022-12-30 18:27 ` Gustavo Sousa
0 siblings, 1 reply; 12+ messages in thread
From: Rodrigo Vivi @ 2022-12-30 16:52 UTC (permalink / raw)
To: Gustavo Sousa; +Cc: Jani Nikula, intel-gfx, Lucas De Marchi
On Fri, Dec 30, 2022 at 09:42:39AM -0300, Gustavo Sousa wrote:
> On Fri, Dec 30, 2022 at 03:32:41AM -0500, Rodrigo Vivi wrote:
> > On Thu, Dec 29, 2022 at 04:07:39PM -0300, Gustavo Sousa wrote:
> > > Currently there is no DMC version requirement with respect to how i915
> > > interacts with it and new versions of the firmware serve as drop-in
> > > replacements of older ones. As such, do not require specific versions.
> > >
> > > References: https://lore.kernel.org/r/Y3081s7c5ksutpMW@intel.com
> >
> > I don't believe this link is a good reference as justification
> > of this patch.
> >
> > Probably https://docs.kernel.org/next/driver-api/firmware/firmware-usage-guidelines.html
> > is a better link?
>
> Yep. I agree this would be better. Is there an "archive" version of this page?
> Just to make sure we link to the exact version of that guide at the time of
> writing this patch.
This question reminded me that I should had used this link instead:
https://www.kernel.org/doc/html/latest/driver-api/firmware/firmware-usage-guidelines.html
And this is the one you are looking for:
https://www.kernel.org/doc/html/v6.1/driver-api/firmware/firmware-usage-guidelines.html
>
> >
> > Also, in the commit message we should be more clear that i915
> > interacts with the Hardware and not with any FW ABI/API, so the API
> > is fixed within the platform, hence no need to get this so-tied
> > version requirement.
>
> Thanks! I'll grab this paragraph and adapt it into the commit message if you
> allow me =)
sure, thanks!
>
> >
> > with the commit msg changed you can count on my reviewed-by,
> > the patch looks good to me.
> >
> > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_dmc.c | 35 ------------------------
> > > drivers/gpu/drm/i915/display/intel_dmc.h | 1 -
> > > 2 files changed, 36 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
> > > index 905b5dcdca14..4124b3d37110 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> > > @@ -53,51 +53,40 @@
> > > #define DISPLAY_VER12_DMC_MAX_FW_SIZE ICL_DMC_MAX_FW_SIZE
> > >
> > > #define DG2_DMC_PATH DMC_PATH(dg2, 2, 08)
> > > -#define DG2_DMC_VERSION_REQUIRED DMC_VERSION(2, 8)
> > > MODULE_FIRMWARE(DG2_DMC_PATH);
> > >
> > > #define ADLP_DMC_PATH DMC_PATH(adlp, 2, 16)
> > > -#define ADLP_DMC_VERSION_REQUIRED DMC_VERSION(2, 16)
> > > MODULE_FIRMWARE(ADLP_DMC_PATH);
> > >
> > > #define ADLS_DMC_PATH DMC_PATH(adls, 2, 01)
> > > -#define ADLS_DMC_VERSION_REQUIRED DMC_VERSION(2, 1)
> > > MODULE_FIRMWARE(ADLS_DMC_PATH);
> > >
> > > #define DG1_DMC_PATH DMC_PATH(dg1, 2, 02)
> > > -#define DG1_DMC_VERSION_REQUIRED DMC_VERSION(2, 2)
> > > MODULE_FIRMWARE(DG1_DMC_PATH);
> > >
> > > #define RKL_DMC_PATH DMC_PATH(rkl, 2, 03)
> > > -#define RKL_DMC_VERSION_REQUIRED DMC_VERSION(2, 3)
> > > MODULE_FIRMWARE(RKL_DMC_PATH);
> > >
> > > #define TGL_DMC_PATH DMC_PATH(tgl, 2, 12)
> > > -#define TGL_DMC_VERSION_REQUIRED DMC_VERSION(2, 12)
> > > MODULE_FIRMWARE(TGL_DMC_PATH);
> > >
> > > #define ICL_DMC_PATH DMC_PATH(icl, 1, 09)
> > > -#define ICL_DMC_VERSION_REQUIRED DMC_VERSION(1, 9)
> > > #define ICL_DMC_MAX_FW_SIZE 0x6000
> > > MODULE_FIRMWARE(ICL_DMC_PATH);
> > >
> > > #define GLK_DMC_PATH DMC_PATH(glk, 1, 04)
> > > -#define GLK_DMC_VERSION_REQUIRED DMC_VERSION(1, 4)
> > > #define GLK_DMC_MAX_FW_SIZE 0x4000
> > > MODULE_FIRMWARE(GLK_DMC_PATH);
> > >
> > > #define KBL_DMC_PATH DMC_PATH(kbl, 1, 04)
> > > -#define KBL_DMC_VERSION_REQUIRED DMC_VERSION(1, 4)
> > > #define KBL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
> > > MODULE_FIRMWARE(KBL_DMC_PATH);
> > >
> > > #define SKL_DMC_PATH DMC_PATH(skl, 1, 27)
> > > -#define SKL_DMC_VERSION_REQUIRED DMC_VERSION(1, 27)
> > > #define SKL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
> > > MODULE_FIRMWARE(SKL_DMC_PATH);
> > >
> > > #define BXT_DMC_PATH DMC_PATH(bxt, 1, 07)
> > > -#define BXT_DMC_VERSION_REQUIRED DMC_VERSION(1, 7)
> > > #define BXT_DMC_MAX_FW_SIZE 0x3000
> > > MODULE_FIRMWARE(BXT_DMC_PATH);
> > >
> > > @@ -765,17 +754,6 @@ static u32 parse_dmc_fw_css(struct intel_dmc *dmc,
> > > return 0;
> > > }
> > >
> > > - if (dmc->required_version &&
> > > - css_header->version != dmc->required_version) {
> > > - drm_info(&i915->drm, "Refusing to load DMC firmware v%u.%u,"
> > > - " please use v%u.%u\n",
> > > - DMC_VERSION_MAJOR(css_header->version),
> > > - DMC_VERSION_MINOR(css_header->version),
> > > - DMC_VERSION_MAJOR(dmc->required_version),
> > > - DMC_VERSION_MINOR(dmc->required_version));
> > > - return 0;
> > > - }
> > > -
> > > dmc->version = css_header->version;
> > >
> > > return sizeof(struct intel_css_header);
> > > @@ -903,49 +881,38 @@ void intel_dmc_ucode_init(struct drm_i915_private *dev_priv)
> > >
> > > if (IS_DG2(dev_priv)) {
> > > dmc->fw_path = DG2_DMC_PATH;
> > > - dmc->required_version = DG2_DMC_VERSION_REQUIRED;
> > > dmc->max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
> > > } else if (IS_ALDERLAKE_P(dev_priv)) {
> > > dmc->fw_path = ADLP_DMC_PATH;
> > > - dmc->required_version = ADLP_DMC_VERSION_REQUIRED;
> > > dmc->max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
> > > } else if (IS_ALDERLAKE_S(dev_priv)) {
> > > dmc->fw_path = ADLS_DMC_PATH;
> > > - dmc->required_version = ADLS_DMC_VERSION_REQUIRED;
> > > dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> > > } else if (IS_DG1(dev_priv)) {
> > > dmc->fw_path = DG1_DMC_PATH;
> > > - dmc->required_version = DG1_DMC_VERSION_REQUIRED;
> > > dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> > > } else if (IS_ROCKETLAKE(dev_priv)) {
> > > dmc->fw_path = RKL_DMC_PATH;
> > > - dmc->required_version = RKL_DMC_VERSION_REQUIRED;
> > > dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> > > } else if (IS_TIGERLAKE(dev_priv)) {
> > > dmc->fw_path = TGL_DMC_PATH;
> > > - dmc->required_version = TGL_DMC_VERSION_REQUIRED;
> > > dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> > > } else if (DISPLAY_VER(dev_priv) == 11) {
> > > dmc->fw_path = ICL_DMC_PATH;
> > > - dmc->required_version = ICL_DMC_VERSION_REQUIRED;
> > > dmc->max_fw_size = ICL_DMC_MAX_FW_SIZE;
> > > } else if (IS_GEMINILAKE(dev_priv)) {
> > > dmc->fw_path = GLK_DMC_PATH;
> > > - dmc->required_version = GLK_DMC_VERSION_REQUIRED;
> > > dmc->max_fw_size = GLK_DMC_MAX_FW_SIZE;
> > > } else if (IS_KABYLAKE(dev_priv) ||
> > > IS_COFFEELAKE(dev_priv) ||
> > > IS_COMETLAKE(dev_priv)) {
> > > dmc->fw_path = KBL_DMC_PATH;
> > > - dmc->required_version = KBL_DMC_VERSION_REQUIRED;
> > > dmc->max_fw_size = KBL_DMC_MAX_FW_SIZE;
> > > } else if (IS_SKYLAKE(dev_priv)) {
> > > dmc->fw_path = SKL_DMC_PATH;
> > > - dmc->required_version = SKL_DMC_VERSION_REQUIRED;
> > > dmc->max_fw_size = SKL_DMC_MAX_FW_SIZE;
> > > } else if (IS_BROXTON(dev_priv)) {
> > > dmc->fw_path = BXT_DMC_PATH;
> > > - dmc->required_version = BXT_DMC_VERSION_REQUIRED;
> > > dmc->max_fw_size = BXT_DMC_MAX_FW_SIZE;
> > > }
> > >
> > > @@ -958,8 +925,6 @@ void intel_dmc_ucode_init(struct drm_i915_private *dev_priv)
> > > }
> > >
> > > dmc->fw_path = dev_priv->params.dmc_firmware_path;
> > > - /* Bypass version check for firmware override. */
> > > - dmc->required_version = 0;
> > > }
> > >
> > > if (!dmc->fw_path) {
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
> > > index 67e03315ef99..435eab9b016b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dmc.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dmc.h
> > > @@ -25,7 +25,6 @@ enum {
> > > struct intel_dmc {
> > > struct work_struct work;
> > > const char *fw_path;
> > > - u32 required_version;
> > > u32 max_fw_size; /* bytes */
> > > u32 version;
> > > struct dmc_fw_info {
> > > --
> > > 2.39.0
> > >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH v3 2/2] drm/i915/dmc: Prepare to use unversioned paths
2022-12-30 13:36 ` Gustavo Sousa
@ 2022-12-30 16:56 ` Rodrigo Vivi
0 siblings, 0 replies; 12+ messages in thread
From: Rodrigo Vivi @ 2022-12-30 16:56 UTC (permalink / raw)
To: Gustavo Sousa; +Cc: Jani Nikula, intel-gfx, Lucas De Marchi
On Fri, Dec 30, 2022 at 10:36:28AM -0300, Gustavo Sousa wrote:
> On Fri, Dec 30, 2022 at 03:47:43AM -0500, Rodrigo Vivi wrote:
> > On Thu, Dec 29, 2022 at 04:07:40PM -0300, Gustavo Sousa wrote:
> > > New DMC releases in linux-firmware will stop using version number in
> > > blob filenames. This new convention provides the following benefits:
> > >
> > > 1. It simplifies code maintenance, as new DMC releases for a platform
> > > using the new convention will always use the same filename for the
> > > blob.
> > >
> > > 2. It allows DMC to be loaded even if the target system does not have
> > > the most recent firmware installed.
> > >
> > > Prepare the driver by:
> > >
> > > - Using the new convention for DMC_PATH() and renaming the currently
> > > used one to make it clear it is for the legacy scheme.
> > >
> > > - Implementing a fallback mechanism for future transitions from
> > > versioned to unversioned paths so that we do not cause a regression
> > > for systems not having the most up-to-date linux-firmware files.
> > >
> > > v2:
> > > - Keep using request_firmware() instead of firmware_request_nowarn().
> > > (Jani)
> > > v3:
> > > - Keep current DMC paths instead of directly using unversioned ones,
> > > so that we do not disturb initrd generation.
> > > (Lucas, Rodrigo)
> > >
> > > References: https://lore.kernel.org/r/Y3081s7c5ksutpMW@intel.com
> >
> > I also don't believe this link is a good reference here...
>
> Noted.
>
> >
> > Regarding the patch, I liked the approach in general.
> >
> > The patch is really neat, but I believe we will need to split it:
> >
> > 1. Add the new DMC_PATH and DMC_LEGACY_PATH only with the introduction
> > of the mtl_dmc.bin
> >
> > 2. And the fallback function, add only if/when we get a real fallback.
>
> Okay. For future reference, how should that be implemented with respect to the
> organization of the patches? I see two ways of doing it and have a personal
> preference for the first one:
>
> a) The future series would have first a patch adding the necessary functionality
> and a second one using it.
>
> b) The addition of the functionality would be incorporated in the same patch
> using it.
>
> For example, for (2), (a) would be a series two patches, the first adding the
> fallback mechanism and the second one changing ADLP to use unversioned paths;
> and (b) would be all of that in a single patch.
>
> I looks to me that approach (b) has a potential issue. For example, let's say we
> in the future we decide to revert that specific firmware update but we already
> have other platforms also using the fallback - a clean revert is not possible
> there and we would need to make sure that the fallback mechanism is kept.
>
> That's why I like (a) more and I think (b) would be more appropriate for cases
> where the functionality and it's user(s) have almost like a "1:1" relationship
> (not strictly speaking, read that as "having a somewhat static and restricted
> set of users").
yeap, it is case by case. The advantage on the (b) approach is that OSVs
can backport only 1 patch.
And the revert doesn't necessarily need to be a git-revert. In this case
it would be only one line getting back to the older firmware.
But really up to you ;)
As long as they are together either in the same patch or same series.
>
> >
> > Oh, and I just realized that when doing the new _dmc.bin path we also
> > need to make sure that we read the fw_version from the header and
> > print as a drm_info msg somewhere.
>
> I think the version is already being read by parse_dmc_fw_css() and printed at
> the end of dmc_load_work_fn() if the blob was loaded and parsed succesfully.
Oh, I had missed that. If it is already happening please disregard my comment.
>
> >
> > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_dmc.c | 62 ++++++++++++++++++------
> > > 1 file changed, 46 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
> > > index 4124b3d37110..12f05b2d33a3 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> > > @@ -42,51 +42,59 @@
> > > #define DMC_VERSION_MAJOR(version) ((version) >> 16)
> > > #define DMC_VERSION_MINOR(version) ((version) & 0xffff)
> > >
> > > -#define DMC_PATH(platform, major, minor) \
> > > - "i915/" \
> > > - __stringify(platform) "_dmc_ver" \
> > > - __stringify(major) "_" \
> > > +#define DMC_PATH(platform) \
> > > + "i915/" __stringify(platform) "_dmc.bin"
> > > +
> > > +/*
> > > + * New DMC additions should not use this. This is used solely to remain
> > > + * compatible with systems that have not yet updated DMC blobs to use
> > > + * unversioned file names.
> > > + */
> > > +#define DMC_LEGACY_PATH(platform, major, minor) \
> > > + "i915/" \
> > > + __stringify(platform) "_dmc_ver" \
> > > + __stringify(major) "_" \
> > > __stringify(minor) ".bin"
> > >
> > > #define DISPLAY_VER13_DMC_MAX_FW_SIZE 0x20000
> > >
> > > #define DISPLAY_VER12_DMC_MAX_FW_SIZE ICL_DMC_MAX_FW_SIZE
> > >
> > > -#define DG2_DMC_PATH DMC_PATH(dg2, 2, 08)
> > > +#define DG2_DMC_PATH DMC_LEGACY_PATH(dg2, 2, 08)
> > > MODULE_FIRMWARE(DG2_DMC_PATH);
> > >
> > > -#define ADLP_DMC_PATH DMC_PATH(adlp, 2, 16)
> > > +#define ADLP_DMC_PATH DMC_LEGACY_PATH(adlp, 2, 16)
> > > MODULE_FIRMWARE(ADLP_DMC_PATH);
> > >
> > > -#define ADLS_DMC_PATH DMC_PATH(adls, 2, 01)
> > > +#define ADLS_DMC_PATH DMC_LEGACY_PATH(adls, 2, 01)
> > > MODULE_FIRMWARE(ADLS_DMC_PATH);
> > >
> > > -#define DG1_DMC_PATH DMC_PATH(dg1, 2, 02)
> > > +#define DG1_DMC_PATH DMC_LEGACY_PATH(dg1, 2, 02)
> > > MODULE_FIRMWARE(DG1_DMC_PATH);
> > >
> > > -#define RKL_DMC_PATH DMC_PATH(rkl, 2, 03)
> > > +#define RKL_DMC_PATH DMC_LEGACY_PATH(rkl, 2, 03)
> > > MODULE_FIRMWARE(RKL_DMC_PATH);
> > >
> > > -#define TGL_DMC_PATH DMC_PATH(tgl, 2, 12)
> > > +#define TGL_DMC_PATH DMC_LEGACY_PATH(tgl, 2, 12)
> > > MODULE_FIRMWARE(TGL_DMC_PATH);
> > >
> > > -#define ICL_DMC_PATH DMC_PATH(icl, 1, 09)
> > > +#define ICL_DMC_PATH DMC_LEGACY_PATH(icl, 1, 09)
> > > #define ICL_DMC_MAX_FW_SIZE 0x6000
> > > MODULE_FIRMWARE(ICL_DMC_PATH);
> > >
> > > -#define GLK_DMC_PATH DMC_PATH(glk, 1, 04)
> > > +#define GLK_DMC_PATH DMC_LEGACY_PATH(glk, 1, 04)
> > > #define GLK_DMC_MAX_FW_SIZE 0x4000
> > > MODULE_FIRMWARE(GLK_DMC_PATH);
> > >
> > > -#define KBL_DMC_PATH DMC_PATH(kbl, 1, 04)
> > > +#define KBL_DMC_PATH DMC_LEGACY_PATH(kbl, 1, 04)
> > > #define KBL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
> > > MODULE_FIRMWARE(KBL_DMC_PATH);
> > >
> > > -#define SKL_DMC_PATH DMC_PATH(skl, 1, 27)
> > > +#define SKL_DMC_PATH DMC_LEGACY_PATH(skl, 1, 27)
> > > #define SKL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
> > > MODULE_FIRMWARE(SKL_DMC_PATH);
> > >
> > > -#define BXT_DMC_PATH DMC_PATH(bxt, 1, 07)
> > > +#define BXT_DMC_PATH DMC_LEGACY_PATH(bxt, 1, 07)
> > > #define BXT_DMC_MAX_FW_SIZE 0x3000
> > > MODULE_FIRMWARE(BXT_DMC_PATH);
> > >
> > > @@ -821,16 +829,38 @@ static void intel_dmc_runtime_pm_put(struct drm_i915_private *dev_priv)
> > > intel_display_power_put(dev_priv, POWER_DOMAIN_INIT, wakeref);
> > > }
> > >
> > > +static const char *dmc_fallback_path(struct drm_i915_private *i915)
> > > +{
> > > + /* No fallback paths for now. */
> > > + return NULL;
> > > +}
> > > +
> > > static void dmc_load_work_fn(struct work_struct *work)
> > > {
> > > struct drm_i915_private *dev_priv;
> > > struct intel_dmc *dmc;
> > > const struct firmware *fw = NULL;
> > > + const char *fallback_path;
> > > + int err;
> > >
> > > dev_priv = container_of(work, typeof(*dev_priv), display.dmc.work);
> > > dmc = &dev_priv->display.dmc;
> > >
> > > - request_firmware(&fw, dev_priv->display.dmc.fw_path, dev_priv->drm.dev);
> > > + err = request_firmware(&fw, dev_priv->display.dmc.fw_path, dev_priv->drm.dev);
> > > +
> > > + if (err == -ENOENT && !dev_priv->params.dmc_firmware_path) {
> > > + fallback_path = dmc_fallback_path(dev_priv);
> > > + if (fallback_path) {
> > > + drm_dbg_kms(&dev_priv->drm,
> > > + "%s not found, falling back to %s\n",
> > > + dmc->fw_path,
> > > + fallback_path);
> > > + err = request_firmware(&fw, fallback_path, dev_priv->drm.dev);
> > > + if (err == 0)
> > > + dev_priv->display.dmc.fw_path = fallback_path;
> > > + }
> > > + }
> > > +
> > > parse_dmc_fw(dev_priv, fw);
> > >
> > > if (intel_dmc_has_payload(dev_priv)) {
> > > --
> > > 2.39.0
> > >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/dmc: Do not require specific versions
2022-12-30 16:52 ` Rodrigo Vivi
@ 2022-12-30 18:27 ` Gustavo Sousa
0 siblings, 0 replies; 12+ messages in thread
From: Gustavo Sousa @ 2022-12-30 18:27 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: Jani Nikula, intel-gfx, Lucas De Marchi
Thanks for all the feedback.
I have just sent a v4 with only this patch. I'll split the second one as
instructed.
--
Gustavo Sousa
On Fri, Dec 30, 2022 at 11:52:07AM -0500, Rodrigo Vivi wrote:
> On Fri, Dec 30, 2022 at 09:42:39AM -0300, Gustavo Sousa wrote:
> > On Fri, Dec 30, 2022 at 03:32:41AM -0500, Rodrigo Vivi wrote:
> > > On Thu, Dec 29, 2022 at 04:07:39PM -0300, Gustavo Sousa wrote:
> > > > Currently there is no DMC version requirement with respect to how i915
> > > > interacts with it and new versions of the firmware serve as drop-in
> > > > replacements of older ones. As such, do not require specific versions.
> > > >
> > > > References: https://lore.kernel.org/r/Y3081s7c5ksutpMW@intel.com
> > >
> > > I don't believe this link is a good reference as justification
> > > of this patch.
> > >
> > > Probably https://docs.kernel.org/next/driver-api/firmware/firmware-usage-guidelines.html
> > > is a better link?
> >
> > Yep. I agree this would be better. Is there an "archive" version of this page?
> > Just to make sure we link to the exact version of that guide at the time of
> > writing this patch.
>
> This question reminded me that I should had used this link instead:
> https://www.kernel.org/doc/html/latest/driver-api/firmware/firmware-usage-guidelines.html
>
> And this is the one you are looking for:
> https://www.kernel.org/doc/html/v6.1/driver-api/firmware/firmware-usage-guidelines.html
>
> >
> > >
> > > Also, in the commit message we should be more clear that i915
> > > interacts with the Hardware and not with any FW ABI/API, so the API
> > > is fixed within the platform, hence no need to get this so-tied
> > > version requirement.
> >
> > Thanks! I'll grab this paragraph and adapt it into the commit message if you
> > allow me =)
>
> sure, thanks!
>
> >
> > >
> > > with the commit msg changed you can count on my reviewed-by,
> > > the patch looks good to me.
> > >
> > > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_dmc.c | 35 ------------------------
> > > > drivers/gpu/drm/i915/display/intel_dmc.h | 1 -
> > > > 2 files changed, 36 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
> > > > index 905b5dcdca14..4124b3d37110 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> > > > @@ -53,51 +53,40 @@
> > > > #define DISPLAY_VER12_DMC_MAX_FW_SIZE ICL_DMC_MAX_FW_SIZE
> > > >
> > > > #define DG2_DMC_PATH DMC_PATH(dg2, 2, 08)
> > > > -#define DG2_DMC_VERSION_REQUIRED DMC_VERSION(2, 8)
> > > > MODULE_FIRMWARE(DG2_DMC_PATH);
> > > >
> > > > #define ADLP_DMC_PATH DMC_PATH(adlp, 2, 16)
> > > > -#define ADLP_DMC_VERSION_REQUIRED DMC_VERSION(2, 16)
> > > > MODULE_FIRMWARE(ADLP_DMC_PATH);
> > > >
> > > > #define ADLS_DMC_PATH DMC_PATH(adls, 2, 01)
> > > > -#define ADLS_DMC_VERSION_REQUIRED DMC_VERSION(2, 1)
> > > > MODULE_FIRMWARE(ADLS_DMC_PATH);
> > > >
> > > > #define DG1_DMC_PATH DMC_PATH(dg1, 2, 02)
> > > > -#define DG1_DMC_VERSION_REQUIRED DMC_VERSION(2, 2)
> > > > MODULE_FIRMWARE(DG1_DMC_PATH);
> > > >
> > > > #define RKL_DMC_PATH DMC_PATH(rkl, 2, 03)
> > > > -#define RKL_DMC_VERSION_REQUIRED DMC_VERSION(2, 3)
> > > > MODULE_FIRMWARE(RKL_DMC_PATH);
> > > >
> > > > #define TGL_DMC_PATH DMC_PATH(tgl, 2, 12)
> > > > -#define TGL_DMC_VERSION_REQUIRED DMC_VERSION(2, 12)
> > > > MODULE_FIRMWARE(TGL_DMC_PATH);
> > > >
> > > > #define ICL_DMC_PATH DMC_PATH(icl, 1, 09)
> > > > -#define ICL_DMC_VERSION_REQUIRED DMC_VERSION(1, 9)
> > > > #define ICL_DMC_MAX_FW_SIZE 0x6000
> > > > MODULE_FIRMWARE(ICL_DMC_PATH);
> > > >
> > > > #define GLK_DMC_PATH DMC_PATH(glk, 1, 04)
> > > > -#define GLK_DMC_VERSION_REQUIRED DMC_VERSION(1, 4)
> > > > #define GLK_DMC_MAX_FW_SIZE 0x4000
> > > > MODULE_FIRMWARE(GLK_DMC_PATH);
> > > >
> > > > #define KBL_DMC_PATH DMC_PATH(kbl, 1, 04)
> > > > -#define KBL_DMC_VERSION_REQUIRED DMC_VERSION(1, 4)
> > > > #define KBL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
> > > > MODULE_FIRMWARE(KBL_DMC_PATH);
> > > >
> > > > #define SKL_DMC_PATH DMC_PATH(skl, 1, 27)
> > > > -#define SKL_DMC_VERSION_REQUIRED DMC_VERSION(1, 27)
> > > > #define SKL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE
> > > > MODULE_FIRMWARE(SKL_DMC_PATH);
> > > >
> > > > #define BXT_DMC_PATH DMC_PATH(bxt, 1, 07)
> > > > -#define BXT_DMC_VERSION_REQUIRED DMC_VERSION(1, 7)
> > > > #define BXT_DMC_MAX_FW_SIZE 0x3000
> > > > MODULE_FIRMWARE(BXT_DMC_PATH);
> > > >
> > > > @@ -765,17 +754,6 @@ static u32 parse_dmc_fw_css(struct intel_dmc *dmc,
> > > > return 0;
> > > > }
> > > >
> > > > - if (dmc->required_version &&
> > > > - css_header->version != dmc->required_version) {
> > > > - drm_info(&i915->drm, "Refusing to load DMC firmware v%u.%u,"
> > > > - " please use v%u.%u\n",
> > > > - DMC_VERSION_MAJOR(css_header->version),
> > > > - DMC_VERSION_MINOR(css_header->version),
> > > > - DMC_VERSION_MAJOR(dmc->required_version),
> > > > - DMC_VERSION_MINOR(dmc->required_version));
> > > > - return 0;
> > > > - }
> > > > -
> > > > dmc->version = css_header->version;
> > > >
> > > > return sizeof(struct intel_css_header);
> > > > @@ -903,49 +881,38 @@ void intel_dmc_ucode_init(struct drm_i915_private *dev_priv)
> > > >
> > > > if (IS_DG2(dev_priv)) {
> > > > dmc->fw_path = DG2_DMC_PATH;
> > > > - dmc->required_version = DG2_DMC_VERSION_REQUIRED;
> > > > dmc->max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
> > > > } else if (IS_ALDERLAKE_P(dev_priv)) {
> > > > dmc->fw_path = ADLP_DMC_PATH;
> > > > - dmc->required_version = ADLP_DMC_VERSION_REQUIRED;
> > > > dmc->max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
> > > > } else if (IS_ALDERLAKE_S(dev_priv)) {
> > > > dmc->fw_path = ADLS_DMC_PATH;
> > > > - dmc->required_version = ADLS_DMC_VERSION_REQUIRED;
> > > > dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> > > > } else if (IS_DG1(dev_priv)) {
> > > > dmc->fw_path = DG1_DMC_PATH;
> > > > - dmc->required_version = DG1_DMC_VERSION_REQUIRED;
> > > > dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> > > > } else if (IS_ROCKETLAKE(dev_priv)) {
> > > > dmc->fw_path = RKL_DMC_PATH;
> > > > - dmc->required_version = RKL_DMC_VERSION_REQUIRED;
> > > > dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> > > > } else if (IS_TIGERLAKE(dev_priv)) {
> > > > dmc->fw_path = TGL_DMC_PATH;
> > > > - dmc->required_version = TGL_DMC_VERSION_REQUIRED;
> > > > dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
> > > > } else if (DISPLAY_VER(dev_priv) == 11) {
> > > > dmc->fw_path = ICL_DMC_PATH;
> > > > - dmc->required_version = ICL_DMC_VERSION_REQUIRED;
> > > > dmc->max_fw_size = ICL_DMC_MAX_FW_SIZE;
> > > > } else if (IS_GEMINILAKE(dev_priv)) {
> > > > dmc->fw_path = GLK_DMC_PATH;
> > > > - dmc->required_version = GLK_DMC_VERSION_REQUIRED;
> > > > dmc->max_fw_size = GLK_DMC_MAX_FW_SIZE;
> > > > } else if (IS_KABYLAKE(dev_priv) ||
> > > > IS_COFFEELAKE(dev_priv) ||
> > > > IS_COMETLAKE(dev_priv)) {
> > > > dmc->fw_path = KBL_DMC_PATH;
> > > > - dmc->required_version = KBL_DMC_VERSION_REQUIRED;
> > > > dmc->max_fw_size = KBL_DMC_MAX_FW_SIZE;
> > > > } else if (IS_SKYLAKE(dev_priv)) {
> > > > dmc->fw_path = SKL_DMC_PATH;
> > > > - dmc->required_version = SKL_DMC_VERSION_REQUIRED;
> > > > dmc->max_fw_size = SKL_DMC_MAX_FW_SIZE;
> > > > } else if (IS_BROXTON(dev_priv)) {
> > > > dmc->fw_path = BXT_DMC_PATH;
> > > > - dmc->required_version = BXT_DMC_VERSION_REQUIRED;
> > > > dmc->max_fw_size = BXT_DMC_MAX_FW_SIZE;
> > > > }
> > > >
> > > > @@ -958,8 +925,6 @@ void intel_dmc_ucode_init(struct drm_i915_private *dev_priv)
> > > > }
> > > >
> > > > dmc->fw_path = dev_priv->params.dmc_firmware_path;
> > > > - /* Bypass version check for firmware override. */
> > > > - dmc->required_version = 0;
> > > > }
> > > >
> > > > if (!dmc->fw_path) {
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
> > > > index 67e03315ef99..435eab9b016b 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dmc.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dmc.h
> > > > @@ -25,7 +25,6 @@ enum {
> > > > struct intel_dmc {
> > > > struct work_struct work;
> > > > const char *fw_path;
> > > > - u32 required_version;
> > > > u32 max_fw_size; /* bytes */
> > > > u32 version;
> > > > struct dmc_fw_info {
> > > > --
> > > > 2.39.0
> > > >
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-12-30 18:27 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-29 19:07 [Intel-gfx] [PATCH v3 0/2] drm/i915/dmc: Make firmware loading backwards-compatible Gustavo Sousa
2022-12-29 19:07 ` [Intel-gfx] [PATCH v3 1/2] drm/i915/dmc: Do not require specific versions Gustavo Sousa
2022-12-30 8:32 ` Rodrigo Vivi
2022-12-30 12:42 ` Gustavo Sousa
2022-12-30 16:52 ` Rodrigo Vivi
2022-12-30 18:27 ` Gustavo Sousa
2022-12-29 19:07 ` [Intel-gfx] [PATCH v3 2/2] drm/i915/dmc: Prepare to use unversioned paths Gustavo Sousa
2022-12-30 8:47 ` Rodrigo Vivi
2022-12-30 13:36 ` Gustavo Sousa
2022-12-30 16:56 ` Rodrigo Vivi
2022-12-29 19:24 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/dmc: Make firmware loading backwards-compatible (rev3) Patchwork
2022-12-29 19:51 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.