Re: [igt-dev] [PATCH i-g-t 49/79] tests/i915/gem_exec_balancer: Don't reset engines on a context

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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 <ashutosh.dixit@intel.com>

>
> > @@ -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