| Message ID | 20181204230032.6352-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks | expand |
On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote: > The old debugfs fields was not following a naming partern and it was > a bit confusing. > > So it went from: > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status > Sink_Support: yes > PSR mode: PSR1 > Enabled: yes > Busy frontbuffer bits: 0x000 > Main link in standby mode: no > HW Enabled & Active bit: yes > Source PSR status: 0x24050006 [SRDONACK] > > To: > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status > Sink support: yes [0x00000003] > Status: PSR1 enabled > Source PSR ctl: enabled [0x81f00e26] > Source PSR status: SRDENT [0x40040006] > Busy frontbuffer bits: 0x00000000 > > The 'Main link in standby mode' was removed as it is not useful but > if needed by someone the information is still in the register value > of 'Source PSR ctl' inside of the brackets, PSR mode and Enabled was > squashed into Status, some renames and reorders and we have this > cleaner version. This will also make easy to parse debugfs for IGT > tests. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++---------- > ---- > 1 file changed, 49 insertions(+), 47 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 38dcee1ca062..86303ba02666 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2665,7 +2665,8 @@ DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status); > static void > psr_source_status(struct drm_i915_private *dev_priv, struct seq_file > *m) > { > - u32 val, psr_status; > + u32 val, status_val; > + const char *status = "unknown"; > > if (dev_priv->psr.psr2_enabled) { > static const char * const live_status[] = { > @@ -2681,14 +2682,11 @@ psr_source_status(struct drm_i915_private > *dev_priv, struct seq_file *m) > "BUF_ON", > "TG_ON" > }; > - psr_status = I915_READ(EDP_PSR2_STATUS); > - val = (psr_status & EDP_PSR2_STATUS_STATE_MASK) >> > - EDP_PSR2_STATUS_STATE_SHIFT; > - if (val < ARRAY_SIZE(live_status)) { > - seq_printf(m, "Source PSR status: 0x%x [%s]\n", > - psr_status, live_status[val]); > - return; > - } > + val = I915_READ(EDP_PSR2_STATUS); > + status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >> > + EDP_PSR2_STATUS_STATE_SHIFT; > + if (status_val < ARRAY_SIZE(live_status)) > + status = live_status[status_val]; > } else { > static const char * const live_status[] = { > "IDLE", > @@ -2700,74 +2698,78 @@ psr_source_status(struct drm_i915_private > *dev_priv, struct seq_file *m) > "SRDOFFACK", > "SRDENT_ON", > }; > - psr_status = I915_READ(EDP_PSR_STATUS); > - val = (psr_status & EDP_PSR_STATUS_STATE_MASK) >> > - EDP_PSR_STATUS_STATE_SHIFT; > - if (val < ARRAY_SIZE(live_status)) { > - seq_printf(m, "Source PSR status: 0x%x [%s]\n", > - psr_status, live_status[val]); > - return; > - } > + val = I915_READ(EDP_PSR_STATUS); > + status_val = (val & EDP_PSR_STATUS_STATE_MASK) >> > + EDP_PSR_STATUS_STATE_SHIFT; > + if (status_val < ARRAY_SIZE(live_status)) > + status = live_status[status_val]; > } > > - seq_printf(m, "Source PSR status: 0x%x [%s]\n", psr_status, > "unknown"); > + seq_printf(m, "Source PSR status: %s [0x%08x]\n", status, val); > } > > static int i915_edp_psr_status(struct seq_file *m, void *data) > { > struct drm_i915_private *dev_priv = node_to_i915(m->private); > - u32 psrperf = 0; > - bool enabled = false; > - bool sink_support; > + struct i915_psr *psr = &dev_priv->psr; > + const char *status; > + bool enabled; > + u32 val; > > if (!HAS_PSR(dev_priv)) > return -ENODEV; > > - sink_support = dev_priv->psr.sink_support; > - seq_printf(m, "Sink_Support: %s\n", yesno(sink_support)); > - if (!sink_support) > + seq_printf(m, "Sink support: %s", yesno(psr->sink_support)); > + if (!psr->sink_support) { > + seq_puts(m, "\n"); > return 0; > + } > > intel_runtime_pm_get(dev_priv); > + mutex_lock(&psr->lock); > > - mutex_lock(&dev_priv->psr.lock); > - seq_printf(m, "PSR mode: %s\n", > - dev_priv->psr.psr2_enabled ? "PSR2" : "PSR1"); > - seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled)); > - seq_printf(m, "Busy frontbuffer bits: 0x%03x\n", > - dev_priv->psr.busy_frontbuffer_bits); > + seq_printf(m, " [0x%08x]\n", psr->dp->psr_dpcd[0]); This can be moved closer to where "Sink support" is printed. Also, the extra zeroes that get printed due to "%08x" look odd, please consider changing it to "%02x". > > - if (dev_priv->psr.psr2_enabled) > - enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE; > + if (psr->enabled) > + status = psr->psr2_enabled ? "PSR2 enabled" : "PSR1 > enabled"; > else > - enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE; > + status = "disabled"; > + seq_printf(m, "Status: %s\n", status); Can we just use the ctl field and get rid of this? This status and and the one below can be confusing to those not familiar with the code base. We should be able to parse all the information we need from two lines Source PSR ctl: [PSR1,PSR2] enabled <reg value> Source PSR status: <state> <reg value> > > - seq_printf(m, "Main link in standby mode: %s\n", > - yesno(dev_priv->psr.link_standby)); > + if (!psr->enabled) > + goto unlock; > > - seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled)); > + if (psr->psr2_enabled) { > + val = I915_READ(EDP_PSR2_CTL); > + enabled = val & EDP_PSR2_ENABLE; > + } else { > + val = I915_READ(EDP_PSR_CTL); > + enabled = val & EDP_PSR_ENABLE; > + } > + seq_printf(m, "Source PSR ctl: %s [0x%08x]\n", > + enableddisabled(enabled), val); > + psr_source_status(dev_priv, m); > + seq_printf(m, "Busy frontbuffer bits: 0x%08x\n", > + psr->busy_frontbuffer_bits); > > /* > * SKL+ Perf counter is reset to 0 everytime DC state is > entered > */ > if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { > - psrperf = I915_READ(EDP_PSR_PERF_CNT) & > - EDP_PSR_PERF_CNT_MASK; > - > - seq_printf(m, "Performance_Counter: %u\n", psrperf); > + val = I915_READ(EDP_PSR_PERF_CNT) & > EDP_PSR_PERF_CNT_MASK; > + seq_printf(m, "Performance counter: %u\n", val); > } > > - psr_source_status(dev_priv, m); > - mutex_unlock(&dev_priv->psr.lock); > - > - if (READ_ONCE(dev_priv->psr.debug) & I915_PSR_DEBUG_IRQ) { > + if (psr->debug & I915_PSR_DEBUG_IRQ) { > seq_printf(m, "Last attempted entry at: %lld\n", > - dev_priv->psr.last_entry_attempt); > - seq_printf(m, "Last exit at: %lld\n", > - dev_priv->psr.last_exit); > + psr->last_entry_attempt); > + seq_printf(m, "Last exit at: %lld\n", psr->last_exit); > } > > +unlock: > + mutex_unlock(&psr->lock); > intel_runtime_pm_put(dev_priv); > + > return 0; > } >
On Mon, 2018-12-10 at 22:51 -0800, Dhinakaran Pandiyan wrote: > On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote: > > The old debugfs fields was not following a naming partern and it > > was > > a bit confusing. > > > > So it went from: > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status > > Sink_Support: yes > > PSR mode: PSR1 > > Enabled: yes > > Busy frontbuffer bits: 0x000 > > Main link in standby mode: no > > HW Enabled & Active bit: yes > > Source PSR status: 0x24050006 [SRDONACK] > > > > To: > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status > > Sink support: yes [0x00000003] > > Status: PSR1 enabled > > Source PSR ctl: enabled [0x81f00e26] > > Source PSR status: SRDENT [0x40040006] > > Busy frontbuffer bits: 0x00000000 > > > > The 'Main link in standby mode' was removed as it is not useful but > > if needed by someone the information is still in the register value > > of 'Source PSR ctl' inside of the brackets, PSR mode and Enabled > > was > > squashed into Status, some renames and reorders and we have this > > cleaner version. This will also make easy to parse debugfs for IGT > > tests. > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++---------- > > ---- > > 1 file changed, 49 insertions(+), 47 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index 38dcee1ca062..86303ba02666 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -2665,7 +2665,8 @@ DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status); > > static void > > psr_source_status(struct drm_i915_private *dev_priv, struct > > seq_file > > *m) > > { > > - u32 val, psr_status; > > + u32 val, status_val; > > + const char *status = "unknown"; > > > > if (dev_priv->psr.psr2_enabled) { > > static const char * const live_status[] = { > > @@ -2681,14 +2682,11 @@ psr_source_status(struct drm_i915_private > > *dev_priv, struct seq_file *m) > > "BUF_ON", > > "TG_ON" > > }; > > - psr_status = I915_READ(EDP_PSR2_STATUS); > > - val = (psr_status & EDP_PSR2_STATUS_STATE_MASK) >> > > - EDP_PSR2_STATUS_STATE_SHIFT; > > - if (val < ARRAY_SIZE(live_status)) { > > - seq_printf(m, "Source PSR status: 0x%x [%s]\n", > > - psr_status, live_status[val]); > > - return; > > - } > > + val = I915_READ(EDP_PSR2_STATUS); > > + status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >> > > + EDP_PSR2_STATUS_STATE_SHIFT; > > + if (status_val < ARRAY_SIZE(live_status)) > > + status = live_status[status_val]; > > } else { > > static const char * const live_status[] = { > > "IDLE", > > @@ -2700,74 +2698,78 @@ psr_source_status(struct drm_i915_private > > *dev_priv, struct seq_file *m) > > "SRDOFFACK", > > "SRDENT_ON", > > }; > > - psr_status = I915_READ(EDP_PSR_STATUS); > > - val = (psr_status & EDP_PSR_STATUS_STATE_MASK) >> > > - EDP_PSR_STATUS_STATE_SHIFT; > > - if (val < ARRAY_SIZE(live_status)) { > > - seq_printf(m, "Source PSR status: 0x%x [%s]\n", > > - psr_status, live_status[val]); > > - return; > > - } > > + val = I915_READ(EDP_PSR_STATUS); > > + status_val = (val & EDP_PSR_STATUS_STATE_MASK) >> > > + EDP_PSR_STATUS_STATE_SHIFT; > > + if (status_val < ARRAY_SIZE(live_status)) > > + status = live_status[status_val]; > > } > > > > - seq_printf(m, "Source PSR status: 0x%x [%s]\n", psr_status, > > "unknown"); > > + seq_printf(m, "Source PSR status: %s [0x%08x]\n", status, val); > > } > > > > static int i915_edp_psr_status(struct seq_file *m, void *data) > > { > > struct drm_i915_private *dev_priv = node_to_i915(m->private); > > - u32 psrperf = 0; > > - bool enabled = false; > > - bool sink_support; > > + struct i915_psr *psr = &dev_priv->psr; > > + const char *status; > > + bool enabled; > > + u32 val; > > > > if (!HAS_PSR(dev_priv)) > > return -ENODEV; > > > > - sink_support = dev_priv->psr.sink_support; > > - seq_printf(m, "Sink_Support: %s\n", yesno(sink_support)); > > - if (!sink_support) > > + seq_printf(m, "Sink support: %s", yesno(psr->sink_support)); > > + if (!psr->sink_support) { > > + seq_puts(m, "\n"); > > return 0; > > + } > > > > intel_runtime_pm_get(dev_priv); > > + mutex_lock(&psr->lock); > > > > - mutex_lock(&dev_priv->psr.lock); > > - seq_printf(m, "PSR mode: %s\n", > > - dev_priv->psr.psr2_enabled ? "PSR2" : "PSR1"); > > - seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled)); > > - seq_printf(m, "Busy frontbuffer bits: 0x%03x\n", > > - dev_priv->psr.busy_frontbuffer_bits); > > + seq_printf(m, " [0x%08x]\n", psr->dp->psr_dpcd[0]); > This can be moved closer to where "Sink support" is printed. > Also, the > extra zeroes that get printed due to "%08x" look odd, please consider > changing it to "%02x". Done > > > > > - if (dev_priv->psr.psr2_enabled) > > - enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE; > > + if (psr->enabled) > > + status = psr->psr2_enabled ? "PSR2 enabled" : "PSR1 > > enabled"; > > else > > - enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE; > > + status = "disabled"; > > + seq_printf(m, "Status: %s\n", status); > Can we just use the ctl field and get rid of this? This status and > and > the one below can be confusing to those not familiar with the code > base. > > We should be able to parse all the information we need from two lines > Source PSR ctl: [PSR1,PSR2] enabled <reg value> > Source PSR status: <state> <reg value> I'm fine in having just one but when there is frontbuffer modifications Status is kept as enabled and Source PSR ctl and Source PSR status are set to idle states, we know that but those not familiar with the code could think it is disabled but it usualy happen so fast that I guess it is not a problem. And when PSR is disabled we print something else other than Sink support? > > > > > > > - seq_printf(m, "Main link in standby mode: %s\n", > > - yesno(dev_priv->psr.link_standby)); > > + if (!psr->enabled) > > + goto unlock; > > > > - seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled)); > > + if (psr->psr2_enabled) { > > + val = I915_READ(EDP_PSR2_CTL); > > + enabled = val & EDP_PSR2_ENABLE; > > + } else { > > + val = I915_READ(EDP_PSR_CTL); > > + enabled = val & EDP_PSR_ENABLE; > > + } > > + seq_printf(m, "Source PSR ctl: %s [0x%08x]\n", > > + enableddisabled(enabled), val); > > + psr_source_status(dev_priv, m); > > + seq_printf(m, "Busy frontbuffer bits: 0x%08x\n", > > + psr->busy_frontbuffer_bits); > > > > /* > > * SKL+ Perf counter is reset to 0 everytime DC state is > > entered > > */ > > if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { > > - psrperf = I915_READ(EDP_PSR_PERF_CNT) & > > - EDP_PSR_PERF_CNT_MASK; > > - > > - seq_printf(m, "Performance_Counter: %u\n", psrperf); > > + val = I915_READ(EDP_PSR_PERF_CNT) & > > EDP_PSR_PERF_CNT_MASK; > > + seq_printf(m, "Performance counter: %u\n", val); > > } > > > > - psr_source_status(dev_priv, m); > > - mutex_unlock(&dev_priv->psr.lock); > > - > > - if (READ_ONCE(dev_priv->psr.debug) & I915_PSR_DEBUG_IRQ) { > > + if (psr->debug & I915_PSR_DEBUG_IRQ) { > > seq_printf(m, "Last attempted entry at: %lld\n", > > - dev_priv->psr.last_entry_attempt); > > - seq_printf(m, "Last exit at: %lld\n", > > - dev_priv->psr.last_exit); > > + psr->last_entry_attempt); > > + seq_printf(m, "Last exit at: %lld\n", psr->last_exit); > > } > > > > +unlock: > > + mutex_unlock(&psr->lock); > > intel_runtime_pm_put(dev_priv); > > + > > return 0; > > } > >
On Tue, 2018-12-11 at 04:44 -0800, Souza, Jose wrote: > On Mon, 2018-12-10 at 22:51 -0800, Dhinakaran Pandiyan wrote: > > On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote: > > > The old debugfs fields was not following a naming partern and it > > > was > > > a bit confusing. > > > > > > So it went from: > > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status > > > Sink_Support: yes > > > PSR mode: PSR1 > > > Enabled: yes > > > Busy frontbuffer bits: 0x000 > > > Main link in standby mode: no > > > HW Enabled & Active bit: yes > > > Source PSR status: 0x24050006 [SRDONACK] > > > > > > To: > > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status > > > Sink support: yes [0x00000003] > > > Status: PSR1 enabled > > > Source PSR ctl: enabled [0x81f00e26] > > > Source PSR status: SRDENT [0x40040006] > > > Busy frontbuffer bits: 0x00000000 > > > > > > The 'Main link in standby mode' was removed as it is not useful > > > but > > > if needed by someone the information is still in the register > > > value > > > of 'Source PSR ctl' inside of the brackets, PSR mode and Enabled > > > was > > > squashed into Status, some renames and reorders and we have this > > > cleaner version. This will also make easy to parse debugfs for > > > IGT > > > tests. > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++------ > > > ---- > > > ---- > > > 1 file changed, 49 insertions(+), 47 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > > b/drivers/gpu/drm/i915/i915_debugfs.c > > > index 38dcee1ca062..86303ba02666 100644 > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > @@ -2665,7 +2665,8 @@ > > > DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status); > > > static void > > > psr_source_status(struct drm_i915_private *dev_priv, struct > > > seq_file > > > *m) > > > { > > > - u32 val, psr_status; > > > + u32 val, status_val; > > > + const char *status = "unknown"; > > > > > > if (dev_priv->psr.psr2_enabled) { > > > static const char * const live_status[] = { > > > @@ -2681,14 +2682,11 @@ psr_source_status(struct drm_i915_private > > > *dev_priv, struct seq_file *m) > > > "BUF_ON", > > > "TG_ON" > > > }; > > > - psr_status = I915_READ(EDP_PSR2_STATUS); > > > - val = (psr_status & EDP_PSR2_STATUS_STATE_MASK) >> > > > - EDP_PSR2_STATUS_STATE_SHIFT; > > > - if (val < ARRAY_SIZE(live_status)) { > > > - seq_printf(m, "Source PSR status: 0x%x [%s]\n", > > > - psr_status, live_status[val]); > > > - return; > > > - } > > > + val = I915_READ(EDP_PSR2_STATUS); > > > + status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >> > > > + EDP_PSR2_STATUS_STATE_SHIFT; > > > + if (status_val < ARRAY_SIZE(live_status)) > > > + status = live_status[status_val]; > > > } else { > > > static const char * const live_status[] = { > > > "IDLE", > > > @@ -2700,74 +2698,78 @@ psr_source_status(struct drm_i915_private > > > *dev_priv, struct seq_file *m) > > > "SRDOFFACK", > > > "SRDENT_ON", > > > }; > > > - psr_status = I915_READ(EDP_PSR_STATUS); > > > - val = (psr_status & EDP_PSR_STATUS_STATE_MASK) >> > > > - EDP_PSR_STATUS_STATE_SHIFT; > > > - if (val < ARRAY_SIZE(live_status)) { > > > - seq_printf(m, "Source PSR status: 0x%x [%s]\n", > > > - psr_status, live_status[val]); > > > - return; > > > - } > > > + val = I915_READ(EDP_PSR_STATUS); > > > + status_val = (val & EDP_PSR_STATUS_STATE_MASK) >> > > > + EDP_PSR_STATUS_STATE_SHIFT; > > > + if (status_val < ARRAY_SIZE(live_status)) > > > + status = live_status[status_val]; > > > } > > > > > > - seq_printf(m, "Source PSR status: 0x%x [%s]\n", psr_status, > > > "unknown"); > > > + seq_printf(m, "Source PSR status: %s [0x%08x]\n", status, val); > > > } > > > > > > static int i915_edp_psr_status(struct seq_file *m, void *data) > > > { > > > struct drm_i915_private *dev_priv = node_to_i915(m->private); > > > - u32 psrperf = 0; > > > - bool enabled = false; > > > - bool sink_support; > > > + struct i915_psr *psr = &dev_priv->psr; > > > + const char *status; > > > + bool enabled; > > > + u32 val; > > > > > > if (!HAS_PSR(dev_priv)) > > > return -ENODEV; > > > > > > - sink_support = dev_priv->psr.sink_support; > > > - seq_printf(m, "Sink_Support: %s\n", yesno(sink_support)); > > > - if (!sink_support) > > > + seq_printf(m, "Sink support: %s", yesno(psr->sink_support)); > > > + if (!psr->sink_support) { > > > + seq_puts(m, "\n"); > > > return 0; > > > + } > > > > > > intel_runtime_pm_get(dev_priv); > > > + mutex_lock(&psr->lock); > > > > > > - mutex_lock(&dev_priv->psr.lock); > > > - seq_printf(m, "PSR mode: %s\n", > > > - dev_priv->psr.psr2_enabled ? "PSR2" : "PSR1"); > > > - seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled)); > > > - seq_printf(m, "Busy frontbuffer bits: 0x%03x\n", > > > - dev_priv->psr.busy_frontbuffer_bits); > > > + seq_printf(m, " [0x%08x]\n", psr->dp->psr_dpcd[0]); > > > > This can be moved closer to where "Sink support" is printed. > > Also, the > > extra zeroes that get printed due to "%08x" look odd, please > > consider > > changing it to "%02x". > > Done > > > > > > > > > - if (dev_priv->psr.psr2_enabled) > > > - enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE; > > > + if (psr->enabled) > > > + status = psr->psr2_enabled ? "PSR2 enabled" : "PSR1 > > > enabled"; > > > else > > > - enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE; > > > + status = "disabled"; > > > + seq_printf(m, "Status: %s\n", status); > > > > Can we just use the ctl field and get rid of this? This status and > > and > > the one below can be confusing to those not familiar with the code > > base. > > > > We should be able to parse all the information we need from two > > lines > > Source PSR ctl: [PSR1,PSR2] enabled <reg value> > > Source PSR status: <state> <reg value> > > I'm fine in having just one but when there is frontbuffer > modifications > Status is kept as enabled and Source PSR ctl and Source PSR status > are > set to idle states, That is a good point - psr_invalidate() would disable the feature in hardware but the driver state is enabled. I guess, you could change "Status" to "PSR mode"/"Feature state". ... PSR mode: {PSR1 enabled, PSR2 enabled, disabled} Source PSR ctl: {enabled, disabled} ... Source PSR status: ... ... Is this better? Also, it might be worth printing the "PSR mode:" line even when there is no sink_support. It's an useful piece of debug information and doesn't access the hardware. > we know that but those not familiar with the code > could think it is disabled but it usualy happen so fast that I guess > it > is not a problem. > > And when PSR is disabled we print something else other than Sink > support? > > > > > > > > > > > > > - seq_printf(m, "Main link in standby mode: %s\n", > > > - yesno(dev_priv->psr.link_standby)); > > > + if (!psr->enabled) > > > + goto unlock; > > > > > > - seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled)); > > > + if (psr->psr2_enabled) { > > > + val = I915_READ(EDP_PSR2_CTL); > > > + enabled = val & EDP_PSR2_ENABLE; > > > + } else { > > > + val = I915_READ(EDP_PSR_CTL); > > > + enabled = val & EDP_PSR_ENABLE; > > > + } > > > + seq_printf(m, "Source PSR ctl: %s [0x%08x]\n", > > > + enableddisabled(enabled), val); > > > + psr_source_status(dev_priv, m); > > > + seq_printf(m, "Busy frontbuffer bits: 0x%08x\n", > > > + psr->busy_frontbuffer_bits); > > > > > > /* > > > * SKL+ Perf counter is reset to 0 everytime DC state is > > > entered > > > */ > > > if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { > > > - psrperf = I915_READ(EDP_PSR_PERF_CNT) & > > > - EDP_PSR_PERF_CNT_MASK; > > > - > > > - seq_printf(m, "Performance_Counter: %u\n", psrperf); > > > + val = I915_READ(EDP_PSR_PERF_CNT) & > > > EDP_PSR_PERF_CNT_MASK; > > > + seq_printf(m, "Performance counter: %u\n", val); > > > } > > > > > > - psr_source_status(dev_priv, m); > > > - mutex_unlock(&dev_priv->psr.lock); > > > - > > > - if (READ_ONCE(dev_priv->psr.debug) & I915_PSR_DEBUG_IRQ) { > > > + if (psr->debug & I915_PSR_DEBUG_IRQ) { > > > seq_printf(m, "Last attempted entry at: %lld\n", > > > - dev_priv->psr.last_entry_attempt); > > > - seq_printf(m, "Last exit at: %lld\n", > > > - dev_priv->psr.last_exit); > > > + psr->last_entry_attempt); > > > + seq_printf(m, "Last exit at: %lld\n", psr->last_exit); > > > } > > > > > > +unlock: > > > + mutex_unlock(&psr->lock); > > > intel_runtime_pm_put(dev_priv); > > > + > > > return 0; > > > } > > >
On Tue, 2018-12-11 at 10:32 -0800, Dhinakaran Pandiyan wrote: > On Tue, 2018-12-11 at 04:44 -0800, Souza, Jose wrote: > > On Mon, 2018-12-10 at 22:51 -0800, Dhinakaran Pandiyan wrote: > > > On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote: > > > > The old debugfs fields was not following a naming partern and > > > > it > > > > was > > > > a bit confusing. > > > > > > > > So it went from: > > > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status > > > > Sink_Support: yes > > > > PSR mode: PSR1 > > > > Enabled: yes > > > > Busy frontbuffer bits: 0x000 > > > > Main link in standby mode: no > > > > HW Enabled & Active bit: yes > > > > Source PSR status: 0x24050006 [SRDONACK] > > > > > > > > To: > > > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status > > > > Sink support: yes [0x00000003] > > > > Status: PSR1 enabled > > > > Source PSR ctl: enabled [0x81f00e26] > > > > Source PSR status: SRDENT [0x40040006] > > > > Busy frontbuffer bits: 0x00000000 > > > > > > > > The 'Main link in standby mode' was removed as it is not useful > > > > but > > > > if needed by someone the information is still in the register > > > > value > > > > of 'Source PSR ctl' inside of the brackets, PSR mode and > > > > Enabled > > > > was > > > > squashed into Status, some renames and reorders and we have > > > > this > > > > cleaner version. This will also make easy to parse debugfs for > > > > IGT > > > > tests. > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > Suggested-by: Dhinakaran Pandiyan < > > > > dhinakaran.pandiyan@intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++------ > > > > ---- > > > > ---- > > > > 1 file changed, 49 insertions(+), 47 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > > > b/drivers/gpu/drm/i915/i915_debugfs.c > > > > index 38dcee1ca062..86303ba02666 100644 > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > > @@ -2665,7 +2665,8 @@ > > > > DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status); > > > > static void > > > > psr_source_status(struct drm_i915_private *dev_priv, struct > > > > seq_file > > > > *m) > > > > { > > > > - u32 val, psr_status; > > > > + u32 val, status_val; > > > > + const char *status = "unknown"; > > > > > > > > if (dev_priv->psr.psr2_enabled) { > > > > static const char * const live_status[] = { > > > > @@ -2681,14 +2682,11 @@ psr_source_status(struct > > > > drm_i915_private > > > > *dev_priv, struct seq_file *m) > > > > "BUF_ON", > > > > "TG_ON" > > > > }; > > > > - psr_status = I915_READ(EDP_PSR2_STATUS); > > > > - val = (psr_status & EDP_PSR2_STATUS_STATE_MASK) > > > > >> > > > > - EDP_PSR2_STATUS_STATE_SHIFT; > > > > - if (val < ARRAY_SIZE(live_status)) { > > > > - seq_printf(m, "Source PSR status: 0x%x > > > > [%s]\n", > > > > - psr_status, > > > > live_status[val]); > > > > - return; > > > > - } > > > > + val = I915_READ(EDP_PSR2_STATUS); > > > > + status_val = (val & EDP_PSR2_STATUS_STATE_MASK) > > > > >> > > > > + EDP_PSR2_STATUS_STATE_SHIFT; > > > > + if (status_val < ARRAY_SIZE(live_status)) > > > > + status = live_status[status_val]; > > > > } else { > > > > static const char * const live_status[] = { > > > > "IDLE", > > > > @@ -2700,74 +2698,78 @@ psr_source_status(struct > > > > drm_i915_private > > > > *dev_priv, struct seq_file *m) > > > > "SRDOFFACK", > > > > "SRDENT_ON", > > > > }; > > > > - psr_status = I915_READ(EDP_PSR_STATUS); > > > > - val = (psr_status & EDP_PSR_STATUS_STATE_MASK) > > > > >> > > > > - EDP_PSR_STATUS_STATE_SHIFT; > > > > - if (val < ARRAY_SIZE(live_status)) { > > > > - seq_printf(m, "Source PSR status: 0x%x > > > > [%s]\n", > > > > - psr_status, > > > > live_status[val]); > > > > - return; > > > > - } > > > > + val = I915_READ(EDP_PSR_STATUS); > > > > + status_val = (val & EDP_PSR_STATUS_STATE_MASK) > > > > >> > > > > + EDP_PSR_STATUS_STATE_SHIFT; > > > > + if (status_val < ARRAY_SIZE(live_status)) > > > > + status = live_status[status_val]; > > > > } > > > > > > > > - seq_printf(m, "Source PSR status: 0x%x [%s]\n", > > > > psr_status, > > > > "unknown"); > > > > + seq_printf(m, "Source PSR status: %s [0x%08x]\n", > > > > status, val); > > > > } > > > > > > > > static int i915_edp_psr_status(struct seq_file *m, void *data) > > > > { > > > > struct drm_i915_private *dev_priv = node_to_i915(m- > > > > >private); > > > > - u32 psrperf = 0; > > > > - bool enabled = false; > > > > - bool sink_support; > > > > + struct i915_psr *psr = &dev_priv->psr; > > > > + const char *status; > > > > + bool enabled; > > > > + u32 val; > > > > > > > > if (!HAS_PSR(dev_priv)) > > > > return -ENODEV; > > > > > > > > - sink_support = dev_priv->psr.sink_support; > > > > - seq_printf(m, "Sink_Support: %s\n", > > > > yesno(sink_support)); > > > > - if (!sink_support) > > > > + seq_printf(m, "Sink support: %s", yesno(psr- > > > > >sink_support)); > > > > + if (!psr->sink_support) { > > > > + seq_puts(m, "\n"); > > > > return 0; > > > > + } > > > > > > > > intel_runtime_pm_get(dev_priv); > > > > + mutex_lock(&psr->lock); > > > > > > > > - mutex_lock(&dev_priv->psr.lock); > > > > - seq_printf(m, "PSR mode: %s\n", > > > > - dev_priv->psr.psr2_enabled ? "PSR2" : > > > > "PSR1"); > > > > - seq_printf(m, "Enabled: %s\n", yesno(dev_priv- > > > > >psr.enabled)); > > > > - seq_printf(m, "Busy frontbuffer bits: 0x%03x\n", > > > > - dev_priv->psr.busy_frontbuffer_bits); > > > > + seq_printf(m, " [0x%08x]\n", psr->dp->psr_dpcd[0]); > > > > > > This can be moved closer to where "Sink support" is printed. > > > Also, the > > > extra zeroes that get printed due to "%08x" look odd, please > > > consider > > > changing it to "%02x". > > > > Done > > > > > > > > > > - if (dev_priv->psr.psr2_enabled) > > > > - enabled = I915_READ(EDP_PSR2_CTL) & > > > > EDP_PSR2_ENABLE; > > > > + if (psr->enabled) > > > > + status = psr->psr2_enabled ? "PSR2 enabled" : > > > > "PSR1 > > > > enabled"; > > > > else > > > > - enabled = I915_READ(EDP_PSR_CTL) & > > > > EDP_PSR_ENABLE; > > > > + status = "disabled"; > > > > + seq_printf(m, "Status: %s\n", status); > > > > > > Can we just use the ctl field and get rid of this? This status > > > and > > > and > > > the one below can be confusing to those not familiar with the > > > code > > > base. > > > > > > We should be able to parse all the information we need from two > > > lines > > > Source PSR ctl: [PSR1,PSR2] enabled <reg value> > > > Source PSR status: <state> <reg value> > > > > I'm fine in having just one but when there is frontbuffer > > modifications > > Status is kept as enabled and Source PSR ctl and Source PSR status > > are > > set to idle states, > > That is a good point - psr_invalidate() would disable the feature > in > hardware but the driver state is enabled. I guess, you could change > "Status" to "PSR mode"/"Feature state". > ... > PSR mode: {PSR1 enabled, PSR2 enabled, disabled} > Source PSR ctl: {enabled, disabled} ... > Source PSR status: ... > ... > > Is this better? > Yeah looks good to me. > Also, it might be worth printing the "PSR mode:" line even when there > is no sink_support. It's an useful piece of debug information and > doesn't access the hardware. Okay Resending serie with fixes soon, thanks for the review. > > > > we know that but those not familiar with the code > > could think it is disabled but it usualy happen so fast that I > > guess > > it > > is not a problem. > > > > And when PSR is disabled we print something else other than Sink > > support? > > > > > > > > > > > > > > > > - seq_printf(m, "Main link in standby mode: %s\n", > > > > - yesno(dev_priv->psr.link_standby)); > > > > + if (!psr->enabled) > > > > + goto unlock; > > > > > > > > - seq_printf(m, "HW Enabled & Active bit: %s\n", > > > > yesno(enabled)); > > > > + if (psr->psr2_enabled) { > > > > + val = I915_READ(EDP_PSR2_CTL); > > > > + enabled = val & EDP_PSR2_ENABLE; > > > > + } else { > > > > + val = I915_READ(EDP_PSR_CTL); > > > > + enabled = val & EDP_PSR_ENABLE; > > > > + } > > > > + seq_printf(m, "Source PSR ctl: %s [0x%08x]\n", > > > > + enableddisabled(enabled), val); > > > > + psr_source_status(dev_priv, m); > > > > + seq_printf(m, "Busy frontbuffer bits: 0x%08x\n", > > > > + psr->busy_frontbuffer_bits); > > > > > > > > /* > > > > * SKL+ Perf counter is reset to 0 everytime DC state > > > > is > > > > entered > > > > */ > > > > if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { > > > > - psrperf = I915_READ(EDP_PSR_PERF_CNT) & > > > > - EDP_PSR_PERF_CNT_MASK; > > > > - > > > > - seq_printf(m, "Performance_Counter: %u\n", > > > > psrperf); > > > > + val = I915_READ(EDP_PSR_PERF_CNT) & > > > > EDP_PSR_PERF_CNT_MASK; > > > > + seq_printf(m, "Performance counter: %u\n", > > > > val); > > > > } > > > > > > > > - psr_source_status(dev_priv, m); > > > > - mutex_unlock(&dev_priv->psr.lock); > > > > - > > > > - if (READ_ONCE(dev_priv->psr.debug) & > > > > I915_PSR_DEBUG_IRQ) { > > > > + if (psr->debug & I915_PSR_DEBUG_IRQ) { > > > > seq_printf(m, "Last attempted entry at: > > > > %lld\n", > > > > - dev_priv->psr.last_entry_attempt); > > > > - seq_printf(m, "Last exit at: %lld\n", > > > > - dev_priv->psr.last_exit); > > > > + psr->last_entry_attempt); > > > > + seq_printf(m, "Last exit at: %lld\n", psr- > > > > >last_exit); > > > > } > > > > > > > > +unlock: > > > > + mutex_unlock(&psr->lock); > > > > intel_runtime_pm_put(dev_priv); > > > > + > > > > return 0; > > > > } > > > >
On Tue, 2018-12-11 at 18:54 +0000, Souza, Jose wrote: > On Tue, 2018-12-11 at 10:32 -0800, Dhinakaran Pandiyan wrote: > > On Tue, 2018-12-11 at 04:44 -0800, Souza, Jose wrote: > > > On Mon, 2018-12-10 at 22:51 -0800, Dhinakaran Pandiyan wrote: > > > > On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote: > > > > > The old debugfs fields was not following a naming partern and > > > > > it > > > > > was > > > > > a bit confusing. > > > > > > > > > > So it went from: > > > > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status > > > > > Sink_Support: yes > > > > > PSR mode: PSR1 > > > > > Enabled: yes > > > > > Busy frontbuffer bits: 0x000 > > > > > Main link in standby mode: no > > > > > HW Enabled & Active bit: yes > > > > > Source PSR status: 0x24050006 [SRDONACK] > > > > > > > > > > To: > > > > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status > > > > > Sink support: yes [0x00000003] > > > > > Status: PSR1 enabled > > > > > Source PSR ctl: enabled [0x81f00e26] > > > > > Source PSR status: SRDENT [0x40040006] > > > > > Busy frontbuffer bits: 0x00000000 > > > > > > > > > > The 'Main link in standby mode' was removed as it is not > > > > > useful > > > > > but > > > > > if needed by someone the information is still in the register > > > > > value > > > > > of 'Source PSR ctl' inside of the brackets, PSR mode and > > > > > Enabled > > > > > was > > > > > squashed into Status, some renames and reorders and we have > > > > > this > > > > > cleaner version. This will also make easy to parse debugfs > > > > > for > > > > > IGT > > > > > tests. > > > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > Suggested-by: Dhinakaran Pandiyan < > > > > > dhinakaran.pandiyan@intel.com> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++-- > > > > > ---- > > > > > ---- > > > > > ---- > > > > > 1 file changed, 49 insertions(+), 47 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > > > > b/drivers/gpu/drm/i915/i915_debugfs.c > > > > > index 38dcee1ca062..86303ba02666 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > > > @@ -2665,7 +2665,8 @@ > > > > > DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status); > > > > > static void > > > > > psr_source_status(struct drm_i915_private *dev_priv, struct > > > > > seq_file > > > > > *m) > > > > > { > > > > > - u32 val, psr_status; > > > > > + u32 val, status_val; > > > > > + const char *status = "unknown"; > > > > > > > > > > if (dev_priv->psr.psr2_enabled) { > > > > > static const char * const live_status[] = { > > > > > @@ -2681,14 +2682,11 @@ psr_source_status(struct > > > > > drm_i915_private > > > > > *dev_priv, struct seq_file *m) > > > > > "BUF_ON", > > > > > "TG_ON" > > > > > }; > > > > > - psr_status = I915_READ(EDP_PSR2_STATUS); > > > > > - val = (psr_status & EDP_PSR2_STATUS_STATE_MASK) > > > > > - EDP_PSR2_STATUS_STATE_SHIFT; > > > > > - if (val < ARRAY_SIZE(live_status)) { > > > > > - seq_printf(m, "Source PSR status: 0x%x > > > > > [%s]\n", > > > > > - psr_status, > > > > > live_status[val]); > > > > > - return; > > > > > - } > > > > > + val = I915_READ(EDP_PSR2_STATUS); > > > > > + status_val = (val & EDP_PSR2_STATUS_STATE_MASK) > > > > > + EDP_PSR2_STATUS_STATE_SHIFT; > > > > > + if (status_val < ARRAY_SIZE(live_status)) > > > > > + status = live_status[status_val]; > > > > > } else { > > > > > static const char * const live_status[] = { > > > > > "IDLE", > > > > > @@ -2700,74 +2698,78 @@ psr_source_status(struct > > > > > drm_i915_private > > > > > *dev_priv, struct seq_file *m) > > > > > "SRDOFFACK", > > > > > "SRDENT_ON", > > > > > }; > > > > > - psr_status = I915_READ(EDP_PSR_STATUS); > > > > > - val = (psr_status & EDP_PSR_STATUS_STATE_MASK) > > > > > - EDP_PSR_STATUS_STATE_SHIFT; > > > > > - if (val < ARRAY_SIZE(live_status)) { > > > > > - seq_printf(m, "Source PSR status: 0x%x > > > > > [%s]\n", > > > > > - psr_status, > > > > > live_status[val]); > > > > > - return; > > > > > - } > > > > > + val = I915_READ(EDP_PSR_STATUS); > > > > > + status_val = (val & EDP_PSR_STATUS_STATE_MASK) > > > > > + EDP_PSR_STATUS_STATE_SHIFT; > > > > > + if (status_val < ARRAY_SIZE(live_status)) > > > > > + status = live_status[status_val]; > > > > > } > > > > > > > > > > - seq_printf(m, "Source PSR status: 0x%x [%s]\n", > > > > > psr_status, > > > > > "unknown"); > > > > > + seq_printf(m, "Source PSR status: %s [0x%08x]\n", > > > > > status, val); > > > > > } > > > > > > > > > > static int i915_edp_psr_status(struct seq_file *m, void > > > > > *data) > > > > > { > > > > > struct drm_i915_private *dev_priv = node_to_i915(m- > > > > > > private); > > > > > - u32 psrperf = 0; > > > > > - bool enabled = false; > > > > > - bool sink_support; > > > > > + struct i915_psr *psr = &dev_priv->psr; > > > > > + const char *status; > > > > > + bool enabled; > > > > > + u32 val; > > > > > > > > > > if (!HAS_PSR(dev_priv)) > > > > > return -ENODEV; > > > > > > > > > > - sink_support = dev_priv->psr.sink_support; > > > > > - seq_printf(m, "Sink_Support: %s\n", > > > > > yesno(sink_support)); > > > > > - if (!sink_support) > > > > > + seq_printf(m, "Sink support: %s", yesno(psr- > > > > > > sink_support)); > > > > > + if (!psr->sink_support) { > > > > > + seq_puts(m, "\n"); > > > > > return 0; > > > > > + } > > > > > > > > > > intel_runtime_pm_get(dev_priv); > > > > > + mutex_lock(&psr->lock); > > > > > > > > > > - mutex_lock(&dev_priv->psr.lock); > > > > > - seq_printf(m, "PSR mode: %s\n", > > > > > - dev_priv->psr.psr2_enabled ? "PSR2" : > > > > > "PSR1"); > > > > > - seq_printf(m, "Enabled: %s\n", yesno(dev_priv- > > > > > > psr.enabled)); > > > > > - seq_printf(m, "Busy frontbuffer bits: 0x%03x\n", > > > > > - dev_priv->psr.busy_frontbuffer_bits); > > > > > + seq_printf(m, " [0x%08x]\n", psr->dp->psr_dpcd[0]); > > > > > > > > This can be moved closer to where "Sink support" is printed. > > > > Also, the > > > > extra zeroes that get printed due to "%08x" look odd, please > > > > consider > > > > changing it to "%02x". > > > > > > Done > > > > > > > > > > > > > - if (dev_priv->psr.psr2_enabled) > > > > > - enabled = I915_READ(EDP_PSR2_CTL) & > > > > > EDP_PSR2_ENABLE; > > > > > + if (psr->enabled) > > > > > + status = psr->psr2_enabled ? "PSR2 enabled" : > > > > > "PSR1 > > > > > enabled"; > > > > > else > > > > > - enabled = I915_READ(EDP_PSR_CTL) & > > > > > EDP_PSR_ENABLE; > > > > > + status = "disabled"; > > > > > + seq_printf(m, "Status: %s\n", status); > > > > > > > > Can we just use the ctl field and get rid of this? This status > > > > and > > > > and > > > > the one below can be confusing to those not familiar with the > > > > code > > > > base. > > > > > > > > We should be able to parse all the information we need from two > > > > lines > > > > Source PSR ctl: [PSR1,PSR2] enabled <reg value> > > > > Source PSR status: <state> <reg value> > > > > > > I'm fine in having just one but when there is frontbuffer > > > modifications > > > Status is kept as enabled and Source PSR ctl and Source PSR > > > status > > > are > > > set to idle states, > > > > That is a good point - psr_invalidate() would disable the feature > > in > > hardware but the driver state is enabled. I guess, you could change > > "Status" to "PSR mode"/"Feature state". > > ... > > PSR mode: {PSR1 enabled, PSR2 enabled, disabled} > > Source PSR ctl: {enabled, disabled} ... > > Source PSR status: ... > > ... > > > > Is this better? > > > > Yeah looks good to me. > > > Also, it might be worth printing the "PSR mode:" line even when > > there > > is no sink_support. It's an useful piece of debug information and > > doesn't access the hardware. About this one, to read psr->enable and psr->psr2_enabled we take PSR mutex and it is only initialized if sink support PSR. If that information is useful when doing IGT changes I will send it in a separate patch, sounds good? > > > Okay > > Resending serie with fixes soon, thanks for the review. > > > > > > we know that but those not familiar with the code > > > could think it is disabled but it usualy happen so fast that I > > > guess > > > it > > > is not a problem. > > > > > > And when PSR is disabled we print something else other than Sink > > > support? > > > > > > > > > > > > > > > > > - seq_printf(m, "Main link in standby mode: %s\n", > > > > > - yesno(dev_priv->psr.link_standby)); > > > > > + if (!psr->enabled) > > > > > + goto unlock; > > > > > > > > > > - seq_printf(m, "HW Enabled & Active bit: %s\n", > > > > > yesno(enabled)); > > > > > + if (psr->psr2_enabled) { > > > > > + val = I915_READ(EDP_PSR2_CTL); > > > > > + enabled = val & EDP_PSR2_ENABLE; > > > > > + } else { > > > > > + val = I915_READ(EDP_PSR_CTL); > > > > > + enabled = val & EDP_PSR_ENABLE; > > > > > + } > > > > > + seq_printf(m, "Source PSR ctl: %s [0x%08x]\n", > > > > > + enableddisabled(enabled), val); > > > > > + psr_source_status(dev_priv, m); > > > > > + seq_printf(m, "Busy frontbuffer bits: 0x%08x\n", > > > > > + psr->busy_frontbuffer_bits); > > > > > > > > > > /* > > > > > * SKL+ Perf counter is reset to 0 everytime DC state > > > > > is > > > > > entered > > > > > */ > > > > > if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { > > > > > - psrperf = I915_READ(EDP_PSR_PERF_CNT) & > > > > > - EDP_PSR_PERF_CNT_MASK; > > > > > - > > > > > - seq_printf(m, "Performance_Counter: %u\n", > > > > > psrperf); > > > > > + val = I915_READ(EDP_PSR_PERF_CNT) & > > > > > EDP_PSR_PERF_CNT_MASK; > > > > > + seq_printf(m, "Performance counter: %u\n", > > > > > val); > > > > > } > > > > > > > > > > - psr_source_status(dev_priv, m); > > > > > - mutex_unlock(&dev_priv->psr.lock); > > > > > - > > > > > - if (READ_ONCE(dev_priv->psr.debug) & > > > > > I915_PSR_DEBUG_IRQ) { > > > > > + if (psr->debug & I915_PSR_DEBUG_IRQ) { > > > > > seq_printf(m, "Last attempted entry at: > > > > > %lld\n", > > > > > - dev_priv->psr.last_entry_attempt); > > > > > - seq_printf(m, "Last exit at: %lld\n", > > > > > - dev_priv->psr.last_exit); > > > > > + psr->last_entry_attempt); > > > > > + seq_printf(m, "Last exit at: %lld\n", psr- > > > > > > last_exit); > > > > > } > > > > > > > > > > +unlock: > > > > > + mutex_unlock(&psr->lock); > > > > > intel_runtime_pm_put(dev_priv); > > > > > + > > > > > return 0; > > > > > } > > > > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2019-01-02 at 09:09 -0800, Souza, Jose wrote: > On Tue, 2018-12-11 at 18:54 +0000, Souza, Jose wrote: > > On Tue, 2018-12-11 at 10:32 -0800, Dhinakaran Pandiyan wrote: > > > On Tue, 2018-12-11 at 04:44 -0800, Souza, Jose wrote: > > > > On Mon, 2018-12-10 at 22:51 -0800, Dhinakaran Pandiyan wrote: > > > > > On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza > > > > > wrote: > > > > > > The old debugfs fields was not following a naming partern > > > > > > and > > > > > > it > > > > > > was > > > > > > a bit confusing. > > > > > > > > > > > > So it went from: > > > > > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status > > > > > > Sink_Support: yes > > > > > > PSR mode: PSR1 > > > > > > Enabled: yes > > > > > > Busy frontbuffer bits: 0x000 > > > > > > Main link in standby mode: no > > > > > > HW Enabled & Active bit: yes > > > > > > Source PSR status: 0x24050006 [SRDONACK] > > > > > > > > > > > > To: > > > > > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status > > > > > > Sink support: yes [0x00000003] > > > > > > Status: PSR1 enabled > > > > > > Source PSR ctl: enabled [0x81f00e26] > > > > > > Source PSR status: SRDENT [0x40040006] > > > > > > Busy frontbuffer bits: 0x00000000 > > > > > > > > > > > > The 'Main link in standby mode' was removed as it is not > > > > > > useful > > > > > > but > > > > > > if needed by someone the information is still in the > > > > > > register > > > > > > value > > > > > > of 'Source PSR ctl' inside of the brackets, PSR mode and > > > > > > Enabled > > > > > > was > > > > > > squashed into Status, some renames and reorders and we have > > > > > > this > > > > > > cleaner version. This will also make easy to parse debugfs > > > > > > for > > > > > > IGT > > > > > > tests. > > > > > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > > Suggested-by: Dhinakaran Pandiyan < > > > > > > dhinakaran.pandiyan@intel.com> > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++-- > > > > > > ---- > > > > > > ---- > > > > > > ---- > > > > > > 1 file changed, 49 insertions(+), 47 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > > > > > b/drivers/gpu/drm/i915/i915_debugfs.c > > > > > > index 38dcee1ca062..86303ba02666 100644 > > > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > > > > @@ -2665,7 +2665,8 @@ > > > > > > DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status); > > > > > > static void > > > > > > psr_source_status(struct drm_i915_private *dev_priv, > > > > > > struct > > > > > > seq_file > > > > > > *m) > > > > > > { > > > > > > - u32 val, psr_status; > > > > > > + u32 val, status_val; > > > > > > + const char *status = "unknown"; > > > > > > > > > > > > if (dev_priv->psr.psr2_enabled) { > > > > > > static const char * const live_status[] = { > > > > > > @@ -2681,14 +2682,11 @@ psr_source_status(struct > > > > > > drm_i915_private > > > > > > *dev_priv, struct seq_file *m) > > > > > > "BUF_ON", > > > > > > "TG_ON" > > > > > > }; > > > > > > - psr_status = I915_READ(EDP_PSR2_STATUS); > > > > > > - val = (psr_status & EDP_PSR2_STATUS_STATE_MASK) > > > > > > - EDP_PSR2_STATUS_STATE_SHIFT; > > > > > > - if (val < ARRAY_SIZE(live_status)) { > > > > > > - seq_printf(m, "Source PSR status: 0x%x > > > > > > [%s]\n", > > > > > > - psr_status, > > > > > > live_status[val]); > > > > > > - return; > > > > > > - } > > > > > > + val = I915_READ(EDP_PSR2_STATUS); > > > > > > + status_val = (val & EDP_PSR2_STATUS_STATE_MASK) > > > > > > + EDP_PSR2_STATUS_STATE_SHIFT; > > > > > > + if (status_val < ARRAY_SIZE(live_status)) > > > > > > + status = live_status[status_val]; > > > > > > } else { > > > > > > static const char * const live_status[] = { > > > > > > "IDLE", > > > > > > @@ -2700,74 +2698,78 @@ psr_source_status(struct > > > > > > drm_i915_private > > > > > > *dev_priv, struct seq_file *m) > > > > > > "SRDOFFACK", > > > > > > "SRDENT_ON", > > > > > > }; > > > > > > - psr_status = I915_READ(EDP_PSR_STATUS); > > > > > > - val = (psr_status & EDP_PSR_STATUS_STATE_MASK) > > > > > > - EDP_PSR_STATUS_STATE_SHIFT; > > > > > > - if (val < ARRAY_SIZE(live_status)) { > > > > > > - seq_printf(m, "Source PSR status: 0x%x > > > > > > [%s]\n", > > > > > > - psr_status, > > > > > > live_status[val]); > > > > > > - return; > > > > > > - } > > > > > > + val = I915_READ(EDP_PSR_STATUS); > > > > > > + status_val = (val & EDP_PSR_STATUS_STATE_MASK) > > > > > > + EDP_PSR_STATUS_STATE_SHIFT; > > > > > > + if (status_val < ARRAY_SIZE(live_status)) > > > > > > + status = live_status[status_val]; > > > > > > } > > > > > > > > > > > > - seq_printf(m, "Source PSR status: 0x%x [%s]\n", > > > > > > psr_status, > > > > > > "unknown"); > > > > > > + seq_printf(m, "Source PSR status: %s [0x%08x]\n", > > > > > > status, val); > > > > > > } > > > > > > > > > > > > static int i915_edp_psr_status(struct seq_file *m, void > > > > > > *data) > > > > > > { > > > > > > struct drm_i915_private *dev_priv = node_to_i915(m- > > > > > > > private); > > > > > > > > > > > > - u32 psrperf = 0; > > > > > > - bool enabled = false; > > > > > > - bool sink_support; > > > > > > + struct i915_psr *psr = &dev_priv->psr; > > > > > > + const char *status; > > > > > > + bool enabled; > > > > > > + u32 val; > > > > > > > > > > > > if (!HAS_PSR(dev_priv)) > > > > > > return -ENODEV; > > > > > > > > > > > > - sink_support = dev_priv->psr.sink_support; > > > > > > - seq_printf(m, "Sink_Support: %s\n", > > > > > > yesno(sink_support)); > > > > > > - if (!sink_support) > > > > > > + seq_printf(m, "Sink support: %s", yesno(psr- > > > > > > > sink_support)); > > > > > > > > > > > > + if (!psr->sink_support) { > > > > > > + seq_puts(m, "\n"); > > > > > > return 0; > > > > > > + } > > > > > > > > > > > > intel_runtime_pm_get(dev_priv); > > > > > > + mutex_lock(&psr->lock); > > > > > > > > > > > > - mutex_lock(&dev_priv->psr.lock); > > > > > > - seq_printf(m, "PSR mode: %s\n", > > > > > > - dev_priv->psr.psr2_enabled ? "PSR2" : > > > > > > "PSR1"); > > > > > > - seq_printf(m, "Enabled: %s\n", yesno(dev_priv- > > > > > > > psr.enabled)); > > > > > > > > > > > > - seq_printf(m, "Busy frontbuffer bits: 0x%03x\n", > > > > > > - dev_priv->psr.busy_frontbuffer_bits); > > > > > > + seq_printf(m, " [0x%08x]\n", psr->dp->psr_dpcd[0]); > > > > > > > > > > This can be moved closer to where "Sink support" is printed. > > > > > Also, the > > > > > extra zeroes that get printed due to "%08x" look odd, please > > > > > consider > > > > > changing it to "%02x". > > > > > > > > Done > > > > > > > > > > > > > > > > - if (dev_priv->psr.psr2_enabled) > > > > > > - enabled = I915_READ(EDP_PSR2_CTL) & > > > > > > EDP_PSR2_ENABLE; > > > > > > + if (psr->enabled) > > > > > > + status = psr->psr2_enabled ? "PSR2 enabled" : > > > > > > "PSR1 > > > > > > enabled"; > > > > > > else > > > > > > - enabled = I915_READ(EDP_PSR_CTL) & > > > > > > EDP_PSR_ENABLE; > > > > > > + status = "disabled"; > > > > > > + seq_printf(m, "Status: %s\n", status); > > > > > > > > > > Can we just use the ctl field and get rid of this? This > > > > > status > > > > > and > > > > > and > > > > > the one below can be confusing to those not familiar with the > > > > > code > > > > > base. > > > > > > > > > > We should be able to parse all the information we need from > > > > > two > > > > > lines > > > > > Source PSR ctl: [PSR1,PSR2] enabled <reg value> > > > > > Source PSR status: <state> <reg value> > > > > > > > > I'm fine in having just one but when there is frontbuffer > > > > modifications > > > > Status is kept as enabled and Source PSR ctl and Source PSR > > > > status > > > > are > > > > set to idle states, > > > > > > That is a good point - psr_invalidate() would disable the > > > feature > > > in > > > hardware but the driver state is enabled. I guess, you could > > > change > > > "Status" to "PSR mode"/"Feature state". > > > ... > > > PSR mode: {PSR1 enabled, PSR2 enabled, disabled} > > > Source PSR ctl: {enabled, disabled} ... > > > Source PSR status: ... > > > ... > > > > > > Is this better? > > > > > > > Yeah looks good to me. > > > > > Also, it might be worth printing the "PSR mode:" line even when > > > there > > > is no sink_support. It's an useful piece of debug information and > > > doesn't access the hardware. > > About this one, to read psr->enable and psr->psr2_enabled we take PSR > mutex and it is only initialized if sink support PSR. > If that information is useful when doing IGT changes I will send it > in > a separate patch, sounds good? Yeah, that's fine. -DK
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 38dcee1ca062..86303ba02666 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2665,7 +2665,8 @@ DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status); static void psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m) { - u32 val, psr_status; + u32 val, status_val; + const char *status = "unknown"; if (dev_priv->psr.psr2_enabled) { static const char * const live_status[] = { @@ -2681,14 +2682,11 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m) "BUF_ON", "TG_ON" }; - psr_status = I915_READ(EDP_PSR2_STATUS); - val = (psr_status & EDP_PSR2_STATUS_STATE_MASK) >> - EDP_PSR2_STATUS_STATE_SHIFT; - if (val < ARRAY_SIZE(live_status)) { - seq_printf(m, "Source PSR status: 0x%x [%s]\n", - psr_status, live_status[val]); - return; - } + val = I915_READ(EDP_PSR2_STATUS); + status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >> + EDP_PSR2_STATUS_STATE_SHIFT; + if (status_val < ARRAY_SIZE(live_status)) + status = live_status[status_val]; } else { static const char * const live_status[] = { "IDLE", @@ -2700,74 +2698,78 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m) "SRDOFFACK", "SRDENT_ON", }; - psr_status = I915_READ(EDP_PSR_STATUS); - val = (psr_status & EDP_PSR_STATUS_STATE_MASK) >> - EDP_PSR_STATUS_STATE_SHIFT; - if (val < ARRAY_SIZE(live_status)) { - seq_printf(m, "Source PSR status: 0x%x [%s]\n", - psr_status, live_status[val]); - return; - } + val = I915_READ(EDP_PSR_STATUS); + status_val = (val & EDP_PSR_STATUS_STATE_MASK) >> + EDP_PSR_STATUS_STATE_SHIFT; + if (status_val < ARRAY_SIZE(live_status)) + status = live_status[status_val]; } - seq_printf(m, "Source PSR status: 0x%x [%s]\n", psr_status, "unknown"); + seq_printf(m, "Source PSR status: %s [0x%08x]\n", status, val); } static int i915_edp_psr_status(struct seq_file *m, void *data) { struct drm_i915_private *dev_priv = node_to_i915(m->private); - u32 psrperf = 0; - bool enabled = false; - bool sink_support; + struct i915_psr *psr = &dev_priv->psr; + const char *status; + bool enabled; + u32 val; if (!HAS_PSR(dev_priv)) return -ENODEV; - sink_support = dev_priv->psr.sink_support; - seq_printf(m, "Sink_Support: %s\n", yesno(sink_support)); - if (!sink_support) + seq_printf(m, "Sink support: %s", yesno(psr->sink_support)); + if (!psr->sink_support) { + seq_puts(m, "\n"); return 0; + } intel_runtime_pm_get(dev_priv); + mutex_lock(&psr->lock); - mutex_lock(&dev_priv->psr.lock); - seq_printf(m, "PSR mode: %s\n", - dev_priv->psr.psr2_enabled ? "PSR2" : "PSR1"); - seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled)); - seq_printf(m, "Busy frontbuffer bits: 0x%03x\n", - dev_priv->psr.busy_frontbuffer_bits); + seq_printf(m, " [0x%08x]\n", psr->dp->psr_dpcd[0]); - if (dev_priv->psr.psr2_enabled) - enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE; + if (psr->enabled) + status = psr->psr2_enabled ? "PSR2 enabled" : "PSR1 enabled"; else - enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE; + status = "disabled"; + seq_printf(m, "Status: %s\n", status); - seq_printf(m, "Main link in standby mode: %s\n", - yesno(dev_priv->psr.link_standby)); + if (!psr->enabled) + goto unlock; - seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled)); + if (psr->psr2_enabled) { + val = I915_READ(EDP_PSR2_CTL); + enabled = val & EDP_PSR2_ENABLE; + } else { + val = I915_READ(EDP_PSR_CTL); + enabled = val & EDP_PSR_ENABLE; + } + seq_printf(m, "Source PSR ctl: %s [0x%08x]\n", + enableddisabled(enabled), val); + psr_source_status(dev_priv, m); + seq_printf(m, "Busy frontbuffer bits: 0x%08x\n", + psr->busy_frontbuffer_bits); /* * SKL+ Perf counter is reset to 0 everytime DC state is entered */ if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { - psrperf = I915_READ(EDP_PSR_PERF_CNT) & - EDP_PSR_PERF_CNT_MASK; - - seq_printf(m, "Performance_Counter: %u\n", psrperf); + val = I915_READ(EDP_PSR_PERF_CNT) & EDP_PSR_PERF_CNT_MASK; + seq_printf(m, "Performance counter: %u\n", val); } - psr_source_status(dev_priv, m); - mutex_unlock(&dev_priv->psr.lock); - - if (READ_ONCE(dev_priv->psr.debug) & I915_PSR_DEBUG_IRQ) { + if (psr->debug & I915_PSR_DEBUG_IRQ) { seq_printf(m, "Last attempted entry at: %lld\n", - dev_priv->psr.last_entry_attempt); - seq_printf(m, "Last exit at: %lld\n", - dev_priv->psr.last_exit); + psr->last_entry_attempt); + seq_printf(m, "Last exit at: %lld\n", psr->last_exit); } +unlock: + mutex_unlock(&psr->lock); intel_runtime_pm_put(dev_priv); + return 0; }
The old debugfs fields was not following a naming partern and it was a bit confusing. So it went from: ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status Sink_Support: yes PSR mode: PSR1 Enabled: yes Busy frontbuffer bits: 0x000 Main link in standby mode: no HW Enabled & Active bit: yes Source PSR status: 0x24050006 [SRDONACK] To: ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status Sink support: yes [0x00000003] Status: PSR1 enabled Source PSR ctl: enabled [0x81f00e26] Source PSR status: SRDENT [0x40040006] Busy frontbuffer bits: 0x00000000 The 'Main link in standby mode' was removed as it is not useful but if needed by someone the information is still in the register value of 'Source PSR ctl' inside of the brackets, PSR mode and Enabled was squashed into Status, some renames and reorders and we have this cleaner version. This will also make easy to parse debugfs for IGT tests. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++-------------- 1 file changed, 49 insertions(+), 47 deletions(-)