Re: [igt-dev] [PATCH i-g-t] lib/igt_kms: Clear all non-atomic properties in legacy/universal commit correctly.

["Kazlauskas, Nicholas" <Nicholas.Kazlauskas@amd.com>]

On 1/31/19 7:05 AM, Maarten Lankhorst wrote:
> Op 31-01-2019 om 11:26 schreef Petri Latvala:
>> On Thu, Jan 31, 2019 at 09:58:02AM +0100, Maarten Lankhorst wrote:
>>> We used to add them all 1 by 1, but we really only care about not
>>> handling a few.
>>>
>>> Only skip unsetting all atomic properties, instead of handling it
>>> through a whitelist.
>>>
>>> This fixes kms_busy, which was updating the VRR hint, even though
>>> we already unset it in the legacy path.
>>>
>>> Cc: Nicholas Kazlauskas <nkazlaus@amd.com>
>>> Cc: Harry Wentland <harry.wentland@amd.com>
>>> Cc: Leo Li <sunpeng.li@amd.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109490
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

This is a much more elegant solution to just clearing VRR_ENABLED by 
itself like I was doing in my other patch. Also helps guard against 
forgetting to clear new properties in the future (like my original patch 
did).

I'll update my "lib/igt_kms: Don't reset VRR_ENABLED on every commit" 
based on this commit. Thanks!

>>> ---
>>>   lib/igt_kms.c | 7 +++----
>>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>>> index 58f32911d999..1f98b26fcd79 100644
>>> --- a/lib/igt_kms.c
>>> +++ b/lib/igt_kms.c
>>> @@ -3298,10 +3298,9 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
>>>   			pipe_obj->values[IGT_CRTC_OUT_FENCE_PTR] = 0;
>>>   			pipe_obj->changed = 0;
>>>   		} else {
>>> -			igt_pipe_obj_clear_prop_changed(pipe_obj, IGT_CRTC_BACKGROUND);
>>> -			igt_pipe_obj_clear_prop_changed(pipe_obj, IGT_CRTC_CTM);
>>> -			igt_pipe_obj_clear_prop_changed(pipe_obj, IGT_CRTC_DEGAMMA_LUT);
>>> -			igt_pipe_obj_clear_prop_changed(pipe_obj, IGT_CRTC_GAMMA_LUT);
>>> +			for (i = 0; i < IGT_NUM_CRTC_PROPS; i++)
>>> +				if (!is_atomic_prop(i))
>>> +					igt_pipe_obj_clear_prop_changed(pipe_obj, i);
>>
>> Does IGT_CRTC_VRR_ENABLED need to be added to is_atomic_prop?
> 
> The legacy path:
> - Performs drmModeSetCrtc on the primary plane to trigger a modeset and connector assignments
> - Uses drmModeSetCursor/drmModeMoveCursor to move the cursor plane
> - Uses drmModeSetPlane on other planes.
> - Uses drmModeConnectorSetProperty to set connector properties.
> - Calls drmModeObjectSetProperty to update any non-atomic crtc properties. Probably gamma/etc as well.
> Primary and cursor properties are mostly ignored, except with what can be set through setcursor/setcrtc.
> 
> The universal path:
> - Uses SetPlane on all planes.
> - Calls drmModeObjectSetProperty for all not ignored properties on each plane/connector/crtc.
> - Can't do modesets.
> 
> The atomic path:
> - Does a single atomic commit, with all changed properties and their new values. All of the above and more (in/out fence handling)..
> 
> Even if it's strictly speaking an atomic property, the fact we clear it in igt_pipe_refresh()
> means we have to make sure that tests keep running, and we have to clear the flag in igt_display_commit()
> because otherwise even more things break. :)
> 
> Probably best to only keep things we absolutely can't reasonably another way than atomic in is_atomic.
> 
>> ...
>>
>> No, that's not a "is this an atomic-only property", is it?

It's a property that's only exposed on atomic drivers but isn't marked 
as atomic only so that legacy userspace could still use it. It shouldn't 
be in the is_atomic_prop list at least.

>>
>>
>> Anyway, the effect matches the second chunk of Nicholas's patch.
>>
>>
>> Reviewed-by: Petri Latvala <petri.latvala@intel.com>
>>
>> There's some queue in shards so stand by a couple of hours for their results before merging.
>>
>>
>>
>>>   
>>>   			if (s != COMMIT_UNIVERSAL) {
>>>   				igt_pipe_obj_clear_prop_changed(pipe_obj, IGT_CRTC_MODE_ID);
>>> -- 
>>> 2.20.1
>>>
>>> _______________________________________________
>>> igt-dev mailing list
>>> igt-dev@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> 

Nicholas Kazlauskas
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev