Re: [PATCH v5 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6

[Imre Deak <imre.deak@intel.com>]

On pe, 2016-01-15 at 16:44 +0200, Imre Deak wrote:
> 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?

Oops, I meant stolen reserved_base and reserved_size.

> 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@intel.com>
> > Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87
> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@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(0x120
> > 54)
> > +#define  PWRCTX_MAXCNT_BCSUNIT			_MMIO(0x2205
> > 4)
> > +#define  PWRCTX_MAXCNT_VECSUNIT			_MMIO(0x1A0
> > 54)
> > +#define  PWRCTX_MAXCNT_VCSUNIT1			_MMIO(0x1C0
> > 54)
> > +#define  IDLE_TIME_MASK				0xFFFFF
> >  #define  FORCEWAKE				_MMIO(0xA18C)
> >  #define  FORCEWAKE_VLV				_MMIO(0x1300
> > b0)
> >  #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);
> >  }
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx