[Intel-gfx] [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Janusz Krzysztofik]

On Monday, November 4, 2019 10:28:37 AM CET Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-11-04 09:23:12)
> > Hi Chris,
> > 
> > On Friday, November 1, 2019 11:08:45 AM CET Chris Wilson wrote:
> > > Quoting Janusz Krzysztofik (2019-10-31 15:28:55)
> > > > 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 if incorrect detection method is used, 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 for actually used backing
> > > > store.
> > > > 
> > > > 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, then
> > > >   - simplify the code (Chris).
> > > > 
> > > > 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>
> > > > ---
> > > >  lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  lib/ioctl_wrappers.h |  2 ++
> > > >  2 files changed, 48 insertions(+)
> > > > 
> > > > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > > > index 628f8b83..f0ef8b2e 100644
> > > > --- a/lib/ioctl_wrappers.c
> > > > +++ b/lib/ioctl_wrappers.c
> > > > @@ -54,6 +54,7 @@
> > > >  #include "intel_io.h"
> > > >  #include "igt_debugfs.h"
> > > >  #include "igt_sysfs.h"
> > > > +#include "igt_x86.h"
> > > >  #include "config.h"
> > > >  
> > > >  #ifdef HAVE_VALGRIND
> > > > @@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
> > > >         return has_softpin;
> > > >  }
> > > >  
> > > > +/**
> > > > + * gem_gtt_min_alignment_order:
> > > > + * @fd: open i915 drm file descriptor
> > > > + *
> > > > + * This function detects the minimum possible alignment of a soft-pinned 
> > gem
> > > > + * object allocated from a default backing store.  It is useful for 
> > calculating
> > > > + * correctly aligned softpin offsets.
> > > > + * Since size order to size conversion (size = 1 << order) is less 
> > trivial
> > > > + * than the opposite, the function returns the alignment order as more 
> > handy.
> > > > + *
> > > > + * Returns:
> > > > + * Size order of the minimum GTT alignment
> > > > + */
> > > > +int gem_gtt_min_alignment_order(int fd)
> > > 
> > > But not part of ioctl_wrappers.c!
> > > 
> > > lib/i915/gem_gtt_topology.c? _query.c? _probe.c?
> > 
> > I was thinking about that but couldn't find a good name.  I'll use one of your 
> > proposed, thanks.
> > 
> > > > +{
> > > > +       struct drm_i915_gem_exec_object2 obj;
> > > > +       struct drm_i915_gem_execbuffer2 eb;
> > > > +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> > > > +       int order;
> > > > +
> > > > +       /* no softpin => 4kB page size */
> > > > +       if (!gem_has_softpin(fd))
> > > > +               return 12;
> > > > +
> > > > +       memset(&obj, 0, sizeof(obj));
> > > > +       memset(&eb, 0, sizeof(eb));
> > > > +
> > > > +       obj.handle = gem_create(fd, 4096);
> > > > +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> > > > +       eb.buffers_ptr = to_user_pointer(&obj);
> > > > +       eb.buffer_count = 1;
> > > > +       gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
> > > > +
> > > > +       for (order = 12; order < 64; order++) {
> > > > +               obj.offset = 1ull << order;
> > > > +               if (__gem_execbuf(fd, &eb) != -EINVAL)
> > > > +                       break;
> > 
> > Should I also follow your advice of checking for -ENOSPC here rather than 
> > !-EINVAL?
> 
> This time we expect EINVAL for the invalid non-aligned offset provided by
> the user. -ENOSPC comes later when we fail to evict -- but we will only
> try that with a valid GTT node.

OK, I thought that was more about proper handling of possible error codes 
other than -EINVAL or -ENOSPC.

Thanks,
Janusz

> -Chris
>