On Thu, 16 Jun 2022, Anshuman Gupta anshuman.gupta@intel.com wrote:

Intel DGFX cards provides a feature Video Ram Self Refrsh(VRSR). DGFX VRSR can be enabled with runtime suspend D3Cold flow and with opportunistic S0ix system wide suspend flow as well.

Without VRSR enablement i915 has to evict the lmem objects to system memory. Depending on some heuristics driver will evict lmem objects without VRSR.

VRSR feature requires Host BIOS support, VRSR will be enable/disable by HOST BIOS using ACPI OpRegion.

Adding OpRegion VRSR support in order to enable/disable VRSR on discrete cards.

BSpec: 53440 Cc: Jani Nikula jani.nikula@intel.com Cc: Rodrigo Vivi rodrigo.vivi@intel.com Signed-off-by: Anshuman Gupta anshuman.gupta@intel.com

---

drivers/gpu/drm/i915/display/intel_opregion.c | 62 ++++++++++++++++++- drivers/gpu/drm/i915/display/intel_opregion.h | 11 ++++ 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c index 6876ba30d5a9..11d8c5bb23ac 100644 --- a/drivers/gpu/drm/i915/display/intel_opregion.c +++ b/drivers/gpu/drm/i915/display/intel_opregion.c @@ -53,6 +53,8 @@ #define MBOX_ASLE_EXT BIT(4) /* Mailbox #5 */ #define MBOX_BACKLIGHT BIT(5) /* Mailbox #2 (valid from v3.x) */

+#define PCON_DGFX_BIOS_SUPPORTS_VRSR BIT(11) +#define PCON_DGFX_BIOS_SUPPORTS_VRSR_FIELD_VALID BIT(12) #define PCON_HEADLESS_SKU BIT(13)

