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

[Gustavo Sousa <gustavo.sousa@intel.com>]

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
>