[Intel-gfx] [RFC 3/4] drm/i915: introduce display_uncore
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Wed Jun 26 20:27:57 UTC 2019
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 at intel.com>
>> Cc: Ville Syrjälä <ville.syrjala at 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
>
More information about the Intel-gfx
mailing list