* [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.
@ 2014-09-04 2:49 Rodrigo Vivi
2014-09-04 2:49 ` [PATCH 2/4] drm/i915: Remove PSR HW tracking Rodrigo Vivi
` (5 more replies)
0 siblings, 6 replies; 19+ messages in thread
From: Rodrigo Vivi @ 2014-09-04 2:49 UTC (permalink / raw)
To: intel-gfx; +Cc: Rodrigo Vivi
With Software tracking we are going to PSR sooner than we should and staying
with blank screens in many cases.
Using 2 identical frames to detect idleness is safier.
Discovered and validated with refactored igt/kms_sink_psr_crc.
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f79473b..a796831 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1813,7 +1813,7 @@ static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
struct drm_device *dev = dig_port->base.base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
uint32_t max_sleep_time = 0x1f;
- uint32_t idle_frames = 1;
+ uint32_t idle_frames = 2;
uint32_t val = 0x0;
const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
bool only_standby = false;
--
1.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 2/4] drm/i915: Remove PSR HW tracking. 2014-09-04 2:49 [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2 Rodrigo Vivi @ 2014-09-04 2:49 ` Rodrigo Vivi [not found] ` <20140904080643.GC4193@intel.com> 2014-09-04 2:49 ` [PATCH 3/4] drm/i915: Move PSR w/as to enable source Rodrigo Vivi ` (4 subsequent siblings) 5 siblings, 1 reply; 19+ messages in thread From: Rodrigo Vivi @ 2014-09-04 2:49 UTC (permalink / raw) To: intel-gfx; +Cc: Rodrigo Vivi Now that we are tracking psr states on Software side we don't need HW help anymore. So we can clean up the code a bit and avoid unecessary sets. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index a796831..9414e67 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1778,10 +1778,7 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) { struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); struct drm_device *dev = dig_port->base.base.dev; - struct drm_i915_private *dev_priv = dev->dev_private; uint32_t aux_clock_divider; - int precharge = 0x3; - int msg_size = 5; /* Header(4) + Message(1) */ bool only_standby = false; aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0); @@ -1796,15 +1793,6 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) else drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE); - - /* Setup AUX registers */ - I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND); - I915_WRITE(EDP_PSR_AUX_DATA2(dev), EDP_PSR_DPCD_NORMAL_OPERATION); - I915_WRITE(EDP_PSR_AUX_CTL(dev), - DP_AUX_CH_CTL_TIME_OUT_400us | - (msg_size << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) | - (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) | - (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT)); } static void intel_edp_psr_enable_source(struct intel_dp *intel_dp) -- 1.9.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
[parent not found: <20140904080643.GC4193@intel.com>]
* Re: [PATCH 2/4] drm/i915: Remove PSR HW tracking. [not found] ` <20140904080643.GC4193@intel.com> @ 2014-09-04 19:53 ` Rodrigo Vivi 2014-09-05 9:14 ` Ville Syrjälä 0 siblings, 1 reply; 19+ messages in thread From: Rodrigo Vivi @ 2014-09-04 19:53 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx, Rodrigo Vivi [-- Attachment #1.1: Type: text/plain, Size: 3263 bytes --] On Thu, Sep 4, 2014 at 1:06 AM, Ville Syrjälä <ville.syrjala@linux.intel.com > wrote: > On Wed, Sep 03, 2014 at 10:49:57PM -0400, Rodrigo Vivi wrote: > > Now that we are tracking psr states on Software side we don't need HW > help anymore. > > So we can clean up the code a bit and avoid unecessary sets. > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 12 ------------ > > 1 file changed, 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > > index a796831..9414e67 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1778,10 +1778,7 @@ static void intel_edp_psr_enable_sink(struct > intel_dp *intel_dp) > > { > > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > > struct drm_device *dev = dig_port->base.base.dev; > > - struct drm_i915_private *dev_priv = dev->dev_private; > > uint32_t aux_clock_divider; > > - int precharge = 0x3; > > - int msg_size = 5; /* Header(4) + Message(1) */ > > bool only_standby = false; > > > > aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0); > > @@ -1796,15 +1793,6 @@ static void intel_edp_psr_enable_sink(struct > intel_dp *intel_dp) > > else > > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, > > DP_PSR_ENABLE | > DP_PSR_MAIN_LINK_ACTIVE); > > - > > - /* Setup AUX registers */ > > - I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND); > > - I915_WRITE(EDP_PSR_AUX_DATA2(dev), EDP_PSR_DPCD_NORMAL_OPERATION); > > - I915_WRITE(EDP_PSR_AUX_CTL(dev), > > - DP_AUX_CH_CTL_TIME_OUT_400us | > > - (msg_size << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) | > > - (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) | > > - (aux_clock_divider << > DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT)); > > As I said I blieve FBC is the one that does the hw tracking. I think the > hardware will still do the entry/exit (including automagic link > re-train) but now you've not told it how to wake up the sink. So I don't > understand how this can even work. > Per spec we have to program aux "registers for HW assist PSR Exit Support." With the exit being made on SW side we don't need HW help anymore. Same reason why this works with VLV. But there is no way to claim full control. So automagically triggers still exist. So I couldn't remove the W/As that mask events on srd_debug. :( But withouth these aux sets we are fine and it just works. > > > } > > > > static void intel_edp_psr_enable_source(struct intel_dp *intel_dp) > > -- > > 1.9.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > -- Rodrigo Vivi Blog: http://blog.vivi.eng.br [-- Attachment #1.2: Type: text/html, Size: 4846 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] drm/i915: Remove PSR HW tracking. 2014-09-04 19:53 ` Rodrigo Vivi @ 2014-09-05 9:14 ` Ville Syrjälä 0 siblings, 0 replies; 19+ messages in thread From: Ville Syrjälä @ 2014-09-05 9:14 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx, Rodrigo Vivi On Thu, Sep 04, 2014 at 12:53:39PM -0700, Rodrigo Vivi wrote: > On Thu, Sep 4, 2014 at 1:06 AM, Ville Syrjälä <ville.syrjala@linux.intel.com > > wrote: > > > On Wed, Sep 03, 2014 at 10:49:57PM -0400, Rodrigo Vivi wrote: > > > Now that we are tracking psr states on Software side we don't need HW > > help anymore. > > > So we can clean up the code a bit and avoid unecessary sets. > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 12 ------------ > > > 1 file changed, 12 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > > index a796831..9414e67 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -1778,10 +1778,7 @@ static void intel_edp_psr_enable_sink(struct > > intel_dp *intel_dp) > > > { > > > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > > > struct drm_device *dev = dig_port->base.base.dev; > > > - struct drm_i915_private *dev_priv = dev->dev_private; > > > uint32_t aux_clock_divider; > > > - int precharge = 0x3; > > > - int msg_size = 5; /* Header(4) + Message(1) */ > > > bool only_standby = false; > > > > > > aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0); > > > @@ -1796,15 +1793,6 @@ static void intel_edp_psr_enable_sink(struct > > intel_dp *intel_dp) > > > else > > > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, > > > DP_PSR_ENABLE | > > DP_PSR_MAIN_LINK_ACTIVE); > > > - > > > - /* Setup AUX registers */ > > > - I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND); > > > - I915_WRITE(EDP_PSR_AUX_DATA2(dev), EDP_PSR_DPCD_NORMAL_OPERATION); > > > - I915_WRITE(EDP_PSR_AUX_CTL(dev), > > > - DP_AUX_CH_CTL_TIME_OUT_400us | > > > - (msg_size << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) | > > > - (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) | > > > - (aux_clock_divider << > > DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT)); > > > > As I said I blieve FBC is the one that does the hw tracking. I think the > > hardware will still do the entry/exit (including automagic link > > re-train) but now you've not told it how to wake up the sink. So I don't > > understand how this can even work. > > > > Per spec we have to program aux "registers for HW assist PSR Exit Support." And how do you exit w/o HW assist exactly? You never do the DPCD write manually so I don't see how it can work. > > With the exit being made on SW side we don't need HW help anymore. > Same reason why this works with VLV. > > But there is no way to claim full control. So automagically triggers still > exist. > So I couldn't remove the W/As that mask events on srd_debug. :( > > But withouth these aux sets we are fine and it just works. Looks like these magic numbers are in fact just the SET_POWER(D0) DPCD write. Might be good to clarify this with Art, but I think what is happening is that you're using link standby, so just the PSR SDP sent inline on the main link is enough to exit PSR. But if you'd use the main link off mode then you'd need this AUX stuff. We should probably kill the magic numbers here and just assemble the AUX data like we would do for normal AUX. At least that way people would understand what it's meant to do from just reading the code. Right now it looks like black magic and the define names don't really help either. > > > > > > > } > > > > > > static void intel_edp_psr_enable_source(struct intel_dp *intel_dp) > > > -- > > > 1.9.3 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > Rodrigo Vivi > Blog: http://blog.vivi.eng.br -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/4] drm/i915: Move PSR w/as to enable source. 2014-09-04 2:49 [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2 Rodrigo Vivi 2014-09-04 2:49 ` [PATCH 2/4] drm/i915: Remove PSR HW tracking Rodrigo Vivi @ 2014-09-04 2:49 ` Rodrigo Vivi 2014-09-04 2:49 ` [PATCH 4/4] drm/i915: Rename psr setup vsc function and set it always Rodrigo Vivi ` (3 subsequent siblings) 5 siblings, 0 replies; 19+ messages in thread From: Rodrigo Vivi @ 2014-09-04 2:49 UTC (permalink / raw) To: intel-gfx; +Cc: Rodrigo Vivi Lets reorganize stuff and make sure these W/As are always set. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 9414e67..a04b69f 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1757,8 +1757,6 @@ static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, static void intel_edp_psr_setup(struct intel_dp *intel_dp) { - struct drm_device *dev = intel_dp_to_dev(intel_dp); - struct drm_i915_private *dev_priv = dev->dev_private; struct edp_vsc_psr psr_vsc; /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ @@ -1768,10 +1766,6 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp) psr_vsc.sdp_header.HB2 = 0x2; psr_vsc.sdp_header.HB3 = 0x8; intel_edp_psr_write_vsc(intel_dp, &psr_vsc); - - /* Avoid continuous PSR exit by masking memup and hpd */ - I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | - EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); } static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) @@ -1806,6 +1800,10 @@ static void intel_edp_psr_enable_source(struct intel_dp *intel_dp) const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; bool only_standby = false; + /* Avoid continuous PSR exit by masking memup and hpd */ + I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | + EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); + if (IS_BROADWELL(dev) && dig_port->port != PORT_A) only_standby = true; -- 1.9.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/4] drm/i915: Rename psr setup vsc function and set it always. 2014-09-04 2:49 [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2 Rodrigo Vivi 2014-09-04 2:49 ` [PATCH 2/4] drm/i915: Remove PSR HW tracking Rodrigo Vivi 2014-09-04 2:49 ` [PATCH 3/4] drm/i915: Move PSR w/as to enable source Rodrigo Vivi @ 2014-09-04 2:49 ` Rodrigo Vivi [not found] ` <20140904075516.GB4193@intel.com> ` (2 subsequent siblings) 5 siblings, 0 replies; 19+ messages in thread From: Rodrigo Vivi @ 2014-09-04 2:49 UTC (permalink / raw) To: intel-gfx; +Cc: Rodrigo Vivi Let's always set VSC header. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index a04b69f..de24e46 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1755,7 +1755,7 @@ static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, POSTING_READ(ctl_reg); } -static void intel_edp_psr_setup(struct intel_dp *intel_dp) +static void intel_edp_psr_setup_vsc(struct intel_dp *intel_dp) { struct edp_vsc_psr psr_vsc; @@ -1877,6 +1877,8 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp) WARN_ON(dev_priv->psr.active); lockdep_assert_held(&dev_priv->psr.lock); + intel_edp_psr_setup_vsc(intel_dp); + /* Enable PSR on the panel */ intel_edp_psr_enable_sink(intel_dp); @@ -1910,9 +1912,6 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp) dev_priv->psr.busy_frontbuffer_bits = 0; - /* Setup PSR once */ - intel_edp_psr_setup(intel_dp); - if (intel_edp_psr_match_conditions(intel_dp)) dev_priv->psr.enabled = intel_dp; mutex_unlock(&dev_priv->psr.lock); -- 1.9.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
[parent not found: <20140904075516.GB4193@intel.com>]
* Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2. [not found] ` <20140904075516.GB4193@intel.com> @ 2014-09-04 18:18 ` Rodrigo Vivi [not found] ` <20140904092916.GF15520@phenom.ffwll.local> 1 sibling, 0 replies; 19+ messages in thread From: Rodrigo Vivi @ 2014-09-04 18:18 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx, Rodrigo Vivi [-- Attachment #1.1: Type: text/plain, Size: 2097 bytes --] On Thu, Sep 4, 2014 at 12:55 AM, Ville Syrjälä < ville.syrjala@linux.intel.com> wrote: > On Wed, Sep 03, 2014 at 10:49:56PM -0400, Rodrigo Vivi wrote: > > With Software tracking we are going to PSR sooner than we should and > staying > > with blank screens in many cases. > > > > Using 2 identical frames to detect idleness is safier. > > This idle frame detection still depends of FBC right? > not sure. and fbc is disabled anyway here on my tests. > I believe if we want to go for full sw tracking on HSW/BDW we need to > use the debug register to force PSR entry/exit. > I tried this many times in different ways but never had success > > > > > Discovered and validated with refactored igt/kms_sink_psr_crc. > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > > index f79473b..a796831 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1813,7 +1813,7 @@ static void intel_edp_psr_enable_source(struct > intel_dp *intel_dp) > > struct drm_device *dev = dig_port->base.base.dev; > > struct drm_i915_private *dev_priv = dev->dev_private; > > uint32_t max_sleep_time = 0x1f; > > - uint32_t idle_frames = 1; > > + uint32_t idle_frames = 2; > > uint32_t val = 0x0; > > const uint32_t link_entry_time = > EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > > bool only_standby = false; > > -- > > 1.9.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > -- Rodrigo Vivi Blog: http://blog.vivi.eng.br [-- Attachment #1.2: Type: text/html, Size: 3848 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <20140904092916.GF15520@phenom.ffwll.local>]
[parent not found: <20140904100427.GD4193@intel.com>]
[parent not found: <20140904101819.GE4193@intel.com>]
* Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2. [not found] ` <20140904101819.GE4193@intel.com> @ 2014-09-04 11:04 ` Daniel Vetter 2014-09-04 18:26 ` Rodrigo Vivi 0 siblings, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2014-09-04 11:04 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx, Rodrigo Vivi On Thu, Sep 04, 2014 at 01:18:19PM +0300, Ville Syrjälä wrote: > On Thu, Sep 04, 2014 at 01:04:27PM +0300, Ville Syrjälä wrote: > > On Thu, Sep 04, 2014 at 11:29:16AM +0200, Daniel Vetter wrote: > > > On Thu, Sep 04, 2014 at 10:55:16AM +0300, Ville Syrjälä wrote: > > > > On Wed, Sep 03, 2014 at 10:49:56PM -0400, Rodrigo Vivi wrote: > > > > > With Software tracking we are going to PSR sooner than we should and staying > > > > > with blank screens in many cases. > > > > > > > > > > Using 2 identical frames to detect idleness is safier. > > > > > > > > This idle frame detection still depends of FBC right? > > > > > > > > I believe if we want to go for full sw tracking on HSW/BDW we need to > > > > use the debug register to force PSR entry/exit. > > > > > > Currently the sw tracking relies upon 1 additional full upload happening > > > after the flush, which hopefully should magically happen if we have just 1 > > > idle frame. > > > > > > If we'd completely switch to sw tracking we'd need to set up a vblank > > > worker to disable psr after the next vblank, which would comlicate the > > > code I think. > > > > vlv/chv have no hw tracking so if the current sw tracking can't deal > > with that then it would seem to need more work. > > Hmm. Actually they seem to have a hw timer mode where we can program the > number of idle frames. I think idle here means "since the last plane > register frobbing" as there's no real modification tracking ala. FBC. > So maybe it can work roughly the same way as HSW in that regard. Essentially the primitive the current code needs (modulo bugs, which seem to still be) is to "upload one more full frame, then enter psr". If a lot of platforms can't do that themselves I guess we could wrap some helpers for them. But if there's some real sw tracking bug still, and Rodrigo's patch looks like this is still the case, we need to fix that ofc. -Daniel > > > > > > -Daniel > > > > > > > > > > > > > > > > > Discovered and validated with refactored igt/kms_sink_psr_crc. > > > > > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_dp.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > > > index f79473b..a796831 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > > @@ -1813,7 +1813,7 @@ static void intel_edp_psr_enable_source(struct intel_dp *intel_dp) > > > > > struct drm_device *dev = dig_port->base.base.dev; > > > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > > uint32_t max_sleep_time = 0x1f; > > > > > - uint32_t idle_frames = 1; > > > > > + uint32_t idle_frames = 2; > > > > > uint32_t val = 0x0; > > > > > const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > > > > > bool only_standby = false; > > > > > -- > > > > > 1.9.3 > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel OTC > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > > > -- > > Ville Syrjälä > > Intel OTC > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2. 2014-09-04 11:04 ` Daniel Vetter @ 2014-09-04 18:26 ` Rodrigo Vivi 0 siblings, 0 replies; 19+ messages in thread From: Rodrigo Vivi @ 2014-09-04 18:26 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, Rodrigo Vivi [-- Attachment #1.1: Type: text/plain, Size: 4691 bytes --] On Thu, Sep 4, 2014 at 4:04 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Sep 04, 2014 at 01:18:19PM +0300, Ville Syrjälä wrote: > > On Thu, Sep 04, 2014 at 01:04:27PM +0300, Ville Syrjälä wrote: > > > On Thu, Sep 04, 2014 at 11:29:16AM +0200, Daniel Vetter wrote: > > > > On Thu, Sep 04, 2014 at 10:55:16AM +0300, Ville Syrjälä wrote: > > > > > On Wed, Sep 03, 2014 at 10:49:56PM -0400, Rodrigo Vivi wrote: > > > > > > With Software tracking we are going to PSR sooner than we should > and staying > > > > > > with blank screens in many cases. > > > > > > > > > > > > Using 2 identical frames to detect idleness is safier. > > > > > > > > > > This idle frame detection still depends of FBC right? > > > > > > > > > > I believe if we want to go for full sw tracking on HSW/BDW we need > to > > > > > use the debug register to force PSR entry/exit. > > > > > > > > Currently the sw tracking relies upon 1 additional full upload > happening > > > > after the flush, which hopefully should magically happen if we have > just 1 > > > > idle frame. > > > > > > > > If we'd completely switch to sw tracking we'd need to set up a vblank > > > > worker to disable psr after the next vblank, which would comlicate > the > > > > code I think. > > > > > > vlv/chv have no hw tracking so if the current sw tracking can't deal > > > with that then it would seem to need more work. > > > > Hmm. Actually they seem to have a hw timer mode where we can program the > > number of idle frames. I think idle here means "since the last plane > > register frobbing" as there's no real modification tracking ala. FBC. > > So maybe it can work roughly the same way as HSW in that regard. > > Essentially the primitive the current code needs (modulo bugs, which seem > to still be) is to "upload one more full frame, then enter psr". If a lot > of platforms can't do that themselves I guess we could wrap some helpers > for them. > > But if there's some real sw tracking bug still, and Rodrigo's patch looks > like this is still the case, we need to fix that ofc. > Yeah, I agree. But I'm afraid I didn't fully get your idea. What do you have in mind? > -Daniel > > > > > > > > > > -Daniel > > > > > > > > > > > > > > > > > > > > > Discovered and validated with refactored igt/kms_sink_psr_crc. > > > > > > > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/intel_dp.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > > > > > > index f79473b..a796831 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > > > @@ -1813,7 +1813,7 @@ static void > intel_edp_psr_enable_source(struct intel_dp *intel_dp) > > > > > > struct drm_device *dev = dig_port->base.base.dev; > > > > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > > > uint32_t max_sleep_time = 0x1f; > > > > > > - uint32_t idle_frames = 1; > > > > > > + uint32_t idle_frames = 2; > > > > > > uint32_t val = 0x0; > > > > > > const uint32_t link_entry_time = > EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > > > > > > bool only_standby = false; > > > > > > -- > > > > > > 1.9.3 > > > > > > > > > > > > _______________________________________________ > > > > > > Intel-gfx mailing list > > > > > > Intel-gfx@lists.freedesktop.org > > > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > -- > > > > > Ville Syrjälä > > > > > Intel OTC > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > -- > > > > Daniel Vetter > > > > Software Engineer, Intel Corporation > > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > > > > > -- > > > Ville Syrjälä > > > Intel OTC > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > -- Rodrigo Vivi Blog: http://blog.vivi.eng.br [-- Attachment #1.2: Type: text/html, Size: 7448 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2. [not found] ` <20140904100427.GD4193@intel.com> [not found] ` <20140904101819.GE4193@intel.com> @ 2014-09-04 18:22 ` Rodrigo Vivi 1 sibling, 0 replies; 19+ messages in thread From: Rodrigo Vivi @ 2014-09-04 18:22 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx, Rodrigo Vivi [-- Attachment #1.1: Type: text/plain, Size: 3273 bytes --] On Thu, Sep 4, 2014 at 3:04 AM, Ville Syrjälä <ville.syrjala@linux.intel.com > wrote: > On Thu, Sep 04, 2014 at 11:29:16AM +0200, Daniel Vetter wrote: > > On Thu, Sep 04, 2014 at 10:55:16AM +0300, Ville Syrjälä wrote: > > > On Wed, Sep 03, 2014 at 10:49:56PM -0400, Rodrigo Vivi wrote: > > > > With Software tracking we are going to PSR sooner than we should and > staying > > > > with blank screens in many cases. > > > > > > > > Using 2 identical frames to detect idleness is safier. > > > > > > This idle frame detection still depends of FBC right? > > > > > > I believe if we want to go for full sw tracking on HSW/BDW we need to > > > use the debug register to force PSR entry/exit. > > > > Currently the sw tracking relies upon 1 additional full upload happening > > after the flush, which hopefully should magically happen if we have just > 1 > > idle frame. > > > > If we'd completely switch to sw tracking we'd need to set up a vblank > > worker to disable psr after the next vblank, which would comlicate the > > code I think. > > vlv/chv have no hw tracking so if the current sw tracking can't deal > with that then it would seem to need more work. > the sw tracking on vlv works well. The only issue is that the force depends on a dpms_on call and I was facing strange hangs when going blank. But overal it is possible. > > > -Daniel > > > > > > > > > > > > > Discovered and validated with refactored igt/kms_sink_psr_crc. > > > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > > > > index f79473b..a796831 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -1813,7 +1813,7 @@ static void intel_edp_psr_enable_source(struct > intel_dp *intel_dp) > > > > struct drm_device *dev = dig_port->base.base.dev; > > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > uint32_t max_sleep_time = 0x1f; > > > > - uint32_t idle_frames = 1; > > > > + uint32_t idle_frames = 2; > > > > uint32_t val = 0x0; > > > > const uint32_t link_entry_time = > EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > > > > bool only_standby = false; > > > > -- > > > > 1.9.3 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Ville Syrjälä > > > Intel OTC > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > -- Rodrigo Vivi Blog: http://blog.vivi.eng.br [-- Attachment #1.2: Type: text/html, Size: 5118 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <87wq9jg62e.fsf@intel.com>]
* Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2. [not found] ` <87wq9jg62e.fsf@intel.com> @ 2014-09-04 18:18 ` Rodrigo Vivi 0 siblings, 0 replies; 19+ messages in thread From: Rodrigo Vivi @ 2014-09-04 18:18 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx, Rodrigo Vivi [-- Attachment #1.1: Type: text/plain, Size: 1807 bytes --] On Thu, Sep 4, 2014 at 1:09 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Thu, 04 Sep 2014, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > With Software tracking we are going to PSR sooner than we should and > staying > > with blank screens in many cases. > > > > Using 2 identical frames to detect idleness is safier. > > > > Discovered and validated with refactored igt/kms_sink_psr_crc. > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > > index f79473b..a796831 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1813,7 +1813,7 @@ static void intel_edp_psr_enable_source(struct > intel_dp *intel_dp) > > struct drm_device *dev = dig_port->base.base.dev; > > struct drm_i915_private *dev_priv = dev->dev_private; > > uint32_t max_sleep_time = 0x1f; > > - uint32_t idle_frames = 1; > > + uint32_t idle_frames = 2; > > IMHO this calls for a comment in the code. > Agree > > BR, > Jani. > > > uint32_t val = 0x0; > > const uint32_t link_entry_time = > EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > > bool only_standby = false; > > -- > > 1.9.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > -- Rodrigo Vivi Blog: http://blog.vivi.eng.br [-- Attachment #1.2: Type: text/html, Size: 3189 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <20140904092253.GD15520@phenom.ffwll.local>]
* Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2. [not found] ` <20140904092253.GD15520@phenom.ffwll.local> @ 2014-09-04 18:20 ` Rodrigo Vivi 2014-09-04 19:05 ` Daniel Vetter 0 siblings, 1 reply; 19+ messages in thread From: Rodrigo Vivi @ 2014-09-04 18:20 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, Rodrigo Vivi [-- Attachment #1.1: Type: text/plain, Size: 4562 bytes --] On Thu, Sep 4, 2014 at 2:22 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Sep 03, 2014 at 10:49:56PM -0400, Rodrigo Vivi wrote: > > With Software tracking we are going to PSR sooner than we should and > staying > > with blank screens in many cases. > > > > Using 2 identical frames to detect idleness is safier. > > > > Discovered and validated with refactored igt/kms_sink_psr_crc. > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > > index f79473b..a796831 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1813,7 +1813,7 @@ static void intel_edp_psr_enable_source(struct > intel_dp *intel_dp) > > struct drm_device *dev = dig_port->base.base.dev; > > struct drm_i915_private *dev_priv = dev->dev_private; > > uint32_t max_sleep_time = 0x1f; > > - uint32_t idle_frames = 1; > > + uint32_t idle_frames = 2; > > Hm, that sounds like we allow psr before it is really possible. And we > still delay the actual re-enable work by 100ms, so it very much looks like > something is broken here. > Agree. leaving idle_frame = 1 and increasing time to 600ms also works. 500ms fails. So I thought we had an issue with frontbuffer tracking but couldn find any. So this idle_frame = 2 was the most clear way I could find for now. > Which exact subcases do fail? > Here are the results with idle_frame=1 IGT-Version: 1.7-gdee8754 (x86_64) (Linux: 3.17.0-rc2+ x86_64) Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: Failed assertion: strcmp(crc, CRC_BLACK) != 0 Subtest primary_page_flip: FAIL Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: Failed assertion: strcmp(crc, CRC_BLACK) != 0 Subtest primary_mmap_gtt: FAIL Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: Failed assertion: strcmp(crc, CRC_BLACK) != 0 Subtest primary_mmap_gtt_waiting: FAIL Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: Failed assertion: strcmp(crc, CRC_BLACK) != 0 Subtest primary_mmap_cpu: FAIL Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: Failed assertion: strcmp(crc, CRC_BLACK) != 0 Subtest primary_blt: FAIL Subtest primary_render: SUCCESS Subtest sprite_mmap_gtt: SUCCESS Waiting 10s... Subtest sprite_mmap_gtt_waiting: SUCCESS Subtest sprite_mmap_cpu: SUCCESS Subtest sprite_blt: SUCCESS Subtest sprite_render: SUCCESS Subtest sprite_plane_move: SUCCESS Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: Failed assertion: strcmp(crc, CRC_BLACK) != 0 Subtest sprite_plane_onoff: FAIL Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: Failed assertion: strcmp(crc, CRC_BLACK) != 0 Subtest cursor_mmap_gtt: FAIL Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: Failed assertion: strcmp(crc, CRC_BLACK) != 0 Subtest cursor_mmap_gtt_waiting: FAIL Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: Failed assertion: strcmp(crc, CRC_BLACK) != 0 Subtest cursor_mmap_cpu: FAIL Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: Failed assertion: strcmp(crc, CRC_BLACK) != 0 Subtest cursor_blt: FAIL Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: Failed assertion: strcmp(crc, CRC_BLACK) != 0 Subtest cursor_render: FAIL Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: Failed assertion: strcmp(crc, CRC_BLACK) != 0 Subtest cursor_plane_move: FAIL Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: Failed assertion: strcmp(crc, CRC_BLACK) != 0 Subtest cursor_plane_onoff: FAIL > -Daniel > > > uint32_t val = 0x0; > > const uint32_t link_entry_time = > EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > > bool only_standby = false; > > -- > > 1.9.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > -- Rodrigo Vivi Blog: http://blog.vivi.eng.br [-- Attachment #1.2: Type: text/html, Size: 6921 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2. 2014-09-04 18:20 ` Rodrigo Vivi @ 2014-09-04 19:05 ` Daniel Vetter 2014-09-04 19:16 ` Rodrigo Vivi 0 siblings, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2014-09-04 19:05 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx, Rodrigo Vivi On Thu, Sep 4, 2014 at 8:20 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote: > On Thu, Sep 4, 2014 at 2:22 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> >> On Wed, Sep 03, 2014 at 10:49:56PM -0400, Rodrigo Vivi wrote: >> > With Software tracking we are going to PSR sooner than we should and >> > staying >> > with blank screens in many cases. >> > >> > Using 2 identical frames to detect idleness is safier. >> > >> > Discovered and validated with refactored igt/kms_sink_psr_crc. >> > >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_dp.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c >> > b/drivers/gpu/drm/i915/intel_dp.c >> > index f79473b..a796831 100644 >> > --- a/drivers/gpu/drm/i915/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/intel_dp.c >> > @@ -1813,7 +1813,7 @@ static void intel_edp_psr_enable_source(struct >> > intel_dp *intel_dp) >> > struct drm_device *dev = dig_port->base.base.dev; >> > struct drm_i915_private *dev_priv = dev->dev_private; >> > uint32_t max_sleep_time = 0x1f; >> > - uint32_t idle_frames = 1; >> > + uint32_t idle_frames = 2; >> >> Hm, that sounds like we allow psr before it is really possible. And we >> still delay the actual re-enable work by 100ms, so it very much looks like >> something is broken here. > > > > Agree. leaving idle_frame = 1 and increasing time to 600ms also works. > 500ms fails. > So I thought we had an issue with frontbuffer tracking but couldn find any. > So this idle_frame = 2 was the most clear way I could find for now. 600ms is roughly 40 frames ... that's a lot. >> >> Which exact subcases do fail? > > > Here are the results with idle_frame=1 > IGT-Version: 1.7-gdee8754 (x86_64) (Linux: 3.17.0-rc2+ x86_64) > Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > Subtest primary_page_flip: FAIL > Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > Subtest primary_mmap_gtt: FAIL > Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > Subtest primary_mmap_gtt_waiting: FAIL > Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > Subtest primary_mmap_cpu: FAIL > Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > Subtest primary_blt: FAIL > Subtest primary_render: SUCCESS > Subtest sprite_mmap_gtt: SUCCESS > Waiting 10s... > Subtest sprite_mmap_gtt_waiting: SUCCESS > Subtest sprite_mmap_cpu: SUCCESS > Subtest sprite_blt: SUCCESS > Subtest sprite_render: SUCCESS > Subtest sprite_plane_move: SUCCESS > Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > Subtest sprite_plane_onoff: FAIL > Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > Subtest cursor_mmap_gtt: FAIL > Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > Subtest cursor_mmap_gtt_waiting: FAIL > Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > Subtest cursor_mmap_cpu: FAIL > Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > Subtest cursor_blt: FAIL > Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > Subtest cursor_render: FAIL > Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > Subtest cursor_plane_move: FAIL > Test assertion failure function get_sink_crc, file kms_psr_sink_crc.c:271: > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > Subtest cursor_plane_onoff: FAIL Hm, I don't see a pattern at all. And that sprites seem to work best also looks funky. Are the results stable when you randomize them (piglit can do that for you)? Can you add a residency checks in the testcase (i.e. before the update and after the update) so that we know it's really psr and not something else funny going on? I assume this is on bdw, any difference in results on bdw? I have no idea tbh what's going on here, but it looks strange. At best my guess would be that psr exit isn't reliable ... We could check that by making sure that in the middle of the delayed gtt mmap (when psr should be disabled) psr residency is 0. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2. 2014-09-04 19:05 ` Daniel Vetter @ 2014-09-04 19:16 ` Rodrigo Vivi 2014-09-04 20:20 ` Daniel Vetter 0 siblings, 1 reply; 19+ messages in thread From: Rodrigo Vivi @ 2014-09-04 19:16 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, Rodrigo Vivi [-- Attachment #1.1: Type: text/plain, Size: 6020 bytes --] On Thu, Sep 4, 2014 at 12:05 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Sep 4, 2014 at 8:20 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> > wrote: > > On Thu, Sep 4, 2014 at 2:22 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > >> > >> On Wed, Sep 03, 2014 at 10:49:56PM -0400, Rodrigo Vivi wrote: > >> > With Software tracking we are going to PSR sooner than we should and > >> > staying > >> > with blank screens in many cases. > >> > > >> > Using 2 identical frames to detect idleness is safier. > >> > > >> > Discovered and validated with refactored igt/kms_sink_psr_crc. > >> > > >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/intel_dp.c | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c > >> > b/drivers/gpu/drm/i915/intel_dp.c > >> > index f79473b..a796831 100644 > >> > --- a/drivers/gpu/drm/i915/intel_dp.c > >> > +++ b/drivers/gpu/drm/i915/intel_dp.c > >> > @@ -1813,7 +1813,7 @@ static void intel_edp_psr_enable_source(struct > >> > intel_dp *intel_dp) > >> > struct drm_device *dev = dig_port->base.base.dev; > >> > struct drm_i915_private *dev_priv = dev->dev_private; > >> > uint32_t max_sleep_time = 0x1f; > >> > - uint32_t idle_frames = 1; > >> > + uint32_t idle_frames = 2; > >> > >> Hm, that sounds like we allow psr before it is really possible. And we > >> still delay the actual re-enable work by 100ms, so it very much looks > like > >> something is broken here. > > > > > > > > Agree. leaving idle_frame = 1 and increasing time to 600ms also works. > > 500ms fails. > > So I thought we had an issue with frontbuffer tracking but couldn find > any. > > So this idle_frame = 2 was the most clear way I could find for now. > > 600ms is roughly 40 frames ... that's a lot. > > >> > >> Which exact subcases do fail? > > > > > > Here are the results with idle_frame=1 > > IGT-Version: 1.7-gdee8754 (x86_64) (Linux: 3.17.0-rc2+ x86_64) > > Test assertion failure function get_sink_crc, file > kms_psr_sink_crc.c:271: > > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > > Subtest primary_page_flip: FAIL > > Test assertion failure function get_sink_crc, file > kms_psr_sink_crc.c:271: > > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > > Subtest primary_mmap_gtt: FAIL > > Test assertion failure function get_sink_crc, file > kms_psr_sink_crc.c:271: > > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > > Subtest primary_mmap_gtt_waiting: FAIL > > Test assertion failure function get_sink_crc, file > kms_psr_sink_crc.c:271: > > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > > Subtest primary_mmap_cpu: FAIL > > Test assertion failure function get_sink_crc, file > kms_psr_sink_crc.c:271: > > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > > Subtest primary_blt: FAIL > > Subtest primary_render: SUCCESS > > Subtest sprite_mmap_gtt: SUCCESS > > Waiting 10s... > > Subtest sprite_mmap_gtt_waiting: SUCCESS > > Subtest sprite_mmap_cpu: SUCCESS > > Subtest sprite_blt: SUCCESS > > Subtest sprite_render: SUCCESS > > Subtest sprite_plane_move: SUCCESS > > Test assertion failure function get_sink_crc, file > kms_psr_sink_crc.c:271: > > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > > Subtest sprite_plane_onoff: FAIL > > Test assertion failure function get_sink_crc, file > kms_psr_sink_crc.c:271: > > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > > Subtest cursor_mmap_gtt: FAIL > > Test assertion failure function get_sink_crc, file > kms_psr_sink_crc.c:271: > > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > > Subtest cursor_mmap_gtt_waiting: FAIL > > Test assertion failure function get_sink_crc, file > kms_psr_sink_crc.c:271: > > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > > Subtest cursor_mmap_cpu: FAIL > > Test assertion failure function get_sink_crc, file > kms_psr_sink_crc.c:271: > > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > > Subtest cursor_blt: FAIL > > Test assertion failure function get_sink_crc, file > kms_psr_sink_crc.c:271: > > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > > Subtest cursor_render: FAIL > > Test assertion failure function get_sink_crc, file > kms_psr_sink_crc.c:271: > > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > > Subtest cursor_plane_move: FAIL > > Test assertion failure function get_sink_crc, file > kms_psr_sink_crc.c:271: > > Failed assertion: strcmp(crc, CRC_BLACK) != 0 > > Subtest cursor_plane_onoff: FAIL > > Hm, I don't see a pattern at all. And that sprites seem to work best > also looks funky. Are the results stable when you randomize them > (piglit can do that for you)? Can you add a residency checks in the > testcase (i.e. before the update and after the update) so that we know > it's really psr and not something else funny going on? I assume this > is on bdw, any difference in results on bdw? > Yes, this is really strange. I couldn't find a pattern at all as well. Do you have any example of piglit helping randomization? The bad things with residency check is that vlv/chv doesn't have performance counters. So this test would just work on hsw/bdw/ I did this work on a HSW. Test on BDW is one of tests I should do next here. > > I have no idea tbh what's going on here, but it looks strange. At best > my guess would be that psr exit isn't reliable ... We could check that > by making sure that in the middle of the delayed gtt mmap (when psr > should be disabled) psr residency is 0. > Yeah, this wasn't designed to exit and get back so often. Also the disable sequece by spec says we have to disable, wait for a vblank, than disable vsc. I tried that and the results got even worse. Another idea was to use srd_debug register to force exit and normal opperation instead of disabled and reenable constantly as Ville also suggested, but my tests with this approach didn't go anywhere... > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > -- Rodrigo Vivi Blog: http://blog.vivi.eng.br [-- Attachment #1.2: Type: text/html, Size: 8142 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2. 2014-09-04 19:16 ` Rodrigo Vivi @ 2014-09-04 20:20 ` Daniel Vetter 2014-09-05 0:40 ` Rodrigo Vivi 0 siblings, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2014-09-04 20:20 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx, Rodrigo Vivi On Thu, Sep 4, 2014 at 9:16 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote: >> Hm, I don't see a pattern at all. And that sprites seem to work best >> also looks funky. Are the results stable when you randomize them >> (piglit can do that for you)? Can you add a residency checks in the >> testcase (i.e. before the update and after the update) so that we know >> it's really psr and not something else funny going on? I assume this >> is on bdw, any difference in results on bdw? > > Yes, this is really strange. I couldn't find a pattern at all as well. > Do you have any example of piglit helping randomization? > > The bad things with residency check is that vlv/chv doesn't have performance > counters. > So this test would just work on hsw/bdw/ > > I did this work on a HSW. Test on BDW is one of tests I should do next here. There's no shuffling in piglit unfortunately. But piglit is good for comparing results, so I wonder whether they are stable or change. And hsw is even more suprising since sprite tests pass more often in your example, but sprite also has now hw invalidate on hsw. Confusing. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2. 2014-09-04 20:20 ` Daniel Vetter @ 2014-09-05 0:40 ` Rodrigo Vivi 2014-09-05 8:37 ` Daniel Vetter 0 siblings, 1 reply; 19+ messages in thread From: Rodrigo Vivi @ 2014-09-05 0:40 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, Rodrigo Vivi [-- Attachment #1.1: Type: text/plain, Size: 3756 bytes --] Here goes results on BDW with pure today's nightly (with idle_frame=1) # First run IGT-Version: 1.7-gd4b43f0 (x86_64) (Linux: 3.17.0-rc2+ x86_64) Subtest primary_page_flip: SUCCESS Subtest primary_mmap_gtt: SUCCESS Test assertion failure function test_crc, file kms_psr_sink_crc.c:307: Failed assertion: strcmp(ref_crc, CRC_GREEN) != 0 Subtest primary_mmap_gtt_waiting: FAIL Subtest primary_mmap_cpu: SUCCESS Subtest primary_blt: SUCCESS Subtest primary_render: SUCCESS Subtest sprite_mmap_gtt: SUCCESS Waiting 10s... Subtest sprite_mmap_gtt_waiting: SUCCESS Subtest sprite_mmap_cpu: SUCCESS Subtest sprite_blt: SUCCESS Subtest sprite_render: SUCCESS Subtest sprite_plane_move: SUCCESS Subtest sprite_plane_onoff: SUCCESS Subtest cursor_mmap_gtt: SUCCESS Waiting 10s... Subtest cursor_mmap_gtt_waiting: SUCCESS Subtest cursor_mmap_cpu: SUCCESS Subtest cursor_blt: SUCCESS Subtest cursor_render: SUCCESS Subtest cursor_plane_move: SUCCESS Subtest cursor_plane_onoff: SUCCESS #second run: IGT-Version: 1.7-gd4b43f0 (x86_64) (Linux: 3.17.0-rc2+ x86_64) Subtest primary_page_flip: SUCCESS Subtest primary_mmap_gtt: SUCCESS Waiting 10s... Subtest primary_mmap_gtt_waiting: SUCCESS Subtest primary_mmap_cpu: SUCCESS Subtest primary_blt: SUCCESS Subtest primary_render: SUCCESS Subtest sprite_mmap_gtt: SUCCESS Waiting 10s... Subtest sprite_mmap_gtt_waiting: SUCCESS Subtest sprite_mmap_cpu: SUCCESS Subtest sprite_blt: SUCCESS Test assertion failure function test_crc, file kms_psr_sink_crc.c:307: Failed assertion: strcmp(ref_crc, CRC_GREEN) != 0 Subtest sprite_render: FAIL Subtest sprite_plane_move: SUCCESS Subtest sprite_plane_onoff: SUCCESS Subtest cursor_mmap_gtt: SUCCESS Waiting 10s... Subtest cursor_mmap_gtt_waiting: SUCCESS Subtest cursor_mmap_cpu: SUCCESS Subtest cursor_blt: SUCCESS Subtest cursor_render: SUCCESS Subtest cursor_plane_move: SUCCESS Subtest cursor_plane_onoff: SUCCESS random failures! but better than hsw at least. However the hardcoded color is indeed a mistake... Green on this panel is different from the green on the other panel. But I'm also not sure about drawing the color with cairo... How can we be sure what is there is what we want anyway. So maybe it is better to fall back to old scheme where we just check if changed when we want it changes... without checking for colors. What do you think? Thanks, Rodrigo. On Thu, Sep 4, 2014 at 1:20 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Sep 4, 2014 at 9:16 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> > wrote: > >> Hm, I don't see a pattern at all. And that sprites seem to work best > >> also looks funky. Are the results stable when you randomize them > >> (piglit can do that for you)? Can you add a residency checks in the > >> testcase (i.e. before the update and after the update) so that we know > >> it's really psr and not something else funny going on? I assume this > >> is on bdw, any difference in results on bdw? > > > > Yes, this is really strange. I couldn't find a pattern at all as well. > > Do you have any example of piglit helping randomization? > > > > The bad things with residency check is that vlv/chv doesn't have > performance > > counters. > > So this test would just work on hsw/bdw/ > > > > I did this work on a HSW. Test on BDW is one of tests I should do next > here. > > There's no shuffling in piglit unfortunately. But piglit is good for > comparing results, so I wonder whether they are stable or change. And > hsw is even more suprising since sprite tests pass more often in your > example, but sprite also has now hw invalidate on hsw. Confusing. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > -- Rodrigo Vivi Blog: http://blog.vivi.eng.br [-- Attachment #1.2: Type: text/html, Size: 5328 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2. 2014-09-05 0:40 ` Rodrigo Vivi @ 2014-09-05 8:37 ` Daniel Vetter 2014-09-13 0:29 ` Rodrigo Vivi 0 siblings, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2014-09-05 8:37 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx, Rodrigo Vivi On Thu, Sep 04, 2014 at 05:40:05PM -0700, Rodrigo Vivi wrote: > Here goes results on BDW with pure today's nightly (with idle_frame=1) > > # First run > > IGT-Version: 1.7-gd4b43f0 (x86_64) (Linux: 3.17.0-rc2+ x86_64) > Subtest primary_page_flip: SUCCESS > Subtest primary_mmap_gtt: SUCCESS > Test assertion failure function test_crc, file kms_psr_sink_crc.c:307: > Failed assertion: strcmp(ref_crc, CRC_GREEN) != 0 > Subtest primary_mmap_gtt_waiting: FAIL > Subtest primary_mmap_cpu: SUCCESS > Subtest primary_blt: SUCCESS > Subtest primary_render: SUCCESS > Subtest sprite_mmap_gtt: SUCCESS > Waiting 10s... > Subtest sprite_mmap_gtt_waiting: SUCCESS > Subtest sprite_mmap_cpu: SUCCESS > Subtest sprite_blt: SUCCESS > Subtest sprite_render: SUCCESS > Subtest sprite_plane_move: SUCCESS > Subtest sprite_plane_onoff: SUCCESS > Subtest cursor_mmap_gtt: SUCCESS > Waiting 10s... > Subtest cursor_mmap_gtt_waiting: SUCCESS > Subtest cursor_mmap_cpu: SUCCESS > Subtest cursor_blt: SUCCESS > Subtest cursor_render: SUCCESS > Subtest cursor_plane_move: SUCCESS > Subtest cursor_plane_onoff: SUCCESS > > #second run: > > IGT-Version: 1.7-gd4b43f0 (x86_64) (Linux: 3.17.0-rc2+ x86_64) > Subtest primary_page_flip: SUCCESS > Subtest primary_mmap_gtt: SUCCESS > Waiting 10s... > Subtest primary_mmap_gtt_waiting: SUCCESS > Subtest primary_mmap_cpu: SUCCESS > Subtest primary_blt: SUCCESS > Subtest primary_render: SUCCESS > Subtest sprite_mmap_gtt: SUCCESS > Waiting 10s... > Subtest sprite_mmap_gtt_waiting: SUCCESS > Subtest sprite_mmap_cpu: SUCCESS > Subtest sprite_blt: SUCCESS > Test assertion failure function test_crc, file kms_psr_sink_crc.c:307: > Failed assertion: strcmp(ref_crc, CRC_GREEN) != 0 > Subtest sprite_render: FAIL > Subtest sprite_plane_move: SUCCESS > Subtest sprite_plane_onoff: SUCCESS > Subtest cursor_mmap_gtt: SUCCESS > Waiting 10s... > Subtest cursor_mmap_gtt_waiting: SUCCESS > Subtest cursor_mmap_cpu: SUCCESS > Subtest cursor_blt: SUCCESS > Subtest cursor_render: SUCCESS > Subtest cursor_plane_move: SUCCESS > Subtest cursor_plane_onoff: SUCCESS > > random failures! but better than hsw at least. Ugh, this is painful :( > However the hardcoded color is indeed a mistake... Green on this panel is > different from the green on the other panel. > But I'm also not sure about drawing the color with cairo... How can we be > sure what is there is what we want anyway. Yeah, the crc value is implementation defined, but otoh we don't really depend upon that, as long as the same output results in the same crc. > So maybe it is better to fall back to old scheme where we just check if > changed when we want it changes... without checking for colors. > What do you think? You can't check for change with crc due to collisions, you really only (at least reliably) can check for matching outputs. And as long as we don't have any color correction enabled a given color on the sprite/cursor plane should exactly match the same color on the primary plane. For inspiration Ville's cursor crc testcase has all the ingredients I think you'll need for the sprite/cursor move tests. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2. 2014-09-05 8:37 ` Daniel Vetter @ 2014-09-13 0:29 ` Rodrigo Vivi 2014-09-15 9:08 ` Daniel Vetter 0 siblings, 1 reply; 19+ messages in thread From: Rodrigo Vivi @ 2014-09-13 0:29 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, Rodrigo Vivi [-- Attachment #1.1: Type: text/plain, Size: 4054 bytes --] Please ignore this full series here. I have a better one that let PSR on HSW in a better stage with only 1 idle frame and without changing the 100ms. Actually if we are brave we can reduce this to 24 or less. The new work is currently under http://cgit.freedesktop.org/~vivijim/drm-intel/log/?h=psr-3.18 However I'm not going to send yet because on BDW it got works. It seems BDW doesn't like to get PSR enabled immediatelly. So I'm justs sendint new series after I'm confortable that PSR is in a better stage for both HSW and BDW. Thanks, Rodrigo. On Fri, Sep 5, 2014 at 1:37 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Sep 04, 2014 at 05:40:05PM -0700, Rodrigo Vivi wrote: > > Here goes results on BDW with pure today's nightly (with idle_frame=1) > > > > # First run > > > > IGT-Version: 1.7-gd4b43f0 (x86_64) (Linux: 3.17.0-rc2+ x86_64) > > Subtest primary_page_flip: SUCCESS > > Subtest primary_mmap_gtt: SUCCESS > > Test assertion failure function test_crc, file kms_psr_sink_crc.c:307: > > Failed assertion: strcmp(ref_crc, CRC_GREEN) != 0 > > Subtest primary_mmap_gtt_waiting: FAIL > > Subtest primary_mmap_cpu: SUCCESS > > Subtest primary_blt: SUCCESS > > Subtest primary_render: SUCCESS > > Subtest sprite_mmap_gtt: SUCCESS > > Waiting 10s... > > Subtest sprite_mmap_gtt_waiting: SUCCESS > > Subtest sprite_mmap_cpu: SUCCESS > > Subtest sprite_blt: SUCCESS > > Subtest sprite_render: SUCCESS > > Subtest sprite_plane_move: SUCCESS > > Subtest sprite_plane_onoff: SUCCESS > > Subtest cursor_mmap_gtt: SUCCESS > > Waiting 10s... > > Subtest cursor_mmap_gtt_waiting: SUCCESS > > Subtest cursor_mmap_cpu: SUCCESS > > Subtest cursor_blt: SUCCESS > > Subtest cursor_render: SUCCESS > > Subtest cursor_plane_move: SUCCESS > > Subtest cursor_plane_onoff: SUCCESS > > > > #second run: > > > > IGT-Version: 1.7-gd4b43f0 (x86_64) (Linux: 3.17.0-rc2+ x86_64) > > Subtest primary_page_flip: SUCCESS > > Subtest primary_mmap_gtt: SUCCESS > > Waiting 10s... > > Subtest primary_mmap_gtt_waiting: SUCCESS > > Subtest primary_mmap_cpu: SUCCESS > > Subtest primary_blt: SUCCESS > > Subtest primary_render: SUCCESS > > Subtest sprite_mmap_gtt: SUCCESS > > Waiting 10s... > > Subtest sprite_mmap_gtt_waiting: SUCCESS > > Subtest sprite_mmap_cpu: SUCCESS > > Subtest sprite_blt: SUCCESS > > Test assertion failure function test_crc, file kms_psr_sink_crc.c:307: > > Failed assertion: strcmp(ref_crc, CRC_GREEN) != 0 > > Subtest sprite_render: FAIL > > Subtest sprite_plane_move: SUCCESS > > Subtest sprite_plane_onoff: SUCCESS > > Subtest cursor_mmap_gtt: SUCCESS > > Waiting 10s... > > Subtest cursor_mmap_gtt_waiting: SUCCESS > > Subtest cursor_mmap_cpu: SUCCESS > > Subtest cursor_blt: SUCCESS > > Subtest cursor_render: SUCCESS > > Subtest cursor_plane_move: SUCCESS > > Subtest cursor_plane_onoff: SUCCESS > > > > random failures! but better than hsw at least. > > Ugh, this is painful :( > > > However the hardcoded color is indeed a mistake... Green on this panel is > > different from the green on the other panel. > > But I'm also not sure about drawing the color with cairo... How can we be > > sure what is there is what we want anyway. > > Yeah, the crc value is implementation defined, but otoh we don't really > depend upon that, as long as the same output results in the same crc. > > > So maybe it is better to fall back to old scheme where we just check if > > changed when we want it changes... without checking for colors. > > What do you think? > > You can't check for change with crc due to collisions, you really only (at > least reliably) can check for matching outputs. And as long as we don't > have any color correction enabled a given color on the sprite/cursor plane > should exactly match the same color on the primary plane. > > For inspiration Ville's cursor crc testcase has all the ingredients I > think you'll need for the sprite/cursor move tests. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > -- Rodrigo Vivi Blog: http://blog.vivi.eng.br [-- Attachment #1.2: Type: text/html, Size: 5392 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2. 2014-09-13 0:29 ` Rodrigo Vivi @ 2014-09-15 9:08 ` Daniel Vetter 0 siblings, 0 replies; 19+ messages in thread From: Daniel Vetter @ 2014-09-15 9:08 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx, Rodrigo Vivi On Fri, Sep 12, 2014 at 05:29:44PM -0700, Rodrigo Vivi wrote: > Please ignore this full series here. > > I have a better one that let PSR on HSW in a better stage with only 1 idle > frame and without changing the 100ms. Actually if we are brave we can > reduce this to 24 or less. The new work is currently under > http://cgit.freedesktop.org/~vivijim/drm-intel/log/?h=psr-3.18 > However I'm not going to send yet because on BDW it got works. It seems BDW > doesn't like to get PSR enabled immediatelly. So I'm justs sendint new > series after I'm confortable that PSR is in a better stage for both HSW and > BDW. How does this interact with the improved psr crc capture code? And if we aim for perfect we should be able to get the delay for the reenable for down to 0. 24ms is still about 1.5 frames ... But sounding much better indeed now, nice work! -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2014-09-15 9:08 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-04 2:49 [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2 Rodrigo Vivi
2014-09-04 2:49 ` [PATCH 2/4] drm/i915: Remove PSR HW tracking Rodrigo Vivi
[not found] ` <20140904080643.GC4193@intel.com>
2014-09-04 19:53 ` Rodrigo Vivi
2014-09-05 9:14 ` Ville Syrjälä
2014-09-04 2:49 ` [PATCH 3/4] drm/i915: Move PSR w/as to enable source Rodrigo Vivi
2014-09-04 2:49 ` [PATCH 4/4] drm/i915: Rename psr setup vsc function and set it always Rodrigo Vivi
[not found] ` <20140904075516.GB4193@intel.com>
2014-09-04 18:18 ` [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2 Rodrigo Vivi
[not found] ` <20140904092916.GF15520@phenom.ffwll.local>
[not found] ` <20140904100427.GD4193@intel.com>
[not found] ` <20140904101819.GE4193@intel.com>
2014-09-04 11:04 ` Daniel Vetter
2014-09-04 18:26 ` Rodrigo Vivi
2014-09-04 18:22 ` Rodrigo Vivi
[not found] ` <87wq9jg62e.fsf@intel.com>
2014-09-04 18:18 ` Rodrigo Vivi
[not found] ` <20140904092253.GD15520@phenom.ffwll.local>
2014-09-04 18:20 ` Rodrigo Vivi
2014-09-04 19:05 ` Daniel Vetter
2014-09-04 19:16 ` Rodrigo Vivi
2014-09-04 20:20 ` Daniel Vetter
2014-09-05 0:40 ` Rodrigo Vivi
2014-09-05 8:37 ` Daniel Vetter
2014-09-13 0:29 ` Rodrigo Vivi
2014-09-15 9:08 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.