Re: [RFC 3/4] drm/i915: introduce display_uncore

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx