[PATCH 00/11] HSW/BDW PSR.

[PATCH 00/11] HSW/BDW PSR.

[Rodrigo Vivi]

MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Hi All,

This series introduces fixes for PSR on HSW and on BDW and "new features"
for PSR on BDW.

The biggest thing on this serie is the introduction of the psr_exit
infrastructure that was actually created for PSR on Baytrail. However
since on Baytrail the HW cannot track absolutelly no screen update lets
put it first to work on HSW and BDW. This brings more reliability to PSR and
make it possible to use even on non GL userspace environments like KDE.

Without this psr_exit infrastructure we will never be able to enable psr by
default because this breaks the userspace for all KDE users.
I understand the possible limitations of this infrastructure, but this is the
best we can do for now.

Other possibilities to solve this issue is let the full control to userspace
over ioctl. I'm thinking about a next rework where userspace could dinamically
switch over some possible PSR levels like:
1 - Full HW control - that works good enough on Gnome HSW and extract the best
                      residency time.
2 - PSR-exit by inactivating - That would be for KDE users
3 - Full SW control - where userspace would control PSR exit/entry flow over\
                      ioctls.

On BDW "new features" there are basically the intorducion of a single frame
update support what in theory improve residency time and also remove
limitations that only affect HSW like DDI only on PORT_A and PSR off when
sprites are in use.

Please help me to get this merged with good suggestions of improvements.
Please do not say just: "that is not good". I know that already. Please provide
good ideas along with the comments.

Thanks in advance,
Rodrigo.

Rodrigo Vivi (11):
  drm/i915: move psr_setup_done to psr struct
  drm/i915: Update PSR on resume.
  drm/i915: Use HAS_PSR to avoid unecessary interactions.
  drm/i915: Don't let update_psr function actually enable PSR.
  drm/i915: Do not try to enable PSR when Panel doesn't suport it.
  drm/i915: Force PSR exit by inactivating it.
  drm/i915: BDW PSR: Add single frame update support.
  drm/i915: BDW PSR: Remove limitations that aren't valid for BDW.
  drm/i915: BDW PSR: Remove DDIA limitation for Broadwell.
  drm/i915: Improve PSR debugfs status.
  drm/i915: PSR HSW: update after enabling sprite.

 drivers/gpu/drm/i915/i915_debugfs.c  |   4 +-
 drivers/gpu/drm/i915/i915_drv.h      |   4 +
 drivers/gpu/drm/i915/i915_gem.c      |   6 ++
 drivers/gpu/drm/i915/i915_reg.h      |   1 +
 drivers/gpu/drm/i915/i915_suspend.c  |   3 +
 drivers/gpu/drm/i915/intel_display.c |  20 ++++-
 drivers/gpu/drm/i915/intel_dp.c      | 153 ++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h     |   4 +-
 drivers/gpu/drm/i915/intel_sprite.c  |   2 +
 9 files changed, 165 insertions(+), 32 deletions(-)

-- 
1.9.0

[PATCH 01/11] drm/i915: move psr_setup_done to psr struct

[Rodrigo Vivi]

v2: Avoid more than one setup. Removing initialization
    and trusting allocation. (By Paulo Zanoni).
v3: rebase.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.h  | 1 +
 drivers/gpu/drm/i915/intel_dp.c  | 6 ++----
 drivers/gpu/drm/i915/intel_drv.h | 1 -
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1a9b4b3..d6fba46 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -640,6 +640,7 @@ struct i915_drrs {
 struct i915_psr {
 	bool sink_support;
 	bool source_ok;
+	bool setup_done;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 66891c9..34e8f7a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1657,7 +1657,7 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct edp_vsc_psr psr_vsc;
 
-	if (intel_dp->psr_setup_done)
+	if (dev_priv->psr.setup_done)
 		return;
 
 	/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
@@ -1672,7 +1672,7 @@ static void intel_edp_psr_setup(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);
 
-	intel_dp->psr_setup_done = true;
+	dev_priv->psr.setup_done = true;
 }
 
 static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
@@ -4199,8 +4199,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 
 	intel_dp_aux_init(intel_dp, intel_connector);
 
-	intel_dp->psr_setup_done = false;
-
 	if (!intel_edp_init_connector(intel_dp, intel_connector, &power_seq)) {
 		drm_dp_aux_unregister_i2c_bus(&intel_dp->aux);
 		if (is_edp(intel_dp)) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index acfc5c8..e72d45a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -527,7 +527,6 @@ struct intel_dp {
 	unsigned long last_power_cycle;
 	unsigned long last_power_on;
 	unsigned long last_backlight_off;
-	bool psr_setup_done;
 	bool use_tps3;
 	struct intel_connector *attached_connector;
 
-- 
1.9.0

Re: [PATCH 01/11] drm/i915: move psr_setup_done to psr struct

[Paulo Zanoni]

2014-05-15 21:13 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> v2: Avoid more than one setup. Removing initialization
>     and trusting allocation. (By Paulo Zanoni).
> v3: rebase.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

I guess the commit message needs a little explanation, such as:

"Because our driver assumes only one panel is PSR capable, and we
already have other PSR information on dev_priv instead of intel_dp. If
we ever support multiple PSR panels, we'll have to move struct
i915_psr to intel_dp anyway."

I've seen this patch so many times on this list that I really think we
could merge it before the others :)

With the message above amended, or some other message: Reviewed-by:
Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h  | 1 +
>  drivers/gpu/drm/i915/intel_dp.c  | 6 ++----
>  drivers/gpu/drm/i915/intel_drv.h | 1 -
>  3 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1a9b4b3..d6fba46 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -640,6 +640,7 @@ struct i915_drrs {
>  struct i915_psr {
>         bool sink_support;
>         bool source_ok;
> +       bool setup_done;
>  };
>
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 66891c9..34e8f7a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1657,7 +1657,7 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct edp_vsc_psr psr_vsc;
>
> -       if (intel_dp->psr_setup_done)
> +       if (dev_priv->psr.setup_done)
>                 return;
>
>         /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
> @@ -1672,7 +1672,7 @@ static void intel_edp_psr_setup(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);
>
> -       intel_dp->psr_setup_done = true;
> +       dev_priv->psr.setup_done = true;
>  }
>
>  static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
> @@ -4199,8 +4199,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>
>         intel_dp_aux_init(intel_dp, intel_connector);
>
> -       intel_dp->psr_setup_done = false;
> -
>         if (!intel_edp_init_connector(intel_dp, intel_connector, &power_seq)) {
>                 drm_dp_aux_unregister_i2c_bus(&intel_dp->aux);
>                 if (is_edp(intel_dp)) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index acfc5c8..e72d45a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -527,7 +527,6 @@ struct intel_dp {
>         unsigned long last_power_cycle;
>         unsigned long last_power_on;
>         unsigned long last_backlight_off;
> -       bool psr_setup_done;
>         bool use_tps3;
>         struct intel_connector *attached_connector;
>
> --
> 1.9.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH] drm/i915: move psr_setup_done to psr struct

[Rodrigo Vivi]

"Because our driver assumes only one panel is PSR capable, and we
already have other PSR information on dev_priv instead of intel_dp. If
we ever support multiple PSR panels, we'll have to move struct
i915_psr to intel_dp anyway." (by Paulo)

v2: Avoid more than one setup. Removing initialization
    and trusting allocation. (By Paulo Zanoni).
v3: rebase.
v4: Adding comment.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.h  | 1 +
 drivers/gpu/drm/i915/intel_dp.c  | 6 ++----
 drivers/gpu/drm/i915/intel_drv.h | 1 -
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1a9b4b3..d6fba46 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -640,6 +640,7 @@ struct i915_drrs {
 struct i915_psr {
 	bool sink_support;
 	bool source_ok;
+	bool setup_done;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 66891c9..34e8f7a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1657,7 +1657,7 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct edp_vsc_psr psr_vsc;
 
-	if (intel_dp->psr_setup_done)
+	if (dev_priv->psr.setup_done)
 		return;
 
 	/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
@@ -1672,7 +1672,7 @@ static void intel_edp_psr_setup(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);
 
-	intel_dp->psr_setup_done = true;
+	dev_priv->psr.setup_done = true;
 }
 
 static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
@@ -4199,8 +4199,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 
 	intel_dp_aux_init(intel_dp, intel_connector);
 
-	intel_dp->psr_setup_done = false;
-
 	if (!intel_edp_init_connector(intel_dp, intel_connector, &power_seq)) {
 		drm_dp_aux_unregister_i2c_bus(&intel_dp->aux);
 		if (is_edp(intel_dp)) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index acfc5c8..e72d45a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -527,7 +527,6 @@ struct intel_dp {
 	unsigned long last_power_cycle;
 	unsigned long last_power_on;
 	unsigned long last_backlight_off;
-	bool psr_setup_done;
 	bool use_tps3;
 	struct intel_connector *attached_connector;
 
-- 
1.9.0

Re: [PATCH] drm/i915: move psr_setup_done to psr struct

[Daniel Vetter]

On Fri, May 23, 2014 at 01:45:51PM -0700, Rodrigo Vivi wrote:
> "Because our driver assumes only one panel is PSR capable, and we
> already have other PSR information on dev_priv instead of intel_dp. If
> we ever support multiple PSR panels, we'll have to move struct
> i915_psr to intel_dp anyway." (by Paulo)
> 
> v2: Avoid more than one setup. Removing initialization
>     and trusting allocation. (By Paulo Zanoni).
> v3: rebase.
> v4: Adding comment.
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

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

[PATCH 02/11] drm/i915: Update PSR on resume.

[Rodrigo Vivi]

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

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 56785e8..a38dee3 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -288,6 +288,9 @@ static void i915_restore_display(struct drm_device *dev)
 		I915_WRITE(PP_CONTROL, dev_priv->regfile.savePP_CONTROL);
 	}
 
+	dev_priv->psr.setup_done = false;
+	intel_edp_psr_update(dev);
+
 	/* only restore FBC info on the platform that supports FBC*/
 	intel_disable_fbc(dev);
 
-- 
1.9.0

Re: [PATCH 02/11] drm/i915: Update PSR on resume.

[Paulo Zanoni]

2014-05-15 21:13 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

Can you please write a little bit on the commit message explaining
what is the problem with the current code? What does this patch fix?
Is this a bug fix? What changes now?

Thanks,
Paulo

> ---
>  drivers/gpu/drm/i915/i915_suspend.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 56785e8..a38dee3 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -288,6 +288,9 @@ static void i915_restore_display(struct drm_device *dev)
>                 I915_WRITE(PP_CONTROL, dev_priv->regfile.savePP_CONTROL);
>         }
>
> +       dev_priv->psr.setup_done = false;
> +       intel_edp_psr_update(dev);
> +
>         /* only restore FBC info on the platform that supports FBC*/
>         intel_disable_fbc(dev);
>
> --
> 1.9.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH] drm/i915: Update PSR on resume.

[Rodrigo Vivi]

Some registers set during setup might not be persistent after suspend/resume.
This was causing bugs for some people that was unable to get PSR entry state
after resume cycle.

v2: Adding some comments and better commit message explaining why this is needed.

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

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 56785e8..1923708 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -288,6 +288,12 @@ static void i915_restore_display(struct drm_device *dev)
 		I915_WRITE(PP_CONTROL, dev_priv->regfile.savePP_CONTROL);
 	}
 
+	/* Forcing a full init sequence after resume to make sure all
+	* registers are properly set. Some might not be persistent after
+	* suspend/resume cycle. */
+	dev_priv->psr.setup_done = false;
+	intel_edp_psr_update(dev);
+
 	/* only restore FBC info on the platform that supports FBC*/
 	intel_disable_fbc(dev);
 
-- 
1.9.0

Re: [PATCH] drm/i915: Update PSR on resume.

[Daniel Vetter]

On Tue, May 27, 2014 at 04:50:14PM -0700, Rodrigo Vivi wrote:
> Some registers set during setup might not be persistent after suspend/resume.
> This was causing bugs for some people that was unable to get PSR entry state
> after resume cycle.
> 
> v2: Adding some comments and better commit message explaining why this is needed.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_suspend.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 56785e8..1923708 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -288,6 +288,12 @@ static void i915_restore_display(struct drm_device *dev)
>  		I915_WRITE(PP_CONTROL, dev_priv->regfile.savePP_CONTROL);
>  	}
>  
> +	/* Forcing a full init sequence after resume to make sure all
> +	* registers are properly set. Some might not be persistent after
> +	* suspend/resume cycle. */
> +	dev_priv->psr.setup_done = false;
> +	intel_edp_psr_update(dev);

No, crtc_enable should take care of this. There's more places (like after
runtime pm) where the hw has potentially lost all register contents, so
crtc_enabl is the right place for this.
-Daniel

> +
>  	/* only restore FBC info on the platform that supports FBC*/
>  	intel_disable_fbc(dev);
>  
> -- 
> 1.9.0
> 
> _______________________________________________
> 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] drm/i915: Update PSR on resume.

[Rodrigo Vivi]

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

On Wed, May 28, 2014 at 5:57 AM, Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, May 27, 2014 at 04:50:14PM -0700, Rodrigo Vivi wrote:
> > Some registers set during setup might not be persistent after
> suspend/resume.
> > This was causing bugs for some people that was unable to get PSR entry
> state
> > after resume cycle.
> >
> > v2: Adding some comments and better commit message explaining why this
> is needed.
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/i915_suspend.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c
> b/drivers/gpu/drm/i915/i915_suspend.c
> > index 56785e8..1923708 100644
> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > @@ -288,6 +288,12 @@ static void i915_restore_display(struct drm_device
> *dev)
> >               I915_WRITE(PP_CONTROL, dev_priv->regfile.savePP_CONTROL);
> >       }
> >
> > +     /* Forcing a full init sequence after resume to make sure all
> > +     * registers are properly set. Some might not be persistent after
> > +     * suspend/resume cycle. */
> > +     dev_priv->psr.setup_done = false;
> > +     intel_edp_psr_update(dev);
>
> No, crtc_enable should take care of this. There's more places (like after
> runtime pm) where the hw has potentially lost all register contents, so
> crtc_enabl is the right place for this.
> -Daniel
>

crtc_enable takes care of the update, but not the setup part that is done
only once...
I do believe that only the setup_done = false is really needed here, but
doesn't heart to trigger the update right here
and fixes the bug...


>
> > +
> >       /* only restore FBC info on the platform that supports FBC*/
> >       intel_disable_fbc(dev);
> >
> > --
> > 1.9.0
> >
> > _______________________________________________
> > 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: 3518 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: Update PSR on resume.

[Daniel Vetter]

On Wed, Jun 04, 2014 at 12:17:14PM -0700, Rodrigo Vivi wrote:
> On Wed, May 28, 2014 at 5:57 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Tue, May 27, 2014 at 04:50:14PM -0700, Rodrigo Vivi wrote:
> > > Some registers set during setup might not be persistent after
> > suspend/resume.
> > > This was causing bugs for some people that was unable to get PSR entry
> > state
> > > after resume cycle.
> > >
> > > v2: Adding some comments and better commit message explaining why this
> > is needed.
> > >
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_suspend.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_suspend.c
> > b/drivers/gpu/drm/i915/i915_suspend.c
> > > index 56785e8..1923708 100644
> > > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > > @@ -288,6 +288,12 @@ static void i915_restore_display(struct drm_device
> > *dev)
> > >               I915_WRITE(PP_CONTROL, dev_priv->regfile.savePP_CONTROL);
> > >       }
> > >
> > > +     /* Forcing a full init sequence after resume to make sure all
> > > +     * registers are properly set. Some might not be persistent after
> > > +     * suspend/resume cycle. */
> > > +     dev_priv->psr.setup_done = false;
> > > +     intel_edp_psr_update(dev);
> >
> > No, crtc_enable should take care of this. There's more places (like after
> > runtime pm) where the hw has potentially lost all register contents, so
> > crtc_enabl is the right place for this.
> > -Daniel
> >
> 
> crtc_enable takes care of the update, but not the setup part that is done
> only once...
> I do believe that only the setup_done = false is really needed here, but
> doesn't heart to trigger the update right here
> and fixes the bug...

restore_display hurts. If there's some setup we need to do, we _really_
need to do it in crtc_enable. Splattering setup code all over the code is
one of the prime sources of bugs we have wrt rpm, s/r and driver load.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Update PSR on resume.

[Rodrigo Vivi]

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

would you be ok by a patch that doesn't trigger the psr_update but just set
psr.setup_done = false on resume?

I don't want to do more setup than really needed.

Or you mean move even psr.setup_done to crtc_enable?


On Thu, Jun 5, 2014 at 2:15 AM, Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Jun 04, 2014 at 12:17:14PM -0700, Rodrigo Vivi wrote:
> > On Wed, May 28, 2014 at 5:57 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > > On Tue, May 27, 2014 at 04:50:14PM -0700, Rodrigo Vivi wrote:
> > > > Some registers set during setup might not be persistent after
> > > suspend/resume.
> > > > This was causing bugs for some people that was unable to get PSR
> entry
> > > state
> > > > after resume cycle.
> > > >
> > > > v2: Adding some comments and better commit message explaining why
> this
> > > is needed.
> > > >
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_suspend.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_suspend.c
> > > b/drivers/gpu/drm/i915/i915_suspend.c
> > > > index 56785e8..1923708 100644
> > > > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > > > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > > > @@ -288,6 +288,12 @@ static void i915_restore_display(struct
> drm_device
> > > *dev)
> > > >               I915_WRITE(PP_CONTROL,
> dev_priv->regfile.savePP_CONTROL);
> > > >       }
> > > >
> > > > +     /* Forcing a full init sequence after resume to make sure all
> > > > +     * registers are properly set. Some might not be persistent
> after
> > > > +     * suspend/resume cycle. */
> > > > +     dev_priv->psr.setup_done = false;
> > > > +     intel_edp_psr_update(dev);
> > >
> > > No, crtc_enable should take care of this. There's more places (like
> after
> > > runtime pm) where the hw has potentially lost all register contents, so
> > > crtc_enabl is the right place for this.
> > > -Daniel
> > >
> >
> > crtc_enable takes care of the update, but not the setup part that is done
> > only once...
> > I do believe that only the setup_done = false is really needed here, but
> > doesn't heart to trigger the update right here
> > and fixes the bug...
>
> restore_display hurts. If there's some setup we need to do, we _really_
> need to do it in crtc_enable. Splattering setup code all over the code is
> one of the prime sources of bugs we have wrt rpm, s/r and driver load.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>



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

[-- Attachment #1.2: Type: text/html, Size: 3957 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: Update PSR on resume.

[Daniel Vetter]

On Tue, Jun 10, 2014 at 08:11:41AM -0700, Rodrigo Vivi wrote:
> would you be ok by a patch that doesn't trigger the psr_update but just set
> psr.setup_done = false on resume?
> 
> I don't want to do more setup than really needed.
> 
> Or you mean move even psr.setup_done to crtc_enable?

Yeah, if there's some setup we need to do (e.g. figure out whether we can
do psr or set up the hw) we should do it in crtc_enable.

That's the only common function for modeset stuff shared between system
resume and runtime resume, and we'll loose register values in both cases.
-Daniel

> 
> 
> On Thu, Jun 5, 2014 at 2:15 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Wed, Jun 04, 2014 at 12:17:14PM -0700, Rodrigo Vivi wrote:
> > > On Wed, May 28, 2014 at 5:57 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > > On Tue, May 27, 2014 at 04:50:14PM -0700, Rodrigo Vivi wrote:
> > > > > Some registers set during setup might not be persistent after
> > > > suspend/resume.
> > > > > This was causing bugs for some people that was unable to get PSR
> > entry
> > > > state
> > > > > after resume cycle.
> > > > >
> > > > > v2: Adding some comments and better commit message explaining why
> > this
> > > > is needed.
> > > > >
> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_suspend.c | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_suspend.c
> > > > b/drivers/gpu/drm/i915/i915_suspend.c
> > > > > index 56785e8..1923708 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > > > > @@ -288,6 +288,12 @@ static void i915_restore_display(struct
> > drm_device
> > > > *dev)
> > > > >               I915_WRITE(PP_CONTROL,
> > dev_priv->regfile.savePP_CONTROL);
> > > > >       }
> > > > >
> > > > > +     /* Forcing a full init sequence after resume to make sure all
> > > > > +     * registers are properly set. Some might not be persistent
> > after
> > > > > +     * suspend/resume cycle. */
> > > > > +     dev_priv->psr.setup_done = false;
> > > > > +     intel_edp_psr_update(dev);
> > > >
> > > > No, crtc_enable should take care of this. There's more places (like
> > after
> > > > runtime pm) where the hw has potentially lost all register contents, so
> > > > crtc_enabl is the right place for this.
> > > > -Daniel
> > > >
> > >
> > > crtc_enable takes care of the update, but not the setup part that is done
> > > only once...
> > > I do believe that only the setup_done = false is really needed here, but
> > > doesn't heart to trigger the update right here
> > > and fixes the bug...
> >
> > restore_display hurts. If there's some setup we need to do, we _really_
> > need to do it in crtc_enable. Splattering setup code all over the code is
> > one of the prime sources of bugs we have wrt rpm, s/r and driver load.
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

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

[PATCH] drm/i915: Force full PSR setup during crtc enable.

[Rodrigo Vivi]

Some registers set during setup might not be persistent after suspend/resume,
and also on crtc off/on cycles.

This was causing bugs for some people that was unable to get PSR entry state
after suspend/resume cycle.

v2: Adding some comments and better commit message explaining why this is
needed. (by Paulo)
v3: Moving psr.setup_done reset to crtc_enable because same issues might apply
also on crtc off/on cycle. (by Daniel)

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5cbb28..077ab0e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3938,6 +3938,10 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	/* Forcing a full init sequence here to make sure all
+	* registers are properly set. Some might not be persistent after
+	* crtc on/off cycle. */
+	dev_priv->psr.setup_done = false;
 	intel_edp_psr_update(dev);
 	mutex_unlock(&dev->struct_mutex);
 }
-- 
1.9.3

Re: [PATCH] drm/i915: Force full PSR setup during crtc enable.

[Daniel Vetter]

On Tue, Jun 10, 2014 at 03:49:57AM -0700, Rodrigo Vivi wrote:
> Some registers set during setup might not be persistent after suspend/resume,
> and also on crtc off/on cycles.
> 
> This was causing bugs for some people that was unable to get PSR entry state
> after suspend/resume cycle.
> 
> v2: Adding some comments and better commit message explaining why this is
> needed. (by Paulo)
> v3: Moving psr.setup_done reset to crtc_enable because same issues might apply
> also on crtc off/on cycle. (by Daniel)
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Joe Konno <joe.konno@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5cbb28..077ab0e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3938,6 +3938,10 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
>  
>  	mutex_lock(&dev->struct_mutex);
>  	intel_update_fbc(dev);
> +	/* Forcing a full init sequence here to make sure all
> +	* registers are properly set. Some might not be persistent after
> +	* crtc on/off cycle. */
> +	dev_priv->psr.setup_done = false;
>  	intel_edp_psr_update(dev);

I think splitting edp_psr_update into a setup and an update function would
be cleaner. We kinda should have the same for fbc I think. This should
also help with the locking since at least for fbc the setup and the update
don't really need to do the same tasks.

For now I think you can just add a bool do_setup to edp_psr_update and
drop the dev_priv->psr.setup_done boolean instead.
-Daniel

>  	mutex_unlock(&dev->struct_mutex);
>  }
> -- 
> 1.9.3
> 

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

[PATCH] drm/i915: Update PSR on resume.

[Rodrigo Vivi]

From: Rodrigo Vivi <rodrigo.vivi@gmail.com>

Some registers set during setup might not be persistent after suspend/resume.
This was causing bugs for some people that was unable to get PSR entry state
after resume cycle.

v2: Adding some comments and better commit message explaining why this is needed.
v3: Getting back old setup_done variable and move from resume to crtc_enable
    as Daniel requested.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c |  5 +++++
 drivers/gpu/drm/i915/intel_dp.c      | 11 ++++++++---
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 125a83c..9ef8dfc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -674,6 +674,7 @@ struct i915_psr {
 	struct mutex lock;
 	bool sink_support;
 	bool source_ok;
+	bool setup_done;
 	struct intel_dp *enabled;
 	bool active;
 	struct delayed_work work;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 89e0ac5..5f8e4f6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3972,6 +3972,11 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE);
 	POSTING_READ(DSPCNTR(plane));
 
+	/* Forcing a full psr init sequence when enabling crtc to make sure all
+	* registers are properly set. Some might not be persistent after
+	* suspend/resume cycle. */
+	dev_priv->psr.setup_done = false;
+
 	dev_priv->display.update_primary_plane(crtc, crtc->primary->fb,
 					       crtc->x, crtc->y);
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 34e3c47..5fde763 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1709,6 +1709,9 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct edp_vsc_psr psr_vsc;
 
+	if (dev_priv->psr.setup_done)
+		return;
+
 	/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
 	memset(&psr_vsc, 0, sizeof(psr_vsc));
 	psr_vsc.sdp_header.HB0 = 0;
@@ -1720,6 +1723,8 @@ static void intel_edp_psr_setup(struct intel_dp *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);
+
+	dev_priv->psr.setup_done = true;
 }
 
 static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
@@ -1839,6 +1844,9 @@ 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);
 
+	/* Setup PSR once */
+	intel_edp_psr_setup(intel_dp);
+
 	/* Enable PSR on the panel */
 	intel_edp_psr_enable_sink(intel_dp);
 
@@ -1872,9 +1880,6 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 
-	/* Setup PSR once */
-	intel_edp_psr_setup(intel_dp);
-
 	if (intel_edp_psr_match_conditions(intel_dp))
 		dev_priv->psr.enabled = intel_dp;
 	mutex_unlock(&dev_priv->psr.lock);
-- 
1.9.3

Re: [PATCH] drm/i915: Update PSR on resume.

[Daniel Vetter]

On Fri, Aug 8, 2014 at 7:19 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> From: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>
> Some registers set during setup might not be persistent after suspend/resume.
> This was causing bugs for some people that was unable to get PSR entry state
> after resume cycle.
>
> v2: Adding some comments and better commit message explaining why this is needed.
> v3: Getting back old setup_done variable and move from resume to crtc_enable
>     as Daniel requested.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

I'm confused ... whats the use of this? Afaict that's exactly what the
code currently does.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Update PSR on resume.

[Rodrigo Vivi]

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

Well, this fix the issue Linus faced.

Actually the issue I was aware of and trying to fix with this patch for a
long time was reported by chromeos guys saying the psr wasn't propperly
working after suspend/resume. They got the screen back but never got psr
back again.

The original patch fix suspend/resume issues with psr and I decided to keep
the same and subject for reference of what the problem this fixes and what
was the patch history, but changed the place for the full setup to
crtc_enable per your recommendation.

Thanks,
Rodrigo.



On Sat, Aug 9, 2014 at 12:40 AM, Daniel Vetter <daniel.vetter@ffwll.ch>
wrote:

> On Fri, Aug 8, 2014 at 7:19 PM, Rodrigo Vivi <rodrigo.vivi@intel.com>
> wrote:
> > From: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> >
> > Some registers set during setup might not be persistent after
> suspend/resume.
> > This was causing bugs for some people that was unable to get PSR entry
> state
> > after resume cycle.
> >
> > v2: Adding some comments and better commit message explaining why this
> is needed.
> > v3: Getting back old setup_done variable and move from resume to
> crtc_enable
> >     as Daniel requested.
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> I'm confused ... whats the use of this? Afaict that's exactly what the
> code currently does.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>



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

[-- Attachment #1.2: Type: text/html, Size: 2555 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: Update PSR on resume.

[Daniel Vetter]

On Mon, Aug 11, 2014 at 09:57:12AM -0700, Rodrigo Vivi wrote:
> Well, this fix the issue Linus faced.
> 
> Actually the issue I was aware of and trying to fix with this patch for a
> long time was reported by chromeos guys saying the psr wasn't propperly
> working after suspend/resume. They got the screen back but never got psr
> back again.
> 
> The original patch fix suspend/resume issues with psr and I decided to keep
> the same and subject for reference of what the problem this fixes and what
> was the patch history, but changed the place for the full setup to
> crtc_enable per your recommendation.

So if I read your patch correctly then we no do the initial psr setup in
crtc_enable, but only after driver load/resume. Before we do it always
when crtc_enable is called. That doesn't make sense. Also note that Linus
said it's also sometimes broken when using X normally.

Otoh if there's really something we need to in crtc_enable in addition to
what we do already, then we _must_ do it always. Otherwise runtime pm
(which is pretty much the same as system resume) will not work.
-Daniel

> 
> Thanks,
> Rodrigo.
> 
> 
> 
> On Sat, Aug 9, 2014 at 12:40 AM, Daniel Vetter <daniel.vetter@ffwll.ch>
> wrote:
> 
> > On Fri, Aug 8, 2014 at 7:19 PM, Rodrigo Vivi <rodrigo.vivi@intel.com>
> > wrote:
> > > From: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > >
> > > Some registers set during setup might not be persistent after
> > suspend/resume.
> > > This was causing bugs for some people that was unable to get PSR entry
> > state
> > > after resume cycle.
> > >
> > > v2: Adding some comments and better commit message explaining why this
> > is needed.
> > > v3: Getting back old setup_done variable and move from resume to
> > crtc_enable
> > >     as Daniel requested.
> > >
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >
> > I'm confused ... whats the use of this? Afaict that's exactly what the
> > code currently does.
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

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

[PATCH 03/11] drm/i915: Use HAS_PSR to avoid unecessary interactions.

[Rodrigo Vivi]

Let's be more conservative and protect platforms that don't
support PSR from unecessary interactions.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 34e8f7a..58537b7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1739,11 +1739,6 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 
 	dev_priv->psr.source_ok = false;
 
-	if (!HAS_PSR(dev)) {
-		DRM_DEBUG_KMS("PSR not supported on this platform\n");
-		return false;
-	}
-
 	if ((intel_encoder->type != INTEL_OUTPUT_EDP) ||
 	    (dig_port->port != PORT_A)) {
 		DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
@@ -1816,6 +1811,11 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 
+	if (!HAS_PSR(dev)) {
+		DRM_DEBUG_KMS("PSR not supported on this platform\n");
+		return;
+	}
+
 	if (intel_edp_psr_match_conditions(intel_dp) &&
 	    !intel_edp_is_psr_enabled(dev))
 		intel_edp_psr_do_enable(intel_dp);
@@ -1843,6 +1843,9 @@ void intel_edp_psr_update(struct drm_device *dev)
 	struct intel_encoder *encoder;
 	struct intel_dp *intel_dp = NULL;
 
+	if (!HAS_PSR(dev))
+		return;
+
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
 		if (encoder->type == INTEL_OUTPUT_EDP) {
 			intel_dp = enc_to_intel_dp(&encoder->base);
-- 
1.9.0

Re: [PATCH 03/11] drm/i915: Use HAS_PSR to avoid unecessary interactions.

[Vijay Purushothaman]

On 5/16/2014 5:43 AM, Rodrigo Vivi wrote:
> Let's be more conservative and protect platforms that don't
> support PSR from unecessary interactions.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 34e8f7a..58537b7 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1739,11 +1739,6 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
>
>   	dev_priv->psr.source_ok = false;
>
> -	if (!HAS_PSR(dev)) {
> -		DRM_DEBUG_KMS("PSR not supported on this platform\n");
> -		return false;
> -	}
> -
>   	if ((intel_encoder->type != INTEL_OUTPUT_EDP) ||
>   	    (dig_port->port != PORT_A)) {
>   		DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
> @@ -1816,6 +1811,11 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
>   {
>   	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>
> +	if (!HAS_PSR(dev)) {
> +		DRM_DEBUG_KMS("PSR not supported on this platform\n");
> +		return;
> +	}
> +
>   	if (intel_edp_psr_match_conditions(intel_dp) &&
>   	    !intel_edp_is_psr_enabled(dev))
>   		intel_edp_psr_do_enable(intel_dp);
> @@ -1843,6 +1843,9 @@ void intel_edp_psr_update(struct drm_device *dev)
>   	struct intel_encoder *encoder;
>   	struct intel_dp *intel_dp = NULL;
>
> +	if (!HAS_PSR(dev))
> +		return;
> +
>   	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
>   		if (encoder->type == INTEL_OUTPUT_EDP) {
>   			intel_dp = enc_to_intel_dp(&encoder->base);
>

Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>

[PATCH 04/11] drm/i915: Don't let update_psr function actually enable PSR.

[Rodrigo Vivi]

Being more conservative by enabling PSR only on psr_enable function.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 58537b7..fe28eb7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1797,9 +1797,6 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 	    intel_edp_is_psr_enabled(dev))
 		return;
 
-	/* Setup PSR once */
-	intel_edp_psr_setup(intel_dp);
-
 	/* Enable PSR on the panel */
 	intel_edp_psr_enable_sink(intel_dp);
 
@@ -1816,6 +1813,9 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 		return;
 	}
 
+	/* Setup PSR once */
+	intel_edp_psr_setup(intel_dp);
+
 	if (intel_edp_psr_match_conditions(intel_dp) &&
 	    !intel_edp_is_psr_enabled(dev))
 		intel_edp_psr_do_enable(intel_dp);
@@ -1840,12 +1840,16 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 
 void intel_edp_psr_update(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *encoder;
 	struct intel_dp *intel_dp = NULL;
 
 	if (!HAS_PSR(dev))
 		return;
 
+	if (!dev_priv->psr.setup_done)
+		return;
+
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
 		if (encoder->type == INTEL_OUTPUT_EDP) {
 			intel_dp = enc_to_intel_dp(&encoder->base);
-- 
1.9.0

Re: [PATCH 04/11] drm/i915: Don't let update_psr function actually enable PSR.

[Vijay Purushothaman]

On 5/16/2014 5:43 AM, Rodrigo Vivi wrote:
> Being more conservative by enabling PSR only on psr_enable function.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 58537b7..fe28eb7 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1797,9 +1797,6 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
>   	    intel_edp_is_psr_enabled(dev))
>   		return;
>
> -	/* Setup PSR once */
> -	intel_edp_psr_setup(intel_dp);
> -
>   	/* Enable PSR on the panel */
>   	intel_edp_psr_enable_sink(intel_dp);
>
> @@ -1816,6 +1813,9 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
>   		return;
>   	}
>
> +	/* Setup PSR once */
> +	intel_edp_psr_setup(intel_dp);
> +
>   	if (intel_edp_psr_match_conditions(intel_dp) &&
>   	    !intel_edp_is_psr_enabled(dev))
>   		intel_edp_psr_do_enable(intel_dp);
> @@ -1840,12 +1840,16 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
>
>   void intel_edp_psr_update(struct drm_device *dev)
>   {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_encoder *encoder;
>   	struct intel_dp *intel_dp = NULL;
>
>   	if (!HAS_PSR(dev))
>   		return;
>
> +	if (!dev_priv->psr.setup_done)
> +		return;
> +
>   	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
>   		if (encoder->type == INTEL_OUTPUT_EDP) {
>   			intel_dp = enc_to_intel_dp(&encoder->base);
>

Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>

[PATCH 05/11] drm/i915: Do not try to enable PSR when Panel doesn't suport it.

[Rodrigo Vivi]

Also do not cache aux info. That info could be related to another panel.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index fe28eb7..8b2f1a4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1605,11 +1605,9 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
 	}
 }
 
-static bool is_edp_psr(struct drm_device *dev)
+static bool is_edp_psr(struct intel_dp *intel_dp)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	return dev_priv->psr.sink_support;
+	return intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED;
 }
 
 static bool intel_edp_is_psr_enabled(struct drm_device *dev)
@@ -1813,6 +1811,11 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 		return;
 	}
 
+	if (!is_edp_psr(intel_dp)) {
+		DRM_DEBUG_KMS("PSR not supported by this panel\n");
+		return;
+	}
+
 	/* Setup PSR once */
 	intel_edp_psr_setup(intel_dp);
 
@@ -1854,9 +1857,6 @@ void intel_edp_psr_update(struct drm_device *dev)
 		if (encoder->type == INTEL_OUTPUT_EDP) {
 			intel_dp = enc_to_intel_dp(&encoder->base);
 
-			if (!is_edp_psr(dev))
-				return;
-
 			if (!intel_edp_psr_match_conditions(intel_dp))
 				intel_edp_psr_disable(intel_dp);
 			else
-- 
1.9.0

Re: [PATCH 05/11] drm/i915: Do not try to enable PSR when Panel doesn't suport it.

[Chris Wilson]

On Thu, May 15, 2014 at 08:13:04PM -0400, Rodrigo Vivi wrote:
> Also do not cache aux info. That info could be related to another panel.

You should kill the bool sink_support then. There are other places that
it used that could be invalid.

I'm not sure though that we can cope with eDP panels being swapped at
runtime.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 05/11] drm/i915: Do not try to enable PSR when Panel doesn't suport it.

[Rodrigo Vivi]

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

On Fri, May 16, 2014 at 3:21 AM, Chris Wilson <chris@chris-wilson.co.uk>wrote:

> On Thu, May 15, 2014 at 08:13:04PM -0400, Rodrigo Vivi wrote:
> > Also do not cache aux info. That info could be related to another panel.
>
> You should kill the bool sink_support then. There are other places that
> it used that could be invalid.


the point here was to keep the info for debugfs in a simple way.


>
> I'm not sure though that we can cope with eDP panels being swapped at
> runtime.
>

the point wan't swap, but the possibility of having more than one eDP in
the future, but anyway other parts of the could should be changed to
support this.

so maybe just get back to the origial version keeping it on sink_support...


> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
>



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

[-- Attachment #1.2: Type: text/html, Size: 1718 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 06/11] drm/i915: Force PSR exit by inactivating it.

[Rodrigo Vivi]

The perfect solution for psr_exit is the hardware tracking the changes and
doing the psr exit by itself. This scenario works for HSW and BDW with some
environments like Gnome and Wayland.

However there are many other scenarios that this isn't true. Mainly one right
now is KDE users on HSW and BDW with PSR on. User would miss many screen
updates. For instances any key typed could be seen only when mouse cursor is
moved. So this patch introduces the ability of trigger PSR exit on kernel side
on some common cases that.

Most of the cases are coverred by psr_exit at set_domain. The remaining cases
are coverred by triggering it at set_domain, busy_ioctl, sw_finish and
mark_busy.

The downside here might be reducing the residency time on the cases this
already work very wall like Gnome environment. But so far let's get focused
on fixinge issues sio PSR couild be used for everybody and we could even
get it enabled by default. Later we can add some alternatives to choose the
level of PSR efficiency over boot flag of even over crtc property.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 ++
 drivers/gpu/drm/i915/i915_gem.c      |  6 +++
 drivers/gpu/drm/i915/intel_display.c | 20 +++++++-
 drivers/gpu/drm/i915/intel_dp.c      | 91 ++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++
 5 files changed, 111 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d6fba46..173114c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -641,6 +641,9 @@ struct i915_psr {
 	bool sink_support;
 	bool source_ok;
 	bool setup_done;
+	bool enabled;
+	bool active;
+	struct delayed_work work;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a000a8b..da66405 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1275,6 +1275,8 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 		goto unlock;
 	}
 
+	intel_edp_psr_exit(dev, true);
+
 	/* Try to flush the object off the GPU without holding the lock.
 	 * We will repeat the flush holding the lock in the normal manner
 	 * to catch cases where we are gazumped.
@@ -1320,6 +1322,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
+	intel_edp_psr_exit(dev, true);
+
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
 		ret = -ENOENT;
@@ -4018,6 +4022,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
+	intel_edp_psr_exit(dev, true);
+
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
 		ret = -ENOENT;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 767ca96..b8b288f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4801,6 +4801,8 @@ static void intel_connector_check_state(struct intel_connector *connector)
  * consider. */
 void intel_connector_dpms(struct drm_connector *connector, int mode)
 {
+	struct drm_device *dev = connector->dev;
+	struct intel_encoder *intel_encoder;
 	/* All the simple cases only support two dpms states. */
 	if (mode != DRM_MODE_DPMS_ON)
 		mode = DRM_MODE_DPMS_OFF;
@@ -4811,9 +4813,15 @@ void intel_connector_dpms(struct drm_connector *connector, int mode)
 	connector->dpms = mode;
 
 	/* Only need to change hw state when actually enabled */
-	if (connector->encoder)
-		intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
+	if (connector->encoder) {
+
+		intel_encoder = to_intel_encoder(connector->encoder);
+		intel_encoder_dpms(intel_encoder, mode);
 
+		if (intel_encoder->type == INTEL_OUTPUT_EDP &&
+		    mode == DRM_MODE_DPMS_OFF)
+			intel_edp_psr_exit(dev, false);
+	}
 	intel_modeset_check_state(connector->dev);
 }
 
@@ -8712,12 +8720,15 @@ out:
 	intel_runtime_pm_put(dev_priv);
 }
 
+
 void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 			struct intel_ring_buffer *ring)
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_crtc *crtc;
 
+	intel_edp_psr_exit(dev, true);
+
 	if (!i915.powersave)
 		return;
 
@@ -9196,6 +9207,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (work == NULL)
 		return -ENOMEM;
 
+	/* Exit PSR early in page flip */
+	intel_edp_psr_exit(dev, true);
+
 	work->event = event;
 	work->crtc = crtc;
 	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
@@ -11072,6 +11086,8 @@ static void intel_setup_outputs(struct drm_device *dev)
 	if (SUPPORTS_TV(dev))
 		intel_tv_init(dev);
 
+	intel_edp_psr_init(dev);
+
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
 		encoder->base.possible_crtcs = encoder->crtc_mask;
 		encoder->base.possible_clones =
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8b2f1a4..59baee5 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1789,10 +1789,11 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 
 static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 {
-	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!intel_edp_psr_match_conditions(intel_dp) ||
-	    intel_edp_is_psr_enabled(dev))
+	if (intel_edp_is_psr_enabled(dev))
 		return;
 
 	/* Enable PSR on the panel */
@@ -1800,6 +1801,9 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 
 	/* Enable PSR on the host */
 	intel_edp_psr_enable_source(intel_dp);
+
+	dev_priv->psr.enabled = true;
+	dev_priv->psr.active = true;
 }
 
 void intel_edp_psr_enable(struct intel_dp *intel_dp)
@@ -1819,8 +1823,7 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 	/* Setup PSR once */
 	intel_edp_psr_setup(intel_dp);
 
-	if (intel_edp_psr_match_conditions(intel_dp) &&
-	    !intel_edp_is_psr_enabled(dev))
+	if (intel_edp_psr_match_conditions(intel_dp))
 		intel_edp_psr_do_enable(intel_dp);
 }
 
@@ -1829,7 +1832,7 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!intel_edp_is_psr_enabled(dev))
+	if (!dev_priv->psr.enabled)
 		return;
 
 	I915_WRITE(EDP_PSR_CTL(dev),
@@ -1839,13 +1842,13 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 	if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &
 		       EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
 		DRM_ERROR("Timed out waiting for PSR Idle State\n");
+
+	dev_priv->psr.enabled = false;
 }
 
 void intel_edp_psr_update(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_encoder *encoder;
-	struct intel_dp *intel_dp = NULL;
 
 	if (!HAS_PSR(dev))
 		return;
@@ -1853,6 +1856,17 @@ void intel_edp_psr_update(struct drm_device *dev)
 	if (!dev_priv->psr.setup_done)
 		return;
 
+	intel_edp_psr_exit(dev, true);
+}
+
+void intel_edp_psr_work(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, typeof(*dev_priv), psr.work.work);
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_encoder *encoder;
+	struct intel_dp *intel_dp = NULL;
+
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
 		if (encoder->type == INTEL_OUTPUT_EDP) {
 			intel_dp = enc_to_intel_dp(&encoder->base);
@@ -1860,9 +1874,66 @@ void intel_edp_psr_update(struct drm_device *dev)
 			if (!intel_edp_psr_match_conditions(intel_dp))
 				intel_edp_psr_disable(intel_dp);
 			else
-				if (!intel_edp_is_psr_enabled(dev))
-					intel_edp_psr_do_enable(intel_dp);
+				intel_edp_psr_do_enable(intel_dp);
+		}
+}
+
+void intel_edp_psr_inactivate(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_connector *connector;
+	struct intel_encoder *encoder;
+	struct intel_crtc *intel_crtc;
+	struct intel_dp *intel_dp = NULL;
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list,
+			    base.head) {
+
+		if (connector->base.dpms != DRM_MODE_DPMS_ON)
+			continue;
+
+		encoder = to_intel_encoder(connector->base.encoder);
+		if (encoder->type == INTEL_OUTPUT_EDP) {
+
+			intel_dp = enc_to_intel_dp(&encoder->base);
+			intel_crtc = to_intel_crtc(encoder->base.crtc);
+
+			dev_priv->psr.active = false;
+
+			I915_WRITE(EDP_PSR_CTL(dev), I915_READ(EDP_PSR_CTL(dev))
+				   & ~EDP_PSR_ENABLE);
 		}
+	}
+}
+
+void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!HAS_PSR(dev))
+		return;
+
+	if (!dev_priv->psr.setup_done)
+		return;
+
+	cancel_delayed_work_sync(&dev_priv->psr.work);
+
+	if (dev_priv->psr.active)
+		intel_edp_psr_inactivate(dev);
+
+	if (schedule_back)
+		schedule_delayed_work(&dev_priv->psr.work,
+				      msecs_to_jiffies(100));
+}
+
+void intel_edp_psr_init(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!HAS_PSR(dev))
+		return;
+
+	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_edp_psr_work);
 }
 
 static void intel_disable_dp(struct intel_encoder *encoder)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e72d45a..92164d6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -815,6 +815,9 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 void intel_edp_psr_update(struct drm_device *dev);
 void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
+void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back);
+void intel_edp_psr_init(struct drm_device *dev);
+
 
 /* intel_dsi.c */
 bool intel_dsi_init(struct drm_device *dev);
-- 
1.9.0

Re: [PATCH 06/11] drm/i915: Force PSR exit by inactivating it.

[Chris Wilson]

On Thu, May 15, 2014 at 08:13:05PM -0400, Rodrigo Vivi wrote:
> The perfect solution for psr_exit is the hardware tracking the changes and
> doing the psr exit by itself. This scenario works for HSW and BDW with some
> environments like Gnome and Wayland.
> 
> However there are many other scenarios that this isn't true. Mainly one right
> now is KDE users on HSW and BDW with PSR on. User would miss many screen
> updates. For instances any key typed could be seen only when mouse cursor is
> moved. So this patch introduces the ability of trigger PSR exit on kernel side
> on some common cases that.

You know that userspace has been waiting for a PSR flag for over a year
now so that it can use the more efficient rendering paths when it makes
sense.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 06/11] drm/i915: Force PSR exit by inactivating it.

[Rodrigo Vivi]

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

On Fri, May 16, 2014 at 3:23 AM, Chris Wilson <chris@chris-wilson.co.uk>wrote:

> On Thu, May 15, 2014 at 08:13:05PM -0400, Rodrigo Vivi wrote:
> > The perfect solution for psr_exit is the hardware tracking the changes
> and
> > doing the psr exit by itself. This scenario works for HSW and BDW with
> some
> > environments like Gnome and Wayland.
> >
> > However there are many other scenarios that this isn't true. Mainly one
> right
> > now is KDE users on HSW and BDW with PSR on. User would miss many screen
> > updates. For instances any key typed could be seen only when mouse
> cursor is
> > moved. So this patch introduces the ability of trigger PSR exit on
> kernel side
> > on some common cases that.
>
> You know that userspace has been waiting for a PSR flag for over a year
> now so that it can use the more efficient rendering paths when it makes
> sense.
>

yeah... this item is lingering on my to do list... but reaching a point
where I won't be able to continue postponing it ;)

> What happened to the front buffer tracking?

What front buffer tracking? hehe
I'm wondering about this since I started looking to fbc and psr and could
never find a reliable way.


-Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
>



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

[-- Attachment #1.2: Type: text/html, Size: 2637 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 06/11] drm/i915: Force PSR exit by inactivating it.

[Vijay Purushothaman]

On 5/16/2014 10:12 PM, Rodrigo Vivi wrote:
>
>
>
> On Fri, May 16, 2014 at 3:23 AM, Chris Wilson <chris@chris-wilson.co.uk
> <mailto:chris@chris-wilson.co.uk>> wrote:
>
>     On Thu, May 15, 2014 at 08:13:05PM -0400, Rodrigo Vivi wrote:
>      > The perfect solution for psr_exit is the hardware tracking the
>     changes and
>      > doing the psr exit by itself. This scenario works for HSW and BDW
>     with some
>      > environments like Gnome and Wayland.
>      >
>      > However there are many other scenarios that this isn't true.
>     Mainly one right
>      > now is KDE users on HSW and BDW with PSR on. User would miss many
>     screen
>      > updates. For instances any key typed could be seen only when
>     mouse cursor is
>      > moved. So this patch introduces the ability of trigger PSR exit
>     on kernel side
>      > on some common cases that.
>
>     You know that userspace has been waiting for a PSR flag for over a year
>     now so that it can use the more efficient rendering paths when it makes
>     sense.
>
>
> yeah... this item is lingering on my to do list... but reaching a point
> where I won't be able to continue postponing it ;)
>
>> What happened to the front buffer tracking?
>
> What front buffer tracking? hehe
> I'm wondering about this since I started looking to fbc and psr and
> could never find a reliable way.
>
FBC should cover most of the scenarios except cursor planes.. When ever 
cursor planes are enabled you can fall back to s/w controlled exit path.

If you can elaborate on the exact issue that you are facing with FBC and 
PSR may be i can help..

Thanks,
Vijay

>
>     -Chris
>
>     --
>     Chris Wilson, Intel Open Source Technology Centre
>
>
>
>
> --
> 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 06/11] drm/i915: Force PSR exit by inactivating it.

