Re: [PATCH 6/6] drm/i915: Add debugfs entry for DRRS

[Rodrigo Vivi <rodrigo.vivi@gmail.com>]

On Fri, Feb 13, 2015 at 2:03 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
> From: Vandana Kannan <vandana.kannan@intel.com>
>
> Adding a debugfs entry to determine if DRRS is supported or not
>
> V2: [By Ram]: Following details about the active crtc will be filled
>         in seq-file of the debugfs
>         1. Encoder output type
>         2. DRRS Support on this CRTC
>         3. DRRS current state
>         4. Current Vrefresh
> Format is as follows:
> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_HIGH_RR, Vrefresh: 60
> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_LOW_RR, Vrefresh: 40
> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
>
> V3: [By Ram]: Readability is improved.
>         Another error case is covered [Daniel]
>
> V4: [By Ram]: Current status of the Idleness DRRS along with
>         the Front buffer bits are added to the debugfs. [Rodrigo]
>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |   99 +++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 164fa82..e08d63f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2869,6 +2869,104 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
>         return 0;
>  }
>
> +static void drrs_status_per_crtc(struct seq_file *m,
> +               struct drm_device *dev, struct intel_crtc *intel_crtc)
> +{
> +       struct intel_encoder *intel_encoder;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct i915_drrs *drrs = &dev_priv->drrs;
> +       int vrefresh = 0;
> +
> +       for_each_encoder_on_crtc(dev, &intel_crtc->base, intel_encoder) {
> +               /* Encoder connected on this CRTC */

Do you really need this info here?

> +               switch (intel_encoder->type) {
> +               case INTEL_OUTPUT_EDP:
> +                       seq_puts(m, "Output: eDP, ");
> +                       break;
> +               case INTEL_OUTPUT_DSI:
> +                       seq_puts(m, "Output: DSI, ");
> +                       break;
> +               case INTEL_OUTPUT_HDMI:
> +                       seq_puts(m, "Output: HDMI, ");
> +                       break;
> +               case INTEL_OUTPUT_DISPLAYPORT:
> +                       seq_puts(m, "Output: DP, ");
> +                       break;
> +               default:
> +                       seq_printf(m, "Output: Others (id=%d), ",
> +                                               intel_encoder->type);
> +               }
> +       }
> +
> +       if (intel_crtc->config->has_drrs) {
> +               struct intel_panel *panel;
> +
> +               panel = &drrs->dp->attached_connector->panel;
> +               /* DRRS Supported */
> +               seq_puts(m, "DRRS Supported: Yes (Seamless), ");

isn't "Yes" enough? Remind that you might want to parse in the future.

> +               seq_printf(m, "busy_frontbuffer_bits: 0x%X,\n\t",
> +                                       drrs->busy_frontbuffer_bits);
> +
> +               if (drrs->busy_frontbuffer_bits) {
> +                       seq_puts(m, "Front buffer: busy, ");
> +                       seq_puts(m, "Idleness Timer: Suspended, ");
> +               } else {
> +                       seq_puts(m, "Front buffer: Idle, ");
> +                       if (drrs->refresh_rate_type == DRRS_HIGH_RR)
> +                               seq_puts(m, "Idleness Timer: Ticking, ");
> +                       else
> +                               seq_puts(m, "Idleness Timer: Suspended, ");
> +               }
> +
> +               if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
> +                       seq_puts(m, "DRRS_State: DRRS_HIGH_RR, ");
> +                       vrefresh = panel->fixed_mode->vrefresh;
> +               } else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
> +                       seq_puts(m, "DRRS_State: DRRS_LOW_RR, ");
> +                       vrefresh = panel->downclock_mode->vrefresh;
> +               } else {
> +                       seq_printf(m, "DRRS_State: Unknown(%d), ",
> +                                               drrs->refresh_rate_type);
> +               }

I wonder what it is printed when DRRS is supported but is disabled?

Also why not print some info like enabled/disabled?

> +               seq_printf(m, "Vrefresh: %d", vrefresh);
> +
> +       } else {
> +               /* DRRS not supported. Print the VBT parameter*/

Why? Should be better a dmesg when enabling DRRS and print why not supported?

Is VBT only reason for not being supported? Is is information useless
when drrs is supported?

> +               seq_puts(m, "DRRS Supported : No, ");
> +               if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
> +                       seq_puts(m, "VBT DRRS_type: Static");
> +               else if (dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT)
> +                       seq_puts(m, "VBT DRRS_type: Seamless");
> +               else if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED)
> +                       seq_puts(m, "VBT DRRS_type: None");
> +               else
> +                       seq_puts(m, "VBT DRRS_type: Unrecognized Value");
> +       }
> +       seq_puts(m, "\n");
> +}
> +
> +static int i915_drrs_status(struct seq_file *m, void *unused)
> +{
> +       struct drm_info_node *node = m->private;
> +       struct drm_device *dev = node->minor->dev;
> +       struct intel_crtc *intel_crtc;
> +       int active_crtc_cnt = 0;
> +
> +       for_each_intel_crtc(dev, intel_crtc) {
> +               if (intel_crtc->active) {
> +                       active_crtc_cnt++;

isn't a bool enough?
why do you need a counter?

> +                       seq_printf(m, "CRTC %d:  ", active_crtc_cnt);
> +
> +                       drrs_status_per_crtc(m, dev, intel_crtc);

Since you are doing this for all active crtcs, have you considered
indenting all other information per crtc to be more clean when having
more than one active crtc?

> +               }
> +       }
> +
> +       if (!active_crtc_cnt)
> +               seq_puts(m, "No active crtc found\n");
> +
> +       return 0;
> +}
> +
>  struct pipe_crc_info {
>         const char *name;
>         struct drm_device *dev;
> @@ -4483,6 +4581,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>         {"i915_dp_mst_info", i915_dp_mst_info, 0},
>         {"i915_wa_registers", i915_wa_registers, 0},
>         {"i915_ddb_info", i915_ddb_info, 0},
> +       {"i915_drrs_status", i915_drrs_status, 0},
>  };
>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx