Re: [Intel-gfx] [PATCH v4 4/6] drm/i915/pxp: Make PXP tee component bind/unbind aware of PXP-owning-GT

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Nov 16, 2022 at 04:30:16PM -0800, Alan Previn wrote:
> Ensure i915_pxp_tee_component_bind / unbind implicitly sorts out
> getting the correct per-GT PXP control-context from the PXP-owning-GT
> when establishing or ending connection. Thus, replace _i915_to_pxp_gt
> with intel_pxp_get_owning_gt (also takes in i915).
> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/pxp/intel_pxp.c     |  6 +++---
>  drivers/gpu/drm/i915/pxp/intel_pxp.h     |  2 ++
>  drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 14 ++++++++++++--
>  3 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index 76a924587543..6a78b6ef0235 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -64,7 +64,7 @@ bool intel_pxp_is_enabled_on_gt(const struct intel_pxp *pxp)
>  	return pxp->ce;
>  }
>  
> -static struct intel_gt *i915_to_pxp_gt(struct drm_i915_private *i915)
> +struct intel_gt *intel_pxp_get_owning_gt(struct drm_i915_private *i915)
>  {
>  	struct intel_gt *gt = NULL;
>  	int i = 0;
> @@ -79,7 +79,7 @@ static struct intel_gt *i915_to_pxp_gt(struct drm_i915_private *i915)
>  
>  bool intel_pxp_is_enabled(struct drm_i915_private *i915)
>  {
> -	struct intel_gt *gt = i915_to_pxp_gt(i915);
> +	struct intel_gt *gt = intel_pxp_get_owning_gt(i915);
>  
>  	if (!gt)
>  		return false;
> @@ -94,7 +94,7 @@ bool intel_pxp_is_active_on_gt(const struct intel_pxp *pxp)
>  
>  bool intel_pxp_is_active(struct drm_i915_private *i915)
>  {
> -	struct intel_gt *gt = i915_to_pxp_gt(i915);
> +	struct intel_gt *gt = intel_pxp_get_owning_gt(i915);
>  
>  	if (!gt)
>  		return false;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> index fe981eebf0ec..c798c3bde957 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -13,6 +13,8 @@ struct intel_pxp;
>  struct drm_i915_gem_object;
>  struct drm_i915_private;
>  
> +struct intel_gt *intel_pxp_get_owning_gt(struct drm_i915_private *i915);
> +
>  struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp);
>  
>  bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp);
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> index a5c9c692c20d..b9198e961cb6 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> @@ -20,8 +20,12 @@
>  static inline struct intel_pxp *i915_dev_to_pxp(struct device *i915_kdev)
>  {
>  	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
> +	struct intel_gt *gt = intel_pxp_get_owning_gt(i915);
>  
> -	return &to_gt(i915)->pxp;
> +	if (!gt)
> +		return NULL;
> +
> +	return &gt->pxp;

or pxp is part of gt, what it looks like, then we use per gt and avoid on the others...

>  }
>  
>  static int intel_pxp_tee_io_message(struct intel_pxp *pxp,
> @@ -128,10 +132,16 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
>  {
>  	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
>  	struct intel_pxp *pxp = i915_dev_to_pxp(i915_kdev);
> -	struct intel_uc *uc = &pxp_to_gt(pxp)->uc;
> +	struct intel_uc *uc;
>  	intel_wakeref_t wakeref;
>  	int ret = 0;
>  
> +	if (!pxp) {
> +		drm_warn(&i915->drm, "tee comp binding without a PXP-owner GT\n");

or we have a single pxp component under i915 and we associate with the gt0 only
and save the gt inside the pxp...

but this whole owning thing seems so convoluted...

> +		return -ENODEV;
> +	}
> +	uc = &pxp_to_gt(pxp)->uc;
> +
>  	mutex_lock(&pxp->tee_mutex);
>  	pxp->pxp_component = data;
>  	pxp->pxp_component->tee_dev = tee_kdev;
> -- 
> 2.34.1
>