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.
--
Gustavo Sousa
>
> i915_param_named_unsafe(gsc_firmware_path, charp, 0400,
> "GSC firmware path to use instead of the default one");
>--
>2.39.2
>
