On Wed, 2022-12-28 at 19:00 -0300, Gustavo Sousa wrote: > On Fri, Dec 23, 2022 at 10:36:05AM -0500, Rodrigo Vivi wrote: > > On Fri, Dec 23, 2022 at 08:51:59AM -0300, Gustavo Sousa wrote: > > > 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/linu > > > > > > x-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. > > > > This is also my suggestion. Don't touch the old ones unless needed. > > Let's live with the versions there for the ones we are not > > updating. > > > > > > > > > > > > > > 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. > > > > I do like this option though... > > > > > > > > > > > > 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 believe this will generate a big warning when the initrd are > > getting updated > > and the firmware is not there. Please run some experiments on your > > side there with > > this idea. We need to avoid these warnings. But we might need to do > > something like > > that, at least temporarily for the cases where we end up in need to > > update the > > minor version and we are out of the force_probe protection. > > I did some research and tested generating initrds. > > The test consisted of generating two images, both including i915. The > firmware > directory was left untouched for the generation of the first image. > For the > second image, ADLP DMC firmware files were removed from there > location in the > system prior to calling the command to generate the image. > > After generating the images, I compared their content. I checked that > the module > object was present in both of them. I also checked that firmware > files were also > present and that the firmware for ADL was missing in the second > image. > > I ran the test on: > > 1. Arch Linux (my host system) using mkinitcpio; > 2. Ubuntu 22.04 (virtual machine) using mkiniramfs; > 3. Fedora 37 (virtual machine) using dracut. > > For all cases, no warnings about missing firmware blobs were raised. just to double check, you ensured you have removed the files from /lib/firmware/i915/ before you triggered the regeneration, right? > > > > > > > > > I think declaring possibly missing fallback paths is less ugly > > > than overwriting > > > the versioned path with a blob of a different version. > > > > Yeap, please, let's not override a path with a different version. > > This will > > lead to confusions later. > > So should we go with keeping the fallback mechanism in place and > declaring both > new (unversioned) and legacy (versioned) paths for the modules that > will be > updated? If no warning is raised when you don't have the firmware, then I believe we have a path. > > Following the direction to update paths only for platforms that get > new DMC > releases, we would have dmc_legacy_path() simply returning NULL for > the time > being (which I think is not that bad, as we know we will have new DMC > releases > for platforms already using versioned paths). > > -- > Gustavo Sousa > > > > > > > > > > > > > > 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. > > > > Yeap, if one (not necessarily developer) updates the kernel, but > > for some > > reason linux-firmware.git was outdated, then the initrd will end up > > empty > > and the firmware won't get loaded. > > > > This is the regression that we should avoid. One of the main > > reasons on why > > we are removing the version from the path. > > > > > > > > > > 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
