[PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.

[PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.

[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

[PATCH 2/4] drm/i915: Remove PSR HW tracking.

[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

[PATCH 3/4] drm/i915: Move PSR w/as to enable source.

[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

[PATCH 4/4] drm/i915: Rename psr setup vsc function and set it always.

[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

Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.

[Daniel Vetter]

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

Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.

[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

Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.

[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

Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.

[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

Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.

[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

Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.

[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

Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.

[Daniel Vetter]

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

Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.

[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

Re: [PATCH 2/4] drm/i915: Remove PSR HW tracking.

[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

Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.

[Daniel Vetter]

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

Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.

[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

Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.

[Daniel Vetter]

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

Re: [PATCH 2/4] drm/i915: Remove PSR HW tracking.

[Ville Syrjälä]

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

Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.

[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

Re: [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.

[Daniel Vetter]

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