Re: [Intel-gfx] [v1.1 01/23] drm/i915: Read graphics/media/display arch version from hw

[Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>]

On 27.07.2022 20:46, 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.
> 
> v1.1: Fix build error
> 
> Bspec: 63361, 64111
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_regs.h       |  2 +
>  drivers/gpu/drm/i915/i915_driver.c            | 80 ++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_drv.h               | 16 ++--
>  drivers/gpu/drm/i915/i915_pci.c               |  1 +
>  drivers/gpu/drm/i915/i915_reg.h               |  6 ++
>  drivers/gpu/drm/i915/intel_device_info.c      | 32 ++++----
>  drivers/gpu/drm/i915/intel_device_info.h      | 14 ++++
>  .../gpu/drm/i915/selftests/mock_gem_device.c  |  1 +
>  8 files changed, 128 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> index 60d6eb5f245b..fab8e4ff74d5 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 deb8a8b76965..33566f6e9546 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -70,6 +70,7 @@
>  #include "gem/i915_gem_pm.h"
>  #include "gt/intel_gt.h"
>  #include "gt/intel_gt_pm.h"
> +#include "gt/intel_gt_regs.h"
>  #include "gt/intel_rc6.h"
>  
>  #include "pxp/intel_pxp_pm.h"
> @@ -306,15 +307,83 @@ static void sanitize_gpu(struct drm_i915_private *i915)
>  		__intel_gt_reset(to_gt(i915), ALL_ENGINES);
>  }
>  
> +#define IP_VER_READ(offset, ri_prefix) \
> +	addr = pci_iomap_range(pdev, 0, offset, sizeof(u32)); \
> +	if (drm_WARN_ON(&i915->drm, !addr)) { \
> +		/* Fall back to whatever was in the device info */ \
> +		RUNTIME_INFO(i915)->ri_prefix.ver = INTEL_INFO(i915)->ri_prefix.ver; \
> +		RUNTIME_INFO(i915)->ri_prefix.rel = INTEL_INFO(i915)->ri_prefix.rel; \
> +		goto ri_prefix##done; \
> +	} \
> +	\
> +	ver = ioread32(addr); \
> +	pci_iounmap(pdev, addr); \
> +	\
> +	RUNTIME_INFO(i915)->ri_prefix.ver = REG_FIELD_GET(GMD_ID_ARCH_MASK, ver); \
> +	RUNTIME_INFO(i915)->ri_prefix.rel = REG_FIELD_GET(GMD_ID_RELEASE_MASK, ver); \
> +	RUNTIME_INFO(i915)->ri_prefix.step = REG_FIELD_GET(GMD_ID_STEP, ver); \
> +	\
> +	/* Sanity check against expected versions from device info */ \
> +	if (RUNTIME_INFO(i915)->ri_prefix.ver != INTEL_INFO(i915)->ri_prefix.ver || \
> +	    RUNTIME_INFO(i915)->ri_prefix.rel > INTEL_INFO(i915)->ri_prefix.rel) \
> +		drm_dbg(&i915->drm, \
> +			"Hardware reports " #ri_prefix " IP version %u.%u but minimum expected is %u.%u\n", \
> +			RUNTIME_INFO(i915)->ri_prefix.ver, \
> +			RUNTIME_INFO(i915)->ri_prefix.rel, \
> +			INTEL_INFO(i915)->ri_prefix.ver, \
> +			INTEL_INFO(i915)->ri_prefix.rel); \
> +ri_prefix##done:
> +
> +/**
> + * intel_ipver_early_init - setup IP version values
> + * @dev_priv: device private
> + *
> + * Setup the graphics version for the current device.  This must be done before
> + * any code that performs checks on GRAPHICS_VER or DISPLAY_VER, so this
> + * function should be called very early in the driver initialization sequence.
> + *
> + * Regular MMIO access is not yet setup at the point this function is called so
> + * we peek at the appropriate MMIO offset directly.  The GMD_ID register is
> + * part of an 'always on' power well by design, so we don't need to worry about
> + * forcewake while reading it.
> + */
> +static void intel_ipver_early_init(struct drm_i915_private *i915)
> +{
> +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> +	void __iomem *addr;
> +	u32 ver;
> +
> +	if (!HAS_GMD_ID(i915)) {
> +		drm_WARN_ON(&i915->drm, INTEL_INFO(i915)->graphics.ver > 12);
> +
> +		RUNTIME_INFO(i915)->graphics.ver = INTEL_INFO(i915)->graphics.ver;
> +		RUNTIME_INFO(i915)->graphics.rel = INTEL_INFO(i915)->graphics.rel;
> +		/* media ver = graphics ver for older platforms */
> +		RUNTIME_INFO(i915)->media.ver = INTEL_INFO(i915)->graphics.ver;
> +		RUNTIME_INFO(i915)->media.rel = INTEL_INFO(i915)->graphics.rel;
> +		RUNTIME_INFO(i915)->display.ver = INTEL_INFO(i915)->display.ver;
> +		RUNTIME_INFO(i915)->display.rel = INTEL_INFO(i915)->display.rel;
> +		return;
> +	}
> +
> +	IP_VER_READ(i915_mmio_reg_offset(GMD_ID_GRAPHICS), graphics);
> +	IP_VER_READ(i915_mmio_reg_offset(GMD_ID_DISPLAY), display);
> +	IP_VER_READ(MTL_MEDIA_GSI_BASE + i915_mmio_reg_offset(GMD_ID_GRAPHICS),
> +		    media);
> +}
> +
>  /**
>   * i915_driver_early_probe - setup state not requiring device access
>   * @dev_priv: device private
> + * @ent: PCI device info entry matched
>   *
>   * Initialize everything that is a "SW-only" state, that is state not
>   * requiring accessing the device or exposing the driver via kernel internal
>   * or userspace interfaces. Example steps belonging here: lock initialization,
>   * system memory allocation, setting up device specific attributes and
>   * function hooks not requiring accessing the device.
> + *
> + * GRAPHICS_VER, DISPLAY_VER, etc. are not yet usable at this point.  For
This looks like an incomplete statement. Is it a typo or you missed the
following sentence?
>   */
>  static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
>  {
> @@ -855,13 +924,22 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		return PTR_ERR(i915);
>  
>  	/* Disable nuclear pageflip by default on pre-ILK */
> -	if (!i915->params.nuclear_pageflip && DISPLAY_VER(i915) < 5)
> +	if (!i915->params.nuclear_pageflip &&
> +	    !HAS_GMD_ID(i915) && DISPLAY_VER(i915) < 5)
DISPLAY_VER can't be used at this point in code till intel_ipver_early_init is
invoked. Can this code block be moved after intel_ipver_early_init()
call?

Regards,
Bala
>  		i915->drm.driver_features &= ~DRIVER_ATOMIC;
>  
>  	ret = pci_enable_device(pdev);
>  	if (ret)
>  		goto out_fini;
>  
> +	/*
> +	 * GRAPHICS_VER() and DISPLAY_VER() will return 0 before this is
> +	 * called, so we want to take care of this very early in the
> +	 * initialization process (as soon as we can peek into the MMIO BAR),
> +	 * even before we setup regular MMIO access.
> +	 */
> +	intel_ipver_early_init(i915);
> +
>  	ret = i915_driver_early_probe(i915);
>  	if (ret < 0)
>  		goto out_pci_disable;