| Message ID | 1452869086.5306.20.camel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Fri, Jan 15, 2016 at 04:44:46PM +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? Had a quick discussion with Imre on irc, and yes this is fine. Though we could move gem_init_stolen even earlier, into i915_gem_gtt_init() or just after. I favour gtt_init since the stolen arena is closely coupled with the gtt probing in i915_gem_gtt_init(). Also intel_setup_mchbar() is semantically related to our dev_priv->regs = pci_iomap(), and I would suggest a new intel_setup_mmio() to do both, nice and early. -Chris
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);