Re: [PATCH i-g-t v3] tests/kms_content_protection: Optimize framebuffer management

[Karthik B S <karthik.b.s@intel.com>]

Hi Jason-JH,

On 3/17/2026 9:31 PM, Jason-JH Lin wrote:
> Refactor framebuffer management to improve reusability:
> - Create framebuffers once per CRTC at test initialization via
>    create_fbs() and reuse them across all subtests
> - Centralize cleanup in remove_fbs() called at test fixture end
> - Use per-CRTC framebuffer array (data.fbs[IGT_MAX_PIPES]) indexed
>    by crtc->crtc_index instead of single global framebuffers
>
> Each framebuffer is created with the actual resolution of the first
> output that can connect to that CRTC, avoiding buffer size mismatch
> issues (e.g., 3504x2190 panel incorrectly using 3840x2160 buffer).
>
> For MST tests, multiple outputs share bandwidth and may require mode
> override to fit within bandwidth constraints. When mode override
> changes the resolution, properly detach planes before removing the
> existing framebuffers, then recreate them with the new dimensions.
>
> Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com>
> Suggested-by: Karthik B.S <karthik.b.s@intel.com>
> ---
>   tests/kms_content_protection.c | 216 +++++++++++++++++++++++++--------
>   1 file changed, 168 insertions(+), 48 deletions(-)
>
> diff --git a/tests/kms_content_protection.c b/tests/kms_content_protection.c
> index f35c9fa73a57..d470a8de5700 100644
> --- a/tests/kms_content_protection.c
> +++ b/tests/kms_content_protection.c
> @@ -107,10 +107,15 @@
>   
>   IGT_TEST_DESCRIPTION("Test content protection (HDCP)");
>   
> +struct hdcp_test_fbs {
> +	struct igt_fb red;
> +	struct igt_fb green;
> +};
> +
>   struct data {
>   	int drm_fd;
>   	igt_display_t display;
> -	struct igt_fb red, green;
> +	struct hdcp_test_fbs fbs[IGT_MAX_PIPES];
>   	unsigned int cp_tests;
>   	struct udev_monitor *uevent_monitor;
>   	bool is_force_hdcp14;
> @@ -257,7 +262,15 @@ commit_display_and_wait_for_flip(enum igt_commit_style commit_style)
>   	}
>   }
>   
> -static void modeset_with_fb(igt_output_t *output,
> +static igt_crtc_t *output_get_driving_crtc_assert(igt_output_t *output)
> +{
> +	igt_crtc_t *crtc = igt_output_get_driving_crtc(output);
> +
> +	igt_assert(crtc);
> +	return crtc;
> +}
> +
> +static void modeset_with_fb(igt_output_t *output, igt_crtc_t *crtc,
>   			    enum igt_commit_style commit_style)
>   {
>   	igt_display_t *display = &data.display;
> @@ -267,18 +280,19 @@ static void modeset_with_fb(igt_output_t *output,
>   	mode = igt_output_get_mode(output);
>   
>   	primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> -	igt_plane_set_fb(primary, &data.red);
> -	igt_fb_set_size(&data.red, primary, mode->hdisplay, mode->vdisplay);
> +	igt_plane_set_fb(primary, &data.fbs[crtc->crtc_index].red);
> +	igt_fb_set_size(&data.fbs[crtc->crtc_index].red, primary, mode->hdisplay, mode->vdisplay);
>   
>   	igt_display_commit2(display, commit_style);
>   
> -	igt_plane_set_fb(primary, &data.green);
> +	igt_plane_set_fb(primary, &data.fbs[crtc->crtc_index].green);
>   
>   	/* Wait for Flip completion before starting the HDCP authentication */
>   	commit_display_and_wait_for_flip(commit_style);
>   }
>   
> -static bool test_cp_enable(igt_output_t *output, enum igt_commit_style commit_style,
> +static bool test_cp_enable(igt_output_t *output, igt_crtc_t *crtc,
> +			   enum igt_commit_style commit_style,
>   			   int content_type, bool type_change)
>   {
>   	igt_display_t *display = &data.display;
> @@ -301,7 +315,7 @@ static bool test_cp_enable(igt_output_t *output, enum igt_commit_style commit_st
>   	ret = wait_for_prop_value(output, CP_ENABLED,
>   				  KERNEL_AUTH_TIME_ALLOWED_MSEC);
>   	if (ret) {
> -		igt_plane_set_fb(primary, &data.green);
> +		igt_plane_set_fb(primary, &data.fbs[crtc->crtc_index].green);
>   		igt_display_commit2(display, commit_style);
>   	}
>   
> @@ -314,13 +328,15 @@ static void test_mst_cp_disable(igt_output_t *hdcp_mst_output[],
>   {
>   	igt_display_t *display = &data.display;
>   	igt_plane_t *primary;
> +	igt_crtc_t *crtc;
>   	bool ret;
>   	int count;
>   	u64 val;
>   
>   	for (count = 0; count < valid_outputs; count++) {
> +		crtc = output_get_driving_crtc_assert(hdcp_mst_output[count]);
>   		primary = igt_output_get_plane_type(hdcp_mst_output[count], DRM_PLANE_TYPE_PRIMARY);
> -		igt_plane_set_fb(primary, &data.red);
> +		igt_plane_set_fb(primary, &data.fbs[crtc->crtc_index].red);
>   		igt_output_set_prop_value(hdcp_mst_output[count], IGT_CONNECTOR_CONTENT_PROTECTION,
>   					  CP_UNDESIRED);
>   	}
> @@ -338,7 +354,8 @@ static void test_mst_cp_disable(igt_output_t *hdcp_mst_output[],
>   	igt_assert_f(ret, "Content Protection not cleared on all MST outputs\n");
>   }
>   
> -static void test_cp_disable(igt_output_t *output, enum igt_commit_style commit_style)
> +static void test_cp_disable(igt_output_t *output, igt_crtc_t *crtc,
> +			    enum igt_commit_style commit_style)
>   {
>   	igt_display_t *display = &data.display;
>   	igt_plane_t *primary;
> @@ -352,7 +369,7 @@ static void test_cp_disable(igt_output_t *output, enum igt_commit_style commit_s
>   	 */
>   	igt_output_set_prop_value(output, IGT_CONNECTOR_CONTENT_PROTECTION,
>   				  CP_UNDESIRED);
> -	igt_plane_set_fb(primary, &data.red);
> +	igt_plane_set_fb(primary, &data.fbs[crtc->crtc_index].red);
>   	igt_display_commit2(display, commit_style);
>   
>   	/* Wait for HDCP to be disabled, before crtc off */
> @@ -361,7 +378,7 @@ static void test_cp_disable(igt_output_t *output, enum igt_commit_style commit_s
>   	igt_assert_f(ret, "Content Protection not cleared\n");
>   }
>   
> -static void test_cp_enable_with_retry(igt_output_t *output,
> +static void test_cp_enable_with_retry(igt_output_t *output, igt_crtc_t *crtc,
>   				      enum igt_commit_style commit_style,
>   				      int retry, int content_type,
>   				      bool expect_failure,
> @@ -372,16 +389,16 @@ static void test_cp_enable_with_retry(igt_output_t *output,
>   
>   	do {
>   		if (!type_change || retry_orig != retry)
> -			test_cp_disable(output, commit_style);
> +			test_cp_disable(output, crtc, commit_style);
>   
> -		ret = test_cp_enable(output, commit_style, content_type, type_change);
> +		ret = test_cp_enable(output, crtc, commit_style, content_type, type_change);
>   
>   		if (!ret && --retry)
>   			igt_debug("Retry (%d/2) ...\n", 3 - retry);
>   	} while (retry && !ret);
>   
>   	if (!ret)
> -		test_cp_disable(output, commit_style);
> +		test_cp_disable(output, crtc, commit_style);
>   
>   	if (expect_failure)
>   		igt_assert_f(!ret,
> @@ -451,16 +468,16 @@ static void test_content_protection_on_output(igt_output_t *output,
>   	igt_display_t *display = &data.display;
>   	bool ret;
>   
> -	test_cp_enable_with_retry(output, commit_style, 3, content_type, false,
> +	test_cp_enable_with_retry(output, crtc, commit_style, 3, content_type, false,
>   				  false);
>   
>   	if (data.cp_tests & CP_TYPE_CHANGE) {
>   		/* Type 1 -> Type 0 */
> -		test_cp_enable_with_retry(output, commit_style, 3,
> +		test_cp_enable_with_retry(output, crtc, commit_style, 3,
>   					  HDCP_CONTENT_TYPE_0, false,
>   					  true);
>   		/* Type 0 -> Type 1 */
> -		test_cp_enable_with_retry(output, commit_style, 3,
> +		test_cp_enable_with_retry(output, crtc, commit_style, 3,
>   					  content_type, false,
>   					  true);
>   	}
> @@ -470,14 +487,14 @@ static void test_content_protection_on_output(igt_output_t *output,
>   			     "mei_hdcp unload failed");
>   
>   		/* Expected to fail */
> -		test_cp_enable_with_retry(output, commit_style, 3,
> +		test_cp_enable_with_retry(output, crtc, commit_style, 3,
>   					  content_type, true, false);
>   
>   		igt_assert_f(!igt_kmod_load("mei_hdcp", NULL),
>   			     "mei_hdcp load failed");
>   
>   		/* Expected to pass */
> -		test_cp_enable_with_retry(output, commit_style, 3,
> +		test_cp_enable_with_retry(output, crtc, commit_style, 3,
>   					  content_type, false, false);
>   	}
>   
> @@ -496,7 +513,7 @@ static void test_content_protection_on_output(igt_output_t *output,
>   		ret = wait_for_prop_value(output, CP_ENABLED,
>   					  KERNEL_AUTH_TIME_ALLOWED_MSEC);
>   		if (!ret)
> -			test_cp_enable_with_retry(output, commit_style, 2,
> +			test_cp_enable_with_retry(output, crtc, commit_style, 2,
>   						  content_type, false,
>   						  false);
>   	}
> @@ -507,7 +524,7 @@ static void test_content_protection_on_output(igt_output_t *output,
>   		ret = wait_for_prop_value(output, CP_ENABLED,
>   					  KERNEL_AUTH_TIME_ALLOWED_MSEC);
>   		if (!ret)
> -			test_cp_enable_with_retry(output, commit_style, 2,
> +			test_cp_enable_with_retry(output, crtc, commit_style, 2,
>   						  content_type, false,
>   						  false);
>   	}
> @@ -577,21 +594,24 @@ static bool sink_hdcp2_capable(igt_output_t *output)
>   	return strstr(buf, "HDCP2.2");
>   }
>   
> -static void prepare_modeset_on_mst_output(igt_output_t *output, bool is_enabled)
> +static void prepare_modeset_on_mst_output(igt_output_t *output, igt_crtc_t *crtc, bool is_enabled)
>   {
>   	drmModeModeInfo *mode;
>   	igt_plane_t *primary;
>   	int width, height;
> +	struct igt_fb *fb;
>   
>   	mode = igt_output_get_mode(output);
>   
>   	width = mode->hdisplay;
>   	height = mode->vdisplay;
>   
> +	fb = is_enabled ? &data.fbs[crtc->crtc_index].green : &data.fbs[crtc->crtc_index].red;
> +
>   	primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
>   	igt_plane_set_fb(primary, NULL);
> -	igt_plane_set_fb(primary, is_enabled ? &data.green : &data.red);
> -	igt_fb_set_size(is_enabled ? &data.green : &data.red, primary, width, height);
> +	igt_plane_set_fb(primary, fb);
> +	igt_fb_set_size(fb, primary, width, height);
>   	igt_plane_set_size(primary, width, height);
>   }
>   
> @@ -663,11 +683,11 @@ static void reset_i915_force_hdcp14(igt_output_t *output)
>   }
>   
>   static void
> -test_fini(igt_output_t *output, enum igt_commit_style commit_style)
> +test_fini(igt_output_t *output, igt_crtc_t *crtc, enum igt_commit_style commit_style)
>   {
>   	igt_plane_t *primary;
>   
> -	test_cp_disable(output, commit_style);
> +	test_cp_disable(output, crtc, commit_style);
>   	primary = igt_output_get_plane_type(output,
>   					    DRM_PLANE_TYPE_PRIMARY);
>   	igt_plane_set_fb(primary, NULL);
> @@ -739,7 +759,7 @@ test_content_protection(enum igt_commit_style commit_style, int content_type)
>   			if (!intel_pipe_output_combo_valid(display))
>   				continue;
>   
> -			modeset_with_fb(output, commit_style);
> +			modeset_with_fb(output, crtc, commit_style);
>   			if (data.is_force_hdcp14)
>   				set_i915_force_hdcp14(output);
>   
> @@ -753,7 +773,8 @@ test_content_protection(enum igt_commit_style commit_style, int content_type)
>   			if (data.is_force_hdcp14)
>   				reset_i915_force_hdcp14(output);
>   
> -			test_fini(output, commit_style);
> +			test_fini(output, crtc, commit_style);
> +
>   			/*
>   			 * Testing a output with a pipe is enough for HDCP
>   			 * testing. No ROI in testing the connector with other
> @@ -846,8 +867,11 @@ test_mst_cp_enable_with_retry(igt_output_t *hdcp_mst_output[], int valid_outputs
>   		if (!ret || retries)
>   			igt_debug("Retry %d/3\n", 3 - retries);
>   
> -		for (i = 0; i < valid_outputs; i++)
> -			prepare_modeset_on_mst_output(hdcp_mst_output[i], ret);
> +		for (i = 0; i < valid_outputs; i++) {
> +			igt_crtc_t *crtc = output_get_driving_crtc_assert(hdcp_mst_output[i]);
> +
> +			prepare_modeset_on_mst_output(hdcp_mst_output[i], crtc, ret);
> +		}
>   
>   		igt_display_commit2(display, COMMIT_ATOMIC);
>   	} while (retries && !ret);
> @@ -869,6 +893,7 @@ test_content_protection_mst(int content_type)
>   	igt_crtc_t *crtc;
>   	bool pipe_found;
>   	igt_output_t *hdcp_mst_output[IGT_MAX_PIPES];
> +	drmModeModeInfo *mode;
>   
>   	for_each_crtc(display, crtc)
>   		max_pipe++;
> @@ -889,7 +914,34 @@ test_content_protection_mst(int content_type)
>   		igt_assert_f(pipe_found, "No valid pipe found for %s\n", output->name);
>   
>   		igt_output_set_crtc(output, crtc);
> -		prepare_modeset_on_mst_output(output, false);
> +
> +		mode = igt_output_get_mode(output);
> +
> +		/*
> +		 * Check if framebuffers already exist for this crtc from create_fbs().
> +		 * Since non-MST tests run first and share the same data.fbs[] array,
> +		 * the existing FB may have a different resolution than this MST output
> +		 * requires. If so, properly detach planes, remove old FBs, and recreate
> +		 * with the correct dimensions for this MST output.
> +		 */
> +		if (data.fbs[crtc->crtc_index].red.fb_id) {
> +			igt_plane_t *primary = igt_output_get_plane_type(output,
> +									 DRM_PLANE_TYPE_PRIMARY);
> +
> +			igt_plane_set_fb(primary, NULL);
> +			igt_display_commit2(display, COMMIT_ATOMIC);
> +			igt_remove_fb(data.drm_fd, &data.fbs[crtc->crtc_index].red);
> +			igt_remove_fb(data.drm_fd, &data.fbs[crtc->crtc_index].green);
> +		}
> +
> +		igt_create_color_fb(data.drm_fd, mode->hdisplay, mode->vdisplay,
> +				    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> +				    1.f, 0.f, 0.f, &data.fbs[crtc->crtc_index].red);
> +		igt_create_color_fb(data.drm_fd, mode->hdisplay, mode->vdisplay,
> +				    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> +				    0.f, 1.f, 0.f, &data.fbs[crtc->crtc_index].green);
> +

Do we really need this recreation here before we even tried 
'igt_override_all_active_output_modes_to_fit_bw'? Because I'm thinking 
that the mode we get here should be the same as we got earlier in the 
create_fbs?

Regards,
Karthik.B.S
> +		prepare_modeset_on_mst_output(output, crtc, false);
>   		dp_mst_outputs++;
>   		if (output_hdcp_capable(output, content_type))
>   			hdcp_mst_output[valid_outputs++] = output;
> @@ -905,9 +957,31 @@ test_content_protection_mst(int content_type)
>   		bool found = igt_override_all_active_output_modes_to_fit_bw(display);
>   		igt_require_f(found, "No valid mode combo found for MST modeset\n");
>   
> -		for (count = 0; count < valid_outputs; count++)
> -			prepare_modeset_on_mst_output(hdcp_mst_output[count], false);
> +		/* Detach planes before removing framebuffers */
> +		for (count = 0; count < valid_outputs; count++) {
> +			crtc = output_get_driving_crtc_assert(hdcp_mst_output[count]);
> +			prepare_modeset_on_mst_output(hdcp_mst_output[count], crtc, false);
> +		}
> +		igt_display_commit2(display, COMMIT_ATOMIC);
> +
> +		/* Need to re-prepare after mode override */
> +		for (count = 0; count < valid_outputs; count++) {
> +			crtc = output_get_driving_crtc_assert(hdcp_mst_output[count]);
> +
> +			igt_remove_fb(data.drm_fd, &data.fbs[crtc->crtc_index].red);
> +			igt_remove_fb(data.drm_fd, &data.fbs[crtc->crtc_index].green);
> +
> +			mode = igt_output_get_mode(hdcp_mst_output[count]);
> +
> +			igt_create_color_fb(data.drm_fd, mode->hdisplay, mode->vdisplay,
> +					    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> +					    1.f, 0.f, 0.f, &data.fbs[crtc->crtc_index].red);
> +			igt_create_color_fb(data.drm_fd, mode->hdisplay, mode->vdisplay,
> +					    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> +					    0.f, 1.f, 0.f, &data.fbs[crtc->crtc_index].green);
>   
> +			prepare_modeset_on_mst_output(hdcp_mst_output[count], crtc, false);
> +		}
>   		ret = igt_display_try_commit2(display, COMMIT_ATOMIC);
>   		igt_require_f(ret == 0, "Commit failure during MST modeset\n");
>   	}
> @@ -933,8 +1007,9 @@ test_content_protection_mst(int content_type)
>   	/*
>   	 * Verify if CP is still enabled on other outputs by disabling CP on the first output.
>   	 */
> +	crtc = output_get_driving_crtc_assert(hdcp_mst_output[0]);
>   	igt_debug("CP Prop being UNDESIRED on %s\n", hdcp_mst_output[0]->name);
> -	test_cp_disable(hdcp_mst_output[0], COMMIT_ATOMIC);
> +	test_cp_disable(hdcp_mst_output[0], crtc, COMMIT_ATOMIC);
>   
>   	/* CP is expected to be still enabled on other outputs*/
>   	for (i = 1; i < valid_outputs; i++) {
> @@ -945,13 +1020,26 @@ test_content_protection_mst(int content_type)
>   
>   	if (data.cp_tests & CP_LIC)
>   		test_cp_lic_on_mst(hdcp_mst_output, valid_outputs, 1);
> -}
>   
> +	/* Detach planes before removing framebuffers */
> +	for (count = 0; count < valid_outputs; count++) {
> +		crtc = output_get_driving_crtc_assert(hdcp_mst_output[count]);
> +		prepare_modeset_on_mst_output(hdcp_mst_output[count], crtc, false);
> +	}
> +	igt_display_commit2(display, COMMIT_ATOMIC);
> +
> +	for (count = 0; count < valid_outputs; count++) {
> +		crtc = output_get_driving_crtc_assert(hdcp_mst_output[count]);
> +		igt_remove_fb(data.drm_fd, &data.fbs[crtc->crtc_index].red);
> +		igt_remove_fb(data.drm_fd, &data.fbs[crtc->crtc_index].green);
> +	}
> +}
>   
>   static void test_content_protection_cleanup(void)
>   {
>   	igt_display_t *display = &data.display;
>   	igt_output_t *output;
> +	igt_crtc_t *crtc;
>   	uint64_t val;
>   
>   	for_each_connected_output(display, output) {
> @@ -963,34 +1051,56 @@ static void test_content_protection_cleanup(void)
>   		if (val == CP_UNDESIRED)
>   			continue;
>   
> +		crtc = igt_output_get_driving_crtc(output);
> +		if (!crtc)
> +			continue;
> +
>   		igt_info("CP Prop being UNDESIRED on %s\n", output->name);
> -		test_cp_disable(output, display->is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> +		test_cp_disable(output, crtc, display->is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
>   	}
> -
> -	igt_remove_fb(data.drm_fd, &data.red);
> -	igt_remove_fb(data.drm_fd, &data.green);
>   }
>   
>   static void create_fbs(void)
>   {
> -	uint16_t width = 0, height = 0;
>   	drmModeModeInfo *mode;
>   	igt_output_t *output;
> +	igt_crtc_t *crtc;
>   
> +	/* Create framebuffers for each connected output's pipe */
>   	for_each_connected_output(&data.display, output) {
>   		mode = igt_output_get_mode(output);
>   		igt_assert(mode);
>   
> -		width = max(width, mode->hdisplay);
> -		height = max(height, mode->vdisplay);
> +		/* Find a valid crtc for this output */
> +		for_each_crtc(&data.display, crtc) {
> +			if (!igt_crtc_connector_valid(crtc, output))
> +				continue;
> +
> +			/* Skip if already created for this crtc */
> +			if (data.fbs[crtc->crtc_index].red.fb_id)
> +				continue;
> +
> +			igt_create_color_fb(data.drm_fd, mode->hdisplay, mode->vdisplay,
> +					    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> +					    1.f, 0.f, 0.f, &data.fbs[crtc->crtc_index].red);
> +			igt_create_color_fb(data.drm_fd, mode->hdisplay, mode->vdisplay,
> +					    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> +					    0.f, 1.f, 0.f, &data.fbs[crtc->crtc_index].green);
> +			break;
> +		}
>   	}
> +}
>   
> -	igt_create_color_fb(data.drm_fd, width, height,
> -			    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> -			    1.f, 0.f, 0.f, &data.red);
> -	igt_create_color_fb(data.drm_fd, width, height,
> -			    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> -			    0.f, 1.f, 0.f, &data.green);
> +static void remove_fbs(void)
> +{
> +	igt_crtc_t *crtc;
> +
> +	for_each_crtc(&data.display, crtc) {
> +		if (data.fbs[crtc->crtc_index].red.fb_id)
> +			igt_remove_fb(data.drm_fd, &data.fbs[crtc->crtc_index].red);
> +		if (data.fbs[crtc->crtc_index].green.fb_id)
> +			igt_remove_fb(data.drm_fd, &data.fbs[crtc->crtc_index].green);
> +	}
>   }
>   
>   static const struct {
> @@ -1155,9 +1265,18 @@ static const struct {
>   int igt_main()
>   {
>   	igt_fixture() {
> +		igt_crtc_t *crtc;
> +
>   		data.drm_fd = drm_open_driver_master(DRIVER_ANY);
>   		igt_display_require(&data.display, data.drm_fd);
>   		igt_display_require_output(&data.display);
> +
> +		/* Ensure all CRTCs can be indexed within our framebuffer array */
> +		for_each_crtc(&data.display, crtc)
> +			igt_assert_f(crtc->crtc_index < IGT_MAX_PIPES,
> +				     "crtc_index %u exceeds IGT_MAX_PIPES %u\n",
> +				     crtc->crtc_index, IGT_MAX_PIPES);
> +
>   		create_fbs();
>   	}
>   
> @@ -1216,6 +1335,7 @@ int igt_main()
>   
>   	igt_fixture() {
>   		test_content_protection_cleanup();
> +		remove_fbs();
>   		igt_display_fini(&data.display);
>   		drm_close_driver(data.drm_fd);
>   	}