Re: [PATCH 3/9] drm/i915: MIPI Port Ctrl related changes for dual link configuration

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 21 Oct 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Oct 21, 2014 at 12:00:30PM +0530, Singh, Gaurav K wrote:
>> 
>> On 9/24/2014 2:57 PM, Jani Nikula wrote:
>> >On Wed, 24 Sep 2014, Gaurav K Singh <gaurav.k.singh@intel.com> wrote:
>> >>Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
>> >>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> >>---
>> >>  drivers/gpu/drm/i915/i915_reg.h            |    1 +
>> >>  drivers/gpu/drm/i915/intel_dsi.c           |   53 ++++++++++++++++++++++------
>> >>  drivers/gpu/drm/i915/intel_dsi.h           |    1 +
>> >>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |    1 +
>> >>  4 files changed, 45 insertions(+), 11 deletions(-)
>> >>
>> >>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> >>index ad8179b..922d807 100644
>> >>--- a/drivers/gpu/drm/i915/i915_reg.h
>> >>+++ b/drivers/gpu/drm/i915/i915_reg.h
>> >>@@ -6215,6 +6215,7 @@ enum punit_power_well {
>> >>  #define  DPI_ENABLE					(1 << 31) /* A + B */
>> >>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT		27
>> >>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_MASK		(0xf << 27)
>> >>+#define  DUAL_LINK_MODE_SHIFT				26
>> >>  #define  DUAL_LINK_MODE_MASK				(1 << 26)
>> >>  #define  DUAL_LINK_MODE_FRONT_BACK			(0 << 26)
>> >>  #define  DUAL_LINK_MODE_PIXEL_ALTERNATIVE		(1 << 26)
>> >>diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> >>index e456ca9..3b1890e 100644
>> >>--- a/drivers/gpu/drm/i915/intel_dsi.c
>> >>+++ b/drivers/gpu/drm/i915/intel_dsi.c
>> >>@@ -109,13 +109,31 @@ static void intel_dsi_port_enable(struct intel_encoder *encoder)
>> >>  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>> >>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>> >>  	enum pipe pipe = intel_crtc->pipe;
>> >>-	u32 temp;
>> >>-
>> >>-	/* assert ip_tg_enable signal */
>> >>-	temp = I915_READ(MIPI_PORT_CTRL(pipe)) & ~LANE_CONFIGURATION_MASK;
>> >>-	temp = temp | intel_dsi->port_bits;
>> >>-	I915_WRITE(MIPI_PORT_CTRL(pipe), temp | DPI_ENABLE);
>> >>-	POSTING_READ(MIPI_PORT_CTRL(pipe));
>> >>+	u32 temp, port_control = 0;
>> >>+
>> >>+	if (intel_dsi->dual_link) {
>> >>+		port_control = (intel_dsi->dual_link - 1)
>> >>+					<< DUAL_LINK_MODE_SHIFT;
>> >>+		port_control |= pipe ? LANE_CONFIGURATION_DUAL_LINK_B :
>> >>+					LANE_CONFIGURATION_DUAL_LINK_A;
>> >>+		/*For Port A */
>> >>+		temp = I915_READ(MIPI_PORT_CTRL(0));
>> >>+		temp = temp | port_control;
>> >>+		I915_WRITE(MIPI_PORT_CTRL(0), temp | DPI_ENABLE);
>> >>+		POSTING_READ(MIPI_PORT_CTRL(0));
>> >>+
>> >>+		/* For Port C */
>> >>+		temp = I915_READ(MIPI_PORT_CTRL(1));
>> >>+		I915_WRITE(MIPI_PORT_CTRL(1), temp | DPI_ENABLE);
>> >>+		POSTING_READ(MIPI_PORT_CTRL(1));
>> >This calls for a cleanup in i915_reg.h for per port vs. per transcoder
>> >registers. MIPI_PORT_CTRL(1) uses _TRANSCODER macro. We also have enum
>> >port with PORT_C == 2. This gets confusing.
>> 
>> Are you suggesting to use _PORT macro instead of _TRANSCODER macro?
>
> I think so, and given that we already have some offenders the existing
> code should be converted with a prep patch first.

I guess what we have now works fine for single-link modes, with fixed
(per HW limitation IIUC) mappings MIPI DSI port A to pipe A and MIPI DSI
port C to pipe B. This particular problem probably leads back to my
original DSI enabling patches. (Except they were _PIPE back then, since
converted to _TRANSCODER.)

The dual-link config works with either pipe. I think you'll need some
helpers to do the mappings. Given a "dsi_config" - which may be just
intel_dsi->dual_link - you could have:

	for_each_dsi_port_per_pipe(port, pipe, dsi_config) {
	}

which would do one of:

	1) one iteration with PORT_A for PIPE_A in single-link
	2) one iteration with PORT_C for PIPE_B in single-link
	3) two iterations first PORT_A then PORT_C in dual-link

Maybe. Just an idea to help out. Maybe look at what's needed in the
future. Some places obviously need to have the dual link config spread
out due to differences in the registers and set up.

In any case it's bound to end in tears if we pass just magic 0 and 1
instead of PORT_A and PORT_C (== 2) to the macros.


BR,
Jani.




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

-- 
Jani Nikula, Intel Open Source Technology Center