[Intel-gfx] [PATCH 2/2] drm/i915/dmc: Use unversioned firmware paths
Vivi, Rodrigo
rodrigo.vivi at intel.com
Wed Dec 28 22:04:46 UTC 2022
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 at intel.com> 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@intel.com
> > > > > > > > Signed-off-by: Gustavo Sousa <gustavo.sousa at intel.com>
> > > > > > > > ---
> > > > > > > > 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
More information about the Intel-gfx
mailing list