On Thu, 18 Apr 2024, Gustavo Sousa <gustavo.so...@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.nik...@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.) But I guess -1 wasn't really the main point about your comment anyway. > 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? 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