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

[Paulo Zanoni <przanoni@gmail.com>]

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



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