Re: [Intel-gfx] [PATCH v6 2/6] drm/i915/pxp: add device link between i915 and mei_pxp

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

On Mon, Jan 23, 2023 at 09:31:46PM -0800, Alan Previn wrote:
> From: Alexander Usyskin <alexander.usyskin@intel.com>
> 
> Add device link with i915 as consumer and mei_pxp as supplier
> to ensure proper ordering of power flows.
> 
> V2: condition on absence of heci_pxp to filter out DG
> 
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> ---
>  drivers/gpu/drm/i915/pxp/intel_pxp_tee.c   | 11 +++++++++++
>  drivers/gpu/drm/i915/pxp/intel_pxp_types.h |  6 ++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> index 73aa8015f828..cd5b86216506 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> @@ -127,6 +127,12 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
>  	intel_wakeref_t wakeref;
>  	int ret = 0;
>  
> +	if (!HAS_HECI_PXP(i915)) {
> +		pxp->component_dev_link = device_link_add(i915_kdev, tee_kdev, DL_FLAG_STATELESS);
> +		if (drm_WARN_ON(&i915->drm, !pxp->component_dev_link))
> +			return -ENODEV;
> +	}
> +
>  	mutex_lock(&pxp->tee_mutex);
>  	pxp->pxp_component = data;
>  	pxp->pxp_component->tee_dev = tee_kdev;
> @@ -169,6 +175,11 @@ static void i915_pxp_tee_component_unbind(struct device *i915_kdev,
>  	mutex_lock(&pxp->tee_mutex);
>  	pxp->pxp_component = NULL;
>  	mutex_unlock(&pxp->tee_mutex);
> +
> +	if (pxp->component_dev_link) {
> +		device_link_remove(i915_kdev, tee_kdev);
> +		pxp->component_dev_link = NULL;
> +	}

use the 'del' version instead of the 'remove' one.

if (pxp->dev_link)
	device_link_del(pxp->dev_link);

>  }
>  
>  static const struct component_ops i915_pxp_tee_component_ops = {
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> index 7dc5f08d1583..efd2f3915abe 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> @@ -32,6 +32,12 @@ struct intel_pxp {
>  	 * which are protected by &tee_mutex.
>  	 */
>  	struct i915_pxp_component *pxp_component;
> +
> +	/**
> +	 * @component_dev_link: device link of the pxp-component enforcing i915 as the
> +	 * consumer. This is needed for legacy platform (TGL/ADL) full-feature usage.

No need to add platform information here.

What about something shorter:

/* @dev_link: Enforce module relationship for power management ordering. */

> +	 */
> +	struct device_link *component_dev_link;

What about
     struct device_link *dev_link;

'component' is already part of the pxp struct.

>  	/**
>  	 * @pxp_component_added: track if the pxp component has been added.
>  	 * Set and cleared in tee init and fini functions respectively.
> -- 
> 2.39.0
>