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

[Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>]

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@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> >   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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx