Re: [PATCH i-g-t v5 5/8] lib/igt_kms: extend igt_plane_set_color_pipeline to accept Bypass

[Harry Wentland <harry.wentland@amd.com>]

On 2026-09-02 13:58, Melissa Wen wrote:
> "Bypass" is just the COLOR_PIPELINE enum value with no colorop object
> behind it, so make igt_plane_set_color_pipeline accept NULL and set
> "Bypass" instead of making callers poke the property directly. Convert
> its callers, igt_plane_reset() included, and drop the now unused
> set_color_pipeline_bypass() from kms_colorop_helper.
> 
> Keep assigned_color_pipeline pointing at the outgoing pipeline when NULL
> is passed: the kernel accepts a colorop update whose pipeline is in the
> plane's old state, so reset_colorops() must still be able to finalize
> colorop settings in the same commit that deactivates them. No functional
> change.
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
> 
> v3:
> - new patch, replaces the open-coded "Bypass" setting in patch 6
> ---
>   lib/igt_kms.c                                 | 20 +++++++++++++------
>   .../chamelium/kms_chamelium_color_pipeline.c  |  2 +-
>   tests/kms_color_pipeline.c                    |  2 +-
>   tests/kms_colorop.c                           |  6 +++---
>   tests/kms_colorop_helper.c                    |  5 -----
>   tests/kms_colorop_helper.h                    |  1 -
>   6 files changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 6039ad18a..cabe60a31 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -2708,7 +2708,7 @@ static void igt_plane_reset(igt_plane_t *plane)
>   		igt_plane_set_prop_value(plane, IGT_PLANE_HOTSPOT_Y, 0);
>   
>   	if (igt_plane_has_prop(plane, IGT_PLANE_COLOR_PIPELINE))
> -		igt_plane_set_prop_enum(plane, IGT_PLANE_COLOR_PIPELINE, "Bypass");
> +		igt_plane_set_color_pipeline(plane, NULL);
>   
>   	igt_plane_clear_prop_changed(plane, IGT_PLANE_IN_FENCE_FD);
>   	plane->values[IGT_PLANE_IN_FENCE_FD] = ~0ULL;
> @@ -4430,17 +4430,25 @@ bool igt_plane_is_valid_colorop(igt_plane_t *plane, igt_colorop_t *colorop)
>   /**
>    * igt_plane_set_color_pipeline:
>    * @plane: Target plane.
> - * @colorop: Colorop to set as color pipeline.
> + * @colorop: Colorop to set as color pipeline, or NULL for "Bypass".
>    *
>    * This function sets the given @colorop as color pipeline on @plane, or fails
> - * the test if it's an invalid color pipeline for the plane.
> + * the test if it's an invalid color pipeline for the plane. Passing NULL sets
> + * the plane color pipeline to "Bypass" but keeps the previously assigned
> + * pipeline, so that pending colorop changes are still submitted with the
> + * commit that deactivates it, which the kernel accepts because the colorop is
> + * in the plane's old state.
>    */
>   void igt_plane_set_color_pipeline(igt_plane_t *plane, igt_colorop_t *colorop)
>   {
> -	igt_assert(igt_plane_is_valid_colorop(plane, colorop));
> +	igt_assert(!colorop || igt_plane_is_valid_colorop(plane, colorop));
>   
> -	plane->assigned_color_pipeline = colorop;
> -	igt_plane_set_prop_enum(plane, IGT_PLANE_COLOR_PIPELINE, colorop->name);
> +	if (colorop)
> +		plane->assigned_color_pipeline = colorop;
> +
> +	igt_plane_set_prop_enum(plane,
> +				IGT_PLANE_COLOR_PIPELINE,
> +				colorop ? colorop->name : "Bypass");
>   }
>   
>   /**
> diff --git a/tests/chamelium/kms_chamelium_color_pipeline.c b/tests/chamelium/kms_chamelium_color_pipeline.c
> index db6107221..5738c6dd1 100644
> --- a/tests/chamelium/kms_chamelium_color_pipeline.c
> +++ b/tests/chamelium/kms_chamelium_color_pipeline.c
> @@ -161,7 +161,7 @@ static void _test_plane_colorops(data_t *data,
>   	chamelium_destroy_frame_dump(frame);
>   
>   	/* Cleanup */
> -	set_color_pipeline_bypass(plane);
> +	igt_plane_set_color_pipeline(plane, NULL);
>   	reset_colorops(colorops);
>   
>   	igt_plane_set_fb(plane, NULL);
> diff --git a/tests/kms_color_pipeline.c b/tests/kms_color_pipeline.c
> index 78860a845..f71416ce1 100644
> --- a/tests/kms_color_pipeline.c
> +++ b/tests/kms_color_pipeline.c
> @@ -168,7 +168,7 @@ static void _test_plane_colorops(data_t *data,
>   	igt_assert_crc_equal(crc_ref, &crc_pipe);
>   
>   	/* Cleanup per-test state */
> -	set_color_pipeline_bypass(plane);
> +	igt_plane_set_color_pipeline(plane, NULL);
>   	reset_colorops(colorops);
>   	igt_plane_set_fb(plane, NULL);
>   	igt_display_commit_atomic(&data->display, 0, NULL);
> diff --git a/tests/kms_colorop.c b/tests/kms_colorop.c
> index 732b9d57f..2bee1eecd 100644
> --- a/tests/kms_colorop.c
> +++ b/tests/kms_colorop.c
> @@ -287,7 +287,7 @@ static void colorop_plane_test(igt_display_t *display,
>   
>   	/* reset color pipeline*/
>   
> -	set_color_pipeline_bypass(plane);
> +	igt_plane_set_color_pipeline(plane, NULL);
>   
>   	/* Commit */
>   	igt_plane_set_fb(plane, input_fb);
> @@ -315,7 +315,7 @@ static void colorop_plane_test(igt_display_t *display,
>   
>   	if (!colorops[0]) {
>   		/* bypass test */
> -		set_color_pipeline_bypass(plane);
> +		igt_plane_set_color_pipeline(plane, NULL);
>   	} else {
>   		/* get COLOR_PIPELINE enum */
>   		color_pipeline = get_color_pipeline(display, plane, colorops);
> @@ -343,7 +343,7 @@ static void colorop_plane_test(igt_display_t *display,
>   	/* Test bypass transition if requested */
>   	if (verify_bypass) {
>   		/* reset color pipeline*/
> -		set_color_pipeline_bypass(plane);
> +		igt_plane_set_color_pipeline(plane, NULL);
>   
>   		/* Commit */
>   		igt_plane_set_fb(plane, input_fb);
> diff --git a/tests/kms_colorop_helper.c b/tests/kms_colorop_helper.c
> index da234410e..a109c1b06 100644
> --- a/tests/kms_colorop_helper.c
> +++ b/tests/kms_colorop_helper.c
> @@ -450,11 +450,6 @@ void set_color_pipeline(igt_display_t *display,
>   	}
>   }
>   
> -void set_color_pipeline_bypass(igt_plane_t *plane)
> -{
> -	igt_plane_set_prop_enum(plane, IGT_PLANE_COLOR_PIPELINE, "Bypass");
> -}
> -
>   static void reset_colorop(kms_colorop_t *colorop)
>   {
>   	igt_assert(colorop->colorop);
> diff --git a/tests/kms_colorop_helper.h b/tests/kms_colorop_helper.h
> index 9a1477666..2a0e3799f 100644
> --- a/tests/kms_colorop_helper.h
> +++ b/tests/kms_colorop_helper.h
> @@ -114,7 +114,6 @@ void set_color_pipeline(igt_display_t *display,
>   			igt_plane_t *plane,
>   			kms_colorop_t *colorops[],
>   			igt_colorop_t *color_pipeline);
> -void set_color_pipeline_bypass(igt_plane_t *plane);
>   void reset_colorops(kms_colorop_t *colorops[]);
>   
>   #endif /* __KMS_COLOROP_HELPER_H__ */