[PATCH 00/31] IPS/DRRS/PSR rework with PSR enabled by default

[PATCH 00/31] IPS/DRRS/PSR rework with PSR enabled by default

[Rodrigo Vivi]

Hi all,

The goal of this series is to Enable PSR by default, but in order to do
that first it was necessary to fix few display PM initialization that was
broken after fastboot got enabled by default. Also it was needed to make
sink crc more reliable.

So, divided by topic:

Patches 1-7: IPS reorg/rework
Patches 8-16: DRRS reorg and PSR and DRRS initialization rework.
Patches 17-26: PSR Fixes
Patches 27-30: Let Sink CRC more reliable
Patch 31: Enable PSR by default.

--- Why all in one single series ---

I didn't split the series in different series because there were dependencies
and because they were touching same part of the code. So the rebase in order
is less painful for me than if the patches started getting merged out of order.

--- Why sysfs toggles are here ---

There is a work in progress to add more sysfs interface but with the requirement
to let them organized and documented. But this sysfs toggles were helping me
during debugs and testings a lot so I decided to send them out even before we
finish the design review on the sysfs interfaces etc.
They also help to justify the ips mutex locks. ;)
But I totally understand if the aren't merged at this point.

--- Remaining Known issues ---

1. Features disabled after suspend: There is also a remaining known issue that
is the initilization of these features now rely on the atomic modeset and the
atomic modeset on resume is commiting a zeroed state so the features gets
disabled with suspend/resume and stays disabled until the next full modeset.
Anyway let's start the review and get this
merged and work to fix atomic commit after suspend/resume in parallel.

2. Automated tests as is in this series stil fail on SKL because the aux failure.
There is aux retries patches on the mailing list to work around this issue and
also an investigation in progress to see if aux mutex could help here.

3. Few kms_frontbuffer tests are failing even with PSR disabled so I'm just ignoring
psr-1p-primscrn-cur-indfb-* test results for now.


--- Validated Hardware ---

manual checks, kms_psr_sink_crc and kms_frontbuffer_tracking were used for validation
on different platforms with different kinds of panels:
- Lenovo Yoga 3 (BDW)
- Lenovo T450s (BDW)
- BDW SDP GT3
- BDW SDP ULT
- VLV/BYT
- SKL
- KBL

So I'm confident we can enable PSR back by default now.

All comments, ideas, suggestions and even bikesheddings are pretty welcome.

Thanks,
Rodrigo.

Rodrigo Vivi (31):
  drm/i915: Rename IPS ready variable at pipe config.
  drm/i915: Move IPS related stuff to intel_ips.c
  drm/i915: Add IPS DockBook.
  drm/i915: Handle actual IPS enabled state.
  drm/i915: Fix IPS initialization.
  drm/i915: Fix IPS disable sequence.
  drm/i915: IPS Sysfs interface.
  drm/i915: Add psr_ready on pipe_config
  drm/i915: Only enable DRRS if PSR won't be enabled on this pipe.
  drm/i915: Detatch i915.enable_psr from psr_ready
  drm/i915: Use intel_crtc instead of intel_dp on PSR enable/disable
    functions.
  drm/i915: Fix PSR initialization.
  drm/i915: Organize Makefile new display pm group
  drm/i915: Create intel_drrs.c
  drm/i915: Use intel_crtc instead of intel_dp on DRRS enable/disable
    functions.
  drm/i915: Fix DRRS initialization.
  drm/i915: Add sys PSR toggle interface.
  drm/i915: Force PSR exit when IRQ_HPD is detected on eDP.
  drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink.
  drm/i915: PSR: Let's rely more on frontbuffer tracking.
  drm/i915: PSR: Mask LPSP hw tracking back again.
  drm/i915: Delay first PSR activation.
  drm/i915: Reduce PSR re-activation time for VLV/CHV.
  drm/i915: PSR: Don't Skip aux handshake on DP_PSR_NO_TRAIN_ON_EXIT.
  drm/i915: Send TP1 TP2/3 even when panel claims no NO_TRAIN_ON_EXIT.
  drm/i915: Fix idle_frames counter.
  drm/i915: Allow 1 vblank to let Sink CRC calculation to start or stop.
  drm/i915: Make Sink crc calculation waiting for counter to reset.
  drm/i915: Stop tracking last calculated Sink CRC.
  drm/i915: Rely on TEST_SINK_START instead of tracking Sink CRC state
    on dev_priv.
  drm/i915: Enable PSR by default.

 Documentation/DocBook/gpu.tmpl           |   5 +
 drivers/gpu/drm/i915/Makefile            |   8 +-
 drivers/gpu/drm/i915/i915_debugfs.c      |  16 +-
 drivers/gpu/drm/i915/i915_drv.h          |   9 +-
 drivers/gpu/drm/i915/i915_params.c       |   4 +-
 drivers/gpu/drm/i915/i915_sysfs.c        | 165 +++++++++++
 drivers/gpu/drm/i915/intel_ddi.c         |   6 +-
 drivers/gpu/drm/i915/intel_display.c     | 190 +++---------
 drivers/gpu/drm/i915/intel_dp.c          | 477 +++----------------------------
 drivers/gpu/drm/i915/intel_drrs.c        | 416 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h         |  47 +--
 drivers/gpu/drm/i915/intel_frontbuffer.c |   4 +-
 drivers/gpu/drm/i915/intel_ips.c         | 259 +++++++++++++++++
 drivers/gpu/drm/i915/intel_psr.c         | 201 ++++++++-----
 14 files changed, 1116 insertions(+), 691 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_drrs.c
 create mode 100644 drivers/gpu/drm/i915/intel_ips.c

-- 
2.4.3

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

[PATCH 01/31] drm/i915: Rename IPS ready variable at pipe config.

[Rodrigo Vivi]

This variable mostly doesn't reflect IPS enabled status
but its readiness.

This patch removes the only place where it was actually meaning ips
enabled, at crtc_load_lut workaround for Haswell only.
The actuall enabled will be properly implemented in a following patch.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 19 +++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0811238..01da1c9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4534,7 +4534,7 @@ void hsw_enable_ips(struct intel_crtc *crtc)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!crtc->config->ips_enabled)
+	if (!crtc->config->ips_ready)
 		return;
 
 	/* We can only enable IPS after we enable a plane and wait for a vblank */
@@ -4567,7 +4567,7 @@ void hsw_disable_ips(struct intel_crtc *crtc)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!crtc->config->ips_enabled)
+	if (!crtc->config->ips_ready)
 		return;
 
 	assert_plane_enabled(dev_priv, crtc->plane);
@@ -4611,7 +4611,8 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
 	/* Workaround : Do not read or write the pipe palette/gamma data while
 	 * GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
 	 */
-	if (IS_HASWELL(dev) && intel_crtc->config->ips_enabled &&
+	/* FIXME: This should be ips_enabled */
+	if (IS_HASWELL(dev) && intel_crtc->config->ips_ready &&
 	    ((I915_READ(GAMMA_MODE(pipe)) & GAMMA_MODE_MODE_MASK) ==
 	     GAMMA_MODE_MODE_SPLIT)) {
 		hsw_disable_ips(intel_crtc);
@@ -6558,7 +6559,7 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc,
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	pipe_config->ips_enabled = i915.enable_ips &&
+	pipe_config->ips_ready = i915.enable_ips &&
 		hsw_crtc_supports_ips(crtc) &&
 		pipe_config_supports_ips(dev_priv, pipe_config);
 }
@@ -9511,7 +9512,7 @@ static int ilk_max_pixel_rate(struct drm_atomic_state *state)
 		pixel_rate = ilk_pipe_pixel_rate(crtc_state);
 
 		/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
-		if (IS_BROADWELL(state->dev) && crtc_state->ips_enabled)
+		if (IS_BROADWELL(state->dev) && crtc_state->ips_ready)
 			pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
 
 		max_pixel_rate = max(max_pixel_rate, pixel_rate);
@@ -9830,9 +9831,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 			ironlake_get_pfit_config(crtc, pipe_config);
 	}
 
-	if (IS_HASWELL(dev))
-		pipe_config->ips_enabled = hsw_crtc_supports_ips(crtc) &&
-			(I915_READ(IPS_CTL) & IPS_ENABLE);
+	pipe_config->ips_ready = hsw_crtc_supports_ips(crtc);
 
 	if (pipe_config->cpu_transcoder != TRANSCODER_EDP) {
 		pipe_config->pixel_multiplier =
@@ -12001,7 +12000,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 		      pipe_config->pch_pfit.pos,
 		      pipe_config->pch_pfit.size,
 		      pipe_config->pch_pfit.enabled ? "enabled" : "disabled");
-	DRM_DEBUG_KMS("ips: %i\n", pipe_config->ips_enabled);
+	DRM_DEBUG_KMS("ips ready: %i\n", pipe_config->ips_ready);
 	DRM_DEBUG_KMS("double wide: %i\n", pipe_config->double_wide);
 
 	if (IS_BROXTON(dev)) {
@@ -12553,7 +12552,7 @@ intel_pipe_config_compare(struct drm_device *dev,
 
 	/* BDW+ don't expose a synchronous way to read the state */
 	if (IS_HASWELL(dev))
-		PIPE_CONF_CHECK_I(ips_enabled);
+		PIPE_CONF_CHECK_I(ips_ready);
 
 	PIPE_CONF_CHECK_I(double_wide);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 92518b4..2aae219 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -476,7 +476,7 @@ struct intel_crtc_state {
 	int fdi_lanes;
 	struct intel_link_m_n fdi_m_n;
 
-	bool ips_enabled;
+	bool ips_ready;
 
 	bool double_wide;
 
-- 
2.4.3

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

[PATCH 02/31] drm/i915: Move IPS related stuff to intel_ips.c

[Rodrigo Vivi]

We need to organize ips a bit in order to fix it initialization
So let's start by following other features by moving all relted
functions to its own file.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/Makefile        |   1 +
 drivers/gpu/drm/i915/i915_debugfs.c  |   4 +-
 drivers/gpu/drm/i915/intel_display.c | 127 ++---------------------------
 drivers/gpu/drm/i915/intel_dp.c      |   6 +-
 drivers/gpu/drm/i915/intel_drv.h     |   8 +-
 drivers/gpu/drm/i915/intel_ips.c     | 151 +++++++++++++++++++++++++++++++++++
 6 files changed, 170 insertions(+), 127 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_ips.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 0851de07..7c1a86e 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -60,6 +60,7 @@ i915-y += intel_audio.o \
 	  intel_fifo_underrun.o \
 	  intel_frontbuffer.o \
 	  intel_hotplug.o \
+	  intel_ips.o \
 	  intel_modes.o \
 	  intel_overlay.o \
 	  intel_psr.o \
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5659d4c..391861d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4034,7 +4034,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		 * user space can't make reliable use of the CRCs, so let's just
 		 * completely disable it.
 		 */
-		hsw_disable_ips(crtc);
+		intel_ips_disable(crtc);
 
 		spin_lock_irq(&pipe_crc->lock);
 		kfree(pipe_crc->entries);
@@ -4079,7 +4079,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		else if (IS_HASWELL(dev) && pipe == PIPE_A)
 			hsw_trans_edp_pipe_A_crc_wa(dev, false);
 
-		hsw_enable_ips(crtc);
+		intel_ips_disable(crtc);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 01da1c9..197c608 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1366,22 +1366,6 @@ void assert_pipe(struct drm_i915_private *dev_priv,
 	     pipe_name(pipe), state_string(state), state_string(cur_state));
 }
 
-static void assert_plane(struct drm_i915_private *dev_priv,
-			 enum plane plane, bool state)
-{
-	u32 val;
-	bool cur_state;
-
-	val = I915_READ(DSPCNTR(plane));
-	cur_state = !!(val & DISPLAY_PLANE_ENABLE);
-	I915_STATE_WARN(cur_state != state,
-	     "plane %c assertion failure (expected %s, current %s)\n",
-	     plane_name(plane), state_string(state), state_string(cur_state));
-}
-
-#define assert_plane_enabled(d, p) assert_plane(d, p, true)
-#define assert_plane_disabled(d, p) assert_plane(d, p, false)
-
 static void assert_planes_disabled(struct drm_i915_private *dev_priv,
 				   enum pipe pipe)
 {
@@ -4529,64 +4513,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
 	}
 }
 
-void hsw_enable_ips(struct intel_crtc *crtc)
-{
-	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (!crtc->config->ips_ready)
-		return;
-
-	/* We can only enable IPS after we enable a plane and wait for a vblank */
-	intel_wait_for_vblank(dev, crtc->pipe);
-
-	assert_plane_enabled(dev_priv, crtc->plane);
-	if (IS_BROADWELL(dev)) {
-		mutex_lock(&dev_priv->rps.hw_lock);
-		WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL, 0xc0000000));
-		mutex_unlock(&dev_priv->rps.hw_lock);
-		/* Quoting Art Runyan: "its not safe to expect any particular
-		 * value in IPS_CTL bit 31 after enabling IPS through the
-		 * mailbox." Moreover, the mailbox may return a bogus state,
-		 * so we need to just enable it and continue on.
-		 */
-	} else {
-		I915_WRITE(IPS_CTL, IPS_ENABLE);
-		/* The bit only becomes 1 in the next vblank, so this wait here
-		 * is essentially intel_wait_for_vblank. If we don't have this
-		 * and don't wait for vblanks until the end of crtc_enable, then
-		 * the HW state readout code will complain that the expected
-		 * IPS_CTL value is not the one we read. */
-		if (wait_for(I915_READ_NOTRACE(IPS_CTL) & IPS_ENABLE, 50))
-			DRM_ERROR("Timed out waiting for IPS enable\n");
-	}
-}
-
-void hsw_disable_ips(struct intel_crtc *crtc)
-{
-	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (!crtc->config->ips_ready)
-		return;
-
-	assert_plane_enabled(dev_priv, crtc->plane);
-	if (IS_BROADWELL(dev)) {
-		mutex_lock(&dev_priv->rps.hw_lock);
-		WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL, 0));
-		mutex_unlock(&dev_priv->rps.hw_lock);
-		/* wait for pcode to finish disabling IPS, which may take up to 42ms */
-		if (wait_for((I915_READ(IPS_CTL) & IPS_ENABLE) == 0, 42))
-			DRM_ERROR("Timed out waiting for IPS disable\n");
-	} else {
-		I915_WRITE(IPS_CTL, 0);
-		POSTING_READ(IPS_CTL);
-	}
-
-	/* We need to wait for a vblank before we can disable the plane. */
-	intel_wait_for_vblank(dev, crtc->pipe);
-}
-
 /** Loads the palette/gamma unit for the CRTC with the prepared values */
 static void intel_crtc_load_lut(struct drm_crtc *crtc)
 {
@@ -4615,7 +4541,7 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
 	if (IS_HASWELL(dev) && intel_crtc->config->ips_ready &&
 	    ((I915_READ(GAMMA_MODE(pipe)) & GAMMA_MODE_MODE_MASK) ==
 	     GAMMA_MODE_MODE_SPLIT)) {
-		hsw_disable_ips(intel_crtc);
+		intel_ips_disable(intel_crtc);
 		reenable_ips = true;
 	}
 
@@ -4634,7 +4560,7 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
 	}
 
 	if (reenable_ips)
-		hsw_enable_ips(intel_crtc);
+		intel_ips_enable(intel_crtc);
 }
 
 static void intel_crtc_dpms_overlay_disable(struct intel_crtc *intel_crtc)
@@ -4687,7 +4613,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	 * when going from primary only to sprite only and vice
 	 * versa.
 	 */
-	hsw_enable_ips(intel_crtc);
+	intel_ips_enable(intel_crtc);
 
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
@@ -4752,7 +4678,7 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 	 * when going from primary only to sprite only and vice
 	 * versa.
 	 */
-	hsw_disable_ips(intel_crtc);
+	intel_ips_disable(intel_crtc);
 }
 
 static void intel_post_plane_update(struct intel_crtc *crtc)
@@ -4791,7 +4717,7 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 		intel_fbc_disable_crtc(crtc);
 
 	if (crtc->atomic.disable_ips)
-		hsw_disable_ips(crtc);
+		intel_ips_disable(crtc);
 
 	if (atomic->pre_disable_primary)
 		intel_pre_disable_primary(&crtc->base);
@@ -4891,12 +4817,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 		cpt_verify_modeset(dev, intel_crtc->pipe);
 }
 
-/* IPS only exists on ULT machines and is tied to pipe A. */
-static bool hsw_crtc_supports_ips(struct intel_crtc *crtc)
-{
-	return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
-}
-
 static void haswell_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -6532,38 +6452,6 @@ retry:
 	return ret;
 }
 
-static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
-				     struct intel_crtc_state *pipe_config)
-{
-	if (pipe_config->pipe_bpp > 24)
-		return false;
-
-	/* HSW can handle pixel rate up to cdclk? */
-	if (IS_HASWELL(dev_priv->dev))
-		return true;
-
-	/*
-	 * We compare against max which means we must take
-	 * the increased cdclk requirement into account when
-	 * calculating the new cdclk.
-	 *
-	 * Should measure whether using a lower cdclk w/o IPS
-	 */
-	return ilk_pipe_pixel_rate(pipe_config) <=
-		dev_priv->max_cdclk_freq * 95 / 100;
-}
-
-static void hsw_compute_ips_config(struct intel_crtc *crtc,
-				   struct intel_crtc_state *pipe_config)
-{
-	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	pipe_config->ips_ready = i915.enable_ips &&
-		hsw_crtc_supports_ips(crtc) &&
-		pipe_config_supports_ips(dev_priv, pipe_config);
-}
-
 static int intel_crtc_compute_config(struct intel_crtc *crtc,
 				     struct intel_crtc_state *pipe_config)
 {
@@ -6609,8 +6497,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 		adjusted_mode->crtc_hsync_start == adjusted_mode->crtc_hdisplay)
 		return -EINVAL;
 
-	if (HAS_IPS(dev))
-		hsw_compute_ips_config(crtc, pipe_config);
+	pipe_config->ips_ready = intel_ips_ready(crtc, pipe_config);
 
 	if (pipe_config->has_pch_encoder)
 		return ironlake_fdi_compute_config(crtc, pipe_config);
@@ -9831,7 +9718,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 			ironlake_get_pfit_config(crtc, pipe_config);
 	}
 
-	pipe_config->ips_ready = hsw_crtc_supports_ips(crtc);
+	pipe_config->ips_ready = intel_ips_ready(crtc, pipe_config);
 
 	if (pipe_config->cpu_transcoder != TRANSCODER_EDP) {
 		pipe_config->pixel_multiplier =
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5264887..273e1ad 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3823,7 +3823,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 
 	intel_dp->sink_crc.started = false;
  out:
-	hsw_enable_ips(intel_crtc);
+	intel_ips_enable(intel_crtc);
 	return ret;
 }
 
@@ -3851,11 +3851,11 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
 		return -EIO;
 
-	hsw_disable_ips(intel_crtc);
+	intel_ips_disable(intel_crtc);
 
 	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
 			       buf | DP_TEST_SINK_START) < 0) {
-		hsw_enable_ips(intel_crtc);
+		intel_ips_enable(intel_crtc);
 		return -EIO;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2aae219..41a88f7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1188,8 +1188,6 @@ bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
 int chv_calc_dpll_params(int refclk, intel_clock_t *pll_clock);
 
 bool intel_crtc_active(struct drm_crtc *crtc);
-void hsw_enable_ips(struct intel_crtc *crtc);
-void hsw_disable_ips(struct intel_crtc *crtc);
 enum intel_display_power_domain
 intel_display_port_power_domain(struct intel_encoder *intel_encoder);
 void intel_mode_from_pipe_config(struct drm_display_mode *mode,
@@ -1207,6 +1205,12 @@ u32 skl_plane_ctl_format(uint32_t pixel_format);
 u32 skl_plane_ctl_tiling(uint64_t fb_modifier);
 u32 skl_plane_ctl_rotation(unsigned int rotation);
 
+/* intel_ips.c */
+bool intel_ips_ready(struct intel_crtc *crtc,
+		     struct intel_crtc_state *crtc_state);
+void intel_ips_enable(struct intel_crtc *crtc);
+void intel_ips_disable(struct intel_crtc *crtc);
+
 /* intel_csr.c */
 void intel_csr_ucode_init(struct drm_device *dev);
 enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_ips.c b/drivers/gpu/drm/i915/intel_ips.c
new file mode 100644
index 0000000..5c659a3
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_ips.c
@@ -0,0 +1,151 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+/* IPS only exists on ULT machines and is tied to pipe A. */
+
+#include "intel_drv.h"
+#include "i915_drv.h"
+
+
+static void assert_plane(struct drm_i915_private *dev_priv,
+			 enum plane plane, bool state)
+{
+	u32 val;
+	bool cur_state;
+
+	val = I915_READ(DSPCNTR(plane));
+	cur_state = !!(val & DISPLAY_PLANE_ENABLE);
+	I915_STATE_WARN(cur_state != state,
+			"plane %c assertion failure (expected %s, current %s)\n",
+			plane_name(plane), state ? "on" : "off",
+			cur_state ? "on" : "off");
+}
+
+#define assert_plane_enabled(d, p) assert_plane(d, p, true)
+#define assert_plane_disabled(d, p) assert_plane(d, p, false)
+
+static bool intel_ips_supported(struct intel_crtc *crtc)
+{
+	return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
+}
+
+static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
+				     struct intel_crtc_state *pipe_config)
+{
+	if (pipe_config->pipe_bpp > 24)
+		return false;
+
+	/* HSW can handle pixel rate up to cdclk? */
+	if (IS_HASWELL(dev_priv->dev))
+		return true;
+
+	/*
+	 * We compare against max which means we must take
+	 * the increased cdclk requirement into account when
+	 * calculating the new cdclk.
+	 *
+	 * Should measure whether using a lower cdclk w/o IPS
+	 */
+	return ilk_pipe_pixel_rate(pipe_config) <=
+	       dev_priv->max_cdclk_freq * 95 / 100;
+}
+
+bool intel_ips_ready(struct intel_crtc *crtc,
+		     struct intel_crtc_state *pipe_config)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	return HAS_IPS(dev) && i915.enable_ips &&
+	       intel_ips_supported(crtc) &&
+	       pipe_config_supports_ips(dev_priv, pipe_config);
+}
+
+void intel_ips_enable(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!crtc->config->ips_ready)
+		return;
+
+	/*
+	 * We can only enable IPS after we enable a plane
+	 * and wait for a vblank.
+	 */
+	intel_wait_for_vblank(dev, crtc->pipe);
+
+	assert_plane_enabled(dev_priv, crtc->plane);
+	if (IS_BROADWELL(dev)) {
+		mutex_lock(&dev_priv->rps.hw_lock);
+		WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL,
+						0xc0000000));
+		mutex_unlock(&dev_priv->rps.hw_lock);
+		/*
+		 * Quoting Art Runyan: "its not safe to expect any particular
+		 * value in IPS_CTL bit 31 after enabling IPS through the
+		 * mailbox." Moreover, the mailbox may return a bogus state,
+		 * so we need to just enable it and continue on.
+		 */
+	} else {
+		I915_WRITE(IPS_CTL, IPS_ENABLE);
+		/*
+		 * The bit only becomes 1 in the next vblank, so this wait here
+		 * is essentially intel_wait_for_vblank. If we don't have this
+		 * and don't wait for vblanks until the end of crtc_enable, then
+		 * the HW state readout code will complain that the expected
+		 * IPS_CTL value is not the one we read.
+		 */
+		if (wait_for(I915_READ_NOTRACE(IPS_CTL) & IPS_ENABLE, 50))
+			DRM_ERROR("Timed out waiting for IPS enable\n");
+	}
+}
+
+void intel_ips_disable(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!crtc->config->ips_ready)
+		return;
+
+	assert_plane_enabled(dev_priv, crtc->plane);
+	if (IS_BROADWELL(dev)) {
+		mutex_lock(&dev_priv->rps.hw_lock);
+		WARN_ON(sandybridge_pcode_write(dev_priv,
+						DISPLAY_IPS_CONTROL, 0));
+		mutex_unlock(&dev_priv->rps.hw_lock);
+		/*
+		 * Wait for pcode to finish disabling IPS,
+		 * which may take up to 42ms.
+		 */
+		if (wait_for((I915_READ(IPS_CTL) & IPS_ENABLE) == 0, 42))
+			DRM_ERROR("Timed out waiting for IPS disable\n");
+	} else {
+		I915_WRITE(IPS_CTL, 0);
+		POSTING_READ(IPS_CTL);
+	}
+
+	/* We need to wait for a vblank before we can disable the plane. */
+	intel_wait_for_vblank(dev, crtc->pipe);
+}
-- 
2.4.3

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

[PATCH 03/31] drm/i915: Add IPS DockBook.

[Rodrigo Vivi]

Intermediate Pixel Storage documentation.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 Documentation/DocBook/gpu.tmpl   |  5 +++++
 drivers/gpu/drm/i915/intel_ips.c | 37 ++++++++++++++++++++++++++++++++++---
 2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
index 54dfd28..e2e01cd 100644
--- a/Documentation/DocBook/gpu.tmpl
+++ b/Documentation/DocBook/gpu.tmpl
@@ -4002,6 +4002,11 @@ int num_ioctls;</synopsis>
 !Idrivers/gpu/drm/i915/intel_fbc.c
       </sect2>
       <sect2>
+	<title>Intermediate Pixel Storage (IPS)</title>
+!Pdrivers/gpu/drm/i915/intel_ips.c Intermediate Pixel Storage (IPS)
+!Idrivers/gpu/drm/i915/intel_ips.c
+      </sect2>
+      <sect2>
         <title>Display Refresh Rate Switching (DRRS)</title>
 !Pdrivers/gpu/drm/i915/intel_dp.c Display Refresh Rate Switching (DRRS)
 !Fdrivers/gpu/drm/i915/intel_dp.c intel_dp_set_drrs_state
diff --git a/drivers/gpu/drm/i915/intel_ips.c b/drivers/gpu/drm/i915/intel_ips.c
index 5c659a3..573bc31 100644
--- a/drivers/gpu/drm/i915/intel_ips.c
+++ b/drivers/gpu/drm/i915/intel_ips.c
@@ -21,12 +21,21 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
-/* IPS only exists on ULT machines and is tied to pipe A. */
-
 #include "intel_drv.h"
 #include "i915_drv.h"
 
-
+/**
+ * DOC: Intermediate Pixel Storage (IPS)
+ *
+ * Haswell-ULT and Broadwell have support to IPS feature where pixel information
+ * can be stored in a special buffer in display that can allow memory to get
+ * into deeper power states and spend more time in low power states
+ *
+ * IPS is only supported on Pipe A with pipe pixel format 8:8:8 or less
+ * and when pixel rate is lesser than 95% of the CDCLK frequency.
+ *
+ * IPS cannot be enabled on the pipe when there is no plane enabled.
+ */
 static void assert_plane(struct drm_i915_private *dev_priv,
 			 enum plane plane, bool state)
 {
@@ -70,6 +79,14 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
 	       dev_priv->max_cdclk_freq * 95 / 100;
 }
 
+/**
+ * intel_ips_ready - Is IPS Ready?
+ * @crtc: intel crtc
+ * @pipe_config: Pipe config
+ *
+ * This function is called to know if IPS is ready to be enabled
+ * on this pipe.
+ */
 bool intel_ips_ready(struct intel_crtc *crtc,
 		     struct intel_crtc_state *pipe_config)
 {
@@ -81,6 +98,13 @@ bool intel_ips_ready(struct intel_crtc *crtc,
 	       pipe_config_supports_ips(dev_priv, pipe_config);
 }
 
+/**
+ * intel_ips_enable - Enable IPS
+ * @crtc: intel crtc
+ *
+ * This function is called to enable IPS on certain pipe.
+ * All needed conditions should've checked already by intel_ips_ready.
+ */
 void intel_ips_enable(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
@@ -121,6 +145,13 @@ void intel_ips_enable(struct intel_crtc *crtc)
 	}
 }
 
