[PATCH 1/4] drm/i915: PSR: organize setup function.

[PATCH 1/4] drm/i915: PSR: organize setup function.

[Rodrigo Vivi]

psr_enabled is already by itself a setup once so let's put the W/As there and
rename old setup once to setup_vsc.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d9091dc7..271788e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1755,10 +1755,8 @@ static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp,
 	POSTING_READ(ctl_reg);
 }
 
-static void intel_edp_psr_setup(struct intel_dp *intel_dp)
+static void intel_edp_psr_setup_vsc(struct intel_dp *intel_dp)
 {
-	struct drm_device *dev = intel_dp_to_dev(intel_dp);
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct edp_vsc_psr psr_vsc;
 
 	/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
@@ -1768,10 +1766,6 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
 	psr_vsc.sdp_header.HB2 = 0x2;
 	psr_vsc.sdp_header.HB3 = 0x8;
 	intel_edp_psr_write_vsc(intel_dp, &psr_vsc);
-
-	/* Avoid continuous PSR exit by masking memup and hpd */
-	I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
-		   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
 }
 
 static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
@@ -1924,8 +1918,11 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 
-	/* Setup PSR once */
-	intel_edp_psr_setup(intel_dp);
+	intel_edp_psr_setup_vsc(intel_dp);
+
+	/* Avoid continuous PSR exit by masking memup and hpd */
+	I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
+		   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
 
 	if (intel_edp_psr_match_conditions(intel_dp))
 		dev_priv->psr.enabled = intel_dp;
-- 
1.9.3

[PATCH 2/4] drm/i915: PSR: Organize PSR enable function

[Rodrigo Vivi]

We don't need to setup everything else if it doesn't match all conditions.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 271788e..168b3c3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1912,10 +1912,12 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 	mutex_lock(&dev_priv->psr.lock);
 	if (dev_priv->psr.enabled) {
 		DRM_DEBUG_KMS("PSR already in use\n");
-		mutex_unlock(&dev_priv->psr.lock);
-		return;
+		goto unlock;
 	}
 
+	if (!intel_edp_psr_match_conditions(intel_dp))
+		goto unlock;
+
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 
 	intel_edp_psr_setup_vsc(intel_dp);
@@ -1924,8 +1926,8 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 	I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
 		   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
 
-	if (intel_edp_psr_match_conditions(intel_dp))
-		dev_priv->psr.enabled = intel_dp;
+	dev_priv->psr.enabled = intel_dp;
+unlock:
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
-- 
1.9.3

[PATCH 3/4] drm/i915: Avoid re-configure panel on every PSR re-enable.

[Rodrigo Vivi]

The panel has to be reconfigured only when it really loose the power.
The traditional enable/disable sequence already take care of this so we can
minimize the time spend on every re-enable.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 168b3c3..2f0eee5 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1885,10 +1885,7 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 	WARN_ON(dev_priv->psr.active);
 	lockdep_assert_held(&dev_priv->psr.lock);
 
-	/* Enable PSR on the panel */
-	intel_edp_psr_enable_sink(intel_dp);
-
-	/* Enable PSR on the host */
+	/* Enable/Re-enable PSR on the host */
 	intel_edp_psr_enable_source(intel_dp);
 
 	dev_priv->psr.active = true;
@@ -1926,6 +1923,9 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 	I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
 		   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
 
+	/* Enable PSR on the panel */
+	intel_edp_psr_enable_sink(intel_dp);
+
 	dev_priv->psr.enabled = intel_dp;
 unlock:
 	mutex_unlock(&dev_priv->psr.lock);
-- 
1.9.3

[PATCH 4/4] drm/i915: Make sure PSR is ready for been re-enabled.

[Rodrigo Vivi]

Let's make sure PSR is propperly disabled before to re-enabled it.

According to Spec, after disabled PSR CTL, the Idle state might occur
up to 24ms, that is one full frame time (1/refresh rate),
plus SRD exit training time (max of 6ms),
plus SRD aux channel handshake (max of 1.5ms).

So if something went wrong PSR will be disabled until next full
enable/disable setup.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2f0eee5..658a911 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1885,6 +1885,17 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 	WARN_ON(dev_priv->psr.active);
 	lockdep_assert_held(&dev_priv->psr.lock);
 
+	/* We have to make sure PSR is ready for re-enable
+	 * otherwise it keeps disabled until next full enable/disable cycle.
+	 * PSR might take up to 24 ms to get fully disabled
+	 * and be ready for re-enable.
+	 */
+	if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &
+		      EDP_PSR_STATUS_STATE_MASK) == 0, 24)) {
+		DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
+		return;
+	}
+
 	/* Enable/Re-enable PSR on the host */
 	intel_edp_psr_enable_source(intel_dp);
 
-- 
1.9.3

Re: [PATCH 4/4] drm/i915: Make sure PSR is ready for been re-enabled.

[Daniel Vetter]

On Tue, Sep 16, 2014 at 07:19:08PM -0400, Rodrigo Vivi wrote:
> Let's make sure PSR is propperly disabled before to re-enabled it.
> 
> According to Spec, after disabled PSR CTL, the Idle state might occur
> up to 24ms, that is one full frame time (1/refresh rate),
> plus SRD exit training time (max of 6ms),
> plus SRD aux channel handshake (max of 1.5ms).
> 
> So if something went wrong PSR will be disabled until next full
> enable/disable setup.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 2f0eee5..658a911 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1885,6 +1885,17 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
>  	WARN_ON(dev_priv->psr.active);
>  	lockdep_assert_held(&dev_priv->psr.lock);
>  
> +	/* We have to make sure PSR is ready for re-enable
> +	 * otherwise it keeps disabled until next full enable/disable cycle.
> +	 * PSR might take up to 24 ms to get fully disabled
> +	 * and be ready for re-enable.
> +	 */
> +	if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &

24ms = frametime for roughly 40Hz. Looks awfully like something Bspec
authors just pulled out of thin air.

Generally our timeout for waiting for one vblank is 50ms (20Hz, which is a
lot less than any sane panel does even with DRRS).

Overall series looks sane, please sign someone up for detailed review.

Thanks, Daniel

> +		      EDP_PSR_STATUS_STATE_MASK) == 0, 24)) {
> +		DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> +		return;
> +	}
> +
>  	/* Enable/Re-enable PSR on the host */
>  	intel_edp_psr_enable_source(intel_dp);
>  
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: Make sure PSR is ready for been re-enabled.

[Rodrigo Vivi]

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

Yeah, this is true. I'm going to send a v2 with 50ms timeout. I forgot that.

I sign up Paulo for this review! :)

On Wed, Sep 17, 2014 at 8:50 AM, Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Sep 16, 2014 at 07:19:08PM -0400, Rodrigo Vivi wrote:
> > Let's make sure PSR is propperly disabled before to re-enabled it.
> >
> > According to Spec, after disabled PSR CTL, the Idle state might occur
> > up to 24ms, that is one full frame time (1/refresh rate),
> > plus SRD exit training time (max of 6ms),
> > plus SRD aux channel handshake (max of 1.5ms).
> >
> > So if something went wrong PSR will be disabled until next full
> > enable/disable setup.
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> > index 2f0eee5..658a911 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1885,6 +1885,17 @@ static void intel_edp_psr_do_enable(struct
> intel_dp *intel_dp)
> >       WARN_ON(dev_priv->psr.active);
> >       lockdep_assert_held(&dev_priv->psr.lock);
> >
> > +     /* We have to make sure PSR is ready for re-enable
> > +      * otherwise it keeps disabled until next full enable/disable
> cycle.
> > +      * PSR might take up to 24 ms to get fully disabled
> > +      * and be ready for re-enable.
> > +      */
> > +     if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &
>
> 24ms = frametime for roughly 40Hz. Looks awfully like something Bspec
> authors just pulled out of thin air.
>
> Generally our timeout for waiting for one vblank is 50ms (20Hz, which is a
> lot less than any sane panel does even with DRRS).
>
> Overall series looks sane, please sign someone up for detailed review.
>
> Thanks, Daniel
>
> > +                   EDP_PSR_STATUS_STATE_MASK) == 0, 24)) {
> > +             DRM_ERROR("Timed out waiting for PSR Idle for
> re-enable\n");
> > +             return;
> > +     }
> > +
> >       /* Enable/Re-enable PSR on the host */
> >       intel_edp_psr_enable_source(intel_dp);
> >
> > --
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 3807 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 4/4] drm/i915: Make sure PSR is ready for been re-enabled.

