> -----Original Message-----
> From: Nikula, Jani <jani.nikula@xxxxxxxxx>
> Sent: Thursday, June 16, 2022 6:26 PM
> To: Gupta, Anshuman <anshuman.gupta@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>; Nilawar, Badal
> <badal.nilawar@xxxxxxxxx>; Ewins, Jon <jon.ewins@xxxxxxxxx>; Vivi, Rodrigo
> <rodrigo.vivi@xxxxxxxxx>; Ursulin, Tvrtko <tvrtko.ursulin@xxxxxxxxx>; Tangudu,
> Tilak <tilak.tangudu@xxxxxxxxx>; Gupta, Anshuman
> <anshuman.gupta@xxxxxxxxx>
> Subject: Re: [PATCH v2 1/9] drm/i915/dgfx: OpRegion VRAM Self Refresh
> Support
>
> On Thu, 16 Jun 2022, Anshuman Gupta <anshuman.gupta@xxxxxxxxx> 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@xxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
> > ---
> > 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
