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

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 27 Mar 2026, Jason-JH Lin <jason-jh.lin@mediatek.com> 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.

Feels like there's a bunch of changes grouped together in one commit
that should probably be split up.

For example, passing the crtc around is a change. Adding the fb cleanup
is another. Adding multiple fbs is another.

The point is, if a bisected regression landed here, would you be able to
immediately tell what the problem is?

> Suggested-by: Karthik B.S <karthik.b.s@intel.com>
> Reviewed-by: Fei Shao <fshao@chromium.org>
> Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com>
> ---
>  tests/kms_content_protection.c | 190 ++++++++++++++++++++++++---------
>  1 file changed, 142 insertions(+), 48 deletions(-)
>
> diff --git a/tests/kms_content_protection.c b/tests/kms_content_protection.c
> index caf3d7a56ae4..8b131277ffea 100644
> --- a/tests/kms_content_protection.c
> +++ b/tests/kms_content_protection.c

> @@ -258,7 +263,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;
> +}

I'm personally not fond of abstracting/hiding asserts like this. Do you
even need it really? It's a test logic issue if you hit this.

> +
> +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;
> @@ -268,18 +281,19 @@ static void modeset_with_fb(igt_output_t *output,


> @@ -1180,9 +1264,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);

This is handled by igt_display_require(), and is completely unnecessary
here.

BR,
Jani.


> +
>  		create_fbs();
>  	}
>  
> @@ -1241,6 +1334,7 @@ int igt_main()

-- 
Jani Nikula, Intel