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

[Joonas Lahtinen]

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