[Rodrigo Vivi]

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

Yeah, this is true. I'm going to send a v2 with 50ms timeout. I forgot that.

I sign up Paulo for this review! :)

On Wed, Sep 17, 2014 at 8:50 AM, Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Sep 16, 2014 at 07:19:08PM -0400, Rodrigo Vivi wrote:
> > Let's make sure PSR is propperly disabled before to re-enabled it.
> >
> > According to Spec, after disabled PSR CTL, the Idle state might occur
> > up to 24ms, that is one full frame time (1/refresh rate),
> > plus SRD exit training time (max of 6ms),
> > plus SRD aux channel handshake (max of 1.5ms).
> >
> > So if something went wrong PSR will be disabled until next full
> > enable/disable setup.
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> > index 2f0eee5..658a911 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1885,6 +1885,17 @@ static void intel_edp_psr_do_enable(struct
> intel_dp *intel_dp)
> >       WARN_ON(dev_priv->psr.active);
> >       lockdep_assert_held(&dev_priv->psr.lock);
> >
> > +     /* We have to make sure PSR is ready for re-enable
> > +      * otherwise it keeps disabled until next full enable/disable
> cycle.
> > +      * PSR might take up to 24 ms to get fully disabled
> > +      * and be ready for re-enable.
> > +      */
> > +     if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &
>
> 24ms = frametime for roughly 40Hz. Looks awfully like something Bspec
> authors just pulled out of thin air.
>
> Generally our timeout for waiting for one vblank is 50ms (20Hz, which is a
> lot less than any sane panel does even with DRRS).
>
> Overall series looks sane, please sign someone up for detailed review.
>
> Thanks, Daniel
>
> > +                   EDP_PSR_STATUS_STATE_MASK) == 0, 24)) {
> > +             DRM_ERROR("Timed out waiting for PSR Idle for
> re-enable\n");
> > +             return;
> > +     }
> > +
> >       /* Enable/Re-enable PSR on the host */
> >       intel_edp_psr_enable_source(intel_dp);
> >
> > --
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 3807 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH] drm/i915: Make sure PSR is ready for been re-enabled.

[Rodrigo Vivi]

Let's make sure PSR is propperly disabled before to re-enabled it.

According to Spec, after disabled PSR CTL, the Idle state might occur
up to 24ms, that is one full frame time (1/refresh rate),
plus SRD exit training time (max of 6ms),
plus SRD aux channel handshake (max of 1.5ms).

v2: The 24ms above takes in account 16ms for refresh rate on 60Hz mode. However
on low frequency modes this can take longer. So let's use 50ms for safeness.

So if something went wrong PSR will be disabled until next full
enable/disable setup.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2f0eee5..2e8c544 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1885,6 +1885,17 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 	WARN_ON(dev_priv->psr.active);
 	lockdep_assert_held(&dev_priv->psr.lock);
 
+	/* We have to make sure PSR is ready for re-enable
+	 * otherwise it keeps disabled until next full enable/disable cycle.
+	 * PSR might take some time to get fully disabled
+	 * and be ready for re-enable.
+	 */
+	if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &
+		      EDP_PSR_STATUS_STATE_MASK) == 0, 50)) {
+		DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
+		return;
+	}
+
 	/* Enable/Re-enable PSR on the host */
 	intel_edp_psr_enable_source(intel_dp);
 
-- 
1.9.3

Re: [PATCH 1/4] drm/i915: PSR: organize setup function.

[Paulo Zanoni]

2014-09-16 20:19 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> psr_enabled is already by itself a setup once so let's put the W/As there and
> rename old setup once to setup_vsc.

Yeah, I prefer the new way too.

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

>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index d9091dc7..271788e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1755,10 +1755,8 @@ static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp,
>         POSTING_READ(ctl_reg);
>  }
>
> -static void intel_edp_psr_setup(struct intel_dp *intel_dp)
> +static void intel_edp_psr_setup_vsc(struct intel_dp *intel_dp)
>  {
> -       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> -       struct drm_i915_private *dev_priv = dev->dev_private;
>         struct edp_vsc_psr psr_vsc;
>
>         /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
> @@ -1768,10 +1766,6 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
>         psr_vsc.sdp_header.HB2 = 0x2;
>         psr_vsc.sdp_header.HB3 = 0x8;
>         intel_edp_psr_write_vsc(intel_dp, &psr_vsc);
> -
> -       /* Avoid continuous PSR exit by masking memup and hpd */
> -       I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
> -                  EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
>  }
>
>  static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
> @@ -1924,8 +1918,11 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
>
>         dev_priv->psr.busy_frontbuffer_bits = 0;
>
> -       /* Setup PSR once */
> -       intel_edp_psr_setup(intel_dp);
> +       intel_edp_psr_setup_vsc(intel_dp);
> +
> +       /* Avoid continuous PSR exit by masking memup and hpd */
> +       I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
> +                  EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
>
>         if (intel_edp_psr_match_conditions(intel_dp))
>                 dev_priv->psr.enabled = intel_dp;
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/4] drm/i915: PSR: Organize PSR enable function

[Paulo Zanoni]

2014-09-16 20:19 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> We don't need to setup everything else if it doesn't match all conditions.

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

>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 271788e..168b3c3 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1912,10 +1912,12 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
>         mutex_lock(&dev_priv->psr.lock);
>         if (dev_priv->psr.enabled) {
>                 DRM_DEBUG_KMS("PSR already in use\n");
> -               mutex_unlock(&dev_priv->psr.lock);
> -               return;
> +               goto unlock;
>         }
>
> +       if (!intel_edp_psr_match_conditions(intel_dp))
> +               goto unlock;
> +
>         dev_priv->psr.busy_frontbuffer_bits = 0;
>
>         intel_edp_psr_setup_vsc(intel_dp);
> @@ -1924,8 +1926,8 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
>         I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
>                    EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
>
> -       if (intel_edp_psr_match_conditions(intel_dp))
> -               dev_priv->psr.enabled = intel_dp;
> +       dev_priv->psr.enabled = intel_dp;
> +unlock:
>         mutex_unlock(&dev_priv->psr.lock);
>  }
>
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/4] drm/i915: PSR: Organize PSR enable function

[Daniel Vetter]

On Tue, Sep 23, 2014 at 06:05:21PM -0300, Paulo Zanoni wrote:
> 2014-09-16 20:19 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> > We don't need to setup everything else if it doesn't match all conditions.
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Merged the first two patches to dinq.
-Daniel

> 
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 271788e..168b3c3 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1912,10 +1912,12 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
> >         mutex_lock(&dev_priv->psr.lock);
> >         if (dev_priv->psr.enabled) {
> >                 DRM_DEBUG_KMS("PSR already in use\n");
> > -               mutex_unlock(&dev_priv->psr.lock);
> > -               return;
> > +               goto unlock;
> >         }
> >
> > +       if (!intel_edp_psr_match_conditions(intel_dp))
> > +               goto unlock;
> > +
> >         dev_priv->psr.busy_frontbuffer_bits = 0;
> >
> >         intel_edp_psr_setup_vsc(intel_dp);
> > @@ -1924,8 +1926,8 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
> >         I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
> >                    EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
> >
> > -       if (intel_edp_psr_match_conditions(intel_dp))
> > -               dev_priv->psr.enabled = intel_dp;
> > +       dev_priv->psr.enabled = intel_dp;
> > +unlock:
> >         mutex_unlock(&dev_priv->psr.lock);
> >  }
> >
> > --
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/4] drm/i915: Avoid re-configure panel on every PSR re-enable.

[Paulo Zanoni]

2014-09-16 20:19 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> The panel has to be reconfigured only when it really loose the power.
> The traditional enable/disable sequence already take care of this so we can
> minimize the time spend on every re-enable.

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

>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 168b3c3..2f0eee5 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1885,10 +1885,7 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
>         WARN_ON(dev_priv->psr.active);
>         lockdep_assert_held(&dev_priv->psr.lock);
>
> -       /* Enable PSR on the panel */
> -       intel_edp_psr_enable_sink(intel_dp);
> -
> -       /* Enable PSR on the host */
> +       /* Enable/Re-enable PSR on the host */
>         intel_edp_psr_enable_source(intel_dp);
>
>         dev_priv->psr.active = true;
> @@ -1926,6 +1923,9 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
>         I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
>                    EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
>
> +       /* Enable PSR on the panel */
> +       intel_edp_psr_enable_sink(intel_dp);
> +
>         dev_priv->psr.enabled = intel_dp;
>  unlock:
>         mutex_unlock(&dev_priv->psr.lock);
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: Make sure PSR is ready for been re-enabled.

[Paulo Zanoni]

2014-09-17 14:23 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> Let's make sure PSR is propperly disabled before to re-enabled it.
>
> According to Spec, after disabled PSR CTL, the Idle state might occur
> up to 24ms, that is one full frame time (1/refresh rate),
> plus SRD exit training time (max of 6ms),
> plus SRD aux channel handshake (max of 1.5ms).
>
> v2: The 24ms above takes in account 16ms for refresh rate on 60Hz mode. However
> on low frequency modes this can take longer. So let's use 50ms for safeness.

Well, the patch looks correct, but it doesn't seem to take into
consideration the fact that we already waited for 100ms before
triggering psr.work. Also, we do the wait that you added with psr.lock
locked, so we could be blocking user-space from doing other stuff for
the whole 50ms, and that's an eternity and a half.

So maybe we should tune the schedule_delayed_work() call at
intel_edp_psr_flush() based on the calculation you did above (or just
keep the 100ms, since it seems to be above the timeout for any modes
bigger than 11Hz). And then when we're inside the work function, we
should just I915_READ(EDP_PSR_STATUS_CTL) - instead of doing
wait_for() -, and in case PSR is not idle yet, there's a huge
probability that waiting for more 50ms won't really help. We could
also try to reschedule psr.work to be triggered again in the future in
case the bits we want are not ready, but by doing this we also risk
rescheduling psr.work forever.

More bikeshed on the timeout thing: can't we try discover the exact
amount of time we need to sleep based on the refresh rate? We could
try to look at the mode structure...

tl;dr: if you remove the wait_for() call and keep just the I915_READ,
I can give a R-B tag, but other patches could be acceptable too.


>
> So if something went wrong PSR will be disabled until next full
> enable/disable setup.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 2f0eee5..2e8c544 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1885,6 +1885,17 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
>         WARN_ON(dev_priv->psr.active);
>         lockdep_assert_held(&dev_priv->psr.lock);
>
> +       /* We have to make sure PSR is ready for re-enable
> +        * otherwise it keeps disabled until next full enable/disable cycle.
> +        * PSR might take some time to get fully disabled
> +        * and be ready for re-enable.
> +        */
> +       if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &
> +                     EDP_PSR_STATUS_STATE_MASK) == 0, 50)) {
> +               DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> +               return;
> +       }
> +
>         /* Enable/Re-enable PSR on the host */
>         intel_edp_psr_enable_source(intel_dp);
>
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: Make sure PSR is ready for been re-enabled.

[Daniel Vetter]

