[Intel-gfx] [PATCH i-g-t] tools: Add intel_dp_compliance for DisplayPort 1.2 compliance automation

Marius Vlad marius.c.vlad at intel.com
Fri May 6 12:10:34 UTC 2016


Pretty please, use a proper email client :-)

On Thu, May 05, 2016 at 10:22:11PM +0300, Navare, Manasi D wrote:
> 
> On Fri, Apr 29, 2016 at 06:13:33PM -0700, Manasi Navare wrote:
> > This is the userspace component of the Displayport Compliance testing 
> > software required for compliance testing of the I915 Display Port 
> > driver. This must be running in order to successfully complete Display 
> > Port compliance testing. This app and the kernel code that accompanies 
> > it has been written to satify the requirements of the Displayport Link 
> > CTS 1.2 rev1.1 specification from VESA.
> > Note that this application does not support eDP compliance testing.
> > This utility ahs an automation support for the EDID tests (4.2.2.3
> typo.
> > - 4.2.2.6) and Video Pattern generation tests (400.3.3.1) from CTS 
> > specification 1.2 Rev 1.1. Currently the video pattern automation has 
> > been explicitly disabled until the link training automation is fully 
> > supported in the kernel.
> > 
> > The Linux DUT running this utility must be in text (console) mode and 
> > cannot have any other display manager running. Since this uses sysfs 
> > nodes for kernel interaction, this utility should be run as Root. Once 
> > this user application is up and running, waiting for
> 
> lower-case. Is this really necessary? You're describe the testing methodology in the description bellow.
> 
> There is no separate README file so I have added the utility USAGE information here.

I understand, but this is the commit message and the setup is already
described in the file.

> 
> > test requests, the test appliance software on the windows host can now 
> > be used to execute the compliance tests.
> > 
> > This app is based on some prior work done in April 2015 (by Todd 
> > Previte <tprevite at gmail.com>)
> > 
> > Signed-off-by: Todd Previte <tprevite at gmail.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> > ---
> >  tools/Makefile.sources      |    1 +
> >  tools/intel_dp_compliance.c | 1091 
> > +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 1092 insertions(+)
> >  create mode 100644 tools/intel_dp_compliance.c
> > 
> > diff --git a/tools/Makefile.sources b/tools/Makefile.sources index 
> > 5d5958d..3054856 100644
> > --- a/tools/Makefile.sources
> > +++ b/tools/Makefile.sources
> > @@ -13,6 +13,7 @@ bin_PROGRAMS = 				\
> >  	intel_bios_reader 		\
> >  	intel_display_crc		\
> >  	intel_display_poller		\
> > +	intel_dp_compliance		\
> >  	intel_dump_decode 		\
> >  	intel_error_decode 		\
> >  	intel_forcewaked		\
> > diff --git a/tools/intel_dp_compliance.c b/tools/intel_dp_compliance.c 
> > new file mode 100644 index 0000000..f3449e2
> > --- /dev/null
> > +++ b/tools/intel_dp_compliance.c
> > @@ -0,0 +1,1091 @@
> Put the copyright first followed by this description.
> > +/*
> > + * Displayport Compliance Testing Application
> > + *
> > + * This is the userspace component of the Displayport Compliance 
> > +testing
> > + * software required for compliance testing of the I915 Display Port driver.
> > + * This must be running in order to successfully complete Display 
> > +Port
> > + * compliance testing. This app and the kernel code that accompanies 
> > +it has been
> > + * written to satify the requirements of the Displayport Link CTS 1.2 
> > +rev1.1
> > + * specification from VESA. Note that this application does not 
> > +support eDP
> > + * compliance testing.
> > + *
> > + * Complaince Testing requires several components:
> typo.
> > + *   - A kernel build that contains the patch set for DP compliance support
> > + *     Make sure it has the EDID compliance automation patch
> > + *   - A Displayport Compliance Testing appliance such as Unigraf-DPR120
> > + *   - This user application
> > + *   - A windows host machine to run the DPR test software
> > + *   - Root access on the DUT due to the use of sysfs utility
> > + *
> > + * Test Setup:
> > + * It is strongly recommended that the windows host, test appliance 
> > + and DUT
> > + * be freshly restarted before any testing begins to ensure that any 
> > + previous
> > + * configurations and settings will not interfere with test process. 
> > + Refer to
> > + * the test appliance documentation for setup, software installation 
> > + and
> > + * operation specific to that device.
> > + *
> > + * The Linux DUT must be in text (console) mode and cannot have any 
> > + other
> > + * display manager running. You must be logged in as root to run this user app.
> > + * Once the user application is up and running, waiting for test 
> > + requests, the
> > + * software on the windows host can now be used to execute the compliance tests.
> > + *
> > + * This userspace application supports following tests from the DP 
> > + CTS Spec
> > + * Rev 1.1:
> > + *   - EDID Tests: Supports EDID read (4.2.2.3),EDID Read failure and corruption
> > + *     detection tests (4.2.2.4, 4.2.2.5, 4.2.2.6)
> > + *   - Video Pattern generation tests: This supports only the 24 and 18bpp color
> > + *     ramp test pattern (400.3.3.1).
> > + *
> > + * Connections (required):
> > + *   - Test Appliance connected to the external Displayport connector on the DUT
> > + *   - Test Appliance Monitor Out connected to Displayport connector on the
> > + * monitor
> > + *   - Test appliance connected to the Windows Host via USB
> > + *
> > + * Debugfs Files:
> > + * The file root for all  the debugfs file:
> > + * /sys/kernel/debug/dri/0/
> > + *
> > + * The specific files are as follows:
> > + *
> > + * i915_dp_test_active
> > + * A simple flag that indicates whether or not compliance testing is 
> > + currently
> > + * active in the kernel. This flag is polled by userspace and once 
> > + set, invokes
> > + * the test handler in the user app.This flag is set by the test 
					^^^
> > + handler in the
> white-space needed.
> There is a whitespace before the handler, where exactly is it required?

> > + * kernel after reading the registers requested by the test appliance.
> > + *
> > + * i915_dp_test_data
> > + * Test data is used by the kernel to pass parameters to the user 
> > + app. Eg: In
> > + * case of EDID tests, the data that is delivered to the userspace is 
> > + the video
> > + * mode to be set for the test.
> > + * In case of video pattern test, the data that is delivered to the 
> > + userspace is
> > + * the width and height of the test pattern and the bits per color value.
> > + *
> > + * i915_dp_test_type
> > + * The test type variable instructs the user app as to what the 
> > + requested test
> > + * was from the sink device.These values defined at the top of the 
> > + application's
> white-space needed.
> > + * main implementation file must be kept in sync with the values 
> > +defined in the
> > + * kernel's drm_dp_helper.h file.
> > + * This app is based on some prior work submitted in April 2015 by 
> > +Todd Previte
> > + * (<tprevite at gmail.com>)
> > + *
> > + * Copyright ? 2014-2015 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person 
> > +obtaining a
> > + * copy of this software and associated documentation files (the 
> > +"Software"),
> > + * to deal in the Software without restriction, including without 
> > +limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, 
> > +sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom 
> > +the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including 
> > +the next
> > + * paragraph) shall be included in all copies or substantial portions 
> > +of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
> > +EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
> > +MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT 
> > +SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
> > +OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
> > +ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
> > +OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + * Authors:
> > + *    Todd Previte <tprevite at gmail.com>
> > + *    Manasi Navare <manasi.d.navare at intel.com>
> > + *
> > + * Elements of the modeset code adapted from David Herrmann's
> > + * DRM modeset example
> > + *
> > +*/
> > +#include "igt.h"
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +#include <limits.h>
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +#include <math.h>
> > +#include "drm_fourcc.h"
> > +
> > +#include <sys/select.h>
> > +#include <stdbool.h>
> > +#include <assert.h>
> > +#include <xf86drm.h>
> > +#include <xf86drmMode.h>
> > +#include <i915_drm.h>
> > +
> > +#include <sys/stat.h>
> > +#include <signal.h>
> > +#include <termios.h>
> > +#include <fcntl.h>
> > +#include <errno.h>
> > +#include <poll.h>
> > +
> > +#include <sys/mman.h>
> > +#include <time.h>
> > +
> Align these.
> > +#define I915_DRIVER_NODE		"/dev/dri/card0"
> > +#define INTEL_DP_TEST_TYPE_FILE		"i915_dp_test_type"
> > +#define INTEL_DP_TEST_ACTIVE_FILE	"i915_dp_test_active"
> > +#define INTEL_DP_TEST_DATA_FILE		"i915_dp_test_data"
> > +
> > +/* DRM definitions - must be kept in sync with the DRM header */
> > +#define DP_TEST_LINK_TRAINING		(1 << 0)
> Same here.
> > +#define DP_TEST_LINK_VIDEO_PATTERN	(1 << 1)
> > +#define DP_TEST_LINK_EDID_READ		(1 << 2)
> > +#define DP_TEST_LINK_PHY_TEST_PATTERN	(1 << 3) /* DPCD >= 1.1 */
> > +
> > +#define DP_COMPLIANCE_TEST_TYPE_MASK	(DP_TEST_LINK_TRAINING      | \
> > +					DP_TEST_LINK_VIDEO_PATTERN | \
> > +					DP_TEST_LINK_EDID_READ     | \
> > +					DP_TEST_LINK_PHY_TEST_PATTERN)
> > +
> And here.
> > +/* NOTE: These must be kept in sync with the definitions in intel_dp.c */
> > +#define INTEL_DP_EDID_SHIFT_MASK	0
> > +#define INTEL_DP_EDID_OK		(0 << INTEL_DP_EDID_SHIFT_MASK)
> > +#define INTEL_DP_EDID_CORRUPT		(1 << INTEL_DP_EDID_SHIFT_MASK)
> > +#define INTEL_DP_RESOLUTION_SHIFT_MASK	0
> > +#define INTEL_DP_RESOLUTION_PREFERRED	(1 << INTEL_DP_RESOLUTION_SHIFT_MASK)
> > +#define INTEL_DP_RESOLUTION_STANDARD	(2 << INTEL_DP_RESOLUTION_SHIFT_MASK)
> > +#define INTEL_DP_RESOLUTION_FAILSAFE	(3 << INTEL_DP_RESOLUTION_SHIFT_MASK)
> > +#define DP_COMPLIANCE_VIDEO_MODE_MASK	(INTEL_DP_RESOLUTION_PREFERRED | \
> > +					INTEL_DP_RESOLUTION_STANDARD  | \
> > +					INTEL_DP_RESOLUTION_FAILSAFE)
> > +
> > +/* Global Variables */
> > +enum {
> > +	INTEL_MODE_INVALID = -1,
> > +	INTEL_MODE_NONE = 0,
> > +	INTEL_MODE_PREFERRED,
> > +	INTEL_MODE_STANDARD,
> > +	INTEL_MODE_FAILSAFE,
> > +	INTEL_MODE_VIDEO_PATTERN_TEST
> > +} intel_display_mode;
> > +
> > +struct test_video_pattern {
> > +	uint16_t hdisplay;
> > +	uint16_t vdisplay;
> > +	uint8_t bitdepth;
> > +	uint32_t fb;
> > +	uint32_t test_pattern_size;
> just size should be sufficient.
> > +	struct igt_fb fb_test_pattern;
> just pattern should pe sufficient.
> > +	drmModeModeInfo mode;
> > +	uint32_t *pixmap;
> > +};
> > +
> > +
> > +struct dp_connector {
> > +	drmModeModeInfo mode_standard, mode_preferred, mode_failsafe;
> > +	drmModeEncoder *encoder;
> > +	/* Standard and preferred frame buffer*/
> > +	uint32_t fb, fb_width, fb_height, fb_size;
> > +	uint8_t *pixmap;
> > +	struct igt_fb fb_video_pattern;
> > +	/* Failsafe framebuffer - note this is a 16-bit buffer */
> > +	uint32_t failsafe_fb, failsafe_width, failsafe_height;
> > +	uint32_t failsafe_size;
> > +	uint8_t *failsafe_pixmap;
> > +	struct igt_fb fb_failsafe_pattern;
> > +	struct test_video_pattern test_pattern;
> > +	uint32_t conn;
> > +	uint32_t crtc;
> > +	struct dp_connector *next;
> > +};
> > +
> > +/* CRTC to restore the default mode */ drmModeCrtc *default_crtc;
> > +/* HEAD of the DP Connectors Linked List */ struct dp_connector 
> > +*dp_connector_list;
> > +/* Number of DP connector nodes in linked list */ unsigned short 
> > +dp_connector_count; struct termios orig_termios;
> > +/* Global file pointers for the sysfs files */ FILE *test_active_fp, 
> > +*test_data_fp, *test_type_fp;
> > +
> 
> You're declaring these as static, yet when you define them they are global.
> Yes I will define all of them as static as opposed to just static declarations
> > +/*Input handling*/
> > +static void reset_terminal_mode(void); static void 
> > +set_conio_terminal_mode(void); static int kbhit(void);
> > +
> > +/* Test Execution function declearations */ static void 
> > +setup_debugfs_files(void); static int setup_crtc_for_connector(int 
> > +fd, drmModeRes *mr, drmModeConnector *c,
> > +			      struct dp_connector *dp_conn); static int 
> > +setup_framebuffers(int drv_fd, struct dp_connector *dp_conn); static 
> > +int setup_failsafe_framebuffer(int drv_fd, struct dp_connector 
> > +*dp_conn); static int setup_video_pattern_framebuffer(int drv_fd,
> > +					   struct dp_connector *dp_conn); static int 
> > +fill_framebuffer(int drv_fd, struct dp_connector *dp_conn); static 
> > +int setup_dp_connector(int drv_fd, drmModeRes *mr, drmModeConnector *c,
> > +			struct dp_connector *dp_conn);
> > +static int setup_connectors(int drv_fd, drmModeResPtr pmr); static 
> > +int setup_drm(int *drv_fd); static void shutdown(int drv_fd); static 
> > +void cleanup_debugfs(void); static int set_video_mode(int drv_fd, int 
> > +mode,
> > +			  struct dp_connector *test_connector); static int 
> > +check_test_active(void); static void clear_test_active(void); static 
> > +unsigned long get_test_EDID_data(void); static unsigned long 
> > +get_test_type(void); static void get_test_VideoPattern_data(struct 
> > +dp_connector *connector); static int save_default_mode(int drv_fd); 
> > +static int restore_default_mode(int drv_fd); static int 
> > +process_test_request(int drv_fd, int test_type,
> > +				struct dp_connector *connector);
> > +
> > +
> static?
> > +void reset_terminal_mode(void)
> > +{
> > +	tcsetattr(0, TCSANOW, &orig_termios); }
> > +
> > +void set_conio_terminal_mode(void)
> > +{
> > +	struct termios new_termios;
> > +	/* take two copies - one for now or one for later*/
> > +	tcgetattr(0, &orig_termios);
> > +	memcpy(&new_termios, &orig_termios, sizeof(new_termios));
> > +
> > +	/* register cleanup handler, and set the new terminal mode */
> > +	atexit(reset_terminal_mode);
> > +	cfmakeraw(&new_termios);
> > +	tcsetattr(0, TCSANOW, &new_termios); }
> > +
> > +int kbhit(void)
> > +{
> > +	struct timeval tv = { 0L, 0L };
> > +	fd_set fds;
> > +
> > +	FD_ZERO(&fds);
> > +	FD_SET(0, &fds);
> > +
> > +	return select(1, &fds, NULL, NULL, &tv); }
> > +
> > +
> > +int save_default_mode(int drv_fd)
> > +{
> > +	drmModeConnector *c;
> > +	int i, j;
> > +	drmModeRes *mode_resources = drmModeGetResources(drv_fd);
> > +	drmModeEncoder *drm_encoder = NULL;
> > +	int32_t crtc;
> > +
> > +	if (!mode_resources) {
> > +		igt_warn("drmModeGetResources failed: %s\n", strerror(errno));
> > +		return -1;
> > +	}
> > +
> > +	for (i = 0; i < mode_resources->count_connectors; i++) {
> > +
> > +		c = drmModeGetConnector(drv_fd, mode_resources->connectors[i]);
> > +		if (!c) {
> > +			printf("Failed to retrieve connector %u:%u\r\n",
> > +			       i, mode_resources->connectors[i]);
> > +			continue;
> > +		}
> > +
> > +		if (c->connector_type != DRM_MODE_CONNECTOR_DisplayPort)
> > +			continue;
> > +		printf("Supported modes for Connector %d are: \r\n",
> > +		       c->connector_id);
> > +		printf("name refresh (Hz) hdisp hss hse htot vdisp"
> > +		       "vss vse vtot flags type clock\r\n");
> > +		for (j = 0; j < c->count_modes; j++) {
> > +			printf("[%d] ", j);
> > +			printf("%s %d %d %d %d %d %d %d %d %d 0x%x 0x%x %d\r\n",
> > +			       c->modes[j].name, c->modes[j].vrefresh,
> > +			       c->modes[j].hdisplay, c->modes[j].hsync_start,
> > +			       c->modes[j].hsync_end, c->modes[j].htotal,
> > +			       c->modes[j].vdisplay, c->modes[j].vsync_start,
> > +			       c->modes[j].vsync_end, c->modes[j].vtotal,
> > +			       c->modes[j].flags, c->modes[j].type,
> > +			       c->modes[j].clock);
> > +		}
> > +		/* Use attached encoder if possible */
> > +		if (c->encoder_id)
> > +			drm_encoder = drmModeGetEncoder(drv_fd, c->encoder_id);
> > +		if (drm_encoder) {
> > +			if (drm_encoder->crtc_id) {
> > +				crtc = drm_encoder->crtc_id;
> > +			}
> > +		}
> > +		default_crtc = drmModeGetCrtc(drv_fd, crtc);
> > +		drmModeFreeEncoder(drm_encoder);
> > +		drmModeFreeConnector(c);
> > +
> > +	}
> > +	drmModeFreeResources(mode_resources);
> > +	printf("Default Mode is:\r\n");
> > +	printf("%s %d %d %d %d %d %d %d %d %d 0x%x 0x%x %d\r\n",
> > +	       default_crtc->mode.name, default_crtc->mode.vrefresh,
> > +	       default_crtc->mode.hdisplay, default_crtc->mode.hsync_start,
> > +	       default_crtc->mode.hsync_end, default_crtc->mode.htotal,
> > +	       default_crtc->mode.vdisplay, default_crtc->mode.vsync_start,
> > +	       default_crtc->mode.vsync_end, default_crtc->mode.vtotal,
> > +	       default_crtc->mode.flags, default_crtc->mode.type,
> > +	       default_crtc->mode.clock);
> > +
> > +	return 0;
> > +}
> > +
> > +int restore_default_mode(int drv_fd)
> > +{
> > +	drmModeConnector *c;
> > +	int i, j;
> > +	drmModeRes *mode_resources = drmModeGetResources(drv_fd);
> > +	drmModeEncoder *drm_encoder = NULL;
> > +	int32_t crtc;
> > +
> > +	if (!mode_resources) {
> > +		igt_warn("drmModeGetResources failed: %s\n", strerror(errno));
> > +		return -1;
> > +	}
> > +
> > +	for (i = 0; i < mode_resources->count_connectors; i++) {
> > +
> > +		c = drmModeGetConnector(drv_fd, mode_resources->connectors[i]);
> > +		if (!c) {
> > +			printf("Failed to retrieve connector %u:%u\r\n",
> > +			       i, mode_resources->connectors[i]);
> > +			continue;
> > +		}
> > +
> > +		if (c->connector_type != DRM_MODE_CONNECTOR_DisplayPort)
> > +			continue;
> > +		printf("Supported modes for Connector %d are: \r\n",
> > +		       c->connector_id);
> > +		printf("name refresh (Hz) hdisp hss hse htot vdisp "
> > +		       "vss vse vtot flags type clock\r\n");
> > +		for (j = 0; j < c->count_modes; j++) {
> > +			printf("[%d] ", j);
> > +			printf("%s %d %d %d %d %d %d %d %d %d 0x%x 0x%x %d\r\n",
> > +			       c->modes[j].name, c->modes[j].vrefresh,
> > +			       c->modes[j].hdisplay, c->modes[j].hsync_start,
> > +			       c->modes[j].hsync_end, c->modes[j].htotal,
> > +			       c->modes[j].vdisplay, c->modes[j].vsync_start,
> > +			       c->modes[j].vsync_end, c->modes[j].vtotal,
> > +			       c->modes[j].flags, c->modes[j].type,
> > +			       c->modes[j].clock);
> > +		}
> > +		/* Use attached encoder if possible */
> > +		if (c->encoder_id)
> > +			drm_encoder = drmModeGetEncoder(drv_fd, c->encoder_id);
> > +		if (drm_encoder) {
> > +			if (drm_encoder->crtc_id) {
> > +				crtc = drm_encoder->crtc_id;
> > +			}
> > +		}
> > +		drmModeSetCrtc(drv_fd, default_crtc->crtc_id,
> > +				       default_crtc->buffer_id,
> > +				       default_crtc->x,
> > +				       default_crtc->y, &c->connector_id, 1,
> > +				       &default_crtc->mode);
> > +		drmModeFreeEncoder(drm_encoder);
> > +		drmModeFreeConnector(c);
> > +
> > +	}
> > +	drmModeFreeResources(mode_resources);
> > +	printf("Default Mode is: \r\n");
> > +	printf("%s %d %d %d %d %d %d %d %d %d 0x%x 0x%x %d\r\n",
> > +	       default_crtc->mode.name, default_crtc->mode.vrefresh,
> > +	       default_crtc->mode.hdisplay, default_crtc->mode.hsync_start,
> > +	       default_crtc->mode.hsync_end, default_crtc->mode.htotal,
> > +	       default_crtc->mode.vdisplay, default_crtc->mode.vsync_start,
> > +	       default_crtc->mode.vsync_end, default_crtc->mode.vtotal,
> > +	       default_crtc->mode.flags, default_crtc->mode.type,
> > +	       default_crtc->mode.clock);
> > +
> > +	return 0;
> > +}
> > +
> no need for upper-case.
> I will change the function name to use lower case
> > +unsigned long get_test_EDID_data(void) {
> > +	unsigned long test_data;
> > +
> > +	if (!test_data_fp)
> > +		fprintf(stderr, "Invalid Test data File Pointer\r\n");
> > +
> > +	rewind(test_data_fp);
> > +	fscanf(test_data_fp, "%lx", &test_data);
> > +	if (test_data <= 0) {
> > +		printf("Test data read failed - %lx\r\n", test_data);
> > +		return 0;
> > +	}
> > +/
> > +	return test_data;
> > +}
> > +
same here (lower-case).
> > +void get_test_VideoPattern_data(struct dp_connector *connector) {
> > +	int count = 0;
> > +	uint16_t video_pattern_value[3];
> > +	char video_pattern_attribute[15];
> > +
> > +	if (!test_data_fp)
> > +		fprintf(stderr, "Invalid Test data File pointer\r\n");
> > +
> > +	rewind(test_data_fp);
> > +	while (!feof(test_data_fp) && count < 3)
> > +		fscanf(test_data_fp, "%s %u\n", video_pattern_attribute,
> > +		       &video_pattern_value[count++]);
> > +
> > +	connector->test_pattern.hdisplay = video_pattern_value[0];
> > +	connector->test_pattern.vdisplay = video_pattern_value[1];
> > +	connector->test_pattern.bitdepth = video_pattern_value[2];
> > +	printf("Hdisplay = %u\r\n", connector->test_pattern.hdisplay);
> > +	printf("Vdisplay = %u\r\n", connector->test_pattern.vdisplay);
> > +	printf("BitDepth = %u\r\n", connector->test_pattern.bitdepth);
> > +
> > +}
> > +
> > +unsigned long get_test_type(void)
> > +{
> > +	unsigned long test_type;
> > +
> > +	if (!test_type_fp)
> > +		fprintf(stderr, "Invalid Test Type File pointr\r\n");
> > +	rewind(test_type_fp);
> > +	fscanf(test_type_fp, "%02lx", &test_type);
> > +	if (test_type <= 0) {
> > +		printf("Test type read failed - %02lx\r\n", test_type);
> > +		return 0;
> > +	}
> > +
> > +	return test_type;
> > +}
> > +
> > +int process_test_request(int drv_fd, int test_type,
> > +			 struct dp_connector *connector)
> > +{
> > +	int status = -1;
> > +	int mode;
> > +	unsigned long test_data_edid;
> > +
> > +	/* Disable the main link before starting the test
> > +	 * Note that this should probably be signaled through a debugfs file
> > +	 * from the kernel at the start of testing.
> > +	 */
> > +	switch (test_type) {
> > +	case DP_TEST_LINK_VIDEO_PATTERN:
> > +		get_test_VideoPattern_data(connector);
> > +		fill_framebuffer(drv_fd, connector);
> > +		mode = INTEL_MODE_VIDEO_PATTERN_TEST;
> > +		set_video_mode(drv_fd, INTEL_MODE_NONE, connector);
> > +		status = set_video_mode(drv_fd, mode, connector);
> > +		clear_test_active();
> > +		break;
> > +	case DP_TEST_LINK_EDID_READ:
> > +		test_data_edid = get_test_EDID_data();
> > +		mode = (test_data_edid & DP_COMPLIANCE_VIDEO_MODE_MASK) >>
> > +			INTEL_DP_RESOLUTION_SHIFT_MASK;
> > +		set_video_mode(drv_fd, INTEL_MODE_NONE, connector);
> > +		status = set_video_mode(drv_fd, mode, connector);
> > +		/* Free up the mapped memory for tests other than EDID */
> > +		munmap(connector->test_pattern.pixmap,
> > +		       connector->test_pattern.test_pattern_size);
> Why do you need to munmap() here? (It is not clear where you mmap it, and all the above functions do not seem to some kind of mmaping).
> 
> This mapping happens as a part of set up in setup_video_pattern_framebuffer but gets unmapped only in case of Video Pattern tests. 
> So in case of other tests (EDID) we need to explicitly unmap it.

Right, you might want to re-work the gem_mmap/unmmap() parts so it's clearly
visible and easier to follow.

> 
> > +		clear_test_active();
> > +		break;
> > +	case DP_TEST_LINK_PHY_TEST_PATTERN:
> > +		/* PHY Test Pattern = future implementation */
> clear_test_active(), or fall-thru?
> > +		break;
> > +	default:
> > +		/* Unknown test type */
> > +		fprintf(stderr, "Invalid test request. Ignored.\r\n");
> > +		clear_test_active();
> > +		break;
> > +	}
> 
> Most likely you can add it at the the end (clear_test_active())
> > +
> > +	/* Return 0 on success and -1 on failure */
> > +	return status;
> > +}
> > +
> > +void cleanup_debugfs(void)
> > +{
> > +	fclose(test_active_fp);
> > +	fclose(test_data_fp);
> > +	fclose(test_type_fp);
> > +}
> > +
> You can use bool as type.
> > +int check_test_active(void)
> > +{
> > +	unsigned long test_active;
> > +
> > +	if (!test_type_fp)
> > +		fprintf(stderr, "Invalid test type file pointer\r\n");
> > +	rewind(test_active_fp);
> > +	fscanf(test_active_fp, "%lx", &test_active);
> > +	if (test_active == 1)
> > +		return 1;
> > +
> > +	return 0;
> > +}
> > +
> > +void clear_test_active(void)
> > +{
> > +	rewind(test_active_fp);
> > +	fprintf(test_active_fp, "%d", 0);
> > +}
> > +
> > +void setup_debugfs_files(void)
> > +{
> > +	test_type_fp = igt_debugfs_fopen(INTEL_DP_TEST_TYPE_FILE, "r");
> > +	igt_require(test_type_fp);
> > +
> > +	test_data_fp = igt_debugfs_fopen(INTEL_DP_TEST_DATA_FILE, "r");
> > +	igt_require(test_data_fp);
> > +
> > +	test_active_fp = igt_debugfs_fopen(INTEL_DP_TEST_ACTIVE_FILE, "w+");
> > +	igt_require(test_active_fp);
> > +
> > +	/* Reset the active flag for safety */
> > +	clear_test_active();
> > +}
> > +
> > +
> Why a pointer?
> This gets called at several places in main and since the value of drv_fd changes in the function, its passed in as a pointer

Right, so why the need to change it?

> > +int setup_drm(int *drv_fd)
> > +{
> > +	int drm_fd;
> > +
> > +	if (!drmAvailable()) {
> > +		printf("Error: DRM not available\r\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	drm_fd = drm_open_driver(DRIVER_INTEL);
> > +	if (drm_fd == -1) {
> > +		fprintf(stderr, "Cannot Open DRM Device\r\n");
> > +		return -1;
> > +	}
> > +	*drv_fd = drm_fd;
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +int setup_connectors(int drv_fd, drmModeResPtr pmr) {
> > +	int ret, i;
> > +	drmModeConnector *c;
> > +	struct dp_connector *dp_conn;
> > +
> > +	pmr = drmModeGetResources(drv_fd);
> > +	if (!pmr) {
> > +		printf("ERROR: Failed to retrieve DRM resources\r\n");
> > +		return -1;
> > +	}
> > +
> > +	for (i = 0; i < pmr->count_connectors; i++) {
> > +
> > +		c = drmModeGetConnector(drv_fd, pmr->connectors[i]);
> > +		if (!c) {
> > +			printf("Failed to retrieve connector %u:%u\r\n",
> > +			       i, pmr->connectors[i]);
> > +			continue;
> > +		}
> > +
> > +		if (c->connector_type != DRM_MODE_CONNECTOR_DisplayPort)
> > +			continue;
> > +
> > +		dp_conn = malloc(sizeof(*dp_conn));
> > +		memset(dp_conn, 0, sizeof(*dp_conn));
> > +		dp_conn->conn = c->connector_id;
> > +		dp_conn->next = dp_connector_list;
> > +		dp_connector_list = dp_conn;
> > +
> > +		/* Setup the DP connector*/
> > +		ret = setup_dp_connector(drv_fd, pmr, c, dp_conn);
> > +		if (ret == -1) {
> > +			printf("Failed to setup DP connector %u:%u\r\n",
> > +			       i,
> > +			       pmr->connectors[i]);
> > +			free(dp_conn);
> > +			drmModeFreeConnector(c);
> > +			continue;
> > +		} else
> > +			dp_connector_count++;
> > +
> > +		/* free connector data and link device into global list */
> > +		drmModeFreeConnector(c);
> > +	}
> > +	drmModeFreeResources(pmr);
> > +
> > +	return 0;
> > +}
> > +
> > +int setup_dp_connector(int drv_fd, drmModeRes *mr, drmModeConnector *c,
> > +		       struct dp_connector *dp_conn) {
> > +	int ret, i;
> > +	bool found_std = false, found_fs = false;
> > +
> > +	/* Ignore any disconnected devices */
> > +	if (c->connection != DRM_MODE_CONNECTED) {
> > +		printf("Connector %u disconnected\r\n", c->connector_id);
> > +		return -ENOENT;
> > +	}
> > +	printf("Connector Setup:\r\n");
> > +	/* Setup preferred mode - should be mode[0] in the list */
> > +	dp_conn->mode_preferred = c->modes[0];
> > +	dp_conn->fb_width = c->modes[0].hdisplay;
> > +	dp_conn->fb_height = c->modes[0].vdisplay;
> > +
> > +	dp_conn->test_pattern.mode = c->modes[0];
> > +	dp_conn->test_pattern.mode.hdisplay = c->modes[0].hdisplay;
> > +	dp_conn->test_pattern.mode.vdisplay = c->modes[0].vdisplay;
> > +
> > +	printf("Preferred mode (mode 0) for connector %u is %ux%u\r\n",
> > +	       c->connector_id, c->modes[0].hdisplay, c->modes[0].vdisplay);
> > +	fflush(stdin);
> > +
> > +	for (i = 1; i < c->count_modes; i++) {
> > +		/* Standard mode is 800x600 at 60 */
> > +		if (c->modes[i].hdisplay == 800 &&
> > +		    c->modes[i].vdisplay == 600 &&
> > +		    c->modes[i].vrefresh == 60 &&
> > +		    found_std == false) {
> > +			dp_conn->mode_standard = c->modes[i];
> > +			printf("Standard mode (%d) for connector %u is %ux%u\r\n",
> > +			       i,
> > +			       c->connector_id,
> > +			       c->modes[i].hdisplay,
> > +			       c->modes[i].vdisplay);
> > +			found_std = true;
> > +		}
> > +		/* Failsafe mode is 640x480 at 60 */
> > +		if (c->modes[i].hdisplay == 640 &&
> > +		    c->modes[i].vdisplay == 480 &&
> > +		    c->modes[i].vrefresh == 60 &&
> > +		    found_fs == false) {
> > +			dp_conn->mode_failsafe = c->modes[i];
> > +			dp_conn->failsafe_width = c->modes[i].hdisplay;
> > +			dp_conn->failsafe_height = c->modes[i].vdisplay;
> > +			printf("Failsafe mode (%d) for connector %u is %ux%u\r\n",
> > +			       i,
> > +			       c->connector_id,
> > +			       c->modes[i].hdisplay,
> > +			       c->modes[i].vdisplay);
> > +			found_fs = true;
> > +		}
> > +	}
> > +
> > +	ret = setup_crtc_for_connector(drv_fd, mr, c, dp_conn);
> > +	if (ret) {
> > +		printf("Set CRTC for connector %u failed (%d)\r\n",
> > +		       c->connector_id, ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = setup_framebuffers(drv_fd, dp_conn);
> > +	if (ret) {
> > +		printf("Create framebuffer for connector %u failed (%d)\r\n",
> > +		       c->connector_id, ret);
> > +		return ret;
> > +	}
> > +
> > +	if (found_fs) {
> > +		ret = setup_failsafe_framebuffer(drv_fd, dp_conn);
> > +		if (ret) {
> > +			printf("Create failsafe framebuffer for connector %u"
> > +			       "failed (%d)\r\n",
> > +			       c->connector_id, ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	ret = setup_video_pattern_framebuffer(drv_fd, dp_conn);
> > +	if (ret) {
> > +		printf("Create framebuffer for connector %u failed (%d)\r\n",
> > +		       c->connector_id, ret);
> > +		return ret;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +int setup_crtc_for_connector(int fd, drmModeRes *mr, drmModeConnector *c,
> > +			     struct dp_connector *dp_conn) {
> > +	drmModeEncoder *drm_encoder = NULL;
> > +	struct dp_connector *dpc;
> > +	int i, j;
> > +	int32_t crtc;
> > +
> > +	/* Use attached encoder if possible */
> > +	if (c->encoder_id)
> > +		drm_encoder = drmModeGetEncoder(fd, c->encoder_id);
> > +
> > +	if (drm_encoder && drm_encoder->crtc_id) {
> > +
> > +		crtc = drm_encoder->crtc_id;
> > +		printf("Encoder CRTC = %d\r\n", crtc);
> > +		for (dpc = dp_connector_list; dpc; dpc = dpc->next) {
> > +			if (dpc->crtc == crtc) {
> > +				crtc = -1;
> > +				break;
> > +			}
> > +		}
> > +		if (crtc >= 0) {
> > +			drmModeFreeEncoder(drm_encoder);
> > +			dp_conn->crtc = crtc;
> > +			return 0;
> > +		}
> > +		drmModeFreeEncoder(drm_encoder);
> > +	}
> > +
> > +	/* Check all encoder/crtc combinations */
> > +	for (i = 0; i < c->count_encoders; i++) {
> > +		drm_encoder = drmModeGetEncoder(fd, c->encoders[i]);
> > +		if (!drm_encoder)
> > +			continue;
> > +		for (j = 0; j < mr->count_crtcs; j++) {
> > +			if (!(drm_encoder->possible_crtcs & (1 << j)))
> > +				continue;
> > +			crtc = mr->crtcs[j];
> > +			for (dpc = dp_connector_list; dpc; dpc = dpc->next) {
> > +				if (dpc->crtc == crtc) {
> > +					crtc = -1;
> > +					break;
> > +				}
> > +			}
> > +			if (crtc >= 0) {
> > +				drmModeFreeEncoder(drm_encoder);
> > +				dp_conn->crtc = crtc;
> > +				return 0;
> > +			}
> > +		}
> > +		drmModeFreeEncoder(drm_encoder);
> > +	}
> > +	printf("No CRTC available for connector %u\r\n", c->connector_id);
> > +	return -1;
> > +}
> > +
> > +int setup_framebuffers(int drv_fd, struct dp_connector *dp_conn) {
> > +
> > +	dp_conn->fb = igt_create_fb(drv_fd,
> > +				    dp_conn->fb_width, dp_conn->fb_height,
> > +				    DRM_FORMAT_XRGB8888,
> > +				    LOCAL_DRM_FORMAT_MOD_NONE,
> > +				    &dp_conn->fb_video_pattern);
> > +	igt_assert(dp_conn->fb);
> > +
> > +	/* Map the mapping of GEM object into the virtual address space */
> > +	dp_conn->pixmap = gem_mmap__gtt(drv_fd,
> > +					dp_conn->fb_video_pattern.gem_handle,
> > +					dp_conn->fb_video_pattern.size,
> > +					PROT_READ | PROT_WRITE);
> > +	gem_set_domain(drv_fd, dp_conn->fb_video_pattern.gem_handle,
> > +		       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > +	dp_conn->fb_size = dp_conn->fb_video_pattern.size;
> > +
> > +	/* After filling the device memory with 0s it needs to be unmapped */
> > +	memset(dp_conn->pixmap, 0, dp_conn->fb_size);
> > +	munmap(dp_conn->pixmap, dp_conn->fb_size);
> > +
> > +	return 0;
> > +}
> > +
> > +int setup_failsafe_framebuffer(int drv_fd, struct dp_connector 
> > +*dp_conn) {
> > +
> > +	dp_conn->failsafe_fb = igt_create_fb(drv_fd,
> > +					     dp_conn->failsafe_width,
> > +					     dp_conn->failsafe_height,
> > +					     DRM_FORMAT_XRGB8888,
> > +					     LOCAL_DRM_FORMAT_MOD_NONE,
> > +					     &dp_conn->fb_failsafe_pattern);
> > +	igt_assert(dp_conn->failsafe_fb);
> > +
> > +	/* Map the mapping of GEM object into the virtual address space */
> > +	dp_conn->failsafe_pixmap = gem_mmap__gtt(drv_fd,
> > +						 dp_conn->fb_failsafe_pattern.gem_handle,
> > +						 dp_conn->fb_failsafe_pattern.size,
> > +						 PROT_READ | PROT_WRITE);
> > +	gem_set_domain(drv_fd, dp_conn->fb_failsafe_pattern.gem_handle,
> > +		       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > +	dp_conn->failsafe_size = dp_conn->fb_failsafe_pattern.size;
> > +
> > +	/* After filling the device framebuffer the mapped memory needs to be freed */
> > +	memset(dp_conn->failsafe_pixmap, 0, dp_conn->failsafe_size);
> > +	munmap(dp_conn->failsafe_pixmap, dp_conn->failsafe_size);
> > +
> > +	return 0;
> > +
> > +}
> > +
> > +
> > +int setup_video_pattern_framebuffer(int drv_fd, struct dp_connector 
> > +*dp_conn) {
> > +	uint32_t  width, height;
> > +
> > +	width = dp_conn->test_pattern.mode.hdisplay;
> > +	height = dp_conn->test_pattern.mode.vdisplay;
> > +	dp_conn->test_pattern.fb = igt_create_fb(drv_fd,
> > +						 width, height,
> > +						 DRM_FORMAT_XRGB8888,
> > +						 LOCAL_DRM_FORMAT_MOD_NONE,
> > +						 &dp_conn->test_pattern.fb_test_pattern);
> > +	igt_assert(dp_conn->test_pattern.fb);
> > +
> > +	/* Map the mapping of GEM object into the virtual address space */
> > +	dp_conn->test_pattern.pixmap = gem_mmap__gtt(drv_fd,
> > +						     dp_conn->test_pattern.fb_test_pattern.gem_handle,
> > +						     dp_conn->test_pattern.fb_test_pattern.size,
> > +						     PROT_READ | PROT_WRITE);
> > +	gem_set_domain(drv_fd, dp_conn->test_pattern.fb_test_pattern.gem_handle,
> > +		       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > +
> > +	dp_conn->test_pattern.test_pattern_size = 
> > +dp_conn->test_pattern.fb_test_pattern.size;
> > +
> > +	memset(dp_conn->test_pattern.pixmap, 0, dp_conn->test_pattern.test_pattern_size);
> > +	return 0;
> > +
> > +}
> > +
> > +
> > +int fill_framebuffer(int drv_fd, struct dp_connector *dp_conn) {
> > +	int ret;
> > +	uint32_t tile_height, tile_width, width, height;
> > +	uint32_t *red_ptr, *green_ptr, *blue_ptr, *white_ptr, *src_ptr, *dst_ptr;
> > +	int x, y;
> > +	int32_t pixel_val;
> > +
> > +	width = dp_conn->test_pattern.mode.hdisplay;
> > +	height = dp_conn->test_pattern.mode.vdisplay;
> > +
> > +	tile_height = 64;
> > +	tile_width = pow(2, dp_conn->test_pattern.bitdepth);
> > +
> > +	red_ptr = dp_conn->test_pattern.pixmap;
> > +	green_ptr = red_ptr + (width * tile_height);
> > +	blue_ptr = green_ptr + (width * tile_height);
> > +	white_ptr = blue_ptr + (width * tile_height);
> > +	x = 0;
> > +
> > +	/* Fill the frame buffer with video pattern from CTS 3.1.5 */
> > +	while (x < width) {
> > +		for (pixel_val = 0; pixel_val < 256;
> > +		     pixel_val = pixel_val + (256 / tile_width)) {
> > +			red_ptr[x] = pixel_val << 16;
> > +			green_ptr[x] = pixel_val << 8;
> > +			blue_ptr[x] = pixel_val << 0;
> > +			white_ptr[x] = red_ptr[x] | green_ptr[x] | blue_ptr[x];
> > +			if (++x >= width)
> > +				break;
> > +		}
> > +	}
> > +	for (y = 0; y < height; y++) {
> > +		if (y == 0 || y == 64 || y == 128 || y == 192)
> > +			continue;
> > +		switch ((y / tile_height) % 4) {
> > +		case 0:
> > +			src_ptr = red_ptr;
> > +			break;
> > +		case 1:
> > +			src_ptr = green_ptr;
> > +			break;
> > +		case 2:
> > +			src_ptr = blue_ptr;
> > +			break;
> > +		case 3:
> > +			src_ptr = white_ptr;
> > +			break;
> > +		}
> > +		dst_ptr = dp_conn->test_pattern.pixmap + (y * width);
> > +		memcpy(dst_ptr, src_ptr, (width * 4));
> > +	}
> > +	munmap(dp_conn->test_pattern.pixmap,
> > +	       dp_conn->test_pattern.test_pattern_size);
> > +	return 0;
> > +}
> > +
> > +
> > +int set_video_mode(int drv_fd, int mode, struct dp_connector 
> > +*test_connector) {
> > +	drmModeModeInfo *requested_mode;
> > +	uint32_t required_fb;
> > +	int ret = 0;
> > +
> > +	switch (mode) {
> > +	case INTEL_MODE_NONE:
> > +		printf("NONE\r\n");
> > +		ret = drmModeSetCrtc(drv_fd, test_connector->crtc,
> > +				     -1, 0, 0, NULL, 0, NULL);
> > +		goto out;
> > +	case INTEL_MODE_PREFERRED:
> > +		printf("PREFERRED\r\n");
> > +		requested_mode =  &test_connector->mode_preferred;
> > +		required_fb = test_connector->fb;
> > +		break;
> > +	case INTEL_MODE_STANDARD:
> > +		printf("STANDARD\r\n");
> > +		requested_mode =  &test_connector->mode_standard;
> > +		required_fb = test_connector->fb;
> > +		break;
> > +	case INTEL_MODE_FAILSAFE:
> > +		printf("FAILSAFE\r\n");
> > +		requested_mode =  &test_connector->mode_failsafe;
> > +		required_fb = test_connector->failsafe_fb;
> > +		break;
> > +	case INTEL_MODE_VIDEO_PATTERN_TEST:
> > +		printf("VIDEO PATTERN TEST\r\n");
> > +		requested_mode = &test_connector->test_pattern.mode;
> > +		required_fb = test_connector->test_pattern.fb;
> > +		break;
> > +	case INTEL_MODE_INVALID:
> > +	default:
> > +		printf("INVALID! (%08x) Mode set aborted!\r\n", mode);
> > +		return -1;
> > +	}
> > +
> > +	ret = drmModeSetCrtc(drv_fd, test_connector->crtc, required_fb, 0, 0,
> > +			     &test_connector->conn, 1, requested_mode);
> > +
> > +out:
> > +	if (ret) {
> > +		printf("Failed to set CRTC for connector %u\r\n",
> > +		       test_connector->conn);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +void shutdown(int drv_fd)
> > +{
> > +	int i;
> > +	struct dp_connector *index = dp_connector_list, *prev;
> > +
> > +	for (i = 0; i < dp_connector_count; i++) {
> > +
> > +		kmstest_dump_mode(&default_crtc->mode);
> > +
> > +		prev = index;
> > +		index = index->next;
> > +		free(prev);
> > +		prev = NULL;
> > +	}
> > +	dp_connector_list = NULL;
> > +	dp_connector_count = 0;
> > +
> > +}
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +	int status = 0;
> > +	int drv_fd = 0;
> > +	drmModeResPtr mode_resources = NULL;
> > +	int done = 0;
> > +	char input;
> > +	struct dp_connector *active_connector;
> > +	unsigned long test_type;
> > +
> > +	dp_connector_list = NULL;
> > +	dp_connector_count = 0;
> > +
> > +	/* Setup input for keyboard handling - from input.c */
> > +	set_conio_terminal_mode();
> > +
> > +	printf("ATTENTION:\r\n"
> > +	       "   Ensure that no display manager is running\r\n"
> > +	       "   App must be run as root in console/text mode\r\n"
> > +	       "   Continue? (Y/N)\r\n"
> > +	       );
> > +	input = getchar();
> > +
> > +	if (input != 'Y' && input != 'y')
> > +		goto exit;
> > +
> > +
> Why not do the setup once?
> 
> This tool is designed to run many different types of DP compliance tests that do different kinds of modesets or link training . 
> These tests have side effects due to which the system goes in an unknown state. Hence we need to start fresh for each test, reset up the 
> Drm and connectors and restore the original configuration each time.
> This avoids any residues from the previous test or even before starting the test.

Okay, why not split this explicitly in different ``tests'', prepend the test 
with setup_drm()/close_drm()/restore... and then call a cb to run the test. It
seems rather convoluted the way you're doing it, although I see no point
in opening/closing drm fd every time. Would closing/opening affect link
training/modeset?

> 
> > +	status = setup_drm(&drv_fd);
> > +	if (status == -1)
> > +		goto exit;
> > +	save_default_mode(drv_fd);
> > +	status = close(drv_fd);
> > +	if (status)
> > +		printf("Error: Failed to close i915 driver\r\n");
> > +	drv_fd = 0;
> Why? You seem to do this all over.
> > +	setup_debugfs_files();
> > +
> > +	printf("Waiting for test requests. 'X' or 'Q' exits\r\n");
> > +
> > +	while (!done) {
> > +		if (kbhit()) {
> > +			input = getchar();
> > +			switch (input) {
> > +			case 'q':
> > +			case 'Q':
> > +			case 'x':
> > +			case 'X':
> > +				done = 1;
> > +				break;
> > +			}
> > +		}
> > +
> > +		usleep(1000);
> > +
> You might want to inverse the logic so you'll not need so many identations.
> > +		if (check_test_active()) {
> > +
> Here you do it again.
> I do it here so that it opens the drm fresh and starts in a known good state for each test (each time test active is 1)
> > +			status = setup_drm(&drv_fd);
> > +			if (status == -1)
> > +				goto exit;
> > +			restore_default_mode(drv_fd);
> > +			status = close(drv_fd);
> > +			if (status)
> > +				printf("Error: Failed to close i915 driver\r\n");
> > +			drv_fd = 0;
> Setting the fd.
> Does closing the drv_fd ensure setting it to 0?
What do you achieve by setting it to zero (stdin?) 

> > +			test_type = get_test_type();
> > +			if (/*test_type == DP_TEST_LINK_VIDEO_PATTERN || */
> > +			    test_type == DP_TEST_LINK_EDID_READ) {
> > +				status = setup_drm(&drv_fd);
> > +				if (status == -1) {
> > +					clear_test_active();
> > +					goto exit;
> > +				}
> > +				setup_connectors(drv_fd, mode_resources);
> > +				active_connector = &dp_connector_list[0];
> 
> > +				process_test_request(drv_fd, test_type,
> > +						     active_connector);
> You're not using the return value for this function.
> > +				shutdown(drv_fd);
> > +				drmModeFreeResources(mode_resources);
> > +				mode_resources = NULL;
> Not neeeded.
> > +				close(drv_fd);
> > +				drv_fd = 0;
> Setting the fd to 0 again.
> > +				usleep(10000);
> > +			} else {
> > +				printf("Test type received is not EDID or"
> > +					"Video Pattern\r\n");
> > +				clear_test_active();
> > +			}
> > +		}
> > +
> > +	}
> > +
> > +exit:
> > +	cleanup_debugfs();
> > +
> > +	shutdown(drv_fd);
> > +	setup_drm(&drv_fd);
> > +	restore_default_mode(drv_fd);
> > +	status = close(drv_fd);
> > +	if (status)
> > +		printf("Error: Failed to close i915 driver\r\n");
> > +	drv_fd = 0;
> > +	drmModeFreeCrtc(default_crtc);
> > +
> > +	printf("Compliance testing application exiting.\r\n"
> > +	       "If testing has been performed, you may want to restart\r\n"
> > +	       "the system to ensure it returns to normal operation.\r\n");
> > +	igt_exit();
> > +
> > +	return status;
> > +
> > +}
> > --
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20160506/2affc7a0/attachment-0001.sig>


More information about the Intel-gfx mailing list