Re: [PATCH i-g-t v2 1/4] lib/igt_kms: clear colorop-changed flag after commit

On 6/3/2026 2:36 AM, Melissa Wen wrote:
> Don't request colorop updates if there is no change in the atomic
> commit. It fixes atomic rejection after test skips that was caused by
> incorrect attempts of changing inactive colorop properties, since
> userspace can update only current active colorops or colorops that will
> be activated or deactivated in the same commit. Active colorop means a
> colorop that is in the chain of an active color pipeline, i.e.  the
> pipeline in plane COLOR_PIPELINE property.
> 

The patch overlooks an issue where we don't update the 
assigned_color_pipeline to NULL even when pipeline set to Bypass. Though 
unlikely,  if anything later sets a property on one of colorops in the 
now bypassed pipeline, the next atomic commit will still walk that stale 
chain via igt_atomic_prepare_colorop_commit() and try to send the change 
which the kernel should reject.

I tried to solve it in [1] but it had the side-effect of making 
reset_colorops() a no-op (done in a single commit with bypass). That is 
again because igt_atomic_prepare_colorop_commit() uses
plane->assigned_color_pipeline to walk through the pipeline. One 
solution could be tracking both the old and new pipeline.

However, that should not block this change. So this LGTM.

Reviewed-by: Chaitanya Kumar Borah <[email protected]>

[1] 
https://lore.kernel.org/igt-dev/[email protected]/

> Assisted-by: Claude:claude-opus-4-7
> Tested-by: Alex Hung <[email protected]>
> Signed-off-by: Melissa Wen <[email protected]>
> ---
>   lib/igt_kms.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index e20307923..d46f15c01 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -4858,6 +4858,7 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
>   {
>   	int i;
>   	igt_crtc_t *crtc;
> +	igt_colorop_t *colorop;
>   
>   	for_each_crtc(display, crtc) {
>   		igt_plane_t *plane;
> @@ -4887,6 +4888,20 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
>   				int fd;
>   				plane->changed = 0;
>   
> +				/* clear changed flags on every colorop in the assigned chain
> +				 * so already-committed property values aren't re-emitted on
> +				 * the next commit.
> +				 */
> +				colorop = plane->assigned_color_pipeline;
> +				while (colorop) {
> +					int next_val;
> +
> +					colorop->changed = 0;
> +					next_val = igt_colorop_get_prop(display, colorop,
> +									IGT_COLOROP_NEXT);
> +					colorop = igt_find_colorop(display, next_val);
> +				}
> +
>   				fd = plane->values[IGT_PLANE_IN_FENCE_FD];
>   				if (fd != -1)
>   					close(fd);