[PATCH 1/2] drm/i915: VLV/CHV PSR: Increase wait delay time before active PSR.

[PATCH 1/2] drm/i915: VLV/CHV PSR: Increase wait delay time before active PSR.

[Rodrigo Vivi]

Since active function on VLV immediately activate PSR let's give more
time for idleness. Different from core platforms where we have idle_frames
count.

Also kms_psr_sink_crc now is automated and always get this:

[drm:intel_enable_pipe] enabling pipe A
[drm:intel_edp_backlight_on]
[drm:intel_panel_enable_backlight] pipe
[drm:intel_panel_enable_backlight] pipe A
[drm:intel_panel_actually_set_backlight] set backlight PWM = 7812

PSR gets enabled somewhere here after backlight.

[drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x0
[drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
[drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp

PSR gets flushed around here by intel_atomic_commit

[drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
[drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp
[drm:intel_set_memory_cxsr] memory self-refresh is enabled
[drm:intel_connector_check_state] [CONNECTOR:39:eDP-1]
[drm:check_encoder_state] [ENCODER:30:DAC-30]
[drm:check_encoder_state] [ENCODER:31:TMDS-31]
[drm:check_encoder_state] [ENCODER:36:TMDS-36]
[drm:check_encoder_state] [ENCODER:38:TMDS-38]
[drm:check_crtc_state] [CRTC:21]
[drm:check_crtc_state] [CRTC:26]
[drm:intel_psr_activate [i915]] *ERROR* PSR Active
[drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x
[drm:intel_set_cpu_fifo_underrun_reporting [i915]] *ERROR* pipe A underrun
[drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe A FIFO
Underrun.

It is true that in a product we won't keep disabling and enabling planes so
frequently, but for safeness let's stay conservative.

It is also true that 500ms is an etternity. But PSR is anyway a power saving
feature for idle scenario. So if it is idle feature stays on and 500ms to get
it reanabled is not that insane.

v2: Rebase over intel_psr.c and fix typo.
v3: Revival: Manual tests indicated that this is needed. With a short delay
    there is a huge risk of getting blank screens when planes are being enabled.
v4: Revival 2 with reasonable delay. 1/2 sec instead of 5. VBT is 10 sec but
    actually time for link training what we aren't doing, but with only 100 sec
    in some cases kms_psr_sink_crc manual was showing blank screen,
    so let's use this for now. Also changed comment by a FIXME.
v5: Rebase after a long time, remove FIXME and update comment above.

Reviewed-by: Durgadoss R <durgadoss.r@intel.com> (v4)
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index acd8ec8..0f446b7 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -698,6 +698,7 @@ void intel_psr_flush(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	enum pipe pipe;
+	int delay = msecs_to_jiffies(HAS_DDI(dev) ? 100 : 500);
 
 	mutex_lock(&dev_priv->psr.lock);
 	if (!dev_priv->psr.enabled) {
@@ -733,7 +734,7 @@ void intel_psr_flush(struct drm_device *dev,
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
 		schedule_delayed_work(&dev_priv->psr.work,
-				      msecs_to_jiffies(100));
+				      msecs_to_jiffies(delay));
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
-- 
1.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Also call frontbuffer flip when disabling planes.

[Rodrigo Vivi]

We also need to call the frontbuffer flip to trigger proper
invalidations when disabling planes. Otherwise we will miss
screen updates when disabling sprites or cursor.

It was caught with kms_psr_sink_crc sprite_plane_onoff
and cursor_plane_onoff subtests.

This is probably a regression since I can also get this
with the manual test case, but with so many changes on atomic
modeset I couldn't track exactly when this was introduced.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index af0bcfe..bb124cc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11716,7 +11716,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		intel_crtc->atomic.update_wm_pre = true;
 	}
 
-	if (visible)
+	if (visible || was_visible)
 		intel_crtc->atomic.fb_bits |=
 			to_intel_plane(plane)->frontbuffer_bit;
 
-- 
1.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: VLV/CHV PSR: Increase wait delay time before active PSR.

[Rodrigo Vivi]

Since active function on VLV immediately activate PSR let's give more
time for idleness. Different from core platforms where we have idle_frames
count.

Also kms_psr_sink_crc now is automated and always get this:

[drm:intel_enable_pipe] enabling pipe A
[drm:intel_edp_backlight_on]
[drm:intel_panel_enable_backlight] pipe
[drm:intel_panel_enable_backlight] pipe A
[drm:intel_panel_actually_set_backlight] set backlight PWM = 7812

PSR gets enabled somewhere here after backlight.

[drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x0
[drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
[drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp

PSR gets flushed around here by intel_atomic_commit

[drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
[drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp
[drm:intel_set_memory_cxsr] memory self-refresh is enabled
[drm:intel_connector_check_state] [CONNECTOR:39:eDP-1]
[drm:check_encoder_state] [ENCODER:30:DAC-30]
[drm:check_encoder_state] [ENCODER:31:TMDS-31]
[drm:check_encoder_state] [ENCODER:36:TMDS-36]
[drm:check_encoder_state] [ENCODER:38:TMDS-38]
[drm:check_crtc_state] [CRTC:21]
[drm:check_crtc_state] [CRTC:26]
[drm:intel_psr_activate [i915]] *ERROR* PSR Active
[drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x
[drm:intel_set_cpu_fifo_underrun_reporting [i915]] *ERROR* pipe A underrun
[drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe A FIFO
Underrun.

It is true that in a product we won't keep disabling and enabling planes so
frequently, but for safeness let's stay conservative.

It is also true that 500ms is an etternity. But PSR is anyway a power saving
feature for idle scenario. So if it is idle feature stays on and 500ms to get
it reanabled is not that insane.

v2: Rebase over intel_psr.c and fix typo.
v3: Revival: Manual tests indicated that this is needed. With a short delay
    there is a huge risk of getting blank screens when planes are being enabled.
v4: Revival 2 with reasonable delay. 1/2 sec instead of 5. VBT is 10 sec but
    actually time for link training what we aren't doing, but with only 100 sec
    in some cases kms_psr_sink_crc manual was showing blank screen,
    so let's use this for now. Also changed comment by a FIXME.
v5: Rebase after a long time, remove FIXME and update comment above.
v6: msecs_to_jiffies is already on delay. remove duplication.

Reviewed-by: Durgadoss R <durgadoss.r@intel.com> (v4)
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index acd8ec8..bec13b8 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -698,6 +698,7 @@ void intel_psr_flush(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	enum pipe pipe;
+	int delay = msecs_to_jiffies(HAS_DDI(dev) ? 100 : 500);
 
 	mutex_lock(&dev_priv->psr.lock);
 	if (!dev_priv->psr.enabled) {
@@ -732,8 +733,7 @@ void intel_psr_flush(struct drm_device *dev,
 	}
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
-		schedule_delayed_work(&dev_priv->psr.work,
-				      msecs_to_jiffies(100));
+		schedule_delayed_work(&dev_priv->psr.work, delay);
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
-- 
1.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: VLV/CHV PSR: Increase wait delay time before active PSR.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6861
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  295/295              295/295
SNB                                  315/315              315/315
IVB                                  342/342              342/342
BYT                 -2              285/285              283/285
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
*BYT  igt@gem_partial_pwrite_pread@reads-display      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Also call frontbuffer flip when disabling planes.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6860
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -1              296/296              295/296
SNB                                  316/316              316/316
IVB                                  342/342              342/342
BYT                 -1              286/286              285/286
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_flip@flip-vs-rmfb-interruptible      PASS(1)      DMESG_WARN(1)
*BYT  igt@gem_partial_pwrite_pread@reads-display      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: VLV/CHV PSR: Increase wait delay time before active PSR.

[Daniel Vetter]

On Fri, Jul 24, 2015 at 04:38:56PM -0700, Rodrigo Vivi wrote:
> Since active function on VLV immediately activate PSR let's give more
> time for idleness. Different from core platforms where we have idle_frames
> count.
> 
> Also kms_psr_sink_crc now is automated and always get this:
> 
> [drm:intel_enable_pipe] enabling pipe A
> [drm:intel_edp_backlight_on]
> [drm:intel_panel_enable_backlight] pipe
> [drm:intel_panel_enable_backlight] pipe A
> [drm:intel_panel_actually_set_backlight] set backlight PWM = 7812
> 
> PSR gets enabled somewhere here after backlight.
> 
> [drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x0
> [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
> [drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp
> 
> PSR gets flushed around here by intel_atomic_commit
> 
> [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
> [drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp
> [drm:intel_set_memory_cxsr] memory self-refresh is enabled
> [drm:intel_connector_check_state] [CONNECTOR:39:eDP-1]
> [drm:check_encoder_state] [ENCODER:30:DAC-30]
> [drm:check_encoder_state] [ENCODER:31:TMDS-31]
> [drm:check_encoder_state] [ENCODER:36:TMDS-36]
> [drm:check_encoder_state] [ENCODER:38:TMDS-38]
> [drm:check_crtc_state] [CRTC:21]
> [drm:check_crtc_state] [CRTC:26]
> [drm:intel_psr_activate [i915]] *ERROR* PSR Active
> [drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x
> [drm:intel_set_cpu_fifo_underrun_reporting [i915]] *ERROR* pipe A underrun
> [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe A FIFO
> Underrun.
> 
> It is true that in a product we won't keep disabling and enabling planes so
> frequently, but for safeness let's stay conservative.
> 
> It is also true that 500ms is an etternity. But PSR is anyway a power saving
> feature for idle scenario. So if it is idle feature stays on and 500ms to get
> it reanabled is not that insane.

I'm a bit confused, does this patch fix the above underruns? Any idea why
underruns happen at all? Do they only happen right after a full modeset?
There's some more details in the changelog below but it's not entirely
clear to me really. Please clarify (preferrably with a paragraph I can add to
the commit message).
-Daniel

> 
> v2: Rebase over intel_psr.c and fix typo.
> v3: Revival: Manual tests indicated that this is needed. With a short delay
>     there is a huge risk of getting blank screens when planes are being enabled.
> v4: Revival 2 with reasonable delay. 1/2 sec instead of 5. VBT is 10 sec but
>     actually time for link training what we aren't doing, but with only 100 sec
>     in some cases kms_psr_sink_crc manual was showing blank screen,
>     so let's use this for now. Also changed comment by a FIXME.
> v5: Rebase after a long time, remove FIXME and update comment above.
> 
> Reviewed-by: Durgadoss R <durgadoss.r@intel.com> (v4)
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index acd8ec8..0f446b7 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -698,6 +698,7 @@ void intel_psr_flush(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc;
>  	enum pipe pipe;
> +	int delay = msecs_to_jiffies(HAS_DDI(dev) ? 100 : 500);
>  
>  	mutex_lock(&dev_priv->psr.lock);
>  	if (!dev_priv->psr.enabled) {
> @@ -733,7 +734,7 @@ void intel_psr_flush(struct drm_device *dev,
>  
>  	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
>  		schedule_delayed_work(&dev_priv->psr.work,
> -				      msecs_to_jiffies(100));
> +				      msecs_to_jiffies(delay));
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> -- 
> 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
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: VLV/CHV PSR: Increase wait delay time before active PSR.

[Daniel Vetter]

On Fri, Jul 24, 2015 at 04:42:27PM -0700, Rodrigo Vivi wrote:
> Since active function on VLV immediately activate PSR let's give more
> time for idleness. Different from core platforms where we have idle_frames
> count.
> 
> Also kms_psr_sink_crc now is automated and always get this:
> 
> [drm:intel_enable_pipe] enabling pipe A
> [drm:intel_edp_backlight_on]
> [drm:intel_panel_enable_backlight] pipe
> [drm:intel_panel_enable_backlight] pipe A
> [drm:intel_panel_actually_set_backlight] set backlight PWM = 7812
> 
> PSR gets enabled somewhere here after backlight.
> 
> [drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x0
> [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
> [drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp
> 
> PSR gets flushed around here by intel_atomic_commit
> 
> [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
> [drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp
> [drm:intel_set_memory_cxsr] memory self-refresh is enabled
> [drm:intel_connector_check_state] [CONNECTOR:39:eDP-1]
> [drm:check_encoder_state] [ENCODER:30:DAC-30]
> [drm:check_encoder_state] [ENCODER:31:TMDS-31]
> [drm:check_encoder_state] [ENCODER:36:TMDS-36]
> [drm:check_encoder_state] [ENCODER:38:TMDS-38]
> [drm:check_crtc_state] [CRTC:21]
> [drm:check_crtc_state] [CRTC:26]
> [drm:intel_psr_activate [i915]] *ERROR* PSR Active
> [drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x
> [drm:intel_set_cpu_fifo_underrun_reporting [i915]] *ERROR* pipe A underrun
> [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe A FIFO
> Underrun.
> 
> It is true that in a product we won't keep disabling and enabling planes so
> frequently, but for safeness let's stay conservative.
> 
> It is also true that 500ms is an etternity. But PSR is anyway a power saving
> feature for idle scenario. So if it is idle feature stays on and 500ms to get
> it reanabled is not that insane.
> 
> v2: Rebase over intel_psr.c and fix typo.
> v3: Revival: Manual tests indicated that this is needed. With a short delay
>     there is a huge risk of getting blank screens when planes are being enabled.
> v4: Revival 2 with reasonable delay. 1/2 sec instead of 5. VBT is 10 sec but
>     actually time for link training what we aren't doing, but with only 100 sec
>     in some cases kms_psr_sink_crc manual was showing blank screen,
>     so let's use this for now. Also changed comment by a FIXME.
> v5: Rebase after a long time, remove FIXME and update comment above.
> v6: msecs_to_jiffies is already on delay. remove duplication.
> 
> Reviewed-by: Durgadoss R <durgadoss.r@intel.com> (v4)
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index acd8ec8..bec13b8 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -698,6 +698,7 @@ void intel_psr_flush(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc;
>  	enum pipe pipe;
> +	int delay = msecs_to_jiffies(HAS_DDI(dev) ? 100 : 500);

Note that for a timeout you should be using our own
msecs_to_jiffies_timeout if it's for a timeout. This is needed since you
can't know when the next jiffy update happens and that might be right
away, therefore waiting for 10 jiffies might only be a wait for 9/HZ.
Anyway tiny nitpick.
-Daniel

>  
>  	mutex_lock(&dev_priv->psr.lock);
>  	if (!dev_priv->psr.enabled) {
> @@ -732,8 +733,7 @@ void intel_psr_flush(struct drm_device *dev,
>  	}
>  
>  	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
> -		schedule_delayed_work(&dev_priv->psr.work,
> -				      msecs_to_jiffies(100));
> +		schedule_delayed_work(&dev_priv->psr.work, delay);
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> -- 
> 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
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: VLV/CHV PSR: Increase wait delay time before active PSR.

[Vivi, Rodrigo]

On Mon, 2015-07-27 at 10:31 +0200, Daniel Vetter wrote:
> On Fri, Jul 24, 2015 at 04:38:56PM -0700, Rodrigo Vivi wrote:
> > Since active function on VLV immediately activate PSR let's give more
> > time for idleness. Different from core platforms where we have idle_frames
> > count.
> > 
> > Also kms_psr_sink_crc now is automated and always get this:
> > 
> > [drm:intel_enable_pipe] enabling pipe A
> > [drm:intel_edp_backlight_on]
> > [drm:intel_panel_enable_backlight] pipe
> > [drm:intel_panel_enable_backlight] pipe A
> > [drm:intel_panel_actually_set_backlight] set backlight PWM = 7812
> > 
> > PSR gets enabled somewhere here after backlight.
> > 
> > [drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x0
> > [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
> > [drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp
> > 
> > PSR gets flushed around here by intel_atomic_commit
> > 
> > [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
> > [drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp
> > [drm:intel_set_memory_cxsr] memory self-refresh is enabled
> > [drm:intel_connector_check_state] [CONNECTOR:39:eDP-1]
> > [drm:check_encoder_state] [ENCODER:30:DAC-30]
> > [drm:check_encoder_state] [ENCODER:31:TMDS-31]
> > [drm:check_encoder_state] [ENCODER:36:TMDS-36]
> > [drm:check_encoder_state] [ENCODER:38:TMDS-38]
> > [drm:check_crtc_state] [CRTC:21]
> > [drm:check_crtc_state] [CRTC:26]
> > [drm:intel_psr_activate [i915]] *ERROR* PSR Active
> > [drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x
> > [drm:intel_set_cpu_fifo_underrun_reporting [i915]] *ERROR* pipe A underrun
> > [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe A FIFO
> > Underrun.
> > 
> > It is true that in a product we won't keep disabling and enabling planes so
> > frequently, but for safeness let's stay conservative.
> > 
> > It is also true that 500ms is an etternity. But PSR is anyway a power saving
> > feature for idle scenario. So if it is idle feature stays on and 500ms to get
> > it reanabled is not that insane.
> 
> I'm a bit confused, does this patch fix the above underruns?2
yes.

>  Any idea why
> underruns happen at all?
No.

>  Do they only happen right after a full modeset?
yes, and only after the full modeset. if test continue to run I will get
all screen uppdates normally. Blank with underrun just happen on the
first screen update right after the full modeset.

350ms would be enough to get ride of this blanks, exept when it happens
after dpms off/on where 350 didn't help and 500ms was needed.



> There's some more details in the changelog below but it's not entirely
> clear to me really. 

Below was just the history on this patch, that I just remember about it
when I run test cases myself on VLV or BSW.

> Please clarify (preferrably with a paragraph I can add to
> the commit message).
> -Daniel
> 
> > 
> > v2: Rebase over intel_psr.c and fix typo.
> > v3: Revival: Manual tests indicated that this is needed. With a short delay
> >     there is a huge risk of getting blank screens when planes are being enabled.
> > v4: Revival 2 with reasonable delay. 1/2 sec instead of 5. VBT is 10 sec but
> >     actually time for link training what we aren't doing, but with only 100 sec
> >     in some cases kms_psr_sink_crc manual was showing blank screen,
> >     so let's use this for now. Also changed comment by a FIXME.
> > v5: Rebase after a long time, remove FIXME and update comment above.
> > 
> > Reviewed-by: Durgadoss R <durgadoss.r@intel.com> (v4)
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index acd8ec8..0f446b7 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -698,6 +698,7 @@ void intel_psr_flush(struct drm_device *dev,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_crtc *crtc;
> >  	enum pipe pipe;
> > +	int delay = msecs_to_jiffies(HAS_DDI(dev) ? 100 : 500);
> >  
> >  	mutex_lock(&dev_priv->psr.lock);
> >  	if (!dev_priv->psr.enabled) {
> > @@ -733,7 +734,7 @@ void intel_psr_flush(struct drm_device *dev,
> >  
> >  	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
> >  		schedule_delayed_work(&dev_priv->psr.work,
> > -				      msecs_to_jiffies(100));
> > +				      msecs_to_jiffies(delay));
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
> >  
> > -- 
> > 1.9.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: VLV/CHV PSR: Increase wait delay time before active PSR.

[Vivi, Rodrigo]

On Mon, 2015-07-27 at 10:36 +0200, Daniel Vetter wrote:
> On Fri, Jul 24, 2015 at 04:42:27PM -0700, Rodrigo Vivi wrote:
> > Since active function on VLV immediately activate PSR let's give more
> > time for idleness. Different from core platforms where we have idle_frames
> > count.
> > 
> > Also kms_psr_sink_crc now is automated and always get this:
> > 
> > [drm:intel_enable_pipe] enabling pipe A
> > [drm:intel_edp_backlight_on]
> > [drm:intel_panel_enable_backlight] pipe
> > [drm:intel_panel_enable_backlight] pipe A
> > [drm:intel_panel_actually_set_backlight] set backlight PWM = 7812
> > 
> > PSR gets enabled somewhere here after backlight.
> > 
> > [drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x0
> > [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
> > [drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp
> > 
> > PSR gets flushed around here by intel_atomic_commit
> > 
> > [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
> > [drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp
> > [drm:intel_set_memory_cxsr] memory self-refresh is enabled
> > [drm:intel_connector_check_state] [CONNECTOR:39:eDP-1]
> > [drm:check_encoder_state] [ENCODER:30:DAC-30]
> > [drm:check_encoder_state] [ENCODER:31:TMDS-31]
> > [drm:check_encoder_state] [ENCODER:36:TMDS-36]
> > [drm:check_encoder_state] [ENCODER:38:TMDS-38]
> > [drm:check_crtc_state] [CRTC:21]
> > [drm:check_crtc_state] [CRTC:26]
> > [drm:intel_psr_activate [i915]] *ERROR* PSR Active
> > [drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x
> > [drm:intel_set_cpu_fifo_underrun_reporting [i915]] *ERROR* pipe A underrun
> > [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe A FIFO
> > Underrun.
> > 
> > It is true that in a product we won't keep disabling and enabling planes so
> > frequently, but for safeness let's stay conservative.
> > 
> > It is also true that 500ms is an etternity. But PSR is anyway a power saving
> > feature for idle scenario. So if it is idle feature stays on and 500ms to get
> > it reanabled is not that insane.
> > 
> > v2: Rebase over intel_psr.c and fix typo.
> > v3: Revival: Manual tests indicated that this is needed. With a short delay
> >     there is a huge risk of getting blank screens when planes are being enabled.
> > v4: Revival 2 with reasonable delay. 1/2 sec instead of 5. VBT is 10 sec but
> >     actually time for link training what we aren't doing, but with only 100 sec
> >     in some cases kms_psr_sink_crc manual was showing blank screen,
> >     so let's use this for now. Also changed comment by a FIXME.
> > v5: Rebase after a long time, remove FIXME and update comment above.
> > v6: msecs_to_jiffies is already on delay. remove duplication.
> > 
> > Reviewed-by: Durgadoss R <durgadoss.r@intel.com> (v4)
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index acd8ec8..bec13b8 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -698,6 +698,7 @@ void intel_psr_flush(struct drm_device *dev,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_crtc *crtc;
> >  	enum pipe pipe;
> > +	int delay = msecs_to_jiffies(HAS_DDI(dev) ? 100 : 500);
> 
> Note that for a timeout you should be using our own
> msecs_to_jiffies_timeout if it's for a timeout. This is needed since you
> can't know when the next jiffy update happens and that might be right
> away, therefore waiting for 10 jiffies might only be a wait for 9/HZ.
> Anyway tiny nitpick.

This is actually only used on schedule_delayed_work that receives the
numer of jiffies to wait.
I believe to avoid confusion I should let the msecs_to_jiffies on
schedule_delayed_work only and delay in ms. Do you want another v++?

> -Daniel
> 
> >  
> >  	mutex_lock(&dev_priv->psr.lock);
> >  	if (!dev_priv->psr.enabled) {
> > @@ -732,8 +733,7 @@ void intel_psr_flush(struct drm_device *dev,
> >  	}
> >  
> >  	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
> > -		schedule_delayed_work(&dev_priv->psr.work,
> > -				      msecs_to_jiffies(100));
> > +		schedule_delayed_work(&dev_priv->psr.work, delay);
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
> >  
> > -- 
> > 1.9.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: VLV/CHV PSR: Increase wait delay time before active PSR.

[Daniel Vetter]

On Mon, Jul 27, 2015 at 06:37:28PM +0000, Vivi, Rodrigo wrote:
> On Mon, 2015-07-27 at 10:36 +0200, Daniel Vetter wrote:
> > On Fri, Jul 24, 2015 at 04:42:27PM -0700, Rodrigo Vivi wrote:
> > > Since active function on VLV immediately activate PSR let's give more
> > > time for idleness. Different from core platforms where we have idle_frames
> > > count.
> > > 
> > > Also kms_psr_sink_crc now is automated and always get this:
> > > 
> > > [drm:intel_enable_pipe] enabling pipe A
> > > [drm:intel_edp_backlight_on]
> > > [drm:intel_panel_enable_backlight] pipe
> > > [drm:intel_panel_enable_backlight] pipe A
> > > [drm:intel_panel_actually_set_backlight] set backlight PWM = 7812
> > > 
> > > PSR gets enabled somewhere here after backlight.
> > > 
> > > [drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x0
> > > [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
> > > [drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp
> > > 
> > > PSR gets flushed around here by intel_atomic_commit
> > > 
> > > [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
> > > [drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp
> > > [drm:intel_set_memory_cxsr] memory self-refresh is enabled
> > > [drm:intel_connector_check_state] [CONNECTOR:39:eDP-1]
> > > [drm:check_encoder_state] [ENCODER:30:DAC-30]
> > > [drm:check_encoder_state] [ENCODER:31:TMDS-31]
> > > [drm:check_encoder_state] [ENCODER:36:TMDS-36]
> > > [drm:check_encoder_state] [ENCODER:38:TMDS-38]
> > > [drm:check_crtc_state] [CRTC:21]
> > > [drm:check_crtc_state] [CRTC:26]
> > > [drm:intel_psr_activate [i915]] *ERROR* PSR Active
> > > [drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x
> > > [drm:intel_set_cpu_fifo_underrun_reporting [i915]] *ERROR* pipe A underrun
> > > [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe A FIFO
> > > Underrun.
> > > 
> > > It is true that in a product we won't keep disabling and enabling planes so
> > > frequently, but for safeness let's stay conservative.
> > > 
> > > It is also true that 500ms is an etternity. But PSR is anyway a power saving
> > > feature for idle scenario. So if it is idle feature stays on and 500ms to get
> > > it reanabled is not that insane.
> > > 
> > > v2: Rebase over intel_psr.c and fix typo.
> > > v3: Revival: Manual tests indicated that this is needed. With a short delay
> > >     there is a huge risk of getting blank screens when planes are being enabled.
> > > v4: Revival 2 with reasonable delay. 1/2 sec instead of 5. VBT is 10 sec but
> > >     actually time for link training what we aren't doing, but with only 100 sec
> > >     in some cases kms_psr_sink_crc manual was showing blank screen,
> > >     so let's use this for now. Also changed comment by a FIXME.
> > > v5: Rebase after a long time, remove FIXME and update comment above.
> > > v6: msecs_to_jiffies is already on delay. remove duplication.
> > > 
> > > Reviewed-by: Durgadoss R <durgadoss.r@intel.com> (v4)
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > index acd8ec8..bec13b8 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -698,6 +698,7 @@ void intel_psr_flush(struct drm_device *dev,
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	struct drm_crtc *crtc;
> > >  	enum pipe pipe;
> > > +	int delay = msecs_to_jiffies(HAS_DDI(dev) ? 100 : 500);
> > 
> > Note that for a timeout you should be using our own
> > msecs_to_jiffies_timeout if it's for a timeout. This is needed since you
> > can't know when the next jiffy update happens and that might be right
> > away, therefore waiting for 10 jiffies might only be a wait for 9/HZ.
> > Anyway tiny nitpick.
> 
> This is actually only used on schedule_delayed_work that receives the
> numer of jiffies to wait.
> I believe to avoid confusion I should let the msecs_to_jiffies on
> schedule_delayed_work only and delay in ms. Do you want another v++?

If you feel like, was kinda just an aside bikeshed really. I'm just
constantly confused about jiffies timeouts myself ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: VLV/CHV PSR: Increase wait delay time before active PSR.

[Rodrigo Vivi]

Since active function on VLV immediately activate PSR let's give more
time for idleness. Different from core platforms where we have idle_frames
count.

Also kms_psr_sink_crc now is automated and always get this:

[drm:intel_enable_pipe] enabling pipe A
[drm:intel_edp_backlight_on]
[drm:intel_panel_enable_backlight] pipe
[drm:intel_panel_enable_backlight] pipe A
[drm:intel_panel_actually_set_backlight] set backlight PWM = 7812

PSR gets enabled somewhere here after backlight.

[drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x0
[drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
[drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp

PSR gets flushed around here by intel_atomic_commit

[drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
[drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp
[drm:intel_set_memory_cxsr] memory self-refresh is enabled
[drm:intel_connector_check_state] [CONNECTOR:39:eDP-1]
[drm:check_encoder_state] [ENCODER:30:DAC-30]
[drm:check_encoder_state] [ENCODER:31:TMDS-31]
[drm:check_encoder_state] [ENCODER:36:TMDS-36]
[drm:check_encoder_state] [ENCODER:38:TMDS-38]
[drm:check_crtc_state] [CRTC:21]
[drm:check_crtc_state] [CRTC:26]
[drm:intel_psr_activate [i915]] *ERROR* PSR Active
[drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x
[drm:intel_set_cpu_fifo_underrun_reporting [i915]] *ERROR* pipe A underrun
[drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe A FIFO
Underrun.

It is true that in a product we won't keep disabling and enabling planes so
frequently, but for safeness let's stay conservative.

It is also true that 500ms is an etternity. But PSR is anyway a power saving
feature for idle scenario. So if it is idle feature stays on and 500ms to get
it reanabled is not that insane.

v2: Rebase over intel_psr.c and fix typo.
v3: Revival: Manual tests indicated that this is needed. With a short delay
    there is a huge risk of getting blank screens when planes are being enabled.
v4: Revival 2 with reasonable delay. 1/2 sec instead of 5. VBT is 10 sec but
    actually time for link training what we aren't doing, but with only 100 sec
    in some cases kms_psr_sink_crc manual was showing blank screen,
    so let's use this for now. Also changed comment by a FIXME.
v5: Rebase after a long time, remove FIXME and update comment above.
v6: msecs_to_jiffies is already on delay. remove duplication.
v7: use msecs_to_jiffies on schedule_delayed_work call.

Reviewed-by: Durgadoss R <durgadoss.r@intel.com> (v4)
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index acd8ec8..a04b4dc 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -698,6 +698,7 @@ void intel_psr_flush(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	enum pipe pipe;
+	int delay_ms = HAS_DDI(dev) ? 100 : 500;
 
 	mutex_lock(&dev_priv->psr.lock);
 	if (!dev_priv->psr.enabled) {
@@ -733,7 +734,7 @@ void intel_psr_flush(struct drm_device *dev,
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
 		schedule_delayed_work(&dev_priv->psr.work,
-				      msecs_to_jiffies(100));
+				      msecs_to_jiffies(delay_ms));
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: VLV/CHV PSR: Increase wait delay time before active PSR.

[Daniel Vetter]

On Thu, Jul 30, 2015 at 05:07:55PM -0700, Rodrigo Vivi wrote:
> Since active function on VLV immediately activate PSR let's give more
> time for idleness. Different from core platforms where we have idle_frames
> count.
> 
> Also kms_psr_sink_crc now is automated and always get this:
> 
> [drm:intel_enable_pipe] enabling pipe A
> [drm:intel_edp_backlight_on]
> [drm:intel_panel_enable_backlight] pipe
> [drm:intel_panel_enable_backlight] pipe A
> [drm:intel_panel_actually_set_backlight] set backlight PWM = 7812
> 
> PSR gets enabled somewhere here after backlight.
> 
> [drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x0
> [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
> [drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp
> 
> PSR gets flushed around here by intel_atomic_commit
> 
> [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 511 / 511
> [drm:vlv_update_wm] Setting FIFO watermarks - A: plane=391, cursor=63, sp
> [drm:intel_set_memory_cxsr] memory self-refresh is enabled
> [drm:intel_connector_check_state] [CONNECTOR:39:eDP-1]
> [drm:check_encoder_state] [ENCODER:30:DAC-30]
> [drm:check_encoder_state] [ENCODER:31:TMDS-31]
> [drm:check_encoder_state] [ENCODER:36:TMDS-36]
> [drm:check_encoder_state] [ENCODER:38:TMDS-38]
> [drm:check_crtc_state] [CRTC:21]
> [drm:check_crtc_state] [CRTC:26]
> [drm:intel_psr_activate [i915]] *ERROR* PSR Active
> [drm:intel_get_hpd_pins] hotplug event received, stat 0x00000000, dig 0x
> [drm:intel_set_cpu_fifo_underrun_reporting [i915]] *ERROR* pipe A underrun
> [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe A FIFO
> Underrun.
> 
> It is true that in a product we won't keep disabling and enabling planes so
> frequently, but for safeness let's stay conservative.
> 
> It is also true that 500ms is an etternity. But PSR is anyway a power saving
> feature for idle scenario. So if it is idle feature stays on and 500ms to get
> it reanabled is not that insane.

Yeah I really like this level of detail in commit messages for tricky
bugs. Queued for -next, thanks for the patch.
-Daniel

> 
> v2: Rebase over intel_psr.c and fix typo.
> v3: Revival: Manual tests indicated that this is needed. With a short delay
>     there is a huge risk of getting blank screens when planes are being enabled.
> v4: Revival 2 with reasonable delay. 1/2 sec instead of 5. VBT is 10 sec but
>     actually time for link training what we aren't doing, but with only 100 sec
>     in some cases kms_psr_sink_crc manual was showing blank screen,
>     so let's use this for now. Also changed comment by a FIXME.
> v5: Rebase after a long time, remove FIXME and update comment above.
> v6: msecs_to_jiffies is already on delay. remove duplication.
> v7: use msecs_to_jiffies on schedule_delayed_work call.
> 
> Reviewed-by: Durgadoss R <durgadoss.r@intel.com> (v4)
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index acd8ec8..a04b4dc 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -698,6 +698,7 @@ void intel_psr_flush(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc;
>  	enum pipe pipe;
> +	int delay_ms = HAS_DDI(dev) ? 100 : 500;
>  
>  	mutex_lock(&dev_priv->psr.lock);
>  	if (!dev_priv->psr.enabled) {
> @@ -733,7 +734,7 @@ void intel_psr_flush(struct drm_device *dev,
>  
>  	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
>  		schedule_delayed_work(&dev_priv->psr.work,
> -				      msecs_to_jiffies(100));
> +				      msecs_to_jiffies(delay_ms));
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Also call frontbuffer flip when disabling planes.

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 1653 bytes --]

I had forgotten I had this patch and lost sometime yesterday debugging and
end up on same fix again :/

Daniel, do you need a reviewer on this? could you please take a quickly
look?

thanks in advance,
Rodrigo.

On Fri, Jul 24, 2015 at 4:41 PM Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:

> We also need to call the frontbuffer flip to trigger proper
> invalidations when disabling planes. Otherwise we will miss
> screen updates when disabling sprites or cursor.
>
> It was caught with kms_psr_sink_crc sprite_plane_onoff
> and cursor_plane_onoff subtests.
>
> This is probably a regression since I can also get this
> with the manual test case, but with so many changes on atomic
> modeset I couldn't track exactly when this was introduced.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index af0bcfe..bb124cc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11716,7 +11716,7 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>                 intel_crtc->atomic.update_wm_pre = true;
>         }
>
> -       if (visible)
> +       if (visible || was_visible)
>                 intel_crtc->atomic.fb_bits |=
>                         to_intel_plane(plane)->frontbuffer_bit;
>
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 2379 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/2] drm/i915: Also call frontbuffer flip when disabling planes.

[Paulo Zanoni]

2015-07-24 20:38 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> We also need to call the frontbuffer flip to trigger proper
> invalidations when disabling planes. Otherwise we will miss
> screen updates when disabling sprites or cursor.
>
> It was caught with kms_psr_sink_crc sprite_plane_onoff
> and cursor_plane_onoff subtests.
>
> This is probably a regression since I can also get this
> with the manual test case, but with so many changes on atomic
> modeset I couldn't track exactly when this was introduced.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Per our conversation on IRC, you need to mention that this patch only
affects the VLV family since on big core the HW tracking helps hide
the problem.

It would also be good to use the Testcase tags :)

But it looks correct. So with the amended message: Reviewed-by: Paulo
Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index af0bcfe..bb124cc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11716,7 +11716,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>                 intel_crtc->atomic.update_wm_pre = true;
>         }
>
> -       if (visible)
> +       if (visible || was_visible)
>                 intel_crtc->atomic.fb_bits |=
>                         to_intel_plane(plane)->frontbuffer_bit;
>
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Also call frontbuffer flip when disabling planes.

[Rodrigo Vivi]

We also need to call the frontbuffer flip to trigger proper
invalidations when disabling planes. Otherwise we will miss
screen updates when disabling sprites or cursor.

On core platforms where HW tracking also works, this issue
is totally masked because HW tracking triggers PSR exit
however on VLV/CHV that has only SW tracking we miss screen
updates when disabling planes.

It was caught with kms_psr_sink_crc sprite_plane_onoff
and cursor_plane_onoff subtests running on VLV/CHV.

This is probably a regression since I can also get this
with the manual test case, but with so many changes on atomic
modeset I couldn't track exactly when this was introduced.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f604ce1..930e5d0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11627,7 +11627,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		intel_crtc->atomic.update_wm_pre = true;
 	}
 
-	if (visible)
+	if (visible || was_visible)
 		intel_crtc->atomic.fb_bits |=
 			to_intel_plane(plane)->frontbuffer_bit;
 
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Also call frontbuffer flip when disabling planes.

[Daniel Vetter]

On Mon, Aug 24, 2015 at 04:38:23PM -0700, Rodrigo Vivi wrote:
> We also need to call the frontbuffer flip to trigger proper
> invalidations when disabling planes. Otherwise we will miss
> screen updates when disabling sprites or cursor.
> 
> On core platforms where HW tracking also works, this issue
> is totally masked because HW tracking triggers PSR exit
> however on VLV/CHV that has only SW tracking we miss screen
> updates when disabling planes.
> 
> It was caught with kms_psr_sink_crc sprite_plane_onoff
> and cursor_plane_onoff subtests running on VLV/CHV.
> 
> This is probably a regression since I can also get this
> with the manual test case, but with so many changes on atomic
> modeset I couldn't track exactly when this was introduced.
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f604ce1..930e5d0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11627,7 +11627,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  		intel_crtc->atomic.update_wm_pre = true;
>  	}
>  
> -	if (visible)
> +	if (visible || was_visible)
>  		intel_crtc->atomic.fb_bits |=
>  			to_intel_plane(plane)->frontbuffer_bit;
>  
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx