[PATCH v2 8/9] drm/i915/pxp: On MTL, KCR enabling doesn't wait on tee component
Ceraolo Spurio, Daniele
daniele.ceraolospurio at intel.com
Thu Jan 19 02:17:37 UTC 2023
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 at 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;
More information about the dri-devel
mailing list