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 2F71589811 for ; Tue, 6 Jul 2021 05:42:49 +0000 (UTC) Date: Mon, 05 Jul 2021 22:23:40 -0700 Message-ID: <87eecct3qb.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <87sg1172cr.wl-ashutosh.dixit@intel.com> References: <20210617191256.577244-1-jason@jlekstrand.net> <20210617191256.577244-50-jason@jlekstrand.net> <87sg1172cr.wl-ashutosh.dixit@intel.com> 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 Mon, 28 Jun 2021 20:53:56 -0700, Dixit, Ashutosh wrote: > > 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. Let's continue discussion on this, I would still like to understand the issues raised previously. However since I am out for next few days I don't want to block the merge of this series on just this one patch. Therefore this is: Reviewed-by: Ashutosh Dixit > > > @@ -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 _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev