Re: [PATCH 07/17] drm/i915: Add and implement the debugfs 'show' functions for Displayport compliance

[Todd Previte <tprevite@gmail.com>]

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
>

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