diff mbox series

[v3,3/4] drm/i915: Perform TLB invalidation on all GTs during suspend/resume

Message ID 20231002172419.1017044-3-jonathan.cavitt@intel.com (mailing list archive)
State New, archived
Headers show
Series [v3,1/4] drm/i915: Define and use GuC and CTB TLB invalidation routines | expand

Commit Message

Cavitt, Jonathan Oct. 2, 2023, 5:24 p.m. UTC 
Consider multi-gt support when cancelling all tlb invalidations on
suspend, and when submitting tlb invalidations on resume.

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Signed-off-by: Fei Yang <fei.yang@intel.com>
Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
CC: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Jani Nikula Oct. 3, 2023, 11:48 a.m. UTC | #1 
On Mon, 02 Oct 2023, Jonathan Cavitt <jonathan.cavitt@intel.com> wrote:
> Consider multi-gt support when cancelling all tlb invalidations on
> suspend, and when submitting tlb invalidations on resume.
>
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Fei Yang <fei.yang@intel.com>
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> CC: John Harrison <John.C.Harrison@Intel.com>

I guess I'm wondering why the top level suspend hook needs to iterate
gts instead of some lower level thing. We should aim to reduce
gem/gt/display details from the top level.

BR,
Jani.

> ---
>  drivers/gpu/drm/i915/i915_driver.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index f5175103ea900..d7655a7b60eda 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1077,6 +1077,8 @@ static int i915_drm_suspend(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
>  	pci_power_t opregion_target_state;
> +	struct intel_gt *gt;
> +	int i;
>  
>  	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
>  
> @@ -1094,7 +1096,8 @@ static int i915_drm_suspend(struct drm_device *dev)
>  
>  	intel_runtime_pm_disable_interrupts(dev_priv);
>  
> -	wake_up_all_tlb_invalidate(&to_gt(dev_priv)->uc.guc);
> +	for_each_gt(gt, dev_priv, i)
> +		wake_up_all_tlb_invalidate(&gt->uc.guc);
>  
>  	intel_hpd_cancel_work(dev_priv);
>  
> @@ -1267,9 +1270,11 @@ static int i915_drm_resume(struct drm_device *dev)
>  
>  	intel_gvt_resume(dev_priv);
>  
> -	if (INTEL_GUC_SUPPORTS_TLB_INVALIDATION(&to_gt(dev_priv)->uc.guc)) {
> -		intel_guc_invalidate_tlb_full(&to_gt(dev_priv)->uc.guc);
> -		intel_guc_invalidate_tlb(&to_gt(dev_priv)->uc.guc);
> +	for_each_gt(gt, dev_priv, i) {
> +		if (!INTEL_GUC_SUPPORTS_TLB_INVALIDATION(&gt->uc.guc))
> +			continue;
> +		intel_guc_invalidate_tlb_full(&gt->uc.guc);
> +		intel_guc_invalidate_tlb(&gt->uc.guc);
>  	}
>  
>  	enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
Andi Shyti Oct. 3, 2023, 3:59 p.m. UTC | #2 
Hi Jani,

> > Consider multi-gt support when cancelling all tlb invalidations on
> > suspend, and when submitting tlb invalidations on resume.
> >
> > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Signed-off-by: Fei Yang <fei.yang@intel.com>
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > CC: John Harrison <John.C.Harrison@Intel.com>
> 
> I guess I'm wondering why the top level suspend hook needs to iterate
> gts instead of some lower level thing. We should aim to reduce
> gem/gt/display details from the top level.

I'm not sure I am understanding the question.

The TLB invalidation details are kept under the GT. But when
suspend is called, then the GT invalidation has to be triggered
by the top levels for each GT. Right?

Thanks,
Andi
John Harrison Oct. 3, 2023, 6:52 p.m. UTC | #3 
On 10/3/2023 08:59, Andi Shyti wrote:
> Hi Jani,
>
>>> Consider multi-gt support when cancelling all tlb invalidations on
>>> suspend, and when submitting tlb invalidations on resume.
>>>
>>> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> Signed-off-by: Fei Yang <fei.yang@intel.com>
>>> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
>>> CC: John Harrison <John.C.Harrison@Intel.com>
>> I guess I'm wondering why the top level suspend hook needs to iterate
>> gts instead of some lower level thing. We should aim to reduce
>> gem/gt/display details from the top level.
> I'm not sure I am understanding the question.
>
> The TLB invalidation details are kept under the GT. But when
> suspend is called, then the GT invalidation has to be triggered
> by the top levels for each GT. Right?
I think Jani's point is that the top level should be:
i915_drm_suspend(...) {
    ...
    intel_tlb_suspend(dev_priv);
}

Then the TLB suspend helper function calls into the GT / UC layers as 
appropriate. But none of that internal only detail is exposed at the top 
level.

John.

>
> Thanks,
> Andi
Jani Nikula Oct. 4, 2023, 7:34 a.m. UTC | #4 
On Tue, 03 Oct 2023, John Harrison <john.c.harrison@intel.com> wrote:
> On 10/3/2023 08:59, Andi Shyti wrote:
>> Hi Jani,
>>
>>>> Consider multi-gt support when cancelling all tlb invalidations on
>>>> suspend, and when submitting tlb invalidations on resume.
>>>>
>>>> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>>> Signed-off-by: Fei Yang <fei.yang@intel.com>
>>>> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
>>>> CC: John Harrison <John.C.Harrison@Intel.com>
>>> I guess I'm wondering why the top level suspend hook needs to iterate
>>> gts instead of some lower level thing. We should aim to reduce
>>> gem/gt/display details from the top level.
>> I'm not sure I am understanding the question.
>>
>> The TLB invalidation details are kept under the GT. But when
>> suspend is called, then the GT invalidation has to be triggered
>> by the top levels for each GT. Right?
> I think Jani's point is that the top level should be:
> i915_drm_suspend(...) {
>     ...
>     intel_tlb_suspend(dev_priv);
> }
>
> Then the TLB suspend helper function calls into the GT / UC layers as 
> appropriate. But none of that internal only detail is exposed at the top 
> level.

That's right, thanks for clarifying.

BR,
Jani.


>
> John.
>
>>
>> Thanks,
>> Andi
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index f5175103ea900..d7655a7b60eda 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1077,6 +1077,8 @@  static int i915_drm_suspend(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
 	pci_power_t opregion_target_state;
+	struct intel_gt *gt;
+	int i;
 
 	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
 
@@ -1094,7 +1096,8 @@  static int i915_drm_suspend(struct drm_device *dev)
 
 	intel_runtime_pm_disable_interrupts(dev_priv);
 
-	wake_up_all_tlb_invalidate(&to_gt(dev_priv)->uc.guc);
+	for_each_gt(gt, dev_priv, i)
+		wake_up_all_tlb_invalidate(&gt->uc.guc);
 
 	intel_hpd_cancel_work(dev_priv);
 
@@ -1267,9 +1270,11 @@  static int i915_drm_resume(struct drm_device *dev)
 
 	intel_gvt_resume(dev_priv);
 
-	if (INTEL_GUC_SUPPORTS_TLB_INVALIDATION(&to_gt(dev_priv)->uc.guc)) {
-		intel_guc_invalidate_tlb_full(&to_gt(dev_priv)->uc.guc);
-		intel_guc_invalidate_tlb(&to_gt(dev_priv)->uc.guc);
+	for_each_gt(gt, dev_priv, i) {
+		if (!INTEL_GUC_SUPPORTS_TLB_INVALIDATION(&gt->uc.guc))
+			continue;
+		intel_guc_invalidate_tlb_full(&gt->uc.guc);
+		intel_guc_invalidate_tlb(&gt->uc.guc);
 	}
 
 	enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);