[igt-dev] [PATCH i-g-t 2/3] tests/i915/kms_dsc: Prep work for extending val support for VDSC YCbCr420

[Nautiyal, Ankit K]

Hi Swati,

Please find the comments inline:

On 10/31/2022 11:46 PM, Swati Sharma wrote:
> Functions are modified to accommodate changes for VDSC YCbCr420.
>
> Signed-off-by: Swati Sharma <swati2.sharma at intel.com>
> ---
>   tests/i915/kms_dsc.c | 29 +++++++++++++++++++----------
>   1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/tests/i915/kms_dsc.c b/tests/i915/kms_dsc.c
> index 330fc050..b162f93d 100644
> --- a/tests/i915/kms_dsc.c
> +++ b/tests/i915/kms_dsc.c
> @@ -61,6 +61,7 @@ typedef struct {
>   	igt_output_t *output;
>   	int input_bpc;
>   	int n_pipes;
> +	int disp_ver;
>   	enum pipe pipe;
>   } data_t;
>   
> @@ -91,7 +92,7 @@ static void force_dsc_enable(data_t *data)
>   	igt_debug("Forcing DSC enable on %s\n", data->output->name);
>   	ret = igt_force_dsc_enable(data->drm_fd,
>   				   data->output->name);
> -	igt_assert_f(ret > 0, "debugfs_write failed");
> +	igt_assert_f(ret > 0, "forcing dsc enable debugfs_write failed\n");
>   }
>   
>   static void force_dsc_enable_bpc(data_t *data)
> @@ -103,7 +104,7 @@ static void force_dsc_enable_bpc(data_t *data)
>   	ret = igt_force_dsc_enable_bpc(data->drm_fd,
>   				       data->output->name,
>   				       data->input_bpc);
> -	igt_assert_f(ret > 0, "debugfs_write failed");
> +	igt_assert_f(ret > 0, "forcing input dsc bpc debugfs_write failed\n");
>   }
>   
>   static void save_force_dsc_en(data_t *data)
> @@ -222,7 +223,8 @@ static void test_cleanup(data_t *data)
>   }
>   
>   /* re-probe connectors and do a modeset with DSC */
> -static void update_display(data_t *data, enum dsc_test_type test_type, unsigned int plane_format)
> +static void update_display(data_t *data, enum dsc_test_type test_type,
> +			   unsigned int plane_format, int flag)


Flag here doesnt seem to have any meaning here. Perhaps use bool 
test_dsc_ycbcr420.

IMHO, If source and sink both support ycbcr420 then call test_dsc with 
the flag set.

Or otherwise define a u8 test_flag, with each bit representing some test 
configuration like DSC+Ycbcr420, DSC+Fractional etc and other future 
uses perhaps.

In that case too the bits of test flag should be set in run_test, based 
on source+sink support.

Regards,

Ankit



>   {
>   	bool enabled;
>   	igt_plane_t *primary;
> @@ -290,7 +292,7 @@ static void update_display(data_t *data, enum dsc_test_type test_type, unsigned
>   }
>   
>   static void test_dsc(data_t *data, enum dsc_test_type test_type, int bpc,
> -		     unsigned int plane_format)
> +		     unsigned int plane_format, int flag)
>   {
>   	igt_display_t *display = &data->display;
>   	igt_output_t *output;
> @@ -320,10 +322,16 @@ static void test_dsc(data_t *data, enum dsc_test_type test_type, int bpc,
>   			snprintf(name, sizeof(name), "-%s", igt_format_str(plane_format));
>   
>   		igt_dynamic_f("pipe-%s-%s%s",  kmstest_pipe_name(data->pipe), data->output->name, name)
> -			update_display(data, test_type, plane_format);
> +			update_display(data, test_type, plane_format, flag);
>   	}
>   }
>   
> +static void run_test(data_t *data, enum dsc_test_type test_type, int bpc,
> +		     unsigned int plane_format)
> +{
> +	test_dsc(data, test_type, bpc, plane_format, 0);
> +}
> +
>   igt_main
>   {
>   	data_t data = {};
> @@ -332,11 +340,12 @@ igt_main
>   	igt_fixture {
>   		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
>   		data.devid = intel_get_drm_devid(data.drm_fd);
> +		data.disp_ver = intel_display_ver(data.devid);
>   		kmstest_set_vt_graphics_mode();
>   		igt_install_exit_handler(kms_dsc_exit_handler);
>   		igt_display_require(&data.display, data.drm_fd);
>   		igt_display_require_output(&data.display);
> -		igt_require(intel_display_ver(data.devid) >= 11);
> +		igt_require(data.disp_ver >= 11);
>   		data.n_pipes = 0;
>   		for_each_pipe(&data.display, i)
>   			data.n_pipes++;
> @@ -346,14 +355,14 @@ igt_main
>   		     "by a connector by forcing DSC on all connectors that support it "
>   		     "with default parameters");
>   	igt_subtest_with_dynamic("basic-dsc")
> -			test_dsc(&data, TEST_DSC_BASIC, 0, DRM_FORMAT_XRGB8888);
> +			run_test(&data, TEST_DSC_BASIC, 0, DRM_FORMAT_XRGB8888);
>   
>   	igt_describe("Tests basic display stream compression functionality if supported "
>   		     "by a connector by forcing DSC on all connectors that support it "
>   		     "with default parameters and creating fb with diff formats");
>   	igt_subtest_with_dynamic("dsc-with-formats") {
>   		for (int k = 0; k < ARRAY_SIZE(format_list); k++)
> -			test_dsc(&data, TEST_DSC_BASIC, 0, format_list[k].format);
> +			run_test(&data, TEST_DSC_BASIC, 0, format_list[k].format);
>   	}
>   
>   	igt_describe("Tests basic display stream compression functionality if supported "
> @@ -361,7 +370,7 @@ igt_main
>   		     "with certain input BPC for the connector");
>   	igt_subtest_with_dynamic("dsc-with-bpc") {
>   		for (int j = 0; j < ARRAY_SIZE(bpc_list); j++)
> -			test_dsc(&data, TEST_DSC_BPC, bpc_list[j], DRM_FORMAT_XRGB8888);
> +			run_test(&data, TEST_DSC_BPC, bpc_list[j], DRM_FORMAT_XRGB8888);
>   	}
>   
>   	igt_describe("Tests basic display stream compression functionality if supported "
> @@ -370,7 +379,7 @@ igt_main
>   	igt_subtest_with_dynamic("dsc-with-bpc-formats") {
>   		for (int j = 0; j < ARRAY_SIZE(bpc_list); j++) {
>   			for (int k = 0; k < ARRAY_SIZE(format_list); k++) {
> -				test_dsc(&data, TEST_DSC_BPC, bpc_list[j], format_list[k].format);
> +				run_test(&data, TEST_DSC_BPC, bpc_list[j], format_list[k].format);
>   			}
>   		}
>   	}