diff mbox series

[v2,8/9] drm/i915/pxp: On MTL, KCR enabling doesn't wait on tee component

Message ID 20230111214226.907536-9-alan.previn.teres.alexis@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/pxp: Add MTL PXP Support | expand

Commit Message

Alan Previn Jan. 11, 2023, 9:42 p.m. UTC 
On legacy platforms, KCR HW enabling is done at the time the mei
component interface is bound. It's also disabled during unbind.
However, for MTL onwards, we don't depend on the tee-component
operation before we can start sending GSC-CS firmware messages.

Thus, immediately enable KCR HW on PXP's init, fini and resume.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/i915/pxp/intel_pxp.c     | 52 ++++++++++++++++++------
 drivers/gpu/drm/i915/pxp/intel_pxp.h     |  4 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_pm.c  | 10 ++---
 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 13 +-----
 4 files changed, 49 insertions(+), 30 deletions(-)

Comments

Daniele Ceraolo Spurio Jan. 19, 2023, 2:17 a.m. UTC | #1 
On 1/11/2023 1:42 PM, Alan Previn wrote:
> On legacy platforms, KCR HW enabling is done at the time the mei
> component interface is bound. It's also disabled during unbind.
> However, for MTL onwards, we don't depend on the tee-component
> operation before we can start sending GSC-CS firmware messages.
>
> Thus, immediately enable KCR HW on PXP's init, fini and resume.
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>   drivers/gpu/drm/i915/pxp/intel_pxp.c     | 52 ++++++++++++++++++------
>   drivers/gpu/drm/i915/pxp/intel_pxp.h     |  4 +-
>   drivers/gpu/drm/i915/pxp/intel_pxp_pm.c  | 10 ++---
>   drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 13 +-----
>   4 files changed, 49 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index 809b49f59594..90e739345924 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -143,10 +143,12 @@ static void pxp_init_full(struct intel_pxp *pxp)
>   	if (ret)
>   		return;
>   
> -	if (pxp->uses_gsccs)
> +	if (pxp->uses_gsccs) {
>   		ret = intel_pxp_gsccs_init(pxp);
> -	else
> +		intel_pxp_init_hw(pxp, true);
> +	} else {
>   		ret = intel_pxp_tee_component_init(pxp);
> +	}
>   	if (ret)
>   		goto out_context;
>   
> @@ -249,10 +251,12 @@ void intel_pxp_fini(struct drm_i915_private *i915)
>   
>   	i915->pxp->arb_is_valid = false;
>   
> -	if (i915->pxp->uses_gsccs)
> +	if (i915->pxp->uses_gsccs) {
> +		intel_pxp_fini_hw(i915->pxp, true);
>   		intel_pxp_gsccs_fini(i915->pxp);
> -	else
> +	} else {
>   		intel_pxp_tee_component_fini(i915->pxp);
> +	}
>   
>   	destroy_vcs_context(i915->pxp);
>   
> @@ -304,8 +308,9 @@ int intel_pxp_start(struct intel_pxp *pxp)
>   	if (!intel_pxp_is_enabled(pxp))
>   		return -ENODEV;
>   
> -	if (wait_for(pxp_component_bound(pxp), 250))
> -		return -ENXIO;
> +	if (!pxp->uses_gsccs)
> +		if (wait_for(pxp_component_bound(pxp), 250))
> +			return -ENXIO;
>   
>   	mutex_lock(&pxp->arb_mutex);
>   
> @@ -331,16 +336,39 @@ int intel_pxp_start(struct intel_pxp *pxp)
>   	return ret;
>   }
>   
> -void intel_pxp_init_hw(struct intel_pxp *pxp)
> +static void
> +intel_pxp_hw_state_change(struct intel_pxp *pxp, bool enable,
> +			  bool skip_if_runtime_pm_off)

"skip_if_runtime_pm_off" is a bit confusing. maybe "needs_rpm" or 
something like that would be a better option?
I'm also not super convinced about this common function with a 
conditional rpm. wouldn't it be simpler to just add an 
with_intel_runtime_pm_if_in_use() before the pxp_init/fini_hw in 
pxp_init_full and pxp_fini? Not a blocker.

> +{
> +	intel_wakeref_t wakeref;
> +
> +	if (skip_if_runtime_pm_off) {
> +		/* if we are suspended, the HW will be re-initialized on resume */
> +		wakeref = intel_runtime_pm_get_if_in_use(&pxp->ctrl_gt->i915->runtime_pm);
> +		if (!wakeref)
> +			return;
> +	}
> +
> +	if (enable) {
> +		kcr_pxp_enable(pxp);
> +		intel_pxp_irq_enable(pxp);
> +	} else {
> +		kcr_pxp_disable(pxp);
> +		intel_pxp_irq_disable(pxp);
> +	}
> +
> +	if (skip_if_runtime_pm_off)
> +		intel_runtime_pm_put(&pxp->ctrl_gt->i915->runtime_pm, wakeref);
> +}
> +
> +void intel_pxp_init_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off)
>   {
> -	kcr_pxp_enable(pxp);
> -	intel_pxp_irq_enable(pxp);
> +	intel_pxp_hw_state_change(pxp, true, skip_if_runtime_pm_off);
>   }
>   
> -void intel_pxp_fini_hw(struct intel_pxp *pxp)
> +void intel_pxp_fini_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off)
>   {
> -	kcr_pxp_disable(pxp);
> -	intel_pxp_irq_disable(pxp);
> +	intel_pxp_hw_state_change(pxp, false, skip_if_runtime_pm_off);
>   }
>   
>   int intel_pxp_key_check(struct intel_pxp *pxp,
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> index 04440fada711..6c1fe3f0a20c 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -20,8 +20,8 @@ bool intel_pxp_is_active(const struct intel_pxp *pxp);
>   int intel_pxp_init(struct drm_i915_private *i915);
>   void intel_pxp_fini(struct drm_i915_private *i915);
>   
> -void intel_pxp_init_hw(struct intel_pxp *pxp);
> -void intel_pxp_fini_hw(struct intel_pxp *pxp);
> +void intel_pxp_init_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off);
> +void intel_pxp_fini_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off);
>   
>   void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
>   
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> index 892d39cc61c1..94c1b2fe1eb2 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> @@ -29,7 +29,7 @@ void intel_pxp_suspend(struct intel_pxp *pxp)
>   		return;
>   
>   	with_intel_runtime_pm(&pxp->ctrl_gt->i915->runtime_pm, wakeref) {
> -		intel_pxp_fini_hw(pxp);
> +		intel_pxp_fini_hw(pxp, false);

If you go with the runtime pm inside the intel_pxp_fini_hw, then drop it 
from here and have:

intel_pxp_fini_hw(pxp, true);

>   		pxp->hw_state_invalidated = false;
>   	}
>   }
> @@ -40,14 +40,14 @@ void intel_pxp_resume(struct intel_pxp *pxp)
>   		return;
>   
>   	/*
> -	 * The PXP component gets automatically unbound when we go into S3 and
> +	 * On Pre-MTL, PXP component gets automatically unbound when we go into S3 and
>   	 * re-bound after we come out, so in that scenario we can defer the
>   	 * hw init to the bind call.
>   	 */
> -	if (!pxp->pxp_component)
> +	if (!pxp->uses_gsccs & !pxp->pxp_component)

You have a bitwise & instead of &&.

Also, I'd go instead with:

if (pxp->pxp_component_added && !pxp->pxp_component)

so we focus on the component availability.

Daniele

>   		return;
>   
> -	intel_pxp_init_hw(pxp);
> +	intel_pxp_init_hw(pxp, false);
>   }
>   
>   void intel_pxp_runtime_suspend(struct intel_pxp *pxp)
> @@ -57,7 +57,7 @@ void intel_pxp_runtime_suspend(struct intel_pxp *pxp)
>   
>   	pxp->arb_is_valid = false;
>   
> -	intel_pxp_fini_hw(pxp);
> +	intel_pxp_fini_hw(pxp, false);
>   
>   	pxp->hw_state_invalidated = false;
>   }
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> index d50354bfb993..9b34f2056b19 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> @@ -141,16 +141,9 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
>   		}
>   	}
>   
> -	/* if we are suspended, the HW will be re-initialized on resume */
> -	wakeref = intel_runtime_pm_get_if_in_use(&i915->runtime_pm);
> -	if (!wakeref)
> -		return 0;
> -
>   	/* the component is required to fully start the PXP HW */
>   	if (intel_pxp_is_enabled(pxp))
> -		intel_pxp_init_hw(pxp);
> -
> -	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> +		intel_pxp_init_hw(pxp, true);
>   
>   	return ret;
>   }
> @@ -160,11 +153,9 @@ static void i915_pxp_tee_component_unbind(struct device *i915_kdev,
>   {
>   	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
>   	struct intel_pxp *pxp = i915->pxp;
> -	intel_wakeref_t wakeref;
>   
>   	if (intel_pxp_is_enabled(pxp))
> -		with_intel_runtime_pm_if_in_use(&i915->runtime_pm, wakeref)
> -			intel_pxp_fini_hw(pxp);
> +		intel_pxp_fini_hw(pxp, true);
>   
>   	mutex_lock(&pxp->tee_mutex);
>   	pxp->pxp_component = NULL;
Alan Previn Jan. 24, 2023, 12:15 a.m. UTC | #2 
Thanks - will go with your suggestion - ditch all the abstraction / consolidation ..
add only the two additional calls with the explicit runtime-takes - that minimizes the changes.
And won't forget to the fix the '&' -> '&&'
...alan

On Wed, 2023-01-18 at 18:17 -0800, Ceraolo Spurio, Daniele wrote:
> 
> 
> On 1/11/2023 1:42 PM, Alan Previn wrote:
> > On legacy platforms, KCR HW enabling is done at the time the mei
> > component interface is bound. It's also disabled during unbind.
> > However, for MTL onwards, we don't depend on the tee-component
> > operation before we can start sending GSC-CS firmware messages.
> > 
> > Thus, immediately enable KCR HW on PXP's init, fini and resume.
alan:snip..
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 809b49f59594..90e739345924 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -143,10 +143,12 @@  static void pxp_init_full(struct intel_pxp *pxp)
 	if (ret)
 		return;
 
-	if (pxp->uses_gsccs)
+	if (pxp->uses_gsccs) {
 		ret = intel_pxp_gsccs_init(pxp);
-	else
+		intel_pxp_init_hw(pxp, true);
+	} else {
 		ret = intel_pxp_tee_component_init(pxp);
+	}
 	if (ret)
 		goto out_context;
 
@@ -249,10 +251,12 @@  void intel_pxp_fini(struct drm_i915_private *i915)
 
 	i915->pxp->arb_is_valid = false;
 
-	if (i915->pxp->uses_gsccs)
+	if (i915->pxp->uses_gsccs) {
+		intel_pxp_fini_hw(i915->pxp, true);
 		intel_pxp_gsccs_fini(i915->pxp);
-	else
+	} else {
 		intel_pxp_tee_component_fini(i915->pxp);
+	}
 
 	destroy_vcs_context(i915->pxp);
 
@@ -304,8 +308,9 @@  int intel_pxp_start(struct intel_pxp *pxp)
 	if (!intel_pxp_is_enabled(pxp))
 		return -ENODEV;
 
-	if (wait_for(pxp_component_bound(pxp), 250))
-		return -ENXIO;
+	if (!pxp->uses_gsccs)
+		if (wait_for(pxp_component_bound(pxp), 250))
+			return -ENXIO;
 
 	mutex_lock(&pxp->arb_mutex);
 
@@ -331,16 +336,39 @@  int intel_pxp_start(struct intel_pxp *pxp)
 	return ret;
 }
 
-void intel_pxp_init_hw(struct intel_pxp *pxp)
+static void
+intel_pxp_hw_state_change(struct intel_pxp *pxp, bool enable,
+			  bool skip_if_runtime_pm_off)
+{
+	intel_wakeref_t wakeref;
+
+	if (skip_if_runtime_pm_off) {
+		/* if we are suspended, the HW will be re-initialized on resume */
+		wakeref = intel_runtime_pm_get_if_in_use(&pxp->ctrl_gt->i915->runtime_pm);
+		if (!wakeref)
+			return;
+	}
+
+	if (enable) {
+		kcr_pxp_enable(pxp);
+		intel_pxp_irq_enable(pxp);
+	} else {
+		kcr_pxp_disable(pxp);
+		intel_pxp_irq_disable(pxp);
+	}
+
+	if (skip_if_runtime_pm_off)
+		intel_runtime_pm_put(&pxp->ctrl_gt->i915->runtime_pm, wakeref);
+}
+
+void intel_pxp_init_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off)
 {
-	kcr_pxp_enable(pxp);
-	intel_pxp_irq_enable(pxp);
+	intel_pxp_hw_state_change(pxp, true, skip_if_runtime_pm_off);
 }
 
-void intel_pxp_fini_hw(struct intel_pxp *pxp)
+void intel_pxp_fini_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off)
 {
-	kcr_pxp_disable(pxp);
-	intel_pxp_irq_disable(pxp);
+	intel_pxp_hw_state_change(pxp, false, skip_if_runtime_pm_off);
 }
 
 int intel_pxp_key_check(struct intel_pxp *pxp,
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 04440fada711..6c1fe3f0a20c 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -20,8 +20,8 @@  bool intel_pxp_is_active(const struct intel_pxp *pxp);
 int intel_pxp_init(struct drm_i915_private *i915);
 void intel_pxp_fini(struct drm_i915_private *i915);
 
-void intel_pxp_init_hw(struct intel_pxp *pxp);
-void intel_pxp_fini_hw(struct intel_pxp *pxp);
+void intel_pxp_init_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off);
+void intel_pxp_fini_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off);
 
 void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
index 892d39cc61c1..94c1b2fe1eb2 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
@@ -29,7 +29,7 @@  void intel_pxp_suspend(struct intel_pxp *pxp)
 		return;
 
 	with_intel_runtime_pm(&pxp->ctrl_gt->i915->runtime_pm, wakeref) {
-		intel_pxp_fini_hw(pxp);
+		intel_pxp_fini_hw(pxp, false);
 		pxp->hw_state_invalidated = false;
 	}
 }
@@ -40,14 +40,14 @@  void intel_pxp_resume(struct intel_pxp *pxp)
 		return;
 
 	/*
-	 * The PXP component gets automatically unbound when we go into S3 and
+	 * On Pre-MTL, PXP component gets automatically unbound when we go into S3 and
 	 * re-bound after we come out, so in that scenario we can defer the
 	 * hw init to the bind call.
 	 */
-	if (!pxp->pxp_component)
+	if (!pxp->uses_gsccs & !pxp->pxp_component)
 		return;
 
-	intel_pxp_init_hw(pxp);
+	intel_pxp_init_hw(pxp, false);
 }
 
 void intel_pxp_runtime_suspend(struct intel_pxp *pxp)
@@ -57,7 +57,7 @@  void intel_pxp_runtime_suspend(struct intel_pxp *pxp)
 
 	pxp->arb_is_valid = false;
 
-	intel_pxp_fini_hw(pxp);
+	intel_pxp_fini_hw(pxp, false);
 
 	pxp->hw_state_invalidated = false;
 }
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
index d50354bfb993..9b34f2056b19 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
@@ -141,16 +141,9 @@  static int i915_pxp_tee_component_bind(struct device *i915_kdev,
 		}
 	}
 
-	/* if we are suspended, the HW will be re-initialized on resume */
-	wakeref = intel_runtime_pm_get_if_in_use(&i915->runtime_pm);
-	if (!wakeref)
-		return 0;
-
 	/* the component is required to fully start the PXP HW */
 	if (intel_pxp_is_enabled(pxp))
-		intel_pxp_init_hw(pxp);
-
-	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
+		intel_pxp_init_hw(pxp, true);
 
 	return ret;
 }
@@ -160,11 +153,9 @@  static void i915_pxp_tee_component_unbind(struct device *i915_kdev,
 {
 	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
 	struct intel_pxp *pxp = i915->pxp;
-	intel_wakeref_t wakeref;
 
 	if (intel_pxp_is_enabled(pxp))
-		with_intel_runtime_pm_if_in_use(&i915->runtime_pm, wakeref)
-			intel_pxp_fini_hw(pxp);
+		intel_pxp_fini_hw(pxp, true);
 
 	mutex_lock(&pxp->tee_mutex);
 	pxp->pxp_component = NULL;