On Wed, Sep 24, 2014 at 12:40:20PM -0300, Paulo Zanoni wrote:
> 2014-09-17 14:23 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > Let's make sure PSR is propperly disabled before to re-enabled it.
> >
> > According to Spec, after disabled PSR CTL, the Idle state might occur
> > up to 24ms, that is one full frame time (1/refresh rate),
> > plus SRD exit training time (max of 6ms),
> > plus SRD aux channel handshake (max of 1.5ms).
> >
> > v2: The 24ms above takes in account 16ms for refresh rate on 60Hz mode. However
> > on low frequency modes this can take longer. So let's use 50ms for safeness.
> 
> Well, the patch looks correct, but it doesn't seem to take into
> consideration the fact that we already waited for 100ms before
> triggering psr.work. Also, we do the wait that you added with psr.lock
> locked, so we could be blocking user-space from doing other stuff for
> the whole 50ms, and that's an eternity and a half.
> 
> So maybe we should tune the schedule_delayed_work() call at
> intel_edp_psr_flush() based on the calculation you did above (or just
> keep the 100ms, since it seems to be above the timeout for any modes
> bigger than 11Hz). And then when we're inside the work function, we
> should just I915_READ(EDP_PSR_STATUS_CTL) - instead of doing
> wait_for() -, and in case PSR is not idle yet, there's a huge
> probability that waiting for more 50ms won't really help. We could
> also try to reschedule psr.work to be triggered again in the future in
> case the bits we want are not ready, but by doing this we also risk
> rescheduling psr.work forever.
> 
> More bikeshed on the timeout thing: can't we try discover the exact
> amount of time we need to sleep based on the refresh rate? We could
> try to look at the mode structure...
> 
> tl;dr: if you remove the wait_for() call and keep just the I915_READ,
> I can give a R-B tag, but other patches could be acceptable too.

Hm, I think just moving the wait_for outside of the psr.lock critical
section should be good enough. Only the work item here can enable PSR, so
there's not really a race. And on the disable side we always sync with the
work before shutting down the psr work, so no synchronization issues
either. At worst the dpms off will take a few ms more.

Merged the 3rd patch meanwhile, thanks.
-Daniel

> 
> 
> >
> > So if something went wrong PSR will be disabled until next full
> > enable/disable setup.
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 2f0eee5..2e8c544 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1885,6 +1885,17 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
> >         WARN_ON(dev_priv->psr.active);
> >         lockdep_assert_held(&dev_priv->psr.lock);
> >
> > +       /* We have to make sure PSR is ready for re-enable
> > +        * otherwise it keeps disabled until next full enable/disable cycle.
> > +        * PSR might take some time to get fully disabled
> > +        * and be ready for re-enable.
> > +        */
> > +       if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &
> > +                     EDP_PSR_STATUS_STATE_MASK) == 0, 50)) {
> > +               DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> > +               return;
> > +       }
> > +
> >         /* Enable/Re-enable PSR on the host */
> >         intel_edp_psr_enable_source(intel_dp);
> >
> > --
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: Make sure PSR is ready for been re-enabled.

[Rodrigo Vivi]

Let's make sure PSR is propperly disabled before to re-enabled it.

According to Spec, after disabled PSR CTL, the Idle state might occur
up to 24ms, that is one full frame time (1/refresh rate),
plus SRD exit training time (max of 6ms),
plus SRD aux channel handshake (max of 1.5ms).

So if something went wrong PSR will be disabled until next full
enable/disable setup.

v2: The 24ms above takes in account 16ms for refresh rate on 60Hz mode. However
on low frequency modes this can take longer. So let's use 50ms for safeness.

v3: Move wait out of psr.lock critical area.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a119b9b..b8699b0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2208,6 +2208,17 @@ static void intel_edp_psr_work(struct work_struct *work)
 		container_of(work, typeof(*dev_priv), psr.work.work);
 	struct intel_dp *intel_dp = dev_priv->psr.enabled;
 
+	/* We have to make sure PSR is ready for re-enable
+	 * otherwise it keeps disabled until next full enable/disable cycle.
+	 * PSR might take some time to get fully disabled
+	 * and be ready for re-enable.
+	 */
+	if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev_priv->dev)) &
+		      EDP_PSR_STATUS_STATE_MASK) == 0, 50)) {
+		DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
+		return;
+	}
+
 	mutex_lock(&dev_priv->psr.lock);
 	intel_dp = dev_priv->psr.enabled;
 
-- 
1.9.3

Re: [PATCH] drm/i915: Make sure PSR is ready for been re-enabled.

[Paulo Zanoni]

