Re: [Intel-gfx] [PATCH v2 6/9] drm/i915/dgfx: Setup VRAM SR with D3COLD

[Jani Nikula <jani.nikula@intel.com>]

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

-- 
Jani Nikula, Intel Open Source Graphics Center