Re: [Intel-gfx] [PATCH v3 2/6] drm/i915/pxp: Make intel_pxp_is_enabled implicitly sort PXP-owning-GT
On Mon, 2022-11-14 at 20:11 -0800, Ceraolo Spurio, Daniele wrote:
>
> On 10/21/2022 10:39 AM, Alan Previn wrote:
> > @@ -68,11 +69,34 @@ bool intel_gtpxp_is_supported(struct intel_pxp *pxp)
> > return false;
> > }
> >
> > -bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
> > +bool intel_gtpxp_is_enabled(const struct intel_pxp *pxp)
>
> I'd rename this to intel_pxp_is_initialized, that way we don't have 2
> almost identically named checkers that mean different things (and also
> avoid the gtpxp prefix).
>
I disagree - one is a wrapper around the other so i rather DO insist we have
the same function-action name in the middle
with a different part of the function name being the qualifier for whether its
a global level checker or a gt-level
checker. Perhaps as per last review reply, we can do "intel_pxp_is_enabled" as
wrapper around "intel_gt_has_pxp_enabled"
- i think the "enabled" part SHOULD be consistent since one is a wrapper around
the other else a new reader will even
more baffled about why "enabled" is different from "initialized" despite trying
to get to the same anchor point, "pxp-
>ce".
> > {
> > return pxp->ce;
> > }
> >
> > +static struct intel_gt *_i915_to_pxp_gt(struct drm_i915_private *i915)
>
> nit: why the "_" prefix? we usually don't use it for x_to_y functions.
> Not a blocker.
I was assuming internal static functions dont have to obey such rules - i like
to use _foo for all local static
functions (so that when reading from a caller's code, i know its a local
static). Again, just another naming convention
preference thing that i feel seems to be happening here and there in the driver
code base but not consistent across all
files / function types. Since its a nit, i won't change this.
>
> > +{
> > + struct intel_gt *gt = NULL;
> > + int i = 0;
> > +
> > + for_each_gt(gt, i915, i) {
> > + /* There can be only one GT that supports PXP */
>
>
>
> > + if (gt && intel_gtpxp_is_supported(>pxp))
>
> for_each_gt already checks for gt not being NULL, no need to check again.
Got it - will fix this.
>
> Daniele
>
>
Re: [Intel-gfx] [PATCH v3 2/6] drm/i915/pxp: Make intel_pxp_is_enabled implicitly sort PXP-owning-GT
On 10/21/2022 10:39 AM, Alan Previn wrote:
Make intel_pxp_is_enabled a global check and implicitly find the
PXP-owning-GT.
PXP feature support is a device-config flag. In preparation for MTL
PXP control-context shall reside on of the two GT's. That said,
update intel_pxp_is_enabled to take in i915 as its input and internally
find the right gt to check if PXP is enabled so its transparent to
callers of this functions.
However we also need to expose the per-gt variation of this internal
pxp files to use (like what intel_pxp_enabled was prior) so also expose
a new intel_gtpxp_is_enabled function for replacement.
Signed-off-by: Alan Previn
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_create.c | 2 +-
drivers/gpu/drm/i915/pxp/intel_pxp.c | 28 ++--
drivers/gpu/drm/i915/pxp/intel_pxp.h | 4 ++-
drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c | 2 +-
drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 2 +-
drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 2 +-
drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 8 +++---
drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 4 +--
9 files changed, 40 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 1e29b1e6d186..72f47ebda75f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -257,7 +257,7 @@ static int proto_context_set_protected(struct
drm_i915_private *i915,
if (!protected) {
pc->uses_protected_content = false;
- } else if (!intel_pxp_is_enabled(_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(_gt(ext_data->i915)->pxp))
+ if (!intel_pxp_is_enabled(ext_data->i915))
return -ENODEV;
ext_data->flags |= I915_BO_PROTECTED;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 545c075bf1aa..f7c909fce97c 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -9,6 +9,7 @@
#include "intel_pxp_tee.h"
#include "gem/i915_gem_context.h"
#include "gt/intel_context.h"
+#include "gt/intel_gt.h"
#include "i915_drv.h"
/**
@@ -68,11 +69,34 @@ bool intel_gtpxp_is_supported(struct intel_pxp *pxp)
return false;
}
-bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
+bool intel_gtpxp_is_enabled(const struct intel_pxp *pxp)
I'd rename this to intel_pxp_is_initialized, that way we don't have 2
almost identically named checkers that mean different things (and also
avoid the gtpxp prefix).
{
return pxp->ce;
}
+static struct intel_gt *_i915_to_pxp_gt(struct drm_i915_private *i915)
nit: why the "_" prefix? we usually don't use it for x_to_y functions.
Not a blocker.
+{
+ struct intel_gt *gt = NULL;
+ int i = 0;
+
+ for_each_gt(gt, i915, i) {
+ /* There can be only one GT that supports PXP */
+ if (gt && intel_gtpxp_is_supported(>pxp))
for_each_gt already checks for gt not being NULL, no need to check again.
Daniele
+ return gt;
+ }
+ return NULL;
+}
+
+bool intel_pxp_is_enabled(struct drm_i915_private *i915)
+{
+ struct intel_gt *gt = _i915_to_pxp_gt(i915);
+
+ if (!gt)
+ return false;
+
+ return intel_gtpxp_is_enabled(>pxp);
+}
+
bool intel_pxp_is_active(const struct intel_pxp *pxp)
{
return pxp->arb_is_valid;
@@ -229,7 +253,7 @@ int intel_pxp_start(struct intel_pxp *pxp)
{
int ret = 0;
- if (!intel_pxp_is_enabled(pxp))
+ if (!intel_gtpxp_is_enabled(pxp))
return -ENODEV;
if (wait_for(pxp_component_bound(pxp), 250))
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h
b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index c12e4d419c78..61472018bc45 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -11,11 +11,13 @@
struct intel_pxp;
struct drm_i915_gem_object;
+struct drm_i915_private;
struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp);
bool intel_gtpxp_is_supported(struct intel_pxp *pxp);
+bool intel_gtpxp_is_enabled(const struct intel_pxp *pxp);
-bool intel_pxp_is_enabled(const struct intel_pxp *pxp);
+bool
[Intel-gfx] [PATCH v3 2/6] drm/i915/pxp: Make intel_pxp_is_enabled implicitly sort PXP-owning-GT
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
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_create.c | 2 +-
drivers/gpu/drm/i915/pxp/intel_pxp.c | 28 ++--
drivers/gpu/drm/i915/pxp/intel_pxp.h | 4 ++-
drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c | 2 +-
drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 2 +-
drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 2 +-
drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 8 +++---
drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 4 +--
9 files changed, 40 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 1e29b1e6d186..72f47ebda75f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -257,7 +257,7 @@ static int proto_context_set_protected(struct
drm_i915_private *i915,
if (!protected) {
pc->uses_protected_content = false;
- } else if (!intel_pxp_is_enabled(_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(_gt(ext_data->i915)->pxp))
+ if (!intel_pxp_is_enabled(ext_data->i915))
return -ENODEV;
ext_data->flags |= I915_BO_PROTECTED;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 545c075bf1aa..f7c909fce97c 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -9,6 +9,7 @@
#include "intel_pxp_tee.h"
#include "gem/i915_gem_context.h"
#include "gt/intel_context.h"
+#include "gt/intel_gt.h"
#include "i915_drv.h"
/**
@@ -68,11 +69,34 @@ bool intel_gtpxp_is_supported(struct intel_pxp *pxp)
return false;
}
-bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
+bool intel_gtpxp_is_enabled(const struct intel_pxp *pxp)
{
return pxp->ce;
}
+static struct intel_gt *_i915_to_pxp_gt(struct drm_i915_private *i915)
+{
+ struct intel_gt *gt = NULL;
+ int i = 0;
+
+ for_each_gt(gt, i915, i) {
+ /* There can be only one GT that supports PXP */
+ if (gt && intel_gtpxp_is_supported(>pxp))
+ return gt;
+ }
+ return NULL;
+}
+
+bool intel_pxp_is_enabled(struct drm_i915_private *i915)
+{
+ struct intel_gt *gt = _i915_to_pxp_gt(i915);
+
+ if (!gt)
+ return false;
+
+ return intel_gtpxp_is_enabled(>pxp);
+}
+
bool intel_pxp_is_active(const struct intel_pxp *pxp)
{
return pxp->arb_is_valid;
@@ -229,7 +253,7 @@ int intel_pxp_start(struct intel_pxp *pxp)
{
int ret = 0;
- if (!intel_pxp_is_enabled(pxp))
+ if (!intel_gtpxp_is_enabled(pxp))
return -ENODEV;
if (wait_for(pxp_component_bound(pxp), 250))
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h
b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index c12e4d419c78..61472018bc45 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -11,11 +11,13 @@
struct intel_pxp;
struct drm_i915_gem_object;
+struct drm_i915_private;
struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp);
bool intel_gtpxp_is_supported(struct intel_pxp *pxp);
+bool intel_gtpxp_is_enabled(const struct intel_pxp *pxp);
-bool intel_pxp_is_enabled(const struct intel_pxp *pxp);
+bool intel_pxp_is_enabled(struct drm_i915_private *i915);
bool intel_pxp_is_active(const struct intel_pxp *pxp);
void intel_pxp_init(struct intel_pxp *pxp);
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c
b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c
index f41e45763d0d..0987bb552eaa 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c
@@ -99,7 +99,7 @@ int
