On Thu, 18 Apr 2024, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> 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@xxxxxxxxx>
>>---
>> 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@xxxxxxxxx>
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
