Re: [Intel-gfx] [PATCH v7 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
On Fri, 2022-12-02 at 19:21 +, Teres Alexis, Alan Previn wrote:
>
>
> On Fri, 2022-12-02 at 11:22 -0500, Vivi, Rodrigo wrote:
> > On Thu, Dec 01, 2022 at 05:14:07PM -0800, Alan Previn wrote:
> > > Starting with MTL, there will be two GT-tiles, a render and media
> > > tile. PXP as a service for supporting workloads with protected
> > > contexts and protected buffers can be subscribed by process
> > > workloads on any tile. However, depending on the platform,
> > > only one of the tiles is used for control events pertaining to
> > > PXP
> > >
> Alan: [snip]
>
> > > @@ -1168,6 +1176,8 @@ static int i915_drm_prepare(struct
> > > drm_device *dev)
> > > {
> > > struct drm_i915_private *i915 = to_i915(dev);
> > >
> > > + intel_pxp_suspend_prepare(i915->pxp);
> > > +
> > > /*
> > > * NB intel_display_suspend() may issue new requests
> > > after we've
> > > * ostensibly marked the GPU as ready-to-sleep here. We
> > > need to
> > > @@ -1248,6 +1258,8 @@ static int i915_drm_suspend_late(struct
> > > drm_device *dev, bool hibernation)
> > >
> > > disable_rpm_wakeref_asserts(rpm);
> > >
> > > + intel_pxp_suspend(dev_priv->pxp);
> >
> > is this really needed here in the suspend_late?
> > why not in suspend()?
> >
> > In general what is needed in suspend_late is resumed from the
> > resume_early,
> > what doesn't look the case here. So something looks off.
> >
>
> Actually this patch is NOT changing the code flow of when these pxp
> pm functions are getting called from an i915-level
> perspective - i am merely moving it from inside gt level to top level
> up:
>
> Original --> i915_drm_suspend_late calls i915_gem_suspend_late calls
> intel_gt_suspend_late calls intel_pxp_suspend
> Patch --> i915_drm_suspend_late calls intel_pxp_suspend (before
> calling i915_gem_suspend_late)
>
> Putting that aside, i think the original code was designed to have
> the suspend-prepare do nearly everything except
> disable the KCR registers which is postponed in order to handle a
> failed system-suspend-prepare (after pxp's suspend-
> prepare). A failed system-suspend-prepare (after pxp's suspend-
> prepare) would be recoverable with no-op from pxp's
> perspective as the UMD would be forced to recreate the pxp context
> that recreates arb session again and because the KCR
> registers hadnt been disabled, nothing more is required. I'm not 100%
> sure so I'll ping Daniele jump in to correct me.
>
> That said, the better way, for code readibility, would be completely
> skip having an intel_pxp_suspend and just disable
> the KCR in intel_pxp_suspend_prepare and instead add an i915 callback
> for resume_complete (which is the symmetrical
> opposite of suspend_prepare and surprisingly non-existend in i915
> codebase) in order to re-start kcr registers there for
> the case of a failed-system-suspend-prepare or completion of resume.
> I have a separate series that is attempting to fix
> some of this lack of symmetry
> here: https://patchwork.freedesktop.org/patch/513279/?series=111409&r
> ev=1 but i hadn't
> removed the intel_pxp_suspend because i am not sure if the i915-
> resume-complete callback would still be called if i915
> itself was the reason for the failed suspend-prepare AND the pxp-
> suspend-prepare had occured. So i need to craft out a
> way to test that.
>
> Do you want to continue pursuing the final fixups for pxp's suspend-
> resume flows in this patch? Again, i am NOT changing
> this flow - just moving it from inside-gt to outside gem-gt where for
> suspend we move it outside-and-before and for
> resume we move it outside-and-after.
Oh! okay, let's move without changing this flow in this patch and work
in a follow up.
>
> > >
> > >
>
> Alan: [snip]
>
> > > +
> > > i915_gem_suspend_late(dev_priv);
> > >
> > > for_each_gt(gt, dev_priv, i)
> > > +int intel_pxp_init(struct drm_i915_private *i915)
> > > +{
> > > + struct intel_pxp *newpxp;
> > > +
> > > + newpxp = kzalloc(sizeof(*newpxp), GFP_KERNEL);
> > > + if (!newpxp)
> > > + return -ENOMEM;
> > > +
> > > + i915->pxp = newpxp;
> >
> > i915->pxp is already an intel_pxp *, why can't we simply
> > do
> > i915->pxp = kzalloc(sizeof(...
> > if (!i915->pxp)
> > return -ENOMEM;
> > ?
> >
> yes but i wanted to avoid using 'i915->pxp' for all the codes below
> and just use a local variable to keep it shorter.
> But since that's what you prefer, I will respin accordingly.
>
> > > +
> > > + newpxp->ctrl_gt = pxp_get_ctrl_gt(i915);
> > > + if (!newpxp->ctrl_gt)
> > > + return -ENODEV;
> > >
> > > /*
> > > * If HuC is loaded by GSC but PXP is disabled, we can
> > > skip the init of
> > > * the full PXP session/object management and just init
> > > the tee channel.
> > > */
> > > - if (HAS_PXP(gt->i915))
> > > - pxp_init_full(pxp);
> > > - else
Re: [Intel-gfx] [PATCH v7 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
On Fri, 2022-12-02 at 11:22 -0500, Vivi, Rodrigo wrote:
> On Thu, Dec 01, 2022 at 05:14:07PM -0800, Alan Previn wrote:
> > Starting with MTL, there will be two GT-tiles, a render and media
> > tile. PXP as a service for supporting workloads with protected
> > contexts and protected buffers can be subscribed by process
> > workloads on any tile. However, depending on the platform,
> > only one of the tiles is used for control events pertaining to PXP
> >
Alan: [snip]
> > @@ -1168,6 +1176,8 @@ static int i915_drm_prepare(struct drm_device *dev)
> > {
> > struct drm_i915_private *i915 = to_i915(dev);
> >
> > + intel_pxp_suspend_prepare(i915->pxp);
> > +
> > /*
> > * NB intel_display_suspend() may issue new requests after we've
> > * ostensibly marked the GPU as ready-to-sleep here. We need to
> > @@ -1248,6 +1258,8 @@ static int i915_drm_suspend_late(struct drm_device
> > *dev, bool hibernation)
> >
> > disable_rpm_wakeref_asserts(rpm);
> >
> > + intel_pxp_suspend(dev_priv->pxp);
>
> is this really needed here in the suspend_late?
> why not in suspend()?
>
> In general what is needed in suspend_late is resumed from the resume_early,
> what doesn't look the case here. So something looks off.
>
Actually this patch is NOT changing the code flow of when these pxp pm
functions are getting called from an i915-level
perspective - i am merely moving it from inside gt level to top level up:
Original --> i915_drm_suspend_late calls i915_gem_suspend_late calls
intel_gt_suspend_late calls intel_pxp_suspend
Patch --> i915_drm_suspend_late calls intel_pxp_suspend (before calling
i915_gem_suspend_late)
Putting that aside, i think the original code was designed to have the
suspend-prepare do nearly everything except
disable the KCR registers which is postponed in order to handle a failed
system-suspend-prepare (after pxp's suspend-
prepare). A failed system-suspend-prepare (after pxp's suspend-prepare) would
be recoverable with no-op from pxp's
perspective as the UMD would be forced to recreate the pxp context that
recreates arb session again and because the KCR
registers hadnt been disabled, nothing more is required. I'm not 100% sure so
I'll ping Daniele jump in to correct me.
That said, the better way, for code readibility, would be completely skip
having an intel_pxp_suspend and just disable
the KCR in intel_pxp_suspend_prepare and instead add an i915 callback for
resume_complete (which is the symmetrical
opposite of suspend_prepare and surprisingly non-existend in i915 codebase) in
order to re-start kcr registers there for
the case of a failed-system-suspend-prepare or completion of resume. I have a
separate series that is attempting to fix
some of this lack of symmetry here:
https://patchwork.freedesktop.org/patch/513279/?series=111409&rev=1 but i hadn't
removed the intel_pxp_suspend because i am not sure if the i915-resume-complete
callback would still be called if i915
itself was the reason for the failed suspend-prepare AND the
pxp-suspend-prepare had occured. So i need to craft out a
way to test that.
Do you want to continue pursuing the final fixups for pxp's suspend-resume
flows in this patch? Again, i am NOT changing
this flow - just moving it from inside-gt to outside gem-gt where for suspend
we move it outside-and-before and for
resume we move it outside-and-after.
> >
> >
Alan: [snip]
> > +
> > i915_gem_suspend_late(dev_priv);
> >
> > for_each_gt(gt, dev_priv, i)
> > +int intel_pxp_init(struct drm_i915_private *i915)
> > +{
> > + struct intel_pxp *newpxp;
> > +
> > + newpxp = kzalloc(sizeof(*newpxp), GFP_KERNEL);
> > + if (!newpxp)
> > + return -ENOMEM;
> > +
> > + i915->pxp = newpxp;
>
> i915->pxp is already an intel_pxp *, why can't we simply
> do
> i915->pxp = kzalloc(sizeof(...
> if (!i915->pxp)
> return -ENOMEM;
> ?
>
yes but i wanted to avoid using 'i915->pxp' for all the codes below and just
use a local variable to keep it shorter.
But since that's what you prefer, I will respin accordingly.
> > +
> > + newpxp->ctrl_gt = pxp_get_ctrl_gt(i915);
> > + if (!newpxp->ctrl_gt)
> > + return -ENODEV;
> >
> > /*
> > * If HuC is loaded by GSC but PXP is disabled, we can skip the init of
> > * the full PXP session/object management and just init the tee channel.
> > */
> > - if (HAS_PXP(gt->i915))
> > - pxp_init_full(pxp);
> > - else if (intel_huc_is_loaded_by_gsc(>->uc.huc) &&
> > intel_uc_uses_huc(>->uc))
> > - intel_pxp_tee_component_init(pxp);
> > + if (intel_pxp_is_supported(newpxp))
> > + pxp_init_full(newpxp);
> > + else if (intel_huc_is_loaded_by_gsc(&newpxp->ctrl_gt->uc.huc) &&
> > +intel_uc_uses_huc(&newpxp->ctrl_gt->uc))
> > + intel_pxp_tee_component_init(newpxp);
> > +
> > + return 0;
> > }
> >
Alan: [snip]
> > static inline struct intel_pxp *i915_dev_to_pxp(struct device *i915_kdev)
Re: [Intel-gfx] [PATCH v7 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
On Thu, Dec 01, 2022 at 05:14:07PM -0800, Alan Previn wrote: > Starting with MTL, there will be two GT-tiles, a render and media > tile. PXP as a service for supporting workloads with protected > contexts and protected buffers can be subscribed by process > workloads on any tile. However, depending on the platform, > only one of the tiles is used for control events pertaining to PXP > operation (such as creating the arbitration session and session > tear-down). > > PXP as a global feature is accessible via batch buffer instructions > on any engine/tile and the coherency across tiles is handled implicitly > by the HW. In fact, for the foreseeable future, we are expecting this > single-control-tile for the PXP subsystem. > > In MTL, it's the standalone media tile (not the root tile) because > it contains the VDBOX and KCR engine (among the assets PXP relies on > for those events). > > Looking at the current code design, each tile is represented by the > intel_gt structure while the intel_pxp structure currently hangs off the > intel_gt structure. > > Keeping the intel_pxp structure within the intel_gt structure makes some > internal functionalities more straight forward but adds code complexity to > code readibility and maintainibility to many external-to-pxp subsystems > which may need to pick the correct intel_gt structure. An example of this > would be the intel_pxp_is_active or intel_pxp_is_enabled functionality > which should be viewed as a global level inquiry, not a per-gt inquiry. > > That said, this series promotes the intel_pxp structure into the > drm_i915_private structure making it a top-level subsystem and the PXP > subsystem will select the control gt internally and keep a pointer to > it for internal reference. Alan, overall you patch looks great. Thanks for the patience. And thanks for the details above as well. Please address the minor remaining things I could notice on this version now and I believe we will be good to get this in after that. Thanks, Rodrigo. > > Changes from prior revs: >v6: - Remove HAS_PXP macro and replace it with intel_pxp_is_supported > because : [1] introduction of 'ctrl_gt' means we correct this > for MTL's upcoming series now. [2] Also, this has little impact > globally as its only used by PXP-internal callers at the moment. >- Change intel_pxp_init/fini to take in i915 as its input to avoid > ptr-to-ptr in init/fini calls.(Jani). >- Remove the backpointer from pxp->i915 since we can use > pxp->ctrl_gt->i915 if we need it. (Rodrigo). >v5: - Switch from series to single patch (Rodrigo). >- change function name from pxp_get_kcr_owner_gt to > pxp_get_ctrl_gt. >- Fix CI BAT failure by removing redundant call to intel_pxp_fini > from driver-remove. >- NOTE: remaining open still persists on using ptr-to-ptr > and back-ptr. >v4: - Instead of maintaining intel_pxp as an intel_gt structure member > and creating a number of convoluted helpers that takes in i915 as > input and redirects to the correct intel_gt or takes any intel_gt > and internally replaces with the correct intel_gt, promote it to > be a top-level i915 structure. >v3: - Rename gt level helper functions to "intel_pxp_is_enabled/ > supported/ active_on_gt" (Daniele) >- Upgrade _gt_supports_pxp to replace what was intel_gtpxp_is > supported as the new intel_pxp_is_supported_on_gt to check for > PXP feature support vs the tee support for huc authentication. > Fix pxp-debugfs-registration to use only the former to decide > support. (Daniele) >- Couple minor optimizations. >v2: - Avoid introduction of new device info or gt variables and use > existing checks / macros to differentiate the correct GT->PXP > control ownership (Daniele Ceraolo Spurio) >- Don't reuse the updated global-checkers for per-GT callers (such > as other files within PXP) to avoid unnecessary GT-reparsing, > expose a replacement helper like the prior ones. (Daniele). >v1: - Add one more patch to the series for the intel_pxp suspend/resume > for similar refactoring > > Signed-off-by: Alan Previn > --- > .../drm/i915/display/skl_universal_plane.c| 2 +- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 6 +- > drivers/gpu/drm/i915/gem/i915_gem_create.c| 2 +- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +- > drivers/gpu/drm/i915/gt/intel_gt.c| 5 -- > drivers/gpu/drm/i915/gt/intel_gt_debugfs.c| 1 - > drivers/gpu/drm/i915/gt/intel_gt_irq.c| 2 +- > drivers/gpu/drm/i915/gt/intel_gt_pm.c | 8 -- > drivers/gpu/drm/i915/gt/intel_gt_types.h | 3 - > drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 2 +- > drivers/gpu/drm/i915/i915_driver.c| 18 + > drivers/gpu/drm/i915/i915_drv.h
