[PATCH i-g-t 3/4] tests/kms_content_protection: Add retry logic for mst usecase

[Kandpal, Suraj]

> -----Original Message-----
> From: B, Jeevan <jeevan.b at intel.com>
> Sent: Monday, August 19, 2024 10:47 AM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>; igt-dev at lists.freedesktop.org
> Cc: Samala, Pranay <pranay.samala at intel.com>; Nautiyal, Ankit K
> <ankit.k.nautiyal at intel.com>; Kandpal, Suraj <suraj.kandpal at intel.com>
> Subject: RE: [PATCH i-g-t 3/4] tests/kms_content_protection: Add retry logic
> for mst usecase
> 
> > -----Original Message-----
> > From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of
> > Suraj Kandpal
> > Sent: Tuesday, August 13, 2024 9:50 AM
> > To: igt-dev at lists.freedesktop.org
> > Cc: Samala, Pranay <pranay.samala at intel.com>; Nautiyal, Ankit K
> > <ankit.k.nautiyal at intel.com>; Kandpal, Suraj <suraj.kandpal at intel.com>
> > Subject: [PATCH i-g-t 3/4] tests/kms_content_protection: Add retry
> > logic for mst usecase
> >
> > Add mst retry logic where it retries 3 times before failing the test.
> > After every retry mst setup needs to be disabled so we need to add a
> > function which disables mst in one commit.
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal at intel.com>
> > ---
> >  tests/kms_content_protection.c | 84
> > +++++++++++++++++++++++++++++-----
> >  1 file changed, 73 insertions(+), 11 deletions(-)
> >
> > diff --git a/tests/kms_content_protection.c
> > b/tests/kms_content_protection.c index db6dc17b1..82a56167b 100644
> > --- a/tests/kms_content_protection.c
> > +++ b/tests/kms_content_protection.c
> > @@ -278,6 +278,35 @@ static bool test_cp_enable(igt_output_t *output,
> > enum igt_commit_style s,
> >  	return ret;
> >  }
> >
> > +static void test_mst_cp_disable(igt_output_t *hdcp_mst_output[], enum
> > igt_commit_style s,
> Update enum from s to some proper name instead of a char.
> > +				int valid_outputs)
> > +{
> > +	igt_display_t *display = &data.display;
> > +	igt_plane_t *primary;
> > +	bool ret;
> > +	int count;
> > +	u64 val;
> > +
> > +	for (count = 0; count < valid_outputs; count++) {
> > +		primary =
> igt_output_get_plane_type(hdcp_mst_output[count],
> > DRM_PLANE_TYPE_PRIMARY);
> > +		igt_plane_set_fb(primary, &data.red);
> > +		igt_output_set_prop_value(hdcp_mst_output[count],
> > IGT_CONNECTOR_CONTENT_PROTECTION,
> > +					  CP_UNDESIRED);
> > +	}
> > +
> > +	igt_display_commit2(display, s);
> > +
> > +	ret = wait_for_prop_value(hdcp_mst_output[0], CP_UNDESIRED,
> > +				  KERNEL_DISABLE_TIME_ALLOWED_MSEC);
> > +	for (count = 1; count < valid_outputs; count++) {
> > +		val = igt_output_get_prop(hdcp_mst_output[count],
> > +
> > IGT_CONNECTOR_CONTENT_PROTECTION);
> > +		ret &= (val == CP_UNDESIRED);
> > +	}
> > +
> > +	igt_assert_f(ret, "Content Protection not cleared\n"); }
> Suggest to update string to "Content Protection not cleared on all outputs"

Sure will do.

> > +
> >  static void test_cp_disable(igt_output_t *output, enum igt_commit_style s)
> {
> >  	igt_display_t *display = &data.display; @@ -630,6 +659,49 @@ static
> > void test_cp_lic_on_mst(igt_output_t *mst_outputs[], int valid_outputs, b
> >  	}
> >  }
> >
> > +static bool
> > +test_mst_cp_enable_with_retry(igt_output_t *hdcp_mst_output[], int
> Here we are trying to enable mst with cp,  "test" should be removed.

I think test here seems appropriate since not only is enabling MST we also have assertions to 
check if CP has been enabled or not also, this follows a similar style of naming found in this
test when retrying for a single connector.
Also a suggestion maybe adding a new line before and after the review make the inline comments
more clearer to see I almost missed the two comments I address in the reply

Regards,
Suraj Kandpal

> > valid_outputs,
> > +			      int retries, int content_type) {
> > +	igt_display_t *display = &data.display;
> > +	int retry_orig = retries, count;
> > +	bool ret;
> > +	u64 val;
> > +
> > +	do {
> > +		if (retry_orig != retries)
> > +			test_mst_cp_disable(hdcp_mst_output,
> > COMMIT_ATOMIC, valid_outputs);
> > +
> > +		for (count = 0; count < valid_outputs; count++) {
> > +			igt_output_set_prop_value(hdcp_mst_output[count],
> > +
> > IGT_CONNECTOR_CONTENT_PROTECTION, CP_DESIRED);
> > +
> > +			if (hdcp_mst_output[count]-
> > >props[IGT_CONNECTOR_HDCP_CONTENT_TYPE])
> > +
> > 	igt_output_set_prop_value(hdcp_mst_output[count],
> > +
> > IGT_CONNECTOR_HDCP_CONTENT_TYPE,
> > +							  content_type);
> > +		}
> > +
> > +		igt_display_commit2(display, COMMIT_ATOMIC);
> > +
> > +		ret = wait_for_prop_value(hdcp_mst_output[0], CP_ENABLED,
> > +
> > KERNEL_AUTH_TIME_ALLOWED_MSEC);
> > +		for (count = 1; count < valid_outputs; count++) {
> > +			val = igt_output_get_prop(hdcp_mst_output[count],
> > +
> > IGT_CONNECTOR_CONTENT_PROTECTION);
> > +			ret &= (val == CP_ENABLED);
> > +		}
> > +
> > +		retries -= 1;
> > +		if (!ret || retries)
> > +			igt_debug("Retry %d/3\n", 3 - retries);
> > +	} while (retries && !ret);
> > +
> > +	igt_assert_f(ret, "Content Protection not enabled on MST output\n");
> > +
> > +	return ret;
> > +}
> > +
> >  static void
> >  test_content_protection_mst(int content_type)  { @@ -684,19 +756,9 @@
> > test_content_protection_mst(int content_type)
> >  		igt_require_f(ret == 0, "Commit failure during MST
> modeset\n");
> >  	}
> >
> > -	for (count = 0; count < valid_outputs; count++) {
> > -		igt_output_set_prop_value(hdcp_mst_output[count],
> > IGT_CONNECTOR_CONTENT_PROTECTION, CP_DESIRED);
> > -
> > -		if (output->props[IGT_CONNECTOR_HDCP_CONTENT_TYPE])
> > -			igt_output_set_prop_value(hdcp_mst_output[count],
> > IGT_CONNECTOR_HDCP_CONTENT_TYPE, content_type);
> > -	}
> > -
> >  	igt_display_commit2(display, COMMIT_ATOMIC);
> >
> > -	for (count = 0; count < valid_outputs; count++) {
> > -		ret = wait_for_prop_value(hdcp_mst_output[count],
> > CP_ENABLED, KERNEL_AUTH_TIME_ALLOWED_MSEC);
> > -		igt_assert_f(ret, "Content Protection not enabled on %s\n",
> > hdcp_mst_output[count]->name);
> > -	}
> > +	ret = test_mst_cp_enable_with_retry(hdcp_mst_output,
> valid_outputs,
> > 2,
> > +content_type);
> >
> >  	if (data.cp_tests & CP_LIC)
> >  		test_cp_lic_on_mst(hdcp_mst_output, valid_outputs, 0);
> > --
> > 2.43.2