RE: [Intel-gfx] [PATCH v4 02/11] drm/i915: Read graphics/media/display arch version from hw

["Sripada, Radhakrishna" <radhakrishna.sripada@intel.com>]

Hi Lucas/Matt,

> -----Original Message-----
> From: De Marchi, Lucas <lucas.demarchi@intel.com>
> Sent: Wednesday, September 7, 2022 3:21 PM
> To: Roper, Matthew D <matthew.d.roper@intel.com>
> Cc: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>; intel-
> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vivi, Rodrigo
> <rodrigo.vivi@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v4 02/11] drm/i915: Read graphics/media/display
> arch version from hw
> 
> On Wed, Sep 07, 2022 at 03:13:31PM -0700, Matt Roper wrote:
> >On Wed, Sep 07, 2022 at 01:49:25PM -0700, Lucas De Marchi wrote:
> >> On Thu, Sep 01, 2022 at 11:03:33PM -0700, Radhakrishna Sripada wrote:
> >> > From: Matt Roper <matthew.d.roper@intel.com>
> >> >
> >> > Going forward, the hardware teams no longer consider new platforms to
> >> > have a "generation" in the way we've defined it for past platforms.
> >> > Instead, each IP block (graphics, media, display) will have their own
> >> > architecture major.minor versions and stepping ID's which should be read
> >> > directly from a register in the MMIO space.  New hardware programming
> >> > styles, features, and workarounds should be conditional solely on the
> >> > architecture version, and should no longer be derived from the PCI
> >> > device ID, revision ID, or platform-specific feature flags.
> >> >
> >> > Bspec: 63361, 64111
> >> >
> >> > v2:
> >> >  - Move the IP version readout to intel_device_info.c
> >> >  - Convert the macro into a function
> >> >
> >> > v3:
> >> >  - Move subplatform init to runtime early init
> >> >  - Cache runtime ver, release info to compare with hardware values.
> >> >
> >> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> >> > ---
> >> > drivers/gpu/drm/i915/gt/intel_gt_regs.h  |  2 +
> >> > drivers/gpu/drm/i915/i915_driver.c       |  3 +-
> >> > drivers/gpu/drm/i915/i915_drv.h          |  2 +
> >> > drivers/gpu/drm/i915/i915_pci.c          |  1 +
> >> > drivers/gpu/drm/i915/i915_reg.h          |  7 +++
> >> > drivers/gpu/drm/i915/intel_device_info.c | 74
> +++++++++++++++++++++++-
> >> > drivers/gpu/drm/i915/intel_device_info.h | 12 +++-
> >> > 7 files changed, 98 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> >> > index d414785003cc..579da62158c4 100644
> >> > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> >> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> >> > @@ -39,6 +39,8 @@
> >> > #define FORCEWAKE_ACK_RENDER_GEN9		_MMIO(0xd84)
> >> > #define FORCEWAKE_ACK_MEDIA_GEN9		_MMIO(0xd88)
> >> >
> >> > +#define GMD_ID_GRAPHICS
> 	_MMIO(0xd8c)
> >> > +
> >> > #define MCFG_MCR_SELECTOR			_MMIO(0xfd0)
> >> > #define SF_MCR_SELECTOR				_MMIO(0xfd8)
> >> > #define GEN8_MCR_SELECTOR			_MMIO(0xfdc)
> >> > diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> >> > index 56a2bcddb2af..a1ab49521d19 100644
> >> > --- a/drivers/gpu/drm/i915/i915_driver.c
> >> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> >> > @@ -323,7 +323,8 @@ static int i915_driver_early_probe(struct
> drm_i915_private *dev_priv)
> >> > 	if (i915_inject_probe_failure(dev_priv))
> >> > 		return -ENODEV;
> >> >
> >> > -	intel_device_info_subplatform_init(dev_priv);
> >> > +	intel_device_info_runtime_init_early(dev_priv);
> >> > +
> >> > 	intel_step_init(dev_priv);
> >> >
> >> > 	intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug);
> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> >> > index f85a470397a5..405b59b8c05c 100644
> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > @@ -936,6 +936,8 @@ IS_SUBPLATFORM(const struct drm_i915_private
> *i915,
> >> >
> >> > #define HAS_GMCH(dev_priv) (INTEL_INFO(dev_priv)->display.has_gmch)
> >> >
> >> > +#define HAS_GMD_ID(i915)	INTEL_INFO(i915)->has_gmd_id
> >> > +
> >> > #define HAS_LSPCON(dev_priv) (IS_DISPLAY_VER(dev_priv, 9, 10))
> >> >
> >> > #define HAS_L3_CCS_READ(i915) (INTEL_INFO(i915)->has_l3_ccs_read)
> >> > diff --git a/drivers/gpu/drm/i915/i915_pci.c
> b/drivers/gpu/drm/i915/i915_pci.c
> >> > index f6aaf938c53c..4672894f4bc1 100644
> >> > --- a/drivers/gpu/drm/i915/i915_pci.c
> >> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> >> > @@ -1129,6 +1129,7 @@ static const struct intel_device_info mtl_info = {
> >> > 	PLATFORM(INTEL_METEORLAKE),
> >> > 	.display.has_modular_fia = 1,
> >> > 	.has_flat_ccs = 0,
> >> > +	.has_gmd_id = 1,
> >> > 	.has_snoop = 1,
> >> > 	.__runtime.memory_regions = REGION_SMEM |
> REGION_STOLEN_LMEM,
> >> > 	.__runtime.platform_engine_mask = BIT(RCS0) | BIT(BCS0) | BIT(CCS0),
> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> >> > index 5e6239864c35..e02e461a4b5d 100644
> >> > --- a/drivers/gpu/drm/i915/i915_reg.h
> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> > @@ -5798,6 +5798,11 @@
> >> > #define ICL_DSSM_CDCLK_PLL_REFCLK_19_2MHz	(1 << 29)
> >> > #define ICL_DSSM_CDCLK_PLL_REFCLK_38_4MHz	(2 << 29)
> >> >
> >> > +#define GMD_ID_DISPLAY				_MMIO(0x510a0)
> >> > +#define   GMD_ID_ARCH_MASK			REG_GENMASK(31, 22)
> >> > +#define   GMD_ID_RELEASE_MASK			REG_GENMASK(21, 14)
> >> > +#define   GMD_ID_STEP				REG_GENMASK(5, 0)
> >> > +
> >> > /*GEN11 chicken */
> >> > #define _PIPEA_CHICKEN				0x70038
> >> > #define _PIPEB_CHICKEN				0x71038
> >> > @@ -8298,4 +8303,6 @@ enum skl_power_gate {
> >> > #define  MTL_LATENCY_LEVEL_EVEN_MASK	REG_GENMASK(12, 0)
> >> > #define  MTL_LATENCY_LEVEL_ODD_MASK	REG_GENMASK(28, 16)
> >> >
> >> > +#define MTL_MEDIA_GSI_BASE		0x380000
> >> > +
> >> > #endif /* _I915_REG_H_ */
> >> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c
> b/drivers/gpu/drm/i915/intel_device_info.c
> >> > index 56f19683dd55..a5bafc9be1fa 100644
> >> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> >> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> >> > @@ -29,6 +29,7 @@
> >> >
> >> > #include "display/intel_cdclk.h"
> >> > #include "display/intel_de.h"
> >> > +#include "gt/intel_gt_regs.h"
> >> > #include "intel_device_info.h"
> >> > #include "i915_drv.h"
> >> > #include "i915_utils.h"
> >> > @@ -231,7 +232,7 @@ static bool find_devid(u16 id, const u16 *p,
> unsigned int num)
> >> > 	return false;
> >> > }
> >> >
> >> > -void intel_device_info_subplatform_init(struct drm_i915_private *i915)
> >> > +static void intel_device_info_subplatform_init(struct drm_i915_private
> *i915)
> >> > {
> >> > 	const struct intel_device_info *info = INTEL_INFO(i915);
> >> > 	const struct intel_runtime_info *rinfo = RUNTIME_INFO(i915);
> >> > @@ -288,6 +289,77 @@ void intel_device_info_subplatform_init(struct
> drm_i915_private *i915)
> >> > 	RUNTIME_INFO(i915)->platform_mask[pi] |= mask;
> >> > }
> >> >
> >> > +static void ip_ver_read(struct drm_i915_private *i915, u32 offset, struct
> ip_version *ip)
> >> > +{
> >> > +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> >> > +	void __iomem *addr;
> >> > +	u32 val;
> >> > +	u8 ver = ip->ver;
> >> > +	u8 rel = ip->rel;
> >> > +
> >> > +	addr = pci_iomap_range(pdev, 0, offset, sizeof(u32));
> >> > +	if (drm_WARN_ON(&i915->drm, !addr))
> >> > +		return;
> >> > +
> >> > +	val = ioread32(addr);
> >> > +	pci_iounmap(pdev, addr);
> >> > +
> >> > +	ip->ver = REG_FIELD_GET(GMD_ID_ARCH_MASK, val);
> >> > +	ip->rel = REG_FIELD_GET(GMD_ID_RELEASE_MASK, val);
> >> > +	ip->step = REG_FIELD_GET(GMD_ID_STEP, val);
> >> > +
> >> > +	/* Sanity check against expected versions from device info */
> >> > +	if (ip->ver != ver || ip->rel > rel)
> >>
> >> this doesn't seem correct.. if we have
> >>
> >> 	ip->ver == 12, 	ip->rel == 1
> >> 	ver == 12, rel == 0
> >>
> >> we will print
> >>
> >> 	Hardware reports GMD IP version 12.1 but minimum expected is 12.0"
> >>
> >> should it be `|| ip->rel < rel`? Once we land the static number in
> >> device info, we can only expect new .rel versions greater than that.
> >
> >I'm not sure how much value there is in keeping this sanity check at all
> >given that we plan to remove the device info versions completely for
> >platforms with GMD_ID.  But if you want to test this in an easy-to-read
> >manner in the short term,
> >
> >        if (IP_VER(ip->ver, ip->rel) < IP_VER(ver, rel))
> >
> >would probably be best.
> 
> I'm ok with removing the sanity check as long as we log the value read.
> We still do when printing all the info, so lgtm.	
I agree with Matt's suggestion. Will spin a new rev with IP_VER based check.
Although we print the info later. It is worth doing a sanity check as we read the info.

-RK
> 
> Lucas De Marchi
> 
> >
> >
> >Matt
> >
> >>
> >> Lucas De Marchi
> >
> >--
> >Matt Roper
> >Graphics Software Engineer
> >VTT-OSGC Platform Enablement
> >Intel Corporation