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.
