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

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

[Jani Nikula]

v2 of https://lore.kernel.org/r/cover.1713450693.git.jani.nikula@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 how to disable DMC firmware using 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      | 175 +++++++++++-------
 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, 112 insertions(+), 75 deletions(-)

-- 
2.39.2

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

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
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 3697a02b51b6..8f2a0afe48ba 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 v2 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,
including a new error message for when the firmware does not contain the
main program.

v2: Print specific error message for payload not found (Gustavo)

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc.c | 41 ++++++++++++++----------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index 8f2a0afe48ba..2e06a39579df 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,13 @@ 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)) {
+		drm_err(&i915->drm, "DMC firmware main program not found\n");
+		return -ENOENT;
+	}
+
+	return 0;
 }
 
 static void intel_dmc_runtime_pm_get(struct drm_i915_private *i915)
@@ -951,22 +958,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 v2 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.

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
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 2e06a39579df..a2027c6cf4f1 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
@@ -1009,48 +1062,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 v2 4/5] drm/i915/dmc: change how to disable DMC firmware using 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, use "/dev/null" as the magic
firmware path to skip DMC firmware loading and disable runtime PM.

v2: Add support for i915.dmc_firmware_path="/dev/null" (Gustavo)

Cc: Gustavo Sousa <gustavo.sousa@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc.c | 31 +++++++++++++++++-------
 drivers/gpu/drm/i915/i915_params.c       |  3 ++-
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index a2027c6cf4f1..7dabfaa636e4 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -73,6 +73,21 @@ 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;
+}
+
+static bool dmc_firmware_param_disabled(struct drm_i915_private *i915)
+{
+	const char *p = dmc_firmware_param(i915);
+
+	/* Magic path to indicate disabled */
+	return p && !strcmp(p, "/dev/null");
+}
+
 #define DMC_VERSION(major, minor)	((major) << 16 | (minor))
 #define DMC_VERSION_MAJOR(version)	((version) >> 16)
 #define DMC_VERSION_MINOR(version)	((version) & 0xffff)
@@ -991,7 +1006,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",
@@ -1064,16 +1079,14 @@ 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_disabled(i915)) {
+		drm_info(&i915->drm, "Disabling DMC firmware and runtime PM\n");
+		goto out;
 	}
 
