| Message ID | 1437781137-1119-1-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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); }
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(-)