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

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

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 <joonas.lahtinen@linux.intel.com>; 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 <joonas.lahtinen@linux.intel.com>
> Sent: Monday, December 7, 2020 2:01 AM
> To: Huang, Sean Z <sean.z.huang@intel.com>; 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.
> 
> <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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx