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

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

On Thu, Jan 12, 2023 at 05:18: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>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>  drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> index d50354bfb993..bef6d7f8ac55 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> @@ -127,6 +127,10 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
>  	intel_wakeref_t wakeref;
>  	int ret = 0;
>  
> +	if (!HAS_HECI_PXP(i915) &&
> +	    drm_WARN_ON(&i915->drm, !device_link_add(i915_kdev, tee_kdev,

I don't like the action here hidden behind the drm_WARN_ON.
Please notice that almost every use of this and other helpers like
this expect the param as a failure. Not an actual action. So,
most of lazy readers like me might ignore that the main function
is actually a param inside  this warn condition.

We should probably stash the link as well...

pxp->dev_link = device_link_add(i915_kdev, tee_kdev,...);

so in the end below, instead of checking the HAS_HECI_PXP again
and use the remove version you check the dev_link and use the del
function.

something like:

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

Also, do you really need the WARN to see the stack when this happens
or you already know the callers?
Why not a simple drm_error msg?

if (!HAS_HECI_PXP(i915) {
	pxp->dev_link = device_link_add(i915_kdev, tee_kdev,...);
	if (!pxp->dev_link) {
	   drm_error();
	   return -ESOMETHING;

>  DL_FLAG_STATELESS)))

do we need the RPM in sync as well?
I mean:

DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME)))

?

> +		return -ENOMEM;

why ENOMEM?

> +
>  	mutex_lock(&pxp->tee_mutex);
>  	pxp->pxp_component = data;
>  	pxp->pxp_component->tee_dev = tee_kdev;
> @@ -169,6 +173,9 @@ 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 (!HAS_HECI_PXP(i915))
> +		device_link_remove(i915_kdev, tee_kdev);
>  }
>  
>  static const struct component_ops i915_pxp_tee_component_ops = {
> -- 
> 2.39.0
>