[PATCH resend 00/15] drm/i915/dsi: Fix / cleanup dsi enable / disable sequences

[PATCH resend 00/15] drm/i915/dsi: Fix / cleanup dsi enable / disable sequences

[Hans de Goede]

Hi All,

Here is a resend of my "cleanup dsi enable / disable sequences" patch
series as requested by Jani.

Here is the original cover letter, there have been no changes other then
dropping 3 patches which have already been merged:

So while trying to fix my cherrytrail tablet's screen sometimes not
initializing properly (*) I started working on this series to cleanup /
(minor) refactor the dsi enable / disable code, with as goal to then
change it to match the enable / disable sequences which Ville Syrjälä
recently dug up from within the spec.

The 1st patch is a (small) functional change, which makes the
patches following it move less code around.

Patches 2 - 4 move some stuff around with as goal to have
intel_dsi_pre_enable() and intel_dsi_pre_disable() +
intel_dsi_post_disable() have the entire (de)init sequence in a
clearly readable one function call per step (more or less)
style, so that the ordering is obvious and we can easily match
the code to the spec

Patches 7 - 15 actually modify the code to match the spec, there are
9 patches here as I've chosen to do one tiny change per patch so that
regressions can be bisected to a commit more specific then a
"change the world" commit.

After this patch-set the dsi enable / disable code is much easier to
read (IMHO), almost fully matches the spec (with the single exception
documented) and still works (for me).

There are still some possible improvements to consider:

1) intel_dsi_pre_enable starts with enabling the pll, but
   intel_dsi_post_disable disables it half-way through the
   sequence, since then it is no longer necessary. From a
   power-management pov this is good, but it is assymetrical

2) intel_dsi_pre_enable calls intel_dsi_prepare before calling
   intel_dsi_device_ready, so intel_dsi_post_disable should
   call intel_dsi_unprepare *after* intel_dsi_clear_device_ready,
   but it calls it *before*. Because of this intel_dsi_unprepare
   itself temporarily clears device-ready and resets it in the end.
   It would be good to move intel_dsi_unprepare to *after*
   intel_dsi_clear_device_ready and drop its toggling of the
   device-ready bit, but I'm not sure if that is safe. Perhaps
   there is a specific reason why this is done this way?

3) While working on this I found the following patch which maybe
   should be merged to the mainline i915 code ?  :
   https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/0002-FOR_UPSTREAM-VPG-drm-i915-Move-port-enable-to-after-.patch

   The date on this patch is from long after the decision to
   move the intel_dsi_enable_port call to intel_dsi_pre_enable,
   so it seems to superseed that decision. Maybe we should move the
   intel_dsi_port_enable call from intel_dsi_pre_enable back
   to intel_dsi_enable[_nop] ?

Regards,

Hans


*) The root cause of which turns out to be something different, but since
I had done most of this work when I found that out, I decided to not throw
away my work and instead finish this series and post it upstream
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH resend 01/15] drm/i915/dsi: Move calling of wait_for_dsi_fifo_empty to mipi_exec_send_packet

[Hans de Goede]

Instead of calling wait_for_dsi_fifo_empty on all dsi ports after calling
a drm_panel_foo helper which calls VBT sequences, move it to the VBT
mipi_exec_send_packet helper, which is the one VBT instruction which
actually puts data in the fifo.

This results in a nice cleanup making it clearer what all the steps on
intel_dsi_enable / disable are and this also makes the VBT code properly
wait till a command has actually been send before executing the next
steps (typically a delay) in the VBT sequence.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c           | 12 +-----------
 drivers/gpu/drm/i915/intel_dsi.h           |  2 ++
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |  2 ++
 3 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index c26fe4f..2f0af98 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -80,7 +80,7 @@ enum mipi_dsi_pixel_format pixel_format_from_register_bits(u32 fmt)
 	}
 }
 
-static void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port)
+void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port)
 {
 	struct drm_encoder *encoder = &intel_dsi->base.base;
 	struct drm_device *dev = encoder->dev;
@@ -525,9 +525,6 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
 
 		drm_panel_enable(intel_dsi->panel);
 
-		for_each_dsi_port(port, intel_dsi->ports)
-			wait_for_dsi_fifo_empty(intel_dsi, port);
-
 		intel_dsi_port_enable(encoder);
 	}
 
@@ -543,7 +540,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
-	enum port port;
 	u32 val;
 
 	DRM_DEBUG_KMS("\n");
@@ -588,9 +584,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 
 	drm_panel_prepare(intel_dsi->panel);
 
-	for_each_dsi_port(port, intel_dsi->ports)
-		wait_for_dsi_fifo_empty(intel_dsi, port);
-
 	/* Enable port in pre-enable phase itself because as per hw team
 	 * recommendation, port should be enabled befor plane & pipe */
 	intel_dsi_enable(encoder);
@@ -672,9 +665,6 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
 	/* if disable packets are sent before sending shutdown packet then in
 	 * some next enable sequence send turn on packet error is observed */
 	drm_panel_disable(intel_dsi->panel);
-
-	for_each_dsi_port(port, intel_dsi->ports)
-		wait_for_dsi_fifo_empty(intel_dsi, port);
 }
 
 static void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index 5967ea6..d567823 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -130,6 +130,8 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
 	return container_of(encoder, struct intel_dsi, base.base);
 }
 
+void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port);
+
 bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv);
 int intel_compute_dsi_pll(struct intel_encoder *encoder,
 			  struct intel_crtc_state *config);
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 84b3683..995f72d 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -192,6 +192,8 @@ static const u8 *mipi_exec_send_packet(struct intel_dsi *intel_dsi,
 		break;
 	}
 
+	wait_for_dsi_fifo_empty(intel_dsi, port);
+
 out:
 	data += len;
 
-- 
2.9.3

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

[PATCH resend 02/15] drm/i915/dsi: Merge intel_dsi_disable/enable into their respective callers

[Hans de Goede]

intel_dsi_disable/enable only have one caller, merge them into their
respective callers.

Change msleep(2) into usleep_range(2000, 5000) to make checkpatch happy,
otherwise no functional changes.

The main advantage of this change is that it makes it easier to
follow all the steps of the panel enable / disable sequence when
reading the code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 108 ++++++++++++++++-----------------------
 1 file changed, 44 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 2f0af98..e808dce 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -505,32 +505,6 @@ static void intel_dsi_port_disable(struct intel_encoder *encoder)
 	}
 }
 
-static void intel_dsi_enable(struct intel_encoder *encoder)
-{
-	struct drm_device *dev = encoder->base.dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
-	enum port port;
-
-	DRM_DEBUG_KMS("\n");
-
-	if (is_cmd_mode(intel_dsi)) {
-		for_each_dsi_port(port, intel_dsi->ports)
-			I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(port), 8 * 4);
-	} else {
-		msleep(20); /* XXX */
-		for_each_dsi_port(port, intel_dsi->ports)
-			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
-		msleep(100);
-
-		drm_panel_enable(intel_dsi->panel);
-
-		intel_dsi_port_enable(encoder);
-	}
-
-	intel_panel_enable_backlight(intel_dsi->attached_connector);
-}
-
 static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
 			      struct intel_crtc_state *pipe_config);
 
@@ -540,6 +514,7 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	enum port port;
 	u32 val;
 
 	DRM_DEBUG_KMS("\n");
@@ -586,7 +561,21 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 
 	/* Enable port in pre-enable phase itself because as per hw team
 	 * recommendation, port should be enabled befor plane & pipe */
-	intel_dsi_enable(encoder);
+	if (is_cmd_mode(intel_dsi)) {
+		for_each_dsi_port(port, intel_dsi->ports)
+			I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(port), 8 * 4);
+	} else {
+		msleep(20); /* XXX */
+		for_each_dsi_port(port, intel_dsi->ports)
+			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
+		msleep(100);
+
+		drm_panel_enable(intel_dsi->panel);
+
+		intel_dsi_port_enable(encoder);
+	}
+
+	intel_panel_enable_backlight(intel_dsi->attached_connector);
 }
 
 static void intel_dsi_enable_nop(struct intel_encoder *encoder,
@@ -631,42 +620,6 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
 	}
 }
 
-static void intel_dsi_disable(struct intel_encoder *encoder)
-{
-	struct drm_device *dev = encoder->base.dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
-	enum port port;
-	u32 temp;
-
-	DRM_DEBUG_KMS("\n");
-
-	if (is_vid_mode(intel_dsi)) {
-		for_each_dsi_port(port, intel_dsi->ports)
-			wait_for_dsi_fifo_empty(intel_dsi, port);
-
-		intel_dsi_port_disable(encoder);
-		msleep(2);
-	}
-
-	for_each_dsi_port(port, intel_dsi->ports) {
-		/* Panel commands can be sent when clock is in LP11 */
-		I915_WRITE(MIPI_DEVICE_READY(port), 0x0);
-
-		intel_dsi_reset_clocks(encoder, port);
-		I915_WRITE(MIPI_EOT_DISABLE(port), CLOCKSTOP);
-
-		temp = I915_READ(MIPI_DSI_FUNC_PRG(port));
-		temp &= ~VID_MODE_FORMAT_MASK;
-		I915_WRITE(MIPI_DSI_FUNC_PRG(port), temp);
-
-		I915_WRITE(MIPI_DEVICE_READY(port), 0x1);
-	}
-	/* if disable packets are sent before sending shutdown packet then in
-	 * some next enable sequence send turn on packet error is observed */
-	drm_panel_disable(intel_dsi->panel);
-}
-
 static void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
@@ -716,11 +669,38 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	enum port port;
 	u32 val;
 
 	DRM_DEBUG_KMS("\n");
 
-	intel_dsi_disable(encoder);
+	if (is_vid_mode(intel_dsi)) {
+		for_each_dsi_port(port, intel_dsi->ports)
+			wait_for_dsi_fifo_empty(intel_dsi, port);
+
+		intel_dsi_port_disable(encoder);
+		usleep_range(2000, 5000);
+	}
+
+	for_each_dsi_port(port, intel_dsi->ports) {
+		/* Panel commands can be sent when clock is in LP11 */
+		I915_WRITE(MIPI_DEVICE_READY(port), 0x0);
+
+		intel_dsi_reset_clocks(encoder, port);
+		I915_WRITE(MIPI_EOT_DISABLE(port), CLOCKSTOP);
+
+		val = I915_READ(MIPI_DSI_FUNC_PRG(port));
+		val &= ~VID_MODE_FORMAT_MASK;
+		I915_WRITE(MIPI_DSI_FUNC_PRG(port), val);
+
+		I915_WRITE(MIPI_DEVICE_READY(port), 0x1);
+	}
+
+	/*
+	 * if disable packets are sent before sending shutdown packet then in
+	 * some next enable sequence send turn on packet error is observed
+	 */
+	drm_panel_disable(intel_dsi->panel);
 
 	intel_dsi_clear_device_ready(encoder);
 
-- 
2.9.3

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

[PATCH resend 03/15] drm/i915/dsi: Add intel_dsi_unprepare() helper

[Hans de Goede]

The enable path has an intel_dsi_prepare() helper which prepares various
registers for the mode-set. Move the code undoing this to a new
intel_dsi_unprepare() helper function for better symmetry between the
enable and disable paths. No functional changes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index e808dce..d741280 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -507,6 +507,7 @@ static void intel_dsi_port_disable(struct intel_encoder *encoder)
 
 static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
 			      struct intel_crtc_state *pipe_config);
+static void intel_dsi_unprepare(struct intel_encoder *encoder);
 
 static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 				 struct intel_crtc_state *pipe_config,
@@ -682,19 +683,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 		usleep_range(2000, 5000);
 	}
 
-	for_each_dsi_port(port, intel_dsi->ports) {
-		/* Panel commands can be sent when clock is in LP11 */
-		I915_WRITE(MIPI_DEVICE_READY(port), 0x0);
-
-		intel_dsi_reset_clocks(encoder, port);
-		I915_WRITE(MIPI_EOT_DISABLE(port), CLOCKSTOP);
-
-		val = I915_READ(MIPI_DSI_FUNC_PRG(port));
-		val &= ~VID_MODE_FORMAT_MASK;
-		I915_WRITE(MIPI_DSI_FUNC_PRG(port), val);
-
-		I915_WRITE(MIPI_DEVICE_READY(port), 0x1);
-	}
+	intel_dsi_unprepare(encoder);
 
 	/*
 	 * if disable packets are sent before sending shutdown packet then in
@@ -1303,6 +1292,28 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
 	}
 }
 
+static void intel_dsi_unprepare(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	enum port port;
+	u32 val;
+
+	for_each_dsi_port(port, intel_dsi->ports) {
+		/* Panel commands can be sent when clock is in LP11 */
+		I915_WRITE(MIPI_DEVICE_READY(port), 0x0);
+
+		intel_dsi_reset_clocks(encoder, port);
+		I915_WRITE(MIPI_EOT_DISABLE(port), CLOCKSTOP);
+
+		val = I915_READ(MIPI_DSI_FUNC_PRG(port));
+		val &= ~VID_MODE_FORMAT_MASK;
+		I915_WRITE(MIPI_DSI_FUNC_PRG(port), val);
+
+		I915_WRITE(MIPI_DEVICE_READY(port), 0x1);
+	}
+}
+
 static int intel_dsi_get_modes(struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
-- 
2.9.3

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

[PATCH resend 04/15] drm/i915/dsi: Move intel_dsi_clear_device_ready()

[Hans de Goede]

Move the intel_dsi_clear_device_ready() function to higher up in
intel_dsi.c this pairs it with intel_dsi_device_ready(); and pairs
intel_dsi_*enable* with intel_dsi_*disable without
intel_dsi_clear_device_ready() sitting in the middle of them.

This commit purely moves code around, it does not make any
changes what-so-ever.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 86 ++++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index d741280..072f99b 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -433,6 +433,49 @@ static void intel_dsi_device_ready(struct intel_encoder *encoder)
 		bxt_dsi_device_ready(encoder);
 }
 
+static void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	enum port port;
+
+	DRM_DEBUG_KMS("\n");
+	for_each_dsi_port(port, intel_dsi->ports) {
+		/* Common bit for both MIPI Port A & MIPI Port C on VLV/CHV */
+		i915_reg_t port_ctrl = IS_GEN9_LP(dev_priv) ?
+			BXT_MIPI_PORT_CTRL(port) : MIPI_PORT_CTRL(PORT_A);
+		u32 val;
+
+		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
+							ULPS_STATE_ENTER);
+		usleep_range(2000, 2500);
+
+		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
+							ULPS_STATE_EXIT);
+		usleep_range(2000, 2500);
+
+		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
+							ULPS_STATE_ENTER);
+		usleep_range(2000, 2500);
+
+		/* Wait till Clock lanes are in LP-00 state for MIPI Port A
+		 * only. MIPI Port C has no similar bit for checking
+		 */
+		if (intel_wait_for_register(dev_priv,
+					    port_ctrl, AFE_LATCHOUT, 0,
+					    30))
+			DRM_ERROR("DSI LP not going Low\n");
+
+		/* Disable MIPI PHY transparent latch */
+		val = I915_READ(port_ctrl);
+		I915_WRITE(port_ctrl, val & ~LP_OUTPUT_HOLD);
+		usleep_range(1000, 1500);
+
+		I915_WRITE(MIPI_DEVICE_READY(port), 0x00);
+		usleep_range(2000, 2500);
+	}
+}
+
 static void intel_dsi_port_enable(struct intel_encoder *encoder)
 {
 	struct drm_device *dev = encoder->base.dev;
@@ -621,49 +664,6 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
 	}
 }
 
-static void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
-{
-	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
-	enum port port;
-
-	DRM_DEBUG_KMS("\n");
-	for_each_dsi_port(port, intel_dsi->ports) {
-		/* Common bit for both MIPI Port A & MIPI Port C on VLV/CHV */
-		i915_reg_t port_ctrl = IS_GEN9_LP(dev_priv) ?
-			BXT_MIPI_PORT_CTRL(port) : MIPI_PORT_CTRL(PORT_A);
-		u32 val;
-
-		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
-							ULPS_STATE_ENTER);
-		usleep_range(2000, 2500);
-
-		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
-							ULPS_STATE_EXIT);
-		usleep_range(2000, 2500);
-
-		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
-							ULPS_STATE_ENTER);
-		usleep_range(2000, 2500);
-
-		/* Wait till Clock lanes are in LP-00 state for MIPI Port A
-		 * only. MIPI Port C has no similar bit for checking
-		 */
-		if (intel_wait_for_register(dev_priv,
-					    port_ctrl, AFE_LATCHOUT, 0,
-					    30))
-			DRM_ERROR("DSI LP not going Low\n");
-
-		/* Disable MIPI PHY transparent latch */
-		val = I915_READ(port_ctrl);
-		I915_WRITE(port_ctrl, val & ~LP_OUTPUT_HOLD);
-		usleep_range(1000, 1500);
-
-		I915_WRITE(MIPI_DEVICE_READY(port), 0x00);
-		usleep_range(2000, 2500);
-	}
-}
-
 static void intel_dsi_post_disable(struct intel_encoder *encoder,
 				   struct intel_crtc_state *pipe_config,
 				   struct drm_connector_state *conn_state)
-- 
2.9.3

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

[PATCH resend 05/15] drm/i915/dsi: Document the panel enable / disable sequences from the spec

[Hans de Goede]

Document the DSI panel enable / disable sequences from the spec,
for easy comparison between the code and the spec.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 68 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 072f99b..8808f87 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -552,6 +552,74 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
 			      struct intel_crtc_state *pipe_config);
 static void intel_dsi_unprepare(struct intel_encoder *encoder);
 
+/*
+ * Panel enable/disable sequences from the VBT spec.
+ *
+ * Note the spec has AssertReset / DeassertReset swapped from their
+ * usual naming. We use the normal names to avoid confusion (so below
+ * they are swapped compared to the spec).
+ *
+ * v2 sequence for video mode:
+ * - power on
+ * - wait t1+t2
+ * - MIPIDeassertResetPin
+ * - clk/data lines to lp-11
+ * - MIPISendInitialDcsCmds
+ * - turn on DPI
+ * - MIPIDisplayOn
+ * - wait t5
+ * - backlight on
+ * ...
+ * - backlight off
+ * - wait t6
+ * - MIPIDisplayOff
+ * - turn off DPI
+ * - clk/data lines to lp-00
+ * - MIPIAssertResetPin
+ * - wait t3
+ * - power off
+ * - wait t4
+ *
+ * v3 sequence for video mode:
+ * - MIPIPanelPowerOn
+ * - MIPIDeassertResetPin
+ * - set clk/data lines to lp-11
+ * - MIPISendInitialDcsCmds (LP)
+ * - turn on DPI
+ * - MIPITearOn (command mode only) + MIPIDisplayOn (LP and HS)
+ * - MIPIBacklightOn
+ * ...
+ * - MIPIBacklightOff
+ * - turn off DPI
+ * - MIPITearOff + MIPIDisplayOff (LP)
+ * - clk/data lines to lp-00
+ * - MIPIAssertResetPin
+ * - MIPIPanelPowerOff
+ *
+ * sequence for command mode:
+ * - power on
+ * - wait t1+t2
+ * - MIPIDeassertResetPin
+ * - clk/data lines to lp-11
+ * - MIPISendInitialDcsCmds
+ * - MIPITearOn
+ * - MIPIDisplayOn
+ * - set pipe to dsr mode
+ * - wait t5
+ * - backlight on
+ * ... issue write_mem_start/write_mem_continue commands ...
+ * - backlight off
+ * - wait t6
+ * - disable pipe dsr mode
+ * - MIPITearOff
+ * - MIPIDisplayOff
+ * - clk/data lines to lp-00
+ * - MIPIAssertResetPin
+ * - wait t3
+ * - power off
+ * - wait t4
+ */
+
 static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 				 struct intel_crtc_state *pipe_config,
 				 struct drm_connector_state *conn_state)
-- 
2.9.3

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

[PATCH resend 06/15] drm/i915/dsi: Make intel_dsi_enable/disable directly exec VBT sequences

[Hans de Goede]

The drm_panel_enable/disable and drm_panel_prepare/unprepare calls are
not fine grained enough to abstract all the different steps we need to
take (and VBT sequences we need to exec) properly. So simply remove the
panel _enable/disable and prepare/unprepare callbacks and instead
export intel_dsi_exec_vbt_sequence() from intel_dsi_panel_vbt.c
and call that from intel_dsi_enable/disable().

No functional changes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c           | 14 +++++++---
 drivers/gpu/drm/i915/intel_dsi.h           |  3 +++
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 43 ++----------------------------
 3 files changed, 15 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 8808f87..78d5884 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -669,7 +669,10 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 	/* put device in ready state */
 	intel_dsi_device_ready(encoder);
 
-	drm_panel_prepare(intel_dsi->panel);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
 
 	/* Enable port in pre-enable phase itself because as per hw team
 	 * recommendation, port should be enabled befor plane & pipe */
@@ -682,7 +685,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
 		msleep(100);
 
-		drm_panel_enable(intel_dsi->panel);
+		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
+		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
 
 		intel_dsi_port_enable(encoder);
 	}
@@ -757,7 +761,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	 * if disable packets are sent before sending shutdown packet then in
 	 * some next enable sequence send turn on packet error is observed
 	 */
-	drm_panel_disable(intel_dsi->panel);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
 
 	intel_dsi_clear_device_ready(encoder);
 
@@ -782,7 +787,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 		I915_WRITE(DSPCLK_GATE_D, val);
 	}
 
-	drm_panel_unprepare(intel_dsi->panel);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
 
 	msleep(intel_dsi->panel_off_delay);
 
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index d567823..5486491 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -132,6 +132,9 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
 
 void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port);
 
+void intel_dsi_exec_vbt_sequence(struct intel_dsi *intel_dsi,
+				 enum mipi_seq seq_id);
+
 bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv);
 int intel_compute_dsi_pll(struct intel_encoder *encoder,
 			  struct intel_crtc_state *config);
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 995f72d..0ce1086 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -426,10 +426,9 @@ static const char *sequence_name(enum mipi_seq seq_id)
 		return "(unknown)";
 }
 
-static void generic_exec_sequence(struct drm_panel *panel, enum mipi_seq seq_id)
+void intel_dsi_exec_vbt_sequence(struct intel_dsi *intel_dsi,
+				 enum mipi_seq seq_id)
 {
-	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
-	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
 	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
 	const u8 *data;
 	fn_mipi_elem_exec mipi_elem_exec;
@@ -493,40 +492,6 @@ static void generic_exec_sequence(struct drm_panel *panel, enum mipi_seq seq_id)
 	}
 }
 
-static int vbt_panel_prepare(struct drm_panel *panel)
-{
-	generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
-	generic_exec_sequence(panel, MIPI_SEQ_POWER_ON);
-	generic_exec_sequence(panel, MIPI_SEQ_DEASSERT_RESET);
-	generic_exec_sequence(panel, MIPI_SEQ_INIT_OTP);
-
-	return 0;
-}
-
-static int vbt_panel_unprepare(struct drm_panel *panel)
-{
-	generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
-	generic_exec_sequence(panel, MIPI_SEQ_POWER_OFF);
-
-	return 0;
-}
-
-static int vbt_panel_enable(struct drm_panel *panel)
-{
-	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_ON);
-	generic_exec_sequence(panel, MIPI_SEQ_BACKLIGHT_ON);
-
-	return 0;
-}
-
-static int vbt_panel_disable(struct drm_panel *panel)
-{
-	generic_exec_sequence(panel, MIPI_SEQ_BACKLIGHT_OFF);
-	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_OFF);
-
-	return 0;
-}
-
 static int vbt_panel_get_modes(struct drm_panel *panel)
 {
 	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
@@ -550,10 +515,6 @@ static int vbt_panel_get_modes(struct drm_panel *panel)
 }
 
 static const struct drm_panel_funcs vbt_panel_funcs = {
-	.disable = vbt_panel_disable,
-	.unprepare = vbt_panel_unprepare,
-	.prepare = vbt_panel_prepare,
-	.enable = vbt_panel_enable,
 	.get_modes = vbt_panel_get_modes,
 };
 
-- 
2.9.3

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

[PATCH resend 07/15] drm/i915/dsi: Drop bogus MIPI_SEQ_ASSERT_RESET before POWER_ON

[Hans de Goede]

MIPI_SEQ_ASSERT_RESET before POWER_ON is not necessary for 2 reasons:
1) The reset should already be asserted before intel_dsi_pre_enable()
   gets called
2) Most (some?) VBTs will ensure reset was asserted in their
   MIPI_SEQ_DEASSERT_RESET themselves

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 78d5884..4ebf308 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -669,7 +669,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 	/* put device in ready state */
 	intel_dsi_device_ready(encoder);
 
-	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
-- 
2.9.3

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

[PATCH resend 08/15] drm/i915/dsi: Move MIPI_SEQ_POWER_ON/OFF calls together with pmic gpio calls

[Hans de Goede]

Now that we are no longer bound to the drm_panel_ callbacks, call
MIPI_SEQ_POWER_ON/OFF at the proper place.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 4ebf308..de0558b 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -651,10 +651,10 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 
 	intel_dsi_prepare(encoder, pipe_config);
 
-	/* Panel Enable over CRC PMIC */
+	/* Power on, try both CRC pmic gpio and VBT */
 	if (intel_dsi->gpio_panel)
 		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
-
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
 	msleep(intel_dsi->panel_on_delay);
 
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
@@ -669,7 +669,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 	/* put device in ready state */
 	intel_dsi_device_ready(encoder);
 
-	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
 
@@ -787,11 +786,10 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	}
 
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
-	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
 
+	/* Power off, try both CRC pmic gpio and VBT */
 	msleep(intel_dsi->panel_off_delay);
-
-	/* Panel Disable over CRC PMIC */
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
 	if (intel_dsi->gpio_panel)
 		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 0);
 
-- 
2.9.3

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

[PATCH resend 09/15] drm/i915/dsi: Group DPOunit clock gate workaround with PLL enable

[Hans de Goede]

Move the DPOunit clock gate workaround to directly after the PLL enable.

The exact location of the workaround does not matter and there are 2
reasons to group it with the PLL enable:

1) This moves it out of the middle of the init sequence from the spec,
   making it easier to follow the init sequence / compare it to the spec

2) It is grouped with the pll disable call in intel_dsi_post_disable,
   so for consistency it should be grouped with the pll enable in
   intel_dsi_pre_enable

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index de0558b..f4539f1 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -649,14 +649,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 		I915_WRITE(BXT_P_DSI_REGULATOR_TX_CTRL, 0);
 	}
 
-	intel_dsi_prepare(encoder, pipe_config);
-
-	/* Power on, try both CRC pmic gpio and VBT */
-	if (intel_dsi->gpio_panel)
-		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
-	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
-	msleep(intel_dsi->panel_on_delay);
-
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		u32 val;
 
@@ -666,6 +658,14 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 		I915_WRITE(DSPCLK_GATE_D, val);
 	}
 
+	intel_dsi_prepare(encoder, pipe_config);
+
+	/* Power on, try both CRC pmic gpio and VBT */
+	if (intel_dsi->gpio_panel)
+		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
+	msleep(intel_dsi->panel_on_delay);
+
 	/* put device in ready state */
 	intel_dsi_device_ready(encoder);
 
-- 
2.9.3

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

[PATCH resend 10/15] drm/i915/dsi: Execute MIPI_SEQ_DEASSERT_RESET before calling device_ready()

[Hans de Goede]

Execute MIPI_SEQ_DEASSERT_RESET before putting the device in ready
state (LP-11), this is the sequence in which things should be done
according to the spec.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index f4539f1..8408f59 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -666,10 +666,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
 	msleep(intel_dsi->panel_on_delay);
 
-	/* put device in ready state */
+	/* Deassert reset */
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
+
+	/* Put device in ready state (LP-11) */
 	intel_dsi_device_ready(encoder);
 
-	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
+	/* Send initialization commands in LP mode */
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
 
 	/* Enable port in pre-enable phase itself because as per hw team
@@ -762,6 +765,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
 
+	/* Transition to LP-00 */
 	intel_dsi_clear_device_ready(encoder);
 
 	if (IS_BROXTON(dev_priv)) {
@@ -785,6 +789,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 		I915_WRITE(DSPCLK_GATE_D, val);
 	}
 
+	/* Assert reset */
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
 
 	/* Power off, try both CRC pmic gpio and VBT */
-- 
2.9.3

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

[PATCH resend 11/15] drm/i915/dsi: Group MIPI_SEQ_BACKLIGHT_ON/OFF with panel_[en|dis]able_backlight

[Hans de Goede]

Execute the MIPI_SEQ_BACKLIGHT_ON/OFF VBT sequences at the same time as
we call intel_panel_enable_backlight() / intel_panel_disable_backlight().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 8408f59..a8d0948 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -687,12 +687,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 		msleep(100);
 
 		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
-		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
 
 		intel_dsi_port_enable(encoder);
 	}
 
+	/* Enable backlight, both pwm and VBT */
 	intel_panel_enable_backlight(intel_dsi->attached_connector);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
 }
 
 static void intel_dsi_enable_nop(struct intel_encoder *encoder,
@@ -718,6 +719,8 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
 
 	DRM_DEBUG_KMS("\n");
 
+	/* Disable backlight, both VBT and pwm */
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
 	intel_panel_disable_backlight(intel_dsi->attached_connector);
 
 	/*
@@ -762,7 +765,6 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	 * if disable packets are sent before sending shutdown packet then in
 	 * some next enable sequence send turn on packet error is observed
 	 */
-	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
 
 	/* Transition to LP-00 */
-- 
2.9.3

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

[PATCH resend 12/15] drm/i915/dsi: Document always using v3 SHUTDOWN / MIPI_SEQ_DISPLAY_OFF order

[Hans de Goede]

According to the spec for v2 VBTs we should call MIPI_SEQ_DISPLAY_OFF
before sending SHUTDOWN, where as for v3 VBTs we should send SHUTDOWN
first.

Since the v2 order has known issues, we use the v3 order everywhere,
add a comment documenting this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index a8d0948..1914311 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -732,6 +732,11 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
 			I915_WRITE(MIPI_DEVICE_READY(port), 0);
 	}
 
+	/*
+	 * XXX: According to the spec we should send SHUTDOWN before
+	 * MIPI_SEQ_DISPLAY_OFF only for v3+ VBTs, but testing in the field
+	 * has shown that we should do this for v2 VBTs too?
+	 */
 	if (is_vid_mode(intel_dsi)) {
 		/* Send Shutdown command to the panel in LP mode */
 		for_each_dsi_port(port, intel_dsi->ports)
@@ -764,6 +769,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	/*
 	 * if disable packets are sent before sending shutdown packet then in
 	 * some next enable sequence send turn on packet error is observed
+	 * XXX spec specifies SHUTDOWN before MIPI_SEQ_DISPLAY_OFF for
+	 * v3 VBTs, but not for v2 VBTs?
 	 */
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
 
-- 
2.9.3

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

[PATCH resend 13/15] drm/i915/dsi: Execute MIPI_SEQ_TEAR_OFF from intel_dsi_post_disable

[Hans de Goede]

For v3 VBTs we should call MIPI_SEQ_TEAR_OFF before
MIPI_SEQ_DISPLAY_OFF, for non v3 VBTs this is a nop.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 1914311..90263d6 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -772,6 +772,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	 * XXX spec specifies SHUTDOWN before MIPI_SEQ_DISPLAY_OFF for
 	 * v3 VBTs, but not for v2 VBTs?
 	 */
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_TEAR_OFF);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
 
 	/* Transition to LP-00 */
-- 
2.9.3

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

[PATCH resend 14/15] drm/i915/dsi: Call MIPI_SEQ_TEAR_ON and DISPLAY_ON for cmd-mode (untested)

[Hans de Goede]

According to the spec we should call MIPI_SEQ_TEAR_ON and DISPLAY_ON
on enable for cmd-mode, just like we already call their counterparts
on disable. Note: untested, my panel is a vid-mode panel.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 90263d6..a001e43 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -680,6 +680,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 	if (is_cmd_mode(intel_dsi)) {
 		for_each_dsi_port(port, intel_dsi->ports)
 			I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(port), 8 * 4);
+		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_TEAR_ON);
+		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
 	} else {
 		msleep(20); /* XXX */
 		for_each_dsi_port(port, intel_dsi->ports)
-- 
2.9.3

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

[PATCH resend 15/15] drm/i915/dsi: Skip delays for v3 VBTs in vid-mode

[Hans de Goede]

For v3 VBTs in vid-mode the delays are part of the VBT sequences, so
we should not also delay ourselves otherwise we get double delays.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index a001e43..9e858c7 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -552,6 +552,17 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
 			      struct intel_crtc_state *pipe_config);
 static void intel_dsi_unprepare(struct intel_encoder *encoder);
 
+static void intel_dsi_msleep(struct intel_dsi *intel_dsi, int msec)
+{
+	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
+
+	/* For v3 VBTs in vid-mode the delays are part of the VBT sequences */
+	if (is_vid_mode(intel_dsi) && dev_priv->vbt.dsi.seq_version >= 3)
+		return;
+
+	msleep(msec);
+}
+
 /*
  * Panel enable/disable sequences from the VBT spec.
  *
@@ -664,7 +675,7 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 	if (intel_dsi->gpio_panel)
 		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
-	msleep(intel_dsi->panel_on_delay);
+	intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay);
 
 	/* Deassert reset */
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
@@ -686,7 +697,7 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 		msleep(20); /* XXX */
 		for_each_dsi_port(port, intel_dsi->ports)
 			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
-		msleep(100);
+		intel_dsi_msleep(intel_dsi, 100);
 
 		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
 
@@ -805,7 +816,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
 
 	/* Power off, try both CRC pmic gpio and VBT */
-	msleep(intel_dsi->panel_off_delay);
+	intel_dsi_msleep(intel_dsi, intel_dsi->panel_off_delay);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
 	if (intel_dsi->gpio_panel)
 		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 0);
@@ -814,7 +825,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	 * FIXME As we do with eDP, just make a note of the time here
 	 * and perform the wait before the next panel power on.
 	 */
-	msleep(intel_dsi->panel_pwr_cycle_delay);
+	intel_dsi_msleep(intel_dsi, intel_dsi->panel_pwr_cycle_delay);
 }
 
 static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
-- 
2.9.3

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

Re: [PATCH resend 01/15] drm/i915/dsi: Move calling of wait_for_dsi_fifo_empty to mipi_exec_send_packet

[Bob Paauwe]

On Mon, 20 Feb 2017 15:08:31 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Instead of calling wait_for_dsi_fifo_empty on all dsi ports after calling
> a drm_panel_foo helper which calls VBT sequences, move it to the VBT
> mipi_exec_send_packet helper, which is the one VBT instruction which
> actually puts data in the fifo.
> 
> This results in a nice cleanup making it clearer what all the steps on
> intel_dsi_enable / disable are and this also makes the VBT code properly
> wait till a command has actually been send before executing the next
> steps (typically a delay) in the VBT sequence.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c           | 12 +-----------
>  drivers/gpu/drm/i915/intel_dsi.h           |  2 ++
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |  2 ++
>  3 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index c26fe4f..2f0af98 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -80,7 +80,7 @@ enum mipi_dsi_pixel_format pixel_format_from_register_bits(u32 fmt)
>  	}
>  }
>  
> -static void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port)

In looking at the code a bit, it looks like there could be an
opportunity to consolidate the all the places where it waits on the
FIFO status into one function.  But that's probably a separate patch.

I like what this is doing.

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>


> +void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port)
>  {
>  	struct drm_encoder *encoder = &intel_dsi->base.base;
>  	struct drm_device *dev = encoder->dev;
> @@ -525,9 +525,6 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>  
>  		drm_panel_enable(intel_dsi->panel);
>  
> -		for_each_dsi_port(port, intel_dsi->ports)
> -			wait_for_dsi_fifo_empty(intel_dsi, port);
> -
>  		intel_dsi_port_enable(encoder);
>  	}
>  
> @@ -543,7 +540,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> -	enum port port;
>  	u32 val;if (is_vid_mode(intel_dsi))
>  
>  	DRM_DEBUG_KMS("\n");
> @@ -588,9 +584,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  
>  	drm_panel_prepare(intel_dsi->panel);
>  
> -	for_each_dsi_port(port, intel_dsi->ports)
> -		wait_for_dsi_fifo_empty(intel_dsi, port);
> -
>  	/* Enable port in pre-enable phase itself because as per hw team
>  	 * recommendation, port should be enabled befor plane & pipe */
>  	intel_dsi_enable(encoder);
> @@ -672,9 +665,6 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
>  	/* if disable packets are sent before sending shutdown packet then in
>  	 * some next enable sequence send turn on packet error is observed */
>  	drm_panel_disable(intel_dsi->panel);
> -
> -	for_each_dsi_port(port, intel_dsi->ports)
> -		wait_for_dsi_fifo_empty(intel_dsi, port);
>  }
>  
>  static void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index 5967ea6..d567823 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -130,6 +130,8 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
>  	return container_of(encoder, struct intel_dsi, base.base);
>  }
>  
> +void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port);
> +
>  bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv);
>  int intel_compute_dsi_pll(struct intel_encoder *encoder,
>  			  struct intel_crtc_state *config);
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 84b3683..995f72d 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -192,6 +192,8 @@ static const u8 *mipi_exec_send_packet(struct intel_dsi *intel_dsi,
>  		break;
>  	}
>  
> +	wait_for_dsi_fifo_empty(intel_dsi, port);
> +
>  out:
>  	data += len;
>  



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 02/15] drm/i915/dsi: Merge intel_dsi_disable/enable into their respective callers

[Bob Paauwe]

On Mon, 20 Feb 2017 15:08:32 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> intel_dsi_disable/enable only have one caller, merge them into their
> respective callers.
> 
> Change msleep(2) into usleep_range(2000, 5000) to make checkpatch happy,
> otherwise no functional changes.
> 
> The main advantage of this change is that it makes it easier to
> follow all the steps of the panel enable / disable sequence when
> reading the code.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 108 ++++++++++++++++-----------------------
>  1 file changed, 44 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 2f0af98..e808dce 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -505,32 +505,6 @@ static void intel_dsi_port_disable(struct intel_encoder *encoder)
>  	}
>  }
>  
> -static void intel_dsi_enable(struct intel_encoder *encoder)
> -{
> -	struct drm_device *dev = encoder->base.dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> -	enum port port;
> -
> -	DRM_DEBUG_KMS("\n");
> -
> -	if (is_cmd_mode(intel_dsi)) {
> -		for_each_dsi_port(port, intel_dsi->ports)
> -			I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(port), 8 * 4);
> -	} else {
> -		msleep(20); /* XXX */
> -		for_each_dsi_port(port, intel_dsi->ports)
> -			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
> -		msleep(100);
> -
> -		drm_panel_enable(intel_dsi->panel);
> -
> -		intel_dsi_port_enable(encoder);
> -	}
> -
> -	intel_panel_enable_backlight(intel_dsi->attached_connector);
> -}
> -
>  static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
>  			      struct intel_crtc_state *pipe_config);
>  
> @@ -540,6 +514,7 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> +	enum port port;
>  	u32 val;
>  
>  	DRM_DEBUG_KMS("\n");
> @@ -586,7 +561,21 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  
>  	/* Enable port in pre-enable phase itself because as per hw team
>  	 * recommendation, port should be enabled befor plane & pipe */
> -	intel_dsi_enable(encoder);
> +	if (is_cmd_mode(intel_dsi)) {
> +		for_each_dsi_port(port, intel_dsi->ports)
> +			I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(port), 8 * 4);
> +	} else {
> +		msleep(20); /* XXX */
> +		for_each_dsi_port(port, intel_dsi->ports)
> +			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
> +		msleep(100);
> +
> +		drm_panel_enable(intel_dsi->panel);
> +
> +		intel_dsi_port_enable(encoder);
> +	}
> +
> +	intel_panel_enable_backlight(intel_dsi->attached_connector);
>  }
>  
>  static void intel_dsi_enable_nop(struct intel_encoder *encoder,
> @@ -631,42 +620,6 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
>  	}
>  }
>  
> -static void intel_dsi_disable(struct intel_encoder *encoder)
> -{
> -	struct drm_device *dev = encoder->base.dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> -	enum port port;
> -	u32 temp;
> -
> -	DRM_DEBUG_KMS("\n");
> -
> -	if (is_vid_mode(intel_dsi)) {
> -		for_each_dsi_port(port, intel_dsi->ports)
> -			wait_for_dsi_fifo_empty(intel_dsi, port);

I realize that this is just moving the code around and the change seems
reasonable. So this is 
Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

But if I'm understanding the b-spec correctly, at least for BXT, the
wait for fifo empty above should only be necessary for command mode,
not video mode. 

> -
> -		intel_dsi_port_disable(encoder);
> -		msleep(2);
> -	}
> -
> -	for_each_dsi_port(port, intel_dsi->ports) {
> -		/* Panel commands can be sent when clock is in LP11 */
> -		I915_WRITE(MIPI_DEVICE_READY(port), 0x0);
> -
> -		intel_dsi_reset_clocks(encoder, port);
> -		I915_WRITE(MIPI_EOT_DISABLE(port), CLOCKSTOP);
> -
> -		temp = I915_READ(MIPI_DSI_FUNC_PRG(port));
> -		temp &= ~VID_MODE_FORMAT_MASK;
> -		I915_WRITE(MIPI_DSI_FUNC_PRG(port), temp);
> -
> -		I915_WRITE(MIPI_DEVICE_READY(port), 0x1);
> -	}
> -	/* if disable packets are sent before sending shutdown packet then in
> -	 * some next enable sequence send turn on packet error is observed */
> -	drm_panel_disable(intel_dsi->panel);
> -}
> -
>  static void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> @@ -716,11 +669,38 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> +	enum port port;
>  	u32 val;
>  
>  	DRM_DEBUG_KMS("\n");
>  
> -	intel_dsi_disable(encoder);
> +	if (is_vid_mode(intel_dsi)) {
> +		for_each_dsi_port(port, intel_dsi->ports)
> +			wait_for_dsi_fifo_empty(intel_dsi, port);
> +
> +		intel_dsi_port_disable(encoder);
> +		usleep_range(2000, 5000);
> +	}
> +
> +	for_each_dsi_port(port, intel_dsi->ports) {
> +		/* Panel commands can be sent when clock is in LP11 */
> +		I915_WRITE(MIPI_DEVICE_READY(port), 0x0);
> +
> +		intel_dsi_reset_clocks(encoder, port);
> +		I915_WRITE(MIPI_EOT_DISABLE(port), CLOCKSTOP);
> +
> +		val = I915_READ(MIPI_DSI_FUNC_PRG(port));
> +		val &= ~VID_MODE_FORMAT_MASK;
> +		I915_WRITE(MIPI_DSI_FUNC_PRG(port), val);
> +
> +		I915_WRITE(MIPI_DEVICE_READY(port), 0x1);
> +	}
> +
> +	/*
> +	 * if disable packets are sent before sending shutdown packet then in
> +	 * some next enable sequence send turn on packet error is observed
> +	 */
> +	drm_panel_disable(intel_dsi->panel);
>  
>  	intel_dsi_clear_device_ready(encoder);
>  



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 03/15] drm/i915/dsi: Add intel_dsi_unprepare() helper

