| Message ID | 08137052f324f47f9ca9aadfc5bdf915a17bfd06.1713450693.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dmc: firmware path handling changes | expand |
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@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. For this specific condition (!intel_dmc_has_payload(i915)), we could complain that there the main DMC program was not found before returning. I think ENOENT might confuse users. -- 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 >
On Thu, 18 Apr 2024, Gustavo Sousa <gustavo.sousa@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@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 >>
Quoting Jani Nikula (2024-04-18 17:03:22-03:00) >On Thu, 18 Apr 2024, Gustavo Sousa <gustavo.sousa@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@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@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
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; + + 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); }
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@intel.com> --- drivers/gpu/drm/i915/display/intel_dmc.c | 39 +++++++++++++----------- 1 file changed, 22 insertions(+), 17 deletions(-)