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

[Maarten Lankhorst]

Op 31-01-2019 om 16:09 schreef 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.

Hmm, I guess there's precedence to this with the other properties. Maybe we should clear out gamma/etc as well for a saner experience, but it 's harmless for now.

You're free to choose tif you want to keep resetting this or not. I'm a bit indifferent, but some tests may not handle VRR enabled correctly.

I think cascading tests should be avoided personally, igt_display_reset() should recover from a not too weird state, but then again it should also clear csc/gamma/etc then. Oh well..


Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>