[Bob Paauwe]

On Mon, 20 Feb 2017 15:08:33 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> The enable path has an intel_dsi_prepare() helper which prepares various
> registers for the mode-set. Move the code undoing this to a new
> intel_dsi_unprepare() helper function for better symmetry between the
> enable and disable paths. No functional changes.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index e808dce..d741280 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -507,6 +507,7 @@ static void intel_dsi_port_disable(struct intel_encoder *encoder)
>  
>  static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
>  			      struct intel_crtc_state *pipe_config);
> +static void intel_dsi_unprepare(struct intel_encoder *encoder);
>  
>  static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  				 struct intel_crtc_state *pipe_config,
> @@ -682,19 +683,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  		usleep_range(2000, 5000);
>  	}
>  
> -	for_each_dsi_port(port, intel_dsi->ports) {
> -		/* Panel commands can be sent when clock is in LP11 */
> -		I915_WRITE(MIPI_DEVICE_READY(port), 0x0);
> -
> -		intel_dsi_reset_clocks(encoder, port);
> -		I915_WRITE(MIPI_EOT_DISABLE(port), CLOCKSTOP);
> -
> -		val = I915_READ(MIPI_DSI_FUNC_PRG(port));
> -		val &= ~VID_MODE_FORMAT_MASK;
> -		I915_WRITE(MIPI_DSI_FUNC_PRG(port), val);
> -
> -		I915_WRITE(MIPI_DEVICE_READY(port), 0x1);
> -	}
> +	intel_dsi_unprepare(encoder);
>  
>  	/*
>  	 * if disable packets are sent before sending shutdown packet then in
> @@ -1303,6 +1292,28 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
>  	}
>  }
>  
> +static void intel_dsi_unprepare(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> +	enum port port;
> +	u32 val;
> +
> +	for_each_dsi_port(port, intel_dsi->ports) {
> +		/* Panel commands can be sent when clock is in LP11 */
> +		I915_WRITE(MIPI_DEVICE_READY(port), 0x0);
> +
> +		intel_dsi_reset_clocks(encoder, port);
> +		I915_WRITE(MIPI_EOT_DISABLE(port), CLOCKSTOP);
> +
> +		val = I915_READ(MIPI_DSI_FUNC_PRG(port));
> +		val &= ~VID_MODE_FORMAT_MASK;
> +		I915_WRITE(MIPI_DSI_FUNC_PRG(port), val);
> +
> +		I915_WRITE(MIPI_DEVICE_READY(port), 0x1);
> +	}
> +}
> +
>  static int intel_dsi_get_modes(struct drm_connector *connector)
>  {
>  	struct intel_connector *intel_connector = to_intel_connector(connector);



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 04/15] drm/i915/dsi: Move intel_dsi_clear_device_ready()

[Bob Paauwe]

On Mon, 20 Feb 2017 15:08:34 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Move the intel_dsi_clear_device_ready() function to higher up in
> intel_dsi.c this pairs it with intel_dsi_device_ready(); and pairs
> intel_dsi_*enable* with intel_dsi_*disable without
> intel_dsi_clear_device_ready() sitting in the middle of them.
> 
> This commit purely moves code around, it does not make any
> changes what-so-ever.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 86 ++++++++++++++++++++--------------------
>  1 file changed, 43 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index d741280..072f99b 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -433,6 +433,49 @@ static void intel_dsi_device_ready(struct intel_encoder *encoder)
>  		bxt_dsi_device_ready(encoder);
>  }
>  
> +static void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> +	enum port port;
> +
> +	DRM_DEBUG_KMS("\n");
> +	for_each_dsi_port(port, intel_dsi->ports) {
> +		/* Common bit for both MIPI Port A & MIPI Port C on VLV/CHV */
> +		i915_reg_t port_ctrl = IS_GEN9_LP(dev_priv) ?
> +			BXT_MIPI_PORT_CTRL(port) : MIPI_PORT_CTRL(PORT_A);
> +		u32 val;
> +
> +		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
> +							ULPS_STATE_ENTER);
> +		usleep_range(2000, 2500);
> +
> +		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
> +							ULPS_STATE_EXIT);
> +		usleep_range(2000, 2500);
> +
> +		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
> +							ULPS_STATE_ENTER);
> +		usleep_range(2000, 2500);
> +
> +		/* Wait till Clock lanes are in LP-00 state for MIPI Port A
> +		 * only. MIPI Port C has no similar bit for checking
> +		 */
> +		if (intel_wait_for_register(dev_priv,
> +					    port_ctrl, AFE_LATCHOUT, 0,
> +					    30))
> +			DRM_ERROR("DSI LP not going Low\n");
> +
> +		/* Disable MIPI PHY transparent latch */
> +		val = I915_READ(port_ctrl);
> +		I915_WRITE(port_ctrl, val & ~LP_OUTPUT_HOLD);
> +		usleep_range(1000, 1500);
> +
> +		I915_WRITE(MIPI_DEVICE_READY(port), 0x00);
> +		usleep_range(2000, 2500);
> +	}
> +}
> +
>  static void intel_dsi_port_enable(struct intel_encoder *encoder)
>  {
>  	struct drm_device *dev = encoder->base.dev;
> @@ -621,49 +664,6 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
>  	}
>  }
>  
> -static void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> -	enum port port;
> -
> -	DRM_DEBUG_KMS("\n");
> -	for_each_dsi_port(port, intel_dsi->ports) {
> -		/* Common bit for both MIPI Port A & MIPI Port C on VLV/CHV */
> -		i915_reg_t port_ctrl = IS_GEN9_LP(dev_priv) ?
> -			BXT_MIPI_PORT_CTRL(port) : MIPI_PORT_CTRL(PORT_A);
> -		u32 val;
> -
> -		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
> -							ULPS_STATE_ENTER);
> -		usleep_range(2000, 2500);
> -
> -		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
> -							ULPS_STATE_EXIT);
> -		usleep_range(2000, 2500);
> -
> -		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
> -							ULPS_STATE_ENTER);
> -		usleep_range(2000, 2500);
> -
> -		/* Wait till Clock lanes are in LP-00 state for MIPI Port A
> -		 * only. MIPI Port C has no similar bit for checking
> -		 */
> -		if (intel_wait_for_register(dev_priv,
> -					    port_ctrl, AFE_LATCHOUT, 0,
> -					    30))
> -			DRM_ERROR("DSI LP not going Low\n");
> -
> -		/* Disable MIPI PHY transparent latch */
> -		val = I915_READ(port_ctrl);
> -		I915_WRITE(port_ctrl, val & ~LP_OUTPUT_HOLD);
> -		usleep_range(1000, 1500);
> -
> -		I915_WRITE(MIPI_DEVICE_READY(port), 0x00);
> -		usleep_range(2000, 2500);
> -	}
> -}
> -
>  static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  				   struct intel_crtc_state *pipe_config,
>  				   struct drm_connector_state *conn_state)



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 05/15] drm/i915/dsi: Document the panel enable / disable sequences from the spec

[Bob Paauwe]

On Mon, 20 Feb 2017 15:08:35 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Document the DSI panel enable / disable sequences from the spec,
> for easy comparison between the code and the spec.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 68 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 072f99b..8808f87 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -552,6 +552,74 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
>  			      struct intel_crtc_state *pipe_config);
>  static void intel_dsi_unprepare(struct intel_encoder *encoder);
>  
> +/*
> + * Panel enable/disable sequences from the VBT spec.
> + *
> + * Note the spec has AssertReset / DeassertReset swapped from their
> + * usual naming. We use the normal names to avoid confusion (so below
> + * they are swapped compared to the spec).
> + *
> + * v2 sequence for video mode:
> + * - power on
> + * - wait t1+t2
> + * - MIPIDeassertResetPin
> + * - clk/data lines to lp-11
> + * - MIPISendInitialDcsCmds
> + * - turn on DPI
> + * - MIPIDisplayOn
> + * - wait t5
> + * - backlight on
> + * ...
> + * - backlight off
> + * - wait t6
> + * - MIPIDisplayOff
> + * - turn off DPI
> + * - clk/data lines to lp-00
> + * - MIPIAssertResetPin
> + * - wait t3
> + * - power off
> + * - wait t4
> + *
> + * v3 sequence for video mode:
> + * - MIPIPanelPowerOn
> + * - MIPIDeassertResetPin
> + * - set clk/data lines to lp-11
> + * - MIPISendInitialDcsCmds (LP)
> + * - turn on DPI
> + * - MIPITearOn (command mode only) + MIPIDisplayOn (LP and HS)

What does (command mode only) mean in the video mode sequence?  My
assumption is that TearOn is only used in command mode, and as such,
shouldn't be done in video mode.  Is that wrong?

> + * - MIPIBacklightOn

What's the difference between MIPIBacklightOn here and "backlight on"
in the v2 sequence? 

It might be cleaner to make this a table with V2 and V3 as columns,
that would also help to highlight the differences in the two versions.
Maybe even put the command mode sequence in a third column if it
doesn't make the lines too long.

> + * ...
> + * - MIPIBacklightOff
> + * - turn off DPI
> + * - MIPITearOff + MIPIDisplayOff (LP)
> + * - clk/data lines to lp-00
> + * - MIPIAssertResetPin
> + * - MIPIPanelPowerOff
> + *
> + * sequence for command mode:

Is this the sequence for both V2 and V3 or only V3?  

> + * - power on
> + * - wait t1+t2
> + * - MIPIDeassertResetPin
> + * - clk/data lines to lp-11
> + * - MIPISendInitialDcsCmds
> + * - MIPITearOn
> + * - MIPIDisplayOn
> + * - set pipe to dsr mode
> + * - wait t5
> + * - backlight on
> + * ... issue write_mem_start/write_mem_continue commands ...
> + * - backlight off
> + * - wait t6
> + * - disable pipe dsr mode
> + * - MIPITearOff
> + * - MIPIDisplayOff
> + * - clk/data lines to lp-00
> + * - MIPIAssertResetPin
> + * - wait t3
> + * - power off
> + * - wait t4
> + */
> +
>  static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  				 struct intel_crtc_state *pipe_config,
>  				 struct drm_connector_state *conn_state)



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 06/15] drm/i915/dsi: Make intel_dsi_enable/disable directly exec VBT sequences

[Bob Paauwe]

On Mon, 20 Feb 2017 15:08:36 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> The drm_panel_enable/disable and drm_panel_prepare/unprepare calls are
> not fine grained enough to abstract all the different steps we need to
> take (and VBT sequences we need to exec) properly. So simply remove the
> panel _enable/disable and prepare/unprepare callbacks and instead
> export intel_dsi_exec_vbt_sequence() from intel_dsi_panel_vbt.c
> and call that from intel_dsi_enable/disable().
> 
> No functional changes.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dsi.c           | 14 +++++++---
>  drivers/gpu/drm/i915/intel_dsi.h           |  3 +++
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 43 ++----------------------------
>  3 files changed, 15 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 8808f87..78d5884 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -669,7 +669,10 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  	/* put device in ready state */
>  	intel_dsi_device_ready(encoder);
>  
> -	drm_panel_prepare(intel_dsi->panel);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
>  
>  	/* Enable port in pre-enable phase itself because as per hw team
>  	 * recommendation, port should be enabled befor plane & pipe */
> @@ -682,7 +685,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
>  		msleep(100);
>  
> -		drm_panel_enable(intel_dsi->panel);
> +		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
> +		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
>  
>  		intel_dsi_port_enable(encoder);
>  	}
> @@ -757,7 +761,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  	 * if disable packets are sent before sending shutdown packet then in
>  	 * some next enable sequence send turn on packet error is observed
>  	 */
> -	drm_panel_disable(intel_dsi->panel);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>  
>  	intel_dsi_clear_device_ready(encoder);
>  
> @@ -782,7 +787,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  		I915_WRITE(DSPCLK_GATE_D, val);
>  	}
>  
> -	drm_panel_unprepare(intel_dsi->panel);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
>  
>  	msleep(intel_dsi->panel_off_delay);
>  
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index d567823..5486491 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -132,6 +132,9 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
>  
>  void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port);
>  
> +void intel_dsi_exec_vbt_sequence(struct intel_dsi *intel_dsi,
> +				 enum mipi_seq seq_id);
> +
>  bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv);
>  int intel_compute_dsi_pll(struct intel_encoder *encoder,
>  			  struct intel_crtc_state *config);
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 995f72d..0ce1086 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -426,10 +426,9 @@ static const char *sequence_name(enum mipi_seq seq_id)
>  		return "(unknown)";
>  }
>  
> -static void generic_exec_sequence(struct drm_panel *panel, enum mipi_seq seq_id)
> +void intel_dsi_exec_vbt_sequence(struct intel_dsi *intel_dsi,
> +				 enum mipi_seq seq_id)
>  {
> -	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> -	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
>  	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
>  	const u8 *data;
>  	fn_mipi_elem_exec mipi_elem_exec;
> @@ -493,40 +492,6 @@ static void generic_exec_sequence(struct drm_panel *panel, enum mipi_seq seq_id)
>  	}
>  }
>  
> -static int vbt_panel_prepare(struct drm_panel *panel)
> -{
> -	generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
> -	generic_exec_sequence(panel, MIPI_SEQ_POWER_ON);
> -	generic_exec_sequence(panel, MIPI_SEQ_DEASSERT_RESET);
> -	generic_exec_sequence(panel, MIPI_SEQ_INIT_OTP);
> -
> -	return 0;
> -}
> -
> -static int vbt_panel_unprepare(struct drm_panel *panel)
> -{
> -	generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
> -	generic_exec_sequence(panel, MIPI_SEQ_POWER_OFF);
> -
> -	return 0;
> -}
> -
> -static int vbt_panel_enable(struct drm_panel *panel)
> -{
> -	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_ON);
> -	generic_exec_sequence(panel, MIPI_SEQ_BACKLIGHT_ON);
> -
> -	return 0;
> -}
> -
> -static int vbt_panel_disable(struct drm_panel *panel)
> -{
> -	generic_exec_sequence(panel, MIPI_SEQ_BACKLIGHT_OFF);
> -	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_OFF);
> -
> -	return 0;
> -}
> -
>  static int vbt_panel_get_modes(struct drm_panel *panel)
>  {
>  	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> @@ -550,10 +515,6 @@ static int vbt_panel_get_modes(struct drm_panel *panel)
>  }
>  
>  static const struct drm_panel_funcs vbt_panel_funcs = {
> -	.disable = vbt_panel_disable,
> -	.unprepare = vbt_panel_unprepare,
> -	.prepare = vbt_panel_prepare,
> -	.enable = vbt_panel_enable,
>  	.get_modes = vbt_panel_get_modes,
>  };
>  



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 07/15] drm/i915/dsi: Drop bogus MIPI_SEQ_ASSERT_RESET before POWER_ON

[Bob Paauwe]

On Mon, 20 Feb 2017 15:08:37 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> MIPI_SEQ_ASSERT_RESET before POWER_ON is not necessary for 2 reasons:
> 1) The reset should already be asserted before intel_dsi_pre_enable()
>    gets called
> 2) Most (some?) VBTs will ensure reset was asserted in their
>    MIPI_SEQ_DEASSERT_RESET themselves
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Using words like "should" and some? don't inspire a lot of confidence
that this is the right thing to do. Is there some way to verify that
these two conditions are true?

> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 78d5884..4ebf308 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -669,7 +669,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  	/* put device in ready state */
>  	intel_dsi_device_ready(encoder);
>  
> -	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 08/15] drm/i915/dsi: Move MIPI_SEQ_POWER_ON/OFF calls together with pmic gpio calls

[Bob Paauwe]

On Mon, 20 Feb 2017 15:08:38 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Now that we are no longer bound to the drm_panel_ callbacks, call
> MIPI_SEQ_POWER_ON/OFF at the proper place.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 4ebf308..de0558b 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -651,10 +651,10 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  
>  	intel_dsi_prepare(encoder, pipe_config);
>  
> -	/* Panel Enable over CRC PMIC */
> +	/* Power on, try both CRC pmic gpio and VBT */
>  	if (intel_dsi->gpio_panel)
>  		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
> -
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
>  	msleep(intel_dsi->panel_on_delay);
>  
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> @@ -669,7 +669,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  	/* put device in ready state */
>  	intel_dsi_device_ready(encoder);
>  
> -	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
>  
> @@ -787,11 +786,10 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  	}
>  
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
> -	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
>  
> +	/* Power off, try both CRC pmic gpio and VBT */
>  	msleep(intel_dsi->panel_off_delay);
> -
> -	/* Panel Disable over CRC PMIC */
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
>  	if (intel_dsi->gpio_panel)
>  		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 0);
>  



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 09/15] drm/i915/dsi: Group DPOunit clock gate workaround with PLL enable

[Bob Paauwe]

On Mon, 20 Feb 2017 15:08:39 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Move the DPOunit clock gate workaround to directly after the PLL enable.
> 
> The exact location of the workaround does not matter and there are 2
> reasons to group it with the PLL enable:
> 
> 1) This moves it out of the middle of the init sequence from the spec,
>    making it easier to follow the init sequence / compare it to the spec
> 
> 2) It is grouped with the pll disable call in intel_dsi_post_disable,
>    so for consistency it should be grouped with the pll enable in
>    intel_dsi_pre_enable
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Makes sense and looks better too.
Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index de0558b..f4539f1 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -649,14 +649,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  		I915_WRITE(BXT_P_DSI_REGULATOR_TX_CTRL, 0);
>  	}
>  
> -	intel_dsi_prepare(encoder, pipe_config);
> -
> -	/* Power on, try both CRC pmic gpio and VBT */
> -	if (intel_dsi->gpio_panel)
> -		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
> -	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
> -	msleep(intel_dsi->panel_on_delay);
> -
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>  		u32 val;
>  
> @@ -666,6 +658,14 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  		I915_WRITE(DSPCLK_GATE_D, val);
>  	}
>  
> +	intel_dsi_prepare(encoder, pipe_config);
> +
> +	/* Power on, try both CRC pmic gpio and VBT */
> +	if (intel_dsi->gpio_panel)
> +		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
> +	msleep(intel_dsi->panel_on_delay);
> +
>  	/* put device in ready state */
>  	intel_dsi_device_ready(encoder);
>  



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 10/15] drm/i915/dsi: Execute MIPI_SEQ_DEASSERT_RESET before calling device_ready()

[Bob Paauwe]

On Mon, 20 Feb 2017 15:08:40 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Execute MIPI_SEQ_DEASSERT_RESET before putting the device in ready
> state (LP-11), this is the sequence in which things should be done
> according to the spec.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index f4539f1..8408f59 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -666,10 +666,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
>  	msleep(intel_dsi->panel_on_delay);
>  
> -	/* put device in ready state */
> +	/* Deassert reset */

I don't think the comment is really necessary. 

> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
> +
> +	/* Put device in ready state (LP-11) */
>  	intel_dsi_device_ready(encoder);
>  
> -	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
> +	/* Send initialization commands in LP mode */
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
>  
>  	/* Enable port in pre-enable phase itself because as per hw team
> @@ -762,6 +765,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>  
> +	/* Transition to LP-00 */
>  	intel_dsi_clear_device_ready(encoder);
>  
>  	if (IS_BROXTON(dev_priv)) {
> @@ -785,6 +789,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  		I915_WRITE(DSPCLK_GATE_D, val);
>  	}
>  
> +	/* Assert reset */

Again, the comment doesn't provide any additional info.

But the rest looks good.
Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
>  
>  	/* Power off, try both CRC pmic gpio and VBT */



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 11/15] drm/i915/dsi: Group MIPI_SEQ_BACKLIGHT_ON/OFF with panel_[en|dis]able_backlight

[Bob Paauwe]

On Mon, 20 Feb 2017 15:08:41 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Execute the MIPI_SEQ_BACKLIGHT_ON/OFF VBT sequences at the same time as
> we call intel_panel_enable_backlight() / intel_panel_disable_backlight().
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I'm not sure that the added comments are necessary.  Maybe if there was
some explanation for why we're using two different mechanisms to
enable/disable backlight instead.

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 8408f59..a8d0948 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -687,12 +687,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  		msleep(100);
>  
>  		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
> -		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
>  
>  		intel_dsi_port_enable(encoder);
>  	}
>  
> +	/* Enable backlight, both pwm and VBT */
>  	intel_panel_enable_backlight(intel_dsi->attached_connector);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
>  }
>  
>  static void intel_dsi_enable_nop(struct intel_encoder *encoder,
> @@ -718,6 +719,8 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
>  
>  	DRM_DEBUG_KMS("\n");
>  
> +	/* Disable backlight, both VBT and pwm */
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
>  	intel_panel_disable_backlight(intel_dsi->attached_connector);
>  
>  	/*
> @@ -762,7 +765,6 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  	 * if disable packets are sent before sending shutdown packet then in
>  	 * some next enable sequence send turn on packet error is observed
>  	 */
> -	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>  
>  	/* Transition to LP-00 */



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 12/15] drm/i915/dsi: Document always using v3 SHUTDOWN / MIPI_SEQ_DISPLAY_OFF order

[Bob Paauwe]

On Mon, 20 Feb 2017 15:08:42 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> According to the spec for v2 VBTs we should call MIPI_SEQ_DISPLAY_OFF
> before sending SHUTDOWN, where as for v3 VBTs we should send SHUTDOWN
> first.
> 
> Since the v2 order has known issues, we use the v3 order everywhere,
> add a comment documenting this.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index a8d0948..1914311 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -732,6 +732,11 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
>  			I915_WRITE(MIPI_DEVICE_READY(port), 0);
>  	}
>  
> +	/*
> +	 * XXX: According to the spec we should send SHUTDOWN before
> +	 * MIPI_SEQ_DISPLAY_OFF only for v3+ VBTs, but testing in the field
> +	 * has shown that we should do this for v2 VBTs too?
drop the '?'
> +	 */
>  	if (is_vid_mode(intel_dsi)) {
>  		/* Send Shutdown command to the panel in LP mode */
>  		for_each_dsi_port(port, intel_dsi->ports)
> @@ -764,6 +769,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  	/*
>  	 * if disable packets are sent before sending shutdown packet then in
>  	 * some next enable sequence send turn on packet error is observed
> +	 * XXX spec specifies SHUTDOWN before MIPI_SEQ_DISPLAY_OFF for
> +	 * v3 VBTs, but not for v2 VBTs?
>  	 */
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>  

Should XXX be replaced with something? 



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 13/15] drm/i915/dsi: Execute MIPI_SEQ_TEAR_OFF from intel_dsi_post_disable

[Bob Paauwe]

On Mon, 20 Feb 2017 15:08:43 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> For v3 VBTs we should call MIPI_SEQ_TEAR_OFF before
> MIPI_SEQ_DISPLAY_OFF, for non v3 VBTs this is a nop.

Isn't this only for command mode?  Or is there conflicting information
on this?

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 1914311..90263d6 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -772,6 +772,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  	 * XXX spec specifies SHUTDOWN before MIPI_SEQ_DISPLAY_OFF for
>  	 * v3 VBTs, but not for v2 VBTs?
>  	 */
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_TEAR_OFF);
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>  
>  	/* Transition to LP-00 */



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 14/15] drm/i915/dsi: Call MIPI_SEQ_TEAR_ON and DISPLAY_ON for cmd-mode (untested)

[Bob Paauwe]

On Mon, 20 Feb 2017 15:08:44 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> According to the spec we should call MIPI_SEQ_TEAR_ON and DISPLAY_ON
> on enable for cmd-mode, just like we already call their counterparts
> on disable. Note: untested, my panel is a vid-mode panel.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 90263d6..a001e43 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -680,6 +680,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  	if (is_cmd_mode(intel_dsi)) {
>  		for_each_dsi_port(port, intel_dsi->ports)
>  			I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(port), 8 * 4);
> +		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_TEAR_ON);

As with the TEAR_OFF, should this only be done for command mode?  Or is
it just a no-op for video mode and doesn't matter?

> +		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
>  	} else {
>  		msleep(20); /* XXX */
>  		for_each_dsi_port(port, intel_dsi->ports)



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 15/15] drm/i915/dsi: Skip delays for v3 VBTs in vid-mode

[Bob Paauwe]

On Mon, 20 Feb 2017 15:08:45 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> For v3 VBTs in vid-mode the delays are part of the VBT sequences, so
> we should not also delay ourselves otherwise we get double delays.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index a001e43..9e858c7 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -552,6 +552,17 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
>  			      struct intel_crtc_state *pipe_config);
>  static void intel_dsi_unprepare(struct intel_encoder *encoder);
>  
> +static void intel_dsi_msleep(struct intel_dsi *intel_dsi, int msec)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
> +
> +	/* For v3 VBTs in vid-mode the delays are part of the VBT sequences */
> +	if (is_vid_mode(intel_dsi) && dev_priv->vbt.dsi.seq_version >= 3)
> +		return;
> +
> +	msleep(msec);
> +}
> +
>  /*
>   * Panel enable/disable sequences from the VBT spec.
>   *
> @@ -664,7 +675,7 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  	if (intel_dsi->gpio_panel)
>  		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
> -	msleep(intel_dsi->panel_on_delay);
> +	intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay);
>  
>  	/* Deassert reset */
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
> @@ -686,7 +697,7 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  		msleep(20); /* XXX */
>  		for_each_dsi_port(port, intel_dsi->ports)
>  			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
> -		msleep(100);
> +		intel_dsi_msleep(intel_dsi, 100);
>  
>  		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
>  
> @@ -805,7 +816,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
>  
>  	/* Power off, try both CRC pmic gpio and VBT */
> -	msleep(intel_dsi->panel_off_delay);
> +	intel_dsi_msleep(intel_dsi, intel_dsi->panel_off_delay);
>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
>  	if (intel_dsi->gpio_panel)
>  		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 0);
> @@ -814,7 +825,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  	 * FIXME As we do with eDP, just make a note of the time here
>  	 * and perform the wait before the next panel power on.
>  	 */

Should this comment be updated now?

Otherwise
Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> -	msleep(intel_dsi->panel_pwr_cycle_delay);
> +	intel_dsi_msleep(intel_dsi, intel_dsi->panel_pwr_cycle_delay);
>  }
>  
>  static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 05/15] drm/i915/dsi: Document the panel enable / disable sequences from the spec

[Hans de Goede]

Hi Bob,

Thank you for the review of this patch-set.

On 24-02-17 18:00, Bob Paauwe wrote:
> On Mon, 20 Feb 2017 15:08:35 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> Document the DSI panel enable / disable sequences from the spec,
>> for easy comparison between the code and the spec.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi.c | 68 ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 68 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 072f99b..8808f87 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -552,6 +552,74 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
>>  			      struct intel_crtc_state *pipe_config);
>>  static void intel_dsi_unprepare(struct intel_encoder *encoder);
>>
>> +/*
>> + * Panel enable/disable sequences from the VBT spec.
>> + *
>> + * Note the spec has AssertReset / DeassertReset swapped from their
>> + * usual naming. We use the normal names to avoid confusion (so below
>> + * they are swapped compared to the spec).
>> + *
>> + * v2 sequence for video mode:
>> + * - power on
>> + * - wait t1+t2
>> + * - MIPIDeassertResetPin
>> + * - clk/data lines to lp-11
>> + * - MIPISendInitialDcsCmds
>> + * - turn on DPI
>> + * - MIPIDisplayOn
>> + * - wait t5
>> + * - backlight on
>> + * ...
>> + * - backlight off
>> + * - wait t6
>> + * - MIPIDisplayOff
>> + * - turn off DPI
>> + * - clk/data lines to lp-00
>> + * - MIPIAssertResetPin
>> + * - wait t3
>> + * - power off
>> + * - wait t4
>> + *
>> + * v3 sequence for video mode:
>> + * - MIPIPanelPowerOn
>> + * - MIPIDeassertResetPin
>> + * - set clk/data lines to lp-11
>> + * - MIPISendInitialDcsCmds (LP)
>> + * - turn on DPI
>> + * - MIPITearOn (command mode only) + MIPIDisplayOn (LP and HS)
>
> What does (command mode only) mean in the video mode sequence?  My
> assumption is that TearOn is only used in command mode, and as such,
> shouldn't be done in video mode.  Is that wrong?

I don't know I don't have access to the VBT spec I merely copied
the contents of the comment this is adding from a mail which contained
the sequences extracted from the doc by someone at Intel (Jani IIRC).

>
>> + * - MIPIBacklightOn
>
> What's the difference between MIPIBacklightOn here and "backlight on"
> in the v2 sequence?

AFAICT there is none, again copy pasted from the mail as is.

> It might be cleaner to make this a table with V2 and V3 as columns,
> that would also help to highlight the differences in the two versions.
> Maybe even put the command mode sequence in a third column if it
> doesn't make the lines too long.

I can try once we've cleared up the other bits.

>> + * ...
>> + * - MIPIBacklightOff
>> + * - turn off DPI
>> + * - MIPITearOff + MIPIDisplayOff (LP)
>> + * - clk/data lines to lp-00
>> + * - MIPIAssertResetPin
>> + * - MIPIPanelPowerOff
>> + *
>> + * sequence for command mode:
>
> Is this the sequence for both V2 and V3 or only V3?

I think command mode is V3 only, but I'm not sure. I only have
access to video mode devices myself.

>> + * - power on
>> + * - wait t1+t2
>> + * - MIPIDeassertResetPin
>> + * - clk/data lines to lp-11
>> + * - MIPISendInitialDcsCmds
>> + * - MIPITearOn
>> + * - MIPIDisplayOn
>> + * - set pipe to dsr mode
>> + * - wait t5
>> + * - backlight on
>> + * ... issue write_mem_start/write_mem_continue commands ...
>> + * - backlight off
>> + * - wait t6
>> + * - disable pipe dsr mode
>> + * - MIPITearOff
>> + * - MIPIDisplayOff
>> + * - clk/data lines to lp-00
>> + * - MIPIAssertResetPin
>> + * - wait t3
>> + * - power off
>> + * - wait t4
>> + */
>> +
>>  static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>>  				 struct intel_crtc_state *pipe_config,
>>  				 struct drm_connector_state *conn_state)
>
>
>

