| Message ID | 1412869090-48010-6-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tood, Paulo already mentioned it, so I'll just rehash quickly: I think the points from the internal pre-review first need to be address before we can dig into details. I know that the discussion there pettered out a bit, but imo it's the patch authors responsibility to pick that up again and ping people. One example I've noticed is the use of signals: We really want a pollable file in debugfs (see e.g. the crc stuff). You can always still redirect that to give you a signal, although I highly recommend to avoid signals like the plague - they're simply too much a pain. But now onto the real comment I wanted to make, see below. On Thu, Oct 09, 2014 at 08:38:05AM -0700, Todd Previte wrote: > Adds an interface that allows for Displayport configuration information to be accessed > through debugfs. The information paramters provided here are as follows: > Link rate > Lane count > Bits per pixel > Voltage swing level > Preemphasis level > Display mode > > These parameters are intended to be used by userspace applications that need access > to the link configuration of any active Displayport connection. Additionally, > applications can make adjustments to these parameters through this interface to > allow userspace application to have fine-grained control over link training > paramters. > > Signed-off-by: Todd Previte <tprevite@gmail.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 389 ++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_dp.c | 13 +- > 2 files changed, 397 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index da4036d..2dada18 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,46 @@ static const char *yesno(int v) > return v ? "yes" : "no"; > } > > +#define DP_PARAMETER_COUNT 8 > +#define MAX_DP_CONFIG_LINE_COUNT 64 > + > +enum dp_config_param { > + DP_CONFIG_PARAM_INVALID = -1, > + DP_CONFIG_PARAM_CONNECTOR = 0, > + 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 > +}; I think it's better to expose this as drm properties on the DP connector. This has a pile of upsides: - We don't need to invent a new parser. - Users can play with them to test out different theories. - We could even use this to fix broken panels/boards without vbt or anything using our plan to set up the initial config with a dt firmware blob. So would be a lot more useful than this private interface. For the properties themselves I think we should go with explicit enumerations with default value "automatic" and then an enumeration of all values allowed by DP. For the naming of the properties I guess something like DP_link_lanes, DP_link_clock, ... would look pretty. The properties should be in dev->mode_config (since they're reallly generic) and I think we want one function to register them all in one go in the drm_dp_helper.c library. Maybe we could even put the values into the existing dp source struct so that we don't have to reinvent the decode logic every single time. Now on the properties themselves I think they all make perfect sense except: - bpp: I've thought if we define lanes+clock plus the bpp from the framebuffer (we support 16bpp, 24bpp, 30bpp and 48bpp) we should be able to hit every possible combination. So I wonder in which precise corner-case the userspace program can't set up the link like it wants to without the bpp property. - hres/vres: Seem unused and look like they're back from an earlier design. We should be able to set that through the mode we're setting with setCrtc. With that plus Paulo&Jesse's internal comments addressed I think this is moving into the right direction. Aside (probably just missed it): Where's the userspace side of this? Thanks, Daniel > + > +struct dp_config { > + enum dp_config_param type; > + unsigned long value; > +}; > + > +const char *dp_conf_tokens[DP_PARAMETER_COUNT] = { > + "Connector", > + "Link Rate", > + "Lane Count", > + "Voltage Swing Level", > + "Preemphasis Level", > + "Horizontal Resolution", > + "Vertical Resolution", > + "Bits Per Pixel" > +}; > +/* Strings for writing out dp_configs */ > +#define DP_CONF_STR_CONNECTOR "Connector: %s\n" > +#define DP_CONF_STR_LINKRATE "Link Rate: %02x\n" > +#define DP_CONF_STR_LANES "Lane Count: %u\n" > +#define DP_CONF_STR_VSWING "Voltage Swing Level: %u\n" > +#define DP_CONF_STR_PREEMP "Preemphasis Level: %u\n" > +#define DP_CONF_STR_HRES "Horizontal Resolution: %d\n" > +#define DP_CONF_STR_VRES "Vertical Resolution: %d\n" > +#define DP_CONF_STR_BPP "Bits Per Pixel: %u\n" > + > /* 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 > @@ -3505,6 +3546,353 @@ static const struct file_operations i915_display_crc_ctl_fops = { > .write = display_crc_ctl_write > }; > > +static void displayport_show_config_info(struct seq_file *m, > + struct intel_connector *intel_connector) > +{ > + struct intel_encoder *intel_encoder = intel_connector->encoder; > + struct intel_dp *intel_dp; > + struct intel_crtc *icrtc; > + int pipe_bpp, hres, vres; > + uint8_t vs[4], pe[4]; > + struct drm_display_mode *mode; > + int i = 0; > + > + if (intel_encoder) { > + 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) >> 3; > + } > + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) { > + if (intel_encoder->new_crtc) { > + pipe_bpp = intel_encoder->new_crtc->config.pipe_bpp; > + mode = &intel_encoder->new_crtc->config.adjusted_mode; > + hres = mode->hdisplay; > + vres = mode->vdisplay; > + } else if (intel_encoder->base.crtc) { > + icrtc = to_intel_crtc(intel_encoder->base.crtc); > + pipe_bpp = icrtc->config.pipe_bpp; > + mode = &icrtc->config.adjusted_mode; > + hres = mode->hdisplay; > + vres = mode->vdisplay; > + } else { > + pipe_bpp = 0; > + hres = vres = 0; > + } > + seq_printf(m, DP_CONF_STR_CONNECTOR, > + intel_connector->base.name); > + seq_printf(m, DP_CONF_STR_LINKRATE, intel_dp->link_bw); > + seq_printf(m, DP_CONF_STR_LANES, intel_dp->lane_count); > + seq_printf(m, DP_CONF_STR_VSWING, vs[0]); > + seq_printf(m, DP_CONF_STR_PREEMP, pe[0]); > + seq_printf(m, DP_CONF_STR_HRES, hres); > + seq_printf(m, DP_CONF_STR_VRES, vres); > + seq_printf(m, DP_CONF_STR_BPP, pipe_bpp); > + } > + } > +} > + > +enum dp_config_param displayport_get_config_param_type(char *input_string) > +{ > + enum dp_config_param param_type = DP_CONFIG_PARAM_INVALID; > + int i = 0; > + > + for (i = 0; i < DP_PARAMETER_COUNT; i++) { > + if (strncmp(input_string, dp_conf_tokens[i], > + strlen(dp_conf_tokens[i])) == 0) { > + param_type = (enum dp_config_param) i; > + break; > + } > + } > + return param_type; > +} > + > +int value_for_config_param(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 tokenize_dp_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 displayport_parse_config(char *input_buffer, > + ssize_t buffer_size, > + struct intel_dp_link_config *dp_conf) > +{ > + 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 = tokenize_dp_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 = displayport_get_config_param_type(lines[i]); > + if (parm_type != DP_CONFIG_PARAM_INVALID) { > + status = value_for_config_param(parm_type, > + lines[i+1], > + &parm_value); > + if (status == 0) { > + parms[parm_type].type = parm_type; > + parms[parm_type].value = parm_value; > + } > + } > + } > + > + for (i = 1; i < DP_PARAMETER_COUNT; i++) > + DRM_DEBUG_DRIVER("%s = %lu\n", > + dp_conf_tokens[parms[i].type], > + parms[i].value); > + > + /* Validate any/all incoming parameters */ > + strcpy(dp_conf->connector_name, > + (char *)parms[DP_CONFIG_PARAM_CONNECTOR].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) { > + dp_conf->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) { > + dp_conf->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) { > + dp_conf->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) { > + dp_conf->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) { > + dp_conf->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) { > + dp_conf->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) { > + dp_conf->vres = > + parms[DP_CONFIG_PARAM_VRES].value; > + } else > + return -EINVAL; > + > + return status; > +} > + > +static int displayport_config_ctl_show(struct seq_file *m, void *data) > +{ > + struct drm_device *dev = m->private; > + struct drm_connector *connector; > + struct intel_encoder *intel_encoder; > + struct intel_connector *intel_connector; > + > + if (!dev) > + return -ENODEV; > + > + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > + intel_connector = to_intel_connector(connector); > + intel_encoder = intel_connector->encoder; > + if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) { > + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT || > + intel_encoder->type == INTEL_OUTPUT_UNKNOWN) { > + if (connector->status == connector_status_connected) { > + displayport_show_config_info(m, intel_connector); > + } else { > + seq_printf(m, DP_CONF_STR_CONNECTOR, > + intel_connector->base.name); > + seq_printf(m, "disconnected\n"); > + } > + } > + } > + } > + > + return 0; > +} > + > +static int displayport_config_ctl_open(struct inode *inode, > + struct file *file) > +{ > + struct drm_device *dev = inode->i_private; > + > + return single_open(file, displayport_config_ctl_show, dev); > +} > + > +static ssize_t 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 intel_encoder *intel_encoder; > + struct intel_connector *intel_connector; > + struct intel_dp *intel_dp; > + struct intel_dp_link_config dp_conf; > + > + m = file->private_data; > + if (!m) { > + status = -ENODEV; > + return status; > + } > + dev = m->private; > + > + if (!dev) { > + status = -ENODEV; > + return status; > + } > + > + 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'; > + memset(&dp_conf, 0, sizeof(struct intel_dp_link_config)); > + DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len); > + status = displayport_parse_config(input_buffer, len, &dp_conf); > + > + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > + intel_connector = to_intel_connector(connector); > + intel_encoder = intel_connector->encoder; > + if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) { > + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT || > + intel_encoder->type == INTEL_OUTPUT_UNKNOWN) { > + intel_dp = enc_to_intel_dp(&intel_encoder->base); > + memcpy(&intel_dp->compliance_config, > + &dp_conf, > + sizeof(struct intel_dp_link_config)); > + } > + } > + } > + > + > +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 = displayport_config_ctl_open, > + .read = seq_read, > + .llseek = seq_lseek, > + .release = single_release, > + .write = displayport_config_ctl_write > +}; > + > static void wm_latency_show(struct seq_file *m, const uint16_t wm[5]) > { > struct drm_device *dev = m->private; > @@ -4208,6 +4596,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_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index a7acc61..b3ddd15 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3968,7 +3968,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector) > static uint8_t > intel_dp_autotest_link_training(struct intel_dp *intel_dp) > { > - uint8_t test_result = DP_TEST_NAK; > + uint8_t test_result = DP_TEST_ACK; > return test_result; > } > > @@ -4013,21 +4013,25 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp) > DRM_DEBUG_KMS("Executing LINK_TRAINING request\n"); > intel_dp->compliance_test_data = DP_TEST_LINK_TRAINING; > response = intel_dp_autotest_link_training(intel_dp); > + status = intel_dp_signal_userspace(intel_dp); > break; > case DP_TEST_LINK_VIDEO_PATTERN: > DRM_DEBUG_KMS("Executing TEST_PATTERN request\n"); > intel_dp->compliance_test_data = DP_TEST_LINK_VIDEO_PATTERN; > response = intel_dp_autotest_video_pattern(intel_dp); > + status = intel_dp_signal_userspace(intel_dp); > break; > case DP_TEST_LINK_EDID_READ: > DRM_DEBUG_KMS("Executing EDID request\n"); > intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ; > response = intel_dp_autotest_edid(intel_dp); > + status = intel_dp_signal_userspace(intel_dp); > break; > case DP_TEST_LINK_PHY_TEST_PATTERN: > DRM_DEBUG_KMS("Executing PHY_PATTERN request\n"); > intel_dp->compliance_test_data = DP_TEST_LINK_PHY_TEST_PATTERN; > response = intel_dp_autotest_phy_pattern(intel_dp); > + status = intel_dp_signal_userspace(intel_dp); > break; > /* FAUX is optional in DP 1.2*/ > case DP_TEST_LINK_FAUX_PATTERN: > @@ -4038,10 +4042,9 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp) > DRM_DEBUG_KMS("Unhandled test request\n"); > break; > } > - if (status != 0) { > - response = DP_TEST_NAK; > - DRM_DEBUG_KMS("Error %d processing test request\n", status); > - } > + if (status != 0) > + DRM_DEBUG_KMS("Status code %d processing test request\n", > + status); > status = drm_dp_dpcd_write(&intel_dp->aux, > DP_TEST_RESPONSE, > &response, 1); > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2014-10-23 10:50 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: > Hi Tood, > > Paulo already mentioned it, so I'll just rehash quickly: I think the > points from the internal pre-review first need to be address before we can > dig into details. I know that the discussion there pettered out a bit, but > imo it's the patch authors responsibility to pick that up again and ping > people. One example I've noticed is the use of signals: We really want a > pollable file in debugfs (see e.g. the crc stuff). You can always still > redirect that to give you a signal, although I highly recommend to avoid > signals like the plague - they're simply too much a pain. > > But now onto the real comment I wanted to make, see below. > > On Thu, Oct 09, 2014 at 08:38:05AM -0700, Todd Previte wrote: >> Adds an interface that allows for Displayport configuration information to be accessed >> through debugfs. The information paramters provided here are as follows: >> Link rate >> Lane count >> Bits per pixel >> Voltage swing level >> Preemphasis level >> Display mode >> >> These parameters are intended to be used by userspace applications that need access >> to the link configuration of any active Displayport connection. Additionally, >> applications can make adjustments to these parameters through this interface to >> allow userspace application to have fine-grained control over link training >> paramters. >> >> Signed-off-by: Todd Previte <tprevite@gmail.com> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 389 ++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_dp.c | 13 +- >> 2 files changed, 397 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >> index da4036d..2dada18 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,46 @@ static const char *yesno(int v) >> return v ? "yes" : "no"; >> } >> >> +#define DP_PARAMETER_COUNT 8 >> +#define MAX_DP_CONFIG_LINE_COUNT 64 >> + >> +enum dp_config_param { >> + DP_CONFIG_PARAM_INVALID = -1, >> + DP_CONFIG_PARAM_CONNECTOR = 0, >> + 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 >> +}; > > I think it's better to expose this as drm properties on the DP connector. > This has a pile of upsides: > - We don't need to invent a new parser. > - Users can play with them to test out different theories. > - We could even use this to fix broken panels/boards without vbt or > anything using our plan to set up the initial config with a dt firmware > blob. > > So would be a lot more useful than this private interface. > > For the properties themselves I think we should go with explicit > enumerations with default value "automatic" and then an enumeration of all > values allowed by DP. For the naming of the properties I guess something > like DP_link_lanes, DP_link_clock, ... would look pretty. The properties > should be in dev->mode_config (since they're reallly generic) and I think > we want one function to register them all in one go in the drm_dp_helper.c > library. Maybe we could even put the values into the existing dp source > struct so that we don't have to reinvent the decode logic every single > time. I disagree. I do prefer the debugfs thing. Think about all the examples of people that break their systems by passing i915.enable_foo=1 or i915.enable_rc6=7 and then open bug reports... With debug stuff exposed as DP properties, we're going to increase that problem, and in a way that will make it even harder to detect. I really really think these things should be exposed only to the debugfs users, because then if the debugfs file is closed, we can just ignore all the arguments and keep doing "normal" unaffected modesets. If we don't want the parser, maybe we can make it a binary protocol: we'lll still have to parse it, but it can get much easier. > > Now on the properties themselves I think they all make perfect sense > except: > - bpp: I've thought if we define lanes+clock plus the bpp from the > framebuffer (we support 16bpp, 24bpp, 30bpp and 48bpp) we should be able > to hit every possible combination. So I wonder in which precise > corner-case the userspace program can't set up the link like it wants to > without the bpp property. > - hres/vres: Seem unused and look like they're back from an earlier > design. We should be able to set that through the mode we're setting > with setCrtc. > > With that plus Paulo&Jesse's internal comments addressed I think this is > moving into the right direction. Aside (probably just missed it): Where's > the userspace side of this? > > Thanks, Daniel > >> + >> +struct dp_config { >> + enum dp_config_param type; >> + unsigned long value; >> +}; >> + >> +const char *dp_conf_tokens[DP_PARAMETER_COUNT] = { >> + "Connector", >> + "Link Rate", >> + "Lane Count", >> + "Voltage Swing Level", >> + "Preemphasis Level", >> + "Horizontal Resolution", >> + "Vertical Resolution", >> + "Bits Per Pixel" >> +}; >> +/* Strings for writing out dp_configs */ >> +#define DP_CONF_STR_CONNECTOR "Connector: %s\n" >> +#define DP_CONF_STR_LINKRATE "Link Rate: %02x\n" >> +#define DP_CONF_STR_LANES "Lane Count: %u\n" >> +#define DP_CONF_STR_VSWING "Voltage Swing Level: %u\n" >> +#define DP_CONF_STR_PREEMP "Preemphasis Level: %u\n" >> +#define DP_CONF_STR_HRES "Horizontal Resolution: %d\n" >> +#define DP_CONF_STR_VRES "Vertical Resolution: %d\n" >> +#define DP_CONF_STR_BPP "Bits Per Pixel: %u\n" >> + >> /* 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 >> @@ -3505,6 +3546,353 @@ static const struct file_operations i915_display_crc_ctl_fops = { >> .write = display_crc_ctl_write >> }; >> >> +static void displayport_show_config_info(struct seq_file *m, >> + struct intel_connector *intel_connector) >> +{ >> + struct intel_encoder *intel_encoder = intel_connector->encoder; >> + struct intel_dp *intel_dp; >> + struct intel_crtc *icrtc; >> + int pipe_bpp, hres, vres; >> + uint8_t vs[4], pe[4]; >> + struct drm_display_mode *mode; >> + int i = 0; >> + >> + if (intel_encoder) { >> + 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) >> 3; >> + } >> + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) { >> + if (intel_encoder->new_crtc) { >> + pipe_bpp = intel_encoder->new_crtc->config.pipe_bpp; >> + mode = &intel_encoder->new_crtc->config.adjusted_mode; >> + hres = mode->hdisplay; >> + vres = mode->vdisplay; >> + } else if (intel_encoder->base.crtc) { >> + icrtc = to_intel_crtc(intel_encoder->base.crtc); >> + pipe_bpp = icrtc->config.pipe_bpp; >> + mode = &icrtc->config.adjusted_mode; >> + hres = mode->hdisplay; >> + vres = mode->vdisplay; >> + } else { >> + pipe_bpp = 0; >> + hres = vres = 0; >> + } >> + seq_printf(m, DP_CONF_STR_CONNECTOR, >> + intel_connector->base.name); >> + seq_printf(m, DP_CONF_STR_LINKRATE, intel_dp->link_bw); >> + seq_printf(m, DP_CONF_STR_LANES, intel_dp->lane_count); >> + seq_printf(m, DP_CONF_STR_VSWING, vs[0]); >> + seq_printf(m, DP_CONF_STR_PREEMP, pe[0]); >> + seq_printf(m, DP_CONF_STR_HRES, hres); >> + seq_printf(m, DP_CONF_STR_VRES, vres); >> + seq_printf(m, DP_CONF_STR_BPP, pipe_bpp); >> + } >> + } >> +} >> + >> +enum dp_config_param displayport_get_config_param_type(char *input_string) >> +{ >> + enum dp_config_param param_type = DP_CONFIG_PARAM_INVALID; >> + int i = 0; >> + >> + for (i = 0; i < DP_PARAMETER_COUNT; i++) { >> + if (strncmp(input_string, dp_conf_tokens[i], >> + strlen(dp_conf_tokens[i])) == 0) { >> + param_type = (enum dp_config_param) i; >> + break; >> + } >> + } >> + return param_type; >> +} >> + >> +int value_for_config_param(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 tokenize_dp_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 displayport_parse_config(char *input_buffer, >> + ssize_t buffer_size, >> + struct intel_dp_link_config *dp_conf) >> +{ >> + 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 = tokenize_dp_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 = displayport_get_config_param_type(lines[i]); >> + if (parm_type != DP_CONFIG_PARAM_INVALID) { >> + status = value_for_config_param(parm_type, >> + lines[i+1], >> + &parm_value); >> + if (status == 0) { >> + parms[parm_type].type = parm_type; >> + parms[parm_type].value = parm_value; >> + } >> + } >> + } >> + >> + for (i = 1; i < DP_PARAMETER_COUNT; i++) >> + DRM_DEBUG_DRIVER("%s = %lu\n", >> + dp_conf_tokens[parms[i].type], >> + parms[i].value); >> + >> + /* Validate any/all incoming parameters */ >> + strcpy(dp_conf->connector_name, >> + (char *)parms[DP_CONFIG_PARAM_CONNECTOR].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) { >> + dp_conf->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) { >> + dp_conf->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) { >> + dp_conf->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) { >> + dp_conf->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) { >> + dp_conf->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) { >> + dp_conf->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) { >> + dp_conf->vres = >> + parms[DP_CONFIG_PARAM_VRES].value; >> + } else >> + return -EINVAL; >> + >> + return status; >> +} >> + >> +static int displayport_config_ctl_show(struct seq_file *m, void *data) >> +{ >> + struct drm_device *dev = m->private; >> + struct drm_connector *connector; >> + struct intel_encoder *intel_encoder; >> + struct intel_connector *intel_connector; >> + >> + if (!dev) >> + return -ENODEV; >> + >> + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { >> + intel_connector = to_intel_connector(connector); >> + intel_encoder = intel_connector->encoder; >> + if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) { >> + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT || >> + intel_encoder->type == INTEL_OUTPUT_UNKNOWN) { >> + if (connector->status == connector_status_connected) { >> + displayport_show_config_info(m, intel_connector); >> + } else { >> + seq_printf(m, DP_CONF_STR_CONNECTOR, >> + intel_connector->base.name); >> + seq_printf(m, "disconnected\n"); >> + } >> + } >> + } >> + } >> + >> + return 0; >> +} >> + >> +static int displayport_config_ctl_open(struct inode *inode, >> + struct file *file) >> +{ >> + struct drm_device *dev = inode->i_private; >> + >> + return single_open(file, displayport_config_ctl_show, dev); >> +} >> + >> +static ssize_t 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 intel_encoder *intel_encoder; >> + struct intel_connector *intel_connector; >> + struct intel_dp *intel_dp; >> + struct intel_dp_link_config dp_conf; >> + >> + m = file->private_data; >> + if (!m) { >> + status = -ENODEV; >> + return status; >> + } >> + dev = m->private; >> + >> + if (!dev) { >> + status = -ENODEV; >> + return status; >> + } >> + >> + 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'; >> + memset(&dp_conf, 0, sizeof(struct intel_dp_link_config)); >> + DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len); >> + status = displayport_parse_config(input_buffer, len, &dp_conf); >> + >> + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { >> + intel_connector = to_intel_connector(connector); >> + intel_encoder = intel_connector->encoder; >> + if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) { >> + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT || >> + intel_encoder->type == INTEL_OUTPUT_UNKNOWN) { >> + intel_dp = enc_to_intel_dp(&intel_encoder->base); >> + memcpy(&intel_dp->compliance_config, >> + &dp_conf, >> + sizeof(struct intel_dp_link_config)); >> + } >> + } >> + } >> + >> + >> +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 = displayport_config_ctl_open, >> + .read = seq_read, >> + .llseek = seq_lseek, >> + .release = single_release, >> + .write = displayport_config_ctl_write >> +}; >> + >> static void wm_latency_show(struct seq_file *m, const uint16_t wm[5]) >> { >> struct drm_device *dev = m->private; >> @@ -4208,6 +4596,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_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index a7acc61..b3ddd15 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -3968,7 +3968,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector) >> static uint8_t >> intel_dp_autotest_link_training(struct intel_dp *intel_dp) >> { >> - uint8_t test_result = DP_TEST_NAK; >> + uint8_t test_result = DP_TEST_ACK; >> return test_result; >> } >> >> @@ -4013,21 +4013,25 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp) >> DRM_DEBUG_KMS("Executing LINK_TRAINING request\n"); >> intel_dp->compliance_test_data = DP_TEST_LINK_TRAINING; >> response = intel_dp_autotest_link_training(intel_dp); >> + status = intel_dp_signal_userspace(intel_dp); >> break; >> case DP_TEST_LINK_VIDEO_PATTERN: >> DRM_DEBUG_KMS("Executing TEST_PATTERN request\n"); >> intel_dp->compliance_test_data = DP_TEST_LINK_VIDEO_PATTERN; >> response = intel_dp_autotest_video_pattern(intel_dp); >> + status = intel_dp_signal_userspace(intel_dp); >> break; >> case DP_TEST_LINK_EDID_READ: >> DRM_DEBUG_KMS("Executing EDID request\n"); >> intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ; >> response = intel_dp_autotest_edid(intel_dp); >> + status = intel_dp_signal_userspace(intel_dp); >> break; >> case DP_TEST_LINK_PHY_TEST_PATTERN: >> DRM_DEBUG_KMS("Executing PHY_PATTERN request\n"); >> intel_dp->compliance_test_data = DP_TEST_LINK_PHY_TEST_PATTERN; >> response = intel_dp_autotest_phy_pattern(intel_dp); >> + status = intel_dp_signal_userspace(intel_dp); >> break; >> /* FAUX is optional in DP 1.2*/ >> case DP_TEST_LINK_FAUX_PATTERN: >> @@ -4038,10 +4042,9 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp) >> DRM_DEBUG_KMS("Unhandled test request\n"); >> break; >> } >> - if (status != 0) { >> - response = DP_TEST_NAK; >> - DRM_DEBUG_KMS("Error %d processing test request\n", status); >> - } >> + if (status != 0) >> + DRM_DEBUG_KMS("Status code %d processing test request\n", >> + status); >> status = drm_dp_dpcd_write(&intel_dp->aux, >> DP_TEST_RESPONSE, >> &response, 1); >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 23, 2014 at 10:58:59AM -0200, Paulo Zanoni wrote: > 2014-10-23 10:50 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: > > Hi Tood, > > > > Paulo already mentioned it, so I'll just rehash quickly: I think the > > points from the internal pre-review first need to be address before we can > > dig into details. I know that the discussion there pettered out a bit, but > > imo it's the patch authors responsibility to pick that up again and ping > > people. One example I've noticed is the use of signals: We really want a > > pollable file in debugfs (see e.g. the crc stuff). You can always still > > redirect that to give you a signal, although I highly recommend to avoid > > signals like the plague - they're simply too much a pain. > > > > But now onto the real comment I wanted to make, see below. > > > > On Thu, Oct 09, 2014 at 08:38:05AM -0700, Todd Previte wrote: > >> Adds an interface that allows for Displayport configuration information to be accessed > >> through debugfs. The information paramters provided here are as follows: > >> Link rate > >> Lane count > >> Bits per pixel > >> Voltage swing level > >> Preemphasis level > >> Display mode > >> > >> These parameters are intended to be used by userspace applications that need access > >> to the link configuration of any active Displayport connection. Additionally, > >> applications can make adjustments to these parameters through this interface to > >> allow userspace application to have fine-grained control over link training > >> paramters. > >> > >> Signed-off-by: Todd Previte <tprevite@gmail.com> > >> --- > >> drivers/gpu/drm/i915/i915_debugfs.c | 389 ++++++++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/i915/intel_dp.c | 13 +- > >> 2 files changed, 397 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > >> index da4036d..2dada18 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,46 @@ static const char *yesno(int v) > >> return v ? "yes" : "no"; > >> } > >> > >> +#define DP_PARAMETER_COUNT 8 > >> +#define MAX_DP_CONFIG_LINE_COUNT 64 > >> + > >> +enum dp_config_param { > >> + DP_CONFIG_PARAM_INVALID = -1, > >> + DP_CONFIG_PARAM_CONNECTOR = 0, > >> + 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 > >> +}; > > > > I think it's better to expose this as drm properties on the DP connector. > > This has a pile of upsides: > > - We don't need to invent a new parser. > > - Users can play with them to test out different theories. > > - We could even use this to fix broken panels/boards without vbt or > > anything using our plan to set up the initial config with a dt firmware > > blob. > > > > So would be a lot more useful than this private interface. > > > > For the properties themselves I think we should go with explicit > > enumerations with default value "automatic" and then an enumeration of all > > values allowed by DP. For the naming of the properties I guess something > > like DP_link_lanes, DP_link_clock, ... would look pretty. The properties > > should be in dev->mode_config (since they're reallly generic) and I think > > we want one function to register them all in one go in the drm_dp_helper.c > > library. Maybe we could even put the values into the existing dp source > > struct so that we don't have to reinvent the decode logic every single > > time. > > I disagree. I do prefer the debugfs thing. Think about all the > examples of people that break their systems by passing > i915.enable_foo=1 or i915.enable_rc6=7 and then open bug reports... > With debug stuff exposed as DP properties, we're going to increase > that problem, and in a way that will make it even harder to detect. I > really really think these things should be exposed only to the debugfs > users, because then if the debugfs file is closed, we can just ignore > all the arguments and keep doing "normal" unaffected modesets. > > If we don't want the parser, maybe we can make it a binary protocol: > we'lll still have to parse it, but it can get much easier. We already expose piles of funky stuff to users in these properties (e.g. audio on hdmi). And contrary to the module options most of these will just result in black screens if you don't understand what you're doing, so I think the risk is low. There's also a bit of an issue with the current interface as it's not clear which line corresponds to which DP interface. Using properties would solve this. Also these options have a much more direct impact - if you set them and the screen goes dark then the user hopefully realizes that this is something they shouldn't touch. For fbc and rc6 and all those the problem is that nothing really happens, the system just becomes a bit more unstable and randomly crashes. Which users then report. But If you're really concerned about users doing crappy stuff we could add a module option to drm_dp_helper.c which hides these, and then taint the kernel if any user sets them. But I really think this is overkill. -Daniel
On Thu, 23 Oct 2014 17:43:01 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Oct 23, 2014 at 10:58:59AM -0200, Paulo Zanoni wrote: > > 2014-10-23 10:50 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: > > > I think it's better to expose this as drm properties on the DP > > > connector. This has a pile of upsides: > > > - We don't need to invent a new parser. > > > - Users can play with them to test out different theories. > > > - We could even use this to fix broken panels/boards without vbt > > > or anything using our plan to set up the initial config with a dt > > > firmware blob. > > > > > > So would be a lot more useful than this private interface. > > > > > > For the properties themselves I think we should go with explicit > > > enumerations with default value "automatic" and then an > > > enumeration of all values allowed by DP. For the naming of the > > > properties I guess something like DP_link_lanes, > > > DP_link_clock, ... would look pretty. The properties should be in > > > dev->mode_config (since they're reallly generic) and I think we > > > want one function to register them all in one go in the > > > drm_dp_helper.c library. Maybe we could even put the values into > > > the existing dp source struct so that we don't have to reinvent > > > the decode logic every single time. > > > > I disagree. I do prefer the debugfs thing. Think about all the > > examples of people that break their systems by passing > > i915.enable_foo=1 or i915.enable_rc6=7 and then open bug reports... > > With debug stuff exposed as DP properties, we're going to increase > > that problem, and in a way that will make it even harder to detect. > > I really really think these things should be exposed only to the > > debugfs users, because then if the debugfs file is closed, we can > > just ignore all the arguments and keep doing "normal" unaffected > > modesets. > > > > If we don't want the parser, maybe we can make it a binary protocol: > > we'lll still have to parse it, but it can get much easier. > > We already expose piles of funky stuff to users in these properties > (e.g. audio on hdmi). And contrary to the module options most of > these will just result in black screens if you don't understand what > you're doing, so I think the risk is low. There's also a bit of an > issue with the current interface as it's not clear which line > corresponds to which DP interface. Using properties would solve this. > Also these options have a much more direct impact - if you set them > and the screen goes dark then the user hopefully realizes that this > is something they shouldn't touch. > > For fbc and rc6 and all those the problem is that nothing really > happens, the system just becomes a bit more unstable and randomly > crashes. Which users then report. > > But If you're really concerned about users doing crappy stuff we > could add a module option to drm_dp_helper.c which hides these, and > then taint the kernel if any user sets them. But I really think this > is overkill. -Daniel Just chatted with Todd about the state of this patchset. He's going to post an update today or tomorrow and summarize the feedback from July ad what he's done to address it, add changelogs, and address outstanding review feedback from this posting. I like the idea of exposing some DP stuff like lane count, preemph, etc, as new properties, but I don't think it's reasonable to block the testing stuff on it, for a few reasons: 1) we should think pretty hard about how we want new ABI like this exposed 2) the compliance spec changes pretty regularly, so keeping an unstable interface for it in debugfs may make sense anyway 3) even if we decide to move the userland test code over to properties in the future, the fact that debugfs is unstable means we can drop it at that time So I asked Todd to file a JIRA for the properties feature request, since it's definitely worth looking at. Which isn't to say the current interface doesn't have issues, just that Todd shouldn't rewrite things again to include a new, stable ABI, probably keeping compliance testing and additional DP test coverage out of the tree for even longer. Thoughts, objections? Thanks, Jesse
On Thu, Nov 13, 2014 at 7:44 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >> > > I think it's better to expose this as drm properties on the DP >> > > connector. This has a pile of upsides: >> > > - We don't need to invent a new parser. >> > > - Users can play with them to test out different theories. >> > > - We could even use this to fix broken panels/boards without vbt >> > > or anything using our plan to set up the initial config with a dt >> > > firmware blob. >> > > >> > > So would be a lot more useful than this private interface. >> > > >> > > For the properties themselves I think we should go with explicit >> > > enumerations with default value "automatic" and then an >> > > enumeration of all values allowed by DP. For the naming of the >> > > properties I guess something like DP_link_lanes, >> > > DP_link_clock, ... would look pretty. The properties should be in >> > > dev->mode_config (since they're reallly generic) and I think we >> > > want one function to register them all in one go in the >> > > drm_dp_helper.c library. Maybe we could even put the values into >> > > the existing dp source struct so that we don't have to reinvent >> > > the decode logic every single time. >> > >> > I disagree. I do prefer the debugfs thing. Think about all the >> > examples of people that break their systems by passing >> > i915.enable_foo=1 or i915.enable_rc6=7 and then open bug reports... >> > With debug stuff exposed as DP properties, we're going to increase >> > that problem, and in a way that will make it even harder to detect. >> > I really really think these things should be exposed only to the >> > debugfs users, because then if the debugfs file is closed, we can >> > just ignore all the arguments and keep doing "normal" unaffected >> > modesets. >> > >> > If we don't want the parser, maybe we can make it a binary protocol: >> > we'lll still have to parse it, but it can get much easier. >> >> We already expose piles of funky stuff to users in these properties >> (e.g. audio on hdmi). And contrary to the module options most of >> these will just result in black screens if you don't understand what >> you're doing, so I think the risk is low. There's also a bit of an >> issue with the current interface as it's not clear which line >> corresponds to which DP interface. Using properties would solve this. >> Also these options have a much more direct impact - if you set them >> and the screen goes dark then the user hopefully realizes that this >> is something they shouldn't touch. >> >> For fbc and rc6 and all those the problem is that nothing really >> happens, the system just becomes a bit more unstable and randomly >> crashes. Which users then report. >> >> But If you're really concerned about users doing crappy stuff we >> could add a module option to drm_dp_helper.c which hides these, and >> then taint the kernel if any user sets them. But I really think this >> is overkill. -Daniel > > Just chatted with Todd about the state of this patchset. He's going to > post an update today or tomorrow and summarize the feedback from July > ad what he's done to address it, add changelogs, and address outstanding > review feedback from this posting. > > I like the idea of exposing some DP stuff like lane count, preemph, > etc, as new properties, but I don't think it's reasonable to block the > testing stuff on it, for a few reasons: > 1) we should think pretty hard about how we want new ABI like > this exposed > 2) the compliance spec changes pretty regularly, so keeping an > unstable interface for it in debugfs may make sense anyway > 3) even if we decide to move the userland test code over to > properties in the future, the fact that debugfs is unstable means > we can drop it at that time > > So I asked Todd to file a JIRA for the properties feature request, > since it's definitely worth looking at. > > Which isn't to say the current interface doesn't have issues, just that > Todd shouldn't rewrite things again to include a new, stable ABI, > probably keeping compliance testing and additional DP test coverage out > of the tree for even longer. > > Thoughts, objections? Ok, so from a really, really high-level perspective the design review from my side in July had one primary goal and a corollary from that: - DP validation should exercise the same code paths (where possible) than what actual customers use in DP mode sets. If we implement completely separate codepaths (which the first iteration partially did) then we validate a separate driver and not actually what customers run. Which is fairly pointless imo. - Corollary was that this means we should drive the full kernel stack and so control the testing from userspace using the same interfaces as any userspace would use for a modeset. And if that interface has gaps, or our kernel side DP driver has gaps we'd need to fill them. debugfs should only serve as the testing sideband between the DP tester (sink device) and the userspace test program. So the kernel just passes stuff back&forth like test requests (e.g. "pls set this mode on now and show test pattern") and return test results (e.g. "I've read the edid now, here's the checksum"). Of course for some test requests we can directly answer them from the kernel (e.g. "tell me how many dp aux failures you've seen"). Examples completely made up ;-) So from that perpective adjusting DP link paramters is either a new tuning bit, which really should be generally available. Or it indicates a bug/omission in our DP link training or re-training logic. In either case I don't think it's really pure test-related sideband date. I guess I didn't make this 100% clear in the original design review what my goal is, or maybe Todd misunderstood things a bit. Aside from all this (and now with my community hat on) just adding code to get a sticker (labelled "passed DP validation") which is separate code and not used by actual users is imo not useful for merging upstream. But that's just my own opinion, not sure what's Dave's stance here or whether there's much precendence either way. So short answer: I still think exposing this with properties is the right approach, presuming we really need it (and it's not just to paper over a deficient link training logic in the kernel). I also think it'll be less code since we can simplify the debugfs option parser. -Daniel
On 14 November 2014 06:44, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Nov 13, 2014 at 7:44 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >>> > > I think it's better to expose this as drm properties on the DP >>> > > connector. This has a pile of upsides: >>> > > - We don't need to invent a new parser. >>> > > - Users can play with them to test out different theories. >>> > > - We could even use this to fix broken panels/boards without vbt >>> > > or anything using our plan to set up the initial config with a dt >>> > > firmware blob. >>> > > >>> > > So would be a lot more useful than this private interface. >>> > > >>> > > For the properties themselves I think we should go with explicit >>> > > enumerations with default value "automatic" and then an >>> > > enumeration of all values allowed by DP. For the naming of the >>> > > properties I guess something like DP_link_lanes, >>> > > DP_link_clock, ... would look pretty. The properties should be in >>> > > dev->mode_config (since they're reallly generic) and I think we >>> > > want one function to register them all in one go in the >>> > > drm_dp_helper.c library. Maybe we could even put the values into >>> > > the existing dp source struct so that we don't have to reinvent >>> > > the decode logic every single time. >>> > >>> > I disagree. I do prefer the debugfs thing. Think about all the >>> > examples of people that break their systems by passing >>> > i915.enable_foo=1 or i915.enable_rc6=7 and then open bug reports... >>> > With debug stuff exposed as DP properties, we're going to increase >>> > that problem, and in a way that will make it even harder to detect. >>> > I really really think these things should be exposed only to the >>> > debugfs users, because then if the debugfs file is closed, we can >>> > just ignore all the arguments and keep doing "normal" unaffected >>> > modesets. >>> > >>> > If we don't want the parser, maybe we can make it a binary protocol: >>> > we'lll still have to parse it, but it can get much easier. >>> >>> We already expose piles of funky stuff to users in these properties >>> (e.g. audio on hdmi). And contrary to the module options most of >>> these will just result in black screens if you don't understand what >>> you're doing, so I think the risk is low. There's also a bit of an >>> issue with the current interface as it's not clear which line >>> corresponds to which DP interface. Using properties would solve this. >>> Also these options have a much more direct impact - if you set them >>> and the screen goes dark then the user hopefully realizes that this >>> is something they shouldn't touch. >>> >>> For fbc and rc6 and all those the problem is that nothing really >>> happens, the system just becomes a bit more unstable and randomly >>> crashes. Which users then report. >>> >>> But If you're really concerned about users doing crappy stuff we >>> could add a module option to drm_dp_helper.c which hides these, and >>> then taint the kernel if any user sets them. But I really think this >>> is overkill. -Daniel >> >> Just chatted with Todd about the state of this patchset. He's going to >> post an update today or tomorrow and summarize the feedback from July >> ad what he's done to address it, add changelogs, and address outstanding >> review feedback from this posting. >> >> I like the idea of exposing some DP stuff like lane count, preemph, >> etc, as new properties, but I don't think it's reasonable to block the >> testing stuff on it, for a few reasons: >> 1) we should think pretty hard about how we want new ABI like >> this exposed >> 2) the compliance spec changes pretty regularly, so keeping an >> unstable interface for it in debugfs may make sense anyway >> 3) even if we decide to move the userland test code over to >> properties in the future, the fact that debugfs is unstable means >> we can drop it at that time >> >> So I asked Todd to file a JIRA for the properties feature request, >> since it's definitely worth looking at. >> >> Which isn't to say the current interface doesn't have issues, just that >> Todd shouldn't rewrite things again to include a new, stable ABI, >> probably keeping compliance testing and additional DP test coverage out >> of the tree for even longer. >> >> Thoughts, objections? > > Ok, so from a really, really high-level perspective the design review > from my side in July had one primary goal and a corollary from that: > > - DP validation should exercise the same code paths (where possible) > than what actual customers use in DP mode sets. If we implement > completely separate codepaths (which the first iteration partially > did) then we validate a separate driver and not actually what > customers run. Which is fairly pointless imo. > > - Corollary was that this means we should drive the full kernel stack > and so control the testing from userspace using the same interfaces as > any userspace would use for a modeset. And if that interface has gaps, > or our kernel side DP driver has gaps we'd need to fill them. debugfs > should only serve as the testing sideband between the DP tester (sink > device) and the userspace test program. So the kernel just passes > stuff back&forth like test requests (e.g. "pls set this mode on now > and show test pattern") and return test results (e.g. "I've read the > edid now, here's the checksum"). Of course for some test requests we > can directly answer them from the kernel (e.g. "tell me how many dp > aux failures you've seen"). Examples completely made up ;-) > > So from that perpective adjusting DP link paramters is either a new > tuning bit, which really should be generally available. Or it > indicates a bug/omission in our DP link training or re-training logic. > In either case I don't think it's really pure test-related sideband > date. > > I guess I didn't make this 100% clear in the original design review > what my goal is, or maybe Todd misunderstood things a bit. > > Aside from all this (and now with my community hat on) just adding > code to get a sticker (labelled "passed DP validation") which is > separate code and not used by actual users is imo not useful for > merging upstream. But that's just my own opinion, not sure what's > Dave's stance here or whether there's much precendence either way. > > So short answer: I still think exposing this with properties is the > right approach, presuming we really need it (and it's not just to > paper over a deficient link training logic in the kernel). I also > think it'll be less code since we can simplify the debugfs option > parser. Don't expose DP stuff in properties, I don't want users controlling the parameters of the DP link in any way. I can't see any use in userspace for controlling this stuff. So I'm happier with debugfs, to avoid making an ABI we hate later. Yes I do prefer we make DP validation go via the same paths, but some parts of DP validation require things userspace shouldn't be allowed setup, and for those we should have bypasses, everything else should be done via normal channels. Dave.
On Fri, 14 Nov 2014 07:00:17 +1000 Dave Airlie <airlied@gmail.com> wrote: > > Aside from all this (and now with my community hat on) just adding > > code to get a sticker (labelled "passed DP validation") which is > > separate code and not used by actual users is imo not useful for > > merging upstream. But that's just my own opinion, not sure what's > > Dave's stance here or whether there's much precendence either way. > > > > So short answer: I still think exposing this with properties is the > > right approach, presuming we really need it (and it's not just to > > paper over a deficient link training logic in the kernel). I also > > think it'll be less code since we can simplify the debugfs option > > parser. > > Don't expose DP stuff in properties, I don't want users controlling > the parameters of the DP link in any way. I can't see any use > in userspace for controlling this stuff. So I'm happier with debugfs, > to avoid making an ABI we hate later. > > Yes I do prefer we make DP validation go via the same paths, > but some parts of DP validation require things userspace shouldn't > be allowed setup, and for those we should have bypasses, everything > else should be done via normal channels. Yeah, agreed on re-using code paths. I just don't think it means we have to re-use ioctl or property entry points. The internals of the debugfs file can just as easily call internal functions as the ioctl/property code, so I think we can be covered that way too. I guess we'll have to check out Todd's latest patches when he posts them. Jesse
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index da4036d..2dada18 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,46 @@ static const char *yesno(int v) return v ? "yes" : "no"; } +#define DP_PARAMETER_COUNT 8 +#define MAX_DP_CONFIG_LINE_COUNT 64 + +enum dp_config_param { + DP_CONFIG_PARAM_INVALID = -1, + DP_CONFIG_PARAM_CONNECTOR = 0, + 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 +}; + +struct dp_config { + enum dp_config_param type; + unsigned long value; +}; + +const char *dp_conf_tokens[DP_PARAMETER_COUNT] = { + "Connector", + "Link Rate", + "Lane Count", + "Voltage Swing Level", + "Preemphasis Level", + "Horizontal Resolution", + "Vertical Resolution", + "Bits Per Pixel" +}; +/* Strings for writing out dp_configs */ +#define DP_CONF_STR_CONNECTOR "Connector: %s\n" +#define DP_CONF_STR_LINKRATE "Link Rate: %02x\n" +#define DP_CONF_STR_LANES "Lane Count: %u\n" +#define DP_CONF_STR_VSWING "Voltage Swing Level: %u\n" +#define DP_CONF_STR_PREEMP "Preemphasis Level: %u\n" +#define DP_CONF_STR_HRES "Horizontal Resolution: %d\n" +#define DP_CONF_STR_VRES "Vertical Resolution: %d\n" +#define DP_CONF_STR_BPP "Bits Per Pixel: %u\n" + /* 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 @@ -3505,6 +3546,353 @@ static const struct file_operations i915_display_crc_ctl_fops = { .write = display_crc_ctl_write }; +static void displayport_show_config_info(struct seq_file *m, + struct intel_connector *intel_connector) +{ + struct intel_encoder *intel_encoder = intel_connector->encoder; + struct intel_dp *intel_dp; + struct intel_crtc *icrtc; + int pipe_bpp, hres, vres; + uint8_t vs[4], pe[4]; + struct drm_display_mode *mode; + int i = 0; + + if (intel_encoder) { + 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) >> 3; + } + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) { + if (intel_encoder->new_crtc) { + pipe_bpp = intel_encoder->new_crtc->config.pipe_bpp; + mode = &intel_encoder->new_crtc->config.adjusted_mode; + hres = mode->hdisplay; + vres = mode->vdisplay; + } else if (intel_encoder->base.crtc) { + icrtc = to_intel_crtc(intel_encoder->base.crtc); + pipe_bpp = icrtc->config.pipe_bpp; + mode = &icrtc->config.adjusted_mode; + hres = mode->hdisplay; + vres = mode->vdisplay; + } else { + pipe_bpp = 0; + hres = vres = 0; + } + seq_printf(m, DP_CONF_STR_CONNECTOR, + intel_connector->base.name); + seq_printf(m, DP_CONF_STR_LINKRATE, intel_dp->link_bw); + seq_printf(m, DP_CONF_STR_LANES, intel_dp->lane_count); + seq_printf(m, DP_CONF_STR_VSWING, vs[0]); + seq_printf(m, DP_CONF_STR_PREEMP, pe[0]); + seq_printf(m, DP_CONF_STR_HRES, hres); + seq_printf(m, DP_CONF_STR_VRES, vres); + seq_printf(m, DP_CONF_STR_BPP, pipe_bpp); + } + } +} + +enum dp_config_param displayport_get_config_param_type(char *input_string) +{ + enum dp_config_param param_type = DP_CONFIG_PARAM_INVALID; + int i = 0; + + for (i = 0; i < DP_PARAMETER_COUNT; i++) { + if (strncmp(input_string, dp_conf_tokens[i], + strlen(dp_conf_tokens[i])) == 0) { + param_type = (enum dp_config_param) i; + break; + } + } + return param_type; +} + +int value_for_config_param(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 tokenize_dp_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 displayport_parse_config(char *input_buffer, + ssize_t buffer_size, + struct intel_dp_link_config *dp_conf) +{ + 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 = tokenize_dp_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 = displayport_get_config_param_type(lines[i]); + if (parm_type != DP_CONFIG_PARAM_INVALID) { + status = value_for_config_param(parm_type, + lines[i+1], + &parm_value); + if (status == 0) { + parms[parm_type].type = parm_type; + parms[parm_type].value = parm_value; + } + } + } + + for (i = 1; i < DP_PARAMETER_COUNT; i++) + DRM_DEBUG_DRIVER("%s = %lu\n", + dp_conf_tokens[parms[i].type], + parms[i].value); + + /* Validate any/all incoming parameters */ + strcpy(dp_conf->connector_name, + (char *)parms[DP_CONFIG_PARAM_CONNECTOR].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) { + dp_conf->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) { + dp_conf->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) { + dp_conf->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) { + dp_conf->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) { + dp_conf->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) { + dp_conf->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) { + dp_conf->vres = + parms[DP_CONFIG_PARAM_VRES].value; + } else + return -EINVAL; + + return status; +} + +static int displayport_config_ctl_show(struct seq_file *m, void *data) +{ + struct drm_device *dev = m->private; + struct drm_connector *connector; + struct intel_encoder *intel_encoder; + struct intel_connector *intel_connector; + + if (!dev) + return -ENODEV; + + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + intel_connector = to_intel_connector(connector); + intel_encoder = intel_connector->encoder; + if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) { + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT || + intel_encoder->type == INTEL_OUTPUT_UNKNOWN) { + if (connector->status == connector_status_connected) { + displayport_show_config_info(m, intel_connector); + } else { + seq_printf(m, DP_CONF_STR_CONNECTOR, + intel_connector->base.name); + seq_printf(m, "disconnected\n"); + } + } + } + } + + return 0; +} + +static int displayport_config_ctl_open(struct inode *inode, + struct file *file) +{ + struct drm_device *dev = inode->i_private; + + return single_open(file, displayport_config_ctl_show, dev); +} + +static ssize_t 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 intel_encoder *intel_encoder; + struct intel_connector *intel_connector; + struct intel_dp *intel_dp; + struct intel_dp_link_config dp_conf; + + m = file->private_data; + if (!m) { + status = -ENODEV; + return status; + } + dev = m->private; + + if (!dev) { + status = -ENODEV; + return status; + } + + 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'; + memset(&dp_conf, 0, sizeof(struct intel_dp_link_config)); + DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len); + status = displayport_parse_config(input_buffer, len, &dp_conf); + + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + intel_connector = to_intel_connector(connector); + intel_encoder = intel_connector->encoder; + if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) { + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT || + intel_encoder->type == INTEL_OUTPUT_UNKNOWN) { + intel_dp = enc_to_intel_dp(&intel_encoder->base); + memcpy(&intel_dp->compliance_config, + &dp_conf, + sizeof(struct intel_dp_link_config)); + } + } + } + + +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 = displayport_config_ctl_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, + .write = displayport_config_ctl_write +}; + static void wm_latency_show(struct seq_file *m, const uint16_t wm[5]) { struct drm_device *dev = m->private; @@ -4208,6 +4596,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_dp.c b/drivers/gpu/drm/i915/intel_dp.c index a7acc61..b3ddd15 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3968,7 +3968,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector) static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp) { - uint8_t test_result = DP_TEST_NAK; + uint8_t test_result = DP_TEST_ACK; return test_result; } @@ -4013,21 +4013,25 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp) DRM_DEBUG_KMS("Executing LINK_TRAINING request\n"); intel_dp->compliance_test_data = DP_TEST_LINK_TRAINING; response = intel_dp_autotest_link_training(intel_dp); + status = intel_dp_signal_userspace(intel_dp); break; case DP_TEST_LINK_VIDEO_PATTERN: DRM_DEBUG_KMS("Executing TEST_PATTERN request\n"); intel_dp->compliance_test_data = DP_TEST_LINK_VIDEO_PATTERN; response = intel_dp_autotest_video_pattern(intel_dp); + status = intel_dp_signal_userspace(intel_dp); break; case DP_TEST_LINK_EDID_READ: DRM_DEBUG_KMS("Executing EDID request\n"); intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ; response = intel_dp_autotest_edid(intel_dp); + status = intel_dp_signal_userspace(intel_dp); break; case DP_TEST_LINK_PHY_TEST_PATTERN: DRM_DEBUG_KMS("Executing PHY_PATTERN request\n"); intel_dp->compliance_test_data = DP_TEST_LINK_PHY_TEST_PATTERN; response = intel_dp_autotest_phy_pattern(intel_dp); + status = intel_dp_signal_userspace(intel_dp); break; /* FAUX is optional in DP 1.2*/ case DP_TEST_LINK_FAUX_PATTERN: @@ -4038,10 +4042,9 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp) DRM_DEBUG_KMS("Unhandled test request\n"); break; } - if (status != 0) { - response = DP_TEST_NAK; - DRM_DEBUG_KMS("Error %d processing test request\n", status); - } + if (status != 0) + DRM_DEBUG_KMS("Status code %d processing test request\n", + status); status = drm_dp_dpcd_write(&intel_dp->aux, DP_TEST_RESPONSE, &response, 1);
Adds an interface that allows for Displayport configuration information to be accessed through debugfs. The information paramters provided here are as follows: Link rate Lane count Bits per pixel Voltage swing level Preemphasis level Display mode These parameters are intended to be used by userspace applications that need access to the link configuration of any active Displayport connection. Additionally, applications can make adjustments to these parameters through this interface to allow userspace application to have fine-grained control over link training paramters. Signed-off-by: Todd Previte <tprevite@gmail.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 389 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_dp.c | 13 +- 2 files changed, 397 insertions(+), 5 deletions(-)