[Intel-gfx] [PATCH v3 2/2] drm/i915/dmc: Prepare to use unversioned paths

Fri Dec 30 16:56:00 UTC 2022

On Fri, Dec 30, 2022 at 10:36:28AM -0300, Gustavo Sousa wrote:
> On Fri, Dec 30, 2022 at 03:47:43AM -0500, Rodrigo Vivi wrote:
> > On Thu, Dec 29, 2022 at 04:07:40PM -0300, Gustavo Sousa wrote:
> > > New DMC releases in linux-firmware will stop using version number in
> > > blob filenames. This new convention provides the following benefits:
> > > 
> > >   1. It simplifies code maintenance, as new DMC releases for a platform
> > >      using the new convention will always use the same filename for the
> > >      blob.
> > > 
> > >   2. It allows DMC to be loaded even if the target system does not have
> > >      the most recent firmware installed.
> > > 
> > > Prepare the driver by:
> > > 
> > >   - Using the new convention for DMC_PATH() and renaming the currently
> > >     used one to make it clear it is for the legacy scheme.
> > > 
> > >   - Implementing a fallback mechanism for future transitions from
> > >     versioned to unversioned paths so that we do not cause a regression
> > >     for systems not having the most up-to-date linux-firmware files.
> > > 
> > > v2:
> > >   - Keep using request_firmware() instead of firmware_request_nowarn().
> > >     (Jani)
> > > v3:
> > >   - Keep current DMC paths instead of directly using unversioned ones,
> > >     so that we do not disturb initrd generation.
> > >     (Lucas, Rodrigo)
> > > 
> > > References: https://lore.kernel.org/r/Y3081s7c5ksutpMW@intel.com
> > 
> > I also don't believe this link is a good reference here...
> 
> Noted.
> 
> > 
> > Regarding the patch, I liked the approach in general.
> > 
> > The patch is really neat, but I believe we will need to split it:
> > 
> > 1. Add the new DMC_PATH and DMC_LEGACY_PATH only with the introduction
> > of the mtl_dmc.bin
> > 
> > 2. And the fallback function, add only if/when we get a real fallback.
> 
> Okay. For future reference, how should that be implemented with respect to the
> organization of the patches? I see two ways of doing it and have a personal
> preference for the first one:
> 
> a) The future series would have first a patch adding the necessary functionality
>    and a second one using it.
> 
> b) The addition of the functionality would be incorporated in the same patch
>    using it.
> 
> For example, for (2), (a) would be a series two patches, the first adding the
> fallback mechanism and the second one changing ADLP to use unversioned paths;
> and (b) would be all of that in a single patch.
> 
> I looks to me that approach (b) has a potential issue. For example, let's say we
> in the future we decide to revert that specific firmware update but we already
> have other platforms also using the fallback - a clean revert is not possible
> there and we would need to make sure that the fallback mechanism is kept.
> 
> That's why I like (a) more and I think (b) would be more appropriate for cases
> where the functionality and it's user(s) have almost like a "1:1" relationship
> (not strictly speaking, read that as "having a somewhat static and restricted
> set of users").

yeap, it is case by case. The advantage on the (b) approach is that OSVs
can backport only 1 patch.

And the revert doesn't necessarily need to be a git-revert. In this case
it would be only one line getting back to the older firmware.

But really up to you ;)
As long as they are together either in the same patch or same series.

> 
> > 
> > Oh, and I just realized that when doing the new _dmc.bin path we also
> > need to make sure that we read the fw_version from the header and
> > print as a drm_info msg somewhere.
> 
> I think the version is already being read by parse_dmc_fw_css() and printed at
> the end of dmc_load_work_fn() if the blob was loaded and parsed succesfully.

Oh, I had missed that. If it is already happening please disregard my comment.

> 
> > 
> > > Signed-off-by: Gustavo Sousa <gustavo.sousa at intel.com>
> > > Cc: Jani Nikula <jani.nikula at intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dmc.c | 62 ++++++++++++++++++------
> > >  1 file changed, 46 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..12f05b2d33a3 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> > > @@ -42,51 +42,59 @@
> > >  #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_LEGACY_PATH(dg2, 2, 08)
> > >  MODULE_FIRMWARE(DG2_DMC_PATH);
> > >  
> > > -#define ADLP_DMC_PATH			DMC_PATH(adlp, 2, 16)
> > > +#define ADLP_DMC_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_LEGACY_PATH(adls, 2, 01)
> > >  MODULE_FIRMWARE(ADLS_DMC_PATH);
> > >  
> > > -#define DG1_DMC_PATH			DMC_PATH(dg1, 2, 02)
> > > +#define DG1_DMC_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_LEGACY_PATH(rkl, 2, 03)
> > >  MODULE_FIRMWARE(RKL_DMC_PATH);
> > >  
> > > -#define TGL_DMC_PATH			DMC_PATH(tgl, 2, 12)
> > > +#define TGL_DMC_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_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_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_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_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_LEGACY_PATH(bxt, 1, 07)
> > >  #define BXT_DMC_MAX_FW_SIZE		0x3000
> > >  MODULE_FIRMWARE(BXT_DMC_PATH);
> > >  
> > > @@ -821,16 +829,38 @@ 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_fallback_path(struct drm_i915_private *i915)
> > > +{
> > > +	/* No fallback paths for now. */
> > > +	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 *fallback_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 = request_firmware(&fw, dev_priv->display.dmc.fw_path, dev_priv->drm.dev);
> > > +
> > > +	if (err == -ENOENT && !dev_priv->params.dmc_firmware_path) {
> > > +		fallback_path = dmc_fallback_path(dev_priv);
> > > +		if (fallback_path) {
> > > +			drm_dbg_kms(&dev_priv->drm,
> > > +				    "%s not found, falling back to %s\n",
> > > +				    dmc->fw_path,
> > > +				    fallback_path);
> > > +			err = request_firmware(&fw, fallback_path, dev_priv->drm.dev);
> > > +			if (err == 0)
> > > +				dev_priv->display.dmc.fw_path = fallback_path;
> > > +		}
> > > +	}
> > > +
> > >  	parse_dmc_fw(dev_priv, fw);
> > >  
> > >  	if (intel_dmc_has_payload(dev_priv)) {
> > > -- 
> > > 2.39.0
> > > 