[PATCH 00/15] Accurate frontbuffer tracking and psr conversion

[PATCH 00/15] Accurate frontbuffer tracking and psr conversion

[Daniel Vetter]

Hi all,

This patch series adds accurate frontbuffer tracking to i915 and then converts
psr over to use. Bunch of things still need to be done.

- PSR needs to be re-tested. I lack the hardware for that. The frontbuffer
  tracking itself is tested.

- PSR igt testcase needs to be extended so that we cover all upload methods on
  all plane types.

- DRRS/downclocking needs to be unified and put into this framework properly.
  Also needs proper locking for all of the DRRS state.

- fbc also needs to be fixed up, with state handling properly split between
  crtc_enable/disable, primary fb updates and the fb tracking for nuking. A bit
  unclear how we want to integrate gtt cpu write tracking through the hw, since
  that seems to be the hw tracking piece that actually works.

General blueprint for display runtime power saving features:
- Have all your state in either intel_crtc or dev_priv, protected by its own
  lock.

- Do state setup in crtc_enable, cleanup in crtc_disable with a pair of
  enable/disable fuctions. Optionally update everywhere else you want (e.g.
  primary plane updates for fbc). Not state setup anywhere else allowed, except
  maybe an _init for setting up work items, locks, ...

- Intercept the invalidation/flush signals you need like
  psr_invalidate/psr_flush. Track internally which frontbuffer bits you're
  interested in and invalidate/flush accordingly. You can also use these for
  workarounds (e.g. on hsw we force-flush for sprite changes since the flip
  signal doesn't result in a hw image upload).


Note that currently the gtt tracking has a bit a gap: We never exit it. Bunch of
fixes are possible:
- Wire up the core dirty_fb ioctl to flush framebuffers. This is used by generic
  userspace to flush dummy buffers, which in our case means gtt mmaps. So maps
  perfectly.

- Do a delayed gtt pte teardown and force-flush. Probably too ugly to care
  really.

- Try to integrate hw gtt write tracking logic. Note that current psr code
  doesn't rely on this - I've killed the X-tiled checks completely.

Big thanks to Chris for some great ideas for the fb tracking scheme and the
precise placement of the invalidate/flush functions.

Comments, flames and especially testing on psr hardware highly welcome.

Cheers, Daniel

Daniel Vetter (15):
  drm/i915: Add missing statics to recent psr functions
  drm/i915: Drop unecessary complexity from psr_inactivate
  drm/i915: Ditch intel_edp_psr_update
  drm/i915: Run psr_setup unconditionally
  drm/i915: Drop schedule_back from psr_exit
  drm/i915: Add a FIXME about drrs/psr interactions
  drm/i915: Track the psr dp connector in dev_priv->psr.enabled
  drm/i915: Don't try to disable psr harder from the work item
  drm/i915: Lock down psr sw/hw state tracking
  drm/i915: More checks for psr.enabled
  drm/i915: Add locking to psr code
  drm/i915: Introduce accurate frontbuffer tracking
  drm/i915: Use new frontbuffer bits to increase pll clock
  drm/i915: Track frontbuffer invalidation/flushing
  drm/i915: Fix up PSR frontbuffer tracking

 drivers/gpu/drm/i915/i915_debugfs.c        |   2 +-
 drivers/gpu/drm/i915/i915_drv.h            |  46 ++++-
 drivers/gpu/drm/i915/i915_gem.c            |  38 +++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   2 +-
 drivers/gpu/drm/i915/intel_display.c       | 283 +++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_dp.c            | 189 +++++++++----------
 drivers/gpu/drm/i915/intel_drv.h           |  16 +-
 drivers/gpu/drm/i915/intel_fbdev.c         |  18 +-
 drivers/gpu/drm/i915/intel_overlay.c       |  14 ++
 drivers/gpu/drm/i915/intel_sprite.c        |  11 +-
 10 files changed, 458 insertions(+), 161 deletions(-)

-- 
2.0.0

[PATCH 01/15] drm/i915: Add missing statics to recent psr functions

[Daniel Vetter]

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b6b26407f11b..190df701edd5 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1888,7 +1888,7 @@ void intel_edp_psr_update(struct drm_device *dev)
 	intel_edp_psr_exit(dev, true);
 }
 
-void intel_edp_psr_work(struct work_struct *work)
+static void intel_edp_psr_work(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
 		container_of(work, typeof(*dev_priv), psr.work.work);
@@ -1907,7 +1907,7 @@ void intel_edp_psr_work(struct work_struct *work)
 		}
 }
 
-void intel_edp_psr_inactivate(struct drm_device *dev)
+static void intel_edp_psr_inactivate(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_connector *connector;
-- 
2.0.0

Re: [PATCH 01/15] drm/i915: Add missing statics to recent psr functions

[Rodrigo Vivi]

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

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch>
wrote:

> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index b6b26407f11b..190df701edd5 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1888,7 +1888,7 @@ void intel_edp_psr_update(struct drm_device *dev)
>         intel_edp_psr_exit(dev, true);
>  }
>
> -void intel_edp_psr_work(struct work_struct *work)
> +static void intel_edp_psr_work(struct work_struct *work)
>  {
>         struct drm_i915_private *dev_priv =
>                 container_of(work, typeof(*dev_priv), psr.work.work);
> @@ -1907,7 +1907,7 @@ void intel_edp_psr_work(struct work_struct *work)
>                 }
>  }
>
> -void intel_edp_psr_inactivate(struct drm_device *dev)
> +static void intel_edp_psr_inactivate(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct intel_connector *connector;
> --
> 2.0.0
>
> _______________________________________________
> 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: 2384 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 01/15] drm/i915: Add missing statics to recent psr functions

[Daniel Vetter]

On Mon, Jun 16, 2014 at 04:57:48PM -0700, Rodrigo Vivi wrote:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Merged, thanks for the review.
-Daniel

> 
> 
> On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch>
> wrote:
> 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index b6b26407f11b..190df701edd5 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1888,7 +1888,7 @@ void intel_edp_psr_update(struct drm_device *dev)
> >         intel_edp_psr_exit(dev, true);
> >  }
> >
> > -void intel_edp_psr_work(struct work_struct *work)
> > +static void intel_edp_psr_work(struct work_struct *work)
> >  {
> >         struct drm_i915_private *dev_priv =
> >                 container_of(work, typeof(*dev_priv), psr.work.work);
> > @@ -1907,7 +1907,7 @@ void intel_edp_psr_work(struct work_struct *work)
> >                 }
> >  }
> >
> > -void intel_edp_psr_inactivate(struct drm_device *dev)
> > +static void intel_edp_psr_inactivate(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         struct intel_connector *connector;
> > --
> > 2.0.0
> >
> > _______________________________________________
> > 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

[PATCH 02/15] drm/i915: Drop unecessary complexity from psr_inactivate

[Daniel Vetter]

It's not needed and further more will get in the way of a sane
locking scheme - psr_exit _can't_ take modeset locks due to lock
inversion, and at least once dp mst hits the connector list
is no longer static.

But since we track all state in dev_priv->psr there is no need
at all.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 190df701edd5..90f6f0a42d15 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1910,29 +1910,11 @@ static void intel_edp_psr_work(struct work_struct *work)
 static 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) {
+	dev_priv->psr.active = false;
 
-			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);
-		}
-	}
+	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)
-- 
2.0.0

Re: [PATCH 02/15] drm/i915: Drop unecessary complexity from psr_inactivate

[Rodrigo Vivi]

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

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch>
wrote:

> It's not needed and further more will get in the way of a sane
> locking scheme - psr_exit _can't_ take modeset locks due to lock
> inversion, and at least once dp mst hits the connector list
> is no longer static.
>
> But since we track all state in dev_priv->psr there is no need
> at all.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 24 +++---------------------
>  1 file changed, 3 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 190df701edd5..90f6f0a42d15 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1910,29 +1910,11 @@ static void intel_edp_psr_work(struct work_struct
> *work)
>  static 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) {
> +       dev_priv->psr.active = false;
>
> -                       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);
> -               }
> -       }
> +       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)
> --
> 2.0.0
>
> _______________________________________________
> 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: 3606 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 03/15] drm/i915: Ditch intel_edp_psr_update

[Daniel Vetter]

We have _enable/_disable interfaces now for the modeset sequence and
intel_edp_psr_exit for workarounds.

The callsites in intel_display.c are all redundant with the modeset
sequence enable/disable calls in intel_ddi.c. The one in
intel_sprite.c is real and needs to be switched to psr_exit.

If this breaks anything then we need to augment the enable/disable
functions accordingly.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |  5 -----
 drivers/gpu/drm/i915/intel_dp.c      | 13 -------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 drivers/gpu/drm/i915/intel_sprite.c  |  2 +-
 4 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a0f4709f9479..c27dadebd0dc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2763,7 +2763,6 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
-	intel_edp_psr_update(dev);
 	mutex_unlock(&dev->struct_mutex);
 
 	return 0;
@@ -3943,7 +3942,6 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
-	intel_edp_psr_update(dev);
 	mutex_unlock(&dev->struct_mutex);
 }
 
@@ -4236,7 +4234,6 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
-	intel_edp_psr_update(dev);
 	mutex_unlock(&dev->struct_mutex);
 }
 
@@ -4284,7 +4281,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
-	intel_edp_psr_update(dev);
 	mutex_unlock(&dev->struct_mutex);
 }
 
@@ -4836,7 +4832,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
-	intel_edp_psr_update(dev);
 	mutex_unlock(&dev->struct_mutex);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 90f6f0a42d15..8717d3b9667c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1875,19 +1875,6 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 	dev_priv->psr.enabled = false;
 }
 
-void intel_edp_psr_update(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (!HAS_PSR(dev))
-		return;
-
-	if (!dev_priv->psr.setup_done)
-		return;
-
-	intel_edp_psr_exit(dev, true);
-}
-
 static void intel_edp_psr_work(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0081f79efad4..87e83c315c4b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -828,7 +828,6 @@ void intel_edp_panel_on(struct intel_dp *intel_dp);
 void intel_edp_panel_off(struct intel_dp *intel_dp);
 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);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 404335d53a89..2a211c64ec8d 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1051,7 +1051,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	intel_edp_psr_update(dev);
+	intel_edp_psr_exit(dev, true);
 
 	return 0;
 }
-- 
2.0.0

Re: [PATCH 03/15] drm/i915: Ditch intel_edp_psr_update

[Rodrigo Vivi]

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

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch>
wrote:

> We have _enable/_disable interfaces now for the modeset sequence and
> intel_edp_psr_exit for workarounds.
>
> The callsites in intel_display.c are all redundant with the modeset
> sequence enable/disable calls in intel_ddi.c. The one in
> intel_sprite.c is real and needs to be switched to psr_exit.
>
> If this breaks anything then we need to augment the enable/disable
> functions accordingly.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  5 -----
>  drivers/gpu/drm/i915/intel_dp.c      | 13 -------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  drivers/gpu/drm/i915/intel_sprite.c  |  2 +-
>  4 files changed, 1 insertion(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index a0f4709f9479..c27dadebd0dc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2763,7 +2763,6 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x,
> int y,
>
>         mutex_lock(&dev->struct_mutex);
>         intel_update_fbc(dev);
> -       intel_edp_psr_update(dev);
>         mutex_unlock(&dev->struct_mutex);
>
>         return 0;
> @@ -3943,7 +3942,6 @@ static void intel_crtc_enable_planes(struct drm_crtc
> *crtc)
>
>         mutex_lock(&dev->struct_mutex);
>         intel_update_fbc(dev);
> -       intel_edp_psr_update(dev);
>         mutex_unlock(&dev->struct_mutex);
>  }
>
> @@ -4236,7 +4234,6 @@ static void ironlake_crtc_disable(struct drm_crtc
> *crtc)
>
>         mutex_lock(&dev->struct_mutex);
>         intel_update_fbc(dev);
> -       intel_edp_psr_update(dev);
>         mutex_unlock(&dev->struct_mutex);
>  }
>
> @@ -4284,7 +4281,6 @@ static void haswell_crtc_disable(struct drm_crtc
> *crtc)
>
>         mutex_lock(&dev->struct_mutex);
>         intel_update_fbc(dev);
> -       intel_edp_psr_update(dev);
>         mutex_unlock(&dev->struct_mutex);
>  }
>
> @@ -4836,7 +4832,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>
>         mutex_lock(&dev->struct_mutex);
>         intel_update_fbc(dev);
> -       intel_edp_psr_update(dev);
>         mutex_unlock(&dev->struct_mutex);
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 90f6f0a42d15..8717d3b9667c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1875,19 +1875,6 @@ void intel_edp_psr_disable(struct intel_dp
> *intel_dp)
>         dev_priv->psr.enabled = false;
>  }
>
> -void intel_edp_psr_update(struct drm_device *dev)
> -{
> -       struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -       if (!HAS_PSR(dev))
> -               return;
> -
> -       if (!dev_priv->psr.setup_done)
> -               return;
> -
> -       intel_edp_psr_exit(dev, true);
> -}
> -
>  static void intel_edp_psr_work(struct work_struct *work)
>  {
>         struct drm_i915_private *dev_priv =
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 0081f79efad4..87e83c315c4b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -828,7 +828,6 @@ void intel_edp_panel_on(struct intel_dp *intel_dp);
>  void intel_edp_panel_off(struct intel_dp *intel_dp);
>  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);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 404335d53a89..2a211c64ec8d 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1051,7 +1051,7 @@ intel_update_plane(struct drm_plane *plane, struct
> drm_crtc *crtc,
>                 mutex_unlock(&dev->struct_mutex);
>         }
>
> -       intel_edp_psr_update(dev);
> +       intel_edp_psr_exit(dev, true);
>
>         return 0;
>  }
> --
> 2.0.0
>
> _______________________________________________
> 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: 5927 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 04/15] drm/i915: Run psr_setup unconditionally

[Daniel Vetter]

Due to runtime pm and system s/r we need to restore hw state every
time we enable a pipe again. Hence trying to avoid that is just
pointless book-keeping which Rodrigo then tried to work around by
manually adding psr_setup calls to our resume code.

Much simpler to just remove code instead.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h | 1 -
 drivers/gpu/drm/i915/intel_dp.c | 8 --------
 2 files changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 64d520f7e8d9..f5db29428406 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -636,7 +636,6 @@ struct i915_drrs {
 struct i915_psr {
 	bool sink_support;
 	bool source_ok;
-	bool setup_done;
 	bool enabled;
 	bool active;
 	struct delayed_work work;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8717d3b9667c..4ab4757fb53d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1663,9 +1663,6 @@ 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;
@@ -1677,8 +1674,6 @@ 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)
@@ -1911,9 +1906,6 @@ void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
 	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)
-- 
2.0.0

Re: [PATCH 04/15] drm/i915: Run psr_setup unconditionally

[Rodrigo Vivi]

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

On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch>
wrote:

> Due to runtime pm and system s/r we need to restore hw state every
> time we enable a pipe again. Hence trying to avoid that is just
> pointless book-keeping which Rodrigo then tried to work around by
> manually adding psr_setup calls to our resume code.
>
> Much simpler to just remove code instead.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 1 -
>  drivers/gpu/drm/i915/intel_dp.c | 8 --------
>  2 files changed, 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 64d520f7e8d9..f5db29428406 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -636,7 +636,6 @@ struct i915_drrs {
>  struct i915_psr {
>         bool sink_support;
>         bool source_ok;
> -       bool setup_done;
>         bool enabled;
>         bool active;
>         struct delayed_work work;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 8717d3b9667c..4ab4757fb53d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1663,9 +1663,6 @@ 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;
> @@ -1677,8 +1674,6 @@ 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)
> @@ -1911,9 +1906,6 @@ void intel_edp_psr_exit(struct drm_device *dev, bool
> schedule_back)
>         if (!HAS_PSR(dev))
>                 return;
>
> -       if (!dev_priv->psr.setup_done)
> -               return;

-
>

I'd add here:
+       if (!dev_priv->psr.enabled)
+               return;



>         cancel_delayed_work_sync(&dev_priv->psr.work);
>
>         if (dev_priv->psr.active)
> --
> 2.0.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-g
> <http://lists.freedesktop.org/mailman/listinfo/intel-gfx>


with or without it:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

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

[-- Attachment #1.2: Type: text/html, Size: 4404 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 04/15] drm/i915: Run psr_setup unconditionally

[Chris Wilson]

On Mon, Jun 16, 2014 at 05:03:02PM -0700, Rodrigo Vivi wrote:
>    On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter
>    <[1]daniel.vetter@ffwll.ch> wrote:
> 
>       static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
>      @@ -1911,9 +1906,6 @@ void intel_edp_psr_exit(struct drm_device *dev,
>      bool schedule_back)
>              if (!HAS_PSR(dev))
>                      return;

Whilst you are here, this should just be a if (dev_priv->psr.enabled)
check.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 04/15] drm/i915: Run psr_setup unconditionally

[Daniel Vetter]

On Tue, Jun 17, 2014 at 07:43:53AM +0100, Chris Wilson wrote:
> On Mon, Jun 16, 2014 at 05:03:02PM -0700, Rodrigo Vivi wrote:
> >    On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter
> >    <[1]daniel.vetter@ffwll.ch> wrote:
> > 
> >       static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
> >      @@ -1911,9 +1906,6 @@ void intel_edp_psr_exit(struct drm_device *dev,
> >      bool schedule_back)
> >              if (!HAS_PSR(dev))
> >                      return;
> 
> Whilst you are here, this should just be a if (dev_priv->psr.enabled)
> check.

I have fixup for psr_invalidate/psr_flush, as this ends up being called to
do exactly that. It blew up ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 04/15] drm/i915: Run psr_setup unconditionally

[Daniel Vetter]

On Tue, Jun 17, 2014 at 09:23:48AM +0200, Daniel Vetter wrote:
> On Tue, Jun 17, 2014 at 07:43:53AM +0100, Chris Wilson wrote:
> > On Mon, Jun 16, 2014 at 05:03:02PM -0700, Rodrigo Vivi wrote:
> > >    On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter
> > >    <[1]daniel.vetter@ffwll.ch> wrote:
> > > 
> > >       static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
> > >      @@ -1911,9 +1906,6 @@ void intel_edp_psr_exit(struct drm_device *dev,
> > >      bool schedule_back)
> > >              if (!HAS_PSR(dev))
> > >                      return;
> > 
> > Whilst you are here, this should just be a if (dev_priv->psr.enabled)
> > check.
> 
> I have fixup for psr_invalidate/psr_flush, as this ends up being called to
> do exactly that. It blew up ;-)

So another bisect failure in the series. I'll fix this up.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 05/15] drm/i915: Drop schedule_back from psr_exit

[Daniel Vetter]

It doesn't make sense to never again schedule the work, since by the
time we might want to re-enable psr the world might have changed and
we can do it again.

The only exception is when we shut down the pipe, but that's an
entirely different thing and needs to be handled in psr_disable.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c      | 6 +++---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 drivers/gpu/drm/i915/intel_dp.c      | 7 +++----
 drivers/gpu/drm/i915/intel_drv.h     | 2 +-
 drivers/gpu/drm/i915/intel_sprite.c  | 2 +-
 5 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1794a041c13c..f22b4aabb945 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1395,7 +1395,7 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 		goto unlock;
 	}
 
-	intel_edp_psr_exit(dev, true);
+	intel_edp_psr_exit(dev);
 
 	/* Try to flush the object off the GPU without holding the lock.
 	 * We will repeat the flush holding the lock in the normal manner
@@ -1442,7 +1442,7 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	intel_edp_psr_exit(dev, true);
+	intel_edp_psr_exit(dev);
 
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
@@ -4240,7 +4240,7 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	intel_edp_psr_exit(dev, true);
+	intel_edp_psr_exit(dev);
 
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c27dadebd0dc..6f2588c95248 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8823,7 +8823,7 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 	struct drm_device *dev = obj->base.dev;
 	struct drm_crtc *crtc;
 
-	intel_edp_psr_exit(dev, true);
+	intel_edp_psr_exit(dev);
 
 	if (!i915.powersave)
 		return;
@@ -9292,7 +9292,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		return -ENOMEM;
 
 	/* Exit PSR early in page flip */
-	intel_edp_psr_exit(dev, true);
+	intel_edp_psr_exit(dev);
 
 	work->event = event;
 	work->crtc = crtc;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4ab4757fb53d..c7d625040e3d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1899,7 +1899,7 @@ static void intel_edp_psr_inactivate(struct drm_device *dev)
 		   & ~EDP_PSR_ENABLE);
 }
 
-void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
+void intel_edp_psr_exit(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -1911,9 +1911,8 @@ void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
 	if (dev_priv->psr.active)
 		intel_edp_psr_inactivate(dev);
 
-	if (schedule_back)
-		schedule_delayed_work(&dev_priv->psr.work,
-				      msecs_to_jiffies(100));
+	schedule_delayed_work(&dev_priv->psr.work,
+			      msecs_to_jiffies(100));
 }
 
 void intel_edp_psr_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 87e83c315c4b..1d45629a6483 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -829,7 +829,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp);
 void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 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_exit(struct drm_device *dev);
 void intel_edp_psr_init(struct drm_device *dev);
 
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 2a211c64ec8d..9038e2ab73c8 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1051,7 +1051,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	intel_edp_psr_exit(dev, true);
+	intel_edp_psr_exit(dev);
 
 	return 0;
 }
-- 
2.0.0

Re: [PATCH 05/15] drm/i915: Drop schedule_back from psr_exit

[Rodrigo Vivi]

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

There were more reasons for disabling it on Baytrail... but you are right..
disable sequence should be better for those cases.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch>
wrote:

> It doesn't make sense to never again schedule the work, since by the
> time we might want to re-enable psr the world might have changed and
> we can do it again.
>
> The only exception is when we shut down the pipe, but that's an
> entirely different thing and needs to be handled in psr_disable.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c      | 6 +++---
>  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>  drivers/gpu/drm/i915/intel_dp.c      | 7 +++----
>  drivers/gpu/drm/i915/intel_drv.h     | 2 +-
>  drivers/gpu/drm/i915/intel_sprite.c  | 2 +-
>  5 files changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index 1794a041c13c..f22b4aabb945 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1395,7 +1395,7 @@ i915_gem_set_domain_ioctl(struct drm_device *dev,
> void *data,
>                 goto unlock;
>         }
>
> -       intel_edp_psr_exit(dev, true);
> +       intel_edp_psr_exit(dev);
>
>         /* Try to flush the object off the GPU without holding the lock.
>          * We will repeat the flush holding the lock in the normal manner
> @@ -1442,7 +1442,7 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev,
> void *data,
>         if (ret)
>                 return ret;
>
> -       intel_edp_psr_exit(dev, true);
> +       intel_edp_psr_exit(dev);
>
>         obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
>         if (&obj->base == NULL) {
> @@ -4240,7 +4240,7 @@ i915_gem_busy_ioctl(struct drm_device *dev, void
> *data,
>         if (ret)
>                 return ret;
>
> -       intel_edp_psr_exit(dev, true);
> +       intel_edp_psr_exit(dev);
>
>         obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
>         if (&obj->base == NULL) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index c27dadebd0dc..6f2588c95248 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8823,7 +8823,7 @@ void intel_mark_fb_busy(struct drm_i915_gem_object
> *obj,
>         struct drm_device *dev = obj->base.dev;
>         struct drm_crtc *crtc;
>
> -       intel_edp_psr_exit(dev, true);
> +       intel_edp_psr_exit(dev);
>
>         if (!i915.powersave)
>                 return;
> @@ -9292,7 +9292,7 @@ static int intel_crtc_page_flip(struct drm_crtc
> *crtc,
>                 return -ENOMEM;
>
>         /* Exit PSR early in page flip */
> -       intel_edp_psr_exit(dev, true);
> +       intel_edp_psr_exit(dev);
>
>         work->event = event;
>         work->crtc = crtc;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 4ab4757fb53d..c7d625040e3d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1899,7 +1899,7 @@ static void intel_edp_psr_inactivate(struct
> drm_device *dev)
>                    & ~EDP_PSR_ENABLE);
>  }
>
> -void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
> +void intel_edp_psr_exit(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
> @@ -1911,9 +1911,8 @@ void intel_edp_psr_exit(struct drm_device *dev, bool
> schedule_back)
>         if (dev_priv->psr.active)
>                 intel_edp_psr_inactivate(dev);
>
> -       if (schedule_back)
> -               schedule_delayed_work(&dev_priv->psr.work,
> -                                     msecs_to_jiffies(100));
> +       schedule_delayed_work(&dev_priv->psr.work,
> +                             msecs_to_jiffies(100));
>  }
>
>  void intel_edp_psr_init(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 87e83c315c4b..1d45629a6483 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -829,7 +829,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp);
>  void intel_edp_psr_enable(struct intel_dp *intel_dp);
>  void intel_edp_psr_disable(struct intel_dp *intel_dp);
>  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_exit(struct drm_device *dev);
>  void intel_edp_psr_init(struct drm_device *dev);
>
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 2a211c64ec8d..9038e2ab73c8 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1051,7 +1051,7 @@ intel_update_plane(struct drm_plane *plane, struct
> drm_crtc *crtc,
>                 mutex_unlock(&dev->struct_mutex);
>         }
>
> -       intel_edp_psr_exit(dev, true);
> +       intel_edp_psr_exit(dev);
>
>         return 0;
>  }
> --
> 2.0.0
>
> _______________________________________________
> 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: 6926 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 05/15] drm/i915: Drop schedule_back from psr_exit

[Daniel Vetter]

On Mon, Jun 16, 2014 at 05:06:06PM -0700, Rodrigo Vivi wrote:
> There were more reasons for disabling it on Baytrail... but you are right..
> disable sequence should be better for those cases.
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Well I should mention that later on we'll split this into psr_invalidate
and psr_flush again. But meanwhile this helps to simplify the code a bit
for the transition. I'll improve the commit message a bit.
-Daniel

> 
> 
> On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch>
> wrote:
> 
> > It doesn't make sense to never again schedule the work, since by the
> > time we might want to re-enable psr the world might have changed and
> > we can do it again.
> >
> > The only exception is when we shut down the pipe, but that's an
> > entirely different thing and needs to be handled in psr_disable.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c      | 6 +++---
> >  drivers/gpu/drm/i915/intel_display.c | 4 ++--
> >  drivers/gpu/drm/i915/intel_dp.c      | 7 +++----
> >  drivers/gpu/drm/i915/intel_drv.h     | 2 +-
> >  drivers/gpu/drm/i915/intel_sprite.c  | 2 +-
> >  5 files changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 1794a041c13c..f22b4aabb945 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1395,7 +1395,7 @@ i915_gem_set_domain_ioctl(struct drm_device *dev,
> > void *data,
> >                 goto unlock;
> >         }
> >
> > -       intel_edp_psr_exit(dev, true);
> > +       intel_edp_psr_exit(dev);
> >
> >         /* Try to flush the object off the GPU without holding the lock.
> >          * We will repeat the flush holding the lock in the normal manner
> > @@ -1442,7 +1442,7 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev,
> > void *data,
> >         if (ret)
> >                 return ret;
> >
> > -       intel_edp_psr_exit(dev, true);
> > +       intel_edp_psr_exit(dev);
> >
> >         obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
> >         if (&obj->base == NULL) {
> > @@ -4240,7 +4240,7 @@ i915_gem_busy_ioctl(struct drm_device *dev, void
> > *data,
> >         if (ret)
> >                 return ret;
> >
> > -       intel_edp_psr_exit(dev, true);
> > +       intel_edp_psr_exit(dev);
> >
> >         obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
> >         if (&obj->base == NULL) {
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index c27dadebd0dc..6f2588c95248 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8823,7 +8823,7 @@ void intel_mark_fb_busy(struct drm_i915_gem_object
> > *obj,
> >         struct drm_device *dev = obj->base.dev;
> >         struct drm_crtc *crtc;
> >
> > -       intel_edp_psr_exit(dev, true);
> > +       intel_edp_psr_exit(dev);
> >
> >         if (!i915.powersave)
> >                 return;
> > @@ -9292,7 +9292,7 @@ static int intel_crtc_page_flip(struct drm_crtc
> > *crtc,
> >                 return -ENOMEM;
> >
> >         /* Exit PSR early in page flip */
> > -       intel_edp_psr_exit(dev, true);
> > +       intel_edp_psr_exit(dev);
> >
> >         work->event = event;
> >         work->crtc = crtc;
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 4ab4757fb53d..c7d625040e3d 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1899,7 +1899,7 @@ static void intel_edp_psr_inactivate(struct
> > drm_device *dev)
> >                    & ~EDP_PSR_ENABLE);
> >  }
> >
> > -void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
> > +void intel_edp_psr_exit(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >
> > @@ -1911,9 +1911,8 @@ void intel_edp_psr_exit(struct drm_device *dev, bool
> > schedule_back)
> >         if (dev_priv->psr.active)
> >                 intel_edp_psr_inactivate(dev);
> >
> > -       if (schedule_back)
> > -               schedule_delayed_work(&dev_priv->psr.work,
> > -                                     msecs_to_jiffies(100));
> > +       schedule_delayed_work(&dev_priv->psr.work,
> > +                             msecs_to_jiffies(100));
> >  }
> >
> >  void intel_edp_psr_init(struct drm_device *dev)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 87e83c315c4b..1d45629a6483 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -829,7 +829,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp);
> >  void intel_edp_psr_enable(struct intel_dp *intel_dp);
> >  void intel_edp_psr_disable(struct intel_dp *intel_dp);
> >  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_exit(struct drm_device *dev);
> >  void intel_edp_psr_init(struct drm_device *dev);
> >
> >
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index 2a211c64ec8d..9038e2ab73c8 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -1051,7 +1051,7 @@ intel_update_plane(struct drm_plane *plane, struct
> > drm_crtc *crtc,
> >                 mutex_unlock(&dev->struct_mutex);
> >         }
> >
> > -       intel_edp_psr_exit(dev, true);
> > +       intel_edp_psr_exit(dev);
> >
> >         return 0;
> >  }
> > --
> > 2.0.0
> >
> > _______________________________________________
> > 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

[PATCH 06/15] drm/i915: Add a FIXME about drrs/psr interactions

[Daniel Vetter]

Can't review this right now due to lack of DRRS code.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Vandana Kannan <vandana.kannan@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 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 c7d625040e3d..ac1cef459a7b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4121,6 +4121,11 @@ void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 		return;
 	}
 
+	/*
+	 * FIXME: This needs proper synchronization with psr state. But really
+	 * hard to tell without seeing the user of this function of this code.
+	 * Check locking and ordering once that lands.
+	 */
 	if (INTEL_INFO(dev)->gen < 8 && intel_edp_is_psr_enabled(dev)) {
 		DRM_DEBUG_KMS("DRRS is disabled as PSR is enabled\n");
 		return;
-- 
2.0.0

[PATCH 07/15] drm/i915: Track the psr dp connector in dev_priv->psr.enabled

[Daniel Vetter]

Trying to fish that one out through looping is a bit a locking
nightmare. So just set it and use it in the work struct.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f5db29428406..759f7c6d1622 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -633,10 +633,11 @@ struct i915_drrs {
 	struct intel_connector *connector;
 };
 
+struct intel_dp;
 struct i915_psr {
 	bool sink_support;
 	bool source_ok;
-	bool enabled;
+	struct intel_dp *enabled;
 	bool active;
 	struct delayed_work work;
 };
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ac1cef459a7b..10bcc052df4b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1826,7 +1826,7 @@ 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.enabled = intel_dp;
 	dev_priv->psr.active = true;
 }
 
@@ -1867,26 +1867,19 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 		       EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
 		DRM_ERROR("Timed out waiting for PSR Idle State\n");
 
-	dev_priv->psr.enabled = false;
+	dev_priv->psr.enabled = NULL;
 }
 
 static 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);
+	struct intel_dp *intel_dp = dev_priv->psr.enabled;
 
-			if (!intel_edp_psr_match_conditions(intel_dp))
-				intel_edp_psr_disable(intel_dp);
-			else
-				intel_edp_psr_do_enable(intel_dp);
-		}
+	if (!intel_edp_psr_match_conditions(intel_dp))
+		intel_edp_psr_disable(intel_dp);
+	else
+		intel_edp_psr_do_enable(intel_dp);
 }
 
 static void intel_edp_psr_inactivate(struct drm_device *dev)
-- 
2.0.0

Re: [PATCH 07/15] drm/i915: Track the psr dp connector in dev_priv->psr.enabled

[Rodrigo Vivi]

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

On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch>
wrote:

> Trying to fish that one out through looping is a bit a locking
> nightmare. So just set it and use it in the work struct.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  3 ++-
>  drivers/gpu/drm/i915/intel_dp.c | 21 +++++++--------------
>  2 files changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index f5db29428406..759f7c6d1622 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -633,10 +633,11 @@ struct i915_drrs {
>         struct intel_connector *connector;
>  };
>
> +struct intel_dp;
>  struct i915_psr {
>         bool sink_support;
>         bool source_ok;
> -       bool enabled;
> +       struct intel_dp *enabled;
>         bool active;
>         struct delayed_work work;
>  };
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index ac1cef459a7b..10bcc052df4b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1826,7 +1826,7 @@ 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.enabled = intel_dp;
>

I liked this very much...


>         dev_priv->psr.active = true;
>  }
>
> @@ -1867,26 +1867,19 @@ void intel_edp_psr_disable(struct intel_dp
> *intel_dp)
>                        EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
>                 DRM_ERROR("Timed out waiting for PSR Idle State\n");
>
> -       dev_priv->psr.enabled = false;
> +       dev_priv->psr.enabled = NULL;
>  }
>
>  static 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);
> +       struct intel_dp *intel_dp = dev_priv->psr.enabled;
>
>
but I'm afraid to give NULL to below functions...
Shouldn't we add a if(!intel_dp) return;  at least just to be on the safe
side?

-                       if (!intel_edp_psr_match_conditions(intel_dp))
> -                               intel_edp_psr_disable(intel_dp);
> -                       else
> -                               intel_edp_psr_do_enable(intel_dp);
> -               }
> +       if (!intel_edp_psr_match_conditions(intel_dp))
> +               intel_edp_psr_disable(intel_dp);
> +       else
> +               intel_edp_psr_do_enable(intel_dp);
>  }
>
>  static void intel_edp_psr_inactivate(struct drm_device *dev)
> --
> 2.0.0
>
> _______________________________________________
> 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: 4862 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 07/15] drm/i915: Track the psr dp connector in dev_priv->psr.enabled

[Daniel Vetter]

