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.
<SNIP>
> +++ 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(&i915->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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
