diff mbox series

[2/5] drm/i915/dmc: improve firmware parse failure propagation

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

Commit Message

Jani Nikula April 18, 2024, 2:39 p.m. UTC
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(-)

Comments

Gustavo Sousa April 18, 2024, 6:53 p.m. UTC | #1
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
>
Jani Nikula April 18, 2024, 8:03 p.m. UTC | #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
>>
Gustavo Sousa April 18, 2024, 8:40 p.m. UTC | #3
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 mbox series

Patch

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);
 }