[Intel-gfx] [PATCH v6 1/3] drm/i915: Read graphics/media/display arch version from hw
Lucas De Marchi
lucas.demarchi at intel.com
Fri Sep 23 17:40:53 UTC 2022
On Thu, Sep 15, 2022 at 06:46:46PM -0700, Radhakrishna Sripada wrote:
>From: Matt Roper <matthew.d.roper at 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.
I'd just remove "New hardware programming ..." until the end, because:
1) Binding to the device will always happen by using the PCI ID.
2) I don't see us moving away from grouping them per-platform.
3) Flags may still be convenient to convey paths to take in the software
regardless of what the hardware provides.
Basically there is a new way to read the individual IP versions: we
didn't have that before and we were just hardcoding the info per
platform. And the new scheme for reading the stepping replaces what we
were reading before from PCI config space.
>
>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.
> - Use IP_VER for snaity check(MattR)
>
>v4:
> - Minor doccumentation changes.
> - Normalize HAS_GMD_ID macro value.(JaniN)
>
>Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at 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 | 67 +++++++++++++++++++++++-
> drivers/gpu/drm/i915/intel_device_info.h | 12 ++++-
> 7 files changed, 91 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 2275ee47da95..2d2044f2ed9d 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 c459eb362c47..e86798eaecb6 100644
>--- a/drivers/gpu/drm/i915/i915_driver.c
>+++ b/drivers/gpu/drm/i915/i915_driver.c
>@@ -337,7 +337,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);
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index 9f9372931fd2..7034ea848d65 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -940,6 +940,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 77e7df21f539..cace897e1db1 100644
>--- a/drivers/gpu/drm/i915/i915_pci.c
>+++ b/drivers/gpu/drm/i915/i915_pci.c
>@@ -1143,6 +1143,7 @@ static const struct intel_device_info mtl_info = {
> .display.has_modular_fia = 1,
> .extra_gt_list = xelpmp_extra_gt,
> .has_flat_ccs = 0,
>+ .has_gmd_id = 1,
heh... this basically proves what I just said above ;)
> .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 1a9bd829fc7e..acfcd155c8d0 100644
>--- a/drivers/gpu/drm/i915/i915_reg.h
>+++ b/drivers/gpu/drm/i915/i915_reg.h
>@@ -5839,6 +5839,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
>@@ -8356,4 +8361,6 @@ enum skl_power_gate {
> #define MTL_TRAS_MASK REG_GENMASK(16, 8)
> #define MTL_TRDPRE_MASK REG_GENMASK(7, 0)
>
>+#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 1434dc33cf49..3464de801b67 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,70 @@ 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;
s/ver/expected_ver/
s/rel/expected_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(ip->ver, ip->rel) < IP_VER(ver, rel))
>+ drm_dbg(&i915->drm,
>+ "Hardware reports GMD IP version %u.%u but minimum expected is %u.%u\n",
>+ ip->ver, ip->rel, ver, rel);
slightly more useful if it says what register this is about so we know
if it's graphics, media or display
drm_dbg(&i915->drm,
"Hardware reports GMD IP version %u.%u (REG[0x%x] = 0x%08x) but minimum expected is %u.%u\n",
ip->ver, ip->rel, offset, val, expected_ver, expected_rel);
>+}
>+
>+/*
>+ * 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 intel_runtime_info *runtime = RUNTIME_INFO(i915);
>+
>+ if (!HAS_GMD_ID(i915))
>+ return;
>+
>+ ip_ver_read(i915, i915_mmio_reg_offset(GMD_ID_GRAPHICS),
>+ &runtime->graphics.ip);
>+ ip_ver_read(i915, i915_mmio_reg_offset(GMD_ID_DISPLAY),
>+ &runtime->display.ip);
>+ ip_ver_read(i915, MTL_MEDIA_GSI_BASE + i915_mmio_reg_offset(GMD_ID_GRAPHICS),
this belongs to a GMD_ID_MEDIA macro?
#define GMD_ID_MEDIA (MTL_MEDIA_GSI_BASE + 0xd8c)
>+ &runtime->media.ip);
>+}
>+
>+/**
>+ * intel_device_info_runtime_init_early - initialize early runtime info
>+ * @i915: the i915 device
>+ *
>+ * Determine early intel_device_info fields at runtime. This function needs
>+ * to be called before the MMIO has been setup.
>+ */
>+void intel_device_info_runtime_init_early(struct drm_i915_private *i915)
>+{
>+ intel_ipver_early_init(i915);
>+ intel_device_info_subplatform_init(i915);
>+}
>+
> /**
> * intel_device_info_runtime_init - initialize runtime info
> * @dev_priv: the i915 device
>diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
>index 09b18910d3ab..e3aa88d33d63 100644
>--- a/drivers/gpu/drm/i915/intel_device_info.h
>+++ b/drivers/gpu/drm/i915/intel_device_info.h
>@@ -153,6 +153,7 @@ enum intel_ppgtt_type {
> func(has_4tile); \
> func(has_flat_ccs); \
> func(has_global_mocs); \
>+ func(has_gmd_id); \
> func(has_gt_uc); \
> func(has_heci_pxp); \
> func(has_heci_gscfi); \
>@@ -198,9 +199,18 @@ enum intel_ppgtt_type {
> struct ip_version {
> u8 ver;
> u8 rel;
>+ u8 step;
> };
>
> struct intel_runtime_info {
>+ /*
>+ * On modern platforms, the architecture major.minor version numbers
>+ * and stepping are read directly from the hardware rather than derived
>+ * from the PCI device and revision ID's.
>+ *
>+ * Note that the hardware gives us a single "graphics" number that
>+ * should represent render, compute, and copy behavior.
>+ */
humn... "modern" doesn't read nice a few months/years from now. This is
already in the intel_runtime_info so it's expected that it's not
hardcoded somewhere in the ro data
I'd just make this paragrapho read like:
/*
* Single "graphics" IP version that represents
* render, compute, and copy behavior.
*/
A few nits above, but this is in general good to me. Feel free
to add Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
Lucas De Marchi
> struct {
> struct ip_version ip;
> } graphics;
>@@ -309,7 +319,7 @@ struct intel_driver_caps {
>
> const char *intel_platform_name(enum intel_platform platform);
>
>-void intel_device_info_subplatform_init(struct drm_i915_private *dev_priv);
>+void intel_device_info_runtime_init_early(struct drm_i915_private *dev_priv);
> void intel_device_info_runtime_init(struct drm_i915_private *dev_priv);
>
> void intel_device_info_print(const struct intel_device_info *info,
>--
>2.34.1
>
More information about the Intel-gfx
mailing list