[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 at 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 at 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