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.
> 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@xxxxxxxxx>)
>
> Signed-off-by: Todd Previte <tprevite@xxxxxxxxx>
> Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>)
> + *
> + * 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@xxxxxxxxx>
> + * Manasi Navare <manasi.d.navare@xxxxxxxxx>
> + *
> + * 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;
> +}
> +
> +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.
> + 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
> +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@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@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.
> + 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?
> + 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
