Quoting Jani Nikula (2024-04-18 17:09:04-03:00) >On Thu, 18 Apr 2024, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> 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@xxxxxxxxx> >>>--- >>> 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