[Chris Wilson]

On Thu, May 15, 2014 at 08:13:05PM -0400, Rodrigo Vivi wrote:
> The perfect solution for psr_exit is the hardware tracking the changes and
> doing the psr exit by itself. This scenario works for HSW and BDW with some
> environments like Gnome and Wayland.
> 
> However there are many other scenarios that this isn't true. Mainly one right
> now is KDE users on HSW and BDW with PSR on. User would miss many screen
> updates. For instances any key typed could be seen only when mouse cursor is
> moved. So this patch introduces the ability of trigger PSR exit on kernel side
> on some common cases that.
> 
> Most of the cases are coverred by psr_exit at set_domain. The remaining cases
> are coverred by triggering it at set_domain, busy_ioctl, sw_finish and
> mark_busy.
> 
> The downside here might be reducing the residency time on the cases this
> already work very wall like Gnome environment. But so far let's get focused
> on fixinge issues sio PSR couild be used for everybody and we could even
> get it enabled by default. Later we can add some alternatives to choose the
> level of PSR efficiency over boot flag of even over crtc property.

What happened to the front buffer tracking?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 07/11] drm/i915: BDW PSR: Add single frame update support.

[Rodrigo Vivi]

When link is in stand by and PSR exit is triggered by a primary or sprite
plane flip this mode allows only one single updated frame to be send to
display than get back to PSR immediately.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 1 +
 drivers/gpu/drm/i915/intel_dp.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 957bd9a..7755a05 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2252,6 +2252,7 @@ enum punit_power_well {
 #define EDP_PSR_BASE(dev)                       (IS_HASWELL(dev) ? 0x64800 : 0x6f800)
 #define EDP_PSR_CTL(dev)			(EDP_PSR_BASE(dev) + 0)
 #define   EDP_PSR_ENABLE			(1<<31)
+#define   BDW_PSR_SINGLE_FRAME			(1<<30)
 #define   EDP_PSR_LINK_DISABLE			(0<<27)
 #define   EDP_PSR_LINK_STANDBY			(1<<27)
 #define   EDP_PSR_MIN_LINK_ENTRY_TIME_MASK	(3<<25)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 59baee5..28144d3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1715,6 +1715,7 @@ static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
 		val |= EDP_PSR_TP2_TP3_TIME_0us;
 		val |= EDP_PSR_TP1_TIME_0us;
 		val |= EDP_PSR_SKIP_AUX_EXIT;
+		val |= IS_BROADWELL(dev) ? BDW_PSR_SINGLE_FRAME : 0;
 	} else
 		val |= EDP_PSR_LINK_DISABLE;
 
-- 
1.9.0

[PATCH 08/11] drm/i915: BDW PSR: Remove limitations that aren't valid for BDW.

[Rodrigo Vivi]

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 28144d3..9421b0b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1768,6 +1768,10 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 		return false;
 	}
 
+	/* Below limitations aren't valid for Broadwell */
+	if (IS_BROADWELL(dev))
+		goto out;
+
 	if (I915_READ(SPRCTL(intel_crtc->pipe)) & SPRITE_ENABLE) {
 		DRM_DEBUG_KMS("PSR condition failed: Sprite is Enabled\n");
 		return false;
@@ -1784,6 +1788,7 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 		return false;
 	}
 
+ out:
 	dev_priv->psr.source_ok = true;
 	return true;
 }
-- 
1.9.0

Re: [PATCH 08/11] drm/i915: BDW PSR: Remove limitations that aren't valid for BDW.

[Vijay Purushothaman]

On 5/16/2014 5:43 AM, Rodrigo Vivi wrote:
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 28144d3..9421b0b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1768,6 +1768,10 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
>   		return false;
>   	}
>
> +	/* Below limitations aren't valid for Broadwell */
> +	if (IS_BROADWELL(dev))
> +		goto out;

I couldn't figure out any sprite related restrictions for HSW as well. 
Is this because FBC logic doesn't track sprites in HSW?

Thanks,
Vijay


> +
>   	if (I915_READ(SPRCTL(intel_crtc->pipe)) & SPRITE_ENABLE) {
>   		DRM_DEBUG_KMS("PSR condition failed: Sprite is Enabled\n");
>   		return false;
> @@ -1784,6 +1788,7 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
>   		return false;
>   	}
>
> + out:
>   	dev_priv->psr.source_ok = true;
>   	return true;
>   }
>

[PATCH 09/11] drm/i915: BDW PSR: Remove DDIA limitation for Broadwell.

[Rodrigo Vivi]

Broadwell has a PSR per transcoder, where DDIA supports
link disable and link standby modes while other
transcoders only support link standby.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9421b0b..47053e6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1675,16 +1675,21 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
 
 static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
 {
-	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t aux_clock_divider;
 	int precharge = 0x3;
 	int msg_size = 5;       /* Header(4) + Message(1) */
+	bool only_standby = false;
 
 	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
 
+	if (IS_BROADWELL(dev) && dig_port->port != PORT_A)
+		only_standby = true;
+
 	/* Enable PSR in sink */
-	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT)
+	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT || only_standby)
 		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
 				   DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
 	else
@@ -1703,14 +1708,19 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
 
 static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
 {
-	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t max_sleep_time = 0x1f;
 	uint32_t idle_frames = 1;
 	uint32_t val = 0x0;
 	const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
+	bool only_standby = false;
 
-	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) {
+	if (IS_BROADWELL(dev) && dig_port->port != PORT_A)
+		only_standby = true;
+
+	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT || only_standby) {
 		val |= EDP_PSR_LINK_STANDBY;
 		val |= EDP_PSR_TP2_TP3_TIME_0us;
 		val |= EDP_PSR_TP1_TIME_0us;
@@ -1738,8 +1748,13 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 
 	dev_priv->psr.source_ok = false;
 
-	if ((intel_encoder->type != INTEL_OUTPUT_EDP) ||
-	    (dig_port->port != PORT_A)) {
+	if (!HAS_PSR(dev)) {
+		DRM_DEBUG_KMS("PSR not supported on this platform\n");
+		return false;
+	}
+
+	if (IS_HASWELL(dev) && (intel_encoder->type != INTEL_OUTPUT_EDP ||
+				dig_port->port != PORT_A)) {
 		DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
 		return false;
 	}
-- 
1.9.0

[PATCH 10/11] drm/i915: Improve PSR debugfs status.

[Rodrigo Vivi]

Now we have the active/inactive state for exit and this actually changes the
HW enable bit the status was a bit confusing for users. So let's provide
more info.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6636ca2..0ca9376 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1975,10 +1975,12 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 
 	seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support));
 	seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok));
+	seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled));
+	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
 
 	enabled = HAS_PSR(dev) &&
 		I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE;
-	seq_printf(m, "Enabled: %s\n", yesno(enabled));
+	seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled));
 
 	if (HAS_PSR(dev))
 		psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) &
-- 
1.9.0

Re: [PATCH 10/11] drm/i915: Improve PSR debugfs status.

[Vijay Purushothaman]

On 5/16/2014 5:43 AM, Rodrigo Vivi wrote:
> Now we have the active/inactive state for exit and this actually changes the
> HW enable bit the status was a bit confusing for users. So let's provide
> more info.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 6636ca2..0ca9376 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1975,10 +1975,12 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>
>   	seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support));
>   	seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok));
> +	seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled));
> +	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
>
>   	enabled = HAS_PSR(dev) &&
>   		I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE;
> -	seq_printf(m, "Enabled: %s\n", yesno(enabled));
> +	seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled));
>
>   	if (HAS_PSR(dev))
>   		psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) &

Please remove all references to PSR performance counter. This register 
is primarily meant as a debug register and its implementation is broken 
in the h/w. Whenever the cdclk is gated to save power, the performance 
counter is stopped. But when the clk is re-enabled it doesn't reset the 
counter. This unnecessarily confuses the end users.. When the system 
goes through suspend / resume cycle the performance counter most likely 
will transition from a non-zero value to zero.. I already received few 
queries from our customers related to this performance customer and they 
refuse to believe me when i tell them PSR is still functional when the 
performance counter reports 0 :-)

