[Intel-gfx] [RFC-v1 01/16] drm/i915/pxp: Introduce Intel PXP component

[Huang, Sean Z]

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 <joonas.lahtinen at linux.intel.com> 
Sent: Monday, December 7, 2020 2:01 AM
To: Huang, Sean Z <sean.z.huang at intel.com>; Intel-gfx at 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.

<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