[PATCH 0/4] PSR general improvements and stabilization.

[PATCH 0/4] PSR general improvements and stabilization.

[Rodrigo Vivi]

Proceeding with the big series split here goes the general PSR
improvements and stabilization work.

There is no critical fix on this series although I believe it is
good to have all of them before we can enable PSR back by default.

Rodrigo Vivi (4):
  drm/i915: Force PSR exit when IRQ_HPD is detected on eDP.
  drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink.
  drm/i915: PSR: Let's rely more on frontbuffer tracking.
  drm/i915: PSR: Mask LPSP hw tracking back again.

 drivers/gpu/drm/i915/intel_dp.c  |  5 ++-
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 67 ++++++++++++++++++++++++++--------------
 3 files changed, 48 insertions(+), 25 deletions(-)

-- 
2.4.3

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

[PATCH 1/4] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP.

[Rodrigo Vivi]

According to VESA spec: "If a Source device receives and IRQ_HPD
while in a PSR active state, and cannot identify what caused the
IRQ_HPD to be generated, based on Sink device status registers,
the Source device can take implementation-specific action.
One such action can be to exit and then re-enter a PSR active
state."

Since we aren't checking for any sink status registers and we
 aren't looking for any other implementation-specific action,
in case we receive any IRQ_HPD and psr is active let's force
the exit and reschedule it back.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  |  5 ++++-
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 99b7f1d..71911b9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4892,6 +4892,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 
 	if (intel_dig_port->base.type != INTEL_OUTPUT_EDP)
 		intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT;
+	else
+		intel_psr_irq_hpd(dev);
 
 	if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
 		/*
@@ -4900,8 +4902,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		 * would end up in an endless cycle of
 		 * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
 		 */
-		DRM_DEBUG_KMS("ignoring long hpd on eDP port %c\n",
+		DRM_DEBUG_KMS("long hpd on eDP port %c\n",
 			      port_name(intel_dig_port->port));
+
 		return IRQ_HANDLED;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e3794d3..95242e6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1411,6 +1411,7 @@ void intel_psr_flush(struct drm_device *dev,
 void intel_psr_init(struct drm_device *dev);
 void intel_psr_single_frame_update(struct drm_device *dev,
 				   unsigned frontbuffer_bits);
+void intel_psr_irq_hpd(struct drm_device *dev);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 213581c..e5ae844 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -741,6 +741,39 @@ void intel_psr_flush(struct drm_device *dev,
 }
 
 /**
+ * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD
+ * @dev: DRM device
+ *
+ * This function is called when IRQ_HPD is received on eDP.
+ */
+void intel_psr_irq_hpd(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int delay_ms = HAS_DDI(dev) ? 100 : 500;
+
+	mutex_lock(&dev_priv->psr.lock);
+
+	/*
+	 * According to VESA spec "If a Source device receives and IRQ_HPD
+	 * while in a PSR active state, and cannot identify what caused the
+	 * IRQ_HPD to be generated, based on Sink device status registers,
+	 * the Source device can take implementation-specific action.
+	 * One such action can be to exit and then re-enter a PSR active
+	 * state." Since we aren't checking for any sink status registers
+	 * and we aren't looking for any other implementation-specific
+	 * action, in case we receive any IRQ_HPD and psr is active let's
+	 * force the exit and reschedule it back.
+	 */
+	if (dev_priv->psr.active) {
+		intel_psr_exit(dev);
+		schedule_delayed_work(&dev_priv->psr.work,
+				      msecs_to_jiffies(delay_ms));
+	}
+
+	mutex_unlock(&dev_priv->psr.lock);
+}
+
+/**
  * intel_psr_init - Init basic PSR work and mutex.
  * @dev: DRM device
  *
-- 
2.4.3

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

[PATCH 2/4] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink.

[Rodrigo Vivi]

This is wrong since my commit (89251b17). The intention of that
commit was to remove this one here that is also wrong anyway,
but it was forgotten.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index e5ae844..e5b3fce 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -172,9 +172,6 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
 
 	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
 
-	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
-			   DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
-
 	/* Enable AUX frame sync at sink */
 	if (dev_priv->psr.aux_frame_sync)
 		drm_dp_dpcd_writeb(&intel_dp->aux,
-- 
2.4.3

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

[PATCH 3/4] drm/i915: PSR: Let's rely more on frontbuffer tracking.

[Rodrigo Vivi]

Many reasons here:

- Hardware tracking also has hidden corner cases
- Frontbuffer tracking is mature and reliable now
- Our sw exit by unseting bit 31 is really fast and reliable.

Also frontbuffer tracking flush means invalidate and flush.

So, let's rely more and do the proper meaning of flush for
all cases without any workaround.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index e5b3fce..b0e343c 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -711,25 +711,9 @@ void intel_psr_flush(struct drm_device *dev,
 	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
 	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
 
-	if (HAS_DDI(dev)) {
-		/*
-		 * By definition every flush should mean invalidate + flush,
-		 * however on core platforms let's minimize the
-		 * disable/re-enable so we can avoid the invalidate when flip
-		 * originated the flush.
-		 */
-		if (frontbuffer_bits && origin != ORIGIN_FLIP)
-			intel_psr_exit(dev);
-	} else {
-		/*
-		 * On Valleyview and Cherryview we don't use hardware tracking
-		 * so any plane updates or cursor moves don't result in a PSR
-		 * invalidating. Which means we need to manually fake this in
-		 * software for all flushes.
-		 */
-		if (frontbuffer_bits)
-			intel_psr_exit(dev);
-	}
+	/* By definition flush = invalidate + flush */
+	if (frontbuffer_bits)
+		intel_psr_exit(dev);
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
 		schedule_delayed_work(&dev_priv->psr.work,
-- 
2.4.3

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

[PATCH 4/4] drm/i915: PSR: Mask LPSP hw tracking back again.

[Rodrigo Vivi]

At the beginning it was masked to allow PSR at all.
Than it got removed later by my
commit 09108b90f040 ("drm/i915: PSR: Remove Low Power HW tracking mask.")
in order to trying fixing one case reported at intel-gfx mailing list
where we were missing screen updates when runtime_pm was enabled.

However I verified that other patch that makes flush to force
invalidate also fixes this issue by itself.
commit 169de1316c1e ("drm/i915: PSR: Flush means invalidate + flush")

Mainly now that we are relying more on frontbuffer tracking it is a
good idea to mask this hw tracking again.

But besides all this above it is important to hightligh that with LPSP
unmasked we started seeing some screen freezings as reported at fd.o.

v2: Update commit message since this patch by itself doesn't solve
    the bugzilla entries.

Tested-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index b0e343c..b1b88d1 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 				skl_psr_setup_su_vsc(intel_dp);
 		}
 
-		/* Avoid continuous PSR exit by masking memup and hpd */
+		/*
+		 * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD.
+		 * Also mask LPSP to avoid dependency on other drivers that
+		 * might block runtime_pm besides preventing other hw tracking
+		 * issues now we can rely on frontbuffer tracking.
+		 */
 		I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
-			   EDP_PSR_DEBUG_MASK_HPD);
+			   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
 
 		/* Enable PSR on the panel */
 		hsw_psr_enable_sink(intel_dp);
-- 
2.4.3

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

[PATCH] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink.

[Rodrigo Vivi]

Commit (89251b17) intended to remove this line and let only one
DP_PSR_EN_CFG set, but it was wrong and this call is now duplicated
at the code.

Also "& ~DP_PSR_MAIN_LINK_ACTIVE" doesn't do anything at all. It
was like that since I introduced this call but probably the idea
was to be informative and make clear statement that we were not using
the link standby. So it is better to remove this one here and let
the code a bit cleaner.

v2: Improve commit message as requested by Paulo.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Tested-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index e5ae844..e5b3fce 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -172,9 +172,6 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
 
 	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
 
-	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
-			   DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
-
 	/* Enable AUX frame sync at sink */
 	if (dev_priv->psr.aux_frame_sync)
 		drm_dp_dpcd_writeb(&intel_dp->aux,
-- 
2.4.3

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

[PATCH] drm/i915: PSR: Let's rely more on frontbuffer tracking.

[Rodrigo Vivi]

The ultimate goal here is to remove the dependency we
currently have on audio driver power to get PSR working.

With audio driver runtime PM disabled the Hardware tracking
believes graphics is fully active and prevent PSR Entry, or
in other words continuously exit PSR.

When we introduced PSR we let LPSP masked allowing us
to get PSR independtly from the audio runtime PM. However
in one of the attempts to get PSR enabled by default one
user reported one specific case where he would miss
screen updates if scrolling the firefox in a Gnome
environment when i915 runtime pm was enabled. So for
this specific case that (I could never create an i-g-t
test case) we decided to remove the LPSP mask and
let HW tracking taking care of this case. So, we
started depending on audio driver again.

An alternative solution that makes us indepent and also
solve this case is to fully rely on our frontbuffer
tracking that is really mature right now.

From the frontbuffer tracking perspective the flush means
invalidate and flush. But without this patch for HSW, BDW
and SKL we just do the invalidate part when the flush wasn't
originated by a page flip because we were trusting the HW
tracking for the flip case, that is exactly the case with
firefox scrolling on gnome.

So let's rely more on frontbuffer tracking and do the
invalidation regardless the origin as expected and for
all platforms.

v2: Improve commit message as suggested by Paulo.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index e5b3fce..b0e343c 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -711,25 +711,9 @@ void intel_psr_flush(struct drm_device *dev,
 	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
 	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
 
-	if (HAS_DDI(dev)) {
-		/*
-		 * By definition every flush should mean invalidate + flush,
-		 * however on core platforms let's minimize the
-		 * disable/re-enable so we can avoid the invalidate when flip
-		 * originated the flush.
-		 */
-		if (frontbuffer_bits && origin != ORIGIN_FLIP)
-			intel_psr_exit(dev);
-	} else {
-		/*
-		 * On Valleyview and Cherryview we don't use hardware tracking
-		 * so any plane updates or cursor moves don't result in a PSR
-		 * invalidating. Which means we need to manually fake this in
-		 * software for all flushes.
-		 */
-		if (frontbuffer_bits)
-			intel_psr_exit(dev);
-	}
+	/* By definition flush = invalidate + flush */
+	if (frontbuffer_bits)
+		intel_psr_exit(dev);
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
 		schedule_delayed_work(&dev_priv->psr.work,
-- 
2.4.3

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

Re: [PATCH] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink.

[Paulo Zanoni]

2015-11-12 20:46 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> Commit (89251b17) intended to remove this line and let only one
> DP_PSR_EN_CFG set, but it was wrong and this call is now duplicated
> at the code.
>
> Also "& ~DP_PSR_MAIN_LINK_ACTIVE" doesn't do anything at all. It
> was like that since I introduced this call but probably the idea
> was to be informative and make clear statement that we were not using
> the link standby. So it is better to remove this one here and let
> the code a bit cleaner.
>
> v2: Improve commit message as requested by Paulo.
>

Another side effect not mentioned in the commit message is that now
we're flipping the DP_PSR_ENABLE bit a little later than we were
before, but it looks like this is not a problem.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Tested-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index e5ae844..e5b3fce 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -172,9 +172,6 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
>
>         aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
>
> -       drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> -                          DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
> -
>         /* Enable AUX frame sync at sink */
>         if (dev_priv->psr.aux_frame_sync)
>                 drm_dp_dpcd_writeb(&intel_dp->aux,
> --
> 2.4.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

Re: [PATCH 1/4] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP.

[Ville Syrjälä]

On Wed, Nov 11, 2015 at 01:19:58PM -0800, Rodrigo Vivi wrote:
> According to VESA spec: "If a Source device receives and IRQ_HPD
> while in a PSR active state, and cannot identify what caused the
> IRQ_HPD to be generated, based on Sink device status registers,
> the Source device can take implementation-specific action.
> One such action can be to exit and then re-enter a PSR active
> state."
> 
> Since we aren't checking for any sink status registers and we
>  aren't looking for any other implementation-specific action,
> in case we receive any IRQ_HPD and psr is active let's force
> the exit and reschedule it back.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  |  5 ++++-
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 99b7f1d..71911b9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4892,6 +4892,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  
>  	if (intel_dig_port->base.type != INTEL_OUTPUT_EDP)
>  		intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT;
> +	else
> +		intel_psr_irq_hpd(dev);

IRQ_HPD means short HPD. This one is now done for long HPD too. And in
any case the placement is a bit weird since now the two branches here
are totally unrelated.

>  
>  	if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
>  		/*
> @@ -4900,8 +4902,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  		 * would end up in an endless cycle of
>  		 * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
>  		 */
> -		DRM_DEBUG_KMS("ignoring long hpd on eDP port %c\n",
> +		DRM_DEBUG_KMS("long hpd on eDP port %c\n",
>  			      port_name(intel_dig_port->port));
> +
>  		return IRQ_HANDLED;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e3794d3..95242e6 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1411,6 +1411,7 @@ void intel_psr_flush(struct drm_device *dev,
>  void intel_psr_init(struct drm_device *dev);
>  void intel_psr_single_frame_update(struct drm_device *dev,
>  				   unsigned frontbuffer_bits);
> +void intel_psr_irq_hpd(struct drm_device *dev);
>  
>  /* intel_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 213581c..e5ae844 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -741,6 +741,39 @@ void intel_psr_flush(struct drm_device *dev,
>  }
>  
>  /**
> + * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD
> + * @dev: DRM device
> + *
> + * This function is called when IRQ_HPD is received on eDP.
> + */
> +void intel_psr_irq_hpd(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int delay_ms = HAS_DDI(dev) ? 100 : 500;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +
> +	/*
> +	 * According to VESA spec "If a Source device receives and IRQ_HPD
> +	 * while in a PSR active state, and cannot identify what caused the
> +	 * IRQ_HPD to be generated, based on Sink device status registers,
> +	 * the Source device can take implementation-specific action.
> +	 * One such action can be to exit and then re-enter a PSR active
> +	 * state." Since we aren't checking for any sink status registers
> +	 * and we aren't looking for any other implementation-specific
> +	 * action, in case we receive any IRQ_HPD and psr is active let's
> +	 * force the exit and reschedule it back.
> +	 */
> +	if (dev_priv->psr.active) {
> +		intel_psr_exit(dev);
> +		schedule_delayed_work(&dev_priv->psr.work,
> +				      msecs_to_jiffies(delay_ms));
> +	}
> +
> +	mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> +/**
>   * intel_psr_init - Init basic PSR work and mutex.
>   * @dev: DRM device
>   *
> -- 
> 2.4.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

Re: [PATCH] drm/i915: PSR: Let's rely more on frontbuffer tracking.

[Paulo Zanoni]

2015-11-13 22:56 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> The ultimate goal here is to remove the dependency we
> currently have on audio driver power to get PSR working.
>
> With audio driver runtime PM disabled the Hardware tracking
> believes graphics is fully active and prevent PSR Entry, or
> in other words continuously exit PSR.
>
> When we introduced PSR we let LPSP masked allowing us
> to get PSR independtly from the audio runtime PM. However
> in one of the attempts to get PSR enabled by default one
> user reported one specific case where he would miss
> screen updates if scrolling the firefox in a Gnome
> environment when i915 runtime pm was enabled. So for
> this specific case that (I could never create an i-g-t
> test case) we decided to remove the LPSP mask and
> let HW tracking taking care of this case. So, we
> started depending on audio driver again.

Thanks for the better commit message, but I still think there's a huge
gap between the paragraph above and the paragraph below. What is still
not clear to me is: what is the relationship between the LPSP mask
problem mentioned above and the automatic page flip tracking mentioned
below? In other words: why not relying on the automatic HW tracking
solves the bug?

Also, did you do any measurements on how this patch affects PC state
residency on the affected platforms? I expect to see some delta here.

The patch itself looks fine. And we can always look into re-adding the
HW piece after we get the current bugs sorted.

With those two explained in the commit message:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> An alternative solution that makes us indepent and also
> solve this case is to fully rely on our frontbuffer
> tracking that is really mature right now.
>
> From the frontbuffer tracking perspective the flush means
> invalidate and flush. But without this patch for HSW, BDW
> and SKL we just do the invalidate part when the flush wasn't
> originated by a page flip because we were trusting the HW
> tracking for the flip case, that is exactly the case with
> firefox scrolling on gnome.
>
> So let's rely more on frontbuffer tracking and do the
> invalidation regardless the origin as expected and for
> all platforms.
>
> v2: Improve commit message as suggested by Paulo.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index e5b3fce..b0e343c 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -711,25 +711,9 @@ void intel_psr_flush(struct drm_device *dev,
>         frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
>         dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
>
> -       if (HAS_DDI(dev)) {
> -               /*
> -                * By definition every flush should mean invalidate + flush,
> -                * however on core platforms let's minimize the
> -                * disable/re-enable so we can avoid the invalidate when flip
> -                * originated the flush.
> -                */
> -               if (frontbuffer_bits && origin != ORIGIN_FLIP)
> -                       intel_psr_exit(dev);
> -       } else {
> -               /*
> -                * On Valleyview and Cherryview we don't use hardware tracking
> -                * so any plane updates or cursor moves don't result in a PSR
> -                * invalidating. Which means we need to manually fake this in
> -                * software for all flushes.
> -                */
> -               if (frontbuffer_bits)
> -                       intel_psr_exit(dev);
> -       }
> +       /* By definition flush = invalidate + flush */
> +       if (frontbuffer_bits)
> +               intel_psr_exit(dev);
>
>         if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
>                 schedule_delayed_work(&dev_priv->psr.work,
> --
> 2.4.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

Re: [PATCH 4/4] drm/i915: PSR: Mask LPSP hw tracking back again.

[Paulo Zanoni]

2015-11-11 19:20 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> At the beginning it was masked to allow PSR at all.
> Than it got removed later by my
> commit 09108b90f040 ("drm/i915: PSR: Remove Low Power HW tracking mask.")
> in order to trying fixing one case reported at intel-gfx mailing list
> where we were missing screen updates when runtime_pm was enabled.
>
> However I verified that other patch that makes flush to force
> invalidate also fixes this issue by itself.
> commit 169de1316c1e ("drm/i915: PSR: Flush means invalidate + flush")
>
> Mainly now that we are relying more on frontbuffer tracking it is a
> good idea to mask this hw tracking again.

Up until this point you suggest that this patch is not a bug fix, but
it also does not add any regression, and it's convenient since it
stops us from depending on runtime PM.

>
> But besides all this above it is important to hightligh that with LPSP
> unmasked we started seeing some screen freezings as reported at fd.o.

But this sentence suggests the patch actually does fix bugs. But there
are also no references to the fd.o bug.

Does this patch fix any bug or not?

>
> v2: Update commit message since this patch by itself doesn't solve
>     the bugzilla entries.
>
> Tested-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index b0e343c..b1b88d1 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>                                 skl_psr_setup_su_vsc(intel_dp);
>                 }
>
> -               /* Avoid continuous PSR exit by masking memup and hpd */
> +               /*
> +                * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD.
> +                * Also mask LPSP to avoid dependency on other drivers that
> +                * might block runtime_pm besides preventing other hw tracking
> +                * issues now we can rely on frontbuffer tracking.
> +                */
>                 I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
> -                          EDP_PSR_DEBUG_MASK_HPD);
> +                          EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
>
>                 /* Enable PSR on the panel */
>                 hsw_psr_enable_sink(intel_dp);
> --
> 2.4.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

Re: [PATCH] drm/i915: PSR: Let's rely more on frontbuffer tracking.

[Vivi, Rodrigo]

On Mon, 2015-11-16 at 16:57 -0200, Paulo Zanoni wrote:
> 2015-11-13 22:56 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > The ultimate goal here is to remove the dependency we
> > currently have on audio driver power to get PSR working.
> > 
> > With audio driver runtime PM disabled the Hardware tracking
> > believes graphics is fully active and prevent PSR Entry, or
> > in other words continuously exit PSR.
> > 
> > When we introduced PSR we let LPSP masked allowing us
> > to get PSR independtly from the audio runtime PM. However
> > in one of the attempts to get PSR enabled by default one
> > user reported one specific case where he would miss
> > screen updates if scrolling the firefox in a Gnome
> > environment when i915 runtime pm was enabled. So for
> > this specific case that (I could never create an i-g-t
> > test case) we decided to remove the LPSP mask and
> > let HW tracking taking care of this case. So, we
> > started depending on audio driver again.
> 
> Thanks for the better commit message, but I still think there's a 
> huge
> gap between the paragraph above and the paragraph below. What is 
> still
> not clear to me is: what is the relationship between the LPSP mask
> problem mentioned above and the automatic page flip tracking 
> mentioned
> below? In other words: why not relying on the automatic HW tracking
> solves the bug?

So, or we solve by fully relying on SW tracking or we solve by fully
relying on HW tracking. The main disadvantage on the HW tracking is
that we end up depending on audio driver. Since the SW tracking is more
mature and covering all our use cases on Linux is better to rely more
on the SW and implement the function as expected (i.e. flush =
inactivate + flush.)

> 
> Also, did you do any measurements on how this patch affects PC state
> residency on the affected platforms? I expect to see some delta here.

No, I didn't. Yes, it can affect the PC residency, but it depends a lot
on the use case, the environment, etc. 
Although on the idle scenario that is the one that targets maximum PC
residency we should see no difference.



> 
> The patch itself looks fine. And we can always look into re-adding 
> the
> HW piece after we get the current bugs sorted.
> 
> With those two explained in the commit message:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I will prepare another v.
Thanks!

> 
> > 
> > An alternative solution that makes us indepent and also
> > solve this case is to fully rely on our frontbuffer
> > tracking that is really mature right now.
> > 
> > From the frontbuffer tracking perspective the flush means
> > invalidate and flush. But without this patch for HSW, BDW
> > and SKL we just do the invalidate part when the flush wasn't
> > originated by a page flip because we were trusting the HW
> > tracking for the flip case, that is exactly the case with
> > firefox scrolling on gnome.
> > 
> > So let's rely more on frontbuffer tracking and do the
> > invalidation regardless the origin as expected and for
> > all platforms.
> > 
> > v2: Improve commit message as suggested by Paulo.
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 22 +++-------------------
> >  1 file changed, 3 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index e5b3fce..b0e343c 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -711,25 +711,9 @@ void intel_psr_flush(struct drm_device *dev,
> >         frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
> >         dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
> > 
> > -       if (HAS_DDI(dev)) {
> > -               /*
> > -                * By definition every flush should mean invalidate 
> > + flush,
> > -                * however on core platforms let's minimize the
> > -                * disable/re-enable so we can avoid the invalidate 
> > when flip
> > -                * originated the flush.
> > -                */
> > -               if (frontbuffer_bits && origin != ORIGIN_FLIP)
> > -                       intel_psr_exit(dev);
> > -       } else {
> > -               /*
> > -                * On Valleyview and Cherryview we don't use 
> > hardware tracking
> > -                * so any plane updates or cursor moves don't 
> > result in a PSR
> > -                * invalidating. Which means we need to manually 
> > fake this in
> > -                * software for all flushes.
> > -                */
> > -               if (frontbuffer_bits)
> > -                       intel_psr_exit(dev);
> > -       }
> > +       /* By definition flush = invalidate + flush */
> > +       if (frontbuffer_bits)
> > +               intel_psr_exit(dev);
> > 
> >         if (!dev_priv->psr.active && !dev_priv
> > ->psr.busy_frontbuffer_bits)
> >                 schedule_delayed_work(&dev_priv->psr.work,
> > --
> > 2.4.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 4/4] drm/i915: PSR: Mask LPSP hw tracking back again.

[Vivi, Rodrigo]

On Mon, 2015-11-16 at 17:27 -0200, Paulo Zanoni wrote:
> 2015-11-11 19:20 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > At the beginning it was masked to allow PSR at all.
> > Than it got removed later by my
> > commit 09108b90f040 ("drm/i915: PSR: Remove Low Power HW tracking 
> > mask.")
> > in order to trying fixing one case reported at intel-gfx mailing 
> > list
> > where we were missing screen updates when runtime_pm was enabled.
> > 
> > However I verified that other patch that makes flush to force
> > invalidate also fixes this issue by itself.
> > commit 169de1316c1e ("drm/i915: PSR: Flush means invalidate + 
> > flush")
> > 
> > Mainly now that we are relying more on frontbuffer tracking it is a
> > good idea to mask this hw tracking again.
> 
> Up until this point you suggest that this patch is not a bug fix, but
> it also does not add any regression, and it's convenient since it
> stops us from depending on runtime PM.
> 
> > 
> > But besides all this above it is important to hightligh that with 
> > LPSP
> > unmasked we started seeing some screen freezings as reported at 
> > fd.o.
> 
> But this sentence suggests the patch actually does fix bugs. But 
> there
> are also no references to the fd.o bug.
> 
> Does this patch fix any bug or not?

Sorry, this was a confusion. The bug was fixed by another patch so
nothing to worrie. I will update the commit message here.

> 
> > 
> > v2: Update commit message since this patch by itself doesn't solve
> >     the bugzilla entries.
> > 
> > Tested-by: Brian Norris <briannorris@chromium.org>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index b0e343c..b1b88d1 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp 
> > *intel_dp)
> >                                 skl_psr_setup_su_vsc(intel_dp);
> >                 }
> > 
> > -               /* Avoid continuous PSR exit by masking memup and 
> > hpd */
> > +               /*
> > +                * Per Spec: Avoid continuous PSR exit by masking 
> > MEMUP and HPD.
> > +                * Also mask LPSP to avoid dependency on other 
> > drivers that
> > +                * might block runtime_pm besides preventing other 
> > hw tracking
> > +                * issues now we can rely on frontbuffer tracking.
> > +                */
> >                 I915_WRITE(EDP_PSR_DEBUG_CTL(dev), 
> > EDP_PSR_DEBUG_MASK_MEMUP |
> > -                          EDP_PSR_DEBUG_MASK_HPD);
> > +                          EDP_PSR_DEBUG_MASK_HPD | 
> > EDP_PSR_DEBUG_MASK_LPSP);
> > 
> >                 /* Enable PSR on the panel */
> >                 hsw_psr_enable_sink(intel_dp);
> > --
> > 2.4.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

[PATCH] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP.

[Rodrigo Vivi]

According to VESA spec: "If a Source device receives and IRQ_HPD
while in a PSR active state, and cannot identify what caused the
IRQ_HPD to be generated, based on Sink device status registers,
the Source device can take implementation-specific action.
One such action can be to exit and then re-enter a PSR active
state."

Since we aren't checking for any sink status registers and we
 aren't looking for any other implementation-specific action,
in case we receive any IRQ_HPD and psr is active let's force
the exit and reschedule it back.

v2: IRQ_HPD means short HPD so handle it correctly
    as Ville pointed out.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  |  3 +++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index bec443a..ca7a798 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5004,6 +5004,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 			goto mst_fail;
 		}
 	} else {
+		if (intel_dig_port->base.type == INTEL_OUTPUT_EDP)
+			intel_psr_irq_hpd(dev);
+
 		if (intel_dp->is_mst) {
 			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
 				goto mst_fail;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ab5c147..1d61551 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1401,6 +1401,7 @@ void intel_psr_flush(struct drm_device *dev,
 void intel_psr_init(struct drm_device *dev);
 void intel_psr_single_frame_update(struct drm_device *dev,
 				   unsigned frontbuffer_bits);
+void intel_psr_irq_hpd(struct drm_device *dev);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index bc5ea2a..465d36b 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -765,6 +765,39 @@ void intel_psr_flush(struct drm_device *dev,
 }
 
 /**
+ * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD
+ * @dev: DRM device
+ *
+ * This function is called when IRQ_HPD is received on eDP.
+ */
+void intel_psr_irq_hpd(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int delay_ms = HAS_DDI(dev) ? 100 : 500;
+
+	mutex_lock(&dev_priv->psr.lock);
+
+	/*
+	 * According to VESA spec "If a Source device receives and IRQ_HPD
+	 * while in a PSR active state, and cannot identify what caused the
+	 * IRQ_HPD to be generated, based on Sink device status registers,
+	 * the Source device can take implementation-specific action.
+	 * One such action can be to exit and then re-enter a PSR active
+	 * state." Since we aren't checking for any sink status registers
+	 * and we aren't looking for any other implementation-specific
+	 * action, in case we receive any IRQ_HPD and psr is active let's
+	 * force the exit and reschedule it back.
+	 */
+	if (dev_priv->psr.active) {
+		intel_psr_exit(dev);
+		schedule_delayed_work(&dev_priv->psr.work,
+				      msecs_to_jiffies(delay_ms));
+	}
+
+	mutex_unlock(&dev_priv->psr.lock);
+}
+
+/**
  * intel_psr_init - Init basic PSR work and mutex.
  * @dev: DRM device
  *
-- 
2.4.3

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

[PATCH 1/2] drm/i915: PSR: Let's rely more on frontbuffer tracking.

[Rodrigo Vivi]

The ultimate goal here is to remove the dependency we
currently have on audio driver power to get PSR working.
Since with audio driver runtime PM disabled the Hardware tracking
believes graphics is fully active and prevent PSR Entry, or
in other words continuously exit PSR.

So, the idea is to transfer the PSR exit responsability
from the HW tracking to the SW tracking (frontbuffer tracking),
who is really mature right now.

However with LPSP masked out there might be cases where we could
miss exit from HW tracking since it can be relying on this,
like a specific case reported at our mailing list who
user reported he would miss screen updates if scrolling firefox
in a Gnome environment when i915 runtimepm was enabled.

So before masking out LPSP again to make us independent from
the audio driver we need to make sure that all our cases
are coverred from the frontbuffer tracking perspective,
where the flush means invalidate and flush.

Without this patch for HSW, BDW and SKL we just do the
invalidate part when the flush wasn't originated by a page flip
because we were trusting the HW tracking for the flip case.

So let's rely more on frontbuffer tracking and do the
invalidation regardless the origin as expected for all platforms.

v2: Improve commit message as suggested by Paulo.

v3: Another attempt to let commit message more clear.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index e5b3fce..b0e343c 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -711,25 +711,9 @@ void intel_psr_flush(struct drm_device *dev,
 	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
 	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
 
-	if (HAS_DDI(dev)) {
-		/*
-		 * By definition every flush should mean invalidate + flush,
-		 * however on core platforms let's minimize the
-		 * disable/re-enable so we can avoid the invalidate when flip
-		 * originated the flush.
-		 */
-		if (frontbuffer_bits && origin != ORIGIN_FLIP)
-			intel_psr_exit(dev);
-	} else {
-		/*
-		 * On Valleyview and Cherryview we don't use hardware tracking
-		 * so any plane updates or cursor moves don't result in a PSR
-		 * invalidating. Which means we need to manually fake this in
-		 * software for all flushes.
-		 */
-		if (frontbuffer_bits)
-			intel_psr_exit(dev);
-	}
+	/* By definition flush = invalidate + flush */
+	if (frontbuffer_bits)
+		intel_psr_exit(dev);
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
 		schedule_delayed_work(&dev_priv->psr.work,
-- 
2.4.3

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

[PATCH 2/2] drm/i915: PSR: Mask LPSP hw tracking back again.

[Rodrigo Vivi]

When we introduced PSR we let LPSP masked allowing us to get PSR
independently from the audio runtime PM. However in one of the
attempts to get PSR enabled by default one user reported one specific
case where he would miss screen updates if scrolling the firefox in a
Gnome environment when i915 runtime pm was enabled. So for
this specific case that (I could never create an i-g-t test case)
we decided to remove the LPSP mask and let HW tracking taking care of
this case. The mask got removed later by my
commit 09108b90f04 ("drm/i915: PSR: Remove Low Power HW tracking mask.")

So we started depending on audio driver again, what is bad.

With previous commit
"drm/i915: PSR: Let's rely more on frontbuffer tracking."
we transfered the PSR exit responsability totally to SW frontbuffer
tracking. So now can safelly shut off a bit the HW tracking, or
at least this case that makes us to depend on other drivers.

v2: Update commit message since this patch by itself doesn't solve
    the bugzilla entries.

v3: Another attempt to improve commit message.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Tested-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index b0e343c..b1b88d1 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 				skl_psr_setup_su_vsc(intel_dp);
 		}
 
-		/* Avoid continuous PSR exit by masking memup and hpd */
+		/*
+		 * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD.
+		 * Also mask LPSP to avoid dependency on other drivers that
+		 * might block runtime_pm besides preventing other hw tracking
+		 * issues now we can rely on frontbuffer tracking.
+		 */
 		I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
-			   EDP_PSR_DEBUG_MASK_HPD);
+			   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
 
 		/* Enable PSR on the panel */
 		hsw_psr_enable_sink(intel_dp);
-- 
2.4.3

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

Re: [PATCH 1/2] drm/i915: PSR: Let's rely more on frontbuffer tracking.

[Zanoni, Paulo R]

Em Qua, 2015-11-18 às 11:21 -0800, Rodrigo Vivi escreveu:
> The ultimate goal here is to remove the dependency we
> currently have on audio driver power to get PSR working.
> Since with audio driver runtime PM disabled the Hardware tracking
> believes graphics is fully active and prevent PSR Entry, or
> in other words continuously exit PSR.
> 
> So, the idea is to transfer the PSR exit responsability
> from the HW tracking to the SW tracking (frontbuffer tracking),
> who is really mature right now.
> 
> However with LPSP masked out there might be cases where we could
> miss exit from HW tracking since it can be relying on this,
> like a specific case reported at our mailing list who
> user reported he would miss screen updates if scrolling firefox
> in a Gnome environment when i915 runtimepm was enabled.
> 
> So before masking out LPSP again to make us independent from
> the audio driver we need to make sure that all our cases
> are coverred from the frontbuffer tracking perspective,
> where the flush means invalidate and flush.
> 
> Without this patch for HSW, BDW and SKL we just do the
> invalidate part when the flush wasn't originated by a page flip
> because we were trusting the HW tracking for the flip case.
> 
> So let's rely more on frontbuffer tracking and do the
> invalidation regardless the origin as expected for all platforms.
> 
> v2: Improve commit message as suggested by Paulo.
> 
> v3: Another attempt to let commit message more clear.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
s/Cc/Reviewed-by/

> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index e5b3fce..b0e343c 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -711,25 +711,9 @@ void intel_psr_flush(struct drm_device *dev,
>  	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
>  	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
>  
> -	if (HAS_DDI(dev)) {
> -		/*
> -		 * By definition every flush should mean invalidate
> + flush,
> -		 * however on core platforms let's minimize the
> -		 * disable/re-enable so we can avoid the invalidate
> when flip
> -		 * originated the flush.
> -		 */
> -		if (frontbuffer_bits && origin != ORIGIN_FLIP)
> -			intel_psr_exit(dev);
> -	} else {
> -		/*
> -		 * On Valleyview and Cherryview we don't use
> hardware tracking
> -		 * so any plane updates or cursor moves don't result
> in a PSR
> -		 * invalidating. Which means we need to manually
> fake this in
> -		 * software for all flushes.
> -		 */
> -		if (frontbuffer_bits)
> -			intel_psr_exit(dev);
> -	}
> +	/* By definition flush = invalidate + flush */
> +	if (frontbuffer_bits)
> +		intel_psr_exit(dev);
>  
>  	if (!dev_priv->psr.active && !dev_priv-
> >psr.busy_frontbuffer_bits)
>  		schedule_delayed_work(&dev_priv->psr.work,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: PSR: Mask LPSP hw tracking back again.

[Zanoni, Paulo R]

Em Qua, 2015-11-18 às 11:21 -0800, Rodrigo Vivi escreveu:
> When we introduced PSR we let LPSP masked allowing us to get PSR
> independently from the audio runtime PM. However in one of the
> attempts to get PSR enabled by default one user reported one specific
> case where he would miss screen updates if scrolling the firefox in a
> Gnome environment when i915 runtime pm was enabled. So for
> this specific case that (I could never create an i-g-t test case)
> we decided to remove the LPSP mask and let HW tracking taking care of
> this case. The mask got removed later by my
> commit 09108b90f04 ("drm/i915: PSR: Remove Low Power HW tracking
> mask.")
> 
> So we started depending on audio driver again, what is bad.
> 
> With previous commit
> "drm/i915: PSR: Let's rely more on frontbuffer tracking."
> we transfered the PSR exit responsability totally to SW frontbuffer
> tracking. So now can safelly shut off a bit the HW tracking, or
> at least this case that makes us to depend on other drivers.
> 
> v2: Update commit message since this patch by itself doesn't solve
>     the bugzilla entries.
> 
> v3: Another attempt to improve commit message.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
s/Cc/Reviewed-by/

> Tested-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index b0e343c..b1b88d1 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>  				skl_psr_setup_su_vsc(intel_dp);
>  		}
>  
> -		/* Avoid continuous PSR exit by masking memup and
> hpd */
> +		/*
> +		 * Per Spec: Avoid continuous PSR exit by masking
> MEMUP and HPD.
> +		 * Also mask LPSP to avoid dependency on other
> drivers that
> +		 * might block runtime_pm besides preventing other
> hw tracking
> +		 * issues now we can rely on frontbuffer tracking.
> +		 */
>  		I915_WRITE(EDP_PSR_DEBUG_CTL(dev),
> EDP_PSR_DEBUG_MASK_MEMUP |
> -			   EDP_PSR_DEBUG_MASK_HPD);
> +			   EDP_PSR_DEBUG_MASK_HPD |
> EDP_PSR_DEBUG_MASK_LPSP);
>  
>  		/* Enable PSR on the panel */
>  		hsw_psr_enable_sink(intel_dp);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: PSR: Mask LPSP hw tracking back again.

[Rodrigo Vivi]

When we introduced PSR we let LPSP masked allowing us to get PSR
independently from the audio runtime PM. However in one of the
attempts to get PSR enabled by default one user reported one specific
case where he would miss screen updates if scrolling the firefox in a
Gnome environment when i915 runtime pm was enabled. So for
this specific case that (I could never create an i-g-t test case)
we decided to remove the LPSP mask and let HW tracking taking care of
this case. The mask got removed later by my
commit 09108b90f04 ("drm/i915: PSR: Remove Low Power HW tracking mask.")

So we started depending on audio driver again, what is bad.

With previous commit
"drm/i915: PSR: Let's rely more on frontbuffer tracking."
we transfered the PSR exit responsability totally to SW frontbuffer
tracking. So now can safelly shut off a bit the HW tracking, or
at least this case that makes us to depend on other drivers.

v2: Update commit message since this patch by itself doesn't solve
    the bugzilla entries.

v3: Another attempt to improve commit message.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Tested-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index b0e343c..b1b88d1 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 				skl_psr_setup_su_vsc(intel_dp);
 		}
 
-		/* Avoid continuous PSR exit by masking memup and hpd */
+		/*
+		 * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD.
+		 * Also mask LPSP to avoid dependency on other drivers that
+		 * might block runtime_pm besides preventing other hw tracking
+		 * issues now we can rely on frontbuffer tracking.
+		 */
 		I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
-			   EDP_PSR_DEBUG_MASK_HPD);
+			   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
 
 		/* Enable PSR on the panel */
 		hsw_psr_enable_sink(intel_dp);
-- 
2.4.3

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

Re: [PATCH 1/2] drm/i915: PSR: Let's rely more on frontbuffer tracking.

[Jani Nikula]

On Wed, 18 Nov 2015, "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> wrote:
> Em Qua, 2015-11-18 às 11:21 -0800, Rodrigo Vivi escreveu:
>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> s/Cc/Reviewed-by/

I don't think our patchwork is quite smart enough for that yet, so for
future reference, please be write the whole thing.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: PSR: Let's rely more on frontbuffer tracking.

[Zanoni, Paulo R]

Em Qui, 2015-11-19 às 13:16 +0200, Jani Nikula escreveu:
> On Wed, 18 Nov 2015, "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>
> wrote:
> > Em Qua, 2015-11-18 às 11:21 -0800, Rodrigo Vivi escreveu:
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > s/Cc/Reviewed-by/
> 
> I don't think our patchwork is quite smart enough for that yet, so
> for
> future reference, please be write the whole thing.

I completely forgot about this. Sorry.

I guess that also means that if I say "If you do this and this and
that, then I'll give you Reviewed-by: Paulo Zanoni <paulo.r.zanoni@inte
l.com> it will mark the patch as reviewed even though it needs rework,
right? I guess we just can't trust the PW automatic tag parsing since
it can have either false positives or false negatives. Not until some
major breakthroughs in AI.

Is suggesting deprecating the use of emails as a way to handle patches
still considered trolling?

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

Re: [PATCH 1/2] drm/i915: PSR: Let's rely more on frontbuffer tracking.

[Damien Lespiau]

On Thu, Nov 19, 2015 at 11:24:22AM +0000, Zanoni, Paulo R wrote:
> Em Qui, 2015-11-19 às 13:16 +0200, Jani Nikula escreveu:
> > On Wed, 18 Nov 2015, "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>
> > wrote:
> > > Em Qua, 2015-11-18 às 11:21 -0800, Rodrigo Vivi escreveu:
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > s/Cc/Reviewed-by/
> > 
> > I don't think our patchwork is quite smart enough for that yet, so
> > for
> > future reference, please be write the whole thing.
> 
> I completely forgot about this. Sorry.
> 
> I guess that also means that if I say "If you do this and this and
> that, then I'll give you Reviewed-by: Paulo Zanoni <paulo.r.zanoni@inte
> l.com> it will mark the patch as reviewed even though it needs rework,
> right? I guess we just can't trust the PW automatic tag parsing since
> it can have either false positives or false negatives. Not until some
> major breakthroughs in AI.

The Reviewed-by parsing is quite simple: it will match '^Reviewed-by:'.
That means indenting the tag should be enough to bypass patchwork's
accounting.

That said, I think we need to extend the language we use for review and
make patchwork understand it.  For instance, being able to review
several patches in one go could be done with:

Patches 1-3,6: Reviewed-by: Damien Lespiau damien.lespiau@intel.com

(See: https://github.com/dlespiau/patchwork/issues/27)

> Is suggesting deprecating the use of emails as a way to handle patches
> still considered trolling?

No :)

The way I see it is that it's easier to progressively enhance what we
have to do what we want -- I'll be the first to say patchwork is very
fragile today -- rather than switching to something totally different,
probably closing the door to non-intel contributors and suddenly having
two different systems for core DRM patch handling, among other things.

Either way, we have to try to improve what we have. I took one path but
it doesn't mean that was the best, someone else can always try to take
another set of tools and convince/force people to use that. The goals
are the important bit: test every patch before it's merged into
-nightly, improve the developer experience and patch flow/tracking
along the way.

For the first goal, we are almost there (in terms of patchwork, the CI
system, piglit, intel-gpu-tools, ... still need quite a bit for work):

For instance on series #890, I've uploaded checkpatch.pl test results:

  http://patchwork.freedesktop.org/series/890/

I'll be deploying that checkpatch.pl test in the next week or so as an
example of patchwork/test integration. QA is looking into that
integration with BATs at the moment. Of course, email/notifications are
part of the plan once happy enough with the tests.

For the second goal, it's a long process of small improvements. We're
really not there, but interesting things have been created already: I'm
quite a fan of git-pw to retrieve series from the ml, for those series
correctly parsed that is...

Which leads me to the last thing: parsing things from the ml is fragile.
The main problems are both people and to a lesser extend mailing-lists:
using mails to carry patches does have problems, the vast majority of
problems come from people though. People will get stuff wrong: attach
patches to the wrong message-id, do things that are plain weird like
suddenly attaching patch "2.4/10" as a way to say "oh actually insert a
11th patch in the middle there", typo the review tags, ...

I think the last part is solvable, by having a tool wrapping git
send-email to do the right things, or at least deterministic things,
when sending patches and reviews. That's a bit blue sky stuff, I
certainly would love to get there eventually though.

Thinking about it, it's wouldn't actually be that complicated to have a
start of such a tool, I've captured the first simple thoughts here:
  https://github.com/dlespiau/patchwork/issues/81

Oh well, it's a way longer email than the one I wanted to write.

HTH,

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

Re: [PATCH] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP.

[Rodrigo Vivi]

Hi Ville,

Is it better now?
Could you please review this patch?

Thanks,
Rodrigo.

On Wed, Nov 18, 2015 at 11:19 AM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> According to VESA spec: "If a Source device receives and IRQ_HPD
> while in a PSR active state, and cannot identify what caused the
> IRQ_HPD to be generated, based on Sink device status registers,
> the Source device can take implementation-specific action.
> One such action can be to exit and then re-enter a PSR active
> state."
>
> Since we aren't checking for any sink status registers and we
>  aren't looking for any other implementation-specific action,
> in case we receive any IRQ_HPD and psr is active let's force
> the exit and reschedule it back.
>
> v2: IRQ_HPD means short HPD so handle it correctly
>     as Ville pointed out.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  |  3 +++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 37 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index bec443a..ca7a798 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5004,6 +5004,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>                         goto mst_fail;
>                 }
>         } else {
> +               if (intel_dig_port->base.type == INTEL_OUTPUT_EDP)
> +                       intel_psr_irq_hpd(dev);
> +
>                 if (intel_dp->is_mst) {
>                         if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
>                                 goto mst_fail;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ab5c147..1d61551 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1401,6 +1401,7 @@ void intel_psr_flush(struct drm_device *dev,
>  void intel_psr_init(struct drm_device *dev);
>  void intel_psr_single_frame_update(struct drm_device *dev,
>                                    unsigned frontbuffer_bits);
> +void intel_psr_irq_hpd(struct drm_device *dev);
>
>  /* intel_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index bc5ea2a..465d36b 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -765,6 +765,39 @@ void intel_psr_flush(struct drm_device *dev,
>  }
>
>  /**
> + * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD
> + * @dev: DRM device
> + *
> + * This function is called when IRQ_HPD is received on eDP.
> + */
> +void intel_psr_irq_hpd(struct drm_device *dev)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       int delay_ms = HAS_DDI(dev) ? 100 : 500;
> +
> +       mutex_lock(&dev_priv->psr.lock);
> +
> +       /*
> +        * According to VESA spec "If a Source device receives and IRQ_HPD
> +        * while in a PSR active state, and cannot identify what caused the
> +        * IRQ_HPD to be generated, based on Sink device status registers,
> +        * the Source device can take implementation-specific action.
> +        * One such action can be to exit and then re-enter a PSR active
> +        * state." Since we aren't checking for any sink status registers
> +        * and we aren't looking for any other implementation-specific
> +        * action, in case we receive any IRQ_HPD and psr is active let's
> +        * force the exit and reschedule it back.
> +        */
> +       if (dev_priv->psr.active) {
> +               intel_psr_exit(dev);
> +               schedule_delayed_work(&dev_priv->psr.work,
> +                                     msecs_to_jiffies(delay_ms));
> +       }
> +
> +       mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> +/**
>   * intel_psr_init - Init basic PSR work and mutex.
>   * @dev: DRM device
>   *
> --
> 2.4.3
>
> _______________________________________________
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: PSR: Mask LPSP hw tracking back again.

[Rodrigo Vivi]

Hi Daniel

Would you please consider merging patches 2,3 and 4 from this series
that are ready to get merged?
They don't depend on patch 1 that is under review yet.

Thanks,

On Wed, Nov 18, 2015 at 1:49 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> When we introduced PSR we let LPSP masked allowing us to get PSR
> independently from the audio runtime PM. However in one of the
> attempts to get PSR enabled by default one user reported one specific
> case where he would miss screen updates if scrolling the firefox in a
> Gnome environment when i915 runtime pm was enabled. So for
> this specific case that (I could never create an i-g-t test case)
> we decided to remove the LPSP mask and let HW tracking taking care of
> this case. The mask got removed later by my
> commit 09108b90f04 ("drm/i915: PSR: Remove Low Power HW tracking mask.")
>
> So we started depending on audio driver again, what is bad.
>
> With previous commit
> "drm/i915: PSR: Let's rely more on frontbuffer tracking."
> we transfered the PSR exit responsability totally to SW frontbuffer
> tracking. So now can safelly shut off a bit the HW tracking, or
> at least this case that makes us to depend on other drivers.
>
> v2: Update commit message since this patch by itself doesn't solve
>     the bugzilla entries.
>
> v3: Another attempt to improve commit message.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Tested-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index b0e343c..b1b88d1 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>                                 skl_psr_setup_su_vsc(intel_dp);
>                 }
>
> -               /* Avoid continuous PSR exit by masking memup and hpd */
> +               /*
> +                * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD.
> +                * Also mask LPSP to avoid dependency on other drivers that
> +                * might block runtime_pm besides preventing other hw tracking
> +                * issues now we can rely on frontbuffer tracking.
> +                */
>                 I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
> -                          EDP_PSR_DEBUG_MASK_HPD);
> +                          EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
>
>                 /* Enable PSR on the panel */
>                 hsw_psr_enable_sink(intel_dp);
> --
> 2.4.3
>
> _______________________________________________
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: PSR: Mask LPSP hw tracking back again.

[Daniel Vetter]

On Mon, Nov 23, 2015 at 01:52:37PM -0800, Rodrigo Vivi wrote:
> Hi Daniel
> 
> Would you please consider merging patches 2,3 and 4 from this series
> that are ready to get merged?
> They don't depend on patch 1 that is under review yet.

Done.
-Daniel

> 
> Thanks,
> 
> On Wed, Nov 18, 2015 at 1:49 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > When we introduced PSR we let LPSP masked allowing us to get PSR
> > independently from the audio runtime PM. However in one of the
> > attempts to get PSR enabled by default one user reported one specific
> > case where he would miss screen updates if scrolling the firefox in a
> > Gnome environment when i915 runtime pm was enabled. So for
> > this specific case that (I could never create an i-g-t test case)
> > we decided to remove the LPSP mask and let HW tracking taking care of
> > this case. The mask got removed later by my
> > commit 09108b90f04 ("drm/i915: PSR: Remove Low Power HW tracking mask.")
> >
> > So we started depending on audio driver again, what is bad.
> >
> > With previous commit
> > "drm/i915: PSR: Let's rely more on frontbuffer tracking."
> > we transfered the PSR exit responsability totally to SW frontbuffer
> > tracking. So now can safelly shut off a bit the HW tracking, or
> > at least this case that makes us to depend on other drivers.
> >
> > v2: Update commit message since this patch by itself doesn't solve
> >     the bugzilla entries.
> >
> > v3: Another attempt to improve commit message.
> >
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Tested-by: Brian Norris <briannorris@chromium.org>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index b0e343c..b1b88d1 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp *intel_dp)
> >                                 skl_psr_setup_su_vsc(intel_dp);
> >                 }
> >
> > -               /* Avoid continuous PSR exit by masking memup and hpd */
> > +               /*
> > +                * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD.
> > +                * Also mask LPSP to avoid dependency on other drivers that
> > +                * might block runtime_pm besides preventing other hw tracking
> > +                * issues now we can rely on frontbuffer tracking.
> > +                */
> >                 I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
> > -                          EDP_PSR_DEBUG_MASK_HPD);
> > +                          EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
> >
> >                 /* Enable PSR on the panel */
> >                 hsw_psr_enable_sink(intel_dp);
> > --
> > 2.4.3
> >
> > _______________________________________________
> > 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

-- 
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 0/4] PSR general improvements and stabilization.

[Daniel Stone]

Hi Rodrigo,

On 11 November 2015 at 21:19, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> Proceeding with the big series split here goes the general PSR
> improvements and stabilization work.
>
> There is no critical fix on this series although I believe it is
> good to have all of them before we can enable PSR back by default.

For the series:
Tested-by: Daniel Stone <daniels@collabora.com> # SKL

I haven't managed to get to BDW yet since that relies on IPS bits.

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

Re: [PATCH 0/4] PSR general improvements and stabilization.

[Vivi, Rodrigo]

On Tue, 2015-11-24 at 17:12 +0000, Daniel Stone wrote:
> Hi Rodrigo,
> 
> On 11 November 2015 at 21:19, Rodrigo Vivi <rodrigo.vivi@intel.com> 
> wrote:
> > Proceeding with the big series split here goes the general PSR
> > improvements and stabilization work.
> > 
> > There is no critical fix on this series although I believe it is
> > good to have all of them before we can enable PSR back by default.
> 
> For the series:
> Tested-by: Daniel Stone <daniels@collabora.com> # SKL

Thank you very much for this and the aux one. 

> 
> I haven't managed to get to BDW yet since that relies on IPS bits.

Oh, Daniel or Jani reverted the fastboot so PSR is just working well on
BDW regardless that initialization rework.

I'm still going to do the encoder fixup that Daniel suggest and accept
your suggestions on the IPS one, but they shouldn't be blocking any of
the PSR work anymore.

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

Re: [PATCH 0/4] PSR general improvements and stabilization.

[Daniel Vetter]

On Tue, Nov 24, 2015 at 08:53:43PM +0000, Vivi, Rodrigo wrote:
> On Tue, 2015-11-24 at 17:12 +0000, Daniel Stone wrote:
> > Hi Rodrigo,
> > 
> > On 11 November 2015 at 21:19, Rodrigo Vivi <rodrigo.vivi@intel.com> 
> > wrote:
> > > Proceeding with the big series split here goes the general PSR
> > > improvements and stabilization work.
> > > 
> > > There is no critical fix on this series although I believe it is
> > > good to have all of them before we can enable PSR back by default.
> > 
> > For the series:
> > Tested-by: Daniel Stone <daniels@collabora.com> # SKL
> 
> Thank you very much for this and the aux one. 
> 
> > 
> > I haven't managed to get to BDW yet since that relies on IPS bits.
> 
> Oh, Daniel or Jani reverted the fastboot so PSR is just working well on
> BDW regardless that initialization rework.
> 
> I'm still going to do the encoder fixup that Daniel suggest and accept
> your suggestions on the IPS one, but they shouldn't be blocking any of
> the PSR work anymore.

Yeah, my plan is to re-enable fastboot just in -nightly for better test
coverage. Similar to how we enable psr/fbc and all that stuff in
testcases, for better coverage.
-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

Re: [PATCH] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP.

[Ville Syrjälä]

On Wed, Nov 18, 2015 at 11:19:06AM -0800, Rodrigo Vivi wrote:
> According to VESA spec: "If a Source device receives and IRQ_HPD
> while in a PSR active state, and cannot identify what caused the
> IRQ_HPD to be generated, based on Sink device status registers,
> the Source device can take implementation-specific action.
> One such action can be to exit and then re-enter a PSR active
> state."
> 
> Since we aren't checking for any sink status registers and we
>  aren't looking for any other implementation-specific action,
> in case we receive any IRQ_HPD and psr is active let's force
> the exit and reschedule it back.
> 
> v2: IRQ_HPD means short HPD so handle it correctly
>     as Ville pointed out.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Do we have a device that needs this for something? Since it's totally
implementation specific I would expect that something like VBT would
tell us if we need to do something on random short HPDs from the panel.

As far as the implementation goes, can't we just call intel_psr_flush()
so that the frontbuffer tracking keeps in sync with the actual PSR
state?

> ---
>  drivers/gpu/drm/i915/intel_dp.c  |  3 +++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index bec443a..ca7a798 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5004,6 +5004,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  			goto mst_fail;
>  		}
>  	} else {
> +		if (intel_dig_port->base.type == INTEL_OUTPUT_EDP)
> +			intel_psr_irq_hpd(dev);
> +
>  		if (intel_dp->is_mst) {
>  			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
>  				goto mst_fail;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ab5c147..1d61551 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1401,6 +1401,7 @@ void intel_psr_flush(struct drm_device *dev,
>  void intel_psr_init(struct drm_device *dev);
>  void intel_psr_single_frame_update(struct drm_device *dev,
>  				   unsigned frontbuffer_bits);
> +void intel_psr_irq_hpd(struct drm_device *dev);
>  
>  /* intel_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index bc5ea2a..465d36b 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -765,6 +765,39 @@ void intel_psr_flush(struct drm_device *dev,
>  }
>  
>  /**
> + * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD
> + * @dev: DRM device
> + *
> + * This function is called when IRQ_HPD is received on eDP.
> + */
> +void intel_psr_irq_hpd(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int delay_ms = HAS_DDI(dev) ? 100 : 500;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +
> +	/*
> +	 * According to VESA spec "If a Source device receives and IRQ_HPD
> +	 * while in a PSR active state, and cannot identify what caused the
> +	 * IRQ_HPD to be generated, based on Sink device status registers,
> +	 * the Source device can take implementation-specific action.
> +	 * One such action can be to exit and then re-enter a PSR active
> +	 * state." Since we aren't checking for any sink status registers
> +	 * and we aren't looking for any other implementation-specific
> +	 * action, in case we receive any IRQ_HPD and psr is active let's
> +	 * force the exit and reschedule it back.
> +	 */
> +	if (dev_priv->psr.active) {
> +		intel_psr_exit(dev);
> +		schedule_delayed_work(&dev_priv->psr.work,
> +				      msecs_to_jiffies(delay_ms));
> +	}
> +
> +	mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> +/**
>   * intel_psr_init - Init basic PSR work and mutex.
>   * @dev: DRM device
>   *
> -- 
> 2.4.3

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP.

[Vivi, Rodrigo]

On Tue, 2015-12-01 at 20:56 +0200, Ville Syrjälä wrote:
> On Wed, Nov 18, 2015 at 11:19:06AM -0800, Rodrigo Vivi wrote:
> > According to VESA spec: "If a Source device receives and IRQ_HPD
> > while in a PSR active state, and cannot identify what caused the
> > IRQ_HPD to be generated, based on Sink device status registers,
> > the Source device can take implementation-specific action.
> > One such action can be to exit and then re-enter a PSR active
> > state."
> > 
> > Since we aren't checking for any sink status registers and we
> >  aren't looking for any other implementation-specific action,
> > in case we receive any IRQ_HPD and psr is active let's force
> > the exit and reschedule it back.
> > 
> > v2: IRQ_HPD means short HPD so handle it correctly
> >     as Ville pointed out.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Do we have a device that needs this for something? 

hopefully not. and I'm glad that I never had a case with PSR that I
needed it.... I was just trying to follow VESA spec a bit to avoid
issues.

I've seeing short pulses with some KBL, but also that came along with
bad flickerings so PSR shouldn't be the worst problem when this short
pulse on port A happens ;)

> Since it's totally
> implementation specific I would expect that something like VBT would
> tell us if we need to do something on random short HPDs from the 
> panel.

hm, I was more afraid from the Sink implementation side... if they were
generating IRQ_HPD for some errors where they believe HW will send a
frame update.

> 
> As far as the implementation goes, can't we just call 
> intel_psr_flush()
> so that the frontbuffer tracking keeps in sync with the actual PSR
> state?

that is a good idea... I believe when I first added this patch we
weren't relying fully on flush being inactivate+flush, but now with all
other patches merged I believe this would be better.

But what are your overall opinion now? should I come with a v3 using
the flush function or should we just ignore this patch?

Thanks for your review and comments!

> 
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  |  3 +++
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 33 
> > +++++++++++++++++++++++++++++++++
> >  3 files changed, 37 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index bec443a..ca7a798 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5004,6 +5004,9 @@ intel_dp_hpd_pulse(struct intel_digital_port 
> > *intel_dig_port, bool long_hpd)
> >  			goto mst_fail;
> >  		}
> >  	} else {
> > +		if (intel_dig_port->base.type == INTEL_OUTPUT_EDP)
> > +			intel_psr_irq_hpd(dev);
> > +
> >  		if (intel_dp->is_mst) {
> >  			if (intel_dp_check_mst_status(intel_dp) == 
> > -EINVAL)
> >  				goto mst_fail;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index ab5c147..1d61551 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1401,6 +1401,7 @@ void intel_psr_flush(struct drm_device *dev,
> >  void intel_psr_init(struct drm_device *dev);
> >  void intel_psr_single_frame_update(struct drm_device *dev,
> >  				   unsigned frontbuffer_bits);
> > +void intel_psr_irq_hpd(struct drm_device *dev);
> >  
> >  /* intel_runtime_pm.c */
> >  int intel_power_domains_init(struct drm_i915_private *);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index bc5ea2a..465d36b 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -765,6 +765,39 @@ void intel_psr_flush(struct drm_device *dev,
> >  }
> >  
> >  /**
> > + * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD
> > + * @dev: DRM device
> > + *
> > + * This function is called when IRQ_HPD is received on eDP.
> > + */
> > +void intel_psr_irq_hpd(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int delay_ms = HAS_DDI(dev) ? 100 : 500;
> > +
> > +	mutex_lock(&dev_priv->psr.lock);
> > +
> > +	/*
> > +	 * According to VESA spec "If a Source device receives and 
> > IRQ_HPD
> > +	 * while in a PSR active state, and cannot identify what 
> > caused the
> > +	 * IRQ_HPD to be generated, based on Sink device status 
> > registers,
> > +	 * the Source device can take implementation-specific 
> > action.
> > +	 * One such action can be to exit and then re-enter a PSR 
> > active
> > +	 * state." Since we aren't checking for any sink status 
> > registers
> > +	 * and we aren't looking for any other implementation
> > -specific
> > +	 * action, in case we receive any IRQ_HPD and psr is 
> > active let's
> > +	 * force the exit and reschedule it back.
> > +	 */
> > +	if (dev_priv->psr.active) {
> > +		intel_psr_exit(dev);
> > +		schedule_delayed_work(&dev_priv->psr.work,
> > +				      msecs_to_jiffies(delay_ms));
> > +	}
> > +
> > +	mutex_unlock(&dev_priv->psr.lock);
> > +}
> > +
> > +/**
> >   * intel_psr_init - Init basic PSR work and mutex.
> >   * @dev: DRM device
> >   *
> > -- 
> > 2.4.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP.

[Ville Syrjälä]

On Tue, Dec 01, 2015 at 07:44:00PM +0000, Vivi, Rodrigo wrote:
> On Tue, 2015-12-01 at 20:56 +0200, Ville Syrjälä wrote:
> > On Wed, Nov 18, 2015 at 11:19:06AM -0800, Rodrigo Vivi wrote:
> > > According to VESA spec: "If a Source device receives and IRQ_HPD
> > > while in a PSR active state, and cannot identify what caused the
> > > IRQ_HPD to be generated, based on Sink device status registers,
> > > the Source device can take implementation-specific action.
> > > One such action can be to exit and then re-enter a PSR active
> > > state."
> > > 
> > > Since we aren't checking for any sink status registers and we
> > >  aren't looking for any other implementation-specific action,
> > > in case we receive any IRQ_HPD and psr is active let's force
> > > the exit and reschedule it back.
> > > 
> > > v2: IRQ_HPD means short HPD so handle it correctly
> > >     as Ville pointed out.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > Do we have a device that needs this for something? 
> 
> hopefully not. and I'm glad that I never had a case with PSR that I
> needed it.... I was just trying to follow VESA spec a bit to avoid
> issues.
> 
> I've seeing short pulses with some KBL, but also that came along with
> bad flickerings so PSR shouldn't be the worst problem when this short
> pulse on port A happens ;)
> 
> > Since it's totally
> > implementation specific I would expect that something like VBT would
> > tell us if we need to do something on random short HPDs from the 
> > panel.
> 
> hm, I was more afraid from the Sink implementation side... if they were
> generating IRQ_HPD for some errors where they believe HW will send a
> frame update.
> 
> > 
> > As far as the implementation goes, can't we just call 
> > intel_psr_flush()
> > so that the frontbuffer tracking keeps in sync with the actual PSR
> > state?
> 
> that is a good idea... I believe when I first added this patch we
> weren't relying fully on flush being inactivate+flush, but now with all
> other patches merged I believe this would be better.
> 
> But what are your overall opinion now? should I come with a v3 using
> the flush function or should we just ignore this patch?

Dunno. I suppose it can't do too much harm. Well, unless there's a sink
that keeps signalling something unrelated to us that we don't understand,
and thus would kick it out of PSR all the time.

> 
> Thanks for your review and comments!
> 
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c  |  3 +++
> > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > >  drivers/gpu/drm/i915/intel_psr.c | 33 
> > > +++++++++++++++++++++++++++++++++
> > >  3 files changed, 37 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index bec443a..ca7a798 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -5004,6 +5004,9 @@ intel_dp_hpd_pulse(struct intel_digital_port 
> > > *intel_dig_port, bool long_hpd)
> > >  			goto mst_fail;
> > >  		}
> > >  	} else {
> > > +		if (intel_dig_port->base.type == INTEL_OUTPUT_EDP)
> > > +			intel_psr_irq_hpd(dev);
> > > +
> > >  		if (intel_dp->is_mst) {
> > >  			if (intel_dp_check_mst_status(intel_dp) == 
> > > -EINVAL)
> > >  				goto mst_fail;
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index ab5c147..1d61551 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1401,6 +1401,7 @@ void intel_psr_flush(struct drm_device *dev,
> > >  void intel_psr_init(struct drm_device *dev);
> > >  void intel_psr_single_frame_update(struct drm_device *dev,
> > >  				   unsigned frontbuffer_bits);
> > > +void intel_psr_irq_hpd(struct drm_device *dev);
> > >  
> > >  /* intel_runtime_pm.c */
> > >  int intel_power_domains_init(struct drm_i915_private *);
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index bc5ea2a..465d36b 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -765,6 +765,39 @@ void intel_psr_flush(struct drm_device *dev,
> > >  }
> > >  
> > >  /**
> > > + * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD
> > > + * @dev: DRM device
> > > + *
> > > + * This function is called when IRQ_HPD is received on eDP.
> > > + */
> > > +void intel_psr_irq_hpd(struct drm_device *dev)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	int delay_ms = HAS_DDI(dev) ? 100 : 500;
> > > +
> > > +	mutex_lock(&dev_priv->psr.lock);
> > > +
> > > +	/*
> > > +	 * According to VESA spec "If a Source device receives and 
> > > IRQ_HPD
> > > +	 * while in a PSR active state, and cannot identify what 
> > > caused the
> > > +	 * IRQ_HPD to be generated, based on Sink device status 
> > > registers,
> > > +	 * the Source device can take implementation-specific 
> > > action.
> > > +	 * One such action can be to exit and then re-enter a PSR 
> > > active
> > > +	 * state." Since we aren't checking for any sink status 
> > > registers
> > > +	 * and we aren't looking for any other implementation
> > > -specific
> > > +	 * action, in case we receive any IRQ_HPD and psr is 
> > > active let's
> > > +	 * force the exit and reschedule it back.
> > > +	 */
> > > +	if (dev_priv->psr.active) {
> > > +		intel_psr_exit(dev);
> > > +		schedule_delayed_work(&dev_priv->psr.work,
> > > +				      msecs_to_jiffies(delay_ms));
> > > +	}
> > > +
> > > +	mutex_unlock(&dev_priv->psr.lock);
> > > +}
> > > +
> > > +/**
> > >   * intel_psr_init - Init basic PSR work and mutex.
> > >   * @dev: DRM device
> > >   *
> > > -- 
> > > 2.4.3
> > 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP.

[Vivi, Rodrigo]

On Wed, 2015-12-02 at 11:42 +0200, Ville Syrjälä wrote:
> On Tue, Dec 01, 2015 at 07:44:00PM +0000, Vivi, Rodrigo wrote:
> > On Tue, 2015-12-01 at 20:56 +0200, Ville Syrjälä wrote:
> > > On Wed, Nov 18, 2015 at 11:19:06AM -0800, Rodrigo Vivi wrote:
> > > > According to VESA spec: "If a Source device receives and 
> > > > IRQ_HPD
> > > > while in a PSR active state, and cannot identify what caused 
> > > > the
> > > > IRQ_HPD to be generated, based on Sink device status registers,
> > > > the Source device can take implementation-specific action.
> > > > One such action can be to exit and then re-enter a PSR active
> > > > state."
> > > > 
> > > > Since we aren't checking for any sink status registers and we
> > > >  aren't looking for any other implementation-specific action,
> > > > in case we receive any IRQ_HPD and psr is active let's force
> > > > the exit and reschedule it back.
> > > > 
> > > > v2: IRQ_HPD means short HPD so handle it correctly
> > > >     as Ville pointed out.
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > 
> > > Do we have a device that needs this for something? 
> > 
> > hopefully not. and I'm glad that I never had a case with PSR that I
> > needed it.... I was just trying to follow VESA spec a bit to avoid
> > issues.
> > 
> > I've seeing short pulses with some KBL, but also that came along 
> > with
> > bad flickerings so PSR shouldn't be the worst problem when this 
> > short
> > pulse on port A happens ;)
> > 
> > > Since it's totally
> > > implementation specific I would expect that something like VBT 
> > > would
> > > tell us if we need to do something on random short HPDs from the 
> > > panel.
> > 
> > hm, I was more afraid from the Sink implementation side... if they 
> > were
> > generating IRQ_HPD for some errors where they believe HW will send 
> > a
> > frame update.
> > 
> > > 
> > > As far as the implementation goes, can't we just call 
> > > intel_psr_flush()
> > > so that the frontbuffer tracking keeps in sync with the actual 
> > > PSR
> > > state?
> > 
> > that is a good idea... I believe when I first added this patch we
> > weren't relying fully on flush being inactivate+flush, but now with 
> > all
> > other patches merged I believe this would be better.
> > 
> > But what are your overall opinion now? should I come with a v3 
> > using
> > the flush function or should we just ignore this patch?
> 
> Dunno. I suppose it can't do too much harm. Well, unless there's a 
> sink
> that keeps signalling something unrelated to us that we don't 
> understand,
> and thus would kick it out of PSR all the time.

yeah, good point... So I will hold this one for now if we need I can
rework the v3 with flush and send again in the future.

Thanks,
Rodrigo.

> 
> > 
> > Thanks for your review and comments!
> > 
> > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c  |  3 +++
> > > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > > >  drivers/gpu/drm/i915/intel_psr.c | 33 
> > > > +++++++++++++++++++++++++++++++++
> > > >  3 files changed, 37 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index bec443a..ca7a798 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -5004,6 +5004,9 @@ intel_dp_hpd_pulse(struct 
> > > > intel_digital_port 
> > > > *intel_dig_port, bool long_hpd)
> > > >  			goto mst_fail;
> > > >  		}
> > > >  	} else {
> > > > +		if (intel_dig_port->base.type == 
> > > > INTEL_OUTPUT_EDP)
> > > > +			intel_psr_irq_hpd(dev);
> > > > +
> > > >  		if (intel_dp->is_mst) {
> > > >  			if 
> > > > (intel_dp_check_mst_status(intel_dp) == 
> > > > -EINVAL)
> > > >  				goto mst_fail;
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index ab5c147..1d61551 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1401,6 +1401,7 @@ void intel_psr_flush(struct drm_device 
> > > > *dev,
> > > >  void intel_psr_init(struct drm_device *dev);
> > > >  void intel_psr_single_frame_update(struct drm_device *dev,
> > > >  				   unsigned frontbuffer_bits);
> > > > +void intel_psr_irq_hpd(struct drm_device *dev);
> > > >  
> > > >  /* intel_runtime_pm.c */
> > > >  int intel_power_domains_init(struct drm_i915_private *);
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index bc5ea2a..465d36b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -765,6 +765,39 @@ void intel_psr_flush(struct drm_device 
> > > > *dev,
> > > >  }
> > > >  
> > > >  /**
> > > > + * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD
> > > > + * @dev: DRM device
> > > > + *
> > > > + * This function is called when IRQ_HPD is received on eDP.
> > > > + */
> > > > +void intel_psr_irq_hpd(struct drm_device *dev)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +	int delay_ms = HAS_DDI(dev) ? 100 : 500;
> > > > +
> > > > +	mutex_lock(&dev_priv->psr.lock);
> > > > +
> > > > +	/*
> > > > +	 * According to VESA spec "If a Source device receives 
> > > > and 
> > > > IRQ_HPD
> > > > +	 * while in a PSR active state, and cannot identify 
> > > > what 
> > > > caused the
> > > > +	 * IRQ_HPD to be generated, based on Sink device 
> > > > status 
> > > > registers,
> > > > +	 * the Source device can take implementation-specific 
> > > > action.
> > > > +	 * One such action can be to exit and then re-enter a 
> > > > PSR 
> > > > active
> > > > +	 * state." Since we aren't checking for any sink 
> > > > status 
> > > > registers
> > > > +	 * and we aren't looking for any other implementation
> > > > -specific
> > > > +	 * action, in case we receive any IRQ_HPD and psr is 
> > > > active let's
> > > > +	 * force the exit and reschedule it back.
> > > > +	 */
> > > > +	if (dev_priv->psr.active) {
> > > > +		intel_psr_exit(dev);
> > > > +		schedule_delayed_work(&dev_priv->psr.work,
> > > > +				     
> > > >  msecs_to_jiffies(delay_ms));
> > > > +	}
> > > > +
> > > > +	mutex_unlock(&dev_priv->psr.lock);
> > > > +}
> > > > +
> > > > +/**
> > > >   * intel_psr_init - Init basic PSR work and mutex.
> > > >   * @dev: DRM device
> > > >   *
> > > > -- 
> > > > 2.4.3
> > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx