From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by gabe.freedesktop.org (Postfix) with ESMTPS id 73E7A6E5D1 for ; Tue, 29 Jun 2021 03:53:59 +0000 (UTC) Date: Mon, 28 Jun 2021 20:53:56 -0700 Message-ID: <87sg1172cr.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <20210617191256.577244-50-jason@jlekstrand.net> References: <20210617191256.577244-1-jason@jlekstrand.net> <20210617191256.577244-50-jason@jlekstrand.net> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Subject: Re: [igt-dev] [PATCH i-g-t 49/79] tests/i915/gem_exec_balancer: Don't reset engines on a context List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Jason Ekstrand Cc: igt-dev@lists.freedesktop.org List-ID: On Thu, 17 Jun 2021 12:12:56 -0700, Jason Ekstrand wrote: > > Instead of resetting the set of engines to break implicit dependencies, > just use a new context. This does drop a theoretical bit of test > coverage in the bonded chain tests because we can't drop the timeline > easily. I don't know enough to comment on the seriousness of this. If this scenario is of interest would you know if this is being tested elsewhere? I guess the original code could "drop the timeline easily" through a set_load_balancer() call on the context which we cannot do after this series. Or is the timeline being replaced/dropped a different way? > However, those tests also use the FENCE_SUBMIT pattern in a way that the > set of engines is swapped out between the first and second half of the > bonded pair. That seems pretty sketchy. I did not understand this comment either. As I mention above I thought this is the way to replace/drop the timeline but maybe I am wrong. The patch itself is fine. If you think the above issues are ok, I can give a R-b after I hear from you. Thanks. > @@ -1067,11 +1072,12 @@ static void __bonded_sema(int i915, uint32_t ctx, > struct drm_i915_gem_execbuffer2 execbuf = { > .buffers_ptr = to_user_pointer(&batch), > .buffer_count = 1, > - .rsvd1 = ctx, > }; > igt_spin_t *spin; > > for (int i = 0; i < ARRAY_SIZE(priorities); i++) { > + uint32_t ctx; > + > /* A: spin forever on seperate render engine */ > spin = igt_spin_new(i915, > .flags = (IGT_SPIN_POLL_RUN | > @@ -1086,16 +1092,21 @@ static void __bonded_sema(int i915, uint32_t ctx, > */ We should probably delete the comment above here in the code similar to __bonded_chain* since we are not replacing/dropping the timeline here anymore? > @@ -1818,8 +1824,8 @@ static void __bonded_early(int i915, uint32_t ctx, > struct drm_i915_gem_execbuffer2 execbuf = { > .buffers_ptr = to_user_pointer(&batch), > .buffer_count = 1, > - .rsvd1 = ctx, > }; > + uint32_t vm, ctx; > igt_spin_t *spin; > > memset(bonds, 0, sizeof(bonds)); > @@ -1833,6 +1839,11 @@ static void __bonded_early(int i915, uint32_t ctx, > bonds[n].engines[0] = siblings[(n + 1) % count]; > } > > + /* We share a VM so that the spin cancel will work without a reloc */ Didn't understand this comment either. Though both __bonded_early() and bonded() are deleted later on in the series so if had reordered the patches we could have avoided changing these functions. Anyway, leave as is, no need to change. _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev