| Message ID | 20220616120106.24353-4-anshuman.gupta@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DG2 VRAM_SR Support | expand |
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); > } 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 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). 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. 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)
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); >> } 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 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. 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)
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); >>> } 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 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. 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) >
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); >>>> } 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 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) >>
On Thu, Jun 16, 2022 at 05:31:00PM +0530, 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), > +}; We only need a single MBD subplatform, not three new subplatforms. Unless I'm forgetting something, a single device ID can be assigned two two independent subplatforms at the same time. So the decision about whether to set the G10, G11, or G12 bit is one decision. The decision about whether to set the MBD bit is a completely separate decision that doesn't care about the G10/G11/G12 stuff. > + > 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); Assuming you consolidate MBD back down to just a single extra subplatform, the lookup and bit setting should happen in a separate 'if' statement (not an 'else' block). if (find_devid(devid, subplatform_mbd_ids, ARRAY_SIZE(subplatform_mbd_ids))) mask |= BIT(INTEL_SUBPLATFORM_MBD); Matt > } 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 5 > > /* 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) > -- > 2.26.2 >
> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@intel.com> > Sent: Friday, June 17, 2022 5:43 AM > To: Gupta, Anshuman <anshuman.gupta@intel.com> > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Nilawar, > Badal <badal.nilawar@intel.com>; Ewins, Jon <jon.ewins@intel.com>; Vivi, > Rodrigo <rodrigo.vivi@intel.com>; Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; > Tangudu, Tilak <tilak.tangudu@intel.com> > Subject: Re: [PATCH v2 3/9] drm/i915/dg2: Add DG2_NB_MBD subplatform > > On Thu, Jun 16, 2022 at 05:31:00PM +0530, 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), > > +}; > > We only need a single MBD subplatform, not three new subplatforms. > Unless I'm forgetting something, a single device ID can be assigned two two > independent subplatforms at the same time. So the decision about whether to > set the G10, G11, or G12 bit is one decision. The decision about whether to set > the MBD bit is a completely separate decision that doesn't care about the > G10/G11/G12 stuff. > > > + > > 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); > > Assuming you consolidate MBD back down to just a single extra subplatform, > the lookup and bit setting should happen in a separate 'if' > statement (not an 'else' block). > > if (find_devid(devid, subplatform_mbd_ids, > ARRAY_SIZE(subplatform_mbd_ids))) > mask |= BIT(INTEL_SUBPLATFORM_MBD); Thanks Matt , Jani and Tvrtko for review comment, I will create only INTEL_SUBPLATFORM_MBD and address it. Regards, Anshuman Gupta. > > > Matt > > > } 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 5 > > > > /* 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) > > -- > > 2.26.2 > > > > -- > Matt Roper > Graphics Software Engineer > VTT-OSGC Platform Enablement > Intel Corporation
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 5 /* 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)
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(-)