| Message ID | 1409798999-1809-1-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > 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
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. 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
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. Which exact subcases do 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
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. -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
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. > -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
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. > > > -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
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
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 >
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 >
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 >
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 >
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 >
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
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 >
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
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 >
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
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 >
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
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;
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(-)