Re: [igt-dev] [PATCH i-g-t] tests/i915/gem_ctx_switch: Convert to intel_ctx_t

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

Some time ago, Dixit, Ashutosh wrote:
>
> On Tue, 22 Jun 2021 22:31:14 -0700, Jason Ekstrand wrote:
> >
> > On Mon, Jun 21, 2021 at 12:28 AM Dixit, Ashutosh
> > <ashutosh.dixit@intel.com> wrote:
> > >
> > > On Sun, 20 Jun 2021 20:48:32 -0700, Jason Ekstrand wrote:
> > > >
> > > > On Fri, Jun 18, 2021 at 2:19 PM Dixit, Ashutosh
> > > > <ashutosh.dixit@intel.com> wrote:
> > > > >
> > > > > On Fri, 18 Jun 2021 09:47:14 -0700, Jason Ekstrand wrote:
> > > > > >
> > > > > > @@ -315,8 +326,8 @@ igt_main
> > > > > >       } phases[] = {
> > > > > >               { "", 0, NULL },
> > > > > >               { "-interruptible", INTERRUPTIBLE, NULL },
> > > > > > -             { "-queue", QUEUE, gem_has_queues },
> > > > > > -             { "-queue-interruptible", QUEUE | INTERRUPTIBLE, gem_has_queues },
> > > > > > +             { "-queue", QUEUE, gem_has_vm },
> > > > > > +             { "-queue-interruptible", QUEUE | INTERRUPTIBLE, gem_has_vm },
> > > > >
> > > > > As I was saying ealier I think here we should replace gem_has_vm with
> > > > > gem_has_queues() where:
> > > > >
> > > > > gem_has_queues == gem_has_vm && gem_context_has_single_timeline
> > > >
> > > > Oh, I see what you mean now!  I've added this locally:
> > > >
> > > > static bool
> > > > has_queues(int fd)
> > > > {
> > > >     return gem_has_vm(fd) && gem_context_has_single_timeline(fd);
> > > > }
> > > >
> > > > and used it instead of gem_has_vm.  How's that sound?
> > >
> > > Thanks, though maybe add to the lib where we added
> > > gem_context_has_single_timeline() (lib/i915/gem_context.c) since there are
> > > other places too where this would be useful.
> >
> > I'm a little hesitant to do that since we don't have a quick-and-easy
> > "create a queue" thing anymore.  It doesn't make sense to me to have a
> > central query for something that isn't a central concept, just because
> > it exists more than once.

My concern here generally has been that the 'queue' concept (a context with
a shared VM and shared timeline) which existed previously in IGT has now
been lost as a result of these changes. So I was thinking if it's possible
to restore it. One way might be to have two functions such as:

void intel_ctx_cfg_for_queues(int fd, intel_ctx_cfg_t *cfg)
{
	cfg->vm = gem_vm_create(fd);
	cfg->flags |= I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE;
}

bool gem_has_queues(int fd)
{
     return gem_has_vm(fd) && gem_context_has_single_timeline(fd);
}

And then we could call these from the couple of places where we've
implicitly used the queue concept (gem_ctx_switch.c and
gem_exec_whisper.c).

In any case, just an idea. But the patch has a R-b as is so feel free to
apply as is. We can debate and tweak such things after we merge this
series. Thanks.

> >
> > --Jason
> >
> >
> > > >
> > > > --Jason
> > > >
> > > > > But even otherwise this is:
> > > > >
> > > > > Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev