| Message ID | 725c3580aae52bf34f32b93275cce6208dfd42b7.1713450693.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/dmc: firmware path handling changes | expand |
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 >
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 >>
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 --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);
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(-)