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@xxxxxxxxxxxxxxx>; Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: [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@xxxxxxxxxxxxxxx>
> Sent: Monday, December 7, 2020 2:01 AM
> To: Huang, Sean Z <sean.z.huang@xxxxxxxxx>; Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
