Re: [Intel-gfx] [RFC-v1 01/16] drm/i915/pxp: Introduce Intel PXP component
Quoting Huang, Sean Z (2020-12-07 20:48:55) > > >Repeating the same comment as on previous review, avoid including anything > >in i915_drv.h and only include in the relevant files that require to touch > >the internals of the structs. > > I would still need to include i915_drv.h for macro INTEL_GEN(), hopefully > it's acceptable. That is fine. I was referring to the opposite, do not include "intel_pxp.h" from i915_drv.h. It's instead better to add "intel_pxp_types.h" etc. (look for *_types.h files from i915 source) and minimize what is included by each file. Regards, Joonas > > -Original Message- > From: Huang, Sean Z > Sent: Monday, December 7, 2020 10:26 AM > To: Joonas Lahtinen ; > Intel-gfx@lists.freedesktop.org > Subject: RE: [Intel-gfx] [RFC-v1 01/16] drm/i915/pxp: Introduce Intel PXP > component > > Hi Joonas, > > Thanks for the details review. I have apply the modification according to the > review, and will update as rev2. > > Description is no more true for single-session only > DONE > > > Same here, needs updating. > DONE > > >Repeating the same comment as on previous review, avoid including anything > >in i915_drv.h and only include in the relevant files that require to touch > >the internals of the structs. > DONE > > > I think this should instead go as part of intel_gt, not here. > DONE > > > We should aim to only take struct intel_pxp as parameter for intel_pxp_* > > functions. > DONE, I think the suggestion is reasonable. I expect that will modify the > code significantly in the future commits, but let me try intel_pxp_* instead > of i915 > > > This would be either a major kernel programmer error or the memory would be > > seriously corrupt. No point leaving such checks to production code, so > > GEM_BUG_ON(!i915) would be enough to run the checks in CI and debug builds. > DONE, I just remove the error check > > > Also, we have not really initialized anything so it's really premature to > > print anything in this patch. > DONE, remove the print > > > Same here, we really want to tighten the scope to intel_pxp and call > > this from intel_gt_fini(), so signature should look like: void > > intel_pxp_fini(struct intel_pxp *pxp) > DONE > > Best regards, > Sean > > -Original Message- > From: Joonas Lahtinen > Sent: Monday, December 7, 2020 2:01 AM > To: Huang, Sean Z ; Intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [RFC-v1 01/16] drm/i915/pxp: Introduce Intel PXP > component > > Quoting Huang, Sean Z (2020-12-07 02:21:19) > > PXP (Protected Xe Path) is an i915 componment, available on GEN12+, > > that helps user space to establish the hardware protected session and > > manage the status of each alive software session, as well as the life > > cycle of each session. > > > > By design PXP will expose ioctl so allow user space to create, set, > > and destroy each session. It will also provide the communication > > chanel to TEE (Trusted Execution Environment) for the protected > > hardware session creation. > > Description is no more true for single-session only. > > > > > +++ b/drivers/gpu/drm/i915/Kconfig > > @@ -130,6 +130,25 @@ config DRM_I915_GVT_KVMGT > > Choose this option if you want to enable KVMGT support for > > Intel GVT-g. > > > > +config DRM_I915_PXP > > + bool "Enable Intel PXP support for Intel Gen12+ platform" > > + depends on DRM_I915 > > + select INTEL_MEI_PXP > > + default n > > + help > > + This option selects INTEL_MEI_ME if it isn't already selected to > > + enabled full PXP Services on Intel platforms. > > + > > + PXP is an i915 componment, available on Gen12+, that helps user > > + space to establish the hardware protected session and manage the > > + status of each alive software session, as well as the life cycle > > + of each session. > > + > > + PXP expose ioctl so allow user space to create, set, and destroy > > + each session. It will also provide the communication chanel to > > + TEE (Trusted Execution Environment) for the protected hardware > > + session creation. > > Same here, needs updating. > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -105,6 +105,8 @@ > > > > #include "intel_region_lmem.h" > > > > +#include "pxp/intel_pxp.h" > > Repeating the same comment as on previous review, avoid including
Re: [Intel-gfx] [RFC-v1 01/16] drm/i915/pxp: Introduce Intel PXP component
>Repeating the same comment as on previous review, avoid including anything in
>i915_drv.h and only include in the relevant files that require to touch the
>internals of the structs.
I would still need to include i915_drv.h for macro INTEL_GEN(), hopefully it's
acceptable.
-Original Message-
From: Huang, Sean Z
Sent: Monday, December 7, 2020 10:26 AM
To: Joonas Lahtinen ;
Intel-gfx@lists.freedesktop.org
Subject: RE: [Intel-gfx] [RFC-v1 01/16] drm/i915/pxp: Introduce Intel PXP
component
Hi Joonas,
Thanks for the details review. I have apply the modification according to the
review, and will update as rev2.
> Description is no more true for single-session only
DONE
> Same here, needs updating.
DONE
>Repeating the same comment as on previous review, avoid including anything in
>i915_drv.h and only include in the relevant files that require to touch the
>internals of the structs.
DONE
> I think this should instead go as part of intel_gt, not here.
DONE
> We should aim to only take struct intel_pxp as parameter for intel_pxp_*
> functions.
DONE, I think the suggestion is reasonable. I expect that will modify the code
significantly in the future commits, but let me try intel_pxp_* instead of i915
> This would be either a major kernel programmer error or the memory would be
> seriously corrupt. No point leaving such checks to production code, so
> GEM_BUG_ON(!i915) would be enough to run the checks in CI and debug builds.
DONE, I just remove the error check
> Also, we have not really initialized anything so it's really premature to
> print anything in this patch.
DONE, remove the print
> Same here, we really want to tighten the scope to intel_pxp and call
> this from intel_gt_fini(), so signature should look like: void
> intel_pxp_fini(struct intel_pxp *pxp)
DONE
Best regards,
Sean
-Original Message-
From: Joonas Lahtinen
Sent: Monday, December 7, 2020 2:01 AM
To: Huang, Sean Z ; Intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [RFC-v1 01/16] drm/i915/pxp: Introduce Intel PXP
component
Quoting Huang, Sean Z (2020-12-07 02:21:19)
> PXP (Protected Xe Path) is an i915 componment, available on GEN12+,
> that helps user space to establish the hardware protected session and
> manage the status of each alive software session, as well as the life
> cycle of each session.
>
> By design PXP will expose ioctl so allow user space to create, set,
> and destroy each session. It will also provide the communication
> chanel to TEE (Trusted Execution Environment) for the protected
> hardware session creation.
Description is no more true for single-session only.
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -130,6 +130,25 @@ config DRM_I915_GVT_KVMGT
> Choose this option if you want to enable KVMGT support for
> Intel GVT-g.
>
> +config DRM_I915_PXP
> + bool "Enable Intel PXP support for Intel Gen12+ platform"
> + depends on DRM_I915
> + select INTEL_MEI_PXP
> + default n
> + help
> + This option selects INTEL_MEI_ME if it isn't already selected to
> + enabled full PXP Services on Intel platforms.
> +
> + PXP is an i915 componment, available on Gen12+, that helps user
> + space to establish the hardware protected session and manage the
> + status of each alive software session, as well as the life cycle
> + of each session.
> +
> + PXP expose ioctl so allow user space to create, set, and destroy
> + each session. It will also provide the communication chanel to
> + TEE (Trusted Execution Environment) for the protected hardware
> + session creation.
Same here, needs updating.
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -105,6 +105,8 @@
>
> #include "intel_region_lmem.h"
>
> +#include "pxp/intel_pxp.h"
Repeating the same comment as on previous review, avoid including anything in
i915_drv.h and only include in the relevant files that require to touch the
internals of the structs.
> +
> /* General customization:
> */
>
> @@ -1215,6 +1217,8 @@ struct drm_i915_private {
> /* Mutex to protect the above hdcp component related values. */
> struct mutex hdcp_comp_mutex;
>
> + struct intel_pxp pxp;
I think this should instead go as part of intel_gt, not here.
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2020 Intel Corporation.
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_pxp.h"
> +
> +int intel_pxp_init(struct drm_i915_private *i915)
We should aim to only take struct intel_pxp as parameter for intel_pxp_*
functions.
> +{
> + i
Re: [Intel-gfx] [RFC-v1 01/16] drm/i915/pxp: Introduce Intel PXP component
Hi Joonas,
Thanks for the details review. I have apply the modification according to the
review, and will update as rev2.
> Description is no more true for single-session only
DONE
> Same here, needs updating.
DONE
>Repeating the same comment as on previous review, avoid including anything in
>i915_drv.h and only include in the relevant files that require to touch the
>internals of the structs.
DONE
> I think this should instead go as part of intel_gt, not here.
DONE
> We should aim to only take struct intel_pxp as parameter for intel_pxp_*
> functions.
DONE, I think the suggestion is reasonable. I expect that will modify the code
significantly in the future commits, but let me try intel_pxp_* instead of i915
> This would be either a major kernel programmer error or the memory would be
> seriously corrupt. No point leaving such checks to production code, so
> GEM_BUG_ON(!i915) would be enough to run the checks in CI and debug builds.
DONE, I just remove the error check
> Also, we have not really initialized anything so it's really premature to
> print anything in this patch.
DONE, remove the print
> Same here, we really want to tighten the scope to intel_pxp and call this
> from intel_gt_fini(), so signature should look like: void
> intel_pxp_fini(struct intel_pxp *pxp)
DONE
Best regards,
Sean
-Original Message-
From: Joonas Lahtinen
Sent: Monday, December 7, 2020 2:01 AM
To: Huang, Sean Z ; Intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [RFC-v1 01/16] drm/i915/pxp: Introduce Intel PXP
component
Quoting Huang, Sean Z (2020-12-07 02:21:19)
> PXP (Protected Xe Path) is an i915 componment, available on GEN12+,
> that helps user space to establish the hardware protected session and
> manage the status of each alive software session, as well as the life
> cycle of each session.
>
> By design PXP will expose ioctl so allow user space to create, set,
> and destroy each session. It will also provide the communication
> chanel to TEE (Trusted Execution Environment) for the protected
> hardware session creation.
Description is no more true for single-session only.
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -130,6 +130,25 @@ config DRM_I915_GVT_KVMGT
> Choose this option if you want to enable KVMGT support for
> Intel GVT-g.
>
> +config DRM_I915_PXP
> + bool "Enable Intel PXP support for Intel Gen12+ platform"
> + depends on DRM_I915
> + select INTEL_MEI_PXP
> + default n
> + help
> + This option selects INTEL_MEI_ME if it isn't already selected to
> + enabled full PXP Services on Intel platforms.
> +
> + PXP is an i915 componment, available on Gen12+, that helps user
> + space to establish the hardware protected session and manage the
> + status of each alive software session, as well as the life cycle
> + of each session.
> +
> + PXP expose ioctl so allow user space to create, set, and destroy
> + each session. It will also provide the communication chanel to
> + TEE (Trusted Execution Environment) for the protected hardware
> + session creation.
Same here, needs updating.
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -105,6 +105,8 @@
>
> #include "intel_region_lmem.h"
>
> +#include "pxp/intel_pxp.h"
Repeating the same comment as on previous review, avoid including anything in
i915_drv.h and only include in the relevant files that require to touch the
internals of the structs.
> +
> /* General customization:
> */
>
> @@ -1215,6 +1217,8 @@ struct drm_i915_private {
> /* Mutex to protect the above hdcp component related values. */
> struct mutex hdcp_comp_mutex;
>
> + struct intel_pxp pxp;
I think this should instead go as part of intel_gt, not here.
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2020 Intel Corporation.
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_pxp.h"
> +
> +int intel_pxp_init(struct drm_i915_private *i915)
We should aim to only take struct intel_pxp as parameter for intel_pxp_*
functions.
> +{
> + if (!i915)
> + return -EINVAL;
This would be either a major kernel programmer error or the memory would be
seriously corrupt. No point leaving such checks to production code, so
GEM_BUG_ON(!i915) would be enough to run the checks in CI and debug builds.
> + /* PXP only available for GEN12+ */
> + if (INTEL_GEN(i915) < 12)
> + return 0;
I think -ENODEV would be more appropriate return value. Also, we should look
into returning this error value from ins
Re: [Intel-gfx] [RFC-v1 01/16] drm/i915/pxp: Introduce Intel PXP component
Quoting Huang, Sean Z (2020-12-07 02:21:19)
> PXP (Protected Xe Path) is an i915 componment, available on GEN12+,
> that helps user space to establish the hardware protected session
> and manage the status of each alive software session, as well as
> the life cycle of each session.
>
> By design PXP will expose ioctl so allow user space to create, set,
> and destroy each session. It will also provide the communication
> chanel to TEE (Trusted Execution Environment) for the protected
> hardware session creation.
Description is no more true for single-session only.
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -130,6 +130,25 @@ config DRM_I915_GVT_KVMGT
> Choose this option if you want to enable KVMGT support for
> Intel GVT-g.
>
> +config DRM_I915_PXP
> + bool "Enable Intel PXP support for Intel Gen12+ platform"
> + depends on DRM_I915
> + select INTEL_MEI_PXP
> + default n
> + help
> + This option selects INTEL_MEI_ME if it isn't already selected to
> + enabled full PXP Services on Intel platforms.
> +
> + PXP is an i915 componment, available on Gen12+, that helps user
> + space to establish the hardware protected session and manage the
> + status of each alive software session, as well as the life cycle
> + of each session.
> +
> + PXP expose ioctl so allow user space to create, set, and destroy
> + each session. It will also provide the communication chanel to
> + TEE (Trusted Execution Environment) for the protected hardware
> + session creation.
Same here, needs updating.
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -105,6 +105,8 @@
>
> #include "intel_region_lmem.h"
>
> +#include "pxp/intel_pxp.h"
Repeating the same comment as on previous review, avoid including
anything in i915_drv.h and only include in the relevant files that
require to touch the internals of the structs.
> +
> /* General customization:
> */
>
> @@ -1215,6 +1217,8 @@ struct drm_i915_private {
> /* Mutex to protect the above hdcp component related values. */
> struct mutex hdcp_comp_mutex;
>
> + struct intel_pxp pxp;
I think this should instead go as part of intel_gt, not here.
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2020 Intel Corporation.
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_pxp.h"
> +
> +int intel_pxp_init(struct drm_i915_private *i915)
We should aim to only take struct intel_pxp as parameter for intel_pxp_*
functions.
> +{
> + if (!i915)
> + return -EINVAL;
This would be either a major kernel programmer error or the memory would
be seriously corrupt. No point leaving such checks to production code,
so GEM_BUG_ON(!i915) would be enough to run the checks in CI and debug
builds.
> + /* PXP only available for GEN12+ */
> + if (INTEL_GEN(i915) < 12)
> + return 0;
I think -ENODEV would be more appropriate return value. Also, we should
look into returning this error value from inside the actual init code.
We want the user to be able to differentiate between kernel does not
support and hardware does not support status.
> + drm_info(>drm, "i915 PXP is inited with i915=[%p]\n", i915);
We shouldn't be printing the pointer values, especially not in INFO
level messages. INFO level messages should be useful for the end-user to
read. This is not very useful, we should instead consider something
along the lines of:
"Protected Xe Path (PXP) protected content support initialized"
Also, we have not really initialized anything so it's really premature
to print anything in this patch.
> +
> + return 0;
> +}
> +
> +void intel_pxp_uninit(struct drm_i915_private *i915)
Same here, we really want to tighten the scope to intel_pxp and call
this from intel_gt_fini(), so signature should look like:
void intel_pxp_fini(struct intel_pxp *pxp)
Regards, Joonas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC-v1 01/16] drm/i915/pxp: Introduce Intel PXP component
PXP (Protected Xe Path) is an i915 componment, available on GEN12+,
that helps user space to establish the hardware protected session
and manage the status of each alive software session, as well as
the life cycle of each session.
By design PXP will expose ioctl so allow user space to create, set,
and destroy each session. It will also provide the communication
chanel to TEE (Trusted Execution Environment) for the protected
hardware session creation.
Signed-off-by: Huang, Sean Z
---
drivers/gpu/drm/i915/Kconfig | 19
drivers/gpu/drm/i915/Makefile| 4
drivers/gpu/drm/i915/i915_drv.c | 4
drivers/gpu/drm/i915/i915_drv.h | 4
drivers/gpu/drm/i915/pxp/intel_pxp.c | 25 +
drivers/gpu/drm/i915/pxp/intel_pxp.h | 33
6 files changed, 89 insertions(+)
create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp.c
create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp.h
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 1e1cb245fca7..f82ccc901b1e 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -130,6 +130,25 @@ config DRM_I915_GVT_KVMGT
Choose this option if you want to enable KVMGT support for
Intel GVT-g.
+config DRM_I915_PXP
+ bool "Enable Intel PXP support for Intel Gen12+ platform"
+ depends on DRM_I915
+ select INTEL_MEI_PXP
+ default n
+ help
+ This option selects INTEL_MEI_ME if it isn't already selected to
+ enabled full PXP Services on Intel platforms.
+
+ PXP is an i915 componment, available on Gen12+, that helps user
+ space to establish the hardware protected session and manage the
+ status of each alive software session, as well as the life cycle
+ of each session.
+
+ PXP expose ioctl so allow user space to create, set, and destroy
+ each session. It will also provide the communication chanel to
+ TEE (Trusted Execution Environment) for the protected hardware
+ session creation.
+
menu "drm/i915 Debugging"
depends on DRM_I915
depends on EXPERT
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index e5574e506a5c..a53ea3c88f71 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -254,6 +254,10 @@ i915-y += \
i915-y += i915_perf.o
+# Protected execution platform (PXP) support
+i915-$(CONFIG_DRM_I915_PXP) += \
+ pxp/intel_pxp.o
+
# Post-mortem debug and GPU hang state capture
i915-$(CONFIG_DRM_I915_CAPTURE_ERROR) += i915_gpu_error.o
i915-$(CONFIG_DRM_I915_SELFTEST) += \
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 320856b665a1..1e5ecaff571f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -889,6 +889,8 @@ int i915_driver_probe(struct pci_dev *pdev, const struct
pci_device_id *ent)
if (ret)
goto out_cleanup_gem;
+ intel_pxp_init(i915);
+
i915_driver_register(i915);
enable_rpm_wakeref_asserts(>runtime_pm);
@@ -938,6 +940,8 @@ void i915_driver_remove(struct drm_i915_private *i915)
/* Flush any external code that still may be under the RCU lock */
synchronize_rcu();
+ intel_pxp_uninit(i915);
+
i915_gem_suspend(i915);
drm_atomic_helper_shutdown(>drm);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fecb5899cbac..33a3f5c387b0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -105,6 +105,8 @@
#include "intel_region_lmem.h"
+#include "pxp/intel_pxp.h"
+
/* General customization:
*/
@@ -1215,6 +1217,8 @@ struct drm_i915_private {
/* Mutex to protect the above hdcp component related values. */
struct mutex hdcp_comp_mutex;
+ struct intel_pxp pxp;
+
I915_SELFTEST_DECLARE(struct i915_selftest_stash selftest;)
/*
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
b/drivers/gpu/drm/i915/pxp/intel_pxp.c
new file mode 100644
index ..3de4593ca495
--- /dev/null
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright(c) 2020 Intel Corporation.
+ */
+
+#include "i915_drv.h"
+#include "intel_pxp.h"
+
+int intel_pxp_init(struct drm_i915_private *i915)
+{
+ if (!i915)
+ return -EINVAL;
+
+ /* PXP only available for GEN12+ */
+ if (INTEL_GEN(i915) < 12)
+ return 0;
+
+ drm_info(>drm, "i915 PXP is inited with i915=[%p]\n", i915);
+
+ return 0;
+}
+
+void intel_pxp_uninit(struct drm_i915_private *i915)
+{
+}
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h
b/drivers/gpu/drm/i915/pxp/intel_pxp.h
new file mode 100644
index ..0b83d33045f3
--- /dev/null
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier:
