[PATCH 0/5] drm/i915/dmc: firmware path handling changes

[PATCH 0/5] drm/i915/dmc: firmware path handling changes

[Jani Nikula]

We tried moving the dmc_firmware_path param from i915 to display params
before, so it would be present in xe as well as i915. This was commit
0d82a0d6f556 ("drm/i915/display: move dmc_firmware_path to display
params"). That failed, so I tried a quick fix, but that failed too
[1]. Revert followed in commit 318e82583ca9 ("Revert "drm/i915/display:
move dmc_firmware_path to display params"").

Do some refactoring and prep work first, with the most significant
change being the removal of special handling of
i915.dmc_firmware_path="" to disable DMC. This will now lead to the
platform default firmware being used.

To disable DMC and runtime PM, you'll need to specify a non-existent
file instead. There's no "magic" name specified for that.

BR,
Jani.



[1] https://lore.kernel.org/r/87h6gieuud.fsf@intel.com

Jani Nikula (5):
  drm/i915/dmc: handle request_firmware() errors separately
  drm/i915/dmc: improve firmware parse failure propagation
  drm/i915/dmc: split out per-platform firmware path selection
  drm/i915/dmc: change meaning of dmc_firmware_path="" module param
  drm/i915/display: move dmc_firmware_path to display params

 .../drm/i915/display/intel_display_params.c   |   4 +
 .../drm/i915/display/intel_display_params.h   |   1 +
 drivers/gpu/drm/i915/display/intel_dmc.c      | 162 ++++++++++--------
 drivers/gpu/drm/i915/i915_params.c            |   3 -
 drivers/gpu/drm/i915/i915_params.h            |   1 -
 drivers/gpu/drm/xe/xe_device_types.h          |   3 -
 6 files changed, 98 insertions(+), 76 deletions(-)

-- 
2.39.2

[PATCH 1/5] drm/i915/dmc: handle request_firmware() errors separately

[Jani Nikula]

Clarify request_firmware() error handling. Don't proceed to trying to
parse non-existent firmware or check for payload when request_firmware()
failed to begin with. There's no reason to release_firmware() either
when request_firmware() failed.

Also move the message about DMC firmware homepage here, as in other
cases the user probably has some firmware, although its parsing fails
for some reason.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index a34ff3383fd3..65880dea9c15 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -942,6 +942,15 @@ static void dmc_load_work_fn(struct work_struct *work)
 		}
 	}
 
+	if (err) {
+		drm_notice(&i915->drm,
+			   "Failed to load DMC firmware %s (%pe). Disabling runtime power management.\n",
+			   dmc->fw_path, ERR_PTR(err));
+		drm_notice(&i915->drm, "DMC firmware homepage: %s",
+			   INTEL_DMC_FIRMWARE_URL);
+		return;
+	}
+
 	parse_dmc_fw(dmc, fw);
 
 	if (intel_dmc_has_payload(i915)) {
@@ -956,8 +965,6 @@ static void dmc_load_work_fn(struct work_struct *work)
 			   "Failed to load DMC firmware %s."
 			   " Disabling runtime power management.\n",
 			   dmc->fw_path);
-		drm_notice(&i915->drm, "DMC firmware homepage: %s",
-			   INTEL_DMC_FIRMWARE_URL);
 	}
 
 	release_firmware(fw);
-- 
2.39.2

[PATCH 2/5] drm/i915/dmc: improve firmware parse failure propagation

[Jani Nikula]

Return failures from parse_dmc_fw() instead of relying on
intel_dmc_has_payload(). Handle and error report them slightly better.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc.c | 39 +++++++++++++-----------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index 65880dea9c15..ee5db1aafd50 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -853,7 +853,7 @@ static u32 parse_dmc_fw_css(struct intel_dmc *dmc,
 	return sizeof(struct intel_css_header);
 }
 
-static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
+static int parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
 {
 	struct drm_i915_private *i915 = dmc->i915;
 	struct intel_css_header *css_header;
@@ -866,13 +866,13 @@ static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
 	u32 r, offset;
 
 	if (!fw)
-		return;
+		return -EINVAL;
 
 	/* Extract CSS Header information */
 	css_header = (struct intel_css_header *)fw->data;
 	r = parse_dmc_fw_css(dmc, css_header, fw->size);
 	if (!r)
-		return;
+		return -EINVAL;
 
 	readcount += r;
 
@@ -880,7 +880,7 @@ static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
 	package_header = (struct intel_package_header *)&fw->data[readcount];
 	r = parse_dmc_fw_package(dmc, package_header, si, fw->size - readcount);
 	if (!r)
-		return;
+		return -EINVAL;
 
 	readcount += r;
 
@@ -897,6 +897,11 @@ static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
 		dmc_header = (struct intel_dmc_header_base *)&fw->data[offset];
 		parse_dmc_fw_header(dmc, dmc_header, fw->size - offset, dmc_id);
 	}
+
+	if (!intel_dmc_has_payload(i915))
+		return -ENOENT;
+
+	return 0;
 }
 
 static void intel_dmc_runtime_pm_get(struct drm_i915_private *i915)
@@ -951,22 +956,22 @@ static void dmc_load_work_fn(struct work_struct *work)
 		return;
 	}
 
-	parse_dmc_fw(dmc, fw);
-
-	if (intel_dmc_has_payload(i915)) {
-		intel_dmc_load_program(i915);
-		intel_dmc_runtime_pm_put(i915);
-
-		drm_info(&i915->drm, "Finished loading DMC firmware %s (v%u.%u)\n",
-			 dmc->fw_path, DMC_VERSION_MAJOR(dmc->version),
-			 DMC_VERSION_MINOR(dmc->version));
-	} else {
+	err = parse_dmc_fw(dmc, fw);
+	if (err) {
 		drm_notice(&i915->drm,
-			   "Failed to load DMC firmware %s."
-			   " Disabling runtime power management.\n",
-			   dmc->fw_path);
+			   "Failed to parse DMC firmware %s (%pe). Disabling runtime power management.\n",
+			   dmc->fw_path, ERR_PTR(err));
+		goto out;
 	}
 
+	intel_dmc_load_program(i915);
+	intel_dmc_runtime_pm_put(i915);
+
+	drm_info(&i915->drm, "Finished loading DMC firmware %s (v%u.%u)\n",
+		 dmc->fw_path, DMC_VERSION_MAJOR(dmc->version),
+		 DMC_VERSION_MINOR(dmc->version));
+
+out:
 	release_firmware(fw);
 }
 
-- 
2.39.2

[PATCH 3/5] drm/i915/dmc: split out per-platform firmware path selection

[Jani Nikula]

The big if ladder clutters intel_dmc_init(). Split it out to a separate
function.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc.c | 96 +++++++++++++-----------
 1 file changed, 54 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index ee5db1aafd50..740c05ce83cc 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -142,6 +142,59 @@ MODULE_FIRMWARE(SKL_DMC_PATH);
 #define BXT_DMC_MAX_FW_SIZE		0x3000
 MODULE_FIRMWARE(BXT_DMC_PATH);
 
+static const char *dmc_firmware_default(struct drm_i915_private *i915, u32 *size)
+{
+	const char *fw_path = NULL;
+	u32 max_fw_size = 0;
+
+	if (DISPLAY_VER_FULL(i915) == IP_VER(20, 0)) {
+		fw_path = XE2LPD_DMC_PATH;
+		max_fw_size = XE2LPD_DMC_MAX_FW_SIZE;
+	} else if (DISPLAY_VER_FULL(i915) == IP_VER(14, 0)) {
+		fw_path = MTL_DMC_PATH;
+		max_fw_size = XELPDP_DMC_MAX_FW_SIZE;
+	} else if (IS_DG2(i915)) {
+		fw_path = DG2_DMC_PATH;
+		max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
+	} else if (IS_ALDERLAKE_P(i915)) {
+		fw_path = ADLP_DMC_PATH;
+		max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
+	} else if (IS_ALDERLAKE_S(i915)) {
+		fw_path = ADLS_DMC_PATH;
+		max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
+	} else if (IS_DG1(i915)) {
+		fw_path = DG1_DMC_PATH;
+		max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
+	} else if (IS_ROCKETLAKE(i915)) {
+		fw_path = RKL_DMC_PATH;
+		max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
+	} else if (IS_TIGERLAKE(i915)) {
+		fw_path = TGL_DMC_PATH;
+		max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
+	} else if (DISPLAY_VER(i915) == 11) {
+		fw_path = ICL_DMC_PATH;
+		max_fw_size = ICL_DMC_MAX_FW_SIZE;
+	} else if (IS_GEMINILAKE(i915)) {
+		fw_path = GLK_DMC_PATH;
+		max_fw_size = GLK_DMC_MAX_FW_SIZE;
+	} else if (IS_KABYLAKE(i915) ||
+		   IS_COFFEELAKE(i915) ||
+		   IS_COMETLAKE(i915)) {
+		fw_path = KBL_DMC_PATH;
+		max_fw_size = KBL_DMC_MAX_FW_SIZE;
+	} else if (IS_SKYLAKE(i915)) {
+		fw_path = SKL_DMC_PATH;
+		max_fw_size = SKL_DMC_MAX_FW_SIZE;
+	} else if (IS_BROXTON(i915)) {
+		fw_path = BXT_DMC_PATH;
+		max_fw_size = BXT_DMC_MAX_FW_SIZE;
+	}
+
+	*size = max_fw_size;
+
+	return fw_path;
+}
+
 #define DMC_DEFAULT_FW_OFFSET		0xFFFFFFFF
 #define PACKAGE_MAX_FW_INFO_ENTRIES	20
 #define PACKAGE_V2_MAX_FW_INFO_ENTRIES	32
@@ -1007,48 +1060,7 @@ void intel_dmc_init(struct drm_i915_private *i915)
 
 	INIT_WORK(&dmc->work, dmc_load_work_fn);
 
