Re: [igt-dev] [RFC i-g-t 1/2] tests/kms_content_protection: Bifurcate tests into HDCP1.4 & 2.2

[Ramalingam C <ramalingam.c@intel.com>]

On 2020-06-01 at 14:09:59 +0530, Ankit Nautiyal wrote:
> Currently, the test kms_content_protection does not distinguish
> between HDCP1.4 and HDCP2.2 tests. Since the driver by default
> always chooses HDCP2.2 protocol if the panel supports HDCP1.4 &
> HDCP2.2, HDCP1.4 does not get tested.
> 
> This patch makes two versions of the existing subtests for testing
> HDCP1.4 and HDCP2.2. For making sure that the required HDCP version
> is chosen by the kernel, the test utilizes the debugfs
> 'i915_hdcp_version_request' for each HDCP supporting connector.
> The desired HDCP version is written via the debugfs and is considered
> by the kernel, if the platform and the panel support that
> HDCP version.
> 
> This patch also makes the existing subtests as dynamic sub-subtests
> based on all outputs supporting the protocol under test.
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  tests/kms_content_protection.c | 230 ++++++++++++++++++++++-----------
>  1 file changed, 158 insertions(+), 72 deletions(-)
> 
> diff --git a/tests/kms_content_protection.c b/tests/kms_content_protection.c
> index 3b9cedcb..a6a64a28 100644
> --- a/tests/kms_content_protection.c
> +++ b/tests/kms_content_protection.c
> @@ -59,6 +59,9 @@ struct data {
>  #define HDCP_CONTENT_TYPE_0				0
>  #define HDCP_CONTENT_TYPE_1				1
>  
> +#define HDCP_14						1
> +#define HDCP_22						2
> +
>  #define LIC_PERIOD_MSEC				(4 * 1000)
>  /* Kernel retry count=3, Max time per authentication allowed = 6Sec */
>  #define KERNEL_AUTH_TIME_ALLOWED_MSEC		(3 *  6 * 1000)
> @@ -593,44 +596,6 @@ static bool sink_hdcp2_capable(igt_output_t *output)
>  	return strstr(buf, "HDCP2.2");
>  }
>  
> -static void
> -test_content_protection(enum igt_commit_style s, int content_type)
> -{
> -	igt_display_t *display = &data.display;
> -	igt_output_t *output;
> -	int valid_tests = 0;
> -
> -	if (data.cp_tests & CP_MEI_RELOAD)
> -		igt_require_f(igt_kmod_is_loaded("mei_hdcp"),
> -			      "mei_hdcp module is not loaded\n");
> -
> -	for_each_connected_output(display, output) {
> -		if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION])
> -			continue;
> -
> -		if (!output->props[IGT_CONNECTOR_HDCP_CONTENT_TYPE] &&
> -		    content_type)
> -			continue;
> -
> -		igt_info("CP Test execution on %s\n", output->name);
> -
> -		if (content_type && !sink_hdcp2_capable(output)) {
> -			igt_info("\tSkip %s (Sink has no HDCP2.2 support)\n",
> -				 output->name);
> -			continue;
> -		} else if (!sink_hdcp_capable(output)) {
> -			igt_info("\tSkip %s (Sink has no HDCP support)\n",
> -				 output->name);
> -			continue;
> -		}
> -
> -		test_content_protection_on_output(output, s, content_type);
> -		valid_tests++;
> -	}
> -
> -	igt_require_f(valid_tests, "No connector found with HDCP capability\n");
> -}
> -
>  static void test_content_protection_cleanup(void)
>  {
>  	igt_display_t *display = &data.display;
> @@ -651,58 +616,63 @@ static void test_content_protection_cleanup(void)
>  	}
>  }
>  
> -igt_main
> -{
> -	igt_fixture {
> -		data.drm_fd = drm_open_driver_master(DRIVER_ANY);
> +static bool
> +is_valid_output(igt_output_t *output, int content_type) {
> +	if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION])
> +		return false;
>  
> -		igt_display_require(&data.display, data.drm_fd);
> +	if (!output->props[IGT_CONNECTOR_HDCP_CONTENT_TYPE] && content_type)
> +		return false;
> +
> +	if (content_type && !sink_hdcp2_capable(output)) {
> +		igt_info("\tSkip %s (Sink has no HDCP2.2 support)\n",
> +			 output->name);
> +		return false;
> +
> +	} else if (!sink_hdcp_capable(output)) {
> +		igt_info("\tSkip %s (Sink has no HDCP support)\n",
> +			 output->name);
> +		return false;
>  	}
> +	return true;
> +}
> +
> +static void
> +test_content_protection_dynamic(igt_output_t *output, int content_type)
> +{
>  
> -	igt_subtest("legacy") {
> +	igt_dynamic_f("legacy-%s", output->name) {
>  		data.cp_tests = 0;
> -		test_content_protection(COMMIT_LEGACY, HDCP_CONTENT_TYPE_0);
> +		test_content_protection_on_output(output, COMMIT_LEGACY,
> +						  content_type);
>  	}
>  
> -	igt_subtest("atomic") {
> +	igt_dynamic_f("atomic-%s", output->name) {
>  		igt_require(data.display.is_atomic);
>  		data.cp_tests = 0;
> -		test_content_protection(COMMIT_ATOMIC, HDCP_CONTENT_TYPE_0);
> +		test_content_protection_on_output(output, COMMIT_ATOMIC,
> +						  content_type);
>  	}
>  
> -	igt_subtest("atomic-dpms") {
> +	igt_dynamic_f("atomic-dpms-%s", output->name) {
>  		igt_require(data.display.is_atomic);
>  		data.cp_tests = CP_DPMS;
> -		test_content_protection(COMMIT_ATOMIC, HDCP_CONTENT_TYPE_0);
> +		test_content_protection_on_output(output, COMMIT_ATOMIC,
> +						  content_type);
>  	}
>  
> -	igt_subtest("LIC") {
> +	igt_dynamic_f("LIC-%s", output->name) {
>  		igt_require(data.display.is_atomic);
>  		data.cp_tests = CP_LIC;
> -		test_content_protection(COMMIT_ATOMIC, HDCP_CONTENT_TYPE_0);
> -	}
> -
> -	igt_subtest("type1") {
> -		igt_require(data.display.is_atomic);
> -		test_content_protection(COMMIT_ATOMIC, HDCP_CONTENT_TYPE_1);
> -	}
> -
> -	igt_subtest("mei_interface") {
> -		igt_require(data.display.is_atomic);
> -		data.cp_tests = CP_MEI_RELOAD;
> -		test_content_protection(COMMIT_ATOMIC, HDCP_CONTENT_TYPE_1);
> +		test_content_protection_on_output(output, COMMIT_ATOMIC,
> +						  content_type);
>  	}
>  
> -	igt_subtest("content_type_change") {
> -		igt_require(data.display.is_atomic);
> -		data.cp_tests = CP_TYPE_CHANGE;
> -		test_content_protection(COMMIT_ATOMIC, HDCP_CONTENT_TYPE_1);
> -	}
> -
> -	igt_subtest("uevent") {
> +	igt_dynamic_f("uevent-%s", output->name) {
>  		igt_require(data.display.is_atomic);
>  		data.cp_tests = CP_UEVENT;
> -		test_content_protection(COMMIT_ATOMIC, HDCP_CONTENT_TYPE_0);
> +		test_content_protection_on_output(output, COMMIT_ATOMIC,
> +						  content_type);
>  	}
>  
>  	/*
> @@ -713,7 +683,7 @@ igt_main
>  	 *  either of these options, we test SRM writing from userspace and
>  	 *  validation of the same at kernel. Something is better than nothing.
>  	 */
> -	igt_subtest("srm") {
> +	igt_dynamic_f("srm-%s", output->name) {
>  		bool ret;
>  
>  		igt_require(data.display.is_atomic);
> @@ -721,7 +691,123 @@ igt_main
>  		ret = write_srm_as_fw((const __u8 *)facsimile_srm,
>  				      sizeof(facsimile_srm));
>  		igt_assert_f(ret, "SRM update failed");
> -		test_content_protection(COMMIT_ATOMIC, HDCP_CONTENT_TYPE_0);
> +		test_content_protection_on_output(output, COMMIT_ATOMIC,
> +						  content_type);
> +	}
> +
> +	/*
> +	 * The tests mei_interface and content_type_change are meant only
> +	 * for HDCP2.2 protocol, hence ignore for HDCP1.4.
> +	 */
> +	if (content_type != HDCP_CONTENT_TYPE_1)
> +		return;
Hope this will be sufficient for not to create the dynamic subtest as
hdcp1.4 + mei_interface....
> +
> +	igt_dynamic_f("mei_interface-%s", output->name) {
> +		igt_require(data.display.is_atomic);
> +		data.cp_tests = CP_MEI_RELOAD;
> +		test_content_protection_on_output(output, COMMIT_ATOMIC,
> +						  content_type);
> +	}
> +
> +	igt_dynamic_f("content_type_change-%s", output->name) {
> +		igt_require(data.display.is_atomic);
> +		data.cp_tests = CP_TYPE_CHANGE;
> +		test_content_protection_on_output(output, COMMIT_ATOMIC,
> +						  content_type);
> +	}
> +}
> +
> +static int request_hdcp_ver(igt_output_t *output, int hdcp_ver)
> +{
> +	char wrbuf[32] = {'0'};
> +	char rdbuf[32] = {'0'};
> +	char filename[128] = {'0'};
> +	char *read_ver_str;
> +	int fd, ret;
> +	unsigned int wr_ver, rd_ver;
> +
> +	fd = igt_debugfs_dir(data.drm_fd);
> +
> +	if (fd < 0) {
> +		igt_info("Failed to open debugfs dir ret = %d\n", errno);
> +		return errno;
> +	}
> +	wr_ver = hdcp_ver;
wr_ver is redundant. We can use the hdcp_ver itself.
> +	snprintf(wrbuf, sizeof(wrbuf), "%d", wr_ver);
> +	strcpy(filename, output->name);
> +	strcat(filename, "/i915_hdcp_version_request");
> +	ret = igt_sysfs_write(fd, filename, wrbuf, 1);
> +	if (ret <= 0) {
> +		igt_info("Failed to write into debugfs ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	close(fd);
Can't we do the write and read on same fd? why do we clsoe and reopen?
> +
> +	fd = igt_debugfs_connector_dir(data.drm_fd, output->name, O_RDONLY);
> +	if (fd < 0) {
> +		igt_info("Failed to open debugfs for read ret = %d", errno);
> +		return errno;
> +	}
> +
> +	debugfs_read(fd, "i915_hdcp_version_request", rdbuf);
> +	igt_debug("%s\n", rdbuf);
> +
> +	close(fd);
> +
> +	read_ver_str = strstr(rdbuf, "HDCP_VER_FLAGS: ");
> +	if (!read_ver_str)
> +		return -1;
> +
> +	igt_debug("HDCP ver = %s\n", read_ver_str + strlen("HDCP_VER_FLAGS: "));
> +
> +	rd_ver = atoi(read_ver_str + strlen("HDCP_VER_FLAGS: "));
> +	if (rd_ver != wr_ver)
can use the hdcp_ver itself.

Approach looks good to me..

-Ram
> +		return -1;
> +
> +	return 0;
> +}
> +
> +igt_main
> +{
> +	int i;
> +	struct cp_protocol {
> +		const char *name;
> +		int content_type;
> +		int version;
> +	} hdcp_protocol[] = {
> +		{ "hdcp-1_4", HDCP_CONTENT_TYPE_0, HDCP_14},
> +		{ "hdcp-2_2", HDCP_CONTENT_TYPE_1, HDCP_22} };
> +
> +	igt_fixture {
> +		data.drm_fd = drm_open_driver_master(DRIVER_ANY);
> +
> +		igt_display_require(&data.display, data.drm_fd);
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(hdcp_protocol); i++) {
> +		igt_subtest_with_dynamic_f("%s", hdcp_protocol[i].name) {
> +			igt_output_t *output;
> +			int content_type = hdcp_protocol[i].content_type;
> +
> +			for_each_connected_output(&data.display, output) {
> +				int ret;
> +
> +				if (!is_valid_output(output, content_type))
> +					continue;
> +				ret = request_hdcp_ver(output,
> +						       hdcp_protocol[i].version);
> +				if (ret) {
> +					igt_info("Cannot set the required version for %s\n",
> +						 output->name);
> +					continue;
> +				}
> +				test_content_protection_dynamic(output,
> +								content_type);
> +
> +				/* TODO: Revert to older HDCP ver that was set, previously */
> +			}
> +		}
>  	}
>  
>  	igt_fixture {
> -- 
> 2.17.1
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev