On 16/06/2022 15:15, Jani Nikula wrote:
On Thu, 16 Jun 2022, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> 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@xxxxxxxxx> Signed-off-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx> --- 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); } else if (find_devid(devid, subplatform_g10_ids, ARRAY_SIZE(subplatform_g10_ids))) { mask = BIT(INTEL_SUBPLATFORM_G10); 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 5Ugh 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.
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)
