Re: [PATCH] [RFC i-g-t] Test Design to verify mipi enable/disable sequence.

[Jani Nikula <jani.nikula@intel.com>]

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