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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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>
>> ---
>>  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?
>
>
> 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


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