| Message ID | 20221021173946.366210-3-alan.previn.teres.alexis@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/pxp: Prepare intel_pxp entry points for MTL | expand |
On 10/21/2022 10:39 AM, Alan Previn wrote: > Make intel_pxp_is_enabled a global check and implicitly find the > PXP-owning-GT. > > PXP feature support is a device-config flag. In preparation for MTL > PXP control-context shall reside on of the two GT's. That said, > update intel_pxp_is_enabled to take in i915 as its input and internally > find the right gt to check if PXP is enabled so its transparent to > callers of this functions. > > However we also need to expose the per-gt variation of this internal > pxp files to use (like what intel_pxp_enabled was prior) so also expose > a new intel_gtpxp_is_enabled function for replacement. > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +- > drivers/gpu/drm/i915/gem/i915_gem_create.c | 2 +- > drivers/gpu/drm/i915/pxp/intel_pxp.c | 28 ++++++++++++++++++-- > drivers/gpu/drm/i915/pxp/intel_pxp.h | 4 ++- > drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c | 2 +- > drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 2 +- > drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 2 +- > drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 8 +++--- > drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 4 +-- > 9 files changed, 40 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > index 1e29b1e6d186..72f47ebda75f 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -257,7 +257,7 @@ static int proto_context_set_protected(struct drm_i915_private *i915, > > if (!protected) { > pc->uses_protected_content = false; > - } else if (!intel_pxp_is_enabled(&to_gt(i915)->pxp)) { > + } else if (!intel_pxp_is_enabled(i915)) { > ret = -ENODEV; > } else if ((pc->user_flags & BIT(UCONTEXT_RECOVERABLE)) || > !(pc->user_flags & BIT(UCONTEXT_BANNABLE))) { > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c > index 33673fe7ee0a..e44803f9bec4 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c > @@ -384,7 +384,7 @@ static int ext_set_protected(struct i915_user_extension __user *base, void *data > if (ext.flags) > return -EINVAL; > > - if (!intel_pxp_is_enabled(&to_gt(ext_data->i915)->pxp)) > + if (!intel_pxp_is_enabled(ext_data->i915)) > return -ENODEV; > > ext_data->flags |= I915_BO_PROTECTED; > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c > index 545c075bf1aa..f7c909fce97c 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c > @@ -9,6 +9,7 @@ > #include "intel_pxp_tee.h" > #include "gem/i915_gem_context.h" > #include "gt/intel_context.h" > +#include "gt/intel_gt.h" > #include "i915_drv.h" > > /** > @@ -68,11 +69,34 @@ bool intel_gtpxp_is_supported(struct intel_pxp *pxp) > return false; > } > > -bool intel_pxp_is_enabled(const struct intel_pxp *pxp) > +bool intel_gtpxp_is_enabled(const struct intel_pxp *pxp) I'd rename this to intel_pxp_is_initialized, that way we don't have 2 almost identically named checkers that mean different things (and also avoid the gtpxp prefix). > { > return pxp->ce; > } > > +static struct intel_gt *_i915_to_pxp_gt(struct drm_i915_private *i915) nit: why the "_" prefix? we usually don't use it for x_to_y functions. Not a blocker. > +{ > + struct intel_gt *gt = NULL; > + int i = 0; > + > + for_each_gt(gt, i915, i) { > + /* There can be only one GT that supports PXP */ > + if (gt && intel_gtpxp_is_supported(>->pxp)) for_each_gt already checks for gt not being NULL, no need to check again. Daniele > + return gt; > + } > + return NULL; > +} > + > +bool intel_pxp_is_enabled(struct drm_i915_private *i915) > +{ > + struct intel_gt *gt = _i915_to_pxp_gt(i915); > + > + if (!gt) > + return false; > + > + return intel_gtpxp_is_enabled(>->pxp); > +} > + > bool intel_pxp_is_active(const struct intel_pxp *pxp) > { > return pxp->arb_is_valid; > @@ -229,7 +253,7 @@ int intel_pxp_start(struct intel_pxp *pxp) > { > int ret = 0; > > - if (!intel_pxp_is_enabled(pxp)) > + if (!intel_gtpxp_is_enabled(pxp)) > return -ENODEV; > > if (wait_for(pxp_component_bound(pxp), 250)) > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h > index c12e4d419c78..61472018bc45 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h > @@ -11,11 +11,13 @@ > > struct intel_pxp; > struct drm_i915_gem_object; > +struct drm_i915_private; > > struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp); > bool intel_gtpxp_is_supported(struct intel_pxp *pxp); > +bool intel_gtpxp_is_enabled(const struct intel_pxp *pxp); > > -bool intel_pxp_is_enabled(const struct intel_pxp *pxp); > +bool intel_pxp_is_enabled(struct drm_i915_private *i915); > bool intel_pxp_is_active(const struct intel_pxp *pxp); > > void intel_pxp_init(struct intel_pxp *pxp); > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c > index f41e45763d0d..0987bb552eaa 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c > @@ -99,7 +99,7 @@ int intel_pxp_terminate_session(struct intel_pxp *pxp, u32 id) > u32 *cs; > int err = 0; > > - if (!intel_pxp_is_enabled(pxp)) > + if (!intel_gtpxp_is_enabled(pxp)) > return 0; > > rq = i915_request_create(ce); > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c > index 124663cf0047..13f517f94bae 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c > @@ -18,7 +18,7 @@ static int pxp_info_show(struct seq_file *m, void *data) > { > struct intel_pxp *pxp = m->private; > struct drm_printer p = drm_seq_file_printer(m); > - bool enabled = intel_pxp_is_enabled(pxp); > + bool enabled = intel_gtpxp_is_enabled(pxp); > > if (!enabled) { > drm_printf(&p, "pxp disabled\n"); > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c > index c28be430718a..8e8e5645e4fc 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c > @@ -22,7 +22,7 @@ void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir) > { > struct intel_gt *gt = pxp_to_gt(pxp); > > - if (GEM_WARN_ON(!intel_pxp_is_enabled(pxp))) > + if (GEM_WARN_ON(!intel_gtpxp_is_enabled(pxp))) > return; > > lockdep_assert_held(gt->irq_lock); > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > index 6a7d4e2ee138..c095a9e0a01f 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > @@ -11,7 +11,7 @@ > > void intel_pxp_suspend_prepare(struct intel_pxp *pxp) > { > - if (!intel_pxp_is_enabled(pxp)) > + if (!intel_gtpxp_is_enabled(pxp)) > return; > > pxp->arb_is_valid = false; > @@ -23,7 +23,7 @@ void intel_pxp_suspend(struct intel_pxp *pxp) > { > intel_wakeref_t wakeref; > > - if (!intel_pxp_is_enabled(pxp)) > + if (!intel_gtpxp_is_enabled(pxp)) > return; > > with_intel_runtime_pm(&pxp_to_gt(pxp)->i915->runtime_pm, wakeref) { > @@ -34,7 +34,7 @@ void intel_pxp_suspend(struct intel_pxp *pxp) > > void intel_pxp_resume(struct intel_pxp *pxp) > { > - if (!intel_pxp_is_enabled(pxp)) > + if (!intel_gtpxp_is_enabled(pxp)) > return; > > /* > @@ -50,7 +50,7 @@ void intel_pxp_resume(struct intel_pxp *pxp) > > void intel_pxp_runtime_suspend(struct intel_pxp *pxp) > { > - if (!intel_pxp_is_enabled(pxp)) > + if (!intel_gtpxp_is_enabled(pxp)) > return; > > pxp->arb_is_valid = false; > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c > index 052fd2f9a583..1c6bc56391b7 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c > @@ -152,7 +152,7 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev, > return 0; > > /* the component is required to fully start the PXP HW */ > - if (intel_pxp_is_enabled(pxp)) > + if (intel_gtpxp_is_enabled(pxp)) > intel_pxp_init_hw(pxp); > > intel_runtime_pm_put(&i915->runtime_pm, wakeref); > @@ -167,7 +167,7 @@ static void i915_pxp_tee_component_unbind(struct device *i915_kdev, > struct intel_pxp *pxp = i915_dev_to_pxp(i915_kdev); > intel_wakeref_t wakeref; > > - if (intel_pxp_is_enabled(pxp)) > + if (intel_gtpxp_is_enabled(pxp)) > with_intel_runtime_pm_if_in_use(&i915->runtime_pm, wakeref) > intel_pxp_fini_hw(pxp); >
On Mon, 2022-11-14 at 20:11 -0800, Ceraolo Spurio, Daniele wrote: > > On 10/21/2022 10:39 AM, Alan Previn wrote: > > @@ -68,11 +69,34 @@ bool intel_gtpxp_is_supported(struct intel_pxp *pxp) > > return false; > > } > > > > -bool intel_pxp_is_enabled(const struct intel_pxp *pxp) > > +bool intel_gtpxp_is_enabled(const struct intel_pxp *pxp) > > I'd rename this to intel_pxp_is_initialized, that way we don't have 2 > almost identically named checkers that mean different things (and also > avoid the gtpxp prefix). > I disagree - one is a wrapper around the other so i rather DO insist we have the same function-action name in the middle with a different part of the function name being the qualifier for whether its a global level checker or a gt-level checker. Perhaps as per last review reply, we can do "intel_pxp_is_enabled" as wrapper around "intel_gt_has_pxp_enabled" - i think the "enabled" part SHOULD be consistent since one is a wrapper around the other else a new reader will even more baffled about why "enabled" is different from "initialized" despite trying to get to the same anchor point, "pxp- >ce". > > { > > return pxp->ce; > > } > > > > +static struct intel_gt *_i915_to_pxp_gt(struct drm_i915_private *i915) > > nit: why the "_" prefix? we usually don't use it for x_to_y functions. > Not a blocker. I was assuming internal static functions dont have to obey such rules - i like to use _foo for all local static functions (so that when reading from a caller's code, i know its a local static). Again, just another naming convention preference thing that i feel seems to be happening here and there in the driver code base but not consistent across all files / function types. Since its a nit, i won't change this. > > > +{ > > + struct intel_gt *gt = NULL; > > + int i = 0; > > + > > + for_each_gt(gt, i915, i) { > > + /* There can be only one GT that supports PXP */ > > > > > + if (gt && intel_gtpxp_is_supported(>->pxp)) > > for_each_gt already checks for gt not being NULL, no need to check again. Got it - will fix this. > > Daniele > >
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 1e29b1e6d186..72f47ebda75f 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -257,7 +257,7 @@ static int proto_context_set_protected(struct drm_i915_private *i915, if (!protected) { pc->uses_protected_content = false; - } else if (!intel_pxp_is_enabled(&to_gt(i915)->pxp)) { + } else if (!intel_pxp_is_enabled(i915)) { ret = -ENODEV; } else if ((pc->user_flags & BIT(UCONTEXT_RECOVERABLE)) || !(pc->user_flags & BIT(UCONTEXT_BANNABLE))) { diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c index 33673fe7ee0a..e44803f9bec4 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c @@ -384,7 +384,7 @@ static int ext_set_protected(struct i915_user_extension __user *base, void *data if (ext.flags) return -EINVAL; - if (!intel_pxp_is_enabled(&to_gt(ext_data->i915)->pxp)) + if (!intel_pxp_is_enabled(ext_data->i915)) return -ENODEV; ext_data->flags |= I915_BO_PROTECTED; diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c index 545c075bf1aa..f7c909fce97c 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c @@ -9,6 +9,7 @@ #include "intel_pxp_tee.h" #include "gem/i915_gem_context.h" #include "gt/intel_context.h" +#include "gt/intel_gt.h" #include "i915_drv.h" /** @@ -68,11 +69,34 @@ bool intel_gtpxp_is_supported(struct intel_pxp *pxp) return false; } -bool intel_pxp_is_enabled(const struct intel_pxp *pxp) +bool intel_gtpxp_is_enabled(const struct intel_pxp *pxp) { return pxp->ce; } +static struct intel_gt *_i915_to_pxp_gt(struct drm_i915_private *i915) +{ + struct intel_gt *gt = NULL; + int i = 0; + + for_each_gt(gt, i915, i) { + /* There can be only one GT that supports PXP */ + if (gt && intel_gtpxp_is_supported(>->pxp)) + return gt; + } + return NULL; +} + +bool intel_pxp_is_enabled(struct drm_i915_private *i915) +{ + struct intel_gt *gt = _i915_to_pxp_gt(i915); + + if (!gt) + return false; + + return intel_gtpxp_is_enabled(>->pxp); +} + bool intel_pxp_is_active(const struct intel_pxp *pxp) { return pxp->arb_is_valid; @@ -229,7 +253,7 @@ int intel_pxp_start(struct intel_pxp *pxp) { int ret = 0; - if (!intel_pxp_is_enabled(pxp)) + if (!intel_gtpxp_is_enabled(pxp)) return -ENODEV; if (wait_for(pxp_component_bound(pxp), 250)) diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h index c12e4d419c78..61472018bc45 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h @@ -11,11 +11,13 @@ struct intel_pxp; struct drm_i915_gem_object; +struct drm_i915_private; struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp); bool intel_gtpxp_is_supported(struct intel_pxp *pxp); +bool intel_gtpxp_is_enabled(const struct intel_pxp *pxp); -bool intel_pxp_is_enabled(const struct intel_pxp *pxp); +bool intel_pxp_is_enabled(struct drm_i915_private *i915); bool intel_pxp_is_active(const struct intel_pxp *pxp); void intel_pxp_init(struct intel_pxp *pxp); diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c index f41e45763d0d..0987bb552eaa 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c @@ -99,7 +99,7 @@ int intel_pxp_terminate_session(struct intel_pxp *pxp, u32 id) u32 *cs; int err = 0; - if (!intel_pxp_is_enabled(pxp)) + if (!intel_gtpxp_is_enabled(pxp)) return 0; rq = i915_request_create(ce); diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c index 124663cf0047..13f517f94bae 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c @@ -18,7 +18,7 @@ static int pxp_info_show(struct seq_file *m, void *data) { struct intel_pxp *pxp = m->private; struct drm_printer p = drm_seq_file_printer(m); - bool enabled = intel_pxp_is_enabled(pxp); + bool enabled = intel_gtpxp_is_enabled(pxp); if (!enabled) { drm_printf(&p, "pxp disabled\n"); diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c index c28be430718a..8e8e5645e4fc 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c @@ -22,7 +22,7 @@ void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir) { struct intel_gt *gt = pxp_to_gt(pxp); - if (GEM_WARN_ON(!intel_pxp_is_enabled(pxp))) + if (GEM_WARN_ON(!intel_gtpxp_is_enabled(pxp))) return; lockdep_assert_held(gt->irq_lock); diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c index 6a7d4e2ee138..c095a9e0a01f 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c @@ -11,7 +11,7 @@ void intel_pxp_suspend_prepare(struct intel_pxp *pxp) { - if (!intel_pxp_is_enabled(pxp)) + if (!intel_gtpxp_is_enabled(pxp)) return; pxp->arb_is_valid = false; @@ -23,7 +23,7 @@ void intel_pxp_suspend(struct intel_pxp *pxp) { intel_wakeref_t wakeref; - if (!intel_pxp_is_enabled(pxp)) + if (!intel_gtpxp_is_enabled(pxp)) return; with_intel_runtime_pm(&pxp_to_gt(pxp)->i915->runtime_pm, wakeref) { @@ -34,7 +34,7 @@ void intel_pxp_suspend(struct intel_pxp *pxp) void intel_pxp_resume(struct intel_pxp *pxp) { - if (!intel_pxp_is_enabled(pxp)) + if (!intel_gtpxp_is_enabled(pxp)) return; /* @@ -50,7 +50,7 @@ void intel_pxp_resume(struct intel_pxp *pxp) void intel_pxp_runtime_suspend(struct intel_pxp *pxp) { - if (!intel_pxp_is_enabled(pxp)) + if (!intel_gtpxp_is_enabled(pxp)) return; pxp->arb_is_valid = false; diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c index 052fd2f9a583..1c6bc56391b7 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c @@ -152,7 +152,7 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev, return 0; /* the component is required to fully start the PXP HW */ - if (intel_pxp_is_enabled(pxp)) + if (intel_gtpxp_is_enabled(pxp)) intel_pxp_init_hw(pxp); intel_runtime_pm_put(&i915->runtime_pm, wakeref); @@ -167,7 +167,7 @@ static void i915_pxp_tee_component_unbind(struct device *i915_kdev, struct intel_pxp *pxp = i915_dev_to_pxp(i915_kdev); intel_wakeref_t wakeref; - if (intel_pxp_is_enabled(pxp)) + if (intel_gtpxp_is_enabled(pxp)) with_intel_runtime_pm_if_in_use(&i915->runtime_pm, wakeref) intel_pxp_fini_hw(pxp);
Make intel_pxp_is_enabled a global check and implicitly find the PXP-owning-GT. PXP feature support is a device-config flag. In preparation for MTL PXP control-context shall reside on of the two GT's. That said, update intel_pxp_is_enabled to take in i915 as its input and internally find the right gt to check if PXP is enabled so its transparent to callers of this functions. However we also need to expose the per-gt variation of this internal pxp files to use (like what intel_pxp_enabled was prior) so also expose a new intel_gtpxp_is_enabled function for replacement. Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +- drivers/gpu/drm/i915/gem/i915_gem_create.c | 2 +- drivers/gpu/drm/i915/pxp/intel_pxp.c | 28 ++++++++++++++++++-- drivers/gpu/drm/i915/pxp/intel_pxp.h | 4 ++- drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c | 2 +- drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 2 +- drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 2 +- drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 8 +++--- drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 4 +-- 9 files changed, 40 insertions(+), 14 deletions(-)