[Intel-gfx] [PATCH v4 0/6] drm/i915/pxp: Prepare intel_pxp entry points for MTL

[Intel-gfx] [PATCH v4 0/6] drm/i915/pxp: Prepare intel_pxp entry points for MTL

[Alan Previn]

MTL has two tiles that  is represented by the intel_gt structure in the i915
code. The PXP feature has a control-structure that contains the PXP context
and this hangs of the intel_gt structure. In MTL, the standalone media tile
(i.e. not the root tile) contains the VDBOX and KCR engine which is what
PXP relies on for establishing and tearing down the PXP session. However
PXP is a global feature as other engines on other tiles can reference the
PXP session in object info within batch buffer instructions.That coherrency
is handled implicitly by the HW. However current intel_pxp functions such
as intel_pxp_enabled, intel_pxp_start and others take in the intel_gt
structure pointer as its input thus creation the perception that PXP is
a GT-tile specific domain that is independant from other GT tiles.

This series updates all of the intel_pxp_foo functions that are accessed
from outside the PXP subsystem so that the callers only need to pass in the
i915 structure as the input param (being a global handle). Internally,
these functions will loop through all available GT structures on the GPU
and find the one GT structure that contains the one PXP+TEE control
structure before proceeding with the rest of its operation.

Changes from prior revs:
   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 similiar refactoring

Patches that received R-B so far:
   Patch 4, 5, 6.

Alan Previn (6):
  drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT
  drm/i915/pxp: Make intel_pxp_is_enabled implicitly sort PXP-owning-GT
  drm/i915/pxp: Make intel_pxp_is_active implicitly sort PXP-owning-GT
  drm/i915/pxp: Make PXP tee component bind/unbind aware of
    PXP-owning-GT
  drm/i915/pxp: Make intel_pxp_start implicitly sort PXP-owning-GT
  drm/i915/pxp: Make intel_pxp_key_check implicitly sort PXP-owning-GT

 .../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/i915_drv.h               |  4 -
 drivers/gpu/drm/i915/pxp/intel_pxp.c          | 83 ++++++++++++++++---
 drivers/gpu/drm/i915/pxp/intel_pxp.h          | 16 +++-
 drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c      |  2 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c  |  8 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_irq.c      |  4 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_pm.c       |  8 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c      | 18 +++-
 12 files changed, 114 insertions(+), 41 deletions(-)


base-commit: b7288a4715c68710aadbd63112b699356e8a2b65
-- 
2.34.1

[Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Alan Previn]

In preparation for future MTL-PXP feature support, PXP control
context should only valid on the correct gt tile. Depending on the
device-info this depends on which tile owns the VEBOX and KCR.
PXP is still a global feature though (despite its control-context
located in the owning GT structure). Additionally, we find
that the HAS_PXP macro is only used within the pxp module,

That said, lets drop that HAS_PXP macro altogether and replace it
with a more fitting named intel_gtpxp_is_supported and helpers
so that PXP init/fini can use to verify if the referenced gt supports
PXP or teelink.

Add TODO for Meteorlake that will come in future series.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h              |  4 ----
 drivers/gpu/drm/i915/pxp/intel_pxp.c         | 22 ++++++++++++++------
 drivers/gpu/drm/i915/pxp/intel_pxp.h         |  3 +++
 drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c |  2 +-
 4 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7e3820d2c404..0616e5f0bd31 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -933,10 +933,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)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 5efe61f67546..d993e752bd36 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -44,6 +44,20 @@ struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
 	return container_of(pxp, struct intel_gt, pxp);
 }
 
+static bool _gt_needs_teelink(struct intel_gt *gt)
+{
+	/* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */
+	return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && intel_huc_is_loaded_by_gsc(&gt->uc.huc) &&
+		intel_uc_uses_huc(&gt->uc));
+}
+
+bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp)
+{
+	/* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */
+	return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && IS_ENABLED(CONFIG_DRM_I915_PXP) &&
+		INTEL_INFO((pxp_to_gt(pxp))->i915)->has_pxp && VDBOX_MASK(pxp_to_gt(pxp)));
+}
+
 bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
 {
 	return pxp->ce;
@@ -142,17 +156,13 @@ void intel_pxp_init(struct intel_pxp *pxp)
 {
 	struct intel_gt *gt = pxp_to_gt(pxp);
 
-	/* we rely on the mei PXP module */
-	if (!IS_ENABLED(CONFIG_INTEL_MEI_PXP))
-		return;
-
 	/*
 	 * 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))
+	if (intel_pxp_supported_on_gt(pxp))
 		pxp_init_full(pxp);
-	else if (intel_huc_is_loaded_by_gsc(&gt->uc.huc) && intel_uc_uses_huc(&gt->uc))
+	else if (_gt_needs_teelink(gt))
 		intel_pxp_tee_component_init(pxp);
 }
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 2da309088c6d..efa83f9d5e24 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -13,6 +13,9 @@ struct intel_pxp;
 struct drm_i915_gem_object;
 
 struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp);
+
+bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp);
+
 bool intel_pxp_is_enabled(const struct intel_pxp *pxp);
 bool intel_pxp_is_active(const struct intel_pxp *pxp);
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
index 4359e8be4101..f0ad6f34624a 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
@@ -70,7 +70,7 @@ void intel_pxp_debugfs_register(struct intel_pxp *pxp, struct dentry *gt_root)
 	if (!gt_root)
 		return;
 
-	if (!HAS_PXP((pxp_to_gt(pxp)->i915)))
+	if (!intel_pxp_supported_on_gt(pxp))
 		return;
 
 	root = debugfs_create_dir("pxp", gt_root);
-- 
2.34.1

[Intel-gfx] [PATCH v4 2/6] drm/i915/pxp: Make intel_pxp_is_enabled implicitly sort PXP-owning-GT

[Alan Previn]

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 7f2831efc798..c123f4847b19 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 d993e752bd36..88105101af79 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"
 
 /**
@@ -58,11 +59,34 @@ bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp)
 		INTEL_INFO((pxp_to_gt(pxp))->i915)->has_pxp && VDBOX_MASK(pxp_to_gt(pxp)));
 }
 
-bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
+bool intel_pxp_is_enabled_on_gt(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 (intel_pxp_supported_on_gt(&gt->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_pxp_is_enabled_on_gt(&gt->pxp);
+}
+
 bool intel_pxp_is_active(const struct intel_pxp *pxp)
 {
 	return pxp->arb_is_valid;
@@ -216,7 +240,7 @@ int intel_pxp_start(struct intel_pxp *pxp)
 {
 	int ret = 0;
 
-	if (!intel_pxp_is_enabled(pxp))
+	if (!intel_pxp_is_enabled_on_gt(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 efa83f9d5e24..3f71b1653f74 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -11,12 +11,14 @@
 
 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_pxp_supported_on_gt(const struct intel_pxp *pxp);
 
-bool intel_pxp_is_enabled(const struct intel_pxp *pxp);
+bool intel_pxp_is_enabled_on_gt(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..f322a49ebadc 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_pxp_is_enabled_on_gt(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 f0ad6f34624a..4d257055434b 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_pxp_is_enabled_on_gt(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..d3c697bf9aab 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_pxp_is_enabled_on_gt(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..19ac8828cbde 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_pxp_is_enabled_on_gt(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_pxp_is_enabled_on_gt(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_pxp_is_enabled_on_gt(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_pxp_is_enabled_on_gt(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 b0c9170b1395..a5c9c692c20d 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_pxp_is_enabled_on_gt(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_pxp_is_enabled_on_gt(pxp))
 		with_intel_runtime_pm_if_in_use(&i915->runtime_pm, wakeref)
 			intel_pxp_fini_hw(pxp);
 
-- 
2.34.1

[Intel-gfx] [PATCH v4 3/6] drm/i915/pxp: Make intel_pxp_is_active implicitly sort PXP-owning-GT

[Alan Previn]

Make intel_pxp_is_active a global check and implicitly find
the PXP-owning-GT.

As per prior two patches, callers of this function shall now
pass in i915 since PXP is a global GPU feature. Make
intel_pxp_is_active implicitly find the right gt so it's transparent
for global view callers (like display or gem-exec).

However we also need to expose the per-gt variation of this for internal
pxp files to use (like what intel_pxp_is_active was prior) so also expose
a new intel_gtpxp_is_active 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/pxp/intel_pxp.c         | 14 ++++++++++++--
 drivers/gpu/drm/i915/pxp/intel_pxp.h         |  3 ++-
 drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c |  4 ++--
 drivers/gpu/drm/i915/pxp/intel_pxp_irq.c     |  2 +-
 5 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index c123f4847b19..165be45a3c13 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -271,7 +271,7 @@ 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))
+		if (!intel_pxp_is_active(i915))
 			ret = intel_pxp_start(&to_gt(i915)->pxp);
 	}
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 88105101af79..76a924587543 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -87,11 +87,21 @@ bool intel_pxp_is_enabled(struct drm_i915_private *i915)
 	return intel_pxp_is_enabled_on_gt(&gt->pxp);
 }
 
-bool intel_pxp_is_active(const struct intel_pxp *pxp)
+bool intel_pxp_is_active_on_gt(const struct intel_pxp *pxp)
 {
 	return pxp->arb_is_valid;
 }
 
+bool intel_pxp_is_active(struct drm_i915_private *i915)
+{
+	struct intel_gt *gt = i915_to_pxp_gt(i915);
+
+	if (!gt)
+		return false;
+
+	return intel_pxp_is_active_on_gt(&gt->pxp);
+}
+
 /* KCR register definitions */
 #define KCR_INIT _MMIO(0x320f0)
 /* Setting KCR Init bit is required after system boot */
@@ -287,7 +297,7 @@ int intel_pxp_key_check(struct intel_pxp *pxp,
 			struct drm_i915_gem_object *obj,
 			bool assign)
 {
-	if (!intel_pxp_is_active(pxp))
+	if (!intel_pxp_is_active_on_gt(pxp))
 		return -ENODEV;
 
 	if (!i915_gem_object_is_protected(obj))
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 3f71b1653f74..fe981eebf0ec 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -19,7 +19,8 @@ bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp);
 
 bool intel_pxp_is_enabled_on_gt(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);
+bool intel_pxp_is_active_on_gt(const struct intel_pxp *pxp);
+bool intel_pxp_is_active(struct drm_i915_private *i915);
 
 void intel_pxp_init(struct intel_pxp *pxp);
 void intel_pxp_fini(struct intel_pxp *pxp);
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
index 4d257055434b..52a808fd4704 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
@@ -25,7 +25,7 @@ static int pxp_info_show(struct seq_file *m, void *data)
 		return 0;
 	}
 
-	drm_printf(&p, "active: %s\n", str_yes_no(intel_pxp_is_active(pxp)));
+	drm_printf(&p, "active: %s\n", str_yes_no(intel_pxp_is_active_on_gt(pxp)));
 	drm_printf(&p, "instance counter: %u\n", pxp->key_instance);
 
 	return 0;
@@ -43,7 +43,7 @@ static int pxp_terminate_set(void *data, u64 val)
 	struct intel_pxp *pxp = data;
 	struct intel_gt *gt = pxp_to_gt(pxp);
 
-	if (!intel_pxp_is_active(pxp))
+	if (!intel_pxp_is_active_on_gt(pxp))
 		return -ENODEV;
 
 	/* simulate a termination interrupt */
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
index d3c697bf9aab..c25c1979cccc 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
@@ -86,7 +86,7 @@ void intel_pxp_irq_disable(struct intel_pxp *pxp)
 	 * called in a path were the driver consider the session as valid and
 	 * doesn't call a termination on restart.
 	 */
-	GEM_WARN_ON(intel_pxp_is_active(pxp));
+	GEM_WARN_ON(intel_pxp_is_active_on_gt(pxp));
 
 	spin_lock_irq(gt->irq_lock);
 
-- 
2.34.1

[Intel-gfx] [PATCH v4 4/6] drm/i915/pxp: Make PXP tee component bind/unbind aware of PXP-owning-GT

[Alan Previn]

Ensure i915_pxp_tee_component_bind / unbind implicitly sorts out
getting the correct per-GT PXP control-context from the PXP-owning-GT
when establishing or ending connection. Thus, replace _i915_to_pxp_gt
with intel_pxp_get_owning_gt (also takes in i915).

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/pxp/intel_pxp.c     |  6 +++---
 drivers/gpu/drm/i915/pxp/intel_pxp.h     |  2 ++
 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 14 ++++++++++++--
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 76a924587543..6a78b6ef0235 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -64,7 +64,7 @@ bool intel_pxp_is_enabled_on_gt(const struct intel_pxp *pxp)
 	return pxp->ce;
 }
 
-static struct intel_gt *i915_to_pxp_gt(struct drm_i915_private *i915)
+struct intel_gt *intel_pxp_get_owning_gt(struct drm_i915_private *i915)
 {
 	struct intel_gt *gt = NULL;
 	int i = 0;
@@ -79,7 +79,7 @@ static struct intel_gt *i915_to_pxp_gt(struct drm_i915_private *i915)
 
 bool intel_pxp_is_enabled(struct drm_i915_private *i915)
 {
-	struct intel_gt *gt = i915_to_pxp_gt(i915);
+	struct intel_gt *gt = intel_pxp_get_owning_gt(i915);
 
 	if (!gt)
 		return false;
@@ -94,7 +94,7 @@ bool intel_pxp_is_active_on_gt(const struct intel_pxp *pxp)
 
 bool intel_pxp_is_active(struct drm_i915_private *i915)
 {
-	struct intel_gt *gt = i915_to_pxp_gt(i915);
+	struct intel_gt *gt = intel_pxp_get_owning_gt(i915);
 
 	if (!gt)
 		return false;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index fe981eebf0ec..c798c3bde957 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -13,6 +13,8 @@ struct intel_pxp;
 struct drm_i915_gem_object;
 struct drm_i915_private;
 
+struct intel_gt *intel_pxp_get_owning_gt(struct drm_i915_private *i915);
+
 struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp);
 
 bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp);
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
index a5c9c692c20d..b9198e961cb6 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
@@ -20,8 +20,12 @@
 static inline struct intel_pxp *i915_dev_to_pxp(struct device *i915_kdev)
 {
 	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
+	struct intel_gt *gt = intel_pxp_get_owning_gt(i915);
 
-	return &to_gt(i915)->pxp;
+	if (!gt)
+		return NULL;
+
+	return &gt->pxp;
 }
 
 static int intel_pxp_tee_io_message(struct intel_pxp *pxp,
@@ -128,10 +132,16 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
 {
 	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
 	struct intel_pxp *pxp = i915_dev_to_pxp(i915_kdev);
-	struct intel_uc *uc = &pxp_to_gt(pxp)->uc;
+	struct intel_uc *uc;
 	intel_wakeref_t wakeref;
 	int ret = 0;
 
+	if (!pxp) {
+		drm_warn(&i915->drm, "tee comp binding without a PXP-owner GT\n");
+		return -ENODEV;
+	}
+	uc = &pxp_to_gt(pxp)->uc;
+
 	mutex_lock(&pxp->tee_mutex);
 	pxp->pxp_component = data;
 	pxp->pxp_component->tee_dev = tee_kdev;
-- 
2.34.1

[Intel-gfx] [PATCH v4 5/6] drm/i915/pxp: Make intel_pxp_start implicitly sort PXP-owning-GT

[Alan Previn]

Make intel_pxp_is_start implicitly find the PXP-owning-GT.
Callers of this function shall now pass in i915 since PXP
is a global GPU feature. Make intel_pxp_start implicitly
find the right gt to start PXP arb session so
it's transparent to the callers.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +-
 drivers/gpu/drm/i915/pxp/intel_pxp.c        | 9 ++++++++-
 drivers/gpu/drm/i915/pxp/intel_pxp.h        | 2 +-
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 165be45a3c13..15c3d435093a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -272,7 +272,7 @@ 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(i915))
-			ret = intel_pxp_start(&to_gt(i915)->pxp);
+			ret = intel_pxp_start(i915);
 	}
 
 	return ret;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 6a78b6ef0235..43f3790e1520 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -246,10 +246,17 @@ static bool pxp_component_bound(struct intel_pxp *pxp)
  * the arb session is restarted from the irq work when we receive the
  * termination completion interrupt
  */
-int intel_pxp_start(struct intel_pxp *pxp)
+int intel_pxp_start(struct drm_i915_private *i915)
 {
+	struct intel_gt *gt = intel_pxp_get_owning_gt(i915);
+	struct intel_pxp *pxp;
 	int ret = 0;
 
+	if (!gt)
+		return -ENODEV;
+
+	pxp = &gt->pxp;
+
 	if (!intel_pxp_is_enabled_on_gt(pxp))
 		return -ENODEV;
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index c798c3bde957..7b2b93a2ba94 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -32,7 +32,7 @@ void intel_pxp_fini_hw(struct intel_pxp *pxp);
 
 void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
 
-int intel_pxp_start(struct intel_pxp *pxp);
+int intel_pxp_start(struct drm_i915_private *i915);
 
 int intel_pxp_key_check(struct intel_pxp *pxp,
 			struct drm_i915_gem_object *obj,
-- 
2.34.1

[Intel-gfx] [PATCH v4 6/6] drm/i915/pxp: Make intel_pxp_key_check implicitly sort PXP-owning-GT

[Alan Previn]

Make intel_pxp_key_check implicitly find the PXP-owning-GT.
Callers of this function shall now pass in i915 since PXP
is a global GPU feature. Make intel_pxp_key_check implicitly
find the right gt to verify pxp session key establishment count
so it's transparent to the callers.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/display/skl_universal_plane.c |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c     |  2 +-
 drivers/gpu/drm/i915/pxp/intel_pxp.c               | 10 +++++++++-
 drivers/gpu/drm/i915/pxp/intel_pxp.h               |  2 +-
 4 files changed, 12 insertions(+), 4 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..3436bf433c10 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->base.dev);
 
-	return intel_pxp_key_check(&to_gt(i915)->pxp, obj, false) == 0;
+	return intel_pxp_key_check(i915, obj, false) == 0;
 }
 
 static bool pxp_is_borked(struct drm_i915_gem_object *obj)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 29e9e8d5b6fe..9943d5827300 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -869,7 +869,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(vm->gt->i915, obj, true);
 			if (err) {
 				i915_gem_object_put(obj);
 				return ERR_PTR(err);
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 43f3790e1520..58219beecfa4 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -300,10 +300,18 @@ void intel_pxp_fini_hw(struct intel_pxp *pxp)
 	intel_pxp_irq_disable(pxp);
 }
 
-int intel_pxp_key_check(struct intel_pxp *pxp,
+int intel_pxp_key_check(struct drm_i915_private *i915,
 			struct drm_i915_gem_object *obj,
 			bool assign)
 {
+	struct intel_gt *gt = intel_pxp_get_owning_gt(i915);
+	struct intel_pxp *pxp;
+
+	if (!gt)
+		return -ENODEV;
+
+	pxp = &gt->pxp;
+
 	if (!intel_pxp_is_active_on_gt(pxp))
 		return -ENODEV;
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 7b2b93a2ba94..6fe1595a84d6 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -34,7 +34,7 @@ void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
 
 int intel_pxp_start(struct drm_i915_private *i915);
 
-int intel_pxp_key_check(struct intel_pxp *pxp,
+int intel_pxp_key_check(struct drm_i915_private *i915,
 			struct drm_i915_gem_object *obj,
 			bool assign);
 
-- 
2.34.1

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/pxp: Prepare intel_pxp entry points for MTL (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915/pxp: Prepare intel_pxp entry points for MTL (rev4)
URL   : https://patchwork.freedesktop.org/series/109429/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/pxp: Prepare intel_pxp entry points for MTL (rev4)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 8641 bytes --]

== Series Details ==

Series: drm/i915/pxp: Prepare intel_pxp entry points for MTL (rev4)
URL   : https://patchwork.freedesktop.org/series/109429/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12390 -> Patchwork_109429v4
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/index.html

Participating hosts (41 -> 39)
------------------------------

  Missing    (2): bat-dg1-6 bat-dg1-5 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_109429v4:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@core_hotunplug@unbind-rebind:
    - {bat-dg2-9}:        [PASS][1] -> [DMESG-WARN][2] +4 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/bat-dg2-9/igt@core_hotunplug@unbind-rebind.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/bat-dg2-9/igt@core_hotunplug@unbind-rebind.html

  * igt@gem_lmem_swapping@parallel-random-engines@lmem0:
    - {bat-dg2-8}:        [PASS][3] -> [DMESG-WARN][4] +8 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/bat-dg2-8/igt@gem_lmem_swapping@parallel-random-engines@lmem0.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/bat-dg2-8/igt@gem_lmem_swapping@parallel-random-engines@lmem0.html

  * igt@i915_pm_rpm@module-reload:
    - {bat-dg2-11}:       [PASS][5] -> [DMESG-WARN][6] +6 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/bat-dg2-11/igt@i915_pm_rpm@module-reload.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/bat-dg2-11/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live@workarounds:
    - {bat-dg2-9}:        [PASS][7] -> [INCOMPLETE][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/bat-dg2-9/igt@i915_selftest@live@workarounds.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/bat-dg2-9/igt@i915_selftest@live@workarounds.html
    - {bat-dg2-11}:       [PASS][9] -> [INCOMPLETE][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/bat-dg2-11/igt@i915_selftest@live@workarounds.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/bat-dg2-11/igt@i915_selftest@live@workarounds.html
    - {bat-dg2-8}:        [PASS][11] -> [INCOMPLETE][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/bat-dg2-8/igt@i915_selftest@live@workarounds.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/bat-dg2-8/igt@i915_selftest@live@workarounds.html

  
Known issues
------------

  Here are the changes found in Patchwork_109429v4 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_lmem_swapping@parallel-random-engines:
    - bat-adlp-4:         NOTRUN -> [SKIP][13] ([i915#4613]) +3 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/bat-adlp-4/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@i915_pm_rps@basic-api:
    - bat-adlp-4:         NOTRUN -> [SKIP][14] ([i915#6621])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/bat-adlp-4/igt@i915_pm_rps@basic-api.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - bat-adlp-4:         NOTRUN -> [SKIP][15] ([fdo#111827])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/bat-adlp-4/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_pipe_crc_basic@suspend-read-crc:
    - bat-adlp-4:         NOTRUN -> [SKIP][16] ([i915#3546])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/bat-adlp-4/igt@kms_pipe_crc_basic@suspend-read-crc.html

  * igt@prime_vgem@basic-userptr:
    - bat-adlp-4:         NOTRUN -> [SKIP][17] ([fdo#109295] / [i915#3301] / [i915#3708])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/bat-adlp-4/igt@prime_vgem@basic-userptr.html

  * igt@prime_vgem@basic-write:
    - bat-adlp-4:         NOTRUN -> [SKIP][18] ([fdo#109295] / [i915#3291] / [i915#3708]) +2 similar issues
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/bat-adlp-4/igt@prime_vgem@basic-write.html

  * igt@runner@aborted:
    - fi-hsw-4770:        NOTRUN -> [FAIL][19] ([fdo#109271] / [i915#4312] / [i915#5594])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/fi-hsw-4770/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - bat-adlp-4:         [DMESG-WARN][20] ([i915#7077]) -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/bat-adlp-4/igt@i915_pm_rpm@basic-pci-d3-state.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/bat-adlp-4/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-2:
    - {bat-dg2-11}:       [FAIL][22] -> [PASS][23]
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-2.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-2.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4258]: https://gitlab.freedesktop.org/drm/intel/issues/4258
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5594]: https://gitlab.freedesktop.org/drm/intel/issues/5594
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6434]: https://gitlab.freedesktop.org/drm/intel/issues/6434
  [i915#6559]: https://gitlab.freedesktop.org/drm/intel/issues/6559
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
  [i915#7029]: https://gitlab.freedesktop.org/drm/intel/issues/7029
  [i915#7077]: https://gitlab.freedesktop.org/drm/intel/issues/7077
  [i915#7346]: https://gitlab.freedesktop.org/drm/intel/issues/7346
  [i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456


Build changes
-------------

  * Linux: CI_DRM_12390 -> Patchwork_109429v4

  CI-20190529: 20190529
  CI_DRM_12390: b7288a4715c68710aadbd63112b699356e8a2b65 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7062: 6539ea5fe17fce683133c45f07fac316593ee1f7 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_109429v4: b7288a4715c68710aadbd63112b699356e8a2b65 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

6d1482dd6ebb drm/i915/pxp: Make intel_pxp_key_check implicitly sort PXP-owning-GT
8df585ff3d63 drm/i915/pxp: Make intel_pxp_start implicitly sort PXP-owning-GT
d93c3b7191ff drm/i915/pxp: Make PXP tee component bind/unbind aware of PXP-owning-GT
2e7b3fdda742 drm/i915/pxp: Make intel_pxp_is_active implicitly sort PXP-owning-GT
34e845c7ead2 drm/i915/pxp: Make intel_pxp_is_enabled implicitly sort PXP-owning-GT
590dd5452e25 drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/index.html

[-- Attachment #2: Type: text/html, Size: 8492 bytes --]

[Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915/pxp: Prepare intel_pxp entry points for MTL (rev4)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 32400 bytes --]

== Series Details ==

Series: drm/i915/pxp: Prepare intel_pxp entry points for MTL (rev4)
URL   : https://patchwork.freedesktop.org/series/109429/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12390_full -> Patchwork_109429v4_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_109429v4_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_109429v4_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Participating hosts (9 -> 9)
------------------------------

  No changes in participating hosts

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_109429v4_full:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_fence@syncobj-backward-timeline-chain-engines:
    - shard-skl:          [PASS][1] -> [WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl3/igt@gem_exec_fence@syncobj-backward-timeline-chain-engines.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl5/igt@gem_exec_fence@syncobj-backward-timeline-chain-engines.html

  * igt@kms_vblank@pipe-c-accuracy-idle:
    - shard-tglb:         [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-tglb7/igt@kms_vblank@pipe-c-accuracy-idle.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb8/igt@kms_vblank@pipe-c-accuracy-idle.html

  
Known issues
------------

  Here are the changes found in Patchwork_109429v4_full that come from known issues:

### CI changes ###

#### Issues hit ####

  * boot:
    - shard-skl:          ([PASS][5], [PASS][6], [PASS][7], [PASS][8], [PASS][9], [PASS][10], [PASS][11], [PASS][12], [PASS][13], [PASS][14], [PASS][15], [PASS][16], [PASS][17], [PASS][18], [PASS][19], [PASS][20], [PASS][21], [PASS][22], [PASS][23], [PASS][24], [PASS][25], [PASS][26]) -> ([PASS][27], [PASS][28], [PASS][29], [PASS][30], [PASS][31], [PASS][32], [PASS][33], [PASS][34], [FAIL][35], [PASS][36], [PASS][37], [PASS][38], [PASS][39], [PASS][40], [PASS][41], [PASS][42], [PASS][43], [PASS][44], [PASS][45], [PASS][46], [PASS][47], [PASS][48]) ([i915#5032])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl9/boot.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl9/boot.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl7/boot.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl7/boot.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl7/boot.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl6/boot.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl6/boot.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl6/boot.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl5/boot.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl5/boot.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl4/boot.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl4/boot.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl4/boot.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl3/boot.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl3/boot.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl2/boot.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl1/boot.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl1/boot.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl1/boot.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl10/boot.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl10/boot.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl10/boot.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl10/boot.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl10/boot.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl10/boot.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl1/boot.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl1/boot.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl1/boot.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl2/boot.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl3/boot.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl3/boot.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl4/boot.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl4/boot.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl4/boot.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl5/boot.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl5/boot.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl6/boot.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl6/boot.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl6/boot.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl7/boot.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl7/boot.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl7/boot.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl9/boot.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl9/boot.html

  

### IGT changes ###

#### Issues hit ####

  * igt@feature_discovery@display-2x:
    - shard-tglb:         NOTRUN -> [SKIP][49] ([i915#1839])
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@feature_discovery@display-2x.html

  * igt@gem_create@create-massive:
    - shard-skl:          NOTRUN -> [DMESG-WARN][50] ([i915#4991])
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl1/igt@gem_create@create-massive.html

  * igt@gem_exec_balancer@parallel-keep-in-fence:
    - shard-iclb:         [PASS][51] -> [SKIP][52] ([i915#4525])
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-iclb1/igt@gem_exec_balancer@parallel-keep-in-fence.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb3/igt@gem_exec_balancer@parallel-keep-in-fence.html

  * igt@gem_exec_fair@basic-none@vecs0:
    - shard-glk:          [PASS][53] -> [FAIL][54] ([i915#2842])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-glk2/igt@gem_exec_fair@basic-none@vecs0.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-glk2/igt@gem_exec_fair@basic-none@vecs0.html

  * igt@gem_exec_fair@basic-pace@vcs1:
    - shard-iclb:         NOTRUN -> [FAIL][55] ([i915#2842])
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb2/igt@gem_exec_fair@basic-pace@vcs1.html

  * igt@gem_lmem_swapping@random-engines:
    - shard-skl:          NOTRUN -> [SKIP][56] ([fdo#109271] / [i915#4613]) +3 similar issues
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl7/igt@gem_lmem_swapping@random-engines.html

  * igt@gem_userptr_blits@unsync-overlap:
    - shard-skl:          NOTRUN -> [SKIP][57] ([fdo#109271]) +181 similar issues
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl1/igt@gem_userptr_blits@unsync-overlap.html

  * igt@gem_userptr_blits@vma-merge:
    - shard-skl:          NOTRUN -> [FAIL][58] ([i915#3318])
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl7/igt@gem_userptr_blits@vma-merge.html

  * igt@gen7_exec_parse@chained-batch:
    - shard-tglb:         NOTRUN -> [SKIP][59] ([fdo#109289]) +1 similar issue
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@gen7_exec_parse@chained-batch.html

  * igt@gen9_exec_parse@allowed-single:
    - shard-apl:          [PASS][60] -> [DMESG-WARN][61] ([i915#5566] / [i915#716])
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-apl7/igt@gen9_exec_parse@allowed-single.html
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-apl2/igt@gen9_exec_parse@allowed-single.html

  * igt@i915_module_load@load:
    - shard-skl:          NOTRUN -> [SKIP][62] ([fdo#109271] / [i915#6227])
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl1/igt@i915_module_load@load.html

  * igt@i915_pm_dc@dc6-dpms:
    - shard-tglb:         NOTRUN -> [FAIL][63] ([i915#3989] / [i915#454])
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@i915_pm_dc@dc6-dpms.html

  * igt@i915_pm_dc@dc6-psr:
    - shard-skl:          NOTRUN -> [FAIL][64] ([i915#3989] / [i915#454])
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl9/igt@i915_pm_dc@dc6-psr.html

  * igt@i915_pm_rc6_residency@rc6-idle@vcs0:
    - shard-skl:          NOTRUN -> [WARN][65] ([i915#1804])
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl1/igt@i915_pm_rc6_residency@rc6-idle@vcs0.html

  * igt@i915_selftest@live@gt_heartbeat:
    - shard-skl:          [PASS][66] -> [DMESG-FAIL][67] ([i915#5334])
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl7/igt@i915_selftest@live@gt_heartbeat.html
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl7/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@hangcheck:
    - shard-tglb:         [PASS][68] -> [DMESG-WARN][69] ([i915#5591])
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-tglb6/igt@i915_selftest@live@hangcheck.html
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb8/igt@i915_selftest@live@hangcheck.html

  * igt@kms_big_fb@yf-tiled-max-hw-stride-32bpp-rotate-180-async-flip:
    - shard-tglb:         NOTRUN -> [SKIP][70] ([fdo#111615])
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@kms_big_fb@yf-tiled-max-hw-stride-32bpp-rotate-180-async-flip.html

  * igt@kms_big_joiner@invalid-modeset:
    - shard-tglb:         NOTRUN -> [SKIP][71] ([i915#2705])
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@kms_big_joiner@invalid-modeset.html

  * igt@kms_ccs@pipe-a-crc-sprite-planes-basic-y_tiled_gen12_rc_ccs_cc:
    - shard-skl:          NOTRUN -> [SKIP][72] ([fdo#109271] / [i915#3886]) +6 similar issues
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl1/igt@kms_ccs@pipe-a-crc-sprite-planes-basic-y_tiled_gen12_rc_ccs_cc.html

  * igt@kms_ccs@pipe-b-bad-aux-stride-yf_tiled_ccs:
    - shard-tglb:         NOTRUN -> [SKIP][73] ([fdo#111615] / [i915#3689])
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@kms_ccs@pipe-b-bad-aux-stride-yf_tiled_ccs.html

  * igt@kms_ccs@pipe-c-random-ccs-data-y_tiled_ccs:
    - shard-tglb:         NOTRUN -> [SKIP][74] ([i915#3689]) +1 similar issue
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@kms_ccs@pipe-c-random-ccs-data-y_tiled_ccs.html

  * igt@kms_chamelium@dp-hpd-storm-disable:
    - shard-tglb:         NOTRUN -> [SKIP][75] ([fdo#109284] / [fdo#111827]) +1 similar issue
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@kms_chamelium@dp-hpd-storm-disable.html

  * igt@kms_color_chamelium@ctm-limited-range:
    - shard-skl:          NOTRUN -> [SKIP][76] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl7/igt@kms_color_chamelium@ctm-limited-range.html

  * igt@kms_cursor_crc@cursor-rapid-movement-max-size:
    - shard-tglb:         NOTRUN -> [SKIP][77] ([i915#3555])
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@kms_cursor_crc@cursor-rapid-movement-max-size.html

  * igt@kms_display_modes@extended-mode-basic:
    - shard-tglb:         NOTRUN -> [SKIP][78] ([fdo#109274])
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@kms_display_modes@extended-mode-basic.html

  * igt@kms_fbcon_fbt@fbc:
    - shard-tglb:         NOTRUN -> [FAIL][79] ([i915#4767])
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@kms_fbcon_fbt@fbc.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1:
    - shard-skl:          NOTRUN -> [FAIL][80] ([i915#79])
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl9/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1.html

  * igt@kms_flip@flip-vs-expired-vblank@c-hdmi-a2:
    - shard-glk:          [PASS][81] -> [FAIL][82] ([i915#79])
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-glk8/igt@kms_flip@flip-vs-expired-vblank@c-hdmi-a2.html
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-glk8/igt@kms_flip@flip-vs-expired-vblank@c-hdmi-a2.html

  * igt@kms_flip@flip-vs-suspend@a-edp1:
    - shard-skl:          [PASS][83] -> [INCOMPLETE][84] ([i915#4839])
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl7/igt@kms_flip@flip-vs-suspend@a-edp1.html
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl5/igt@kms_flip@flip-vs-suspend@a-edp1.html

  * igt@kms_flip@wf_vblank-ts-check@a-edp1:
    - shard-skl:          [PASS][85] -> [FAIL][86] ([i915#2122]) +1 similar issue
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl7/igt@kms_flip@wf_vblank-ts-check@a-edp1.html
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl7/igt@kms_flip@wf_vblank-ts-check@a-edp1.html

  * igt@kms_flip_scaled_crc@flip-32bpp-yftile-to-64bpp-yftile-downscaling@pipe-a-valid-mode:
    - shard-iclb:         NOTRUN -> [SKIP][87] ([i915#2587] / [i915#2672]) +3 similar issues
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb6/igt@kms_flip_scaled_crc@flip-32bpp-yftile-to-64bpp-yftile-downscaling@pipe-a-valid-mode.html

  * igt@kms_flip_scaled_crc@flip-64bpp-4tile-to-16bpp-4tile-downscaling@pipe-a-valid-mode:
    - shard-tglb:         NOTRUN -> [SKIP][88] ([i915#2587] / [i915#2672])
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@kms_flip_scaled_crc@flip-64bpp-4tile-to-16bpp-4tile-downscaling@pipe-a-valid-mode.html

  * igt@kms_flip_scaled_crc@flip-64bpp-4tile-to-32bpp-4tile-downscaling@pipe-a-default-mode:
    - shard-iclb:         NOTRUN -> [SKIP][89] ([i915#2672]) +2 similar issues
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb2/igt@kms_flip_scaled_crc@flip-64bpp-4tile-to-32bpp-4tile-downscaling@pipe-a-default-mode.html

  * igt@kms_flip_scaled_crc@flip-64bpp-linear-to-32bpp-linear-downscaling@pipe-a-default-mode:
    - shard-iclb:         NOTRUN -> [SKIP][90] ([i915#3555]) +3 similar issues
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb2/igt@kms_flip_scaled_crc@flip-64bpp-linear-to-32bpp-linear-downscaling@pipe-a-default-mode.html

  * igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilegen12rcccs-upscaling@pipe-a-valid-mode:
    - shard-iclb:         NOTRUN -> [SKIP][91] ([i915#2672] / [i915#3555])
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb8/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilegen12rcccs-upscaling@pipe-a-valid-mode.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-draw-mmap-wc:
    - shard-tglb:         NOTRUN -> [SKIP][92] ([fdo#109280] / [fdo#111825]) +1 similar issue
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-draw-mmap-wc.html

  * igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite:
    - shard-glk:          [PASS][93] -> [FAIL][94] ([i915#2546])
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-glk3/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-glk7/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-mmap-wc:
    - shard-tglb:         NOTRUN -> [SKIP][95] ([i915#6497]) +2 similar issues
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-mmap-wc.html

  * igt@kms_plane_alpha_blend@alpha-basic@pipe-a-edp-1:
    - shard-skl:          NOTRUN -> [FAIL][96] ([i915#4573]) +4 similar issues
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl9/igt@kms_plane_alpha_blend@alpha-basic@pipe-a-edp-1.html

  * igt@kms_plane_alpha_blend@alpha-basic@pipe-c-edp-1:
    - shard-skl:          NOTRUN -> [DMESG-FAIL][97] ([IGT#6])
   [97]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl9/igt@kms_plane_alpha_blend@alpha-basic@pipe-c-edp-1.html

  * igt@kms_plane_scaling@plane-downscale-with-pixel-format-factor-0-5@pipe-b-edp-1:
    - shard-iclb:         [PASS][98] -> [SKIP][99] ([i915#5176]) +2 similar issues
   [98]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-iclb8/igt@kms_plane_scaling@plane-downscale-with-pixel-format-factor-0-5@pipe-b-edp-1.html
   [99]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb2/igt@kms_plane_scaling@plane-downscale-with-pixel-format-factor-0-5@pipe-b-edp-1.html

  * igt@kms_plane_scaling@planes-unity-scaling-downscale-factor-0-5@pipe-c-edp-1:
    - shard-iclb:         [PASS][100] -> [SKIP][101] ([i915#5235]) +2 similar issues
   [100]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-iclb8/igt@kms_plane_scaling@planes-unity-scaling-downscale-factor-0-5@pipe-c-edp-1.html
   [101]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb2/igt@kms_plane_scaling@planes-unity-scaling-downscale-factor-0-5@pipe-c-edp-1.html

  * igt@kms_psr2_sf@overlay-plane-move-continuous-sf:
    - shard-skl:          NOTRUN -> [SKIP][102] ([fdo#109271] / [i915#658]) +1 similar issue
   [102]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl1/igt@kms_psr2_sf@overlay-plane-move-continuous-sf.html

  * igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-big-fb:
    - shard-tglb:         NOTRUN -> [SKIP][103] ([i915#2920])
   [103]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-big-fb.html

  * igt@kms_psr2_su@page_flip-nv12@pipe-b-edp-1:
    - shard-iclb:         NOTRUN -> [FAIL][104] ([i915#5939]) +2 similar issues
   [104]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb2/igt@kms_psr2_su@page_flip-nv12@pipe-b-edp-1.html

  * igt@kms_psr2_su@page_flip-p010:
    - shard-iclb:         NOTRUN -> [SKIP][105] ([fdo#109642] / [fdo#111068] / [i915#658])
   [105]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb5/igt@kms_psr2_su@page_flip-p010.html

  * igt@kms_psr@psr2_cursor_plane_onoff:
    - shard-iclb:         [PASS][106] -> [SKIP][107] ([fdo#109441])
   [106]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-iclb2/igt@kms_psr@psr2_cursor_plane_onoff.html
   [107]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb5/igt@kms_psr@psr2_cursor_plane_onoff.html

  * igt@kms_psr@psr2_sprite_plane_onoff:
    - shard-tglb:         NOTRUN -> [FAIL][108] ([i915#132] / [i915#3467])
   [108]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@kms_psr@psr2_sprite_plane_onoff.html

  * igt@sysfs_clients@fair-3:
    - shard-skl:          NOTRUN -> [SKIP][109] ([fdo#109271] / [i915#2994]) +1 similar issue
   [109]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl7/igt@sysfs_clients@fair-3.html

  
#### Possible fixes ####

  * igt@gem_exec_balancer@parallel-out-fence:
    - shard-iclb:         [SKIP][110] ([i915#4525]) -> [PASS][111] +1 similar issue
   [110]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-iclb5/igt@gem_exec_balancer@parallel-out-fence.html
   [111]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb1/igt@gem_exec_balancer@parallel-out-fence.html

  * igt@gem_exec_fair@basic-none-solo@rcs0:
    - shard-apl:          [FAIL][112] ([i915#2842]) -> [PASS][113] +1 similar issue
   [112]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-apl2/igt@gem_exec_fair@basic-none-solo@rcs0.html
   [113]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-apl6/igt@gem_exec_fair@basic-none-solo@rcs0.html

  * igt@i915_pm_rps@engine-order:
    - shard-tglb:         [FAIL][114] ([i915#6537]) -> [PASS][115]
   [114]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-tglb8/igt@i915_pm_rps@engine-order.html
   [115]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@i915_pm_rps@engine-order.html

  * igt@kms_ccs@pipe-a-bad-rotation-90-y_tiled_gen12_rc_ccs:
    - shard-tglb:         [INCOMPLETE][116] ([i915#6453]) -> [PASS][117]
   [116]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-tglb8/igt@kms_ccs@pipe-a-bad-rotation-90-y_tiled_gen12_rc_ccs.html
   [117]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-tglb5/igt@kms_ccs@pipe-a-bad-rotation-90-y_tiled_gen12_rc_ccs.html

  * igt@kms_flip@plain-flip-fb-recreate@a-edp1:
    - shard-skl:          [FAIL][118] ([i915#2122]) -> [PASS][119]
   [118]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl1/igt@kms_flip@plain-flip-fb-recreate@a-edp1.html
   [119]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl6/igt@kms_flip@plain-flip-fb-recreate@a-edp1.html

  * igt@kms_plane_cursor@viewport@pipe-b-edp-1-size-256:
    - shard-skl:          [INCOMPLETE][120] ([i915#7557]) -> [PASS][121]
   [120]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl2/igt@kms_plane_cursor@viewport@pipe-b-edp-1-size-256.html
   [121]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl9/igt@kms_plane_cursor@viewport@pipe-b-edp-1-size-256.html

  * igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-5@pipe-a-edp-1:
    - shard-iclb:         [SKIP][122] ([i915#5235]) -> [PASS][123] +2 similar issues
   [122]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-iclb2/igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-5@pipe-a-edp-1.html
   [123]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb5/igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-5@pipe-a-edp-1.html

  * igt@kms_psr@psr2_sprite_blt:
    - shard-iclb:         [SKIP][124] ([fdo#109441]) -> [PASS][125] +2 similar issues
   [124]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-iclb8/igt@kms_psr@psr2_sprite_blt.html
   [125]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb2/igt@kms_psr@psr2_sprite_blt.html

  * igt@kms_psr_stress_test@invalidate-primary-flip-overlay:
    - shard-iclb:         [SKIP][126] ([i915#5519]) -> [PASS][127]
   [126]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-iclb1/igt@kms_psr_stress_test@invalidate-primary-flip-overlay.html
   [127]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb5/igt@kms_psr_stress_test@invalidate-primary-flip-overlay.html

  * igt@perf_pmu@interrupts:
    - shard-skl:          [FAIL][128] ([i915#7318]) -> [PASS][129]
   [128]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-skl4/igt@perf_pmu@interrupts.html
   [129]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-skl10/igt@perf_pmu@interrupts.html

  
#### Warnings ####

  * igt@gem_exec_balancer@parallel-ordering:
    - shard-iclb:         [SKIP][130] ([i915#4525]) -> [FAIL][131] ([i915#6117])
   [130]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-iclb8/igt@gem_exec_balancer@parallel-ordering.html
   [131]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb2/igt@gem_exec_balancer@parallel-ordering.html

  * igt@i915_pm_dc@dc3co-vpb-simulation:
    - shard-iclb:         [SKIP][132] ([i915#658]) -> [SKIP][133] ([i915#588])
   [132]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-iclb8/igt@i915_pm_dc@dc3co-vpb-simulation.html
   [133]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb2/igt@i915_pm_dc@dc3co-vpb-simulation.html

  * igt@kms_psr2_sf@cursor-plane-move-continuous-sf:
    - shard-iclb:         [SKIP][134] ([i915#658]) -> [SKIP][135] ([i915#2920])
   [134]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-iclb8/igt@kms_psr2_sf@cursor-plane-move-continuous-sf.html
   [135]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb2/igt@kms_psr2_sf@cursor-plane-move-continuous-sf.html

  * igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-sf:
    - shard-iclb:         [SKIP][136] ([i915#2920]) -> [SKIP][137] ([i915#658])
   [136]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-iclb2/igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-sf.html
   [137]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb8/igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-sf.html

  * igt@kms_psr2_sf@primary-plane-update-sf-dmg-area:
    - shard-iclb:         [SKIP][138] ([i915#2920]) -> [SKIP][139] ([fdo#111068] / [i915#658])
   [138]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-iclb2/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area.html
   [139]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-iclb1/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area.html

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-glk:          [FAIL][140] ([i915#5852]) -> [DMESG-FAIL][141] ([i915#118])
   [140]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-glk7/igt@kms_rotation_crc@multiplane-rotation-cropping-top.html
   [141]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-glk8/igt@kms_rotation_crc@multiplane-rotation-cropping-top.html

  * igt@runner@aborted:
    - shard-apl:          ([FAIL][142], [FAIL][143]) ([i915#3002] / [i915#4312]) -> ([FAIL][144], [FAIL][145], [FAIL][146]) ([fdo#109271] / [i915#3002] / [i915#4312])
   [142]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-apl2/igt@runner@aborted.html
   [143]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/shard-apl7/igt@runner@aborted.html
   [144]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-apl2/igt@runner@aborted.html
   [145]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-apl6/igt@runner@aborted.html
   [146]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/shard-apl8/igt@runner@aborted.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [IGT#6]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/6
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#118]: https://gitlab.freedesktop.org/drm/intel/issues/118
  [i915#132]: https://gitlab.freedesktop.org/drm/intel/issues/132
  [i915#1804]: https://gitlab.freedesktop.org/drm/intel/issues/1804
  [i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
  [i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
  [i915#2546]: https://gitlab.freedesktop.org/drm/intel/issues/2546
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2705]: https://gitlab.freedesktop.org/drm/intel/issues/2705
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#2920]: https://gitlab.freedesktop.org/drm/intel/issues/2920
  [i915#2994]: https://gitlab.freedesktop.org/drm/intel/issues/2994
  [i915#3002]: https://gitlab.freedesktop.org/drm/intel/issues/3002
  [i915#3318]: https://gitlab.freedesktop.org/drm/intel/issues/3318
  [i915#3467]: https://gitlab.freedesktop.org/drm/intel/issues/3467
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#3989]: https://gitlab.freedesktop.org/drm/intel/issues/3989
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4525]: https://gitlab.freedesktop.org/drm/intel/issues/4525
  [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
  [i915#4573]: https://gitlab.freedesktop.org/drm/intel/issues/4573
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4767]: https://gitlab.freedesktop.org/drm/intel/issues/4767
  [i915#4839]: https://gitlab.freedesktop.org/drm/intel/issues/4839
  [i915#4991]: https://gitlab.freedesktop.org/drm/intel/issues/4991
  [i915#5032]: https://gitlab.freedesktop.org/drm/intel/issues/5032
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#5519]: https://gitlab.freedesktop.org/drm/intel/issues/5519
  [i915#5566]: https://gitlab.freedesktop.org/drm/intel/issues/5566
  [i915#5591]: https://gitlab.freedesktop.org/drm/intel/issues/5591
  [i915#5852]: https://gitlab.freedesktop.org/drm/intel/issues/5852
  [i915#588]: https://gitlab.freedesktop.org/drm/intel/issues/588
  [i915#5939]: https://gitlab.freedesktop.org/drm/intel/issues/5939
  [i915#6117]: https://gitlab.freedesktop.org/drm/intel/issues/6117
  [i915#6227]: https://gitlab.freedesktop.org/drm/intel/issues/6227
  [i915#6453]: https://gitlab.freedesktop.org/drm/intel/issues/6453
  [i915#6497]: https://gitlab.freedesktop.org/drm/intel/issues/6497
  [i915#6537]: https://gitlab.freedesktop.org/drm/intel/issues/6537
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#716]: https://gitlab.freedesktop.org/drm/intel/issues/716
  [i915#7318]: https://gitlab.freedesktop.org/drm/intel/issues/7318
  [i915#7557]: https://gitlab.freedesktop.org/drm/intel/issues/7557
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79


Build changes
-------------

  * Linux: CI_DRM_12390 -> Patchwork_109429v4

  CI-20190529: 20190529
  CI_DRM_12390: b7288a4715c68710aadbd63112b699356e8a2b65 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7062: 6539ea5fe17fce683133c45f07fac316593ee1f7 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_109429v4: b7288a4715c68710aadbd63112b699356e8a2b65 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109429v4/index.html

[-- Attachment #2: Type: text/html, Size: 37540 bytes --]

Re: [Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Rodrigo Vivi]

On Wed, Nov 16, 2022 at 04:30:13PM -0800, Alan Previn wrote:
> In preparation for future MTL-PXP feature support, PXP control
> context should only valid on the correct gt tile. Depending on the
> device-info this depends on which tile owns the VEBOX and KCR.
> PXP is still a global feature though (despite its control-context
> located in the owning GT structure). Additionally, we find
> that the HAS_PXP macro is only used within the pxp module,
> 
> That said, lets drop that HAS_PXP macro altogether and replace it
> with a more fitting named intel_gtpxp_is_supported and helpers
> so that PXP init/fini can use to verify if the referenced gt supports
> PXP or teelink.

Yep, I understand you as I'm not fan of these macros, specially
single usage. But we need to consider that we have multiple dependencies
there and other cases like this in the driver... Well, but I'm not
opposing, but probably better to first get rid of the macro,
then change the behavior of the function on the next patch.

> 
> Add TODO for Meteorlake that will come in future series.

This refactor patch should be standalone, without poputing it with
the changes that didn't came yet to this point.

> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h              |  4 ----
>  drivers/gpu/drm/i915/pxp/intel_pxp.c         | 22 ++++++++++++++------
>  drivers/gpu/drm/i915/pxp/intel_pxp.h         |  3 +++
>  drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c |  2 +-
>  4 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7e3820d2c404..0616e5f0bd31 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -933,10 +933,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)
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index 5efe61f67546..d993e752bd36 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -44,6 +44,20 @@ struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
>  	return container_of(pxp, struct intel_gt, pxp);
>  }
>  
> +static bool _gt_needs_teelink(struct intel_gt *gt)
> +{
> +	/* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */
> +	return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && intel_huc_is_loaded_by_gsc(&gt->uc.huc) &&
> +		intel_uc_uses_huc(&gt->uc));
> +}
> +
> +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp)

If we are asking if it is supported on gt, then the argument must be a gt struct.

> +{
> +	/* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */
> +	return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && IS_ENABLED(CONFIG_DRM_I915_PXP) &&
> +		INTEL_INFO((pxp_to_gt(pxp))->i915)->has_pxp && VDBOX_MASK(pxp_to_gt(pxp)));
> +}
> +
>  bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
>  {
>  	return pxp->ce;
> @@ -142,17 +156,13 @@ void intel_pxp_init(struct intel_pxp *pxp)
>  {
>  	struct intel_gt *gt = pxp_to_gt(pxp);
>  
> -	/* we rely on the mei PXP module */
> -	if (!IS_ENABLED(CONFIG_INTEL_MEI_PXP))
> -		return;

I took a time to understand this movement based on the commit description.
I have the feeling that this patch deserves further split in different patches.

But also, looking a few lines above pxp_to_gt(pxp), I believe we
have further refactor to do sooner. is is one pxp per gt, then we
need to only enable in the gt0?

> -
>  	/*
>  	 * 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))
> +	if (intel_pxp_supported_on_gt(pxp))
>  		pxp_init_full(pxp);
> -	else if (intel_huc_is_loaded_by_gsc(&gt->uc.huc) && intel_uc_uses_huc(&gt->uc))
> +	else if (_gt_needs_teelink(gt))
>  		intel_pxp_tee_component_init(pxp);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> index 2da309088c6d..efa83f9d5e24 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -13,6 +13,9 @@ struct intel_pxp;
>  struct drm_i915_gem_object;
>  
>  struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp);
> +
> +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp);
> +
>  bool intel_pxp_is_enabled(const struct intel_pxp *pxp);
>  bool intel_pxp_is_active(const struct intel_pxp *pxp);
>  
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> index 4359e8be4101..f0ad6f34624a 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> @@ -70,7 +70,7 @@ void intel_pxp_debugfs_register(struct intel_pxp *pxp, struct dentry *gt_root)
>  	if (!gt_root)
>  		return;
>  
> -	if (!HAS_PXP((pxp_to_gt(pxp)->i915)))
> +	if (!intel_pxp_supported_on_gt(pxp))
>  		return;
>  
>  	root = debugfs_create_dir("pxp", gt_root);
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [PATCH v4 2/6] drm/i915/pxp: Make intel_pxp_is_enabled implicitly sort PXP-owning-GT

[Rodrigo Vivi]

On Wed, Nov 16, 2022 at 04:30:14PM -0800, 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 7f2831efc798..c123f4847b19 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)) {

if we are asking about pxp we should pass pxp, not 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 d993e752bd36..88105101af79 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"
>  
>  /**
> @@ -58,11 +59,34 @@ bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp)
>  		INTEL_INFO((pxp_to_gt(pxp))->i915)->has_pxp && VDBOX_MASK(pxp_to_gt(pxp)));
>  }
>  
> -bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
> +bool intel_pxp_is_enabled_on_gt(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 (intel_pxp_supported_on_gt(&gt->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_pxp_is_enabled_on_gt(&gt->pxp);
> +}
> +
>  bool intel_pxp_is_active(const struct intel_pxp *pxp)
>  {
>  	return pxp->arb_is_valid;
> @@ -216,7 +240,7 @@ int intel_pxp_start(struct intel_pxp *pxp)
>  {
>  	int ret = 0;
>  
> -	if (!intel_pxp_is_enabled(pxp))
> +	if (!intel_pxp_is_enabled_on_gt(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 efa83f9d5e24..3f71b1653f74 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -11,12 +11,14 @@
>  
>  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_pxp_supported_on_gt(const struct intel_pxp *pxp);
>  
> -bool intel_pxp_is_enabled(const struct intel_pxp *pxp);
> +bool intel_pxp_is_enabled_on_gt(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..f322a49ebadc 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_pxp_is_enabled_on_gt(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 f0ad6f34624a..4d257055434b 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_pxp_is_enabled_on_gt(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..d3c697bf9aab 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_pxp_is_enabled_on_gt(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..19ac8828cbde 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_pxp_is_enabled_on_gt(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_pxp_is_enabled_on_gt(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_pxp_is_enabled_on_gt(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_pxp_is_enabled_on_gt(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 b0c9170b1395..a5c9c692c20d 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_pxp_is_enabled_on_gt(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_pxp_is_enabled_on_gt(pxp))
>  		with_intel_runtime_pm_if_in_use(&i915->runtime_pm, wakeref)
>  			intel_pxp_fini_hw(pxp);
>  
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [PATCH v4 3/6] drm/i915/pxp: Make intel_pxp_is_active implicitly sort PXP-owning-GT

[Rodrigo Vivi]

On Wed, Nov 16, 2022 at 04:30:15PM -0800, Alan Previn wrote:
> Make intel_pxp_is_active a global check and implicitly find
> the PXP-owning-GT.
> 
> As per prior two patches, callers of this function shall now
> pass in i915 since PXP is a global GPU feature. Make
> intel_pxp_is_active implicitly find the right gt so it's transparent
> for global view callers (like display or gem-exec).
> 
> However we also need to expose the per-gt variation of this for internal
> pxp files to use (like what intel_pxp_is_active was prior) so also expose
> a new intel_gtpxp_is_active 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/pxp/intel_pxp.c         | 14 ++++++++++++--
>  drivers/gpu/drm/i915/pxp/intel_pxp.h         |  3 ++-
>  drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c |  4 ++--
>  drivers/gpu/drm/i915/pxp/intel_pxp_irq.c     |  2 +-
>  5 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index c123f4847b19..165be45a3c13 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -271,7 +271,7 @@ 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))
> +		if (!intel_pxp_is_active(i915))
>  			ret = intel_pxp_start(&to_gt(i915)->pxp);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index 88105101af79..76a924587543 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -87,11 +87,21 @@ bool intel_pxp_is_enabled(struct drm_i915_private *i915)
>  	return intel_pxp_is_enabled_on_gt(&gt->pxp);
>  }
>  
> -bool intel_pxp_is_active(const struct intel_pxp *pxp)
> +bool intel_pxp_is_active_on_gt(const struct intel_pxp *pxp)

if we are asking about the gt we should pass gt

>  {
>  	return pxp->arb_is_valid;
>  }
>  
> +bool intel_pxp_is_active(struct drm_i915_private *i915)
> +{
> +	struct intel_gt *gt = i915_to_pxp_gt(i915);
> +
> +	if (!gt)
> +		return false;
> +
> +	return intel_pxp_is_active_on_gt(&gt->pxp);

> +}
> +
>  /* KCR register definitions */
>  #define KCR_INIT _MMIO(0x320f0)
>  /* Setting KCR Init bit is required after system boot */
> @@ -287,7 +297,7 @@ int intel_pxp_key_check(struct intel_pxp *pxp,
>  			struct drm_i915_gem_object *obj,
>  			bool assign)
>  {
> -	if (!intel_pxp_is_active(pxp))
> +	if (!intel_pxp_is_active_on_gt(pxp))
>  		return -ENODEV;
>  
>  	if (!i915_gem_object_is_protected(obj))
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> index 3f71b1653f74..fe981eebf0ec 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -19,7 +19,8 @@ bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp);
>  
>  bool intel_pxp_is_enabled_on_gt(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);
> +bool intel_pxp_is_active_on_gt(const struct intel_pxp *pxp);
> +bool intel_pxp_is_active(struct drm_i915_private *i915);
>  
>  void intel_pxp_init(struct intel_pxp *pxp);
>  void intel_pxp_fini(struct intel_pxp *pxp);
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> index 4d257055434b..52a808fd4704 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> @@ -25,7 +25,7 @@ static int pxp_info_show(struct seq_file *m, void *data)
>  		return 0;
>  	}
>  
> -	drm_printf(&p, "active: %s\n", str_yes_no(intel_pxp_is_active(pxp)));
> +	drm_printf(&p, "active: %s\n", str_yes_no(intel_pxp_is_active_on_gt(pxp)));
>  	drm_printf(&p, "instance counter: %u\n", pxp->key_instance);
>  
>  	return 0;
> @@ -43,7 +43,7 @@ static int pxp_terminate_set(void *data, u64 val)
>  	struct intel_pxp *pxp = data;
>  	struct intel_gt *gt = pxp_to_gt(pxp);
>  
> -	if (!intel_pxp_is_active(pxp))
> +	if (!intel_pxp_is_active_on_gt(pxp))
>  		return -ENODEV;
>  
>  	/* simulate a termination interrupt */
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> index d3c697bf9aab..c25c1979cccc 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> @@ -86,7 +86,7 @@ void intel_pxp_irq_disable(struct intel_pxp *pxp)
>  	 * called in a path were the driver consider the session as valid and
>  	 * doesn't call a termination on restart.
>  	 */
> -	GEM_WARN_ON(intel_pxp_is_active(pxp));
> +	GEM_WARN_ON(intel_pxp_is_active_on_gt(pxp));
>  
>  	spin_lock_irq(gt->irq_lock);
>  
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [PATCH v4 4/6] drm/i915/pxp: Make PXP tee component bind/unbind aware of PXP-owning-GT

[Rodrigo Vivi]

On Wed, Nov 16, 2022 at 04:30:16PM -0800, Alan Previn wrote:
> Ensure i915_pxp_tee_component_bind / unbind implicitly sorts out
> getting the correct per-GT PXP control-context from the PXP-owning-GT
> when establishing or ending connection. Thus, replace _i915_to_pxp_gt
> with intel_pxp_get_owning_gt (also takes in i915).
> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/pxp/intel_pxp.c     |  6 +++---
>  drivers/gpu/drm/i915/pxp/intel_pxp.h     |  2 ++
>  drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 14 ++++++++++++--
>  3 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index 76a924587543..6a78b6ef0235 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -64,7 +64,7 @@ bool intel_pxp_is_enabled_on_gt(const struct intel_pxp *pxp)
>  	return pxp->ce;
>  }
>  
> -static struct intel_gt *i915_to_pxp_gt(struct drm_i915_private *i915)
> +struct intel_gt *intel_pxp_get_owning_gt(struct drm_i915_private *i915)
>  {
>  	struct intel_gt *gt = NULL;
>  	int i = 0;
> @@ -79,7 +79,7 @@ static struct intel_gt *i915_to_pxp_gt(struct drm_i915_private *i915)
>  
>  bool intel_pxp_is_enabled(struct drm_i915_private *i915)
>  {
> -	struct intel_gt *gt = i915_to_pxp_gt(i915);
> +	struct intel_gt *gt = intel_pxp_get_owning_gt(i915);
>  
>  	if (!gt)
>  		return false;
> @@ -94,7 +94,7 @@ bool intel_pxp_is_active_on_gt(const struct intel_pxp *pxp)
>  
>  bool intel_pxp_is_active(struct drm_i915_private *i915)
>  {
> -	struct intel_gt *gt = i915_to_pxp_gt(i915);
> +	struct intel_gt *gt = intel_pxp_get_owning_gt(i915);
>  
>  	if (!gt)
>  		return false;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> index fe981eebf0ec..c798c3bde957 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -13,6 +13,8 @@ struct intel_pxp;
>  struct drm_i915_gem_object;
>  struct drm_i915_private;
>  
> +struct intel_gt *intel_pxp_get_owning_gt(struct drm_i915_private *i915);
> +
>  struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp);
>  
>  bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp);
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> index a5c9c692c20d..b9198e961cb6 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> @@ -20,8 +20,12 @@
>  static inline struct intel_pxp *i915_dev_to_pxp(struct device *i915_kdev)
>  {
>  	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
> +	struct intel_gt *gt = intel_pxp_get_owning_gt(i915);
>  
> -	return &to_gt(i915)->pxp;
> +	if (!gt)
> +		return NULL;
> +
> +	return &gt->pxp;

or pxp is part of gt, what it looks like, then we use per gt and avoid on the others...

>  }
>  
>  static int intel_pxp_tee_io_message(struct intel_pxp *pxp,
> @@ -128,10 +132,16 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
>  {
>  	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
>  	struct intel_pxp *pxp = i915_dev_to_pxp(i915_kdev);
> -	struct intel_uc *uc = &pxp_to_gt(pxp)->uc;
> +	struct intel_uc *uc;
>  	intel_wakeref_t wakeref;
>  	int ret = 0;
>  
> +	if (!pxp) {
> +		drm_warn(&i915->drm, "tee comp binding without a PXP-owner GT\n");

or we have a single pxp component under i915 and we associate with the gt0 only
and save the gt inside the pxp...

but this whole owning thing seems so convoluted...

> +		return -ENODEV;
> +	}
> +	uc = &pxp_to_gt(pxp)->uc;
> +
>  	mutex_lock(&pxp->tee_mutex);
>  	pxp->pxp_component = data;
>  	pxp->pxp_component->tee_dev = tee_kdev;
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Teres Alexis, Alan Previn]

On Thu, 2022-11-17 at 11:02 -0500, Vivi, Rodrigo wrote:
> On Wed, Nov 16, 2022 at 04:30:13PM -0800, Alan Previn wrote:
> > In preparation for future MTL-PXP feature support, PXP control
> > context should only valid on the correct gt tile. Depending on the
> > device-info this depends on which tile owns the VEBOX and KCR.
> > PXP is still a global feature though (despite its control-context
> > located in the owning GT structure). Additionally, we find
> > that the HAS_PXP macro is only used within the pxp module,
> > 
> > That said, lets drop that HAS_PXP macro altogether and replace it
> > with a more fitting named intel_gtpxp_is_supported and helpers
> > so that PXP init/fini can use to verify if the referenced gt supports
> > PXP or teelink.
> 
> Yep, I understand you as I'm not fan of these macros, specially
> single usage. But we need to consider that we have multiple dependencies
> there and other cases like this in the driver... Well, but I'm not
> opposing, but probably better to first get rid of the macro,
> then change the behavior of the function on the next patch.
> 
> > 
> > Add TODO for Meteorlake that will come in future series.
> 
> This refactor patch should be standalone, without poputing it with
> the changes that didn't came yet to this point.
> 
Sure i can follow this rule, but it would then raise the question of "nothign is really changing anywhere for MTL, why
are we doing this" thats why i wanted to add that placeholder. I see "TODO"s are a common thing in the driver for larger
features that cant all be enabled at once. Respectfully and humbly, is there some documented rule? Can you show it to
me?

> > 
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h              |  4 ----
> >  drivers/gpu/drm/i915/pxp/intel_pxp.c         | 22 ++++++++++++++------
> >  drivers/gpu/drm/i915/pxp/intel_pxp.h         |  3 +++
> >  drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c |  2 +-
> >  4 files changed, 20 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 7e3820d2c404..0616e5f0bd31 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -933,10 +933,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)
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > index 5efe61f67546..d993e752bd36 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > @@ -44,6 +44,20 @@ struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
> >  	return container_of(pxp, struct intel_gt, pxp);
> >  }
> >  
> > +static bool _gt_needs_teelink(struct intel_gt *gt)
> > +{
> > +	/* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */
> > +	return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && intel_huc_is_loaded_by_gsc(&gt->uc.huc) &&
> > +		intel_uc_uses_huc(&gt->uc));
> > +}
> > +
> > +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp)
> 
> If we are asking if it is supported on gt, then the argument must be a gt struct.
> 
I agree with you but Daniele said above is more consistent with existing ways that is considered the standard.
Respectfully and humbly I would like to request both yourself and Daniele to show me the coding guidelines somewhere.

Honestly, this is one of the first few hunks of the first patch of the first series in a very large complex design to
enable PXP on MTL and it only a helper utility function. Respecfully and humbly, I rather we focus our energy for review
+ redo  on more critical things like the e2e usage and top-to-bottom design or coding logic flows or find actual bugs
instead of debating about coding styles for internal only helper functions.


> > +{
> > +	/* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */
> > +	return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && IS_ENABLED(CONFIG_DRM_I915_PXP) &&
> > +		INTEL_INFO((pxp_to_gt(pxp))->i915)->has_pxp && VDBOX_MASK(pxp_to_gt(pxp)));
> > +}
> > +
> >  bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
> >  {
> >  	return pxp->ce;
> > @@ -142,17 +156,13 @@ void intel_pxp_init(struct intel_pxp *pxp)
> >  {
> >  	struct intel_gt *gt = pxp_to_gt(pxp);
> >  
> > -	/* we rely on the mei PXP module */
> > -	if (!IS_ENABLED(CONFIG_INTEL_MEI_PXP))
> > -		return;
> 
> I took a time to understand this movement based on the commit description.
> I have the feeling that this patch deserves further split in different patches.
> 
> But also, looking a few lines above pxp_to_gt(pxp), I believe we
> have further refactor to do sooner. is is one pxp per gt, then we
> need to only enable in the gt0?
> 
In our driver, PXP as reflected by the device info is being treated as a global feature. 
PXP as a HW subsystem is "usable" device-wide from any workload on any engine on any tile (due to the internal mirror
component and additional plumbing across the tiles). So in line with that I rather not have the gem-exec-buf, gem-create
and gem-context calls be bothered about which GT to access to query of this global hw feature is enabled or active.
However the control point for allocating sessions, talking to the gsc firmware and doing global teardowns are only meant
to occur on and via the tile that owns the KCR engine which the media tile. This includes things like per-tile uncore
power gating controls of the GSC-CS. (although some aspects like IRQ for KCR global). So as u see its not a clean per-GT
feature.

I did speak to Daniele many months back when enabling the full feature set (on internal POC work) about whether we
should make PXP a global subsystem instead of hanging off gt but we both agreed that because the control engines are
only located on one tile, so you might face some its gonna be a trade off one way or the other:
     - pxp a global structure, then all of the init / shutdown / suspend-resume flows would then have a different set of
convoluted functions that try to get access to gt specific controls from a top level function flow.


Additionally, humbly and respectfully, perhaps you can read through the internal arch HW specs through which it can be
infered that PXP will continue to have a single entity for control events despite the feature being usable / accessible
across all tiles.

> > -
> >  	/*
> >  	 * 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))
> > +	if (intel_pxp_supported_on_gt(pxp))
> >  		pxp_init_full(pxp);
> > -	else if (intel_huc_is_loaded_by_gsc(&gt->uc.huc) && intel_uc_uses_huc(&gt->uc))
> > +	else if (_gt_needs_teelink(gt))
> >  		intel_pxp_tee_component_init(pxp);
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > index 2da309088c6d..efa83f9d5e24 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > @@ -13,6 +13,9 @@ struct intel_pxp;
> >  struct drm_i915_gem_object;
> >  
> >  struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp);
> > +
> > +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp);
> > +
> >  bool intel_pxp_is_enabled(const struct intel_pxp *pxp);
> >  bool intel_pxp_is_active(const struct intel_pxp *pxp);
> >  
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > index 4359e8be4101..f0ad6f34624a 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > @@ -70,7 +70,7 @@ void intel_pxp_debugfs_register(struct intel_pxp *pxp, struct dentry *gt_root)
> >  	if (!gt_root)
> >  		return;
> >  
> > -	if (!HAS_PXP((pxp_to_gt(pxp)->i915)))
> > +	if (!intel_pxp_supported_on_gt(pxp))
> >  		return;
> >  
> >  	root = debugfs_create_dir("pxp", gt_root);
> > -- 
> > 2.34.1
> > 

Re: [Intel-gfx] [PATCH v4 2/6] drm/i915/pxp: Make intel_pxp_is_enabled implicitly sort PXP-owning-GT

[Teres Alexis, Alan Previn]

On Thu, 2022-11-17 at 11:04 -0500, Vivi, Rodrigo wrote:
> On Wed, Nov 16, 2022 at 04:30:14PM -0800, 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 7f2831efc798..c123f4847b19 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)) {
> 
> if we are asking about pxp we should pass pxp, not i915...
> 
> 
The function is being called by gem-exec / gem-context / gem-create about the availibility of this feature globally.
I had previously discussed this with Daniele with the goal to have 2 versions (one a wrapper over the other) where u can
query "is the pxp feature available on this hw?" vs "does this gt have the enabled pxp controls"? where the latter is
more for internal PXP usage while the former is for external (gem-exec, gem-context, etc). So the naming above was
decided by Daniele. Or perhaps this might work better?

Another direction is to have the external callers not change at all (so gem-exec would continue call with either the
render-gt-pxp or the media-gt-pxp and have the internal subsystem sort out which is the correct subsystem. Internally in
our display code, when we have shared functions like clocks, buffers and such where i've seen code that takes in the
caller's crtc the top level and then internally parse across all crtcs to take the proper global actions (where
sometimes the control unit might reside on only 1 crtc). Actually, this was where rev1 was originally heading but
Daniele said that was convoluted (the internal rerouting from callers gt-pxp to the correct gt-pxp).

Respectfully and humbly, i would like to request where is the coding guideline for function naming when u have 2nd level
subsystem IPs owning control over global hw features so that we dont need to have this back and forth of conflicting
direction from different reviewers especially so long after initial reviews have started. (internally reworking future
MTL PXP series end up getting impacted here).

...alan

Re: [Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Tvrtko Ursulin]

On 17/11/2022 22:34, Teres Alexis, Alan Previn wrote:
> On Thu, 2022-11-17 at 11:02 -0500, Vivi, Rodrigo wrote:
>> On Wed, Nov 16, 2022 at 04:30:13PM -0800, Alan Previn wrote:
>>> In preparation for future MTL-PXP feature support, PXP control
>>> context should only valid on the correct gt tile. Depending on the
>>> device-info this depends on which tile owns the VEBOX and KCR.
>>> PXP is still a global feature though (despite its control-context
>>> located in the owning GT structure). Additionally, we find
>>> that the HAS_PXP macro is only used within the pxp module,
>>>
>>> That said, lets drop that HAS_PXP macro altogether and replace it
>>> with a more fitting named intel_gtpxp_is_supported and helpers
>>> so that PXP init/fini can use to verify if the referenced gt supports
>>> PXP or teelink.
>>
>> Yep, I understand you as I'm not fan of these macros, specially
>> single usage. But we need to consider that we have multiple dependencies
>> there and other cases like this in the driver... Well, but I'm not
>> opposing, but probably better to first get rid of the macro,
>> then change the behavior of the function on the next patch.
>>
>>>
>>> Add TODO for Meteorlake that will come in future series.
>>
>> This refactor patch should be standalone, without poputing it with
>> the changes that didn't came yet to this point.
>>
> Sure i can follow this rule, but it would then raise the question of "nothign is really changing anywhere for MTL, why
> are we doing this" thats why i wanted to add that placeholder. I see "TODO"s are a common thing in the driver for larger
> features that cant all be enabled at once. Respectfully and humbly, is there some documented rule? Can you show it to
> me?

Separating refactoring from functional changes is one of the most basic 
principles we follow (and not just us) and there should never be 
pushback on the former due lack of functional changes.

On the basic level following this guideline makes it trivial to review 
the refactoring patch, and in the vast majority of cases much easier to 
review the functional change patch as well.

And easy to review means happy reviewers, faster turnaround time (easier 
to carry a rebase), happier authors, easier reverts when things go bad 
(only small functional patch needs to be reverted), sometimes even 
easier backporting if code diverged a lot.

Oh, and it even has potential to decrease internal technical debt. Often 
you can push refactoring upstream and keep a smaller delta behind the 
closed doors, when that is required.

>>> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h              |  4 ----
>>>   drivers/gpu/drm/i915/pxp/intel_pxp.c         | 22 ++++++++++++++------
>>>   drivers/gpu/drm/i915/pxp/intel_pxp.h         |  3 +++
>>>   drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c |  2 +-
>>>   4 files changed, 20 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 7e3820d2c404..0616e5f0bd31 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -933,10 +933,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)
>>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>>> index 5efe61f67546..d993e752bd36 100644
>>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
>>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>>> @@ -44,6 +44,20 @@ struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
>>>   	return container_of(pxp, struct intel_gt, pxp);
>>>   }
>>>   
>>> +static bool _gt_needs_teelink(struct intel_gt *gt)
>>> +{
>>> +	/* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */
>>> +	return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && intel_huc_is_loaded_by_gsc(&gt->uc.huc) &&
>>> +		intel_uc_uses_huc(&gt->uc));
>>> +}
>>> +
>>> +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp)
>>
>> If we are asking if it is supported on gt, then the argument must be a gt struct.
>>
> I agree with you but Daniele said above is more consistent with existing ways that is considered the standard.
> Respectfully and humbly I would like to request both yourself and Daniele to show me the coding guidelines somewhere.
> 
> Honestly, this is one of the first few hunks of the first patch of the first series in a very large complex design to
> enable PXP on MTL and it only a helper utility function. Respecfully and humbly, I rather we focus our energy for review
> + redo  on more critical things like the e2e usage and top-to-bottom design or coding logic flows or find actual bugs
> instead of debating about coding styles for internal only helper functions.

My 2c/p is that the intel_pxp_supported_on_gt the "on_gt" part reads a 
bit clumsy because it makes it sound like the two are independent 
objects. Like I could pass one pxp to different GTs and ask if that one 
works here, or maybe there.

I am though a fan of intel_pxp_ prefix meaning function operates on 
struct intel_pxp.

In this case I don't know what is the correct relationship. If it is 1:1 
between intel_pxp:intel_gt then intel_pxp_supported(pxp) sounds fine. 
Even if a singleton that works for me. If we do have 1:1 but only want 
to init the first one, but PXP truly lives in the GT block, we could 
have pointers per GT, with only the gt0 one being initialized, and 
perhaps a shadow i915->pxp pointer which points to gt[0]->pxp, if that 
makes for better code organisation?

Regards,

Tvrtko

> 
> 
>>> +{
>>> +	/* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */
>>> +	return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && IS_ENABLED(CONFIG_DRM_I915_PXP) &&
>>> +		INTEL_INFO((pxp_to_gt(pxp))->i915)->has_pxp && VDBOX_MASK(pxp_to_gt(pxp)));
>>> +}
>>> +
>>>   bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
>>>   {
>>>   	return pxp->ce;
>>> @@ -142,17 +156,13 @@ void intel_pxp_init(struct intel_pxp *pxp)
>>>   {
>>>   	struct intel_gt *gt = pxp_to_gt(pxp);
>>>   
>>> -	/* we rely on the mei PXP module */
>>> -	if (!IS_ENABLED(CONFIG_INTEL_MEI_PXP))
>>> -		return;
>>
>> I took a time to understand this movement based on the commit description.
>> I have the feeling that this patch deserves further split in different patches.
>>
>> But also, looking a few lines above pxp_to_gt(pxp), I believe we
>> have further refactor to do sooner. is is one pxp per gt, then we
>> need to only enable in the gt0?
>>
> In our driver, PXP as reflected by the device info is being treated as a global feature.
> PXP as a HW subsystem is "usable" device-wide from any workload on any engine on any tile (due to the internal mirror
> component and additional plumbing across the tiles). So in line with that I rather not have the gem-exec-buf, gem-create
> and gem-context calls be bothered about which GT to access to query of this global hw feature is enabled or active.
> However the control point for allocating sessions, talking to the gsc firmware and doing global teardowns are only meant
> to occur on and via the tile that owns the KCR engine which the media tile. This includes things like per-tile uncore
> power gating controls of the GSC-CS. (although some aspects like IRQ for KCR global). So as u see its not a clean per-GT
> feature.
> 
> I did speak to Daniele many months back when enabling the full feature set (on internal POC work) about whether we
> should make PXP a global subsystem instead of hanging off gt but we both agreed that because the control engines are
> only located on one tile, so you might face some its gonna be a trade off one way or the other:
>       - pxp a global structure, then all of the init / shutdown / suspend-resume flows would then have a different set of
> convoluted functions that try to get access to gt specific controls from a top level function flow.
> 
> 
> Additionally, humbly and respectfully, perhaps you can read through the internal arch HW specs through which it can be
> infered that PXP will continue to have a single entity for control events despite the feature being usable / accessible
> across all tiles.
> 
>>> -
>>>   	/*
>>>   	 * 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))
>>> +	if (intel_pxp_supported_on_gt(pxp))
>>>   		pxp_init_full(pxp);
>>> -	else if (intel_huc_is_loaded_by_gsc(&gt->uc.huc) && intel_uc_uses_huc(&gt->uc))
>>> +	else if (_gt_needs_teelink(gt))
>>>   		intel_pxp_tee_component_init(pxp);
>>>   }
>>>   
>>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
>>> index 2da309088c6d..efa83f9d5e24 100644
>>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
>>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
>>> @@ -13,6 +13,9 @@ struct intel_pxp;
>>>   struct drm_i915_gem_object;
>>>   
>>>   struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp);
>>> +
>>> +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp);
>>> +
>>>   bool intel_pxp_is_enabled(const struct intel_pxp *pxp);
>>>   bool intel_pxp_is_active(const struct intel_pxp *pxp);
>>>   
>>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
>>> index 4359e8be4101..f0ad6f34624a 100644
>>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
>>> @@ -70,7 +70,7 @@ void intel_pxp_debugfs_register(struct intel_pxp *pxp, struct dentry *gt_root)
>>>   	if (!gt_root)
>>>   		return;
>>>   
>>> -	if (!HAS_PXP((pxp_to_gt(pxp)->i915)))
>>> +	if (!intel_pxp_supported_on_gt(pxp))
>>>   		return;
>>>   
>>>   	root = debugfs_create_dir("pxp", gt_root);
>>> -- 
>>> 2.34.1
>>>
> 

Re: [Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Jani Nikula]

On Mon, 21 Nov 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 17/11/2022 22:34, Teres Alexis, Alan Previn wrote:
>> On Thu, 2022-11-17 at 11:02 -0500, Vivi, Rodrigo wrote:
>>> On Wed, Nov 16, 2022 at 04:30:13PM -0800, Alan Previn wrote:
>>>> In preparation for future MTL-PXP feature support, PXP control
>>>> context should only valid on the correct gt tile. Depending on the
>>>> device-info this depends on which tile owns the VEBOX and KCR.
>>>> PXP is still a global feature though (despite its control-context
>>>> located in the owning GT structure). Additionally, we find
>>>> that the HAS_PXP macro is only used within the pxp module,
>>>>
>>>> That said, lets drop that HAS_PXP macro altogether and replace it
>>>> with a more fitting named intel_gtpxp_is_supported and helpers
>>>> so that PXP init/fini can use to verify if the referenced gt supports
>>>> PXP or teelink.
>>>
>>> Yep, I understand you as I'm not fan of these macros, specially
>>> single usage. But we need to consider that we have multiple dependencies
>>> there and other cases like this in the driver... Well, but I'm not
>>> opposing, but probably better to first get rid of the macro,
>>> then change the behavior of the function on the next patch.
>>>
>>>>
>>>> Add TODO for Meteorlake that will come in future series.
>>>
>>> This refactor patch should be standalone, without poputing it with
>>> the changes that didn't came yet to this point.
>>>
>> Sure i can follow this rule, but it would then raise the question of "nothign is really changing anywhere for MTL, why
>> are we doing this" thats why i wanted to add that placeholder. I see "TODO"s are a common thing in the driver for larger
>> features that cant all be enabled at once. Respectfully and humbly, is there some documented rule? Can you show it to
>> me?
>
> Separating refactoring from functional changes is one of the most basic 
> principles we follow (and not just us) and there should never be 
> pushback on the former due lack of functional changes.

It's also documented [1][2] but that never seems to make a difference.

BR,
Jani.


[1] https://docs.kernel.org/process/submitting-patches.html#separate-your-changes
[2] https://docs.kernel.org/process/5.Posting.html#patch-preparation




>
> On the basic level following this guideline makes it trivial to review 
> the refactoring patch, and in the vast majority of cases much easier to 
> review the functional change patch as well.
>
> And easy to review means happy reviewers, faster turnaround time (easier 
> to carry a rebase), happier authors, easier reverts when things go bad 
> (only small functional patch needs to be reverted), sometimes even 
> easier backporting if code diverged a lot.
>
> Oh, and it even has potential to decrease internal technical debt. Often 
> you can push refactoring upstream and keep a smaller delta behind the 
> closed doors, when that is required.
>
>>>> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_drv.h              |  4 ----
>>>>   drivers/gpu/drm/i915/pxp/intel_pxp.c         | 22 ++++++++++++++------
>>>>   drivers/gpu/drm/i915/pxp/intel_pxp.h         |  3 +++
>>>>   drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c |  2 +-
>>>>   4 files changed, 20 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 7e3820d2c404..0616e5f0bd31 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -933,10 +933,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)
>>>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>>>> index 5efe61f67546..d993e752bd36 100644
>>>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
>>>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>>>> @@ -44,6 +44,20 @@ struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
>>>>   	return container_of(pxp, struct intel_gt, pxp);
>>>>   }
>>>>   
>>>> +static bool _gt_needs_teelink(struct intel_gt *gt)
>>>> +{
>>>> +	/* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */
>>>> +	return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && intel_huc_is_loaded_by_gsc(&gt->uc.huc) &&
>>>> +		intel_uc_uses_huc(&gt->uc));
>>>> +}
>>>> +
>>>> +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp)
>>>
>>> If we are asking if it is supported on gt, then the argument must be a gt struct.
>>>
>> I agree with you but Daniele said above is more consistent with existing ways that is considered the standard.
>> Respectfully and humbly I would like to request both yourself and Daniele to show me the coding guidelines somewhere.
>> 
>> Honestly, this is one of the first few hunks of the first patch of the first series in a very large complex design to
>> enable PXP on MTL and it only a helper utility function. Respecfully and humbly, I rather we focus our energy for review
>> + redo  on more critical things like the e2e usage and top-to-bottom design or coding logic flows or find actual bugs
>> instead of debating about coding styles for internal only helper functions.
>
> My 2c/p is that the intel_pxp_supported_on_gt the "on_gt" part reads a 
> bit clumsy because it makes it sound like the two are independent 
> objects. Like I could pass one pxp to different GTs and ask if that one 
> works here, or maybe there.
>
> I am though a fan of intel_pxp_ prefix meaning function operates on 
> struct intel_pxp.
>
> In this case I don't know what is the correct relationship. If it is 1:1 
> between intel_pxp:intel_gt then intel_pxp_supported(pxp) sounds fine. 
> Even if a singleton that works for me. If we do have 1:1 but only want 
> to init the first one, but PXP truly lives in the GT block, we could 
> have pointers per GT, with only the gt0 one being initialized, and 
> perhaps a shadow i915->pxp pointer which points to gt[0]->pxp, if that 
> makes for better code organisation?
>
> Regards,
>
> Tvrtko
>
>> 
>> 
>>>> +{
>>>> +	/* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */
>>>> +	return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && IS_ENABLED(CONFIG_DRM_I915_PXP) &&
>>>> +		INTEL_INFO((pxp_to_gt(pxp))->i915)->has_pxp && VDBOX_MASK(pxp_to_gt(pxp)));
>>>> +}
>>>> +
>>>>   bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
>>>>   {
>>>>   	return pxp->ce;
>>>> @@ -142,17 +156,13 @@ void intel_pxp_init(struct intel_pxp *pxp)
>>>>   {
>>>>   	struct intel_gt *gt = pxp_to_gt(pxp);
>>>>   
>>>> -	/* we rely on the mei PXP module */
>>>> -	if (!IS_ENABLED(CONFIG_INTEL_MEI_PXP))
>>>> -		return;
>>>
>>> I took a time to understand this movement based on the commit description.
>>> I have the feeling that this patch deserves further split in different patches.
>>>
>>> But also, looking a few lines above pxp_to_gt(pxp), I believe we
>>> have further refactor to do sooner. is is one pxp per gt, then we
>>> need to only enable in the gt0?
>>>
>> In our driver, PXP as reflected by the device info is being treated as a global feature.
>> PXP as a HW subsystem is "usable" device-wide from any workload on any engine on any tile (due to the internal mirror
>> component and additional plumbing across the tiles). So in line with that I rather not have the gem-exec-buf, gem-create
>> and gem-context calls be bothered about which GT to access to query of this global hw feature is enabled or active.
>> However the control point for allocating sessions, talking to the gsc firmware and doing global teardowns are only meant
>> to occur on and via the tile that owns the KCR engine which the media tile. This includes things like per-tile uncore
>> power gating controls of the GSC-CS. (although some aspects like IRQ for KCR global). So as u see its not a clean per-GT
>> feature.
>> 
>> I did speak to Daniele many months back when enabling the full feature set (on internal POC work) about whether we
>> should make PXP a global subsystem instead of hanging off gt but we both agreed that because the control engines are
>> only located on one tile, so you might face some its gonna be a trade off one way or the other:
>>       - pxp a global structure, then all of the init / shutdown / suspend-resume flows would then have a different set of
>> convoluted functions that try to get access to gt specific controls from a top level function flow.
>> 
>> 
>> Additionally, humbly and respectfully, perhaps you can read through the internal arch HW specs through which it can be
>> infered that PXP will continue to have a single entity for control events despite the feature being usable / accessible
>> across all tiles.
>> 
>>>> -
>>>>   	/*
>>>>   	 * 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))
>>>> +	if (intel_pxp_supported_on_gt(pxp))
>>>>   		pxp_init_full(pxp);
>>>> -	else if (intel_huc_is_loaded_by_gsc(&gt->uc.huc) && intel_uc_uses_huc(&gt->uc))
>>>> +	else if (_gt_needs_teelink(gt))
>>>>   		intel_pxp_tee_component_init(pxp);
>>>>   }
>>>>   
>>>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
>>>> index 2da309088c6d..efa83f9d5e24 100644
>>>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
>>>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
>>>> @@ -13,6 +13,9 @@ struct intel_pxp;
>>>>   struct drm_i915_gem_object;
>>>>   
>>>>   struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp);
>>>> +
>>>> +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp);
>>>> +
>>>>   bool intel_pxp_is_enabled(const struct intel_pxp *pxp);
>>>>   bool intel_pxp_is_active(const struct intel_pxp *pxp);
>>>>   
>>>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
>>>> index 4359e8be4101..f0ad6f34624a 100644
>>>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
>>>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
>>>> @@ -70,7 +70,7 @@ void intel_pxp_debugfs_register(struct intel_pxp *pxp, struct dentry *gt_root)
>>>>   	if (!gt_root)
>>>>   		return;
>>>>   
>>>> -	if (!HAS_PXP((pxp_to_gt(pxp)->i915)))
>>>> +	if (!intel_pxp_supported_on_gt(pxp))
>>>>   		return;
>>>>   
>>>>   	root = debugfs_create_dir("pxp", gt_root);
>>>> -- 
>>>> 2.34.1
>>>>
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Vivi, Rodrigo]

On Mon, 2022-11-21 at 11:39 +0000, Tvrtko Ursulin wrote:
> 
> On 17/11/2022 22:34, Teres Alexis, Alan Previn wrote:
> > On Thu, 2022-11-17 at 11:02 -0500, Vivi, Rodrigo wrote:
> > > On Wed, Nov 16, 2022 at 04:30:13PM -0800, Alan Previn wrote:
> > > > In preparation for future MTL-PXP feature support, PXP control
> > > > context should only valid on the correct gt tile. Depending on
> > > > the
> > > > device-info this depends on which tile owns the VEBOX and KCR.
> > > > PXP is still a global feature though (despite its control-
> > > > context
> > > > located in the owning GT structure). Additionally, we find
> > > > that the HAS_PXP macro is only used within the pxp module,
> > > > 
> > > > That said, lets drop that HAS_PXP macro altogether and replace
> > > > it
> > > > with a more fitting named intel_gtpxp_is_supported and helpers
> > > > so that PXP init/fini can use to verify if the referenced gt
> > > > supports
> > > > PXP or teelink.
> > > 
> > > Yep, I understand you as I'm not fan of these macros, specially
> > > single usage. But we need to consider that we have multiple
> > > dependencies
> > > there and other cases like this in the driver... Well, but I'm
> > > not
> > > opposing, but probably better to first get rid of the macro,
> > > then change the behavior of the function on the next patch.
> > > 
> > > > 
> > > > Add TODO for Meteorlake that will come in future series.
> > > 
> > > This refactor patch should be standalone, without poputing it
> > > with
> > > the changes that didn't came yet to this point.
> > > 
> > Sure i can follow this rule, but it would then raise the question
> > of "nothign is really changing anywhere for MTL, why
> > are we doing this" thats why i wanted to add that placeholder. I
> > see "TODO"s are a common thing in the driver for larger
> > features that cant all be enabled at once. Respectfully and humbly,
> > is there some documented rule? Can you show it to
> > me?
> 
> Separating refactoring from functional changes is one of the most
> basic 
> principles we follow (and not just us) and there should never be 
> pushback on the former due lack of functional changes.
> 
> On the basic level following this guideline makes it trivial to
> review 
> the refactoring patch, and in the vast majority of cases much easier
> to 
> review the functional change patch as well.
> 
> And easy to review means happy reviewers, faster turnaround time
> (easier 
> to carry a rebase), happier authors, easier reverts when things go
> bad 
> (only small functional patch needs to be reverted), sometimes even 
> easier backporting if code diverged a lot.
> 
> Oh, and it even has potential to decrease internal technical debt.
> Often 
> you can push refactoring upstream and keep a smaller delta behind the
> closed doors, when that is required.
> 
> > > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/i915_drv.h              |  4 ----
> > > >   drivers/gpu/drm/i915/pxp/intel_pxp.c         | 22
> > > > ++++++++++++++------
> > > >   drivers/gpu/drm/i915/pxp/intel_pxp.h         |  3 +++
> > > >   drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c |  2 +-
> > > >   4 files changed, 20 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 7e3820d2c404..0616e5f0bd31 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -933,10 +933,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)
> > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > > b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > > index 5efe61f67546..d993e752bd36 100644
> > > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > > @@ -44,6 +44,20 @@ struct intel_gt *pxp_to_gt(const struct
> > > > intel_pxp *pxp)
> > > >         return container_of(pxp, struct intel_gt, pxp);
> > > >   }
> > > >   
> > > > +static bool _gt_needs_teelink(struct intel_gt *gt)
> > > > +{
> > > > +       /* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on
> > > > GSC engine */
> > > > +       return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) &&
> > > > intel_huc_is_loaded_by_gsc(&gt->uc.huc) &&
> > > > +               intel_uc_uses_huc(&gt->uc));
> > > > +}
> > > > +
> > > > +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp)
> > > 
> > > If we are asking if it is supported on gt, then the argument must
> > > be a gt struct.
> > > 
> > I agree with you but Daniele said above is more consistent with
> > existing ways that is considered the standard.
> > Respectfully and humbly I would like to request both yourself and
> > Daniele to show me the coding guidelines somewhere.
> > 
> > Honestly, this is one of the first few hunks of the first patch of
> > the first series in a very large complex design to
> > enable PXP on MTL and it only a helper utility function.
> > Respecfully and humbly, I rather we focus our energy for review
> > + redo  on more critical things like the e2e usage and top-to-
> > bottom design or coding logic flows or find actual bugs
> > instead of debating about coding styles for internal only helper
> > functions.
> 
> My 2c/p is that the intel_pxp_supported_on_gt the "on_gt" part reads
> a 
> bit clumsy because it makes it sound like the two are independent 
> objects. Like I could pass one pxp to different GTs and ask if that
> one 
> works here, or maybe there.
> 
> I am though a fan of intel_pxp_ prefix meaning function operates on 
> struct intel_pxp.
> 
> In this case I don't know what is the correct relationship. If it is
> 1:1 
> between intel_pxp:intel_gt

Yeap, this is my main point here. It is not clear what is the correct
relationship here.

Or we make the intel_pxp under the drm_i915_private, and then have the
associated gt (always gt0?!) link inside the intel_pxp.

Or we keep it inside intel_gt as is today, but then we run all the
functions gt agnostically and then skip when not enabled (!gt0?).

But all these functions to check "on_gt" makes the code hard to
understand the relation and hard to maintain long term.

The argument that we have more patches in the pipeline to come on top
of this series here just make it stronger the need for a very clean
start.

>  then intel_pxp_supported(pxp) sounds fine.
> Even if a singleton that works for me. If we do have 1:1 but only
> want 
> to init the first one, but PXP truly lives in the GT block, we could 
> have pointers per GT, with only the gt0 one being initialized, and 
> perhaps a shadow i915->pxp pointer which points to gt[0]->pxp, if
> that 
> makes for better code organisation?
> 
> Regards,
> 
> Tvrtko
> 
> > 
> > 
> > > > +{
> > > > +       /* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on
> > > > GSC engine */
> > > > +       return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) &&
> > > > IS_ENABLED(CONFIG_DRM_I915_PXP) &&
> > > > +               INTEL_INFO((pxp_to_gt(pxp))->i915)->has_pxp &&
> > > > VDBOX_MASK(pxp_to_gt(pxp)));
> > > > +}
> > > > +
> > > >   bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
> > > >   {
> > > >         return pxp->ce;
> > > > @@ -142,17 +156,13 @@ void intel_pxp_init(struct intel_pxp
> > > > *pxp)
> > > >   {
> > > >         struct intel_gt *gt = pxp_to_gt(pxp);
> > > >   
> > > > -       /* we rely on the mei PXP module */
> > > > -       if (!IS_ENABLED(CONFIG_INTEL_MEI_PXP))
> > > > -               return;
> > > 
> > > I took a time to understand this movement based on the commit
> > > description.
> > > I have the feeling that this patch deserves further split in
> > > different patches.
> > > 
> > > But also, looking a few lines above pxp_to_gt(pxp), I believe we
> > > have further refactor to do sooner. is is one pxp per gt, then we
> > > need to only enable in the gt0?
> > > 
> > In our driver, PXP as reflected by the device info is being treated
> > as a global feature.
> > PXP as a HW subsystem is "usable" device-wide from any workload on
> > any engine on any tile (due to the internal mirror
> > component and additional plumbing across the tiles). So in line
> > with that I rather not have the gem-exec-buf, gem-create
> > and gem-context calls be bothered about which GT to access to query
> > of this global hw feature is enabled or active.
> > However the control point for allocating sessions, talking to the
> > gsc firmware and doing global teardowns are only meant
> > to occur on and via the tile that owns the KCR engine which the
> > media tile. This includes things like per-tile uncore
> > power gating controls of the GSC-CS. (although some aspects like
> > IRQ for KCR global). So as u see its not a clean per-GT
> > feature.
> > 
> > I did speak to Daniele many months back when enabling the full
> > feature set (on internal POC work) about whether we
> > should make PXP a global subsystem instead of hanging off gt but we
> > both agreed that because the control engines are
> > only located on one tile, so you might face some its gonna be a
> > trade off one way or the other:
> >       - pxp a global structure, then all of the init / shutdown /
> > suspend-resume flows would then have a different set of
> > convoluted functions that try to get access to gt specific controls
> > from a top level function flow.
> > 
> > 
> > Additionally, humbly and respectfully, perhaps you can read through
> > the internal arch HW specs through which it can be
> > infered that PXP will continue to have a single entity for control
> > events despite the feature being usable / accessible
> > across all tiles.
> > 
> > > > -
> > > >         /*
> > > >          * 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))
> > > > +       if (intel_pxp_supported_on_gt(pxp))
> > > >                 pxp_init_full(pxp);
> > > > -       else if (intel_huc_is_loaded_by_gsc(&gt->uc.huc) &&
> > > > intel_uc_uses_huc(&gt->uc))
> > > > +       else if (_gt_needs_teelink(gt))
> > > >                 intel_pxp_tee_component_init(pxp);
> > > >   }
> > > >   
> > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > > > b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > > > index 2da309088c6d..efa83f9d5e24 100644
> > > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > > > @@ -13,6 +13,9 @@ struct intel_pxp;
> > > >   struct drm_i915_gem_object;
> > > >   
> > > >   struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp);
> > > > +
> > > > +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp);
> > > > +
> > > >   bool intel_pxp_is_enabled(const struct intel_pxp *pxp);
> > > >   bool intel_pxp_is_active(const struct intel_pxp *pxp);
> > > >   
> > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > > > b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > > > index 4359e8be4101..f0ad6f34624a 100644
> > > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > > > @@ -70,7 +70,7 @@ void intel_pxp_debugfs_register(struct
> > > > intel_pxp *pxp, struct dentry *gt_root)
> > > >         if (!gt_root)
> > > >                 return;
> > > >   
> > > > -       if (!HAS_PXP((pxp_to_gt(pxp)->i915)))
> > > > +       if (!intel_pxp_supported_on_gt(pxp))
> > > >                 return;
> > > >   
> > > >         root = debugfs_create_dir("pxp", gt_root);
> > > > -- 
> > > > 2.34.1
> > > > 
> > 

Re: [Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Teres Alexis, Alan Previn]

Thank you Jani - this was clearly my mistake (apologies to Daniele/Rodrigo) - didn't realize we had this documented. I
will go through that first.

...alan

On Mon, 2022-11-21 at 14:12 +0200, Jani Nikula wrote:
> On Mon, 21 Nov 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> > On 17/11/2022 22:34, Teres Alexis, Alan Previn wrote:
> > > On Thu, 2022-11-17 at 11:02 -0500, Vivi, Rodrigo wrote:
> > > > On Wed, Nov 16, 2022 at 04:30:13PM -0800, Alan Previn wrote:
> > > > > In preparation for future MTL-PXP feature support, PXP control
> > > > > context should only valid on the correct gt tile. Depending on the
> > > > > device-info this depends on which tile owns the VEBOX and KCR.
> > > > > PXP is still a global feature though (despite its control-context
> > > > > located in the owning GT structure). Additionally, we find
> > > > > that the HAS_PXP macro is only used within the pxp module,
> > > > > 
> > > > > That said, lets drop that HAS_PXP macro altogether and replace it
> > > > > with a more fitting named intel_gtpxp_is_supported and helpers
> > > > > so that PXP init/fini can use to verify if the referenced gt supports
> > > > > PXP or teelink.
> > > > 
> > > > Yep, I understand you as I'm not fan of these macros, specially
> > > > single usage. But we need to consider that we have multiple dependencies
> > > > there and other cases like this in the driver... Well, but I'm not
> > > > opposing, but probably better to first get rid of the macro,
> > > > then change the behavior of the function on the next patch.
> > > > 
> > > > > 
> > > > > Add TODO for Meteorlake that will come in future series.
> > > > 
> > > > This refactor patch should be standalone, without poputing it with
> > > > the changes that didn't came yet to this point.
> > > > 
> > > Sure i can follow this rule, but it would then raise the question of "nothign is really changing anywhere for MTL, why
> > > are we doing this" thats why i wanted to add that placeholder. I see "TODO"s are a common thing in the driver for larger
> > > features that cant all be enabled at once. Respectfully and humbly, is there some documented rule? Can you show it to
> > > me?
> > 
> > Separating refactoring from functional changes is one of the most basic 
> > principles we follow (and not just us) and there should never be 
> > pushback on the former due lack of functional changes.
> 
> It's also documented [1][2] but that never seems to make a difference.
> 
> BR,
> Jani.
> 
> 
> [1] https://docs.kernel.org/process/submitting-patches.html#separate-your-changes
> [2] https://docs.kernel.org/process/5.Posting.html#patch-preparation
> 
> 
> 
> 
> > 
> > On the basic level following this guideline makes it trivial to review 
> > the refactoring patch, and in the vast majority of cases much easier to 
> > review the functional change patch as well.
> > 
> > And easy to review means happy reviewers, faster turnaround time (easier 
> > to carry a rebase), happier authors, easier reverts when things go bad 
> > (only small functional patch needs to be reverted), sometimes even 
> > easier backporting if code diverged a lot.
> > 
> > Oh, and it even has potential to decrease internal technical debt. Often 
> > you can push refactoring upstream and keep a smaller delta behind the 
> > closed doors, when that is required.
> > 
> > > > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > > > > ---
> > > > >   drivers/gpu/drm/i915/i915_drv.h              |  4 ----
> > > > >   drivers/gpu/drm/i915/pxp/intel_pxp.c         | 22 ++++++++++++++------
> > > > >   drivers/gpu/drm/i915/pxp/intel_pxp.h         |  3 +++
> > > > >   drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c |  2 +-
> > > > >   4 files changed, 20 insertions(+), 11 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 7e3820d2c404..0616e5f0bd31 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -933,10 +933,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)
> > > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > > > index 5efe61f67546..d993e752bd36 100644
> > > > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > > > @@ -44,6 +44,20 @@ struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
> > > > >   	return container_of(pxp, struct intel_gt, pxp);
> > > > >   }
> > > > >   
> > > > > +static bool _gt_needs_teelink(struct intel_gt *gt)
> > > > > +{
> > > > > +	/* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */
> > > > > +	return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && intel_huc_is_loaded_by_gsc(&gt->uc.huc) &&
> > > > > +		intel_uc_uses_huc(&gt->uc));
> > > > > +}
> > > > > +
> > > > > +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp)
> > > > 
> > > > If we are asking if it is supported on gt, then the argument must be a gt struct.
> > > > 
> > > I agree with you but Daniele said above is more consistent with existing ways that is considered the standard.
> > > Respectfully and humbly I would like to request both yourself and Daniele to show me the coding guidelines somewhere.
> > > 
> > > Honestly, this is one of the first few hunks of the first patch of the first series in a very large complex design to
> > > enable PXP on MTL and it only a helper utility function. Respecfully and humbly, I rather we focus our energy for review
> > > + redo  on more critical things like the e2e usage and top-to-bottom design or coding logic flows or find actual bugs
> > > instead of debating about coding styles for internal only helper functions.
> > 
> > My 2c/p is that the intel_pxp_supported_on_gt the "on_gt" part reads a 
> > bit clumsy because it makes it sound like the two are independent 
> > objects. Like I could pass one pxp to different GTs and ask if that one 
> > works here, or maybe there.
> > 
> > I am though a fan of intel_pxp_ prefix meaning function operates on 
> > struct intel_pxp.
> > 
> > In this case I don't know what is the correct relationship. If it is 1:1 
> > between intel_pxp:intel_gt then intel_pxp_supported(pxp) sounds fine. 
> > Even if a singleton that works for me. If we do have 1:1 but only want 
> > to init the first one, but PXP truly lives in the GT block, we could 
> > have pointers per GT, with only the gt0 one being initialized, and 
> > perhaps a shadow i915->pxp pointer which points to gt[0]->pxp, if that 
> > makes for better code organisation?
> > 
> > Regards,
> > 
> > Tvrtko
> > 
> > > 
> > > 
> > > > > +{
> > > > > +	/* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */
> > > > > +	return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && IS_ENABLED(CONFIG_DRM_I915_PXP) &&
> > > > > +		INTEL_INFO((pxp_to_gt(pxp))->i915)->has_pxp && VDBOX_MASK(pxp_to_gt(pxp)));
> > > > > +}
> > > > > +
> > > > >   bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
> > > > >   {
> > > > >   	return pxp->ce;
> > > > > @@ -142,17 +156,13 @@ void intel_pxp_init(struct intel_pxp *pxp)
> > > > >   {
> > > > >   	struct intel_gt *gt = pxp_to_gt(pxp);
> > > > >   
> > > > > -	/* we rely on the mei PXP module */
> > > > > -	if (!IS_ENABLED(CONFIG_INTEL_MEI_PXP))
> > > > > -		return;
> > > > 
> > > > I took a time to understand this movement based on the commit description.
> > > > I have the feeling that this patch deserves further split in different patches.
> > > > 
> > > > But also, looking a few lines above pxp_to_gt(pxp), I believe we
> > > > have further refactor to do sooner. is is one pxp per gt, then we
> > > > need to only enable in the gt0?
> > > > 
> > > In our driver, PXP as reflected by the device info is being treated as a global feature.
> > > PXP as a HW subsystem is "usable" device-wide from any workload on any engine on any tile (due to the internal mirror
> > > component and additional plumbing across the tiles). So in line with that I rather not have the gem-exec-buf, gem-create
> > > and gem-context calls be bothered about which GT to access to query of this global hw feature is enabled or active.
> > > However the control point for allocating sessions, talking to the gsc firmware and doing global teardowns are only meant
> > > to occur on and via the tile that owns the KCR engine which the media tile. This includes things like per-tile uncore
> > > power gating controls of the GSC-CS. (although some aspects like IRQ for KCR global). So as u see its not a clean per-GT
> > > feature.
> > > 
> > > I did speak to Daniele many months back when enabling the full feature set (on internal POC work) about whether we
> > > should make PXP a global subsystem instead of hanging off gt but we both agreed that because the control engines are
> > > only located on one tile, so you might face some its gonna be a trade off one way or the other:
> > >       - pxp a global structure, then all of the init / shutdown / suspend-resume flows would then have a different set of
> > > convoluted functions that try to get access to gt specific controls from a top level function flow.
> > > 
> > > 
> > > Additionally, humbly and respectfully, perhaps you can read through the internal arch HW specs through which it can be
> > > infered that PXP will continue to have a single entity for control events despite the feature being usable / accessible
> > > across all tiles.
> > > 
> > > > > -
> > > > >   	/*
> > > > >   	 * 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))
> > > > > +	if (intel_pxp_supported_on_gt(pxp))
> > > > >   		pxp_init_full(pxp);
> > > > > -	else if (intel_huc_is_loaded_by_gsc(&gt->uc.huc) && intel_uc_uses_huc(&gt->uc))
> > > > > +	else if (_gt_needs_teelink(gt))
> > > > >   		intel_pxp_tee_component_init(pxp);
> > > > >   }
> > > > >   
> > > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > > > > index 2da309088c6d..efa83f9d5e24 100644
> > > > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > > > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > > > > @@ -13,6 +13,9 @@ struct intel_pxp;
> > > > >   struct drm_i915_gem_object;
> > > > >   
> > > > >   struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp);
> > > > > +
> > > > > +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp);
> > > > > +
> > > > >   bool intel_pxp_is_enabled(const struct intel_pxp *pxp);
> > > > >   bool intel_pxp_is_active(const struct intel_pxp *pxp);
> > > > >   
> > > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > > > > index 4359e8be4101..f0ad6f34624a 100644
> > > > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > > > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > > > > @@ -70,7 +70,7 @@ void intel_pxp_debugfs_register(struct intel_pxp *pxp, struct dentry *gt_root)
> > > > >   	if (!gt_root)
> > > > >   		return;
> > > > >   
> > > > > -	if (!HAS_PXP((pxp_to_gt(pxp)->i915)))
> > > > > +	if (!intel_pxp_supported_on_gt(pxp))
> > > > >   		return;
> > > > >   
> > > > >   	root = debugfs_create_dir("pxp", gt_root);
> > > > > -- 
> > > > > 2.34.1
> > > > > 
> > > 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Teres Alexis, Alan Previn]

On Mon, 2022-11-21 at 11:39 +0000, Tvrtko Ursulin wrote:
> On 17/11/2022 22:34, Teres Alexis, Alan Previn wrote:
> > On Thu, 2022-11-17 at 11:02 -0500, Vivi, Rodrigo wrote:
> > > On Wed, Nov 16, 2022 at 04:30:13PM -0800, Alan Previn wrote:
> > > > In preparation for future MTL-PXP feature support, PXP control
> > > > context should only valid on the correct gt tile. Depending on the
> > > > device-info this depends on which tile owns the VEBOX and KCR.
> > > > PXP is still a global feature though (despite its control-context
> > > > located in the owning GT structure). Additionally, we find
> > > > that the HAS_PXP macro is only used within the pxp module,
> > > > 
> > > > That said, lets drop that HAS_PXP macro altogether and replace it
> > > > with a more fitting named intel_gtpxp_is_supported and helpers
> > > > so that PXP init/fini can use to verify if the referenced gt supports
> > > > PXP or teelink.
> > > 
> > > Yep, I understand you as I'm not fan of these macros, specially
> > > single usage. But we need to consider that we have multiple dependencies
> > > there and other cases like this in the driver... Well, but I'm not
> > > opposing, but probably better to first get rid of the macro,
> > > then change the behavior of the function on the next patch.
> > > 
> > > > 
> > > > Add TODO for Meteorlake that will come in future series.
> > > 
> > > This refactor patch should be standalone, without poputing it with
> > > the changes that didn't came yet to this point.
> > > 
> > Sure i can follow this rule, but it would then raise the question of "nothign is really changing anywhere for MTL, why
> > are we doing this" thats why i wanted to add that placeholder. I see "TODO"s are a common thing in the driver for larger
> > features that cant all be enabled at once. Respectfully and humbly, is there some documented rule? Can you show it to
> > me?
> 
> Separating refactoring from functional changes is one of the most basic 
> principles we follow (and not just us) and there should never be 
> pushback on the former due lack of functional changes.
> 
> On the basic level following this guideline makes it trivial to review 
> the refactoring patch, and in the vast majority of cases much easier to 
> review the functional change patch as well.
> 
> And easy to review means happy reviewers, faster turnaround time (easier 
> to carry a rebase), happier authors, easier reverts when things go bad 
> (only small functional patch needs to be reverted), sometimes even 
> easier backporting if code diverged a lot.
> 
> Oh, and it even has potential to decrease internal technical debt. Often 
> you can push refactoring upstream and keep a smaller delta behind the 
> closed doors, when that is required.
> 
> > > 
Okay got it - will remove that comment and ammend the commit msg to emphasis that this patch is for refactoring.

Re: [Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Teres Alexis, Alan Previn]

On Mon, 2022-11-21 at 14:06 +0000, Vivi, Rodrigo wrote:
> On Mon, 2022-11-21 at 11:39 +0000, Tvrtko Ursulin wrote:
> > 
> > On 17/11/2022 22:34, Teres Alexis, Alan Previn wrote:
> > > On Thu, 2022-11-17 at 11:02 -0500, Vivi, Rodrigo wrote:
> > > > On Wed, Nov 16, 2022 at 04:30:13PM -0800, Alan Previn wrote:
> > > > > In preparation for future MTL-PXP feature support, PXP control
> > > > > context should only valid on the correct gt tile. Depending on
> > > > > the
> > > > > +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp)
> > > > 
> > > > If we are asking if it is supported on gt, then the argument must
> > > > be a gt struct.
> > > > 
> > > I agree with you but Daniele said above is more consistent with
> > > existing ways that is considered the standard.
> > > Respectfully and humbly I would like to request both yourself and
> > > Daniele to show me the coding guidelines somewhere.
> > > 
> > > Honestly, this is one of the first few hunks of the first patch of
> > > the first series in a very large complex design to
> > > enable PXP on MTL and it only a helper utility function.
> > > Respecfully and humbly, I rather we focus our energy for review
> > > + redo  on more critical things like the e2e usage and top-to-
> > > bottom design or coding logic flows or find actual bugs
> > > instead of debating about coding styles for internal only helper
> > > functions.
> > 
> > My 2c/p is that the intel_pxp_supported_on_gt the "on_gt" part reads
> > a 
> > bit clumsy because it makes it sound like the two are independent 
> > objects. Like I could pass one pxp to different GTs and ask if that
> > one 
> > works here, or maybe there.
> > 
> > I am though a fan of intel_pxp_ prefix meaning function operates on 
> > struct intel_pxp.
> > 
> > In this case I don't know what is the correct relationship. If it is
> > 1:1 
> > between intel_pxp:intel_gt
> 
> Yeap, this is my main point here. It is not clear what is the correct
> relationship here.
> 
> Or we make the intel_pxp under the drm_i915_private, and then have the
> associated gt (always gt0?!) link inside the intel_pxp.
> 
> Or we keep it inside intel_gt as is today, but then we run all the
> functions gt agnostically and then skip when not enabled (!gt0?).
> 
> But all these functions to check "on_gt" makes the code hard to
> understand the relation and hard to maintain long term.
> 
> The argument that we have more patches in the pipeline to come on top
> of this series here just make it stronger the need for a very clean
> start.
> 
> 

Rodrigo, this is the jist of the contention - something Daniele and i discussed months back, and the direction we picked
with Option-1 below.


We have Option-1 where we stick with the current structure hierarchy - i.e. pxp is a gt-substructure. But for MTL, gt0's
pxp is not usable and media-tile's pxp is the control point. This means that backend code that accesses hw will be clean
(always already on the correct tile) - but then gem-execbuf / gem-create / gem-context (which are tile-agnostic layers
right?) are forced to pick the correct gt or have a dedicated helper to redirect from i915 layer to correct gt-tile
depending on platform. HW wise any context / buffer / request on any tile can still access PXP feature via batch level
commands.

Or for Option-2, we elevate pxp to a global level (or use ptr-to-pxp in gt) so tile-agnostic-layers call pxp without
needing to care about which tile. However, since pxp requires the ability to send commands or touch registers of the
media tile and not there other tile, such backend-code will be need that additional layer to pick the right gt. Also, we
will need to ensure the flow of init/fini and suspend/resume are "aligned" with the gt-tile level init/fini and
suspend/resume. So that presents another level of convoluted code to follow (when looking from a top-down e2e flow and
what params are being passed into different functions). 

I personally would prefer Option-2 where we elevate "intel_pxp" to a i915 level sub-structure. Based on my discussions
with the architects, they assured me that for the foreseeable future, there would always be a single "control-point" for
pxp feature and access to the backend firmware - i.e. "single" here meaning one tile only. 

In either case (option-1 or option-2) we will always be presented with one form readability confusion or another. Also
in either case we will likely have both types of function names : intel_pxp_is_enabled(pxp) vs
intel_gt_has_enabled_pxp(gt) - where one is a wrapper over the other - so we ought to keep the "enabled" part of the
name the same.

I havent thought about using a gt->pxp_ptr (like MTL's interrupt code). Lets consider this Option-3. If you think that
is an even better alternative, let me know, only the "pxp-to-gt" helpers would then need a rewrite but the init/fini
code would also have a tiny bit of kludginess since we will have to skip the allocation of said sub-structure for gt0
for MTL but not for ADL.

...alan

Re: [Intel-gfx] [PATCH v4 2/6] drm/i915/pxp: Make intel_pxp_is_enabled implicitly sort PXP-owning-GT

[Jani Nikula]

On Thu, 17 Nov 2022, "Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com> wrote:
> Respectfully and humbly, i would like to request where is the coding
> guideline for function naming when u have 2nd level subsystem IPs
> owning control over global hw features so that we dont need to have
> this back and forth of conflicting direction from different reviewers
> especially so long after initial reviews have started. (internally
> reworking future MTL PXP series end up getting impacted here).

Do you seriously think we could pre-emptively codify everything in a
coding guideline?

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Rodrigo Vivi]

On Mon, Nov 21, 2022 at 05:51:36PM +0000, Teres Alexis, Alan Previn wrote:
> 
> 
> On Mon, 2022-11-21 at 14:06 +0000, Vivi, Rodrigo wrote:
> > On Mon, 2022-11-21 at 11:39 +0000, Tvrtko Ursulin wrote:
> > > 
> > > On 17/11/2022 22:34, Teres Alexis, Alan Previn wrote:
> > > > On Thu, 2022-11-17 at 11:02 -0500, Vivi, Rodrigo wrote:
> > > > > On Wed, Nov 16, 2022 at 04:30:13PM -0800, Alan Previn wrote:
> > > > > > In preparation for future MTL-PXP feature support, PXP control
> > > > > > context should only valid on the correct gt tile. Depending on
> > > > > > the
> > > > > > +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp)
> > > > > 
> > > > > If we are asking if it is supported on gt, then the argument must
> > > > > be a gt struct.
> > > > > 
> > > > I agree with you but Daniele said above is more consistent with
> > > > existing ways that is considered the standard.
> > > > Respectfully and humbly I would like to request both yourself and
> > > > Daniele to show me the coding guidelines somewhere.
> > > > 
> > > > Honestly, this is one of the first few hunks of the first patch of
> > > > the first series in a very large complex design to
> > > > enable PXP on MTL and it only a helper utility function.
> > > > Respecfully and humbly, I rather we focus our energy for review
> > > > + redo  on more critical things like the e2e usage and top-to-
> > > > bottom design or coding logic flows or find actual bugs
> > > > instead of debating about coding styles for internal only helper
> > > > functions.
> > > 
> > > My 2c/p is that the intel_pxp_supported_on_gt the "on_gt" part reads
> > > a 
> > > bit clumsy because it makes it sound like the two are independent 
> > > objects. Like I could pass one pxp to different GTs and ask if that
> > > one 
> > > works here, or maybe there.
> > > 
> > > I am though a fan of intel_pxp_ prefix meaning function operates on 
> > > struct intel_pxp.
> > > 
> > > In this case I don't know what is the correct relationship. If it is
> > > 1:1 
> > > between intel_pxp:intel_gt
> > 
> > Yeap, this is my main point here. It is not clear what is the correct
> > relationship here.
> > 
> > Or we make the intel_pxp under the drm_i915_private, and then have the
> > associated gt (always gt0?!) link inside the intel_pxp.
> > 
> > Or we keep it inside intel_gt as is today, but then we run all the
> > functions gt agnostically and then skip when not enabled (!gt0?).
> > 
> > But all these functions to check "on_gt" makes the code hard to
> > understand the relation and hard to maintain long term.
> > 
> > The argument that we have more patches in the pipeline to come on top
> > of this series here just make it stronger the need for a very clean
> > start.
> > 
> > 
> 
> Rodrigo, this is the jist of the contention - something Daniele and i discussed months back, and the direction we picked
> with Option-1 below.
> 
> 
> We have Option-1 where we stick with the current structure hierarchy - i.e. pxp is a gt-substructure. But for MTL, gt0's
> pxp is not usable and media-tile's pxp is the control point. This means that backend code that accesses hw will be clean
> (always already on the correct tile) - but then gem-execbuf / gem-create / gem-context (which are tile-agnostic layers
> right?) are forced to pick the correct gt or have a dedicated helper to redirect from i915 layer to correct gt-tile
> depending on platform. HW wise any context / buffer / request on any tile can still access PXP feature via batch level
> commands.
> 
> Or for Option-2, we elevate pxp to a global level (or use ptr-to-pxp in gt) so tile-agnostic-layers call pxp without
> needing to care about which tile. However, since pxp requires the ability to send commands or touch registers of the
> media tile and not there other tile, such backend-code will be need that additional layer to pick the right gt. Also, we
> will need to ensure the flow of init/fini and suspend/resume are "aligned" with the gt-tile level init/fini and
> suspend/resume. So that presents another level of convoluted code to follow (when looking from a top-down e2e flow and
> what params are being passed into different functions). 
> 
> I personally would prefer Option-2 where we elevate "intel_pxp" to a i915 level sub-structure. Based on my discussions
> with the architects, they assured me that for the foreseeable future, there would always be a single "control-point" for
> pxp feature and access to the backend firmware - i.e. "single" here meaning one tile only. 
> 
> In either case (option-1 or option-2) we will always be presented with one form readability confusion or another. Also
> in either case we will likely have both types of function names : intel_pxp_is_enabled(pxp) vs
> intel_gt_has_enabled_pxp(gt) - where one is a wrapper over the other - so we ought to keep the "enabled" part of the
> name the same.
> 
> I havent thought about using a gt->pxp_ptr (like MTL's interrupt code). Lets consider this Option-3. If you think that
> is an even better alternative, let me know, only the "pxp-to-gt" helpers would then need a rewrite but the init/fini
> code would also have a tiny bit of kludginess since we will have to skip the allocation of said sub-structure for gt0
> for MTL but not for ADL.

As I had told I don't have a strong preference, as long as it keep clean
and without these many helpers of something "on_gt"...

If this stays inside the gt, just make sure that you call for all the gts,
but then you inside the functions you can skip if pxp not enabled... without
asking if enabled on_gt... 

> 
> ...alan

Re: [Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Teres Alexis, Alan Previn]

On Tue, 2022-11-22 at 12:57 -0500, Vivi, Rodrigo wrote:
> 
> 
[Alan:snip]

> As I had told I don't have a strong preference, as long as it keep clean
> and without these many helpers of something "on_gt"...
> 
> If this stays inside the gt, just make sure that you call for all the gts,
> but then you inside the functions you can skip if pxp not enabled... without
> asking if enabled on_gt... 
> 
I think we have here conflicting requests. The "consumers" of pxp feature are gem-execbuf, gem-context, gem-create (and
even display, for checking). Are we okay with making these callers be aware of "if mtl, ensure i call intel_pxp_foo with
the media-tile's pxp, agnostic to the request, context or buffer i am dealing with now". If you are okay with this, then
we can make this stay inside gt without "enabled on_gt" functions. But if dont want to polute such low level backend
awareness into those upper layers, we need them to call something more global like "intel_gpu_has_pxp_enabled" or
"intel_gpu_has_pxp_started" at the least with an i915 param. So that these callers dont need to worry about it. Or
intel_pxp_enabled has to internally check with gt we are being passed with and verify we are on the correct gt to - but
you said you dont want to have an "enabled on_gt" inside the pxp folder since pxp is a subset of gt. The only thing i
can think of is just a rename  - i.e. instead of "intel_pxp_enabled_on_gt", have a "intel_gpu_has_pxp_enabled(i915)" -
but it would reside in the pxp folder. Would this work?

> > 
> > ...alan

Re: [Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Teres Alexis, Alan Previn]

After a more comprehensive offline discussion with Daniele and Rodrigo, design direction was made to go with Option2
where we elevate pxp to a global subsystem and within it it establish a pointer to the correct gt for pxp-controls. This
also reflects the current HW architectures where the PXP feature (when viewed as a service for contexts and buffers) is
global to all subsystems including any workload on any tile, despite its single control-knobs being only in the media
tile (because PXP controls needs to be global to the GPU to avoid any races).

This would mean we move 'struct intel_pxp' to be under i915 so that all subsystems that need to call into PXP would now
pass in i915 as its parameter. PXP internally would have a pointer to the correct GT (media-tile for MTL and gt0 for
prior).

It would also mean that certain code will still look a little kludgy or needs attention:
 - power-related operations like init/fini and suspend/resume would now need to called either before or after all-gt
equivalents to ensure proper flow.
 - KCR IRQ will although being a gt level IRQ will now require passing i915 into the pxp subsystem.

*NOTE: above list, even if i missed any, would still be nowhere near the amount of other external facing interfaces that
would be called by global subsystems that would now look clean with i915->pxp as its param.

...alan


On Tue, 2022-11-22 at 10:52 -0800, Alan Previn Teres Alexis wrote:
> 
> On Tue, 2022-11-22 at 12:57 -0500, Vivi, Rodrigo wrote:
> > 
> > 
> [Alan:snip]
> 
> > As I had told I don't have a strong preference, as long as it keep clean
> > and without these many helpers of something "on_gt"...
> > 
> > If this stays inside the gt, just make sure that you call for all the gts,
> > but then you inside the functions you can skip if pxp not enabled... without
> > asking if enabled on_gt... 
> > 
> I think we have here conflicting requests. The "consumers" of pxp feature are gem-execbuf, gem-context, gem-create (and
> even display, for checking). Are we okay with making these callers be aware of "if mtl, ensure i call intel_pxp_foo with
> the media-tile's pxp, agnostic to the request, context or buffer i am dealing with now". If you are okay with this, then
> we can make this stay inside gt without "enabled on_gt" functions. But if dont want to polute such low level backend
> awareness into those upper layers, we need them to call something more global like "intel_gpu_has_pxp_enabled" or
> "intel_gpu_has_pxp_started" at the least with an i915 param. So that these callers dont need to worry about it. Or
> intel_pxp_enabled has to internally check with gt we are being passed with and verify we are on the correct gt to - but
> you said you dont want to have an "enabled on_gt" inside the pxp folder since pxp is a subset of gt. The only thing i
> can think of is just a rename  - i.e. instead of "intel_pxp_enabled_on_gt", have a "intel_gpu_has_pxp_enabled(i915)" -
> but it would reside in the pxp folder. Would this work?
> 
> > > 
> > > ...alan
> 

Re: [Intel-gfx] [PATCH v4 2/6] drm/i915/pxp: Make intel_pxp_is_enabled implicitly sort PXP-owning-GT

[Teres Alexis, Alan Previn]

Not everything of course, but intel_feature_action(param1, ...) enforcing param1 to always be struct intel_feature_t i
assumed was what Rodrigo meant. And my intention wasn't to verify that rule but rather look for surrounding precedence
for any exceptions to it (i felt PXP was a candidate for an exception since its services and consumers are global but
its control-points are within the media tile alone).

Either way, this exception won't be required give the new design direction from Rodrigo based on that last reply to
patch #1. We will elevate pxp to be a global subsystem (despite the controls being gt specific).


...alan

On Tue, 2022-11-22 at 13:17 +0200, Jani Nikula wrote:
> On Thu, 17 Nov 2022, "Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com> wrote:
> > Respectfully and humbly, i would like to request where is the coding
> > guideline for function naming when u have 2nd level subsystem IPs
> > owning control over global hw features so that we dont need to have
> > this back and forth of conflicting direction from different reviewers
> > especially so long after initial reviews have started. (internally
> > reworking future MTL PXP series end up getting impacted here).
> 
> Do you seriously think we could pre-emptively codify everything in a
> coding guideline?
> 
> BR,
> Jani.
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Rodrigo Vivi]

On Tue, Nov 22, 2022 at 06:50:14PM +0000, Teres Alexis, Alan Previn wrote:
> 
> 
> On Tue, 2022-11-22 at 12:57 -0500, Vivi, Rodrigo wrote:
> > 
> > 
> [Alan:snip]
> 
> > As I had told I don't have a strong preference, as long as it keep clean
> > and without these many helpers of something "on_gt"...
> > 
> > If this stays inside the gt, just make sure that you call for all the gts,
> > but then you inside the functions you can skip if pxp not enabled... without
> > asking if enabled on_gt... 
> > 
> I think we have here conflicting requests. The "consumers" of pxp feature are gem-execbuf, gem-context, gem-create (and
> even display, for checking). Are we okay with making these callers be aware of "if mtl, ensure i call intel_pxp_foo with
> the media-tile's pxp, agnostic to the request, context or buffer i am dealing with now". If you are okay with this, then
> we can make this stay inside gt without "enabled on_gt" functions. But if dont want to polute such low level backend
> awareness into those upper layers, we need them to call something more global like "intel_gpu_has_pxp_enabled" or
> "intel_gpu_has_pxp_started" at the least with an i915 param. So that these callers dont need to worry about it. Or
> intel_pxp_enabled has to internally check with gt we are being passed with and verify we are on the correct gt to - but
> you said you dont want to have an "enabled on_gt" inside the pxp folder since pxp is a subset of gt. The only thing i
> can think of is just a rename  - i.e. instead of "intel_pxp_enabled_on_gt", have a "intel_gpu_has_pxp_enabled(i915)" -
> but it would reside in the pxp folder. Would this work?

In the end I believe that the pxp needs to be the one with knowledge of the
serving_gt. Either gt0 on TGL or media_gt on MTL.

So, if we keep the intel_pxp inside the intel_gt to make the initialization,
irq, and pm flows cleaner, we need probably need to save the
struct intel_gt *gt_serving_pxp;
inside all the intel_pxp, or in drm_i915_private, and then
have a helper that returns the ...gt_serving_pxp(...)
but then skipping the init/fini functions if gt parent != gt_serving_pxp.

> 
> > > 
> > > ...alan
> 

Re: [Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Teres Alexis, Alan Previn]

typo correction...

On Tue, 2022-11-22 at 12:13 -0800, Alan Previn Teres Alexis wrote:
> After a more comprehensive offline discussion with Daniele and Rodrigo, design direction was made to go with Option2
> where we elevate pxp to a global subsystem and within it it establish a pointer to the correct gt for pxp-controls. This
> also reflects the current HW architectures where the PXP feature (when viewed as a service for contexts and buffers) is
> global to all subsystems including any workload on any tile, despite its single control-knobs being only in the media
> tile (because PXP controls needs to be global to the GPU to avoid any races).
> 
> This would mean we move 'struct intel_pxp' to be under i915 so that all subsystems that need to call into PXP would now
> pass in i915 as its parameter. PXP internally would have a pointer to the correct GT (media-tile for MTL and gt0 for
typo: "pass in i915->pxp as its parameter"
> prior).
> 
> It would also mean that certain code will still look a little kludgy or needs attention:
>  - power-related operations like init/fini and suspend/resume would now need to called either before or after all-gt
> equivalents to ensure proper flow.
>  - KCR IRQ will although being a gt level IRQ will now require passing i915 into the pxp subsystem.
> 
> *NOTE: above list, even if i missed any, would still be nowhere near the amount of other external facing interfaces that
> would be called by global subsystems that would now look clean with i915->pxp as its param.
> 
> ...alan