[igt-dev] [PATCH i-g-t v18 24/31] tests/gem_linear_blits: Use intel allocator

[Zbigniew Kempczyński]

On Fri, Jan 29, 2021 at 03:49:38PM +0000, Chris Wilson wrote:
> Quoting Zbigniew Kempczyński (2021-01-29 14:44:47)
> > From: Dominik Grzegorzek <dominik.grzegorzek at intel.com>
> > 
> > Use intel allocator directly, without intel-bb infrastructure.
> > 
> > Signed-off-by: Dominik Grzegorzek <dominik.grzegorzek at intel.com>
> > Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> 
> So I did not spot anything in these test changes that make me thing the
> underlying coverage was changed.
> 
> > ---
> >  tests/i915/gem_linear_blits.c | 103 ++++++++++++++++++++++++----------
> >  1 file changed, 73 insertions(+), 30 deletions(-)
> > 
> > diff --git a/tests/i915/gem_linear_blits.c b/tests/i915/gem_linear_blits.c
> > index cae42d52a..372d6ad81 100644
> > --- a/tests/i915/gem_linear_blits.c
> > +++ b/tests/i915/gem_linear_blits.c
> > @@ -56,12 +56,14 @@ IGT_TEST_DESCRIPTION("Test doing many blits with a working set larger than the"
> >  static uint32_t linear[WIDTH*HEIGHT];
> >  
> >  static void
> > -copy(int fd, uint32_t dst, uint32_t src)
> > +copy(int fd, uint64_t ahnd, uint32_t dst, uint32_t src,
> > +     uint64_t dst_offset, uint64_t src_offset)
> >  {
> >         uint32_t batch[12];
> >         struct drm_i915_gem_relocation_entry reloc[2];
> >         struct drm_i915_gem_exec_object2 obj[3];
> >         struct drm_i915_gem_execbuffer2 exec;
> > +       bool do_relocs = false;
> >         int i = 0;
> >  
> >         batch[i++] = XY_SRC_COPY_BLT_CMD |
> > @@ -77,41 +79,59 @@ copy(int fd, uint32_t dst, uint32_t src)
> >                   WIDTH*4;
> >         batch[i++] = 0; /* dst x1,y1 */
> >         batch[i++] = (HEIGHT << 16) | WIDTH; /* dst x2,y2 */
> > -       batch[i++] = 0; /* dst reloc */
> > +       batch[i++] = dst_offset;
> >         if (intel_gen(intel_get_drm_devid(fd)) >= 8)
> > -               batch[i++] = 0;
> > +               batch[i++] = dst_offset >> 32;
> >         batch[i++] = 0; /* src x1,y1 */
> >         batch[i++] = WIDTH*4;
> > -       batch[i++] = 0; /* src reloc */
> > +       batch[i++] = src_offset;
> >         if (intel_gen(intel_get_drm_devid(fd)) >= 8)
> > -               batch[i++] = 0;
> > +               batch[i++] = src_offset >> 32;
> >         batch[i++] = MI_BATCH_BUFFER_END;
> >         batch[i++] = MI_NOOP;
> >  
> > -       memset(reloc, 0, sizeof(reloc));
> > -       reloc[0].target_handle = dst;
> > -       reloc[0].delta = 0;
> > -       reloc[0].offset = 4 * sizeof(batch[0]);
> > -       reloc[0].presumed_offset = 0;
> > -       reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
> > -       reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
> > -
> > -       reloc[1].target_handle = src;
> > -       reloc[1].delta = 0;
> > -       reloc[1].offset = 7 * sizeof(batch[0]);
> > -       if (intel_gen(intel_get_drm_devid(fd)) >= 8)
> > -               reloc[1].offset += sizeof(batch[0]);
> > -       reloc[1].presumed_offset = 0;
> > -       reloc[1].read_domains = I915_GEM_DOMAIN_RENDER;
> > -       reloc[1].write_domain = 0;
> > -
> >         memset(obj, 0, sizeof(obj));
> >         obj[0].handle = dst;
> >         obj[1].handle = src;
> >         obj[2].handle = gem_create(fd, 4096);
> >         gem_write(fd, obj[2].handle, 0, batch, i * sizeof(batch[0]));
> > -       obj[2].relocation_count = 2;
> > -       obj[2].relocs_ptr = to_user_pointer(reloc);
> > +
> > +       if (src_offset == 0 && dst_offset == 0) {
> > +               do_relocs = true;
> > +               memset(reloc, 0, sizeof(reloc));
> > +               reloc[0].target_handle = dst;
> > +               reloc[0].delta = 0;
> > +               reloc[0].offset = 4 * sizeof(batch[0]);
> > +               reloc[0].presumed_offset = 0;
> 
> Is now dst_offset
> 
> > +               reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
> > +               reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
> > +
> > +               reloc[1].target_handle = src;
> > +               reloc[1].delta = 0;
> > +               reloc[1].offset = 7 * sizeof(batch[0]);
> > +               if (intel_gen(intel_get_drm_devid(fd)) >= 8)
> > +                       reloc[1].offset += sizeof(batch[0]);
> > +               reloc[1].presumed_offset = 0;
> 
> src_offset. Yes they may be zero, but semantics matter.

Ack.  

> 
> Why leave them as zero though? You can seed them with sensible
> placements. Sell me on some benefits of this allocator.
> -Chris

For relocs using allocator doesn't makes sense (buffers offsets can be
migrated within the kernel). And we have no method to synchronize it.
I see no additional value of using offsets passed by allocator which
could lead to confusion on the reader side.

--
Zbigniew