+/**
+ * intel_ips_disable - Disable IPS
+ * @crtc: intel crtc
+ *
+ * This function is called to disable IPS on certain pipe whenever it is needed
+ * to disable IPS on the pipe.
+ */
 void intel_ips_disable(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
-- 
2.4.3

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

[PATCH 04/31] drm/i915: Handle actual IPS enabled state.

[Rodrigo Vivi]

With this we know if IPS is actually enabled.
It might not be activated on BDW since Hardware take
the decision and do its transition. However we have
the visibility of the state on our driver what we didn't
had until this patch. At least on BDW.

Since ips_ready means that ips will be enabled and ips_disable()
checks for the state of our enabled/disabled state machine
we can remove that FIXME that was there for crtc_load_lut
workaround for Haswell.

With this state machine and ips being disabled from
different places and many times when testcases with sink_crtc
for instance it is better to have it protected with its own mutex lock.
Ohterwise we cannot guarantee consitent ips.enabled state with the
register bit.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 12 ++++--------
 drivers/gpu/drm/i915/i915_drv.h      |  9 ++++++++-
 drivers/gpu/drm/i915/intel_display.c |  3 ++-
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_ips.c     | 35 ++++++++++++++++++++++++++++++++---
 5 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 391861d..f741f13 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1711,14 +1711,10 @@ static int i915_ips_status(struct seq_file *m, void *unused)
 	seq_printf(m, "Enabled by kernel parameter: %s\n",
 		   yesno(i915.enable_ips));
 
-	if (INTEL_INFO(dev)->gen >= 8) {
-		seq_puts(m, "Currently: unknown\n");
-	} else {
-		if (I915_READ(IPS_CTL) & IPS_ENABLE)
-			seq_puts(m, "Currently: enabled\n");
-		else
-			seq_puts(m, "Currently: disabled\n");
-	}
+	if (dev_priv->display_ips.enabled)
+		seq_puts(m, "Currently: enabled\n");
+	else
+		seq_puts(m, "Currently: disabled\n");
 
 	intel_runtime_pm_put(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d2a546a..516c0b1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -971,6 +971,11 @@ struct i915_psr {
 	bool aux_frame_sync;
 };
 
+struct i915_ips {
+	struct mutex lock;
+	bool enabled;
+};
+
 enum intel_pch {
 	PCH_NONE = 0,	/* No PCH present */
 	PCH_IBX,	/* Ibexpeak PCH */
@@ -1850,12 +1855,14 @@ struct drm_i915_private {
 
 	/* ilk-only ips/rps state. Everything in here is protected by the global
 	 * mchdev_lock in intel_pm.c */
-	struct intel_ilk_power_mgmt ips;
+	struct intel_ilk_power_mgmt ips; /* Intelligent Power Sharing */
 
 	struct i915_power_domains power_domains;
 
 	struct i915_psr psr;
 
+	struct i915_ips display_ips; /* Intermediate Pixel Storage */
+
 	struct i915_gpu_error gpu_error;
 
 	struct drm_i915_gem_object *vlv_pctx;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 197c608..e5c4056 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4537,7 +4537,6 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
 	/* Workaround : Do not read or write the pipe palette/gamma data while
 	 * GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
 	 */
-	/* FIXME: This should be ips_enabled */
 	if (IS_HASWELL(dev) && intel_crtc->config->ips_ready &&
 	    ((I915_READ(GAMMA_MODE(pipe)) & GAMMA_MODE_MODE_MASK) ==
 	     GAMMA_MODE_MODE_SPLIT)) {
@@ -14190,6 +14189,8 @@ static void intel_setup_outputs(struct drm_device *dev)
 
 	intel_psr_init(dev);
 
+	intel_ips_init(dev_priv);
+
 	for_each_intel_encoder(dev, encoder) {
 		encoder->base.possible_crtcs = encoder->crtc_mask;
 		encoder->base.possible_clones =
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 41a88f7..cc08566 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1210,6 +1210,7 @@ bool intel_ips_ready(struct intel_crtc *crtc,
 		     struct intel_crtc_state *crtc_state);
 void intel_ips_enable(struct intel_crtc *crtc);
 void intel_ips_disable(struct intel_crtc *crtc);
+void intel_ips_init(struct drm_i915_private *dev_priv);
 
 /* intel_csr.c */
 void intel_csr_ucode_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_ips.c b/drivers/gpu/drm/i915/intel_ips.c
index 573bc31..1d0d8ff 100644
--- a/drivers/gpu/drm/i915/intel_ips.c
+++ b/drivers/gpu/drm/i915/intel_ips.c
@@ -113,6 +113,11 @@ void intel_ips_enable(struct intel_crtc *crtc)
 	if (!crtc->config->ips_ready)
 		return;
 
+	mutex_lock(&dev_priv->display_ips.lock);
+
+	if (dev_priv->display_ips.enabled)
+		goto out;
+
 	/*
 	 * We can only enable IPS after we enable a plane
 	 * and wait for a vblank.
@@ -140,9 +145,15 @@ void intel_ips_enable(struct intel_crtc *crtc)
 		 * the HW state readout code will complain that the expected
 		 * IPS_CTL value is not the one we read.
 		 */
-		if (wait_for(I915_READ_NOTRACE(IPS_CTL) & IPS_ENABLE, 50))
+		if (wait_for(I915_READ_NOTRACE(IPS_CTL) & IPS_ENABLE, 50)) {
 			DRM_ERROR("Timed out waiting for IPS enable\n");
+			goto out;
+		}
 	}
+
+	dev_priv->display_ips.enabled = true;
+out:
+	mutex_unlock(&dev_priv->display_ips.lock);
 }
 
 /**
@@ -157,8 +168,10 @@ void intel_ips_disable(struct intel_crtc *crtc)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!crtc->config->ips_ready)
-		return;
+	mutex_lock(&dev_priv->display_ips.lock);
+
+	if (!dev_priv->display_ips.enabled)
+		goto out;
 
 	assert_plane_enabled(dev_priv, crtc->plane);
 	if (IS_BROADWELL(dev)) {
@@ -179,4 +192,20 @@ void intel_ips_disable(struct intel_crtc *crtc)
 
 	/* We need to wait for a vblank before we can disable the plane. */
 	intel_wait_for_vblank(dev, crtc->pipe);
+
+	dev_priv->display_ips.enabled = false;
+out:
+	mutex_unlock(&dev_priv->display_ips.lock);
+}
+
+/**
+ * intel_ips_init - Init IPS
+ * @dev_priv: drm i915 private.
+ *
+ * This function should be called only once to initialize what ever needed
+ * for IPS.
+ */
+void intel_ips_init(struct drm_i915_private dev_priv)
+{
+	mutex_init(&dev_priv->display_ips.lock);
 }
-- 
2.4.3

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

[PATCH 05/31] drm/i915: Fix IPS initialization.

[Rodrigo Vivi]

With Fastboot by default we don't necessarily do a
full modeset enabling the primary plane.
So IPS enable call that was in that path wasn't being
called anymore.

So, let's relly on post atomic modeset path
and on ips_ready to enabled IPS when we judge necessary.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 +++++---------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e5c4056..b2c5c55 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4607,14 +4607,6 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 		intel_wait_for_vblank(dev, pipe);
 
 	/*
-	 * FIXME IPS should be fine as long as one plane is
-	 * enabled, but in practice it seems to have problems
-	 * when going from primary only to sprite only and vice
-	 * versa.
-	 */
-	intel_ips_enable(intel_crtc);
-
-	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
 	 * So don't enable underrun reporting before at least some planes
 	 * are enabled.
@@ -4700,6 +4692,9 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 	if (atomic->update_fbc)
 		intel_fbc_update(dev_priv);
 
+	if (atomic->enable_ips)
+		intel_ips_enable(crtc);
+
 	if (atomic->post_enable_primary)
 		intel_post_enable_primary(&crtc->base);
 
@@ -11578,7 +11573,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 
 			intel_crtc->atomic.disable_fbc = true;
 		}
-
+		if (visible && intel_crtc->config->ips_ready)
+			intel_crtc->atomic.enable_ips = true;
 		/*
 		 * FBC does not work on some platforms for rotated
 		 * planes, so disable it when rotation is not 0 and
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cc08566..92010f7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -538,6 +538,7 @@ struct intel_crtc_atomic_commit {
 	unsigned fb_bits;
 	bool wait_vblank;
 	bool update_fbc;
+	bool enable_ips;
 	bool post_enable_primary;
 	unsigned update_sprite_watermarks;
 };
-- 
2.4.3

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

[PATCH 06/31] drm/i915: Fix IPS disable sequence.

[Rodrigo Vivi]

We cannot let IPS enabled in the pipe if there is
no plane enabled so whenever disabling the primary
plane we check the state of other planes and disable
IPS if needed.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 18 +++---------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_ips.c     | 35 ++++++++++++++++++++++++++++++++++-
 3 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b2c5c55..a0e053e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4662,14 +4662,6 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 		dev_priv->wm.vlv.cxsr = false;
 		intel_wait_for_vblank(dev, pipe);
 	}
-
-	/*
-	 * FIXME IPS should be fine as long as one plane is
-	 * enabled, but in practice it seems to have problems
-	 * when going from primary only to sprite only and vice
-	 * versa.
-	 */
-	intel_ips_disable(intel_crtc);
 }
 
 static void intel_post_plane_update(struct intel_crtc *crtc)
@@ -4711,7 +4703,7 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 		intel_fbc_disable_crtc(crtc);
 
 	if (crtc->atomic.disable_ips)
-		intel_ips_disable(crtc);
+		intel_ips_disable_if_alone(crtc);
 
 	if (atomic->pre_disable_primary)
 		intel_pre_disable_primary(&crtc->base);
@@ -11562,12 +11554,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 
 		if (turn_off) {
 			/*
-			 * FIXME: Actually if we will still have any other
-			 * plane enabled on the pipe we could let IPS enabled
-			 * still, but for now lets consider that when we make
-			 * primary invisible by setting DSPCNTR to 0 on
-			 * update_primary_plane function IPS needs to be
-			 * disable.
+			 * IPS disable_if_alone function will be called
+			 * in order to decide if IPS disable is really needed.
 			 */
 			intel_crtc->atomic.disable_ips = true;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 92010f7..04c1fc4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1211,6 +1211,7 @@ bool intel_ips_ready(struct intel_crtc *crtc,
 		     struct intel_crtc_state *crtc_state);
 void intel_ips_enable(struct intel_crtc *crtc);
 void intel_ips_disable(struct intel_crtc *crtc);
+void intel_ips_disable_if_alone(struct intel_crtc *crtc);
 void intel_ips_init(struct drm_i915_private *dev_priv);
 
 /* intel_csr.c */
diff --git a/drivers/gpu/drm/i915/intel_ips.c b/drivers/gpu/drm/i915/intel_ips.c
index 1d0d8ff..b867aba 100644
--- a/drivers/gpu/drm/i915/intel_ips.c
+++ b/drivers/gpu/drm/i915/intel_ips.c
@@ -199,13 +199,46 @@ out:
 }
 
 /**
+ * intel_ips_disable_if_alone - Disable IPS if alone in the pipe.
+ * @crtc: intel crtc
+ *
+ * This function should be called when primary plane is being disabled.
+ * It checks if there is any other plane enabled on the pipe when primary is
+ * going to be disabled. In this case IPS can continue enabled, but it needs
+ * to be disabled otherwise.
+ */
+void intel_ips_disable_if_alone(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool ips_enabled;
+	struct intel_plane *intel_plane;
+
+	mutex_lock(&dev_priv->display_ips.lock);
+	ips_enabled = dev_priv->display_ips.enabled;
+	mutex_unlock(&dev_priv->display_ips.lock);
+
+	if (!ips_enabled)
+		return;
+
+	for_each_intel_plane_on_crtc(dev, crtc, intel_plane) {
+		enum plane plane = intel_plane->plane;
+
+		if (plane != PLANE_A &&
+		    !!(I915_READ(DSPCNTR(plane)) & DISPLAY_PLANE_ENABLE))
+			return;
+		intel_ips_disable(crtc);
+	}
+}
+
+/**
  * intel_ips_init - Init IPS
  * @dev_priv: drm i915 private.
  *
  * This function should be called only once to initialize what ever needed
  * for IPS.
  */
-void intel_ips_init(struct drm_i915_private dev_priv)
+void intel_ips_init(struct drm_i915_private *dev_priv)
 {
 	mutex_init(&dev_priv->display_ips.lock);
 }
-- 
2.4.3

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

[PATCH 07/31] drm/i915: IPS Sysfs interface.

[Rodrigo Vivi]

With the lock in place we can expose ips enabled/disable on sysfs
for developing, debugging and information purposes.

The main goal is to have an informative tab at powertop that expose
the current status of power features in our driver. So let's start
with this IPS one. Please remind that powertop has no access to drm
properties so sysfs will be used as it is currently used for RC6.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_sysfs.c | 83 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h  |  4 +-
 drivers/gpu/drm/i915/intel_ips.c  | 25 +++++++++---
 3 files changed, 105 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 50ce9ce..2d092c1 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -35,6 +35,82 @@
 #define dev_to_drm_minor(d) dev_get_drvdata((d))
 
 #ifdef CONFIG_PM
+
+static ssize_t
+ips_show(struct device *kdev, struct device_attribute *attr, char *buf)
+{
+	struct drm_minor *dminor = dev_to_drm_minor(kdev);
+	struct drm_device *dev = dminor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	ssize_t ret;
+
+	mutex_lock(&dev_priv->display_ips.lock);
+	ret = snprintf(buf, PAGE_SIZE, "%s\n", dev_priv->display_ips.enabled ?
+		       "enabled" : "disabled");
+	mutex_unlock(&dev_priv->display_ips.lock);
+
+	return ret;
+}
+
+
+static ssize_t
+ips_toggle(struct device *kdev, struct device_attribute *attr,
+	   const char *buf, size_t count)
+{
+	struct drm_minor *dminor = dev_to_drm_minor(kdev);
+	struct drm_device *dev = dminor->dev;
+	struct intel_connector *connector;
+	struct intel_encoder *encoder;
+	struct intel_crtc *crtc = NULL;
+	u32 val;
+	ssize_t ret;
+
+	ret = kstrtou32(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	for_each_intel_connector(dev, connector) {
+		if (!connector->base.encoder)
+			continue;
+		encoder = to_intel_encoder(connector->base.encoder);
+		crtc = to_intel_crtc(encoder->base.crtc);
+	}
+
+	if (!crtc)
+		return -ENODEV;
+
+	switch (val) {
+	case 0:
+		ret = intel_ips_disable(crtc);
+		if (ret)
+			return ret;
+		break;
+	case 1:
+		ret = intel_ips_enable(crtc);
+		if (ret)
+			return ret;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return count;
+
+
+}
+
+static DEVICE_ATTR(ips_enable, S_IRUGO | S_IWUSR, ips_show, ips_toggle);
+
+static struct attribute *ips_attrs[] = {
+	&dev_attr_ips_enable.attr,
+	NULL
+};
+
+static struct attribute_group ips_attr_group = {
+	.name = power_group_name,
+	.attrs =  ips_attrs
+};
+
 static u32 calc_residency(struct drm_device *dev, const u32 reg)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -585,6 +661,12 @@ void i915_setup_sysfs(struct drm_device *dev)
 	int ret;
 
 #ifdef CONFIG_PM
+	if (HAS_IPS(dev)) {
+		ret = sysfs_merge_group(&dev->primary->kdev->kobj,
+					&ips_attr_group);
+		if (ret)
+			DRM_ERROR("IPS sysfs setup failed\n");
+	}
 	if (HAS_RC6(dev)) {
 		ret = sysfs_merge_group(&dev->primary->kdev->kobj,
 					&rc6_attr_group);
@@ -641,6 +723,7 @@ void i915_teardown_sysfs(struct drm_device *dev)
 	device_remove_bin_file(dev->primary->kdev,  &dpf_attrs_1);
 	device_remove_bin_file(dev->primary->kdev,  &dpf_attrs);
 #ifdef CONFIG_PM
+	sysfs_unmerge_group(&dev->primary->kdev->kobj, &ips_attr_group);
 	sysfs_unmerge_group(&dev->primary->kdev->kobj, &rc6_attr_group);
 	sysfs_unmerge_group(&dev->primary->kdev->kobj, &rc6p_attr_group);
 #endif
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 04c1fc4..6f0b359 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1209,8 +1209,8 @@ u32 skl_plane_ctl_rotation(unsigned int rotation);
 /* intel_ips.c */
 bool intel_ips_ready(struct intel_crtc *crtc,
 		     struct intel_crtc_state *crtc_state);
-void intel_ips_enable(struct intel_crtc *crtc);
-void intel_ips_disable(struct intel_crtc *crtc);
+int intel_ips_enable(struct intel_crtc *crtc);
+int intel_ips_disable(struct intel_crtc *crtc);
 void intel_ips_disable_if_alone(struct intel_crtc *crtc);
 void intel_ips_init(struct drm_i915_private *dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_ips.c b/drivers/gpu/drm/i915/intel_ips.c
index b867aba..6bc5c55 100644
--- a/drivers/gpu/drm/i915/intel_ips.c
+++ b/drivers/gpu/drm/i915/intel_ips.c
@@ -105,18 +105,21 @@ bool intel_ips_ready(struct intel_crtc *crtc,
  * This function is called to enable IPS on certain pipe.
  * All needed conditions should've checked already by intel_ips_ready.
  */
-void intel_ips_enable(struct intel_crtc *crtc)
+int intel_ips_enable(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret = 0;
 
 	if (!crtc->config->ips_ready)
-		return;
+		return -EINVAL;
 
 	mutex_lock(&dev_priv->display_ips.lock);
 
-	if (dev_priv->display_ips.enabled)
+	if (dev_priv->display_ips.enabled) {
+		ret = -EALREADY;
 		goto out;
+	}
 
 	/*
 	 * We can only enable IPS after we enable a plane
@@ -147,6 +150,7 @@ void intel_ips_enable(struct intel_crtc *crtc)
 		 */
 		if (wait_for(I915_READ_NOTRACE(IPS_CTL) & IPS_ENABLE, 50)) {
 			DRM_ERROR("Timed out waiting for IPS enable\n");
+			ret = -ETIMEDOUT;
 			goto out;
 		}
 	}
@@ -154,6 +158,7 @@ void intel_ips_enable(struct intel_crtc *crtc)
 	dev_priv->display_ips.enabled = true;
 out:
 	mutex_unlock(&dev_priv->display_ips.lock);
+	return ret;
 }
 
 /**
@@ -162,16 +167,22 @@ out:
  *
  * This function is called to disable IPS on certain pipe whenever it is needed
  * to disable IPS on the pipe.
+ *
+ * Returns:
+ * 0 on success and -errno otherwise.
  */
-void intel_ips_disable(struct intel_crtc *crtc)
+int intel_ips_disable(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret = 0;
 
 	mutex_lock(&dev_priv->display_ips.lock);
 
-	if (!dev_priv->display_ips.enabled)
+	if (!dev_priv->display_ips.enabled) {
+		ret = -EALREADY;
 		goto out;
+	}
 
 	assert_plane_enabled(dev_priv, crtc->plane);
 	if (IS_BROADWELL(dev)) {
@@ -196,6 +207,7 @@ void intel_ips_disable(struct intel_crtc *crtc)
 	dev_priv->display_ips.enabled = false;
 out:
 	mutex_unlock(&dev_priv->display_ips.lock);
+	return ret;
 }
 
 /**
@@ -206,6 +218,9 @@ out:
  * It checks if there is any other plane enabled on the pipe when primary is
  * going to be disabled. In this case IPS can continue enabled, but it needs
  * to be disabled otherwise.
+ *
+ * Returns:
+ * 0 on success and -errno otherwise.
  */
 void intel_ips_disable_if_alone(struct intel_crtc *crtc)
 {
-- 
2.4.3

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

[PATCH 08/31] drm/i915: Add psr_ready on pipe_config

[Rodrigo Vivi]

Let's know beforehand if PSR is ready and will be enabled so we can
prevent DRRS to get enabled.

WARN_ON(!drm_modeset_is_locked(&crtc->mutex)) on intel_psr_ready()
has been removed on v3. We don't dereferrence crtc here anymore so
we don't need this check. All configs are now checked from received
pipe config.

v2: Removing is_edp_psr func that is not used after this patch.
    Rename match_conditions and document it since it is now external.
    Moving to a propper place as pointed out by Sivakumar.
    Use a better name as pointed out by Ram.

v3: Don't dereferrence drm_encoder->crtc and intel_crtc->config on
    psr_ready check. Fix a opps caused with previous versions.

v4: Mention and explain on commit message the crtc->mutex check
    removal that happened on v3.

v5: Rebase on top of ips_ready rework.

Cc: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Cc: Ramalingam C <ramalingam.c@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel@ffwll.ch>
Reviewed-by: Ramalingam C <ramalingam.c@intel.com> (v2)
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  1 +
 drivers/gpu/drm/i915/intel_dp.c      |  2 ++
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++
 drivers/gpu/drm/i915/intel_psr.c     | 57 ++++++++++++++++++++----------------
 4 files changed, 38 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a0e053e..869929d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11871,6 +11871,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 		      pipe_config->pch_pfit.size,
 		      pipe_config->pch_pfit.enabled ? "enabled" : "disabled");
 	DRM_DEBUG_KMS("ips ready: %i\n", pipe_config->ips_ready);
+	DRM_DEBUG_KMS("psr ready: %i\n", pipe_config->psr_ready);
 	DRM_DEBUG_KMS("double wide: %i\n", pipe_config->double_wide);
 
 	if (IS_BROXTON(dev)) {
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 273e1ad..e1c3e9a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1467,6 +1467,8 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 		 */
 		min_lane_count = max_lane_count;
 		min_clock = max_clock;
+
+		pipe_config->psr_ready = intel_psr_ready(intel_dp, pipe_config);
 	}
 
 	for (; bpp >= 6*3; bpp -= 2*3) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6f0b359..06511d2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -477,6 +477,7 @@ struct intel_crtc_state {
 	struct intel_link_m_n fdi_m_n;
 
 	bool ips_ready;
+	bool psr_ready;
 
 	bool double_wide;
 
@@ -1396,6 +1397,8 @@ void intel_backlight_unregister(struct drm_device *dev);
 
 
 /* intel_psr.c */
+bool intel_psr_ready(struct intel_dp *intel_dp,
+		     struct intel_crtc_state *pipe_config);
 void intel_psr_enable(struct intel_dp *intel_dp);
 void intel_psr_disable(struct intel_dp *intel_dp);
 void intel_psr_invalidate(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 213581c..4a9d620 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -56,11 +56,6 @@
 #include "intel_drv.h"
 #include "i915_drv.h"
 
-static bool is_edp_psr(struct intel_dp *intel_dp)
-{
-	return intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED;
-}
-
 static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -287,19 +282,32 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 				EDP_SU_TRACK_ENABLE | EDP_PSR2_TP2_TIME_100);
 }
 
-static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
+/**
+ * intel_psr_ready - PSR ready
+ * @intel_dp: Intel DP
+ * @pipe_config: Pipe Config
+ *
+ * This function Checks if PSR is supported by Hardware/Source and
+ * Panel/Sink and if all conditions to be enabled are fulfilled.
+ *
+ * It is used to know beforehand if PSR is going to be enabled.
+ *
+ * Returns:
+ * True when PSR is ready to be enabled, false otherwise.
+ */
+bool intel_psr_ready(struct intel_dp *intel_dp,
+		     struct intel_crtc_state *pipe_config)
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc = dig_port->base.base.crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	lockdep_assert_held(&dev_priv->psr.lock);
-	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
-	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+	if (!HAS_PSR(dev)) {
+		DRM_DEBUG_KMS("PSR not supported on this platform\n");
+		return false;
+	}
 
-	dev_priv->psr.source_ok = false;
+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
 	if (IS_HASWELL(dev) && dig_port->port != PORT_A) {
 		DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
@@ -312,14 +320,14 @@ static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
 	}
 
 	if (IS_HASWELL(dev) &&
-	    I915_READ(HSW_STEREO_3D_CTL(intel_crtc->config->cpu_transcoder)) &
+	    I915_READ(HSW_STEREO_3D_CTL(pipe_config->cpu_transcoder)) &
 		      S3D_ENABLE) {
 		DRM_DEBUG_KMS("PSR condition failed: Stereo 3D is Enabled\n");
 		return false;
 	}
 
 	if (IS_HASWELL(dev) &&
-	    intel_crtc->config->base.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE) {
+	    pipe_config->base.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE) {
 		DRM_DEBUG_KMS("PSR condition failed: Interlaced is Enabled\n");
 		return false;
 	}
@@ -330,10 +338,19 @@ static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
 		return false;
 	}
 
+	/* At this point we can tell HW/Source supports PSR */
 	dev_priv->psr.source_ok = true;
+
+	/* Now check if Panel/Sink supports it */
+	if (!dev_priv->psr.sink_support) {
+		DRM_DEBUG_KMS("PSR not supported by this panel\n");
+		return false;
+	}
+
 	return true;
 }
 
+
 static void intel_psr_activate(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -370,15 +387,8 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
 
-	if (!HAS_PSR(dev)) {
-		DRM_DEBUG_KMS("PSR not supported on this platform\n");
-		return;
-	}
-
-	if (!is_edp_psr(intel_dp)) {
-		DRM_DEBUG_KMS("PSR not supported by this panel\n");
+	if (!crtc->config->psr_ready)
 		return;
-	}
 
 	mutex_lock(&dev_priv->psr.lock);
 	if (dev_priv->psr.enabled) {
@@ -386,9 +396,6 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 		goto unlock;
 	}
 
-	if (!intel_psr_match_conditions(intel_dp))
-		goto unlock;
-
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 
 	if (HAS_DDI(dev)) {
-- 
2.4.3

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

[PATCH 09/31] drm/i915: Only enable DRRS if PSR won't be enabled on this pipe.

[Rodrigo Vivi]

With PSR enabled being pre computed on pipe_config we can now
prevent DRRS to be enabled along with PSR.

v2: Rebase after changing previous patch

v3: Fix commit subject. No change in the already reviewed patch.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e1c3e9a..6ab127c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5248,11 +5248,6 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 		return;
 	}
 
-	/*
-	 * FIXME: This needs proper synchronization with psr state for some
-	 * platforms that cannot have PSR and DRRS enabled at the same time.
-	 */
-
 	dig_port = dp_to_dig_port(intel_dp);
 	encoder = &dig_port->base;
 	intel_crtc = to_intel_crtc(encoder->base.crtc);
@@ -5339,6 +5334,11 @@ void intel_edp_drrs_enable(struct intel_dp *intel_dp)
 		return;
 	}
 
+	if (intel_crtc->config->psr_ready) {
+		DRM_DEBUG_KMS("DRRS: PSR will be enabled on this crtc\n");
+		return;
+	}
+
 	mutex_lock(&dev_priv->drrs.mutex);
 	if (WARN_ON(dev_priv->drrs.dp)) {
 		DRM_ERROR("DRRS already enabled\n");
-- 
2.4.3

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

[PATCH 10/31] drm/i915: Detatch i915.enable_psr from psr_ready

[Rodrigo Vivi]

PSR will be enabled on every post primary update when it is
ready and parameter allows.
With this we allow test cases to continue using this parameter
for enabling disabling the feature.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 2 +-
 drivers/gpu/drm/i915/intel_psr.c | 5 -----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6ab127c..e154a2e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5334,7 +5334,7 @@ void intel_edp_drrs_enable(struct intel_dp *intel_dp)
 		return;
 	}
 
-	if (intel_crtc->config->psr_ready) {
+	if (intel_crtc->config->psr_ready && i915.enable_psr) {
 		DRM_DEBUG_KMS("DRRS: PSR will be enabled on this crtc\n");
 		return;
 	}
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 4a9d620..e690db3 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -314,11 +314,6 @@ bool intel_psr_ready(struct intel_dp *intel_dp,
 		return false;
 	}
 
-	if (!i915.enable_psr) {
-		DRM_DEBUG_KMS("PSR disable by flag\n");
-		return false;
-	}
-
 	if (IS_HASWELL(dev) &&
 	    I915_READ(HSW_STEREO_3D_CTL(pipe_config->cpu_transcoder)) &
 		      S3D_ENABLE) {
-- 
2.4.3

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

[PATCH 11/31] drm/i915: Use intel_crtc instead of intel_dp on PSR enable/disable functions.

[Rodrigo Vivi]

This will give us the flexibility to move them to
post atomic modeset functions.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c |  4 ++--
 drivers/gpu/drm/i915/intel_dp.c  |  5 +++--
 drivers/gpu/drm/i915/intel_drv.h |  4 ++--
 drivers/gpu/drm/i915/intel_psr.c | 38 ++++++++++++++++++++++++--------------
 4 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 06d3002..b8f8dee 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2404,7 +2404,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
 			intel_dp_stop_link_train(intel_dp);
 
 		intel_edp_backlight_on(intel_dp);
-		intel_psr_enable(intel_dp);
+		intel_psr_enable(intel_crtc);
 		intel_edp_drrs_enable(intel_dp);
 	}
 
@@ -2432,7 +2432,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
 		intel_edp_drrs_disable(intel_dp);
-		intel_psr_disable(intel_dp);
+		intel_psr_disable(intel_crtc);
 		intel_edp_backlight_off(intel_dp);
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e154a2e..92f59cc 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2374,7 +2374,7 @@ static void intel_disable_dp(struct intel_encoder *encoder)
 		intel_audio_codec_disable(encoder);
 
 	if (HAS_PSR(dev) && !HAS_DDI(dev))
-		intel_psr_disable(intel_dp);
+		intel_psr_disable(crtc);
 
 	/* Make sure the panel is off before trying to change the mode. But also
 	 * ensure that we have vdd while we switch off the panel. */
@@ -2629,9 +2629,10 @@ static void g4x_enable_dp(struct intel_encoder *encoder)
 static void vlv_enable_dp(struct intel_encoder *encoder)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
+	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
 
 	intel_edp_backlight_on(intel_dp);
-	intel_psr_enable(intel_dp);
+	intel_psr_enable(crtc);
 }
 
 static void g4x_pre_enable_dp(struct intel_encoder *encoder)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 06511d2..cafe4c1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1399,8 +1399,8 @@ void intel_backlight_unregister(struct drm_device *dev);
 /* intel_psr.c */
 bool intel_psr_ready(struct intel_dp *intel_dp,
 		     struct intel_crtc_state *pipe_config);
-void intel_psr_enable(struct intel_dp *intel_dp);
-void intel_psr_disable(struct intel_dp *intel_dp);
+void intel_psr_enable(struct intel_crtc *intel_crtc);
+void intel_psr_disable(struct intel_crtc *intel_crtc);
 void intel_psr_invalidate(struct drm_device *dev,
 			  unsigned frontbuffer_bits);
 void intel_psr_flush(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index e690db3..bcf2d9d 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -371,18 +371,19 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
 
 /**
  * intel_psr_enable - Enable PSR
- * @intel_dp: Intel DP
+ * @intel_crtc: Intel CRTC
  *
  * This function can only be called after the pipe is fully trained and enabled.
  */
-void intel_psr_enable(struct intel_dp *intel_dp)
+void intel_psr_enable(struct intel_crtc *intel_crtc)
 {
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
+	struct intel_encoder *intel_encoder;
+	struct intel_dp *intel_dp = NULL;
 
-	if (!crtc->config->psr_ready)
+	if (!intel_crtc->config->psr_ready)
 		return;
 
 	mutex_lock(&dev_priv->psr.lock);
@@ -391,6 +392,16 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 		goto unlock;
 	}
 
+	for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
+		if (intel_encoder->type == INTEL_OUTPUT_EDP)
+			intel_dp = enc_to_intel_dp(&intel_encoder->base);
+	}
+
+	if (!intel_dp) {
+		DRM_DEBUG_KMS("No eDP found\n");
+		goto unlock;
+	}
+
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 
 	if (HAS_DDI(dev)) {
@@ -398,8 +409,8 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 
 		if (dev_priv->psr.psr2_support) {
 			/* PSR2 is restricted to work with panel resolutions upto 3200x2000 */
-			if (crtc->config->pipe_src_w > 3200 ||
-				crtc->config->pipe_src_h > 2000)
+			if (intel_crtc->config->pipe_src_w > 3200 ||
+				intel_crtc->config->pipe_src_h > 2000)
 				dev_priv->psr.psr2_support = false;
 			else
 				skl_psr_setup_su_vsc(intel_dp);
@@ -484,14 +495,13 @@ static void hsw_psr_disable(struct intel_dp *intel_dp)
 
 /**
  * intel_psr_disable - Disable PSR
- * @intel_dp: Intel DP
+ * @intel_crtc: Intel CRTC
  *
  * This function needs to be called before disabling pipe.
  */
-void intel_psr_disable(struct intel_dp *intel_dp)
+void intel_psr_disable(struct intel_crtc *intel_crtc)
 {
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_device *dev = intel_crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	mutex_lock(&dev_priv->psr.lock);
@@ -501,9 +511,9 @@ void intel_psr_disable(struct intel_dp *intel_dp)
 	}
 
 	if (HAS_DDI(dev))
-		hsw_psr_disable(intel_dp);
+		hsw_psr_disable(dev_priv->psr.enabled);
 	else
-		vlv_psr_disable(intel_dp);
+		vlv_psr_disable(dev_priv->psr.enabled);
 
 	dev_priv->psr.enabled = NULL;
 	mutex_unlock(&dev_priv->psr.lock);
-- 
2.4.3

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

[PATCH 12/31] drm/i915: Fix PSR initialization.

[Rodrigo Vivi]

PSR is still disabled by default, but even passing i915.enable_psr=1
at this point we weren't able to get PSR working because with
fastboot by default in place we weren't executing the path that enables
encoder and consequently PSR.

Now with psr_ready in place and PSR using crtc signature we can move
its enable/disable sequences from the encoder enable to the post
atomic modeset functions.

i915.enable_psr parameter is still used to enable/disable psr feature
on the next primary plane update. So current test cases that relies
on this flow still works.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     |  2 --
 drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c      |  5 -----
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index b8f8dee..36db970 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2404,7 +2404,6 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
 			intel_dp_stop_link_train(intel_dp);
 
 		intel_edp_backlight_on(intel_dp);
-		intel_psr_enable(intel_crtc);
 		intel_edp_drrs_enable(intel_dp);
 	}
 
@@ -2432,7 +2431,6 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
 		intel_edp_drrs_disable(intel_dp);
-		intel_psr_disable(intel_crtc);
 		intel_edp_backlight_off(intel_dp);
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 869929d..f67e2ee 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4687,6 +4687,9 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 	if (atomic->enable_ips)
 		intel_ips_enable(crtc);
 
+	if (atomic->enable_psr)
+		intel_psr_enable(crtc);
+
 	if (atomic->post_enable_primary)
 		intel_post_enable_primary(&crtc->base);
 
@@ -4705,6 +4708,9 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 	if (crtc->atomic.disable_ips)
 		intel_ips_disable_if_alone(crtc);
 
+	if (crtc->atomic.disable_psr)
+		intel_psr_disable(crtc);
+
 	if (atomic->pre_disable_primary)
 		intel_pre_disable_primary(&crtc->base);
 
@@ -11560,9 +11566,18 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 			intel_crtc->atomic.disable_ips = true;
 
 			intel_crtc->atomic.disable_fbc = true;
+
+			intel_crtc->atomic.disable_psr = true;
 		}
 		if (visible && intel_crtc->config->ips_ready)
 			intel_crtc->atomic.enable_ips = true;
+
+		if (visible && intel_crtc->config->psr_ready) {
+			if (i915.enable_psr)
+				intel_crtc->atomic.enable_psr = true;
+			else
+				intel_crtc->atomic.disable_psr = true;
+		}
 		/*
 		 * FBC does not work on some platforms for rotated
 		 * planes, so disable it when rotation is not 0 and
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 92f59cc..f0ee497 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2373,9 +2373,6 @@ static void intel_disable_dp(struct intel_encoder *encoder)
 	if (crtc->config->has_audio)
 		intel_audio_codec_disable(encoder);
 
-	if (HAS_PSR(dev) && !HAS_DDI(dev))
-		intel_psr_disable(crtc);
-
 	/* Make sure the panel is off before trying to change the mode. But also
 	 * ensure that we have vdd while we switch off the panel. */
 	intel_edp_panel_vdd_on(intel_dp);
@@ -2629,10 +2626,8 @@ static void g4x_enable_dp(struct intel_encoder *encoder)
 static void vlv_enable_dp(struct intel_encoder *encoder)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
-	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
 
 	intel_edp_backlight_on(intel_dp);
-	intel_psr_enable(crtc);
 }
 
 static void g4x_pre_enable_dp(struct intel_encoder *encoder)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cafe4c1..d599d54 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -531,6 +531,7 @@ struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
 	bool disable_fbc;
 	bool disable_ips;
+	bool disable_psr;
 	bool disable_cxsr;
 	bool pre_disable_primary;
 	bool update_wm_pre, update_wm_post;
@@ -540,6 +541,7 @@ struct intel_crtc_atomic_commit {
 	bool wait_vblank;
 	bool update_fbc;
 	bool enable_ips;
+	bool enable_psr;
 	bool post_enable_primary;
 	unsigned update_sprite_watermarks;
 };
-- 
2.4.3

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

[PATCH 13/31] drm/i915: Organize Makefile new display pm group

[Rodrigo Vivi]

No functional changes, just to organize the Makefile
by creating a new group for display pm since it doesn't
make sense to let them on core modeset group.

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

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 7c1a86e..5c92a47 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -50,20 +50,22 @@ i915-y += intel_renderstate_gen6.o \
 	  intel_renderstate_gen8.o \
 	  intel_renderstate_gen9.o
 
+# display pm
+i915-y += intel_fbc.o \
+	  intel_ips.o \
+	  intel_psr.o
+
 # modesetting core code
 i915-y += intel_audio.o \
 	  intel_atomic.o \
 	  intel_atomic_plane.o \
 	  intel_bios.o \
 	  intel_display.o \
-	  intel_fbc.o \
 	  intel_fifo_underrun.o \
 	  intel_frontbuffer.o \
 	  intel_hotplug.o \
-	  intel_ips.o \
 	  intel_modes.o \
 	  intel_overlay.o \
-	  intel_psr.o \
 	  intel_sideband.o \
 	  intel_sprite.o
 i915-$(CONFIG_ACPI)		+= intel_acpi.o intel_opregion.o
-- 
2.4.3

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

[PATCH 14/31] drm/i915: Create intel_drrs.c

[Rodrigo Vivi]

No functional change. Jusr organizing display pm features
in a standardized and uniform way.

This first step allow us to fix drrs initialization that
is broken with fastboot by default.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/Makefile            |   3 +-
 drivers/gpu/drm/i915/intel_ddi.c         |   4 +-
 drivers/gpu/drm/i915/intel_dp.c          | 386 +----------------------------
 drivers/gpu/drm/i915/intel_drrs.c        | 408 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h         |  15 +-
 drivers/gpu/drm/i915/intel_frontbuffer.c |   4 +-
 6 files changed, 426 insertions(+), 394 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_drrs.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 5c92a47..da3d2cc 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -51,7 +51,8 @@ i915-y += intel_renderstate_gen6.o \
 	  intel_renderstate_gen9.o
 
 # display pm
-i915-y += intel_fbc.o \
+i915-y += intel_drrs.o \
+	  intel_fbc.o \
 	  intel_ips.o \
 	  intel_psr.o
 
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 36db970..972acef 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2404,7 +2404,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
 			intel_dp_stop_link_train(intel_dp);
 
 		intel_edp_backlight_on(intel_dp);
-		intel_edp_drrs_enable(intel_dp);
+		intel_drrs_enable(intel_dp);
 	}
 
 	if (intel_crtc->config->has_audio) {
@@ -2430,7 +2430,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 	if (type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
-		intel_edp_drrs_disable(intel_dp);
+		intel_drrs_disable(intel_dp);
 		intel_edp_backlight_off(intel_dp);
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f0ee497..a421c17 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5212,388 +5212,6 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 		      I915_READ(pp_div_reg));
 }
 
-/**
- * intel_dp_set_drrs_state - program registers for RR switch to take effect
- * @dev: DRM device
- * @refresh_rate: RR to be programmed
- *
- * This function gets called when refresh rate (RR) has to be changed from
- * one frequency to another. Switches can be between high and low RR
- * supported by the panel or to any other RR based on media playback (in
- * this case, RR value needs to be passed from user space).
- *
- * The caller of this function needs to take a lock on dev_priv->drrs.
- */
-static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_encoder *encoder;
-	struct intel_digital_port *dig_port = NULL;
-	struct intel_dp *intel_dp = dev_priv->drrs.dp;
-	struct intel_crtc_state *config = NULL;
-	struct intel_crtc *intel_crtc = NULL;
-	enum drrs_refresh_rate_type index = DRRS_HIGH_RR;
-
-	if (refresh_rate <= 0) {
-		DRM_DEBUG_KMS("Refresh rate should be positive non-zero.\n");
-		return;
-	}
-
-	if (intel_dp == NULL) {
-		DRM_DEBUG_KMS("DRRS not supported.\n");
-		return;
-	}
-
-	dig_port = dp_to_dig_port(intel_dp);
-	encoder = &dig_port->base;
-	intel_crtc = to_intel_crtc(encoder->base.crtc);
-
-	if (!intel_crtc) {
-		DRM_DEBUG_KMS("DRRS: intel_crtc not initialized\n");
-		return;
-	}
-
-	config = intel_crtc->config;
-
-	if (dev_priv->drrs.type < SEAMLESS_DRRS_SUPPORT) {
-		DRM_DEBUG_KMS("Only Seamless DRRS supported.\n");
-		return;
-	}
-
-	if (intel_dp->attached_connector->panel.downclock_mode->vrefresh ==
-			refresh_rate)
-		index = DRRS_LOW_RR;
-
-	if (index == dev_priv->drrs.refresh_rate_type) {
-		DRM_DEBUG_KMS(
-			"DRRS requested for previously set RR...ignoring\n");
-		return;
-	}
-
-	if (!intel_crtc->active) {
-		DRM_DEBUG_KMS("eDP encoder disabled. CRTC not Active\n");
-		return;
-	}
-
-	if (INTEL_INFO(dev)->gen >= 8 && !IS_CHERRYVIEW(dev)) {
-		switch (index) {
-		case DRRS_HIGH_RR:
-			intel_dp_set_m_n(intel_crtc, M1_N1);
-			break;
-		case DRRS_LOW_RR:
-			intel_dp_set_m_n(intel_crtc, M2_N2);
-			break;
-		case DRRS_MAX_RR:
-		default:
-			DRM_ERROR("Unsupported refreshrate type\n");
-		}
-	} else if (INTEL_INFO(dev)->gen > 6) {
-		u32 reg = PIPECONF(intel_crtc->config->cpu_transcoder);
-		u32 val;
-
-		val = I915_READ(reg);
-		if (index > DRRS_HIGH_RR) {
-			if (IS_VALLEYVIEW(dev))
-				val |= PIPECONF_EDP_RR_MODE_SWITCH_VLV;
-			else
-				val |= PIPECONF_EDP_RR_MODE_SWITCH;
-		} else {
-			if (IS_VALLEYVIEW(dev))
-				val &= ~PIPECONF_EDP_RR_MODE_SWITCH_VLV;
-			else
-				val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
-		}
-		I915_WRITE(reg, val);
-	}
-
-	dev_priv->drrs.refresh_rate_type = index;
-
-	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
-}
-
-/**
- * intel_edp_drrs_enable - init drrs struct if supported
- * @intel_dp: DP struct
- *
- * Initializes frontbuffer_bits and drrs.dp
- */
-void intel_edp_drrs_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;
-	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
-	struct drm_crtc *crtc = dig_port->base.base.crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
-	if (!intel_crtc->config->has_drrs) {
-		DRM_DEBUG_KMS("Panel doesn't support DRRS\n");
-		return;
-	}
-
-	if (intel_crtc->config->psr_ready && i915.enable_psr) {
-		DRM_DEBUG_KMS("DRRS: PSR will be enabled on this crtc\n");
-		return;
-	}
-
-	mutex_lock(&dev_priv->drrs.mutex);
-	if (WARN_ON(dev_priv->drrs.dp)) {
-		DRM_ERROR("DRRS already enabled\n");
-		goto unlock;
-	}
-
-	dev_priv->drrs.busy_frontbuffer_bits = 0;
-
-	dev_priv->drrs.dp = intel_dp;
-
-unlock:
-	mutex_unlock(&dev_priv->drrs.mutex);
-}
-
-/**
- * intel_edp_drrs_disable - Disable DRRS
- * @intel_dp: DP struct
- *
- */
-void intel_edp_drrs_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;
-	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
-	struct drm_crtc *crtc = dig_port->base.base.crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
-	if (!intel_crtc->config->has_drrs)
-		return;
-
-	mutex_lock(&dev_priv->drrs.mutex);
-	if (!dev_priv->drrs.dp) {
-		mutex_unlock(&dev_priv->drrs.mutex);
-		return;
-	}
-
-	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
-		intel_dp_set_drrs_state(dev_priv->dev,
-			intel_dp->attached_connector->panel.
-			fixed_mode->vrefresh);
-
-	dev_priv->drrs.dp = NULL;
-	mutex_unlock(&dev_priv->drrs.mutex);
-
-	cancel_delayed_work_sync(&dev_priv->drrs.work);
-}
-
-static void intel_edp_drrs_downclock_work(struct work_struct *work)
-{
-	struct drm_i915_private *dev_priv =
-		container_of(work, typeof(*dev_priv), drrs.work.work);
-	struct intel_dp *intel_dp;
-
-	mutex_lock(&dev_priv->drrs.mutex);
-
-	intel_dp = dev_priv->drrs.dp;
-
-	if (!intel_dp)
-		goto unlock;
-
-	/*
-	 * The delayed work can race with an invalidate hence we need to
-	 * recheck.
-	 */
-
-	if (dev_priv->drrs.busy_frontbuffer_bits)
-		goto unlock;
-
-	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR)
-		intel_dp_set_drrs_state(dev_priv->dev,
-			intel_dp->attached_connector->panel.
-			downclock_mode->vrefresh);
-
-unlock:
-	mutex_unlock(&dev_priv->drrs.mutex);
-}
-
-/**
- * intel_edp_drrs_invalidate - Disable Idleness DRRS
- * @dev: DRM device
- * @frontbuffer_bits: frontbuffer plane tracking bits
- *
- * This function gets called everytime rendering on the given planes start.
- * Hence DRRS needs to be Upclocked, i.e. (LOW_RR -> HIGH_RR).
- *
- * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits.
- */
-void intel_edp_drrs_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 (dev_priv->drrs.type == DRRS_NOT_SUPPORTED)
-		return;
-
-	cancel_delayed_work(&dev_priv->drrs.work);
-
-	mutex_lock(&dev_priv->drrs.mutex);
-	if (!dev_priv->drrs.dp) {
-		mutex_unlock(&dev_priv->drrs.mutex);
-		return;
-	}
-
-	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
-	pipe = to_intel_crtc(crtc)->pipe;
-
-	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
-	dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits;
-
-	/* invalidate means busy screen hence upclock */
-	if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
-		intel_dp_set_drrs_state(dev_priv->dev,
-				dev_priv->drrs.dp->attached_connector->panel.
-				fixed_mode->vrefresh);
-
-	mutex_unlock(&dev_priv->drrs.mutex);
-}
-
-/**
- * intel_edp_drrs_flush - Restart Idleness DRRS
- * @dev: DRM device
- * @frontbuffer_bits: frontbuffer plane tracking bits
- *
- * This function gets called every time rendering on the given planes has
- * completed or flip on a crtc is completed. So DRRS should be upclocked
- * (LOW_RR -> HIGH_RR). And also Idleness detection should be started again,
- * if no other planes are dirty.
- *
- * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits.
- */
-void intel_edp_drrs_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 (dev_priv->drrs.type == DRRS_NOT_SUPPORTED)
-		return;
-
-	cancel_delayed_work(&dev_priv->drrs.work);
-
-	mutex_lock(&dev_priv->drrs.mutex);
-	if (!dev_priv->drrs.dp) {
-		mutex_unlock(&dev_priv->drrs.mutex);
-		return;
-	}
-
-	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
-	pipe = to_intel_crtc(crtc)->pipe;
-
-	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
-	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
-
-	/* flush means busy screen hence upclock */
-	if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
-		intel_dp_set_drrs_state(dev_priv->dev,
-				dev_priv->drrs.dp->attached_connector->panel.
-				fixed_mode->vrefresh);
-
-	/*
-	 * flush also means no more activity hence schedule downclock, if all
-	 * other fbs are quiescent too
-	 */
-	if (!dev_priv->drrs.busy_frontbuffer_bits)
-		schedule_delayed_work(&dev_priv->drrs.work,
-				msecs_to_jiffies(1000));
-	mutex_unlock(&dev_priv->drrs.mutex);
-}
-
-/**
- * DOC: Display Refresh Rate Switching (DRRS)
- *
- * Display Refresh Rate Switching (DRRS) is a power conservation feature
- * which enables swtching between low and high refresh rates,
- * dynamically, based on the usage scenario. This feature is applicable
- * for internal panels.
- *
- * Indication that the panel supports DRRS is given by the panel EDID, which
- * would list multiple refresh rates for one resolution.
- *
- * DRRS is of 2 types - static and seamless.
- * Static DRRS involves changing refresh rate (RR) by doing a full modeset
- * (may appear as a blink on screen) and is used in dock-undock scenario.
- * Seamless DRRS involves changing RR without any visual effect to the user
- * and can be used during normal system usage. This is done by programming
- * certain registers.
- *
- * Support for static/seamless DRRS may be indicated in the VBT based on
- * inputs from the panel spec.
- *
- * DRRS saves power by switching to low RR based on usage scenarios.
- *
- * eDP DRRS:-
- *        The implementation is based on frontbuffer tracking implementation.
- * When there is a disturbance on the screen triggered by user activity or a
- * periodic system activity, DRRS is disabled (RR is changed to high RR).
- * When there is no movement on screen, after a timeout of 1 second, a switch
- * to low RR is made.
- *        For integration with frontbuffer tracking code,
- * intel_edp_drrs_invalidate() and intel_edp_drrs_flush() are called.
- *
- * DRRS can be further extended to support other internal panels and also
- * the scenario of video playback wherein RR is set based on the rate
- * requested by userspace.
- */
-
-/**
- * intel_dp_drrs_init - Init basic DRRS work and mutex.
- * @intel_connector: eDP connector
- * @fixed_mode: preferred mode of panel
- *
- * This function is  called only once at driver load to initialize basic
- * DRRS stuff.
- *
- * Returns:
- * Downclock mode if panel supports it, else return NULL.
- * DRRS support is determined by the presence of downclock mode (apart
- * from VBT setting).
- */
-static struct drm_display_mode *
-intel_dp_drrs_init(struct intel_connector *intel_connector,
-		struct drm_display_mode *fixed_mode)
-{
-	struct drm_connector *connector = &intel_connector->base;
-	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_display_mode *downclock_mode = NULL;
-
-	INIT_DELAYED_WORK(&dev_priv->drrs.work, intel_edp_drrs_downclock_work);
-	mutex_init(&dev_priv->drrs.mutex);
-
-	if (INTEL_INFO(dev)->gen <= 6) {
-		DRM_DEBUG_KMS("DRRS supported for Gen7 and above\n");
-		return NULL;
-	}
-
-	if (dev_priv->vbt.drrs_type != SEAMLESS_DRRS_SUPPORT) {
-		DRM_DEBUG_KMS("VBT doesn't support DRRS\n");
-		return NULL;
-	}
-
-	downclock_mode = intel_find_panel_downclock
-					(dev, fixed_mode, connector);
-
-	if (!downclock_mode) {
-		DRM_DEBUG_KMS("Downclock mode is not found. DRRS not supported\n");
-		return NULL;
-	}
-
-	dev_priv->drrs.type = dev_priv->vbt.drrs_type;
-
-	dev_priv->drrs.refresh_rate_type = DRRS_HIGH_RR;
-	DRM_DEBUG_KMS("seamless DRRS supported for eDP panel.\n");
-	return downclock_mode;
-}
-
 static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 				     struct intel_connector *intel_connector)
 {
@@ -5655,8 +5273,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	list_for_each_entry(scan, &connector->probed_modes, head) {
 		if ((scan->type & DRM_MODE_TYPE_PREFERRED)) {
 			fixed_mode = drm_mode_duplicate(dev, scan);
-			downclock_mode = intel_dp_drrs_init(
-						intel_connector, fixed_mode);
+			downclock_mode = intel_drrs_init(intel_connector,
+							 fixed_mode);
 			break;
 		}
 	}
diff --git a/drivers/gpu/drm/i915/intel_drrs.c b/drivers/gpu/drm/i915/intel_drrs.c
new file mode 100644
index 0000000..dce16ea
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_drrs.c
@@ -0,0 +1,408 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include "intel_drv.h"
+#include "i915_drv.h"
+
+/**
+ * DOC: Display Refresh Rate Switching (DRRS)
+ *
+ * Display Refresh Rate Switching (DRRS) is a power conservation feature
+ * which enables swtching between low and high refresh rates,
+ * dynamically, based on the usage scenario. This feature is applicable
+ * for internal panels.
+ *
+ * Indication that the panel supports DRRS is given by the panel EDID, which
+ * would list multiple refresh rates for one resolution.
+ *
+ * DRRS is of 2 types - static and seamless.
+ * Static DRRS involves changing refresh rate (RR) by doing a full modeset
+ * (may appear as a blink on screen) and is used in dock-undock scenario.
+ * Seamless DRRS involves changing RR without any visual effect to the user
+ * and can be used during normal system usage. This is done by programming
+ * certain registers.
+ *
+ * Support for static/seamless DRRS may be indicated in the VBT based on
+ * inputs from the panel spec.
+ *
+ * DRRS saves power by switching to low RR based on usage scenarios.
+ *
+ * eDP DRRS:-
+ *        The implementation is based on frontbuffer tracking implementation.
+ * When there is a disturbance on the screen triggered by user activity or a
+ * periodic system activity, DRRS is disabled (RR is changed to high RR).
+ * When there is no movement on screen, after a timeout of 1 second, a switch
+ * to low RR is made.
+ *        For integration with frontbuffer tracking code,
+ * intel_edp_drrs_invalidate() and intel_edp_drrs_flush() are called.
+ *
+ * DRRS can be further extended to support other internal panels and also
+ * the scenario of video playback wherein RR is set based on the rate
+ * requested by userspace.
+ */
+
+/**
+ * intel_drrs_set_state - program registers for RR switch to take effect
+ * @dev: DRM device
+ * @refresh_rate: RR to be programmed
+ *
+ * This function gets called when refresh rate (RR) has to be changed from
+ * one frequency to another. Switches can be between high and low RR
+ * supported by the panel or to any other RR based on media playback (in
+ * this case, RR value needs to be passed from user space).
+ *
+ * The caller of this function needs to take a lock on dev_priv->drrs.
+ */
+static void intel_drrs_set_state(struct drm_device *dev, int refresh_rate)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_encoder *encoder;
+	struct intel_digital_port *dig_port = NULL;
+	struct intel_dp *intel_dp = dev_priv->drrs.dp;
+	struct intel_crtc_state *config = NULL;
+	struct intel_crtc *intel_crtc = NULL;
+	enum drrs_refresh_rate_type index = DRRS_HIGH_RR;
+
+	if (refresh_rate <= 0) {
+		DRM_DEBUG_KMS("Refresh rate should be positive non-zero.\n");
+		return;
+	}
+
+	if (intel_dp == NULL) {
+		DRM_DEBUG_KMS("DRRS not supported.\n");
+		return;
+	}
+
+	dig_port = dp_to_dig_port(intel_dp);
+	encoder = &dig_port->base;
+	intel_crtc = to_intel_crtc(encoder->base.crtc);
+
+	if (!intel_crtc) {
+		DRM_DEBUG_KMS("DRRS: intel_crtc not initialized\n");
+		return;
+	}
+
+	config = intel_crtc->config;
+
+	if (dev_priv->drrs.type < SEAMLESS_DRRS_SUPPORT) {
+		DRM_DEBUG_KMS("Only Seamless DRRS supported.\n");
+		return;
+	}
+
+	if (intel_dp->attached_connector->panel.downclock_mode->vrefresh ==
+			refresh_rate)
+		index = DRRS_LOW_RR;
+
+	if (index == dev_priv->drrs.refresh_rate_type) {
+		DRM_DEBUG_KMS(
+			"DRRS requested for previously set RR...ignoring\n");
+		return;
+	}
+
+	if (!intel_crtc->active) {
+		DRM_DEBUG_KMS("eDP encoder disabled. CRTC not Active\n");
+		return;
+	}
+
+	if (INTEL_INFO(dev)->gen >= 8 && !IS_CHERRYVIEW(dev)) {
+		switch (index) {
+		case DRRS_HIGH_RR:
+			intel_dp_set_m_n(intel_crtc, M1_N1);
+			break;
+		case DRRS_LOW_RR:
+			intel_dp_set_m_n(intel_crtc, M2_N2);
+			break;
+		case DRRS_MAX_RR:
+		default:
+			DRM_ERROR("Unsupported refreshrate type\n");
+		}
+	} else if (INTEL_INFO(dev)->gen > 6) {
+		u32 reg = PIPECONF(intel_crtc->config->cpu_transcoder);
+		u32 val;
+
+		val = I915_READ(reg);
+		if (index > DRRS_HIGH_RR) {
+			if (IS_VALLEYVIEW(dev))
+				val |= PIPECONF_EDP_RR_MODE_SWITCH_VLV;
+			else
+				val |= PIPECONF_EDP_RR_MODE_SWITCH;
+		} else {
+			if (IS_VALLEYVIEW(dev))
+				val &= ~PIPECONF_EDP_RR_MODE_SWITCH_VLV;
+			else
+				val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
+		}
+		I915_WRITE(reg, val);
+	}
+
+	dev_priv->drrs.refresh_rate_type = index;
+
+	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
+}
+
+/**
+ * intel_drrs_enable - init drrs struct if supported
+ * @intel_dp: DP struct
+ *
+ * Initializes frontbuffer_bits and drrs.dp
+ */
+void intel_drrs_enable(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc = dig_port->base.base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	if (!intel_crtc->config->has_drrs) {
+		DRM_DEBUG_KMS("Panel doesn't support DRRS\n");
+		return;
+	}
+
+	if (intel_crtc->config->psr_ready && i915.enable_psr) {
+		DRM_DEBUG_KMS("DRRS: PSR will be enabled on this crtc\n");
+		return;
+	}
+
+	mutex_lock(&dev_priv->drrs.mutex);
+	if (WARN_ON(dev_priv->drrs.dp)) {
+		DRM_ERROR("DRRS already enabled\n");
+		goto unlock;
+	}
+
+	dev_priv->drrs.busy_frontbuffer_bits = 0;
+
+	dev_priv->drrs.dp = intel_dp;
+
+unlock:
+	mutex_unlock(&dev_priv->drrs.mutex);
+}
+
+/**
+ * intel_edp_drrs_disable - Disable DRRS
+ * @intel_dp: DP struct
+ *
+ */
+void intel_drrs_disable(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc = dig_port->base.base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_panel *panel = &intel_dp->attached_connector->panel;
+
+	if (!intel_crtc->config->has_drrs)
+		return;
+
+	mutex_lock(&dev_priv->drrs.mutex);
+	if (!dev_priv->drrs.dp) {
+		mutex_unlock(&dev_priv->drrs.mutex);
+		return;
+	}
+
+	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
+		intel_drrs_set_state(dev_priv->dev,
+				     panel->fixed_mode->vrefresh);
+
+	dev_priv->drrs.dp = NULL;
+	mutex_unlock(&dev_priv->drrs.mutex);
+
+	cancel_delayed_work_sync(&dev_priv->drrs.work);
+}
+
+static void intel_drrs_downclock_work(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, typeof(*dev_priv), drrs.work.work);
+	struct intel_dp *intel_dp;
+	struct intel_panel *panel;
+
+	mutex_lock(&dev_priv->drrs.mutex);
+	intel_dp = dev_priv->drrs.dp;
+
+	if (!intel_dp)
+		goto unlock;
+
+	panel = &intel_dp->attached_connector->panel;
+	/*
+	 * The delayed work can race with an invalidate hence we need to
+	 * recheck.
+	 */
+
+	if (dev_priv->drrs.busy_frontbuffer_bits)
+		goto unlock;
+
+	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR)
+		intel_drrs_set_state(dev_priv->dev,
+				     panel->fixed_mode->vrefresh);
+
+unlock:
+	mutex_unlock(&dev_priv->drrs.mutex);
+}
+
+/**
+ * intel_drrs_invalidate - Disable Idleness DRRS
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called everytime rendering on the given planes start.
+ * Hence DRRS needs to be Upclocked, i.e. (LOW_RR -> HIGH_RR).
+ *
+ * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits.
+ */
+void intel_drrs_invalidate(struct drm_device *dev,
+		unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+	struct intel_panel *panel;
+	enum pipe pipe;
+
+	if (dev_priv->drrs.type == DRRS_NOT_SUPPORTED)
+		return;
+
+	cancel_delayed_work(&dev_priv->drrs.work);
+
+	mutex_lock(&dev_priv->drrs.mutex);
+	if (!dev_priv->drrs.dp) {
+		mutex_unlock(&dev_priv->drrs.mutex);
+		return;
+	}
+
+	panel = &dev_priv->drrs.dp->attached_connector->panel;
+	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
+	pipe = to_intel_crtc(crtc)->pipe;
+
+	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
+	dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits;
+
+	/* invalidate means busy screen hence upclock */
+	if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
+		intel_drrs_set_state(dev_priv->dev,
+				     panel->fixed_mode->vrefresh);
+
+	mutex_unlock(&dev_priv->drrs.mutex);
+}
+
+/**
+ * intel_drrs_flush - Restart Idleness DRRS
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called every time rendering on the given planes has
+ * completed or flip on a crtc is completed. So DRRS should be upclocked
+ * (LOW_RR -> HIGH_RR). And also Idleness detection should be started again,
+ * if no other planes are dirty.
+ *
+ * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits.
+ */
+void intel_drrs_flush(struct drm_device *dev,
+		unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+	struct intel_panel *panel;
+	enum pipe pipe;
+
+	if (dev_priv->drrs.type == DRRS_NOT_SUPPORTED)
+		return;
+
+	cancel_delayed_work(&dev_priv->drrs.work);
+
+	mutex_lock(&dev_priv->drrs.mutex);
+	if (!dev_priv->drrs.dp) {
+		mutex_unlock(&dev_priv->drrs.mutex);
+		return;
+	}
+	panel = &dev_priv->drrs.dp->attached_connector->panel;
+	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
+	pipe = to_intel_crtc(crtc)->pipe;
+
+	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
+	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
+
+	/* flush means busy screen hence upclock */
+	if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
+		intel_drrs_set_state(dev_priv->dev,
+				     panel->fixed_mode->vrefresh);
+
+	/*
+	 * flush also means no more activity hence schedule downclock, if all
+	 * other fbs are quiescent too
+	 */
+	if (!dev_priv->drrs.busy_frontbuffer_bits)
+		schedule_delayed_work(&dev_priv->drrs.work,
+				msecs_to_jiffies(1000));
+	mutex_unlock(&dev_priv->drrs.mutex);
+}
+
+/**
+ * intel_drrs_init - Init basic DRRS work and mutex.
+ * @intel_connector: eDP connector
+ * @fixed_mode: preferred mode of panel
+ *
+ * This function is  called only once at driver load to initialize basic
+ * DRRS stuff.
+ *
+ * Returns:
+ * Downclock mode if panel supports it, else return NULL.
+ * DRRS support is determined by the presence of downclock mode (apart
+ * from VBT setting).
+ */
+struct drm_display_mode *
+intel_drrs_init(struct intel_connector *intel_connector,
+		struct drm_display_mode *fixed_mode)
+{
+	struct drm_connector *connector = &intel_connector->base;
+	struct drm_device *dev = connector->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_display_mode *downclock_mode = NULL;
+
+	INIT_DELAYED_WORK(&dev_priv->drrs.work, intel_drrs_downclock_work);
+	mutex_init(&dev_priv->drrs.mutex);
+
+	if (INTEL_INFO(dev)->gen <= 6) {
+		DRM_DEBUG_KMS("DRRS supported for Gen7 and above\n");
+		return NULL;
+	}
+
+	if (dev_priv->vbt.drrs_type != SEAMLESS_DRRS_SUPPORT) {
+		DRM_DEBUG_KMS("VBT doesn't support DRRS\n");
+		return NULL;
+	}
+
+	downclock_mode = intel_find_panel_downclock
+					(dev, fixed_mode, connector);
+
+	if (!downclock_mode) {
+		DRM_DEBUG_KMS("Downclock mode is not found. DRRS not supported\n");
+		return NULL;
+	}
+
+	dev_priv->drrs.type = dev_priv->vbt.drrs_type;
+
+	dev_priv->drrs.refresh_rate_type = DRRS_HIGH_RR;
+	DRM_DEBUG_KMS("seamless DRRS supported for eDP panel.\n");
+	return downclock_mode;
+}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d599d54..73406c3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1209,6 +1209,16 @@ u32 skl_plane_ctl_format(uint32_t pixel_format);
 u32 skl_plane_ctl_tiling(uint64_t fb_modifier);
 u32 skl_plane_ctl_rotation(unsigned int rotation);
 
