Re: [PATCH i-g-t] RFC tests/intel/kms_frontbuffer_tracking: Introduce plane-fbc-dr test

["Naladala, Ramanaidu" <Ramanaidu.naladala@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5537 bytes --]

Hi Santhosh,

Still observed some styling issues in the patch.

On 9/9/2024 9:44 AM, Santhosh Reddy Guddati wrote:
> Use the FP_DAMAGE_CLIPS property of the plane for non psr modes
> and fill the damaged rectangles on the plane with framebuffer
> coordinates and verify that fbc dirty ctl is enabled.
>
> v2: Fix typo , add version check for feature support,
>      extend support for all pipes (Rama Naidu)
>
> Signed-off-by: Santhosh Reddy Guddati<santhosh.reddy.guddati@intel.com>
> ---
>   tests/intel/kms_frontbuffer_tracking.c | 78 ++++++++++++++++++++++++++
>   1 file changed, 78 insertions(+)
>
> diff --git a/tests/intel/kms_frontbuffer_tracking.c b/tests/intel/kms_frontbuffer_tracking.c
> index e45d17dd6..b15527209 100644
> --- a/tests/intel/kms_frontbuffer_tracking.c
> +++ b/tests/intel/kms_frontbuffer_tracking.c
> @@ -57,6 +57,10 @@
>    * Description: Sanity test to enable FBC on a plane.
>    * Functionality: fbc
>    *
> + * SUBTEST: plane-fbc-dr
> + * Description: Sanity test to verify FBC DR by sending multiple damaged areas with non psr modes
> + * Functionality: fbc
> + *
>    * SUBTEST: pipe-fbc-rte
>    * Description: Sanity test to enable FBC on each pipe.
>    * Functionality: fbc
> @@ -1168,6 +1172,7 @@
>    */
>   
>   #define TIME SLOW_QUICK(1000, 10000)
> +#define MAX_DIRTY_RECT 3
>   
>   IGT_TEST_DESCRIPTION("Test the Kernel's frontbuffer tracking mechanism and "
>   		     "its related features: FBC, PSR and DRRS");
> @@ -4288,6 +4293,73 @@ static int opt_handler(int option, int option_index, void *data)
>   	return IGT_OPT_HANDLER_SUCCESS;
>   }
>   
> +static void fbc_dr_subtest(const struct test_mode *t) {
> +	struct drm_mode_rect rect_data[MAX_DIRTY_RECT];
> +	uint32_t color;
> +	drmModeModeInfo *mode;
> +
> +	igt_require_f(drm.display_ver > 20, "FBC with dirty region is not supported\n");
imho, Add this Display version check into main function before dynamic 
test start.
> +
> +	prepare_subtest_data(t, NULL);
> +
> +	igt_output_override_mode(prim_mode_params.output, &prim_mode_params.mode);
> +	igt_output_set_pipe(prim_mode_params.output, prim_mode_params.pipe);
> +
> +	mode = igt_output_get_mode(prim_mode_params.output);
> +
> +	for (int i = 0; i < MAX_DIRTY_RECT; i++) {
> +		// Full Frame update
> +		rect_data[0].x1 = 0;
> +		rect_data[0].y1 = 0;
> +		rect_data[0].x2 = mode->hdisplay;
> +		rect_data[0].y2 = mode->vdisplay;
x2 and y2 should be mode->*display -1.
> +
> +		// Half Frame update till last-1 scanline
> +		rect_data[1].x1 = mode->hdisplay / 2;
> +		rect_data[1].y1 = mode->vdisplay / 2;
> +		rect_data[1].x2 = mode->hdisplay - 1;
> +		rect_data[1].y2 = mode->vdisplay - 1;
> +
> +		// last 10 scanlines update
> +		rect_data[2].x1 = mode->hdisplay - 10;
> +		rect_data[2].y1 = mode->vdisplay - 10;
> +		rect_data[2].x2 = mode->hdisplay - 1;
> +		rect_data[2].y2 = mode->vdisplay - 1;
> +
> +		igt_debug("Dirty Rectangle data = {%d, %d, %d, %d}, {%d, %d, %d, %d}, {%d, %d, %d, %d}\n",
> +					rect_data[0].x1, rect_data[0].y1, rect_data[0].x2, rect_data[0].y2,
> +					rect_data[1].x1, rect_data[1].y1, rect_data[1].x2, rect_data[1].y2,
> +					rect_data[2].x1, rect_data[2].y1, rect_data[2].x2, rect_data[2].y2);

Shouldn't this be aligned with the opening brackets for readability.

> +	}
> +
> +	for(int i = 0; i < MAX_DIRTY_RECT; i++) {
> +		igt_plane_t *primary = igt_output_get_plane_type(prim_mode_params.output, DRM_PLANE_TYPE_PRIMARY);
> +		igt_fb_t fb[MAX_DIRTY_RECT];
> +
> +		create_fb(t->format, prim_mode_params.mode.hdisplay, prim_mode_params.mode.vdisplay, t->tiling, t->plane, &fb[i]);
> +		color = pick_color(&fb[i], COLOR_PRIM_BG);
> +		igt_draw_rect_fb(drm.fd, drm.bops, 0, &fb[i], t->method,
> +			rect_data[i].x1, rect_data[i].y1, rect_data[i].x2 - rect_data[i].x1, rect_data[i].y2 - rect_data[i].y1,
> +			color);

Shouldn't this be aligned with the opening brackets for readability.

> +
> +		igt_plane_set_fb(primary, &fb[i]);
> +		igt_plane_set_position(primary, rect_data[i].x1, rect_data[i].y1);
> +		igt_plane_set_size(primary, rect_data[i].x2 - rect_data[i].x1, rect_data[i].y2 - rect_data[i].y1);
> +		igt_fb_set_size(&fb[i], primary, rect_data[i].x2 - rect_data[i].x1, rect_data[i].y2 - rect_data[i].y1);
> +		igt_plane_replace_prop_blob(primary, IGT_PLANE_FB_DAMAGE_CLIPS, &rect_data[i], sizeof(rect_data[i]));
> +
> +		igt_display_commit_atomic(&drm.display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> +
> +		fbc_update_last_action();
> +		do_assertions(ASSERT_FBC_ENABLED | ASSERT_NO_ACTION_CHANGE);
> +		igt_assert_f(fbc_enable_per_plane(primary->index + 1, prim_mode_params.pipe), "FBC disabled\n");
> +
> +		igt_remove_fb(drm.fd, &fb[i]);
> +		igt_plane_set_fb(primary, NULL);
> +		igt_display_commit2(&drm.display, COMMIT_ATOMIC);
> +	}
> +}
> +
>   const char *help_str =
>   "  --no-status-check           Don't check for enable/disable status\n"
>   "  --no-crc-check              Don't check for CRC values\n"
> @@ -4513,6 +4585,12 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
>   		plane_fbc_rte_subtest(&t);
>   	}
>   
> +	igt_subtest_f("plane-fbc-dr") {
> +		t.feature = FEATURE_FBC;
> +		igt_dynamic_f("pipe-%s", kmstest_pipe_name(prim_mode_params.pipe))
> +			fbc_dr_subtest(&t);
> +	}
> +
>   	igt_subtest_group {
>   		igt_subtest_with_dynamic("pipe-fbc-rte") {
>   


imho,
Add one more subtest to create dirty rectangles in different places in a 
frame (e.g., dr1: (0,50), dr2: center of the frame, dr3: last 50 lines). 
Then, provide one atomic commit for all three dirty rectangles.

[-- Attachment #2: Type: text/html, Size: 8341 bytes --]