On Mon, Jun 16, 2014 at 05:10:14PM -0700, Rodrigo Vivi wrote:
> On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch>
> wrote:
> 
> > Trying to fish that one out through looping is a bit a locking
> > nightmare. So just set it and use it in the work struct.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  3 ++-
> >  drivers/gpu/drm/i915/intel_dp.c | 21 +++++++--------------
> >  2 files changed, 9 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index f5db29428406..759f7c6d1622 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -633,10 +633,11 @@ struct i915_drrs {
> >         struct intel_connector *connector;
> >  };
> >
> > +struct intel_dp;
> >  struct i915_psr {
> >         bool sink_support;
> >         bool source_ok;
> > -       bool enabled;
> > +       struct intel_dp *enabled;
> >         bool active;
> >         struct delayed_work work;
> >  };
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index ac1cef459a7b..10bcc052df4b 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1826,7 +1826,7 @@ 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.enabled = intel_dp;
> >
> 
> I liked this very much...
> 
> 
> >         dev_priv->psr.active = true;
> >  }
> >
> > @@ -1867,26 +1867,19 @@ void intel_edp_psr_disable(struct intel_dp
> > *intel_dp)
> >                        EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
> >                 DRM_ERROR("Timed out waiting for PSR Idle State\n");
> >
> > -       dev_priv->psr.enabled = false;
> > +       dev_priv->psr.enabled = NULL;
> >  }
> >
> >  static 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);
> > +       struct intel_dp *intel_dp = dev_priv->psr.enabled;
> >
> >
> but I'm afraid to give NULL to below functions...
> Shouldn't we add a if(!intel_dp) return;  at least just to be on the safe
> side?

Good catch, I'll put that into the series since it breaks bisect. With the
fixup from last night the end result is safe again.
-Daniel

> 
> -                       if (!intel_edp_psr_match_conditions(intel_dp))
> > -                               intel_edp_psr_disable(intel_dp);
> > -                       else
> > -                               intel_edp_psr_do_enable(intel_dp);
> > -               }
> > +       if (!intel_edp_psr_match_conditions(intel_dp))
> > +               intel_edp_psr_disable(intel_dp);
> > +       else
> > +               intel_edp_psr_do_enable(intel_dp);
> >  }
> >
> >  static void intel_edp_psr_inactivate(struct drm_device *dev)
> > --
> > 2.0.0
> >
> > _______________________________________________
> > 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

[PATCH 08/15] drm/i915: Don't try to disable psr harder from the work item

[Daniel Vetter]

It's disabled already except when we've raced.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 10bcc052df4b..3e0861be9c5d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1876,9 +1876,7 @@ 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;
 
-	if (!intel_edp_psr_match_conditions(intel_dp))
-		intel_edp_psr_disable(intel_dp);
-	else
+	if (intel_edp_psr_match_conditions(intel_dp))
 		intel_edp_psr_do_enable(intel_dp);
 }
 
-- 
2.0.0

Re: [PATCH 08/15] drm/i915: Don't try to disable psr harder from the work item

[Rodrigo Vivi]

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

Now I'm wondering about the psr_updated you removed and without this
disabling at this point if you alternate to fbcon you might miss most of
screen updates if not all...


On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch>
wrote:

> It's disabled already except when we've raced.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 10bcc052df4b..3e0861be9c5d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1876,9 +1876,7 @@ 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;
>
> -       if (!intel_edp_psr_match_conditions(intel_dp))
> -               intel_edp_psr_disable(intel_dp);
> -       else
> +       if (intel_edp_psr_match_conditions(intel_dp))
>                 intel_edp_psr_do_enable(intel_dp);
>  }
>
> --
> 2.0.0
>
> _______________________________________________
> 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: 2268 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 08/15] drm/i915: Don't try to disable psr harder from the work item

[Daniel Vetter]

On Mon, Jun 16, 2014 at 05:20:48PM -0700, Rodrigo Vivi wrote:
> Now I'm wondering about the psr_updated you removed and without this
> disabling at this point if you alternate to fbcon you might miss most of
> screen updates if not all...

The psr work only has a 100 ms timer, so won't catch too much really. And
we really can't call psr_match_conditions from the work again due to
locking inversion (once locking is added).

I agree that this change here breaks a few things, but the problem is that
I don't see a way to smoothly transition from your current scheme, which
mostly works due to massive amounts of invalidation as long as anything is
going on, to the new precise frontbuffer tracking. Smashing everything
into the final patch feels wrong, too.

If you're too worried about such issues I can merge more patches, but I
really have no idea how to split up the last psr patch to switch over to
invalidate/flush.
-Daniel

> 
> 
> On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch>
> wrote:
> 
> > It's disabled already except when we've raced.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 10bcc052df4b..3e0861be9c5d 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1876,9 +1876,7 @@ 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;
> >
> > -       if (!intel_edp_psr_match_conditions(intel_dp))
> > -               intel_edp_psr_disable(intel_dp);
> > -       else
> > +       if (intel_edp_psr_match_conditions(intel_dp))
> >                 intel_edp_psr_do_enable(intel_dp);
> >  }
> >
> > --
> > 2.0.0
> >
> > _______________________________________________
> > 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

[PATCH 09/15] drm/i915: Lock down psr sw/hw state tracking

[Daniel Vetter]

Make sure we track the sw side (psr.active) correctly and WARN
everywhere it might get out of sync with the hw.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 43 ++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3e0861be9c5d..923a9f6991f7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1817,8 +1817,8 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (intel_edp_is_psr_enabled(dev))
-		return;
+	WARN_ON(I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE);
+	WARN_ON(dev_priv->psr.active);
 
 	/* Enable PSR on the panel */
 	intel_edp_psr_enable_sink(intel_dp);
@@ -1859,13 +1859,19 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 	if (!dev_priv->psr.enabled)
 		return;
 
-	I915_WRITE(EDP_PSR_CTL(dev),
-		   I915_READ(EDP_PSR_CTL(dev)) & ~EDP_PSR_ENABLE);
+	if (dev_priv->psr.active) {
+		I915_WRITE(EDP_PSR_CTL(dev),
+			   I915_READ(EDP_PSR_CTL(dev)) & ~EDP_PSR_ENABLE);
+
+		/* Wait till PSR is idle */
+		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");
 
-	/* Wait till PSR is idle */
-	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.active = false;
+	} else {
+		WARN_ON(I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE);
+	}
 
 	dev_priv->psr.enabled = NULL;
 }
@@ -1880,16 +1886,6 @@ static void intel_edp_psr_work(struct work_struct *work)
 		intel_edp_psr_do_enable(intel_dp);
 }
 
-static void intel_edp_psr_inactivate(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	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)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1899,8 +1895,15 @@ void intel_edp_psr_exit(struct drm_device *dev)
 
 	cancel_delayed_work_sync(&dev_priv->psr.work);
 
-	if (dev_priv->psr.active)
-		intel_edp_psr_inactivate(dev);
+	if (dev_priv->psr.active) {
+		u32 val = I915_READ(EDP_PSR_CTL(dev));
+
+		WARN_ON(val & EDP_PSR_ENABLE);
+
+		I915_WRITE(EDP_PSR_CTL(dev), val & ~EDP_PSR_ENABLE);
+
+		dev_priv->psr.active = false;
+	}
 
 	schedule_delayed_work(&dev_priv->psr.work,
 			      msecs_to_jiffies(100));
-- 
2.0.0

[PATCH 10/15] drm/i915: More checks for psr.enabled

[Daniel Vetter]

We need to make sure that no one else is using this in the
enable function and also that the work item hasn't raced
with the disabled function.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 923a9f6991f7..503809e4e6f3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1844,6 +1844,11 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 		return;
 	}
 
+	if (dev_priv->psr.enabled) {
+		DRM_DEBUG_KMS("PSR already in use\n");
+		return;
+	}
+
 	/* Setup PSR once */
 	intel_edp_psr_setup(intel_dp);
 
@@ -1882,6 +1887,9 @@ 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;
 
+	if (!intel_dp)
+		return;
+
 	if (intel_edp_psr_match_conditions(intel_dp))
 		intel_edp_psr_do_enable(intel_dp);
 }
-- 
2.0.0

[PATCH 11/15] drm/i915: Add locking to psr code

[Daniel Vetter]

It's not really optional to have locking ...

The ugly part is how much locking the psr work needs since it has to
recheck everything. Which is way too much. But we need to ditch the
psr work in it's current form anyway and implement proper frontbuffer
tracking.

The other nasty bit that had to go was the delayed work cancle in
psr_exit. Which means a bunch of races just became a bit more likely,
but mea culpa.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a8b81407338a..0e0e6b6bffd1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1973,7 +1973,7 @@ 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, "Enabled: %s\n", yesno(!!dev_priv->psr.enabled));
 	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
 
 	enabled = HAS_PSR(dev) &&
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 759f7c6d1622..3d3f3b1ef9ae 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -635,6 +635,7 @@ struct i915_drrs {
 
 struct intel_dp;
 struct i915_psr {
+	struct mutex lock;
 	bool sink_support;
 	bool source_ok;
 	struct intel_dp *enabled;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 503809e4e6f3..13c3ec7ec451 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1749,6 +1749,11 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 	struct drm_i915_gem_object *obj = to_intel_framebuffer(crtc->primary->fb)->obj;
 	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 
+	lockdep_assert_held(&dev_priv->psr.lock);
+	lockdep_assert_held(&dev->struct_mutex);
+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+
 	dev_priv->psr.source_ok = false;
 
 	if (!HAS_PSR(dev)) {
@@ -1819,6 +1824,7 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 
 	WARN_ON(I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE);
 	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);
@@ -1833,6 +1839,7 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 void intel_edp_psr_enable(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 (!HAS_PSR(dev)) {
 		DRM_DEBUG_KMS("PSR not supported on this platform\n");
@@ -1844,8 +1851,10 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 		return;
 	}
 
+	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;
 	}
 
@@ -1854,6 +1863,7 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 
 	if (intel_edp_psr_match_conditions(intel_dp))
 		intel_edp_psr_do_enable(intel_dp);
+	mutex_unlock(&dev_priv->psr.lock);
 }
 
 void intel_edp_psr_disable(struct intel_dp *intel_dp)
@@ -1861,8 +1871,11 @@ 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 (!dev_priv->psr.enabled)
+	mutex_lock(&dev_priv->psr.lock);
+	if (!dev_priv->psr.enabled) {
+		mutex_unlock(&dev_priv->psr.lock);
 		return;
+	}
 
 	if (dev_priv->psr.active) {
 		I915_WRITE(EDP_PSR_CTL(dev),
@@ -1879,19 +1892,30 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 	}
 
 	dev_priv->psr.enabled = NULL;
+	mutex_unlock(&dev_priv->psr.lock);
 }
 
 static 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_dp *intel_dp = dev_priv->psr.enabled;
 
+	drm_modeset_lock_all(dev);
+	mutex_lock(&dev->struct_mutex);
+	mutex_lock(&dev_priv->psr.lock);
+	intel_dp = dev_priv->psr.enabled;
+
 	if (!intel_dp)
-		return;
+		goto unlock;
 
 	if (intel_edp_psr_match_conditions(intel_dp))
 		intel_edp_psr_do_enable(intel_dp);
+unlock:
+	mutex_unlock(&dev_priv->psr.lock);
+	mutex_unlock(&dev->struct_mutex);
+	drm_modeset_unlock_all(dev);
 }
 
 void intel_edp_psr_exit(struct drm_device *dev)
@@ -1901,8 +1925,7 @@ void intel_edp_psr_exit(struct drm_device *dev)
 	if (!HAS_PSR(dev))
 		return;
 
-	cancel_delayed_work_sync(&dev_priv->psr.work);
-
+	mutex_lock(&dev_priv->psr.lock);
 	if (dev_priv->psr.active) {
 		u32 val = I915_READ(EDP_PSR_CTL(dev));
 
@@ -1915,6 +1938,7 @@ void intel_edp_psr_exit(struct drm_device *dev)
 
 	schedule_delayed_work(&dev_priv->psr.work,
 			      msecs_to_jiffies(100));
+	mutex_unlock(&dev_priv->psr.lock);
 }
 
 void intel_edp_psr_init(struct drm_device *dev)
@@ -1925,6 +1949,7 @@ void intel_edp_psr_init(struct drm_device *dev)
 		return;
 
 	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_edp_psr_work);
+	mutex_init(&dev_priv->psr.lock);
 }
 
 static void intel_disable_dp(struct intel_encoder *encoder)
-- 
2.0.0

[PATCH 12/15] drm/i915: Introduce accurate frontbuffer tracking

[Daniel Vetter]

So from just a quick look we seem to have enough information to
accurately figure out whether a given gem bo is used as a frontbuffer
and where exactly: We have obj->pin_count as a first check with no
false negatives and only negligible false positives. And then we can
just walk the modeset objects and figure out where exactly a buffer is
used as scanout.

Except that we can't due to locking order: If we already hold
dev->struct_mutex we can't acquire any modeset locks, so could
potential chase freed pointers and other evil stuff.

So we need something else. For that introduce a new set of bits
obj->frontbuffer_bits to track where a buffer object is used. That we
can then chase without grabbing any modeset locks.

Of course the consumers of this (DRRS, PSR, FBC, ...) still need to be
able to do their magic both when called from modeset and from gem
code. But that can be easily achieved by adding locks for these
specific subsystems which always nest within either kms or gem
locking.

This patch just adds the relevant update code to all places.

Note that if we ever support multi-planar scanout targets then we need
one frontbuffer tracking bit per attachment point that we expose to
userspace.

v2:
- Fix more oopsen. Oops.
- WARN if we leak obj->frontbuffer_bits when freeing a gem buffer. Fix
  the bugs this brought to light.
- s/update_frontbuffer_bits/update_fb_bits/. More consistent with the
  fb tracking functions (fb for gem object, frontbuffer for raw bits).
  And the function name was way too long.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h      | 24 ++++++++++++
 drivers/gpu/drm/i915/i915_gem.c      | 19 ++++++++++
 drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_overlay.c | 11 ++++++
 drivers/gpu/drm/i915/intel_sprite.c  |  7 ++++
 5 files changed, 116 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3d3f3b1ef9ae..870ca04e0888 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1592,6 +1592,24 @@ struct drm_i915_gem_object_ops {
 	void (*release)(struct drm_i915_gem_object *);
 };
 
+/*
+ * Frontbuffer tracking bits. Set in obj->frontbuffer_bits while a gem bo is
+ * considered to be the frontbuffer for the given plane interface-vise. This
+ * doesn't mean that the hw necessarily already scans it out, but that any
+ * rendering (by the cpu or gpu) will land in the frontbuffer eventually.
+ *
+ * We have one bit per pipe and per scanout plane type.
+ */
+#define INTEL_FRONTBUFFER_BITS_PER_PIPE 4
+#define INTEL_FRONTBUFFER_PRIMARY(pipe) \
+	(1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
+#define INTEL_FRONTBUFFER_CURSOR(pipe) \
+	(1 << (1 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
+#define INTEL_FRONTBUFFER_SPRITE(pipe) \
+	(1 << (2 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
+#define INTEL_FRONTBUFFER_OVERLAY(pipe) \
+	(1 << (3 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
+
 struct drm_i915_gem_object {
 	struct drm_gem_object base;
 
@@ -1673,6 +1691,8 @@ struct drm_i915_gem_object {
 	unsigned int has_global_gtt_mapping:1;
 	unsigned int has_dma_mapping:1;
 
+	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS_PER_PIPE;
+
 	struct sg_table *pages;
 	int pages_pin_count;
 
@@ -1719,6 +1739,10 @@ struct drm_i915_gem_object {
 };
 #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
 
+void i915_gem_update_fb_bits(struct drm_i915_gem_object *old,
+			     struct drm_i915_gem_object *new,
+			     unsigned frontbuffer_bits);
+
 /**
  * Request queue structure.
  *
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f22b4aabb945..9722991a0d19 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4456,6 +4456,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
 	if (obj->stolen)
 		i915_gem_object_unpin_pages(obj);
 
+	WARN_ON(obj->frontbuffer_bits);
+
 	if (WARN_ON(obj->pages_pin_count))
 		obj->pages_pin_count = 0;
 	if (discard_backing_storage(obj))
@@ -5000,6 +5002,23 @@ int i915_gem_open(struct drm_device *dev, struct drm_file *file)
 	return ret;
 }
 
+void i915_gem_update_fb_bits(struct drm_i915_gem_object *old,
+			     struct drm_i915_gem_object *new,
+			     unsigned frontbuffer_bits)
+{
+	if (old) {
+		WARN_ON(!mutex_is_locked(&old->base.dev->struct_mutex));
+		WARN_ON(!(old->frontbuffer_bits & frontbuffer_bits));
+		old->frontbuffer_bits &= ~frontbuffer_bits;
+	}
+
+	if (new) {
+		WARN_ON(!mutex_is_locked(&new->base.dev->struct_mutex));
+		WARN_ON(new->frontbuffer_bits & frontbuffer_bits);
+		new->frontbuffer_bits |= frontbuffer_bits;
+	}
+}
+
 static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
 {
 	if (!mutex_is_locked(mutex))
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6f2588c95248..64145aeefe28 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2351,6 +2351,7 @@ static bool intel_alloc_plane_obj(struct intel_crtc *crtc,
 		goto out_unref_obj;
 	}
 
+	obj->frontbuffer_bits = INTEL_FRONTBUFFER_PRIMARY(crtc->pipe);
 	mutex_unlock(&dev->struct_mutex);
 
 	DRM_DEBUG_KMS("plane fb obj %p\n", obj);
@@ -2396,6 +2397,7 @@ static void intel_find_plane_obj(struct intel_crtc *intel_crtc,
 		if (i915_gem_obj_ggtt_offset(fb->obj) == plane_config->base) {
 			drm_framebuffer_reference(c->primary->fb);
 			intel_crtc->base.primary->fb = c->primary->fb;
+			fb->obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
 			break;
 		}
 	}
@@ -2684,7 +2686,9 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *old_fb;
+	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
 	int ret;
 
 	if (intel_crtc_has_pending_flip(crtc)) {
@@ -2705,10 +2709,12 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		return -EINVAL;
 	}
 
+	old_fb = crtc->primary->fb;
+
 	mutex_lock(&dev->struct_mutex);
-	ret = intel_pin_and_fence_fb_obj(dev,
-					 to_intel_framebuffer(fb)->obj,
-					 NULL);
+	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+	i915_gem_update_fb_bits(to_intel_framebuffer(old_fb)->obj, obj,
+				INTEL_FRONTBUFFER_PRIMARY(pipe));
 	mutex_unlock(&dev->struct_mutex);
 	if (ret != 0) {
 		DRM_ERROR("pin & fence failed\n");
@@ -2748,7 +2754,6 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 
 	dev_priv->display.update_primary_plane(crtc, fb, x, y);
 
-	old_fb = crtc->primary->fb;
 	crtc->primary->fb = fb;
 	crtc->x = x;
 	crtc->y = y;
@@ -4922,6 +4927,8 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_connector *connector;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *old_obj;
+	enum pipe pipe = to_intel_crtc(crtc)->pipe;
 
 	/* crtc should still be enabled when we disable it. */
 	WARN_ON(!crtc->enabled);
@@ -4935,8 +4942,11 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	assert_pipe_disabled(dev->dev_private, to_intel_crtc(crtc)->pipe);
 
 	if (crtc->primary->fb) {
+		old_obj = to_intel_framebuffer(crtc->primary->fb)->obj;
 		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(to_intel_framebuffer(crtc->primary->fb)->obj);
+		intel_unpin_fb_obj(old_obj);
+		i915_gem_update_fb_bits(old_obj, NULL,
+					INTEL_FRONTBUFFER_PRIMARY(pipe));
 		mutex_unlock(&dev->struct_mutex);
 		crtc->primary->fb = NULL;
 	}
@@ -8103,6 +8113,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
 	unsigned old_width;
 	uint32_t addr;
 	int ret;
@@ -8182,6 +8193,8 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
 	}
 
+	i915_gem_update_fb_bits(intel_crtc->cursor_bo, obj,
+				INTEL_FRONTBUFFER_CURSOR(pipe));
 	mutex_unlock(&dev->struct_mutex);
 
 	old_width = intel_crtc->cursor_width;
@@ -9266,6 +9279,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	struct drm_framebuffer *old_fb = crtc->primary->fb;
 	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
 	struct intel_unpin_work *work;
 	struct intel_engine_cs *ring;
 	unsigned long flags;
@@ -9360,6 +9374,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (ret)
 		goto cleanup_unpin;
 
+	i915_gem_update_fb_bits(work->old_fb_obj, obj,
+				INTEL_FRONTBUFFER_PRIMARY(pipe));
+
 	intel_disable_fbc(dev);
 	intel_mark_fb_busy(obj, NULL);
 	mutex_unlock(&dev->struct_mutex);
@@ -10426,10 +10443,14 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	 */
 	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
 		struct drm_framebuffer *old_fb;
+		struct drm_i915_gem_object *old_obj = NULL;
+		struct drm_i915_gem_object *obj =
+			to_intel_framebuffer(fb)->obj;
+		enum pipe pipe = intel_crtc->pipe;
 
 		mutex_lock(&dev->struct_mutex);
 		ret = intel_pin_and_fence_fb_obj(dev,
-						 to_intel_framebuffer(fb)->obj,
+						 obj,
 						 NULL);
 		if (ret != 0) {
 			DRM_ERROR("pin & fence failed\n");
@@ -10437,8 +10458,12 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 			goto done;
 		}
 		old_fb = crtc->primary->fb;
-		if (old_fb)
-			intel_unpin_fb_obj(to_intel_framebuffer(old_fb)->obj);
+		if (old_fb) {
+			old_obj = to_intel_framebuffer(old_fb)->obj;
+			intel_unpin_fb_obj(old_obj);
+		}
+		i915_gem_update_fb_bits(old_obj, obj,
+					INTEL_FRONTBUFFER_PRIMARY(pipe));
 		mutex_unlock(&dev->struct_mutex);
 
 		crtc->primary->fb = fb;
@@ -11030,6 +11055,7 @@ intel_primary_plane_disable(struct drm_plane *plane)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_crtc *intel_crtc;
+	enum pipe pipe;
 
 	if (!plane->fb)
 		return 0;
@@ -11037,6 +11063,7 @@ intel_primary_plane_disable(struct drm_plane *plane)
 	BUG_ON(!plane->crtc);
 
 	intel_crtc = to_intel_crtc(plane->crtc);
+	pipe = intel_crtc->pipe;
 
 	/*
 	 * Even though we checked plane->fb above, it's still possible that
@@ -11053,8 +11080,9 @@ intel_primary_plane_disable(struct drm_plane *plane)
 	intel_crtc_wait_for_pending_flips(plane->crtc);
 	intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
 				       intel_plane->pipe);
-
 disable_unpin:
+	i915_gem_update_fb_bits(to_intel_framebuffer(plane->fb)->obj, NULL,
+				INTEL_FRONTBUFFER_PRIMARY(pipe));
 	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
 	plane->fb = NULL;
 
@@ -11071,7 +11099,9 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_i915_gem_object *obj, *old_obj = NULL;
 	struct drm_rect dest = {
 		/* integer pixels */
 		.x1 = crtc_x,
@@ -11103,6 +11133,10 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
+	if (plane->fb)
+		old_obj = to_intel_framebuffer(plane->fb)->obj;
+	obj = to_intel_framebuffer(fb)->obj;
+
 	/*
 	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
 	 * updating the fb pointer, and returning without touching the
@@ -11110,17 +11144,20 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * turn on the display with all planes setup as desired.
 	 */
 	if (!crtc->enabled) {
+		enum pipe pipe = intel_crtc->pipe;
+
 		/*
 		 * If we already called setplane while the crtc was disabled,
 		 * we may have an fb pinned; unpin it.
 		 */
 		if (plane->fb)
-			intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+			intel_unpin_fb_obj(old_obj);
+
+		i915_gem_update_fb_bits(old_obj, obj,
+					INTEL_FRONTBUFFER_PRIMARY(pipe));
 
 		/* Pin and return without programming hardware */
-		return intel_pin_and_fence_fb_obj(dev,
-						  to_intel_framebuffer(fb)->obj,
-						  NULL);
+		return intel_pin_and_fence_fb_obj(dev, obj, NULL);
 	}
 
 	intel_crtc_wait_for_pending_flips(crtc);
@@ -11137,13 +11174,14 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 		 * fail.
 		 */
 		if (plane->fb != fb) {
-			ret = intel_pin_and_fence_fb_obj(dev,
-							 to_intel_framebuffer(fb)->obj,
-							 NULL);
+			ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 			if (ret)
 				return ret;
 		}
 
+		i915_gem_update_fb_bits(old_obj, obj,
+					INTEL_FRONTBUFFER_PRIMARY(pipe));
+
 		if (intel_crtc->primary_enabled)
 			intel_disable_primary_hw_plane(dev_priv,
 						       intel_plane->plane,
@@ -11152,7 +11190,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 		if (plane->fb != fb)
 			if (plane->fb)
-				intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+				intel_unpin_fb_obj(old_obj);
 
 		return 0;
 	}
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index daa118978eec..6aeda6a489a0 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -390,6 +390,7 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
 	struct drm_device *dev = overlay->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *ring = &dev_priv->ring[RCS];
+	enum pipe pipe;
 	int ret;
 
 	/* Only wait if there is actually an old frame to release to
@@ -398,6 +399,8 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
 	if (!overlay->old_vid_bo)
 		return 0;
 
+	pipe = overlay->crtc->pipe;
+
 	if (I915_READ(ISR) & I915_OVERLAY_PLANE_FLIP_PENDING_INTERRUPT) {
 		/* synchronous slowpath */
 		ret = intel_ring_begin(ring, 2);
@@ -415,6 +418,10 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
 	}
 
 	intel_overlay_release_old_vid_tail(overlay);
+
+
+	i915_gem_update_fb_bits(overlay->old_vid_bo, NULL,
+				INTEL_FRONTBUFFER_OVERLAY(pipe));
 	return 0;
 }
 
@@ -686,6 +693,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	bool scale_changed = false;
 	struct drm_device *dev = overlay->dev;
 	u32 swidth, swidthsw, sheight, ostride;
+	enum pipe pipe = overlay->crtc->pipe;
 
 	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
 	BUG_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
@@ -776,6 +784,9 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret)
 		goto out_unpin;
 
+	i915_gem_update_fb_bits(overlay->vid_bo, new_bo,
+				INTEL_FRONTBUFFER_OVERLAY(pipe));
+
 	overlay->old_vid_bo = overlay->vid_bo;
 	overlay->vid_bo = new_bo;
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 9038e2ab73c8..3648594aeb7e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -811,6 +811,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	enum pipe pipe = intel_crtc->pipe;
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct drm_i915_gem_object *old_obj = intel_plane->obj;
@@ -998,6 +999,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 */
 	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 
+	i915_gem_update_fb_bits(old_obj, obj,
+				INTEL_FRONTBUFFER_SPRITE(pipe));
 	mutex_unlock(&dev->struct_mutex);
 
 	if (ret)
@@ -1062,6 +1065,7 @@ intel_disable_plane(struct drm_plane *plane)
 	struct drm_device *dev = plane->dev;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_crtc *intel_crtc;
+	enum pipe pipe;
 
 	if (!plane->fb)
 		return 0;
@@ -1070,6 +1074,7 @@ intel_disable_plane(struct drm_plane *plane)
 		return -EINVAL;
 
 	intel_crtc = to_intel_crtc(plane->crtc);
+	pipe = intel_crtc->pipe;
 
 	if (intel_crtc->active) {
 		bool primary_was_enabled = intel_crtc->primary_enabled;
@@ -1088,6 +1093,8 @@ intel_disable_plane(struct drm_plane *plane)
 
 		mutex_lock(&dev->struct_mutex);
 		intel_unpin_fb_obj(intel_plane->obj);
+		i915_gem_update_fb_bits(intel_plane->obj, NULL,
+					INTEL_FRONTBUFFER_SPRITE(pipe));
 		mutex_unlock(&dev->struct_mutex);
 
 		intel_plane->obj = NULL;
-- 
2.0.0

[PATCH 13/15] drm/i915: Use new frontbuffer bits to increase pll clock

[Daniel Vetter]

The downclocking checks a few more things, so not that simple to
convert. Also, this should get unified with the drrs handling and also
use the locking of that. Otoh the drrs locking is about as hapzardous
as no locking, at least on first sight.

For easier conversion ditch the upclocking on unload - we'll turn off
everything anyway.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++----------------------
 2 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 870ca04e0888..4c3787699cdf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1609,6 +1609,8 @@ struct drm_i915_gem_object_ops {
 	(1 << (2 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
 #define INTEL_FRONTBUFFER_OVERLAY(pipe) \
 	(1 << (3 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
+#define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
+	(0xf << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
 
 struct drm_i915_gem_object {
 	struct drm_gem_object base;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 64145aeefe28..b8359f4a6dc4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -76,7 +76,8 @@ static const uint32_t intel_cursor_formats[] = {
 #define DIV_ROUND_CLOSEST_ULL(ll, d)	\
 ({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
 
-static void intel_increase_pllclock(struct drm_crtc *crtc);
+static void intel_increase_pllclock(struct drm_device *dev,
+				    enum pipe pipe);
 static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
 
 static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
@@ -2585,7 +2586,7 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 
 	if (dev_priv->display.disable_fbc)
 		dev_priv->display.disable_fbc(dev);
-	intel_increase_pllclock(crtc);
+	intel_increase_pllclock(dev, to_intel_crtc(crtc)->pipe);
 
 	dev_priv->display.update_primary_plane(crtc, fb, x, y);
 
@@ -8723,12 +8724,10 @@ struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
 	return mode;
 }
 
-static void intel_increase_pllclock(struct drm_crtc *crtc)
+static void intel_increase_pllclock(struct drm_device *dev,
+				    enum pipe pipe)
 {
-	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int pipe = intel_crtc->pipe;
 	int dpll_reg = DPLL(pipe);
 	int dpll;
 
@@ -8834,21 +8833,19 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 			struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = obj->base.dev;
-	struct drm_crtc *crtc;
+	enum pipe pipe;
 
 	intel_edp_psr_exit(dev);
 
 	if (!i915.powersave)
 		return;
 
-	for_each_crtc(dev, crtc) {
-		if (!crtc->primary->fb)
-			continue;
-
-		if (to_intel_framebuffer(crtc->primary->fb)->obj != obj)
+	for_each_pipe(pipe) {
+		if (!(obj->frontbuffer_bits &
+		      INTEL_FRONTBUFFER_ALL_MASK(pipe)))
 			continue;
 
-		intel_increase_pllclock(crtc);
+		intel_increase_pllclock(dev, pipe);
 		if (ring && intel_fbc_enabled(dev))
 			ring->fbc_dirty = true;
 	}
@@ -12658,7 +12655,6 @@ void intel_connector_unregister(struct intel_connector *intel_connector)
 void intel_modeset_cleanup(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc;
 	struct drm_connector *connector;
 
 	/*
@@ -12678,14 +12674,6 @@ void intel_modeset_cleanup(struct drm_device *dev)
 
 	intel_unregister_dsm_handler();
 
-	for_each_crtc(dev, crtc) {
-		/* Skip inactive CRTCs */
-		if (!crtc->primary->fb)
-			continue;
-
-		intel_increase_pllclock(crtc);
-	}
-
 	intel_disable_fbc(dev);
 
 	intel_disable_gt_powersave(dev);
-- 
2.0.0

[PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

So these are the guts of the new beast. This tracks when a frontbuffer
gets invalidated (due to frontbuffer rendering) and hence should be
constantly scaned out, and when it's flushed again and can be
compressed/one-shot-upload.

Rules for flushing are simple: The frontbuffer needs one more full
upload starting from the next vblank. Which means that the flushing
can _only_ be called once the frontbuffer update has been latched.

But this poses a problem for pageflips: We can't just delay the
flushing until the pageflip is latched, since that would pose the risk
that we override frontbuffer rendering that has been scheduled
in-between the pageflip ioctl and the actual latching.

To handle this track asynchronous invalidations (and also pageflip)
state per-ring and delay any in-between flushing until the rendering
has completed. And also cancel any delayed flushing if we get a new
invalidation request (whether delayed or not).

Also call intel_mark_fb_busy in both cases in all cases to make sure
that we keep the screen at the highest refresh rate both on flips,
synchronous plane updates and for frontbuffer rendering.

v2: Lots of improvements

Suggestions from Chris:
- Move invalidate/flush in flush_*_domain and set_to_*_domain.
- Drop the flush in busy_ioctl since it's redundant. Was a leftover
  from an earlier concept to track flips/delayed flushes.
- Don't forget about the initial modeset enable/final disable.
  Suggested by Chris.

Track flips accurately, too. Since flips complete independently of
rendering we need to track pending flips in a separate mask. Again if
an invalidate happens we need to cancel the evenutal flush to avoid
races.

v3:
Provide correct header declarations for flip functions. Currently not
needed outside of intel_display.c, but part of the proper interface.

v4: Add proper domain management to fbcon so that the fbcon buffer is
also tracked correctly.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h            |  14 +++
 drivers/gpu/drm/i915/i915_gem.c            |  19 ++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   2 +-
 drivers/gpu/drm/i915/intel_display.c       | 178 +++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h           |  10 +-
 drivers/gpu/drm/i915/intel_fbdev.c         |  18 ++-
 drivers/gpu/drm/i915/intel_overlay.c       |   3 +
 drivers/gpu/drm/i915/intel_sprite.c        |   4 +-
 8 files changed, 225 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4c3787699cdf..30c2ae3cbab7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1332,6 +1332,17 @@ struct intel_pipe_crc {
 	wait_queue_head_t wq;
 };
 
+struct i915_frontbuffer_tracking {
+	struct mutex lock;
+
+	/*
+	 * Tracking bits for delayed frontbuffer flushing du to gpu activity or
+	 * scheduled flips.
+	 */
+	unsigned busy_bits;
+	unsigned flip_bits;
+};
+
 struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
@@ -1475,6 +1486,9 @@ struct drm_i915_private {
 	bool lvds_downclock_avail;
 	/* indicates the reduced downclock for LVDS*/
 	int lvds_downclock;
+
+	struct i915_frontbuffer_tracking fb_tracking;
+
 	u16 orig_clock;
 
 	bool mchbar_need_disable;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9722991a0d19..2ccdd9dcd253 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1395,8 +1395,6 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 		goto unlock;
 	}
 
-	intel_edp_psr_exit(dev);
-
 	/* 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.
@@ -1442,8 +1440,6 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	intel_edp_psr_exit(dev);
-
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
 		ret = -ENOENT;
@@ -2227,6 +2223,9 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 			list_move_tail(&vma->mm_list, &vm->inactive_list);
 	}
 
+	if (obj->frontbuffer_bits)
+		intel_fb_flush(obj, true);
+
 	list_del_init(&obj->ring_list);
 	obj->ring = NULL;
 
@@ -3556,6 +3555,8 @@ i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
 	old_write_domain = obj->base.write_domain;
 	obj->base.write_domain = 0;
 
+	intel_fb_flush(obj, false);
+
 	trace_i915_gem_object_change_domain(obj,
 					    obj->base.read_domains,
 					    old_write_domain);
@@ -3577,6 +3578,8 @@ i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj,
 	old_write_domain = obj->base.write_domain;
 	obj->base.write_domain = 0;
 
+	intel_fb_flush(obj, false);
+
 	trace_i915_gem_object_change_domain(obj,
 					    obj->base.read_domains,
 					    old_write_domain);
@@ -3630,6 +3633,8 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
 		obj->dirty = 1;
 	}
 
+	intel_fb_invalidate(obj, NULL);
+
 	trace_i915_gem_object_change_domain(obj,
 					    old_read_domains,
 					    old_write_domain);
@@ -3966,6 +3971,8 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
 		obj->base.write_domain = I915_GEM_DOMAIN_CPU;
 	}
 
+	intel_fb_invalidate(obj, NULL);
+
 	trace_i915_gem_object_change_domain(obj,
 					    old_read_domains,
 					    old_write_domain);
@@ -4240,8 +4247,6 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	intel_edp_psr_exit(dev);
-
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
 		ret = -ENOENT;
@@ -4941,6 +4946,8 @@ i915_gem_load(struct drm_device *dev)
 
 	dev_priv->mm.oom_notifier.notifier_call = i915_gem_shrinker_oom;
 	register_oom_notifier(&dev_priv->mm.oom_notifier);
+
+	mutex_init(&dev_priv->fb_tracking.lock);
 }
 
 void i915_gem_release(struct drm_device *dev, struct drm_file *file)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 93d7f7246588..2d06aad4a954 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -978,7 +978,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
 			/* check for potential scanout */
 			if (i915_gem_obj_ggtt_bound(obj) &&
 			    i915_gem_obj_to_ggtt(obj)->pin_count)
-				intel_mark_fb_busy(obj, ring);
+				intel_fb_invalidate(obj, ring);
 
 			/* update for the implicit flush after a batch */
 			obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b8359f4a6dc4..dfdbf2a02844 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2755,6 +2755,8 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 
 	dev_priv->display.update_primary_plane(crtc, fb, x, y);
 
+	intel_frontbuffer_flush(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
+
 	crtc->primary->fb = fb;
 	crtc->x = x;
 	crtc->y = y;
@@ -3949,6 +3951,8 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
+
+	intel_frontbuffer_flush(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
 }
 
 static void intel_crtc_disable_planes(struct drm_crtc *crtc)
@@ -3971,6 +3975,8 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
 	intel_disable_planes(crtc);
 	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
 
+	intel_frontbuffer_flush(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
+
 	drm_vblank_off(dev, pipe);
 }
 
@@ -8211,6 +8217,8 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
 	}
 
+	intel_frontbuffer_flush(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
+
 	return 0;
 fail_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
@@ -8829,20 +8837,26 @@ out:
 }
 
 
-void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
-			struct intel_engine_cs *ring)
+/**
+ * intel_mark_fb_busy - mark given planes as busy
+ * @dev: DRM device
+ * @frontbuffer_bits: bits for the affected planes
+ * @ring: optional ring for asynchronous commands
+ *
+ * This function gets called every time the screen contents change. It can be
+ * used to keep e.g. the update rate at the nominal refresh rate with DRRS.
+ */
+static void intel_mark_fb_busy(struct drm_device *dev,
+			       unsigned frontbuffer_bits,
+			       struct intel_engine_cs *ring)
 {
-	struct drm_device *dev = obj->base.dev;
 	enum pipe pipe;
 
-	intel_edp_psr_exit(dev);
-
 	if (!i915.powersave)
 		return;
 
 	for_each_pipe(pipe) {
-		if (!(obj->frontbuffer_bits &
-		      INTEL_FRONTBUFFER_ALL_MASK(pipe)))
+		if (!(frontbuffer_bits & INTEL_FRONTBUFFER_ALL_MASK(pipe)))
 			continue;
 
 		intel_increase_pllclock(dev, pipe);
@@ -8851,6 +8865,148 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 	}
 }
 
+/**
+ * intel_fb_invalidate - invalidate frontbuffer
+ * @obj: GEM object to invalidate
+ * @ring: set for asynchronous rendering
+ *
+ * This function gets called every time rendering on the given object starts and
+ * frontbuffer caching (fbc, low refresh rate for DRRS, panel self refresh) must
+ * be invalidated. If @ring is non-NULL any subsequent invalidation will be delayed
+ * until the rendering completes or a flip on this frontbuffer plane is
+ * scheduled.
+ */
+void intel_fb_invalidate(struct drm_i915_gem_object *obj,
+			 struct intel_engine_cs *ring)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	if (!obj->frontbuffer_bits)
+		return;
+
+	if (ring) {
+		mutex_lock(&dev_priv->fb_tracking.lock);
+		dev_priv->fb_tracking.busy_bits
+			|= obj->frontbuffer_bits;
+		dev_priv->fb_tracking.flip_bits
+			&= ~obj->frontbuffer_bits;
+		mutex_unlock(&dev_priv->fb_tracking.lock);
+	}
+
+	intel_mark_fb_busy(dev, obj->frontbuffer_bits, ring);
+
+	intel_edp_psr_exit(dev);
+}
+
+/**
+ * intel_frontbuffer_flush - flush frontbuffer
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called every time rendering on the given planes has
+ * completed and frontbuffer caching can be started again. Flushes will get
+ * delayed if they're blocked by some oustanding asynchronous rendering.
+ *
+ * Can be called without any locks held.
+ */
+void intel_frontbuffer_flush(struct drm_device *dev,
+			     unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* Delay flushing when rings are still busy.*/
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	frontbuffer_bits &= ~dev_priv->fb_tracking.busy_bits;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+
+	intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
+
+	intel_edp_psr_exit(dev);
+}
+
+/**
+ * intel_fb_flush - flush frontbuffer
+ * @obj: GEM object to flush
+ * @retire: set when retiring asynchronous rendering
+ *
+ * This function gets called every time rendering on the given object has
+ * completed and frontbuffer caching can be started again. If @retire is true
+ * then any delayed flushes will be unblocked.
+ */
+void intel_fb_flush(struct drm_i915_gem_object *obj,
+		    bool retire)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	if (!obj->frontbuffer_bits)
+		return;
+
+	if (retire) {
+		mutex_lock(&dev_priv->fb_tracking.lock);
+		dev_priv->fb_tracking.busy_bits
+			&= ~obj->frontbuffer_bits;
+		mutex_unlock(&dev_priv->fb_tracking.lock);
+	}
+
+	intel_frontbuffer_flush(dev, obj->frontbuffer_bits);
+}
+
+/**
+ * intel_fb_flip_prepare - flip frontbuffer
+ * @obj: GEM object to flush
+ *
+ * This function gets called after scheduling a flip on @obj. The actual
+ * frontbuffer flushing will be delayed until completion is signalled with
+ * intel_frontbuffer_flip_complete. If an invalidate happens in between this
+ * flush will be cancelled.
+ *
+ * Note that the frontbuffer tracking bits in obj->frontbuffer_bits must have
+ * been updated already.
+ */
+void intel_fb_flip_prepare(struct drm_i915_gem_object *obj)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+	WARN_ON(!obj->frontbuffer_bits);
+
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	dev_priv->fb_tracking.flip_bits
+		|= obj->frontbuffer_bits;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+}
+
+/**
+ * intel_frontbuffer_flip_complete - complete frontbuffer flip
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called after the flip has been latched and will complete
+ * on the next vblank. It will execute the fush if it hasn't been cancalled yet.
+ *
+ * Can be called without any locks held.
+ */
+void intel_frontbuffer_flip_complete(struct drm_device *dev,
+				     unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	/* Mask any cancelled flips. */
+	frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
+	dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+
+	intel_frontbuffer_flush(dev, frontbuffer_bits);
+}
+
 static void intel_crtc_destroy(struct drm_crtc *crtc)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -8878,6 +9034,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	struct intel_unpin_work *work =
 		container_of(__work, struct intel_unpin_work, work);
 	struct drm_device *dev = work->crtc->dev;
+	enum pipe pipe = to_intel_crtc(work->crtc)->pipe;
 
 	mutex_lock(&dev->struct_mutex);
 	intel_unpin_fb_obj(work->old_fb_obj);
@@ -8887,6 +9044,8 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	intel_update_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
 
+	intel_frontbuffer_flip_complete(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
+
 	BUG_ON(atomic_read(&to_intel_crtc(work->crtc)->unpin_work_count) == 0);
 	atomic_dec(&to_intel_crtc(work->crtc)->unpin_work_count);
 
@@ -9302,9 +9461,6 @@ 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);
-
 	work->event = event;
 	work->crtc = crtc;
 	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
@@ -9375,7 +9531,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 				INTEL_FRONTBUFFER_PRIMARY(pipe));
 
 	intel_disable_fbc(dev);
-	intel_mark_fb_busy(obj, NULL);
+	intel_fb_flip_prepare(obj);
 	mutex_unlock(&dev->struct_mutex);
 
 	trace_i915_flip_request(intel_crtc->plane, obj);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1d45629a6483..41776bfeddbd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -718,8 +718,14 @@ bool intel_has_pending_fb_unpin(struct drm_device *dev);
 int intel_pch_rawclk(struct drm_device *dev);
 int valleyview_cur_cdclk(struct drm_i915_private *dev_priv);
 void intel_mark_busy(struct drm_device *dev);
-void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
-			struct intel_engine_cs *ring);
+void intel_fb_invalidate(struct drm_i915_gem_object *obj,
+			 struct intel_engine_cs *ring);
+void intel_fb_flip_prepare(struct drm_i915_gem_object *obj);
+void intel_frontbuffer_flip_complete(struct drm_device *dev,
+				     unsigned frontbuffer_bits);
+void intel_frontbuffer_flush(struct drm_device *dev,
+			     unsigned frontbuffer_bits);
+void intel_fb_flush(struct drm_i915_gem_object *obj, bool retire);
 void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 27975c3e21c5..12276c39d14d 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -43,10 +43,26 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static int intel_fbdev_set_par(struct fb_info *info)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+	struct intel_fbdev *ifbdev =
+		container_of(fb_helper, struct intel_fbdev, helper);
+	int ret;
+
+	ret = drm_fb_helper_set_par(info);
+
+	if (ret == 0)
+		ret = i915_gem_object_set_to_gtt_domain(ifbdev->fb->obj,
+							true);
+
+	return ret;
+}
+
 static struct fb_ops intelfb_ops = {
 	.owner = THIS_MODULE,
 	.fb_check_var = drm_fb_helper_check_var,
-	.fb_set_par = drm_fb_helper_set_par,
+	.fb_set_par = intel_fbdev_set_par,
 	.fb_fillrect = cfb_fillrect,
 	.fb_copyarea = cfb_copyarea,
 	.fb_imageblit = cfb_imageblit,
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 6aeda6a489a0..16e42071d56e 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -790,6 +790,9 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	overlay->old_vid_bo = overlay->vid_bo;
 	overlay->vid_bo = new_bo;
 
+	intel_frontbuffer_flush(dev,
+				INTEL_FRONTBUFFER_OVERLAY(pipe));
+
 	return 0;
 
 out_unpin:
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 3648594aeb7e..4d9c47368094 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1034,6 +1034,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		else
 			intel_plane->disable_plane(plane, crtc);
 
+		intel_frontbuffer_flush(dev, INTEL_FRONTBUFFER_SPRITE(pipe));
+
 		if (!primary_was_enabled && primary_enabled)
 			intel_post_enable_primary(crtc);
 	}
@@ -1054,8 +1056,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	intel_edp_psr_exit(dev);
-
 	return 0;
 }
 
-- 
2.0.0

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 27975c3e21c5..12276c39d14d 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -43,10 +43,26 @@
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  
> +static int intel_fbdev_set_par(struct fb_info *info)
> +{
> +	struct drm_fb_helper *fb_helper = info->par;
> +	struct intel_fbdev *ifbdev =
> +		container_of(fb_helper, struct intel_fbdev, helper);
> +	int ret;
> +
> +	ret = drm_fb_helper_set_par(info);
> +
> +	if (ret == 0)
> +		ret = i915_gem_object_set_to_gtt_domain(ifbdev->fb->obj,
> +							true);
> +
> +	return ret;
> +}

Ah, I had missed you added this. Yes, this is what I had in mind for
fixing fbcon. However, this is worth splitting out into a separate path
as this is about to get hairy!

ret = drm_fb_helper_set_par(info)
if (ret) return ret;

 /* Invalidate the fb for all further writes whilst on the console */
 if (!in_atomic() && mutex_trylock(&dev->struct_mutex)) {
   ret = i915_gem_object_set_tp_gtt_domain(obj, true);
   mutex_unlock(&dev->srtuct_mutex);
  }
 return ret;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

On Tue, Jun 17, 2014 at 07:41:48AM +0100, Chris Wilson wrote:
> On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 27975c3e21c5..12276c39d14d 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -43,10 +43,26 @@
> >  #include <drm/i915_drm.h>
> >  #include "i915_drv.h"
> >  
> > +static int intel_fbdev_set_par(struct fb_info *info)
> > +{
> > +	struct drm_fb_helper *fb_helper = info->par;
> > +	struct intel_fbdev *ifbdev =
> > +		container_of(fb_helper, struct intel_fbdev, helper);
> > +	int ret;
> > +
> > +	ret = drm_fb_helper_set_par(info);
> > +
> > +	if (ret == 0)
> > +		ret = i915_gem_object_set_to_gtt_domain(ifbdev->fb->obj,
> > +							true);
> > +
> > +	return ret;
> > +}
> 
> Ah, I had missed you added this. Yes, this is what I had in mind for
> fixing fbcon. However, this is worth splitting out into a separate path
> as this is about to get hairy!
> 
> ret = drm_fb_helper_set_par(info)
> if (ret) return ret;
> 
>  /* Invalidate the fb for all further writes whilst on the console */
>  if (!in_atomic() && mutex_trylock(&dev->struct_mutex)) {
>    ret = i915_gem_object_set_tp_gtt_domain(obj, true);
>    mutex_unlock(&dev->srtuct_mutex);
>   }
>  return ret;

We already have ridiculous amounts of not trylocked state in set_par, so
doing this won't help really. And fixing up the emergency console logic is
imo out of scope for this ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Tue, Jun 17, 2014 at 09:32:47AM +0200, Daniel Vetter wrote:
> On Tue, Jun 17, 2014 at 07:41:48AM +0100, Chris Wilson wrote:
> > On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > > index 27975c3e21c5..12276c39d14d 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > @@ -43,10 +43,26 @@
> > >  #include <drm/i915_drm.h>
> > >  #include "i915_drv.h"
> > >  
> > > +static int intel_fbdev_set_par(struct fb_info *info)
> > > +{
> > > +	struct drm_fb_helper *fb_helper = info->par;
> > > +	struct intel_fbdev *ifbdev =
> > > +		container_of(fb_helper, struct intel_fbdev, helper);
> > > +	int ret;
> > > +
> > > +	ret = drm_fb_helper_set_par(info);
> > > +
> > > +	if (ret == 0)
> > > +		ret = i915_gem_object_set_to_gtt_domain(ifbdev->fb->obj,
> > > +							true);
> > > +
> > > +	return ret;
> > > +}
> > 
> > Ah, I had missed you added this. Yes, this is what I had in mind for
> > fixing fbcon. However, this is worth splitting out into a separate path
> > as this is about to get hairy!
> > 
> > ret = drm_fb_helper_set_par(info)
> > if (ret) return ret;
> > 
> >  /* Invalidate the fb for all further writes whilst on the console */
> >  if (!in_atomic() && mutex_trylock(&dev->struct_mutex)) {
> >    ret = i915_gem_object_set_tp_gtt_domain(obj, true);
> >    mutex_unlock(&dev->srtuct_mutex);
> >   }
> >  return ret;
> 
> We already have ridiculous amounts of not trylocked state in set_par, so
> doing this won't help really. And fixing up the emergency console logic is
> imo out of scope for this ;-)

But at least separate the dubious from the rest of the patch. :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

On Tue, Jun 17, 2014 at 08:36:48AM +0100, Chris Wilson wrote:
> On Tue, Jun 17, 2014 at 09:32:47AM +0200, Daniel Vetter wrote:
> > On Tue, Jun 17, 2014 at 07:41:48AM +0100, Chris Wilson wrote:
> > > On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > > > index 27975c3e21c5..12276c39d14d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > > @@ -43,10 +43,26 @@
> > > >  #include <drm/i915_drm.h>
> > > >  #include "i915_drv.h"
> > > >  
> > > > +static int intel_fbdev_set_par(struct fb_info *info)
> > > > +{
> > > > +	struct drm_fb_helper *fb_helper = info->par;
> > > > +	struct intel_fbdev *ifbdev =
> > > > +		container_of(fb_helper, struct intel_fbdev, helper);
> > > > +	int ret;
> > > > +
> > > > +	ret = drm_fb_helper_set_par(info);
> > > > +
> > > > +	if (ret == 0)
> > > > +		ret = i915_gem_object_set_to_gtt_domain(ifbdev->fb->obj,
> > > > +							true);
> > > > +
> > > > +	return ret;
> > > > +}
> > > 
> > > Ah, I had missed you added this. Yes, this is what I had in mind for
> > > fixing fbcon. However, this is worth splitting out into a separate path
> > > as this is about to get hairy!
> > > 
> > > ret = drm_fb_helper_set_par(info)
> > > if (ret) return ret;
> > > 
> > >  /* Invalidate the fb for all further writes whilst on the console */
> > >  if (!in_atomic() && mutex_trylock(&dev->struct_mutex)) {
> > >    ret = i915_gem_object_set_tp_gtt_domain(obj, true);
> > >    mutex_unlock(&dev->srtuct_mutex);
> > >   }
> > >  return ret;
> > 
> > We already have ridiculous amounts of not trylocked state in set_par, so
> > doing this won't help really. And fixing up the emergency console logic is
> > imo out of scope for this ;-)
> 
> But at least separate the dubious from the rest of the patch. :)

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

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
>  void i915_gem_release(struct drm_device *dev, struct drm_file *file)
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 93d7f7246588..2d06aad4a954 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -978,7 +978,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
>  			/* check for potential scanout */
>  			if (i915_gem_obj_ggtt_bound(obj) &&
>  			    i915_gem_obj_to_ggtt(obj)->pin_count)

What is it about these checks you like so much? You haven't taken the
patches to convert this to if (obj->pin_display) and you haven't
converted them to obj->frontbuffer_bits in this patch either!

> -				intel_mark_fb_busy(obj, ring);
> +				intel_fb_invalidate(obj, ring);
>  
>  			/* update for the implicit flush after a batch */
>  			obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

On Tue, Jun 17, 2014 at 07:46:46AM +0100, Chris Wilson wrote:
> On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> >  void i915_gem_release(struct drm_device *dev, struct drm_file *file)
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 93d7f7246588..2d06aad4a954 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -978,7 +978,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
> >  			/* check for potential scanout */
> >  			if (i915_gem_obj_ggtt_bound(obj) &&
> >  			    i915_gem_obj_to_ggtt(obj)->pin_count)
> 
> What is it about these checks you like so much? You haven't taken the
> patches to convert this to if (obj->pin_display) and you haven't
> converted them to obj->frontbuffer_bits in this patch either!

Just didn't touch them. But yeah, these should be converted to
obj->frontbuffer_bits I guess.
-Daniel

> 
> > -				intel_mark_fb_busy(obj, ring);
> > +				intel_fb_invalidate(obj, ring);
> >  
> >  			/* update for the implicit flush after a batch */
> >  			obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> 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 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> @@ -3966,6 +3971,8 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
>  		obj->base.write_domain = I915_GEM_DOMAIN_CPU;
>  	}
>  
/* We do not need to explicitly invalidate the fb when moving to the
 * CPU domain as it is not coherent with the display. Therefore the user
 * much flush updates to the scanout through the frontbuffer whilst in
 * the CPU domain by explicitly flushing it - either by manually moving
 * to the GTT domain, or by calling sw-finish.
 */
> +	intel_fb_invalidate(obj, NULL);
And so kill this.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

On Tue, Jun 17, 2014 at 07:49:30AM +0100, Chris Wilson wrote:
> On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > @@ -3966,6 +3971,8 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
> >  		obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> >  	}
> >  
> /* We do not need to explicitly invalidate the fb when moving to the
>  * CPU domain as it is not coherent with the display. Therefore the user
>  * much flush updates to the scanout through the frontbuffer whilst in
>  * the CPU domain by explicitly flushing it - either by manually moving
>  * to the GTT domain, or by calling sw-finish.
>  */
> > +	intel_fb_invalidate(obj, NULL);
> And so kill this.

The idea of the interface is that you'll always see an invalidate before a
flush for all frontbuffer rendering. If we don't do that we'd need to
filter out all the surplus flushes we generate without an invalidate
beforehand, but imo passing all that down to the consumers is better since
it allows hacks like the hsw sprite psr w/a.

And moving the invalidate next to the flush in sw_finish_ioctl imo doesn't
make sense.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> @@ -2227,6 +2223,9 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
>  			list_move_tail(&vma->mm_list, &vm->inactive_list);
>  	}
>  
> +	if (obj->frontbuffer_bits)
> +		intel_fb_flush(obj, true);
> +
>  	list_del_init(&obj->ring_list);
>  	obj->ring = NULL;
>  
> @@ -3556,6 +3555,8 @@ i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
>  	old_write_domain = obj->base.write_domain;
>  	obj->base.write_domain = 0;
>  
> +	intel_fb_flush(obj, false);
> +
I think it is worth the if (obj->frontbuffer_bit) check everywhere.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

On Tue, Jun 17, 2014 at 07:50:45AM +0100, Chris Wilson wrote:
> On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > @@ -2227,6 +2223,9 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
> >  			list_move_tail(&vma->mm_list, &vm->inactive_list);
> >  	}
> >  
> > +	if (obj->frontbuffer_bits)
> > +		intel_fb_flush(obj, true);
> > +
> >  	list_del_init(&obj->ring_list);
> >  	obj->ring = NULL;
> >  
> > @@ -3556,6 +3555,8 @@ i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
> >  	old_write_domain = obj->base.write_domain;
> >  	obj->base.write_domain = 0;
> >  
> > +	intel_fb_flush(obj, false);
> > +
> I think it is worth the if (obj->frontbuffer_bit) check everywhere.

Well it's the first thing fb_flush checks, so the additional ones are just
stupid micro-optimizations I guess. Should I remove them everywhere?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Tue, Jun 17, 2014 at 09:37:35AM +0200, Daniel Vetter wrote:
> On Tue, Jun 17, 2014 at 07:50:45AM +0100, Chris Wilson wrote:
> > On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > > @@ -2227,6 +2223,9 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
> > >  			list_move_tail(&vma->mm_list, &vm->inactive_list);
> > >  	}
> > >  
> > > +	if (obj->frontbuffer_bits)
> > > +		intel_fb_flush(obj, true);
> > > +
> > >  	list_del_init(&obj->ring_list);
> > >  	obj->ring = NULL;
> > >  
> > > @@ -3556,6 +3555,8 @@ i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
> > >  	old_write_domain = obj->base.write_domain;
> > >  	obj->base.write_domain = 0;
> > >  
> > > +	intel_fb_flush(obj, false);
> > > +
> > I think it is worth the if (obj->frontbuffer_bit) check everywhere.
> 
> Well it's the first thing fb_flush checks, so the additional ones are just
> stupid micro-optimizations I guess. Should I remove them everywhere?

Yes. The most frequent of these would be in execbuffer, so perhaps keep
that one. But the rest are equally in the noise.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Tue, Jun 17, 2014 at 08:40:10AM +0100, Chris Wilson wrote:
> On Tue, Jun 17, 2014 at 09:37:35AM +0200, Daniel Vetter wrote:
> > On Tue, Jun 17, 2014 at 07:50:45AM +0100, Chris Wilson wrote:
> > > On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > > > @@ -2227,6 +2223,9 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
> > > >  			list_move_tail(&vma->mm_list, &vm->inactive_list);
> > > >  	}
> > > >  
> > > > +	if (obj->frontbuffer_bits)
> > > > +		intel_fb_flush(obj, true);
> > > > +
> > > >  	list_del_init(&obj->ring_list);
> > > >  	obj->ring = NULL;
> > > >  
> > > > @@ -3556,6 +3555,8 @@ i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
> > > >  	old_write_domain = obj->base.write_domain;
> > > >  	obj->base.write_domain = 0;
> > > >  
> > > > +	intel_fb_flush(obj, false);
> > > > +
> > > I think it is worth the if (obj->frontbuffer_bit) check everywhere.
> > 
> > Well it's the first thing fb_flush checks, so the additional ones are just
> > stupid micro-optimizations I guess. Should I remove them everywhere?
> 
> Yes. The most frequent of these would be in execbuffer, so perhaps keep
> that one. But the rest are equally in the noise.

Equally, move-to-inactive. Slightly less hit than execbuffer, but often
enough to make me concerned.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> +static void intel_mark_fb_busy(struct drm_device *dev,
> +			       unsigned frontbuffer_bits,
> +			       struct intel_engine_cs *ring)
>  {
> -	struct drm_device *dev = obj->base.dev;
>  	enum pipe pipe;
>  
> -	intel_edp_psr_exit(dev);
> -
>  	if (!i915.powersave)
>  		return;

I think this wants to be moved from here down to
intel_increase_pllclock. It's a little dangerous here as we use it
inconsistently, and we certainly want to keep tracking fb bits for other
reasons.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

On Tue, Jun 17, 2014 at 07:52:25AM +0100, Chris Wilson wrote:
> On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > +static void intel_mark_fb_busy(struct drm_device *dev,
> > +			       unsigned frontbuffer_bits,
> > +			       struct intel_engine_cs *ring)
> >  {
> > -	struct drm_device *dev = obj->base.dev;
> >  	enum pipe pipe;
> >  
> > -	intel_edp_psr_exit(dev);
> > -
> >  	if (!i915.powersave)
> >  		return;
> 
> I think this wants to be moved from here down to
> intel_increase_pllclock. It's a little dangerous here as we use it
> inconsistently, and we certainly want to keep tracking fb bits for other
> reasons.

powersave was originally the overall blocker for all things power saving
related. We already have separate knobs for all real features, so I guess
we could just rip this out - it doesn't actually guard much of anything,
is inconsistent and always enabled anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> +/**
> + * intel_frontbuffer_flush - flush frontbuffer
> + * @dev: DRM device
> + * @frontbuffer_bits: frontbuffer plane tracking bits
> + *
> + * This function gets called every time rendering on the given planes has
> + * completed and frontbuffer caching can be started again. Flushes will get
> + * delayed if they're blocked by some oustanding asynchronous rendering.
> + *
> + * Can be called without any locks held.
> + */
> +void intel_frontbuffer_flush(struct drm_device *dev,
> +			     unsigned frontbuffer_bits)

intel_fb_complete.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

On Tue, Jun 17, 2014 at 07:54:38AM +0100, Chris Wilson wrote:
> On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > +/**
> > + * intel_frontbuffer_flush - flush frontbuffer
> > + * @dev: DRM device
> > + * @frontbuffer_bits: frontbuffer plane tracking bits
> > + *
> > + * This function gets called every time rendering on the given planes has
> > + * completed and frontbuffer caching can be started again. Flushes will get
> > + * delayed if they're blocked by some oustanding asynchronous rendering.
> > + *
> > + * Can be called without any locks held.
> > + */
> > +void intel_frontbuffer_flush(struct drm_device *dev,
> > +			     unsigned frontbuffer_bits)
> 
> intel_fb_complete.

My naming convetion was:
- intel_frontbuffer: Deals in raw frontbuffer tracking bits.
- intel_fb: Takes a gem object, assumes dev->struct_mutex is held. I've
  stolen these from the intel_mark_fb_busy function.

The main functions are flush/invalidate, all the others just add a bit of
magic (which delays) around them. Imo invalidate is definitely the right
pick if you look at fbc/psr as fancy caches, not sure about flush. Maybe
finish given our sw_finish ioctl, but not sure that's better than flush.
My thinking is:

- invalidate the cache fully (i.e. disable it)
- flush data into the cache again (i.e. enable it again)

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

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Tue, Jun 17, 2014 at 09:45:00AM +0200, Daniel Vetter wrote:
> On Tue, Jun 17, 2014 at 07:54:38AM +0100, Chris Wilson wrote:
> > On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > > +/**
> > > + * intel_frontbuffer_flush - flush frontbuffer
> > > + * @dev: DRM device
> > > + * @frontbuffer_bits: frontbuffer plane tracking bits
> > > + *
> > > + * This function gets called every time rendering on the given planes has
> > > + * completed and frontbuffer caching can be started again. Flushes will get
> > > + * delayed if they're blocked by some oustanding asynchronous rendering.
> > > + *
> > > + * Can be called without any locks held.
> > > + */
> > > +void intel_frontbuffer_flush(struct drm_device *dev,
> > > +			     unsigned frontbuffer_bits)
> > 
> > intel_fb_complete.
> 
> My naming convetion was:
> - intel_frontbuffer: Deals in raw frontbuffer tracking bits.
> - intel_fb: Takes a gem object, assumes dev->struct_mutex is held. I've
>   stolen these from the intel_mark_fb_busy function.
> 
> The main functions are flush/invalidate, all the others just add a bit of
> magic (which delays) around them. Imo invalidate is definitely the right
> pick if you look at fbc/psr as fancy caches, not sure about flush. Maybe
> finish given our sw_finish ioctl, but not sure that's better than flush.
> My thinking is:
> 
> - invalidate the cache fully (i.e. disable it)
> - flush data into the cache again (i.e. enable it again)

That's also how I named it, so we agree here.

I am not fond of intermixing intel_frontbuffer and intel_fb though.
Certainly not having the function definitions intertwined.

Maybe intel_frontbuffer and intel_frontbuffer_obj?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

On Tue, Jun 17, 2014 at 08:53:47AM +0100, Chris Wilson wrote:
> On Tue, Jun 17, 2014 at 09:45:00AM +0200, Daniel Vetter wrote:
> > On Tue, Jun 17, 2014 at 07:54:38AM +0100, Chris Wilson wrote:
> > > On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > > > +/**
> > > > + * intel_frontbuffer_flush - flush frontbuffer
> > > > + * @dev: DRM device
> > > > + * @frontbuffer_bits: frontbuffer plane tracking bits
> > > > + *
> > > > + * This function gets called every time rendering on the given planes has
> > > > + * completed and frontbuffer caching can be started again. Flushes will get
> > > > + * delayed if they're blocked by some oustanding asynchronous rendering.
> > > > + *
> > > > + * Can be called without any locks held.
> > > > + */
> > > > +void intel_frontbuffer_flush(struct drm_device *dev,
> > > > +			     unsigned frontbuffer_bits)
> > > 
> > > intel_fb_complete.
> > 
> > My naming convetion was:
> > - intel_frontbuffer: Deals in raw frontbuffer tracking bits.
> > - intel_fb: Takes a gem object, assumes dev->struct_mutex is held. I've
> >   stolen these from the intel_mark_fb_busy function.
> > 
> > The main functions are flush/invalidate, all the others just add a bit of
> > magic (which delays) around them. Imo invalidate is definitely the right
> > pick if you look at fbc/psr as fancy caches, not sure about flush. Maybe
> > finish given our sw_finish ioctl, but not sure that's better than flush.
> > My thinking is:
> > 
> > - invalidate the cache fully (i.e. disable it)
> > - flush data into the cache again (i.e. enable it again)
> 
> That's also how I named it, so we agree here.
> 
> I am not fond of intermixing intel_frontbuffer and intel_fb though.
> Certainly not having the function definitions intertwined.
> 
> Maybe intel_frontbuffer and intel_frontbuffer_obj?

intel_fb_obj would be good for me since frontbuffer is awfully long and
leads to ugly code. But I agree that sprinkling obj in would be good, and
we have precedence for fb_obj with pin_and_fence.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> +/**
> + * intel_fb_flip_prepare - flip frontbuffer
> + * @obj: GEM object to flush
> + *
> + * This function gets called after scheduling a flip on @obj. The actual
> + * frontbuffer flushing will be delayed until completion is signalled with
> + * intel_frontbuffer_flip_complete. If an invalidate happens in between this
> + * flush will be cancelled.
> + *
> + * Note that the frontbuffer tracking bits in obj->frontbuffer_bits must have
> + * been updated already.
> + */
> +void intel_fb_flip_prepare(struct drm_i915_gem_object *obj)
> +{
> +	struct drm_device *dev = obj->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> +	WARN_ON(!obj->frontbuffer_bits);
> +
> +	mutex_lock(&dev_priv->fb_tracking.lock);
> +	dev_priv->fb_tracking.flip_bits
> +		|= obj->frontbuffer_bits;
> +	mutex_unlock(&dev_priv->fb_tracking.lock);
> +}
> +
> +/**
> + * intel_frontbuffer_flip_complete - complete frontbuffer flip
> + * @dev: DRM device
> + * @frontbuffer_bits: frontbuffer plane tracking bits
> + *
> + * This function gets called after the flip has been latched and will complete
> + * on the next vblank. It will execute the fush if it hasn't been cancalled yet.
> + *
> + * Can be called without any locks held.
> + */
> +void intel_frontbuffer_flip_complete(struct drm_device *dev,
> +				     unsigned frontbuffer_bits)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	mutex_lock(&dev_priv->fb_tracking.lock);
> +	/* Mask any cancelled flips. */
> +	frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
> +	dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
> +	mutex_unlock(&dev_priv->fb_tracking.lock);
> +
> +	intel_frontbuffer_flush(dev, frontbuffer_bits);
> +}

Let's be consistent and call this intel_fb_flip_complete. I am happy
with the slight confusion between intel_fb and
i915_frontbuffer_tracking, since they are related and it is easy enough
to make the conceptual leap that only the fb that are on the scanout
(ala frontbuffers) need to be tracked - but we only every need to track
fbs...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

On Tue, Jun 17, 2014 at 07:57:22AM +0100, Chris Wilson wrote:
> On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > +/**
> > + * intel_fb_flip_prepare - flip frontbuffer
> > + * @obj: GEM object to flush
> > + *
> > + * This function gets called after scheduling a flip on @obj. The actual
> > + * frontbuffer flushing will be delayed until completion is signalled with
> > + * intel_frontbuffer_flip_complete. If an invalidate happens in between this
> > + * flush will be cancelled.
> > + *
> > + * Note that the frontbuffer tracking bits in obj->frontbuffer_bits must have
> > + * been updated already.
> > + */
> > +void intel_fb_flip_prepare(struct drm_i915_gem_object *obj)
> > +{
> > +	struct drm_device *dev = obj->base.dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> > +	WARN_ON(!obj->frontbuffer_bits);
> > +
> > +	mutex_lock(&dev_priv->fb_tracking.lock);
> > +	dev_priv->fb_tracking.flip_bits
> > +		|= obj->frontbuffer_bits;
> > +	mutex_unlock(&dev_priv->fb_tracking.lock);
> > +}
> > +
> > +/**
> > + * intel_frontbuffer_flip_complete - complete frontbuffer flip
> > + * @dev: DRM device
> > + * @frontbuffer_bits: frontbuffer plane tracking bits
> > + *
> > + * This function gets called after the flip has been latched and will complete
> > + * on the next vblank. It will execute the fush if it hasn't been cancalled yet.
> > + *
> > + * Can be called without any locks held.
> > + */
> > +void intel_frontbuffer_flip_complete(struct drm_device *dev,
> > +				     unsigned frontbuffer_bits)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	mutex_lock(&dev_priv->fb_tracking.lock);
> > +	/* Mask any cancelled flips. */
> > +	frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
> > +	dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
> > +	mutex_unlock(&dev_priv->fb_tracking.lock);
> > +
> > +	intel_frontbuffer_flush(dev, frontbuffer_bits);
> > +}
> 
> Let's be consistent and call this intel_fb_flip_complete. I am happy
> with the slight confusion between intel_fb and
> i915_frontbuffer_tracking, since they are related and it is easy enough
> to make the conceptual leap that only the fb that are on the scanout
> (ala frontbuffers) need to be tracked - but we only every need to track
> fbs...

You need to pair an fb_flip_prepare with a flip_complete, otherwise
there's no flush. It needs to be two-step to make sure we don't override
any intermediate rendering (e.g. if someone flips and then right away goes
ahead with a frontbuffer rendering batch). A direct flush otoh is
different since it's not async, so we don't need any special protection
for that window.

Note that all flushing means: "Please upload the next full frame still to
the screen", i.e. one implied vblank wait is always there. Even for the
above synchronous flush.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Tue, Jun 17, 2014 at 09:48:39AM +0200, Daniel Vetter wrote:
> On Tue, Jun 17, 2014 at 07:57:22AM +0100, Chris Wilson wrote:
> > On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > > +/**
> > > + * intel_fb_flip_prepare - flip frontbuffer
> > > + * @obj: GEM object to flush
> > > + *
> > > + * This function gets called after scheduling a flip on @obj. The actual
> > > + * frontbuffer flushing will be delayed until completion is signalled with
> > > + * intel_frontbuffer_flip_complete. If an invalidate happens in between this
> > > + * flush will be cancelled.
> > > + *
> > > + * Note that the frontbuffer tracking bits in obj->frontbuffer_bits must have
> > > + * been updated already.
> > > + */
> > > +void intel_fb_flip_prepare(struct drm_i915_gem_object *obj)
> > > +{
> > > +	struct drm_device *dev = obj->base.dev;
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +
> > > +	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> > > +	WARN_ON(!obj->frontbuffer_bits);
> > > +
> > > +	mutex_lock(&dev_priv->fb_tracking.lock);
> > > +	dev_priv->fb_tracking.flip_bits
> > > +		|= obj->frontbuffer_bits;
> > > +	mutex_unlock(&dev_priv->fb_tracking.lock);
> > > +}
> > > +
> > > +/**
> > > + * intel_frontbuffer_flip_complete - complete frontbuffer flip
> > > + * @dev: DRM device
> > > + * @frontbuffer_bits: frontbuffer plane tracking bits
> > > + *
> > > + * This function gets called after the flip has been latched and will complete
> > > + * on the next vblank. It will execute the fush if it hasn't been cancalled yet.
> > > + *
> > > + * Can be called without any locks held.
> > > + */
> > > +void intel_frontbuffer_flip_complete(struct drm_device *dev,
> > > +				     unsigned frontbuffer_bits)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +
> > > +	mutex_lock(&dev_priv->fb_tracking.lock);
> > > +	/* Mask any cancelled flips. */
> > > +	frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
> > > +	dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
> > > +	mutex_unlock(&dev_priv->fb_tracking.lock);
> > > +
> > > +	intel_frontbuffer_flush(dev, frontbuffer_bits);
> > > +}
> > 
> > Let's be consistent and call this intel_fb_flip_complete. I am happy
> > with the slight confusion between intel_fb and
> > i915_frontbuffer_tracking, since they are related and it is easy enough
> > to make the conceptual leap that only the fb that are on the scanout
> > (ala frontbuffers) need to be tracked - but we only every need to track
> > fbs...
> 
> You need to pair an fb_flip_prepare with a flip_complete, otherwise
> there's no flush. It needs to be two-step to make sure we don't override
> any intermediate rendering (e.g. if someone flips and then right away goes
> ahead with a frontbuffer rendering batch). A direct flush otoh is
> different since it's not async, so we don't need any special protection
> for that window.

I was only critising the function name :) Treating the mmio updates as
single step flips comes later...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b8359f4a6dc4..dfdbf2a02844 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2755,6 +2755,8 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  
>  	dev_priv->display.update_primary_plane(crtc, fb, x, y);
>  
> +	intel_frontbuffer_flush(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));

Conceptually all of these are just intel_fb_flip_complete. It may be
easier in your tracking scheme to have:

intel_fb_flip() { intel_fb_flip_prepare(); intel_fb_fip_complete(); }
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

On Tue, Jun 17, 2014 at 08:00:05AM +0100, Chris Wilson wrote:
> On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b8359f4a6dc4..dfdbf2a02844 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2755,6 +2755,8 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> >  
> >  	dev_priv->display.update_primary_plane(crtc, fb, x, y);
> >  
> > +	intel_frontbuffer_flush(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
> 
> Conceptually all of these are just intel_fb_flip_complete. It may be
> easier in your tracking scheme to have:
> 
> intel_fb_flip() { intel_fb_flip_prepare(); intel_fb_fip_complete(); }

Yeah, this is just the direct flush for synchronous updates. Implementing
that as a prepare+complete is imo too confusing - the entire point of
prepare+complete is to catch an intermediate invalidates and not complete
the flush and so only really required for async flips. Hence why I prefer
to only do the full prepare+complete dance where needed. Otherwise there
is nothing special about flips.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Tue, Jun 17, 2014 at 09:52:17AM +0200, Daniel Vetter wrote:
> On Tue, Jun 17, 2014 at 08:00:05AM +0100, Chris Wilson wrote:
> > On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index b8359f4a6dc4..dfdbf2a02844 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2755,6 +2755,8 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> > >  
> > >  	dev_priv->display.update_primary_plane(crtc, fb, x, y);
> > >  
> > > +	intel_frontbuffer_flush(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
> > 
> > Conceptually all of these are just intel_fb_flip_complete. It may be
> > easier in your tracking scheme to have:
> > 
> > intel_fb_flip() { intel_fb_flip_prepare(); intel_fb_fip_complete(); }
> 
> Yeah, this is just the direct flush for synchronous updates. Implementing
> that as a prepare+complete is imo too confusing - the entire point of
> prepare+complete is to catch an intermediate invalidates and not complete
> the flush and so only really required for async flips. Hence why I prefer
> to only do the full prepare+complete dance where needed. Otherwise there
> is nothing special about flips.

I just prefer to have the like operations having like names. The key
point here is that we just want to treat the set-plane operations as a
synchronous flip, which is distinct to the invalidate/flush operations on
an active frontbuffer.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

On Tue, Jun 17, 2014 at 09:10:14AM +0100, Chris Wilson wrote:
> On Tue, Jun 17, 2014 at 09:52:17AM +0200, Daniel Vetter wrote:
> > On Tue, Jun 17, 2014 at 08:00:05AM +0100, Chris Wilson wrote:
> > > On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index b8359f4a6dc4..dfdbf2a02844 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -2755,6 +2755,8 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> > > >  
> > > >  	dev_priv->display.update_primary_plane(crtc, fb, x, y);
> > > >  
> > > > +	intel_frontbuffer_flush(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
> > > 
> > > Conceptually all of these are just intel_fb_flip_complete. It may be
> > > easier in your tracking scheme to have:
> > > 
> > > intel_fb_flip() { intel_fb_flip_prepare(); intel_fb_fip_complete(); }
> > 
> > Yeah, this is just the direct flush for synchronous updates. Implementing
> > that as a prepare+complete is imo too confusing - the entire point of
> > prepare+complete is to catch an intermediate invalidates and not complete
> > the flush and so only really required for async flips. Hence why I prefer
> > to only do the full prepare+complete dance where needed. Otherwise there
> > is nothing special about flips.
> 
> I just prefer to have the like operations having like names. The key
> point here is that we just want to treat the set-plane operations as a
> synchronous flip, which is distinct to the invalidate/flush operations on
> an active frontbuffer.

Well the main thing is the setplane is a flush operation. Flip is also a
flush, but an asynchronous one. Maybe s/flip/flush/ instead to make it
clear that the special part isn't that it's a flip, but that it's
two-stage?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Tue, Jun 17, 2014 at 11:33:41AM +0200, Daniel Vetter wrote:
> On Tue, Jun 17, 2014 at 09:10:14AM +0100, Chris Wilson wrote:
> > On Tue, Jun 17, 2014 at 09:52:17AM +0200, Daniel Vetter wrote:
> > > On Tue, Jun 17, 2014 at 08:00:05AM +0100, Chris Wilson wrote:
> > > > On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > index b8359f4a6dc4..dfdbf2a02844 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -2755,6 +2755,8 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> > > > >  
> > > > >  	dev_priv->display.update_primary_plane(crtc, fb, x, y);
> > > > >  
> > > > > +	intel_frontbuffer_flush(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
> > > > 
> > > > Conceptually all of these are just intel_fb_flip_complete. It may be
> > > > easier in your tracking scheme to have:
> > > > 
> > > > intel_fb_flip() { intel_fb_flip_prepare(); intel_fb_fip_complete(); }
> > > 
> > > Yeah, this is just the direct flush for synchronous updates. Implementing
> > > that as a prepare+complete is imo too confusing - the entire point of
> > > prepare+complete is to catch an intermediate invalidates and not complete
> > > the flush and so only really required for async flips. Hence why I prefer
> > > to only do the full prepare+complete dance where needed. Otherwise there
> > > is nothing special about flips.
> > 
> > I just prefer to have the like operations having like names. The key
> > point here is that we just want to treat the set-plane operations as a
> > synchronous flip, which is distinct to the invalidate/flush operations on
> > an active frontbuffer.
> 
> Well the main thing is the setplane is a flush operation. Flip is also a
> flush, but an asynchronous one. Maybe s/flip/flush/ instead to make it
> clear that the special part isn't that it's a flip, but that it's
> two-stage?

Yes, having s/flip/flush/ is preferrable from a consistency pov. I am
just not yet sold on that setplane is a intel_fb_flush. I think the busy
state tracking on the new fb is distinct from the state tracking on the
old fb, and we are marking the transition from one fb to the other. I
would rather keep the invalidate/flush as boundaries on the current fb,
with the distinction made for change of fb.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

On Tue, Jun 17, 2014 at 11:42 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Yes, having s/flip/flush/ is preferrable from a consistency pov. I am
> just not yet sold on that setplane is a intel_fb_flush. I think the busy
> state tracking on the new fb is distinct from the state tracking on the
> old fb, and we are marking the transition from one fb to the other. I
> would rather keep the invalidate/flush as boundaries on the current fb,
> with the distinction made for change of fb.

Well my idea is that on every flip all old frontbuffer rendering can
be ignored and so we should have an unconditional flush of any
invalidations. And at least on modern hw (pre-g4x fbc is different)
the hw works for purely flip based screen updates already. Well,
except for sprites on hsw with psr.

So currently I don't see a need for a distinct flip type of event.
This might chance once we have single-shot upload for psr/dsi cmd
mode. But then I think that's better done as part of the magic and
all-encompassing nuclear pageflip. We have a bunch of opens in that
area still which are all similar, e.g. enabling ips with a 1 vblank
delay, or disabling fbc 1 vblank before disabling the primary plane.
Getting that all correct and async will be work, and my gut says most
of it won't fit into this fb tracking scheme which is centered around
catching frontbuffer rendering.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Tue, Jun 17, 2014 at 11:54:16AM +0200, Daniel Vetter wrote:
> On Tue, Jun 17, 2014 at 11:42 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Yes, having s/flip/flush/ is preferrable from a consistency pov. I am
> > just not yet sold on that setplane is a intel_fb_flush. I think the busy
> > state tracking on the new fb is distinct from the state tracking on the
> > old fb, and we are marking the transition from one fb to the other. I
> > would rather keep the invalidate/flush as boundaries on the current fb,
> > with the distinction made for change of fb.
> 
> Well my idea is that on every flip all old frontbuffer rendering can
> be ignored and so we should have an unconditional flush of any
> invalidations.

But that would be a discard, a different type of beastie.

> And at least on modern hw (pre-g4x fbc is different)
> the hw works for purely flip based screen updates already. Well,
> except for sprites on hsw with psr.
> 
> So currently I don't see a need for a distinct flip type of event.
> This might chance once we have single-shot upload for psr/dsi cmd
> mode. But then I think that's better done as part of the magic and
> all-encompassing nuclear pageflip. We have a bunch of opens in that
> area still which are all similar, e.g. enabling ips with a 1 vblank
> delay, or disabling fbc 1 vblank before disabling the primary plane.
> Getting that all correct and async will be work, and my gut says most
> of it won't fit into this fb tracking scheme which is centered around
> catching frontbuffer rendering.

Exactly, I'm trying to make the frontbuffer rendering tracker clean,
but I think you are conflating hw details with the tracking layer.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 15/15] drm/i915: Fix up PSR frontbuffer tracking

[Daniel Vetter]

I've tried to split this up, but all the changes are so tightly
related that I didn't find a good way to do this without breaking
bisecting. Essentially this completely changes how psr is glued into
the overall driver, and there's not much you can do to soften such a
paradigm change.

- Use frontbuffer tracking bits stuff to separate disable and
  re-enable.

- Don't re-check everything in the psr work. We have now accurate
  tracking for everything, so no need to check for sprites or tiling
  really. Allows us to ditch tons of locks.

- That in turn allows us to properly cancel the work in the disable
  function - no more deadlocks.

- Add a check for HSW sprites and force a flush. Apparently the
  hardware doesn't forward the flushing when updating the sprite base
  address. We can do the same trick everywhere else we have such
  issues, e.g. on baytrail with ... everything.

- Don't re-enable psr with a delay in psr_exit. It really must be
  turned off forever if we detect a gtt write. At least with the
  current frontbuffer render tracking. Userspace can do a busy ioctl
  call or no-op pageflip to re-enable psr.

- Drop redundant checks for crtc and crtc->active - now that they're
  only called from enable this is guaranteed.

- Fix up the hsw port check. eDP can also happen on port D, but the
  issue is exactly that it doesn't work there. So an || check is
  wrong.

- We still schedule the psr work with a delay. The frontbuffer
  flushing interface mandates that we upload the next full frame, so
  need to wait a bit. Once we have single-shot frame uploads we can do
  better here.

v2: Don't enable psr initially, rely upon the fb flush of the initial
plane setup for that. Gives us more unified code flow and makes the
crtc enable sequence less a special case.

v3: s/psr_exit/psr_invalidate/ for consistency

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 30c2ae3cbab7..ea69925e0df5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -641,6 +641,7 @@ struct i915_psr {
 	struct intel_dp *enabled;
 	bool active;
 	struct delayed_work work;
+	unsigned busy_frontbuffer_bits;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dfdbf2a02844..9cfc6d003f7a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8898,7 +8898,7 @@ void intel_fb_invalidate(struct drm_i915_gem_object *obj,
 
 	intel_mark_fb_busy(dev, obj->frontbuffer_bits, ring);
 
-	intel_edp_psr_exit(dev);
+	intel_edp_psr_invalidate(dev, obj->frontbuffer_bits);
 }
 
 /**
@@ -8924,7 +8924,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
 
 	intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
 
-	intel_edp_psr_exit(dev);
+	intel_edp_psr_flush(dev, frontbuffer_bits);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 13c3ec7ec451..50477262b76e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1746,8 +1746,6 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = dig_port->base.base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct drm_i915_gem_object *obj = to_intel_framebuffer(crtc->primary->fb)->obj;
-	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 
 	lockdep_assert_held(&dev_priv->psr.lock);
 	lockdep_assert_held(&dev->struct_mutex);
@@ -1761,8 +1759,7 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 		return false;
 	}
 
-	if (IS_HASWELL(dev) && (intel_encoder->type != INTEL_OUTPUT_EDP ||
-				dig_port->port != PORT_A)) {
+	if (IS_HASWELL(dev) && dig_port->port != PORT_A) {
 		DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
 		return false;
 	}
@@ -1772,34 +1769,10 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 		return false;
 	}
 
-	crtc = dig_port->base.base.crtc;
-	if (crtc == NULL) {
-		DRM_DEBUG_KMS("crtc not active for PSR\n");
-		return false;
-	}
-
-	intel_crtc = to_intel_crtc(crtc);
-	if (!intel_crtc_active(crtc)) {
-		DRM_DEBUG_KMS("crtc not active for PSR\n");
-		return false;
-	}
-
-	obj = to_intel_framebuffer(crtc->primary->fb)->obj;
-	if (obj->tiling_mode != I915_TILING_X ||
-	    obj->fence_reg == I915_FENCE_REG_NONE) {
-		DRM_DEBUG_KMS("PSR condition failed: fb not tiled or fenced\n");
-		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;
-	}
-
 	if (I915_READ(HSW_STEREO_3D_CTL(intel_crtc->config.cpu_transcoder)) &
 	    S3D_ENABLE) {
 		DRM_DEBUG_KMS("PSR condition failed: Stereo 3D is Enabled\n");
@@ -1832,7 +1805,6 @@ 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 = intel_dp;
 	dev_priv->psr.active = true;
 }
 
@@ -1858,11 +1830,13 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 		return;
 	}
 
+	dev_priv->psr.busy_frontbuffer_bits = 0;
+
 	/* Setup PSR once */
 	intel_edp_psr_setup(intel_dp);
 
 	if (intel_edp_psr_match_conditions(intel_dp))
-		intel_edp_psr_do_enable(intel_dp);
+		dev_priv->psr.enabled = intel_dp;
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
@@ -1893,39 +1867,34 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 
 	dev_priv->psr.enabled = NULL;
 	mutex_unlock(&dev_priv->psr.lock);
+
+	cancel_delayed_work_sync(&dev_priv->psr.work);
 }
 
 static 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_dp *intel_dp = dev_priv->psr.enabled;
 
-	drm_modeset_lock_all(dev);
-	mutex_lock(&dev->struct_mutex);
 	mutex_lock(&dev_priv->psr.lock);
 	intel_dp = dev_priv->psr.enabled;
 
 	if (!intel_dp)
 		goto unlock;
 
-	if (intel_edp_psr_match_conditions(intel_dp))
-		intel_edp_psr_do_enable(intel_dp);
+	if (dev_priv->psr.busy_frontbuffer_bits)
+		goto unlock;
+
+	intel_edp_psr_do_enable(intel_dp);
 unlock:
 	mutex_unlock(&dev_priv->psr.lock);
-	mutex_unlock(&dev->struct_mutex);
-	drm_modeset_unlock_all(dev);
 }
 
-void intel_edp_psr_exit(struct drm_device *dev)
+static void intel_edp_psr_do_exit(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!HAS_PSR(dev))
-		return;
-
-	mutex_lock(&dev_priv->psr.lock);
 	if (dev_priv->psr.active) {
 		u32 val = I915_READ(EDP_PSR_CTL(dev));
 
@@ -1936,8 +1905,54 @@ void intel_edp_psr_exit(struct drm_device *dev)
 		dev_priv->psr.active = false;
 	}
 
-	schedule_delayed_work(&dev_priv->psr.work,
-			      msecs_to_jiffies(100));
+}
+
+void intel_edp_psr_invalidate(struct drm_device *dev,
+			      unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+	enum pipe pipe;
+
+	if (!HAS_PSR(dev))
+		return;
+
+
+	mutex_lock(&dev_priv->psr.lock);
+	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
+	pipe = to_intel_crtc(crtc)->pipe;
+
+	intel_edp_psr_do_exit(dev);
+
+	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
+
+	dev_priv->psr.busy_frontbuffer_bits |= frontbuffer_bits;
+	mutex_unlock(&dev_priv->psr.lock);
+}
+
+void intel_edp_psr_flush(struct drm_device *dev,
+			 unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+	enum pipe pipe;
+
+	if (!HAS_PSR(dev))
+		return;
+
+
+	mutex_lock(&dev_priv->psr.lock);
+	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
+	pipe = to_intel_crtc(crtc)->pipe;
+	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
+
+	if (IS_HASWELL(dev) &&
+	    (frontbuffer_bits & INTEL_FRONTBUFFER_SPRITE(pipe)))
+		intel_edp_psr_do_exit(dev);
+
+	if (!dev_priv->psr.busy_frontbuffer_bits)
+		schedule_delayed_work(&dev_priv->psr.work,
+				      msecs_to_jiffies(100));
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 41776bfeddbd..305f07b3c99b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -835,7 +835,10 @@ void intel_edp_panel_off(struct intel_dp *intel_dp);
 void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
-void intel_edp_psr_exit(struct drm_device *dev);
+void intel_edp_psr_invalidate(struct drm_device *dev,
+			      unsigned frontbuffer_bits);
+void intel_edp_psr_flush(struct drm_device *dev,
+			 unsigned frontbuffer_bits);
 void intel_edp_psr_init(struct drm_device *dev);
 
 
-- 
2.0.0

Re: [PATCH 15/15] drm/i915: Fix up PSR frontbuffer tracking

[Chris Wilson]

On Mon, Jun 16, 2014 at 07:51:35PM +0200, Daniel Vetter wrote:
>  static 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_dp *intel_dp = dev_priv->psr.enabled;
>  
> -	drm_modeset_lock_all(dev);
> -	mutex_lock(&dev->struct_mutex);
>  	mutex_lock(&dev_priv->psr.lock);
>  	intel_dp = dev_priv->psr.enabled;
>  
>  	if (!intel_dp)
>  		goto unlock;
>  
> -	if (intel_edp_psr_match_conditions(intel_dp))
> -		intel_edp_psr_do_enable(intel_dp);
> +	if (dev_priv->psr.busy_frontbuffer_bits)
> +		goto unlock;

Hmm, I requeued the work item out of paranoia. But I think a comment
here about how invalidate *will* be called again if any of the
busy_frontbuffer_bits change and so the work will be requeued on the
next update.

> +
> +	intel_edp_psr_do_enable(intel_dp);
>  unlock:
>  	mutex_unlock(&dev_priv->psr.lock);
> -	mutex_unlock(&dev->struct_mutex);
> -	drm_modeset_unlock_all(dev);
>  }
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 15/15] drm/i915: Fix up PSR frontbuffer tracking

