[Intel-gfx] [PATCH 07/17] drm/i915: Add and implement the debugfs 'show' functions for Displayport compliance
Todd Previte
tprevite at gmail.com
Wed Feb 18 08:41:28 PST 2015
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 at 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 at 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list