-	if (DISPLAY_VER_FULL(i915) == IP_VER(20, 0)) {
-		dmc->fw_path = XE2LPD_DMC_PATH;
-		dmc->max_fw_size = XE2LPD_DMC_MAX_FW_SIZE;
-	} else if (DISPLAY_VER_FULL(i915) == IP_VER(14, 0)) {
-		dmc->fw_path = MTL_DMC_PATH;
-		dmc->max_fw_size = XELPDP_DMC_MAX_FW_SIZE;
-	} else if (IS_DG2(i915)) {
-		dmc->fw_path = DG2_DMC_PATH;
-		dmc->max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
-	} else if (IS_ALDERLAKE_P(i915)) {
-		dmc->fw_path = ADLP_DMC_PATH;
-		dmc->max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
-	} else if (IS_ALDERLAKE_S(i915)) {
-		dmc->fw_path = ADLS_DMC_PATH;
-		dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
-	} else if (IS_DG1(i915)) {
-		dmc->fw_path = DG1_DMC_PATH;
-		dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
-	} else if (IS_ROCKETLAKE(i915)) {
-		dmc->fw_path = RKL_DMC_PATH;
-		dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
-	} else if (IS_TIGERLAKE(i915)) {
-		dmc->fw_path = TGL_DMC_PATH;
-		dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
-	} else if (DISPLAY_VER(i915) == 11) {
-		dmc->fw_path = ICL_DMC_PATH;
-		dmc->max_fw_size = ICL_DMC_MAX_FW_SIZE;
-	} else if (IS_GEMINILAKE(i915)) {
-		dmc->fw_path = GLK_DMC_PATH;
-		dmc->max_fw_size = GLK_DMC_MAX_FW_SIZE;
-	} else if (IS_KABYLAKE(i915) ||
-		   IS_COFFEELAKE(i915) ||
-		   IS_COMETLAKE(i915)) {
-		dmc->fw_path = KBL_DMC_PATH;
-		dmc->max_fw_size = KBL_DMC_MAX_FW_SIZE;
-	} else if (IS_SKYLAKE(i915)) {
-		dmc->fw_path = SKL_DMC_PATH;
-		dmc->max_fw_size = SKL_DMC_MAX_FW_SIZE;
-	} else if (IS_BROXTON(i915)) {
-		dmc->fw_path = BXT_DMC_PATH;
-		dmc->max_fw_size = BXT_DMC_MAX_FW_SIZE;
-	}
+	dmc->fw_path = dmc_firmware_default(i915, &dmc->max_fw_size);
 
 	if (i915->params.dmc_firmware_path) {
 		if (strlen(i915->params.dmc_firmware_path) == 0) {
-- 
2.39.2

[PATCH 4/5] drm/i915/dmc: change meaning of dmc_firmware_path="" module param

[Jani Nikula]

The distinction between the dmc_firmware_path module param being NULL
and the empty string "" is problematic. It's not possible to set the
parameter back to NULL via sysfs or debugfs. Remove the distinction, and
consider NULL and the empty string to be the same thing, and use the
platform default for them.

This removes the possibility to disable DMC (and runtime PM) via
i915.dmc_firmware_path="". Instead, you will need to specify a
non-existent file or a file that will not parse correctly.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc.c | 20 ++++++++++----------
 drivers/gpu/drm/i915/i915_params.c       |  3 ++-
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index 740c05ce83cc..3e510c2be1eb 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -73,6 +73,13 @@ static struct intel_dmc *i915_to_dmc(struct drm_i915_private *i915)
 	return i915->display.dmc.dmc;
 }
 
+static const char *dmc_firmware_param(struct drm_i915_private *i915)
+{
+	const char *p = i915->params.dmc_firmware_path;
+
+	return p && *p ? p : NULL;
+}
+
 #define DMC_VERSION(major, minor)	((major) << 16 | (minor))
 #define DMC_VERSION_MAJOR(version)	((version) >> 16)
 #define DMC_VERSION_MINOR(version)	((version) & 0xffff)
@@ -989,7 +996,7 @@ static void dmc_load_work_fn(struct work_struct *work)
 
 	err = request_firmware(&fw, dmc->fw_path, i915->drm.dev);
 
-	if (err == -ENOENT && !i915->params.dmc_firmware_path) {
+	if (err == -ENOENT && !dmc_firmware_param(i915)) {
 		fallback_path = dmc_fallback_path(i915);
 		if (fallback_path) {
 			drm_dbg_kms(&i915->drm, "%s not found, falling back to %s\n",
@@ -1062,15 +1069,8 @@ void intel_dmc_init(struct drm_i915_private *i915)
 
 	dmc->fw_path = dmc_firmware_default(i915, &dmc->max_fw_size);
 
-	if (i915->params.dmc_firmware_path) {
-		if (strlen(i915->params.dmc_firmware_path) == 0) {
-			drm_info(&i915->drm,
-				 "Disabling DMC firmware and runtime PM\n");
-			goto out;
-		}
-
-		dmc->fw_path = i915->params.dmc_firmware_path;
-	}
+	if (dmc_firmware_param(i915))
+		dmc->fw_path = dmc_firmware_param(i915);
 
 	if (!dmc->fw_path) {
 		drm_dbg_kms(&i915->drm,
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index de43048543e8..9e7f2a9f6287 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -109,7 +109,8 @@ i915_param_named_unsafe(huc_firmware_path, charp, 0400,
 	"HuC firmware path to use instead of the default one");
 
 i915_param_named_unsafe(dmc_firmware_path, charp, 0400,
-	"DMC firmware path to use instead of the default one");
+	"DMC firmware path to use instead of the default one. "
+	"Use non-existent file to disable DMC and runtime PM.");
 
 i915_param_named_unsafe(gsc_firmware_path, charp, 0400,
 	"GSC firmware path to use instead of the default one");
-- 
2.39.2

[PATCH 5/5] drm/i915/display: move dmc_firmware_path to display params

[Jani Nikula]

The dmc_firmware_path parameter is clearly a display parameter. Move it
there so it's available to both i915 and xe modules. This also cleans up
the ugly member in struct xe_device.

v2:
- New try with the NULL/"" param value issue resolved

Link: https://patchwork.freedesktop.org/patch/msgid/20240321161856.3517856-1-jani.nikula@intel.com
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display_params.c | 4 ++++
 drivers/gpu/drm/i915/display/intel_display_params.h | 1 +
 drivers/gpu/drm/i915/display/intel_dmc.c            | 2 +-
 drivers/gpu/drm/i915/i915_params.c                  | 4 ----
 drivers/gpu/drm/i915/i915_params.h                  | 1 -
 drivers/gpu/drm/xe/xe_device_types.h                | 3 ---
 6 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
index f40b223cc8a1..ddce5a2c53d9 100644
--- a/drivers/gpu/drm/i915/display/intel_display_params.c
+++ b/drivers/gpu/drm/i915/display/intel_display_params.c
@@ -27,6 +27,10 @@ static struct intel_display_params intel_display_modparams __read_mostly = {
  * debugfs mode to 0.
  */
 
+intel_display_param_named_unsafe(dmc_firmware_path, charp, 0400,
+	"DMC firmware path to use instead of the default one. "
+	"Use non-existent file to disable DMC and runtime PM.");
+
 intel_display_param_named_unsafe(vbt_firmware, charp, 0400,
 	"Load VBT from specified file under /lib/firmware");
 
diff --git a/drivers/gpu/drm/i915/display/intel_display_params.h b/drivers/gpu/drm/i915/display/intel_display_params.h
index bf8dbbdb20a1..1208a62c16d2 100644
--- a/drivers/gpu/drm/i915/display/intel_display_params.h
+++ b/drivers/gpu/drm/i915/display/intel_display_params.h
@@ -24,6 +24,7 @@ struct drm_i915_private;
  *       debugfs file
  */
 #define INTEL_DISPLAY_PARAMS_FOR_EACH(param) \
+	param(char *, dmc_firmware_path, NULL, 0400) \
 	param(char *, vbt_firmware, NULL, 0400) \
 	param(int, lvds_channel_mode, 0, 0400) \
 	param(int, panel_use_ssc, -1, 0600) \
diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index 3e510c2be1eb..175669f7d61d 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -75,7 +75,7 @@ static struct intel_dmc *i915_to_dmc(struct drm_i915_private *i915)
 
 static const char *dmc_firmware_param(struct drm_i915_private *i915)
 {
-	const char *p = i915->params.dmc_firmware_path;
+	const char *p = i915->display.params.dmc_firmware_path;
 
 	return p && *p ? p : NULL;
 }
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 9e7f2a9f6287..8c00169e3ab7 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -108,10 +108,6 @@ i915_param_named_unsafe(guc_firmware_path, charp, 0400,
 i915_param_named_unsafe(huc_firmware_path, charp, 0400,
 	"HuC firmware path to use instead of the default one");
 
-i915_param_named_unsafe(dmc_firmware_path, charp, 0400,
-	"DMC firmware path to use instead of the default one. "
-	"Use non-existent file to disable DMC and runtime PM.");
-
 i915_param_named_unsafe(gsc_firmware_path, charp, 0400,
 	"GSC firmware path to use instead of the default one");
 
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 1315d7fac850..2eb3f2115ff2 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -51,7 +51,6 @@ struct drm_printer;
 	param(int, guc_log_level, -1, 0400) \
 	param(char *, guc_firmware_path, NULL, 0400) \
 	param(char *, huc_firmware_path, NULL, 0400) \
-	param(char *, dmc_firmware_path, NULL, 0400) \
 	param(char *, gsc_firmware_path, NULL, 0400) \
 	param(bool, memtest, false, 0400) \
 	param(int, mmio_debug, -IS_ENABLED(CONFIG_DRM_I915_DEBUG_MMIO), 0600) \
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 60ced5f90c2b..c70047762222 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -519,9 +519,6 @@ struct xe_device {
 		unsigned int czclk_freq;
 		unsigned int fsb_freq, mem_freq, is_ddr3;
 	};
-	struct {
-		const char *dmc_firmware_path;
-	} params;
 
 	void *pxp;
 #endif
-- 
2.39.2

✗ Fi.CI.CHECKPATCH: warning for drm/i915/dmc: firmware path handling changes

[Patchwork]

== Series Details ==

Series: drm/i915/dmc: firmware path handling changes
URL   : https://patchwork.freedesktop.org/series/132609/
State : warning

== Summary ==

Error: dim checkpatch failed
11a5475a5872 drm/i915/dmc: handle request_firmware() errors separately
2a996e8e9e0b drm/i915/dmc: improve firmware parse failure propagation
6d62c1714485 drm/i915/dmc: split out per-platform firmware path selection
5056359d9123 drm/i915/dmc: change meaning of dmc_firmware_path="" module param
08e1827e711e drm/i915/display: move dmc_firmware_path to display params
-:25: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#25: FILE: drivers/gpu/drm/i915/display/intel_display_params.c:31:
+intel_display_param_named_unsafe(dmc_firmware_path, charp, 0400,
+	"DMC firmware path to use instead of the default one. "

total: 0 errors, 0 warnings, 1 checks, 51 lines checked

✗ Fi.CI.SPARSE: warning for drm/i915/dmc: firmware path handling changes

[Patchwork]

== Series Details ==

Series: drm/i915/dmc: firmware path handling changes
URL   : https://patchwork.freedesktop.org/series/132609/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

✗ Fi.CI.BAT: failure for drm/i915/dmc: firmware path handling changes

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 12594 bytes --]

== Series Details ==

Series: drm/i915/dmc: firmware path handling changes
URL   : https://patchwork.freedesktop.org/series/132609/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14603 -> Patchwork_132609v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_132609v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_132609v1, please notify your bug team ("I915-ci-infra@lists.freedesktop.org") 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_132609v1/index.html

Participating hosts (38 -> 39)
------------------------------

  Additional (4): fi-kbl-7567u bat-kbl-2 bat-mtlp-6 bat-arls-3 
  Missing    (3): bat-dg1-7 fi-cfl-8109u fi-elk-e7500 

Possible new issues
-------------------

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

### IGT changes ###

#### Possible regressions ####

  * igt@debugfs_test@read_all_entries:
    - bat-atsm-1:         NOTRUN -> [INCOMPLETE][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-atsm-1/igt@debugfs_test@read_all_entries.html

  * igt@gem_exec_parallel@engines@contexts:
    - bat-arls-2:         [PASS][2] -> [ABORT][3]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/bat-arls-2/igt@gem_exec_parallel@engines@contexts.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-arls-2/igt@gem_exec_parallel@engines@contexts.html

  * igt@gem_exec_parallel@engines@fds:
    - bat-arls-2:         [PASS][4] -> [DMESG-WARN][5]
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/bat-arls-2/igt@gem_exec_parallel@engines@fds.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-arls-2/igt@gem_exec_parallel@engines@fds.html

  * igt@kms_flip@basic-flip-vs-dpms@b-dp1:
    - fi-kbl-7567u:       NOTRUN -> [INCOMPLETE][6]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/fi-kbl-7567u/igt@kms_flip@basic-flip-vs-dpms@b-dp1.html

  * igt@kms_flip@basic-flip-vs-dpms@b-hdmi-a1:
    - fi-tgl-1115g4:      [PASS][7] -> [INCOMPLETE][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/fi-tgl-1115g4/igt@kms_flip@basic-flip-vs-dpms@b-hdmi-a1.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/fi-tgl-1115g4/igt@kms_flip@basic-flip-vs-dpms@b-hdmi-a1.html

  
Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@core_auth@basic-auth:
    - fi-kbl-x1275:       NOTRUN -> [INCOMPLETE][9] ([i915#2295])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/fi-kbl-x1275/igt@core_auth@basic-auth.html

  * igt@debugfs_test@basic-hwmon:
    - bat-arls-3:         NOTRUN -> [SKIP][10] ([i915#9318])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-arls-3/igt@debugfs_test@basic-hwmon.html
    - bat-mtlp-6:         NOTRUN -> [SKIP][11] ([i915#9318])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-mtlp-6/igt@debugfs_test@basic-hwmon.html

  * igt@fbdev@info:
    - bat-mtlp-6:         NOTRUN -> [SKIP][12] ([i915#1849] / [i915#2582])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-mtlp-6/igt@fbdev@info.html

  * igt@fbdev@write:
    - bat-mtlp-6:         NOTRUN -> [SKIP][13] ([i915#2582]) +3 other tests skip
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-mtlp-6/igt@fbdev@write.html

  * igt@gem_busy@busy@all-engines:
    - fi-kbl-8809g:       [PASS][14] -> [ABORT][15] ([i915#10875])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/fi-kbl-8809g/igt@gem_busy@busy@all-engines.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/fi-kbl-8809g/igt@gem_busy@busy@all-engines.html

  * igt@gem_close_race@basic-process:
    - bat-mtlp-6:         NOTRUN -> [DMESG-WARN][16] ([i915#10875])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-mtlp-6/igt@gem_close_race@basic-process.html

  * igt@gem_close_race@basic-threads:
    - bat-dg2-9:          NOTRUN -> [DMESG-WARN][17] ([i915#10875])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-dg2-9/igt@gem_close_race@basic-threads.html

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-7567u:       NOTRUN -> [SKIP][18] ([i915#2190])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/fi-kbl-7567u/igt@gem_huc_copy@huc-copy.html

  * igt@gem_mmap@basic:
    - bat-arls-3:         NOTRUN -> [SKIP][19] ([i915#4083])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-arls-3/igt@gem_mmap@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-arls-3:         NOTRUN -> [SKIP][20] ([i915#10197] / [i915#10211] / [i915#4079])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-arls-3/igt@gem_render_tiled_blits@basic.html

  * igt@gem_tiled_blits@basic:
    - bat-arls-3:         NOTRUN -> [SKIP][21] ([i915#10196] / [i915#4077]) +2 other tests skip
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-arls-3/igt@gem_tiled_blits@basic.html

  * igt@gem_tiled_pread_basic:
    - bat-arls-3:         NOTRUN -> [SKIP][22] ([i915#10206] / [i915#4079])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-arls-3/igt@gem_tiled_pread_basic.html

  * igt@i915_module_load@load:
    - bat-kbl-2:          NOTRUN -> [INCOMPLETE][23] ([i915#10877])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-kbl-2/igt@i915_module_load@load.html

  * igt@kms_addfb_basic@addfb25-x-tiled-legacy:
    - bat-arls-3:         NOTRUN -> [SKIP][24] ([i915#10200]) +9 other tests skip
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-arls-3/igt@kms_addfb_basic@addfb25-x-tiled-legacy.html

  * igt@kms_busy@basic@modeset:
    - fi-cfl-guc:         [PASS][25] -> [INCOMPLETE][26] ([i915#10056])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/fi-cfl-guc/igt@kms_busy@basic@modeset.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/fi-cfl-guc/igt@kms_busy@basic@modeset.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - bat-arls-3:         NOTRUN -> [SKIP][27] ([i915#10202]) +1 other test skip
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-arls-3/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size:
    - bat-arls-3:         NOTRUN -> [ABORT][28] ([i915#10875])
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-arls-3/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html

  * igt@kms_dsc@dsc-basic:
    - fi-kbl-7567u:       NOTRUN -> [SKIP][29] +3 other tests skip
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/fi-kbl-7567u/igt@kms_dsc@dsc-basic.html

  
#### Possible fixes ####

  * igt@gem_close_race@basic-process:
    - bat-dg2-9:          [DMESG-WARN][30] ([i915#10875]) -> [PASS][31]
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/bat-dg2-9/igt@gem_close_race@basic-process.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-dg2-9/igt@gem_close_race@basic-process.html

  * igt@i915_module_load@load:
    - bat-dg2-13:         [INCOMPLETE][32] ([i915#10875]) -> [PASS][33]
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/bat-dg2-13/igt@i915_module_load@load.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-dg2-13/igt@i915_module_load@load.html
    - bat-atsm-1:         [INCOMPLETE][34] ([i915#10875]) -> [PASS][35]
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/bat-atsm-1/igt@i915_module_load@load.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-atsm-1/igt@i915_module_load@load.html

  * igt@kms_busy@basic@flip:
    - {bat-mtlp-9}:       [ABORT][36] -> [PASS][37]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/bat-mtlp-9/igt@kms_busy@basic@flip.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-mtlp-9/igt@kms_busy@basic@flip.html
    - fi-pnv-d510:        [ABORT][38] -> [PASS][39]
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/fi-pnv-d510/igt@kms_busy@basic@flip.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/fi-pnv-d510/igt@kms_busy@basic@flip.html

  * igt@kms_busy@basic@modeset:
    - bat-adlp-9:         [DMESG-WARN][40] ([i915#10875]) -> [PASS][41]
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/bat-adlp-9/igt@kms_busy@basic@modeset.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-adlp-9/igt@kms_busy@basic@modeset.html
    - {bat-mtlp-9}:       [DMESG-WARN][42] -> [PASS][43]
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/bat-mtlp-9/igt@kms_busy@basic@modeset.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-mtlp-9/igt@kms_busy@basic@modeset.html

  * igt@kms_flip@basic-flip-vs-dpms@b-dp1:
    - fi-apl-guc:         [INCOMPLETE][44] -> [PASS][45]
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/fi-apl-guc/igt@kms_flip@basic-flip-vs-dpms@b-dp1.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/fi-apl-guc/igt@kms_flip@basic-flip-vs-dpms@b-dp1.html

  
#### Warnings ####

  * igt@i915_module_load@load:
    - bat-dg2-8:          [INCOMPLETE][46] ([i915#10875] / [i915#10877]) -> [INCOMPLETE][47] ([i915#10877])
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/bat-dg2-8/igt@i915_module_load@load.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-dg2-8/igt@i915_module_load@load.html

  * igt@kms_busy@basic@modeset:
    - fi-pnv-d510:        [SKIP][48] -> [ABORT][49] ([i915#10875])
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/fi-pnv-d510/igt@kms_busy@basic@modeset.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/fi-pnv-d510/igt@kms_busy@basic@modeset.html
    - bat-dg2-14:         [DMESG-WARN][50] ([i915#10875]) -> [INCOMPLETE][51] ([i915#10056] / [i915#10875])
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14603/bat-dg2-14/igt@kms_busy@basic@modeset.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/bat-dg2-14/igt@kms_busy@basic@modeset.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#10056]: https://gitlab.freedesktop.org/drm/intel/issues/10056
  [i915#10196]: https://gitlab.freedesktop.org/drm/intel/issues/10196
  [i915#10197]: https://gitlab.freedesktop.org/drm/intel/issues/10197
  [i915#10200]: https://gitlab.freedesktop.org/drm/intel/issues/10200
  [i915#10202]: https://gitlab.freedesktop.org/drm/intel/issues/10202
  [i915#10206]: https://gitlab.freedesktop.org/drm/intel/issues/10206
  [i915#10211]: https://gitlab.freedesktop.org/drm/intel/issues/10211
  [i915#10875]: https://gitlab.freedesktop.org/drm/intel/issues/10875
  [i915#10877]: https://gitlab.freedesktop.org/drm/intel/issues/10877
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2295]: https://gitlab.freedesktop.org/drm/intel/issues/2295
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [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#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#9318]: https://gitlab.freedesktop.org/drm/intel/issues/9318


Build changes
-------------

  * Linux: CI_DRM_14603 -> Patchwork_132609v1

  CI-20190529: 20190529
  CI_DRM_14603: 610019be3a56c982044361759f6ed366ae99a2a1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7813: 66841b7d9024447be4f4f5449aaf4f021e6323e5 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_132609v1: 610019be3a56c982044361759f6ed366ae99a2a1 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v1/index.html

[-- Attachment #2: Type: text/html, Size: 15094 bytes --]

Re: [PATCH 1/5] drm/i915/dmc: handle request_firmware() errors separately

[Gustavo Sousa]

Quoting Jani Nikula (2024-04-18 11:39:50-03:00)
>Clarify request_firmware() error handling. Don't proceed to trying to
>parse non-existent firmware or check for payload when request_firmware()
>failed to begin with. There's no reason to release_firmware() either
>when request_firmware() failed.
>
>Also move the message about DMC firmware homepage here, as in other
>cases the user probably has some firmware, although its parsing fails
>for some reason.
>
>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>---
> drivers/gpu/drm/i915/display/intel_dmc.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>index a34ff3383fd3..65880dea9c15 100644
>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>@@ -942,6 +942,15 @@ static void dmc_load_work_fn(struct work_struct *work)
>                 }
>         }
> 
>+        if (err) {
>+                drm_notice(&i915->drm,
>+                           "Failed to load DMC firmware %s (%pe). Disabling runtime power management.\n",
>+                           dmc->fw_path, ERR_PTR(err));
>+                drm_notice(&i915->drm, "DMC firmware homepage: %s",
>+                           INTEL_DMC_FIRMWARE_URL);

This could also be:

    drm_notice(&i915->drm, "DMC firmware homepage: " INTEL_DMC_FIRMWARE_URL)

>+                return;
>+        }
>+
>         parse_dmc_fw(dmc, fw);

Maybe also remove the now unnecessary NULL check for fw in
parse_dmc_fw()?

> 
>         if (intel_dmc_has_payload(i915)) {
>@@ -956,8 +965,6 @@ static void dmc_load_work_fn(struct work_struct *work)
>                            "Failed to load DMC firmware %s."

Should we tweak the message to differentiate from the previous one? At
this point, we know the blob file exists, but there is a problem with
its content.

I think the patch looks good and to me all of the above suggestions are
just that :-)  So, with or without them:

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

--
Gustavo Sousa

>                            " Disabling runtime power management.\n",
>                            dmc->fw_path);
>-                drm_notice(&i915->drm, "DMC firmware homepage: %s",
>-                           INTEL_DMC_FIRMWARE_URL);
>         }
> 
>         release_firmware(fw);
>-- 
>2.39.2
>

Re: [PATCH 2/5] drm/i915/dmc: improve firmware parse failure propagation

[Gustavo Sousa]

Quoting Jani Nikula (2024-04-18 11:39:51-03:00)
>Return failures from parse_dmc_fw() instead of relying on
>intel_dmc_has_payload(). Handle and error report them slightly better.
>
>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>---
> drivers/gpu/drm/i915/display/intel_dmc.c | 39 +++++++++++++-----------
> 1 file changed, 22 insertions(+), 17 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>index 65880dea9c15..ee5db1aafd50 100644
>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>@@ -853,7 +853,7 @@ static u32 parse_dmc_fw_css(struct intel_dmc *dmc,
>         return sizeof(struct intel_css_header);
> }
> 
>-static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
>+static int parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
> {
>         struct drm_i915_private *i915 = dmc->i915;
>         struct intel_css_header *css_header;
>@@ -866,13 +866,13 @@ static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
>         u32 r, offset;
> 
>         if (!fw)
>-                return;
>+                return -EINVAL;
> 
>         /* Extract CSS Header information */
>         css_header = (struct intel_css_header *)fw->data;
>         r = parse_dmc_fw_css(dmc, css_header, fw->size);
>         if (!r)
>-                return;
>+                return -EINVAL;
> 
>         readcount += r;
> 
>@@ -880,7 +880,7 @@ static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
>         package_header = (struct intel_package_header *)&fw->data[readcount];
>         r = parse_dmc_fw_package(dmc, package_header, si, fw->size - readcount);
>         if (!r)
>-                return;
>+                return -EINVAL;
> 
>         readcount += r;
> 
>@@ -897,6 +897,11 @@ static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
>                 dmc_header = (struct intel_dmc_header_base *)&fw->data[offset];
>                 parse_dmc_fw_header(dmc, dmc_header, fw->size - offset, dmc_id);
>         }
>+
>+        if (!intel_dmc_has_payload(i915))
>+                return -ENOENT;

This function and also the parsing helpers (parse_dmc_fw_*) already have
the pattern of providing error messages for issues found. We could maybe
have parse_dmc_fw() simply returning -1 for errors here.

For this specific condition (!intel_dmc_has_payload(i915)), we could
complain that there the main DMC program was not found before returning.
I think ENOENT might confuse users.

--
Gustavo Sousa

>+
>+        return 0;
> }
> 
> static void intel_dmc_runtime_pm_get(struct drm_i915_private *i915)
>@@ -951,22 +956,22 @@ static void dmc_load_work_fn(struct work_struct *work)
>                 return;
>         }
> 
>-        parse_dmc_fw(dmc, fw);
>-
>-        if (intel_dmc_has_payload(i915)) {
>-                intel_dmc_load_program(i915);
>-                intel_dmc_runtime_pm_put(i915);
>-
>-                drm_info(&i915->drm, "Finished loading DMC firmware %s (v%u.%u)\n",
>-                         dmc->fw_path, DMC_VERSION_MAJOR(dmc->version),
>-                         DMC_VERSION_MINOR(dmc->version));
>-        } else {
>+        err = parse_dmc_fw(dmc, fw);
>+        if (err) {
>                 drm_notice(&i915->drm,
>-                           "Failed to load DMC firmware %s."
>-                           " Disabling runtime power management.\n",
>-                           dmc->fw_path);
>+                           "Failed to parse DMC firmware %s (%pe). Disabling runtime power management.\n",
>+                           dmc->fw_path, ERR_PTR(err));
>+                goto out;
>         }
> 
>+        intel_dmc_load_program(i915);
>+        intel_dmc_runtime_pm_put(i915);
>+
>+        drm_info(&i915->drm, "Finished loading DMC firmware %s (v%u.%u)\n",
>+                 dmc->fw_path, DMC_VERSION_MAJOR(dmc->version),
>+                 DMC_VERSION_MINOR(dmc->version));
>+
>+out:
>         release_firmware(fw);
> }
> 
>-- 
>2.39.2
>

Re: [PATCH 3/5] drm/i915/dmc: split out per-platform firmware path selection

[Gustavo Sousa]

Quoting Jani Nikula (2024-04-18 11:39:52-03:00)
>The big if ladder clutters intel_dmc_init(). Split it out to a separate
>function.
>
>Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

>---
> drivers/gpu/drm/i915/display/intel_dmc.c | 96 +++++++++++++-----------
> 1 file changed, 54 insertions(+), 42 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>index ee5db1aafd50..740c05ce83cc 100644
>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>@@ -142,6 +142,59 @@ MODULE_FIRMWARE(SKL_DMC_PATH);
> #define BXT_DMC_MAX_FW_SIZE                0x3000
> MODULE_FIRMWARE(BXT_DMC_PATH);
> 
>+static const char *dmc_firmware_default(struct drm_i915_private *i915, u32 *size)
>+{
>+        const char *fw_path = NULL;
>+        u32 max_fw_size = 0;
>+
>+        if (DISPLAY_VER_FULL(i915) == IP_VER(20, 0)) {
>+                fw_path = XE2LPD_DMC_PATH;
>+                max_fw_size = XE2LPD_DMC_MAX_FW_SIZE;
>+        } else if (DISPLAY_VER_FULL(i915) == IP_VER(14, 0)) {
>+                fw_path = MTL_DMC_PATH;
>+                max_fw_size = XELPDP_DMC_MAX_FW_SIZE;
>+        } else if (IS_DG2(i915)) {
>+                fw_path = DG2_DMC_PATH;
>+                max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
>+        } else if (IS_ALDERLAKE_P(i915)) {
>+                fw_path = ADLP_DMC_PATH;
>+                max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
>+        } else if (IS_ALDERLAKE_S(i915)) {
>+                fw_path = ADLS_DMC_PATH;
>+                max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
>+        } else if (IS_DG1(i915)) {
>+                fw_path = DG1_DMC_PATH;
>+                max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
>+        } else if (IS_ROCKETLAKE(i915)) {
>+                fw_path = RKL_DMC_PATH;
>+                max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
>+        } else if (IS_TIGERLAKE(i915)) {
>+                fw_path = TGL_DMC_PATH;
>+                max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
>+        } else if (DISPLAY_VER(i915) == 11) {
>+                fw_path = ICL_DMC_PATH;
>+                max_fw_size = ICL_DMC_MAX_FW_SIZE;
>+        } else if (IS_GEMINILAKE(i915)) {
>+                fw_path = GLK_DMC_PATH;
>+                max_fw_size = GLK_DMC_MAX_FW_SIZE;
>+        } else if (IS_KABYLAKE(i915) ||
>+                   IS_COFFEELAKE(i915) ||
>+                   IS_COMETLAKE(i915)) {
>+                fw_path = KBL_DMC_PATH;
>+                max_fw_size = KBL_DMC_MAX_FW_SIZE;
>+        } else if (IS_SKYLAKE(i915)) {
>+                fw_path = SKL_DMC_PATH;
>+                max_fw_size = SKL_DMC_MAX_FW_SIZE;
>+        } else if (IS_BROXTON(i915)) {
>+                fw_path = BXT_DMC_PATH;
>+                max_fw_size = BXT_DMC_MAX_FW_SIZE;
>+        }
>+
>+        *size = max_fw_size;
>+
>+        return fw_path;
>+}
>+
> #define DMC_DEFAULT_FW_OFFSET                0xFFFFFFFF
> #define PACKAGE_MAX_FW_INFO_ENTRIES        20
> #define PACKAGE_V2_MAX_FW_INFO_ENTRIES        32
>@@ -1007,48 +1060,7 @@ void intel_dmc_init(struct drm_i915_private *i915)
> 
>         INIT_WORK(&dmc->work, dmc_load_work_fn);
> 
>-        if (DISPLAY_VER_FULL(i915) == IP_VER(20, 0)) {
>-                dmc->fw_path = XE2LPD_DMC_PATH;
>-                dmc->max_fw_size = XE2LPD_DMC_MAX_FW_SIZE;
>-        } else if (DISPLAY_VER_FULL(i915) == IP_VER(14, 0)) {
>-                dmc->fw_path = MTL_DMC_PATH;
>-                dmc->max_fw_size = XELPDP_DMC_MAX_FW_SIZE;
>-        } else if (IS_DG2(i915)) {
>-                dmc->fw_path = DG2_DMC_PATH;
>-                dmc->max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
>-        } else if (IS_ALDERLAKE_P(i915)) {
>-                dmc->fw_path = ADLP_DMC_PATH;
>-                dmc->max_fw_size = DISPLAY_VER13_DMC_MAX_FW_SIZE;
>-        } else if (IS_ALDERLAKE_S(i915)) {
>-                dmc->fw_path = ADLS_DMC_PATH;
>-                dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
>-        } else if (IS_DG1(i915)) {
>-                dmc->fw_path = DG1_DMC_PATH;
>-                dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
>-        } else if (IS_ROCKETLAKE(i915)) {
>-                dmc->fw_path = RKL_DMC_PATH;
>-                dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
>-        } else if (IS_TIGERLAKE(i915)) {
>-                dmc->fw_path = TGL_DMC_PATH;
>-                dmc->max_fw_size = DISPLAY_VER12_DMC_MAX_FW_SIZE;
>-        } else if (DISPLAY_VER(i915) == 11) {
>-                dmc->fw_path = ICL_DMC_PATH;
>-                dmc->max_fw_size = ICL_DMC_MAX_FW_SIZE;
>-        } else if (IS_GEMINILAKE(i915)) {
>-                dmc->fw_path = GLK_DMC_PATH;
>-                dmc->max_fw_size = GLK_DMC_MAX_FW_SIZE;
>-        } else if (IS_KABYLAKE(i915) ||
>-                   IS_COFFEELAKE(i915) ||
>-                   IS_COMETLAKE(i915)) {
>-                dmc->fw_path = KBL_DMC_PATH;
>-                dmc->max_fw_size = KBL_DMC_MAX_FW_SIZE;
>-        } else if (IS_SKYLAKE(i915)) {
>-                dmc->fw_path = SKL_DMC_PATH;
>-                dmc->max_fw_size = SKL_DMC_MAX_FW_SIZE;
>-        } else if (IS_BROXTON(i915)) {
>-                dmc->fw_path = BXT_DMC_PATH;
>-                dmc->max_fw_size = BXT_DMC_MAX_FW_SIZE;
>-        }
>+        dmc->fw_path = dmc_firmware_default(i915, &dmc->max_fw_size);
> 
>         if (i915->params.dmc_firmware_path) {
>                 if (strlen(i915->params.dmc_firmware_path) == 0) {
>-- 
>2.39.2
>

Re: [PATCH 4/5] drm/i915/dmc: change meaning of dmc_firmware_path="" module param

[Gustavo Sousa]

Quoting Jani Nikula (2024-04-18 11:39:53-03:00)
>The distinction between the dmc_firmware_path module param being NULL
>and the empty string "" is problematic. It's not possible to set the
>parameter back to NULL via sysfs or debugfs. Remove the distinction, and
>consider NULL and the empty string to be the same thing, and use the
>platform default for them.
>
>This removes the possibility to disable DMC (and runtime PM) via
>i915.dmc_firmware_path="". Instead, you will need to specify a
>non-existent file or a file that will not parse correctly.
>
>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>---
> drivers/gpu/drm/i915/display/intel_dmc.c | 20 ++++++++++----------
> drivers/gpu/drm/i915/i915_params.c       |  3 ++-
> 2 files changed, 12 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>index 740c05ce83cc..3e510c2be1eb 100644
>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>@@ -73,6 +73,13 @@ static struct intel_dmc *i915_to_dmc(struct drm_i915_private *i915)
>         return i915->display.dmc.dmc;
> }
> 
>+static const char *dmc_firmware_param(struct drm_i915_private *i915)
>+{
>+        const char *p = i915->params.dmc_firmware_path;
>+
>+        return p && *p ? p : NULL;
>+}
>+
> #define DMC_VERSION(major, minor)        ((major) << 16 | (minor))
> #define DMC_VERSION_MAJOR(version)        ((version) >> 16)
> #define DMC_VERSION_MINOR(version)        ((version) & 0xffff)
>@@ -989,7 +996,7 @@ static void dmc_load_work_fn(struct work_struct *work)
> 
>         err = request_firmware(&fw, dmc->fw_path, i915->drm.dev);
> 
>-        if (err == -ENOENT && !i915->params.dmc_firmware_path) {
>+        if (err == -ENOENT && !dmc_firmware_param(i915)) {
>                 fallback_path = dmc_fallback_path(i915);
>                 if (fallback_path) {
>                         drm_dbg_kms(&i915->drm, "%s not found, falling back to %s\n",
>@@ -1062,15 +1069,8 @@ void intel_dmc_init(struct drm_i915_private *i915)
> 
>         dmc->fw_path = dmc_firmware_default(i915, &dmc->max_fw_size);
> 
>-        if (i915->params.dmc_firmware_path) {
>-                if (strlen(i915->params.dmc_firmware_path) == 0) {
>-                        drm_info(&i915->drm,
>-                                 "Disabling DMC firmware and runtime PM\n");
>-                        goto out;
>-                }
>-
>-                dmc->fw_path = i915->params.dmc_firmware_path;
>-        }
>+        if (dmc_firmware_param(i915))
>+                dmc->fw_path = dmc_firmware_param(i915);
> 
>         if (!dmc->fw_path) {
>                 drm_dbg_kms(&i915->drm,
>diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>index de43048543e8..9e7f2a9f6287 100644
>--- a/drivers/gpu/drm/i915/i915_params.c
>+++ b/drivers/gpu/drm/i915/i915_params.c
>@@ -109,7 +109,8 @@ i915_param_named_unsafe(huc_firmware_path, charp, 0400,
>         "HuC firmware path to use instead of the default one");
> 
> i915_param_named_unsafe(dmc_firmware_path, charp, 0400,
>-        "DMC firmware path to use instead of the default one");
>+        "DMC firmware path to use instead of the default one. "
>+        "Use non-existent file to disable DMC and runtime PM.");

Okay. But is it too bad to have a magic string for it? The up side is
that there wouldn't be error messages in the log if we had such option.

--
Gustavo Sousa

> 
> i915_param_named_unsafe(gsc_firmware_path, charp, 0400,
>         "GSC firmware path to use instead of the default one");
>-- 
>2.39.2
>

Re: [PATCH 5/5] drm/i915/display: move dmc_firmware_path to display params

[Gustavo Sousa]

Quoting Jani Nikula (2024-04-18 11:39:54-03:00)
>The dmc_firmware_path parameter is clearly a display parameter. Move it
>there so it's available to both i915 and xe modules. This also cleans up
>the ugly member in struct xe_device.
>
>v2:
>- New try with the NULL/"" param value issue resolved
>
>Link: https://patchwork.freedesktop.org/patch/msgid/20240321161856.3517856-1-jani.nikula@intel.com
>Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

>---
> drivers/gpu/drm/i915/display/intel_display_params.c | 4 ++++
> drivers/gpu/drm/i915/display/intel_display_params.h | 1 +
> drivers/gpu/drm/i915/display/intel_dmc.c            | 2 +-
> drivers/gpu/drm/i915/i915_params.c                  | 4 ----
> drivers/gpu/drm/i915/i915_params.h                  | 1 -
> drivers/gpu/drm/xe/xe_device_types.h                | 3 ---
> 6 files changed, 6 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
>index f40b223cc8a1..ddce5a2c53d9 100644
>--- a/drivers/gpu/drm/i915/display/intel_display_params.c
>+++ b/drivers/gpu/drm/i915/display/intel_display_params.c
>@@ -27,6 +27,10 @@ static struct intel_display_params intel_display_modparams __read_mostly = {
>  * debugfs mode to 0.
>  */
> 
>+intel_display_param_named_unsafe(dmc_firmware_path, charp, 0400,
>+        "DMC firmware path to use instead of the default one. "
>+        "Use non-existent file to disable DMC and runtime PM.");
>+
> intel_display_param_named_unsafe(vbt_firmware, charp, 0400,
>         "Load VBT from specified file under /lib/firmware");
> 
>diff --git a/drivers/gpu/drm/i915/display/intel_display_params.h b/drivers/gpu/drm/i915/display/intel_display_params.h
>index bf8dbbdb20a1..1208a62c16d2 100644
>--- a/drivers/gpu/drm/i915/display/intel_display_params.h
>+++ b/drivers/gpu/drm/i915/display/intel_display_params.h
>@@ -24,6 +24,7 @@ struct drm_i915_private;
>  *       debugfs file
>  */
> #define INTEL_DISPLAY_PARAMS_FOR_EACH(param) \
>+        param(char *, dmc_firmware_path, NULL, 0400) \
>         param(char *, vbt_firmware, NULL, 0400) \
>         param(int, lvds_channel_mode, 0, 0400) \
>         param(int, panel_use_ssc, -1, 0600) \
>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>index 3e510c2be1eb..175669f7d61d 100644
>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>@@ -75,7 +75,7 @@ static struct intel_dmc *i915_to_dmc(struct drm_i915_private *i915)
> 
> static const char *dmc_firmware_param(struct drm_i915_private *i915)
> {
>-        const char *p = i915->params.dmc_firmware_path;
>+        const char *p = i915->display.params.dmc_firmware_path;
> 
>         return p && *p ? p : NULL;
> }
>diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>index 9e7f2a9f6287..8c00169e3ab7 100644
>--- a/drivers/gpu/drm/i915/i915_params.c
>+++ b/drivers/gpu/drm/i915/i915_params.c
>@@ -108,10 +108,6 @@ i915_param_named_unsafe(guc_firmware_path, charp, 0400,
> i915_param_named_unsafe(huc_firmware_path, charp, 0400,
>         "HuC firmware path to use instead of the default one");
> 
>-i915_param_named_unsafe(dmc_firmware_path, charp, 0400,
>-        "DMC firmware path to use instead of the default one. "
>-        "Use non-existent file to disable DMC and runtime PM.");
>-
> i915_param_named_unsafe(gsc_firmware_path, charp, 0400,
>         "GSC firmware path to use instead of the default one");
> 
>diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
>index 1315d7fac850..2eb3f2115ff2 100644
>--- a/drivers/gpu/drm/i915/i915_params.h
>+++ b/drivers/gpu/drm/i915/i915_params.h
>@@ -51,7 +51,6 @@ struct drm_printer;
>         param(int, guc_log_level, -1, 0400) \
>         param(char *, guc_firmware_path, NULL, 0400) \
>         param(char *, huc_firmware_path, NULL, 0400) \
>-        param(char *, dmc_firmware_path, NULL, 0400) \
>         param(char *, gsc_firmware_path, NULL, 0400) \
>         param(bool, memtest, false, 0400) \
>         param(int, mmio_debug, -IS_ENABLED(CONFIG_DRM_I915_DEBUG_MMIO), 0600) \
>diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>index 60ced5f90c2b..c70047762222 100644
>--- a/drivers/gpu/drm/xe/xe_device_types.h
>+++ b/drivers/gpu/drm/xe/xe_device_types.h
>@@ -519,9 +519,6 @@ struct xe_device {
>                 unsigned int czclk_freq;
>                 unsigned int fsb_freq, mem_freq, is_ddr3;
>         };
>-        struct {
>-                const char *dmc_firmware_path;
>-        } params;
> 
>         void *pxp;
> #endif
>-- 
>2.39.2
>

Re: [PATCH 1/5] drm/i915/dmc: handle request_firmware() errors separately

[Jani Nikula]

On Thu, 18 Apr 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> Quoting Jani Nikula (2024-04-18 11:39:50-03:00)
>>Clarify request_firmware() error handling. Don't proceed to trying to
>>parse non-existent firmware or check for payload when request_firmware()
>>failed to begin with. There's no reason to release_firmware() either
>>when request_firmware() failed.
>>
>>Also move the message about DMC firmware homepage here, as in other
>>cases the user probably has some firmware, although its parsing fails
>>for some reason.
>>
>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>---
>> drivers/gpu/drm/i915/display/intel_dmc.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>>index a34ff3383fd3..65880dea9c15 100644
>>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>>@@ -942,6 +942,15 @@ static void dmc_load_work_fn(struct work_struct *work)
>>                 }
>>         }
>> 
>>+        if (err) {
>>+                drm_notice(&i915->drm,
>>+                           "Failed to load DMC firmware %s (%pe). Disabling runtime power management.\n",
>>+                           dmc->fw_path, ERR_PTR(err));
>>+                drm_notice(&i915->drm, "DMC firmware homepage: %s",
>>+                           INTEL_DMC_FIRMWARE_URL);
>
> This could also be:
>
>     drm_notice(&i915->drm, "DMC firmware homepage: " INTEL_DMC_FIRMWARE_URL)

Although it currently doesn't, a URL could contain printf format
characters.

>
>>+                return;
>>+        }
>>+
>>         parse_dmc_fw(dmc, fw);
>
> Maybe also remove the now unnecessary NULL check for fw in
> parse_dmc_fw()?

Yeah, was ambivalent about it, could go either way.

>
>> 
>>         if (intel_dmc_has_payload(i915)) {
>>@@ -956,8 +965,6 @@ static void dmc_load_work_fn(struct work_struct *work)
>>                            "Failed to load DMC firmware %s."
>
> Should we tweak the message to differentiate from the previous one? At
> this point, we know the blob file exists, but there is a problem with
> its content.

That's done in the next patch. :)

>
> I think the patch looks good and to me all of the above suggestions are
> just that :-)  So, with or without them:
>
> Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

Thanks!

BR,
Jani.

>
> --
> Gustavo Sousa
>
>>                            " Disabling runtime power management.\n",
>>                            dmc->fw_path);
>>-                drm_notice(&i915->drm, "DMC firmware homepage: %s",
>>-                           INTEL_DMC_FIRMWARE_URL);
>>         }
>> 
>>         release_firmware(fw);
>>-- 
>>2.39.2
>>

-- 
Jani Nikula, Intel

Re: [PATCH 1/5] drm/i915/dmc: handle request_firmware() errors separately

[Gustavo Sousa]

Quoting Jani Nikula (2024-04-18 16:56:06-03:00)
>On Thu, 18 Apr 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> Quoting Jani Nikula (2024-04-18 11:39:50-03:00)
>>>Clarify request_firmware() error handling. Don't proceed to trying to
>>>parse non-existent firmware or check for payload when request_firmware()
>>>failed to begin with. There's no reason to release_firmware() either
>>>when request_firmware() failed.
>>>
>>>Also move the message about DMC firmware homepage here, as in other
>>>cases the user probably has some firmware, although its parsing fails
>>>for some reason.
>>>
>>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>---
>>> drivers/gpu/drm/i915/display/intel_dmc.c | 11 +++++++++--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>>>index a34ff3383fd3..65880dea9c15 100644
>>>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>>>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>>>@@ -942,6 +942,15 @@ static void dmc_load_work_fn(struct work_struct *work)
>>>                 }
>>>         }
>>> 
>>>+        if (err) {
>>>+                drm_notice(&i915->drm,
>>>+                           "Failed to load DMC firmware %s (%pe). Disabling runtime power management.\n",
>>>+                           dmc->fw_path, ERR_PTR(err));
>>>+                drm_notice(&i915->drm, "DMC firmware homepage: %s",
>>>+                           INTEL_DMC_FIRMWARE_URL);
>>
>> This could also be:
>>
>>     drm_notice(&i915->drm, "DMC firmware homepage: " INTEL_DMC_FIRMWARE_URL)
>
>Although it currently doesn't, a URL could contain printf format
>characters.

Good point.

>
>>
>>>+                return;
>>>+        }
>>>+
>>>         parse_dmc_fw(dmc, fw);
>>
>> Maybe also remove the now unnecessary NULL check for fw in
>> parse_dmc_fw()?
>
>Yeah, was ambivalent about it, could go either way.
>
>>
>>> 
>>>         if (intel_dmc_has_payload(i915)) {
>>>@@ -956,8 +965,6 @@ static void dmc_load_work_fn(struct work_struct *work)
>>>                            "Failed to load DMC firmware %s."
>>
>> Should we tweak the message to differentiate from the previous one? At
>> this point, we know the blob file exists, but there is a problem with
>> its content.
>
>That's done in the next patch. :)

Yeah, realized that later on. :-)

--
Gustavo Sousa

>
>>
>> I think the patch looks good and to me all of the above suggestions are
>> just that :-)  So, with or without them:
>>
>> Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
>
>Thanks!
>
>BR,
>Jani.
>
>>
>> --
>> Gustavo Sousa
>>
>>>                            " Disabling runtime power management.\n",
>>>                            dmc->fw_path);
>>>-                drm_notice(&i915->drm, "DMC firmware homepage: %s",
>>>-                           INTEL_DMC_FIRMWARE_URL);
>>>         }
>>> 
>>>         release_firmware(fw);
>>>-- 
>>>2.39.2
>>>
>
>-- 
>Jani Nikula, Intel

Re: [PATCH 2/5] drm/i915/dmc: improve firmware parse failure propagation

[Jani Nikula]

On Thu, 18 Apr 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> Quoting Jani Nikula (2024-04-18 11:39:51-03:00)
>>Return failures from parse_dmc_fw() instead of relying on
>>intel_dmc_has_payload(). Handle and error report them slightly better.
>>
>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>---
>> drivers/gpu/drm/i915/display/intel_dmc.c | 39 +++++++++++++-----------
>> 1 file changed, 22 insertions(+), 17 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>>index 65880dea9c15..ee5db1aafd50 100644
>>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>>@@ -853,7 +853,7 @@ static u32 parse_dmc_fw_css(struct intel_dmc *dmc,
>>         return sizeof(struct intel_css_header);
>> }
>> 
>>-static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
>>+static int parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
>> {
>>         struct drm_i915_private *i915 = dmc->i915;
>>         struct intel_css_header *css_header;
>>@@ -866,13 +866,13 @@ static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
>>         u32 r, offset;
>> 
>>         if (!fw)
>>-                return;
>>+                return -EINVAL;
>> 
>>         /* Extract CSS Header information */
>>         css_header = (struct intel_css_header *)fw->data;
>>         r = parse_dmc_fw_css(dmc, css_header, fw->size);
>>         if (!r)
>>-                return;
>>+                return -EINVAL;
>> 
>>         readcount += r;
>> 
>>@@ -880,7 +880,7 @@ static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
>>         package_header = (struct intel_package_header *)&fw->data[readcount];
>>         r = parse_dmc_fw_package(dmc, package_header, si, fw->size - readcount);
>>         if (!r)
>>-                return;
>>+                return -EINVAL;
>> 
>>         readcount += r;
>> 
>>@@ -897,6 +897,11 @@ static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
>>                 dmc_header = (struct intel_dmc_header_base *)&fw->data[offset];
>>                 parse_dmc_fw_header(dmc, dmc_header, fw->size - offset, dmc_id);
>>         }
>>+
>>+        if (!intel_dmc_has_payload(i915))
>>+                return -ENOENT;
>
> This function and also the parsing helpers (parse_dmc_fw_*) already have
> the pattern of providing error messages for issues found. We could maybe
> have parse_dmc_fw() simply returning -1 for errors here.

I've become increasingly opposed to the magic -1 error return in the
kernel. Basically all negative error codes have a meaning, and -1 is
-EPERM. (I even have a branch converting a bunch of "return -1" to
something more meaningful.)

But I guess -1 wasn't really the main point about your comment anyway.

> For this specific condition (!intel_dmc_has_payload(i915)), we could
> complain that there the main DMC program was not found before returning.

Agreed.

> I think ENOENT might confuse users.

So would you rather just skip printing the error code returned by
parse_dmc_fw()? I take it this was really the main point?

BR,
Jani.


>
> --
> Gustavo Sousa
>
>>+
>>+        return 0;
>> }
>> 
>> static void intel_dmc_runtime_pm_get(struct drm_i915_private *i915)
>>@@ -951,22 +956,22 @@ static void dmc_load_work_fn(struct work_struct *work)
>>                 return;
>>         }
>> 
>>-        parse_dmc_fw(dmc, fw);
>>-
>>-        if (intel_dmc_has_payload(i915)) {
>>-                intel_dmc_load_program(i915);
>>-                intel_dmc_runtime_pm_put(i915);
>>-
>>-                drm_info(&i915->drm, "Finished loading DMC firmware %s (v%u.%u)\n",
>>-                         dmc->fw_path, DMC_VERSION_MAJOR(dmc->version),
>>-                         DMC_VERSION_MINOR(dmc->version));
>>-        } else {
>>+        err = parse_dmc_fw(dmc, fw);
>>+        if (err) {
>>                 drm_notice(&i915->drm,
>>-                           "Failed to load DMC firmware %s."
>>-                           " Disabling runtime power management.\n",
>>-                           dmc->fw_path);
>>+                           "Failed to parse DMC firmware %s (%pe). Disabling runtime power management.\n",
>>+                           dmc->fw_path, ERR_PTR(err));
>>+                goto out;
>>         }
>> 
>>+        intel_dmc_load_program(i915);
>>+        intel_dmc_runtime_pm_put(i915);
>>+
>>+        drm_info(&i915->drm, "Finished loading DMC firmware %s (v%u.%u)\n",
>>+                 dmc->fw_path, DMC_VERSION_MAJOR(dmc->version),
>>+                 DMC_VERSION_MINOR(dmc->version));
>>+
>>+out:
>>         release_firmware(fw);
>> }
>> 
>>-- 
>>2.39.2
>>

-- 
Jani Nikula, Intel

Re: [PATCH 4/5] drm/i915/dmc: change meaning of dmc_firmware_path="" module param

[Jani Nikula]

On Thu, 18 Apr 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> Quoting Jani Nikula (2024-04-18 11:39:53-03:00)
>>The distinction between the dmc_firmware_path module param being NULL
>>and the empty string "" is problematic. It's not possible to set the
>>parameter back to NULL via sysfs or debugfs. Remove the distinction, and
>>consider NULL and the empty string to be the same thing, and use the
>>platform default for them.
>>
>>This removes the possibility to disable DMC (and runtime PM) via
>>i915.dmc_firmware_path="". Instead, you will need to specify a
>>non-existent file or a file that will not parse correctly.
>>
>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>---
>> drivers/gpu/drm/i915/display/intel_dmc.c | 20 ++++++++++----------
>> drivers/gpu/drm/i915/i915_params.c       |  3 ++-
>> 2 files changed, 12 insertions(+), 11 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>>index 740c05ce83cc..3e510c2be1eb 100644
>>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>>@@ -73,6 +73,13 @@ static struct intel_dmc *i915_to_dmc(struct drm_i915_private *i915)
>>         return i915->display.dmc.dmc;
>> }
>> 
>>+static const char *dmc_firmware_param(struct drm_i915_private *i915)
>>+{
>>+        const char *p = i915->params.dmc_firmware_path;
>>+
>>+        return p && *p ? p : NULL;
>>+}
>>+
>> #define DMC_VERSION(major, minor)        ((major) << 16 | (minor))
>> #define DMC_VERSION_MAJOR(version)        ((version) >> 16)
>> #define DMC_VERSION_MINOR(version)        ((version) & 0xffff)
>>@@ -989,7 +996,7 @@ static void dmc_load_work_fn(struct work_struct *work)
>> 
>>         err = request_firmware(&fw, dmc->fw_path, i915->drm.dev);
>> 
>>-        if (err == -ENOENT && !i915->params.dmc_firmware_path) {
>>+        if (err == -ENOENT && !dmc_firmware_param(i915)) {
>>                 fallback_path = dmc_fallback_path(i915);
>>                 if (fallback_path) {
>>                         drm_dbg_kms(&i915->drm, "%s not found, falling back to %s\n",
>>@@ -1062,15 +1069,8 @@ void intel_dmc_init(struct drm_i915_private *i915)
>> 
>>         dmc->fw_path = dmc_firmware_default(i915, &dmc->max_fw_size);
>> 
>>-        if (i915->params.dmc_firmware_path) {
>>-                if (strlen(i915->params.dmc_firmware_path) == 0) {
>>-                        drm_info(&i915->drm,
>>-                                 "Disabling DMC firmware and runtime PM\n");
>>-                        goto out;
>>-                }
>>-
>>-                dmc->fw_path = i915->params.dmc_firmware_path;
>>-        }
>>+        if (dmc_firmware_param(i915))
>>+                dmc->fw_path = dmc_firmware_param(i915);
>> 
>>         if (!dmc->fw_path) {
>>                 drm_dbg_kms(&i915->drm,
>>diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>>index de43048543e8..9e7f2a9f6287 100644
>>--- a/drivers/gpu/drm/i915/i915_params.c
>>+++ b/drivers/gpu/drm/i915/i915_params.c
>>@@ -109,7 +109,8 @@ i915_param_named_unsafe(huc_firmware_path, charp, 0400,
>>         "HuC firmware path to use instead of the default one");
>> 
>> i915_param_named_unsafe(dmc_firmware_path, charp, 0400,
>>-        "DMC firmware path to use instead of the default one");
>>+        "DMC firmware path to use instead of the default one. "
>>+        "Use non-existent file to disable DMC and runtime PM.");
>
> Okay. But is it too bad to have a magic string for it? The up side is
> that there wouldn't be error messages in the log if we had such option.

Another upside is that we could also just skip requesting the firmware
altogether, similar to what we have currently.

It's just a small naming problem... what should the magic string for
"disabled" be? Like, yes, that's the obvious choice right there, but
it's also a valid filename. Who am I to say how people should name their
firmware blobs. :)

"/dev/null"?


BR,
Jani.



>
> --
> Gustavo Sousa
>
>> 
>> i915_param_named_unsafe(gsc_firmware_path, charp, 0400,
>>         "GSC firmware path to use instead of the default one");
>>-- 
>>2.39.2
>>

-- 
Jani Nikula, Intel

Re: [PATCH 2/5] drm/i915/dmc: improve firmware parse failure propagation

[Gustavo Sousa]

Quoting Jani Nikula (2024-04-18 17:03:22-03:00)
>On Thu, 18 Apr 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> Quoting Jani Nikula (2024-04-18 11:39:51-03:00)
>>>Return failures from parse_dmc_fw() instead of relying on
>>>intel_dmc_has_payload(). Handle and error report them slightly better.
>>>
>>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>---
>>> drivers/gpu/drm/i915/display/intel_dmc.c | 39 +++++++++++++-----------
>>> 1 file changed, 22 insertions(+), 17 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>>>index 65880dea9c15..ee5db1aafd50 100644
>>>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>>>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>>>@@ -853,7 +853,7 @@ static u32 parse_dmc_fw_css(struct intel_dmc *dmc,
>>>         return sizeof(struct intel_css_header);
>>> }
>>> 
>>>-static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
>>>+static int parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
>>> {
>>>         struct drm_i915_private *i915 = dmc->i915;
>>>         struct intel_css_header *css_header;
>>>@@ -866,13 +866,13 @@ static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
>>>         u32 r, offset;
>>> 
>>>         if (!fw)
>>>-                return;
>>>+                return -EINVAL;
>>> 
>>>         /* Extract CSS Header information */
>>>         css_header = (struct intel_css_header *)fw->data;
>>>         r = parse_dmc_fw_css(dmc, css_header, fw->size);
>>>         if (!r)
>>>-                return;
>>>+                return -EINVAL;
>>> 
>>>         readcount += r;
>>> 
>>>@@ -880,7 +880,7 @@ static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
>>>         package_header = (struct intel_package_header *)&fw->data[readcount];
>>>         r = parse_dmc_fw_package(dmc, package_header, si, fw->size - readcount);
>>>         if (!r)
>>>-                return;
>>>+                return -EINVAL;
>>> 
>>>         readcount += r;
>>> 
>>>@@ -897,6 +897,11 @@ static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
>>>                 dmc_header = (struct intel_dmc_header_base *)&fw->data[offset];
>>>                 parse_dmc_fw_header(dmc, dmc_header, fw->size - offset, dmc_id);
>>>         }
>>>+
>>>+        if (!intel_dmc_has_payload(i915))
>>>+                return -ENOENT;
>>
>> This function and also the parsing helpers (parse_dmc_fw_*) already have
>> the pattern of providing error messages for issues found. We could maybe
>> have parse_dmc_fw() simply returning -1 for errors here.
>
>I've become increasingly opposed to the magic -1 error return in the
>kernel. Basically all negative error codes have a meaning, and -1 is
>-EPERM. (I even have a branch converting a bunch of "return -1" to
>something more meaningful.)

Oh! I didn't realize that. I've always seen -1 as some generic error
indication (i.e. just something != 0). Thanks!

Well, -EINVAL indeed seems more appropriate then.

>
>But I guess -1 wasn't really the main point about your comment anyway.

Correct.

>
>> For this specific condition (!intel_dmc_has_payload(i915)), we could
>> complain that there the main DMC program was not found before returning.
>
>Agreed.
>
>> I think ENOENT might confuse users.
>
>So would you rather just skip printing the error code returned by
>parse_dmc_fw()? I take it this was really the main point?

Yep, that was my point initially. Specific messages are already printed
during the parsing. So, I thought just a generic message at the end
would suffice (i.e. just removing the " (%pe)" portion of it).

And I was worried that ENOENT would confuse users. However, now I
realize that "%pe" actually simply shows the symbolic error name (e.g.
"ENOENT") instead of the "traditional" phrases for the error (e.g. "No
such file or directory"). I should've checked that earlier... So, I take
this part back now. Sorry for the noise.

With only the addition of the specific error message for
(!intel_dmc_has_payload(i915)):

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

>
>BR,
>Jani.
>
>
>>
>> --
>> Gustavo Sousa
>>
>>>+
>>>+        return 0;
>>> }
>>> 
>>> static void intel_dmc_runtime_pm_get(struct drm_i915_private *i915)
>>>@@ -951,22 +956,22 @@ static void dmc_load_work_fn(struct work_struct *work)
>>>                 return;
>>>         }
>>> 
>>>-        parse_dmc_fw(dmc, fw);
>>>-
>>>-        if (intel_dmc_has_payload(i915)) {
>>>-                intel_dmc_load_program(i915);
>>>-                intel_dmc_runtime_pm_put(i915);
>>>-
>>>-                drm_info(&i915->drm, "Finished loading DMC firmware %s (v%u.%u)\n",
>>>-                         dmc->fw_path, DMC_VERSION_MAJOR(dmc->version),
>>>-                         DMC_VERSION_MINOR(dmc->version));
>>>-        } else {
>>>+        err = parse_dmc_fw(dmc, fw);
>>>+        if (err) {
>>>                 drm_notice(&i915->drm,
>>>-                           "Failed to load DMC firmware %s."
>>>-                           " Disabling runtime power management.\n",
>>>-                           dmc->fw_path);
>>>+                           "Failed to parse DMC firmware %s (%pe). Disabling runtime power management.\n",
>>>+                           dmc->fw_path, ERR_PTR(err));
>>>+                goto out;
>>>         }
>>> 
>>>+        intel_dmc_load_program(i915);
>>>+        intel_dmc_runtime_pm_put(i915);
>>>+
>>>+        drm_info(&i915->drm, "Finished loading DMC firmware %s (v%u.%u)\n",
>>>+                 dmc->fw_path, DMC_VERSION_MAJOR(dmc->version),
>>>+                 DMC_VERSION_MINOR(dmc->version));
>>>+
>>>+out:
>>>         release_firmware(fw);
>>> }
>>> 
>>>-- 
>>>2.39.2
>>>
>
>-- 
>Jani Nikula, Intel

Re: [PATCH 4/5] drm/i915/dmc: change meaning of dmc_firmware_path="" module param

[Gustavo Sousa]

Quoting Jani Nikula (2024-04-18 17:09:04-03:00)
>On Thu, 18 Apr 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> Quoting Jani Nikula (2024-04-18 11:39:53-03:00)
>>>The distinction between the dmc_firmware_path module param being NULL
>>>and the empty string "" is problematic. It's not possible to set the
>>>parameter back to NULL via sysfs or debugfs. Remove the distinction, and
>>>consider NULL and the empty string to be the same thing, and use the
>>>platform default for them.
>>>
>>>This removes the possibility to disable DMC (and runtime PM) via
>>>i915.dmc_firmware_path="". Instead, you will need to specify a
>>>non-existent file or a file that will not parse correctly.
>>>
>>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>---
>>> drivers/gpu/drm/i915/display/intel_dmc.c | 20 ++++++++++----------
>>> drivers/gpu/drm/i915/i915_params.c       |  3 ++-
>>> 2 files changed, 12 insertions(+), 11 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>>>index 740c05ce83cc..3e510c2be1eb 100644
>>>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>>>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>>>@@ -73,6 +73,13 @@ static struct intel_dmc *i915_to_dmc(struct drm_i915_private *i915)
>>>         return i915->display.dmc.dmc;
>>> }
>>> 
>>>+static const char *dmc_firmware_param(struct drm_i915_private *i915)
>>>+{
>>>+        const char *p = i915->params.dmc_firmware_path;
>>>+
>>>+        return p && *p ? p : NULL;
>>>+}
>>>+
>>> #define DMC_VERSION(major, minor)        ((major) << 16 | (minor))
>>> #define DMC_VERSION_MAJOR(version)        ((version) >> 16)
>>> #define DMC_VERSION_MINOR(version)        ((version) & 0xffff)
>>>@@ -989,7 +996,7 @@ static void dmc_load_work_fn(struct work_struct *work)
>>> 
>>>         err = request_firmware(&fw, dmc->fw_path, i915->drm.dev);
>>> 
>>>-        if (err == -ENOENT && !i915->params.dmc_firmware_path) {
>>>+        if (err == -ENOENT && !dmc_firmware_param(i915)) {
>>>                 fallback_path = dmc_fallback_path(i915);
>>>                 if (fallback_path) {
>>>                         drm_dbg_kms(&i915->drm, "%s not found, falling back to %s\n",
>>>@@ -1062,15 +1069,8 @@ void intel_dmc_init(struct drm_i915_private *i915)
>>> 
>>>         dmc->fw_path = dmc_firmware_default(i915, &dmc->max_fw_size);
>>> 
>>>-        if (i915->params.dmc_firmware_path) {
>>>-                if (strlen(i915->params.dmc_firmware_path) == 0) {
>>>-                        drm_info(&i915->drm,
>>>-                                 "Disabling DMC firmware and runtime PM\n");
>>>-                        goto out;
>>>-                }
>>>-
>>>-                dmc->fw_path = i915->params.dmc_firmware_path;
>>>-        }
>>>+        if (dmc_firmware_param(i915))
>>>+                dmc->fw_path = dmc_firmware_param(i915);
>>> 
>>>         if (!dmc->fw_path) {
>>>                 drm_dbg_kms(&i915->drm,
>>>diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>>>index de43048543e8..9e7f2a9f6287 100644
>>>--- a/drivers/gpu/drm/i915/i915_params.c
>>>+++ b/drivers/gpu/drm/i915/i915_params.c
>>>@@ -109,7 +109,8 @@ i915_param_named_unsafe(huc_firmware_path, charp, 0400,
>>>         "HuC firmware path to use instead of the default one");
>>> 
>>> i915_param_named_unsafe(dmc_firmware_path, charp, 0400,
>>>-        "DMC firmware path to use instead of the default one");
>>>+        "DMC firmware path to use instead of the default one. "
>>>+        "Use non-existent file to disable DMC and runtime PM.");
>>
>> Okay. But is it too bad to have a magic string for it? The up side is
>> that there wouldn't be error messages in the log if we had such option.
>
>Another upside is that we could also just skip requesting the firmware
>altogether, similar to what we have currently.
>
>It's just a small naming problem... what should the magic string for
>"disabled" be? Like, yes, that's the obvious choice right there, but
>it's also a valid filename. Who am I to say how people should name their
>firmware blobs. :)
>
>"/dev/null"?

I like this one!

--
Gustavo Sousa

>
>
>BR,
>Jani.
>
>
>
>>
>> --
>> Gustavo Sousa
>>
>>> 
>>> i915_param_named_unsafe(gsc_firmware_path, charp, 0400,
>>>         "GSC firmware path to use instead of the default one");
>>>-- 
>>>2.39.2
>>>
>
>-- 
>Jani Nikula, Intel

Re: [PATCH 4/5] drm/i915/dmc: change meaning of dmc_firmware_path="" module param

[Lucas De Marchi]

On Thu, Apr 18, 2024 at 05:44:13PM GMT, Gustavo Sousa wrote:
>Quoting Jani Nikula (2024-04-18 17:09:04-03:00)
>>On Thu, 18 Apr 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>>> Quoting Jani Nikula (2024-04-18 11:39:53-03:00)
>>>>The distinction between the dmc_firmware_path module param being NULL
>>>>and the empty string "" is problematic. It's not possible to set the
>>>>parameter back to NULL via sysfs or debugfs. Remove the distinction, and
>>>>consider NULL and the empty string to be the same thing, and use the
>>>>platform default for them.
>>>>
>>>>This removes the possibility to disable DMC (and runtime PM) via
>>>>i915.dmc_firmware_path="". Instead, you will need to specify a
>>>>non-existent file or a file that will not parse correctly.
>>>>
>>>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>>---
>>>> drivers/gpu/drm/i915/display/intel_dmc.c | 20 ++++++++++----------
>>>> drivers/gpu/drm/i915/i915_params.c       |  3 ++-
>>>> 2 files changed, 12 insertions(+), 11 deletions(-)
>>>>
>>>>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>>>>index 740c05ce83cc..3e510c2be1eb 100644
>>>>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>>>>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>>>>@@ -73,6 +73,13 @@ static struct intel_dmc *i915_to_dmc(struct drm_i915_private *i915)
>>>>         return i915->display.dmc.dmc;
>>>> }
>>>>
>>>>+static const char *dmc_firmware_param(struct drm_i915_private *i915)
>>>>+{
>>>>+        const char *p = i915->params.dmc_firmware_path;
>>>>+
>>>>+        return p && *p ? p : NULL;
>>>>+}
>>>>+
>>>> #define DMC_VERSION(major, minor)        ((major) << 16 | (minor))
>>>> #define DMC_VERSION_MAJOR(version)        ((version) >> 16)
>>>> #define DMC_VERSION_MINOR(version)        ((version) & 0xffff)
>>>>@@ -989,7 +996,7 @@ static void dmc_load_work_fn(struct work_struct *work)
>>>>
>>>>         err = request_firmware(&fw, dmc->fw_path, i915->drm.dev);
>>>>
>>>>-        if (err == -ENOENT && !i915->params.dmc_firmware_path) {
>>>>+        if (err == -ENOENT && !dmc_firmware_param(i915)) {
>>>>                 fallback_path = dmc_fallback_path(i915);
>>>>                 if (fallback_path) {
>>>>                         drm_dbg_kms(&i915->drm, "%s not found, falling back to %s\n",
>>>>@@ -1062,15 +1069,8 @@ void intel_dmc_init(struct drm_i915_private *i915)
>>>>
>>>>         dmc->fw_path = dmc_firmware_default(i915, &dmc->max_fw_size);
>>>>
>>>>-        if (i915->params.dmc_firmware_path) {
>>>>-                if (strlen(i915->params.dmc_firmware_path) == 0) {
>>>>-                        drm_info(&i915->drm,
>>>>-                                 "Disabling DMC firmware and runtime PM\n");
>>>>-                        goto out;
>>>>-                }
>>>>-
>>>>-                dmc->fw_path = i915->params.dmc_firmware_path;
>>>>-        }
>>>>+        if (dmc_firmware_param(i915))
>>>>+                dmc->fw_path = dmc_firmware_param(i915);
>>>>
>>>>         if (!dmc->fw_path) {
>>>>                 drm_dbg_kms(&i915->drm,
>>>>diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>>>>index de43048543e8..9e7f2a9f6287 100644
>>>>--- a/drivers/gpu/drm/i915/i915_params.c
>>>>+++ b/drivers/gpu/drm/i915/i915_params.c
>>>>@@ -109,7 +109,8 @@ i915_param_named_unsafe(huc_firmware_path, charp, 0400,
>>>>         "HuC firmware path to use instead of the default one");
>>>>
>>>> i915_param_named_unsafe(dmc_firmware_path, charp, 0400,
>>>>-        "DMC firmware path to use instead of the default one");
>>>>+        "DMC firmware path to use instead of the default one. "
>>>>+        "Use non-existent file to disable DMC and runtime PM.");
>>>
>>> Okay. But is it too bad to have a magic string for it? The up side is
>>> that there wouldn't be error messages in the log if we had such option.
>>
>>Another upside is that we could also just skip requesting the firmware
>>altogether, similar to what we have currently.
>>
>>It's just a small naming problem... what should the magic string for
>>"disabled" be? Like, yes, that's the obvious choice right there, but
>>it's also a valid filename. Who am I to say how people should name their
>>firmware blobs. :)
>>
>>"/dev/null"?
>
>I like this one!

+1

Lucas De Marchi