[Daniel Vetter]

On Tue, Jun 17, 2014 at 08:02:27AM +0100, Chris Wilson wrote:
> On Mon, Jun 16, 2014 at 07:51:35PM +0200, Daniel Vetter wrote:
> >  static 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_dp *intel_dp = dev_priv->psr.enabled;
> >  
> > -	drm_modeset_lock_all(dev);
> > -	mutex_lock(&dev->struct_mutex);
> >  	mutex_lock(&dev_priv->psr.lock);
> >  	intel_dp = dev_priv->psr.enabled;
> >  
> >  	if (!intel_dp)
> >  		goto unlock;
> >  
> > -	if (intel_edp_psr_match_conditions(intel_dp))
> > -		intel_edp_psr_do_enable(intel_dp);
> > +	if (dev_priv->psr.busy_frontbuffer_bits)
> > +		goto unlock;
> 
> Hmm, I requeued the work item out of paranoia. But I think a comment
> here about how invalidate *will* be called again if any of the
> busy_frontbuffer_bits change and so the work will be requeued on the
> next update.

Yeah this deserves a comment. It only happens if invalidate was called
since the flush that queued the work. Eventually a new flush will happen,
but since we're seeing a non-NULL busy bits that didn't happen yet. And
because flush clears the bits first before rescheduling we're guaranteed
to not miss it.
-Daniel
> 
> > +
> > +	intel_edp_psr_do_enable(intel_dp);
> >  unlock:
> >  	mutex_unlock(&dev_priv->psr.lock);
> > -	mutex_unlock(&dev->struct_mutex);
> > -	drm_modeset_unlock_all(dev);
> >  }
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> 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 15/15] drm/i915: Fix up PSR frontbuffer tracking

[Chris Wilson]

On Mon, Jun 16, 2014 at 07:51:35PM +0200, Daniel Vetter wrote:
> +void intel_edp_psr_invalidate(struct drm_device *dev,
> +			      unsigned frontbuffer_bits)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	enum pipe pipe;
> +
> +	if (!HAS_PSR(dev))
> +		return;

if (!psr.enabled)
   return;

> +
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +
> +	intel_edp_psr_do_exit(dev);
> +
> +	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
> +
> +	dev_priv->psr.busy_frontbuffer_bits |= frontbuffer_bits;
> +	mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> +void intel_edp_psr_flush(struct drm_device *dev,
> +			 unsigned frontbuffer_bits)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	enum pipe pipe;
> +
> +	if (!HAS_PSR(dev))
> +		return;

if (!psr.enabled)
   return;

> +
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
> +
> +	if (IS_HASWELL(dev) &&
> +	    (frontbuffer_bits & INTEL_FRONTBUFFER_SPRITE(pipe)))
> +		intel_edp_psr_do_exit(dev);

This deserves comment explaining/referencing the discrepancy.

> +	if (!dev_priv->psr.busy_frontbuffer_bits)
> +		schedule_delayed_work(&dev_priv->psr.work,
> +				      msecs_to_jiffies(100));

