From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id 478176EA58 for ; Fri, 10 Sep 2021 19:11:40 +0000 (UTC) Date: Fri, 10 Sep 2021 15:11:37 -0400 From: Rodrigo Vivi Message-ID: References: <20210902173043.821318-1-alan.previn.teres.alexis@intel.com> <20210902173043.821318-12-alan.previn.teres.alexis@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [igt-dev] [PATCH i-g-t v9 11/16] Verify execbuf fails with stale PXP buffer after teardown List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Alan Previn Cc: igt-dev@lists.freedesktop.org List-ID: On Fri, Sep 10, 2021 at 03:09:00PM -0400, Rodrigo Vivi wrote: > On Thu, Sep 02, 2021 at 10:30:38AM -0700, Alan Previn wrote: > > Add a check to verify that reusing a stale protected buffer > > in a gem_execbuff (with a protected context) after a teardown > > event fails with -ENOEXEC error. Trigger the teardown via the > > pxp invalidation debugfs that simulates a HW teardown IRQ. > > > > NOTE: The end-to-end architecture requirement includes that > > any break in the links of the PXP sessions needs to trigger a > > full teardown and the application needs to be made aware of that > > allowing it to re-establish the end-to-end pipeline of buffers, > > contexts and renders again if it chooses to. This stricter > > behavior targets only contexts created with PXP enabled. > > > > Signed-off-by: Alan Previn > > --- > > tests/i915/gem_pxp.c | 35 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 35 insertions(+) > > > > diff --git a/tests/i915/gem_pxp.c b/tests/i915/gem_pxp.c > > index 6884e0d7..fab8e28d 100644 > > --- a/tests/i915/gem_pxp.c > > +++ b/tests/i915/gem_pxp.c > > @@ -794,6 +794,39 @@ static void test_pxp_stale_ctx_execution(int i915) > > free_exec_assets(i915, &data); > > } > > > > +static void test_pxp_stale_buf_execution(int i915) > > +{ > > + int ret; > > + struct simple_exec_assets data = {0}; > > + uint32_t ctx2; > > + struct intel_bb *ibb2; > > + > > + /* Use pxp buffers with pxp context for testing for invalidation of protected buffers. */ > > + prepare_exec_assets(i915, &data, true, true); > > + ret = gem_execbuf_flush_store_dw(i915, data.ibb, data.ctx, data.fencebuf); > > + igt_assert(ret == 0); > > + > > + trigger_pxp_debugfs_forced_teardown(i915); > > + > > + /* > > + * After teardown, use a new pxp context but reuse the stale bo to ensure > > + * the kernel is catching the invalidated bo (not context) > > + */ > > + ret = create_ctx_with_params(i915, true, true, true, false, &ctx2); > > + igt_assert_eq(ret, 0); > > + igt_assert_eq(get_ctx_protected_param(i915, ctx2), 1); > > + ibb2 = intel_bb_create_with_context(i915, ctx2, 4096); > > + igt_assert(ibb2); > > + intel_bb_remove_intel_buf(data.ibb, data.fencebuf); > > + intel_bb_add_intel_buf(ibb2, data.fencebuf, true); > > + ret = gem_execbuf_flush_store_dw(i915, ibb2, ctx2, data.fencebuf); > > + igt_assert_f((ret == -ENOEXEC), "Executing stale pxp buffer didn't fail with -ENOEXEC\n"); > > I believe this is opposite as well... > Or I am getting confused... I am very confused myself... and crazy.. My bad! Of course, the message is to tell what happened if assertion failed! Duh! Sorry, Rodrigo. Ah, and Reviewed-by: Rodrigo Vivi > > other than this, everything looks good > > > + > > + intel_bb_destroy(ibb2); > > + gem_context_destroy(i915, ctx2); > > + free_exec_assets(i915, &data); > > +} > > + > > igt_main > > { > > int i915 = -1; > > @@ -885,6 +918,8 @@ igt_main > > test_pxp_pwrcycle_teardown_keychange(i915, &pm); > > igt_subtest("verify-pxp-stale-ctx-execution") > > test_pxp_stale_ctx_execution(i915); > > + igt_subtest("verify-pxp-stale-buf-execution") > > + test_pxp_stale_buf_execution(i915); > > } > > > > igt_fixture { > > -- > > 2.25.1 > >