+/* intel_drrs.c */
+void intel_drrs_enable(struct intel_dp *intel_dp);
+void intel_drrs_disable(struct intel_dp *intel_dp);
+void intel_drrs_invalidate(struct drm_device *dev,
+		unsigned frontbuffer_bits);
+void intel_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
+struct drm_display_mode *
+intel_drrs_init(struct intel_connector *intel_connector,
+		struct drm_display_mode *fixed_mode);
+
 /* intel_ips.c */
 bool intel_ips_ready(struct intel_crtc *crtc,
 		     struct intel_crtc_state *crtc_state);
@@ -1256,11 +1266,6 @@ void intel_dp_hot_plug(struct intel_encoder *intel_encoder);
 void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv);
 uint32_t intel_dp_pack_aux(const uint8_t *src, int src_bytes);
 void intel_plane_destroy(struct drm_plane *plane);
-void intel_edp_drrs_enable(struct intel_dp *intel_dp);
-void intel_edp_drrs_disable(struct intel_dp *intel_dp);
-void intel_edp_drrs_invalidate(struct drm_device *dev,
-		unsigned frontbuffer_bits);
-void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
 bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
 					 struct intel_digital_port *port);
 void hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config);
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index ac85357..f76952a 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -97,7 +97,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
 	}
 
 	intel_psr_invalidate(dev, obj->frontbuffer_bits);
-	intel_edp_drrs_invalidate(dev, obj->frontbuffer_bits);
+	intel_drrs_invalidate(dev, obj->frontbuffer_bits);
 	intel_fbc_invalidate(dev_priv, obj->frontbuffer_bits, origin);
 }
 
@@ -127,7 +127,7 @@ static void intel_frontbuffer_flush(struct drm_device *dev,
 	if (!frontbuffer_bits)
 		return;
 
-	intel_edp_drrs_flush(dev, frontbuffer_bits);
+	intel_drrs_flush(dev, frontbuffer_bits);
 	intel_psr_flush(dev, frontbuffer_bits, origin);
 	intel_fbc_flush(dev_priv, frontbuffer_bits, origin);
 }
-- 
2.4.3

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

[PATCH 15/31] drm/i915: Use intel_crtc instead of intel_dp on DRRS enable/disable functions.

[Rodrigo Vivi]

No functional changes at this point. Only using intel_crtc instead
of intel_dp to let DRRS enable/disable functions more flexible to be
moved around on a following patch.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c  |  4 ++--
 drivers/gpu/drm/i915/intel_drrs.c | 34 +++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h  |  4 ++--
 3 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 972acef..92918cf 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2404,7 +2404,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
 			intel_dp_stop_link_train(intel_dp);
 
 		intel_edp_backlight_on(intel_dp);
-		intel_drrs_enable(intel_dp);
+		intel_drrs_enable(intel_crtc);
 	}
 
 	if (intel_crtc->config->has_audio) {
@@ -2430,7 +2430,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 	if (type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
-		intel_drrs_disable(intel_dp);
+		intel_drrs_disable(intel_crtc);
 		intel_edp_backlight_off(intel_dp);
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_drrs.c b/drivers/gpu/drm/i915/intel_drrs.c
index dce16ea..6287aee 100644
--- a/drivers/gpu/drm/i915/intel_drrs.c
+++ b/drivers/gpu/drm/i915/intel_drrs.c
@@ -162,17 +162,17 @@ static void intel_drrs_set_state(struct drm_device *dev, int refresh_rate)
 
 /**
  * intel_drrs_enable - init drrs struct if supported
- * @intel_dp: DP struct
+ * @intel_crtc: Intel CRTC
  *
  * Initializes frontbuffer_bits and drrs.dp
  */
-void intel_drrs_enable(struct intel_dp *intel_dp)
+void intel_drrs_enable(struct intel_crtc *intel_crtc)
 {
-	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = dig_port->base.base.dev;
+	struct drm_device *dev = intel_crtc->base.dev;
 	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_crtc *crtc = &intel_crtc->base;
+	struct intel_encoder *intel_encoder;
+	struct intel_dp *intel_dp = NULL;
 
 	if (!intel_crtc->config->has_drrs) {
 		DRM_DEBUG_KMS("Panel doesn't support DRRS\n");
@@ -190,6 +190,16 @@ void intel_drrs_enable(struct intel_dp *intel_dp)
 		goto unlock;
 	}
 
+	for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
+		if (intel_encoder->type == INTEL_OUTPUT_EDP)
+			intel_dp = enc_to_intel_dp(&intel_encoder->base);
+	}
+
+	if (!intel_dp) {
+		DRM_DEBUG_KMS("No eDP found\n");
+		goto unlock;
+	}
+
 	dev_priv->drrs.busy_frontbuffer_bits = 0;
 
 	dev_priv->drrs.dp = intel_dp;
@@ -200,17 +210,14 @@ unlock:
 
 /**
  * intel_edp_drrs_disable - Disable DRRS
- * @intel_dp: DP struct
+ * @intel_crtc: Intel CRTC
  *
  */
-void intel_drrs_disable(struct intel_dp *intel_dp)
+void intel_drrs_disable(struct intel_crtc *intel_crtc)
 {
-	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = dig_port->base.base.dev;
+	struct drm_device *dev = intel_crtc->base.dev;
 	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 intel_panel *panel = &intel_dp->attached_connector->panel;
+	struct intel_panel *panel;
 
 	if (!intel_crtc->config->has_drrs)
 		return;
@@ -221,6 +228,7 @@ void intel_drrs_disable(struct intel_dp *intel_dp)
 		return;
 	}
 
+	panel = &dev_priv->drrs.dp->attached_connector->panel;
 	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
 		intel_drrs_set_state(dev_priv->dev,
 				     panel->fixed_mode->vrefresh);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 73406c3..48f461f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1210,8 +1210,8 @@ u32 skl_plane_ctl_tiling(uint64_t fb_modifier);
 u32 skl_plane_ctl_rotation(unsigned int rotation);
 
 /* intel_drrs.c */
-void intel_drrs_enable(struct intel_dp *intel_dp);
-void intel_drrs_disable(struct intel_dp *intel_dp);
+void intel_drrs_enable(struct intel_crtc *intel_crtc);
+void intel_drrs_disable(struct intel_crtc *intel_crtc);
 void intel_drrs_invalidate(struct drm_device *dev,
 		unsigned frontbuffer_bits);
 void intel_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
-- 
2.4.3

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

[PATCH 16/31] drm/i915: Fix DRRS initialization.

[Rodrigo Vivi]

With Fastboot by default we don't necessarily do a
full modeset enabling the primary plane.
So DRRS enable call that was in that path wasn't being
called anymore.

So, let's relly on post atomic modeset path
and on has_drrs to enabled DRRS when we judge necessary.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f67e2ee..6647bfe 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4681,6 +4681,9 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 	if (crtc->atomic.update_wm_post)
 		intel_update_watermarks(&crtc->base);
 
+	if (atomic->enable_drrs)
+		intel_drrs_enable(crtc);
+
 	if (atomic->update_fbc)
 		intel_fbc_update(dev_priv);
 
@@ -4702,6 +4705,9 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 
+	if (crtc->atomic.disable_drrs)
+		intel_drrs_disable(crtc);
+
 	if (atomic->disable_fbc)
 		intel_fbc_disable_crtc(crtc);
 
@@ -11565,10 +11571,15 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 			 */
 			intel_crtc->atomic.disable_ips = true;
 
+			intel_crtc->atomic.disable_drrs = true;
+
 			intel_crtc->atomic.disable_fbc = true;
 
 			intel_crtc->atomic.disable_psr = true;
 		}
+		if (visible && intel_crtc->config->has_drrs)
+			intel_crtc->atomic.enable_drrs = true;
+
 		if (visible && intel_crtc->config->ips_ready)
 			intel_crtc->atomic.enable_ips = true;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 48f461f..bf5e77c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -529,6 +529,7 @@ struct intel_mmio_flip {
  */
 struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
+	bool disable_drrs;
 	bool disable_fbc;
 	bool disable_ips;
 	bool disable_psr;
@@ -539,6 +540,7 @@ struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
 	bool wait_vblank;
+	bool enable_drrs;
 	bool update_fbc;
 	bool enable_ips;
 	bool enable_psr;
-- 
2.4.3

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

[PATCH 17/31] drm/i915: Add sys PSR toggle interface.

[Rodrigo Vivi]

This interface allows an immediate enabling of PSR feature.
What allow us to see immediately the PSR savings and will
allow us to expose this through powertop interface.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_sysfs.c | 82 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h  |  4 +-
 drivers/gpu/drm/i915/intel_psr.c  | 19 +++++++--
 3 files changed, 99 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 2d092c1..0f371c6 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -111,6 +111,81 @@ static struct attribute_group ips_attr_group = {
 	.attrs =  ips_attrs
 };
 
+static ssize_t
+psr_show(struct device *kdev, struct device_attribute *attr, char *buf)
+{
+	struct drm_minor *dminor = dev_to_drm_minor(kdev);
+	struct drm_device *dev = dminor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	ssize_t ret;
+
+	mutex_lock(&dev_priv->psr.lock);
+	ret = snprintf(buf, PAGE_SIZE, "%s\n", dev_priv->psr.enabled ?
+		       "enabled" : "disabled");
+	mutex_unlock(&dev_priv->psr.lock);
+
+	return ret;
+}
+
+
+static ssize_t
+psr_toggle(struct device *kdev, struct device_attribute *attr,
+	   const char *buf, size_t count)
+{
+	struct drm_minor *dminor = dev_to_drm_minor(kdev);
+	struct drm_device *dev = dminor->dev;
+	struct intel_connector *connector;
+	struct intel_encoder *encoder;
+	struct intel_crtc *crtc = NULL;
+	u32 val;
+	ssize_t ret;
+
+	ret = kstrtou32(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	for_each_intel_connector(dev, connector) {
+		if (!connector->base.encoder)
+			continue;
+		encoder = to_intel_encoder(connector->base.encoder);
+		crtc = to_intel_crtc(encoder->base.crtc);
+	}
+
+	if (!crtc)
+		return -ENODEV;
+
+	switch (val) {
+	case 0:
+		ret = intel_psr_disable(crtc);
+		if (ret)
+			return ret;
+		break;
+	case 1:
+		ret = intel_psr_enable(crtc);
+		if (ret)
+			return ret;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return count;
+
+
+}
+
+static DEVICE_ATTR(psr_enable, S_IRUGO | S_IWUSR, psr_show, psr_toggle);
+
+static struct attribute *psr_attrs[] = {
+	&dev_attr_psr_enable.attr,
+	NULL
+};
+
+static struct attribute_group psr_attr_group = {
+	.name = power_group_name,
+	.attrs =  psr_attrs
+};
+
 static u32 calc_residency(struct drm_device *dev, const u32 reg)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -667,6 +742,12 @@ void i915_setup_sysfs(struct drm_device *dev)
 		if (ret)
 			DRM_ERROR("IPS sysfs setup failed\n");
 	}
+	if (HAS_PSR(dev)) {
+		ret = sysfs_merge_group(&dev->primary->kdev->kobj,
+					&psr_attr_group);
+		if (ret)
+			DRM_ERROR("PSR sysfs setup failed\n");
+	}
 	if (HAS_RC6(dev)) {
 		ret = sysfs_merge_group(&dev->primary->kdev->kobj,
 					&rc6_attr_group);
@@ -724,6 +805,7 @@ void i915_teardown_sysfs(struct drm_device *dev)
 	device_remove_bin_file(dev->primary->kdev,  &dpf_attrs);
 #ifdef CONFIG_PM
 	sysfs_unmerge_group(&dev->primary->kdev->kobj, &ips_attr_group);
+	sysfs_unmerge_group(&dev->primary->kdev->kobj, &psr_attr_group);
 	sysfs_unmerge_group(&dev->primary->kdev->kobj, &rc6_attr_group);
 	sysfs_unmerge_group(&dev->primary->kdev->kobj, &rc6p_attr_group);
 #endif
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bf5e77c..76e0805 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1408,8 +1408,8 @@ void intel_backlight_unregister(struct drm_device *dev);
 /* intel_psr.c */
 bool intel_psr_ready(struct intel_dp *intel_dp,
 		     struct intel_crtc_state *pipe_config);
-void intel_psr_enable(struct intel_crtc *intel_crtc);
-void intel_psr_disable(struct intel_crtc *intel_crtc);
+int intel_psr_enable(struct intel_crtc *intel_crtc);
+int intel_psr_disable(struct intel_crtc *intel_crtc);
 void intel_psr_invalidate(struct drm_device *dev,
 			  unsigned frontbuffer_bits);
 void intel_psr_flush(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index bcf2d9d..4ca682a 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -374,21 +374,26 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
  * @intel_crtc: Intel CRTC
  *
  * This function can only be called after the pipe is fully trained and enabled.
+ *
+ * Returns:
+ * 0 on success and -errno otherwise.
  */
-void intel_psr_enable(struct intel_crtc *intel_crtc)
+int intel_psr_enable(struct intel_crtc *intel_crtc)
 {
 	struct drm_crtc *crtc = &intel_crtc->base;
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *intel_encoder;
 	struct intel_dp *intel_dp = NULL;
+	int ret = 0;
 
 	if (!intel_crtc->config->psr_ready)
-		return;
+		return -EINVAL;
 
 	mutex_lock(&dev_priv->psr.lock);
 	if (dev_priv->psr.enabled) {
 		DRM_DEBUG_KMS("PSR already in use\n");
+		ret = -EALREADY;
 		goto unlock;
 	}
 
@@ -399,6 +404,7 @@ void intel_psr_enable(struct intel_crtc *intel_crtc)
 
 	if (!intel_dp) {
 		DRM_DEBUG_KMS("No eDP found\n");
+		ret = -ENOTTY;
 		goto unlock;
 	}
 
@@ -443,6 +449,7 @@ void intel_psr_enable(struct intel_crtc *intel_crtc)
 	dev_priv->psr.enabled = intel_dp;
 unlock:
 	mutex_unlock(&dev_priv->psr.lock);
+	return ret;
 }
 
 static void vlv_psr_disable(struct intel_dp *intel_dp)
@@ -498,8 +505,11 @@ static void hsw_psr_disable(struct intel_dp *intel_dp)
  * @intel_crtc: Intel CRTC
  *
  * This function needs to be called before disabling pipe.
+ *
+ * Returns:
+ * 0 on success and -errno otherwise.
  */
-void intel_psr_disable(struct intel_crtc *intel_crtc)
+int intel_psr_disable(struct intel_crtc *intel_crtc)
 {
 	struct drm_device *dev = intel_crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -507,7 +517,7 @@ void intel_psr_disable(struct intel_crtc *intel_crtc)
 	mutex_lock(&dev_priv->psr.lock);
 	if (!dev_priv->psr.enabled) {
 		mutex_unlock(&dev_priv->psr.lock);
-		return;
+		return -EALREADY;
 	}
 
 	if (HAS_DDI(dev))
@@ -519,6 +529,7 @@ void intel_psr_disable(struct intel_crtc *intel_crtc)
 	mutex_unlock(&dev_priv->psr.lock);
 
 	cancel_delayed_work_sync(&dev_priv->psr.work);
+	return 0;
 }
 
 static void intel_psr_work(struct work_struct *work)
-- 
2.4.3

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

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

[Rodrigo Vivi]

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

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

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

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

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

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

[Rodrigo Vivi]

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

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

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

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

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

[Rodrigo Vivi]

Many reasons here:

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

Also frontbuffer tracking flush means invalidate and flush.

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

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

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

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

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

[Rodrigo Vivi]

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

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

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

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

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

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

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

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

[PATCH 22/31] drm/i915: Delay first PSR activation.

[Rodrigo Vivi]

When debuging the frozen screen caused by HW tracking with low
power state I noticed that if we keep moving the mouse non stop
you will miss the screen updates for a while. At least
until we stop moving the mouse for a small time and move again.

The actual enabling should happen immediately after
Display Port enabling sequence finished with links trained and
everything enabled. However we face many issues when enabling PSR
right after a modeset.

On VLV/CHV we face blank screens on this scenario and on HSW+
we face a recoverable frozen screen, at least until next
exit-activate sequence.

Another workaround for the same issue here would be to increase
re-enable idle time from 100 to 500 as we did for VLV/CHV.
However this patch workaround this issue in a better
way since it doesn't reduce PSR residency and also
allow us to reduce the delay time between re-enables at least
on VLV/CHV.

This is also important to make the sysfs toggle working properly.

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

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index da28f0c..6e7e8e6 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -448,6 +448,19 @@ int intel_psr_enable(struct intel_crtc *intel_crtc)
 		vlv_psr_enable_source(intel_dp);
 	}
 
+	/*
+	 * FIXME: Activation should happen immediately since this function
+	 * is just called after pipe is fully trained and enabled.
+	 * However on every platform we face issues when first activation
+	 * follows a modeset so quickly.
+	 *     - On VLV/CHV we get bank screen on first activation
+	 *     - On HSW/BDW we get a recoverable frozen screen until next
+	 *       exit-activate sequence.
+	 */
+	if (INTEL_INFO(dev)->gen < 9)
+		schedule_delayed_work(&dev_priv->psr.work,
+				      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
+
 	dev_priv->psr.enabled = intel_dp;
 unlock:
 	mutex_unlock(&dev_priv->psr.lock);
@@ -744,8 +757,9 @@ void intel_psr_flush(struct drm_device *dev,
 		intel_psr_exit(dev);
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
-		schedule_delayed_work(&dev_priv->psr.work,
-				      msecs_to_jiffies(delay_ms));
+		if (!work_busy(&dev_priv->psr.work.work))
+			schedule_delayed_work(&dev_priv->psr.work,
+					      msecs_to_jiffies(delay_ms));
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
-- 
2.4.3

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

[PATCH 23/31] drm/i915: Reduce PSR re-activation time for VLV/CHV.

[Rodrigo Vivi]

With 'commit 30886c5a ("drm/i915: VLV/CHV PSR: Increase wait delay
 time before active PSR.")' we fixed a blank screen when first
activation was happening immediately after PSR being enabled.
There we gave more time for idleness by increasing the delay
between re-activating sequences.

However, commit "drm/i915: Delay first PSR activation."
delay the first activation in a better way keeping a good PSR
residency. So, we can now reduce the delay on re-enable.

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

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 6e7e8e6..c090f38 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -738,7 +738,6 @@ void intel_psr_flush(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	enum pipe pipe;
-	int delay_ms = HAS_DDI(dev) ? 100 : 500;
 
 	mutex_lock(&dev_priv->psr.lock);
 	if (!dev_priv->psr.enabled) {
@@ -759,7 +758,7 @@ void intel_psr_flush(struct drm_device *dev,
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
 		if (!work_busy(&dev_priv->psr.work.work))
 			schedule_delayed_work(&dev_priv->psr.work,
-					      msecs_to_jiffies(delay_ms));
+					      msecs_to_jiffies(100));
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
-- 
2.4.3

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

[PATCH 24/31] drm/i915: PSR: Don't Skip aux handshake on DP_PSR_NO_TRAIN_ON_EXIT.

[Rodrigo Vivi]

Since the beginning there is a confusion on the meaning of this bit.

A previous patch had identified this already and fixed it partially:
'commit 3301d409 ("drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic")

DP_PSR_NO_TRAIN_ON_EXIT means the source doesn't need to do the
training, but it doesn't tell to avoid TP patterns or to skip
aux handshake.

This patch fixes the hard freeze reported.

Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91436
Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91437

Cc: Ivan Mitev <ivan.mitev@gmail.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index c090f38..4e88e2e 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -263,7 +263,6 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 		   send the minimal TP1 possible and skip TP2. */
 		val |= EDP_PSR_TP1_TIME_100us;
 		val |= EDP_PSR_TP2_TP3_TIME_0us;
-		val |= EDP_PSR_SKIP_AUX_EXIT;
 		/* Sink should be able to train with the 5 or 6 idle patterns */
 		idle_frames += 4;
 	}
-- 
2.4.3

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

[PATCH 25/31] drm/i915: Send TP1 TP2/3 even when panel claims no NO_TRAIN_ON_EXIT.

[Rodrigo Vivi]

On the commit 3301d4092106 ("drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic")'
we already had identified that DP_PSR_NO_TRAIN_ON_EXIT
doesn't mean we shouldn't send TPS patterns, however we start sending the
minimal TP1 as possible and no TP2.

For most of the panels this is ok, but we found a reported case where
this is not true and panel keeps frozen without updating the screen for a while.

We could just get this case after patch "PSR: Don't Skip aux handshake on
DP_PSR_NO_TRAIN_ON_EXIT." is applied since that one fix the
hard freeze on this kind of panels.

Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91436#c19

Cc: Ivan Mitev <ivan.mitev@gmail.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 4e88e2e..ee426ea 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -259,10 +259,6 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 	const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
 
 	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) {
-		/* It doesn't mean we shouldn't send TPS patters, so let's
-		   send the minimal TP1 possible and skip TP2. */
-		val |= EDP_PSR_TP1_TIME_100us;
-		val |= EDP_PSR_TP2_TP3_TIME_0us;
 		/* Sink should be able to train with the 5 or 6 idle patterns */
 		idle_frames += 4;
 	}
-- 
2.4.3

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

[PATCH 26/31] drm/i915: Fix idle_frames counter.

[Rodrigo Vivi]

'commit 97173eaf5 ("drm/i915: PSR: Increase idle_frames")' was a mistake.
The special case it tried to cover is already being covered by
the DP_PSR_NO_TRAIN_ON_EXIT. So let's remove this mistake instead of
start using 9 idle frames for that case.

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

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index ee426ea..4e3d315 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -249,12 +249,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 	/* Lately it was identified that depending on panel idle frame count
 	 * calculated at HW can be off by 1. So let's use what came
 	 * from VBT + 1.
-	 * There are also other cases where panel demands at least 4
-	 * but VBT is not being set. To cover these 2 cases lets use
-	 * at least 5 when VBT isn't set to be on the safest side.
 	 */
 	uint32_t idle_frames = dev_priv->vbt.psr.idle_frames ?
-			       dev_priv->vbt.psr.idle_frames + 1 : 5;
+			       dev_priv->vbt.psr.idle_frames + 1 : 2;
 	uint32_t val = 0x0;
 	const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
 
-- 
2.4.3

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

[PATCH 27/31] drm/i915: Allow 1 vblank to let Sink CRC calculation to start or stop.

[Rodrigo Vivi]

According to VESA DP Spec, setting TEST_SINK_START (bit 0)
of TEST_SINK (00270h) "Stop/Start calculating CRC on the next frame"

So let's wait at least 1 vblank to really say the calculation
stopped or started.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4feb47e..c0fa90a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3802,6 +3802,7 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
 static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
 	u8 buf;
 	int ret = 0;
@@ -3819,6 +3820,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 		goto out;
 	}
 
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
 	intel_dp->sink_crc.started = false;
  out:
 	intel_ips_enable(intel_crtc);
@@ -3828,6 +3830,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
 	u8 buf;
 	int ret;
@@ -3857,6 +3860,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 		return -EIO;
 	}
 
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
 	intel_dp->sink_crc.started = true;
 	return 0;
 }
-- 
2.4.3

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

[PATCH 28/31] drm/i915: Make Sink crc calculation waiting for counter to reset.

[Rodrigo Vivi]

According to VESA DP spec TEST_CRC_COUNT (Bits 3:0) at
TEST_SINK_MISC (00246h) is "Reset to 0 when TEST_SINK bit 0 = 0;

So let's give few vblanks so we are really sure that this counter
is really zeroed on the next sink_crc read.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c0fa90a..5d810cd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3806,6 +3806,8 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
 	u8 buf;
 	int ret = 0;
+	int count = 0;
+	int attempts = 10;
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
 		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
@@ -3820,7 +3822,22 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 		goto out;
 	}
 
-	intel_wait_for_vblank(dev, intel_crtc->pipe);
+	do {
+		intel_wait_for_vblank(dev, intel_crtc->pipe);
+
+		if (drm_dp_dpcd_readb(&intel_dp->aux,
+				      DP_TEST_SINK_MISC, &buf) < 0) {
+			ret = -EIO;
+			goto out;
+		}
+		count = buf & DP_TEST_COUNT_MASK;
+	} while (--attempts && count);
+
+	if (attempts == 0) {
+		DRM_ERROR("TIMEOUT: Sink CRC counter is not zeroed\n");
+		ret = -ETIMEDOUT;
+	}
+
 	intel_dp->sink_crc.started = false;
  out:
 	intel_ips_enable(intel_crtc);
-- 
2.4.3

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

[PATCH 29/31] drm/i915: Stop tracking last calculated Sink CRC.

[Rodrigo Vivi]

It was created at 'commit aabc95dcf20 (drm/i915: Dont -ETIMEDOUT
on identical new and previous (count, crc).")' becase the counter
wasn't reliable.

Now that we properly wait for the counter to be reset we can rely
a bit more in the counter.

Also that patch stopped to return -ETIMEDOUT so the test case is
unable to skip when it is unreliable and end up in many fails
that should be skip instead.

So, with the counter more reliable we can remove
this hack that just makes things more confusing when test cases
are really expecting the same CRC and let test case skip if that's
not the case.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 39 +++++++++------------------------------
 drivers/gpu/drm/i915/intel_drv.h |  2 --
 2 files changed, 9 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5d810cd..24d2bff 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3864,8 +3864,6 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 	if (!(buf & DP_TEST_CRC_SUPPORTED))
 		return -ENOTTY;
 
-	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
-
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
 		return -EIO;
 
@@ -3890,7 +3888,6 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 	u8 buf;
 	int count, ret;
 	int attempts = 6;
-	bool old_equal_new;
 
 	ret = intel_dp_sink_crc_start(intel_dp);
 	if (ret)
@@ -3906,35 +3903,17 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 		}
 		count = buf & DP_TEST_COUNT_MASK;
 
-		/*
-		 * Count might be reset during the loop. In this case
-		 * last known count needs to be reset as well.
-		 */
-		if (count == 0)
-			intel_dp->sink_crc.last_count = 0;
-
-		if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
-			ret = -EIO;
-			goto stop;
-		}
-
-		old_equal_new = (count == intel_dp->sink_crc.last_count &&
-				 !memcmp(intel_dp->sink_crc.last_crc, crc,
-					 6 * sizeof(u8)));
-
-	} while (--attempts && (count == 0 || old_equal_new));
-
-	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
-	memcpy(intel_dp->sink_crc.last_crc, crc, 6 * sizeof(u8));
+	} while (--attempts && count == 0);
 
 	if (attempts == 0) {
-		if (old_equal_new) {
-			DRM_DEBUG_KMS("Unreliable Sink CRC counter: Current returned CRC is identical to the previous one\n");
-		} else {
-			DRM_ERROR("Panel is unable to calculate any CRC after 6 vblanks\n");
-			ret = -ETIMEDOUT;
-			goto stop;
-		}
+		DRM_ERROR("Panel is unable to calculate any CRC after 6 vblanks\n");
+		ret = -ETIMEDOUT;
+		goto stop;
+	}
+
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
+		ret = -EIO;
+		goto stop;
 	}
 
 stop:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index baa560c..de2a8ff 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -746,8 +746,6 @@ enum link_m_n_set {
 
 struct sink_crc {
 	bool started;
-	u8 last_crc[6];
-	int last_count;
 };
 
 struct intel_dp {
-- 
2.4.3

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

[PATCH 30/31] drm/i915: Rely on TEST_SINK_START instead of tracking Sink CRC state on dev_priv.

[Rodrigo Vivi]

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 14 ++++++--------
 drivers/gpu/drm/i915/intel_drv.h |  5 -----
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 24d2bff..5e14f07 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3838,7 +3838,6 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 		ret = -ETIMEDOUT;
 	}
 
-	intel_dp->sink_crc.started = false;
  out:
 	intel_ips_enable(intel_crtc);
 	return ret;
@@ -3852,12 +3851,6 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 	u8 buf;
 	int ret;
 
-	if (intel_dp->sink_crc.started) {
-		ret = intel_dp_sink_crc_stop(intel_dp);
-		if (ret)
-			return ret;
-	}
-
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
 		return -EIO;
 
@@ -3867,6 +3860,12 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
 		return -EIO;
 
+	if (buf & DP_TEST_SINK_START) {
+		ret = intel_dp_sink_crc_stop(intel_dp);
+		if (ret)
+			return ret;
+	}
+
 	intel_ips_disable(intel_crtc);
 
 	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
@@ -3876,7 +3875,6 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 	}
 
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
-	intel_dp->sink_crc.started = true;
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index de2a8ff..c28f24a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -744,10 +744,6 @@ enum link_m_n_set {
 	M2_N2
 };
 
