On Fri, Dec 30, 2022 at 10:36:28AM -0300, Gustavo Sousa wrote: > On Fri, Dec 30, 2022 at 03:47:43AM -0500, Rodrigo Vivi wrote: > > On Thu, Dec 29, 2022 at 04:07:40PM -0300, Gustavo Sousa wrote: > > > New DMC releases in linux-firmware will stop using version number in > > > blob filenames. This new convention provides the following benefits: > > > > > > 1. It simplifies code maintenance, as new DMC releases for a platform > > > using the new convention will always use the same filename for the > > > blob. > > > > > > 2. It allows DMC to be loaded even if the target system does not have > > > the most recent firmware installed. > > > > > > Prepare the driver by: > > > > > > - Using the new convention for DMC_PATH() and renaming the currently > > > used one to make it clear it is for the legacy scheme. > > > > > > - Implementing a fallback mechanism for future transitions from > > > versioned to unversioned paths so that we do not cause a regression > > > for systems not having the most up-to-date linux-firmware files. > > > > > > v2: > > > - Keep using request_firmware() instead of firmware_request_nowarn(). > > > (Jani) > > > v3: > > > - Keep current DMC paths instead of directly using unversioned ones, > > > so that we do not disturb initrd generation. > > > (Lucas, Rodrigo) > > > > > > References: https://lore.kernel.org/r/Y3081s7c5ksutpMW@xxxxxxxxx > > > > I also don't believe this link is a good reference here... > > Noted. > > > > > Regarding the patch, I liked the approach in general. > > > > The patch is really neat, but I believe we will need to split it: > > > > 1. Add the new DMC_PATH and DMC_LEGACY_PATH only with the introduction > > of the mtl_dmc.bin > > > > 2. And the fallback function, add only if/when we get a real fallback. > > Okay. For future reference, how should that be implemented with respect to the > organization of the patches? I see two ways of doing it and have a personal > preference for the first one: > > a) The future series would have first a patch adding the necessary functionality > and a second one using it. > > b) The addition of the functionality would be incorporated in the same patch > using it. > > For example, for (2), (a) would be a series two patches, the first adding the > fallback mechanism and the second one changing ADLP to use unversioned paths; > and (b) would be all of that in a single patch. > > I looks to me that approach (b) has a potential issue. For example, let's say we > in the future we decide to revert that specific firmware update but we already > have other platforms also using the fallback - a clean revert is not possible > there and we would need to make sure that the fallback mechanism is kept. > > That's why I like (a) more and I think (b) would be more appropriate for cases > where the functionality and it's user(s) have almost like a "1:1" relationship > (not strictly speaking, read that as "having a somewhat static and restricted > set of users"). yeap, it is case by case. The advantage on the (b) approach is that OSVs can backport only 1 patch. And the revert doesn't necessarily need to be a git-revert. In this case it would be only one line getting back to the older firmware. But really up to you ;) As long as they are together either in the same patch or same series. > > > > > Oh, and I just realized that when doing the new _dmc.bin path we also > > need to make sure that we read the fw_version from the header and > > print as a drm_info msg somewhere. > > I think the version is already being read by parse_dmc_fw_css() and printed at > the end of dmc_load_work_fn() if the blob was loaded and parsed succesfully. Oh, I had missed that. If it is already happening please disregard my comment. > > > > > > Signed-off-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx> > > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx> > > > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx> > > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/display/intel_dmc.c | 62 ++++++++++++++++++------ > > > 1 file changed, 46 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c > > > index 4124b3d37110..12f05b2d33a3 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dmc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c > > > @@ -42,51 +42,59 @@ > > > #define DMC_VERSION_MAJOR(version) ((version) >> 16) > > > #define DMC_VERSION_MINOR(version) ((version) & 0xffff) > > > > > > -#define DMC_PATH(platform, major, minor) \ > > > - "i915/" \ > > > - __stringify(platform) "_dmc_ver" \ > > > - __stringify(major) "_" \ > > > +#define DMC_PATH(platform) \ > > > + "i915/" __stringify(platform) "_dmc.bin" > > > + > > > +/* > > > + * New DMC additions should not use this. This is used solely to remain > > > + * compatible with systems that have not yet updated DMC blobs to use > > > + * unversioned file names. > > > + */ > > > +#define DMC_LEGACY_PATH(platform, major, minor) \ > > > + "i915/" \ > > > + __stringify(platform) "_dmc_ver" \ > > > + __stringify(major) "_" \ > > > __stringify(minor) ".bin" > > > > > > #define DISPLAY_VER13_DMC_MAX_FW_SIZE 0x20000 > > > > > > #define DISPLAY_VER12_DMC_MAX_FW_SIZE ICL_DMC_MAX_FW_SIZE > > > > > > -#define DG2_DMC_PATH DMC_PATH(dg2, 2, 08) > > > +#define DG2_DMC_PATH DMC_LEGACY_PATH(dg2, 2, 08) > > > MODULE_FIRMWARE(DG2_DMC_PATH); > > > > > > -#define ADLP_DMC_PATH DMC_PATH(adlp, 2, 16) > > > +#define ADLP_DMC_PATH DMC_LEGACY_PATH(adlp, 2, 16) > > > MODULE_FIRMWARE(ADLP_DMC_PATH); > > > > > > -#define ADLS_DMC_PATH DMC_PATH(adls, 2, 01) > > > +#define ADLS_DMC_PATH DMC_LEGACY_PATH(adls, 2, 01) > > > MODULE_FIRMWARE(ADLS_DMC_PATH); > > > > > > -#define DG1_DMC_PATH DMC_PATH(dg1, 2, 02) > > > +#define DG1_DMC_PATH DMC_LEGACY_PATH(dg1, 2, 02) > > > MODULE_FIRMWARE(DG1_DMC_PATH); > > > > > > -#define RKL_DMC_PATH DMC_PATH(rkl, 2, 03) > > > +#define RKL_DMC_PATH DMC_LEGACY_PATH(rkl, 2, 03) > > > MODULE_FIRMWARE(RKL_DMC_PATH); > > > > > > -#define TGL_DMC_PATH DMC_PATH(tgl, 2, 12) > > > +#define TGL_DMC_PATH DMC_LEGACY_PATH(tgl, 2, 12) > > > MODULE_FIRMWARE(TGL_DMC_PATH); > > > > > > -#define ICL_DMC_PATH DMC_PATH(icl, 1, 09) > > > +#define ICL_DMC_PATH DMC_LEGACY_PATH(icl, 1, 09) > > > #define ICL_DMC_MAX_FW_SIZE 0x6000 > > > MODULE_FIRMWARE(ICL_DMC_PATH); > > > > > > -#define GLK_DMC_PATH DMC_PATH(glk, 1, 04) > > > +#define GLK_DMC_PATH DMC_LEGACY_PATH(glk, 1, 04) > > > #define GLK_DMC_MAX_FW_SIZE 0x4000 > > > MODULE_FIRMWARE(GLK_DMC_PATH); > > > > > > -#define KBL_DMC_PATH DMC_PATH(kbl, 1, 04) > > > +#define KBL_DMC_PATH DMC_LEGACY_PATH(kbl, 1, 04) > > > #define KBL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE > > > MODULE_FIRMWARE(KBL_DMC_PATH); > > > > > > -#define SKL_DMC_PATH DMC_PATH(skl, 1, 27) > > > +#define SKL_DMC_PATH DMC_LEGACY_PATH(skl, 1, 27) > > > #define SKL_DMC_MAX_FW_SIZE BXT_DMC_MAX_FW_SIZE > > > MODULE_FIRMWARE(SKL_DMC_PATH); > > > > > > -#define BXT_DMC_PATH DMC_PATH(bxt, 1, 07) > > > +#define BXT_DMC_PATH DMC_LEGACY_PATH(bxt, 1, 07) > > > #define BXT_DMC_MAX_FW_SIZE 0x3000 > > > MODULE_FIRMWARE(BXT_DMC_PATH); > > > > > > @@ -821,16 +829,38 @@ static void intel_dmc_runtime_pm_put(struct drm_i915_private *dev_priv) > > > intel_display_power_put(dev_priv, POWER_DOMAIN_INIT, wakeref); > > > } > > > > > > +static const char *dmc_fallback_path(struct drm_i915_private *i915) > > > +{ > > > + /* No fallback paths for now. */ > > > + return NULL; > > > +} > > > + > > > static void dmc_load_work_fn(struct work_struct *work) > > > { > > > struct drm_i915_private *dev_priv; > > > struct intel_dmc *dmc; > > > const struct firmware *fw = NULL; > > > + const char *fallback_path; > > > + int err; > > > > > > dev_priv = container_of(work, typeof(*dev_priv), display.dmc.work); > > > dmc = &dev_priv->display.dmc; > > > > > > - request_firmware(&fw, dev_priv->display.dmc.fw_path, dev_priv->drm.dev); > > > + err = request_firmware(&fw, dev_priv->display.dmc.fw_path, dev_priv->drm.dev); > > > + > > > + if (err == -ENOENT && !dev_priv->params.dmc_firmware_path) { > > > + fallback_path = dmc_fallback_path(dev_priv); > > > + if (fallback_path) { > > > + drm_dbg_kms(&dev_priv->drm, > > > + "%s not found, falling back to %s\n", > > > + dmc->fw_path, > > > + fallback_path); > > > + err = request_firmware(&fw, fallback_path, dev_priv->drm.dev); > > > + if (err == 0) > > > + dev_priv->display.dmc.fw_path = fallback_path; > > > + } > > > + } > > > + > > > parse_dmc_fw(dev_priv, fw); > > > > > > if (intel_dmc_has_payload(dev_priv)) { > > > -- > > > 2.39.0 > > >