100ms made sense when queueing the task at the start of the write cycle,
but on flush, it only wants to be ~2frames. As we know we have an active
pipe here we could queue it for exactly N frames...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 15/15] drm/i915: Fix up PSR frontbuffer tracking

[Daniel Vetter]

On Tue, Jun 17, 2014 at 08:07:28AM +0100, Chris Wilson wrote:
> On Mon, Jun 16, 2014 at 07:51:35PM +0200, Daniel Vetter wrote:
> > +void intel_edp_psr_invalidate(struct drm_device *dev,
> > +			      unsigned frontbuffer_bits)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_crtc *crtc;
> > +	enum pipe pipe;
> > +
> > +	if (!HAS_PSR(dev))
> > +		return;
> 
> if (!psr.enabled)
>    return;

Already fixed.
> 
> > +
> > +
> > +	mutex_lock(&dev_priv->psr.lock);
> > +	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
> > +	pipe = to_intel_crtc(crtc)->pipe;
> > +
> > +	intel_edp_psr_do_exit(dev);
> > +
> > +	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
> > +
> > +	dev_priv->psr.busy_frontbuffer_bits |= frontbuffer_bits;
> > +	mutex_unlock(&dev_priv->psr.lock);
> > +}
> > +
> > +void intel_edp_psr_flush(struct drm_device *dev,
> > +			 unsigned frontbuffer_bits)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_crtc *crtc;
> > +	enum pipe pipe;
> > +
> > +	if (!HAS_PSR(dev))
> > +		return;
> 
> if (!psr.enabled)
>    return;
> 
> > +
> > +
> > +	mutex_lock(&dev_priv->psr.lock);
> > +	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
> > +	pipe = to_intel_crtc(crtc)->pipe;
> > +	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
> > +
> > +	if (IS_HASWELL(dev) &&
> > +	    (frontbuffer_bits & INTEL_FRONTBUFFER_SPRITE(pipe)))
> > +		intel_edp_psr_do_exit(dev);
> 
> This deserves comment explaining/referencing the discrepancy.

Agreed.
> 
> > +	if (!dev_priv->psr.busy_frontbuffer_bits)
> > +		schedule_delayed_work(&dev_priv->psr.work,
> > +				      msecs_to_jiffies(100));
> 
> 100ms made sense when queueing the task at the start of the write cycle,
> but on flush, it only wants to be ~2frames. As we know we have an active
> pipe here we could queue it for exactly N frames...

Well I don't have real hw to test so figured I'll leave the actual hw
touching code unchanged. What we really want is a function which uploads 1
final frame (starting from the next vblank) and then goes into psr
immediately. But getting there is a 2nd step.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 00/15] Accurate frontbuffer tracking and psr conversion

[Chris Wilson]

On Mon, Jun 16, 2014 at 07:51:20PM +0200, Daniel Vetter wrote:
> Note that currently the gtt tracking has a bit a gap: We never exit it. Bunch of
> fixes are possible:
>
> - Wire up the core dirty_fb ioctl to flush framebuffers. This is used by generic
>   userspace to flush dummy buffers, which in our case means gtt mmaps. So maps
>   perfectly.
> 
> - Do a delayed gtt pte teardown and force-flush. Probably too ugly to care
>   really.

I don't consider that to be a gap, but accurate reflection of how
userspace is using the GTT. The most important usecase in this regard is
perhaps how the scanout is permenantly mapped for writing after a
terminal GPU hang. Trying to be smart in the kernel is more likely to
upset userspace, especially in this last resort effort at keeping
everything running.

> - Try to integrate hw gtt write tracking logic. Note that current psr code
>   doesn't rely on this - I've killed the X-tiled checks completely.

Also probably not worth it. In the normal sporadic use, we can rely on
the GTT being flushed as required.

Note, that the tracking as proposed will unfortunately get fbcon wrong
after it is touched by X. I think however there is some merit in
improving how we handle fbcon here.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 00/15] Accurate frontbuffer tracking and psr conversion

[Daniel Vetter]

On Mon, Jun 16, 2014 at 9:37 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> - Try to integrate hw gtt write tracking logic. Note that current psr code
>>   doesn't rely on this - I've killed the X-tiled checks completely.
>
> Also probably not worth it. In the normal sporadic use, we can rely on
> the GTT being flushed as required.
>
> Note, that the tracking as proposed will unfortunately get fbcon wrong
> after it is touched by X. I think however there is some merit in
> improving how we handle fbcon here.

Where would that go wrong? After every set_par we do a
set_to_gtt_domain, I've hoped that would be good enough to get fbcon
into shape and make sure the fbcon fb is in the right domain (i.e.
permanently invalidated) again?

Or does fbcon not do a set_par when X drops the vt back into text
mode? I didn't check that ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 00/15] Accurate frontbuffer tracking and psr conversion

[Chris Wilson]

On Mon, Jun 16, 2014 at 07:51:20PM +0200, Daniel Vetter wrote:
> Hi all,
> 
> This patch series adds accurate frontbuffer tracking to i915 and then converts
> psr over to use. Bunch of things still need to be done.
> 
> - PSR needs to be re-tested. I lack the hardware for that. The frontbuffer
>   tracking itself is tested.
> 
> - PSR igt testcase needs to be extended so that we cover all upload methods on
>   all plane types.
> 
> - DRRS/downclocking needs to be unified and put into this framework properly.
>   Also needs proper locking for all of the DRRS state.
> 
> - fbc also needs to be fixed up, with state handling properly split between
>   crtc_enable/disable, primary fb updates and the fb tracking for nuking. A bit
>   unclear how we want to integrate gtt cpu write tracking through the hw, since
>   that seems to be the hw tracking piece that actually works.
> 
> General blueprint for display runtime power saving features:
> - Have all your state in either intel_crtc or dev_priv, protected by its own
>   lock.
> 
> - Do state setup in crtc_enable, cleanup in crtc_disable with a pair of
>   enable/disable fuctions. Optionally update everywhere else you want (e.g.
>   primary plane updates for fbc). Not state setup anywhere else allowed, except
>   maybe an _init for setting up work items, locks, ...
> 
> - Intercept the invalidation/flush signals you need like
>   psr_invalidate/psr_flush. Track internally which frontbuffer bits you're
>   interested in and invalidate/flush accordingly. You can also use these for
>   workarounds (e.g. on hsw we force-flush for sprite changes since the flip
>   signal doesn't result in a hw image upload).
> 
> 
> Note that currently the gtt tracking has a bit a gap: We never exit it. Bunch of
> fixes are possible:
> - Wire up the core dirty_fb ioctl to flush framebuffers. This is used by generic
>   userspace to flush dummy buffers, which in our case means gtt mmaps. So maps
>   perfectly.
> 
> - Do a delayed gtt pte teardown and force-flush. Probably too ugly to care
>   really.
> 
> - Try to integrate hw gtt write tracking logic. Note that current psr code
>   doesn't rely on this - I've killed the X-tiled checks completely.
> 
> Big thanks to Chris for some great ideas for the fb tracking scheme and the
> precise placement of the invalidate/flush functions.
> 
> Comments, flames and especially testing on psr hardware highly welcome.

I like it. I think this is a big step in the right direction, and
doesn't look too intrusive to normal operations. I would like to get
Ville's FBC tracking on top so we check for any idiosynacracies we may
have missed.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 00/15] Accurate frontbuffer tracking and psr conversion

[Daniel Vetter]

On Tue, Jun 17, 2014 at 08:09:47AM +0100, Chris Wilson wrote:
> On Mon, Jun 16, 2014 at 07:51:20PM +0200, Daniel Vetter wrote:
> > Hi all,
> > 
> > This patch series adds accurate frontbuffer tracking to i915 and then converts
> > psr over to use. Bunch of things still need to be done.
> > 
> > - PSR needs to be re-tested. I lack the hardware for that. The frontbuffer
> >   tracking itself is tested.
> > 
> > - PSR igt testcase needs to be extended so that we cover all upload methods on
> >   all plane types.
> > 
> > - DRRS/downclocking needs to be unified and put into this framework properly.
> >   Also needs proper locking for all of the DRRS state.
> > 
> > - fbc also needs to be fixed up, with state handling properly split between
> >   crtc_enable/disable, primary fb updates and the fb tracking for nuking. A bit
> >   unclear how we want to integrate gtt cpu write tracking through the hw, since
> >   that seems to be the hw tracking piece that actually works.
> > 
> > General blueprint for display runtime power saving features:
> > - Have all your state in either intel_crtc or dev_priv, protected by its own
> >   lock.
> > 
> > - Do state setup in crtc_enable, cleanup in crtc_disable with a pair of
> >   enable/disable fuctions. Optionally update everywhere else you want (e.g.
> >   primary plane updates for fbc). Not state setup anywhere else allowed, except
> >   maybe an _init for setting up work items, locks, ...
> > 
> > - Intercept the invalidation/flush signals you need like
> >   psr_invalidate/psr_flush. Track internally which frontbuffer bits you're
> >   interested in and invalidate/flush accordingly. You can also use these for
> >   workarounds (e.g. on hsw we force-flush for sprite changes since the flip
> >   signal doesn't result in a hw image upload).
> > 
> > 
> > Note that currently the gtt tracking has a bit a gap: We never exit it. Bunch of
> > fixes are possible:
> > - Wire up the core dirty_fb ioctl to flush framebuffers. This is used by generic
> >   userspace to flush dummy buffers, which in our case means gtt mmaps. So maps
> >   perfectly.
> > 
> > - Do a delayed gtt pte teardown and force-flush. Probably too ugly to care
> >   really.
> > 
> > - Try to integrate hw gtt write tracking logic. Note that current psr code
> >   doesn't rely on this - I've killed the X-tiled checks completely.
> > 
> > Big thanks to Chris for some great ideas for the fb tracking scheme and the
> > precise placement of the invalidate/flush functions.
> > 
> > Comments, flames and especially testing on psr hardware highly welcome.
> 
> I like it. I think this is a big step in the right direction, and
> doesn't look too intrusive to normal operations. I would like to get
> Ville's FBC tracking on top so we check for any idiosynacracies we may
> have missed.

My plan is to check out how this works with byt psr (since the hw tracking
there is completely broken) and then get this all in. More polish might be
needed on top for fbc, but fixing fbc will be a lot of work (and Ville's
going on summer vacation soonish). Also with that we could fix up fbc and
drrs in parallel.

I hope that byt psr is a nasty enough case to validate the framework
overall.

Anyway, tons of changes in the patches from Chris' and Rodrigo's review
plus fixing a bunch of things when debugging psr with Rodrigo yesterday.
Unfortunately the psr sink crc tests are broken currently (for unrelated
reasons) so the verdict's still out on whether it works.

New patch series pushed to

http://cgit.freedesktop.org/~danvet/drm/log/?h=psr-stuff

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