struct opregion_header { @@ -130,7 +132,8 @@ struct opregion_asle { u64 rvda; /* Physical (2.0) or relative from opregion (2.1+) * address of raw VBT data. */ u32 rvds; /* Size of raw vbt data */

• u8 rsvd[58];

• u8 vrsr; /* DGFX Video Ram Self Refresh */
• u8 rsvd[57];

} __packed;

/* OpRegion mailbox #5: ASLE ext */ @@ -201,6 +204,9 @@ struct opregion_asle_ext {

#define ASLE_PHED_EDID_VALID_MASK 0x3

+/* VRAM SR */ +#define ASLE_VRSR_ENABLE BIT(0)

/* Software System Control Interrupt (SWSCI) */ #define SWSCI_SCIC_INDICATOR (1 << 0) #define SWSCI_SCIC_MAIN_FUNCTION_SHIFT 1 @@ -921,6 +927,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) opregion->header->over.minor, opregion->header->over.revision);

• drm_dbg(&dev_priv->drm, "OpRegion PCON values 0x%x\n", opregion->header->pcon);
• mboxes = opregion->header->mboxes; if (mboxes & MBOX_ACPI) { drm_dbg(&dev_priv->drm, "Public ACPI methods supported\n");

@@ -1246,3 +1254,55 @@ void intel_opregion_unregister(struct drm_i915_private *i915) opregion->vbt = NULL; opregion->lid_state = NULL; }

+/**

• • intel_opregion_bios_supports_vram_sr() get HOST BIOS VRAM Self
• • Refresh capability support.
• • @i915: pointer to i915 device.
• • It checks opregion pcon vram_sr fields to get HOST BIOS vram_sr
• • capability support. It is only applocable to DGFX.
• • Returns:
• • true when bios supports vram_sr, or false if bios doesn't support.
• */

+bool intel_opregion_bios_supports_vram_sr(struct drm_i915_private *i915) +{

• struct intel_opregion *opregion = &i915->opregion;
• if (!IS_DGFX(i915))

• return false;
  

• if (!opregion)

This is always true. You should check for !opregion->header.

• return false;
  

• if (opregion->header->pcon & PCON_DGFX_BIOS_SUPPORTS_VRSR_FIELD_VALID)

• return opregion->header->pcon & PCON_DGFX_BIOS_SUPPORTS_VRSR;
  

• else

• return false;
  

+}

+/**

• • intel_opregion_vram_sr() - enable/disable VRAM Self Refresh.
• • @i915: pointer to i915 device.
• • @enable: Argument to enable/disable VRSR.
• • It enables/disables vram_sr in opregion ASLE MBOX, based upon that
• • HOST BIOS will enables and disbales VRAM_SR during
• • ACPI _PS3/_OFF and _PS/_ON glue method.
• */

+void intel_opregion_vram_sr(struct drm_i915_private *i915, bool enable) +{

• struct intel_opregion *opregion = &i915->opregion;
• if (!opregion)

Same as above.

• return;
  

• if (drm_WARN(&i915->drm, !opregion->asle, "ASLE MAILBOX3 is not available\n"))

• return;
  

I'd just bundle !opregion->asle into the early return.

• if (enable)

• opregion->asle->vrsr |= ASLE_VRSR_ENABLE;
  

• else

• opregion->asle->vrsr &= ~ASLE_VRSR_ENABLE;
  

+} diff --git a/drivers/gpu/drm/i915/display/intel_opregion.h b/drivers/gpu/drm/i915/display/intel_opregion.h index 2f261f985400..73c9d81d5ee6 100644 --- a/drivers/gpu/drm/i915/display/intel_opregion.h +++ b/drivers/gpu/drm/i915/display/intel_opregion.h @@ -75,6 +75,8 @@ int intel_opregion_notify_adapter(struct drm_i915_private *dev_priv, pci_power_t state); int intel_opregion_get_panel_type(struct drm_i915_private *dev_priv); struct edid *intel_opregion_get_edid(struct intel_connector *connector); +bool intel_opregion_bios_supports_vram_sr(struct drm_i915_private *i915); +void intel_opregion_vram_sr(struct drm_i915_private *i915, bool enable);

bool intel_opregion_headless_sku(struct drm_i915_private *i915);

@@ -134,6 +136,15 @@ static inline bool intel_opregion_headless_sku(struct drm_i915_private *i915) return false; }

+static bool intel_opregion_bios_supports_vram_sr(struct drm_i915_private *i915) +{

• return false;

+}

+static void intel_opregion_vram_sr(struct drm_i915_private *i915, bool enable) +{ +}

Both of these stubs need to be static inline.

BR, Jani.

#endif /* CONFIG_ACPI */

#endif

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 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)

-----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

On Thu, 16 Jun 2022, Anshuman Gupta anshuman.gupta@intel.com wrote:

Setup VRAM Self Refresh with D3COLD state. VRAM Self Refresh will retain the context of VRAM, driver need to save any corresponding hardware state that needs to be restore on D3COLD exit, example PCI state.

Cc: Jani Nikula jani.nikula@intel.com Cc: Rodrigo Vivi rodrigo.vivi@intel.com Signed-off-by: Anshuman Gupta anshuman.gupta@intel.com

---

drivers/gpu/drm/i915/i915_driver.c | 2 ++ drivers/gpu/drm/i915/i915_drv.h | 7 +++++ drivers/gpu/drm/i915/i915_reg.h | 4 +++ drivers/gpu/drm/i915/intel_pcode.c | 28 +++++++++++++++++++ drivers/gpu/drm/i915/intel_pcode.h | 2 ++ drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_pm.h | 2 ++ 7 files changed, 88 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c index d26dcca7e654..aa1fb15b1f11 100644 --- a/drivers/gpu/drm/i915/i915_driver.c +++ b/drivers/gpu/drm/i915/i915_driver.c @@ -649,6 +649,8 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv) if (ret) goto err_msi;

• intel_pm_vram_sr_setup(dev_priv);
• /*
  • Fill the dram structure to get the system dram info. This will be
  • used for memory latency calculation.

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 7983b36c1720..09f53aeda8d0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -624,6 +624,13 @@ struct drm_i915_private { u32 bxt_phy_grc;

u32 suspend_count;

• struct {

• /* lock to protect vram_sr flags */
  

• struct mutex lock;
  

• bool supported;
  

• } vram_sr;
• struct i915_suspend_saved_registers regfile; struct vlv_s0ix_state *vlv_s0ix_state;

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 932bd6aa4a0a..0e3dc4a8846a 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6766,6 +6766,8 @@ #define DG1_PCODE_STATUS 0x7E #define DG1_UNCORE_GET_INIT_STATUS 0x0 #define DG1_UNCORE_INIT_STATUS_COMPLETE 0x1 +#define DG1_PCODE_D3_VRAM_SR 0x71 +#define DG1_ENABLE_SR 0x1 #define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US 0x23 #define XEHPSDV_PCODE_FREQUENCY_CONFIG 0x6e /* xehpsdv, pvc */ /* XEHPSDV_PCODE_FREQUENCY_CONFIG sub-commands (param1) */ @@ -6779,6 +6781,8 @@ #define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8 #define GEN6_PCODE_FREQ_RING_RATIO_SHIFT 16 #define GEN6_PCODE_DATA1 _MMIO(0x13812C) +#define VRAM_CAPABILITY _MMIO(0x138144) +#define VRAM_SUPPORTED REG_BIT(0)

/* IVYBRIDGE DPF */ #define GEN7_L3CDERRST1(slice) _MMIO(0xB008 + (slice) * 0x200) /* L3CD Error Status 1 */ diff --git a/drivers/gpu/drm/i915/intel_pcode.c b/drivers/gpu/drm/i915/intel_pcode.c index a234d9b4ed14..88bd1f44cfb2 100644 --- a/drivers/gpu/drm/i915/intel_pcode.c +++ b/drivers/gpu/drm/i915/intel_pcode.c @@ -246,3 +246,31 @@ int snb_pcode_write_p(struct intel_uncore *uncore, u32 mbcmd, u32 p1, u32 p2, u3

return err; }

+/**

• • intel_pcode_enable_vram_sr - Enable pcode vram_sr.
• • @dev_priv: i915 device
• • This function triggers the required pcode flow to enable vram_sr.
• • This function stictly need to call from rpm handlers, as i915 is
• • transitioning to rpm idle/suspend, it doesn't require to grab
• • rpm wakeref.
• • Returns:
• • returns returned value from pcode mbox write.
• */

+int intel_pcode_enable_vram_sr(struct drm_i915_private *i915) +{

• int ret = 0;
• if (!HAS_LMEM_SR(i915))

• return ret;
  

• ret = snb_pcode_write(&i915->uncore,

• 	      REG_FIELD_PREP(GEN6_PCODE_MB_COMMAND,
  

• 	      DG1_PCODE_D3_VRAM_SR) |
  

• 	      REG_FIELD_PREP(GEN6_PCODE_MB_PARAM1,
  

• 	      DG1_ENABLE_SR), 0); /* no data needed for this cmd */
  

• return ret;

+}

This function doesn't belong here. intel_pcode.c provides the *mechanisms* for pcode access, not specific stuff like this. Just put this near the use in intel_pm.c I think.

diff --git a/drivers/gpu/drm/i915/intel_pcode.h b/drivers/gpu/drm/i915/intel_pcode.h index 8d2198e29422..295594514d49 100644 --- a/drivers/gpu/drm/i915/intel_pcode.h +++ b/drivers/gpu/drm/i915/intel_pcode.h @@ -9,6 +9,7 @@ #include <linux/types.h>

struct intel_uncore; +struct drm_i915_private;

