[Intel-gfx] [PATCH v5 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6
Imre Deak
imre.deak at intel.com
Fri Jan 15 06:44:46 PST 2016
Adding Chris. We would need stolen_base and size early, could you check
if moving i915_gem_init_stolen() earlier based on the diff at the end
is ok?
On to, 2016-01-14 at 21:26 +0530, Sagar Arun Kamble wrote:
> RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6
> setup registers. If those are not setup Driver should not enable RC6.
> For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values
> to know if BIOS has enabled HW/SW RC6.
> This will also enable user to control RC6 using BIOS settings alone.
> RC6 related instability can be avoided by disabling via BIOS settings
> till driver fixes it.
>
> v2: Had placed logic in gen8 function by mistake. Fixed it.
> Ensuring RPM is not enabled in case BIOS disabled RC6.
>
> v3: Need to disable RPM if RC6 is disabled due to BIOS settings. (Daniel)
> Runtime PM enabling happens before gen9_enable_rc6.
> Moved the updation of enable_rc6 parameter in intel_uncore_sanitize.
>
> v4: Added elaborate check for BIOS RC6 setup. Prepared check_pctx for bxt. (Imre)
>
> v5: Caching reserved stolen base and size in the driver private data.
> Reorganized RC6 setup check. Moved from gen9_enable_rc6 to intel_uncore_sanitize. (Imre)
>
> Cc: Imre Deak <imre.deak at intel.com>
> Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++
> drivers/gpu/drm/i915/i915_gem_stolen.c | 39 ++++++++++++++++++----
> drivers/gpu/drm/i915/i915_reg.h | 11 +++++++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 59 ++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/i915/intel_uncore.c | 4 +++
> 7 files changed, 109 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cf7e0fc..0c8e61c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3223,6 +3223,7 @@ int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv,
> u64 end);
> void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
> struct drm_mm_node *node);
> +int i915_gem_init_stolen_reserved(struct drm_device *dev);
> int i915_gem_init_stolen(struct drm_device *dev);
> void i915_gem_cleanup_stolen(struct drm_device *dev);
> struct drm_i915_gem_object *
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index b448ad8..20ff6ca 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -343,6 +343,8 @@ struct i915_gtt {
>
> size_t stolen_size; /* Total size of stolen memory */
> size_t stolen_usable_size; /* Total size minus BIOS reserved */
> + size_t stolen_reserved_base;
> + size_t stolen_reserved_size;
> u64 mappable_end; /* End offset that we can CPU map */
> struct io_mapping *mappable; /* Mapping to our CPU mappable region */
> phys_addr_t mappable_base; /* PA of our GMADR */
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 3476877..d5a65d9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -386,14 +386,12 @@ static void bdw_get_stolen_reserved(struct drm_i915_private *dev_priv,
> *size = stolen_top - *base;
> }
>
> -int i915_gem_init_stolen(struct drm_device *dev)
> +int i915_gem_init_stolen_reserved(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - unsigned long reserved_total, reserved_base = 0, reserved_size;
> + unsigned long reserved_base = 0, reserved_size;
> unsigned long stolen_top;
>
> - mutex_init(&dev_priv->mm.stolen_lock);
> -
> #ifdef CONFIG_INTEL_IOMMU
> if (intel_iommu_gfx_mapped && INTEL_INFO(dev)->gen < 8) {
> DRM_INFO("DMAR active, disabling use of stolen memory\n");
> @@ -458,9 +456,38 @@ int i915_gem_init_stolen(struct drm_device *dev)
> return 0;
> }
>
> + dev_priv->gtt.stolen_reserved_base = reserved_base;
> + dev_priv->gtt.stolen_reserved_size = reserved_size;
> +
> + return 0;
> +}
> +
> +int i915_gem_init_stolen(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + unsigned long reserved_total;
> + unsigned long stolen_top;
> +
> + mutex_init(&dev_priv->mm.stolen_lock);
> +
> +#ifdef CONFIG_INTEL_IOMMU
> + if (intel_iommu_gfx_mapped && INTEL_INFO(dev)->gen < 8) {
> + DRM_INFO("DMAR active, disabling use of stolen memory\n");
> + return 0;
> + }
> +#endif
> +
> + if (dev_priv->gtt.stolen_size == 0)
> + return 0;
> +
> + if (dev_priv->mm.stolen_base == 0)
> + return 0;
> +
> + stolen_top = dev_priv->mm.stolen_base + dev_priv->gtt.stolen_size;
> +
> /* It is possible for the reserved area to end before the end of stolen
> - * memory, so just consider the start. */
> - reserved_total = stolen_top - reserved_base;
> + * memory, so just consider the start. */
> + reserved_total = stolen_top - dev_priv->gtt.stolen_reserved_base;
>
> DRM_DEBUG_KMS("Memory reserved for graphics device: %zuK, usable: %luK\n",
> dev_priv->gtt.stolen_size >> 10,
Hm, I don't see a reason to split i915_gem_init_stolen()
and i915_gem_init_stolen_reserved(). We could just bring
i915_gem_init_stolen() earlier (in a separate patch), see what I meant
at the end.
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 007ae83..19ddf79 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6765,6 +6765,16 @@ enum skl_disp_power_wells {
>
> #define VLV_PMWGICZ _MMIO(0x1300a4)
>
> +#define RC6_LOCATION _MMIO(0xD40)
> +#define RC6_CTX_IN_DRAM 1
> +#define RC6_CTX_BASE _MMIO(0xD48)
> +#define RC6_CTX_BASE_MASK 0xFFFFFFF0
> +#define PWRCTX_MAXCNT_RCSUNIT _MMIO(0x2054)
> +#define PWRCTX_MAXCNT_VCSUNIT0 _MMIO(0x12054)
> +#define PWRCTX_MAXCNT_BCSUNIT _MMIO(0x22054)
> +#define PWRCTX_MAXCNT_VECSUNIT _MMIO(0x1A054)
> +#define PWRCTX_MAXCNT_VCSUNIT1 _MMIO(0x1C054)
> +#define IDLE_TIME_MASK 0xFFFFF
> #define FORCEWAKE _MMIO(0xA18C)
> #define FORCEWAKE_VLV _MMIO(0x1300b0)
> #define FORCEWAKE_ACK_VLV _MMIO(0x1300b4)
> @@ -6903,6 +6913,7 @@ enum skl_disp_power_wells {
> #define GEN6_RPDEUC _MMIO(0xA084)
> #define GEN6_RPDEUCSW _MMIO(0xA088)
> #define GEN6_RC_STATE _MMIO(0xA094)
> +#define RC6_STATE (1<<18)
> #define GEN6_RC1_WAKE_RATE_LIMIT _MMIO(0xA098)
> #define GEN6_RC6_WAKE_RATE_LIMIT _MMIO(0xA09C)
> #define GEN6_RC6pp_WAKE_RATE_LIMIT _MMIO(0xA0A0)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0438b57..f22baef 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1564,6 +1564,7 @@ void skl_wm_get_hw_state(struct drm_device *dev);
> void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
> struct skl_ddb_allocation *ddb /* out */);
> uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
> +int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6);
>
> /* intel_sdvo.c */
> bool intel_sdvo_init(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 02fe081..c9a32a4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4518,12 +4518,68 @@ static void intel_print_rc6_info(struct drm_device *dev, u32 mode)
> (mode & GEN6_RC_CTL_RC6_ENABLE) ? "on" : "off");
> }
>
> -static int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6)
> +static bool bxt_check_bios_rc6_setup(const struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + bool enable_rc6 = true;
> + unsigned long rc6_ctx_base;
> + bool bios_hw_rc6_enabled, bios_sw_rc6_enabled;
> +
> + bios_hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) &
> + (GEN6_RC_CTL_RC6_ENABLE |
> + GEN6_RC_CTL_HW_ENABLE);
> + bios_sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) &
> + GEN6_RC_CTL_HW_ENABLE) &&
> + (I915_READ(GEN6_RC_STATE) & RC6_STATE);
> +
> + if (!(I915_READ(RC6_LOCATION) & RC6_CTX_IN_DRAM)) {
> + DRM_DEBUG_KMS("RC6 Base location not set properly.\n");
> + enable_rc6 = false;
> + }
> +
> + rc6_ctx_base = I915_READ(RC6_CTX_BASE) & RC6_CTX_BASE_MASK;
> + if (!((rc6_ctx_base >= dev_priv->gtt.stolen_reserved_base) &&
> + (rc6_ctx_base <= (dev_priv->gtt.stolen_reserved_base +
> + dev_priv->gtt.stolen_reserved_size)))) {
> + DRM_DEBUG_KMS("RC6 Base address not as expected.\n");
> + enable_rc6 = false;
> + }
> +
> + if (!(((I915_READ(PWRCTX_MAXCNT_RCSUNIT) & IDLE_TIME_MASK) > 1) &&
> + ((I915_READ(PWRCTX_MAXCNT_VCSUNIT0) & IDLE_TIME_MASK) > 1) &&
> + ((I915_READ(PWRCTX_MAXCNT_BCSUNIT) & IDLE_TIME_MASK) > 1) &&
> + ((I915_READ(PWRCTX_MAXCNT_VECSUNIT) & IDLE_TIME_MASK) > 1))) {
> + DRM_DEBUG_KMS("Engine Idle wait time not set properly.\n");
> + enable_rc6 = false;
> + }
> +
> + if (HAS_BSD2(dev) &&
> + ((I915_READ(PWRCTX_MAXCNT_VCSUNIT1) & IDLE_TIME_MASK) > 1)) {
> + DRM_DEBUG_KMS("VCSUNIT1 Idle wait time not set properly.\n");
> + enable_rc6 = false;
> + }
> +
> + if (!bios_hw_rc6_enabled && !bios_sw_rc6_enabled) {
> + DRM_DEBUG_KMS("HW/SW RC6 is not enabled by BIOS.\n");
> + enable_rc6 = false;
> + }
> +
> + return enable_rc6;
> +}
> +
> +int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6)
> {
> /* No RC6 before Ironlake and code is gone for ilk. */
> if (INTEL_INFO(dev)->gen < 6)
> return 0;
Please also add here an early return if enable_rc6 is already 0, so we
don't get redundant "RC6 disabled" messages in the log.
>
> + if (IS_BROXTON(dev)) {
> + if (!bxt_check_bios_rc6_setup(dev)) {
Nitpick, could be a single if.
> + DRM_INFO("RC6 disabled by BIOS\n");
> + return 0;
> + }
> + }
> +
> /* Respect the kernel parameter if it is set */
> if (enable_rc6 >= 0) {
> int mask;
> @@ -6014,7 +6070,6 @@ void intel_init_gt_powersave(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> - i915.enable_rc6 = sanitize_rc6_option(dev, i915.enable_rc6);
> /*
> * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
> * requirement.
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 277e60a..43fc3e8 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -359,6 +359,10 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake)
>
> void intel_uncore_sanitize(struct drm_device *dev)
> {
> + i915_gem_init_stolen_reserved(dev);
This would now be called both during driver loading and resume which is
redundant. Also it would run before intel_setup_mchbar() which isn't
good. So I would suggest instead moving i915_gem_init_stolen() earlier
according to the following and calling intel_uncore_sanitize() right
after it, provided that Chris doesn't see any problem with that:
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index a0f5659..f8cc66e 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -391,20 +391,13 @@ static int i915_load_modeset_init(struct drm_device *dev)
if (ret)
goto cleanup_vga_client;
- /* Initialise stolen first so that we may reserve preallocated
- * objects for the BIOS to KMS transition.
- */
- ret = i915_gem_init_stolen(dev);
- if (ret)
- goto cleanup_vga_switcheroo;
-
intel_power_domains_init_hw(dev_priv, false);
intel_csr_ucode_init(dev_priv);
ret = intel_irq_install(dev_priv);
if (ret)
- goto cleanup_gem_stolen;
+ goto cleanup_vga_switcheroo;
intel_setup_gmbus(dev);
@@ -458,8 +451,6 @@ cleanup_irq:
intel_guc_ucode_fini(dev);
drm_irq_uninstall(dev);
intel_teardown_gmbus(dev);
-cleanup_gem_stolen:
- i915_gem_cleanup_stolen(dev);
cleanup_vga_switcheroo:
vga_switcheroo_unregister_client(dev->pdev);
cleanup_vga_client:
@@ -1032,6 +1023,12 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
/* Try to make sure MCHBAR is enabled before poking at it */
intel_setup_mchbar(dev);
+ /* Initialise stolen first so that we may reserve preallocated
+ * objects for the BIOS to KMS transition.
+ */
+ ret = i915_gem_init_stolen(dev);
+ if (ret)
+ goto out_free_hangcheckwq;
intel_opregion_setup(dev);
i915_gem_load(dev);
@@ -1104,8 +1101,10 @@ out_gem_unload:
if (dev->pdev->msi_enabled)
pci_disable_msi(dev->pdev);
+ i915_gem_cleanup_stolen(dev);
intel_teardown_mchbar(dev);
pm_qos_remove_request(&dev_priv->pm_qos);
+out_free_hangcheckwq:
destroy_workqueue(dev_priv->gpu_error.hangcheck_wq);
out_freedpwq:
destroy_workqueue(dev_priv->hotplug.dp_wq);
> +
> + i915.enable_rc6 = sanitize_rc6_option(dev, i915.enable_rc6);
> +
> /* BIOS often leaves RC6 enabled, but disable it for hw init */
> intel_disable_gt_powersave(dev);
> }
More information about the Intel-gfx
mailing list