[igt-dev] [PATCH i-g-t 03/16] Add basic PXP testing of buffer and context alloc
Teres Alexis, Alan Previn
alan.previn.teres.alexis at intel.com
Fri May 14 09:40:26 UTC 2021
On Fri, 2021-05-14 at 11:00 +0200, Michal Wajdeczko wrote:
>
> On 14.05.2021 08:49, Alan Previn wrote:
> > Test PXP capability support as well as the allocation of protected
> > buffers and protected contexts.
> >
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis at intel.com>
> > ---
> > tests/i915/gem_pxp.c | 366
> > +++++++++++++++++++++++++++++++++++++++++++
> > tests/meson.build | 1 +
> > 2 files changed, 367 insertions(+)
> > create mode 100644 tests/i915/gem_pxp.c
> >
> > diff --git a/tests/i915/gem_pxp.c b/tests/i915/gem_pxp.c
> > new file mode 100644
> > index 00000000..6340daae
> > --- /dev/null
> > +++ b/tests/i915/gem_pxp.c
> > @@ -0,0 +1,366 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2021 Intel Corporation
> > + */
> > +
> > +#include <sys/ioctl.h>
> > +
> > +#include "igt.h"
> > +#include "i915/gem.h"
> > +
> > +IGT_TEST_DESCRIPTION("Test PXP that manages protected content
> > through arbitrated HW-PXP-session");
> > +/* Note: PXP = "Protected Xe Path" */
> > +
> > +/* test-configs for protected buffer creation*/
> > +#define BO_ALLOC_NO_HW_SUPPORT 1
> > +#define BO_ALLOC_PROTECT_ON 2
> > +#define BO_ALLOC_PROTECT_OFF 3
> > +
> > +/* test-configs for protected context creation*/
> > +#define CTX_ALLOC_NO_HW_SUPPORT 1
> > +#define CTX_ALLOC_RECOVER_OFF_PROTECT_OFF 2
> > +#define CTX_ALLOC_RECOVER_OFF_PROTECT_ON 3
> > +#define CTX_ALLOC_RECOVER_ON_PROTECT_OFF 4
> > +#define CTX_ALLOC_RECOVER_ON_PROTECT_ON 5
> > +#define CTX_MODIFY_PROTECTED_RECOVER_OFF_TO_ON 6
> > +#define CTX_MODIFY_PROTECTED_PROTECT_ON_TO_OFF 7
> > +#define CTX_MODIFY_PROTECTED_BOTHPARAMS_TO_INVALID 8
> > +#define CTX_MODIFY_UNPROTECTED_BOTHPARAMS_TO_VALID 9
>
> why not use enum instead?
>
> > +
> > +static bool is_pxp_hw_supported(int i915)
> > +{
> > + uint32_t devid = intel_get_drm_devid(i915);
> > + const struct intel_device_info *devinfo =
> > intel_get_device_info(devid);
>
> can intel_get_device_info really return NULL ?
>
> > +
> > + if (!devinfo) {
> > + igt_info("No device info found - assume no PXP
> > support.\n");
> > + return false;
> > + }
> > + if (devinfo->is_tigerlake || devinfo->is_rocketlake ||
> > + devinfo->is_alderlake_s) {
> > + return true;
>
> shouldn't IS_TIGERLAKE and similar be used instead ?
>
> > + }
> > +
> > + return false;
> > +}
> > +
> > +static uint32_t create_protected_bo(int i915, uint32_t size, bool
> > protected_is_true,
> > + int expected_return)
> > +{
> > + int ret;
> > +
> > + struct drm_i915_gem_create_ext_protected_content protected_ext
> > = {
> > + .base = { .name = I915_GEM_CREATE_EXT_PROTECTED_CONTENT
> > },
> > + .flags = 0,
> > + };
> > +
> > + struct drm_i915_gem_create_ext create_ext = {
> > + .size = size,
> > + .extensions = 0,
> > + };
> > +
> > + if (protected_is_true)
> > + create_ext.extensions = (uintptr_t)&protected_ext;
>
> hmm, if call to "create_protected" can be used to create
> "unprotected"
> bo, then maybe function shall be named as "create_bo_ext" ?
>
> > +
> > + ret = ioctl(i915, DRM_IOCTL_I915_GEM_CREATE_EXT, &create_ext);
> > + igt_assert_eq(ret, expected_return);
>
> hmm, usually such asserts are done at caller's level
> so maybe this function shall have signature like:
>
> int create_protected_bo(int i915, uint32_t size, uint32_t *bo_out)
>
> > +
> > + return create_ext.handle;
> > +}
> > +
> > +static void test_create_protected_buffer(int i915, uint32_t
> > test_cfg)
> > +{
> > + uint32_t bo;
> > +
> > + switch (test_cfg) {
> > + case BO_ALLOC_NO_HW_SUPPORT:
> > + bo = create_protected_bo(i915, 4096, false, 0);
>
> igt_assert(bo) ?
>
> > + gem_close(i915, bo);
> > + bo = create_protected_bo(i915, 4096, true, -ENODEV);
> > + igt_assert_eq(bo, 0);
> > + break;
> > +
> > + case BO_ALLOC_PROTECT_OFF:
> > + bo = create_protected_bo(i915, 4096, false, 0);
> > + gem_close(i915, bo);
> > + break;
> > +
> > + case BO_ALLOC_PROTECT_ON:
> > + bo = create_protected_bo(i915, 4096, true, 0);
> > + gem_close(i915, bo);
> > + break;
> > +
> > + default:
> > + igt_info("Skipping unknown buffer test_cfg = %d\n",
> > test_cfg);
> > + break;
> > + }
> > +}
> > +
> > +static int create_ext_ioctl(int i915, struct
> > drm_i915_gem_context_create_ext *arg)
>
> I guess you should rather export this function from lib, not copy it
>
> > +{
> > + int err;
> > +
> > + err = 0;
> > + if (igt_ioctl(i915, DRM_IOCTL_I915_GEM_CONTEXT_CREATE_EXT,
> > arg)) {
> > + err = -errno;
> > + igt_assume(err);
> > + }
> > +
> > + errno = 0;
> > + return err;
> > +}
> > +
> > +static uint32_t create_protected_ctx(int i915, bool
> > with_protected_param, bool protected_is_true,
> > + bool with_recoverable_param, bool
> > recoverable_is_true,
> > + int expected_return)
> > +{
> > + int ret;
> > +
> > + struct drm_i915_gem_context_create_ext_setparam p_prot = {
> > + .base = {
> > + .name = I915_CONTEXT_CREATE_EXT_SETPARAM,
> > + .next_extension = 0,
> > + },
> > + .param = {
> > + .param = I915_CONTEXT_PARAM_PROTECTED_CONTENT,
> > + .value = 0,
> > + }
> > + };
> > + struct drm_i915_gem_context_create_ext_setparam p_norecover = {
> > + .base = {
> > + .name = I915_CONTEXT_CREATE_EXT_SETPARAM,
> > + .next_extension = 0,
> > + },
> > + .param = {
> > + .param = I915_CONTEXT_PARAM_RECOVERABLE,
> > + .value = 0,
> > + }
> > + };
> > + struct drm_i915_gem_context_create_ext create = {
> > + .flags = I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS,
> > + .extensions = 0,
> > + };
> > +
> > + p_prot.param.value = protected_is_true;
> > + p_norecover.param.value = recoverable_is_true;
> > +
> > + if (with_protected_param && with_recoverable_param) {
> > + create.extensions =
> > to_user_pointer(&(p_norecover.base));
> > + p_norecover.base.next_extension =
> > to_user_pointer(&(p_prot.base));
> > + } else if (!with_protected_param && with_recoverable_param) {
> > + create.extensions =
> > to_user_pointer(&(p_norecover.base));
> > + p_norecover.base.next_extension = 0;
> > + } else if (with_protected_param && !with_recoverable_param) {
> > + create.extensions = to_user_pointer(&(p_prot.base));
> > + p_prot.base.next_extension = 0;
> > + } else if (!with_protected_param && !with_recoverable_param) {
> > + create.flags = 0;
> > + }
> > +
> > + ret = create_ext_ioctl(i915, &create);
> > + igt_assert_eq(ret, expected_return);
> > +
> > + return create.ctx_id;
> > +}
> > +
> > +#define CHANGE_PARAM_PROTECTED 0x0001
> > +#define CHANGE_PARAM_RECOVERY 0x0002
> > +static void modify_ctx_protected_param(int i915, uint32_t ctx_id,
> > uint32_t change_param_mask,
> > + bool param_value, int
> > expected_return)
> > +{
> > + int ret;
> > +
> > + struct drm_i915_gem_context_param ctx_param = {
> > + .ctx_id = ctx_id,
> > + .param = 0,
> > + .value = 0,
> > + };
> > +
> > + if (change_param_mask == CHANGE_PARAM_PROTECTED) {
> > + ctx_param.param = I915_CONTEXT_PARAM_PROTECTED_CONTENT;
> > + ctx_param.value = (int)param_value;
> > + } else if (change_param_mask == CHANGE_PARAM_RECOVERY) {
> > + ctx_param.param = I915_CONTEXT_PARAM_RECOVERABLE;
> > + ctx_param.value = (int)param_value;
> > + } else {
> > + return;
> > + }
> > +
> > + ret = ioctl(i915, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM,
> > &ctx_param);
> > +
> > + igt_assert_eq(ret, expected_return);
> > +}
> > +
> > +static void assert_ctx_protected_param(int i915, uint32_t ctx_id,
> > bool is_protected)
> > +{
> > + int ret;
> > +
> > + struct drm_i915_gem_context_param ctx_param = {
> > + .ctx_id = ctx_id,
> > + .param = I915_CONTEXT_PARAM_PROTECTED_CONTENT,
> > + };
> > +
> > + ret = ioctl(i915, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM,
> > &ctx_param);
> > + igt_assert_eq(ret, 0);
> > + igt_assert_eq(ctx_param.value, (int)is_protected);
> > +}
> > +
> > +static void assert_ctx_recovery_param(int i915, uint32_t ctx_id,
> > bool is_recoverable)
> > +{
> > + int ret;
> > +
> > + struct drm_i915_gem_context_param ctx_param = {
> > + .ctx_id = ctx_id,
> > + .param = I915_CONTEXT_PARAM_RECOVERABLE,
> > + };
> > +
> > + ret = ioctl(i915, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM,
> > &ctx_param);
> > + igt_assert_eq(ret, 0);
> > + igt_assert_eq(ctx_param.value, (int)is_recoverable);
> > +}
> > +
> > +static void test_create_protected_context(int i915, uint32_t
> > test_cfg)
> > +{
> > + uint32_t ctx;
> > +
> > + switch (test_cfg) {
> > + case CTX_ALLOC_NO_HW_SUPPORT:
> > + ctx = create_protected_ctx(i915, true, true, true,
> > false, -ENODEV);
> > + igt_assert_eq(ctx, 0);
> > + case CTX_ALLOC_RECOVER_OFF_PROTECT_OFF:
> > + ctx = create_protected_ctx(i915, true, false, true,
> > false, 0);
> > + assert_ctx_protected_param(i915, ctx, false);
> > + assert_ctx_recovery_param(i915, ctx, false);
> > + gem_context_destroy(i915, ctx);
> > + break;
> > +
> > + case CTX_ALLOC_RECOVER_OFF_PROTECT_ON:
> > + ctx = create_protected_ctx(i915, true, true, true,
> > false, 0);
> > + assert_ctx_protected_param(i915, ctx, true);
> > + assert_ctx_recovery_param(i915, ctx, false);
> > + gem_context_destroy(i915, ctx);
> > + break;
> > +
> > + case CTX_ALLOC_RECOVER_ON_PROTECT_OFF:
> > + ctx = create_protected_ctx(i915, true, false, true,
> > true, 0);
> > + assert_ctx_protected_param(i915, ctx, false);
> > + assert_ctx_recovery_param(i915, ctx, true);
> > + gem_context_destroy(i915, ctx);
> > + break;
> > +
> > + case CTX_ALLOC_RECOVER_ON_PROTECT_ON:
> > + ctx = create_protected_ctx(i915, true, true, true,
> > true, -EPERM);
> > + igt_assert_eq(ctx, 0);
> > + ctx = create_protected_ctx(i915, true, true, false,
> > false, -EPERM);
> > + igt_assert_eq(ctx, 0);
> > + break;
> > +
> > + case CTX_MODIFY_PROTECTED_RECOVER_OFF_TO_ON:
> > + ctx = create_protected_ctx(i915, true, true, true,
> > false, 0);
> > + assert_ctx_protected_param(i915, ctx, true);
> > + assert_ctx_recovery_param(i915, ctx, false);
> > + modify_ctx_protected_param(i915, ctx,
> > CHANGE_PARAM_RECOVERY, true, -EPERM);
> > + assert_ctx_recovery_param(i915, ctx, false);
> > + gem_context_destroy(i915, ctx);
> > + break;
> > +
> > + case CTX_MODIFY_PROTECTED_PROTECT_ON_TO_OFF:
> > + ctx = create_protected_ctx(i915, true, true, true,
> > false, 0);
> > + assert_ctx_protected_param(i915, ctx, true);
> > + assert_ctx_recovery_param(i915, ctx, false);
> > + modify_ctx_protected_param(i915, ctx,
> > CHANGE_PARAM_PROTECTED, false, -EPERM);
> > + assert_ctx_protected_param(i915, ctx, true);
> > + assert_ctx_recovery_param(i915, ctx, false);
> > + gem_context_destroy(i915, ctx);
> > + break;
> > +
> > + case CTX_MODIFY_PROTECTED_BOTHPARAMS_TO_INVALID:
> > + ctx = create_protected_ctx(i915, true, true, true,
> > false, 0);
> > + assert_ctx_protected_param(i915, ctx, true);
> > + assert_ctx_recovery_param(i915, ctx, false);
> > + modify_ctx_protected_param(i915, ctx,
> > CHANGE_PARAM_RECOVERY, true, -EPERM);
> > + modify_ctx_protected_param(i915, ctx,
> > CHANGE_PARAM_PROTECTED, false, -EPERM);
> > + assert_ctx_protected_param(i915, ctx, true);
> > + assert_ctx_recovery_param(i915, ctx, false);
> > + gem_context_destroy(i915, ctx);
> > + break;
> > +
> > + case CTX_MODIFY_UNPROTECTED_BOTHPARAMS_TO_VALID:
> > + ctx = create_protected_ctx(i915, false, false, false,
> > false, 0);
> > + assert_ctx_protected_param(i915, ctx, false);
> > + assert_ctx_recovery_param(i915, ctx, true);
> > + modify_ctx_protected_param(i915, ctx,
> > CHANGE_PARAM_RECOVERY, false, 0);
> > + modify_ctx_protected_param(i915, ctx,
> > CHANGE_PARAM_PROTECTED, true, -EPERM);
> > + assert_ctx_protected_param(i915, ctx, false);
> > + assert_ctx_recovery_param(i915, ctx, false);
> > + gem_context_destroy(i915, ctx);
> > + break;
> > +
> > + default:
> > + igt_info("Skipping unknown context test_cfg = %d\n",
> > test_cfg);
> > + break;
>
> as there is nothing common between all these test configs, why bother
> with yet another dispatcher function with this giant switch
> statement?
>
> note that igt_main is dispatching tests anyway...
>
> IMHO better to use set of dedicated functions:
>
> test_ctx_no_hw_support()
> test_ctx_alloc_recover_off_protect_of()
> ...
>
>
> > + }
> > +}
> > +
> > +igt_main
> > +{
> > + int i915 = -1;
> > + bool pxp_supported = false;
> > +
> > + igt_fixture
> > + {
> > + i915 = drm_open_driver(DRIVER_INTEL);
> > + igt_require(i915);
> > + igt_require_gem(i915);
> > + pxp_supported = is_pxp_hw_supported(i915);
> > + }
> > +
> > + igt_subtest_group {
> > + igt_fixture {
> > + igt_require((pxp_supported == 0));
> > + }
> > +
> > + igt_describe("Verify protected buffer on unsupported
> > hw:");
> > + igt_subtest("hw-rejects-pxp-buffer")
> > + test_create_protected_buffer(i915,
> > BO_ALLOC_NO_HW_SUPPORT);
> > + igt_describe("Verify protected context on unsupported
> > hw:");
> > + igt_subtest("hw-rejects-pxp-context")
> > + test_create_protected_context(i915,
> > CTX_ALLOC_NO_HW_SUPPORT);
> > + }
> > +
> > + igt_subtest_group {
> > + igt_fixture {
> > + igt_require(pxp_supported);
> > + }
> > +
> > + igt_describe("Verify protected buffer on supported
> > hw:");
> > + igt_subtest("create-regular-buffer")
> > + test_create_protected_buffer(i915,
> > BO_ALLOC_PROTECT_OFF);
> > + igt_subtest("create-protected-buffer")
> > + test_create_protected_buffer(i915,
> > BO_ALLOC_PROTECT_ON);
> > +
> > + igt_describe("Verify protected context on supported
> > hw:");
> > + igt_subtest("create-regular-context-1")
> > + test_create_protected_context(i915,
> > CTX_ALLOC_RECOVER_OFF_PROTECT_OFF);
> > + igt_subtest("create-regular-context-2")
> > + test_create_protected_context(i915,
> > CTX_ALLOC_RECOVER_ON_PROTECT_OFF);
> > + igt_subtest("fail-invalid-protected-context")
> > + test_create_protected_context(i915,
> > CTX_ALLOC_RECOVER_ON_PROTECT_ON);
> > + igt_subtest("create-valid-protected-context")
> > + test_create_protected_context(i915,
> > CTX_ALLOC_RECOVER_OFF_PROTECT_ON);
> > +
> > + igt_describe("Verify protected context integrity:");
> > + igt_subtest("reject-modify-context-protection-on")
> > + test_create_protected_context(i915,
> > CTX_MODIFY_UNPROTECTED_BOTHPARAMS_TO_VALID);
> > + igt_subtest("reject-modify-context-protection-off-1")
> > + test_create_protected_context(i915,
> > CTX_MODIFY_PROTECTED_RECOVER_OFF_TO_ON);
> > + igt_subtest("reject-modify-context-protection-off-2")
> > + test_create_protected_context(i915,
> > CTX_MODIFY_PROTECTED_PROTECT_ON_TO_OFF);
> > + igt_subtest("reject-modify-context-protection-off-3")
> > + test_create_protected_context(i915,
> > CTX_MODIFY_PROTECTED_BOTHPARAMS_TO_INVALID);
> > + }
> > +
> > + igt_fixture {
> > + close(i915);
> > + }
> > +}
> > diff --git a/tests/meson.build b/tests/meson.build
> > index 19cc4ebe..e0bff4d1 100644
> > --- a/tests/meson.build
> > +++ b/tests/meson.build
> > @@ -187,6 +187,7 @@ i915_progs = [
> > 'gem_pread_after_blit',
> > 'gem_pwrite',
> > 'gem_pwrite_snooped',
> > + 'gem_pxp',
> > 'gem_read_read_speed',
> > 'gem_readwrite',
> > 'gem_reg_read',
> >
Okay - i will fix them all and push a new rev.
...alan
More information about the igt-dev
mailing list