int snb_pcode_read(struct intel_uncore *uncore, u32 mbox, u32 *val, u32 *val1); int snb_pcode_write_timeout(struct intel_uncore *uncore, u32 mbox, u32 val, @@ -26,5 +27,6 @@ int intel_pcode_init(struct intel_uncore *uncore); */ int snb_pcode_read_p(struct intel_uncore *uncore, u32 mbcmd, u32 p1, u32 p2, u32 *val); int snb_pcode_write_p(struct intel_uncore *uncore, u32 mbcmd, u32 p1, u32 p2, u32 val); +int intel_pcode_enable_vram_sr(struct drm_i915_private *i915);

#endif /* _INTEL_PCODE_H */ diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 5a61fc3f26c1..299fbc5375a9 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -8166,6 +8166,49 @@ void intel_pm_setup(struct drm_i915_private *dev_priv) atomic_set(&dev_priv->runtime_pm.wakeref_count, 0); }

+void intel_pm_vram_sr_setup(struct drm_i915_private *i915) +{

• if (!HAS_LMEM_SR(i915))

• return;
  

• mutex_init(&i915->vram_sr.lock);
• i915->vram_sr.supported = intel_uncore_read(&i915->uncore,

• 				    VRAM_CAPABILITY) & VRAM_SUPPORTED;
  

• if (intel_opregion_vram_sr_required(i915))

• i915->vram_sr.supported = i915->vram_sr.supported &&
  

• 				intel_opregion_bios_supports_vram_sr(i915);
  

+}

+int intel_pm_vram_sr(struct drm_i915_private *i915, bool enable) +{

• int ret = 0;
• if (!HAS_LMEM_SR(i915))

• return -EOPNOTSUPP;
  

You can drop this and only look at i915->vram_sr.supported.

• mutex_lock(&i915->vram_sr.lock);
• if (!i915->vram_sr.supported) {

• drm_dbg(&i915->drm, "VRAM Self Refresh is not supported\n");
  

• ret = -EOPNOTSUPP;
  

• goto unlock;
  

• }

This part doesn't need the mutex protection. You don't actually change i915->vram_sr.supported anywhere after initialization.

• drm_dbg(&i915->drm, "VRAM Self Refresh supported\n");
• if (enable)

• ret = intel_pcode_enable_vram_sr(i915);
  

• if (ret)

• goto unlock;
  

• intel_opregion_vram_sr(i915, enable);

+unlock:

• mutex_unlock(&i915->vram_sr.lock);
• return ret;

+}

static struct intel_global_state *intel_dbuf_duplicate_state(struct intel_global_obj *obj) { struct intel_dbuf_state *dbuf_state; diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h index 50604cf7398c..0da85d6b9ea7 100644 --- a/drivers/gpu/drm/i915/intel_pm.h +++ b/drivers/gpu/drm/i915/intel_pm.h @@ -31,6 +31,8 @@ int ilk_wm_max_level(const struct drm_i915_private *dev_priv); void intel_init_pm(struct drm_i915_private *dev_priv); void intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv); void intel_pm_setup(struct drm_i915_private *dev_priv); +void intel_pm_vram_sr_setup(struct drm_i915_private *i915); +int intel_pm_vram_sr(struct drm_i915_private *i915, bool enable); void g4x_wm_get_hw_state(struct drm_i915_private *dev_priv); void vlv_wm_get_hw_state(struct drm_i915_private *dev_priv); void ilk_wm_get_hw_state(struct drm_i915_private *dev_priv);

On Thu, 16 Jun 2022, Anshuman Gupta anshuman.gupta@intel.com wrote:

Intel Client DGFX card supports D3Cold with two option. D3Cold-off zero watt, D3Cold-VRAM Self Refresh.

i915 requires to evict the lmem objects to smem in order to support D3Cold-Off, which increases i915 the suspend/resume latency. Enabling VRAM Self Refresh feature optimize the latency with additional power cost which required to retain the lmem.

Adding intel_runtime_idle (runtime_idle callback) to enable VRAM_SR, it will be used for policy to choose between D3Cold-off vs D3Cold-VRAM_SR.

Since we have introduced i915 runtime_idle callback. It need to be warranted that Runtime PM Core invokes runtime_idle callback when runtime usages count becomes zero. That requires to use pm_runtime_put instead of pm_runtime_put_autosuspend.

TODO: GuC interface state save/restore.

Cc: Rodrigo Vivi rodrigo.vivi@intel.com Cc: Chris Wilson chris.p.wilson@intel.com Signed-off-by: Anshuman Gupta anshuman.gupta@intel.com

---

drivers/gpu/drm/i915/i915_driver.c | 26 +++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +-- 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c index aa1fb15b1f11..fcff5f3fe05e 100644 --- a/drivers/gpu/drm/i915/i915_driver.c +++ b/drivers/gpu/drm/i915/i915_driver.c @@ -1557,6 +1557,31 @@ static int i915_pm_restore(struct device *kdev) return i915_pm_resume(kdev); }

+static int intel_runtime_idle(struct device *kdev) +{

• struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
• int ret = 1;
• if (!HAS_LMEM_SR(dev_priv)) {

• /*TODO: Prepare for D3Cold-Off */
  

• goto out;
  

• }
• disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
• ret = intel_pm_vram_sr(dev_priv, true);
• if (!ret)

• drm_dbg(&dev_priv->drm, "VRAM Self Refresh enabled\n");
  

Please add the debug in the intel_pm_vram_sr() function instead.

BR, Jani.

• enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);

+out:

• pm_runtime_mark_last_busy(kdev);
• pm_runtime_autosuspend(kdev);
• return ret;

+}

static int intel_runtime_suspend(struct device *kdev) { struct drm_i915_private *dev_priv = kdev_to_i915(kdev); @@ -1742,6 +1767,7 @@ const struct dev_pm_ops i915_pm_ops = { .restore = i915_pm_restore,

/* S0ix (via runtime suspend) event handlers */

• .runtime_idle = intel_runtime_idle, .runtime_suspend = intel_runtime_suspend, .runtime_resume = intel_runtime_resume,

}; diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 6ed5786bcd29..4dade7e8a795 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -492,8 +492,7 @@ static void __intel_runtime_pm_put(struct intel_runtime_pm *rpm,

intel_runtime_pm_release(rpm, wakelock);

• pm_runtime_mark_last_busy(kdev);
• pm_runtime_put_autosuspend(kdev);

• pm_runtime_put(kdev);

}

/**

Hi,

On Thu, Jun 16, 2022 at 05:31:05PM +0530, Anshuman Gupta wrote:

From: Tvrtko Ursulin tvrtko.ursulin@intel.com

Store a pointer to respective local memory region in intel_gt so it can be used when memory local to a GT needs to be allocated.

Cc: Andi Shyti andi.shyti@intel.com Signed-off-by: Tvrtko Ursulin tvrtko.ursulin@intel.com Signed-off-by: Anshuman Gupta anshuman.gupta@intel.com

---

drivers/gpu/drm/i915/gt/intel_gt.c | 1 + drivers/gpu/drm/i915/gt/intel_gt_types.h | 3 +++ 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index f33290358c51..7a535f670ae1 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -91,6 +91,7 @@ static int intel_gt_probe_lmem(struct intel_gt *gt) GEM_BUG_ON(!HAS_REGION(i915, id)); GEM_BUG_ON(i915->mm.regions[id]); i915->mm.regions[id] = mem;

• gt->lmem = mem;

  return 0;

} diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h index df708802889d..cd7744eaaeaa 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h @@ -23,6 +23,7 @@ #include "intel_gt_buffer_pool_types.h" #include "intel_hwconfig.h" #include "intel_llc_types.h" +#include "intel_memory_region.h" #include "intel_reset_types.h" #include "intel_rc6_types.h" #include "intel_rps_types.h" @@ -202,6 +203,8 @@ struct intel_gt { */ phys_addr_t phys_addr;

• struct intel_memory_region *lmem;

this was somewhere in my next patch that is getting very delayed... anyway, with Jani's include note:

Reviewed-by: Andi Shyti andi.shyti@linux.intel.com

Thanks, Andi

struct intel_gt_info { unsigned int id;

-- 2.26.2

On Thu, 16 Jun 2022, Anshuman Gupta anshuman.gupta@intel.com wrote:

Add d3cold_sr_lmem_threshold modparam to choose between d3cold-off zero watt and d3cold-VRAM Self Refresh. i915 requires to evict the lmem objects to smem in order to support d3cold-Off.

If gfx root port is not capable of sending PME from d3cold then i915 don't need to program d3cold-off/d3cold-vram_sr sequence.

FIXME: Eviction of lmem objects in case of D3Cold off is wip.

Cc: Rodrigo Vivi rodrigo.vivi@intel.com Signed-off-by: Anshuman Gupta anshuman.gupta@intel.com

---

drivers/gpu/drm/i915/i915_driver.c | 27 ++++++++++++++++++++++++--- drivers/gpu/drm/i915/i915_params.c | 4 ++++ drivers/gpu/drm/i915/i915_params.h | 3 ++- 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c index fcff5f3fe05e..aef4b17efdbe 100644 --- a/drivers/gpu/drm/i915/i915_driver.c +++ b/drivers/gpu/drm/i915/i915_driver.c @@ -1560,15 +1560,36 @@ static int i915_pm_restore(struct device *kdev) static int intel_runtime_idle(struct device *kdev) { struct drm_i915_private *dev_priv = kdev_to_i915(kdev);

• struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
• u64 lmem_total = to_gt(dev_priv)->lmem->total;
• u64 lmem_avail = to_gt(dev_priv)->lmem->avail;
• u64 lmem_used = lmem_total - lmem_avail;
• struct pci_dev *root_pdev; int ret = 1;

• if (!HAS_LMEM_SR(dev_priv)) {

• /*TODO: Prepare for D3Cold-Off */
  

• root_pdev = pcie_find_root_port(pdev);
• if (!root_pdev)

• goto out;
  

• if (!pci_pme_capable(root_pdev, PCI_D3cold)) goto out;

• }

  disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);

• if (lmem_used < dev_priv->params.d3cold_sr_lmem_threshold * 1024 * 1024) {

• drm_dbg(&dev_priv->drm, "Prepare for D3Cold off\n");
  

• pci_d3cold_enable(root_pdev);
  

• /* FIXME: Eviction of lmem objects and guc reset is wip */
  

• intel_pm_vram_sr(dev_priv, false);
  

• enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
  

• goto out;
  

• } else if (!HAS_LMEM_SR(dev_priv)) {

• /* Disable D3Cold to reduce the eviction latency */
  

• pci_d3cold_disable(root_pdev);
  

• enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
  

• goto out;
  

• }

This is *way* too low level code for such high level function. This needs to be abstracted better.

• ret = intel_pm_vram_sr(dev_priv, true); if (!ret) drm_dbg(&dev_priv->drm, "VRAM Self Refresh enabled\n");

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 701fbc98afa0..6c6b3c372d4d 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -197,6 +197,10 @@ i915_param_named(enable_gvt, bool, 0400, "Enable support for Intel GVT-g graphics virtualization host support(default:false)"); #endif

+i915_param_named_unsafe(d3cold_sr_lmem_threshold, int, 0400,

• "Enable Vidoe RAM Self refresh when size of lmem is greater to this threshold. "
• "It helps to optimize the suspend/resume latecy. (default: 300mb)");

#if CONFIG_DRM_I915_REQUEST_TIMEOUT i915_param_named_unsafe(request_timeout_ms, uint, 0600, "Default request/fence/batch buffer expiration timeout."); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index b5e7ea45d191..28f20ebaf41f 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -83,7 +83,8 @@ struct drm_printer; param(bool, verbose_state_checks, true, 0) \ param(bool, nuclear_pageflip, false, 0400) \ param(bool, enable_dp_mst, true, 0600) \

• param(bool, enable_gvt, false, IS_ENABLED(CONFIG_DRM_I915_GVT) ? 0400 : 0)

• param(bool, enable_gvt, false, IS_ENABLED(CONFIG_DRM_I915_GVT) ? 0400 : 0) \
• param(int, d3cold_sr_lmem_threshold, 300, 0600) \

What's the point of the parameter?

Also, please read the comment /* leave bools at the end to not create holes */ above.

BR, Jani.

#define MEMBER(T, member, ...) T member; struct i915_params {