Regards,

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

Re: [PATCH resend 07/15] drm/i915/dsi: Drop bogus MIPI_SEQ_ASSERT_RESET before POWER_ON

[Hans de Goede]

Hi,

On 24-02-17 18:00, Bob Paauwe wrote:
> On Mon, 20 Feb 2017 15:08:37 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> MIPI_SEQ_ASSERT_RESET before POWER_ON is not necessary for 2 reasons:
>> 1) The reset should already be asserted before intel_dsi_pre_enable()
>>    gets called
>> 2) Most (some?) VBTs will ensure reset was asserted in their
>>    MIPI_SEQ_DEASSERT_RESET themselves
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Using words like "should" and some? don't inspire a lot of confidence
> that this is the right thing to do. Is there some way to verify that
> these two conditions are true?

Well the i915 code should never call intel_dsi_[pre_]enable without
first having called intel_dsi_post_disable() which does:

intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);

Already, also this is undoing a recent (4.9 kernel IIRC) change,
before that the i915 did not call the MIPI_SEQ_ASSERT_RESET
sequence from the enable path. Also doing this is against the VBT
spec.

Regards,

Hans



>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 78d5884..4ebf308 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -669,7 +669,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>>  	/* put device in ready state */
>>  	intel_dsi_device_ready(encoder);
>>
>> -	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
>>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
>>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
>>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH resend 11/15] drm/i915/dsi: Group MIPI_SEQ_BACKLIGHT_ON/OFF with panel_[en|dis]able_backlight

[Hans de Goede]

Hi,

On 24-02-17 18:00, Bob Paauwe wrote:
> On Mon, 20 Feb 2017 15:08:41 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> Execute the MIPI_SEQ_BACKLIGHT_ON/OFF VBT sequences at the same time as
>> we call intel_panel_enable_backlight() / intel_panel_disable_backlight().
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> I'm not sure that the added comments are necessary.  Maybe if there was
> some explanation for why we're using two different mechanisms to
> enable/disable backlight instead.

That assumes I know why there are 2 mechanisms of I have to guess it
is because on some boards only one of the mechanisms works, but that
is just a guess. Just calling the VBT sequence should be enough on
(most?) boards.

Regards,

Hans


>
> Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>
>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 8408f59..a8d0948 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -687,12 +687,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>>  		msleep(100);
>>
>>  		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
>> -		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
>>
>>  		intel_dsi_port_enable(encoder);
>>  	}
>>
>> +	/* Enable backlight, both pwm and VBT */
>>  	intel_panel_enable_backlight(intel_dsi->attached_connector);
>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
>>  }
>>
>>  static void intel_dsi_enable_nop(struct intel_encoder *encoder,
>> @@ -718,6 +719,8 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
>>
>>  	DRM_DEBUG_KMS("\n");
>>
>> +	/* Disable backlight, both VBT and pwm */
>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
>>  	intel_panel_disable_backlight(intel_dsi->attached_connector);
>>
>>  	/*
>> @@ -762,7 +765,6 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>>  	 * if disable packets are sent before sending shutdown packet then in
>>  	 * some next enable sequence send turn on packet error is observed
>>  	 */
>> -	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
>>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>>
>>  	/* Transition to LP-00 */
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH resend 12/15] drm/i915/dsi: Document always using v3 SHUTDOWN / MIPI_SEQ_DISPLAY_OFF order

[Hans de Goede]

Hi,

On 24-02-17 18:02, Bob Paauwe wrote:
> On Mon, 20 Feb 2017 15:08:42 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> According to the spec for v2 VBTs we should call MIPI_SEQ_DISPLAY_OFF
>> before sending SHUTDOWN, where as for v3 VBTs we should send SHUTDOWN
>> first.
>>
>> Since the v2 order has known issues, we use the v3 order everywhere,
>> add a comment documenting this.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index a8d0948..1914311 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -732,6 +732,11 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
>>  			I915_WRITE(MIPI_DEVICE_READY(port), 0);
>>  	}
>>
>> +	/*
>> +	 * XXX: According to the spec we should send SHUTDOWN before
>> +	 * MIPI_SEQ_DISPLAY_OFF only for v3+ VBTs, but testing in the field
>> +	 * has shown that we should do this for v2 VBTs too?
> drop the '?'

I added that because I'm not 100% sure this is true, looking through git
history (and android x86 kernel patch-sets) I managed to piece together that
at one point in time the v2 sequence was used, but that yielded problems
during some testing, what the commits do not tell if is that testing was
using boards with v3 VBTs, but assuming v2 tables are out there in the
wild then it seems that the v3 order works fine for v2 too.

TLDR I'm not 100% sure about this hence the '?', my main goal with this
patch is to document that we're deviating from the spec for v2 tables here.

>> +	 */
>>  	if (is_vid_mode(intel_dsi)) {
>>  		/* Send Shutdown command to the panel in LP mode */
>>  		for_each_dsi_port(port, intel_dsi->ports)
>> @@ -764,6 +769,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>>  	/*
>>  	 * if disable packets are sent before sending shutdown packet then in
>>  	 * some next enable sequence send turn on packet error is observed
>> +	 * XXX spec specifies SHUTDOWN before MIPI_SEQ_DISPLAY_OFF for
>> +	 * v3 VBTs, but not for v2 VBTs?
>>  	 */
>>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>>
>
> Should XXX be replaced with something?

XXX is used in many places in intel_dsi.c to indicate code which may need
work / which may needs to be investigated further. I followed that and
added XXX here since this code is deviating from the spec.

Regards,

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

Re: [PATCH resend 13/15] drm/i915/dsi: Execute MIPI_SEQ_TEAR_OFF from intel_dsi_post_disable

[Hans de Goede]

Hi,

On 24-02-17 18:02, Bob Paauwe wrote:
> On Mon, 20 Feb 2017 15:08:43 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> For v3 VBTs we should call MIPI_SEQ_TEAR_OFF before
>> MIPI_SEQ_DISPLAY_OFF, for non v3 VBTs this is a nop.
>
> Isn't this only for command mode?  Or is there conflicting information
> on this?

In my (limited) set of test hardware vidmode panel VBTs simply
do not define a MIPI_SEQ_TEAR_ON/OFF sequence. But it would probably
be a good idea to guard this with a "if (is_cmd_mode(intel_dsi))"
assuming we only want this for commandmode, which I think we do,
but it would be good to have this confirmed.

I'll add the guard when I send out a v2 after we've clarified
some of the other review questions.

Regards,

Hans



>
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 1914311..90263d6 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -772,6 +772,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>>  	 * XXX spec specifies SHUTDOWN before MIPI_SEQ_DISPLAY_OFF for
>>  	 * v3 VBTs, but not for v2 VBTs?
>>  	 */
>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_TEAR_OFF);
>>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>>
>>  	/* Transition to LP-00 */
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH resend 14/15] drm/i915/dsi: Call MIPI_SEQ_TEAR_ON and DISPLAY_ON for cmd-mode (untested)

[Hans de Goede]

Hi,

On 24-02-17 18:02, Bob Paauwe wrote:
> On Mon, 20 Feb 2017 15:08:44 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> According to the spec we should call MIPI_SEQ_TEAR_ON and DISPLAY_ON
>> on enable for cmd-mode, just like we already call their counterparts
>> on disable. Note: untested, my panel is a vid-mode panel.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 90263d6..a001e43 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -680,6 +680,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>>  	if (is_cmd_mode(intel_dsi)) {
>>  		for_each_dsi_port(port, intel_dsi->ports)
>>  			I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(port), 8 * 4);
>> +		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_TEAR_ON);
>
> As with the TEAR_OFF, should this only be done for command mode?  Or is
> it just a no-op for video mode and doesn't matter?

In this case we are actually in a "if (is_cmd_mode(intel_dsi)) {" code
block (the if is visible in the diff context).

Which I guess also shows that we really need to add the guard to the
other path, so as to be consistent.

Note as mentioned in the commit msg:

untested, my panel is a vid-mode panel.

>
>> +		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
>>  	} else {
>>  		msleep(20); /* XXX */
>>  		for_each_dsi_port(port, intel_dsi->ports)
>
>
>

Regards,

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

Re: [PATCH resend 15/15] drm/i915/dsi: Skip delays for v3 VBTs in vid-mode

[Hans de Goede]

HI,

On 24-02-17 18:02, Bob Paauwe wrote:
> On Mon, 20 Feb 2017 15:08:45 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> For v3 VBTs in vid-mode the delays are part of the VBT sequences, so
>> we should not also delay ourselves otherwise we get double delays.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi.c | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index a001e43..9e858c7 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -552,6 +552,17 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
>>  			      struct intel_crtc_state *pipe_config);
>>  static void intel_dsi_unprepare(struct intel_encoder *encoder);
>>
>> +static void intel_dsi_msleep(struct intel_dsi *intel_dsi, int msec)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
>> +
>> +	/* For v3 VBTs in vid-mode the delays are part of the VBT sequences */
>> +	if (is_vid_mode(intel_dsi) && dev_priv->vbt.dsi.seq_version >= 3)
>> +		return;
>> +
>> +	msleep(msec);
>> +}
>> +
>>  /*
>>   * Panel enable/disable sequences from the VBT spec.
>>   *
>> @@ -664,7 +675,7 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>>  	if (intel_dsi->gpio_panel)
>>  		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
>>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
>> -	msleep(intel_dsi->panel_on_delay);
>> +	intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay);
>>
>>  	/* Deassert reset */
>>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
>> @@ -686,7 +697,7 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>>  		msleep(20); /* XXX */
>>  		for_each_dsi_port(port, intel_dsi->ports)
>>  			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
>> -		msleep(100);
>> +		intel_dsi_msleep(intel_dsi, 100);
>>
>>  		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
>>
>> @@ -805,7 +816,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
>>
>>  	/* Power off, try both CRC pmic gpio and VBT */
>> -	msleep(intel_dsi->panel_off_delay);
>> +	intel_dsi_msleep(intel_dsi, intel_dsi->panel_off_delay);
>>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
>>  	if (intel_dsi->gpio_panel)
>>  		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 0);
>> @@ -814,7 +825,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>>  	 * FIXME As we do with eDP, just make a note of the time here
>>  	 * and perform the wait before the next panel power on.
>>  	 */
>
> Should this comment be updated now?

It is still valid for non-vidmode and v2 VBT panels, so I think it can
be left as is.

Regards,

Hans




>
> Otherwise
> Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>
>
>> -	msleep(intel_dsi->panel_pwr_cycle_delay);
>> +	intel_dsi_msleep(intel_dsi, intel_dsi->panel_pwr_cycle_delay);
>>  }
>>
>>  static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH resend 07/15] drm/i915/dsi: Drop bogus MIPI_SEQ_ASSERT_RESET before POWER_ON

[Bob Paauwe]

On Sat, 25 Feb 2017 11:35:03 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 24-02-17 18:00, Bob Paauwe wrote:
> > On Mon, 20 Feb 2017 15:08:37 +0100
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >  
> >> MIPI_SEQ_ASSERT_RESET before POWER_ON is not necessary for 2 reasons:
> >> 1) The reset should already be asserted before intel_dsi_pre_enable()
> >>    gets called
> >> 2) Most (some?) VBTs will ensure reset was asserted in their
> >>    MIPI_SEQ_DEASSERT_RESET themselves
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>  
> >
> > Using words like "should" and some? don't inspire a lot of confidence
> > that this is the right thing to do. Is there some way to verify that
> > these two conditions are true?  
> 
> Well the i915 code should never call intel_dsi_[pre_]enable without
> first having called intel_dsi_post_disable() which does:
> 
> intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
> 
> Already, also this is undoing a recent (4.9 kernel IIRC) change,
> before that the i915 did not call the MIPI_SEQ_ASSERT_RESET
> sequence from the enable path. Also doing this is against the VBT
> spec.
> 
> Regards,
> 
> Hans
> 

So let's try to make the commit message state that.  Using what you
just wrote maybe something like:

intel_dsi_post_disable(), which does the MIPI_SEQ_ASSERT_RESET,
will always be called at some point before intel_dsi_pre_enable()
making the MIPI_SEQ_ASSERT_RESET in intel_dsi_pre_enable() redundent. 

In addition, calling MIPI_SEQ_ASSERT_RESET in the enable path goes
against the VBT spec.

With the commit message updated the change looks to be correct.


> 
> 
> >  
> >> ---
> >>  drivers/gpu/drm/i915/intel_dsi.c | 1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> >> index 78d5884..4ebf308 100644
> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> >> @@ -669,7 +669,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
> >>  	/* put device in ready state */
> >>  	intel_dsi_device_ready(encoder);
> >>
> >> -	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
> >>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
> >>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
> >>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);  
> >
> >
> >  



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 11/15] drm/i915/dsi: Group MIPI_SEQ_BACKLIGHT_ON/OFF with panel_[en|dis]able_backlight

[Bob Paauwe]

On Sat, 25 Feb 2017 11:37:50 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 24-02-17 18:00, Bob Paauwe wrote:
> > On Mon, 20 Feb 2017 15:08:41 +0100
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >  
> >> Execute the MIPI_SEQ_BACKLIGHT_ON/OFF VBT sequences at the same time as
> >> we call intel_panel_enable_backlight() / intel_panel_disable_backlight().
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>  
> >
> > I'm not sure that the added comments are necessary.  Maybe if there was
> > some explanation for why we're using two different mechanisms to
> > enable/disable backlight instead.  
> 
> That assumes I know why there are 2 mechanisms of I have to guess it
> is because on some boards only one of the mechanisms works, but that
> is just a guess. Just calling the VBT sequence should be enough on
> (most?) boards.
> 
> Regards,
> 
> Hans

I don't think I can describe why we have two mechanisms so just
dropping the comment would be my preference at this point.

It may be that calling the intel_panel_enable_backlight() will make
sure the sysfs entries are correct while the VBT sequence simply
modifies the hardware. 

Possibly a good solution would be to enhance the code that executes the
VBT sequence to also call the intel_panel_enable_backlight(), but
that's not something to change here.

> 
> 
> >
> > Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>
> >  
> >> ---
> >>  drivers/gpu/drm/i915/intel_dsi.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> >> index 8408f59..a8d0948 100644
> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> >> @@ -687,12 +687,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
> >>  		msleep(100);
> >>
> >>  		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
> >> -		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
> >>
> >>  		intel_dsi_port_enable(encoder);
> >>  	}
> >>
> >> +	/* Enable backlight, both pwm and VBT */
> >>  	intel_panel_enable_backlight() (intel_dsi->attached_connector);
> >> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
> >>  }
> >>
> >>  static void intel_dsi_enable_nop(struct intel_encoder *encoder,
> >> @@ -718,6 +719,8 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
> >>
> >>  	DRM_DEBUG_KMS("\n");
> >>
> >> +	/* Disable backlight, both VBT and pwm */
> >> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
> >>  	intel_panel_disable_backlight(intel_dsi->attached_connector);
> >>
> >>  	/*
> >> @@ -762,7 +765,6 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
> >>  	 * if disable packets are sent before sending shutdown packet then in
> >>  	 * some next enable sequence send turn on packet error is observed
> >>  	 */
> >> -	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
> >>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
> >>
> >>  	/* Transition to LP-00 */  
> >
> >
> >  



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 12/15] drm/i915/dsi: Document always using v3 SHUTDOWN / MIPI_SEQ_DISPLAY_OFF order

[Bob Paauwe]

On Sat, 25 Feb 2017 11:42:09 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 24-02-17 18:02, Bob Paauwe wrote:
> > On Mon, 20 Feb 2017 15:08:42 +0100
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >  
> >> According to the spec for v2 VBTs we should call MIPI_SEQ_DISPLAY_OFF
> >> before sending SHUTDOWN, where as for v3 VBTs we should send SHUTDOWN
> >> first.
> >>
> >> Since the v2 order has known issues, we use the v3 order everywhere,
> >> add a comment documenting this.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dsi.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> >> index a8d0948..1914311 100644
> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> >> @@ -732,6 +732,11 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
> >>  			I915_WRITE(MIPI_DEVICE_READY(port), 0);
> >>  	}
> >>
> >> +	/*
> >> +	 * XXX: According to the spec we should send SHUTDOWN before
> >> +	 * MIPI_SEQ_DISPLAY_OFF only for v3+ VBTs, but testing in the field
> >> +	 * has shown that we should do this for v2 VBTs too?  
> > drop the '?'  
> 
> I added that because I'm not 100% sure this is true, looking through git
> history (and android x86 kernel patch-sets) I managed to piece together that
> at one point in time the v2 sequence was used, but that yielded problems
> during some testing, what the commits do not tell if is that testing was
> using boards with v3 VBTs, but assuming v2 tables are out there in the
> wild then it seems that the v3 order works fine for v2 too.
> 
> TLDR I'm not 100% sure about this hence the '?', my main goal with this
> patch is to document that we're deviating from the spec for v2 tables here.

If anyone else, Jani?, has more information about this, that would be
good to know.  

I'd be OK with just stating that "field testing has shown that the v3
sequence works with v2 VBT's so just use that."

> 
> >> +	 */
> >>  	if (is_vid_mode(intel_dsi)) {
> >>  		/* Send Shutdown command to the panel in LP mode */
> >>  		for_each_dsi_port(port, intel_dsi->ports)
> >> @@ -764,6 +769,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
> >>  	/*
> >>  	 * if disable packets are sent before sending shutdown packet then in
> >>  	 * some next enable sequence send turn on packet error is observed
> >> +	 * XXX spec specifies SHUTDOWN before MIPI_SEQ_DISPLAY_OFF for
> >> +	 * v3 VBTs, but not for v2 VBTs?
> >>  	 */
> >>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
> >>  
> >
> > Should XXX be replaced with something?  
> 
> XXX is used in many places in intel_dsi.c to indicate code which may need
> work / which may needs to be investigated further. I followed that and
> added XXX here since this code is deviating from the spec.
> 
> Regards,
> 
> Hans



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 14/15] drm/i915/dsi: Call MIPI_SEQ_TEAR_ON and DISPLAY_ON for cmd-mode (untested)

[Bob Paauwe]

On Sat, 25 Feb 2017 11:47:32 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 24-02-17 18:02, Bob Paauwe wrote:
> > On Mon, 20 Feb 2017 15:08:44 +0100
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >  
> >> According to the spec we should call MIPI_SEQ_TEAR_ON and DISPLAY_ON
> >> on enable for cmd-mode, just like we already call their counterparts
> >> on disable. Note: untested, my panel is a vid-mode panel.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dsi.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> >> index 90263d6..a001e43 100644
> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> >> @@ -680,6 +680,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
> >>  	if (is_cmd_mode(intel_dsi)) {
> >>  		for_each_dsi_port(port, intel_dsi->ports)
> >>  			I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(port), 8 * 4);
> >> +		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_TEAR_ON);  
> >
> > As with the TEAR_OFF, should this only be done for command mode?  Or is
> > it just a no-op for video mode and doesn't matter?  
> 
> In this case we are actually in a "if (is_cmd_mode(intel_dsi)) {" code
> block (the if is visible in the diff context).
> 
> Which I guess also shows that we really need to add the guard to the
> other path, so as to be consistent.
> 
> Note as mentioned in the commit msg:
> 
> untested, my panel is a vid-mode panel.

Oops, sorry.  I think there's another block of code somewhere that
looks almost like this but was wrapped in an if (is_vid_mode) condition
and I confused the two.

So disregard my comment and add:

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> 
> >  
> >> +		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
> >>  	} else {
> >>  		msleep(20); /* XXX */
> >>  		for_each_dsi_port(port, intel_dsi->ports)  
> >
> >
> >  
> 
> Regards,
> 
> Hans



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 15/15] drm/i915/dsi: Skip delays for v3 VBTs in vid-mode

[Bob Paauwe]

On Sat, 25 Feb 2017 11:49:09 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> HI,
> 
> On 24-02-17 18:02, Bob Paauwe wrote:
> > On Mon, 20 Feb 2017 15:08:45 +0100
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >  
> >> For v3 VBTs in vid-mode the delays are part of the VBT sequences, so
> >> we should not also delay ourselves otherwise we get double delays.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dsi.c | 19 +++++++++++++++----
> >>  1 file changed, 15 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> >> index a001e43..9e858c7 100644
> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> >> @@ -552,6 +552,17 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
> >>  			      struct intel_crtc_state *pipe_config);
> >>  static void intel_dsi_unprepare(struct intel_encoder *encoder);
> >>
> >> +static void intel_dsi_msleep(struct intel_dsi *intel_dsi, int msec)
> >> +{
> >> +	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
> >> +
> >> +	/* For v3 VBTs in vid-mode the delays are part of the VBT sequences */
> >> +	if (is_vid_mode(intel_dsi) && dev_priv->vbt.dsi.seq_version >= 3)
> >> +		return;
> >> +
> >> +	msleep(msec);
> >> +}
> >> +
> >>  /*
> >>   * Panel enable/disable sequences from the VBT spec.
> >>   *
> >> @@ -664,7 +675,7 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
> >>  	if (intel_dsi->gpio_panel)
> >>  		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
> >>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
> >> -	msleep(intel_dsi->panel_on_delay);
> >> +	intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay);
> >>
> >>  	/* Deassert reset */
> >>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
> >> @@ -686,7 +697,7 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
> >>  		msleep(20); /* XXX */
> >>  		for_each_dsi_port(port, intel_dsi->ports)
> >>  			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
> >> -		msleep(100);
> >> +		intel_dsi_msleep(intel_dsi, 100);
> >>
> >>  		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
> >>
> >> @@ -805,7 +816,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
> >>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
> >>
> >>  	/* Power off, try both CRC pmic gpio and VBT */
> >> -	msleep(intel_dsi->panel_off_delay);
> >> +	intel_dsi_msleep(intel_dsi, intel_dsi->panel_off_delay);
> >>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
> >>  	if (intel_dsi->gpio_panel)
> >>  		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 0);
> >> @@ -814,7 +825,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
> >>  	 * FIXME As we do with eDP, just make a note of the time here
> >>  	 * and perform the wait before the next panel power on.
> >>  	 */  
> >
> > Should this comment be updated now?  
> 
> It is still valid for non-vidmode and v2 VBT panels, so I think it can
> be left as is.
> 
> Regards,
> 
> Hans

OK, that makes sense.

> 
> 
> >
> > Otherwise
> > Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>
> >  
> >> -	msleep(intel_dsi->panel_pwr_cycle_delay);
> >> +	intel_dsi_msleep(intel_dsi, intel_dsi->panel_pwr_cycle_delay);
> >>  }
> >>
> >>  static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,  
> >
> >
> >  



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH resend 11/15] drm/i915/dsi: Group MIPI_SEQ_BACKLIGHT_ON/OFF with panel_[en|dis]able_backlight

[Jani Nikula]

On Mon, 27 Feb 2017, Bob Paauwe <bob.j.paauwe@intel.com> wrote:
> On Sat, 25 Feb 2017 11:37:50 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> Hi,
>> 
>> On 24-02-17 18:00, Bob Paauwe wrote:
>> > On Mon, 20 Feb 2017 15:08:41 +0100
>> > Hans de Goede <hdegoede@redhat.com> wrote:
>> >  
>> >> Execute the MIPI_SEQ_BACKLIGHT_ON/OFF VBT sequences at the same time as
>> >> we call intel_panel_enable_backlight() / intel_panel_disable_backlight().
>> >>
>> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>  
>> >
>> > I'm not sure that the added comments are necessary.  Maybe if there was
>> > some explanation for why we're using two different mechanisms to
>> > enable/disable backlight instead.  
>> 
>> That assumes I know why there are 2 mechanisms of I have to guess it
>> is because on some boards only one of the mechanisms works, but that
>> is just a guess. Just calling the VBT sequence should be enough on
>> (most?) boards.
>> 
>> Regards,
>> 
>> Hans
>
> I don't think I can describe why we have two mechanisms so just
> dropping the comment would be my preference at this point.

I think they're mostly alternatives, and only one or the other gets
used, but since the VBT sequence is rather generic, I think it's
possible one controls the SoC side and the other the panel side.

> It may be that calling the intel_panel_enable_backlight() will make
> sure the sysfs entries are correct while the VBT sequence simply
> modifies the hardware. 
>
> Possibly a good solution would be to enhance the code that executes the
> VBT sequence to also call the intel_panel_enable_backlight(), but
> that's not something to change here.

Please leave it as it is.

BR,
Jani.

>
>> 
>> 
>> >
>> > Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>
>> >  
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_dsi.c | 6 ++++--
>> >>  1 file changed, 4 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> >> index 8408f59..a8d0948 100644
>> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> >> @@ -687,12 +687,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>> >>  		msleep(100);
>> >>
>> >>  		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
>> >> -		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
>> >>
>> >>  		intel_dsi_port_enable(encoder);
>> >>  	}
>> >>
>> >> +	/* Enable backlight, both pwm and VBT */
>> >>  	intel_panel_enable_backlight() (intel_dsi->attached_connector);
>> >> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
>> >>  }
>> >>
>> >>  static void intel_dsi_enable_nop(struct intel_encoder *encoder,
>> >> @@ -718,6 +719,8 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
>> >>
>> >>  	DRM_DEBUG_KMS("\n");
>> >>
>> >> +	/* Disable backlight, both VBT and pwm */
>> >> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
>> >>  	intel_panel_disable_backlight(intel_dsi->attached_connector);
>> >>
>> >>  	/*
>> >> @@ -762,7 +765,6 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>> >>  	 * if disable packets are sent before sending shutdown packet then in
>> >>  	 * some next enable sequence send turn on packet error is observed
>> >>  	 */
>> >> -	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
>> >>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>> >>
>> >>  	/* Transition to LP-00 */  
>> >
>> >
>> >  

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

Re: [PATCH resend 12/15] drm/i915/dsi: Document always using v3 SHUTDOWN / MIPI_SEQ_DISPLAY_OFF order

[Jani Nikula]

On Mon, 27 Feb 2017, Bob Paauwe <bob.j.paauwe@intel.com> wrote:
> On Sat, 25 Feb 2017 11:42:09 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> Hi,
>> 
>> On 24-02-17 18:02, Bob Paauwe wrote:
>> > On Mon, 20 Feb 2017 15:08:42 +0100
>> > Hans de Goede <hdegoede@redhat.com> wrote:
>> >  
>> >> According to the spec for v2 VBTs we should call MIPI_SEQ_DISPLAY_OFF
>> >> before sending SHUTDOWN, where as for v3 VBTs we should send SHUTDOWN
>> >> first.
>> >>
>> >> Since the v2 order has known issues, we use the v3 order everywhere,
>> >> add a comment documenting this.
>> >>
>> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_dsi.c | 7 +++++++
>> >>  1 file changed, 7 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> >> index a8d0948..1914311 100644
>> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> >> @@ -732,6 +732,11 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
>> >>  			I915_WRITE(MIPI_DEVICE_READY(port), 0);
>> >>  	}
>> >>
>> >> +	/*
>> >> +	 * XXX: According to the spec we should send SHUTDOWN before
>> >> +	 * MIPI_SEQ_DISPLAY_OFF only for v3+ VBTs, but testing in the field
>> >> +	 * has shown that we should do this for v2 VBTs too?  
>> > drop the '?'  
>> 
>> I added that because I'm not 100% sure this is true, looking through git
>> history (and android x86 kernel patch-sets) I managed to piece together that
>> at one point in time the v2 sequence was used, but that yielded problems
>> during some testing, what the commits do not tell if is that testing was
>> using boards with v3 VBTs, but assuming v2 tables are out there in the
>> wild then it seems that the v3 order works fine for v2 too.
>> 
>> TLDR I'm not 100% sure about this hence the '?', my main goal with this
>> patch is to document that we're deviating from the spec for v2 tables here.
>
> If anyone else, Jani?, has more information about this, that would be
> good to know.  

I wish. The documentation on this is disgraceful.

> I'd be OK with just stating that "field testing has shown that the v3
> sequence works with v2 VBT's so just use that."

Ack.

>
>> 
>> >> +	 */
>> >>  	if (is_vid_mode(intel_dsi)) {
>> >>  		/* Send Shutdown command to the panel in LP mode */
>> >>  		for_each_dsi_port(port, intel_dsi->ports)
>> >> @@ -764,6 +769,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>> >>  	/*
>> >>  	 * if disable packets are sent before sending shutdown packet then in
>> >>  	 * some next enable sequence send turn on packet error is observed
>> >> +	 * XXX spec specifies SHUTDOWN before MIPI_SEQ_DISPLAY_OFF for
>> >> +	 * v3 VBTs, but not for v2 VBTs?
>> >>  	 */
>> >>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>> >>  
>> >
>> > Should XXX be replaced with something?  
>> 
>> XXX is used in many places in intel_dsi.c to indicate code which may need
>> work / which may needs to be investigated further. I followed that and
>> added XXX here since this code is deviating from the spec.
>> 
>> Regards,
>> 
>> Hans

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

Re: [PATCH resend 13/15] drm/i915/dsi: Execute MIPI_SEQ_TEAR_OFF from intel_dsi_post_disable

[Jani Nikula]

On Sat, 25 Feb 2017, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 24-02-17 18:02, Bob Paauwe wrote:
>> On Mon, 20 Feb 2017 15:08:43 +0100
>> Hans de Goede <hdegoede@redhat.com> wrote:
>>
>>> For v3 VBTs we should call MIPI_SEQ_TEAR_OFF before
>>> MIPI_SEQ_DISPLAY_OFF, for non v3 VBTs this is a nop.
>>
>> Isn't this only for command mode?  Or is there conflicting information
>> on this?
>
> In my (limited) set of test hardware vidmode panel VBTs simply
> do not define a MIPI_SEQ_TEAR_ON/OFF sequence. But it would probably
> be a good idea to guard this with a "if (is_cmd_mode(intel_dsi))"
> assuming we only want this for commandmode, which I think we do,
> but it would be good to have this confirmed.

Logically you wouldn't do tear on/off on video mode panels. But that's
really not the point. I wouldn't be surprised if there were setups where
someone noticed that this particular sequence gets run at an appropriate
time on Windows, and shoved some other stuff there, completely unrelated
to tear on/off.

BR,
Jani.


>
> I'll add the guard when I send out a v2 after we've clarified
> some of the other review questions.
>
> Regards,
>
> Hans
>
>
>
>>
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_dsi.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>>> index 1914311..90263d6 100644
>>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>>> @@ -772,6 +772,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>>>  	 * XXX spec specifies SHUTDOWN before MIPI_SEQ_DISPLAY_OFF for
>>>  	 * v3 VBTs, but not for v2 VBTs?
>>>  	 */
>>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_TEAR_OFF);
>>>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>>>
>>>  	/* Transition to LP-00 */
>>
>>
>>

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

Re: [PATCH resend 11/15] drm/i915/dsi: Group MIPI_SEQ_BACKLIGHT_ON/OFF with panel_[en|dis]able_backlight

[Hans de Goede]

Hi,

On 27-02-17 18:06, Bob Paauwe wrote:
> On Sat, 25 Feb 2017 11:37:50 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> Hi,
>>
>> On 24-02-17 18:00, Bob Paauwe wrote:
>>> On Mon, 20 Feb 2017 15:08:41 +0100
>>> Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>>> Execute the MIPI_SEQ_BACKLIGHT_ON/OFF VBT sequences at the same time as
>>>> we call intel_panel_enable_backlight() / intel_panel_disable_backlight().
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> I'm not sure that the added comments are necessary.  Maybe if there was
>>> some explanation for why we're using two different mechanisms to
>>> enable/disable backlight instead.
>>
>> That assumes I know why there are 2 mechanisms of I have to guess it
>> is because on some boards only one of the mechanisms works, but that
>> is just a guess. Just calling the VBT sequence should be enough on
>> (most?) boards.
>>
>> Regards,
>>
>> Hans
>
> I don't think I can describe why we have two mechanisms so just
> dropping the comment would be my preference at this point.

Ok, done for v2 of the patch-set I will go through your other comments
tomorrow and then send a v2.

Regards,

Hans


> It may be that calling the intel_panel_enable_backlight() will make
> sure the sysfs entries are correct while the VBT sequence simply
> modifies the hardware.
>
> Possibly a good solution would be to enhance the code that executes the
> VBT sequence to also call the intel_panel_enable_backlight(), but
> that's not something to change here.
>
>>
>>
>>>
>>> Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>
>>>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_dsi.c | 6 ++++--
>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>>>> index 8408f59..a8d0948 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>>>> @@ -687,12 +687,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>>>>  		msleep(100);
>>>>
>>>>  		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
>>>> -		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
>>>>
>>>>  		intel_dsi_port_enable(encoder);
>>>>  	}
>>>>
>>>> +	/* Enable backlight, both pwm and VBT */
>>>>  	intel_panel_enable_backlight() (intel_dsi->attached_connector);
>>>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
>>>>  }
>>>>
>>>>  static void intel_dsi_enable_nop(struct intel_encoder *encoder,
>>>> @@ -718,6 +719,8 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
>>>>
>>>>  	DRM_DEBUG_KMS("\n");
>>>>
>>>> +	/* Disable backlight, both VBT and pwm */
>>>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
>>>>  	intel_panel_disable_backlight(intel_dsi->attached_connector);
>>>>
>>>>  	/*
>>>> @@ -762,7 +765,6 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>>>>  	 * if disable packets are sent before sending shutdown packet then in
>>>>  	 * some next enable sequence send turn on packet error is observed
>>>>  	 */
>>>> -	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
>>>>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>>>>
>>>>  	/* Transition to LP-00 */
>>>
>>>
>>>
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH resend 12/15] drm/i915/dsi: Document always using v3 SHUTDOWN / MIPI_SEQ_DISPLAY_OFF order

[Hans de Goede]

Hi,

On 27-02-17 18:16, Bob Paauwe wrote:
> On Sat, 25 Feb 2017 11:42:09 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> Hi,
>>
>> On 24-02-17 18:02, Bob Paauwe wrote:
>>> On Mon, 20 Feb 2017 15:08:42 +0100
>>> Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>>> According to the spec for v2 VBTs we should call MIPI_SEQ_DISPLAY_OFF
>>>> before sending SHUTDOWN, where as for v3 VBTs we should send SHUTDOWN
>>>> first.
>>>>
>>>> Since the v2 order has known issues, we use the v3 order everywhere,
>>>> add a comment documenting this.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_dsi.c | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>>>> index a8d0948..1914311 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>>>> @@ -732,6 +732,11 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
>>>>  			I915_WRITE(MIPI_DEVICE_READY(port), 0);
>>>>  	}
>>>>
>>>> +	/*
>>>> +	 * XXX: According to the spec we should send SHUTDOWN before
>>>> +	 * MIPI_SEQ_DISPLAY_OFF only for v3+ VBTs, but testing in the field
>>>> +	 * has shown that we should do this for v2 VBTs too?
>>> drop the '?'
>>
>> I added that because I'm not 100% sure this is true, looking through git
>> history (and android x86 kernel patch-sets) I managed to piece together that
>> at one point in time the v2 sequence was used, but that yielded problems
>> during some testing, what the commits do not tell if is that testing was
>> using boards with v3 VBTs, but assuming v2 tables are out there in the
>> wild then it seems that the v3 order works fine for v2 too.
>>
>> TLDR I'm not 100% sure about this hence the '?', my main goal with this
>> patch is to document that we're deviating from the spec for v2 tables here.
>
> If anyone else, Jani?, has more information about this, that would be
> good to know.
>
> I'd be OK with just stating that "field testing has shown that the v3
> sequence works with v2 VBT's so just use that."

Ok, done for v2.

Regards,

Hans


>
>>
>>>> +	 */
>>>>  	if (is_vid_mode(intel_dsi)) {
>>>>  		/* Send Shutdown command to the panel in LP mode */
>>>>  		for_each_dsi_port(port, intel_dsi->ports)
>>>> @@ -764,6 +769,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>>>>  	/*
>>>>  	 * if disable packets are sent before sending shutdown packet then in
>>>>  	 * some next enable sequence send turn on packet error is observed
>>>> +	 * XXX spec specifies SHUTDOWN before MIPI_SEQ_DISPLAY_OFF for
>>>> +	 * v3 VBTs, but not for v2 VBTs?
>>>>  	 */
>>>>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>>>>
>>>
>>> Should XXX be replaced with something?
>>
>> XXX is used in many places in intel_dsi.c to indicate code which may need
>> work / which may needs to be investigated further. I followed that and
>> added XXX here since this code is deviating from the spec.
>>
>> Regards,
>>
>> Hans
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH resend 05/15] drm/i915/dsi: Document the panel enable / disable sequences from the spec

[Hans de Goede]

Hi,

On 25-02-17 11:31, Hans de Goede wrote:
> Hi Bob,
>
> Thank you for the review of this patch-set.
>
> On 24-02-17 18:00, Bob Paauwe wrote:
>> On Mon, 20 Feb 2017 15:08:35 +0100
>> Hans de Goede <hdegoede@redhat.com> wrote:
>>
>>> Document the DSI panel enable / disable sequences from the spec,
>>> for easy comparison between the code and the spec.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_dsi.c | 68 ++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 68 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>>> index 072f99b..8808f87 100644
>>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>>> @@ -552,6 +552,74 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
>>>                    struct intel_crtc_state *pipe_config);
>>>  static void intel_dsi_unprepare(struct intel_encoder *encoder);
>>>
>>> +/*
>>> + * Panel enable/disable sequences from the VBT spec.
>>> + *
>>> + * Note the spec has AssertReset / DeassertReset swapped from their
>>> + * usual naming. We use the normal names to avoid confusion (so below
>>> + * they are swapped compared to the spec).
>>> + *
>>> + * v2 sequence for video mode:
>>> + * - power on
>>> + * - wait t1+t2
>>> + * - MIPIDeassertResetPin
>>> + * - clk/data lines to lp-11
>>> + * - MIPISendInitialDcsCmds
>>> + * - turn on DPI
>>> + * - MIPIDisplayOn
>>> + * - wait t5
>>> + * - backlight on
>>> + * ...
>>> + * - backlight off
>>> + * - wait t6
>>> + * - MIPIDisplayOff
>>> + * - turn off DPI
>>> + * - clk/data lines to lp-00
>>> + * - MIPIAssertResetPin
>>> + * - wait t3
>>> + * - power off
>>> + * - wait t4
>>> + *
>>> + * v3 sequence for video mode:
>>> + * - MIPIPanelPowerOn
>>> + * - MIPIDeassertResetPin
>>> + * - set clk/data lines to lp-11
>>> + * - MIPISendInitialDcsCmds (LP)
>>> + * - turn on DPI
>>> + * - MIPITearOn (command mode only) + MIPIDisplayOn (LP and HS)
>>
>> What does (command mode only) mean in the video mode sequence?  My
>> assumption is that TearOn is only used in command mode, and as such,
>> shouldn't be done in video mode.  Is that wrong?
>
> I don't know I don't have access to the VBT spec I merely copied
> the contents of the comment this is adding from a mail which contained
> the sequences extracted from the doc by someone at Intel (Jani IIRC).
>
>>
>>> + * - MIPIBacklightOn
>>
>> What's the difference between MIPIBacklightOn here and "backlight on"
>> in the v2 sequence?
>
> AFAICT there is none, again copy pasted from the mail as is.

Wrong, after working on this a bit again I remember now what the difference
is, in the v2 sequence power-on and backlight on are left to the driver,
which is e.g. why the code has:

         if (intel_dsi->gpio_panel)
                 gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);

Which is really necessary for v2 VBTs only, where as for v3 we've
this call:

         intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);

Which is a nop for v2 VBTs since they don't define that sequence.

So for v2 I've added the following extra paragraph to the comment:

  * Steps starting with MIPI refer to VBT sequences, note that for v2
  * VBTs several steps which have a VBT in v2 are expected to be handled
  * directly by the driver, by directly driving gpios for example.


>> It might be cleaner to make this a table with V2 and V3 as columns,
>> that would also help to highlight the differences in the two versions.
>> Maybe even put the command mode sequence in a third column if it
>> doesn't make the lines too long.
>
> I can try once we've cleared up the other bits.

Done, using a table with columns is a good idea and I've done so
for v2.

Regards,

Hans


>
>>> + * ...
>>> + * - MIPIBacklightOff
>>> + * - turn off DPI
>>> + * - MIPITearOff + MIPIDisplayOff (LP)
>>> + * - clk/data lines to lp-00
>>> + * - MIPIAssertResetPin
>>> + * - MIPIPanelPowerOff
>>> + *
>>> + * sequence for command mode:
>>
>> Is this the sequence for both V2 and V3 or only V3?
>
> I think command mode is V3 only, but I'm not sure. I only have
> access to video mode devices myself.
>
>>> + * - power on
>>> + * - wait t1+t2
>>> + * - MIPIDeassertResetPin
>>> + * - clk/data lines to lp-11
>>> + * - MIPISendInitialDcsCmds
>>> + * - MIPITearOn
>>> + * - MIPIDisplayOn
>>> + * - set pipe to dsr mode
>>> + * - wait t5
>>> + * - backlight on
>>> + * ... issue write_mem_start/write_mem_continue commands ...
>>> + * - backlight off
>>> + * - wait t6
>>> + * - disable pipe dsr mode
>>> + * - MIPITearOff
>>> + * - MIPIDisplayOff
>>> + * - clk/data lines to lp-00
>>> + * - MIPIAssertResetPin
>>> + * - wait t3
>>> + * - power off
>>> + * - wait t4
>>> + */
>>> +
>>>  static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>>>                   struct intel_crtc_state *pipe_config,
>>>                   struct drm_connector_state *conn_state)
>>
>>
>>
>
> Regards,
>
> Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx