Skip to content

Commit

Permalink
drm/i915/pxp: Promote pxp subsystem to top-level of i915
Browse files Browse the repository at this point in the history
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 readability 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.

This promotion comes with two noteworthy changes:

1. Exported pxp functions that are called by external subsystems
   (such as intel_pxp_enabled/active) will have to check implicitly
   if i915->pxp is valid as that structure will not be allocated
   for HW that doesn't support PXP.

2. Since GT is now considered a soft-dependency of PXP we are
   ensuring that GT init happens before PXP init and vice versa
   for fini. This causes a minor ordering change whereby we previously
   called intel_pxp_suspend after intel_uc_suspend but now is before
   i915_gem_suspend_late but the change is required for correct
   dependency flows. Additionally, this re-order change doesn't
   have any impact because at that point in either case, the top level
   entry to i915 won't observe any PXP events (since the GPU was
   quiesced during suspend_prepare). Also, any PXP event doesn't
   really matter when we disable the PXP HW (global GT irqs are
   already off anyway, so even if there was a bug that generated
   spurious events we wouldn't see it and we would just clean it
   up on resume which is okay since the default fallback action
   for PXP would be to keep the sessions off at this suspend stage).

Changes from prior revs:
  v11: - Reformat a comment (Tvrtko).
  v10: - Change the code flow for intel_pxp_init to make it more
         cleaner and readible with better comments explaining the
         difference between full-PXP-feature vs the partial-teelink
         inits depending on the platform. Additionally, only do
         the pxp allocation when we are certain the subsystem is
         needed. (Tvrtko).
   v9: - Cosmetic cleanups in supported/enabled/active. (Daniele).
       - Add comments for intel_pxp_init and pxp_get_ctrl_gt that
         explain the functional flow for when PXP is not supported
         but the backend-assets are needed for HuC authentication
         (Daniele and Tvrtko).
       - Fix two remaining functions that are accessible outside
         PXP that need to be checking pxp ptrs before using them:
         intel_pxp_irq_handler and intel_pxp_huc_load_and_auth
         (Tvrtko and Daniele).
       - User helper macro in pxp-debugfs (Tvrtko).
   v8: - Remove pxp_to_gt macro (Daniele).
       - Fix a bug in pxp_get_ctrl_gt for the case of MTL and we don't
         support GSC-FW on it. (Daniele).
       - Leave i915->pxp as NULL if we dont support PXP and in line
         with that, do additional validity check on i915->pxp for
         intel_pxp_is_supported/enabled/active (Daniele).
       - Remove unncessary include header from intel_gt_debugfs.c
         and check drm_minor i915->drm.primary (Daniele).
       - Other cosmetics / minor issues / more comments on suspend
         flow order change (Daniele).
   v7: - Drop i915_dev_to_pxp and in intel_pxp_init use 'i915->pxp'
         through out instead of local variable newpxp. (Rodrigo)
       - In the case intel_pxp_fini is called during driver unload but
         after i915 loading failed without pxp being allocated, check
         i915->pxp before referencing it. (Alan)
   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

References: https://patchwork.freedesktop.org/patch/msgid/20221202011407.4068371-1-alan.previn.teres.alexis@intel.com
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20221208180542.998148-1-alan.previn.teres.alexis@intel.com
  • Loading branch information
Alan Previn authored and Daniele Ceraolo Spurio committed Dec 9, 2022
1 parent e6d6e9d commit f67986b
Show file tree
Hide file tree
Showing 23 changed files with 210 additions and 116 deletions.
2 changes: 1 addition & 1 deletion drivers/gpu/drm/i915/display/skl_universal_plane.c
Original file line number Diff line number Diff line change
Expand Up @@ -1841,7 +1841,7 @@ static bool bo_has_valid_encryption(struct drm_i915_gem_object *obj)
{
struct drm_i915_private *i915 = to_i915(obj->base.dev);

return intel_pxp_key_check(&to_gt(i915)->pxp, obj, false) == 0;
return intel_pxp_key_check(i915->pxp, obj, false) == 0;
}

static bool pxp_is_borked(struct drm_i915_gem_object *obj)
Expand Down
6 changes: 3 additions & 3 deletions drivers/gpu/drm/i915/gem/i915_gem_context.c
Original file line number Diff line number Diff line change
Expand Up @@ -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->pxp)) {
ret = -ENODEV;
} else if ((pc->user_flags & BIT(UCONTEXT_RECOVERABLE)) ||
!(pc->user_flags & BIT(UCONTEXT_BANNABLE))) {
Expand All @@ -271,8 +271,8 @@ static int proto_context_set_protected(struct drm_i915_private *i915,
*/
pc->pxp_wakeref = intel_runtime_pm_get(&i915->runtime_pm);

if (!intel_pxp_is_active(&to_gt(i915)->pxp))
ret = intel_pxp_start(&to_gt(i915)->pxp);
if (!intel_pxp_is_active(i915->pxp))
ret = intel_pxp_start(i915->pxp);
}

return ret;
Expand Down
2 changes: 1 addition & 1 deletion drivers/gpu/drm/i915/gem/i915_gem_create.c
Original file line number Diff line number Diff line change
Expand Up @@ -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->pxp))
return -ENODEV;

ext_data->flags |= I915_BO_PROTECTED;
Expand Down
2 changes: 1 addition & 1 deletion drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
Original file line number Diff line number Diff line change
Expand Up @@ -871,7 +871,7 @@ static struct i915_vma *eb_lookup_vma(struct i915_execbuffer *eb, u32 handle)
*/
if (i915_gem_context_uses_protected_content(eb->gem_context) &&
i915_gem_object_is_protected(obj)) {
err = intel_pxp_key_check(&vm->gt->pxp, obj, true);
err = intel_pxp_key_check(eb->i915->pxp, obj, true);
if (err) {
i915_gem_object_put(obj);
return ERR_PTR(err);
Expand Down
5 changes: 0 additions & 5 deletions drivers/gpu/drm/i915/gt/intel_gt.c
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

#include "gem/i915_gem_internal.h"
#include "gem/i915_gem_lmem.h"
#include "pxp/intel_pxp.h"

#include "i915_drv.h"
#include "i915_perf_oa_regs.h"
Expand Down Expand Up @@ -746,8 +745,6 @@ int intel_gt_init(struct intel_gt *gt)

intel_migrate_init(&gt->migrate, gt);

intel_pxp_init(&gt->pxp);

goto out_fw;
err_gt:
__intel_gt_disable(gt);
Expand Down Expand Up @@ -787,8 +784,6 @@ void intel_gt_driver_unregister(struct intel_gt *gt)
intel_rps_driver_unregister(&gt->rps);
intel_gsc_fini(&gt->gsc);

intel_pxp_fini(&gt->pxp);

/*
* Upon unregistering the device to prevent any new users, cancel
* all in-flight requests so that we can quickly unbind the active
Expand Down
2 changes: 0 additions & 2 deletions drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include "intel_gt_mcr.h"
#include "intel_gt_pm_debugfs.h"
#include "intel_sseu_debugfs.h"
#include "pxp/intel_pxp_debugfs.h"
#include "uc/intel_uc_debugfs.h"

int intel_gt_debugfs_reset_show(struct intel_gt *gt, u64 *val)
Expand Down Expand Up @@ -99,7 +98,6 @@ void intel_gt_debugfs_register(struct intel_gt *gt)
intel_sseu_debugfs_register(gt, root);

intel_uc_debugfs_register(&gt->uc, root);
intel_pxp_debugfs_register(&gt->pxp, root);
}

void intel_gt_debugfs_register_files(struct dentry *root,
Expand Down
2 changes: 1 addition & 1 deletion drivers/gpu/drm/i915/gt/intel_gt_irq.c
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ gen11_other_irq_handler(struct intel_gt *gt, const u8 instance,
return gen11_rps_irq_handler(&media_gt->rps, iir);

if (instance == OTHER_KCR_INSTANCE)
return intel_pxp_irq_handler(&gt->pxp, iir);
return intel_pxp_irq_handler(gt->i915->pxp, iir);

if (instance == OTHER_GSC_INSTANCE)
return intel_gsc_irq_handler(gt, iir);
Expand Down
8 changes: 0 additions & 8 deletions drivers/gpu/drm/i915/gt/intel_gt_pm.c
Original file line number Diff line number Diff line change
Expand Up @@ -303,8 +303,6 @@ int intel_gt_resume(struct intel_gt *gt)

intel_uc_resume(&gt->uc);

intel_pxp_resume(&gt->pxp);

user_forcewake(gt, false);

out_fw:
Expand Down Expand Up @@ -338,8 +336,6 @@ void intel_gt_suspend_prepare(struct intel_gt *gt)
{
user_forcewake(gt, true);
wait_for_suspend(gt);

intel_pxp_suspend_prepare(&gt->pxp);
}

static suspend_state_t pm_suspend_target(void)
Expand All @@ -364,7 +360,6 @@ void intel_gt_suspend_late(struct intel_gt *gt)
GEM_BUG_ON(gt->awake);

intel_uc_suspend(&gt->uc);
intel_pxp_suspend(&gt->pxp);

/*
* On disabling the device, we want to turn off HW access to memory
Expand Down Expand Up @@ -392,7 +387,6 @@ void intel_gt_suspend_late(struct intel_gt *gt)

void intel_gt_runtime_suspend(struct intel_gt *gt)
{
intel_pxp_runtime_suspend(&gt->pxp);
intel_uc_runtime_suspend(&gt->uc);

GT_TRACE(gt, "\n");
Expand All @@ -410,8 +404,6 @@ int intel_gt_runtime_resume(struct intel_gt *gt)
if (ret)
return ret;

intel_pxp_runtime_resume(&gt->pxp);

return 0;
}

Expand Down
3 changes: 0 additions & 3 deletions drivers/gpu/drm/i915/gt/intel_gt_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
#include "intel_rps_types.h"
#include "intel_migrate_types.h"
#include "intel_wakeref.h"
#include "pxp/intel_pxp_types.h"
#include "intel_wopcm.h"

struct drm_i915_private;
Expand Down Expand Up @@ -275,8 +274,6 @@ struct intel_gt {
u8 wb_index; /* Only used on HAS_L3_CCS_READ() platforms */
} mocs;

struct intel_pxp pxp;

/* gt/gtN sysfs */
struct kobject sysfs_gt;

Expand Down
2 changes: 1 addition & 1 deletion drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ int intel_huc_fw_load_and_auth_via_gsc(struct intel_huc *huc)

GEM_WARN_ON(intel_uc_fw_is_loaded(&huc->fw));

ret = intel_pxp_huc_load_and_auth(&huc_to_gt(huc)->pxp);
ret = intel_pxp_huc_load_and_auth(huc_to_gt(huc)->i915->pxp);
if (ret)
return ret;

Expand Down
18 changes: 18 additions & 0 deletions drivers/gpu/drm/i915/i915_driver.c
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@
#include "gt/intel_gt_pm.h"
#include "gt/intel_rc6.h"

#include "pxp/intel_pxp.h"
#include "pxp/intel_pxp_debugfs.h"
#include "pxp/intel_pxp_pm.h"

#include "i915_file_private.h"
Expand Down Expand Up @@ -756,6 +758,8 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
for_each_gt(gt, dev_priv, i)
intel_gt_driver_register(gt);

intel_pxp_debugfs_register(dev_priv->pxp);

i915_hwmon_register(dev_priv);

intel_display_driver_register(dev_priv);
Expand Down Expand Up @@ -787,6 +791,8 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)