2014-09-24 19:16 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> Let's make sure PSR is propperly disabled before to re-enabled it.
>
> According to Spec, after disabled PSR CTL, the Idle state might occur
> up to 24ms, that is one full frame time (1/refresh rate),
> plus SRD exit training time (max of 6ms),
> plus SRD aux channel handshake (max of 1.5ms).
>
> So if something went wrong PSR will be disabled until next full
> enable/disable setup.
>
> v2: The 24ms above takes in account 16ms for refresh rate on 60Hz mode. However
> on low frequency modes this can take longer. So let's use 50ms for safeness.
>
> v3: Move wait out of psr.lock critical area.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Again, since we already waited for 100ms while queueing
intel_edp_psr_work, an additional 50ms is basically useless. I'd
really like this function to just have an I915_READ instead of yet
another wait, so any necessary wait-time-tuning would be part of the
schedule_delayed_work(dev_priv->psr.work) call.

That said, the current patch is already an improvement, so:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

But I'd prefer the solution I proposed :)

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a119b9b..b8699b0 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2208,6 +2208,17 @@ static void intel_edp_psr_work(struct work_struct *work)
>                 container_of(work, typeof(*dev_priv), psr.work.work);
>         struct intel_dp *intel_dp = dev_priv->psr.enabled;
>
> +       /* We have to make sure PSR is ready for re-enable
> +        * otherwise it keeps disabled until next full enable/disable cycle.
> +        * PSR might take some time to get fully disabled
> +        * and be ready for re-enable.
> +        */
> +       if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev_priv->dev)) &
> +                     EDP_PSR_STATUS_STATE_MASK) == 0, 50)) {
> +               DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> +               return;
> +       }
> +
>         mutex_lock(&dev_priv->psr.lock);
>         intel_dp = dev_priv->psr.enabled;
>
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: Make sure PSR is ready for been re-enabled.

[Rodrigo Vivi]

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

On Thu, Sep 25, 2014 at 10:36 AM, Paulo Zanoni <przanoni@gmail.com> wrote:

> 2014-09-24 19:16 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > Let's make sure PSR is propperly disabled before to re-enabled it.
> >
> > According to Spec, after disabled PSR CTL, the Idle state might occur
> > up to 24ms, that is one full frame time (1/refresh rate),
> > plus SRD exit training time (max of 6ms),
> > plus SRD aux channel handshake (max of 1.5ms).
> >
> > So if something went wrong PSR will be disabled until next full
> > enable/disable setup.
> >
> > v2: The 24ms above takes in account 16ms for refresh rate on 60Hz mode.
> However
> > on low frequency modes this can take longer. So let's use 50ms for
> safeness.
> >
> > v3: Move wait out of psr.lock critical area.
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> Again, since we already waited for 100ms while queueing
> intel_edp_psr_work, an additional 50ms is basically useless.


Agree. But it doesn't hurt it is just a timeout to give more time in case
psr is still on transition.
what is unlike I know...



> I'd
> really like this function to just have an I915_READ instead of yet
> another wait, so any necessary wait-time-tuning would be part of the
> schedule_delayed_work(dev_priv->psr.work) call.
>

Uhm. that was my first idea actually. I was willing to kill the delayed
work at all and use just this read scheme.
However this didn't worked.... It seems hardware doesn't like when we have
to much sequential on-off psr switches.

So 100ms is enough to avoid this high frequency on-off and avoid sloweness
and other issues.

The 50ms extra is just to be on the safe side checking if we can reaable it
or give a bit more time for it instead of just wait until next full
enable/disable sequence.


>
> That said, the current patch is already an improvement, so:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>

Thank you very much.


>
> But I'd prefer the solution I proposed :)
>

me too. :)


>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> > index a119b9b..b8699b0 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2208,6 +2208,17 @@ static void intel_edp_psr_work(struct work_struct
> *work)
> >                 container_of(work, typeof(*dev_priv), psr.work.work);
> >         struct intel_dp *intel_dp = dev_priv->psr.enabled;
> >
> > +       /* We have to make sure PSR is ready for re-enable
> > +        * otherwise it keeps disabled until next full enable/disable
> cycle.
> > +        * PSR might take some time to get fully disabled
> > +        * and be ready for re-enable.
> > +        */
> > +       if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev_priv->dev)) &
> > +                     EDP_PSR_STATUS_STATE_MASK) == 0, 50)) {
> > +               DRM_ERROR("Timed out waiting for PSR Idle for
> re-enable\n");
> > +               return;
> > +       }
> > +
> >         mutex_lock(&dev_priv->psr.lock);
> >         intel_dp = dev_priv->psr.enabled;
> >
> > --
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 6004 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Make sure PSR is ready for been re-enabled.

[Daniel Vetter]

On Thu, Sep 25, 2014 at 10:50:51AM -0700, Rodrigo Vivi wrote:
> On Thu, Sep 25, 2014 at 10:36 AM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > 2014-09-24 19:16 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > > Let's make sure PSR is propperly disabled before to re-enabled it.
> > >
> > > According to Spec, after disabled PSR CTL, the Idle state might occur
> > > up to 24ms, that is one full frame time (1/refresh rate),
> > > plus SRD exit training time (max of 6ms),
> > > plus SRD aux channel handshake (max of 1.5ms).
> > >
> > > So if something went wrong PSR will be disabled until next full
> > > enable/disable setup.
> > >
> > > v2: The 24ms above takes in account 16ms for refresh rate on 60Hz mode.
> > However
> > > on low frequency modes this can take longer. So let's use 50ms for
> > safeness.
> > >
> > > v3: Move wait out of psr.lock critical area.
> > >
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >
> > Again, since we already waited for 100ms while queueing
> > intel_edp_psr_work, an additional 50ms is basically useless.
> 
> 
> Agree. But it doesn't hurt it is just a timeout to give more time in case
> psr is still on transition.
> what is unlike I know...

Yeah, looks good enough for now imo, patch merged.

> > I'd
> > really like this function to just have an I915_READ instead of yet
> > another wait, so any necessary wait-time-tuning would be part of the
> > schedule_delayed_work(dev_priv->psr.work) call.
> >
> 
> Uhm. that was my first idea actually. I was willing to kill the delayed
> work at all and use just this read scheme.
> However this didn't worked.... It seems hardware doesn't like when we have
> to much sequential on-off psr switches.
> 
> So 100ms is enough to avoid this high frequency on-off and avoid sloweness
> and other issues.
> 
> The 50ms extra is just to be on the safe side checking if we can reaable it
> or give a bit more time for it instead of just wait until next full
> enable/disable sequence.

Is there a way to have a less massive psr entry/exit knob? Maybe one that
just does a one-shot upload?

If that doesn't work then I think the timeout is totally ok - if we need
to upload a few frames anyway to keep hw happy then a short delay won't
make things worse really. Hopfully single-shot upload for pageflips still
work.

Cheers, Daniel


> 
> 
> >
> > That said, the current patch is already an improvement, so:
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> 
> Thank you very much.
> 
> 
> >
> > But I'd prefer the solution I proposed :)
> >
> 
> me too. :)
> 
> 
> >
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > > index a119b9b..b8699b0 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -2208,6 +2208,17 @@ static void intel_edp_psr_work(struct work_struct
> > *work)
> > >                 container_of(work, typeof(*dev_priv), psr.work.work);
> > >         struct intel_dp *intel_dp = dev_priv->psr.enabled;
> > >
> > > +       /* We have to make sure PSR is ready for re-enable
> > > +        * otherwise it keeps disabled until next full enable/disable
> > cycle.
> > > +        * PSR might take some time to get fully disabled
> > > +        * and be ready for re-enable.
> > > +        */
> > > +       if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev_priv->dev)) &
> > > +                     EDP_PSR_STATUS_STATE_MASK) == 0, 50)) {
> > > +               DRM_ERROR("Timed out waiting for PSR Idle for
> > re-enable\n");
> > > +               return;
> > > +       }
> > > +
> > >         mutex_lock(&dev_priv->psr.lock);
> > >         intel_dp = dev_priv->psr.enabled;
> > >
> > > --
> > > 1.9.3
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> >
> > --
> > Paulo Zanoni
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch