Re: [Intel-gfx] [PATCH 2/2] drm/i915/dmc: Use unversioned firmware paths

[Lucas De Marchi <lucas.demarchi@intel.com>]

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@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@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/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);

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