[Intel-gfx] [PATCH i-g-t v2] kms_rotation_crc: 90 degree flip test is not a stress test

[Tvrtko Ursulin]

On 04/09/2017 15:43, Daniel Vetter wrote:
> On Mon, Sep 04, 2017 at 03:36:56PM +0100, Tvrtko Ursulin wrote:
>>
>> On 04/09/2017 15:27, Tvrtko Ursulin wrote:
>>> On 07/08/2017 16:53, Daniel Vetter wrote:
>>>> On Fri, Aug 04, 2017 at 09:43:41AM +0100, Tvrtko Ursulin wrote:
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>>
>>>>> To the best of my recollection the page flipping test was added
>>>>> simply to start exercising page flips with 90/270 rotation.
>>>>>
>>>>> There is no need to do 60 flips which can take quite some time,
>>>>> because we do 60 flips against each pipe and each fb geometry.
>>>>>
>>>>> Also, calling this a stress test is also not matching the
>>>>> original idea of the test.
>>>>>
>>>>> Several changes:
>>>>>
>>>>> 1. Remove the stress from the name and reduce the number of
>>>>> flips to one only.
>>>>>
>>>>> 2. Move the page flip before CRC collection for a more useful
>>>>> test.
>>>>>
>>>>> 3. Add more flipping tests, for different rotation and sprite
>>>>> planes.
>>>>
>>>> I assume you didn't make the test overall slower with this?
>>>>
>>>>> 4. Convert to table driven subtest generation.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>>>
>>>> Didn't do a full review, but sounds all reasonable. And I assume you've
>>>> tested this at least locally (the igt patchwork CI instance doesn't do
>>>> full runs ... yet).
>>>
>>> Yes I've ran it on SKL. It adds more subtests so the runtime is longer,
>>> but it also finds new bugs.
>>
>> Runtime is longer compared to v1 of this patch BTW, I wasn't clear. It is
>> roughly on par with the situation without this patch.
>>
>> But v2 adds a lot more subtests, three of which fail (flip on a rotated
>> sprite plane). So there is value in it I think even like that.
> 
> Failing new subtests is ok imo, but pls do give a heads-up to CI folks
> before merging (but I think as long as it's failing stable, it shouldn't
> cause noise). Anything that takes out the machine or driver isn't ok, and
> needs to be fixed first.
> 
> So sounds all good to me to go ahead.

I thought we said full review rules on IGT, no? So I won't be merging 
this until someone can look at it in more detail.

Tvrtko