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