[Intel-gfx] [PATCH v2 1/9] drm/i915/dgfx: OpRegion VRAM Self Refresh Support

[Gupta, Anshuman]

> -----Original Message-----
> From: Nikula, Jani <jani.nikula at intel.com>
> Sent: Thursday, June 16, 2022 6:26 PM
> To: Gupta, Anshuman <anshuman.gupta at intel.com>; intel-
> gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
> Cc: Roper, Matthew D <matthew.d.roper at intel.com>; Nilawar, Badal
> <badal.nilawar at intel.com>; Ewins, Jon <jon.ewins at intel.com>; Vivi, Rodrigo
> <rodrigo.vivi at intel.com>; Ursulin, Tvrtko <tvrtko.ursulin at intel.com>; Tangudu,
> Tilak <tilak.tangudu at intel.com>; Gupta, Anshuman
> <anshuman.gupta at intel.com>
> Subject: Re: [PATCH v2 1/9] drm/i915/dgfx: OpRegion VRAM Self Refresh
> Support
> 
> On Thu, 16 Jun 2022, Anshuman Gupta <anshuman.gupta at 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 at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta at 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.
Thanks for I will fix all of above comment.
Regards,
Anshuman Gupta.
> 
> BR,
> Jani.
> 
> >  #endif /* CONFIG_ACPI */
> >
> >  #endif
> 
> --
> Jani Nikula, Intel Open Source Graphics Center