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

[Wentland, Harry]

On 2019-01-31 10:57 a.m., Maarten Lankhorst wrote:
> 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>
> 

Not nice that igt_display_reset can't properly reset things without potentially impacting tests that run after. This might have unintended consequences but would it make sense to do a commit from igt_display_reset to commit the state and therefore actually go into any subsequent test with a "default" state?

Either way, based on current behavior this patch is
Reviewed-by: Harry Wentland <harry.wentland at amd.com>

Harry