+	if (dmc_firmware_param(i915))
+		dmc->fw_path = dmc_firmware_param(i915);
+
 	if (!dmc->fw_path) {
 		drm_dbg_kms(&i915->drm,
 			    "No known DMC firmware for platform, disabling runtime PM\n");
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index de43048543e8..edfd1f54a882 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 /dev/null 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 v2 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

Reviewed-by: Gustavo Sousa <gustavo.sousa@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..1799a6643128 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 /dev/null 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 7dabfaa636e4..36c361cf72fc 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 edfd1f54a882..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 /dev/null 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 (rev2)

[Patchwork]

== Series Details ==

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

== Summary ==

Error: dim checkpatch failed
3d1c3762fcfb drm/i915/dmc: handle request_firmware() errors separately
73f10c70a834 drm/i915/dmc: improve firmware parse failure propagation
d8a2394657a8 drm/i915/dmc: split out per-platform firmware path selection
f722c67cc401 drm/i915/dmc: change how to disable DMC firmware using module param
7186dc6862f7 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 (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/dmc: firmware path handling changes (rev2)
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 (rev2)

[Patchwork]

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

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_14611 -> Patchwork_132609v2
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_132609v2 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_132609v2, 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_132609v2/index.html

Participating hosts (34 -> 30)
------------------------------

  Missing    (4): fi-kbl-7567u bat-jsl-1 fi-apl-guc fi-kbl-8809g 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@dmabuf:
    - bat-arls-2:         NOTRUN -> [ABORT][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v2/bat-arls-2/igt@i915_selftest@live@dmabuf.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_lmem_swapping@verify-random:
    - bat-arls-2:         NOTRUN -> [SKIP][2] ([i915#10213]) +3 other tests skip
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v2/bat-arls-2/igt@gem_lmem_swapping@verify-random.html

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

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

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

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

  * igt@i915_pm_rps@basic-api:
    - bat-arls-2:         NOTRUN -> [SKIP][7] ([i915#10209])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v2/bat-arls-2/igt@i915_pm_rps@basic-api.html

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

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

  * igt@kms_dsc@dsc-basic:
    - bat-arls-2:         NOTRUN -> [SKIP][10] ([i915#9886])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v2/bat-arls-2/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-arls-2:         NOTRUN -> [SKIP][11] ([i915#10207])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v2/bat-arls-2/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_psr@psr-primary-mmap-gtt@edp-1:
    - bat-arls-2:         NOTRUN -> [SKIP][12] ([i915#10196] / [i915#4077] / [i915#9688])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v2/bat-arls-2/igt@kms_psr@psr-primary-mmap-gtt@edp-1.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-arls-2:         NOTRUN -> [SKIP][13] ([i915#10208] / [i915#8809])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v2/bat-arls-2/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-mmap:
    - bat-arls-2:         NOTRUN -> [SKIP][14] ([i915#10196] / [i915#3708] / [i915#4077]) +1 other test skip
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v2/bat-arls-2/igt@prime_vgem@basic-fence-mmap.html

  * igt@prime_vgem@basic-fence-read:
    - bat-arls-2:         NOTRUN -> [SKIP][15] ([i915#10212] / [i915#3708])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v2/bat-arls-2/igt@prime_vgem@basic-fence-read.html

  * igt@prime_vgem@basic-read:
    - bat-arls-2:         NOTRUN -> [SKIP][16] ([i915#10214] / [i915#3708])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v2/bat-arls-2/igt@prime_vgem@basic-read.html

  * igt@prime_vgem@basic-write:
    - bat-arls-2:         NOTRUN -> [SKIP][17] ([i915#10216] / [i915#3708])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v2/bat-arls-2/igt@prime_vgem@basic-write.html

  
#### Possible fixes ####

  * igt@gem_exec_create@basic@smem:
    - bat-arls-2:         [ABORT][18] -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14611/bat-arls-2/igt@gem_exec_create@basic@smem.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132609v2/bat-arls-2/igt@gem_exec_create@basic@smem.html

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

  [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#10207]: https://gitlab.freedesktop.org/drm/intel/issues/10207
  [i915#10208]: https://gitlab.freedesktop.org/drm/intel/issues/10208
  [i915#10209]: https://gitlab.freedesktop.org/drm/intel/issues/10209
  [i915#10211]: https://gitlab.freedesktop.org/drm/intel/issues/10211
  [i915#10212]: https://gitlab.freedesktop.org/drm/intel/issues/10212
  [i915#10213]: https://gitlab.freedesktop.org/drm/intel/issues/10213
  [i915#10214]: https://gitlab.freedesktop.org/drm/intel/issues/10214
  [i915#10216]: https://gitlab.freedesktop.org/drm/intel/issues/10216
  [i915#10435]: https://gitlab.freedesktop.org/drm/intel/issues/10435
  [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#8809]: https://gitlab.freedesktop.org/drm/intel/issues/8809
  [i915#9688]: https://gitlab.freedesktop.org/drm/intel/issues/9688
  [i915#9886]: https://gitlab.freedesktop.org/drm/intel/issues/9886


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

  * Linux: CI_DRM_14611 -> Patchwork_132609v2

  CI-20190529: 20190529
  CI_DRM_14611: d02ac9d1c1a99eac3bb111d443de62d7286f7708 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7814: 7814
  Patchwork_132609v2: d02ac9d1c1a99eac3bb111d443de62d7286f7708 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

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

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

Re: [PATCH v2 4/5] drm/i915/dmc: change how to disable DMC firmware using module param

[Gustavo Sousa]

Quoting Jani Nikula (2024-04-19 06:41:57-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, use "/dev/null" as the magic
>firmware path to skip DMC firmware loading and disable runtime PM.
>
>v2: Add support for i915.dmc_firmware_path="/dev/null" (Gustavo)
>
>Cc: Gustavo Sousa <gustavo.sousa@intel.com>
>Cc: Lucas De Marchi <lucas.demarchi@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_dmc.c | 31 +++++++++++++++++-------
> drivers/gpu/drm/i915/i915_params.c       |  3 ++-
> 2 files changed, 24 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>index a2027c6cf4f1..7dabfaa636e4 100644
>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>@@ -73,6 +73,21 @@ 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;
>+}
>+
>+static bool dmc_firmware_param_disabled(struct drm_i915_private *i915)
>+{
>+        const char *p = dmc_firmware_param(i915);
>+
>+        /* Magic path to indicate disabled */
>+        return p && !strcmp(p, "/dev/null");
>+}
>+
> #define DMC_VERSION(major, minor)        ((major) << 16 | (minor))
> #define DMC_VERSION_MAJOR(version)        ((version) >> 16)
> #define DMC_VERSION_MINOR(version)        ((version) & 0xffff)
>@@ -991,7 +1006,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",
>@@ -1064,16 +1079,14 @@ 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_disabled(i915)) {
>+                drm_info(&i915->drm, "Disabling DMC firmware and runtime PM\n");
>+                goto out;
>         }
> 
>+        if (dmc_firmware_param(i915))
>+                dmc->fw_path = dmc_firmware_param(i915);
>+
>         if (!dmc->fw_path) {
>                 drm_dbg_kms(&i915->drm,
>                             "No known DMC firmware for platform, disabling runtime PM\n");
>diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>index de43048543e8..edfd1f54a882 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 /dev/null 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
>

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

[Lucas De Marchi]

On Fri, Apr 19, 2024 at 12:41:53PM GMT, Jani Nikula wrote:
>v2 of https://lore.kernel.org/r/cover.1713450693.git.jani.nikula@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 how to disable DMC firmware using 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      | 175 +++++++++++-------
> 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 -

Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

> 6 files changed, 112 insertions(+), 75 deletions(-)
>
>-- 
>2.39.2
>

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

[Jani Nikula]

On Fri, 19 Apr 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Fri, Apr 19, 2024 at 12:41:53PM GMT, Jani Nikula wrote:
>>v2 of https://lore.kernel.org/r/cover.1713450693.git.jani.nikula@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 how to disable DMC firmware using 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      | 175 +++++++++++-------
>> 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 -
>
> Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>

Thanks for the reviews and acks, pushed to drm-intel-next.

BR,
Jani.



>
> thanks
> Lucas De Marchi
>
>> 6 files changed, 112 insertions(+), 75 deletions(-)
>>
>>-- 
>>2.39.2
>>

-- 
Jani Nikula, Intel