On Tue, 2018-05-22 at 14:27 +0530, vathsala nagaraju wrote:
From: Vathsala Nagaraju <vathsala.nagar...@intel.com>
Prints live state of psr1.Extending the existing
PSR2 live state function to cover psr1.
Tested on KBL with psr2 and psr1 panel.
v2: rebase
v3: DK
Rename psr2_live_status to psr_source_status
Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
Signed-off-by: Vathsala Nagaraju <vathsala.nagar...@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 66 +++++++++++++++++++++++--
------------
drivers/gpu/drm/i915/i915_reg.h | 1 +
2 files changed, 43 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 5251544..e4a2f15 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2596,25 +2596,42 @@ static int i915_guc_log_relay_release(struct
inode *inode, struct file *file)
.release = i915_guc_log_relay_release,
};
-static const char *psr2_live_status(u32 val)
-{
- static const char * const live_status[] = {
- "IDLE",
- "CAPTURE",
- "CAPTURE_FS",
- "SLEEP",
- "BUFON_FW",
- "ML_UP",
- "SU_STANDBY",
- "FAST_SLEEP",
- "DEEP_SLEEP",
- "BUF_ON",
- "TG_ON"
- };
-
- val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
EDP_PSR2_STATUS_STATE_SHIFT;
- if (val < ARRAY_SIZE(live_status))
- return live_status[val];
+static const char *psr_source_status(u32 val, bool is_psr2_enabled)
Please change this to psr_source_status(drm_i915_private *dev_priv)
+{
+ if (is_psr2_enabled) {
+ static const char * const live_status[] = {
+ "IDLE",
+ "CAPTURE",
+ "CAPTURE_FS",
+ "SLEEP",
+ "BUFON_FW",
+ "ML_UP",
+ "SU_STANDBY",
+ "FAST_SLEEP",
+ "DEEP_SLEEP",
+ "BUF_ON",
+ "TG_ON"
+ };
With that, you can
live_status = I915_READ(EDP_PSR2_STATUS);
+ val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
+ EDP_PSR2_STATUS_STATE_SHIFT;
+ if (val < ARRAY_SIZE(live_status))
+ return live_status[val];
+ } else {
+ static const char * const live_status[] = {
+ "IDLE",
+ "SRDONACK",
+ "SRDENT",
+ "BUFOFF",
+ "BUFON",
+ "AUXACK",
+ "SRDOFFACK",
+ "SRDENT_ON",
+ };
live_status = I915_READ(EDP_PSR_STATUS);
+ val = (val & EDP_PSR_STATUS_STATE_MASK) >>
+ EDP_PSR_STATUS_STATE_SHIFT;
+ if (val < ARRAY_SIZE(live_status))
+ return live_status[val];
+ }
return "unknown";
}
@@ -2647,6 +2664,7 @@ static int i915_edp_psr_status(struct seq_file
*m, void *data)
enum pipe pipe;
bool enabled = false;
bool sink_support;
+ u32 psr_status;
if (!HAS_PSR(dev_priv))
return -ENODEV;
@@ -2714,12 +2732,12 @@ static int i915_edp_psr_status(struct
seq_file *m, void *data)
seq_printf(m, "Performance_Counter: %u\n", psrperf);
}
- if (dev_priv->psr.psr2_enabled) {
- u32 psr2 = I915_READ(EDP_PSR2_STATUS);
- seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n",
- psr2, psr2_live_status(psr2));
- }
+ psr_status = (dev_priv->psr.psr2_enabled) ?
I915_READ(EDP_PSR2_STATUS) :
+ I915_READ(EDP_PS
R_STATUS);
Please move this inside psr_source_status(), I don't see a point in
checking for psr2_enabled here and again in psr_source_status()
+ seq_printf(m, "SOURCE_PSR_STATUS: %x[%s]\n",
It's easier on the eyes if these strings are consistent, there is no
benefit in writing only this string differently.
Sink status is printed as "Sink PSR status: 0x%x [%s]\n". Please do the
same by writing this as "Source PSR status: 0x%x [%s]\n"
+ psr_status,
+ psr_source_status(psr_status, dev_priv-
psr.psr2_enabled));
if (dev_priv->psr.enabled) {
struct drm_dp_aux *aux = &dev_priv->psr.enabled-
aux;
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index 513b4a4..3c42021 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4069,6 +4069,7 @@ enum {
#define EDP_PSR_STATUS_SENDING_TP2_TP3 (1<<8)
#define EDP_PSR_STATUS_SENDING_TP1 (1<<4)
#define EDP_PSR_STATUS_IDLE_MASK 0xf
+#define EDP_PSR_STATUS_STATE_SHIFT 29
You ignored my review from last time, please move this where the mask
'EDP_PSR_STATUS_STATE_MASK' is defined. The rest of the file maintains
this consistently and there is no reason to change that.
