On Friday 15 November 2013 01:57 PM, Jani Nikula wrote:
On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@xxxxxxxxx> wrote:
Basically ULPS handling during enable/disable has been moved to
pre_enable and post_disable phases. PLL and panel power disable
also has been moved to post_disable phase. The ULPS entry/exit
sequneces as suggested by HW team is as follows -
During enable time -
set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY
And during disable time to flush all FIFOs -
set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP
Also during disbale sequnece sub-encoder disable is moved to the end
after port is disabled.
v2: Based on comments from Ville
- Detailed epxlaination in the commit messgae
- Moved parameter changes out into another patch
- Backlight enabling will be a new patch
Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@xxxxxxxxx>
Signed-off-by: Shobhit Kumar <shobhit.kumar@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_drv.h | 11 ++++
drivers/gpu/drm/i915/intel_dsi.c | 111 ++++++++++++++++++++++++++------------
drivers/gpu/drm/i915/intel_dsi.h | 2 +
3 files changed, 91 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a2bbff9..55c16cb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2433,6 +2433,17 @@ int vlv_freq_opcode(int ddr_freq, int val);
#define POSTING_READ(reg) (void)I915_READ_NOTRACE(reg)
#define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg)
+#define I915_WRITE_BITS(reg, val, mask) \
+do { \
+ u32 tmp, data; \
+ tmp = I915_READ((reg)); \
+ tmp &= ~(mask); \
+ data = (val) & (mask); \
+ data = data | tmp; \
+ I915_WRITE((reg), data); \
+} while(0)
I would still prefer the explicit read, modify, and write in the code
instead of this, but it's a matter of taste I'll leave for Daniel to
call the shots on.
One reason for my dislike is how easy it will be to accidentally get the
val and mask parameters mixed up. I would instinctively put the mask
before the value (common convention of context before the rest), which
would be wrong here. I am not saying changing the order would make me
like this.
As mentioned in another mail, this is not required and I will remove it
+
+
/* "Broadcast RGB" property */
#define INTEL_BROADCAST_RGB_AUTO 0
#define INTEL_BROADCAST_RGB_FULL 1
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 8dc9a38..9e67f78 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -101,46 +101,59 @@ static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder)
vlv_enable_dsi_pll(encoder);
}
-static void intel_dsi_pre_enable(struct intel_encoder *encoder)
-{
- DRM_DEBUG_KMS("\n");
-}
-
-static void intel_dsi_enable(struct intel_encoder *encoder)
+void intel_dsi_device_ready(struct intel_encoder *encoder)
{
struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
int pipe = intel_crtc->pipe;
- u32 temp;
DRM_DEBUG_KMS("\n");
if (intel_dsi->dev.dev_ops->panel_reset)
intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
- temp = I915_READ(MIPI_DEVICE_READY(pipe));
- if ((temp & DEVICE_READY) == 0) {
- temp &= ~ULPS_STATE_MASK;
- I915_WRITE(MIPI_DEVICE_READY(pipe), temp | DEVICE_READY);
- } else if (temp & ULPS_STATE_MASK) {
- temp &= ~ULPS_STATE_MASK;
- I915_WRITE(MIPI_DEVICE_READY(pipe), temp | ULPS_STATE_EXIT);
- /*
- * We need to ensure that there is a minimum of 1 ms time
- * available before clearing the UPLS exit state.
- */
- msleep(2);
- I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
- }
+ I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), LP_OUTPUT_HOLD, LP_OUTPUT_HOLD);
+ usleep_range(1000, 1500);
+ I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY |
+ ULPS_STATE_EXIT, DEVICE_READY | ULPS_STATE_MASK);
+ usleep_range(2000, 2500);
+ I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
+ DEVICE_READY | ULPS_STATE_MASK);
+ usleep_range(2000, 2500);
+ I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00,
+ DEVICE_READY | ULPS_STATE_MASK);
+ usleep_range(2000, 2500);
+ I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
+ DEVICE_READY | ULPS_STATE_MASK);
It seems like an odd dance, but if that's what the hw folks say, I guess
we'll just have to take their word for it...
Yeah, but I reconfirmed from HW team and this is also now updated in
documents
+ usleep_range(2000, 2500);
if (intel_dsi->dev.dev_ops->send_otp_cmds)
intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
Maybe the hooks would better belong in intel_dsi_pre_enable, not
here. The panel_reset hook could be renamed pre_enable while at it.
Ok, moved the hooks into pre_enable, but I am not too sure on changing
the names as the current name suggests what exactly we are doing with
the panel. If you agree I will leave that bit out but do other changes.
+}
+static void intel_dsi_pre_enable(struct intel_encoder *encoder)
+{
+ DRM_DEBUG_KMS("\n");
+
+ /* put device in ready state */
+ intel_dsi_device_ready(encoder);
+}
+
+static void intel_dsi_enable(struct intel_encoder *encoder)
+{
+ struct drm_device *dev = encoder->base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+ struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+ int pipe = intel_crtc->pipe;
+ u32 temp;
+
+ DRM_DEBUG_KMS("\n");
+
if (is_cmd_mode(intel_dsi))
I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4);
-
- if (is_vid_mode(intel_dsi)) {
+ else {
msleep(20); /* XXX */
dpi_send_cmd(intel_dsi, TURN_ON);
msleep(100);
@@ -157,7 +170,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
static void intel_dsi_disable(struct intel_encoder *encoder)
{
- struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+ struct drm_device *dev = encoder->base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
int pipe = intel_crtc->pipe;
@@ -165,8 +179,6 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
DRM_DEBUG_KMS("\n");
- intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
-
if (is_vid_mode(intel_dsi)) {
dpi_send_cmd(intel_dsi, SHUTDOWN);
msleep(10);
@@ -179,19 +191,52 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
msleep(2);
}
- temp = I915_READ(MIPI_DEVICE_READY(pipe));
- if (temp & DEVICE_READY) {
- temp &= ~DEVICE_READY;
- temp &= ~ULPS_STATE_MASK;
- I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
- }
+ /* if disable packets are sent before sending shutdown packet then in
+ * some next enable sequence send turn on packet error is observed */
+ if (intel_dsi->dev.dev_ops->disable)
+ intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
}
-static void intel_dsi_post_disable(struct intel_encoder *encoder)
+void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
{
+ struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+ struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+ struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+ int pipe = intel_crtc->pipe;
+
DRM_DEBUG_KMS("\n");
+ I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
+ ULPS_STATE_MASK);
+ usleep_range(2000, 2500);
+
+ I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_EXIT,
+ ULPS_STATE_MASK);
+ usleep_range(2000, 2500);
+
+ I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
+ ULPS_STATE_MASK);
+ usleep_range(2000, 2500);
+
+ I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), 0, LP_OUTPUT_HOLD);
+ usleep_range(1000, 1500);
+
+ if (wait_for(((I915_READ(MIPI_PORT_CTRL(pipe)) & 0x20000)
+ == 0x00000), 30))
+ DRM_ERROR("DSI LP not going Low\n");
AFE_LATCHOUT?
Yeah, fixed it. Rap on me hardcodings :)
+
+ I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00, DEVICE_READY);
+ usleep_range(2000, 2500);
+
vlv_disable_dsi_pll(encoder);
+
+ if (intel_dsi->dev.dev_ops->disable_panel_power)
+ intel_dsi->dev.dev_ops->disable_panel_power(&intel_dsi->dev);
Here too, hook better suited at intel_dsi_post_disable. And call it
post_disable.
Will move the hook but not too sure on changing the name as above
You can expect updated patches by Monday. Sorry it took me some time to
get back to this.
Regards
Shobiht
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
