Re: [PATCH 4/9] drm/i915: Add debugfs functions for Displayport compliance testing

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 3/9/2015 10:57 AM, Jani Nikula wrote:
On Thu, 19 Feb 2015, Todd Previte <tprevite@xxxxxxxxx> wrote:
This patch is the amalgamation of 7 patches from the V2 series. These
patches all involve the implementation of the debugfs mechanism for
handling Displayport compliance testing. The following are the commit
messages from those 7 patches (included here to try and preserve the
patch set history):
I think there should be per connector debugfs files for this, instead of
doing it for all DP connectors.

See my dpcd debugfs patch [1] for an example. I wish that got reviewed
and merged...

BR,
Jani.

When it comes to compliance testing, you generally don't want anything else plugged in anyways. The user app is going to kill all the other displays, since it needs to be DRM master, so it's really kind of pointless. It also makes the implementation less complex. During development and testing, I've run the user app locally (logged in as root directly on the DUT) and over the network (ssh as root into the machine and launched the user app from the console) and the network method seems easier and more efficient to me. This is especially important as I can see the debug output from the user app right on the console instead of staring at a blank screen waiting for the test to complete.

That said, I think this is a reasonable request that can be addressed at a future time, once the base code is in place. The architectural and design changes necessary for this would drag out the integration of basic compliance testing even longer and that's not something I believe we can afford to do right now.

I'll go look for your DPCD patch and get a review on there.

-T


[1] http://mid.gmane.org/1424867645-21608-1-git-send-email-jani.nikula@xxxxxxxxx





-------------------------------------------------------------------------------
[PATCH 04/17] drm/i915: Add debugfs information for Displayport compliance
               testing

Originally, this patch was part of "[PATCH 05/10] drm/i915: Add debugfs
interface for Displayport debug and compliance testing".  It contained
definitions/declarations for some of the constants and data structures
added to support debugfs output for Displayport compliance
testing.
-------------------------------------------------------------------------------
[PATCH 05/17] drm/i915: Add file ops for Displayport configuration in debugfs

This patch was also part of "[PATCH 05/10] drm/i915: Add debugfs interface
for Displayport debug and compliance testing". This one added file operations
structures for Displayport configuration and the declarations for open() and
write() in the file ops structure.
-------------------------------------------------------------------------------
[PATCH 06/17] drm/i915: Add support functions in debugfs for handling
               Displayport compliance configuration

This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs interface
for Displayport debug  and compliance testing". It added two support functions
for handling Displayport configuration parameters that are used for compliance
testing.
-------------------------------------------------------------------------------
[PATCH 07/17] drm/i915: Add and implement the debugfs 'show' functions for
               Displayport compliance

This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs
interface for Displayport debug and compliance testing". This patch implemented
the show() function.
-------------------------------------------------------------------------------
[PATCH 08/17] drm/i915: Add Displayport link configuration structure

This patch was previously part of "[PATCH 07/10] drm/i915: Add structures for
Displayport compliance testing parameters". It added a struct to maintain link
configuration data. These were meant purely for Displayport compliance use.
-------------------------------------------------------------------------------
[PATCH 09/17] drm/i915: Add config parsing utilities in debugfs for Displayport
               compliance

This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs
interface for Displayport debug and compliance testing". This patch adds two
functions to handle parsing of Displayport configuration information as it
formatted in the debugfs file. It is used to process incoming configuration
changes from the userspace compliance application during testing.

-------------------------------------------------------------------------------
[PATCH 10/17] drm/i915: Implement the 'open' and 'write' debugfs functions for
               Displayport compliance

This patch is a combination of sections out of the following two previous
patches:
[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and
compliance testing
[PATCH 07/10] drm/i915: Add structures for Displayport compliance testing
parameters

This patch implements the debugfs functions for 'open' and 'write' as they
are specified in the file ops structure. The 'open' function outputs
Displayport data to the appropriate debugfs file while the 'write' function
handles parsing of user data written to that same file.
-------------------------------------------------------------------------------

The nature and scope of the contents of this patch make breaking it into
smaller chunks difficult. With everthing contained in a single patch, the
code is easier to review and understand.

That said, this monster of a patch does quite a few things. First, it adds
some necessary data structures for performing compliance testing operations
on the kernel side. Specifically, it adds link configuration parameters and
string tokens for the debugfs files. There are multiple utility functions
added here as well in support of the debugfs system for handling compliance
testing. Functions for printing strings to the files with the correct format,
parsing tokens, tokenizing strings and doing the grunt work for writing the
data to the debugfs files.

This patch also adds the basic debugfs structures to build the file for the
link configuration output as well as the file operations structures to handle
the open and show functions.

V2:
- N/A
V3:
- Merged patches 4-10 into a single patch per review feedback
- Added previous commit messages from the other commits integrated
   here to preserve the integrity of the patch sequence
- Adjusted the config_ctl_write function to only check the drm_connector
   struct for a) type == Displayport and b) status == active. This is
   to eliminate patches 14 and 15 from V2, which become unnecessary
   in light of this change.
- Fixed checkpatch.pl errors

Signed-off-by: Todd Previte <tprevite@xxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_debugfs.c | 471 ++++++++++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_drv.h    |  13 +
  2 files changed, 484 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b315f01..b77574e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -33,6 +33,7 @@
  #include <linux/slab.h>
  #include <linux/export.h>
  #include <linux/list_sort.h>
+#include <linux/string.h>
  #include <asm/msr-index.h>
  #include <drm/drmP.h>
  #include "intel_drv.h"
@@ -51,6 +52,121 @@ static const char *yesno(int v)
  	return v ? "yes" : "no";
  }
+#define MAX_DP_CONFIG_LINE_COUNT 64
+
+enum dp_config_param {
+	DP_CONFIG_PARAM_CONNECTOR = 0,
+	DP_CONFIG_PARAM_CONNECTOR_ID,
+	DP_CONFIG_PARAM_LINK_RATE,
+	DP_CONFIG_PARAM_LANE_COUNT,
+	DP_CONFIG_PARAM_VOLTAGE_SWING,
+	DP_CONFIG_PARAM_PREEMPHASIS,
+	DP_CONFIG_PARAM_HRES,
+	DP_CONFIG_PARAM_VRES,
+	DP_CONFIG_PARAM_BPP,
+	DP_PARAMETER_COUNT,
+	DP_CONFIG_PARAM_INVALID = -1
+};
+
+struct dp_config {
+	enum dp_config_param type;
+	unsigned long value;
+};
+
+static char *dp_get_config_str(int for_token)
+{
+	char *s;
+
+	switch (for_token) {
+	case DP_CONFIG_PARAM_CONNECTOR:
+		s = "Connector";
+		break;
+	case DP_CONFIG_PARAM_CONNECTOR_ID:
+		s = "Connector ID";
+		break;
+	case DP_CONFIG_PARAM_LINK_RATE:
+		s = "Link Rate";
+		break;
+	case DP_CONFIG_PARAM_LANE_COUNT:
+		s = "Lane Count";
+		break;
+	case DP_CONFIG_PARAM_VOLTAGE_SWING:
+		s = "Voltage Swing Level";
+		break;
+	case DP_CONFIG_PARAM_PREEMPHASIS:
+		s = "Preemphasis Level";
+		break;
+	case DP_CONFIG_PARAM_HRES:
+		s = "Horizontal Resolution";
+		break;
+	case DP_CONFIG_PARAM_VRES:
+		s = "Vertical Resolution";
+		break;
+	case DP_CONFIG_PARAM_BPP:
+		s = "Bits Per Pixel";
+		break;
+	default:
+		s = "";
+		break;
+	}
+
+	return s;
+}
+
+static void dp_print_string(struct seq_file *m,
+			    enum dp_config_param for_param,
+			    void *data)
+{
+	char *format_string;
+	unsigned long output_value;
+
+	switch (for_param) {
+	case DP_CONFIG_PARAM_CONNECTOR:
+		/* Special case for the string parameter */
+		format_string = "Connector:             %s\n";
+		seq_printf(m, format_string, (char *) data);
+		break;
+	case DP_CONFIG_PARAM_CONNECTOR_ID:
+		format_string = "Connector ID:          %u\n";
+		output_value = *(unsigned int *) data;
+		break;
+	case DP_CONFIG_PARAM_LINK_RATE:
+		format_string = "Link Rate:             %02x\n";
+		output_value = *(unsigned char *) data;
+		break;
+	case DP_CONFIG_PARAM_LANE_COUNT:
+		format_string = "Lane Count:            %u\n";
+		output_value = *(unsigned int *) data;
+		break;
+	case DP_CONFIG_PARAM_VOLTAGE_SWING:
+		format_string = "Voltage Swing Level:   %u\n";
+		output_value = *(unsigned char *) data;
+		break;
+	case DP_CONFIG_PARAM_PREEMPHASIS:
+		format_string = "Preemphasis Level:     %u\n";
+		output_value = *(unsigned char *) data;
+		break;
+	case DP_CONFIG_PARAM_HRES:
+		format_string = "Horizontal Resolution: %d\n";
+		output_value = *(int *) data;
+		break;
+	case DP_CONFIG_PARAM_VRES:
+		format_string = "Vertical Resolution:   %d\n";
+		output_value = *(int *) data;
+		break;
+	case DP_CONFIG_PARAM_BPP:
+		format_string = "Bits Per Pixel:        %u\n";
+		output_value = *(unsigned char *) data;
+		break;
+	default:
+		format_string = NULL;
+		break;
+	}
+
+	if (format_string != NULL && for_param != DP_CONFIG_PARAM_CONNECTOR)
+		seq_printf(m, format_string, output_value);
+}
+
  /* As the drm_debugfs_init() routines are called before dev->dev_private is
   * allocated we need to hook into the minor for release. */
  static int
@@ -3701,6 +3817,360 @@ static const struct file_operations i915_display_crc_ctl_fops = {
  	.write = display_crc_ctl_write
  };
+static void dp_show_config(struct seq_file *m,
+			   struct drm_connector *connector)
+{
+	struct intel_dp *intel_dp;
+	struct intel_crtc *crtc;
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct intel_encoder *intel_encoder = intel_connector->encoder;
+	int pipe_bpp, hres, vres;
+	uint8_t vs[4], pe[4];
+	struct drm_display_mode *mode;
+	int i = 0;
+
+	if (!intel_encoder)
+		return;
+
+	if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT)
+		return;
+
+	intel_dp = enc_to_intel_dp(&intel_encoder->base);
+
+	for (i = 0; i < intel_dp->lane_count; i++) {
+		vs[i] = intel_dp->train_set[i]
+			& DP_TRAIN_VOLTAGE_SWING_MASK;
+		pe[i] = (intel_dp->train_set[i]
+		      & DP_TRAIN_PRE_EMPHASIS_MASK) >>
+			DP_TRAIN_PRE_EMPHASIS_SHIFT;
+	}
+
+	/* Output mode info only if a valid CRTC exists */
+	if (intel_encoder->base.crtc) {
+		crtc = to_intel_crtc(intel_encoder->base.crtc);
+		pipe_bpp = crtc->config->pipe_bpp;
+		mode = &crtc->config->base.adjusted_mode;
+		hres = mode->hdisplay;
+		vres = mode->vdisplay;
+	} else {
+		pipe_bpp = 0;
+		hres = vres = 0;
+	}
+
+	dp_print_string(m,
+			DP_CONFIG_PARAM_CONNECTOR,
+			connector->name);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_CONNECTOR_ID,
+			&connector->base.id);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_LINK_RATE,
+			&intel_dp->link_bw);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_LANE_COUNT,
+			&intel_dp->lane_count);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_VOLTAGE_SWING,
+			&vs[0]);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_PREEMPHASIS,
+			&pe[0]);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_HRES,
+			&hres);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_VRES,
+			&vres);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_BPP,
+			&pipe_bpp);
+}
+
+enum dp_config_param dp_get_param_type(char *input_string)
+{
+	enum dp_config_param param_type = DP_CONFIG_PARAM_INVALID;
+	char *s;
+	int i = 0;
+
+	for (i = 0; i < DP_PARAMETER_COUNT; i++) {
+		s = dp_get_config_str(i);
+		if (strncmp(input_string, s,
+			    strlen(s)) == 0) {
+			param_type = (enum dp_config_param) i;
+			break;
+		}
+	}
+	return param_type;
+}
+
+int dp_get_param_value(enum dp_config_param param,
+			   char *input_string,
+			   void *output_value)
+{
+	int status = 0;
+	unsigned int *out_ptr = (unsigned int *)output_value;
+	char *index = input_string;
+
+	switch (param) {
+	case DP_CONFIG_PARAM_CONNECTOR:
+		output_value = (char *)index;
+		break;
+	case DP_CONFIG_PARAM_LINK_RATE:
+	case DP_CONFIG_PARAM_LANE_COUNT:
+	case DP_CONFIG_PARAM_VOLTAGE_SWING:
+	case DP_CONFIG_PARAM_PREEMPHASIS:
+		status = kstrtol(index, 16, (long *)out_ptr);
+		break;
+	case DP_CONFIG_PARAM_HRES:
+	case DP_CONFIG_PARAM_VRES:
+	case DP_CONFIG_PARAM_BPP:
+		status = kstrtol(index, 10, (long *)out_ptr);
+		break;
+	default:
+		/* Unhandled case */
+		status = -EINVAL;
+		*out_ptr = 0;
+		break;
+	}
+
+	return status;
+}
+
+int dp_tokenize_config(char *input_buffer, char *output[])
+{
+	char *base = input_buffer, *index, *end;
+	int line_count = 0;
+	int i = 0, len = 0;
+	int done = 0;
+
+	if (!input_buffer)
+		return 0;
+
+	while (!done) {
+		index = strpbrk(base, ":");
+		if (index) {
+			len = index - base;
+			*index = '\0';
+			index++;
+			/* Save the type string */
+			output[i] = base;
+			i++;
+			line_count++;
+			end = strpbrk(index, "\n\0");
+			if (end) {
+				*end = '\0';
+				/* Eat up whitespace */
+				while (*index <= 0x20)
+					index++;
+				output[i] = index;
+				i++;
+				line_count++;
+			} else
+				done = 1;
+			/* Move to the next section of the string */
+			base = end + 1;
+		} else
+			done = 1;
+	}
+	return line_count;
+}
+
+static int dp_parse_config(char *input_buffer,
+			   ssize_t buffer_size,
+			   struct intel_dp *intel_dp)
+{
+	int status = 0;
+	char *lines[MAX_DP_CONFIG_LINE_COUNT];
+	int i = 0;
+	struct dp_config parms[DP_PARAMETER_COUNT];
+	int line_count = 0;
+	char *buffer = input_buffer;
+	enum dp_config_param parm_type;
+	unsigned long parm_value;
+
+	line_count = dp_tokenize_config(buffer, lines);
+
+	if (line_count == 0) {
+		DRM_DEBUG_DRIVER("No lines to process\n");
+		return 0;
+	}
+
+	for (i = 0; i < line_count; i += 2) {
+		parm_type = dp_get_param_type(lines[i]);
+		if (parm_type != DP_CONFIG_PARAM_INVALID) {
+			status = dp_get_param_value(parm_type,
+							lines[i+1],
+							&parm_value);
+			if (status == 0) {
+				parms[parm_type].type = parm_type;
+				parms[parm_type].value = parm_value;
+			}
+		}
+	}
+
+	if (parms[DP_CONFIG_PARAM_LINK_RATE].value == 0x06 ||
+	    parms[DP_CONFIG_PARAM_LINK_RATE].value == 0x0a ||
+	    parms[DP_CONFIG_PARAM_LINK_RATE].value == 0x14) {
+		intel_dp->compliance_config.link_rate =
+			parms[DP_CONFIG_PARAM_LINK_RATE].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_LANE_COUNT].value == 0x01 ||
+	    parms[DP_CONFIG_PARAM_LANE_COUNT].value == 0x02 ||
+	    parms[DP_CONFIG_PARAM_LANE_COUNT].value == 0x04) {
+		intel_dp->compliance_config.lane_count =
+			parms[DP_CONFIG_PARAM_LANE_COUNT].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_VOLTAGE_SWING].value <= 0x03 &&
+	    parms[DP_CONFIG_PARAM_VOLTAGE_SWING].value >= 0x00) {
+		intel_dp->compliance_config.vswing_level =
+			parms[DP_CONFIG_PARAM_VOLTAGE_SWING].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_PREEMPHASIS].value <= 0x03 &&
+	    parms[DP_CONFIG_PARAM_PREEMPHASIS].value >= 0x00) {
+		intel_dp->compliance_config.preemp_level =
+			parms[DP_CONFIG_PARAM_PREEMPHASIS].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_BPP].value == 18 ||
+	    parms[DP_CONFIG_PARAM_BPP].value == 24 ||
+	    parms[DP_CONFIG_PARAM_BPP].value == 30 ||
+	    parms[DP_CONFIG_PARAM_BPP].value == 36) {
+		intel_dp->compliance_config.bits_per_pixel =
+			parms[DP_CONFIG_PARAM_PREEMPHASIS].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_HRES].value > 0 &&
+	    parms[DP_CONFIG_PARAM_HRES].value <= 8192) {
+		intel_dp->compliance_config.hres =
+			parms[DP_CONFIG_PARAM_HRES].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_VRES].value > 0 &&
+	    parms[DP_CONFIG_PARAM_VRES].value <= 8192) {
+		intel_dp->compliance_config.vres =
+			parms[DP_CONFIG_PARAM_VRES].value;
+	} else
+		return -EINVAL;
+
+	return status;
+}
+
+static int i915_displayport_config_ctl_show(struct seq_file *m, void *data)
+{
+	struct drm_device *dev = m->private;
+	struct drm_connector *connector;
+	struct list_head *connector_list = &dev->mode_config.connector_list;
+
+	if (!dev)
+		return -ENODEV;
+
+	list_for_each_entry(connector, connector_list, head) {
+
+		if (connector->connector_type !=
+		    DRM_MODE_CONNECTOR_DisplayPort)
+			continue;
+
+		if (connector->status == connector_status_connected) {
+			dp_show_config(m, connector);
+		} else {
+			dp_print_string(m,
+					DP_CONFIG_PARAM_CONNECTOR,
+					connector->name);
+			dp_print_string(m,
+					DP_CONFIG_PARAM_CONNECTOR_ID,
+					&connector->base.id);
+			seq_puts(m, "disconnected\n");
+		}
+	}
+
+	return 0;
+}
+
+static int i915_displayport_config_ctl_open(struct inode *inode,
+				       struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, i915_displayport_config_ctl_show, dev);
+}
+
+static ssize_t i915_displayport_config_ctl_write(struct file *file,
+					    const char __user *ubuf,
+					    size_t len, loff_t *offp)
+{
+	char *input_buffer;
+	int status = 0;
+	struct seq_file *m;
+	struct drm_device *dev;
+	struct drm_connector *connector;
+	struct list_head *connector_list;
+	struct intel_dp *intel_dp;
+
+	m = file->private_data;
+	if (!m) {
+		status = -ENODEV;
+		return status;
+	}
+	dev = m->private;
+
+	if (!dev) {
+		status = -ENODEV;
+		return status;
+	}
+	connector_list = &dev->mode_config.connector_list;
+
+	if (len == 0)
+		return 0;
+
+	input_buffer = kmalloc(len + 1, GFP_KERNEL);
+	if (!input_buffer)
+		return -ENOMEM;
+
+	if (copy_from_user(input_buffer, ubuf, len)) {
+		status = -EFAULT;
+		goto out;
+	}
+
+	input_buffer[len] = '\0';
+	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
+
+	list_for_each_entry(connector, connector_list, head) {
+		if (connector->connector_type ==
+		    DRM_MODE_CONNECTOR_DisplayPort &&
+		    connector->status == connector_status_connected) {
+			intel_dp = enc_to_intel_dp(connector->encoder);
+			status = dp_parse_config(input_buffer, len, intel_dp);
+			if (status < 0)
+				goto out;
+		}
+	}
+out:
+	kfree(input_buffer);
+	if (status < 0)
+		return status;
+
+	*offp += len;
+	return len;
+}
+
+static const struct file_operations i915_displayport_config_ctl_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_displayport_config_ctl_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+	.write = i915_displayport_config_ctl_write
+};
+
  static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
  {
  	struct drm_device *dev = m->private;
@@ -4450,6 +4920,7 @@ static const struct i915_debugfs_files {
  	{"i915_spr_wm_latency", &i915_spr_wm_latency_fops},
  	{"i915_cur_wm_latency", &i915_cur_wm_latency_fops},
  	{"i915_fbc_false_color", &i915_fbc_fc_fops},
+	{"i915_displayport_config_ctl", &i915_displayport_config_ctl_fops}
  };
void intel_display_crc_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 74b30c1..c813d3c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -592,6 +592,18 @@ struct intel_hdmi {
  struct intel_dp_mst_encoder;
  #define DP_MAX_DOWNSTREAM_PORTS		0x10
+struct intel_dp_link_config {
+	uint8_t lane_count;
+	uint8_t link_rate;
+	uint8_t vswing_level;
+	uint8_t preemp_level;
+	uint32_t hres;
+	uint32_t vres;
+	uint8_t bits_per_pixel;
+	struct drm_display_mode *compliance_mode;
+	char connector_name[12];
+};
+
  struct intel_dp {
  	uint32_t output_reg;
  	uint32_t aux_ch_ctl_reg;
@@ -651,6 +663,7 @@ struct intel_dp {
  	/* Displayport compliance testing */
  	unsigned long compliance_test_data;
  	bool compliance_testing_active;
+	struct intel_dp_link_config compliance_config;
  };
struct intel_digital_port {
--
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux