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

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

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