intel_display_driver_unregister(dev_priv);

intel_pxp_fini(dev_priv);

for_each_gt(gt, dev_priv, i)
intel_gt_driver_unregister(gt);

Expand Down Expand Up @@ -930,6 +936,8 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
if (ret)
goto out_cleanup_modeset2;

intel_pxp_init(i915);

ret = intel_modeset_init(i915);
if (ret)
goto out_cleanup_gem;
Expand Down Expand Up @@ -1165,6 +1173,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
Expand Down Expand Up @@ -1245,6 +1255,8 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)

disable_rpm_wakeref_asserts(rpm);

intel_pxp_suspend(dev_priv->pxp);

i915_gem_suspend_late(dev_priv);

for_each_gt(gt, dev_priv, i)
Expand Down Expand Up @@ -1357,6 +1369,8 @@ static int i915_drm_resume(struct drm_device *dev)

i915_gem_resume(dev_priv);

intel_pxp_resume(dev_priv->pxp);

intel_modeset_init_hw(dev_priv);
intel_init_clock_gating(dev_priv);
intel_hpd_init(dev_priv);
Expand Down Expand Up @@ -1627,6 +1641,8 @@ static int intel_runtime_suspend(struct device *kdev)
*/
i915_gem_runtime_suspend(dev_priv);

intel_pxp_runtime_suspend(dev_priv->pxp);

for_each_gt(gt, dev_priv, i)
intel_gt_runtime_suspend(gt);

Expand Down Expand Up @@ -1731,6 +1747,8 @@ static int intel_runtime_resume(struct device *kdev)
for_each_gt(gt, dev_priv, i)
intel_gt_runtime_resume(gt);

intel_pxp_runtime_resume(dev_priv->pxp);

/*
* On VLV/CHV display interrupts are part of the display
* power well, so hpd is reinitialized from there. For
Expand Down
7 changes: 3 additions & 4 deletions drivers/gpu/drm/i915/i915_drv.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ struct intel_encoder;
struct intel_limit;
struct intel_overlay_error_state;
struct vlv_s0ix_state;
struct intel_pxp;

#define I915_GEM_GPU_DOMAINS \
(I915_GEM_DOMAIN_RENDER | \
Expand Down Expand Up @@ -379,6 +380,8 @@ struct drm_i915_private {
struct file *mmap_singleton;
} gem;

struct intel_pxp *pxp;

u8 pch_ssc_use;

/* For i915gm/i945gm vblank irq workaround */
Expand Down Expand Up @@ -933,10 +936,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,

#define HAS_GLOBAL_MOCS_REGISTERS(dev_priv) (INTEL_INFO(dev_priv)->has_global_mocs)

#define HAS_PXP(dev_priv) ((IS_ENABLED(CONFIG_DRM_I915_PXP) && \
INTEL_INFO(dev_priv)->has_pxp) && \
VDBOX_MASK(to_gt(dev_priv)))

#define HAS_GMCH(dev_priv) (INTEL_INFO(dev_priv)->display.has_gmch)

#define HAS_GMD_ID(i915) (INTEL_INFO(i915)->has_gmd_id)
Expand Down
Loading

0 comments on commit f67986b

Please sign in to comment.