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

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 6/7/19 1:58 AM, Tvrtko Ursulin wrote:
> 
> + Jani & Ville
> 

Jani, Ville, any feedback on this approach?

BTW, If we want to keep the accesses short we could also have display_* 
register access wrappers that take i915 and internally use &i915->de_uncore.

> 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.
> 

I was also thinking of adding something along these lines, but having it 
under a debug build flag. I was also hoping to get rid of 
NEEDS_FORCE_WAKE and GEN11_NEEDS_FORCE_WAKE if/when the transition 
completes, which should help reduce the per-gen deltas in the MMIO 
accessors.

Daniele

> Regards,
> 
> Tvrtko
> 
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>   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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx