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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

Quoting Huang, Sean Z (2020-11-15 23:07:49)
> 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.

We should add a link and/or description here to the Open Source
userspace component that will be using this.

> Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com>

<SNIP>

> +++ 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

This should be a compile-time option, preferrably a sub-module
like GVT is trending.

> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -105,6 +105,8 @@
>  
>  #include "intel_region_lmem.h"
>  
> +#include "pxp/intel_pxp.h"

Only include the file in a more scoped fashion where it is needed,
we're trying to avoid everything to be included everywhere through
i915_drv.h header.

> +++ 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");

The messages should be human readable and bit informative.

> +++ 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;
> +};

This hunk is somewhat premature to add in uAPI header without any IOCTL.
The uAPI bits should always be added by the last patches in the series
so that bisecting will not break.

Based on the commit message, this aims to be a general purpose multiplexer
IOCTL which is discouraged.

PS. "pxp_" is tautology in variable naming when the struct is pxp_ops.

Regards, Joonas

> +
>  struct drm_i915_reg_read {
>         /*
>          * Register offset.
> -- 
> 2.17.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx