| Message ID | 1526979431-716-1-git-send-email-vathsala.nagaraju@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2018-05-22 at 14:27 +0530, vathsala nagaraju wrote: > From: Vathsala Nagaraju <vathsala.nagaraju@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.vivi@intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@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.
On 5/23/2018 1:28 AM, Dhinakaran Pandiyan wrote: > On Tue, 2018-05-22 at 14:27 +0530, vathsala nagaraju wrote: >> From: Vathsala Nagaraju <vathsala.nagaraju@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.vivi@intel.com> >> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >> >> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@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) to print in format , source psr status %x [%s] , where %x = complete psr source register value(0x6f940) , %s = psr_status_bits [31 :28/29]. if we want handle everything as part of psr_source_status() , then we need to return register value in some pointer. if not then we read the reg and then pass it to psr_source_status which returns live status string. >> +{ >> + 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. > > >
On Wed, 2018-05-23 at 11:07 +0530, Nagaraju, Vathsala wrote: > > On 5/23/2018 1:28 AM, Dhinakaran Pandiyan wrote: > > > > On Tue, 2018-05-22 at 14:27 +0530, vathsala nagaraju wrote: > > > > > > From: Vathsala Nagaraju <vathsala.nagaraju@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.vivi@intel.com> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > > Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@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) > to print in format , source psr status %x [%s] , where %x = complete > psr > source register value(0x6f940) , %s = psr_status_bits [31 :28/29]. > if we want handle everything as part of psr_source_status() , then > we > need to return register value in some pointer. > if not then we read the reg and then pass it to psr_source_status > which > returns live status string. Pass struct seq_file *m to psr_source_status() and print the status there.
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) +{ + 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" + }; + 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", + }; + 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_PSR_STATUS); + seq_printf(m, "SOURCE_PSR_STATUS: %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 #define EDP_PSR_PERF_CNT _MMIO(dev_priv->psr_mmio_base + 0x44) #define EDP_PSR_PERF_CNT_MASK 0xffffff