[igt-dev] [PATCH i-g-t v2] lib/igt_kms: Don't reset VRR_ENABLED on every commit

[Kazlauskas, Nicholas]

On 1/31/19 9:53 AM, Maarten Lankhorst wrote:
> Op 31-01-2019 om 15:48 schreef Nicholas Kazlauskas:
>> If the VRR tests failed then there was a chance that VRR could be
>> left enabled when exiting the test so the VRR_ENABLED property was
>> reset to 0 whenever the pipe was reset.
>>
>> However, in doing so the pipe's state was considered changed even
>> if VRR_ENABLED was already 0. This causes the pipe to be added to
>> commits where it previously wasn't.
>>
>> Other tests with properties that can persist for failures (like color
>> management) don't bother resetting the properties - so this patch
>> changes VRR_ENABLED to work the same.
>>
>> Cc: Harry Wentland <harry.wentland at amd.com>
>> Cc: Petri Latvala <petri.latvala at intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109490
>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
>> ---
>>   lib/igt_kms.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index 71df98d2..0bec3734 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -1791,9 +1791,6 @@ static void igt_pipe_reset(igt_pipe_t *pipe)
>>   	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_ACTIVE, 0);
>>   	igt_pipe_obj_clear_prop_changed(pipe, IGT_CRTC_OUT_FENCE_PTR);
>>   
>> -	if (igt_pipe_obj_has_prop(pipe, IGT_CRTC_VRR_ENABLED))
>> -		igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_VRR_ENABLED, 0);
>> -
>>   	pipe->out_fence_fd = -1;
>>   }
>>    
> 
> It's ok to force it disabled if any test could start failing because of it? Most tests are probably not aware of VRR.
> 

A lot of atomic state can persist outside of an intended test. I 
couldn't really find anywhere that explicitly resets CRTC properties 
like gamma/background color/etc.

So I currently disable vrr before I check the main pass/fail threshold 
in the test. There are a few cases where the test can still fail while 
it's enabled, but those are mostly system failures. This bit in igt_kms 
is kind of redundant in that sense.

This patch aims to keep things they were before, where one test failure 
can cascade and cause many other tests to fail. I personally favor full 
test isolation when possible so I can tell what specifically went wrong 
when something happens but when everything breaks I guess it gives more 
incentive to address the breakage itself.

Nicholas Kazlauskas