AFAIK this register definition is missing in open source HSW B spec as 
well..

Thanks,
Vijay
>

Re: [PATCH 10/11] drm/i915: Improve PSR debugfs status.

[Daniel Vetter]

On Mon, Jun 02, 2014 at 11:54:10PM +0530, Vijay Purushothaman wrote:
> On 5/16/2014 5:43 AM, Rodrigo Vivi wrote:
> >Now we have the active/inactive state for exit and this actually changes the
> >HW enable bit the status was a bit confusing for users. So let's provide
> >more info.
> >
> >Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> >---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >index 6636ca2..0ca9376 100644
> >--- a/drivers/gpu/drm/i915/i915_debugfs.c
> >+++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >@@ -1975,10 +1975,12 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> >
> >  	seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support));
> >  	seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok));
> >+	seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled));
> >+	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
> >
> >  	enabled = HAS_PSR(dev) &&
> >  		I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE;
> >-	seq_printf(m, "Enabled: %s\n", yesno(enabled));
> >+	seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled));
> >
> >  	if (HAS_PSR(dev))
> >  		psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) &
> 
> Please remove all references to PSR performance counter. This register is
> primarily meant as a debug register and its implementation is broken in the
> h/w. Whenever the cdclk is gated to save power, the performance counter is
> stopped. But when the clk is re-enabled it doesn't reset the counter. This
> unnecessarily confuses the end users.. When the system goes through suspend
> / resume cycle the performance counter most likely will transition from a
> non-zero value to zero.. I already received few queries from our customers
> related to this performance customer and they refuse to believe me when i
> tell them PSR is still functional when the performance counter reports 0 :-)

We expose other such perf registers and imo this is handy for debugging.
Also we have a big push to expose all this perf stuff recently ...

Imo we should keep this, if we can. If confused customers noodle around in
debugfs without clue, maybe they shouldn't.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 10/11] drm/i915: Improve PSR debugfs status.

[Vijay Purushothaman]

On 6/3/2014 1:10 PM, Daniel Vetter wrote:
> On Mon, Jun 02, 2014 at 11:54:10PM +0530, Vijay Purushothaman wrote:
>> On 5/16/2014 5:43 AM, Rodrigo Vivi wrote:
>>> Now we have the active/inactive state for exit and this actually changes the
>>> HW enable bit the status was a bit confusing for users. So let's provide
>>> more info.
>>>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_debugfs.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 6636ca2..0ca9376 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -1975,10 +1975,12 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>>>
>>>   	seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support));
>>>   	seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok));
>>> +	seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled));
>>> +	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
>>>
>>>   	enabled = HAS_PSR(dev) &&
>>>   		I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE;
>>> -	seq_printf(m, "Enabled: %s\n", yesno(enabled));
>>> +	seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled));
>>>
>>>   	if (HAS_PSR(dev))
>>>   		psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) &
>>
>> Please remove all references to PSR performance counter. This register is
>> primarily meant as a debug register and its implementation is broken in the
>> h/w. Whenever the cdclk is gated to save power, the performance counter is
>> stopped. But when the clk is re-enabled it doesn't reset the counter. This
>> unnecessarily confuses the end users.. When the system goes through suspend
>> / resume cycle the performance counter most likely will transition from a
>> non-zero value to zero.. I already received few queries from our customers
>> related to this performance customer and they refuse to believe me when i
>> tell them PSR is still functional when the performance counter reports 0 :-)
>
> We expose other such perf registers and imo this is handy for debugging.
> Also we have a big push to expose all this perf stuff recently ...
>
> Imo we should keep this, if we can. If confused customers noodle around in
> debugfs without clue, maybe they shouldn't.
> -Daniel

In that case here is my

Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>

Thanks,
Vijay
>

[PATCH 11/11] drm/i915: PSR HSW: update after enabling sprite.

[Rodrigo Vivi]

On the current structure HSW doesn't support PSR with sprites enabled
but sprites can be enabled after PSR was enabled what would cause
user to miss screen updates.

v2: move it to update_plane.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 213cd58..4b85400 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1051,6 +1051,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		mutex_unlock(&dev->struct_mutex);
 	}
 
+	intel_edp_psr_update(dev);
+
 	return 0;
 }
 
-- 
1.9.0

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

Re: [PATCH 11/11] drm/i915: PSR HSW: update after enabling sprite.

[Vijay Purushothaman]

On 5/16/2014 5:43 AM, Rodrigo Vivi wrote:
> On the current structure HSW doesn't support PSR with sprites enabled
> but sprites can be enabled after PSR was enabled what would cause
> user to miss screen updates.

Could you please explain this a bit more? Did you get a confirmation 
from h/w team that this is not possible? AFAIK, HSW should be able to 
support PSR with sprites enabled.

Thanks,
Vijay


>
> v2: move it to update_plane.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>   drivers/gpu/drm/i915/intel_sprite.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 213cd58..4b85400 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1051,6 +1051,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   		mutex_unlock(&dev->struct_mutex);
>   	}
>
> +	intel_edp_psr_update(dev);
> +
>   	return 0;
>   }
>
>

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

Re: [PATCH 00/11] HSW/BDW PSR.

[Vijay Purushothaman]

On 5/16/2014 5:42 AM, Rodrigo Vivi wrote:
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Hi All,
>
> This series introduces fixes for PSR on HSW and on BDW and "new features"
> for PSR on BDW.
>
> The biggest thing on this serie is the introduction of the psr_exit
> infrastructure that was actually created for PSR on Baytrail. However
> since on Baytrail the HW cannot track absolutelly no screen update lets
> put it first to work on HSW and BDW. This brings more reliability to PSR and
> make it possible to use even on non GL userspace environments like KDE.
>
> Without this psr_exit infrastructure we will never be able to enable psr by
> default because this breaks the userspace for all KDE users.
> I understand the possible limitations of this infrastructure, but this is the
> best we can do for now.
>
> Other possibilities to solve this issue is let the full control to userspace
> over ioctl. I'm thinking about a next rework where userspace could dinamically
> switch over some possible PSR levels like:
> 1 - Full HW control - that works good enough on Gnome HSW and extract the best
>                        residency time.
> 2 - PSR-exit by inactivating - That would be for KDE users
> 3 - Full SW control - where userspace would control PSR exit/entry flow over\
>                        ioctls.
>
> On BDW "new features" there are basically the intorducion of a single frame
> update support what in theory improve residency time and also remove
> limitations that only affect HSW like DDI only on PORT_A and PSR off when
> sprites are in use.
>
> Please help me to get this merged with good suggestions of improvements.
> Please do not say just: "that is not good". I know that already. Please provide
> good ideas along with the comments.
>
> Thanks in advance,
> Rodrigo.
>
> Rodrigo Vivi (11):
>    drm/i915: move psr_setup_done to psr struct
>    drm/i915: Update PSR on resume.
>    drm/i915: Use HAS_PSR to avoid unecessary interactions.
>    drm/i915: Don't let update_psr function actually enable PSR.
>    drm/i915: Do not try to enable PSR when Panel doesn't suport it.
>    drm/i915: Force PSR exit by inactivating it.
>    drm/i915: BDW PSR: Add single frame update support.
>    drm/i915: BDW PSR: Remove limitations that aren't valid for BDW.
>    drm/i915: BDW PSR: Remove DDIA limitation for Broadwell.
>    drm/i915: Improve PSR debugfs status.
>    drm/i915: PSR HSW: update after enabling sprite.
>
>   drivers/gpu/drm/i915/i915_debugfs.c  |   4 +-
>   drivers/gpu/drm/i915/i915_drv.h      |   4 +
>   drivers/gpu/drm/i915/i915_gem.c      |   6 ++
>   drivers/gpu/drm/i915/i915_reg.h      |   1 +
>   drivers/gpu/drm/i915/i915_suspend.c  |   3 +
>   drivers/gpu/drm/i915/intel_display.c |  20 ++++-
>   drivers/gpu/drm/i915/intel_dp.c      | 153 ++++++++++++++++++++++++++++-------
>   drivers/gpu/drm/i915/intel_drv.h     |   4 +-
>   drivers/gpu/drm/i915/intel_sprite.c  |   2 +
>   9 files changed, 165 insertions(+), 32 deletions(-)
>

Thanks for sharing the PM enabling guide document. After going through 
that document, things are clear for me.

For the series,
Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>

Thanks,
Vijay