-struct sink_crc {
-	bool started;
-};
-
 struct intel_dp {
 	uint32_t output_reg;
 	uint32_t aux_ch_ctl_reg;
@@ -764,7 +760,6 @@ struct intel_dp {
 	/* sink rates as reported by DP_SUPPORTED_LINK_RATES */
 	uint8_t num_sink_rates;
 	int sink_rates[DP_MAX_SUPPORTED_RATES];
-	struct sink_crc sink_crc;
 	struct drm_dp_aux aux;
 	uint8_t train_set[4];
 	int panel_power_up_delay;
-- 
2.4.3

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

[PATCH 31/31] drm/i915: Enable PSR by default.

[Rodrigo Vivi]

With a reliable frontbuffer tracking and all instability corner cases
solved let's re-enabled PSR by default on all supported platforms.

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

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 368df67..ce20917 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -36,7 +36,7 @@ struct i915_params i915 __read_mostly = {
 	.enable_execlists = -1,
 	.enable_hangcheck = true,
 	.enable_ppgtt = -1,
-	.enable_psr = 0,
+	.enable_psr = 1,
 	.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
 	.disable_power_well = 1,
 	.enable_ips = 1,
@@ -119,7 +119,7 @@ MODULE_PARM_DESC(enable_execlists,
 	"(-1=auto [default], 0=disabled, 1=enabled)");
 
 module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600);
-MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)");
+MODULE_PARM_DESC(enable_psr, "Enable PSR (default: true)");
 
 module_param_named_unsafe(preliminary_hw_support, i915.preliminary_hw_support, int, 0600);
 MODULE_PARM_DESC(preliminary_hw_support,
-- 
2.4.3

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

Re: [PATCH 17/31] drm/i915: Add sys PSR toggle interface.

[Chris Wilson]

On Thu, Nov 05, 2015 at 10:50:09AM -0800, Rodrigo Vivi wrote:
> This interface allows an immediate enabling of PSR feature.
> What allow us to see immediately the PSR savings and will
> allow us to expose this through powertop interface.

Pardon? It's a connector property.

> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_sysfs.c | 82 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h  |  4 +-
>  drivers/gpu/drm/i915/intel_psr.c  | 19 +++++++--
>  3 files changed, 99 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 2d092c1..0f371c6 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -111,6 +111,81 @@ static struct attribute_group ips_attr_group = {
>  	.attrs =  ips_attrs
>  };
>  
> +static ssize_t
> +psr_show(struct device *kdev, struct device_attribute *attr, char *buf)
> +{
> +	struct drm_minor *dminor = dev_to_drm_minor(kdev);
> +	struct drm_device *dev = dminor->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	ssize_t ret;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +	ret = snprintf(buf, PAGE_SIZE, "%s\n", dev_priv->psr.enabled ?
> +		       "enabled" : "disabled");
> +	mutex_unlock(&dev_priv->psr.lock);

Superfluous locking.

> +
> +	return ret;
> +}
> +
> +
> +static ssize_t
> +psr_toggle(struct device *kdev, struct device_attribute *attr,
> +	   const char *buf, size_t count)
> +{
> +	struct drm_minor *dminor = dev_to_drm_minor(kdev);
> +	struct drm_device *dev = dminor->dev;
> +	struct intel_connector *connector;
> +	struct intel_encoder *encoder;
> +	struct intel_crtc *crtc = NULL;
> +	u32 val;
> +	ssize_t ret;
> +
> +	ret = kstrtou32(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	for_each_intel_connector(dev, connector) {
> +		if (!connector->base.encoder)
> +			continue;
> +		encoder = to_intel_encoder(connector->base.encoder);
> +		crtc = to_intel_crtc(encoder->base.crtc);
> +	}

"Let's pick a connector at random".
-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

Re: [PATCH 07/31] drm/i915: IPS Sysfs interface.

[Chris Wilson]

On Thu, Nov 05, 2015 at 10:49:59AM -0800, Rodrigo Vivi wrote:
> With the lock in place we can expose ips enabled/disable on sysfs
> for developing, debugging and information purposes.

No. sysfs is not for developement, debugging or information. It is a
userspace ABI.
-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

Re: [PATCH 31/31] drm/i915: Enable PSR by default.

[Chris Wilson]

On Thu, Nov 05, 2015 at 10:50:23AM -0800, Rodrigo Vivi wrote:
> With a reliable frontbuffer tracking and all instability corner cases
> solved let's re-enabled PSR by default on all supported platforms.

Nope. You still haven't provided the Panel Self-Refresh property for
userspace to know what's going on.
-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

Re: [PATCH 31/31] drm/i915: Enable PSR by default.

[Vivi, Rodrigo]

On Thu, 2015-11-05 at 21:07 +0000, Chris Wilson wrote:
> On Thu, Nov 05, 2015 at 10:50:23AM -0800, Rodrigo Vivi wrote:
> > With a reliable frontbuffer tracking and all instability corner 
> > cases
> > solved let's re-enabled PSR by default on all supported platforms.
> 
> Nope. You still haven't provided the Panel Self-Refresh property for
> userspace to know what's going on.

Yes, I have. But it was nacked by Daniel.

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

Re: [PATCH 04/31] drm/i915: Handle actual IPS enabled state.

[Daniel Stone]

Hi,

On 5 November 2015 at 18:49, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> +void intel_ips_init(struct drm_i915_private dev_priv)

That would be *dev_priv.

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

Re: [PATCH 24/31] drm/i915: PSR: Don't Skip aux handshake on DP_PSR_NO_TRAIN_ON_EXIT.

[Jani Nikula]

On Thu, 05 Nov 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> Since the beginning there is a confusion on the meaning of this bit.
>
> A previous patch had identified this already and fixed it partially:
> 'commit 3301d409 ("drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic")
>
> DP_PSR_NO_TRAIN_ON_EXIT means the source doesn't need to do the
> training, but it doesn't tell to avoid TP patterns or to skip
> aux handshake.
>
> This patch fixes the hard freeze reported.
>
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91436
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91437
>
> Cc: Ivan Mitev <ivan.mitev@gmail.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

I'm unhappy about mixing fixes like this in 30+ patch series.

Jani.


> ---
>  drivers/gpu/drm/i915/intel_psr.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index c090f38..4e88e2e 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -263,7 +263,6 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>  		   send the minimal TP1 possible and skip TP2. */
>  		val |= EDP_PSR_TP1_TIME_100us;
>  		val |= EDP_PSR_TP2_TP3_TIME_0us;
> -		val |= EDP_PSR_SKIP_AUX_EXIT;
>  		/* Sink should be able to train with the 5 or 6 idle patterns */
>  		idle_frames += 4;
>  	}

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

Re: [PATCH 25/31] drm/i915: Send TP1 TP2/3 even when panel claims no NO_TRAIN_ON_EXIT.

[Jani Nikula]

On Thu, 05 Nov 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On the commit 3301d4092106 ("drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic")'
> we already had identified that DP_PSR_NO_TRAIN_ON_EXIT
> doesn't mean we shouldn't send TPS patterns, however we start sending the
> minimal TP1 as possible and no TP2.
>
> For most of the panels this is ok, but we found a reported case where
> this is not true and panel keeps frozen without updating the screen for a while.
>
> We could just get this case after patch "PSR: Don't Skip aux handshake on
> DP_PSR_NO_TRAIN_ON_EXIT." is applied since that one fix the
> hard freeze on this kind of panels.
>
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91436#c19
>
> Cc: Ivan Mitev <ivan.mitev@gmail.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Ditto. Do these really depend on all the other patches in the series?

Jani.

> ---
>  drivers/gpu/drm/i915/intel_psr.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 4e88e2e..ee426ea 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -259,10 +259,6 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>  	const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
>  
>  	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) {
> -		/* It doesn't mean we shouldn't send TPS patters, so let's
> -		   send the minimal TP1 possible and skip TP2. */
> -		val |= EDP_PSR_TP1_TIME_100us;
> -		val |= EDP_PSR_TP2_TP3_TIME_0us;
>  		/* Sink should be able to train with the 5 or 6 idle patterns */
>  		idle_frames += 4;
>  	}

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

Re: [PATCH 07/31] drm/i915: IPS Sysfs interface.

[Daniel Stone]

Hi Rodrigo,

On 5 November 2015 at 18:49, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> diff --git a/drivers/gpu/drm/i915/intel_ips.c b/drivers/gpu/drm/i915/intel_ips.c
> index b867aba..6bc5c55 100644
> --- a/drivers/gpu/drm/i915/intel_ips.c
> +++ b/drivers/gpu/drm/i915/intel_ips.c
> @@ -105,18 +105,21 @@ bool intel_ips_ready(struct intel_crtc *crtc,
>   * This function is called to enable IPS on certain pipe.
>   * All needed conditions should've checked already by intel_ips_ready.
>   */
> -void intel_ips_enable(struct intel_crtc *crtc)
> +int intel_ips_enable(struct intel_crtc *crtc)
>  {
>         struct drm_device *dev = crtc->base.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> +       int ret = 0;
>
>         if (!crtc->config->ips_ready)
> -               return;
> +               return -EINVAL;
>
>         mutex_lock(&dev_priv->display_ips.lock);
>
> -       if (dev_priv->display_ips.enabled)
> +       if (dev_priv->display_ips.enabled) {
> +               ret = -EALREADY;
>                 goto out;
> +       }
>
>         /*
>          * We can only enable IPS after we enable a plane
> @@ -147,6 +150,7 @@ void intel_ips_enable(struct intel_crtc *crtc)
>                  */
>                 if (wait_for(I915_READ_NOTRACE(IPS_CTL) & IPS_ENABLE, 50)) {
>                         DRM_ERROR("Timed out waiting for IPS enable\n");
> +                       ret = -ETIMEDOUT;
>                         goto out;
>                 }
>         }
> @@ -154,6 +158,7 @@ void intel_ips_enable(struct intel_crtc *crtc)
>         dev_priv->display_ips.enabled = true;
>  out:
>         mutex_unlock(&dev_priv->display_ips.lock);
> +       return ret;
>  }
>
>  /**
> @@ -162,16 +167,22 @@ out:
>   *
>   * This function is called to disable IPS on certain pipe whenever it is needed
>   * to disable IPS on the pipe.
> + *
> + * Returns:
> + * 0 on success and -errno otherwise.
>   */
> -void intel_ips_disable(struct intel_crtc *crtc)
> +int intel_ips_disable(struct intel_crtc *crtc)
>  {
>         struct drm_device *dev = crtc->base.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> +       int ret = 0;
>
>         mutex_lock(&dev_priv->display_ips.lock);
>
> -       if (!dev_priv->display_ips.enabled)
> +       if (!dev_priv->display_ips.enabled) {
> +               ret = -EALREADY;
>                 goto out;
> +       }
>
>         assert_plane_enabled(dev_priv, crtc->plane);
>         if (IS_BROADWELL(dev)) {
> @@ -196,6 +207,7 @@ void intel_ips_disable(struct intel_crtc *crtc)
>         dev_priv->display_ips.enabled = false;
>  out:
>         mutex_unlock(&dev_priv->display_ips.lock);
> +       return ret;
>  }

It would be nice to have these from the beginning, rather than
modifying them part-way through.

> @@ -206,6 +218,9 @@ out:
>   * It checks if there is any other plane enabled on the pipe when primary is
>   * going to be disabled. In this case IPS can continue enabled, but it needs
>   * to be disabled otherwise.
> + *
> + * Returns:
> + * 0 on success and -errno otherwise.
>   */
>  void intel_ips_disable_if_alone(struct intel_crtc *crtc)
>  {

... this one is still left void.

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

Re: [PATCH 00/31] IPS/DRRS/PSR rework with PSR enabled by default

[Daniel Stone]

Hi Rodrigo,

On 5 November 2015 at 18:49, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> So I'm confident we can enable PSR back by default now.
>
> All comments, ideas, suggestions and even bikesheddings are pretty welcome.

You did ask for it ...

I've been looking at pulling this on top of Maarten's tree, and
currently my overriding wish is that, rather than the checks sprinkled
all over various state-change functions, we instead had:
static bool intel_ips_should_enable(struct intel_crtc_state *crtc_state)

In the pre-atomic commit path, this could look like:
    bool ips = intel_ips_should_enable(crtc_state);
    if (ips && !intel_crtc->ips_enabled)
        intel_ips_enable(intel_crtc);
    else if (!ips && intel_crtc->ips_enabled)
        intel_ips_disable(intel_crtc);

Post-atomic, this would be:
    intel_flip_work->enable_ips = intel_ips_should_enable(crtc_state);
and actually doing the enable/disable in the work handler.

Having one place to inspect the state overall seems better, e.g. in
the case where we disable the primary plane but retain an overlay
plane on a CRTC, we keep IPS enabled. However, it doesn't seem like
there's anything to handle the case where we then disable that overlay
plane, where with no planes enabled at all, IPS should be disabled.

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

Re: [PATCH 24/31] drm/i915: PSR: Don't Skip aux handshake on DP_PSR_NO_TRAIN_ON_EXIT.

[Vivi, Rodrigo]

On Mon, 2015-11-09 at 12:38 +0200, Jani Nikula wrote:
> On Thu, 05 Nov 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > Since the beginning there is a confusion on the meaning of this 
> > bit.
> > 
> > A previous patch had identified this already and fixed it 
> > partially:
> > 'commit 3301d409 ("drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT 
> > logic")
> > 
> > DP_PSR_NO_TRAIN_ON_EXIT means the source doesn't need to do the
> > training, but it doesn't tell to avoid TP patterns or to skip
> > aux handshake.
> > 
> > This patch fixes the hard freeze reported.
> > 
> > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91436
> > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91437
> > 
> > Cc: Ivan Mitev <ivan.mitev@gmail.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> I'm unhappy about mixing fixes like this in 30+ patch series.

I'm sorry about that.
I've wondered if you would prefer those separated but I concluded that
since PSR is disabled by default it would be better to let all fixes
and rework togheter to avoid having the patch that enablas it by
default in a branch without this fix or in the wrong order.

But it is really up to you so if it makes you happy I'm going to split
the series.

> 
> Jani.
> 
> 
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index c090f38..4e88e2e 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -263,7 +263,6 @@ static void hsw_psr_enable_source(struct 
> > intel_dp *intel_dp)
> >  		   send the minimal TP1 possible and skip TP2. */
> >  		val |= EDP_PSR_TP1_TIME_100us;
> >  		val |= EDP_PSR_TP2_TP3_TIME_0us;
> > -		val |= EDP_PSR_SKIP_AUX_EXIT;
> >  		/* Sink should be able to train with the 5 or 6 
> > idle patterns */
> >  		idle_frames += 4;
> >  	}
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 25/31] drm/i915: Send TP1 TP2/3 even when panel claims no NO_TRAIN_ON_EXIT.

[Vivi, Rodrigo]

On Mon, 2015-11-09 at 12:39 +0200, Jani Nikula wrote:
> On Thu, 05 Nov 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > On the commit 3301d4092106 ("drm/i915: PSR: Fix 
> > DP_PSR_NO_TRAIN_ON_EXIT logic")'
> > we already had identified that DP_PSR_NO_TRAIN_ON_EXIT
> > doesn't mean we shouldn't send TPS patterns, however we start 
> > sending the
> > minimal TP1 as possible and no TP2.
> > 
> > For most of the panels this is ok, but we found a reported case 
> > where
> > this is not true and panel keeps frozen without updating the screen 
> > for a while.
> > 
> > We could just get this case after patch "PSR: Don't Skip aux 
> > handshake on
> > DP_PSR_NO_TRAIN_ON_EXIT." is applied since that one fix the
> > hard freeze on this kind of panels.
> > 
> > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91436#c19
> > 
> > Cc: Ivan Mitev <ivan.mitev@gmail.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Ditto. Do these really depend on all the other patches in the series?

No, but the last patch in the series depend on this and on the rest of
the series.

> 
> Jani.
> 
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 4e88e2e..ee426ea 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -259,10 +259,6 @@ static void hsw_psr_enable_source(struct 
> > intel_dp *intel_dp)
> >  	const uint32_t link_entry_time = 
> > EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
> >  
> >  	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) {
> > -		/* It doesn't mean we shouldn't send TPS patters, 
> > so let's
> > -		   send the minimal TP1 possible and skip TP2. */
> > -		val |= EDP_PSR_TP1_TIME_100us;
> > -		val |= EDP_PSR_TP2_TP3_TIME_0us;
> >  		/* Sink should be able to train with the 5 or 6 
> > idle patterns */
> >  		idle_frames += 4;
> >  	}
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/31] IPS/DRRS/PSR rework with PSR enabled by default

[Vivi, Rodrigo]

On Mon, 2015-11-09 at 11:47 +0000, Daniel Stone wrote:
> Hi Rodrigo,
> 
> On 5 November 2015 at 18:49, Rodrigo Vivi <rodrigo.vivi@intel.com> 
> wrote:
> > So I'm confident we can enable PSR back by default now.
> > 
> > All comments, ideas, suggestions and even bikesheddings are pretty 
> > welcome.
> 
> You did ask for it ...

Thank you! I really appreciate your comments.

> 
> I've been looking at pulling this on top of Maarten's tree, and

I'm afraid I didn't followed completely your idea and maybe because I
don't know the code you are referring to. What tree specifically are
you talking about?

> currently my overriding wish is that, rather than the checks 
> sprinkled
> all over various state-change functions,

Ok, so you are suggesting we totally remove the pipe config variables
right? I haven't considered this possibility since I believed some
people would prefer to let the variables there. And also for PSRxDRRS
conflict this would be safier since DRRS cannot be enabled if PSR is
going to be enabled. With your solution we need to pay attention and
never let someone invert PSR and DRRS orders.


>  we instead had:
> static bool intel_ips_should_enable(struct intel_crtc_state 
> *crtc_state)
> 
> In the pre-atomic commit path, this could look like:
>     bool ips = intel_ips_should_enable(crtc_state);
>     if (ips && !intel_crtc->ips_enabled)
>         intel_ips_enable(intel_crtc);
>     else if (!ips && intel_crtc->ips_enabled)
>         intel_ips_disable(intel_crtc);
> 
> Post-atomic, this would be:
>     intel_flip_work->enable_ips = 
> intel_ips_should_enable(crtc_state);
> and actually doing the enable/disable in the work handler.
> 
> Having one place to inspect the state overall seems better, e.g. in
> the case where we disable the primary plane but retain an overlay
> plane on a CRTC, we keep IPS enabled. However, it doesn't seem like
> there's anything to handle the case where we then disable that 
> overlay
> plane, where with no planes enabled at all, IPS should be disabled.
> 
> Cheers,
> Daniel

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

Re: [PATCH 00/31] IPS/DRRS/PSR rework with PSR enabled by default

[Daniel Stone]

Hi Rodrigo,

On 10 November 2015 at 15:57, Vivi, Rodrigo <rodrigo.vivi@intel.com> wrote:
> On Mon, 2015-11-09 at 11:47 +0000, Daniel Stone wrote:
>> I've been looking at pulling this on top of Maarten's tree, and
>
> I'm afraid I didn't followed completely your idea and maybe because I
> don't know the code you are referring to. What tree specifically are
> you talking about?

Ah, it's at git://people.freedesktop.org/~mlankhorst/linux#rework-page-flip.
The code itself isn't that important though - it makes for a very
messy rebase but has little functional impact - it's more about the
style overall. I'll try to explain a bit better. :\

>> currently my overriding wish is that, rather than the checks
>> sprinkled
>> all over various state-change functions,
>
> Ok, so you are suggesting we totally remove the pipe config variables
> right? I haven't considered this possibility since I believed some
> people would prefer to let the variables there. And also for PSRxDRRS
> conflict this would be safier since DRRS cannot be enabled if PSR is
> going to be enabled. With your solution we need to pay attention and
> never let someone invert PSR and DRRS orders.

Hm, not so much removing as making more use of them!

At the moment, the IPS/PSR logic is quite distributed. Taking this as
an example, from patch #6:

@@ -11562,12 +11554,8 @@ int intel_plane_atomic_calc_changes(struct
drm_crtc_state *crtc_state,

                if (turn_off) {
                        /*
-                        * FIXME: Actually if we will still have any other
-                        * plane enabled on the pipe we could let IPS enabled
-                        * still, but for now lets consider that when we make
-                        * primary invisible by setting DSPCNTR to 0 on
-                        * update_primary_plane function IPS needs to be
-                        * disable.
+                        * IPS disable_if_alone function will be called
+                        * in order to decide if IPS disable is really needed.
                         */
                        intel_crtc->atomic.disable_ips = true;

This gets called when the primary plane is being disabled. So the logic is:
  - if IPS was enabled
  - and the primary plane was on
  - and the primary plane was no longer on
  - maybe disable the plane

The implementation is buggy (see reply to patch coming shortly), but
more importantly, I think the state-machine concept - where you react
to very specific changes (e.g. 'disable a plane') is quite fragile. As
an example, there is nothing I can see in your patch that handles the
following case:
  - IPS is enabled with one primary and one overlay plane
  - disable the primary plane: intel_ips_disable_if_alone() is called,
and does not disable IPS, since the overlay plane is still active
  - disable the overlay plane, but IPS remains enabled even though no
planes are active

For me, a better way of approaching this - very much in line with the
atomic conversion work that has been ongoing ever since
intel_crtc_state was created - would be to examine the actual state as
a whole. So, rather than performing specific checks in response to one
specific action (disabling the primary plane), you would have one
function in intel_ips.c, which examines the entire CRTC state, and
returns a boolean answer: should IPS be enabled or disabled?

This keeps the logic in one place, and means that rather than
sprinkling magic enable/disable calls in response to specific
_actions_ - which gets difficult to do anyway with atomic changes -
you only ever perform enable/disable on the result of one function
which inspects the overall state ('are there any planes enabled in
this target state?') and comes up with an answer. Thus the interaction
with the core code is limited to:
    bool target_ips = intel_ips_target_state(intel_crtc_state);

Does that make sense? It's just about deriving the conditions from an
overall target state, rather than making them reactive to specific
actions.

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

Re: [PATCH 06/31] drm/i915: Fix IPS disable sequence.

[Daniel Stone]

Hi,

On 5 November 2015 at 18:49, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>  /**
> + * intel_ips_disable_if_alone - Disable IPS if alone in the pipe.
> + * @crtc: intel crtc
> + *
> + * This function should be called when primary plane is being disabled.
> + * It checks if there is any other plane enabled on the pipe when primary is
> + * going to be disabled. In this case IPS can continue enabled, but it needs
> + * to be disabled otherwise.
> + */

As an example of what I meant before, I would reword this to reflect
its actual functionality, which doesn't necessarily have anything to
do specifically with disabling the primary plane:
'This function examines the CRTC state to determine if IPS should
be disabled. Currently, IPS is disabled if no planes are active on the
CRTC.'

Discussing its use in the context of disabling the primary plane I
think obscures its intent, and also introduces a bug. :)

> +void intel_ips_disable_if_alone(struct intel_crtc *crtc)
> +{
> +       struct drm_device *dev = crtc->base.dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       bool ips_enabled;
> +       struct intel_plane *intel_plane;
> +
> +       mutex_lock(&dev_priv->display_ips.lock);
> +       ips_enabled = dev_priv->display_ips.enabled;
> +       mutex_unlock(&dev_priv->display_ips.lock);
> +
> +       if (!ips_enabled)
> +               return;
> +
> +       for_each_intel_plane_on_crtc(dev, crtc, intel_plane) {
> +               enum plane plane = intel_plane->plane;
> +
> +               if (plane != PLANE_A &&
> +                   !!(I915_READ(DSPCNTR(plane)) & DISPLAY_PLANE_ENABLE))
> +                       return;
> +               intel_ips_disable(crtc);
> +       }
> +}

Rather than reading the registers, this should just inspect
plane_state->visible. Reading the registers introduces the same bug as
I mentioned the last mail, but in a different way:
  - IPS is enabled
  - primary and overlay planes are both enabled
  - user commits an atomic state which disables both primary and
overlay planes, so IPS must be disabled
  - disabling the primary plane calls this function, which sees that
the overlay plane is still active, so IPS can remain enabled
  - the overlay plane gets disabled, with IPS still active
  - :(

Making this work on states would eliminate this entire class of bugs,
and also make it much easier to handle async modesets.

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

Re: [PATCH 27/31] drm/i915: Allow 1 vblank to let Sink CRC calculation to start or stop.

[Paulo Zanoni]

2015-11-05 16:50 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> According to VESA DP Spec, setting TEST_SINK_START (bit 0)
> of TEST_SINK (00270h) "Stop/Start calculating CRC on the next frame"
>
> So let's wait at least 1 vblank to really say the calculation
> stopped or started.

I'm hoping this will fix bugs somewhere.

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

>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 4feb47e..c0fa90a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3802,6 +3802,7 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
>  static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>  {
>         struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +       struct drm_device *dev = dig_port->base.base.dev;
>         struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
>         u8 buf;
>         int ret = 0;
> @@ -3819,6 +3820,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>                 goto out;
>         }
>
> +       intel_wait_for_vblank(dev, intel_crtc->pipe);
>         intel_dp->sink_crc.started = false;
>   out:
>         intel_ips_enable(intel_crtc);
> @@ -3828,6 +3830,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>  static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>  {
>         struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +       struct drm_device *dev = dig_port->base.base.dev;
>         struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
>         u8 buf;
>         int ret;
> @@ -3857,6 +3860,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>                 return -EIO;
>         }
>
> +       intel_wait_for_vblank(dev, intel_crtc->pipe);
>         intel_dp->sink_crc.started = true;
>         return 0;
>  }
> --
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 28/31] drm/i915: Make Sink crc calculation waiting for counter to reset.

[Paulo Zanoni]

2015-11-05 16:50 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> According to VESA DP spec TEST_CRC_COUNT (Bits 3:0) at
> TEST_SINK_MISC (00246h) is "Reset to 0 when TEST_SINK bit 0 = 0;
>
> So let's give few vblanks so we are really sure that this counter
> is really zeroed on the next sink_crc read.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c0fa90a..5d810cd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3806,6 +3806,8 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>         struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
>         u8 buf;
>         int ret = 0;
> +       int count = 0;
> +       int attempts = 10;
>
>         if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
>                 DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> @@ -3820,7 +3822,22 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>                 goto out;
>         }
>
> -       intel_wait_for_vblank(dev, intel_crtc->pipe);
> +       do {
> +               intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
> +               if (drm_dp_dpcd_readb(&intel_dp->aux,
> +                                     DP_TEST_SINK_MISC, &buf) < 0) {
> +                       ret = -EIO;
> +                       goto out;

This "goto out" will make sink_crc.started remain as true even though
we already sent the DPCD message telling it to stop, and it
acknowledged our message. And it won't even print stuff on dmesg. I
guess I'd probably write something on dmesg and flip started to false.

> +               }
> +               count = buf & DP_TEST_COUNT_MASK;
> +       } while (--attempts && count);
> +
> +       if (attempts == 0) {
> +               DRM_ERROR("TIMEOUT: Sink CRC counter is not zeroed\n");

The other errors are all DRM_DEBUG_KMS. On one hand we can't do
anything about them since they're most likely panel errors so
DRM_ERROR doesn't look good. On the other hand normal users are not
going to ever run this code, and DRM_ERROR may make us - and our
testing robots - notice the possible failures, so maybe DRM_ERROR is
the way to go here. Anyway, we should be consistent regardless of the
decision.

Besides, at intel_dp_sink_crc_start(), we read the last_count, but
it's supposed to be zero. Can't we use a check for this there too?
Maybe just an informative DRM_DEBUG_KMS("this was supposed to be zero
but it's not\n") without really returning.

Everything else looks good.

> +               ret = -ETIMEDOUT;
> +       }
> +
>         intel_dp->sink_crc.started = false;
>   out:
>         intel_ips_enable(intel_crtc);
> --
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 29/31] drm/i915: Stop tracking last calculated Sink CRC.

[Paulo Zanoni]

2015-11-05 16:50 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> It was created at 'commit aabc95dcf20 (drm/i915: Dont -ETIMEDOUT
> on identical new and previous (count, crc).")' becase the counter
> wasn't reliable.
>
> Now that we properly wait for the counter to be reset we can rely
> a bit more in the counter.
>
> Also that patch stopped to return -ETIMEDOUT so the test case is
> unable to skip when it is unreliable and end up in many fails
> that should be skip instead.
>
> So, with the counter more reliable we can remove
> this hack that just makes things more confusing when test cases
> are really expecting the same CRC and let test case skip if that's
> not the case.
>

Much better this way.

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

> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 39 +++++++++------------------------------
>  drivers/gpu/drm/i915/intel_drv.h |  2 --
>  2 files changed, 9 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5d810cd..24d2bff 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3864,8 +3864,6 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>         if (!(buf & DP_TEST_CRC_SUPPORTED))
>                 return -ENOTTY;
>
> -       intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
> -
>         if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
>                 return -EIO;
>
> @@ -3890,7 +3888,6 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>         u8 buf;
>         int count, ret;
>         int attempts = 6;
> -       bool old_equal_new;
>
>         ret = intel_dp_sink_crc_start(intel_dp);
>         if (ret)
> @@ -3906,35 +3903,17 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>                 }
>                 count = buf & DP_TEST_COUNT_MASK;
>
> -               /*
> -                * Count might be reset during the loop. In this case
> -                * last known count needs to be reset as well.
> -                */
> -               if (count == 0)
> -                       intel_dp->sink_crc.last_count = 0;
> -
> -               if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
> -                       ret = -EIO;
> -                       goto stop;
> -               }
> -
> -               old_equal_new = (count == intel_dp->sink_crc.last_count &&
> -                                !memcmp(intel_dp->sink_crc.last_crc, crc,
> -                                        6 * sizeof(u8)));
> -
> -       } while (--attempts && (count == 0 || old_equal_new));
> -
> -       intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
> -       memcpy(intel_dp->sink_crc.last_crc, crc, 6 * sizeof(u8));
> +       } while (--attempts && count == 0);
>
>         if (attempts == 0) {
> -               if (old_equal_new) {
> -                       DRM_DEBUG_KMS("Unreliable Sink CRC counter: Current returned CRC is identical to the previous one\n");
> -               } else {
> -                       DRM_ERROR("Panel is unable to calculate any CRC after 6 vblanks\n");
> -                       ret = -ETIMEDOUT;
> -                       goto stop;
> -               }
> +               DRM_ERROR("Panel is unable to calculate any CRC after 6 vblanks\n");
> +               ret = -ETIMEDOUT;
> +               goto stop;
> +       }
> +
> +       if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
> +               ret = -EIO;
> +               goto stop;
>         }
>
>  stop:
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index baa560c..de2a8ff 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -746,8 +746,6 @@ enum link_m_n_set {
>
>  struct sink_crc {
>         bool started;
> -       u8 last_crc[6];
> -       int last_count;
>  };
>
>  struct intel_dp {
> --
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 30/31] drm/i915: Rely on TEST_SINK_START instead of tracking Sink CRC state on dev_priv.

[Paulo Zanoni]

2015-11-05 16:50 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Another patch I was going to request in the review of the previous ones.

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

> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 14 ++++++--------
>  drivers/gpu/drm/i915/intel_drv.h |  5 -----
>  2 files changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 24d2bff..5e14f07 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3838,7 +3838,6 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>                 ret = -ETIMEDOUT;
>         }
>
> -       intel_dp->sink_crc.started = false;
>   out:
>         intel_ips_enable(intel_crtc);
>         return ret;
> @@ -3852,12 +3851,6 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>         u8 buf;
>         int ret;
>
> -       if (intel_dp->sink_crc.started) {
> -               ret = intel_dp_sink_crc_stop(intel_dp);
> -               if (ret)
> -                       return ret;
> -       }
> -
>         if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
>                 return -EIO;
>
> @@ -3867,6 +3860,12 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>         if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
>                 return -EIO;
>
> +       if (buf & DP_TEST_SINK_START) {
> +               ret = intel_dp_sink_crc_stop(intel_dp);
> +               if (ret)
> +                       return ret;
> +       }
> +
>         intel_ips_disable(intel_crtc);
>
>         if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> @@ -3876,7 +3875,6 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>         }
>
>         intel_wait_for_vblank(dev, intel_crtc->pipe);
> -       intel_dp->sink_crc.started = true;
>         return 0;
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index de2a8ff..c28f24a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -744,10 +744,6 @@ enum link_m_n_set {
>         M2_N2
>  };
>
> -struct sink_crc {
> -       bool started;
> -};
> -
>  struct intel_dp {
>         uint32_t output_reg;
>         uint32_t aux_ch_ctl_reg;
> @@ -764,7 +760,6 @@ struct intel_dp {
>         /* sink rates as reported by DP_SUPPORTED_LINK_RATES */
>         uint8_t num_sink_rates;
>         int sink_rates[DP_MAX_SUPPORTED_RATES];
> -       struct sink_crc sink_crc;
>         struct drm_dp_aux aux;
>         uint8_t train_set[4];
>         int panel_power_up_delay;
> --
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 28/31] drm/i915: Make Sink crc calculation waiting for counter to reset.

[Paulo Zanoni]

2015-11-10 18:31 GMT-02:00 Paulo Zanoni <przanoni@gmail.com>:
> 2015-11-05 16:50 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
>> According to VESA DP spec TEST_CRC_COUNT (Bits 3:0) at
>> TEST_SINK_MISC (00246h) is "Reset to 0 when TEST_SINK bit 0 = 0;
>>
>> So let's give few vblanks so we are really sure that this counter
>> is really zeroed on the next sink_crc read.
>>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++++++++++++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index c0fa90a..5d810cd 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3806,6 +3806,8 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>>         struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
>>         u8 buf;
>>         int ret = 0;
>> +       int count = 0;
>> +       int attempts = 10;
>>
>>         if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
>>                 DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
>> @@ -3820,7 +3822,22 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>>                 goto out;
>>         }
>>
>> -       intel_wait_for_vblank(dev, intel_crtc->pipe);
>> +       do {
>> +               intel_wait_for_vblank(dev, intel_crtc->pipe);
>> +
>> +               if (drm_dp_dpcd_readb(&intel_dp->aux,
>> +                                     DP_TEST_SINK_MISC, &buf) < 0) {
>> +                       ret = -EIO;
>> +                       goto out;
>
> This "goto out" will make sink_crc.started remain as true even though
> we already sent the DPCD message telling it to stop, and it
> acknowledged our message. And it won't even print stuff on dmesg. I
> guess I'd probably write something on dmesg and flip started to false.

Now I see that patch 30 deals with this issue.

>
>> +               }
>> +               count = buf & DP_TEST_COUNT_MASK;
>> +       } while (--attempts && count);
>> +
>> +       if (attempts == 0) {
>> +               DRM_ERROR("TIMEOUT: Sink CRC counter is not zeroed\n");
>
> The other errors are all DRM_DEBUG_KMS. On one hand we can't do
> anything about them since they're most likely panel errors so
> DRM_ERROR doesn't look good. On the other hand normal users are not
> going to ever run this code, and DRM_ERROR may make us - and our
> testing robots - notice the possible failures, so maybe DRM_ERROR is
> the way to go here. Anyway, we should be consistent regardless of the
> decision.
>
> Besides, at intel_dp_sink_crc_start(), we read the last_count, but
> it's supposed to be zero. Can't we use a check for this there too?
> Maybe just an informative DRM_DEBUG_KMS("this was supposed to be zero
> but it's not\n") without really returning.

This is addressed by patch 29.

>
> Everything else looks good.

So with or without the changes between the log level of the messages
(since end users shouldn't be running them):
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I also vote that we merge 27, 28, 29 and 30 right now since they don't
require patches 1-26. The only conflict is the rename of the IPS
functions, and this can be easily fixed in the patch file.

>
>> +               ret = -ETIMEDOUT;
>> +       }
>> +
>>         intel_dp->sink_crc.started = false;
>>   out:
>>         intel_ips_enable(intel_crtc);
>> --
>> 2.4.3
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni



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

Re: [PATCH 06/31] drm/i915: Fix IPS disable sequence.

[Vivi, Rodrigo]

On Tue, 2015-11-10 at 16:34 +0000, Daniel Stone wrote:
> Hi,
> 
> On 5 November 2015 at 18:49, Rodrigo Vivi <rodrigo.vivi@intel.com> 
> wrote:
> >  /**
> > + * intel_ips_disable_if_alone - Disable IPS if alone in the pipe.
> > + * @crtc: intel crtc
> > + *
> > + * This function should be called when primary plane is being 
> > disabled.
> > + * It checks if there is any other plane enabled on the pipe when 
> > primary is
> > + * going to be disabled. In this case IPS can continue enabled, 
> > but it needs
> > + * to be disabled otherwise.
> > + */
> 
> As an example of what I meant before, I would reword this to reflect
> its actual functionality, which doesn't necessarily have anything to
> do specifically with disabling the primary plane:
> 'This function examines the CRTC state to determine if IPS should
> be disabled. Currently, IPS is disabled if no planes are active on 
> the
> CRTC.'
> 
> Discussing its use in the context of disabling the primary plane I
> think obscures its intent, and also introduces a bug. :)
> 
> > +void intel_ips_disable_if_alone(struct intel_crtc *crtc)
> > +{
> > +       struct drm_device *dev = crtc->base.dev;
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       bool ips_enabled;
> > +       struct intel_plane *intel_plane;
> > +
> > +       mutex_lock(&dev_priv->display_ips.lock);
> > +       ips_enabled = dev_priv->display_ips.enabled;
> > +       mutex_unlock(&dev_priv->display_ips.lock);
> > +
> > +       if (!ips_enabled)
> > +               return;
> > +
> > +       for_each_intel_plane_on_crtc(dev, crtc, intel_plane) {
> > +               enum plane plane = intel_plane->plane;
> > +
> > +               if (plane != PLANE_A &&
> > +                   !!(I915_READ(DSPCNTR(plane)) & 
> > DISPLAY_PLANE_ENABLE))
> > +                       return;
> > +               intel_ips_disable(crtc);
> > +       }
> > +}
> 
> Rather than reading the registers, this should just inspect
> plane_state->visible. Reading the registers introduces the same bug 
> as
> I mentioned the last mail, but in a different way:
>   - IPS is enabled
>   - primary and overlay planes are both enabled
>   - user commits an atomic state which disables both primary and
> overlay planes, so IPS must be disabled
>   - disabling the primary plane calls this function, which sees that
> the overlay plane is still active, so IPS can remain enabled
>   - the overlay plane gets disabled, with IPS still active
>   - :(

You are absolutely right on this case... :/ Thanks for spotting this
case.

So I was considering your idea for the unified place but I ended up in
some concerns questions here.

First is the disable must occur on pre-update and enable on post
-update, so I would prefer to still let them spread and reactive.

But now I believe that we need to detach the atomic
->ips_{enable,disable} from primary and do for every plane on/off. So
if we are enabling any plane we just call ips_enable(). 
And if plane is being disabled and there is no other plane->visible in
this crtc we call intel_disable().

But I wonder how to skip the plane itself on for_each_plane_in_state...
Or should I just counter the number of state->visible and disable if <=
1 and let enable if we count more than 1 visible plane. Any better
idea?


> 
> Making this work on states would eliminate this entire class of bugs,
> and also make it much easier to handle async modesets.
> 
> Cheers,
> Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/31] drm/i915: Fix IPS disable sequence.

[Daniel Stone]

Hi,

On 11 November 2015 at 23:31, Vivi, Rodrigo <rodrigo.vivi@intel.com> wrote:
> On Tue, 2015-11-10 at 16:34 +0000, Daniel Stone wrote:
>> On 5 November 2015 at 18:49, Rodrigo Vivi <rodrigo.vivi@intel.com>
>> wrote:
>> > +void intel_ips_disable_if_alone(struct intel_crtc *crtc)
>> > +{
>> > +       struct drm_device *dev = crtc->base.dev;
>> > +       struct drm_i915_private *dev_priv = dev->dev_private;
>> > +       bool ips_enabled;
>> > +       struct intel_plane *intel_plane;
>> > +
>> > +       mutex_lock(&dev_priv->display_ips.lock);
>> > +       ips_enabled = dev_priv->display_ips.enabled;
>> > +       mutex_unlock(&dev_priv->display_ips.lock);
>> > +
>> > +       if (!ips_enabled)
>> > +               return;
>> > +
>> > +       for_each_intel_plane_on_crtc(dev, crtc, intel_plane) {
>> > +               enum plane plane = intel_plane->plane;
>> > +
>> > +               if (plane != PLANE_A &&
>> > +                   !!(I915_READ(DSPCNTR(plane)) &
>> > DISPLAY_PLANE_ENABLE))
>> > +                       return;
>> > +               intel_ips_disable(crtc);
>> > +       }
>> > +}
>>
>> Rather than reading the registers, this should just inspect
>> plane_state->visible. Reading the registers introduces the same bug
>> as
>> I mentioned the last mail, but in a different way:
>>   - IPS is enabled
>>   - primary and overlay planes are both enabled
>>   - user commits an atomic state which disables both primary and
>> overlay planes, so IPS must be disabled
>>   - disabling the primary plane calls this function, which sees that
>> the overlay plane is still active, so IPS can remain enabled
>>   - the overlay plane gets disabled, with IPS still active
>>   - :(
>
> You are absolutely right on this case... :/ Thanks for spotting this
> case.
>
> So I was considering your idea for the unified place but I ended up in
> some concerns questions here.
>
> First is the disable must occur on pre-update and enable on post
> -update, so I would prefer to still let them spread and reactive.

Right, they have to be separate - which applies doubly for async
modesets as well.

> But now I believe that we need to detach the atomic
> ->ips_{enable,disable} from primary and do for every plane on/off. So
> if we are enabling any plane we just call ips_enable().
> And if plane is being disabled and there is no other plane->visible in
> this crtc we call intel_disable().
>
> But I wonder how to skip the plane itself on for_each_plane_in_state...
> Or should I just counter the number of state->visible and disable if <=
> 1 and let enable if we count more than 1 visible plane. Any better
> idea?

Yeah, exactly that, but even easier: bool ips_target_state =
!!crtc_state->plane_mask;

So my idea would be that, in prepare_commit, you calculate the target
state based on the updated plane_mask. When actually committing the
state, call intel_ips_disable() in intel_pre_plane_update (if
!ips_target_state && intel_crtc->ips_enabled), and intel_ips_enable()
after the commit has finished (if ips_target_state &&
!intel_crtc->ips_enabled). That split would be a really good fit for
async/atomic, where we need to calculate the target state in advance,
and only apply it some time later. Same goes for PSR.

Basically, any work we need to do for modeset needs to be quite
statically calculated in the prepare stage, so we can apply it from
the commit stage, without needing to pull any further state pointers
(we can't do this with async), and certainly without reference to the
actual hardware configuration (e.g. inspecting registers).

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

Re: [PATCH 04/31] drm/i915: Handle actual IPS enabled state.

[Daniel Stone]

Hi Rodrigo,

On 5 November 2015 at 18:49, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> With this we know if IPS is actually enabled.
> It might not be activated on BDW since Hardware take
> the decision and do its transition. However we have
> the visibility of the state on our driver what we didn't
> had until this patch. At least on BDW.
>
> Since ips_ready means that ips will be enabled and ips_disable()
> checks for the state of our enabled/disabled state machine
> we can remove that FIXME that was there for crtc_load_lut
> workaround for Haswell.
>
> With this state machine and ips being disabled from
> different places and many times when testcases with sink_crtc
> for instance it is better to have it protected with its own mutex lock.
> Ohterwise we cannot guarantee consitent ips.enabled state with the
> register bit.

Thinking about this, I have two comments (and second Chris's
similar-ish comment about PSR):

Should this perhaps be a CRTC property rather than a device property?
Is it tied to a particular pipe, or can it move away from pipe A?

Secondly, having a vblank wait to enable IPS is pretty unfortunate, as
it makes modesets take longer. I like the PSR enable being split away
from the modeset sequence, so perhaps we could do something like:

enum ips_state {
  IPS_DISABLED = 0, /**< unsupported or explicitly disabled by module param */
  IPS_READY, /**< IPS can be enabled if a suitable state is applied to
the CRTC (planes enabled, cdclk not exceeding 95% on BDW) */
  IPS_ARMED, /**< suitable configuration applied; IPS pending activation */
  IPS_ACTIVE
};

Having this on the CRTC state means that we could walk through the
following process:
  - IPS_READY set on suitable platforms when not explictly disabled
  - modeset arrives: atomic_check examines conditions and changes
state from IPS_READY to IPS_ARMED
  - next pageflip arrives and changes state from IPS_ARMED to
IPS_ACTIVE, activates IPS

Being a member of the CRTC state means that anyone duplicating the
pipe's CRTC state could discover the IPS status like that, and
eliminates the need for a second mutex.

Maybe that's not the best approach, but I think we need to find a way
to take the synchronous vblank wait out of the modeset path. Using a
workqueue is another option, but synchronisation would need to be
quite carefully handled.

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

Re: [PATCH 04/31] drm/i915: Handle actual IPS enabled state.

[Ville Syrjälä]

On Fri, Nov 13, 2015 at 06:20:00PM +0000, Daniel Stone wrote:
> Hi Rodrigo,
> 
> On 5 November 2015 at 18:49, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > With this we know if IPS is actually enabled.
> > It might not be activated on BDW since Hardware take
> > the decision and do its transition. However we have
> > the visibility of the state on our driver what we didn't
> > had until this patch. At least on BDW.
> >
> > Since ips_ready means that ips will be enabled and ips_disable()
> > checks for the state of our enabled/disabled state machine
> > we can remove that FIXME that was there for crtc_load_lut
> > workaround for Haswell.
> >
> > With this state machine and ips being disabled from
> > different places and many times when testcases with sink_crtc
> > for instance it is better to have it protected with its own mutex lock.
> > Ohterwise we cannot guarantee consitent ips.enabled state with the
> > register bit.
> 
> Thinking about this, I have two comments (and second Chris's
> similar-ish comment about PSR):
> 
> Should this perhaps be a CRTC property rather than a device property?
> Is it tied to a particular pipe, or can it move away from pipe A?
> 
> Secondly, having a vblank wait to enable IPS is pretty unfortunate, as
> it makes modesets take longer. I like the PSR enable being split away
> from the modeset sequence, so perhaps we could do something like:
> 
> enum ips_state {
>   IPS_DISABLED = 0, /**< unsupported or explicitly disabled by module param */
>   IPS_READY, /**< IPS can be enabled if a suitable state is applied to
> the CRTC (planes enabled, cdclk not exceeding 95% on BDW) */
>   IPS_ARMED, /**< suitable configuration applied; IPS pending activation */
>   IPS_ACTIVE
> };
> 
> Having this on the CRTC state means that we could walk through the
> following process:
>   - IPS_READY set on suitable platforms when not explictly disabled
>   - modeset arrives: atomic_check examines conditions and changes
> state from IPS_READY to IPS_ARMED
>   - next pageflip arrives and changes state from IPS_ARMED to
> IPS_ACTIVE, activates IPS
> 
> Being a member of the CRTC state means that anyone duplicating the
> pipe's CRTC state could discover the IPS status like that, and
> eliminates the need for a second mutex.
> 
> Maybe that's not the best approach, but I think we need to find a way
> to take the synchronous vblank wait out of the modeset path. Using a
> workqueue is another option, but synchronisation would need to be
> quite carefully handled.

Long time ago I posted a patch to make ips enable asynchronously from
a vblank work.

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

Re: [PATCH 04/31] drm/i915: Handle actual IPS enabled state.

[Daniel Stone]

Hi,

On 13 November 2015 at 18:38, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Nov 13, 2015 at 06:20:00PM +0000, Daniel Stone wrote:
>> Maybe that's not the best approach, but I think we need to find a way
>> to take the synchronous vblank wait out of the modeset path. Using a
>> workqueue is another option, but synchronisation would need to be
>> quite carefully handled.
>
> Long time ago I posted a patch to make ips enable asynchronously from
> a vblank work.

Too long ago for my mail client, it seems ... got a link handy?

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

Re: [PATCH 04/31] drm/i915: Handle actual IPS enabled state.

[Ville Syrjälä]

On Fri, Nov 13, 2015 at 06:55:18PM +0000, Daniel Stone wrote:
> Hi,
> 
> On 13 November 2015 at 18:38, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Nov 13, 2015 at 06:20:00PM +0000, Daniel Stone wrote:
> >> Maybe that's not the best approach, but I think we need to find a way
> >> to take the synchronous vblank wait out of the modeset path. Using a
> >> workqueue is another option, but synchronisation would need to be
> >> quite carefully handled.
> >
> > Long time ago I posted a patch to make ips enable asynchronously from
> > a vblank work.
> 
> Too long ago for my mail client, it seems ... got a link handy?

http://lists.freedesktop.org/archives/intel-gfx/2014-June/047626.html

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

Re: [PATCH 04/31] drm/i915: Handle actual IPS enabled state.

[Daniel Stone]

On 13 November 2015 at 20:28, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Nov 13, 2015 at 06:55:18PM +0000, Daniel Stone wrote:
>> On 13 November 2015 at 18:38, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > On Fri, Nov 13, 2015 at 06:20:00PM +0000, Daniel Stone wrote:
>> >> Maybe that's not the best approach, but I think we need to find a way
>> >> to take the synchronous vblank wait out of the modeset path. Using a
>> >> workqueue is another option, but synchronisation would need to be
>> >> quite carefully handled.
>> >
>> > Long time ago I posted a patch to make ips enable asynchronously from
>> > a vblank work.
>>
>> Too long ago for my mail client, it seems ... got a link handy?
>
> http://lists.freedesktop.org/archives/intel-gfx/2014-June/047626.html

+struct intel_vblank_notify {

Oh dear ...

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

Re: [PATCH 07/31] drm/i915: IPS Sysfs interface.

[Daniel Vetter]

On Thu, Nov 05, 2015 at 09:04:02PM +0000, Chris Wilson wrote:
> On Thu, Nov 05, 2015 at 10:49:59AM -0800, Rodrigo Vivi wrote:
> > With the lock in place we can expose ips enabled/disable on sysfs
> > for developing, debugging and information purposes.
> 
> No. sysfs is not for developement, debugging or information. It is a
> userspace ABI.

Yup, this should be for debugfs.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/31] drm/i915: Detatch i915.enable_psr from psr_ready

[Daniel Vetter]

On Thu, Nov 05, 2015 at 10:50:02AM -0800, Rodrigo Vivi wrote:
> PSR will be enabled on every post primary update when it is
> ready and parameter allows.
> With this we allow test cases to continue using this parameter
> for enabling disabling the feature.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 2 +-
>  drivers/gpu/drm/i915/intel_psr.c | 5 -----
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6ab127c..e154a2e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5334,7 +5334,7 @@ void intel_edp_drrs_enable(struct intel_dp *intel_dp)
>  		return;
>  	}
>  
> -	if (intel_crtc->config->psr_ready) {
> +	if (intel_crtc->config->psr_ready && i915.enable_psr) {

I don't get this change ... The pipe config should take into account
enable_psr already. If we allow testcases to change this without a
modeset, this could result in a lot of racy fun.
-Daniel

>  		DRM_DEBUG_KMS("DRRS: PSR will be enabled on this crtc\n");
>  		return;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 4a9d620..e690db3 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -314,11 +314,6 @@ bool intel_psr_ready(struct intel_dp *intel_dp,
>  		return false;
>  	}
>  
> -	if (!i915.enable_psr) {
> -		DRM_DEBUG_KMS("PSR disable by flag\n");
> -		return false;
> -	}
> -
>  	if (IS_HASWELL(dev) &&
>  	    I915_READ(HSW_STEREO_3D_CTL(pipe_config->cpu_transcoder)) &
>  		      S3D_ENABLE) {
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/31] drm/i915: Fix PSR initialization.

[Daniel Vetter]

On Thu, Nov 05, 2015 at 10:50:04AM -0800, Rodrigo Vivi wrote:
> PSR is still disabled by default, but even passing i915.enable_psr=1
> at this point we weren't able to get PSR working because with
> fastboot by default in place we weren't executing the path that enables
> encoder and consequently PSR.
> 
> Now with psr_ready in place and PSR using crtc signature we can move
> its enable/disable sequences from the encoder enable to the post
> atomic modeset functions.
> 
> i915.enable_psr parameter is still used to enable/disable psr feature
> on the next primary plane update. So current test cases that relies
> on this flow still works.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |  2 --
>  drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c      |  5 -----
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  4 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index b8f8dee..36db970 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2404,7 +2404,6 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
>  			intel_dp_stop_link_train(intel_dp);
>  
>  		intel_edp_backlight_on(intel_dp);
> -		intel_psr_enable(intel_crtc);
>  		intel_edp_drrs_enable(intel_dp);
>  	}
>  
> @@ -2432,7 +2431,6 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  
>  		intel_edp_drrs_disable(intel_dp);
> -		intel_psr_disable(intel_crtc);
>  		intel_edp_backlight_off(intel_dp);
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 869929d..f67e2ee 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4687,6 +4687,9 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
>  	if (atomic->enable_ips)
>  		intel_ips_enable(crtc);
>  
> +	if (atomic->enable_psr)
> +		intel_psr_enable(crtc);

What we need here is a post-modeset fixup hook for encoders/connectors.
That would avoid the layering violation of going through a crtc and then
doing the crtc->encoder lookup you do in the previous patch. And we have
other uses for this, e.g. mipi self refresh, fixing up infoframes and all
those things.
-Daniel

> +
>  	if (atomic->post_enable_primary)
>  		intel_post_enable_primary(&crtc->base);
>  
> @@ -4705,6 +4708,9 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
>  	if (crtc->atomic.disable_ips)
>  		intel_ips_disable_if_alone(crtc);
>  
> +	if (crtc->atomic.disable_psr)
> +		intel_psr_disable(crtc);
> +
>  	if (atomic->pre_disable_primary)
>  		intel_pre_disable_primary(&crtc->base);
>  
> @@ -11560,9 +11566,18 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  			intel_crtc->atomic.disable_ips = true;
>  
>  			intel_crtc->atomic.disable_fbc = true;
> +
> +			intel_crtc->atomic.disable_psr = true;
>  		}
>  		if (visible && intel_crtc->config->ips_ready)
>  			intel_crtc->atomic.enable_ips = true;
> +
> +		if (visible && intel_crtc->config->psr_ready) {
> +			if (i915.enable_psr)
> +				intel_crtc->atomic.enable_psr = true;
> +			else
> +				intel_crtc->atomic.disable_psr = true;
> +		}
>  		/*
>  		 * FBC does not work on some platforms for rotated
>  		 * planes, so disable it when rotation is not 0 and
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 92f59cc..f0ee497 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2373,9 +2373,6 @@ static void intel_disable_dp(struct intel_encoder *encoder)
>  	if (crtc->config->has_audio)
>  		intel_audio_codec_disable(encoder);
>  
> -	if (HAS_PSR(dev) && !HAS_DDI(dev))
> -		intel_psr_disable(crtc);
> -
>  	/* Make sure the panel is off before trying to change the mode. But also
>  	 * ensure that we have vdd while we switch off the panel. */
>  	intel_edp_panel_vdd_on(intel_dp);
> @@ -2629,10 +2626,8 @@ static void g4x_enable_dp(struct intel_encoder *encoder)
>  static void vlv_enable_dp(struct intel_encoder *encoder)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> -	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
>  
>  	intel_edp_backlight_on(intel_dp);
> -	intel_psr_enable(crtc);
>  }
>  
>  static void g4x_pre_enable_dp(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cafe4c1..d599d54 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -531,6 +531,7 @@ struct intel_crtc_atomic_commit {
>  	/* Sleepable operations to perform before commit */
>  	bool disable_fbc;
>  	bool disable_ips;
> +	bool disable_psr;
>  	bool disable_cxsr;
>  	bool pre_disable_primary;
>  	bool update_wm_pre, update_wm_post;
> @@ -540,6 +541,7 @@ struct intel_crtc_atomic_commit {
>  	bool wait_vblank;
>  	bool update_fbc;
>  	bool enable_ips;
> +	bool enable_psr;
>  	bool post_enable_primary;
>  	unsigned update_sprite_watermarks;
>  };
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 16/31] drm/i915: Fix DRRS initialization.

[Daniel Vetter]

On Thu, Nov 05, 2015 at 10:50:08AM -0800, Rodrigo Vivi wrote:
> With Fastboot by default we don't necessarily do a
> full modeset enabling the primary plane.
> So DRRS enable call that was in that path wasn't being
> called anymore.
> 
> So, let's relly on post atomic modeset path
> and on has_drrs to enabled DRRS when we judge necessary.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f67e2ee..6647bfe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4681,6 +4681,9 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
>  	if (crtc->atomic.update_wm_post)
>  		intel_update_watermarks(&crtc->base);
>  
> +	if (atomic->enable_drrs)
> +		intel_drrs_enable(crtc);

Same comment as with psr. And encoder post_modeset fixup function would be
a lot cleaner instead of leaking this all into crtc code.
-Daniel

> +
>  	if (atomic->update_fbc)
>  		intel_fbc_update(dev_priv);
>  
> @@ -4702,6 +4705,9 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>  
> +	if (crtc->atomic.disable_drrs)
> +		intel_drrs_disable(crtc);
> +
>  	if (atomic->disable_fbc)
>  		intel_fbc_disable_crtc(crtc);
>  
> @@ -11565,10 +11571,15 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  			 */
>  			intel_crtc->atomic.disable_ips = true;
>  
> +			intel_crtc->atomic.disable_drrs = true;
> +
>  			intel_crtc->atomic.disable_fbc = true;
>  
>  			intel_crtc->atomic.disable_psr = true;
>  		}
> +		if (visible && intel_crtc->config->has_drrs)
> +			intel_crtc->atomic.enable_drrs = true;
> +
>  		if (visible && intel_crtc->config->ips_ready)
>  			intel_crtc->atomic.enable_ips = true;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 48f461f..bf5e77c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -529,6 +529,7 @@ struct intel_mmio_flip {
>   */
>  struct intel_crtc_atomic_commit {
>  	/* Sleepable operations to perform before commit */
> +	bool disable_drrs;
>  	bool disable_fbc;
>  	bool disable_ips;
>  	bool disable_psr;
> @@ -539,6 +540,7 @@ struct intel_crtc_atomic_commit {
>  	/* Sleepable operations to perform after commit */
>  	unsigned fb_bits;
>  	bool wait_vblank;
> +	bool enable_drrs;
>  	bool update_fbc;
>  	bool enable_ips;
>  	bool enable_psr;
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 28/31] drm/i915: Make Sink crc calculation waiting for counter to reset.

[Daniel Vetter]

On Tue, Nov 10, 2015 at 07:49:51PM -0200, Paulo Zanoni wrote:
> 2015-11-10 18:31 GMT-02:00 Paulo Zanoni <przanoni@gmail.com>:
> > 2015-11-05 16:50 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> >> According to VESA DP spec TEST_CRC_COUNT (Bits 3:0) at
> >> TEST_SINK_MISC (00246h) is "Reset to 0 when TEST_SINK bit 0 = 0;
> >>
> >> So let's give few vblanks so we are really sure that this counter
> >> is really zeroed on the next sink_crc read.
> >>
> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++++++++++++-
> >>  1 file changed, 18 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index c0fa90a..5d810cd 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -3806,6 +3806,8 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> >>         struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> >>         u8 buf;
> >>         int ret = 0;
> >> +       int count = 0;
> >> +       int attempts = 10;
> >>
> >>         if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> >>                 DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> >> @@ -3820,7 +3822,22 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> >>                 goto out;
> >>         }
> >>
> >> -       intel_wait_for_vblank(dev, intel_crtc->pipe);
> >> +       do {
> >> +               intel_wait_for_vblank(dev, intel_crtc->pipe);
> >> +
> >> +               if (drm_dp_dpcd_readb(&intel_dp->aux,
> >> +                                     DP_TEST_SINK_MISC, &buf) < 0) {
> >> +                       ret = -EIO;
> >> +                       goto out;
> >
> > This "goto out" will make sink_crc.started remain as true even though
> > we already sent the DPCD message telling it to stop, and it
> > acknowledged our message. And it won't even print stuff on dmesg. I
> > guess I'd probably write something on dmesg and flip started to false.
> 
> Now I see that patch 30 deals with this issue.
> 
> >
> >> +               }
> >> +               count = buf & DP_TEST_COUNT_MASK;
> >> +       } while (--attempts && count);
> >> +
> >> +       if (attempts == 0) {
> >> +               DRM_ERROR("TIMEOUT: Sink CRC counter is not zeroed\n");
> >
> > The other errors are all DRM_DEBUG_KMS. On one hand we can't do
> > anything about them since they're most likely panel errors so
> > DRM_ERROR doesn't look good. On the other hand normal users are not
> > going to ever run this code, and DRM_ERROR may make us - and our
> > testing robots - notice the possible failures, so maybe DRM_ERROR is
> > the way to go here. Anyway, we should be consistent regardless of the
> > decision.
> >
> > Besides, at intel_dp_sink_crc_start(), we read the last_count, but
> > it's supposed to be zero. Can't we use a check for this there too?
> > Maybe just an informative DRM_DEBUG_KMS("this was supposed to be zero
> > but it's not\n") without really returning.
> 
> This is addressed by patch 29.
> 
> >
> > Everything else looks good.
> 
> So with or without the changes between the log level of the messages
> (since end users shouldn't be running them):
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I also vote that we merge 27, 28, 29 and 30 right now since they don't
> require patches 1-26. The only conflict is the rename of the IPS
> functions, and this can be easily fixed in the patch file.

Good idea, all 4 pulled into dinq. Rodrigo, is this all we need to make
sink CRC reliable? Or is the read_wake stuff still needed?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/31] drm/i915: IPS Sysfs interface.

[Vivi, Rodrigo]

On Wed, 2015-11-18 at 11:04 +0100, Daniel Vetter wrote:
> On Thu, Nov 05, 2015 at 09:04:02PM +0000, Chris Wilson wrote:
> > On Thu, Nov 05, 2015 at 10:49:59AM -0800, Rodrigo Vivi wrote:
> > > With the lock in place we can expose ips enabled/disable on sysfs
> > > for developing, debugging and information purposes.
> > 
> > No. sysfs is not for developement, debugging or information. It is 
> > a
> > userspace ABI.
> 
> Yup, this should be for debugfs.
> -Daniel

Yes, please ignore for now both patches creating sysfs in this series.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/31] drm/i915: Detatch i915.enable_psr from psr_ready

[Vivi, Rodrigo]

On Wed, 2015-11-18 at 11:07 +0100, Daniel Vetter wrote:
> On Thu, Nov 05, 2015 at 10:50:02AM -0800, Rodrigo Vivi wrote:
> > PSR will be enabled on every post primary update when it is
> > ready and parameter allows.
> > With this we allow test cases to continue using this parameter
> > for enabling disabling the feature.
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  | 2 +-
> >  drivers/gpu/drm/i915/intel_psr.c | 5 -----
> >  2 files changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 6ab127c..e154a2e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5334,7 +5334,7 @@ void intel_edp_drrs_enable(struct intel_dp 
> > *intel_dp)
> >  		return;
> >  	}
> >  
> > -	if (intel_crtc->config->psr_ready) {
> > +	if (intel_crtc->config->psr_ready && i915.enable_psr) {
> 
> I don't get this change ... The pipe config should take into account
> enable_psr already. If we allow testcases to change this without a
> modeset, this could result in a lot of racy fun.

Yes, I agree, however igt test cases uses i915.enable_psr to
enable/disable psr so I'm afraid we will loose this if we add it to
psr_ready.

> -Daniel
> 
> >  		DRM_DEBUG_KMS("DRRS: PSR will be enabled on this 
> > crtc\n");
> >  		return;
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 4a9d620..e690db3 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -314,11 +314,6 @@ bool intel_psr_ready(struct intel_dp 
> > *intel_dp,
> >  		return false;
> >  	}
> >  
> > -	if (!i915.enable_psr) {
> > -		DRM_DEBUG_KMS("PSR disable by flag\n");
> > -		return false;
> > -	}
> > -
> >  	if (IS_HASWELL(dev) &&
> >  	    I915_READ(HSW_STEREO_3D_CTL(pipe_config
> > ->cpu_transcoder)) &
> >  		      S3D_ENABLE) {
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/31] drm/i915: Fix PSR initialization.

[Vivi, Rodrigo]

On Wed, 2015-11-18 at 11:12 +0100, Daniel Vetter wrote:
> On Thu, Nov 05, 2015 at 10:50:04AM -0800, Rodrigo Vivi wrote:
> > PSR is still disabled by default, but even passing 
> > i915.enable_psr=1
> > at this point we weren't able to get PSR working because with
> > fastboot by default in place we weren't executing the path that 
> > enables
> > encoder and consequently PSR.
> > 
> > Now with psr_ready in place and PSR using crtc signature we can 
> > move
> > its enable/disable sequences from the encoder enable to the post
> > atomic modeset functions.
> > 
> > i915.enable_psr parameter is still used to enable/disable psr 
> > feature
> > on the next primary plane update. So current test cases that relies
> > on this flow still works.
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c     |  2 --
> >  drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++
> >  drivers/gpu/drm/i915/intel_dp.c      |  5 -----
> >  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> >  4 files changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index b8f8dee..36db970 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2404,7 +2404,6 @@ static void intel_enable_ddi(struct 
> > intel_encoder *intel_encoder)
> >  			intel_dp_stop_link_train(intel_dp);
> >  
> >  		intel_edp_backlight_on(intel_dp);
> > -		intel_psr_enable(intel_crtc);
> >  		intel_edp_drrs_enable(intel_dp);
> >  	}
> >  
> > @@ -2432,7 +2431,6 @@ static void intel_disable_ddi(struct 
> > intel_encoder *intel_encoder)
> >  		struct intel_dp *intel_dp = 
> > enc_to_intel_dp(encoder);
> >  
> >  		intel_edp_drrs_disable(intel_dp);
> > -		intel_psr_disable(intel_crtc);
> >  		intel_edp_backlight_off(intel_dp);
> >  	}
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 869929d..f67e2ee 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4687,6 +4687,9 @@ static void intel_post_plane_update(struct 
> > intel_crtc *crtc)
> >  	if (atomic->enable_ips)
> >  		intel_ips_enable(crtc);
> >  
> > +	if (atomic->enable_psr)
> > +		intel_psr_enable(crtc);
> 
> What we need here is a post-modeset fixup hook for 
> encoders/connectors.
> That would avoid the layering violation of going through a crtc and 
> then
> doing the crtc->encoder lookup you do in the previous patch. And we 
> have
> other uses for this, e.g. mipi self refresh, fixing up infoframes and 
> all
> those things.

I see your point but I'm not sure if I totally got your idea.

So would it be a generic hook when we detect encoder/connector being
enabled/disabled?

and how to determine that? could you please elaborate your idea a bit
more?

> -Daniel

Thank you very much,
Rodrigo.

> 
> > +
> >  	if (atomic->post_enable_primary)
> >  		intel_post_enable_primary(&crtc->base);
> >  
> > @@ -4705,6 +4708,9 @@ static void intel_pre_plane_update(struct 
> > intel_crtc *crtc)
> >  	if (crtc->atomic.disable_ips)
> >  		intel_ips_disable_if_alone(crtc);
> >  
> > +	if (crtc->atomic.disable_psr)
> > +		intel_psr_disable(crtc);
> > +
> >  	if (atomic->pre_disable_primary)
> >  		intel_pre_disable_primary(&crtc->base);
> >  
> > @@ -11560,9 +11566,18 @@ int intel_plane_atomic_calc_changes(struct 
> > drm_crtc_state *crtc_state,
> >  			intel_crtc->atomic.disable_ips = true;
> >  
> >  			intel_crtc->atomic.disable_fbc = true;
> > +
> > +			intel_crtc->atomic.disable_psr = true;
> >  		}
> >  		if (visible && intel_crtc->config->ips_ready)
> >  			intel_crtc->atomic.enable_ips = true;
> > +
> > +		if (visible && intel_crtc->config->psr_ready) {
> > +			if (i915.enable_psr)
> > +				intel_crtc->atomic.enable_psr = 
> > true;
> > +			else
> > +				intel_crtc->atomic.disable_psr = 
> > true;
> > +		}
> >  		/*
> >  		 * FBC does not work on some platforms for rotated
> >  		 * planes, so disable it when rotation is not 0 
> > and
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 92f59cc..f0ee497 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2373,9 +2373,6 @@ static void intel_disable_dp(struct 
> > intel_encoder *encoder)
> >  	if (crtc->config->has_audio)
> >  		intel_audio_codec_disable(encoder);
> >  
> > -	if (HAS_PSR(dev) && !HAS_DDI(dev))
> > -		intel_psr_disable(crtc);
> > -
> >  	/* Make sure the panel is off before trying to change the 
> > mode. But also
> >  	 * ensure that we have vdd while we switch off the panel. 
> > */
> >  	intel_edp_panel_vdd_on(intel_dp);
> > @@ -2629,10 +2626,8 @@ static void g4x_enable_dp(struct 
> > intel_encoder *encoder)
> >  static void vlv_enable_dp(struct intel_encoder *encoder)
> >  {
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder
> > ->base);
> > -	struct intel_crtc *crtc = to_intel_crtc(encoder
> > ->base.crtc);
> >  
> >  	intel_edp_backlight_on(intel_dp);
> > -	intel_psr_enable(crtc);
> >  }
> >  
> >  static void g4x_pre_enable_dp(struct intel_encoder *encoder)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index cafe4c1..d599d54 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -531,6 +531,7 @@ struct intel_crtc_atomic_commit {
> >  	/* Sleepable operations to perform before commit */
> >  	bool disable_fbc;
> >  	bool disable_ips;
> > +	bool disable_psr;
> >  	bool disable_cxsr;
> >  	bool pre_disable_primary;
> >  	bool update_wm_pre, update_wm_post;
> > @@ -540,6 +541,7 @@ struct intel_crtc_atomic_commit {
> >  	bool wait_vblank;
> >  	bool update_fbc;
> >  	bool enable_ips;
> > +	bool enable_psr;
> >  	bool post_enable_primary;
> >  	unsigned update_sprite_watermarks;
> >  };
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 28/31] drm/i915: Make Sink crc calculation waiting for counter to reset.

[Vivi, Rodrigo]

On Wed, 2015-11-18 at 11:25 +0100, Daniel Vetter wrote:
> On Tue, Nov 10, 2015 at 07:49:51PM -0200, Paulo Zanoni wrote:
> > 2015-11-10 18:31 GMT-02:00 Paulo Zanoni <przanoni@gmail.com>:
> > > 2015-11-05 16:50 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > > > According to VESA DP spec TEST_CRC_COUNT (Bits 3:0) at
> > > > TEST_SINK_MISC (00246h) is "Reset to 0 when TEST_SINK bit 0 = 
> > > > 0;
> > > > 
> > > > So let's give few vblanks so we are really sure that this 
> > > > counter
> > > > is really zeroed on the next sink_crc read.
> > > > 
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++++++++++++-
> > > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index c0fa90a..5d810cd 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -3806,6 +3806,8 @@ static int intel_dp_sink_crc_stop(struct 
> > > > intel_dp *intel_dp)
> > > >         struct intel_crtc *intel_crtc = to_intel_crtc(dig_port
> > > > ->base.base.crtc);
> > > >         u8 buf;
> > > >         int ret = 0;
> > > > +       int count = 0;
> > > > +       int attempts = 10;
> > > > 
> > > >         if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, 
> > > > &buf) < 0) {
> > > >                 DRM_DEBUG_KMS("Sink CRC couldn't be stopped 
> > > > properly\n");
> > > > @@ -3820,7 +3822,22 @@ static int intel_dp_sink_crc_stop(struct 
> > > > intel_dp *intel_dp)
> > > >                 goto out;
> > > >         }
> > > > 
> > > > -       intel_wait_for_vblank(dev, intel_crtc->pipe);
> > > > +       do {
> > > > +               intel_wait_for_vblank(dev, intel_crtc->pipe);
> > > > +
> > > > +               if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > > +                                     DP_TEST_SINK_MISC, &buf) 
> > > > < 0) {
> > > > +                       ret = -EIO;
> > > > +                       goto out;
> > > 
> > > This "goto out" will make sink_crc.started remain as true even 
> > > though
> > > we already sent the DPCD message telling it to stop, and it
> > > acknowledged our message. And it won't even print stuff on dmesg. 
> > > I
> > > guess I'd probably write something on dmesg and flip started to 
> > > false.
> > 
> > Now I see that patch 30 deals with this issue.
> > 
> > > 
> > > > +               }
> > > > +               count = buf & DP_TEST_COUNT_MASK;
> > > > +       } while (--attempts && count);
> > > > +
> > > > +       if (attempts == 0) {
> > > > +               DRM_ERROR("TIMEOUT: Sink CRC counter is not 
> > > > zeroed\n");
> > > 
> > > The other errors are all DRM_DEBUG_KMS. On one hand we can't do
> > > anything about them since they're most likely panel errors so
> > > DRM_ERROR doesn't look good. On the other hand normal users are 
> > > not
> > > going to ever run this code, and DRM_ERROR may make us - and our
> > > testing robots - notice the possible failures, so maybe DRM_ERROR 
> > > is
> > > the way to go here. Anyway, we should be consistent regardless of 
> > > the
> > > decision.
> > > 
> > > Besides, at intel_dp_sink_crc_start(), we read the last_count, 
> > > but
> > > it's supposed to be zero. Can't we use a check for this there 
> > > too?
> > > Maybe just an informative DRM_DEBUG_KMS("this was supposed to be 
> > > zero
> > > but it's not\n") without really returning.
> > 
> > This is addressed by patch 29.
> > 
> > > 
> > > Everything else looks good.
> > 
> > So with or without the changes between the log level of the 
> > messages
> > (since end users shouldn't be running them):
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > I also vote that we merge 27, 28, 29 and 30 right now since they 
> > don't
> > require patches 1-26. The only conflict is the rename of the IPS
> > functions, and this can be easily fixed in the patch file.
> 
> Good idea, all 4 pulled into dinq. Rodrigo, is this all we need to 
> make
> sink CRC reliable? Or is the read_wake stuff still needed?

On SKL and KBL the read_wake is still needed. or we treat that case
where we read the message size 0 what is forbidden and retry or return
EBUSY to let drm level retrie when we have no idea what is happening
with hardware...



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

Re: [PATCH 10/31] drm/i915: Detatch i915.enable_psr from psr_ready

[Daniel Vetter]

On Wed, Nov 18, 2015 at 06:35:15PM +0000, Vivi, Rodrigo wrote:
> On Wed, 2015-11-18 at 11:07 +0100, Daniel Vetter wrote:
> > On Thu, Nov 05, 2015 at 10:50:02AM -0800, Rodrigo Vivi wrote:
> > > PSR will be enabled on every post primary update when it is
> > > ready and parameter allows.
> > > With this we allow test cases to continue using this parameter
> > > for enabling disabling the feature.
> > > 
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c  | 2 +-
> > >  drivers/gpu/drm/i915/intel_psr.c | 5 -----
> > >  2 files changed, 1 insertion(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 6ab127c..e154a2e 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -5334,7 +5334,7 @@ void intel_edp_drrs_enable(struct intel_dp 
> > > *intel_dp)
> > >  		return;
> > >  	}
> > >  
> > > -	if (intel_crtc->config->psr_ready) {
> > > +	if (intel_crtc->config->psr_ready && i915.enable_psr) {
> > 
> > I don't get this change ... The pipe config should take into account
> > enable_psr already. If we allow testcases to change this without a
> > modeset, this could result in a lot of racy fun.
> 
> Yes, I agree, however igt test cases uses i915.enable_psr to
> enable/disable psr so I'm afraid we will loose this if we add it to
> psr_ready.

Can't the testcase force a full modeset to update psr state? That should
be enough to force-enable psr for testing and then disable it again. And
for a full modeset we need to reevaluate psr anyway, so this won't require
any special code.

full modeset = setCrtc(NULL); setCrtc(mode);
-Daniel

> 
> > -Daniel
> > 
> > >  		DRM_DEBUG_KMS("DRRS: PSR will be enabled on this 
> > > crtc\n");
> > >  		return;
> > >  	}
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 4a9d620..e690db3 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -314,11 +314,6 @@ bool intel_psr_ready(struct intel_dp 
> > > *intel_dp,
> > >  		return false;
> > >  	}
> > >  
> > > -	if (!i915.enable_psr) {
> > > -		DRM_DEBUG_KMS("PSR disable by flag\n");
> > > -		return false;
> > > -	}
> > > -
> > >  	if (IS_HASWELL(dev) &&
> > >  	    I915_READ(HSW_STEREO_3D_CTL(pipe_config
> > > ->cpu_transcoder)) &
> > >  		      S3D_ENABLE) {
> > > -- 
> > > 2.4.3
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/31] drm/i915: Fix PSR initialization.

[Daniel Vetter]

On Wed, Nov 18, 2015 at 06:39:45PM +0000, Vivi, Rodrigo wrote:
> On Wed, 2015-11-18 at 11:12 +0100, Daniel Vetter wrote:
> > On Thu, Nov 05, 2015 at 10:50:04AM -0800, Rodrigo Vivi wrote:
> > > PSR is still disabled by default, but even passing 
> > > i915.enable_psr=1
> > > at this point we weren't able to get PSR working because with
> > > fastboot by default in place we weren't executing the path that 
> > > enables
> > > encoder and consequently PSR.
> > > 
> > > Now with psr_ready in place and PSR using crtc signature we can 
> > > move
> > > its enable/disable sequences from the encoder enable to the post
> > > atomic modeset functions.
> > > 
> > > i915.enable_psr parameter is still used to enable/disable psr 
> > > feature
> > > on the next primary plane update. So current test cases that relies
> > > on this flow still works.
> > > 
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c     |  2 --
> > >  drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++
> > >  drivers/gpu/drm/i915/intel_dp.c      |  5 -----
> > >  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> > >  4 files changed, 17 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index b8f8dee..36db970 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -2404,7 +2404,6 @@ static void intel_enable_ddi(struct 
> > > intel_encoder *intel_encoder)
> > >  			intel_dp_stop_link_train(intel_dp);
> > >  
> > >  		intel_edp_backlight_on(intel_dp);
> > > -		intel_psr_enable(intel_crtc);
> > >  		intel_edp_drrs_enable(intel_dp);
> > >  	}
> > >  
> > > @@ -2432,7 +2431,6 @@ static void intel_disable_ddi(struct 
> > > intel_encoder *intel_encoder)
> > >  		struct intel_dp *intel_dp = 
> > > enc_to_intel_dp(encoder);
> > >  
> > >  		intel_edp_drrs_disable(intel_dp);
> > > -		intel_psr_disable(intel_crtc);
> > >  		intel_edp_backlight_off(intel_dp);
> > >  	}
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 869929d..f67e2ee 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -4687,6 +4687,9 @@ static void intel_post_plane_update(struct 
> > > intel_crtc *crtc)
> > >  	if (atomic->enable_ips)
> > >  		intel_ips_enable(crtc);
> > >  
> > > +	if (atomic->enable_psr)
> > > +		intel_psr_enable(crtc);
> > 
> > What we need here is a post-modeset fixup hook for 
> > encoders/connectors.
> > That would avoid the layering violation of going through a crtc and 
> > then
> > doing the crtc->encoder lookup you do in the previous patch. And we 
> > have
> > other uses for this, e.g. mipi self refresh, fixing up infoframes and 
> > all
> > those things.
> 
> I see your point but I'm not sure if I totally got your idea.
> 
> So would it be a generic hook when we detect encoder/connector being
> enabled/disabled?
> 
> and how to determine that? could you please elaborate your idea a bit
> more?


diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c3bc6ab77508..427daac480f5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13273,6 +13273,11 @@ static int intel_atomic_commit(struct drm_device *dev,
 			modeset_put_power_domains(dev_priv, put_domains);
 
 		intel_post_plane_update(intel_crtc);
+
+		if (needs_modeset || update_pipe) {
+			for_each_encoder_on_crtc(dev, crtc, encoder)
+				encoder->enable_fixup(encoder);
+		}
 	}
 
 	/* FIXME: add subpixel order */


With this enable_fixup (maybe we should call it post_plane_enable) will
will get called both when we do a full modeset, but also when we just do a
fast modeset/pipe update. Then we can move everything that we're currently
doing in ->enable which also must be done for fastboot into
->post_plane_enable, like infoframes, DRRS, psr or whatever.

The only tricky part is that the core won't do the book-keeping for us
anymore, i.e. we need to check ourselves in this new hook whether we need
to do the setup or not, since if userspace only changes the scaling a lot
we might get a lot of ->post_plane_enable calls without any ->disable
calls in between. So that's also where you have to put your checks:

intel_dp_post_plane_enable()
{
	if (!psr_enabled)
		enable_psr();

	if (!DRRS_on)
		enable_drrs();

	/* maybe in the future */
	if (!audio_enabled)
		enable_audio();
}


intel_hdmi_post_plane_enable()
{
	/* ifxup infoframes if they're crap from the bios */
}

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx