All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] [RFC i-g-t] Test Design to verify mipi enable/disable sequence.
@ 2017-01-07 18:07 Yadav Jyoti
  2017-01-09  9:00 ` Jani Nikula
  0 siblings, 1 reply; 5+ messages in thread
From: Yadav Jyoti @ 2017-01-07 18:07 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, ville.syrjala, Jenkins Val

From: Jenkins Val <jenkins@intel.com>

Signed-off-by: Jyoti Yadav <jyoti.r.yadav@intel.com>
---
 tests/Makefile.am                  |   3 +-
 tests/Makefile.sources             |   1 +
 tests/mipi_sequence_verification.c | 201 +++++++++++++++++++++++++++++++++++++
 3 files changed, 204 insertions(+), 1 deletion(-)
 create mode 100644 tests/mipi_sequence_verification.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index a408126..5b938a2 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -61,6 +61,7 @@ AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) -Wno-unused-result $(DEBUG_CFLAGS)\
 	-DIGT_SRCDIR=\""$(abs_srcdir)"\" \
 	-DIGT_DATADIR=\""$(pkgdatadir)"\" \
 	$(LIBUNWIND_CFLAGS) $(WERROR_CFLAGS) \
+	-I/usr/include/libxml2 \
 	$(NULL)
 
 LDADD = ../lib/libintel_tools.la $(GLIB_LIBS)
@@ -104,7 +105,7 @@ gem_userptr_blits_LDADD = $(LDADD) -lpthread
 gem_wait_LDADD = $(LDADD) -lrt
 kms_flip_LDADD = $(LDADD) -lrt -lpthread
 pm_rc6_residency_LDADD = $(LDADD) -lrt
-
+mipi_sequence_verification.c = $(LDADD) -lxml2
 prime_nv_test_CFLAGS = $(AM_CFLAGS) $(DRM_NOUVEAU_CFLAGS)
 prime_nv_test_LDADD = $(LDADD) $(DRM_NOUVEAU_LIBS)
 prime_nv_api_CFLAGS = $(AM_CFLAGS) $(DRM_NOUVEAU_CFLAGS)
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 598ec6f..8d9696d 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -211,6 +211,7 @@ TESTS_progs = \
 	kms_pwrite_crc \
 	kms_sink_crc_basic \
 	prime_udl \
+	mipi_sequence_verification \
 	$(NULL)
 
 # IMPORTANT: The ZZ_ tests need to be run last!
diff --git a/tests/mipi_sequence_verification.c b/tests/mipi_sequence_verification.c
new file mode 100644
index 0000000..378ccd2
--- /dev/null
+++ b/tests/mipi_sequence_verification.c
@@ -0,0 +1,201 @@
+/*
+ * "MIPI enable/disable sequence verification" Test Design:
+ * This patch is all about test design to verify mipi enable/disable sequence
+   mentioned in BSpec.
+ *
+ * Targeted Platform: BXT(including) onwards.
+ * Test Input:
+		a). XML file which details about mipi enable/disable sequence
+		    mentioned in BSpec.
+		    This file contain data about each regsiters which need to
+		    be programmed.
+		    Following tags are used in XML file.
+		    1. <Sequence> tag describes which sequence it is --> enable
+			sequence or disable sequence.
+		    2. <Step> tag describes about subtasks which need to be
+			completed in an order for the enable/disable sequence
+			to be completed.
+		    3. </Register> tag descibes about particular register's name
+			,offset and time when it was programmed.
+			A <Step> tag mentions about list of registers which need
+			to be programmed as part of that subtask.
+		    4. </Factor> tag describes about particular bit which need
+			to be programmed in a register. Here we maintain mask
+			and value field.
+
+  Below snippet is about XML file.
+  Two subtasks for mipi enable sequence are presented in XML form.
+
+  <Sequence name="mipi_enable_sequence">
+
+    <Step name="Enable_power_well_2">
+	<Register name="PWR_WELL_CTL2" offset="45404" time ="0">
+	     <Factor name="PWR_WELL_2_ENABLE" mask="80000000" value="80000000">
+	     </Factor>
+	</Register>
+    </Step>
+
+    <Step name="Configure_and_enable_MIPI_clocks">
+	<Register name="DSI_PLL_CTL" offset="161000" time="0">
+	</Register>
+	<Register name="MIPI_CLOCK_CTL" offset="46090" time="0">
+	</Register>
+	<Register name="DSI_PLL_ENABLE" offset="46080" time="0">
+		<Factor name="PLL_ENABLE" mask="80000000" value="80000000">
+		</Factor>
+	</Register>
+    </Step>
+
+   </Sequence>
+
+	      b) Another input to the test is Dmesg Log file. This log file
+		includes our customized logs apart from other dmesg logs.
+		Customized logs are enabled with one patch, which inserts some
+		printk message, when mipi related registers are written.
+		Our customized logs are for those registers which are related to
+		mipi enable/disable sequence.
+		This is how customized logs looks like.
+
+		[    1.754127]	dbg_gen9_write32 :	0x45404	0x70000000
+		[    1.754173]	dbg_gen9_write32 :	0x138090	0x3
+
+		1 log line contains "time" value, "dbg_gen9_write32",string to
+		distinguish our customized message, "register_offset" and
+		"register_value".
+
+How test works:
+
+	1. For particular sequence, test will pick one by one registers from
+	   steps of the sequence, and will try to find this register offset in
+	   the log file, if register offset and value matches, then we
+	   store time of the register, in the "time" attribute of <Register> tag
+	   in XML file.
+
+	2. Once we store time of each register(mentioned in XML file), from the
+	   dmesg log file, then we check order between each <Step> is maintained
+	   or not.
+	   "time" values for all the registers in Step 1 should be less than the
+	   "time" values for all the registers in Step 2.
+
+	3. If this order is maintained between all the Steps in the Sequence,
+	   then test is Passed, otherwise not.
+
+Library used: libxml library is used to parse XML file.
+*/
+code snippet:
+/* command line parameters.*/
+struct cmd_param {
+	char *xml_filename;
+	char *dmesg_log_filename;
+};
+static struct cmd_param opt = {
+      	.xml_filename = "./tests/mipi_sequence.xml";
+	.dmesg_log_filename = "./test/bxt_dmesg.log"
+};
+
+static int opt_handler(int option, int option_index, void *input)
+{
+	switch (option) {
+	case 'x':
+		opt.xml_filename = optarg;
+		break;
+	case 'd':
+		opt.dmesg_log_filename = optarg;
+		break;
+	default:
+		igt_assert(false);
+	}
+
+	return 0;
+}
+
+int
+main(int argc, char **argv)
+{
+	int fd;
+	int gen;
+	xmlDoc *document;
+	xmlNode *root, *first_child, *node;
+	FILE *fp;
+	fd = drm_open_driver_master(DRIVER_ANY);
+	if (!is_i915_device(fd))
+		return 0;
+	gen = intel_gen(intel_get_drm_devid(fd));
+	igt_skip_on(gen < 9);
+	const char *help_str =
+	       "  --xml\t\tXml file contains mipi enable/disable sequence mentioned in BSpec.\n \
+	          --dmesg_log_file\t\t Dmesg logs collected after applying patch which enabled custom logs";
+	struct option long_opts[] = {
+					{"xml", required_arguement, 0, 'x'},
+					{"dmesg_log_file", required_arguement, 'd'},
+					{"help", 0, 0, 'h'},
+					{0, 0, 0, 0}
+				};
+	igt_subtest_init_parse_opts(&argc, argv, "", long_opts,
+				    help_str, opt_handler, NULL);
+	igt_skip_on_simulation();
+	fp = fopen(opt.dmesg_log_filename, "r");
+	igt_assert(fp);
+	/* extract_custom_logs() function will extract customized logs from
+	 * dmesg log file and will return an array of custom log lines
+	 * which are tokenized. Tokens will be "time", "offset" and "value".
+	 */
+	int size1, size2 = 0;
+	char ***enable_seq_logs = extract_custom_logs(fp, "MIPI_ENABLE_SEQ",
+						      &size1);
+	char ***disable_seq_logs = extract_customized_logs(fp,
+							   "MIPI_DISABLE_SEQ",
+							   &size2);
+	document = xmlReadFile(opt.xml_filename, NULL, 0);
+	root = xmlDocGetRootElement(document);
+	/* first_child will be <Sequence> node.*/
+	first_child = root->children;
+	for (node = first_child; node; node = node->next) {
+		if (node->type == XML_ELEMENT_NODE) {
+			/* <Step> nodes will be children of
+			 * <Sequence> node.
+			*/
+			xmlNode *step_root = node->children;
+			xmlNode *step_temp_node = step_root;
+			for (step_temp_node = step_root; step_temp_node;
+			     step_temp_node = step_temp_node->next) {
+				if (step_temp_node->type == XML_ELEMENT_NODE) {
+					/* <Register> node will be children of
+					 * <Step> node.
+					 */
+					xmlNode *reg_root =
+					step_temp_node->children;
+					xmlNode *reg_temp_node = reg_root;
+					for (reg_temp_node = reg_root;
+					     temp_node;
+					     temp_node = temp_node->next) {
+						if (temp_node->type ==
+						    XML_ELEMENT_NODE) {
+							int it = 0;
+							/* found flag to check
+							 * that register is
+							 * found in the logs.
+							 */
+							int found = 0;
+							/* check register in log
+							 * array.
+							 */
+							found = check_reg(
+								temp_node,
+								enable_seq_logs,
+								size1)
+								;
+						}
+					}
+				}
+			}
+		}
+		/* TODO: when we arrive here, we have stored time of each
+		 * register in the XML file, from the dmesg logs when it is
+		 * programmed.
+		 * Now we have to see that registers are programmed in the order
+		 * which is mentioned in Bspec, based on timing information.
+		 */
+	}
+	igt_exit();
+}
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] [RFC i-g-t] Test Design to verify mipi enable/disable sequence.
  2017-01-07 18:07 [PATCH] [RFC i-g-t] Test Design to verify mipi enable/disable sequence Yadav Jyoti
@ 2017-01-09  9:00 ` Jani Nikula
  2017-01-09 17:09   ` Yadav, Jyoti R
  2017-01-11  8:14   ` Daniel Vetter
  0 siblings, 2 replies; 5+ messages in thread
From: Jani Nikula @ 2017-01-09  9:00 UTC (permalink / raw)
  To: Yadav Jyoti, intel-gfx; +Cc: ville.syrjala, Jenkins Val

On Sat, 07 Jan 2017, Yadav Jyoti <jyoti.r.yadav@intel.com> wrote:
> From: Jenkins Val <jenkins@intel.com>
>

This place here is for the commit message, where you should explain
*why* we need this change.

Where do you get the XML file? Do you write it manually? How do you
manage them? The kernel will execute the sequences from the VBT, not
from your XML file, so you'll have a problem of maintaining XML files
for each machine you ever run this test on.

I'm also not thrilled about adding special debug messages that the test
depends on finding in dmesg. The test also doesn't actually do anything
to cause the sequences to be run, so you expect some other, undefined
tests to have been run, the dmesg from that run captured, and saved to a
file that you feed to this test.

I think the design is rather fragile.

BR,
Jani.


> Signed-off-by: Jyoti Yadav <jyoti.r.yadav@intel.com>
> ---
>  tests/Makefile.am                  |   3 +-
>  tests/Makefile.sources             |   1 +
>  tests/mipi_sequence_verification.c | 201 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 204 insertions(+), 1 deletion(-)
>  create mode 100644 tests/mipi_sequence_verification.c
>
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index a408126..5b938a2 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -61,6 +61,7 @@ AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) -Wno-unused-result $(DEBUG_CFLAGS)\
>  	-DIGT_SRCDIR=\""$(abs_srcdir)"\" \
>  	-DIGT_DATADIR=\""$(pkgdatadir)"\" \
>  	$(LIBUNWIND_CFLAGS) $(WERROR_CFLAGS) \
> +	-I/usr/include/libxml2 \
>  	$(NULL)
>  
>  LDADD = ../lib/libintel_tools.la $(GLIB_LIBS)
> @@ -104,7 +105,7 @@ gem_userptr_blits_LDADD = $(LDADD) -lpthread
>  gem_wait_LDADD = $(LDADD) -lrt
>  kms_flip_LDADD = $(LDADD) -lrt -lpthread
>  pm_rc6_residency_LDADD = $(LDADD) -lrt
> -
> +mipi_sequence_verification.c = $(LDADD) -lxml2
>  prime_nv_test_CFLAGS = $(AM_CFLAGS) $(DRM_NOUVEAU_CFLAGS)
>  prime_nv_test_LDADD = $(LDADD) $(DRM_NOUVEAU_LIBS)
>  prime_nv_api_CFLAGS = $(AM_CFLAGS) $(DRM_NOUVEAU_CFLAGS)
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 598ec6f..8d9696d 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -211,6 +211,7 @@ TESTS_progs = \
>  	kms_pwrite_crc \
>  	kms_sink_crc_basic \
>  	prime_udl \
> +	mipi_sequence_verification \
>  	$(NULL)
>  
>  # IMPORTANT: The ZZ_ tests need to be run last!
> diff --git a/tests/mipi_sequence_verification.c b/tests/mipi_sequence_verification.c
> new file mode 100644
> index 0000000..378ccd2
> --- /dev/null
> +++ b/tests/mipi_sequence_verification.c
> @@ -0,0 +1,201 @@
> +/*
> + * "MIPI enable/disable sequence verification" Test Design:
> + * This patch is all about test design to verify mipi enable/disable sequence
> +   mentioned in BSpec.
> + *
> + * Targeted Platform: BXT(including) onwards.
> + * Test Input:
> +		a). XML file which details about mipi enable/disable sequence
> +		    mentioned in BSpec.
> +		    This file contain data about each regsiters which need to
> +		    be programmed.
> +		    Following tags are used in XML file.
> +		    1. <Sequence> tag describes which sequence it is --> enable
> +			sequence or disable sequence.
> +		    2. <Step> tag describes about subtasks which need to be
> +			completed in an order for the enable/disable sequence
> +			to be completed.
> +		    3. </Register> tag descibes about particular register's name
> +			,offset and time when it was programmed.
> +			A <Step> tag mentions about list of registers which need
> +			to be programmed as part of that subtask.
> +		    4. </Factor> tag describes about particular bit which need
> +			to be programmed in a register. Here we maintain mask
> +			and value field.
> +
> +  Below snippet is about XML file.
> +  Two subtasks for mipi enable sequence are presented in XML form.
> +
> +  <Sequence name="mipi_enable_sequence">
> +
> +    <Step name="Enable_power_well_2">
> +	<Register name="PWR_WELL_CTL2" offset="45404" time ="0">
> +	     <Factor name="PWR_WELL_2_ENABLE" mask="80000000" value="80000000">
> +	     </Factor>
> +	</Register>
> +    </Step>
> +
> +    <Step name="Configure_and_enable_MIPI_clocks">
> +	<Register name="DSI_PLL_CTL" offset="161000" time="0">
> +	</Register>
> +	<Register name="MIPI_CLOCK_CTL" offset="46090" time="0">
> +	</Register>
> +	<Register name="DSI_PLL_ENABLE" offset="46080" time="0">
> +		<Factor name="PLL_ENABLE" mask="80000000" value="80000000">
> +		</Factor>
> +	</Register>
> +    </Step>
> +
> +   </Sequence>
> +
> +	      b) Another input to the test is Dmesg Log file. This log file
> +		includes our customized logs apart from other dmesg logs.
> +		Customized logs are enabled with one patch, which inserts some
> +		printk message, when mipi related registers are written.
> +		Our customized logs are for those registers which are related to
> +		mipi enable/disable sequence.
> +		This is how customized logs looks like.
> +
> +		[    1.754127]	dbg_gen9_write32 :	0x45404	0x70000000
> +		[    1.754173]	dbg_gen9_write32 :	0x138090	0x3
> +
> +		1 log line contains "time" value, "dbg_gen9_write32",string to
> +		distinguish our customized message, "register_offset" and
> +		"register_value".
> +
> +How test works:
> +
> +	1. For particular sequence, test will pick one by one registers from
> +	   steps of the sequence, and will try to find this register offset in
> +	   the log file, if register offset and value matches, then we
> +	   store time of the register, in the "time" attribute of <Register> tag
> +	   in XML file.
> +
> +	2. Once we store time of each register(mentioned in XML file), from the
> +	   dmesg log file, then we check order between each <Step> is maintained
> +	   or not.
> +	   "time" values for all the registers in Step 1 should be less than the
> +	   "time" values for all the registers in Step 2.
> +
> +	3. If this order is maintained between all the Steps in the Sequence,
> +	   then test is Passed, otherwise not.
> +
> +Library used: libxml library is used to parse XML file.
> +*/
> +code snippet:
> +/* command line parameters.*/
> +struct cmd_param {
> +	char *xml_filename;
> +	char *dmesg_log_filename;
> +};
> +static struct cmd_param opt = {
> +      	.xml_filename = "./tests/mipi_sequence.xml";
> +	.dmesg_log_filename = "./test/bxt_dmesg.log"
> +};
> +
> +static int opt_handler(int option, int option_index, void *input)
> +{
> +	switch (option) {
> +	case 'x':
> +		opt.xml_filename = optarg;
> +		break;
> +	case 'd':
> +		opt.dmesg_log_filename = optarg;
> +		break;
> +	default:
> +		igt_assert(false);
> +	}
> +
> +	return 0;
> +}
> +
> +int
> +main(int argc, char **argv)
> +{
> +	int fd;
> +	int gen;
> +	xmlDoc *document;
> +	xmlNode *root, *first_child, *node;
> +	FILE *fp;
> +	fd = drm_open_driver_master(DRIVER_ANY);
> +	if (!is_i915_device(fd))
> +		return 0;
> +	gen = intel_gen(intel_get_drm_devid(fd));
> +	igt_skip_on(gen < 9);
> +	const char *help_str =
> +	       "  --xml\t\tXml file contains mipi enable/disable sequence mentioned in BSpec.\n \
> +	          --dmesg_log_file\t\t Dmesg logs collected after applying patch which enabled custom logs";
> +	struct option long_opts[] = {
> +					{"xml", required_arguement, 0, 'x'},
> +					{"dmesg_log_file", required_arguement, 'd'},
> +					{"help", 0, 0, 'h'},
> +					{0, 0, 0, 0}
> +				};
> +	igt_subtest_init_parse_opts(&argc, argv, "", long_opts,
> +				    help_str, opt_handler, NULL);
> +	igt_skip_on_simulation();
> +	fp = fopen(opt.dmesg_log_filename, "r");
> +	igt_assert(fp);
> +	/* extract_custom_logs() function will extract customized logs from
> +	 * dmesg log file and will return an array of custom log lines
> +	 * which are tokenized. Tokens will be "time", "offset" and "value".
> +	 */
> +	int size1, size2 = 0;
> +	char ***enable_seq_logs = extract_custom_logs(fp, "MIPI_ENABLE_SEQ",
> +						      &size1);
> +	char ***disable_seq_logs = extract_customized_logs(fp,
> +							   "MIPI_DISABLE_SEQ",
> +							   &size2);
> +	document = xmlReadFile(opt.xml_filename, NULL, 0);
> +	root = xmlDocGetRootElement(document);
> +	/* first_child will be <Sequence> node.*/
> +	first_child = root->children;
> +	for (node = first_child; node; node = node->next) {
> +		if (node->type == XML_ELEMENT_NODE) {
> +			/* <Step> nodes will be children of
> +			 * <Sequence> node.
> +			*/
> +			xmlNode *step_root = node->children;
> +			xmlNode *step_temp_node = step_root;
> +			for (step_temp_node = step_root; step_temp_node;
> +			     step_temp_node = step_temp_node->next) {
> +				if (step_temp_node->type == XML_ELEMENT_NODE) {
> +					/* <Register> node will be children of
> +					 * <Step> node.
> +					 */
> +					xmlNode *reg_root =
> +					step_temp_node->children;
> +					xmlNode *reg_temp_node = reg_root;
> +					for (reg_temp_node = reg_root;
> +					     temp_node;
> +					     temp_node = temp_node->next) {
> +						if (temp_node->type ==
> +						    XML_ELEMENT_NODE) {
> +							int it = 0;
> +							/* found flag to check
> +							 * that register is
> +							 * found in the logs.
> +							 */
> +							int found = 0;
> +							/* check register in log
> +							 * array.
> +							 */
> +							found = check_reg(
> +								temp_node,
> +								enable_seq_logs,
> +								size1)
> +								;
> +						}
> +					}
> +				}
> +			}
> +		}
> +		/* TODO: when we arrive here, we have stored time of each
> +		 * register in the XML file, from the dmesg logs when it is
> +		 * programmed.
> +		 * Now we have to see that registers are programmed in the order
> +		 * which is mentioned in Bspec, based on timing information.
> +		 */
> +	}
> +	igt_exit();
> +}

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] [RFC i-g-t] Test Design to verify mipi enable/disable sequence.
  2017-01-09  9:00 ` Jani Nikula
@ 2017-01-09 17:09   ` Yadav, Jyoti R
  2017-01-10  7:49     ` Jani Nikula
  2017-01-11  8:14   ` Daniel Vetter
  1 sibling, 1 reply; 5+ messages in thread
From: Yadav, Jyoti R @ 2017-01-09 17:09 UTC (permalink / raw)
  To: Nikula, Jani, intel-gfx; +Cc: mi-jenkins-ci, Syrjala, Ville

Hi Jani,

Thanks for finding time to review the patch. Please find my comments inline.

Regards
Jyoti

-----Original Message-----
From: Nikula, Jani 
Sent: Monday, January 9, 2017 2:30 PM
To: Yadav, Jyoti R <jyoti.r.yadav@intel.com>; intel-gfx@lists.freedesktop.org
Cc: Kahola, Mika <mika.kahola@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; mi-jenkins-ci <mi-jenkins-ci@intel.com>
Subject: Re: [PATCH] [Intel-gfx] [RFC i-g-t] Test Design to verify mipi enable/disable sequence.

On Sat, 07 Jan 2017, Yadav Jyoti <jyoti.r.yadav@intel.com> wrote:
> From: Jenkins Val <jenkins@intel.com>
>

This place here is for the commit message, where you should explain
*why* we need this change.

[Jyoti]: yeah, sure we can move "what and why for the test" here.

Where do you get the XML file? Do you write it manually? How do you manage them? The kernel will execute the sequences from the VBT, not from your XML file, so you'll have a problem of maintaining XML files for each machine you ever run this test on.

[Jyoti]: 		XML files are written manually. Sequence mentioned in XML file are taken from the BSpec. Yes, we have to maintain different XML file for each platform.  XML files are not supposed to be the source of execution for kernel, As 		you already mentioned that kernel will execute the sequence from the VBT.  With the help of XML file, we will be able to verify BSpec sequence for mipi enable/disable calls. And I believe, ideally both the sequence (sequence 		mentioned in VBT and  sequence mentioned in BSpec should match) should match. Please correct me, I am wrong here.

		We can discuss on any other alternative  way:)
		For our internal validation , we are taking help of XML files. 

I'm also not thrilled about adding special debug messages that the test depends on finding in dmesg. The test also doesn't actually do anything to cause the sequences to be run, so you expect some other, undefined tests to have been run, the dmesg from that run captured, and saved to a file that you feed to this test

[Jyoti]: 		Yes , of course, test is not supposed to made the sequence run.  For running this test, we need to follow an order.
		Initially,  when Mipi panel is brought up, we can capture the logs and feed those logs to this test, if we want to verify Mipi Enable sequence.
		Similarly, during suspend cycle, when modeset disable sequence is called, and also mipi disable sequence is called, then we have to capture logs and feed to this test, if we want to verify mipi disable sequence.

		Importance of Debug Message:  With the help of debug message, we can find out the time stamp, when particular register is programmed. This time stamp helps in validating the order which should be maintained among different 
		sub sequences of entire enable/disable sequence.
.

I think the design is rather fragile.

[Jyoti] : Please help me in improving the design part and strengthen the mipi validation suit.

BR,
Jani.


> Signed-off-by: Jyoti Yadav <jyoti.r.yadav@intel.com>
> ---
>  tests/Makefile.am                  |   3 +-
>  tests/Makefile.sources             |   1 +
>  tests/mipi_sequence_verification.c | 201 
> +++++++++++++++++++++++++++++++++++++
>  3 files changed, 204 insertions(+), 1 deletion(-)  create mode 100644 
> tests/mipi_sequence_verification.c
>
> diff --git a/tests/Makefile.am b/tests/Makefile.am index 
> a408126..5b938a2 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -61,6 +61,7 @@ AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) -Wno-unused-result $(DEBUG_CFLAGS)\
>  	-DIGT_SRCDIR=\""$(abs_srcdir)"\" \
>  	-DIGT_DATADIR=\""$(pkgdatadir)"\" \
>  	$(LIBUNWIND_CFLAGS) $(WERROR_CFLAGS) \
> +	-I/usr/include/libxml2 \
>  	$(NULL)
>  
>  LDADD = ../lib/libintel_tools.la $(GLIB_LIBS) @@ -104,7 +105,7 @@ 
> gem_userptr_blits_LDADD = $(LDADD) -lpthread  gem_wait_LDADD = 
> $(LDADD) -lrt  kms_flip_LDADD = $(LDADD) -lrt -lpthread  
> pm_rc6_residency_LDADD = $(LDADD) -lrt
> -
> +mipi_sequence_verification.c = $(LDADD) -lxml2
>  prime_nv_test_CFLAGS = $(AM_CFLAGS) $(DRM_NOUVEAU_CFLAGS)  
> prime_nv_test_LDADD = $(LDADD) $(DRM_NOUVEAU_LIBS)  
> prime_nv_api_CFLAGS = $(AM_CFLAGS) $(DRM_NOUVEAU_CFLAGS) diff --git 
> a/tests/Makefile.sources b/tests/Makefile.sources index 
> 598ec6f..8d9696d 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -211,6 +211,7 @@ TESTS_progs = \
>  	kms_pwrite_crc \
>  	kms_sink_crc_basic \
>  	prime_udl \
> +	mipi_sequence_verification \
>  	$(NULL)
>  
>  # IMPORTANT: The ZZ_ tests need to be run last!
> diff --git a/tests/mipi_sequence_verification.c 
> b/tests/mipi_sequence_verification.c
> new file mode 100644
> index 0000000..378ccd2
> --- /dev/null
> +++ b/tests/mipi_sequence_verification.c
> @@ -0,0 +1,201 @@
> +/*
> + * "MIPI enable/disable sequence verification" Test Design:
> + * This patch is all about test design to verify mipi enable/disable sequence
> +   mentioned in BSpec.
> + *
> + * Targeted Platform: BXT(including) onwards.
> + * Test Input:
> +		a). XML file which details about mipi enable/disable sequence
> +		    mentioned in BSpec.
> +		    This file contain data about each regsiters which need to
> +		    be programmed.
> +		    Following tags are used in XML file.
> +		    1. <Sequence> tag describes which sequence it is --> enable
> +			sequence or disable sequence.
> +		    2. <Step> tag describes about subtasks which need to be
> +			completed in an order for the enable/disable sequence
> +			to be completed.
> +		    3. </Register> tag descibes about particular register's name
> +			,offset and time when it was programmed.
> +			A <Step> tag mentions about list of registers which need
> +			to be programmed as part of that subtask.
> +		    4. </Factor> tag describes about particular bit which need
> +			to be programmed in a register. Here we maintain mask
> +			and value field.
> +
> +  Below snippet is about XML file.
> +  Two subtasks for mipi enable sequence are presented in XML form.
> +
> +  <Sequence name="mipi_enable_sequence">
> +
> +    <Step name="Enable_power_well_2">
> +	<Register name="PWR_WELL_CTL2" offset="45404" time ="0">
> +	     <Factor name="PWR_WELL_2_ENABLE" mask="80000000" value="80000000">
> +	     </Factor>
> +	</Register>
> +    </Step>
> +
> +    <Step name="Configure_and_enable_MIPI_clocks">
> +	<Register name="DSI_PLL_CTL" offset="161000" time="0">
> +	</Register>
> +	<Register name="MIPI_CLOCK_CTL" offset="46090" time="0">
> +	</Register>
> +	<Register name="DSI_PLL_ENABLE" offset="46080" time="0">
> +		<Factor name="PLL_ENABLE" mask="80000000" value="80000000">
> +		</Factor>
> +	</Register>
> +    </Step>
> +
> +   </Sequence>
> +
> +	      b) Another input to the test is Dmesg Log file. This log file
> +		includes our customized logs apart from other dmesg logs.
> +		Customized logs are enabled with one patch, which inserts some
> +		printk message, when mipi related registers are written.
> +		Our customized logs are for those registers which are related to
> +		mipi enable/disable sequence.
> +		This is how customized logs looks like.
> +
> +		[    1.754127]	dbg_gen9_write32 :	0x45404	0x70000000
> +		[    1.754173]	dbg_gen9_write32 :	0x138090	0x3
> +
> +		1 log line contains "time" value, "dbg_gen9_write32",string to
> +		distinguish our customized message, "register_offset" and
> +		"register_value".
> +
> +How test works:
> +
> +	1. For particular sequence, test will pick one by one registers from
> +	   steps of the sequence, and will try to find this register offset in
> +	   the log file, if register offset and value matches, then we
> +	   store time of the register, in the "time" attribute of <Register> tag
> +	   in XML file.
> +
> +	2. Once we store time of each register(mentioned in XML file), from the
> +	   dmesg log file, then we check order between each <Step> is maintained
> +	   or not.
> +	   "time" values for all the registers in Step 1 should be less than the
> +	   "time" values for all the registers in Step 2.
> +
> +	3. If this order is maintained between all the Steps in the Sequence,
> +	   then test is Passed, otherwise not.
> +
> +Library used: libxml library is used to parse XML file.
> +*/
> +code snippet:
> +/* command line parameters.*/
> +struct cmd_param {
> +	char *xml_filename;
> +	char *dmesg_log_filename;
> +};
> +static struct cmd_param opt = {
> +      	.xml_filename = "./tests/mipi_sequence.xml";
> +	.dmesg_log_filename = "./test/bxt_dmesg.log"
> +};
> +
> +static int opt_handler(int option, int option_index, void *input) {
> +	switch (option) {
> +	case 'x':
> +		opt.xml_filename = optarg;
> +		break;
> +	case 'd':
> +		opt.dmesg_log_filename = optarg;
> +		break;
> +	default:
> +		igt_assert(false);
> +	}
> +
> +	return 0;
> +}
> +
> +int
> +main(int argc, char **argv)
> +{
> +	int fd;
> +	int gen;
> +	xmlDoc *document;
> +	xmlNode *root, *first_child, *node;
> +	FILE *fp;
> +	fd = drm_open_driver_master(DRIVER_ANY);
> +	if (!is_i915_device(fd))
> +		return 0;
> +	gen = intel_gen(intel_get_drm_devid(fd));
> +	igt_skip_on(gen < 9);
> +	const char *help_str =
> +	       "  --xml\t\tXml file contains mipi enable/disable sequence mentioned in BSpec.\n \
> +	          --dmesg_log_file\t\t Dmesg logs collected after applying patch which enabled custom logs";
> +	struct option long_opts[] = {
> +					{"xml", required_arguement, 0, 'x'},
> +					{"dmesg_log_file", required_arguement, 'd'},
> +					{"help", 0, 0, 'h'},
> +					{0, 0, 0, 0}
> +				};
> +	igt_subtest_init_parse_opts(&argc, argv, "", long_opts,
> +				    help_str, opt_handler, NULL);
> +	igt_skip_on_simulation();
> +	fp = fopen(opt.dmesg_log_filename, "r");
> +	igt_assert(fp);
> +	/* extract_custom_logs() function will extract customized logs from
> +	 * dmesg log file and will return an array of custom log lines
> +	 * which are tokenized. Tokens will be "time", "offset" and "value".
> +	 */
> +	int size1, size2 = 0;
> +	char ***enable_seq_logs = extract_custom_logs(fp, "MIPI_ENABLE_SEQ",
> +						      &size1);
> +	char ***disable_seq_logs = extract_customized_logs(fp,
> +							   "MIPI_DISABLE_SEQ",
> +							   &size2);
> +	document = xmlReadFile(opt.xml_filename, NULL, 0);
> +	root = xmlDocGetRootElement(document);
> +	/* first_child will be <Sequence> node.*/
> +	first_child = root->children;
> +	for (node = first_child; node; node = node->next) {
> +		if (node->type == XML_ELEMENT_NODE) {
> +			/* <Step> nodes will be children of
> +			 * <Sequence> node.
> +			*/
> +			xmlNode *step_root = node->children;
> +			xmlNode *step_temp_node = step_root;
> +			for (step_temp_node = step_root; step_temp_node;
> +			     step_temp_node = step_temp_node->next) {
> +				if (step_temp_node->type == XML_ELEMENT_NODE) {
> +					/* <Register> node will be children of
> +					 * <Step> node.
> +					 */
> +					xmlNode *reg_root =
> +					step_temp_node->children;
> +					xmlNode *reg_temp_node = reg_root;
> +					for (reg_temp_node = reg_root;
> +					     temp_node;
> +					     temp_node = temp_node->next) {
> +						if (temp_node->type ==
> +						    XML_ELEMENT_NODE) {
> +							int it = 0;
> +							/* found flag to check
> +							 * that register is
> +							 * found in the logs.
> +							 */
> +							int found = 0;
> +							/* check register in log
> +							 * array.
> +							 */
> +							found = check_reg(
> +								temp_node,
> +								enable_seq_logs,
> +								size1)
> +								;
> +						}
> +					}
> +				}
> +			}
> +		}
> +		/* TODO: when we arrive here, we have stored time of each
> +		 * register in the XML file, from the dmesg logs when it is
> +		 * programmed.
> +		 * Now we have to see that registers are programmed in the order
> +		 * which is mentioned in Bspec, based on timing information.
> +		 */
> +	}
> +	igt_exit();
> +}

--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] [RFC i-g-t] Test Design to verify mipi enable/disable sequence.
  2017-01-09 17:09   ` Yadav, Jyoti R
@ 2017-01-10  7:49     ` Jani Nikula
  0 siblings, 0 replies; 5+ messages in thread
From: Jani Nikula @ 2017-01-10  7:49 UTC (permalink / raw)
  To: Yadav, Jyoti R, intel-gfx; +Cc: Daniel Vetter, mi-jenkins-ci, Syrjala, Ville

On Mon, 09 Jan 2017, "Yadav, Jyoti R" <jyoti.r.yadav@intel.com> wrote:
> Hi Jani,
>
> Thanks for finding time to review the patch. Please find my comments inline.

Please try to use the usual reply quoting style of the mailist lists.

> -----Original Message-----
> From: Nikula, Jani 
> Sent: Monday, January 9, 2017 2:30 PM
> To: Yadav, Jyoti R <jyoti.r.yadav@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Kahola, Mika <mika.kahola@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; mi-jenkins-ci <mi-jenkins-ci@intel.com>
> Subject: Re: [PATCH] [Intel-gfx] [RFC i-g-t] Test Design to verify mipi enable/disable sequence.
>
> On Sat, 07 Jan 2017, Yadav Jyoti <jyoti.r.yadav@intel.com> wrote:
>> From: Jenkins Val <jenkins@intel.com>
>>
>
> This place here is for the commit message, where you should explain
> *why* we need this change.
>
> [Jyoti]: yeah, sure we can move "what and why for the test" here.
>
> Where do you get the XML file? Do you write it manually? How do you manage them? The kernel will execute the sequences from the VBT, not from your XML file, so you'll have a problem of maintaining XML files for each machine you ever run this test on.
>
> [Jyoti]: 		XML files are written manually. Sequence mentioned in XML file are taken from the BSpec. Yes, we have to maintain different XML file for each platform.  XML files are not supposed to be the source of execution for kernel, As 		you already mentioned that kernel will execute the sequence from the VBT.  With the help of XML file, we will be able to verify BSpec sequence for mipi enable/disable calls. And I believe, ideally both the sequence (sequence 		mentioned in VBT and  sequence mentioned in BSpec should match) should match. Please correct me, I am wrong here.
>
> 		We can discuss on any other alternative  way:)
> 		For our internal validation , we are taking help of XML
> 		files.

There's two parts here: 1) Making sure the driver executes the sequences
in the VBT correctly, and 2) Making sure the VBT is correct for the
platform. Your test does both at once. Instead, you should split it. The
VBT on the machine is the pivot here.

You should read the sequences from VBT. The VBT is available at
/sys/kernel/debug/dri/0/i915_vbt. The IGT tool intel_bios_reader will
decode much of the VBT; you can add more to decode sequences. You should
use that to verify both 1) and 2) separately. Compare the VBT on the
machine to whatever you have to make sure it's correct. This is mostly
uninteresting to upstream. And then you can correlate the sequences to
what the driver actually does.

> I'm also not thrilled about adding special debug messages that the test depends on finding in dmesg. The test also doesn't actually do anything to cause the sequences to be run, so you expect some other, undefined tests to have been run, the dmesg from that run captured, and saved to a file that you feed to this test
>
> [Jyoti]: 		Yes , of course, test is not supposed to made the sequence run.  For running this test, we need to follow an order.
> 		Initially,  when Mipi panel is brought up, we can capture the logs and feed those logs to this test, if we want to verify Mipi Enable sequence.
> 		Similarly, during suspend cycle, when modeset disable sequence is called, and also mipi disable sequence is called, then we have to capture logs and feed to this test, if we want to verify mipi disable sequence.
>
> 		Importance of Debug Message:  With the help of debug message, we can find out the time stamp, when particular register is programmed. This time stamp helps in validating the order which should be maintained among different 
> 		sub sequences of entire enable/disable sequence.

I think you should come up with a test (or reuse an existing test) that
actually does the mode setting and all the parts that should cause the
VBT sequences to be executed, and at each step of the way, you should
verify the sequences were actually executed. And no others were.

I'm still dubious about relying on dmesg to test this. I'll defer to
others for input. Daniel?


BR,
Jani.


> .
>
> I think the design is rather fragile.
>
> [Jyoti] : Please help me in improving the design part and strengthen the mipi validation suit.
>
> BR,
> Jani.
>
>
>> Signed-off-by: Jyoti Yadav <jyoti.r.yadav@intel.com>
>> ---
>>  tests/Makefile.am                  |   3 +-
>>  tests/Makefile.sources             |   1 +
>>  tests/mipi_sequence_verification.c | 201 
>> +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 204 insertions(+), 1 deletion(-)  create mode 100644 
>> tests/mipi_sequence_verification.c
>>
>> diff --git a/tests/Makefile.am b/tests/Makefile.am index 
>> a408126..5b938a2 100644
>> --- a/tests/Makefile.am
>> +++ b/tests/Makefile.am
>> @@ -61,6 +61,7 @@ AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) -Wno-unused-result $(DEBUG_CFLAGS)\
>>  	-DIGT_SRCDIR=\""$(abs_srcdir)"\" \
>>  	-DIGT_DATADIR=\""$(pkgdatadir)"\" \
>>  	$(LIBUNWIND_CFLAGS) $(WERROR_CFLAGS) \
>> +	-I/usr/include/libxml2 \
>>  	$(NULL)
>>  
>>  LDADD = ../lib/libintel_tools.la $(GLIB_LIBS) @@ -104,7 +105,7 @@ 
>> gem_userptr_blits_LDADD = $(LDADD) -lpthread  gem_wait_LDADD = 
>> $(LDADD) -lrt  kms_flip_LDADD = $(LDADD) -lrt -lpthread  
>> pm_rc6_residency_LDADD = $(LDADD) -lrt
>> -
>> +mipi_sequence_verification.c = $(LDADD) -lxml2
>>  prime_nv_test_CFLAGS = $(AM_CFLAGS) $(DRM_NOUVEAU_CFLAGS)  
>> prime_nv_test_LDADD = $(LDADD) $(DRM_NOUVEAU_LIBS)  
>> prime_nv_api_CFLAGS = $(AM_CFLAGS) $(DRM_NOUVEAU_CFLAGS) diff --git 
>> a/tests/Makefile.sources b/tests/Makefile.sources index 
>> 598ec6f..8d9696d 100644
>> --- a/tests/Makefile.sources
>> +++ b/tests/Makefile.sources
>> @@ -211,6 +211,7 @@ TESTS_progs = \
>>  	kms_pwrite_crc \
>>  	kms_sink_crc_basic \
>>  	prime_udl \
>> +	mipi_sequence_verification \
>>  	$(NULL)
>>  
>>  # IMPORTANT: The ZZ_ tests need to be run last!
>> diff --git a/tests/mipi_sequence_verification.c 
>> b/tests/mipi_sequence_verification.c
>> new file mode 100644
>> index 0000000..378ccd2
>> --- /dev/null
>> +++ b/tests/mipi_sequence_verification.c
>> @@ -0,0 +1,201 @@
>> +/*
>> + * "MIPI enable/disable sequence verification" Test Design:
>> + * This patch is all about test design to verify mipi enable/disable sequence
>> +   mentioned in BSpec.
>> + *
>> + * Targeted Platform: BXT(including) onwards.
>> + * Test Input:
>> +		a). XML file which details about mipi enable/disable sequence
>> +		    mentioned in BSpec.
>> +		    This file contain data about each regsiters which need to
>> +		    be programmed.
>> +		    Following tags are used in XML file.
>> +		    1. <Sequence> tag describes which sequence it is --> enable
>> +			sequence or disable sequence.
>> +		    2. <Step> tag describes about subtasks which need to be
>> +			completed in an order for the enable/disable sequence
>> +			to be completed.
>> +		    3. </Register> tag descibes about particular register's name
>> +			,offset and time when it was programmed.
>> +			A <Step> tag mentions about list of registers which need
>> +			to be programmed as part of that subtask.
>> +		    4. </Factor> tag describes about particular bit which need
>> +			to be programmed in a register. Here we maintain mask
>> +			and value field.
>> +
>> +  Below snippet is about XML file.
>> +  Two subtasks for mipi enable sequence are presented in XML form.
>> +
>> +  <Sequence name="mipi_enable_sequence">
>> +
>> +    <Step name="Enable_power_well_2">
>> +	<Register name="PWR_WELL_CTL2" offset="45404" time ="0">
>> +	     <Factor name="PWR_WELL_2_ENABLE" mask="80000000" value="80000000">
>> +	     </Factor>
>> +	</Register>
>> +    </Step>
>> +
>> +    <Step name="Configure_and_enable_MIPI_clocks">
>> +	<Register name="DSI_PLL_CTL" offset="161000" time="0">
>> +	</Register>
>> +	<Register name="MIPI_CLOCK_CTL" offset="46090" time="0">
>> +	</Register>
>> +	<Register name="DSI_PLL_ENABLE" offset="46080" time="0">
>> +		<Factor name="PLL_ENABLE" mask="80000000" value="80000000">
>> +		</Factor>
>> +	</Register>
>> +    </Step>
>> +
>> +   </Sequence>
>> +
>> +	      b) Another input to the test is Dmesg Log file. This log file
>> +		includes our customized logs apart from other dmesg logs.
>> +		Customized logs are enabled with one patch, which inserts some
>> +		printk message, when mipi related registers are written.
>> +		Our customized logs are for those registers which are related to
>> +		mipi enable/disable sequence.
>> +		This is how customized logs looks like.
>> +
>> +		[    1.754127]	dbg_gen9_write32 :	0x45404	0x70000000
>> +		[    1.754173]	dbg_gen9_write32 :	0x138090	0x3
>> +
>> +		1 log line contains "time" value, "dbg_gen9_write32",string to
>> +		distinguish our customized message, "register_offset" and
>> +		"register_value".
>> +
>> +How test works:
>> +
>> +	1. For particular sequence, test will pick one by one registers from
>> +	   steps of the sequence, and will try to find this register offset in
>> +	   the log file, if register offset and value matches, then we
>> +	   store time of the register, in the "time" attribute of <Register> tag
>> +	   in XML file.
>> +
>> +	2. Once we store time of each register(mentioned in XML file), from the
>> +	   dmesg log file, then we check order between each <Step> is maintained
>> +	   or not.
>> +	   "time" values for all the registers in Step 1 should be less than the
>> +	   "time" values for all the registers in Step 2.
>> +
>> +	3. If this order is maintained between all the Steps in the Sequence,
>> +	   then test is Passed, otherwise not.
>> +
>> +Library used: libxml library is used to parse XML file.
>> +*/
>> +code snippet:
>> +/* command line parameters.*/
>> +struct cmd_param {
>> +	char *xml_filename;
>> +	char *dmesg_log_filename;
>> +};
>> +static struct cmd_param opt = {
>> +      	.xml_filename = "./tests/mipi_sequence.xml";
>> +	.dmesg_log_filename = "./test/bxt_dmesg.log"
>> +};
>> +
>> +static int opt_handler(int option, int option_index, void *input) {
>> +	switch (option) {
>> +	case 'x':
>> +		opt.xml_filename = optarg;
>> +		break;
>> +	case 'd':
>> +		opt.dmesg_log_filename = optarg;
>> +		break;
>> +	default:
>> +		igt_assert(false);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int
>> +main(int argc, char **argv)
>> +{
>> +	int fd;
>> +	int gen;
>> +	xmlDoc *document;
>> +	xmlNode *root, *first_child, *node;
>> +	FILE *fp;
>> +	fd = drm_open_driver_master(DRIVER_ANY);
>> +	if (!is_i915_device(fd))
>> +		return 0;
>> +	gen = intel_gen(intel_get_drm_devid(fd));
>> +	igt_skip_on(gen < 9);
>> +	const char *help_str =
>> +	       "  --xml\t\tXml file contains mipi enable/disable sequence mentioned in BSpec.\n \
>> +	          --dmesg_log_file\t\t Dmesg logs collected after applying patch which enabled custom logs";
>> +	struct option long_opts[] = {
>> +					{"xml", required_arguement, 0, 'x'},
>> +					{"dmesg_log_file", required_arguement, 'd'},
>> +					{"help", 0, 0, 'h'},
>> +					{0, 0, 0, 0}
>> +				};
>> +	igt_subtest_init_parse_opts(&argc, argv, "", long_opts,
>> +				    help_str, opt_handler, NULL);
>> +	igt_skip_on_simulation();
>> +	fp = fopen(opt.dmesg_log_filename, "r");
>> +	igt_assert(fp);
>> +	/* extract_custom_logs() function will extract customized logs from
>> +	 * dmesg log file and will return an array of custom log lines
>> +	 * which are tokenized. Tokens will be "time", "offset" and "value".
>> +	 */
>> +	int size1, size2 = 0;
>> +	char ***enable_seq_logs = extract_custom_logs(fp, "MIPI_ENABLE_SEQ",
>> +						      &size1);
>> +	char ***disable_seq_logs = extract_customized_logs(fp,
>> +							   "MIPI_DISABLE_SEQ",
>> +							   &size2);
>> +	document = xmlReadFile(opt.xml_filename, NULL, 0);
>> +	root = xmlDocGetRootElement(document);
>> +	/* first_child will be <Sequence> node.*/
>> +	first_child = root->children;
>> +	for (node = first_child; node; node = node->next) {
>> +		if (node->type == XML_ELEMENT_NODE) {
>> +			/* <Step> nodes will be children of
>> +			 * <Sequence> node.
>> +			*/
>> +			xmlNode *step_root = node->children;
>> +			xmlNode *step_temp_node = step_root;
>> +			for (step_temp_node = step_root; step_temp_node;
>> +			     step_temp_node = step_temp_node->next) {
>> +				if (step_temp_node->type == XML_ELEMENT_NODE) {
>> +					/* <Register> node will be children of
>> +					 * <Step> node.
>> +					 */
>> +					xmlNode *reg_root =
>> +					step_temp_node->children;
>> +					xmlNode *reg_temp_node = reg_root;
>> +					for (reg_temp_node = reg_root;
>> +					     temp_node;
>> +					     temp_node = temp_node->next) {
>> +						if (temp_node->type ==
>> +						    XML_ELEMENT_NODE) {
>> +							int it = 0;
>> +							/* found flag to check
>> +							 * that register is
>> +							 * found in the logs.
>> +							 */
>> +							int found = 0;
>> +							/* check register in log
>> +							 * array.
>> +							 */
>> +							found = check_reg(
>> +								temp_node,
>> +								enable_seq_logs,
>> +								size1)
>> +								;
>> +						}
>> +					}
>> +				}
>> +			}
>> +		}
>> +		/* TODO: when we arrive here, we have stored time of each
>> +		 * register in the XML file, from the dmesg logs when it is
>> +		 * programmed.
>> +		 * Now we have to see that registers are programmed in the order
>> +		 * which is mentioned in Bspec, based on timing information.
>> +		 */
>> +	}
>> +	igt_exit();
>> +}
>
> --
> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] [RFC i-g-t] Test Design to verify mipi enable/disable sequence.
  2017-01-09  9:00 ` Jani Nikula
  2017-01-09 17:09   ` Yadav, Jyoti R
@ 2017-01-11  8:14   ` Daniel Vetter
  1 sibling, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2017-01-11  8:14 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx, ville.syrjala, Jenkins Val

On Mon, Jan 09, 2017 at 11:00:02AM +0200, Jani Nikula wrote:
> On Sat, 07 Jan 2017, Yadav Jyoti <jyoti.r.yadav@intel.com> wrote:
> > From: Jenkins Val <jenkins@intel.com>
> >
> 
> This place here is for the commit message, where you should explain
> *why* we need this change.
> 
> Where do you get the XML file? Do you write it manually? How do you
> manage them? The kernel will execute the sequences from the VBT, not
> from your XML file, so you'll have a problem of maintaining XML files
> for each machine you ever run this test on.
> 
> I'm also not thrilled about adding special debug messages that the test
> depends on finding in dmesg. The test also doesn't actually do anything
> to cause the sequences to be run, so you expect some other, undefined
> tests to have been run, the dmesg from that run captured, and saved to a
> file that you feed to this test.
> 
> I think the design is rather fragile.

Also, igt are black-box testcases, they should not assume any specific
implementation. Every time we break that, we are adding api (even if it's
just for tests in debugfs), and that means coordination issues.

On top of that Chris is building up a neat selftest infrastructure which
helps to cover anything which cannot easily be tested using a blackbox
approach.

Furthermore writing the same stuff twice (like the xml and vbt sequence
this test seems to rely) on isn't validation, it's just typing stuff
twice. Real validation tries to verify (preferrably orthogonal)
invariants, or at least entirely indepent approachs to the implementation.
Another similar case was the color manager testcase, which did not check
functional outcome using crc, but instead checked that the kernel wrote
the right register values in the right places. That's not independent
validation, an hence not really useful as a testcase.

If you want to validate dsi, then either there needs to be some indication
from the sink (on edp we have sink crcs and status flags) that thing went
well, or we need a special testing board like chamelium (but that doesn't
do dsi unfortunately). Everything else is already covered by the generic
modeset testcases and the kernel's selftest.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-01-11  8:14 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-07 18:07 [PATCH] [RFC i-g-t] Test Design to verify mipi enable/disable sequence Yadav Jyoti
2017-01-09  9:00 ` Jani Nikula
2017-01-09 17:09   ` Yadav, Jyoti R
2017-01-10  7:49     ` Jani Nikula
2017-01-11  8:14   ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.