diff mbox series

[1/5] drm/i915/dmc: handle request_firmware() errors separately

Message ID 725c3580aae52bf34f32b93275cce6208dfd42b7.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
Clarify request_firmware() error handling. Don't proceed to trying to
parse non-existent firmware or check for payload when request_firmware()
failed to begin with. There's no reason to release_firmware() either
when request_firmware() failed.

Also move the message about DMC firmware homepage here, as in other
cases the user probably has some firmware, although its parsing fails
for some reason.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Gustavo Sousa April 18, 2024, 6:30 p.m. UTC | #1
Quoting Jani Nikula (2024-04-18 11:39:50-03:00)
>Clarify request_firmware() error handling. Don't proceed to trying to
>parse non-existent firmware or check for payload when request_firmware()
>failed to begin with. There's no reason to release_firmware() either
>when request_firmware() failed.
>
>Also move the message about DMC firmware homepage here, as in other
>cases the user probably has some firmware, although its parsing fails
>for some reason.
>
>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>---
> drivers/gpu/drm/i915/display/intel_dmc.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>index a34ff3383fd3..65880dea9c15 100644
>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>@@ -942,6 +942,15 @@ static void dmc_load_work_fn(struct work_struct *work)
>                 }
>         }
> 
>+        if (err) {
>+                drm_notice(&i915->drm,
>+                           "Failed to load DMC firmware %s (%pe). Disabling runtime power management.\n",
>+                           dmc->fw_path, ERR_PTR(err));
>+                drm_notice(&i915->drm, "DMC firmware homepage: %s",
>+                           INTEL_DMC_FIRMWARE_URL);

This could also be:

    drm_notice(&i915->drm, "DMC firmware homepage: " INTEL_DMC_FIRMWARE_URL)

>+                return;
>+        }
>+
>         parse_dmc_fw(dmc, fw);

Maybe also remove the now unnecessary NULL check for fw in
parse_dmc_fw()?

> 
>         if (intel_dmc_has_payload(i915)) {
>@@ -956,8 +965,6 @@ static void dmc_load_work_fn(struct work_struct *work)
>                            "Failed to load DMC firmware %s."

Should we tweak the message to differentiate from the previous one? At
this point, we know the blob file exists, but there is a problem with
its content.

I think the patch looks good and to me all of the above suggestions are
just that :-)  So, with or without them:

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

--
Gustavo Sousa

>                            " Disabling runtime power management.\n",
>                            dmc->fw_path);
>-                drm_notice(&i915->drm, "DMC firmware homepage: %s",
>-                           INTEL_DMC_FIRMWARE_URL);
>         }
> 
>         release_firmware(fw);
>-- 
>2.39.2
>
Jani Nikula April 18, 2024, 7:56 p.m. UTC | #2
On Thu, 18 Apr 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> Quoting Jani Nikula (2024-04-18 11:39:50-03:00)
>>Clarify request_firmware() error handling. Don't proceed to trying to
>>parse non-existent firmware or check for payload when request_firmware()
>>failed to begin with. There's no reason to release_firmware() either
>>when request_firmware() failed.
>>
>>Also move the message about DMC firmware homepage here, as in other
>>cases the user probably has some firmware, although its parsing fails
>>for some reason.
>>
>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>---
>> drivers/gpu/drm/i915/display/intel_dmc.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>>index a34ff3383fd3..65880dea9c15 100644
>>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>>@@ -942,6 +942,15 @@ static void dmc_load_work_fn(struct work_struct *work)
>>                 }
>>         }
>> 
>>+        if (err) {
>>+                drm_notice(&i915->drm,
>>+                           "Failed to load DMC firmware %s (%pe). Disabling runtime power management.\n",
>>+                           dmc->fw_path, ERR_PTR(err));
>>+                drm_notice(&i915->drm, "DMC firmware homepage: %s",
>>+                           INTEL_DMC_FIRMWARE_URL);
>
> This could also be:
>
>     drm_notice(&i915->drm, "DMC firmware homepage: " INTEL_DMC_FIRMWARE_URL)

Although it currently doesn't, a URL could contain printf format
characters.

>
>>+                return;
>>+        }
>>+
>>         parse_dmc_fw(dmc, fw);
>
> Maybe also remove the now unnecessary NULL check for fw in
> parse_dmc_fw()?

Yeah, was ambivalent about it, could go either way.

>
>> 
>>         if (intel_dmc_has_payload(i915)) {
>>@@ -956,8 +965,6 @@ static void dmc_load_work_fn(struct work_struct *work)
>>                            "Failed to load DMC firmware %s."
>
> Should we tweak the message to differentiate from the previous one? At
> this point, we know the blob file exists, but there is a problem with
> its content.

That's done in the next patch. :)

>
> I think the patch looks good and to me all of the above suggestions are
> just that :-)  So, with or without them:
>
> Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

Thanks!

BR,
Jani.

>
> --
> Gustavo Sousa
>
>>                            " Disabling runtime power management.\n",
>>                            dmc->fw_path);
>>-                drm_notice(&i915->drm, "DMC firmware homepage: %s",
>>-                           INTEL_DMC_FIRMWARE_URL);
>>         }
>> 
>>         release_firmware(fw);
>>-- 
>>2.39.2
>>
Gustavo Sousa April 18, 2024, 7:59 p.m. UTC | #3
Quoting Jani Nikula (2024-04-18 16:56:06-03:00)
>On Thu, 18 Apr 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> Quoting Jani Nikula (2024-04-18 11:39:50-03:00)
>>>Clarify request_firmware() error handling. Don't proceed to trying to
>>>parse non-existent firmware or check for payload when request_firmware()
>>>failed to begin with. There's no reason to release_firmware() either
>>>when request_firmware() failed.
>>>
>>>Also move the message about DMC firmware homepage here, as in other
>>>cases the user probably has some firmware, although its parsing fails
>>>for some reason.
>>>
>>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>---
>>> drivers/gpu/drm/i915/display/intel_dmc.c | 11 +++++++++--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>>>index a34ff3383fd3..65880dea9c15 100644
>>>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>>>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>>>@@ -942,6 +942,15 @@ static void dmc_load_work_fn(struct work_struct *work)
>>>                 }
>>>         }
>>> 
>>>+        if (err) {
>>>+                drm_notice(&i915->drm,
>>>+                           "Failed to load DMC firmware %s (%pe). Disabling runtime power management.\n",
>>>+                           dmc->fw_path, ERR_PTR(err));
>>>+                drm_notice(&i915->drm, "DMC firmware homepage: %s",
>>>+                           INTEL_DMC_FIRMWARE_URL);
>>
>> This could also be:
>>
>>     drm_notice(&i915->drm, "DMC firmware homepage: " INTEL_DMC_FIRMWARE_URL)
>
>Although it currently doesn't, a URL could contain printf format
>characters.

Good point.

>
>>
>>>+                return;
>>>+        }
>>>+
>>>         parse_dmc_fw(dmc, fw);
>>
>> Maybe also remove the now unnecessary NULL check for fw in
>> parse_dmc_fw()?
>
>Yeah, was ambivalent about it, could go either way.
>
>>
>>> 
>>>         if (intel_dmc_has_payload(i915)) {
>>>@@ -956,8 +965,6 @@ static void dmc_load_work_fn(struct work_struct *work)
>>>                            "Failed to load DMC firmware %s."
>>
>> Should we tweak the message to differentiate from the previous one? At
>> this point, we know the blob file exists, but there is a problem with
>> its content.
>
>That's done in the next patch. :)

Yeah, realized that later on. :-)

--
Gustavo Sousa

>
>>
>> I think the patch looks good and to me all of the above suggestions are
>> just that :-)  So, with or without them:
>>
>> Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
>
>Thanks!
>
>BR,
>Jani.
>
>>
>> --
>> Gustavo Sousa
>>
>>>                            " Disabling runtime power management.\n",
>>>                            dmc->fw_path);
>>>-                drm_notice(&i915->drm, "DMC firmware homepage: %s",
>>>-                           INTEL_DMC_FIRMWARE_URL);
>>>         }
>>> 
>>>         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 a34ff3383fd3..65880dea9c15 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -942,6 +942,15 @@  static void dmc_load_work_fn(struct work_struct *work)
 		}
 	}
 
+	if (err) {
+		drm_notice(&i915->drm,
+			   "Failed to load DMC firmware %s (%pe). Disabling runtime power management.\n",
+			   dmc->fw_path, ERR_PTR(err));
+		drm_notice(&i915->drm, "DMC firmware homepage: %s",
+			   INTEL_DMC_FIRMWARE_URL);
+		return;
+	}
+
 	parse_dmc_fw(dmc, fw);
 
 	if (intel_dmc_has_payload(i915)) {
@@ -956,8 +965,6 @@  static void dmc_load_work_fn(struct work_struct *work)
 			   "Failed to load DMC firmware %s."
 			   " Disabling runtime power management.\n",
 			   dmc->fw_path);
-		drm_notice(&i915->drm, "DMC firmware homepage: %s",
-			   INTEL_DMC_FIRMWARE_URL);
 	}
 
 	release_firmware(fw);