| Message ID | 20190624203152.13725-4-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Display uncore | expand |
On Mon, Jun 24, 2019 at 01:31:51PM -0700, Daniele Ceraolo Spurio wrote: > A forcewake-less uncore to be used to decouple GT accesses from display > ones to avoid serializing them when there is no need. > > New accessors that implicitly use the new uncore have also been added. > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 17 ++++++++- > drivers/gpu/drm/i915/i915_drv.h | 58 +++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_uncore.c | 9 ++++- > 3 files changed, 81 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 2d6c643dde51..e22c0a6b3992 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -903,6 +903,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) > > intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug); > intel_uncore_init_early(&dev_priv->uncore, dev_priv); > + intel_uncore_init_early(&dev_priv->de_uncore, dev_priv); > > spin_lock_init(&dev_priv->irq_lock); > spin_lock_init(&dev_priv->gpu_error.lock); > @@ -1001,6 +1002,10 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) > if (ret < 0) > goto err_bridge; > > + ret = intel_uncore_init_mmio(&dev_priv->de_uncore); > + if (ret < 0) > + goto err_uncore; > + > /* Try to make sure MCHBAR is enabled before poking at it */ > intel_setup_mchbar(dev_priv); > > @@ -1012,14 +1017,16 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) > > ret = intel_engines_init_mmio(dev_priv); > if (ret) > - goto err_uncore; > + goto err_mchbar; > > i915_gem_init_mmio(dev_priv); > > return 0; > > -err_uncore: > +err_mchbar: > intel_teardown_mchbar(dev_priv); > + intel_uncore_fini_mmio(&dev_priv->de_uncore); > +err_uncore: > intel_uncore_fini_mmio(&dev_priv->uncore); > err_bridge: > pci_dev_put(dev_priv->bridge_dev); > @@ -1034,6 +1041,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) > static void i915_driver_cleanup_mmio(struct drm_i915_private *dev_priv) > { > intel_teardown_mchbar(dev_priv); > + intel_uncore_fini_mmio(&dev_priv->de_uncore); > intel_uncore_fini_mmio(&dev_priv->uncore); > pci_dev_put(dev_priv->bridge_dev); > } > @@ -2166,6 +2174,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) > > i915_gem_suspend_late(dev_priv); > > + intel_uncore_suspend(&dev_priv->de_uncore); > intel_uncore_suspend(&dev_priv->uncore); > > intel_power_domains_suspend(dev_priv, > @@ -2363,6 +2372,7 @@ static int i915_drm_resume_early(struct drm_device *dev) > ret); > > intel_uncore_resume_early(&dev_priv->uncore); > + intel_uncore_resume_early(&dev_priv->de_uncore); > > intel_gt_check_and_clear_faults(&dev_priv->gt); > > @@ -2937,6 +2947,7 @@ static int intel_runtime_suspend(struct device *kdev) > > intel_runtime_pm_disable_interrupts(dev_priv); > > + intel_uncore_suspend(&dev_priv->de_uncore); > intel_uncore_suspend(&dev_priv->uncore); > > ret = 0; > @@ -2955,6 +2966,7 @@ static int intel_runtime_suspend(struct device *kdev) > if (ret) { > DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); > intel_uncore_runtime_resume(&dev_priv->uncore); > + intel_uncore_runtime_resume(&dev_priv->de_uncore); > > intel_runtime_pm_enable_interrupts(dev_priv); > > @@ -3053,6 +3065,7 @@ static int intel_runtime_resume(struct device *kdev) > } > > intel_uncore_runtime_resume(&dev_priv->uncore); > + intel_uncore_runtime_resume(&dev_priv->de_uncore); > > intel_runtime_pm_enable_interrupts(dev_priv); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a53b65d78159..c554b7697bdc 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1341,6 +1341,7 @@ struct drm_i915_private { > resource_size_t stolen_usable_size; /* Total size minus reserved ranges */ > > struct intel_uncore uncore; > + struct intel_uncore de_uncore; > struct intel_uncore_mmio_debug mmio_debug; > > struct i915_virtual_gpu vgpu; > @@ -2746,6 +2747,63 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e, > #define I915_READ_FW(reg__) __I915_REG_OP(read_fw, dev_priv, (reg__)) > #define I915_WRITE_FW(reg__, val__) __I915_REG_OP(write_fw, dev_priv, (reg__), (val__)) > > +/* > + * The following are mmio-accessors that use an independent lock and skip all > + * the forcewake logic, to be used to access display registers, which are > + * outside the GT forcewake wells. > + */ > +static inline void > +intel_assert_register_is_display(struct drm_i915_private *i915, i915_reg_t reg) > +{ > +#ifdef CONFIG_DRM_I915_DEBUG_MMIO > + u32 offset = i915_mmio_reg_offset(reg); > + bool is_de_register; > + > + if (INTEL_GEN(i915) >= 5) { > + is_de_register = offset >= 0x40000 && > + (INTEL_GEN(i915) < 11 || offset < 0x1c0000); I think the gen11+ thing is wrong. There are eg. PHY registers above 0x162000. > + } else { > + is_de_register = > + (offset >= 0x5000 && offset <= 0x5fff) || > + (offset >= 0x6000 && offset <= 0x6fff) || > + (offset >= 0xa000 && offset <= 0xafff) || > + offset >= 0x30000; I think the WARNs are coming from the mchbar range (0x10000+ on these platforms). Not really sure if we should designate that gt or de. Unfortunately the register ranges don't seem to follow much of a sensible pattern. Eg. we seem to have both gt and de related stuff around the 0x130000 mark :( Maybe we should try separate only the pipe/plane registers which are part of your typical atomic commit under their own lock. Could even do per-pipe locks maybe... > + } > + > + WARN_ONCE(!is_de_register, > + "display reg access function used for non-display reg 0x%08x\n", > + offset); > +#endif > +} > + > +static inline u32 > +intel_de_read(struct drm_i915_private *i915, i915_reg_t reg) > +{ > + intel_assert_register_is_display(i915, reg); > + return intel_uncore_read(&i915->de_uncore, reg); > +} > + > +static inline void > +intel_de_posting_read(struct drm_i915_private *i915, i915_reg_t reg) > +{ > + intel_assert_register_is_display(i915, reg); > + intel_uncore_posting_read(&i915->de_uncore, reg); > +} > + > +static inline void > +intel_de_write(struct drm_i915_private *i915, i915_reg_t reg, u32 val) > +{ > + intel_assert_register_is_display(i915, reg); > + intel_uncore_write(&i915->de_uncore, reg, val); > +} > + > +static inline void > +intel_de_rmw(struct drm_i915_private *i915, i915_reg_t reg, u32 clear, u32 set) > +{ > + intel_assert_register_is_display(i915, reg); > + intel_uncore_rmw(&i915->de_uncore, reg, clear, set); > +} > + > /* "Broadcast RGB" property */ > #define INTEL_BROADCAST_RGB_AUTO 0 > #define INTEL_BROADCAST_RGB_FULL 1 > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 1905fd94dd3c..94e153acb0af 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -1674,6 +1674,12 @@ static int uncore_forcewake_init(struct intel_uncore *uncore) > return 0; > } > > +static bool > +intel_uncore_is_display(const struct intel_uncore *uncore) > +{ > + return uncore == &uncore->i915->de_uncore; > +} > + > int intel_uncore_init_mmio(struct intel_uncore *uncore) > { > struct drm_i915_private *i915 = uncore->i915; > @@ -1683,7 +1689,8 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) > if (ret) > return ret; > > - if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915)) > + if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915) && > + !intel_uncore_is_display(uncore)) > uncore->flags |= UNCORE_HAS_FORCEWAKE; > > if (!intel_uncore_has_forcewake(uncore)) { > -- > 2.20.1
On 6/26/19 11:42 AM, Ville Syrjälä wrote: > On Mon, Jun 24, 2019 at 01:31:51PM -0700, Daniele Ceraolo Spurio wrote: >> A forcewake-less uncore to be used to decouple GT accesses from display >> ones to avoid serializing them when there is no need. >> >> New accessors that implicitly use the new uncore have also been added. >> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 17 ++++++++- >> drivers/gpu/drm/i915/i915_drv.h | 58 +++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_uncore.c | 9 ++++- >> 3 files changed, 81 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index 2d6c643dde51..e22c0a6b3992 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -903,6 +903,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) >> >> intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug); >> intel_uncore_init_early(&dev_priv->uncore, dev_priv); >> + intel_uncore_init_early(&dev_priv->de_uncore, dev_priv); >> >> spin_lock_init(&dev_priv->irq_lock); >> spin_lock_init(&dev_priv->gpu_error.lock); >> @@ -1001,6 +1002,10 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) >> if (ret < 0) >> goto err_bridge; >> >> + ret = intel_uncore_init_mmio(&dev_priv->de_uncore); >> + if (ret < 0) >> + goto err_uncore; >> + >> /* Try to make sure MCHBAR is enabled before poking at it */ >> intel_setup_mchbar(dev_priv); >> >> @@ -1012,14 +1017,16 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) >> >> ret = intel_engines_init_mmio(dev_priv); >> if (ret) >> - goto err_uncore; >> + goto err_mchbar; >> >> i915_gem_init_mmio(dev_priv); >> >> return 0; >> >> -err_uncore: >> +err_mchbar: >> intel_teardown_mchbar(dev_priv); >> + intel_uncore_fini_mmio(&dev_priv->de_uncore); >> +err_uncore: >> intel_uncore_fini_mmio(&dev_priv->uncore); >> err_bridge: >> pci_dev_put(dev_priv->bridge_dev); >> @@ -1034,6 +1041,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) >> static void i915_driver_cleanup_mmio(struct drm_i915_private *dev_priv) >> { >> intel_teardown_mchbar(dev_priv); >> + intel_uncore_fini_mmio(&dev_priv->de_uncore); >> intel_uncore_fini_mmio(&dev_priv->uncore); >> pci_dev_put(dev_priv->bridge_dev); >> } >> @@ -2166,6 +2174,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) >> >> i915_gem_suspend_late(dev_priv); >> >> + intel_uncore_suspend(&dev_priv->de_uncore); >> intel_uncore_suspend(&dev_priv->uncore); >> >> intel_power_domains_suspend(dev_priv, >> @@ -2363,6 +2372,7 @@ static int i915_drm_resume_early(struct drm_device *dev) >> ret); >> >> intel_uncore_resume_early(&dev_priv->uncore); >> + intel_uncore_resume_early(&dev_priv->de_uncore); >> >> intel_gt_check_and_clear_faults(&dev_priv->gt); >> >> @@ -2937,6 +2947,7 @@ static int intel_runtime_suspend(struct device *kdev) >> >> intel_runtime_pm_disable_interrupts(dev_priv); >> >> + intel_uncore_suspend(&dev_priv->de_uncore); >> intel_uncore_suspend(&dev_priv->uncore); >> >> ret = 0; >> @@ -2955,6 +2966,7 @@ static int intel_runtime_suspend(struct device *kdev) >> if (ret) { >> DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); >> intel_uncore_runtime_resume(&dev_priv->uncore); >> + intel_uncore_runtime_resume(&dev_priv->de_uncore); >> >> intel_runtime_pm_enable_interrupts(dev_priv); >> >> @@ -3053,6 +3065,7 @@ static int intel_runtime_resume(struct device *kdev) >> } >> >> intel_uncore_runtime_resume(&dev_priv->uncore); >> + intel_uncore_runtime_resume(&dev_priv->de_uncore); >> >> intel_runtime_pm_enable_interrupts(dev_priv); >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index a53b65d78159..c554b7697bdc 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1341,6 +1341,7 @@ struct drm_i915_private { >> resource_size_t stolen_usable_size; /* Total size minus reserved ranges */ >> >> struct intel_uncore uncore; >> + struct intel_uncore de_uncore; >> struct intel_uncore_mmio_debug mmio_debug; >> >> struct i915_virtual_gpu vgpu; >> @@ -2746,6 +2747,63 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e, >> #define I915_READ_FW(reg__) __I915_REG_OP(read_fw, dev_priv, (reg__)) >> #define I915_WRITE_FW(reg__, val__) __I915_REG_OP(write_fw, dev_priv, (reg__), (val__)) >> >> +/* >> + * The following are mmio-accessors that use an independent lock and skip all >> + * the forcewake logic, to be used to access display registers, which are >> + * outside the GT forcewake wells. >> + */ >> +static inline void >> +intel_assert_register_is_display(struct drm_i915_private *i915, i915_reg_t reg) >> +{ >> +#ifdef CONFIG_DRM_I915_DEBUG_MMIO >> + u32 offset = i915_mmio_reg_offset(reg); >> + bool is_de_register; >> + >> + if (INTEL_GEN(i915) >= 5) { >> + is_de_register = offset >= 0x40000 && >> + (INTEL_GEN(i915) < 11 || offset < 0x1c0000); > > I think the gen11+ thing is wrong. There are eg. PHY registers > above 0x162000. But that's still below 0x1c0000. Media engines start at 0x1c0000, so we're back in GT-land at least until 0x1dffff. > >> + } else { >> + is_de_register = >> + (offset >= 0x5000 && offset <= 0x5fff) || >> + (offset >= 0x6000 && offset <= 0x6fff) || >> + (offset >= 0xa000 && offset <= 0xafff) || >> + offset >= 0x30000; > > I think the WARNs are coming from the mchbar range (0x10000+ on > these platforms). Not really sure if we should designate that > gt or de. > > Unfortunately the register ranges don't seem to follow much > of a sensible pattern. Eg. we seem to have both gt and de > related stuff around the 0x130000 mark :( > Should I just drop the check for now then? Alternatively, I could change to WARN if we try to access a register that is inside one of the forcewake ranges if CONFIG_DRM_I915_DEBUG_MMIO is set. Daniele > Maybe we should try separate only the pipe/plane registers > which are part of your typical atomic commit under their > own lock. Could even do per-pipe locks maybe... > >> + } >> + >> + WARN_ONCE(!is_de_register, >> + "display reg access function used for non-display reg 0x%08x\n", >> + offset); >> +#endif >> +} >> + >> +static inline u32 >> +intel_de_read(struct drm_i915_private *i915, i915_reg_t reg) >> +{ >> + intel_assert_register_is_display(i915, reg); >> + return intel_uncore_read(&i915->de_uncore, reg); >> +} >> + >> +static inline void >> +intel_de_posting_read(struct drm_i915_private *i915, i915_reg_t reg) >> +{ >> + intel_assert_register_is_display(i915, reg); >> + intel_uncore_posting_read(&i915->de_uncore, reg); >> +} >> + >> +static inline void >> +intel_de_write(struct drm_i915_private *i915, i915_reg_t reg, u32 val) >> +{ >> + intel_assert_register_is_display(i915, reg); >> + intel_uncore_write(&i915->de_uncore, reg, val); >> +} >> + >> +static inline void >> +intel_de_rmw(struct drm_i915_private *i915, i915_reg_t reg, u32 clear, u32 set) >> +{ >> + intel_assert_register_is_display(i915, reg); >> + intel_uncore_rmw(&i915->de_uncore, reg, clear, set); >> +} >> + >> /* "Broadcast RGB" property */ >> #define INTEL_BROADCAST_RGB_AUTO 0 >> #define INTEL_BROADCAST_RGB_FULL 1 >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c >> index 1905fd94dd3c..94e153acb0af 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.c >> +++ b/drivers/gpu/drm/i915/intel_uncore.c >> @@ -1674,6 +1674,12 @@ static int uncore_forcewake_init(struct intel_uncore *uncore) >> return 0; >> } >> >> +static bool >> +intel_uncore_is_display(const struct intel_uncore *uncore) >> +{ >> + return uncore == &uncore->i915->de_uncore; >> +} >> + >> int intel_uncore_init_mmio(struct intel_uncore *uncore) >> { >> struct drm_i915_private *i915 = uncore->i915; >> @@ -1683,7 +1689,8 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) >> if (ret) >> return ret; >> >> - if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915)) >> + if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915) && >> + !intel_uncore_is_display(uncore)) >> uncore->flags |= UNCORE_HAS_FORCEWAKE; >> >> if (!intel_uncore_has_forcewake(uncore)) { >> -- >> 2.20.1 >
On Wed, Jun 26, 2019 at 01:27:57PM -0700, Daniele Ceraolo Spurio wrote: > > > On 6/26/19 11:42 AM, Ville Syrjälä wrote: > > On Mon, Jun 24, 2019 at 01:31:51PM -0700, Daniele Ceraolo Spurio wrote: > >> A forcewake-less uncore to be used to decouple GT accesses from display > >> ones to avoid serializing them when there is no need. > >> > >> New accessors that implicitly use the new uncore have also been added. > >> > >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_drv.c | 17 ++++++++- > >> drivers/gpu/drm/i915/i915_drv.h | 58 +++++++++++++++++++++++++++++ > >> drivers/gpu/drm/i915/intel_uncore.c | 9 ++++- > >> 3 files changed, 81 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > >> index 2d6c643dde51..e22c0a6b3992 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.c > >> +++ b/drivers/gpu/drm/i915/i915_drv.c > >> @@ -903,6 +903,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) > >> > >> intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug); > >> intel_uncore_init_early(&dev_priv->uncore, dev_priv); > >> + intel_uncore_init_early(&dev_priv->de_uncore, dev_priv); > >> > >> spin_lock_init(&dev_priv->irq_lock); > >> spin_lock_init(&dev_priv->gpu_error.lock); > >> @@ -1001,6 +1002,10 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) > >> if (ret < 0) > >> goto err_bridge; > >> > >> + ret = intel_uncore_init_mmio(&dev_priv->de_uncore); > >> + if (ret < 0) > >> + goto err_uncore; > >> + > >> /* Try to make sure MCHBAR is enabled before poking at it */ > >> intel_setup_mchbar(dev_priv); > >> > >> @@ -1012,14 +1017,16 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) > >> > >> ret = intel_engines_init_mmio(dev_priv); > >> if (ret) > >> - goto err_uncore; > >> + goto err_mchbar; > >> > >> i915_gem_init_mmio(dev_priv); > >> > >> return 0; > >> > >> -err_uncore: > >> +err_mchbar: > >> intel_teardown_mchbar(dev_priv); > >> + intel_uncore_fini_mmio(&dev_priv->de_uncore); > >> +err_uncore: > >> intel_uncore_fini_mmio(&dev_priv->uncore); > >> err_bridge: > >> pci_dev_put(dev_priv->bridge_dev); > >> @@ -1034,6 +1041,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) > >> static void i915_driver_cleanup_mmio(struct drm_i915_private *dev_priv) > >> { > >> intel_teardown_mchbar(dev_priv); > >> + intel_uncore_fini_mmio(&dev_priv->de_uncore); > >> intel_uncore_fini_mmio(&dev_priv->uncore); > >> pci_dev_put(dev_priv->bridge_dev); > >> } > >> @@ -2166,6 +2174,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) > >> > >> i915_gem_suspend_late(dev_priv); > >> > >> + intel_uncore_suspend(&dev_priv->de_uncore); > >> intel_uncore_suspend(&dev_priv->uncore); > >> > >> intel_power_domains_suspend(dev_priv, > >> @@ -2363,6 +2372,7 @@ static int i915_drm_resume_early(struct drm_device *dev) > >> ret); > >> > >> intel_uncore_resume_early(&dev_priv->uncore); > >> + intel_uncore_resume_early(&dev_priv->de_uncore); > >> > >> intel_gt_check_and_clear_faults(&dev_priv->gt); > >> > >> @@ -2937,6 +2947,7 @@ static int intel_runtime_suspend(struct device *kdev) > >> > >> intel_runtime_pm_disable_interrupts(dev_priv); > >> > >> + intel_uncore_suspend(&dev_priv->de_uncore); > >> intel_uncore_suspend(&dev_priv->uncore); > >> > >> ret = 0; > >> @@ -2955,6 +2966,7 @@ static int intel_runtime_suspend(struct device *kdev) > >> if (ret) { > >> DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); > >> intel_uncore_runtime_resume(&dev_priv->uncore); > >> + intel_uncore_runtime_resume(&dev_priv->de_uncore); > >> > >> intel_runtime_pm_enable_interrupts(dev_priv); > >> > >> @@ -3053,6 +3065,7 @@ static int intel_runtime_resume(struct device *kdev) > >> } > >> > >> intel_uncore_runtime_resume(&dev_priv->uncore); > >> + intel_uncore_runtime_resume(&dev_priv->de_uncore); > >> > >> intel_runtime_pm_enable_interrupts(dev_priv); > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >> index a53b65d78159..c554b7697bdc 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> @@ -1341,6 +1341,7 @@ struct drm_i915_private { > >> resource_size_t stolen_usable_size; /* Total size minus reserved ranges */ > >> > >> struct intel_uncore uncore; > >> + struct intel_uncore de_uncore; > >> struct intel_uncore_mmio_debug mmio_debug; > >> > >> struct i915_virtual_gpu vgpu; > >> @@ -2746,6 +2747,63 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e, > >> #define I915_READ_FW(reg__) __I915_REG_OP(read_fw, dev_priv, (reg__)) > >> #define I915_WRITE_FW(reg__, val__) __I915_REG_OP(write_fw, dev_priv, (reg__), (val__)) > >> > >> +/* > >> + * The following are mmio-accessors that use an independent lock and skip all > >> + * the forcewake logic, to be used to access display registers, which are > >> + * outside the GT forcewake wells. > >> + */ > >> +static inline void > >> +intel_assert_register_is_display(struct drm_i915_private *i915, i915_reg_t reg) > >> +{ > >> +#ifdef CONFIG_DRM_I915_DEBUG_MMIO > >> + u32 offset = i915_mmio_reg_offset(reg); > >> + bool is_de_register; > >> + > >> + if (INTEL_GEN(i915) >= 5) { > >> + is_de_register = offset >= 0x40000 && > >> + (INTEL_GEN(i915) < 11 || offset < 0x1c0000); > > > > I think the gen11+ thing is wrong. There are eg. PHY registers > > above 0x162000. > > But that's still below 0x1c0000. Media engines start at 0x1c0000, so > we're back in GT-land at least until 0x1dffff. Doh. Misread the offset. > > > > >> + } else { > >> + is_de_register = > >> + (offset >= 0x5000 && offset <= 0x5fff) || > >> + (offset >= 0x6000 && offset <= 0x6fff) || > >> + (offset >= 0xa000 && offset <= 0xafff) || > >> + offset >= 0x30000; > > > > I think the WARNs are coming from the mchbar range (0x10000+ on > > these platforms). Not really sure if we should designate that > > gt or de. > > > > Unfortunately the register ranges don't seem to follow much > > of a sensible pattern. Eg. we seem to have both gt and de > > related stuff around the 0x130000 mark :( > > > > Should I just drop the check for now then? Alternatively, I could change > to WARN if we try to access a register that is inside one of the > forcewake ranges if CONFIG_DRM_I915_DEBUG_MMIO is set. Would be nice to have a warning like this so we don't accidentally end up touching the same register (or cacheline) under two different locks because that could trigger a system hang. I guess from correcness POV it doesn't really matter which ranges are under which lock. So we could try to include the mchbar range under the de lock on old platforms as well. Maybe just s/0x30000/0x10000/ on old platforms? I don't immediately see anything gt related above 0x10000. Hmm. But that still leaves out ilk which still has mchbar at 0x10000. So we'd need to account for that as well.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 2d6c643dde51..e22c0a6b3992 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -903,6 +903,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug); intel_uncore_init_early(&dev_priv->uncore, dev_priv); + intel_uncore_init_early(&dev_priv->de_uncore, dev_priv); spin_lock_init(&dev_priv->irq_lock); spin_lock_init(&dev_priv->gpu_error.lock); @@ -1001,6 +1002,10 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) if (ret < 0) goto err_bridge; + ret = intel_uncore_init_mmio(&dev_priv->de_uncore); + if (ret < 0) + goto err_uncore; + /* Try to make sure MCHBAR is enabled before poking at it */ intel_setup_mchbar(dev_priv); @@ -1012,14 +1017,16 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) ret = intel_engines_init_mmio(dev_priv); if (ret) - goto err_uncore; + goto err_mchbar; i915_gem_init_mmio(dev_priv); return 0; -err_uncore: +err_mchbar: intel_teardown_mchbar(dev_priv); + intel_uncore_fini_mmio(&dev_priv->de_uncore); +err_uncore: intel_uncore_fini_mmio(&dev_priv->uncore); err_bridge: pci_dev_put(dev_priv->bridge_dev); @@ -1034,6 +1041,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) static void i915_driver_cleanup_mmio(struct drm_i915_private *dev_priv) { intel_teardown_mchbar(dev_priv); + intel_uncore_fini_mmio(&dev_priv->de_uncore); intel_uncore_fini_mmio(&dev_priv->uncore); pci_dev_put(dev_priv->bridge_dev); } @@ -2166,6 +2174,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) i915_gem_suspend_late(dev_priv); + intel_uncore_suspend(&dev_priv->de_uncore); intel_uncore_suspend(&dev_priv->uncore); intel_power_domains_suspend(dev_priv, @@ -2363,6 +2372,7 @@ static int i915_drm_resume_early(struct drm_device *dev) ret); intel_uncore_resume_early(&dev_priv->uncore); + intel_uncore_resume_early(&dev_priv->de_uncore); intel_gt_check_and_clear_faults(&dev_priv->gt); @@ -2937,6 +2947,7 @@ static int intel_runtime_suspend(struct device *kdev) intel_runtime_pm_disable_interrupts(dev_priv); + intel_uncore_suspend(&dev_priv->de_uncore); intel_uncore_suspend(&dev_priv->uncore); ret = 0; @@ -2955,6 +2966,7 @@ static int intel_runtime_suspend(struct device *kdev) if (ret) { DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); intel_uncore_runtime_resume(&dev_priv->uncore); + intel_uncore_runtime_resume(&dev_priv->de_uncore); intel_runtime_pm_enable_interrupts(dev_priv); @@ -3053,6 +3065,7 @@ static int intel_runtime_resume(struct device *kdev) } intel_uncore_runtime_resume(&dev_priv->uncore); + intel_uncore_runtime_resume(&dev_priv->de_uncore); intel_runtime_pm_enable_interrupts(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a53b65d78159..c554b7697bdc 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1341,6 +1341,7 @@ struct drm_i915_private { resource_size_t stolen_usable_size; /* Total size minus reserved ranges */ struct intel_uncore uncore; + struct intel_uncore de_uncore; struct intel_uncore_mmio_debug mmio_debug; struct i915_virtual_gpu vgpu; @@ -2746,6 +2747,63 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e, #define I915_READ_FW(reg__) __I915_REG_OP(read_fw, dev_priv, (reg__)) #define I915_WRITE_FW(reg__, val__) __I915_REG_OP(write_fw, dev_priv, (reg__), (val__)) +/* + * The following are mmio-accessors that use an independent lock and skip all + * the forcewake logic, to be used to access display registers, which are + * outside the GT forcewake wells. + */ +static inline void +intel_assert_register_is_display(struct drm_i915_private *i915, i915_reg_t reg) +{ +#ifdef CONFIG_DRM_I915_DEBUG_MMIO + u32 offset = i915_mmio_reg_offset(reg); + bool is_de_register; + + if (INTEL_GEN(i915) >= 5) { + is_de_register = offset >= 0x40000 && + (INTEL_GEN(i915) < 11 || offset < 0x1c0000); + } else { + is_de_register = + (offset >= 0x5000 && offset <= 0x5fff) || + (offset >= 0x6000 && offset <= 0x6fff) || + (offset >= 0xa000 && offset <= 0xafff) || + offset >= 0x30000; + } + + WARN_ONCE(!is_de_register, + "display reg access function used for non-display reg 0x%08x\n", + offset); +#endif +} + +static inline u32 +intel_de_read(struct drm_i915_private *i915, i915_reg_t reg) +{ + intel_assert_register_is_display(i915, reg); + return intel_uncore_read(&i915->de_uncore, reg); +} + +static inline void +intel_de_posting_read(struct drm_i915_private *i915, i915_reg_t reg) +{ + intel_assert_register_is_display(i915, reg); + intel_uncore_posting_read(&i915->de_uncore, reg); +} + +static inline void +intel_de_write(struct drm_i915_private *i915, i915_reg_t reg, u32 val) +{ + intel_assert_register_is_display(i915, reg); + intel_uncore_write(&i915->de_uncore, reg, val); +} + +static inline void +intel_de_rmw(struct drm_i915_private *i915, i915_reg_t reg, u32 clear, u32 set) +{ + intel_assert_register_is_display(i915, reg); + intel_uncore_rmw(&i915->de_uncore, reg, clear, set); +} + /* "Broadcast RGB" property */ #define INTEL_BROADCAST_RGB_AUTO 0 #define INTEL_BROADCAST_RGB_FULL 1 diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 1905fd94dd3c..94e153acb0af 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1674,6 +1674,12 @@ static int uncore_forcewake_init(struct intel_uncore *uncore) return 0; } +static bool +intel_uncore_is_display(const struct intel_uncore *uncore) +{ + return uncore == &uncore->i915->de_uncore; +} + int intel_uncore_init_mmio(struct intel_uncore *uncore) { struct drm_i915_private *i915 = uncore->i915; @@ -1683,7 +1689,8 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore) if (ret) return ret; - if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915)) + if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915) && + !intel_uncore_is_display(uncore)) uncore->flags |= UNCORE_HAS_FORCEWAKE; if (!intel_uncore_has_forcewake(uncore)) {
A forcewake-less uncore to be used to decouple GT accesses from display ones to avoid serializing them when there is no need. New accessors that implicitly use the new uncore have also been added. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 17 ++++++++- drivers/gpu/drm/i915/i915_drv.h | 58 +++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_uncore.c | 9 ++++- 3 files changed, 81 insertions(+), 3 deletions(-)