[igt-dev] [PATCH i-g-t] igt: Another combinatorial exercise for blits

[Chris Wilson]

Quoting Katarzyna Dec (2018-08-01 15:47:39)
> On Wed, Jul 25, 2018 at 10:38:23PM +0100, Chris Wilson wrote:
> > +static unsigned int
> > +get_tiling_stride(const struct device *device,
> > +               unsigned int width, unsigned int tiling)
> > +{
> > +     unsigned int stride = 4u * width;
> > +
> > +     if (tiling) {
> > +             if (device->gen < 3)
> > +                     stride = ALIGN(stride, 128);
> > +             else if (device->gen < 4 || tiling == I915_TILING_X)
> > +                     stride = ALIGN(stride, 512);
> > +             else
> > +                     stride = ALIGN(stride, 128);
> > +             if (device->gen < 4)
> > +                     stride = 1 << fls(stride - 1);
> Shouldn't 'else' be last here ^? What about order of 'else if' and 'if'
> at the end?

First align to the stride, then round up to a pot for old gen. It can be
done either away around; the rule is just it has to be both a multiple
of tile_width and a power of two.

> > +     } else {
> > +             if (device->gen >= 8)
> > +                     stride = ALIGN(stride, 64);

This is one that is more dubious as it papering over a hw bump that is a
bit more subtle. After all part of the reason for this test is to detect
errors like that.

> > +     if ((tiling | buffer->tiling) >= I915_TILING_Y) {
> > +             unsigned int mask;
> > +
> > +             batch[i++] = MI_LOAD_REGISTER_IMM;
> > +             batch[i++] = BCS_SWCTRL;
> > +
> 
> All batch creation staring from here ^ looks misterious. Maybe we can make
> buffer_set_tiling and buffer_linear functions more clearer?
> It is hard do read and understand.

About the only thing I might touch is the lri. But I am not that
convinced it's worth it in the grand scheme.

> > +static void *download(const struct device *device,
> > +                   const struct buffer *buffer,
> > +                   enum mode mode)
> > +{
> > +     void *linear, *src;
> > +
> > +     igt_assert(posix_memalign(&linear, 4096, buffer->size) == 0);
> > +
> > +     switch (mode) {
> > +     case CPU:
> > +             if (buffer->tiling) {
> > +                     if (buffer->caching && !device->llc) {
> > +                             mode = GTT;
> > +                             break;
> > +                     }
> > +                     if (device->gen < 3) {
> > +                             mode = GTT;
> > +                             break;
> > +                     }
> Why do we need 2 if-s here ^? Both are setting the same mode.

Semantics, they are for different reasons, but related reasons.
Not allowed to copy between snooped surfaces; and gen2 doesn't allow
copy to snoop, just copy from snoop.

> > +     if (dst->caching) {
> > +             igt_assert(device->gen >= 3);
> > +             igt_assert(device->llc || !src->caching);
> > +     }
> > +
> How did you calculated this values above ^ ?

^^

> Whole test with all helper functions seems to be not clear enough. 
> I would prefer to have more comments of what is going on in the code,
> especially in creating batches.

There isn't much going on, no tricks, just straight forward XY_SRC_COPY.
 
> General question - does this test gives additional coverge or will be
> a substitue for some existing?

Both more and less at the moment. There's better testing for the copy
and access patterns, but less around the limits of the GTT. But atm we
only do the sanitychecks in CI, so from that perspective this is much
more complete testing of things like pixel/cacheline/page access between
tiling modes.

The challenge I have in mind is how to let it explore by itself. A small
bytecode to try out different combinations of state is one way, but that
only covers the primitives we have modelled and therein lies the rub.
How to go completely crazy yet still detect error (other than oopses).
-Chris