On Thu, Dec 22, 2022 at 04:52:21PM -0800, Lucas De Marchi wrote: > On Thu, Dec 22, 2022 at 07:12:08PM -0300, Gustavo Sousa wrote: > > On Wed, Dec 21, 2022 at 04:23:45PM -0800, Lucas De Marchi wrote: > > > On Wed, Dec 21, 2022 at 12:26:26PM +0200, Jani Nikula wrote: > > > > On Tue, 20 Dec 2022, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> wrote: > > > > > As we do not require specific versions anymore, change the convention > > > > > for blob filenames to stop using version numbers. This simplifies code > > > > > maintenance, since we do not need to keep updating blob paths for new > > > > > DMC releases, and also makes DMC loading compatible with systems that do > > > > > not have the latest firmware release. > > > > > > > > > > References: https://lore.kernel.org/r/Y3081s7c5ksutpMW@xxxxxxxxx > > > > > Signed-off-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_dmc.c | 98 ++++++++++++++++++++---- > > > > > 1 file changed, 82 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..b11f0f451dd7 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_dmc.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c > > > > > @@ -42,51 +42,70 @@ > > > > > #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_PATH(dg2) > > > > > +#define DG2_DMC_LEGACY_PATH DMC_LEGACY_PATH(dg2, 2, 08) > > > > > MODULE_FIRMWARE(DG2_DMC_PATH); > > > > > > We have an issue here. Previously `modinfo --field=firmware i915` > > > would report i915/dg2_dmc_ver2_08.bin. Now it's going to report > > > i915/dg2_dmc.bin > > > > > > that modinfo call is what distros use to bundle the firmware blobs in > > > the initrd. It may also be used for creating package dependendies. > > > > > > If we do this, unless they have updated their linux-firmware > > > packages, the initrd will be left without the firmware. > > > Just checked > > > git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git main > > > and we still don't have the unversioned firmware there. > > > > Interesting. Thanks for the explanation! > > > > I think one way of approaching the issue would be to synchronize the process: > > > > 1. Work toward getting approval for the patch (i.e. r-b); > > 2. With the approval, send a PR to linux-firmware with the necessary changes; > > 3. After the linux-firmware PR is merged, the patch could be integraged. > > > > I think that would still apply if going with your proposal on your next comment. > > > > > > > > > > > > > > > -#define ADLP_DMC_PATH DMC_PATH(adlp, 2, 16) > > > > > +#define ADLP_DMC_PATH DMC_PATH(adlp) > > > > > +#define ADLP_DMC_LEGACY_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_PATH(adls) > > > > > +#define ADLS_DMC_LEGACY_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_PATH(dg1) > > > > > +#define DG1_DMC_LEGACY_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_PATH(rkl) > > > > > +#define RKL_DMC_LEGACY_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_PATH(tgl) > > > > > +#define TGL_DMC_LEGACY_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_PATH(icl) > > > > > +#define ICL_DMC_LEGACY_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_PATH(glk) > > > > > +#define GLK_DMC_LEGACY_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_PATH(kbl) > > > > > +#define KBL_DMC_LEGACY_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_PATH(skl) > > > > > +#define SKL_DMC_LEGACY_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_PATH(bxt) > > > > > +#define BXT_DMC_LEGACY_PATH DMC_LEGACY_PATH(bxt, 1, 07) > > > > > #define BXT_DMC_MAX_FW_SIZE 0x3000 > > > > > MODULE_FIRMWARE(BXT_DMC_PATH); > > > > > > > > > > @@ -821,16 +840,63 @@ 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_legacy_path(struct drm_i915_private *i915) > > > > > +{ > > > > > + if (IS_DG2(i915)) { > > > > > + return DG2_DMC_LEGACY_PATH; > > > > > + } else if (IS_ALDERLAKE_P(i915)) { > > > > > + return ADLP_DMC_LEGACY_PATH; > > > > > + } else if (IS_ALDERLAKE_S(i915)) { > > > > > + return ADLS_DMC_LEGACY_PATH; > > > > > + } else if (IS_DG1(i915)) { > > > > > + return DG1_DMC_LEGACY_PATH; > > > > > + } else if (IS_ROCKETLAKE(i915)) { > > > > > + return RKL_DMC_LEGACY_PATH; > > > > > + } else if (IS_TIGERLAKE(i915)) { > > > > > + return TGL_DMC_LEGACY_PATH; > > > > > + } else if (DISPLAY_VER(i915) == 11) { > > > > > + return ICL_DMC_LEGACY_PATH; > > > > > + } else if (IS_GEMINILAKE(i915)) { > > > > > + return GLK_DMC_LEGACY_PATH; > > > > > + } else if (IS_KABYLAKE(i915) || > > > > > + IS_COFFEELAKE(i915) || > > > > > + IS_COMETLAKE(i915)) { > > > > > + return KBL_DMC_LEGACY_PATH; > > > > > + } else if (IS_SKYLAKE(i915)) { > > > > > + return SKL_DMC_LEGACY_PATH; > > > > > + } else if (IS_BROXTON(i915)) { > > > > > + return BXT_DMC_LEGACY_PATH; > > > > > + } > > > > > + > > > > > + 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 *legacy_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 = firmware_request_nowarn(&fw, dev_priv->display.dmc.fw_path, dev_priv->drm.dev); > > > > > + > > > > > + if (err == -ENOENT && !dev_priv->params.dmc_firmware_path) { > > > > > + legacy_path = dmc_legacy_path(dev_priv); > > > > > + if (legacy_path) { > > > > > + drm_dbg_kms(&dev_priv->drm, > > > > > + "%s not found, falling back to %s\n", > > > > > + dmc->fw_path, > > > > > + legacy_path); > > > > > + err = firmware_request_nowarn(&fw, legacy_path, dev_priv->drm.dev); > > > > > + if (err == 0) > > > > > + dev_priv->display.dmc.fw_path = legacy_path; > > > > > + } > > > > > + } > > > > > + > > > > > > > > I'd keep the request_firmware() with warnings. > > > > > > then not only it will be missing from initrd but we will also trigger > > > new warnings. Humn, I think one alternative approach and my proposal > > > would be: > > > > > > leave platforms that already have published firmware as is as long as > > > they are not behind a force_probe. For the new platforms, like mtl, > > > just use the platform name and don't bother about the version. > > > We will also have to fix it in the linux-firmware repo. > > > > > > We are likely not updating DMC for very old platforms anyway, no need to > > > rename them. I think that after having symlinks in place in > > > linux-firmware for a few years/months, then we can go back and kill the > > > version numbers if we really want to. > > > > Sounds good. > > > > This patch was an attempt to have all supported platforms changed to the new > > convention and keep us from having to send a new patch for each platform that > > would need the change because of a new firmware release. But to avoid warnings, > > I think your proposal would be better indeed. > > > > It seems that currently the only platforms with display that are > > using require_force_probe = 1 are DG1 and MTL, and the latter does not have an > > entry in intel_dmc.c yet. Moving forward with your proposal, I guess we could > > also keep DG1 as is and only update it when/if the time comes. > > > > That said, I still think we would need the logic for loading from legacy paths > > as fallback so that we do not cause regressions when, for example, ADL has an > > update and we "move" it to the new convention. Do you agree? > > > > So here is my proposal: > > > > - Keep using the same paths (i.e. versioned ones) for the current entries in > > intel_dmc.c, but define them with DMC_LEGACY_PATH() and reserve DMC_PATH() for > > the new convention. > > > > - Keep the logic for the fallback in place because we know that it will be > > needed soon enough for some more recent platforms. > > here is where we disagree. I don't think we need any fallback, because > it will likely not work: > > MODULE_FIRMWARE(ADLP_DMC_PATH); Yeah... I was thinking about this and maybe we could also have MODULE_FIRMWARE() calls for legacy paths as well. Looking at the documentation for MODULE_FIRMWARE(), the module files are understood as "optional", so I think it would be somewhat okay for one of the two missing, as long as the one is found. I think declaring possibly missing fallback paths is less ugly than overwriting the versioned path with a blob of a different version. > > this means that distros will only package and or update their initrd > with the unversioned path. If a developer updates the kernel, the > fallback will simply not work if i915 is loaded from the initrd. > > For those older platforms I think we can simply keep updating > linux-firmware overwriting the same dmc_adlp_2_16.bin. It's ugly, but > doesn't break compatibility. > > I defer to maintainers to chime in on that though. Jani/Rodrigo, what do > you think? > > Lucas De Marchi > > > > > - Similarly to your last remark, if we find it necessary, we could in the future > > remove the fallback logic after linux-firmware has all blobs using the new > > convention for good enough time. > > > > > > -- > > Gustavo Sousa
