Re: [PATCH 2/2] drm/i915/BXT: Fixed COS blanking issue

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

Thanks for the review comments. Addressing them in next version.

On Thursday 04 February 2016 07:24 PM, Jani Nikula wrote:
On Wed, 03 Feb 2016, Ramalingam C <ramalingam.c@xxxxxxxxx> wrote:
From: Uma Shankar <uma.shankar@xxxxxxxxx>

During Charging OS mode, mipi display was blanking.This is
because during driver load, though encoder, connector were
active but crtc returned inactive. This caused sanitize
function to disable the DSI panel. In AOS, this is fine
since HWC will do a modeset and crtc, connector, encoder
mapping will be restored. But in COS, no modeset is called,
it just calls DPMS ON/OFF.

This is fine on BYT/CHT since transcoder is common b/w
all encoders. But for BXT, there is a separate mipi
transcoder. Hence this needs special handling for BXT.

Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
---
  drivers/gpu/drm/i915/intel_display.c |  115 ++++++++++++++++++++++++++++++++--
  1 file changed, 109 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a66220a..58d2cd9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -34,6 +34,7 @@
  #include <drm/drm_edid.h>
  #include <drm/drmP.h>
  #include "intel_drv.h"
+#include "intel_dsi.h"
  #include <drm/i915_drm.h>
  #include "i915_drv.h"
  #include "i915_trace.h"
@@ -7814,6 +7815,69 @@ static void intel_set_pipe_timings(struct intel_crtc *intel_crtc)
  		   (intel_crtc->config->pipe_src_h - 1));
  }
+static void intel_get_dsi_pipe_timings(struct intel_crtc *crtc,
+				   struct intel_crtc_state *pipe_config)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
Hum, how does this work for DSI transcoders.

+	struct intel_encoder *encoder;
+	uint32_t tmp;
+
+	tmp = I915_READ(HTOTAL(cpu_transcoder));
+	pipe_config->base.adjusted_mode.crtc_hdisplay = (tmp & 0xffff) + 1;
+	pipe_config->base.adjusted_mode.crtc_htotal =
+						((tmp >> 16) & 0xffff) + 1;
+	tmp = I915_READ(HBLANK(cpu_transcoder));
+	pipe_config->base.adjusted_mode.crtc_hblank_start = (tmp & 0xffff) + 1;
+	pipe_config->base.adjusted_mode.crtc_hblank_end =
+						((tmp >> 16) & 0xffff) + 1;
+	tmp = I915_READ(HSYNC(cpu_transcoder));
+	pipe_config->base.adjusted_mode.crtc_hsync_start = (tmp & 0xffff) + 1;
+	pipe_config->base.adjusted_mode.crtc_hsync_end =
+						((tmp >> 16) & 0xffff) + 1;
+
+	tmp = I915_READ(VBLANK(cpu_transcoder));
+	pipe_config->base.adjusted_mode.crtc_vblank_start = (tmp & 0xffff) + 1;
+	pipe_config->base.adjusted_mode.crtc_vblank_end =
+						((tmp >> 16) & 0xffff) + 1;
+	tmp = I915_READ(VSYNC(cpu_transcoder));
+	pipe_config->base.adjusted_mode.crtc_vsync_start = (tmp & 0xffff) + 1;
+	pipe_config->base.adjusted_mode.crtc_vsync_end =
+						((tmp >> 16) & 0xffff) + 1;
+
+	if (I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_INTERLACE_MASK) {
+		pipe_config->base.adjusted_mode.flags |=
+						DRM_MODE_FLAG_INTERLACE;
+		pipe_config->base.adjusted_mode.crtc_vtotal += 1;
+		pipe_config->base.adjusted_mode.crtc_vblank_end += 1;
+	}
+
+
+	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
+		struct intel_dsi *intel_dsi =
+			enc_to_intel_dsi(&encoder->base);
+		enum port port;
+
+		pipe_config->pipe_bpp = intel_dsi->dsi_bpp;
+		for_each_dsi_port(port, intel_dsi->ports) {
+			pipe_config->base.adjusted_mode.crtc_hdisplay =
+				I915_READ(BXT_MIPI_TRANS_HACTIVE(port));
+			pipe_config->base.adjusted_mode.crtc_vdisplay =
+				I915_READ(BXT_MIPI_TRANS_VACTIVE(port));
+			pipe_config->base.adjusted_mode.crtc_vtotal =
+				I915_READ(BXT_MIPI_TRANS_VTOTAL(port));
+		}
+	}
Since you already figure out the port/pipe in bxt_get_pipe_config_dsi, I
feel it would be better to use that instead of doing the for loops here
and semi-blindly casting the encoder to intel_dsi.

