* [PATCH 1/1] drm/i915/gen9: Check BIOS RC6 setup before enabling RC6 @ 2015-10-27 9:38 Sagar Arun Kamble 2015-10-30 11:30 ` [PATCH v2 " Sagar Arun Kamble 0 siblings, 1 reply; 10+ messages in thread From: Sagar Arun Kamble @ 2015-10-27 9:38 UTC (permalink / raw) To: intel-gfx; +Cc: shashidhar.hiremath RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6 configuration 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. Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87 Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 9dda3ea..8c595e0 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4836,6 +4836,15 @@ static void gen8_enable_rps(struct drm_device *dev) struct intel_engine_cs *ring; uint32_t rc6_mask = 0; int unused; + bool hw_rc6_enabled, sw_rc6_enabled; + + /* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */ + hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) & + (GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE); + sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE) + && (I915_READ(GEN6_RC_STATE) & 0x40000); + if (!(hw_rc6_enabled || sw_rc6_enabled)) + i915.enable_rc6 = 0; /* 1a: Software RC state - RC0 */ I915_WRITE(GEN6_RC_STATE, 0); -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 1/1] drm/i915/gen9: Check BIOS RC6 setup before enabling RC6 2015-10-27 9:38 [PATCH 1/1] drm/i915/gen9: Check BIOS RC6 setup before enabling RC6 Sagar Arun Kamble @ 2015-10-30 11:30 ` Sagar Arun Kamble 2015-10-30 16:08 ` Daniel Vetter 2015-10-30 16:09 ` Daniel Vetter 0 siblings, 2 replies; 10+ messages in thread From: Sagar Arun Kamble @ 2015-10-30 11:30 UTC (permalink / raw) To: intel-gfx RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6 configuration 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. v2: Had placed logic in gen8 function by mistake. Fixed it. Ensuring RPM is not enabled in case BIOS disabled RC6. Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87 Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2c7c9fc..1ec52f2 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -30,6 +30,7 @@ #include "intel_drv.h" #include "../../../platform/x86/intel_ips.h" #include <linux/module.h> +#include <linux/pm_runtime.h> /** * RC6 is a special power stage which allows the GPU to enter an very @@ -4763,6 +4764,20 @@ static void gen9_enable_rc6(struct drm_device *dev) struct intel_engine_cs *ring; uint32_t rc6_mask = 0; int unused; + bool hw_rc6_enabled, sw_rc6_enabled; + struct device *device = &dev->pdev->dev; + + /* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */ + hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) & + (GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE); + sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE) + && (I915_READ(GEN6_RC_STATE) & 0x40000); + if (!(hw_rc6_enabled || sw_rc6_enabled)) { + i915.enable_rc6 = 0; + pm_runtime_forbid(device); + DRM_INFO("RC6 disabled by BIOS, disabled runtime PM support\n"); + } + /* 1a: Software RC state - RC0 */ I915_WRITE(GEN6_RC_STATE, 0); -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] drm/i915/gen9: Check BIOS RC6 setup before enabling RC6 2015-10-30 11:30 ` [PATCH v2 " Sagar Arun Kamble @ 2015-10-30 16:08 ` Daniel Vetter 2015-10-30 16:09 ` Daniel Vetter 1 sibling, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2015-10-30 16:08 UTC (permalink / raw) To: Sagar Arun Kamble; +Cc: intel-gfx On Fri, Oct 30, 2015 at 05:00:49PM +0530, Sagar Arun Kamble wrote: > RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6 > configuration 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. > > v2: Had placed logic in gen8 function by mistake. Fixed it. Ensuring RPM is > not enabled in case BIOS disabled RC6. > > Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87 > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 2c7c9fc..1ec52f2 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -30,6 +30,7 @@ > #include "intel_drv.h" > #include "../../../platform/x86/intel_ips.h" > #include <linux/module.h> > +#include <linux/pm_runtime.h> > > /** > * RC6 is a special power stage which allows the GPU to enter an very > @@ -4763,6 +4764,20 @@ static void gen9_enable_rc6(struct drm_device *dev) > struct intel_engine_cs *ring; > uint32_t rc6_mask = 0; > int unused; > + bool hw_rc6_enabled, sw_rc6_enabled; > + struct device *device = &dev->pdev->dev; > + > + /* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */ > + hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) & > + (GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE); > + sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE) > + && (I915_READ(GEN6_RC_STATE) & 0x40000); Please add a define for the magic value 0x40000. Also, should we check this on earlier platforms too? In that case a small helper and calling it everywhere would be great. -Daniel > + if (!(hw_rc6_enabled || sw_rc6_enabled)) { > + i915.enable_rc6 = 0; > + pm_runtime_forbid(device); > + DRM_INFO("RC6 disabled by BIOS, disabled runtime PM support\n"); > + } > + > > /* 1a: Software RC state - RC0 */ > I915_WRITE(GEN6_RC_STATE, 0); > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] drm/i915/gen9: Check BIOS RC6 setup before enabling RC6 2015-10-30 11:30 ` [PATCH v2 " Sagar Arun Kamble 2015-10-30 16:08 ` Daniel Vetter @ 2015-10-30 16:09 ` Daniel Vetter 2015-11-04 10:04 ` [PATCH v3 1/1] drm/i915/bxt: " Sagar Arun Kamble 1 sibling, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2015-10-30 16:09 UTC (permalink / raw) To: Sagar Arun Kamble; +Cc: intel-gfx On Fri, Oct 30, 2015 at 05:00:49PM +0530, Sagar Arun Kamble wrote: > RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6 > configuration 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. > > v2: Had placed logic in gen8 function by mistake. Fixed it. Ensuring RPM is > not enabled in case BIOS disabled RC6. > > Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87 > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 2c7c9fc..1ec52f2 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -30,6 +30,7 @@ > #include "intel_drv.h" > #include "../../../platform/x86/intel_ips.h" > #include <linux/module.h> > +#include <linux/pm_runtime.h> > > /** > * RC6 is a special power stage which allows the GPU to enter an very > @@ -4763,6 +4764,20 @@ static void gen9_enable_rc6(struct drm_device *dev) > struct intel_engine_cs *ring; > uint32_t rc6_mask = 0; > int unused; > + bool hw_rc6_enabled, sw_rc6_enabled; > + struct device *device = &dev->pdev->dev; > + > + /* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */ > + hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) & > + (GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE); > + sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE) > + && (I915_READ(GEN6_RC_STATE) & 0x40000); > + if (!(hw_rc6_enabled || sw_rc6_enabled)) { > + i915.enable_rc6 = 0; > + pm_runtime_forbid(device); > + DRM_INFO("RC6 disabled by BIOS, disabled runtime PM support\n"); One more: You don't disable runtime PM here, only RC6. Please fix this. -Daniel > + } > + > > /* 1a: Software RC state - RC0 */ > I915_WRITE(GEN6_RC_STATE, 0); > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6 2015-10-30 16:09 ` Daniel Vetter @ 2015-11-04 10:04 ` Sagar Arun Kamble 2015-11-17 16:45 ` Daniel Vetter 2015-11-26 20:59 ` Imre Deak 0 siblings, 2 replies; 10+ messages in thread From: Sagar Arun Kamble @ 2015-11-04 10:04 UTC (permalink / raw) To: intel-gfx; +Cc: shashidhar.hiremath 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. RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6 configuration 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. 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. Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87 Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_uncore.c | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 8942532..6ed3542 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6823,6 +6823,7 @@ enum skl_disp_power_wells { #define GEN6_RPDEUC 0xA084 #define GEN6_RPDEUCSW 0xA088 #define GEN6_RC_STATE 0xA094 +#define RC6_STATE (1<<18) #define GEN6_RC1_WAKE_RATE_LIMIT 0xA098 #define GEN6_RC6_WAKE_RATE_LIMIT 0xA09C #define GEN6_RC6pp_WAKE_RATE_LIMIT 0xA0A0 diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index f0f97b2..bedb089 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -364,8 +364,35 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake) i915_check_and_clear_faults(dev); } +static void sanitize_bios_rc6_setup(const struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + bool hw_rc6_enabled = false, sw_rc6_enabled = false; + + if (IS_BROXTON(dev)) { + /* Get forcewake during program sequence. Although the driver + * hasn't enabled a state yet where we need forcewake, BIOS may have.*/ + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); + + /* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */ + hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) & + (GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE); + sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE) + && (I915_READ(GEN6_RC_STATE) & RC6_STATE); + + intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); + + if (!hw_rc6_enabled && !sw_rc6_enabled) { + i915.enable_rc6 = 0; + DRM_INFO("RC6 disabled by BIOS\n"); + } + } +} + void intel_uncore_sanitize(struct drm_device *dev) { + sanitize_bios_rc6_setup(dev); + /* BIOS often leaves RC6 enabled, but disable it for hw init */ intel_disable_gt_powersave(dev); } -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6 2015-11-04 10:04 ` [PATCH v3 1/1] drm/i915/bxt: " Sagar Arun Kamble @ 2015-11-17 16:45 ` Daniel Vetter 2015-11-17 16:47 ` Daniel Vetter 2015-11-26 20:59 ` Imre Deak 1 sibling, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2015-11-17 16:45 UTC (permalink / raw) To: Sagar Arun Kamble; +Cc: intel-gfx, shashidhar.hiremath On Wed, Nov 04, 2015 at 03:34:54PM +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. > > RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6 configuration 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. > > 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. > > Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87 > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_uncore.c | 27 +++++++++++++++++++++++++++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 8942532..6ed3542 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6823,6 +6823,7 @@ enum skl_disp_power_wells { > #define GEN6_RPDEUC 0xA084 > #define GEN6_RPDEUCSW 0xA088 > #define GEN6_RC_STATE 0xA094 > +#define RC6_STATE (1<<18) > #define GEN6_RC1_WAKE_RATE_LIMIT 0xA098 > #define GEN6_RC6_WAKE_RATE_LIMIT 0xA09C > #define GEN6_RC6pp_WAKE_RATE_LIMIT 0xA0A0 > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index f0f97b2..bedb089 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -364,8 +364,35 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake) > i915_check_and_clear_faults(dev); > } > > +static void sanitize_bios_rc6_setup(const struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + bool hw_rc6_enabled = false, sw_rc6_enabled = false; > + > + if (IS_BROXTON(dev)) { > + /* Get forcewake during program sequence. Although the driver > + * hasn't enabled a state yet where we need forcewake, BIOS may have.*/ > + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); > + > + /* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */ > + hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) & > + (GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE); > + sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE) > + && (I915_READ(GEN6_RC_STATE) & RC6_STATE); > + > + intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); > + > + if (!hw_rc6_enabled && !sw_rc6_enabled) { > + i915.enable_rc6 = 0; > + DRM_INFO("RC6 disabled by BIOS\n"); > + } > + } > +} > + > void intel_uncore_sanitize(struct drm_device *dev) > { > + sanitize_bios_rc6_setup(dev); Why did you move this out of the enable_rc6 functions? It seems to fit much better there, instead of here. Also I think we shouldn't change the module option, instead it's conceptually cleaner to just not set up rc6 in genX_enable_rc6, i.e. if (!check_bios_rc6_setup()) return; somewhere at the beginning of gen9_enable_rc6 like you had in the previous patch. -Daniel > + > /* BIOS often leaves RC6 enabled, but disable it for hw init */ > intel_disable_gt_powersave(dev); > } > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6 2015-11-17 16:45 ` Daniel Vetter @ 2015-11-17 16:47 ` Daniel Vetter 0 siblings, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2015-11-17 16:47 UTC (permalink / raw) To: Sagar Arun Kamble; +Cc: intel-gfx, shashidhar.hiremath On Tue, Nov 17, 2015 at 05:45:06PM +0100, Daniel Vetter wrote: > On Wed, Nov 04, 2015 at 03:34:54PM +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. > > > > RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6 configuration 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. > > > > 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. > > > > Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87 > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > drivers/gpu/drm/i915/intel_uncore.c | 27 +++++++++++++++++++++++++++ > > 2 files changed, 28 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 8942532..6ed3542 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -6823,6 +6823,7 @@ enum skl_disp_power_wells { > > #define GEN6_RPDEUC 0xA084 > > #define GEN6_RPDEUCSW 0xA088 > > #define GEN6_RC_STATE 0xA094 > > +#define RC6_STATE (1<<18) > > #define GEN6_RC1_WAKE_RATE_LIMIT 0xA098 > > #define GEN6_RC6_WAKE_RATE_LIMIT 0xA09C > > #define GEN6_RC6pp_WAKE_RATE_LIMIT 0xA0A0 > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > > index f0f97b2..bedb089 100644 > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > @@ -364,8 +364,35 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake) > > i915_check_and_clear_faults(dev); > > } > > > > +static void sanitize_bios_rc6_setup(const struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + bool hw_rc6_enabled = false, sw_rc6_enabled = false; > > + > > + if (IS_BROXTON(dev)) { > > + /* Get forcewake during program sequence. Although the driver > > + * hasn't enabled a state yet where we need forcewake, BIOS may have.*/ > > + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); > > + > > + /* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */ > > + hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) & > > + (GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE); > > + sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE) > > + && (I915_READ(GEN6_RC_STATE) & RC6_STATE); > > + > > + intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); > > + > > + if (!hw_rc6_enabled && !sw_rc6_enabled) { > > + i915.enable_rc6 = 0; > > + DRM_INFO("RC6 disabled by BIOS\n"); > > + } > > + } > > +} > > + > > void intel_uncore_sanitize(struct drm_device *dev) > > { > > + sanitize_bios_rc6_setup(dev); > > Why did you move this out of the enable_rc6 functions? It seems to fit > much better there, instead of here. > > Also I think we shouldn't change the module option, instead it's > conceptually cleaner to just not set up rc6 in genX_enable_rc6, i.e. > > if (!check_bios_rc6_setup()) > return; > > somewhere at the beginning of gen9_enable_rc6 like you had in the previous > patch. Well scrap this since it's just a bikeshed, we do adjust the module options in other places too. Patch looks fine, I'll pull it in if someone with domain knowledge reviews it. -Daniel > -Daniel > > > + > > /* BIOS often leaves RC6 enabled, but disable it for hw init */ > > intel_disable_gt_powersave(dev); > > } > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6 2015-11-04 10:04 ` [PATCH v3 1/1] drm/i915/bxt: " Sagar Arun Kamble 2015-11-17 16:45 ` Daniel Vetter @ 2015-11-26 20:59 ` Imre Deak 2015-12-11 8:44 ` [PATCH v4 " Sagar Arun Kamble 1 sibling, 1 reply; 10+ messages in thread From: Imre Deak @ 2015-11-26 20:59 UTC (permalink / raw) To: Sagar Arun Kamble, intel-gfx; +Cc: shashidhar.hiremath Hi Sagar, sorry for the delay, see below for my comments. On Wed, 2015-11-04 at 15:34 +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. > > RC6 setup is shared between BIOS and Driver. BIOS sets up subset of > RC6 configuration 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. > > 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. > > Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87 > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_uncore.c | 27 +++++++++++++++++++++++++++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 8942532..6ed3542 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6823,6 +6823,7 @@ enum skl_disp_power_wells { > #define GEN6_RPDEUC 0xA084 > #define GEN6_RPDEUCSW 0xA088 > #define GEN6_RC_STATE 0xA094 > +#define RC6_STATE (1<<18) > #define GEN6_RC1_WAKE_RATE_LIMIT 0xA098 > #define GEN6_RC6_WAKE_RATE_LIMIT 0xA09C > #define GEN6_RC6pp_WAKE_RATE_LIMIT 0xA0A0 > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index f0f97b2..bedb089 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -364,8 +364,35 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake) > i915_check_and_clear_faults(dev); > } > > +static void sanitize_bios_rc6_setup(const struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + bool hw_rc6_enabled = false, sw_rc6_enabled = false; > + > + if (IS_BROXTON(dev)) { > + /* Get forcewake during program sequence. Although the driver > + * hasn't enabled a state yet where we need forcewake, BIOS may have.*/ > + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); Do we really need FW for reading? We don't program things here and I915_READ() does take FW for the access itself. > + > + /* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */ > + hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) & > + (GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE); > + sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE) > + && (I915_READ(GEN6_RC_STATE) & RC6_STATE); Could you also add the following checks and bail out if anyone is invalid?: RC6LOCATION (0xd40) [0] should be 1. RC6CTXBASE (0xd48) [31:4] should be non-zero. We could also check if it falls within the WOPCM as it should. PWRCTX_MAXCNT_RCSUNIT (0x2054), PWRCTX_MAXCNT_VCSUNIT0 (0x12054), PWRCTX_MAXCNT_BCSUNIT (0x22054), PWRCTX_MAXCNT_VECSUNIT (0x1A054), PWRCTX_MAXCNT_VCSUNIT1 (0x1C054) [19:0] should be greater than 1. > + > + intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); > + > + if (!hw_rc6_enabled && !sw_rc6_enabled) { > + i915.enable_rc6 = 0; > + DRM_INFO("RC6 disabled by BIOS\n"); > + } > + } > +} > + > void intel_uncore_sanitize(struct drm_device *dev) > { > + sanitize_bios_rc6_setup(dev); > + I'd prefer keeping this together with the RC6-sanitize code done for other platforms in intel_init_gt_powersave(). You could also add a broxton_check_pctx() similar to VLV/CHV that would perform the above sanity checks any time intel_enable_gt_powersave() is called (in addition to the check during intel_init_gt_powersave()). > /* 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6 2015-11-26 20:59 ` Imre Deak @ 2015-12-11 8:44 ` Sagar Arun Kamble 2015-12-14 16:36 ` Imre Deak 0 siblings, 1 reply; 10+ messages in thread From: Sagar Arun Kamble @ 2015-12-11 8:44 UTC (permalink / raw) To: intel-gfx 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) Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87 Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 4 +++ drivers/gpu/drm/i915/i915_gem_stolen.c | 9 +++++ drivers/gpu/drm/i915/i915_reg.h | 12 +++++++ drivers/gpu/drm/i915/intel_pm.c | 62 ++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_uncore.c | 10 ++++++ 5 files changed, 97 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 4c03449..265d08e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1696,6 +1696,8 @@ struct drm_i915_private { void __iomem *regs; + bool bios_hw_rc6_enabled; + bool bios_sw_rc6_enabled; struct intel_uncore uncore; struct i915_virtual_gpu vgpu; @@ -3234,6 +3236,8 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, u32 stolen_offset, u32 gtt_offset, u32 size); +void i915_get_stolen_reserved(struct drm_i915_private *dev_priv, + unsigned long *base, unsigned long *size); /* i915_gem_shrinker.c */ unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv, diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 598ed2f..92ea7c0 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -386,6 +386,15 @@ static void bdw_get_stolen_reserved(struct drm_i915_private *dev_priv, *size = stolen_top - *base; } +void i915_get_stolen_reserved(struct drm_i915_private *dev_priv, + unsigned long *base, unsigned long *size) +{ + if (IS_SKYLAKE(dev_priv)) + bdw_get_stolen_reserved(dev_priv, base, size); + else + gen8_get_stolen_reserved(dev_priv, base, size); +} + int i915_gem_init_stolen(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index ed0e785..fd4f907 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6765,6 +6765,17 @@ 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 +6914,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_pm.c b/drivers/gpu/drm/i915/intel_pm.c index ee05ce8..4d9cbab 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4619,6 +4619,65 @@ static void gen6_init_rps_frequencies(struct drm_device *dev) } } +static void bxt_check_pctx(const struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + bool enable_rc6 = true; + unsigned long reserved_base = 0, reserved_size, rc6_ctx_base; + + i915_get_stolen_reserved(dev_priv, &reserved_base, + &reserved_size); + + if (!(I915_READ(RC6_LOCATION) & RC6_CTX_IN_DRAM)) { + DRM_ERROR("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 >= reserved_base) && + (rc6_ctx_base <= (reserved_base + reserved_size)))) { + DRM_ERROR("RC6 Base address not as expected.\n"); + enable_rc6 = false; + } + + if (!enable_rc6) { + i915.enable_rc6 = 0; + DRM_ERROR("RC6 disabled by BIOS\n"); + } +} + +static void bxt_check_bios_rc6_setup(const struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + bool enable_rc6 = true; + + bxt_check_pctx(dev); + + 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_ERROR("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_ERROR("VCSUNIT1 Idle wait time not set properly.\n"); + enable_rc6 = false; + } + + if (!dev_priv->bios_hw_rc6_enabled && !dev_priv->bios_sw_rc6_enabled) { + DRM_ERROR("HW/SW RC6 is not enabled by BIOS.\n"); + enable_rc6 = false; + } + + if (!enable_rc6) { + i915.enable_rc6 = 0; + DRM_ERROR("RC6 disabled by BIOS\n"); + } +} + /* See the Gen9_GT_PM_Programming_Guide doc for the below */ static void gen9_enable_rps(struct drm_device *dev) { @@ -4660,6 +4719,9 @@ static void gen9_enable_rc6(struct drm_device *dev) uint32_t rc6_mask = 0; int unused; + if (IS_BROXTON(dev)) + bxt_check_bios_rc6_setup(dev); + /* 1a: Software RC state - RC0 */ I915_WRITE(GEN6_RC_STATE, 0); diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index c2358ba..c76c076 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -366,6 +366,16 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake) void intel_uncore_sanitize(struct drm_device *dev) { + struct drm_i915_private *dev_priv = dev->dev_private; + + if (IS_BROXTON(dev)) { + /* Store HW/SW RC6 status set by BIOS before we disable.*/ + dev_priv->bios_hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) & + (GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE); + dev_priv->bios_sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE) + && (I915_READ(GEN6_RC_STATE) & RC6_STATE); + } + /* BIOS often leaves RC6 enabled, but disable it for hw init */ intel_disable_gt_powersave(dev); } -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6 2015-12-11 8:44 ` [PATCH v4 " Sagar Arun Kamble @ 2015-12-14 16:36 ` Imre Deak 0 siblings, 0 replies; 10+ messages in thread From: Imre Deak @ 2015-12-14 16:36 UTC (permalink / raw) To: Sagar Arun Kamble, intel-gfx On pe, 2015-12-11 at 14:14 +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) > > Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87 > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 4 +++ > drivers/gpu/drm/i915/i915_gem_stolen.c | 9 +++++ > drivers/gpu/drm/i915/i915_reg.h | 12 +++++++ > drivers/gpu/drm/i915/intel_pm.c | 62 ++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_uncore.c | 10 ++++++ > 5 files changed, 97 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 4c03449..265d08e 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1696,6 +1696,8 @@ struct drm_i915_private { > > void __iomem *regs; > > + bool bios_hw_rc6_enabled; > + bool bios_sw_rc6_enabled; > struct intel_uncore uncore; > > struct i915_virtual_gpu vgpu; > @@ -3234,6 +3236,8 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, > u32 stolen_offset, > u32 gtt_offset, > u32 size); > +void i915_get_stolen_reserved(struct drm_i915_private *dev_priv, > + unsigned long *base, unsigned long *size); For this and other similar changes in the patch: wrapped function arguments need to be aligned to start at the column next to the function's opening brace. > > /* i915_gem_shrinker.c */ > unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv, > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index 598ed2f..92ea7c0 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -386,6 +386,15 @@ static void bdw_get_stolen_reserved(struct drm_i915_private *dev_priv, > *size = stolen_top - *base; > } > > +void i915_get_stolen_reserved(struct drm_i915_private *dev_priv, > + unsigned long *base, unsigned long *size) > +{ > + if (IS_SKYLAKE(dev_priv)) > + bdw_get_stolen_reserved(dev_priv, base, size); > + else > + gen8_get_stolen_reserved(dev_priv, base, size); > +} > + Hm, but then we are missing all the rest of the platforms and leave things open-coded in i915_gem_init_stolen(). I suggest just making i915_gem_init_stolen() cache the reserved base and size in dev_priv- >gtt too, since we will use these to check the HW state both during driver loading and resume. > int i915_gem_init_stolen(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index ed0e785..fd4f907 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6765,6 +6765,17 @@ 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 No spaces before tabs, and indent register flag names with one or two extra spaces starting from the column of the register name. > + > #define FORCEWAKE _MMIO(0xA18C) > #define FORCEWAKE_VLV _MMIO(0x1300b0) > #define FORCEWAKE_ACK_VLV _MMIO(0x1300b4) > @@ -6903,6 +6914,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) Needs proper indent as above. > #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_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index ee05ce8..4d9cbab 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4619,6 +4619,65 @@ static void gen6_init_rps_frequencies(struct drm_device *dev) > } > } > > +static void bxt_check_pctx(const struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + bool enable_rc6 = true; > + unsigned long reserved_base = 0, reserved_size, rc6_ctx_base; > + > + i915_get_stolen_reserved(dev_priv, &reserved_base, > + &reserved_size); > + > + if (!(I915_READ(RC6_LOCATION) & RC6_CTX_IN_DRAM)) { > + DRM_ERROR("RC6 Base location not set properly.\n"); It's not necessarily an error, since the user could've disabled it in BIOS, so let's turn these into DRM_DEBUG_KMS and let the caller print a DRM_INFO("RC6 disabled by BIOS\n") if any of the conditions are unmet. > + enable_rc6 = false; > + } > + > + rc6_ctx_base = I915_READ(RC6_CTX_BASE) & RC6_CTX_BASE_MASK; > + if (!((rc6_ctx_base >= reserved_base) && > + (rc6_ctx_base <= (reserved_base + reserved_size)))) { > + DRM_ERROR("RC6 Base address not as expected.\n"); > + enable_rc6 = false; > + } > + > + if (!enable_rc6) { > + i915.enable_rc6 = 0; > + DRM_ERROR("RC6 disabled by BIOS\n"); > + } > +} > + > +static void bxt_check_bios_rc6_setup(const struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + bool enable_rc6 = true; > + > + bxt_check_pctx(dev); > + > + 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_ERROR("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_ERROR("VCSUNIT1 Idle wait time not set properly.\n"); > + enable_rc6 = false; > + } > + > + if (!dev_priv->bios_hw_rc6_enabled && !dev_priv->bios_sw_rc6_enabled) { > + DRM_ERROR("HW/SW RC6 is not enabled by BIOS.\n"); > + enable_rc6 = false; > + } > + > + if (!enable_rc6) { > + i915.enable_rc6 = 0; > + DRM_ERROR("RC6 disabled by BIOS\n"); > + } > +} No need to separate the checks into two functions, we can have all the above checks in a single bool function returning success if all the conditions are met. > + > /* See the Gen9_GT_PM_Programming_Guide doc for the below */ > static void gen9_enable_rps(struct drm_device *dev) > { > @@ -4660,6 +4719,9 @@ static void gen9_enable_rc6(struct drm_device *dev) > uint32_t rc6_mask = 0; > int unused; > > + if (IS_BROXTON(dev)) > + bxt_check_bios_rc6_setup(dev); Actually we can get rid of the check here, and ... > + > /* 1a: Software RC state - RC0 */ > I915_WRITE(GEN6_RC_STATE, 0); > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index c2358ba..c76c076 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -366,6 +366,16 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake) > > void intel_uncore_sanitize(struct drm_device *dev) > { > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + if (IS_BROXTON(dev)) { > + /* Store HW/SW RC6 status set by BIOS before we disable.*/ > + dev_priv->bios_hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) & > + (GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE); > + dev_priv->bios_sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE) > + && (I915_READ(GEN6_RC_STATE) & RC6_STATE); do all the checks here once, by adding these checks to the above combined bxt_check_bios_rc6_setup(), moving i915.enable_rc6 = sanitize_rc6_option() from intel_init_gt_powersave() here and calling bxt_check_bios_rc6_setup() from sanitize_rc6_option(). We can then do away with bios_hw_rc6_enabled and bios_sw_rc6_enabled. --Imre > + } > + > /* 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-12-14 16:36 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-10-27 9:38 [PATCH 1/1] drm/i915/gen9: Check BIOS RC6 setup before enabling RC6 Sagar Arun Kamble 2015-10-30 11:30 ` [PATCH v2 " Sagar Arun Kamble 2015-10-30 16:08 ` Daniel Vetter 2015-10-30 16:09 ` Daniel Vetter 2015-11-04 10:04 ` [PATCH v3 1/1] drm/i915/bxt: " Sagar Arun Kamble 2015-11-17 16:45 ` Daniel Vetter 2015-11-17 16:47 ` Daniel Vetter 2015-11-26 20:59 ` Imre Deak 2015-12-11 8:44 ` [PATCH v4 " Sagar Arun Kamble 2015-12-14 16:36 ` Imre Deak
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).