On Fri, 2018-04-27 at 11:54 +0530, vathsala nagaraju wrote: > From: Vathsala Nagaraju <[email protected]> > > Prints live state of psr1.Extending the existing > PSR2 live state function to cover psr1. > > Tested on KBL with psr2 and psr1 panel.
Thanks for the patch, please see comments below. > > v2: rebase > > Cc: Rodrigo Vivi <[email protected]> > Cc: Dhinakaran Pandiyan <[email protected]> > > Signed-off-by: Vathsala Nagaraju <[email protected]> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 67 ++++++++++++++++++++++++--- > ---------- > drivers/gpu/drm/i915/i915_reg.h | 1 + > 2 files changed, 44 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index cb1a804..1bf2245 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2580,25 +2580,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_live_status(bool is_psr2_enabled, u32 val) I thought you'd agreed to change the function name to psr_source_status in v2. If you change the signature to static void psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m) > +{ > + if (is_psr2_enabled) { if (dev_priv->psr.psr2_enabled) { status = I915_READ(EDP_PSR2_STATUS); > + 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]; seq_printf(m, "Source status: %x [%s]", status, live_status[index]); > + } else { <snip> > @@ -2698,12 +2716,13 @@ 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); > + seq_printf(m, "EDP_SOURCE_PSR%s_STATUS: %x [%s]\n", > + dev_priv->psr.psr2_enabled ? "2" : "1", you can avoid these extra branches on psr2_enabled. And there is also too much going on in i915_edp_psr_status() . > + psr_status, > + psr_live_status(dev_priv->psr.psr2_enabled, > psr_status)); > > 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 391825a..2642b97 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4065,6 +4065,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 Please move this below EDP_PSR_STATUS_STATE_MASK > > #define EDP_PSR_PERF_CNT _MMIO(dev_priv- > >psr_mmio_base + 0x44) > #define EDP_PSR_PERF_CNT_MASK 0xffffff _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
