On Tue, 2018-05-22 at 14:27 +0530, vathsala nagaraju wrote:
> From: Vathsala Nagaraju <vathsala.nagaraju@xxxxxxxxx>
>
> 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.vivi@xxxxxxxxx>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@xxxxxxxxx>
> ---
> 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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
