Re: [RFC 7/8] drm/i915: introduce display_uncore

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

+ Jani & Ville

On 06/06/2019 22:52, 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.
>
> All the uncore suspend/resume functions are forcewake-related, so no
> need to call them for display_uncore.

Looks like a promising concept. Display experts can give a final verdict.

Would it be possible to add something like Ville did to verify 
non-display registers are not used with wrong uncore and vice-versa 
(https://github.com/vsyrjala/linux/commit/ddd01ad0836f2aad3bb78d6e27a572d2ae43960e)? 
Another vfunc per uncore to verify register range or something.

Regards,

Tvrtko

> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/i915_drv.c     | 14 +++++++++++---
>   drivers/gpu/drm/i915/i915_drv.h     |  6 +++++-
>   drivers/gpu/drm/i915/intel_uncore.c | 23 +++++++++++++++++------
>   drivers/gpu/drm/i915/intel_uncore.h |  9 ++++++++-
>   4 files changed, 41 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 024f270f6f00..635024cad005 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -893,7 +893,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv)
>   
>   	intel_device_info_subplatform_init(dev_priv);
>   
> -	intel_uncore_init_early(&dev_priv->uncore);
> +	intel_uncore_init_early(&dev_priv->uncore, 0);
> +	intel_uncore_init_early(&dev_priv->de_uncore, UNCORE_IS_DISPLAY);
>   
>   	spin_lock_init(&dev_priv->irq_lock);
>   	spin_lock_init(&dev_priv->gpu_error.lock);
> @@ -991,6 +992,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);
>   
> @@ -1000,14 +1005,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);
> @@ -1022,6 +1029,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);
>   }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index dc6b3e4af575..87dcc7addc53 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1398,6 +1398,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;
>   	atomic_t user_forcewake_count;
>   
> @@ -2013,7 +2014,10 @@ static inline struct drm_i915_private *huc_to_i915(struct intel_huc *huc)
>   
>   static inline struct drm_i915_private *uncore_to_i915(struct intel_uncore *uncore)
>   {
> -	return container_of(uncore, struct drm_i915_private, uncore);
> +	if (intel_uncore_is_display(uncore))
> +		return container_of(uncore, struct drm_i915_private, de_uncore);
> +	else
> +		return container_of(uncore, struct drm_i915_private, uncore);
>   }
>   
>   /* Simple iterator over all initialised engines */
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index c460426b0562..64479a746f56 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -549,6 +549,9 @@ static void __intel_uncore_early_sanitize(struct intel_uncore *uncore,
>   
>   void intel_uncore_suspend(struct intel_uncore *uncore)
>   {
> +	if (!intel_uncore_is_display(uncore))
> +		return;
> +
>   	iosf_mbi_punit_acquire();
>   	iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
>   		&uncore->pmic_bus_access_nb);
> @@ -560,6 +563,9 @@ void intel_uncore_resume_early(struct intel_uncore *uncore)
>   {
>   	unsigned int restore_forcewake;
>   
> +	if (!intel_uncore_is_display(uncore))
> +		return;
> +
>   	restore_forcewake = fetch_and_zero(&uncore->fw_domains_saved);
>   	__intel_uncore_early_sanitize(uncore, restore_forcewake);
>   
> @@ -568,6 +574,9 @@ void intel_uncore_resume_early(struct intel_uncore *uncore)
>   
>   void intel_uncore_runtime_resume(struct intel_uncore *uncore)
>   {
> +	if (!intel_uncore_is_display(uncore))
> +		return;
> +
>   	iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
>   }
>   
> @@ -1556,9 +1565,10 @@ static void uncore_mmio_cleanup(struct intel_uncore *uncore)
>   	pci_iounmap(pdev, uncore->regs);
>   }
>   
> -void intel_uncore_init_early(struct intel_uncore *uncore)
> +void intel_uncore_init_early(struct intel_uncore *uncore, u32 flags)
>   {
>   	spin_lock_init(&uncore->lock);
> +	uncore->flags = flags;
>   }
>   
>   int intel_uncore_init_mmio(struct intel_uncore *uncore)
> @@ -1575,7 +1585,8 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>   
>   	i915_check_vgpu(i915);
>   
> -	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;
>   
>   	ret = intel_uncore_fw_domains_init(uncore);
> @@ -1586,9 +1597,6 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>   
>   	__intel_uncore_early_sanitize(uncore, 0);
>   
> -	uncore->pmic_bus_access_nb.notifier_call =
> -		i915_pmic_bus_access_notifier;
> -
>   	if (!intel_uncore_has_forcewake(uncore)) {
>   		if (IS_GEN(i915, 5)) {
>   			ASSIGN_WRITE_MMIO_VFUNCS_NO_FW(uncore, gen5);
> @@ -1641,7 +1649,10 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>   	if (IS_GEN_RANGE(i915, 6, 7))
>   		uncore->flags |= UNCORE_HAS_FIFO;
>   
> -	iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
> +	if (intel_uncore_has_forcewake(uncore)) {
> +		uncore->pmic_bus_access_nb.notifier_call = i915_pmic_bus_access_notifier;
> +		iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
> +	}
>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index 1de1e8505124..07e79cb6c756 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -118,6 +118,7 @@ struct intel_uncore {
>   #define UNCORE_HAS_FPGA_DBG_UNCLAIMED	BIT(1)
>   #define UNCORE_HAS_DBG_UNCLAIMED	BIT(2)
>   #define UNCORE_HAS_FIFO			BIT(3)
> +#define UNCORE_IS_DISPLAY		BIT(4)
>   
>   	const struct intel_forcewake_range *fw_domains_table;
>   	unsigned int fw_domains_table_entries;
> @@ -177,12 +178,18 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore)
>   	return uncore->flags & UNCORE_HAS_FIFO;
>   }
>   
> +static inline bool
> +intel_uncore_is_display(const struct intel_uncore *uncore)
> +{
> +	return uncore->flags & UNCORE_IS_DISPLAY;
> +}
> +
>   void
>   intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug);
>   void intel_uncore_mmio_debug_suspend(struct intel_uncore_mmio_debug *mmio_debug);
>   void intel_uncore_mmio_debug_resume(struct intel_uncore_mmio_debug *mmio_debug);
>   
> -void intel_uncore_init_early(struct intel_uncore *uncore);
> +void intel_uncore_init_early(struct intel_uncore *uncore, u32 flags);
>   int intel_uncore_init_mmio(struct intel_uncore *uncore);
>   void intel_uncore_fw_domain_prune(struct intel_uncore *uncore,
>   				  enum forcewake_domain_id domain_id);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx