[Intel-gfx] [PATCH igt v3] igt/gen7_forcewake_mt: Make the mmio register as volatile

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-03-06 09:37:50)
> 
> On 05/03/2018 17:30, Chris Wilson wrote:
> > -static void *igfx_get_mmio(void)
> > +static volatile uint32_t *igfx_mmio_forcewake_mt(void)
> 
> I think its overkill to bother with volatile all throughout the chain, 
> it's not like it's const. But never mind.

When you stub your toe, you should amputate the leg. Just to be sure.

> >   static void *thread(void *arg)
> >   {
> > +     static const char acquire_error[] = "acquire";
> > +     static const char release_error[] = "release";
> > +
> >       struct thread *t = arg;
> > -     uint32_t *forcewake_mt = (uint32_t *)((char *)t->mmio + FORCEWAKE_MT);
> > -     uint32_t bit = 1 << t->bit;
> > +     const uint32_t bit = 1 << t->bit;
> > +     volatile uint32_t *forcewake_mt = t->forcewake_mt;
> >   
> > -     while (1) {
> > +     while (!READ_ONCE(t->done)) {
> 
> Not really important, but isn't this not allowed to be compiled out 
> since it comes from external to the function memory?

If there wasn't an ::memory barrier inside the loop, the compiler can
assume that the value doesn't change. (It is not marked as volatile).
However, we do have volatiles (and so external memory barriers) inside
the loop, so should be a moot point.

> >               *forcewake_mt = bit << 16 | bit;
> > -             igt_assert(*forcewake_mt & bit);
> > +             if (!igt_wait(*forcewake_mt & bit, 50, 1))
> > +                     return (void *)acquire_error;
> > +
> >               *forcewake_mt = bit << 16;
> > -             igt_assert((*forcewake_mt & bit) == 0);
> > +             if (!igt_wait((*forcewake_mt & bit) == 0, 50, 1))
> > +                     return (void *)release_error;
> >       }

> > +
> > +     for (i = 2; i < 16; i++) {
> > +             void *result;
> > +
> > +             t[i].done = true;
> > +             pthread_join(t[i].thread, &result);
> 
> Check return value just in case?

Here it can only fail in a programming error, shrug. We are better off
with an igt_assert() on pthread_create().
-Chris