Re: [igt-dev] [PATCH i-g-t 60/79] tests/i915/gem_ctx_create: Convert benchmarks to intel_ctx_t

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

On Tue, 22 Jun 2021 23:09:51 -0700, Jason Ekstrand wrote:
>
> On Mon, Jun 21, 2021 at 10:30 PM Dixit, Ashutosh
> <ashutosh.dixit@intel.com> wrote:
> >
> > On Thu, 17 Jun 2021 12:14:57 -0700, Jason Ekstrand wrote:
> > >
> > > -static void maximum(int fd, int ncpus, unsigned mode)
> > > +static void maximum(int fd, const intel_ctx_cfg_t *cfg,
> > > +                 int ncpus, unsigned mode)
> > >  {
> > >       const uint32_t bbe = MI_BATCH_BUFFER_END;
> > >       struct drm_i915_gem_execbuffer2 execbuf;
> > > @@ -300,9 +302,7 @@ static void maximum(int fd, int ncpus, unsigned mode)
> > >
> > >               err = -ENOMEM;
> > >               if (avail_mem > (count + 1) * ctx_size)
> > > -                     err =  __gem_context_clone(fd, 0,
> > > -                                                I915_CONTEXT_CLONE_ENGINES,
> > > -                                                0, &ctx_id);
> > > +                     err =  __gem_context_create(fd, &ctx_id);
> > >               if (err) {
> > >                       igt_info("Created %lu contexts, before failing with '%s' [%d]\n",
> > >                                count, strerror(-err), -err);
> > > @@ -323,6 +323,7 @@ static void maximum(int fd, int ncpus, unsigned mode)
> > >
> > >       igt_fork(child, ncpus) {
> > >               struct timespec start, end;
> > > +             const intel_ctx_t *ctx;
> > >               int i915;
> > >
> > >               i915 = gem_reopen_driver(fd);
> >
> > Just a minor nit here. I'm thinking if we should remove the i915 variable
> > and the gem_reopen_driver() here and just use fd? Because it seems we have
> > exhausted the memory creating contexts earlier on, and now the previous
> > code was creating one additional context using gem_reopen_driver() whereas
> > we are creating two, one with gem_reopen_driver() and a second with
> > intel_ctx_create(). Not sure if it matters or not but just in case it does
> > we can get rid of gem_reopen_driver() and i915.
>
> Wait a second... I'm not sure how this was expected to ever work.  In
> the first loop, the test creates a bunch of contexts with fd.  Then,
> it forks off children.  Each of those children re-open the driver and
> then try to execute using said contexts on i915.  But those contexts
> don't exist on the newly opened DRM device.  I don't see how this ever
> worked.

Yes you are right, the older code seems to have this bug. But with your
changes shall we remove i915 and just use fd and that should fix it,
correct?

> --Jason
>
> > Apart from this, this is:
> >
> > Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >
> > > @@ -331,7 +332,7 @@ static void maximum(int fd, int ncpus, unsigned mode)
> > >                * a nop execbuf and stalling for it.
> > >                */
> > >               gem_quiescent_gpu(i915);
> > > -             gem_context_copy_engines(fd, 0, i915, 0);
> > > +             ctx = intel_ctx_create(i915, cfg);
> > >
> > >               hars_petruska_f54_1_random_perturb(child);
> > >               obj[0].handle = gem_create(i915, 4096);
> > > @@ -352,6 +353,7 @@ static void maximum(int fd, int ncpus, unsigned mode)
> > >               gem_sync(i915, obj[0].handle);
> > >               clock_gettime(CLOCK_MONOTONIC, &end);
> > >               gem_close(i915, obj[0].handle);
> > > +             intel_ctx_destroy(i915, ctx);
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev