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

Manasi Navare manasi.d.navare at intel.com
Thu May 19 21:30:36 UTC 2016


Thanks for reviewing, I have a few comments/queries as below:

Manasi

On Thu, May 19, 2016 at 06:40:45PM +0300, Marius Vlad wrote:
> On Mon, May 16, 2016 at 05:50:40PM -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
> > - 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
> > 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>)
> > 
> > v1:
> > * Reworked gem_mmap/unmap logic so these calls go
> > together (by Marius Vlad)
> > * Cleaned up the setup_drm/restore/close calls so that
> > it happens before the tests, after every test and at exit
> > (by Marius Vlad)
> > 
> > Signed-off-by: Marius Vlad <marius.c.vlad at intel.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 | 1089 +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 1090 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..31996ba
> > --- /dev/null
> > +++ b/tools/intel_dp_compliance.c
> > @@ -0,0 +1,1089 @@
> > +/*
> > + * 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.
> > + *
> > + * 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.
> > + *
> > + * Compliance Testing requires several components:
> > + *   - 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
> > + * 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
> > + * 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>)
> > + *
> > + * 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>
> > +
> > +#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)
> > +#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)
> > +
> > +/* 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 size;
> > +	struct igt_fb fb_pattern;
> > +	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;
> > +/* Flag needed to mmap for Video pattern test */
> > +bool video_pattern_flag = false;
> > +
> > +/*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 bool 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,
> > +				drmModeResPtr pmr);
> > +
> > +
> > +static void reset_terminal_mode(void)
> > +{
> > +	tcsetattr(0, TCSANOW, &orig_termios);
> > +}
> > +
> > +static 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);
> > +}
> > +
> > +static 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);
> No point in passing &tv.

I agree, since tv would hold the struct timeval * anyway
> 
> > +}
> > +
> > +
> > +static 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;
> > +}
> > +
> > +static 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;
> > +}
> > +
> > +static 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;
> > +}
> > +
> lower-case.
> > +static 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);
> > +
> > +}
> > +
> > +static 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;
> > +}
> > +
> > +static int process_test_request(int drv_fd, int test_type,
> > +				drmModeResPtr pmr)
> > +{
> > +	int status = -1;
> > +	int mode;
> > +	unsigned long test_data_edid;
> > +	struct dp_connector *connector;
> > +
> > +	/* 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:
> > +		video_pattern_flag = true;
> > +		setup_connectors(drv_fd, pmr);
> setup_connectors() returns 0 even if it failed to find a suitable
> connector is !DRM_MODE_CONNECTOR_DisplayPort or setup_dp_connector()
> failed -> dp_connector_list will be NULL so is the connector.

I will change the logic to return -1 if the connector is not DP or 
if setup_dp_connector failed and check the return value here
> > +		connector = &dp_connector_list[0];
> 
> Why are you indexing it? (connector = &dp_connector_list). It just seems
> you're trying it to access it as an array. On the other hand you
> seem to also have a var to count the number of connectors. Maybe
> it makes more sense to use as it/treat it array, that is, to
> remove the next ptr and use the cntr variable.

This is being indexed here to find the active connector which will be 
the first node in the linked list. So instead of indexing it here I can use
connector = dp_connector_list
In case of compliance testing, the DPR device will be used for each DP separately so
for this tool, it will only have 1 dp connector so only 1 node in the list always.
This was inherited from previous work by Todd and so I kept the design to use the
same linked list. But ideally we will be dealing with only 1 dp connector in this case.

> 
> > +		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);
> > +		break;
> > +	case DP_TEST_LINK_EDID_READ:
> > +		setup_connectors(drv_fd, pmr);
> > +		connector = &dp_connector_list[0];
> > +		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);
> > +		break;
> > +	case DP_TEST_LINK_PHY_TEST_PATTERN:
> > +		/* PHY Test Pattern = future implementation */
> > +		break;
> > +	default:
> > +		/* Unknown test type */
> > +		fprintf(stderr, "Invalid test request. Ignored.\r\n");
> > +		break;
> > +	}
> > +	clear_test_active();
> > +
> > +	/* Return 0 on success and -1 on failure */
> > +	return status;
> > +}
> > +
> > +static void cleanup_debugfs(void)
> > +{
> > +	fclose(test_active_fp);
> > +	fclose(test_data_fp);
> > +	fclose(test_type_fp);
> > +}
> > +
> > +static bool 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;
> > +}
> > +
> > +static void clear_test_active(void)
> > +{
> > +	rewind(test_active_fp);
> > +	fprintf(test_active_fp, "%d", 0);
> > +}
> > +
> > +static 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();
> > +}
> > +
> > +
> > +static 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;
> > +}
> > +
> > +
> > +static 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;
> The head of the list will be NULL if this is not a
> DRM_MODE_CONNECTOR_DisplayPort. See comment where you call this
> function without checking its return value.
> > +
> > +		/* 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++;
> Keep the same style (i.e., if () {} else {} ).
> > +
> > +		/* free connector data and link device into global list */
> > +		drmModeFreeConnector(c);
> > +	}
> > +	drmModeFreeResources(pmr);
> > +
> > +	return 0;
> > +}
> > +
> > +static 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;
> > +		}
> > +	}
> > +
> > +	if (video_pattern_flag) {
> > +		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;
> Maybe it makes sense to call igt_remove_fb()....
> > +		}
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static 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;
> > +}
> > +
> > +static 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);
> No igt_remove_fb()?

This framebuffer gets used in the set_video_mode call and gets rendered.
I can add igt_remove_fb() call at the end of the test.
> > +
> > +	return 0;
> > +}
> > +
> > +static 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;
> > +
> > +}
> > +
> > +
> > +static 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_pattern);
> Somehow this will leak.

Its not clear why this will leak. All the existing igt tests use this igt_assert to make sure 
fb_id returned is valid after calling igt_create_fb

> > +	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_pattern.gem_handle,
> > +						     dp_conn->test_pattern.fb_pattern.size,
> > +						     PROT_READ | PROT_WRITE);
> > +	gem_set_domain(drv_fd, dp_conn->test_pattern.fb_pattern.gem_handle,
> > +		       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > +
> > +	dp_conn->test_pattern.size = dp_conn->test_pattern.fb_pattern.size;
> > +
> > +	memset(dp_conn->test_pattern.pixmap, 0, dp_conn->test_pattern.size);
> > +	return 0;
> > +
> > +}
> > +
> > +
> > +static 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);
> These are all ints, so why not (1 << 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.size);
> > +	return 0;
> > +}
> > +
> > +
> > +static 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;
> > +}
> > +
> > +static 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;
> > +	unsigned long test_type;
> > +
> > +	dp_connector_list = NULL;
> Being global it's already initialized to NULL.
> > +	dp_connector_count = 0;
> Same here.
> > +
> > +	/* 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;
> > +
> > +	/* Set up DRM device before starting any tests */
> > +	status = setup_drm(&drv_fd);
> > +	if (status == -1)
> > +		goto exit;
> > +	/* Save the deafult mode that needs to be restored after every test */
> > +	save_default_mode(drv_fd);
> > +	/* Close the DRM device so it does not interfere with Link training */
> > +	status = close(drv_fd);
> > +	if (status)
> > +		printf("Error: Failed to close i915 driver\r\n");
> > +	setup_debugfs_files();
> > +
> > +	printf("Waiting for test requests. 'X' or 'Q' exits\r\n");
> > +
> > +	while (!done) {
> I have a feeling that getchar() is blocked so kbhit() is useless...

only if any key press is detected through kbhit(), then it calls getchar to catch  the
key pressed and uses that to quit from the tool.
Until then it will just spin through the while loop waiting for test_active to become 1
which happens after the kernel sets test active through debugfs node.
I have verified this behaivour and seems to be working.
> > +		if (kbhit()) {
> > +			input = getchar();
> > +			switch (input) {
> > +			case 'q':
> > +			case 'Q':
> > +			case 'x':
> > +			case 'X':
> > +				done = 1;
> > +				break;
> > +			}
> > +		}
> > +
> > +		usleep(1000);
> > +
> > +		if (check_test_active()) {
> > +
> > +			/* Open the DRM device and restore default mode before
> > +			 * each test
> > +			 */
> > +			status = setup_drm(&drv_fd);
> > +			if (status == -1)
> > +				goto exit;
> > +			restore_default_mode(drv_fd);
> > +
> > +			test_type = get_test_type();
> > +			if (/*test_type == DP_TEST_LINK_VIDEO_PATTERN || */
> > +			    test_type == DP_TEST_LINK_EDID_READ) {
> > +				process_test_request(drv_fd, test_type,
> > +						     mode_resources);
> > +				shutdown(drv_fd);
> > +				drmModeFreeResources(mode_resources);
> > +				mode_resources = NULL;
> > +				video_pattern_flag = false;
> I do not like global variable that gets modified someplace else...

I can get rid of this global flag and pass this as an argument to 
setup_connector function so that it can set them up differently if it
is video pattern test. 
> > +			} else {
> > +				printf("Test type received is not EDID or"
> > +					"Video Pattern\r\n");
> > +				clear_test_active();
> > +			}
> > +			/* Close drm device after each test */
> > +			close(drv_fd);
> > +			usleep(10000);
> Why wait for 10s?

This is to poll the test active flag every 10 secs, as the Compliance test
times out after 10 secs and sends the new test request as per CTS spec.
> > +		}
> > +
> > +	}
> > +
> > +exit:
> > +	cleanup_debugfs();
> > +	/* Set Up DRM device and restore default mode before exit */
> > +	setup_drm(&drv_fd);
> > +	restore_default_mode(drv_fd);
> > +	status = close(drv_fd);
> > +	if (status)
> > +		printf("Error: Failed to close i915 driver\r\n");
> > +	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
> > 




More information about the Intel-gfx mailing list