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

[Ramalingam C <ramalingam.c@intel.com>]

Hi,

On Friday 20 February 2015 12:15 AM, Rodrigo Vivi wrote:
> 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?
Better to have the encoder details
>
>> +               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.
Agreed. Yes alone will be simple.
>
>> +               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?
It will be printing as "Idleness Timer: Suspended". I will rename this 
to remove this confusions :)
>
> Also why not print some info like enabled/disabled?
Sure we will do it.
>
>> +               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?
VBT details and also the lower refresh rate from EDID both are 
controlling factors for DRRS.
If DRRS is not supported only then VBT details are added so that modes 
can be verified along with. Its understood that VBT supports seamless 
type 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?
Actually we are disabling the DRRS in case of clone mode on Android.
So I added a counter for active CRTCs. So that we can inform about 
cloned mode.
But here it doesn't make any sense.
>
>> +                       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?
Sure. we can make more presentable.
>
>> +               }
>> +       }
>> +
>> +       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
>
>
I will submit a patch for this by monday.

-- 
Ram

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