[igt-dev] [PATCH i-g-t v1] Data Port Cache Coherency tests.

[Lis, Tomasz]

On 2018-06-21 07:53, Joonas Lahtinen wrote:
> Quoting Tomasz Lis (2018-06-20 18:14:38)
>> This adds a new test binary, containing tests for the Data Port Coherency
>> option. The tests check whether the option value is stored properly on the
>> kernel side, but also whether is is correctly set to proper GPU register.
> I'm fairly sure there already was review feedback that simply checking
> the register state is not a good IGT test.
>
> IGT tests make sure that whatever uAPI the kernel has, the promised effects
> of that uAPI do not get broken. And the promise for cache coherency uAPI
> for surely isn't that some register value gets written, it's that the
> cache coherency is traded with some performance. The chicken bits or the
> whole implementation of the feature might be turned upside down, but as
> long as the userspace is still working, userspace should not care.
>
> So the chicken bit setting should be scrapped, and actual cache
> coherency observed. It might be a worthy kernel selftest that register
> writes stick and remain over sleep states as a generic thing, but not
> here.
>
> If you don't address the feedback given, but hammer the mailing list
> without addressing it, don't expect further feedback.
>
> Regards, Joonas
Thank you for your feedback, both now and in the previous round.

Developing a test which checks whether the hardware is really doing what 
is should is possible, but also it is quite problematic.
On the UMD side, there are Khronos OCL 2.1 tests which verify coherency; 
but they have graphics pipeline fully configured, and it is a lot easier 
to verify it on the user level.

We could write a test which does the following:
- Allocates a very big buffer
- Executes a shader which fills the whole buffer over and over, with 
increasing numbers
- Does it long enough for the IGT to notice changes in the buffer while 
the shader is being executed
- If there were increasing changes in buffer content - test passes

But there are issues with this approach:
- We need a lot of memory to do the test; the faster the GPU is, the 
more we need
- We need to write a shader, in assembly, for each platform
- A new shader will need to be written to test future platforms
- We need to configure the graphics pipeline within IGT test, which is 
not trivial
- The test results may be unreliable because even if coherency is 
disabled, there is no guarantee that the memory will not be updated
- Actually the buffer probably will be updated while the shader is 
executing without coherency, but details may depend on platform, 
pipeline settings and buffer size
- The test results may be unreliable due to timing, so it will likely 
produce sporadics
- The test would require a separate pre-silicon part
- Test execution time would be considerable

After reading the answers in previous review, I agree that to achieve 
the IGTs goal testing hardware should be included. But I don't think 
it's a good idea here.

But maybe there is an easier way to test the functionality which I 
didn't noticed? Or maybe some of my arguments are invalid?

We do have gem_gpgpu_fill test which does execute a simple shader. The 
shader needs to be written for each gen to re-enable the test, but it 
works.

-Tomasz

Adding the last mail in previous review, for reference:

Quoting Lis, Tomasz (2018-03-20 18:27:44)
 >
 >
 > On 2018-03-20 16:36, Chris Wilson wrote:
 > > Quoting Chris Wilson (2018-03-20 15:30:59)
 > >> Quoting Tomasz Lis (2018-03-20 15:12:32)
 > >>> From: "Lis, Tomasz" <tomasz.lis at intel.com>
 > >>>
 > >>> This adds a new test binary, containing tests for the Data Port
 > >>> Coherency option. The tests check whether the option is correctly
 > >>> set to proper GPU register.
 > >> But where's the test that it *does* anything? I.e. what's the
 > >> expected change in user visible behaviour? (What does the flag
 > >> really mean, now and for years to come?)
 >
 > With a test that "does anything" (which I understand as - submitting 
large workload and checking the coherency while the workload is 
executing) we would be testing the hardware, not the kernel.
 > I prepared a test binary designed to test the kernel change I 
proposed. This is what I consider good practices.
 > Do we have another approach when developing IGT?

What we try to avoid is "visible userspace breakage". So when some added 
kernel feature introduces a behavioral change for userspace, the 
accompanying tests should make sure that change is observed with the 
feature bit flipped. That's pretty much the only way we can make sure 
the written userspace applications using the feature will keep working 
when going forward.

Writing to MMIO address X so that the bits Y and Z stick will only 
guarantee that there is some hardware register in there. It is not very 
valuable of a test because making sure the feature keeps working will 
implicitly test the same thing.

Hopefully this answers the question.

Regards, Joonas

 >
 > > To be clear; this test illustrates that it is a context register
 > > being tweaked and not an execbuf property.
 > > -Chris
 > I agree the hardware flag is within hardware context. Both the kernel
 > patch and the IGT test are accessing that bit, so they both illustrate
 > it is a context register.
 > But both patches also implement UMD interface to that flag via execbuf
 > flag. What do you mean when you write this is "not an execbuf property"?
 >
 >