Re: [Intel-gfx] [PATCH v6 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
i just realized nothing external to PXP is calling HAS_PXP so I'll probably drop this macro and create a helper for pxp_debugfs (the only caller). ...alan On Thu, 2022-12-01 at 23:55 +, Teres Alexis, Alan Previn wrote: > > On Tue, 2022-11-29 at 18:02 -0800, Teres Alexis, Alan Previn wrote: > Alan: [snip] > > + newpxp->ctrl_gt = pxp_get_ctrl_gt(newpxp->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 (HAS_PXP(newpxp->i915)) > > + pxp_init_full(newpxp); > > I realize with rev6 now having pxp as top-level and defining ctrl_gt properly > in the header, its actually the correct > time to switch HAS_PXP(i915) to HAS_PXP(pxp) so we can check the VDBOX mask > on the ctrl_gt (instead of root gt as it is > now). This assures HAS_PXP continues to be a global-macro as was it was > originally intended. > > ...alan
Re: [Intel-gfx] [PATCH v6 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
On Tue, 2022-11-29 at 18:02 -0800, Teres Alexis, Alan Previn wrote: Alan: [snip] > + newpxp->ctrl_gt = pxp_get_ctrl_gt(newpxp->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 (HAS_PXP(newpxp->i915)) > + pxp_init_full(newpxp); I realize with rev6 now having pxp as top-level and defining ctrl_gt properly in the header, its actually the correct time to switch HAS_PXP(i915) to HAS_PXP(pxp) so we can check the VDBOX mask on the ctrl_gt (instead of root gt as it is now). This assures HAS_PXP continues to be a global-macro as was it was originally intended. ...alan
Re: [Intel-gfx] [PATCH v6 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
On Wed, 2022-11-30 at 17:42 +, Vivi, Rodrigo wrote: > On Wed, 2022-11-30 at 09:32 -0800, Matt Roper wrote: > > On Tue, Nov 29, 2022 at 06:02:45PM -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 > > > > Drive-by comment: we've been a bit inconsistent about terminology in > > the past, but my understanding is that we're trying to standardize on > > "GT" for the unit that MTL has two of, and keeping the term "tile" > > for > > the PVC-style unit that is a combination of (GT+lmem). > > I agree that this gets really confusing... but it will be hard to > standardize this I'm afraid. Specially when we do the PVC-style-tile a > intel_gt struct and we apparently are doing the same on MTL, no?! > > So, unless the topology gets organized in the code with a standard > name, it is hard to demand everyone to use the same one. > > Besides that whenever we were discussing the pvc's one we all had > agreed that the term "tile" was bad, hence we focused on keep the > intel_gt ready for that. > > Whenever I hear tile I think of the display buffer organization... > and there are other "tiles" examples iirc. > > Yeah... GPU is so complex these days, a single subssytem is like a whole SOC back in the day ... beside display tiling formats, media's HEVC has "tiles" that is kinda like macro-blocks and who can forget render's tiled-rendering (all 3 similar but can be completely orthogonal depending on the hw config / buffer state / usage). I'm assuming this conversation is a nit. Please correct me otherwise.
Re: [Intel-gfx] [PATCH v6 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
On Wed, 2022-11-30 at 09:32 -0800, Matt Roper wrote: > On Tue, Nov 29, 2022 at 06:02:45PM -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 > > Drive-by comment: we've been a bit inconsistent about terminology in > the past, but my understanding is that we're trying to standardize on > "GT" for the unit that MTL has two of, and keeping the term "tile" > for > the PVC-style unit that is a combination of (GT+lmem). I agree that this gets really confusing... but it will be hard to standardize this I'm afraid. Specially when we do the PVC-style-tile a intel_gt struct and we apparently are doing the same on MTL, no?! So, unless the topology gets organized in the code with a standard name, it is hard to demand everyone to use the same one. Besides that whenever we were discussing the pvc's one we all had agreed that the term "tile" was bad, hence we focused on keep the intel_gt ready for that. Whenever I hear tile I think of the display buffer organization... and there are other "tiles" examples iirc. > > > Matt > > > 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. > > > > Changes from prior revs: > > 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
Re: [Intel-gfx] [PATCH v6 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
On Tue, Nov 29, 2022 at 06:02:45PM -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 Drive-by comment: we've been a bit inconsistent about terminology in the past, but my understanding is that we're trying to standardize on "GT" for the unit that MTL has two of, and keeping the term "tile" for the PVC-style unit that is a combination of (GT+lmem). Matt > 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. > > Changes from prior revs: >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 | 3 + > drivers/gpu/drm/i915/pxp/intel_pxp.c | 72 ++- > drivers/gpu/drm/i915/pxp/intel_pxp.h | 6 +- > drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c | 8 ++- > drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 41 +++ > drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.h | 4 +- > drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 10 ++- > drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 4 +- > drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 8 ++- > drivers/gpu/drm/i915/
[Intel-gfx] [PATCH v6 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
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.
Changes from prior revs:
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 | 3 +
drivers/gpu/drm/i915/pxp/intel_pxp.c | 72 ++-
drivers/gpu/drm/i915/pxp/intel_pxp.h | 6 +-
drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c | 8 ++-
drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 41 +++
drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.h | 4 +-
drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 10 ++-
drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 4 +-
drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 8 ++-
drivers/gpu/drm/i915/pxp/intel_pxp_types.h| 11 +++
21 files changed, 148 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 76490cc59d8f..4b79c2d2d617 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -1848,7 +1848,7 @@ static bool bo_has_valid_encryption(struct
drm_i915_gem_object *obj)
{
struct drm_i915_private *i915 = to_i915(obj-
