Message ID | 20210106231223.8323-1-sean.z.huang@intel.com (mailing list archive) |
---|---|
Headers | show |
Series | Introduce Intel PXP component - Mesa single session | expand |
On Wed, 2021-01-06 at 15:12 -0800, Huang, Sean Z wrote: > PXP (Protected Xe Path) is an i915 component, available on > GEN12+ that helps to establish the hardware protected session > and manage the status of the alive software session, as well > as its life cycle. > > This patch series is to allow the kernel space to create and > manage a single hardware session (a.k.a. default session or > arbitrary session). So user can allocate the protected buffer, > which is encrypted with the leverage of the arbitrary hardware > session. > > v2: > - modification based on code review feedbacks received > - passing pxp instead of i915 as function argument > - remove dead code only for multi-session > - move the pxp init call from i915_drv.c to intel_gt.c > - remove the tautology naming > > v3: > - rebase to latest drm-tip > > v4: > - Append the split non-mesa patch sereis (commit #14 - #21) into > this patch series > > v5: > - include "intel_pxp.h" in intel_pxp_sm.h at commit #14 to fix > the build problem. > > v6: > - Fix the null pointer arb_session access bug in intel_pxp_arb.c > in > "04 [RFC-v5] drm/i915/pxp: Create the arbitrary session after > boot" > > v7: > - Use list_for_each_entry_safe instead of list_for_each_entry > > v8: > - Add MEI vtag support for PXP multi-session usage > > v9: > - Fix error handling bug in commit #5 "Func to send hardware > session > termination". In intel_pxp_cmd.c, we should properly assign > "err = PTR_ERR(x)" if hitting the error case "IS_ERR(x)", this > is > the only change in v9. > > v10 > - Remove the multi session commits #14-#21, for now we would like > to > keep the multi session patches as downstream. > - Adopt the code review suggestion from Wilson in commit #1 > > v11 > - In commit #05 "drm/i915/pxp: Func to send hardware session > termination", we should not assume VCS0 is always on. > Instead we use available VCS#, could be VCS0, VCS2, etc. > > v12 > - Add "#include <linux/mutex.h> in #1 intel_pxp_types.h > > v13 > - Add "#include <linux/mutex.h> in #1 intel_pxp_types.h (#v12 > didn't > actually update the _types.h file...) > > v14 > - Add "if (INTEL_GEN(gt->i915) < 12) return;" in #1 > intel_pxp_fini(), just skip for non gen12+ sku > > v15 > In #04: > - Make intel_pxp_arb_reserve_session() as static function to fix > the > sparse warning > - Update value of PXP_TEE_ARB_CMD_BIN > > v16 > In #04: > - Remove the binary from source code via defining the TEE command > header > > v17 > - In #04, directly return intel_pxp_tee_component_fini() if > pxp_tee_comp_added is off > > v18 > - In #09, Add intel_pxp_gem_context_set_protected() to check the > arb > session before setting protected flag for gem context > > - In #12, Replace i915_gem_context_set_protected() with > intel_pxp_gem_context_set_protected() to check whether the > arbitrary > session is alive > > v19 > - In #01, put intel_pxp_fini() in intel_gt_driver_remove() > instead of > intel_gt_driver_release() since intel_gt_driver_release() is > the last > stage of i915 unbind and we shouldn't call the component_del > here. > > - In #04, check if arbitrary session is alive before > intel_pxp_arb_create_session() Please reduce the amount of series revision and start using --in-reply- to so we can easily see what was addressed from the previous comments versus what is open. > > > Anshuman Gupta (1): > drm/i915/pxp: Add plane decryption support > > Bommu Krishnaiah (2): > drm/i915/uapi: introduce drm_i915_gem_create_ext > drm/i915/pxp: User interface for Protected buffer > > Huang, Sean Z (9): > drm/i915/pxp: Introduce Intel PXP component > drm/i915/pxp: set KCR reg init during the boot time > drm/i915/pxp: Implement funcs to create the TEE channel > drm/i915/pxp: Create the arbitrary session after boot > drm/i915/pxp: Func to send hardware session termination > drm/i915/pxp: Enable PXP irq worker and callback stub > drm/i915/pxp: Destroy arb session upon teardown > drm/i915/pxp: Enable PXP power management > drm/i915/pxp: Expose session state for display protection flip > > Vitaly Lubart (1): > mei: pxp: export pavp client to me client bus > > drivers/gpu/drm/i915/Kconfig | 22 ++ > drivers/gpu/drm/i915/Makefile | 9 + > drivers/gpu/drm/i915/display/intel_sprite.c | 21 +- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 19 +- > drivers/gpu/drm/i915/gem/i915_gem_context.h | 5 + > .../gpu/drm/i915/gem/i915_gem_context_types.h | 2 +- > .../gpu/drm/i915/gem/i915_gem_object_types.h | 5 + > drivers/gpu/drm/i915/gt/intel_gt.c | 5 + > drivers/gpu/drm/i915/gt/intel_gt_irq.c | 4 + > drivers/gpu/drm/i915/gt/intel_gt_pm.c | 4 + > drivers/gpu/drm/i915/gt/intel_gt_types.h | 3 + > drivers/gpu/drm/i915/i915_drv.c | 7 +- > drivers/gpu/drm/i915/i915_drv.h | 6 + > drivers/gpu/drm/i915/i915_gem.c | 63 +++- > drivers/gpu/drm/i915/i915_reg.h | 4 +- > drivers/gpu/drm/i915/pxp/intel_pxp.c | 181 ++++++++++++ > drivers/gpu/drm/i915/pxp/intel_pxp.h | 81 +++++ > drivers/gpu/drm/i915/pxp/intel_pxp_arb.c | 207 +++++++++++++ > drivers/gpu/drm/i915/pxp/intel_pxp_arb.h | 17 ++ > drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c | 278 > ++++++++++++++++++ > drivers/gpu/drm/i915/pxp/intel_pxp_cmd.h | 20 ++ > drivers/gpu/drm/i915/pxp/intel_pxp_context.c | 28 ++ > drivers/gpu/drm/i915/pxp/intel_pxp_context.h | 15 + > drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 65 ++++ > drivers/gpu/drm/i915/pxp/intel_pxp_pm.h | 31 ++ > drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 210 +++++++++++++ > drivers/gpu/drm/i915/pxp/intel_pxp_tee.h | 17 ++ > drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 60 ++++ > drivers/misc/mei/Kconfig | 2 + > drivers/misc/mei/Makefile | 1 + > drivers/misc/mei/pxp/Kconfig | 13 + > drivers/misc/mei/pxp/Makefile | 7 + > drivers/misc/mei/pxp/mei_pxp.c | 230 +++++++++++++++ > drivers/misc/mei/pxp/mei_pxp.h | 18 ++ > include/drm/i915_component.h | 1 + > include/drm/i915_pxp_tee_interface.h | 45 +++ > include/uapi/drm/i915_drm.h | 66 +++++ > 37 files changed, 1760 insertions(+), 12 deletions(-) > create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp.c > create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp.h > create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_arb.c > create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_arb.h > create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c > create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_cmd.h > create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_context.c > create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_context.h > create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_pm.h > create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c > create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_tee.h > create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_types.h > create mode 100644 drivers/misc/mei/pxp/Kconfig > create mode 100644 drivers/misc/mei/pxp/Makefile > create mode 100644 drivers/misc/mei/pxp/mei_pxp.c > create mode 100644 drivers/misc/mei/pxp/mei_pxp.h > create mode 100644 include/drm/i915_pxp_tee_interface.h >
On Wed, 2021-01-06 at 23:47 +0000, Patchwork wrote: > == Series Details == > > Series: Introduce Intel PXP component - Mesa single session (rev19) > URL : https://patchwork.freedesktop.org/series/84620/ > State : warning > > == Summary == > > $ dim checkpatch origin/drm-tip > fbdd4e2e287d drm/i915/pxp: Introduce Intel PXP component > -:119: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), > does MAINTAINERS need updating? > #119: > new file mode 100644 > > total: 0 errors, 1 warnings, 0 checks, 194 lines checked > fdb764188c85 drm/i915/pxp: set KCR reg init during the boot time > 20c2f538fd26 drm/i915/pxp: Implement funcs to create the TEE channel > -:8: WARNING:TYPO_SPELLING: 'defualt' may be misspelled - perhaps > 'default'? > #8: > (defualt) session. > ^^^^^^^ > > -:85: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), > does MAINTAINERS need updating? > #85: > new file mode 100644 > > total: 0 errors, 2 warnings, 0 checks, 253 lines checked > e77ddc43854a drm/i915/pxp: Create the arbitrary session after boot > -:68: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), > does MAINTAINERS need updating? > #68: > new file mode 100644 > > total: 0 errors, 1 warnings, 0 checks, 330 lines checked > 60d904942ebb drm/i915/pxp: Func to send hardware session termination > -:53: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), > does MAINTAINERS need updating? > #53: > new file mode 100644 > > total: 0 errors, 1 warnings, 0 checks, 222 lines checked > e06c62d5fd8d drm/i915/pxp: Enable PXP irq worker and callback stub > -:51: WARNING:LONG_LINE_COMMENT: line length of 113 exceeds 100 > columns > #51: FILE: drivers/gpu/drm/i915/i915_reg.h:7970: > +#define GEN11_CRYPTO_INTR_MASK _MMIO(0x1900f0) /* crypto > mask is in bit31-16 (Engine1 Interrupt Mask) */ > > total: 0 errors, 1 warnings, 0 checks, 230 lines checked > 2b451866410d drm/i915/pxp: Destroy arb session upon teardown > f74cff978a9a drm/i915/pxp: Enable PXP power management > -:78: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), > does MAINTAINERS need updating? > #78: > new file mode 100644 > > total: 0 errors, 1 warnings, 0 checks, 148 lines checked > f45f6bf27787 drm/i915/pxp: Expose session state for display > protection flip > ed1733ee9985 mei: pxp: export pavp client to me client bus > -:32: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), > does MAINTAINERS need updating? > #32: > new file mode 100644 > > total: 0 errors, 1 warnings, 0 checks, 277 lines checked > 5ed5ea72d630 drm/i915/uapi: introduce drm_i915_gem_create_ext > -:12: ERROR:BAD_SIGN_OFF: Unrecognized email address: 'Joonas > Lahtinen joonas.lahtinen@linux.intel.com' > #12: > Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com Please make sure you address some of the checkpatch complains like this > > -:13: ERROR:BAD_SIGN_OFF: Unrecognized email address: 'Matthew Auld > matthew.auld@intel.com' > #13: > Cc: Matthew Auld matthew.auld@intel.com this > > -:46: ERROR:CODE_INDENT: code indent should use tabs where possible > #46: FILE: drivers/gpu/drm/i915/i915_gem.c:265: > + struct drm_i915_private *i915;$ this > > -:46: WARNING:LEADING_SPACE: please, no spaces at the start of a line > #46: FILE: drivers/gpu/drm/i915/i915_gem.c:265: > + struct drm_i915_private *i915;$ > this > -:50: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open > parenthesis > #50: FILE: drivers/gpu/drm/i915/i915_gem.c:269: > +static int __create_setparam(struct drm_i915_gem_object_param *args, > + struct > create_ext *ext_data) > this > -:95: CHECK:LINE_SPACING: Please don't use multiple blank lines > #95: FILE: drivers/gpu/drm/i915/i915_gem.c:317: > + > + this > -:107: WARNING:LONG_LINE: line length of 120 exceeds 100 columns > #107: FILE: include/uapi/drm/i915_drm.h:395: > +#define DRM_IOCTL_I915_GEM_CREATE_EXT DRM_IOWR(DRM_COMMAND_BASE + > DRM_I915_GEM_CREATE, struct drm_i915_gem_create_ext) > > -:155: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > #155: FILE: include/uapi/drm/i915_drm.h:1736: > +#define I915_OBJECT_PARAM (1ull<<32) this > ^ > > total: 3 errors, 2 warnings, 3 checks, 136 lines checked > ae1f0edf901f drm/i915/pxp: User interface for Protected buffer > 7ce48b165a12 drm/i915/pxp: Add plane decryption support > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Vivi, Rodrigo (2021-01-07 17:42:12) > On Wed, 2021-01-06 at 15:12 -0800, Huang, Sean Z wrote: > > PXP (Protected Xe Path) is an i915 component, available on > > GEN12+ that helps to establish the hardware protected session > > and manage the status of the alive software session, as well > > as its life cycle. > > > > This patch series is to allow the kernel space to create and > > manage a single hardware session (a.k.a. default session or > > arbitrary session). So user can allocate the protected buffer, > > which is encrypted with the leverage of the arbitrary hardware > > session. > > > > v2: > > - modification based on code review feedbacks received > > - passing pxp instead of i915 as function argument > > - remove dead code only for multi-session > > - move the pxp init call from i915_drv.c to intel_gt.c > > - remove the tautology naming > > > > v3: > > - rebase to latest drm-tip > > > > v4: > > - Append the split non-mesa patch sereis (commit #14 - #21) into > > this patch series > > > > v5: > > - include "intel_pxp.h" in intel_pxp_sm.h at commit #14 to fix > > the build problem. > > > > v6: > > - Fix the null pointer arb_session access bug in intel_pxp_arb.c > > in > > "04 [RFC-v5] drm/i915/pxp: Create the arbitrary session after > > boot" > > > > v7: > > - Use list_for_each_entry_safe instead of list_for_each_entry > > > > v8: > > - Add MEI vtag support for PXP multi-session usage > > > > v9: > > - Fix error handling bug in commit #5 "Func to send hardware > > session > > termination". In intel_pxp_cmd.c, we should properly assign > > "err = PTR_ERR(x)" if hitting the error case "IS_ERR(x)", this > > is > > the only change in v9. > > > > v10 > > - Remove the multi session commits #14-#21, for now we would like > > to > > keep the multi session patches as downstream. > > - Adopt the code review suggestion from Wilson in commit #1 > > > > v11 > > - In commit #05 "drm/i915/pxp: Func to send hardware session > > termination", we should not assume VCS0 is always on. > > Instead we use available VCS#, could be VCS0, VCS2, etc. > > > > v12 > > - Add "#include <linux/mutex.h> in #1 intel_pxp_types.h > > > > v13 > > - Add "#include <linux/mutex.h> in #1 intel_pxp_types.h (#v12 > > didn't > > actually update the _types.h file...) > > > > v14 > > - Add "if (INTEL_GEN(gt->i915) < 12) return;" in #1 > > intel_pxp_fini(), just skip for non gen12+ sku > > > > v15 > > In #04: > > - Make intel_pxp_arb_reserve_session() as static function to fix > > the > > sparse warning > > - Update value of PXP_TEE_ARB_CMD_BIN > > > > v16 > > In #04: > > - Remove the binary from source code via defining the TEE command > > header > > > > v17 > > - In #04, directly return intel_pxp_tee_component_fini() if > > pxp_tee_comp_added is off > > > > v18 > > - In #09, Add intel_pxp_gem_context_set_protected() to check the > > arb > > session before setting protected flag for gem context > > > > - In #12, Replace i915_gem_context_set_protected() with > > intel_pxp_gem_context_set_protected() to check whether the > > arbitrary > > session is alive > > > > v19 > > - In #01, put intel_pxp_fini() in intel_gt_driver_remove() > > instead of > > intel_gt_driver_release() since intel_gt_driver_release() is > > the last > > stage of i915 unbind and we shouldn't call the component_del > > here. > > > > - In #04, check if arbitrary session is alive before > > intel_pxp_arb_create_session() > > Please reduce the amount of series revision and start using --in-reply- > to so we can easily see what was addressed from the previous comments > versus what is open. CI doesn't always work nicely with --in-reply-to :/ The changes made to each patch should be noted in the commit message of the patch, not just in the cover letter. And level of detail of the changes doesn't need to be verbose. That way when reviewing v10 after v9, reviewer can also can through patches which have v10 change entry in them. Use trybot instead of the mailing list to catch things like missing includes, so those should not result in new revisions. Only send to the mailing list a new revision that you have checked to compile and fully work. But most importantly, I notice plenty of previously given feedback that is unaddressed (type0/type1, unnecessary variables like session type etc. that can only have single value in this series). So all of the previous feedback needs addressing before continuing the review. There is lot to be fixed about the coding style to match surroundings, but that only makes sense as the last step after the series is otherwise ready. Regards, Joonas