Re: [PATCH i-g-t v2 3/4] tests/kms_properties: check colorop properties on active color pipelines

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

Hello Melissa,

On 6/3/2026 2:36 AM, Melissa Wen wrote:
> Colorop properties can only be changed if its color pipeline is active,
> i.e. the colorop is in the chain of the color pipeline set in plane
> COLOR_PIPELINE property and its plane is active. For AMD, a CRTC must
> have a primary plane active, so overlay needs its own fb and cannot
> reuse primary fb. Also, color pipeline cannot be enabled on planes
> without COLOR_PIPELINE properties.
> 

The patch is doing two things together. Can you please separate them out?

==
Chaitanya

> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   tests/kms_properties.c | 26 ++++++++++++++++++++++++--
>   1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/kms_properties.c b/tests/kms_properties.c
> index f735839da..79d691448 100644
> --- a/tests/kms_properties.c
> +++ b/tests/kms_properties.c
> @@ -237,7 +237,7 @@ static void run_colorop_property_tests(igt_display_t *display,
>   				       igt_crtc_t *crtc, igt_output_t *output,
>   				       bool atomic)
>   {
> -	struct igt_fb fb;
> +	struct igt_fb fb, afb;
>   	igt_plane_t *plane;
>   	igt_colorop_t *colorop;
>   	int i;
> @@ -247,14 +247,31 @@ static void run_colorop_property_tests(igt_display_t *display,
>   		     &fb);
>   
>   	for_each_plane_on_crtc(crtc, plane) {
> +		/* don't check colorop in planes without support to color pipeline */
> +		if (!igt_plane_has_prop(plane, IGT_PLANE_COLOR_PIPELINE))
> +			continue;
> +
>   		igt_info("Testing colorop properties on plane %s.#%d-%s (output: %s)\n",
>   			 igt_crtc_name(crtc), plane->index,
>   			 kmstest_plane_type_name(plane->type), output->name);
>   
> +		/* AMD requires primary plane enabled to keep CRTC enabled */
> +		if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
> +			drmModeModeInfo *mode = igt_output_get_mode(output);
> +
> +			igt_create_pattern_fb(display->drm_fd, mode->hdisplay, mode->vdisplay,
> +					      DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR, &afb);
> +
> +			igt_plane_set_fb(plane, &afb);
> +		}
> +
>   		/* iterate over all color pipelines on plane */
>   		for (i = 0; i < plane->num_color_pipelines; ++i) {
>   			/* iterate over all colorops in pipeline*/
>   			colorop = plane->color_pipelines[i];
> +			igt_plane_set_color_pipeline(plane, colorop);
> +			igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> +
>   			while (colorop) {
>   				igt_info("Testing colorop properties on %s.#%d.#%d-%s (output: %s)\n",
>   					  igt_crtc_name(crtc), plane->index,
> @@ -268,8 +285,13 @@ static void run_colorop_property_tests(igt_display_t *display,
>   				colorop = igt_find_colorop(display, colorop_id);
>   			}
>   		}
> -	}
> +		igt_plane_set_prop_enum(plane, IGT_PLANE_COLOR_PIPELINE, "Bypass");
>   
> +		if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
> +			igt_plane_set_fb(plane, NULL);
> +			igt_remove_fb(display->drm_fd, &afb);
> +		}
> +	}
>   	cleanup_crtc(display, crtc, output,
>   		     &fb);
>   }