Re: [Intel-gfx] [PXP CLEAN PATCH v06 01/27] drm/i915/pxp: Introduce Intel PXP component

Souza, Jose
2020-11-16T14:24:50Z

On Fri, 2020-11-13 at 16:36 -0800, Sean Z Huang wrote:
> From: "Huang, Sean Z" <sean.z.huang@intel.com>
>
> PXP (Protected Xe Path) is an i915 componment, that
> helps ring3 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 ring3 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 <sean.z.huang@intel.com>
> ---
>  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 | 20 ++++++++++++++++++++
>  drivers/gpu/drm/i915/pxp/intel_pxp.h | 22 ++++++++++++++++++++++
>  include/uapi/drm/i915_drm.h | 5 +++++
>  6 files changed, 59 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/Makefile b/drivers/gpu/drm/i915/Makefile
> index e5574e506a5c..8274fea96009 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-y += \
> + 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 f2389ba49c69..c8b9c42fcbd6 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(&i915->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(&i915->drm);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 15be8debae54..f34ed07a68ee 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 000000000000..a469c55e3e54
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -0,0 +1,20 @@
> +/* 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)
> +{
> + int ret;
> +
> + drm_info(&i915->drm, "i915_pxp_init\n");

why info? this should be debug in my opinion, we don't print info for every component initialized.

> +
> + return ret;

Returning not initialized variable.

> +}
> +
> +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 000000000000..578f1126bada
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __INTEL_PXP_H__
> +#define __INTEL_PXP_H__
> +
> +#include <drm/drm_file.h>
> +
> +struct pxp_context;
> +
> +struct intel_pxp {
> + struct pxp_context *r0ctx;
> +};
> +
> +struct drm_i915_private;
> +
> +int intel_pxp_init(struct drm_i915_private *i915);
> +void intel_pxp_uninit(struct drm_i915_private *i915);
> +
> +#endif
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index fa1f3d62f9a6..dc101264176b 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1898,6 +1898,11 @@ struct drm_i915_gem_vm_control {
>   __u32 vm_id;
>  };
>
>
>
>
>
>
>
>
> +struct drm_i915_pxp_ops {
> + __u64 pxp_info_ptr;
> + __u32 pxp_info_size;

new users should use only "u64", "u32"...

> +};
> +
>  struct drm_i915_reg_read {
>   /*
>   * Register offset.