| Message ID | 20230113011850.1463965-3-alan.previn.teres.alexis@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/pxp: Add missing cleanup steps for PXP global-teardown | expand |
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 >
Thanks for reviewing the patch. I will fix the code according to your recommendation. I assume we could probably go with -ENOLINK as the error (instead of -ENOMEM). However, i'll wait for Alexander to respond on whether he needs drm_WARN_ON and your question on RPM. ...alan On Thu, 2023-01-19 at 14:25 -0500, Vivi, Rodrigo wrote: > 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(+) Alan: [snip]
> > 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. > Honestly, copy-pasted from drivers/gpu/drm/i915/display/intel_audio.c +1266 I don't have deep knowledge of drm macros, so thought this should be ok. Can make it any other form that acceptable in drm tree... > 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); > Not sure that this simplification warrants additional clutter in struct. > 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))) > > ? No, the mei device should not be active all the time when i915 is active, only when pxp requires it. > > > + return -ENOMEM; > > why ENOMEM? Copy-paste from i915 audio. > > > + > > 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 > >
On Sun, 22 Jan 2023, "Usyskin, Alexander" <alexander.usyskin@intel.com> wrote: >> > 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. >> > Honestly, copy-pasted from drivers/gpu/drm/i915/display/intel_audio.c +1266 > I don't have deep knowledge of drm macros, so thought this should be ok. > Can make it any other form that acceptable in drm tree... Unfortunately, some pattern being present in the driver does not mean it's a good example to be emulated. If we copy a bad pattern, it seems more acceptable, and even more people will copy it. BR, Jani. > >> 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); >> > Not sure that this simplification warrants additional clutter in struct. > >> 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))) >> >> ? > > No, the mei device should not be active all the time when i915 is active, only when pxp requires it. > >> >> > + return -ENOMEM; >> >> why ENOMEM? > Copy-paste from i915 audio. > >> >> > + >> > 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 >> >
On Sun, Jan 22, 2023 at 06:57:24AM +0000, Usyskin, Alexander wrote: > > > 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. > > > Honestly, copy-pasted from drivers/gpu/drm/i915/display/intel_audio.c +1266 > I don't have deep knowledge of drm macros, so thought this should be ok. > Can make it any other form that acceptable in drm tree... something like I suggested in my previous reply please. > > > 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); > > > Not sure that this simplification warrants additional clutter in struct. > > > 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))) > > > > ? > > No, the mei device should not be active all the time when i915 is active, only when pxp requires it. okay then > > > > > > + return -ENOMEM; > > > > why ENOMEM? > Copy-paste from i915 audio. It doesn't make sense to me. Looking to other derivers -ENODEV or -EINVAL seems to be the most used choices... looking to the error codes probably ECHILD would make sense but no one is using it... so let's stay with ENODEV?! > > > > > > + > > > 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 > > >
On Mon, 2023-01-23 at 09:31 -0500, Vivi, Rodrigo wrote: > On Sun, Jan 22, 2023 at 06:57:24AM +0000, Usyskin, Alexander wrote: > > > > 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 > > > alan:snip. Thanks Jani, Rodrigo and Alex. I'll re-rev with all of Rodrigo's recommendation: - will break down the initial code so we dont hide device-link behind drm_WARN_ON - since i didnt hear any hard objection - I will stick with Rodrigo's recommendation to stash the device-link. - dont need DL_FLAG_PM_RUNTIME - use -ENODEV probably like this: 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; } alan:snip.
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, DL_FLAG_STATELESS))) + return -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 = {