[Intel-gfx] [igt-dev] [PATCH i-g-t v5 1/4] lib/i915: Add minimum GTT alignment helper

[Janusz Krzysztofik]

Hi Jonas,

On Tuesday, November 5, 2019 10:14:20 AM CET Joonas Lahtinen wrote:
> Quoting Janusz Krzysztofik (2019-11-04 19:13:27)
> > Some tests assume 4kB offset alignment while using softpin.  That
> > assumption may be wrong on future GEM backends with possibly larger
> > minimum page sizes.  As a result, those tests may either fail on
> > softpin at offsets which are incorrectly aligned, may silently skip
> > such incorrectly aligned addresses assuming them occupied by other
> > users, or may always succeed when examining invalid use patterns.
> > 
> > Provide a helper function that detects minimum GTT alignment.  Tests
> > may use it to calculate softpin offsets valid on actually used backing
> > store.
> > 
> > Also expose a new object validation helper just created, it may be
> > useful for checking if a shared GTT address is not reserved, for
> > example.
> > 
> > v2: Rename the helper, use 'minimum GTT alignment' term across the
> >     change (Chris),
> >   - use error numbers to distinguish between invalid offsets and
> >     addresses occupied by other users,
> >   - simplify the code (Chris).
> > v3: Put the code under lib/i915/, not in lib/ioctl_wrappers.c (Chris),
> >   - validate objects with an invalid reloc applied so execbuf requests
> >     called only for validation purposes are actually not emitted to GPU
> >     (Chris),
> >   - move object validation code to a separate helper.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > Cc: Stuart Summers <stuart.summers at intel.com>
> 
> <SNIP>
> 
> > +int gem_gtt_validate_object(int fd, struct drm_i915_gem_exec_object2 *obj)
> > +{
> > +       struct drm_i915_gem_relocation_entry reloc;
> > +       struct drm_i915_gem_execbuffer2 execbuf;
> > +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> > +       uintptr_t old_reloc;
> > +       int old_count, err;
> > +
> > +       memset(&reloc, 0, sizeof(reloc));
> > +       memset(&execbuf, 0, sizeof(execbuf));
> > +
> > +       /* use invalid reloc to save request emission */
> > +       old_reloc = obj->relocs_ptr;
> > +       old_count = obj->relocation_count;
> > +       obj->relocs_ptr = to_user_pointer(&reloc);
> > +       obj->relocation_count = 1;
> > +       gem_write(fd, obj->handle, 0, &bbe, sizeof(bbe));
> 
> Don't use relocations for anything new. Also, don't depend on such
> quirk behavior as to kernel validating parameters in certain order.
> 
> > +int gem_gtt_min_alignment_order(int fd)
> > +{
> > +       struct drm_i915_gem_exec_object2 obj;
> > +       int order;
> > +
> > +       /* no softpin => 4kB page size */
> > +       if (!gem_has_softpin(fd))
> > +               return 12;
> > +
> > +       memset(&obj, 0, sizeof(obj));
> > +
> > +       obj.handle = gem_create(fd, 4096);
> > +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> 
> Default will be system memory, so it's unclear what this would establish.
> 
> Just use PCI ID to detect this. 

I'm not sure what I'm supposed to detect if default will be system memory.  Do we expect 
different, device dependent minimum GTT alignments in system memory?

Thanks,
Janusz


> It's not going to be dynamic within a
> device. This just adds extra startup cost for detecting something that
> is not dynamic.
> 
> > +/*
> > + * Copyright © 2019 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + * Authors:
> > + *    Eric Anholt <eric at anholt.net>
> > + *    Daniel Vetter <daniel.vetter at ffwll.ch>
> > + *
> > + */
> 
> Please don't blindly copy "Authors:" around in the code.
> 
> Regards, Joonas
>