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

[Katarzyna Dec]

On Wed, Aug 01, 2018 at 04:10:07PM +0100, Chris Wilson wrote:
> 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) {
                    /* align to the stride */
> > > +             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);
                    /* round up to a pot for old gen */
> > > +             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.
>
Now I understand. Maybe it would be worth to add some comment about that
in the code? It would be much more clear for everybody.
> > > +     } 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.
>
Some documentation is better then no documentation. I know that code should be
it's own judge, but you need to have deep understanding in driver code to get
what is going on with creating these batches.
And by docs I do not mean every super formatted detail, just small comments
inline comments.

> > > +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.
Same as above, it would be nice to heve even one line comment here.
> 
> > > +     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.
>
It would be nice if you introduce some one-line comments in few places. Maybe
code is simple, but it doesn't look like that.

Kasia :)
> > 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