[2/2] drm/i915/dmc: Use unversioned firmware paths

Commit Message

Gustavo Sousa Dec. 20, 2022, 8:11 p.m. UTC

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/[e-mail]
Signed-off-by: Gustavo Sousa <[e-mail]>
---
 drivers/gpu/drm/i915/display/intel_dmc.c | 98 ++++++++++++++++++++----
 1 file changed, 82 insertions(+), 16 deletions(-)

Comments

Jani Nikula Dec. 21, 2022, 10:26 a.m. UTC | #1

On Tue, 20 Dec 2022, Gustavo Sousa <[e-mail]> 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/[e-mail]
> Signed-off-by: Gustavo Sousa <[e-mail]>
> ---
>  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);
>  
> -#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.

BR,
Jani.

>  	parse_dmc_fw(dev_priv, fw);
>  
>  	if (intel_dmc_has_payload(dev_priv)) {

Gustavo Sousa Dec. 21, 2022, 3:08 p.m. UTC | #2

On Wed, Dec 21, 2022 at 12:26:26PM +0200, Jani Nikula wrote:
> > -	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.

Thanks. Just sent a v2 with that.

--
Gustavo Sousa

Lucas De Marchi Dec. 22, 2022, 12:23 a.m. UTC | #3

On Wed, Dec 21, 2022 at 12:26:26PM +0200, Jani Nikula wrote:
>On Tue, 20 Dec 2022, Gustavo Sousa <[e-mail]> 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/[e-mail]
>> Signed-off-by: Gustavo Sousa <[e-mail]>
>> ---
>>  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.

>>
>> -#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.

Lucas De Marchi

>
>BR,
>Jani.
>
>>  	parse_dmc_fw(dev_priv, fw);
>>
>>  	if (intel_dmc_has_payload(dev_priv)) {
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center

Gustavo Sousa Dec. 22, 2022, 10:12 p.m. UTC | #4

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 <[e-mail]> 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/[e-mail]
> > > Signed-off-by: Gustavo Sousa <[e-mail]>
> > > ---
> > >  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.

- 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

Lucas De Marchi Dec. 23, 2022, 12:52 a.m. UTC | #5

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 <[e-mail]> 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/[e-mail]
>> > > Signed-off-by: Gustavo Sousa <[e-mail]>
>> > > ---
>> > >  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

Gustavo Sousa Dec. 23, 2022, 11:51 a.m. UTC | #6

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 <[e-mail]> 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/[e-mail]
> > > > > Signed-off-by: Gustavo Sousa <[e-mail]>
> > > > > ---
> > > > >  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

Rodrigo Vivi Dec. 23, 2022, 3:36 p.m. UTC | #7

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 <[e-mail]> 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/[e-mail]
> > > > > > Signed-off-by: Gustavo Sousa <[e-mail]>
> > > > > > ---
> > > > > >  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.

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 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.

> 
> > 
> > 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

Gustavo Sousa Dec. 28, 2022, 10 p.m. UTC | #8

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 <[e-mail]> 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/[e-mail]
> > > > > > > Signed-off-by: Gustavo Sousa <[e-mail]>
> > > > > > > ---
> > > > > > >  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.
> 
> 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.

> 
> > 
> > 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?

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

Rodrigo Vivi Dec. 28, 2022, 10:04 p.m. UTC | #9

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
> > > > > > > <[e-mail]> 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/[e-mail]
> > > > > > > > Signed-off-by: Gustavo Sousa <[e-mail]>
> > > > > > > > ---
> > > > > > > >  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

Gustavo Sousa Dec. 28, 2022, 10:19 p.m. UTC | #10

On Wed, Dec 28, 2022 at 07:04:46PM -0300, Vivi, Rodrigo wrote:
> 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
> > > > > > > > <[e-mail]> 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/[e-mail]
> > > > > > > > > Signed-off-by: Gustavo Sousa <[e-mail]>
> > > > > > > > > ---
> > > > > > > > >  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?

Yes.

> 
> > 
> > > 
> > > > 
> > > > 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.

Cool.

By the way, specifically for mkinicpio (the one used for Arch Linux), it does
raise warnings for missing firmware, but only if *not even one* firmware file
listed for the module was found - if *at least one* is found, it does not
complain.

I haven't checked if the behavior is the same for the other tools though - just
checked that they do not complain for the test scenario explained above, which
should suffice our case.

--
Gustavo Sousa

> 
> > 
> > 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
>

Patch

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);
 
-#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;
+		}
+	}
+
 	parse_dmc_fw(dev_priv, fw);
 
 	if (intel_dmc_has_payload(dev_priv)) {