| Message ID | 1400199190-2424-11-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 5/16/2014 5:43 AM, Rodrigo Vivi wrote: > Now we have the active/inactive state for exit and this actually changes the > HW enable bit the status was a bit confusing for users. So let's provide > more info. > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 6636ca2..0ca9376 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1975,10 +1975,12 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > > seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support)); > seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok)); > + seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled)); > + seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active)); > > enabled = HAS_PSR(dev) && > I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; > - seq_printf(m, "Enabled: %s\n", yesno(enabled)); > + seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled)); > > if (HAS_PSR(dev)) > psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) & Please remove all references to PSR performance counter. This register is primarily meant as a debug register and its implementation is broken in the h/w. Whenever the cdclk is gated to save power, the performance counter is stopped. But when the clk is re-enabled it doesn't reset the counter. This unnecessarily confuses the end users.. When the system goes through suspend / resume cycle the performance counter most likely will transition from a non-zero value to zero.. I already received few queries from our customers related to this performance customer and they refuse to believe me when i tell them PSR is still functional when the performance counter reports 0 :-) AFAIK this register definition is missing in open source HSW B spec as well.. Thanks, Vijay >
On Mon, Jun 02, 2014 at 11:54:10PM +0530, Vijay Purushothaman wrote: > On 5/16/2014 5:43 AM, Rodrigo Vivi wrote: > >Now we have the active/inactive state for exit and this actually changes the > >HW enable bit the status was a bit confusing for users. So let's provide > >more info. > > > >Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > >--- > > drivers/gpu/drm/i915/i915_debugfs.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > >index 6636ca2..0ca9376 100644 > >--- a/drivers/gpu/drm/i915/i915_debugfs.c > >+++ b/drivers/gpu/drm/i915/i915_debugfs.c > >@@ -1975,10 +1975,12 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > > > > seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support)); > > seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok)); > >+ seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled)); > >+ seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active)); > > > > enabled = HAS_PSR(dev) && > > I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; > >- seq_printf(m, "Enabled: %s\n", yesno(enabled)); > >+ seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled)); > > > > if (HAS_PSR(dev)) > > psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) & > > Please remove all references to PSR performance counter. This register is > primarily meant as a debug register and its implementation is broken in the > h/w. Whenever the cdclk is gated to save power, the performance counter is > stopped. But when the clk is re-enabled it doesn't reset the counter. This > unnecessarily confuses the end users.. When the system goes through suspend > / resume cycle the performance counter most likely will transition from a > non-zero value to zero.. I already received few queries from our customers > related to this performance customer and they refuse to believe me when i > tell them PSR is still functional when the performance counter reports 0 :-) We expose other such perf registers and imo this is handy for debugging. Also we have a big push to expose all this perf stuff recently ... Imo we should keep this, if we can. If confused customers noodle around in debugfs without clue, maybe they shouldn't. -Daniel
On 6/3/2014 1:10 PM, Daniel Vetter wrote: > On Mon, Jun 02, 2014 at 11:54:10PM +0530, Vijay Purushothaman wrote: >> On 5/16/2014 5:43 AM, Rodrigo Vivi wrote: >>> Now we have the active/inactive state for exit and this actually changes the >>> HW enable bit the status was a bit confusing for users. So let's provide >>> more info. >>> >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> >>> --- >>> drivers/gpu/drm/i915/i915_debugfs.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >>> index 6636ca2..0ca9376 100644 >>> --- a/drivers/gpu/drm/i915/i915_debugfs.c >>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >>> @@ -1975,10 +1975,12 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) >>> >>> seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support)); >>> seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok)); >>> + seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled)); >>> + seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active)); >>> >>> enabled = HAS_PSR(dev) && >>> I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; >>> - seq_printf(m, "Enabled: %s\n", yesno(enabled)); >>> + seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled)); >>> >>> if (HAS_PSR(dev)) >>> psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) & >> >> Please remove all references to PSR performance counter. This register is >> primarily meant as a debug register and its implementation is broken in the >> h/w. Whenever the cdclk is gated to save power, the performance counter is >> stopped. But when the clk is re-enabled it doesn't reset the counter. This >> unnecessarily confuses the end users.. When the system goes through suspend >> / resume cycle the performance counter most likely will transition from a >> non-zero value to zero.. I already received few queries from our customers >> related to this performance customer and they refuse to believe me when i >> tell them PSR is still functional when the performance counter reports 0 :-) > > We expose other such perf registers and imo this is handy for debugging. > Also we have a big push to expose all this perf stuff recently ... > > Imo we should keep this, if we can. If confused customers noodle around in > debugfs without clue, maybe they shouldn't. > -Daniel In that case here is my Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> Thanks, Vijay >
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 6636ca2..0ca9376 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1975,10 +1975,12 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support)); seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok)); + seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled)); + seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active)); enabled = HAS_PSR(dev) && I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; - seq_printf(m, "Enabled: %s\n", yesno(enabled)); + seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled)); if (HAS_PSR(dev)) psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) &
Now we have the active/inactive state for exit and this actually changes the HW enable bit the status was a bit confusing for users. So let's provide more info. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)