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