| Message ID | 1418255597-4716-8-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>: > This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs > interface for Displayport debug and compliance testing". This patch implements > the 'show' functions for the debugfs interface for Displayport compliance > testing. > > Signed-off-by: Todd Previte <tprevite@gmail.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 85 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 85 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index ce091c1..184797d 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -3694,6 +3694,56 @@ 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; Please don't use "icrtc": the current trend is to call intel_crtc as just "crtc". In the past, "struct drm_crtc" would be "crtc" and "struct intel_crtc" would be "intel_crtc", but I don't remember seeing icrtc. > + struct intel_crtc_config *crtc_config; > + char *conn_name = intel_connector->base.name; > + int pipe_bpp, hres, vres; > + uint8_t vs[4], pe[4]; > + struct drm_display_mode *mode; > + int i = 0; > + > + if (intel_encoder) { Bikeshedding: since the whole function implementation is inside the "if" statement, we usually prefer to just "if (!intel_encoder) return;", because that saves one indentation level. > + 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; Please use DP_TRAIN_PRE_EMPHASIS_SHIFT instead of the hardcoded "3". > + } > + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) { > + if (intel_encoder->new_crtc) { > + crtc_config = &intel_encoder->new_crtc->config; > + pipe_bpp = crtc_config->pipe_bpp; > + mode = &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; > + } Why do you have this new_crtc vs current_crtc vs no crtc distinction here? Is it really necessary? Shouldn't the "DP testing debugfs protocol" establish when exactly the information should be queried, and then give some errors in case information is being requested at the wrong time? > + seq_printf(m, DP_CONF_STR_CONNECTOR, conn_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; > @@ -3742,6 +3792,41 @@ int value_for_config_param(enum dp_config_param param, > return status; > } > > +static int displayport_config_ctl_show(struct seq_file *m, void *data) > +{ > + struct drm_device *dev = m->private; > + struct drm_connector *drm_connector; > + struct intel_encoder *encoder; > + struct intel_connector *connector; The standard is to make "struct drm_connector *connector" and "struct intel_connector *intel_connector", or "struct intel_connector *connector" and then always use &connector->base for when drm_connector is needed. Like the encoders and crtcs. > + > + if (!dev) > + return -ENODEV; > + > + list_for_each_entry(drm_connector, > + &dev->mode_config.connector_list, > + head) { > + connector = to_intel_connector(drm_connector); > + encoder = connector->encoder; > + if (drm_connector->connector_type == > + DRM_MODE_CONNECTOR_DisplayPort) { > + if (encoder->type == INTEL_OUTPUT_DISPLAYPORT || > + encoder->type == INTEL_OUTPUT_UNKNOWN) { If the connector is DP, I think you don't need to check the encoder type (because you check for "connected" below). Also, you could have used "&&" instead of nesting if statements, which would save one indentation level. > + if (drm_connector->status == > + connector_status_connected) { > + displayport_show_config_info(m, > + connector); > + } else { > + seq_printf(m, DP_CONF_STR_CONNECTOR, > + encoder->base.name); > + seq_puts(m, "disconnected\n"); > + } > + } > + } > + } > + > + return 0; > +} > + > static int displayport_config_ctl_open(struct inode *inode, > struct file *file) > { > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Dec 16, 2014 at 05:00:38PM -0200, Paulo Zanoni wrote: > 2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>: > > + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) { > > + if (intel_encoder->new_crtc) { > > + crtc_config = &intel_encoder->new_crtc->config; > > + pipe_bpp = crtc_config->pipe_bpp; > > + mode = &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; > > + } > > Why do you have this new_crtc vs current_crtc vs no crtc distinction > here? Is it really necessary? Shouldn't the "DP testing debugfs > protocol" establish when exactly the information should be queried, > and then give some errors in case information is being requested at > the wrong time? Presuming adequate locking exists (haven't checked tbh) new_* pointers always match the equivalent base pointers. new_ pointers/structures are exclusively for modeset code (and specifically state computation). I guess the new_crtc case needs to be dropped here. -Daniel
Responses inline below. Any changes have been rolled into the monster patch. -T On 12/16/14 12:00 PM, Paulo Zanoni wrote: > 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>: >> This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs >> interface for Displayport debug and compliance testing". This patch implements >> the 'show' functions for the debugfs interface for Displayport compliance >> testing. >> >> Signed-off-by: Todd Previte<tprevite@gmail.com> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 85 +++++++++++++++++++++++++++++++++++++ >> 1 file changed, 85 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >> index ce091c1..184797d 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -3694,6 +3694,56 @@ 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; > Please don't use "icrtc": the current trend is to call intel_crtc as > just "crtc". In the past, "struct drm_crtc" would be "crtc" and > "struct intel_crtc" would be "intel_crtc", but I don't remember seeing > icrtc. Just an attempt to get the code under the 80 char line limit. With the other changes below it's unnecessary anyways. It's fixed in V3. >> + struct intel_crtc_config *crtc_config; >> + char *conn_name = intel_connector->base.name; >> + int pipe_bpp, hres, vres; >> + uint8_t vs[4], pe[4]; >> + struct drm_display_mode *mode; >> + int i = 0; >> + >> + if (intel_encoder) { > Bikeshedding: since the whole function implementation is inside the > "if" statement, we usually prefer to just "if (!intel_encoder) > return;", because that saves one indentation level. Changed for V3. Also changed the intel_encode->type check to the same thing, again saving a level of indent. > >> + 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; > Please use DP_TRAIN_PRE_EMPHASIS_SHIFT instead of the hardcoded "3". Fixed. >> + } >> + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) { >> + if (intel_encoder->new_crtc) { >> + crtc_config = &intel_encoder->new_crtc->config; >> + pipe_bpp = crtc_config->pipe_bpp; >> + mode = &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; >> + } > Why do you have this new_crtc vs current_crtc vs no crtc distinction > here? Is it really necessary? Shouldn't the "DP testing debugfs > protocol" establish when exactly the information should be queried, > and then give some errors in case information is being requested at > the wrong time? That code is a hold-over from when I was making sure I wasn't missing test events because of invalid data structures. Part of the problem is that link training is tied to mode sets, which is where this code originally came from as I recall. In any case, I've removed the new_crtc case (in light of Daniel's reply) so it shouldn't be an issue. >> + seq_printf(m, DP_CONF_STR_CONNECTOR, conn_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; >> @@ -3742,6 +3792,41 @@ int value_for_config_param(enum dp_config_param param, >> return status; >> } >> >> +static int displayport_config_ctl_show(struct seq_file *m, void *data) >> +{ >> + struct drm_device *dev = m->private; >> + struct drm_connector *drm_connector; >> + struct intel_encoder *encoder; >> + struct intel_connector *connector; > The standard is to make "struct drm_connector *connector" and "struct > intel_connector *intel_connector", or "struct intel_connector > *connector" and then always use &connector->base for when > drm_connector is needed. Like the encoders and crtcs. Fair enough. Fixed in V3. >> + >> + if (!dev) >> + return -ENODEV; >> + >> + list_for_each_entry(drm_connector, >> + &dev->mode_config.connector_list, >> + head) { >> + connector = to_intel_connector(drm_connector); >> + encoder = connector->encoder; >> + if (drm_connector->connector_type == >> + DRM_MODE_CONNECTOR_DisplayPort) { >> + if (encoder->type == INTEL_OUTPUT_DISPLAYPORT || >> + encoder->type == INTEL_OUTPUT_UNKNOWN) { > If the connector is DP, I think you don't need to check the encoder > type (because you check for "connected" below). Also, you could have > used "&&" instead of nesting if statements, which would save one > indentation level. I changed the code around to save some indentation levels and address this particular issue. There was an issue I ran across where just using the DRM connector wasn't enough, but I haven't been able to reproduce that problem with the current code. So this has been removed. >> + if (drm_connector->status == >> + connector_status_connected) { >> + displayport_show_config_info(m, >> + connector); >> + } else { >> + seq_printf(m, DP_CONF_STR_CONNECTOR, >> + encoder->base.name); >> + seq_puts(m, "disconnected\n"); >> + } >> + } >> + } >> + } >> + >> + return 0; >> +} >> + >> static int displayport_config_ctl_open(struct inode *inode, >> struct file *file) >> { >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On 12/17/14 1:12 PM, Daniel Vetter wrote: > On Tue, Dec 16, 2014 at 05:00:38PM -0200, Paulo Zanoni wrote: >> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>: >>> + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) { >>> + if (intel_encoder->new_crtc) { >>> + crtc_config = &intel_encoder->new_crtc->config; >>> + pipe_bpp = crtc_config->pipe_bpp; >>> + mode = &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; >>> + } >> Why do you have this new_crtc vs current_crtc vs no crtc distinction >> here? Is it really necessary? Shouldn't the "DP testing debugfs >> protocol" establish when exactly the information should be queried, >> and then give some errors in case information is being requested at >> the wrong time? > Presuming adequate locking exists (haven't checked tbh) new_* pointers always > match the equivalent base pointers. new_ pointers/structures are > exclusively for modeset code (and specifically state computation). I guess > the new_crtc case needs to be dropped here. > -Daniel I tested this again with the new_crtc case removed and it appears to work correctly. So the new_crtc case has been removed. The changes have been integrated into the monster patch. -T
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index ce091c1..184797d 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -3694,6 +3694,56 @@ 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; + struct intel_crtc_config *crtc_config; + char *conn_name = intel_connector->base.name; + 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) { + crtc_config = &intel_encoder->new_crtc->config; + pipe_bpp = crtc_config->pipe_bpp; + mode = &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, conn_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; @@ -3742,6 +3792,41 @@ int value_for_config_param(enum dp_config_param param, return status; } +static int displayport_config_ctl_show(struct seq_file *m, void *data) +{ + struct drm_device *dev = m->private; + struct drm_connector *drm_connector; + struct intel_encoder *encoder; + struct intel_connector *connector; + + if (!dev) + return -ENODEV; + + list_for_each_entry(drm_connector, + &dev->mode_config.connector_list, + head) { + connector = to_intel_connector(drm_connector); + encoder = connector->encoder; + if (drm_connector->connector_type == + DRM_MODE_CONNECTOR_DisplayPort) { + if (encoder->type == INTEL_OUTPUT_DISPLAYPORT || + encoder->type == INTEL_OUTPUT_UNKNOWN) { + if (drm_connector->status == + connector_status_connected) { + displayport_show_config_info(m, + connector); + } else { + seq_printf(m, DP_CONF_STR_CONNECTOR, + encoder->base.name); + seq_puts(m, "disconnected\n"); + } + } + } + } + + return 0; +} + static int displayport_config_ctl_open(struct inode *inode, struct file *file) {
This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing". This patch implements the 'show' functions for the debugfs interface for Displayport compliance testing. Signed-off-by: Todd Previte <tprevite@gmail.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 85 +++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+)