Re: [PATCH 1/5] drm/i915/dmc: handle request_firmware() errors separately
Quoting Jani Nikula (2024-04-18 16:56:06-03:00)
>On Thu, 18 Apr 2024, Gustavo Sousa 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
>>>---
>>> 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(>drm,
>>>+ "Failed to load DMC firmware %s (%pe). Disabling
>>>runtime power management.\n",
>>>+ dmc->fw_path, ERR_PTR(err));
>>>+drm_notice(>drm, "DMC firmware homepage: %s",
>>>+ INTEL_DMC_FIRMWARE_URL);
>>
>> This could also be:
>>
>> drm_notice(>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
>
>Thanks!
>
>BR,
>Jani.
>
>>
>> --
>> Gustavo Sousa
>>
>>>" Disabling runtime power management.\n",
>>>dmc->fw_path);
>>>-drm_notice(>drm, "DMC firmware homepage: %s",
>>>- INTEL_DMC_FIRMWARE_URL);
>>> }
>>>
>>> release_firmware(fw);
>>>--
>>>2.39.2
>>>
>
>--
>Jani Nikula, Intel
Re: [PATCH 1/5] drm/i915/dmc: handle request_firmware() errors separately
On Thu, 18 Apr 2024, Gustavo Sousa 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
>>---
>> 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(>drm,
>>+ "Failed to load DMC firmware %s (%pe). Disabling
>>runtime power management.\n",
>>+ dmc->fw_path, ERR_PTR(err));
>>+drm_notice(>drm, "DMC firmware homepage: %s",
>>+ INTEL_DMC_FIRMWARE_URL);
>
> This could also be:
>
> drm_notice(>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
Thanks!
BR,
Jani.
>
> --
> Gustavo Sousa
>
>>" Disabling runtime power management.\n",
>>dmc->fw_path);
>>-drm_notice(>drm, "DMC firmware homepage: %s",
>>- INTEL_DMC_FIRMWARE_URL);
>> }
>>
>> release_firmware(fw);
>>--
>>2.39.2
>>
--
Jani Nikula, Intel
Re: [PATCH 1/5] drm/i915/dmc: handle request_firmware() errors separately
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
>---
> 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(>drm,
>+ "Failed to load DMC firmware %s (%pe). Disabling
>runtime power management.\n",
>+ dmc->fw_path, ERR_PTR(err));
>+drm_notice(>drm, "DMC firmware homepage: %s",
>+ INTEL_DMC_FIRMWARE_URL);
This could also be:
drm_notice(>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
>" Disabling runtime power management.\n",
>dmc->fw_path);
>-drm_notice(>drm, "DMC firmware homepage: %s",
>- INTEL_DMC_FIRMWARE_URL);
> }
>
> release_firmware(fw);
>--
>2.39.2
>
[PATCH 1/5] drm/i915/dmc: handle request_firmware() errors separately
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
---
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(>drm,
+ "Failed to load DMC firmware %s (%pe). Disabling
runtime power management.\n",
+ dmc->fw_path, ERR_PTR(err));
+ drm_notice(>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(>drm, "DMC firmware homepage: %s",
- INTEL_DMC_FIRMWARE_URL);
}
release_firmware(fw);
--
2.39.2
