On Thu, 16 Jun 2022, Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:
On 16/06/2022 15:15, Jani Nikula wrote:
On Thu, 16 Jun 2022, Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:
On 16/06/2022 13:01, Anshuman Gupta wrote:
DG2 NB SKU need to distinguish between MBD and AIC to probe the VRAM Self Refresh feature support. Adding those sub platform accordingly.
Cc: Matt Roper matthew.d.roper@intel.com Signed-off-by: Anshuman Gupta anshuman.gupta@intel.com
drivers/gpu/drm/i915/i915_drv.h | 3 +++ drivers/gpu/drm/i915/intel_device_info.c | 21 +++++++++++++++++++++ drivers/gpu/drm/i915/intel_device_info.h | 11 +++++++---- include/drm/i915_pciids.h | 23 ++++++++++++++++------- 4 files changed, 47 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a5bc6a774c5a..f1f8699eedfd 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1007,10 +1007,13 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, #define IS_PONTEVECCHIO(dev_priv) IS_PLATFORM(dev_priv, INTEL_PONTEVECCHIO)
#define IS_DG2_G10(dev_priv) \
- IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G10_NB_MBD) || \ IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G10) #define IS_DG2_G11(dev_priv) \
- IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G11_NB_MBD) || \ IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G11) #define IS_DG2_G12(dev_priv) \
- IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G12_NB_MBD) || \ IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G12) #define IS_ADLS_RPLS(dev_priv) \ IS_SUBPLATFORM(dev_priv, INTEL_ALDERLAKE_S, INTEL_SUBPLATFORM_RPL)
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c index f0bf23726ed8..93da555adc4e 100644 --- a/drivers/gpu/drm/i915/intel_device_info.c +++ b/drivers/gpu/drm/i915/intel_device_info.c @@ -187,6 +187,18 @@ static const u16 subplatform_rpl_ids[] = { INTEL_RPLP_IDS(0), };
+static const u16 subplatform_g10_mb_mbd_ids[] = {
- INTEL_DG2_G10_NB_MBD_IDS(0),
+};
+static const u16 subplatform_g11_mb_mbd_ids[] = {
- INTEL_DG2_G11_NB_MBD_IDS(0),
+};
+static const u16 subplatform_g12_mb_mbd_ids[] = {
- INTEL_DG2_G12_NB_MBD_IDS(0),
+};
- static const u16 subplatform_g10_ids[] = { INTEL_DG2_G10_IDS(0), INTEL_ATS_M150_IDS(0),
@@ -246,6 +258,15 @@ void intel_device_info_subplatform_init(struct drm_i915_private *i915) } else if (find_devid(devid, subplatform_rpl_ids, ARRAY_SIZE(subplatform_rpl_ids))) { mask = BIT(INTEL_SUBPLATFORM_RPL);
- } else if (find_devid(devid, subplatform_g10_mb_mbd_ids,
ARRAY_SIZE(subplatform_g10_mb_mbd_ids))) {mask = BIT(INTEL_SUBPLATFORM_G10_NB_MBD);- } else if (find_devid(devid, subplatform_g11_mb_mbd_ids,
ARRAY_SIZE(subplatform_g11_mb_mbd_ids))) {mask = BIT(INTEL_SUBPLATFORM_G11_NB_MBD);- } else if (find_devid(devid, subplatform_g12_mb_mbd_ids,
ARRAY_SIZE(subplatform_g12_mb_mbd_ids))) {mask = BIT(INTEL_SUBPLATFORM_G12_NB_MBD);diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h index 08341174ee0a..c929e2d7e59c 100644 --- a/drivers/gpu/drm/i915/intel_device_info.h +++ b/drivers/gpu/drm/i915/intel_device_info.h @@ -97,7 +97,7 @@ enum intel_platform { * it is fine for the same bit to be used on multiple parent platforms. */
-#define INTEL_SUBPLATFORM_BITS (3) +#define INTEL_SUBPLATFORM_BITS (6) #define INTEL_SUBPLATFORM_MASK (BIT(INTEL_SUBPLATFORM_BITS) - 1)
/* HSW/BDW/SKL/KBL/CFL */ @@ -111,9 +111,12 @@ enum intel_platform { #define INTEL_SUBPLATFORM_UY (0)
/* DG2 */ -#define INTEL_SUBPLATFORM_G10 0 -#define INTEL_SUBPLATFORM_G11 1 -#define INTEL_SUBPLATFORM_G12 2 +#define INTEL_SUBPLATFORM_G10_NB_MBD 0 +#define INTEL_SUBPLATFORM_G11_NB_MBD 1 +#define INTEL_SUBPLATFORM_G12_NB_MBD 2 +#define INTEL_SUBPLATFORM_G10 3 +#define INTEL_SUBPLATFORM_G11 4 +#define INTEL_SUBPLATFORM_G12 5
Ugh I feel this "breaks" the subplatform idea.. feels like it is just too many bits when two separate sets of information get tracked (Gxx plus MBD).
I think they could be specified independent of each other, though. The subplatform if-else ladder would have to be replaced with independent ifs. You'd have the G10/G11/G12 and 1 bit separately for MBD.
Only the macros for PCI IDs need to be separate (MBD vs not). You'll then have:
static const u16 subplatform_g10_ids[] = { INTEL_DG2_G10_IDS(0), INTEL_DG2_G10_NB_MBD_IDS(0), INTEL_ATS_M150_IDS(0), };
Ditto for g11 and g12, and separately:
static const u16 subplatform_mbd_ids[] = { INTEL_DG2_G10_NB_MBD_IDS(0), INTEL_DG2_G11_NB_MBD_IDS(0), INTEL_DG2_G12_NB_MBD_IDS(0), };
The IS_DG2_G10() etc. macros would remain unchanged. IS_DG2_MBD() would only be IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_MBD).
Main point is, a platform could belong to multiple independent subplatforms.
Unless I'm missing something. ;)
How about a separate "is_mbd" flag in runtime_info? You can split the PCI IDs split as you have done, but do a search against the MBD ones and set the flag.
What I dislike about this is that it's really not *runtime* info in any sense, and it adds another way to define platform features. And we already have too many.
I was reluctant to suggest extending usage of subplatform bits in this way but it would be acceptable. My reservation/uncertainty was whether MBP is a "proper" subplatform. I see it's separate PCI IDs and even separate HW features, as seen in this series, but wasn't sure. Anyway, your proposal works for me. Better 4 bits than 6 so as much as possible remain for platform bits.
The alternative is separate struct intel_device_info with a static is_mbd flag. But the duplication there is also getting out of hands. C is really crap at this.
BR, Jani.
Regards,
Tvrtko
BR, Jani.
Regards,
Tvrtko
/* ADL */ #define INTEL_SUBPLATFORM_RPL 0 diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h index 4585fed4e41e..198be417bb2d 100644 --- a/include/drm/i915_pciids.h +++ b/include/drm/i915_pciids.h @@ -693,32 +693,41 @@ INTEL_VGA_DEVICE(0xA7A9, info)
/* DG2 */ -#define INTEL_DG2_G10_IDS(info) \ +#define INTEL_DG2_G10_NB_MBD_IDS(info) \ INTEL_VGA_DEVICE(0x5690, info), \ INTEL_VGA_DEVICE(0x5691, info), \
- INTEL_VGA_DEVICE(0x5692, info), \
- INTEL_VGA_DEVICE(0x5692, info)
+#define INTEL_DG2_G11_NB_MBD_IDS(info) \
- INTEL_VGA_DEVICE(0x5693, info), \
- INTEL_VGA_DEVICE(0x5694, info), \
- INTEL_VGA_DEVICE(0x5695, info)
+#define INTEL_DG2_G12_NB_MBD_IDS(info) \
- INTEL_VGA_DEVICE(0x5696, info), \
- INTEL_VGA_DEVICE(0x5697, info)
+#define INTEL_DG2_G10_IDS(info) \ INTEL_VGA_DEVICE(0x56A0, info), \ INTEL_VGA_DEVICE(0x56A1, info), \ INTEL_VGA_DEVICE(0x56A2, info)
#define INTEL_DG2_G11_IDS(info) \
INTEL_VGA_DEVICE(0x5693, info), \
INTEL_VGA_DEVICE(0x5694, info), \
INTEL_VGA_DEVICE(0x5695, info), \ INTEL_VGA_DEVICE(0x56A5, info), \ INTEL_VGA_DEVICE(0x56A6, info), \ INTEL_VGA_DEVICE(0x56B0, info), \ INTEL_VGA_DEVICE(0x56B1, info)
#define INTEL_DG2_G12_IDS(info) \
INTEL_VGA_DEVICE(0x5696, info), \
INTEL_VGA_DEVICE(0x5697, info), \ INTEL_VGA_DEVICE(0x56A3, info), \ INTEL_VGA_DEVICE(0x56A4, info), \ INTEL_VGA_DEVICE(0x56B2, info), \ INTEL_VGA_DEVICE(0x56B3, info)
#define INTEL_DG2_IDS(info) \
- INTEL_DG2_G10_NB_MBD_IDS(info), \
- INTEL_DG2_G11_NB_MBD_IDS(info), \
- INTEL_DG2_G12_NB_MBD_IDS(info), \ INTEL_DG2_G10_IDS(info), \ INTEL_DG2_G11_IDS(info), \ INTEL_DG2_G12_IDS(info)