+
+	tmp = I915_READ(PIPESRC(crtc->pipe));
+	pipe_config->pipe_src_h = (tmp & 0xffff) + 1;
+	pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1;
+
+	pipe_config->base.mode.vdisplay = pipe_config->pipe_src_h;
+	pipe_config->base.mode.hdisplay = pipe_config->pipe_src_w;
+}
What's the point of duplicating most of intel_get_pipe_timings() when
you could just call that first, and then do your bxt dsi specific stuff?
I can't spot any differences.

Do we even need to read the generic HTOTAL etc. registers for DSI? Do
they even make sense?

BR,
Jani.

+
  static void intel_get_pipe_timings(struct intel_crtc *crtc,
  				   struct intel_crtc_state *pipe_config)
  {
@@ -9962,6 +10026,40 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
  	}
  }
+static void bxt_get_pipe_config_dsi(struct intel_crtc *crtc,
+					struct intel_crtc_state *pipe_config)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum port port = (dev_priv->vbt.dsi.port == DVO_PORT_MIPIA)
+				? PORT_A : PORT_C;
+	uint32_t dsi_ctrl = I915_READ(MIPI_CTRL(port));
+	uint32_t tmp;
+
+	tmp = I915_READ(BXT_MIPI_PORT_CTRL(port));
+	if (tmp & DPI_ENABLE) {
+		enum pipe trans_dsi_pipe;
+
+		switch (dsi_ctrl & BXT_PIPE_SELECT_MASK) {
+		default:
+			WARN(1, "unknown pipe linked to dsi transcoder\n");
+			return;
+		case BXT_PIPE_SELECT(PIPE_A):
+			trans_dsi_pipe = PIPE_A;
+			break;
+		case BXT_PIPE_SELECT(PIPE_B):
+			trans_dsi_pipe = PIPE_B;
+			break;
+		case BXT_PIPE_SELECT(PIPE_C):
+			trans_dsi_pipe = PIPE_C;
+			break;
+		}
+
+		if (trans_dsi_pipe == crtc->pipe)
+			pipe_config->has_dsi_encoder = true;
+	}
+}
+
  static bool haswell_get_pipe_config(struct intel_crtc *crtc,
  				    struct intel_crtc_state *pipe_config)
  {
@@ -9999,17 +10097,22 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
  			pipe_config->cpu_transcoder = TRANSCODER_EDP;
  	}
+ if (dev_priv->vbt.has_mipi && IS_BROXTON(dev))
+		bxt_get_pipe_config_dsi(crtc, pipe_config);
+
  	if (!intel_display_power_is_enabled(dev_priv,
  			POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder)))
  		return false;
- tmp = I915_READ(PIPECONF(pipe_config->cpu_transcoder));
-	if (!(tmp & PIPECONF_ENABLE))
-		return false;
-
-	haswell_get_ddi_port_state(crtc, pipe_config);
+	if (!pipe_config->has_dsi_encoder) {
+		tmp = I915_READ(PIPECONF(pipe_config->cpu_transcoder));
+		if (!(tmp & PIPECONF_ENABLE))
+			return false;
- intel_get_pipe_timings(crtc, pipe_config);
+		haswell_get_ddi_port_state(crtc, pipe_config);
+		intel_get_pipe_timings(crtc, pipe_config);
+	} else
+		intel_get_dsi_pipe_timings(crtc, pipe_config);
if (INTEL_INFO(dev)->gen >= 9) {
  		skl_init_scalers(dev, crtc, pipe_config);

--
Thanks,
--Ram

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux