[PATCH 0/9] drm/i915: HDMI 12bpc fixes

[PATCH 0/9] drm/i915: HDMI 12bpc fixes

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Our HDMI 12bpc support has always been broken. This series aims to fix that.

The problems addressed include:
- missing GCP infoframes entirely
- IBX w/a code was mostly nonsense
- missing w/a for CPT/PPT
- 12bpc vs. DBLCLK was busted

Part of this was already posted [1] quite a while ago, but it's grown some
new stuff since. The entire series is available in git [2].

I have additional stuff to fix the IBX transcoder B workarounds and
some other random thigns on PCH platforms. I'll post that as a followup.

[1] http://lists.freedesktop.org/archives/intel-gfx/2014-October/053340.html
[2] hit://github.com/vsyrjala/linux.git hdmi_12bpc_fixes_9

Ville Syrjälä (9):
  drm/i915: Implement WaEnableHDMI8bpcBefore12bpc:snb,ivb
  drm/i915: Send GCP infoframes for deep color HDMI sinks
  drm/i915: Enable default_phase in GCP when possible
  drm/i915: Fix HDMI 12bpc TRANSCONF bpc value
  drm/i915: Fix 12bpc HDMI enable for IBX
  drm/i915: Disable all infoframes when turning off the HDMI port
  drm/i915: Check infoframe state more diligently.
  drm/i915: Fix hdmi clock readout with pixel repeat
  drm/i915: Double the port clock when using double clocked modes with
    12bpc

 drivers/gpu/drm/i915/i915_reg.h      |   4 +
 drivers/gpu/drm/i915/intel_display.c |  10 +-
 drivers/gpu/drm/i915/intel_hdmi.c    | 359 ++++++++++++++++++++++++++++-------
 3 files changed, 306 insertions(+), 67 deletions(-)

-- 
2.0.5

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

[PATCH v2 1/9] drm/i915: Implement WaEnableHDMI8bpcBefore12bpc:snb, ivb

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

CPT/PPT require a specific procedure for enabling 12bpc HDMI. Implement
it, and to keep things neat pull the code into a function.

v2: Rebased due to crtc->config changes
    s/HDMI_GC/HDMIUNIT_GC/ to match spec better
    Factor out intel_enable_hdmi_audio()

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h   |  1 +
 drivers/gpu/drm/i915/intel_hdmi.c | 74 +++++++++++++++++++++++++++++++++++----
 2 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2339ffa..e619e41 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6149,6 +6149,7 @@ enum skl_disp_power_wells {
 #define _TRANSA_CHICKEN1	 0xf0060
 #define _TRANSB_CHICKEN1	 0xf1060
 #define TRANS_CHICKEN1(pipe) _PIPE(pipe, _TRANSA_CHICKEN1, _TRANSB_CHICKEN1)
+#define  TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE	(1<<10)
 #define  TRANS_CHICKEN1_DP0UNIT_GC_DISABLE	(1<<4)
 #define _TRANSA_CHICKEN2	 0xf0064
 #define _TRANSB_CHICKEN2	 0xf1064
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index a84c040..79cf445 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -814,6 +814,16 @@ static void intel_hdmi_get_config(struct intel_encoder *encoder,
 	pipe_config->base.adjusted_mode.crtc_clock = dotclock;
 }
 
+static void intel_enable_hdmi_audio(struct intel_encoder *encoder)
+{
+	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
+
+	WARN_ON(!crtc->config->has_hdmi_sink);
+	DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
+			 pipe_name(crtc->pipe));
+	intel_audio_codec_enable(encoder);
+}
+
 static void intel_enable_hdmi(struct intel_encoder *encoder)
 {
 	struct drm_device *dev = encoder->base.dev;
@@ -854,12 +864,61 @@ static void intel_enable_hdmi(struct intel_encoder *encoder)
 		POSTING_READ(intel_hdmi->hdmi_reg);
 	}
 
-	if (intel_crtc->config->has_audio) {
-		WARN_ON(!intel_crtc->config->has_hdmi_sink);
-		DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
-				 pipe_name(intel_crtc->pipe));
-		intel_audio_codec_enable(encoder);
+	if (intel_crtc->config->has_audio)
+		intel_enable_hdmi_audio(encoder);
+}
+
+static void cpt_enable_hdmi(struct intel_encoder *encoder)
+{
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
+	enum pipe pipe = crtc->pipe;
+	u32 temp;
+
+	temp = I915_READ(intel_hdmi->hdmi_reg);
+
+	temp |= SDVO_ENABLE;
+	if (crtc->config->has_audio)
+		temp |= SDVO_AUDIO_ENABLE;
+
+	/*
+	 * WaEnableHDMI8bpcBefore12bpc:snb,ivb
+	 *
+	 * The procedure for 12bpc is as follows:
+	 * 1. disable HDMI clock gating
+	 * 2. enable HDMI with 8bpc
+	 * 3. enable HDMI with 12bpc
+	 * 4. enable HDMI clock gating
+	 */
+
+	if (crtc->config->pipe_bpp > 24) {
+		I915_WRITE(TRANS_CHICKEN1(pipe),
+			   I915_READ(TRANS_CHICKEN1(pipe)) |
+			   TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
+
+		temp &= ~SDVO_COLOR_FORMAT_MASK;
+		temp |= SDVO_COLOR_FORMAT_8bpc;
 	}
+
+	I915_WRITE(intel_hdmi->hdmi_reg, temp);
+	POSTING_READ(intel_hdmi->hdmi_reg);
+
+	if (crtc->config->pipe_bpp > 24) {
+		temp &= ~SDVO_COLOR_FORMAT_MASK;
+		temp |= HDMI_COLOR_FORMAT_12bpc;
+
+		I915_WRITE(intel_hdmi->hdmi_reg, temp);
+		POSTING_READ(intel_hdmi->hdmi_reg);
+
+		I915_WRITE(TRANS_CHICKEN1(pipe),
+			   I915_READ(TRANS_CHICKEN1(pipe)) &
+			   ~TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
+	}
+
+	if (crtc->config->has_audio)
+		intel_enable_hdmi_audio(encoder);
 }
 
 static void vlv_enable_hdmi(struct intel_encoder *encoder)
@@ -1829,7 +1888,10 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 		intel_encoder->post_disable = vlv_hdmi_post_disable;
 	} else {
 		intel_encoder->pre_enable = intel_hdmi_pre_enable;
-		intel_encoder->enable = intel_enable_hdmi;
+		if (HAS_PCH_CPT(dev))
+			intel_encoder->enable = cpt_enable_hdmi;
+		else
+			intel_encoder->enable = intel_enable_hdmi;
 	}
 
 	intel_encoder->type = INTEL_OUTPUT_HDMI;
-- 
2.0.5

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

[PATCH v2 2/9] drm/i915: Send GCP infoframes for deep color HDMI sinks

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

GCP infoframes are required to inform the HDMI sink about the color
depth.

Send the GCP infoframe whenever the sink supports any deep color modes
since such sinks must anyway be capable of receiving them. For sinks
that don't support deep color let's skip the GCP in case it might
confuse the sink, although HDMI 1.4 spec does say all sinks must be
capable of reciving them. In theory we could skip the GCP infoframe
for deep color sinks in 8bpc mode as well since sinks must fall back to
8bpc whenever GCP isn't received for some time.

BSpec says we should disable GCP after disabling the port, so do that as
well.

v2: s/intel_set_gcp_infoframe/intel_hdmi_set_gcp_infoframe/
    Rebased due to crtc->config changes

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h   |  3 ++
 drivers/gpu/drm/i915/intel_hdmi.c | 74 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e619e41..dcd93b5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6010,6 +6010,9 @@ enum skl_disp_power_wells {
 #define _VIDEO_DIP_CTL_A         0xe0200
 #define _VIDEO_DIP_DATA_A        0xe0208
 #define _VIDEO_DIP_GCP_A         0xe0210
+#define  GCP_COLOR_INDICATION		(1 << 2)
+#define  GCP_DEFAULT_PHASE_ENABLE	(1 << 1)
+#define  GCP_AV_MUTE			(1 << 0)
 
 #define _VIDEO_DIP_CTL_B         0xe1200
 #define _VIDEO_DIP_DATA_B        0xe1208
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 79cf445..87c4905 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -541,6 +541,66 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 	intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode);
 }
 
+static bool hdmi_sink_is_deep_color(struct drm_encoder *encoder)
+{
+	struct drm_device *dev = encoder->dev;
+	struct drm_connector *connector;
+
+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+
+	/*
+	 * HDMI cloning is only supported on g4x which doesn't
+	 * support deep color or GCP infoframes anyway so no
+	 * need to worry about multiple HDMI sinks here.
+	 */
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
+		if (connector->encoder == encoder)
+			return connector->display_info.bpc > 8;
+
+	return false;
+}
+
+static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
+	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
+	u32 reg, val = 0;
+
+	if (HAS_DDI(dev_priv))
+		reg = HSW_TVIDEO_DIP_GCP(crtc->config->cpu_transcoder);
+	else if (IS_VALLEYVIEW(dev_priv))
+		reg = VLV_TVIDEO_DIP_GCP(crtc->pipe);
+	else if (HAS_PCH_SPLIT(dev_priv->dev))
+		reg = TVIDEO_DIP_GCP(crtc->pipe);
+	else
+		return false;
+
+	/* Indicate color depth wheneven the sink supports deep color */
+	if (hdmi_sink_is_deep_color(encoder))
+		val |= GCP_COLOR_INDICATION;
+
+	I915_WRITE(reg, val);
+
+	return val != 0;
+}
+
+static void intel_disable_gcp_infoframe(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	u32 reg;
+
+	if (HAS_DDI(dev_priv))
+		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
+	else if (IS_VALLEYVIEW(dev_priv))
+		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
+	else if (HAS_PCH_SPLIT(dev_priv->dev))
+		reg = TVIDEO_DIP_CTL(crtc->pipe);
+	else
+		return;
+
+	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP);
+}
+
 static void ibx_set_infoframes(struct drm_encoder *encoder,
 			       bool enable,
 			       struct drm_display_mode *adjusted_mode)
@@ -581,6 +641,9 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
 		 VIDEO_DIP_ENABLE_GCP);
 
+	if (intel_hdmi_set_gcp_infoframe(encoder))
+		val |= VIDEO_DIP_ENABLE_GCP;
+
 	I915_WRITE(reg, val);
 	POSTING_READ(reg);
 
@@ -618,6 +681,9 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
 	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
 		 VIDEO_DIP_ENABLE_GCP);
 
+	if (intel_hdmi_set_gcp_infoframe(encoder))
+		val |= VIDEO_DIP_ENABLE_GCP;
+
 	I915_WRITE(reg, val);
 	POSTING_READ(reg);
 
@@ -666,6 +732,9 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
 	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
 		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
 
+	if (intel_hdmi_set_gcp_infoframe(encoder))
+		val |= VIDEO_DIP_ENABLE_GCP;
+
 	I915_WRITE(reg, val);
 	POSTING_READ(reg);
 
@@ -695,6 +764,9 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
 	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_GCP_HSW |
 		 VIDEO_DIP_ENABLE_VS_HSW | VIDEO_DIP_ENABLE_GMP_HSW);
 
+	if (intel_hdmi_set_gcp_infoframe(encoder))
+		val |= VIDEO_DIP_ENABLE_GCP_HSW;
+
 	I915_WRITE(reg, val);
 	POSTING_READ(reg);
 
@@ -986,6 +1058,8 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
 
 	if (IS_CHERRYVIEW(dev))
 		chv_powergate_phy_lanes(encoder, 0xf);
+
+	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));
 }
 
 static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
-- 
2.0.5

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

[PATCH v2 3/9] drm/i915: Enable default_phase in GCP when possible

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When the video timings are suitably aligned so that all different
periods start at phase 0 (ie. none of the periods start mid-pixel)
we can inform the sink about this. Supposedly the sink can then
optimize certain things. Obviously this is only relevant when
outputting >8bpc data since otherwise there are no mid-pixel phases.

v2: Rebased due to crtc->config changes

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 48 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 87c4905..2e98e33 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -560,6 +560,49 @@ static bool hdmi_sink_is_deep_color(struct drm_encoder *encoder)
 	return false;
 }
 
+/*
+ * Determine if default_phase=1 can be indicated in the GCP infoframe.
+ *
+ * From HDMI specification 1.4a:
+ * - The first pixel of each Video Data Period shall always have a pixel packing phase of 0
+ * - The first pixel following each Video Data Period shall have a pixel packing phase of 0
+ * - The PP bits shall be constant for all GCPs and will be equal to the last packing phase
+ * - The first pixel following every transition of HSYNC or VSYNC shall have a pixel packing
+ *   phase of 0
+ */
+static bool gcp_default_phase_possible(int pipe_bpp,
+				       const struct drm_display_mode *mode)
+{
+	unsigned int pixels_per_group;
+
+	switch (pipe_bpp) {
+	case 30:
+		/* 4 pixels in 5 clocks */
+		pixels_per_group = 4;
+		break;
+	case 36:
+		/* 2 pixels in 3 clocks */
+		pixels_per_group = 2;
+		break;
+	case 48:
+		/* 1 pixel in 2 clocks */
+		pixels_per_group = 1;
+		break;
+	default:
+		/* phase information not relevant for 8bpc */
+		return false;
+	}
+
+	return mode->crtc_hdisplay % pixels_per_group == 0 &&
+		mode->crtc_htotal % pixels_per_group == 0 &&
+		mode->crtc_hblank_start % pixels_per_group == 0 &&
+		mode->crtc_hblank_end % pixels_per_group == 0 &&
+		mode->crtc_hsync_start % pixels_per_group == 0 &&
+		mode->crtc_hsync_end % pixels_per_group == 0 &&
+		((mode->flags & DRM_MODE_FLAG_INTERLACE) == 0 ||
+		 mode->crtc_htotal/2 % pixels_per_group == 0);
+}
+
 static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
@@ -579,6 +622,11 @@ static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)
 	if (hdmi_sink_is_deep_color(encoder))
 		val |= GCP_COLOR_INDICATION;
 
+	/* Enable default_phase whenever the display mode is suitably aligned */
+	if (gcp_default_phase_possible(crtc->config->pipe_bpp,
+				       &crtc->config->base.adjusted_mode))
+		val |= GCP_DEFAULT_PHASE_ENABLE;
+
 	I915_WRITE(reg, val);
 
 	return val != 0;
-- 
2.0.5

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

[PATCH v2 4/9] drm/i915: Fix HDMI 12bpc TRANSCONF bpc value

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

IBX BSpec says we must specify 8bpc in TRANSCONF for both 8bpc
and 12bpc HDMI output. Do so.

v2: Pass intel_crtc to intel_pipe_has_type()

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 96ce8f6..a392188 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2001,11 +2001,15 @@ static void ironlake_enable_pch_transcoder(struct drm_i915_private *dev_priv,
 
 	if (HAS_PCH_IBX(dev_priv->dev)) {
 		/*
-		 * make the BPC in transcoder be consistent with
-		 * that in pipeconf reg.
+		 * Make the BPC in transcoder be consistent with
+		 * that in pipeconf reg. For HDMI we must use 8bpc
+		 * here for both 8bpc and 12bpc.
 		 */
 		val &= ~PIPECONF_BPC_MASK;
-		val |= pipeconf_val & PIPECONF_BPC_MASK;
+		if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_HDMI))
+			val |= PIPECONF_8BPC;
+		else
+			val |= pipeconf_val & PIPECONF_BPC_MASK;
 	}
 
 	val &= ~TRANS_INTERLACE_MASK;
-- 
2.0.5

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

[PATCH v2 5/9] drm/i915: Fix 12bpc HDMI enable for IBX

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Follow the procedure listed in Bspec to toggle the port enable bit off
and on when enabling HDMI with 12bpc and pixel repeat on IBX. The old
code didn't actually enable the port before "toggling" the bit back off,
so the whole workaround was essentially a nop.

Also take the opportunity to clarify the code by splitting the gmch
platforms to a separate (much more straightforward) function.

v2: Rebased due to crtc->config changes

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 78 ++++++++++++++++++++++++++-------------
 1 file changed, 53 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 2e98e33..766bdb1 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -944,47 +944,73 @@ static void intel_enable_hdmi_audio(struct intel_encoder *encoder)
 	intel_audio_codec_enable(encoder);
 }
 
-static void intel_enable_hdmi(struct intel_encoder *encoder)
+static void g4x_enable_hdmi(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_crtc *crtc = to_intel_crtc(encoder->base.crtc);
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
 	u32 temp;
-	u32 enable_bits = SDVO_ENABLE;
-
-	if (intel_crtc->config->has_audio)
-		enable_bits |= SDVO_AUDIO_ENABLE;
 
 	temp = I915_READ(intel_hdmi->hdmi_reg);
 
-	/* HW workaround for IBX, we need to move the port to transcoder A
-	 * before disabling it, so restore the transcoder select bit here. */
-	if (HAS_PCH_IBX(dev))
-		enable_bits |= SDVO_PIPE_SEL(intel_crtc->pipe);
+	temp |= SDVO_ENABLE;
+	if (crtc->config->has_audio)
+		temp |= SDVO_AUDIO_ENABLE;
 
-	/* HW workaround, need to toggle enable bit off and on for 12bpc, but
-	 * we do this anyway which shows more stable in testing.
-	 */
-	if (HAS_PCH_SPLIT(dev)) {
-		I915_WRITE(intel_hdmi->hdmi_reg, temp & ~SDVO_ENABLE);
-		POSTING_READ(intel_hdmi->hdmi_reg);
-	}
+	I915_WRITE(intel_hdmi->hdmi_reg, temp);
+	POSTING_READ(intel_hdmi->hdmi_reg);
 
-	temp |= enable_bits;
+	if (crtc->config->has_audio)
+		intel_enable_hdmi_audio(encoder);
+}
 
+static void ibx_enable_hdmi(struct intel_encoder *encoder)
+{
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
+	u32 temp;
+
+	temp = I915_READ(intel_hdmi->hdmi_reg);
+
+	temp |= SDVO_ENABLE;
+	if (crtc->config->has_audio)
+		temp |= SDVO_AUDIO_ENABLE;
+
+	/*
+	 * HW workaround, need to write this twice for issue
+	 * that may result in first write getting masked.
+	 */
+	I915_WRITE(intel_hdmi->hdmi_reg, temp);
+	POSTING_READ(intel_hdmi->hdmi_reg);
 	I915_WRITE(intel_hdmi->hdmi_reg, temp);
 	POSTING_READ(intel_hdmi->hdmi_reg);
 
-	/* HW workaround, need to write this twice for issue that may result
-	 * in first write getting masked.
+	/*
+	 * HW workaround, need to toggle enable bit off and on
+	 * for 12bpc with pixel repeat.
+	 *
+	 * FIXME: BSpec says this should be done at the end of
+	 * of the modeset sequence, so not sure if this isn't too soon.
 	 */
-	if (HAS_PCH_SPLIT(dev)) {
+	if (crtc->config->pipe_bpp > 24 &&
+	    crtc->config->pixel_multiplier > 1) {
+		I915_WRITE(intel_hdmi->hdmi_reg, temp & ~SDVO_ENABLE);
+		POSTING_READ(intel_hdmi->hdmi_reg);
+
+		/*
+		 * HW workaround, need to write this twice for issue
+		 * that may result in first write getting masked.
+		 */
+		I915_WRITE(intel_hdmi->hdmi_reg, temp);
+		POSTING_READ(intel_hdmi->hdmi_reg);
 		I915_WRITE(intel_hdmi->hdmi_reg, temp);
 		POSTING_READ(intel_hdmi->hdmi_reg);
 	}
 
-	if (intel_crtc->config->has_audio)
+	if (crtc->config->has_audio)
 		intel_enable_hdmi_audio(encoder);
 }
 
@@ -1509,7 +1535,7 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
 				   intel_crtc->config->has_hdmi_sink,
 				   adjusted_mode);
 
-	intel_enable_hdmi(encoder);
+	g4x_enable_hdmi(encoder);
 
 	vlv_wait_port_ready(dev_priv, dport, 0x0);
 }
@@ -1828,7 +1854,7 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
 
 	chv_powergate_phy_lanes(encoder, 0x0);
 
-	intel_enable_hdmi(encoder);
+	g4x_enable_hdmi(encoder);
 
 	vlv_wait_port_ready(dev_priv, dport, 0x0);
 }
@@ -2012,8 +2038,10 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 		intel_encoder->pre_enable = intel_hdmi_pre_enable;
 		if (HAS_PCH_CPT(dev))
 			intel_encoder->enable = cpt_enable_hdmi;
+		else if (HAS_PCH_IBX(dev))
+			intel_encoder->enable = ibx_enable_hdmi;
 		else
-			intel_encoder->enable = intel_enable_hdmi;
+			intel_encoder->enable = g4x_enable_hdmi;
 	}
 
 	intel_encoder->type = INTEL_OUTPUT_HDMI;
-- 
2.0.5

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

[PATCH v2 6/9] drm/i915: Disable all infoframes when turning off the HDMI port

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we just disable the GCP infoframe when turning off the port.
That means if the same transcoder is used on a DP port next, we might
end up pushing infoframes over DP, which isn't intended. Just disable
all the infoframes when turning off the port.

Also protect against two ports stomping on each other on g4x due to
the single video DIP instance. Now only the first port to enable
gets to send infoframes.

v2: Rebase

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 85 ++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 766bdb1..03b4759 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -514,7 +514,13 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 	if (!enable) {
 		if (!(val & VIDEO_DIP_ENABLE))
 			return;
-		val &= ~VIDEO_DIP_ENABLE;
+		if (port != (val & VIDEO_DIP_PORT_MASK)) {
+			DRM_DEBUG_KMS("video DIP still enabled on port %c\n",
+				      (val & VIDEO_DIP_PORT_MASK) >> 29);
+			return;
+		}
+		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
+			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);
 		I915_WRITE(reg, val);
 		POSTING_READ(reg);
 		return;
@@ -522,16 +528,17 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 
 	if (port != (val & VIDEO_DIP_PORT_MASK)) {
 		if (val & VIDEO_DIP_ENABLE) {
-			val &= ~VIDEO_DIP_ENABLE;
-			I915_WRITE(reg, val);
-			POSTING_READ(reg);
+			DRM_DEBUG_KMS("video DIP already enabled on port %c\n",
+				      (val & VIDEO_DIP_PORT_MASK) >> 29);
+			return;
 		}
 		val &= ~VIDEO_DIP_PORT_MASK;
 		val |= port;
 	}
 
 	val |= VIDEO_DIP_ENABLE;
-	val &= ~VIDEO_DIP_ENABLE_VENDOR;
+	val &= ~(VIDEO_DIP_ENABLE_AVI |
+		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);
 
 	I915_WRITE(reg, val);
 	POSTING_READ(reg);
@@ -632,23 +639,6 @@ static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)
 	return val != 0;
 }
 
-static void intel_disable_gcp_infoframe(struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
-	u32 reg;
-
-	if (HAS_DDI(dev_priv))
-		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
-	else if (IS_VALLEYVIEW(dev_priv))
-		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
-	else if (HAS_PCH_SPLIT(dev_priv->dev))
-		reg = TVIDEO_DIP_CTL(crtc->pipe);
-	else
-		return;
-
-	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP);
-}
-
 static void ibx_set_infoframes(struct drm_encoder *encoder,
 			       bool enable,
 			       struct drm_display_mode *adjusted_mode)
@@ -669,25 +659,26 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 	if (!enable) {
 		if (!(val & VIDEO_DIP_ENABLE))
 			return;
-		val &= ~VIDEO_DIP_ENABLE;
+		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
+			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
 		I915_WRITE(reg, val);
 		POSTING_READ(reg);
 		return;
 	}
 
 	if (port != (val & VIDEO_DIP_PORT_MASK)) {
-		if (val & VIDEO_DIP_ENABLE) {
-			val &= ~VIDEO_DIP_ENABLE;
-			I915_WRITE(reg, val);
-			POSTING_READ(reg);
-		}
+		WARN(val & VIDEO_DIP_ENABLE,
+		     "DIP already enabled on port %c\n",
+		     (val & VIDEO_DIP_PORT_MASK) >> 29);
 		val &= ~VIDEO_DIP_PORT_MASK;
 		val |= port;
 	}
 
 	val |= VIDEO_DIP_ENABLE;
-	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
-		 VIDEO_DIP_ENABLE_GCP);
+	val &= ~(VIDEO_DIP_ENABLE_AVI |
+		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
 
 	if (intel_hdmi_set_gcp_infoframe(encoder))
 		val |= VIDEO_DIP_ENABLE_GCP;
@@ -718,7 +709,9 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
 	if (!enable) {
 		if (!(val & VIDEO_DIP_ENABLE))
 			return;
-		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI);
+		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
+			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
 		I915_WRITE(reg, val);
 		POSTING_READ(reg);
 		return;
@@ -727,7 +720,7 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
 	/* Set both together, unset both together: see the spec. */
 	val |= VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI;
 	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
-		 VIDEO_DIP_ENABLE_GCP);
+		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
 
 	if (intel_hdmi_set_gcp_infoframe(encoder))
 		val |= VIDEO_DIP_ENABLE_GCP;
@@ -760,25 +753,26 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
 	if (!enable) {
 		if (!(val & VIDEO_DIP_ENABLE))
 			return;
-		val &= ~VIDEO_DIP_ENABLE;
+		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
+			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
 		I915_WRITE(reg, val);
 		POSTING_READ(reg);
 		return;
 	}
 
 	if (port != (val & VIDEO_DIP_PORT_MASK)) {
-		if (val & VIDEO_DIP_ENABLE) {
-			val &= ~VIDEO_DIP_ENABLE;
-			I915_WRITE(reg, val);
-			POSTING_READ(reg);
-		}
+		WARN(val & VIDEO_DIP_ENABLE,
+		     "DIP already enabled on port %c\n",
+		     (val & VIDEO_DIP_PORT_MASK) >> 29);
 		val &= ~VIDEO_DIP_PORT_MASK;
 		val |= port;
 	}
 
 	val |= VIDEO_DIP_ENABLE;
-	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
-		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
+	val &= ~(VIDEO_DIP_ENABLE_AVI |
+		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
 
 	if (intel_hdmi_set_gcp_infoframe(encoder))
 		val |= VIDEO_DIP_ENABLE_GCP;
@@ -803,15 +797,16 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
 
 	assert_hdmi_port_disabled(intel_hdmi);
 
+	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_AVI_HSW |
+		 VIDEO_DIP_ENABLE_GCP_HSW | VIDEO_DIP_ENABLE_VS_HSW |
+		 VIDEO_DIP_ENABLE_GMP_HSW | VIDEO_DIP_ENABLE_SPD_HSW);
+
 	if (!enable) {
-		I915_WRITE(reg, 0);
+		I915_WRITE(reg, val);
 		POSTING_READ(reg);
 		return;
 	}
 
-	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_GCP_HSW |
-		 VIDEO_DIP_ENABLE_VS_HSW | VIDEO_DIP_ENABLE_GMP_HSW);
-
 	if (intel_hdmi_set_gcp_infoframe(encoder))
 		val |= VIDEO_DIP_ENABLE_GCP_HSW;
 
@@ -1133,7 +1128,7 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
 	if (IS_CHERRYVIEW(dev))
 		chv_powergate_phy_lanes(encoder, 0xf);
 
-	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));
+	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
 }
 
 static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
-- 
2.0.5

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

[PATCH 7/9] drm/i915: Check infoframe state more diligently.

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Check that the DIP is enabled on the right port on IBX and VLV/CHV as
we're doing on g4x, and also check for all the infoframe enable bits on
all platforms.

Eventually we should track each infoframe type independently, and also
their contents. This is a small step in that direction as .infoframe_enabled()
return value could be easily turned into a bitmask.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 44 ++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 03b4759..ce595c3 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -174,10 +174,14 @@ static bool g4x_infoframe_enabled(struct drm_encoder *encoder)
 	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
 	u32 val = I915_READ(VIDEO_DIP_CTL);
 
-	if (VIDEO_DIP_PORT(intel_dig_port->port) == (val & VIDEO_DIP_PORT_MASK))
-		return val & VIDEO_DIP_ENABLE;
+	if ((val & VIDEO_DIP_ENABLE) == 0)
+		return false;
 
-	return false;
+	if ((val & VIDEO_DIP_PORT_MASK) != VIDEO_DIP_PORT(intel_dig_port->port))
+		return false;
+
+	return val & (VIDEO_DIP_ENABLE_AVI |
+		      VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);
 }
 
 static void ibx_write_infoframe(struct drm_encoder *encoder,
@@ -227,10 +231,15 @@ static bool ibx_infoframe_enabled(struct drm_encoder *encoder)
 	int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
 	u32 val = I915_READ(reg);
 
-	if (VIDEO_DIP_PORT(intel_dig_port->port) == (val & VIDEO_DIP_PORT_MASK))
-		return val & VIDEO_DIP_ENABLE;
+	if ((val & VIDEO_DIP_ENABLE) == 0)
+		return false;
 
-	return false;
+	if ((val & VIDEO_DIP_PORT_MASK) != VIDEO_DIP_PORT(intel_dig_port->port))
+		return false;
+
+	return val & (VIDEO_DIP_ENABLE_AVI |
+		      VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+		      VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
 }
 
 static void cpt_write_infoframe(struct drm_encoder *encoder,
@@ -282,7 +291,12 @@ static bool cpt_infoframe_enabled(struct drm_encoder *encoder)
 	int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
 	u32 val = I915_READ(reg);
 
-	return val & VIDEO_DIP_ENABLE;
+	if ((val & VIDEO_DIP_ENABLE) == 0)
+		return false;
+
+	return val & (VIDEO_DIP_ENABLE_AVI |
+		      VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+		      VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
 }
 
 static void vlv_write_infoframe(struct drm_encoder *encoder,
@@ -332,10 +346,15 @@ static bool vlv_infoframe_enabled(struct drm_encoder *encoder)
 	int reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
 	u32 val = I915_READ(reg);
 
-	if (VIDEO_DIP_PORT(intel_dig_port->port) == (val & VIDEO_DIP_PORT_MASK))
-		return val & VIDEO_DIP_ENABLE;
+	if ((val & VIDEO_DIP_ENABLE) == 0)
+		return false;
 
-	return false;
+	if ((val & VIDEO_DIP_PORT_MASK) != VIDEO_DIP_PORT(intel_dig_port->port))
+		return false;
+
+	return val & (VIDEO_DIP_ENABLE_AVI |
+		      VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+		      VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
 }
 
 static void hsw_write_infoframe(struct drm_encoder *encoder,
@@ -383,8 +402,9 @@ static bool hsw_infoframe_enabled(struct drm_encoder *encoder)
 	u32 ctl_reg = HSW_TVIDEO_DIP_CTL(intel_crtc->config->cpu_transcoder);
 	u32 val = I915_READ(ctl_reg);
 
-	return val & (VIDEO_DIP_ENABLE_AVI_HSW | VIDEO_DIP_ENABLE_SPD_HSW |
-		      VIDEO_DIP_ENABLE_VS_HSW);
+	return val & (VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_AVI_HSW |
+		      VIDEO_DIP_ENABLE_GCP_HSW | VIDEO_DIP_ENABLE_VS_HSW |
+		      VIDEO_DIP_ENABLE_GMP_HSW | VIDEO_DIP_ENABLE_SPD_HSW);
 }
 
 /*
-- 
2.0.5

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

[PATCH 8/9] drm/i915: Fix hdmi clock readout with pixel repeat

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Account for the pixel multiplier when reading out the HDMI
mode dotclock. Makes the state checked happier on my ILK when using
double clocked modes.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index ce595c3..1810242 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -943,6 +943,9 @@ static void intel_hdmi_get_config(struct intel_encoder *encoder,
 	else
 		dotclock = pipe_config->port_clock;
 
+	if (pipe_config->pixel_multiplier)
+		dotclock /= pipe_config->pixel_multiplier;
+
 	if (HAS_PCH_SPLIT(dev_priv->dev))
 		ironlake_check_encoder_dotclock(pipe_config, dotclock);
 
-- 
2.0.5

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

[PATCH 9/9] drm/i915: Double the port clock when using double clocked modes with 12bpc

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we're forgetting to double the port clock when using double
clocked modes with 12bpc on HDMI. We're only accounting for the 1.5x
factor due to the 12bpc. So further double the 1.5x port clock when we
have a double clocked mode.

Unfortunately I don't have any displays that support both 12bpc and
double clocked modes, so I was unable to test this.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 1810242..f3eec38 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1248,6 +1248,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 
 	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) {
 		pipe_config->pixel_multiplier = 2;
+		clock_12bpc *= 2;
 	}
 
 	if (intel_hdmi->color_range)
-- 
2.0.5

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

Re: [PATCH v2 1/9] drm/i915: Implement WaEnableHDMI8bpcBefore12bpc:snb, ivb

[Jani Nikula]

On Tue, 05 May 2015, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> CPT/PPT require a specific procedure for enabling 12bpc HDMI. Implement
> it, and to keep things neat pull the code into a function.
>
> v2: Rebased due to crtc->config changes
>     s/HDMI_GC/HDMIUNIT_GC/ to match spec better
>     Factor out intel_enable_hdmi_audio()
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h   |  1 +
>  drivers/gpu/drm/i915/intel_hdmi.c | 74 +++++++++++++++++++++++++++++++++++----
>  2 files changed, 69 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2339ffa..e619e41 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6149,6 +6149,7 @@ enum skl_disp_power_wells {
>  #define _TRANSA_CHICKEN1	 0xf0060
>  #define _TRANSB_CHICKEN1	 0xf1060
>  #define TRANS_CHICKEN1(pipe) _PIPE(pipe, _TRANSA_CHICKEN1, _TRANSB_CHICKEN1)
> +#define  TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE	(1<<10)
>  #define  TRANS_CHICKEN1_DP0UNIT_GC_DISABLE	(1<<4)
>  #define _TRANSA_CHICKEN2	 0xf0064
>  #define _TRANSB_CHICKEN2	 0xf1064
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index a84c040..79cf445 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -814,6 +814,16 @@ static void intel_hdmi_get_config(struct intel_encoder *encoder,
>  	pipe_config->base.adjusted_mode.crtc_clock = dotclock;
>  }
>  
> +static void intel_enable_hdmi_audio(struct intel_encoder *encoder)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> +
> +	WARN_ON(!crtc->config->has_hdmi_sink);
> +	DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
> +			 pipe_name(crtc->pipe));

The debug message is redundant and should've been removed with the audio
rework, as there's plenty of debug in intel_audio.c. Please just drop it
here.

With that gone, I'm not so sure if it makes sense to have a separate
function for audio enable. But up to you.

BR,
Jani.


> +	intel_audio_codec_enable(encoder);
> +}
> +
>  static void intel_enable_hdmi(struct intel_encoder *encoder)
>  {
>  	struct drm_device *dev = encoder->base.dev;
> @@ -854,12 +864,61 @@ static void intel_enable_hdmi(struct intel_encoder *encoder)
>  		POSTING_READ(intel_hdmi->hdmi_reg);
>  	}
>  
> -	if (intel_crtc->config->has_audio) {
> -		WARN_ON(!intel_crtc->config->has_hdmi_sink);
> -		DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
> -				 pipe_name(intel_crtc->pipe));
> -		intel_audio_codec_enable(encoder);
> +	if (intel_crtc->config->has_audio)
> +		intel_enable_hdmi_audio(encoder);
> +}
> +
> +static void cpt_enable_hdmi(struct intel_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> +	enum pipe pipe = crtc->pipe;
> +	u32 temp;
> +
> +	temp = I915_READ(intel_hdmi->hdmi_reg);
> +
> +	temp |= SDVO_ENABLE;
> +	if (crtc->config->has_audio)
> +		temp |= SDVO_AUDIO_ENABLE;
> +
> +	/*
> +	 * WaEnableHDMI8bpcBefore12bpc:snb,ivb
> +	 *
> +	 * The procedure for 12bpc is as follows:
> +	 * 1. disable HDMI clock gating
> +	 * 2. enable HDMI with 8bpc
> +	 * 3. enable HDMI with 12bpc
> +	 * 4. enable HDMI clock gating
> +	 */
> +
> +	if (crtc->config->pipe_bpp > 24) {
> +		I915_WRITE(TRANS_CHICKEN1(pipe),
> +			   I915_READ(TRANS_CHICKEN1(pipe)) |
> +			   TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
> +
> +		temp &= ~SDVO_COLOR_FORMAT_MASK;
> +		temp |= SDVO_COLOR_FORMAT_8bpc;
>  	}
> +
> +	I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +	POSTING_READ(intel_hdmi->hdmi_reg);
> +
> +	if (crtc->config->pipe_bpp > 24) {
> +		temp &= ~SDVO_COLOR_FORMAT_MASK;
> +		temp |= HDMI_COLOR_FORMAT_12bpc;
> +
> +		I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +		POSTING_READ(intel_hdmi->hdmi_reg);
> +
> +		I915_WRITE(TRANS_CHICKEN1(pipe),
> +			   I915_READ(TRANS_CHICKEN1(pipe)) &
> +			   ~TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
> +	}
> +
> +	if (crtc->config->has_audio)
> +		intel_enable_hdmi_audio(encoder);
>  }
>  
>  static void vlv_enable_hdmi(struct intel_encoder *encoder)
> @@ -1829,7 +1888,10 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
>  		intel_encoder->post_disable = vlv_hdmi_post_disable;
>  	} else {
>  		intel_encoder->pre_enable = intel_hdmi_pre_enable;
> -		intel_encoder->enable = intel_enable_hdmi;
> +		if (HAS_PCH_CPT(dev))
> +			intel_encoder->enable = cpt_enable_hdmi;
> +		else
> +			intel_encoder->enable = intel_enable_hdmi;
>  	}
>  
>  	intel_encoder->type = INTEL_OUTPUT_HDMI;
> -- 
> 2.0.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 9/9] drm/i915: Double the port clock when using double clocked modes with 12bpc

[Ville Syrjälä]

On Tue, May 05, 2015 at 05:06:27PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we're forgetting to double the port clock when using double
> clocked modes with 12bpc on HDMI. We're only accounting for the 1.5x
> factor due to the 12bpc. So further double the 1.5x port clock when we
> have a double clocked mode.
> 
> Unfortunately I don't have any displays that support both 12bpc and
> double clocked modes, so I was unable to test this.

Actually I was able to test this by manually adding some CEA double
clocked modes. My TV happily accepted them even if it didn't list them
in the EDID.

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 1810242..f3eec38 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1248,6 +1248,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) {
>  		pipe_config->pixel_multiplier = 2;
> +		clock_12bpc *= 2;
>  	}
>  
>  	if (intel_hdmi->color_range)
> -- 
> 2.0.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 1/9] drm/i915: Implement WaEnableHDMI8bpcBefore12bpc:snb, ivb

[Ander Conselvan De Oliveira]

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

On Tue, 2015-05-05 at 17:06 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> CPT/PPT require a specific procedure for enabling 12bpc HDMI. Implement
> it, and to keep things neat pull the code into a function.
> 
> v2: Rebased due to crtc->config changes
>     s/HDMI_GC/HDMIUNIT_GC/ to match spec better
>     Factor out intel_enable_hdmi_audio()
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h   |  1 +
>  drivers/gpu/drm/i915/intel_hdmi.c | 74 +++++++++++++++++++++++++++++++++++----
>  2 files changed, 69 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2339ffa..e619e41 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6149,6 +6149,7 @@ enum skl_disp_power_wells {
>  #define _TRANSA_CHICKEN1	 0xf0060
>  #define _TRANSB_CHICKEN1	 0xf1060
>  #define TRANS_CHICKEN1(pipe) _PIPE(pipe, _TRANSA_CHICKEN1, _TRANSB_CHICKEN1)
> +#define  TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE	(1<<10)
>  #define  TRANS_CHICKEN1_DP0UNIT_GC_DISABLE	(1<<4)
>  #define _TRANSA_CHICKEN2	 0xf0064
>  #define _TRANSB_CHICKEN2	 0xf1064
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index a84c040..79cf445 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -814,6 +814,16 @@ static void intel_hdmi_get_config(struct intel_encoder *encoder,
>  	pipe_config->base.adjusted_mode.crtc_clock = dotclock;
>  }
>  
> +static void intel_enable_hdmi_audio(struct intel_encoder *encoder)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> +
> +	WARN_ON(!crtc->config->has_hdmi_sink);
> +	DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
> +			 pipe_name(crtc->pipe));
> +	intel_audio_codec_enable(encoder);
> +}
> +
>  static void intel_enable_hdmi(struct intel_encoder *encoder)
>  {
>  	struct drm_device *dev = encoder->base.dev;
> @@ -854,12 +864,61 @@ static void intel_enable_hdmi(struct intel_encoder *encoder)
>  		POSTING_READ(intel_hdmi->hdmi_reg);
>  	}
>  
> -	if (intel_crtc->config->has_audio) {
> -		WARN_ON(!intel_crtc->config->has_hdmi_sink);
> -		DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
> -				 pipe_name(intel_crtc->pipe));
> -		intel_audio_codec_enable(encoder);
> +	if (intel_crtc->config->has_audio)
> +		intel_enable_hdmi_audio(encoder);
> +}
> +
> +static void cpt_enable_hdmi(struct intel_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> +	enum pipe pipe = crtc->pipe;
> +	u32 temp;
> +
> +	temp = I915_READ(intel_hdmi->hdmi_reg);
> +
> +	temp |= SDVO_ENABLE;
> +	if (crtc->config->has_audio)
> +		temp |= SDVO_AUDIO_ENABLE;
> +
> +	/*
> +	 * WaEnableHDMI8bpcBefore12bpc:snb,ivb
> +	 *
> +	 * The procedure for 12bpc is as follows:
> +	 * 1. disable HDMI clock gating
> +	 * 2. enable HDMI with 8bpc
> +	 * 3. enable HDMI with 12bpc
> +	 * 4. enable HDMI clock gating
> +	 */
> +
> +	if (crtc->config->pipe_bpp > 24) {
> +		I915_WRITE(TRANS_CHICKEN1(pipe),
> +			   I915_READ(TRANS_CHICKEN1(pipe)) |
> +			   TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
> +
> +		temp &= ~SDVO_COLOR_FORMAT_MASK;
> +		temp |= SDVO_COLOR_FORMAT_8bpc;
>  	}
> +
> +	I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +	POSTING_READ(intel_hdmi->hdmi_reg);
> +
> +	if (crtc->config->pipe_bpp > 24) {
> +		temp &= ~SDVO_COLOR_FORMAT_MASK;
> +		temp |= HDMI_COLOR_FORMAT_12bpc;
> +
> +		I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +		POSTING_READ(intel_hdmi->hdmi_reg);
> +
> +		I915_WRITE(TRANS_CHICKEN1(pipe),
> +			   I915_READ(TRANS_CHICKEN1(pipe)) &
> +			   ~TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
> +	}
> +
> +	if (crtc->config->has_audio)
> +		intel_enable_hdmi_audio(encoder);
>  }
>  
>  static void vlv_enable_hdmi(struct intel_encoder *encoder)
> @@ -1829,7 +1888,10 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
>  		intel_encoder->post_disable = vlv_hdmi_post_disable;
>  	} else {
>  		intel_encoder->pre_enable = intel_hdmi_pre_enable;
> -		intel_encoder->enable = intel_enable_hdmi;
> +		if (HAS_PCH_CPT(dev))
> +			intel_encoder->enable = cpt_enable_hdmi;
> +		else
> +			intel_encoder->enable = intel_enable_hdmi;
>  	}
>  
>  	intel_encoder->type = INTEL_OUTPUT_HDMI;


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

Re: [PATCH v2 2/9] drm/i915: Send GCP infoframes for deep color HDMI sinks

[Ander Conselvan De Oliveira]

On Tue, 2015-05-05 at 17:06 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> GCP infoframes are required to inform the HDMI sink about the color
> depth.
> 
> Send the GCP infoframe whenever the sink supports any deep color modes
> since such sinks must anyway be capable of receiving them. For sinks
> that don't support deep color let's skip the GCP in case it might
> confuse the sink, although HDMI 1.4 spec does say all sinks must be
> capable of reciving them. In theory we could skip the GCP infoframe
> for deep color sinks in 8bpc mode as well since sinks must fall back to
> 8bpc whenever GCP isn't received for some time.
> 
> BSpec says we should disable GCP after disabling the port, so do that as
> well.
> 
> v2: s/intel_set_gcp_infoframe/intel_hdmi_set_gcp_infoframe/
>     Rebased due to crtc->config changes
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h   |  3 ++
>  drivers/gpu/drm/i915/intel_hdmi.c | 74 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 77 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e619e41..dcd93b5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6010,6 +6010,9 @@ enum skl_disp_power_wells {
>  #define _VIDEO_DIP_CTL_A         0xe0200
>  #define _VIDEO_DIP_DATA_A        0xe0208
>  #define _VIDEO_DIP_GCP_A         0xe0210
> +#define  GCP_COLOR_INDICATION		(1 << 2)
> +#define  GCP_DEFAULT_PHASE_ENABLE	(1 << 1)
> +#define  GCP_AV_MUTE			(1 << 0)
>  
>  #define _VIDEO_DIP_CTL_B         0xe1200
>  #define _VIDEO_DIP_DATA_B        0xe1208
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 79cf445..87c4905 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -541,6 +541,66 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
>  	intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode);
>  }
>  
> +static bool hdmi_sink_is_deep_color(struct drm_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->dev;
> +	struct drm_connector *connector;
> +
> +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> +
> +	/*
> +	 * HDMI cloning is only supported on g4x which doesn't
> +	 * support deep color or GCP infoframes anyway so no
> +	 * need to worry about multiple HDMI sinks here.
> +	 */
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> +		if (connector->encoder == encoder)
> +			return connector->display_info.bpc > 8;
> +
> +	return false;
> +}
> +
> +static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
> +	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> +	u32 reg, val = 0;
> +
> +	if (HAS_DDI(dev_priv))
> +		reg = HSW_TVIDEO_DIP_GCP(crtc->config->cpu_transcoder);
> +	else if (IS_VALLEYVIEW(dev_priv))
> +		reg = VLV_TVIDEO_DIP_GCP(crtc->pipe);
> +	else if (HAS_PCH_SPLIT(dev_priv->dev))
> +		reg = TVIDEO_DIP_GCP(crtc->pipe);
> +	else
> +		return false;
> +
> +	/* Indicate color depth wheneven the sink supports deep color */
> +	if (hdmi_sink_is_deep_color(encoder))
> +		val |= GCP_COLOR_INDICATION;
> +
> +	I915_WRITE(reg, val);
> +
> +	return val != 0;
> +}
> +
> +static void intel_disable_gcp_infoframe(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	u32 reg;
> +
> +	if (HAS_DDI(dev_priv))
> +		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> +	else if (IS_VALLEYVIEW(dev_priv))
> +		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
> +	else if (HAS_PCH_SPLIT(dev_priv->dev))
> +		reg = TVIDEO_DIP_CTL(crtc->pipe);
> +	else
> +		return;
> +
> +	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP);
> +}
> +
>  static void ibx_set_infoframes(struct drm_encoder *encoder,
>  			       bool enable,
>  			       struct drm_display_mode *adjusted_mode)
> @@ -581,6 +641,9 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
>  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
>  		 VIDEO_DIP_ENABLE_GCP);
>  
> +	if (intel_hdmi_set_gcp_infoframe(encoder))
> +		val |= VIDEO_DIP_ENABLE_GCP;
> +
>  	I915_WRITE(reg, val);
>  	POSTING_READ(reg);
>  
> @@ -618,6 +681,9 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
>  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
>  		 VIDEO_DIP_ENABLE_GCP);
>  
> +	if (intel_hdmi_set_gcp_infoframe(encoder))
> +		val |= VIDEO_DIP_ENABLE_GCP;
> +
>  	I915_WRITE(reg, val);
>  	POSTING_READ(reg);
>  
> @@ -666,6 +732,9 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
>  	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
>  		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
>  
> +	if (intel_hdmi_set_gcp_infoframe(encoder))
> +		val |= VIDEO_DIP_ENABLE_GCP;
> +
>  	I915_WRITE(reg, val);
>  	POSTING_READ(reg);
>  
> @@ -695,6 +764,9 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
>  	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_GCP_HSW |
>  		 VIDEO_DIP_ENABLE_VS_HSW | VIDEO_DIP_ENABLE_GMP_HSW);
>  
> +	if (intel_hdmi_set_gcp_infoframe(encoder))
> +		val |= VIDEO_DIP_ENABLE_GCP_HSW;
> +
>  	I915_WRITE(reg, val);
>  	POSTING_READ(reg);
>  
> @@ -986,6 +1058,8 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>  
>  	if (IS_CHERRYVIEW(dev))
>  		chv_powergate_phy_lanes(encoder, 0xf);
> +
> +	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));

BSpec says this should be disabled after disabling TRANS_DDI_FUNC_CTL,
so shouldn't this go in post_disable?

Ander

>  }
>  
>  static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)


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

Re: [PATCH v2 2/9] drm/i915: Send GCP infoframes for deep color HDMI sinks

[Ville Syrjälä]

On Mon, May 25, 2015 at 03:32:52PM +0300, Ander Conselvan De Oliveira wrote:
> On Tue, 2015-05-05 at 17:06 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > GCP infoframes are required to inform the HDMI sink about the color
> > depth.
> > 
> > Send the GCP infoframe whenever the sink supports any deep color modes
> > since such sinks must anyway be capable of receiving them. For sinks
> > that don't support deep color let's skip the GCP in case it might
> > confuse the sink, although HDMI 1.4 spec does say all sinks must be
> > capable of reciving them. In theory we could skip the GCP infoframe
> > for deep color sinks in 8bpc mode as well since sinks must fall back to
> > 8bpc whenever GCP isn't received for some time.
> > 
> > BSpec says we should disable GCP after disabling the port, so do that as
> > well.
> > 
> > v2: s/intel_set_gcp_infoframe/intel_hdmi_set_gcp_infoframe/
> >     Rebased due to crtc->config changes
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h   |  3 ++
> >  drivers/gpu/drm/i915/intel_hdmi.c | 74 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 77 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index e619e41..dcd93b5 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6010,6 +6010,9 @@ enum skl_disp_power_wells {
> >  #define _VIDEO_DIP_CTL_A         0xe0200
> >  #define _VIDEO_DIP_DATA_A        0xe0208
> >  #define _VIDEO_DIP_GCP_A         0xe0210
> > +#define  GCP_COLOR_INDICATION		(1 << 2)
> > +#define  GCP_DEFAULT_PHASE_ENABLE	(1 << 1)
> > +#define  GCP_AV_MUTE			(1 << 0)
> >  
> >  #define _VIDEO_DIP_CTL_B         0xe1200
> >  #define _VIDEO_DIP_DATA_B        0xe1208
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 79cf445..87c4905 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -541,6 +541,66 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
> >  	intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode);
> >  }
> >  
> > +static bool hdmi_sink_is_deep_color(struct drm_encoder *encoder)
> > +{
> > +	struct drm_device *dev = encoder->dev;
> > +	struct drm_connector *connector;
> > +
> > +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> > +
> > +	/*
> > +	 * HDMI cloning is only supported on g4x which doesn't
> > +	 * support deep color or GCP infoframes anyway so no
> > +	 * need to worry about multiple HDMI sinks here.
> > +	 */
> > +	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> > +		if (connector->encoder == encoder)
> > +			return connector->display_info.bpc > 8;
> > +
> > +	return false;
> > +}
> > +
> > +static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)
> > +{
> > +	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
> > +	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> > +	u32 reg, val = 0;
> > +
> > +	if (HAS_DDI(dev_priv))
> > +		reg = HSW_TVIDEO_DIP_GCP(crtc->config->cpu_transcoder);
> > +	else if (IS_VALLEYVIEW(dev_priv))
> > +		reg = VLV_TVIDEO_DIP_GCP(crtc->pipe);
> > +	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > +		reg = TVIDEO_DIP_GCP(crtc->pipe);
> > +	else
> > +		return false;
> > +
> > +	/* Indicate color depth wheneven the sink supports deep color */
> > +	if (hdmi_sink_is_deep_color(encoder))
> > +		val |= GCP_COLOR_INDICATION;
> > +
> > +	I915_WRITE(reg, val);
> > +
> > +	return val != 0;
> > +}
> > +
> > +static void intel_disable_gcp_infoframe(struct intel_crtc *crtc)
> > +{
> > +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > +	u32 reg;
> > +
> > +	if (HAS_DDI(dev_priv))
> > +		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> > +	else if (IS_VALLEYVIEW(dev_priv))
> > +		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
> > +	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > +		reg = TVIDEO_DIP_CTL(crtc->pipe);
> > +	else
> > +		return;
> > +
> > +	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP);
> > +}
> > +
> >  static void ibx_set_infoframes(struct drm_encoder *encoder,
> >  			       bool enable,
> >  			       struct drm_display_mode *adjusted_mode)
> > @@ -581,6 +641,9 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
> >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> >  		 VIDEO_DIP_ENABLE_GCP);
> >  
> > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > +		val |= VIDEO_DIP_ENABLE_GCP;
> > +
> >  	I915_WRITE(reg, val);
> >  	POSTING_READ(reg);
> >  
> > @@ -618,6 +681,9 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
> >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> >  		 VIDEO_DIP_ENABLE_GCP);
> >  
> > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > +		val |= VIDEO_DIP_ENABLE_GCP;
> > +
> >  	I915_WRITE(reg, val);
> >  	POSTING_READ(reg);
> >  
> > @@ -666,6 +732,9 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
> >  	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
> >  		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
> >  
> > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > +		val |= VIDEO_DIP_ENABLE_GCP;
> > +
> >  	I915_WRITE(reg, val);
> >  	POSTING_READ(reg);
> >  
> > @@ -695,6 +764,9 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
> >  	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_GCP_HSW |
> >  		 VIDEO_DIP_ENABLE_VS_HSW | VIDEO_DIP_ENABLE_GMP_HSW);
> >  
> > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > +		val |= VIDEO_DIP_ENABLE_GCP_HSW;
> > +
> >  	I915_WRITE(reg, val);
> >  	POSTING_READ(reg);
> >  
> > @@ -986,6 +1058,8 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
> >  
> >  	if (IS_CHERRYVIEW(dev))
> >  		chv_powergate_phy_lanes(encoder, 0xf);
> > +
> > +	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));
> 
> BSpec says this should be disabled after disabling TRANS_DDI_FUNC_CTL,
> so shouldn't this go in post_disable?

intel_disable_hdmi() isn't used on DDI platforms. I've not looked at the
DDI code too much, but I expect it could have similar issues with the
disable sequence like the earlier PCH platforms had.

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

Re: [PATCH v2 2/9] drm/i915: Send GCP infoframes for deep color HDMI sinks

[Ander Conselvan De Oliveira]

On Mon, 2015-05-25 at 15:44 +0300, Ville Syrjälä wrote:
> On Mon, May 25, 2015 at 03:32:52PM +0300, Ander Conselvan De Oliveira wrote:
> > On Tue, 2015-05-05 at 17:06 +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > GCP infoframes are required to inform the HDMI sink about the color
> > > depth.
> > > 
> > > Send the GCP infoframe whenever the sink supports any deep color modes
> > > since such sinks must anyway be capable of receiving them. For sinks
> > > that don't support deep color let's skip the GCP in case it might
> > > confuse the sink, although HDMI 1.4 spec does say all sinks must be
> > > capable of reciving them. In theory we could skip the GCP infoframe
> > > for deep color sinks in 8bpc mode as well since sinks must fall back to
> > > 8bpc whenever GCP isn't received for some time.
> > > 
> > > BSpec says we should disable GCP after disabling the port, so do that as
> > > well.
> > > 
> > > v2: s/intel_set_gcp_infoframe/intel_hdmi_set_gcp_infoframe/
> > >     Rebased due to crtc->config changes
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h   |  3 ++
> > >  drivers/gpu/drm/i915/intel_hdmi.c | 74 +++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 77 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index e619e41..dcd93b5 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -6010,6 +6010,9 @@ enum skl_disp_power_wells {
> > >  #define _VIDEO_DIP_CTL_A         0xe0200
> > >  #define _VIDEO_DIP_DATA_A        0xe0208
> > >  #define _VIDEO_DIP_GCP_A         0xe0210
> > > +#define  GCP_COLOR_INDICATION		(1 << 2)
> > > +#define  GCP_DEFAULT_PHASE_ENABLE	(1 << 1)
> > > +#define  GCP_AV_MUTE			(1 << 0)
> > >  
> > >  #define _VIDEO_DIP_CTL_B         0xe1200
> > >  #define _VIDEO_DIP_DATA_B        0xe1208
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index 79cf445..87c4905 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -541,6 +541,66 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
> > >  	intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode);
> > >  }
> > >  
> > > +static bool hdmi_sink_is_deep_color(struct drm_encoder *encoder)
> > > +{
> > > +	struct drm_device *dev = encoder->dev;
> > > +	struct drm_connector *connector;
> > > +
> > > +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> > > +
> > > +	/*
> > > +	 * HDMI cloning is only supported on g4x which doesn't
> > > +	 * support deep color or GCP infoframes anyway so no
> > > +	 * need to worry about multiple HDMI sinks here.
> > > +	 */
> > > +	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> > > +		if (connector->encoder == encoder)
> > > +			return connector->display_info.bpc > 8;
> > > +
> > > +	return false;
> > > +}
> > > +
> > > +static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)
> > > +{
> > > +	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
> > > +	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> > > +	u32 reg, val = 0;
> > > +
> > > +	if (HAS_DDI(dev_priv))
> > > +		reg = HSW_TVIDEO_DIP_GCP(crtc->config->cpu_transcoder);
> > > +	else if (IS_VALLEYVIEW(dev_priv))
> > > +		reg = VLV_TVIDEO_DIP_GCP(crtc->pipe);
> > > +	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > > +		reg = TVIDEO_DIP_GCP(crtc->pipe);
> > > +	else
> > > +		return false;
> > > +
> > > +	/* Indicate color depth wheneven the sink supports deep color */
> > > +	if (hdmi_sink_is_deep_color(encoder))
> > > +		val |= GCP_COLOR_INDICATION;
> > > +
> > > +	I915_WRITE(reg, val);
> > > +
> > > +	return val != 0;
> > > +}
> > > +
> > > +static void intel_disable_gcp_infoframe(struct intel_crtc *crtc)
> > > +{
> > > +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > > +	u32 reg;
> > > +
> > > +	if (HAS_DDI(dev_priv))
> > > +		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> > > +	else if (IS_VALLEYVIEW(dev_priv))
> > > +		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
> > > +	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > > +		reg = TVIDEO_DIP_CTL(crtc->pipe);
> > > +	else
> > > +		return;
> > > +
> > > +	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP);
> > > +}
> > > +
> > >  static void ibx_set_infoframes(struct drm_encoder *encoder,
> > >  			       bool enable,
> > >  			       struct drm_display_mode *adjusted_mode)
> > > @@ -581,6 +641,9 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
> > >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > >  		 VIDEO_DIP_ENABLE_GCP);
> > >  
> > > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > +		val |= VIDEO_DIP_ENABLE_GCP;
> > > +
> > >  	I915_WRITE(reg, val);
> > >  	POSTING_READ(reg);
> > >  
> > > @@ -618,6 +681,9 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
> > >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > >  		 VIDEO_DIP_ENABLE_GCP);
> > >  
> > > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > +		val |= VIDEO_DIP_ENABLE_GCP;
> > > +
> > >  	I915_WRITE(reg, val);
> > >  	POSTING_READ(reg);
> > >  
> > > @@ -666,6 +732,9 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
> > >  	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
> > >  		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
> > >  
> > > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > +		val |= VIDEO_DIP_ENABLE_GCP;
> > > +
> > >  	I915_WRITE(reg, val);
> > >  	POSTING_READ(reg);
> > >  
> > > @@ -695,6 +764,9 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
> > >  	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_GCP_HSW |
> > >  		 VIDEO_DIP_ENABLE_VS_HSW | VIDEO_DIP_ENABLE_GMP_HSW);
> > >  
> > > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > +		val |= VIDEO_DIP_ENABLE_GCP_HSW;
> > > +
> > >  	I915_WRITE(reg, val);
> > >  	POSTING_READ(reg);
> > >  
> > > @@ -986,6 +1058,8 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
> > >  
> > >  	if (IS_CHERRYVIEW(dev))
> > >  		chv_powergate_phy_lanes(encoder, 0xf);
> > > +
> > > +	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));
> > 
> > BSpec says this should be disabled after disabling TRANS_DDI_FUNC_CTL,
> > so shouldn't this go in post_disable?
> 
> intel_disable_hdmi() isn't used on DDI platforms. I've not looked at the
> DDI code too much, but I expect it could have similar issues with the
> disable sequence like the earlier PCH platforms had.

Ah, right. So if the GCP would be disabled that would only be done in
->set_infoframes() called from ->pre_enable(), that is called while
TRANS_DDI_FUNC_CTL is disabled. So no issue there.

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>



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

Re: [PATCH v2 2/9] drm/i915: Send GCP infoframes for deep color HDMI sinks

[Ville Syrjälä]

On Mon, May 25, 2015 at 04:09:57PM +0300, Ander Conselvan De Oliveira wrote:
> On Mon, 2015-05-25 at 15:44 +0300, Ville Syrjälä wrote:
> > On Mon, May 25, 2015 at 03:32:52PM +0300, Ander Conselvan De Oliveira wrote:
> > > On Tue, 2015-05-05 at 17:06 +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > GCP infoframes are required to inform the HDMI sink about the color
> > > > depth.
> > > > 
> > > > Send the GCP infoframe whenever the sink supports any deep color modes
> > > > since such sinks must anyway be capable of receiving them. For sinks
> > > > that don't support deep color let's skip the GCP in case it might
> > > > confuse the sink, although HDMI 1.4 spec does say all sinks must be
> > > > capable of reciving them. In theory we could skip the GCP infoframe
> > > > for deep color sinks in 8bpc mode as well since sinks must fall back to
> > > > 8bpc whenever GCP isn't received for some time.
> > > > 
> > > > BSpec says we should disable GCP after disabling the port, so do that as
> > > > well.
> > > > 
> > > > v2: s/intel_set_gcp_infoframe/intel_hdmi_set_gcp_infoframe/
> > > >     Rebased due to crtc->config changes
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_reg.h   |  3 ++
> > > >  drivers/gpu/drm/i915/intel_hdmi.c | 74 +++++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 77 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index e619e41..dcd93b5 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -6010,6 +6010,9 @@ enum skl_disp_power_wells {
> > > >  #define _VIDEO_DIP_CTL_A         0xe0200
> > > >  #define _VIDEO_DIP_DATA_A        0xe0208
> > > >  #define _VIDEO_DIP_GCP_A         0xe0210
> > > > +#define  GCP_COLOR_INDICATION		(1 << 2)
> > > > +#define  GCP_DEFAULT_PHASE_ENABLE	(1 << 1)
> > > > +#define  GCP_AV_MUTE			(1 << 0)
> > > >  
> > > >  #define _VIDEO_DIP_CTL_B         0xe1200
> > > >  #define _VIDEO_DIP_DATA_B        0xe1208
> > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > index 79cf445..87c4905 100644
> > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > @@ -541,6 +541,66 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
> > > >  	intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode);
> > > >  }
> > > >  
> > > > +static bool hdmi_sink_is_deep_color(struct drm_encoder *encoder)
> > > > +{
> > > > +	struct drm_device *dev = encoder->dev;
> > > > +	struct drm_connector *connector;
> > > > +
> > > > +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> > > > +
> > > > +	/*
> > > > +	 * HDMI cloning is only supported on g4x which doesn't
> > > > +	 * support deep color or GCP infoframes anyway so no
> > > > +	 * need to worry about multiple HDMI sinks here.
> > > > +	 */
> > > > +	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> > > > +		if (connector->encoder == encoder)
> > > > +			return connector->display_info.bpc > 8;
> > > > +
> > > > +	return false;
> > > > +}
> > > > +
> > > > +static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
> > > > +	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> > > > +	u32 reg, val = 0;
> > > > +
> > > > +	if (HAS_DDI(dev_priv))
> > > > +		reg = HSW_TVIDEO_DIP_GCP(crtc->config->cpu_transcoder);
> > > > +	else if (IS_VALLEYVIEW(dev_priv))
> > > > +		reg = VLV_TVIDEO_DIP_GCP(crtc->pipe);
> > > > +	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > > > +		reg = TVIDEO_DIP_GCP(crtc->pipe);
> > > > +	else
> > > > +		return false;
> > > > +
> > > > +	/* Indicate color depth wheneven the sink supports deep color */
> > > > +	if (hdmi_sink_is_deep_color(encoder))
> > > > +		val |= GCP_COLOR_INDICATION;
> > > > +
> > > > +	I915_WRITE(reg, val);
> > > > +
> > > > +	return val != 0;
> > > > +}
> > > > +
> > > > +static void intel_disable_gcp_infoframe(struct intel_crtc *crtc)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > > > +	u32 reg;
> > > > +
> > > > +	if (HAS_DDI(dev_priv))
> > > > +		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> > > > +	else if (IS_VALLEYVIEW(dev_priv))
> > > > +		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
> > > > +	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > > > +		reg = TVIDEO_DIP_CTL(crtc->pipe);
> > > > +	else
> > > > +		return;
> > > > +
> > > > +	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP);
> > > > +}
> > > > +
> > > >  static void ibx_set_infoframes(struct drm_encoder *encoder,
> > > >  			       bool enable,
> > > >  			       struct drm_display_mode *adjusted_mode)
> > > > @@ -581,6 +641,9 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
> > > >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > >  		 VIDEO_DIP_ENABLE_GCP);
> > > >  
> > > > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > > +		val |= VIDEO_DIP_ENABLE_GCP;
> > > > +
> > > >  	I915_WRITE(reg, val);
> > > >  	POSTING_READ(reg);
> > > >  
> > > > @@ -618,6 +681,9 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
> > > >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > >  		 VIDEO_DIP_ENABLE_GCP);
> > > >  
> > > > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > > +		val |= VIDEO_DIP_ENABLE_GCP;
> > > > +
> > > >  	I915_WRITE(reg, val);
> > > >  	POSTING_READ(reg);
> > > >  
> > > > @@ -666,6 +732,9 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
> > > >  	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
> > > >  		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
> > > >  
> > > > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > > +		val |= VIDEO_DIP_ENABLE_GCP;
> > > > +
> > > >  	I915_WRITE(reg, val);
> > > >  	POSTING_READ(reg);
> > > >  
> > > > @@ -695,6 +764,9 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
> > > >  	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_GCP_HSW |
> > > >  		 VIDEO_DIP_ENABLE_VS_HSW | VIDEO_DIP_ENABLE_GMP_HSW);
> > > >  
> > > > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > > +		val |= VIDEO_DIP_ENABLE_GCP_HSW;
> > > > +
> > > >  	I915_WRITE(reg, val);
> > > >  	POSTING_READ(reg);
> > > >  
> > > > @@ -986,6 +1058,8 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
> > > >  
> > > >  	if (IS_CHERRYVIEW(dev))
> > > >  		chv_powergate_phy_lanes(encoder, 0xf);
> > > > +
> > > > +	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));
> > > 
> > > BSpec says this should be disabled after disabling TRANS_DDI_FUNC_CTL,
> > > so shouldn't this go in post_disable?
> > 
> > intel_disable_hdmi() isn't used on DDI platforms. I've not looked at the
> > DDI code too much, but I expect it could have similar issues with the
> > disable sequence like the earlier PCH platforms had.
> 
> Ah, right. So if the GCP would be disabled that would only be done in
> ->set_infoframes() called from ->pre_enable(), that is called while
> TRANS_DDI_FUNC_CTL is disabled. So no issue there.

Perhaps. As stated I didn't really look at DDI. I do think we should
change it do things the same way as everyone else, but for the time
being I have enough things on my plate so I'll not take on another
project.

> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> 
> 

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

Re: [PATCH 0/9] drm/i915: HDMI 12bpc fixes

[Konduru, Chandra]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> ville.syrjala@linux.intel.com
> Sent: Tuesday, May 05, 2015 7:06 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 0/9] drm/i915: HDMI 12bpc fixes
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Our HDMI 12bpc support has always been broken. This series aims to fix that.
> 
> The problems addressed include:
> - missing GCP infoframes entirely
> - IBX w/a code was mostly nonsense
> - missing w/a for CPT/PPT
> - 12bpc vs. DBLCLK was busted
> 
> Part of this was already posted [1] quite a while ago, but it's grown some new
> stuff since. The entire series is available in git [2].
> 
> I have additional stuff to fix the IBX transcoder B workarounds and some other
> random thigns on PCH platforms. I'll post that as a followup.
> 
> [1] http://lists.freedesktop.org/archives/intel-gfx/2014-October/053340.html
> [2] hit://github.com/vsyrjala/linux.git hdmi_12bpc_fixes_9

Broken link [2]. Some typo?

> 
> Ville Syrjälä (9):
>   drm/i915: Implement WaEnableHDMI8bpcBefore12bpc:snb,ivb
>   drm/i915: Send GCP infoframes for deep color HDMI sinks
>   drm/i915: Enable default_phase in GCP when possible
>   drm/i915: Fix HDMI 12bpc TRANSCONF bpc value
>   drm/i915: Fix 12bpc HDMI enable for IBX
>   drm/i915: Disable all infoframes when turning off the HDMI port
>   drm/i915: Check infoframe state more diligently.
>   drm/i915: Fix hdmi clock readout with pixel repeat
>   drm/i915: Double the port clock when using double clocked modes with
>     12bpc
> 
>  drivers/gpu/drm/i915/i915_reg.h      |   4 +
>  drivers/gpu/drm/i915/intel_display.c |  10 +-
>  drivers/gpu/drm/i915/intel_hdmi.c    | 359 ++++++++++++++++++++++++++++---
> ----
>  3 files changed, 306 insertions(+), 67 deletions(-)
> 
> --
> 2.0.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/9] drm/i915: Fix HDMI 12bpc TRANSCONF bpc value

[Konduru, Chandra]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> ville.syrjala@linux.intel.com
> Sent: Tuesday, May 05, 2015 7:06 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH v2 4/9] drm/i915: Fix HDMI 12bpc TRANSCONF bpc
> value
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> IBX BSpec says we must specify 8bpc in TRANSCONF for both 8bpc and 12bpc
> HDMI output. Do so.
> 
> v2: Pass intel_crtc to intel_pipe_has_type()
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 96ce8f6..a392188 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2001,11 +2001,15 @@ static void ironlake_enable_pch_transcoder(struct
> drm_i915_private *dev_priv,
> 
>  	if (HAS_PCH_IBX(dev_priv->dev)) {
>  		/*
> -		 * make the BPC in transcoder be consistent with
> -		 * that in pipeconf reg.
> +		 * Make the BPC in transcoder be consistent with
> +		 * that in pipeconf reg. For HDMI we must use 8bpc
> +		 * here for both 8bpc and 12bpc.
>  		 */
>  		val &= ~PIPECONF_BPC_MASK;
> -		val |= pipeconf_val & PIPECONF_BPC_MASK;
> +		if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_HDMI))
> +			val |= PIPECONF_8BPC;
> +		else
> +			val |= pipeconf_val & PIPECONF_BPC_MASK;
>  	}
> 
>  	val &= ~TRANS_INTERLACE_MASK;
> --

Reviewed by: Chandra Konduru <Chandra.konduru@intel.com>

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

Re: [PATCH v2 1/9] drm/i915: Implement WaEnableHDMI8bpcBefore12bpc:snb, ivb

[Konduru, Chandra]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> ville.syrjala@linux.intel.com
> Sent: Tuesday, May 05, 2015 7:06 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH v2 1/9] drm/i915: Implement
> WaEnableHDMI8bpcBefore12bpc:snb, ivb
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> CPT/PPT require a specific procedure for enabling 12bpc HDMI. Implement it,
> and to keep things neat pull the code into a function.
> 
> v2: Rebased due to crtc->config changes
>     s/HDMI_GC/HDMIUNIT_GC/ to match spec better
>     Factor out intel_enable_hdmi_audio()
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Please call out any existing or new i-g-tests (or other tests) testing this feature.
One that is done, 
Reviewed-By: Chandra Konduru <Chandra.konduru@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_reg.h   |  1 +
>  drivers/gpu/drm/i915/intel_hdmi.c | 74
> +++++++++++++++++++++++++++++++++++----
>  2 files changed, 69 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2339ffa..e619e41 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6149,6 +6149,7 @@ enum skl_disp_power_wells {
>  #define _TRANSA_CHICKEN1	 0xf0060
>  #define _TRANSB_CHICKEN1	 0xf1060
>  #define TRANS_CHICKEN1(pipe) _PIPE(pipe, _TRANSA_CHICKEN1,
> _TRANSB_CHICKEN1)
> +#define  TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE	(1<<10)
>  #define  TRANS_CHICKEN1_DP0UNIT_GC_DISABLE	(1<<4)
>  #define _TRANSA_CHICKEN2	 0xf0064
>  #define _TRANSB_CHICKEN2	 0xf1064
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index a84c040..79cf445 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -814,6 +814,16 @@ static void intel_hdmi_get_config(struct intel_encoder
> *encoder,
>  	pipe_config->base.adjusted_mode.crtc_clock = dotclock;  }
> 
> +static void intel_enable_hdmi_audio(struct intel_encoder *encoder) {
> +	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> +
> +	WARN_ON(!crtc->config->has_hdmi_sink);
> +	DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
> +			 pipe_name(crtc->pipe));
> +	intel_audio_codec_enable(encoder);
> +}
> +
>  static void intel_enable_hdmi(struct intel_encoder *encoder)  {
>  	struct drm_device *dev = encoder->base.dev; @@ -854,12 +864,61 @@
> static void intel_enable_hdmi(struct intel_encoder *encoder)
>  		POSTING_READ(intel_hdmi->hdmi_reg);
>  	}
> 
> -	if (intel_crtc->config->has_audio) {
> -		WARN_ON(!intel_crtc->config->has_hdmi_sink);
> -		DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
> -				 pipe_name(intel_crtc->pipe));
> -		intel_audio_codec_enable(encoder);
> +	if (intel_crtc->config->has_audio)
> +		intel_enable_hdmi_audio(encoder);
> +}
> +
> +static void cpt_enable_hdmi(struct intel_encoder *encoder) {
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> +	enum pipe pipe = crtc->pipe;
> +	u32 temp;
> +
> +	temp = I915_READ(intel_hdmi->hdmi_reg);
> +
> +	temp |= SDVO_ENABLE;
> +	if (crtc->config->has_audio)
> +		temp |= SDVO_AUDIO_ENABLE;
> +
> +	/*
> +	 * WaEnableHDMI8bpcBefore12bpc:snb,ivb
> +	 *
> +	 * The procedure for 12bpc is as follows:
> +	 * 1. disable HDMI clock gating
> +	 * 2. enable HDMI with 8bpc
> +	 * 3. enable HDMI with 12bpc
> +	 * 4. enable HDMI clock gating
> +	 */
> +
> +	if (crtc->config->pipe_bpp > 24) {
> +		I915_WRITE(TRANS_CHICKEN1(pipe),
> +			   I915_READ(TRANS_CHICKEN1(pipe)) |
> +			   TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
> +
> +		temp &= ~SDVO_COLOR_FORMAT_MASK;
> +		temp |= SDVO_COLOR_FORMAT_8bpc;
>  	}
> +
> +	I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +	POSTING_READ(intel_hdmi->hdmi_reg);
> +
> +	if (crtc->config->pipe_bpp > 24) {
> +		temp &= ~SDVO_COLOR_FORMAT_MASK;
> +		temp |= HDMI_COLOR_FORMAT_12bpc;
> +
> +		I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +		POSTING_READ(intel_hdmi->hdmi_reg);
> +
> +		I915_WRITE(TRANS_CHICKEN1(pipe),
> +			   I915_READ(TRANS_CHICKEN1(pipe)) &
> +			   ~TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
> +	}
> +
> +	if (crtc->config->has_audio)
> +		intel_enable_hdmi_audio(encoder);
>  }
> 
>  static void vlv_enable_hdmi(struct intel_encoder *encoder) @@ -1829,7
> +1888,10 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum
> port port)
>  		intel_encoder->post_disable = vlv_hdmi_post_disable;
>  	} else {
>  		intel_encoder->pre_enable = intel_hdmi_pre_enable;
> -		intel_encoder->enable = intel_enable_hdmi;
> +		if (HAS_PCH_CPT(dev))
> +			intel_encoder->enable = cpt_enable_hdmi;
> +		else
> +			intel_encoder->enable = intel_enable_hdmi;
>  	}
> 
>  	intel_encoder->type = INTEL_OUTPUT_HDMI;
> --
> 2.0.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/9] drm/i915: Send GCP infoframes for deep color HDMI sinks

[Konduru, Chandra]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> ville.syrjala@linux.intel.com
> Sent: Tuesday, May 05, 2015 7:06 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH v2 2/9] drm/i915: Send GCP infoframes for deep
> color HDMI sinks
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> GCP infoframes are required to inform the HDMI sink about the color depth.
> 
> Send the GCP infoframe whenever the sink supports any deep color modes since
> such sinks must anyway be capable of receiving them. For sinks that don't
> support deep color let's skip the GCP in case it might confuse the sink, although
> HDMI 1.4 spec does say all sinks must be capable of reciving them. In theory we
> could skip the GCP infoframe for deep color sinks in 8bpc mode as well since
> sinks must fall back to 8bpc whenever GCP isn't received for some time.
> 
> BSpec says we should disable GCP after disabling the port, so do that as well.
> 
> v2: s/intel_set_gcp_infoframe/intel_hdmi_set_gcp_infoframe/
>     Rebased due to crtc->config changes
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h   |  3 ++
>  drivers/gpu/drm/i915/intel_hdmi.c | 74
> +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 77 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e619e41..dcd93b5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6010,6 +6010,9 @@ enum skl_disp_power_wells {
>  #define _VIDEO_DIP_CTL_A         0xe0200
>  #define _VIDEO_DIP_DATA_A        0xe0208
>  #define _VIDEO_DIP_GCP_A         0xe0210
> +#define  GCP_COLOR_INDICATION		(1 << 2)
> +#define  GCP_DEFAULT_PHASE_ENABLE	(1 << 1)
> +#define  GCP_AV_MUTE			(1 << 0)
> 
>  #define _VIDEO_DIP_CTL_B         0xe1200
>  #define _VIDEO_DIP_DATA_B        0xe1208
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index 79cf445..87c4905 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -541,6 +541,66 @@ static void g4x_set_infoframes(struct drm_encoder
> *encoder,
>  	intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode);  }
> 
> +static bool hdmi_sink_is_deep_color(struct drm_encoder *encoder) {

Some static functions are prefixed but this isn't. Can you add prefix?

> +	struct drm_device *dev = encoder->dev;
> +	struct drm_connector *connector;
> +
> +	WARN_ON(!drm_modeset_is_locked(&dev-
> >mode_config.connection_mutex));
> +
> +	/*
> +	 * HDMI cloning is only supported on g4x which doesn't
> +	 * support deep color or GCP infoframes anyway so no
> +	 * need to worry about multiple HDMI sinks here.
> +	 */

There isn't any code specific for HDMI cloning or multiple HDMI sinks here, 
any relevance of above comment. Or am I missing something here?

> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> head)
> +		if (connector->encoder == encoder)
> +			return connector->display_info.bpc > 8;
> +
> +	return false;
> +}
> +
> +static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder) {
> +	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
> +	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> +	u32 reg, val = 0;
> +
> +	if (HAS_DDI(dev_priv))
> +		reg = HSW_TVIDEO_DIP_GCP(crtc->config->cpu_transcoder);
> +	else if (IS_VALLEYVIEW(dev_priv))
> +		reg = VLV_TVIDEO_DIP_GCP(crtc->pipe);
> +	else if (HAS_PCH_SPLIT(dev_priv->dev))
> +		reg = TVIDEO_DIP_GCP(crtc->pipe);
> +	else
> +		return false;
> +
> +	/* Indicate color depth wheneven the sink supports deep color */

Typo

> +	if (hdmi_sink_is_deep_color(encoder))
> +		val |= GCP_COLOR_INDICATION;
> +
> +	I915_WRITE(reg, val);
> +
> +	return val != 0;
> +}
> +
> +static void intel_disable_gcp_infoframe(struct intel_crtc *crtc) {
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	u32 reg;
> +
> +	if (HAS_DDI(dev_priv))
> +		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> +	else if (IS_VALLEYVIEW(dev_priv))
> +		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
> +	else if (HAS_PCH_SPLIT(dev_priv->dev))
> +		reg = TVIDEO_DIP_CTL(crtc->pipe);
> +	else
> +		return;
> +
> +	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP); }
> +
>  static void ibx_set_infoframes(struct drm_encoder *encoder,
>  			       bool enable,
>  			       struct drm_display_mode *adjusted_mode) @@ -
> 581,6 +641,9 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
>  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
>  		 VIDEO_DIP_ENABLE_GCP);
> 
> +	if (intel_hdmi_set_gcp_infoframe(encoder))
> +		val |= VIDEO_DIP_ENABLE_GCP;
> +
>  	I915_WRITE(reg, val);
>  	POSTING_READ(reg);
> 
> @@ -618,6 +681,9 @@ static void cpt_set_infoframes(struct drm_encoder
> *encoder,
>  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
>  		 VIDEO_DIP_ENABLE_GCP);
> 
> +	if (intel_hdmi_set_gcp_infoframe(encoder))
> +		val |= VIDEO_DIP_ENABLE_GCP;
> +
>  	I915_WRITE(reg, val);
>  	POSTING_READ(reg);
> 
> @@ -666,6 +732,9 @@ static void vlv_set_infoframes(struct drm_encoder
> *encoder,
>  	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
>  		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
> 
> +	if (intel_hdmi_set_gcp_infoframe(encoder))
> +		val |= VIDEO_DIP_ENABLE_GCP;
> +

There is a comment in the code that 
"Note that g4x/vlv don't support 12bpc hdmi outputs."
Is above call still required for vlv? Or need an update to that comment.

>  	I915_WRITE(reg, val);
>  	POSTING_READ(reg);
> 
> @@ -695,6 +764,9 @@ static void hsw_set_infoframes(struct drm_encoder
> *encoder,
>  	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW |
> VIDEO_DIP_ENABLE_GCP_HSW |
>  		 VIDEO_DIP_ENABLE_VS_HSW |
> VIDEO_DIP_ENABLE_GMP_HSW);
> 
> +	if (intel_hdmi_set_gcp_infoframe(encoder))
> +		val |= VIDEO_DIP_ENABLE_GCP_HSW;
> +
>  	I915_WRITE(reg, val);
>  	POSTING_READ(reg);
> 
> @@ -986,6 +1058,8 @@ static void intel_disable_hdmi(struct intel_encoder
> *encoder)
> 
>  	if (IS_CHERRYVIEW(dev))
>  		chv_powergate_phy_lanes(encoder, 0xf);
> +
> +	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));
>  }
> 
>  static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
> --
> 2.0.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/9] drm/i915: Enable default_phase in GCP when possible

[Konduru, Chandra]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> ville.syrjala@linux.intel.com
> Sent: Tuesday, May 05, 2015 7:06 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH v2 3/9] drm/i915: Enable default_phase in GCP when
> possible
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When the video timings are suitably aligned so that all different periods start at
> phase 0 (ie. none of the periods start mid-pixel) we can inform the sink about
> this. Supposedly the sink can then optimize certain things. Obviously this is only
> relevant when outputting >8bpc data since otherwise there are no mid-pixel
> phases.
> 
> v2: Rebased due to crtc->config changes
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 48
> +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index 87c4905..2e98e33 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -560,6 +560,49 @@ static bool hdmi_sink_is_deep_color(struct
> drm_encoder *encoder)
>  	return false;
>  }
> 
> +/*
> + * Determine if default_phase=1 can be indicated in the GCP infoframe.
> + *
> + * From HDMI specification 1.4a:
> + * - The first pixel of each Video Data Period shall always have a
> +pixel packing phase of 0
> + * - The first pixel following each Video Data Period shall have a
> +pixel packing phase of 0
> + * - The PP bits shall be constant for all GCPs and will be equal to
> +the last packing phase
> + * - The first pixel following every transition of HSYNC or VSYNC shall have a
> pixel packing
> + *   phase of 0
> + */
> +static bool gcp_default_phase_possible(int pipe_bpp,
> +				       const struct drm_display_mode *mode) {
> +	unsigned int pixels_per_group;
> +
> +	switch (pipe_bpp) {
> +	case 30:
> +		/* 4 pixels in 5 clocks */
> +		pixels_per_group = 4;
> +		break;
> +	case 36:
> +		/* 2 pixels in 3 clocks */
> +		pixels_per_group = 2;
> +		break;
> +	case 48:
> +		/* 1 pixel in 2 clocks */
> +		pixels_per_group = 1;
> +		break;
> +	default:
> +		/* phase information not relevant for 8bpc */
> +		return false;
> +	}
> +
> +	return mode->crtc_hdisplay % pixels_per_group == 0 &&
> +		mode->crtc_htotal % pixels_per_group == 0 &&
> +		mode->crtc_hblank_start % pixels_per_group == 0 &&
> +		mode->crtc_hblank_end % pixels_per_group == 0 &&
> +		mode->crtc_hsync_start % pixels_per_group == 0 &&
> +		mode->crtc_hsync_end % pixels_per_group == 0 &&

For hsync, bspec says Hsync is an even number.
Isn't it above check should be something like (hsync_end - hsync_start) % 2 == 0?
And similarly for front & back porches, right? 

> +		((mode->flags & DRM_MODE_FLAG_INTERLACE) == 0 ||
> +		 mode->crtc_htotal/2 % pixels_per_group == 0); }
> +
>  static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)  {
>  	struct drm_i915_private *dev_priv = encoder->dev->dev_private; @@ -
> 579,6 +622,11 @@ static bool intel_hdmi_set_gcp_infoframe(struct
> drm_encoder *encoder)
>  	if (hdmi_sink_is_deep_color(encoder))
>  		val |= GCP_COLOR_INDICATION;
> 
> +	/* Enable default_phase whenever the display mode is suitably aligned
> */
> +	if (gcp_default_phase_possible(crtc->config->pipe_bpp,
> +				       &crtc->config->base.adjusted_mode))
> +		val |= GCP_DEFAULT_PHASE_ENABLE;
> +
>  	I915_WRITE(reg, val);
> 
>  	return val != 0;
> --
> 2.0.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 6/9] drm/i915: Disable all infoframes when turning off the HDMI port

[Konduru, Chandra]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> ville.syrjala@linux.intel.com
> Sent: Tuesday, May 05, 2015 7:06 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH v2 6/9] drm/i915: Disable all infoframes when
> turning off the HDMI port
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we just disable the GCP infoframe when turning off the port.
> That means if the same transcoder is used on a DP port next, we might
> end up pushing infoframes over DP, which isn't intended. Just disable

Wonder how it is working. May be it is ok, or never hit using a previously 
used transcoder for HDMI port for DP.

By the way, you mean end up pushing "other" infoframes over DP?

> all the infoframes when turning off the port.
> 
> Also protect against two ports stomping on each other on g4x due to
> the single video DIP instance. Now only the first port to enable
> gets to send infoframes.

So is, 2nd port doesn't gets to send infoframes, the expected behavior 
when two ports trying with a single DIP?

Seems like a corner case to test thoroughly. Is this path exercised in your testing?

> 
> v2: Rebase
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 85 ++++++++++++++++++---------------------
>  1 file changed, 40 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index 766bdb1..03b4759 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -514,7 +514,13 @@ static void g4x_set_infoframes(struct drm_encoder
> *encoder,
>  	if (!enable) {
>  		if (!(val & VIDEO_DIP_ENABLE))
>  			return;
> -		val &= ~VIDEO_DIP_ENABLE;
> +		if (port != (val & VIDEO_DIP_PORT_MASK)) {
> +			DRM_DEBUG_KMS("video DIP still enabled on port
> %c\n",
> +				      (val & VIDEO_DIP_PORT_MASK) >> 29);
> +			return;
> +		}
> +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> +			 VIDEO_DIP_ENABLE_VENDOR |
> VIDEO_DIP_ENABLE_SPD);
>  		I915_WRITE(reg, val);
>  		POSTING_READ(reg);
>  		return;
> @@ -522,16 +528,17 @@ static void g4x_set_infoframes(struct drm_encoder
> *encoder,
> 
>  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
>  		if (val & VIDEO_DIP_ENABLE) {
> -			val &= ~VIDEO_DIP_ENABLE;
> -			I915_WRITE(reg, val);
> -			POSTING_READ(reg);
> +			DRM_DEBUG_KMS("video DIP already enabled on port
> %c\n",
> +				      (val & VIDEO_DIP_PORT_MASK) >> 29);
> +			return;
>  		}
>  		val &= ~VIDEO_DIP_PORT_MASK;
>  		val |= port;
>  	}
> 
>  	val |= VIDEO_DIP_ENABLE;
> -	val &= ~VIDEO_DIP_ENABLE_VENDOR;
> +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);
> 
>  	I915_WRITE(reg, val);
>  	POSTING_READ(reg);
> @@ -632,23 +639,6 @@ static bool intel_hdmi_set_gcp_infoframe(struct
> drm_encoder *encoder)
>  	return val != 0;
>  }
> 
> -static void intel_disable_gcp_infoframe(struct intel_crtc *crtc)
> -{
> -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> -	u32 reg;
> -
> -	if (HAS_DDI(dev_priv))
> -		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> -	else if (IS_VALLEYVIEW(dev_priv))
> -		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
> -	else if (HAS_PCH_SPLIT(dev_priv->dev))
> -		reg = TVIDEO_DIP_CTL(crtc->pipe);
> -	else
> -		return;
> -
> -	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP);
> -}
> -
>  static void ibx_set_infoframes(struct drm_encoder *encoder,
>  			       bool enable,
>  			       struct drm_display_mode *adjusted_mode)
> @@ -669,25 +659,26 @@ static void ibx_set_infoframes(struct drm_encoder
> *encoder,
>  	if (!enable) {
>  		if (!(val & VIDEO_DIP_ENABLE))
>  			return;
> -		val &= ~VIDEO_DIP_ENABLE;
> +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> +			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
>  		I915_WRITE(reg, val);
>  		POSTING_READ(reg);
>  		return;
>  	}
> 
>  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> -		if (val & VIDEO_DIP_ENABLE) {
> -			val &= ~VIDEO_DIP_ENABLE;
> -			I915_WRITE(reg, val);
> -			POSTING_READ(reg);
> -		}
> +		WARN(val & VIDEO_DIP_ENABLE,
> +		     "DIP already enabled on port %c\n",
> +		     (val & VIDEO_DIP_PORT_MASK) >> 29);
>  		val &= ~VIDEO_DIP_PORT_MASK;
>  		val |= port;
>  	}
> 
>  	val |= VIDEO_DIP_ENABLE;
> -	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> -		 VIDEO_DIP_ENABLE_GCP);
> +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> 
>  	if (intel_hdmi_set_gcp_infoframe(encoder))
>  		val |= VIDEO_DIP_ENABLE_GCP;
> @@ -718,7 +709,9 @@ static void cpt_set_infoframes(struct drm_encoder
> *encoder,
>  	if (!enable) {
>  		if (!(val & VIDEO_DIP_ENABLE))
>  			return;
> -		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI);
> +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> +			 VIDEO_DIP_ENABLE_VENDOR |
> VIDEO_DIP_ENABLE_GAMUT |
> +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
>  		I915_WRITE(reg, val);
>  		POSTING_READ(reg);
>  		return;
> @@ -727,7 +720,7 @@ static void cpt_set_infoframes(struct drm_encoder
> *encoder,
>  	/* Set both together, unset both together: see the spec. */
>  	val |= VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI;
>  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> -		 VIDEO_DIP_ENABLE_GCP);
> +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> 
>  	if (intel_hdmi_set_gcp_infoframe(encoder))
>  		val |= VIDEO_DIP_ENABLE_GCP;
> @@ -760,25 +753,26 @@ static void vlv_set_infoframes(struct drm_encoder
> *encoder,
>  	if (!enable) {
>  		if (!(val & VIDEO_DIP_ENABLE))
>  			return;
> -		val &= ~VIDEO_DIP_ENABLE;
> +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> +			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
>  		I915_WRITE(reg, val);
>  		POSTING_READ(reg);
>  		return;
>  	}
> 
>  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> -		if (val & VIDEO_DIP_ENABLE) {
> -			val &= ~VIDEO_DIP_ENABLE;
> -			I915_WRITE(reg, val);
> -			POSTING_READ(reg);
> -		}
> +		WARN(val & VIDEO_DIP_ENABLE,
> +		     "DIP already enabled on port %c\n",
> +		     (val & VIDEO_DIP_PORT_MASK) >> 29);
>  		val &= ~VIDEO_DIP_PORT_MASK;
>  		val |= port;
>  	}
> 
>  	val |= VIDEO_DIP_ENABLE;
> -	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
> -		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
> +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> 
>  	if (intel_hdmi_set_gcp_infoframe(encoder))
>  		val |= VIDEO_DIP_ENABLE_GCP;
> @@ -803,15 +797,16 @@ static void hsw_set_infoframes(struct drm_encoder
> *encoder,
> 
>  	assert_hdmi_port_disabled(intel_hdmi);
> 
> +	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_AVI_HSW |
> +		 VIDEO_DIP_ENABLE_GCP_HSW | VIDEO_DIP_ENABLE_VS_HSW |
> +		 VIDEO_DIP_ENABLE_GMP_HSW | VIDEO_DIP_ENABLE_SPD_HSW);
> +
>  	if (!enable) {
> -		I915_WRITE(reg, 0);
> +		I915_WRITE(reg, val);
>  		POSTING_READ(reg);
>  		return;
>  	}
> 
> -	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_GCP_HSW |
> -		 VIDEO_DIP_ENABLE_VS_HSW | VIDEO_DIP_ENABLE_GMP_HSW);
> -
>  	if (intel_hdmi_set_gcp_infoframe(encoder))
>  		val |= VIDEO_DIP_ENABLE_GCP_HSW;
> 
> @@ -1133,7 +1128,7 @@ static void intel_disable_hdmi(struct intel_encoder
> *encoder)
>  	if (IS_CHERRYVIEW(dev))
>  		chv_powergate_phy_lanes(encoder, 0xf);
> 
> -	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));
> +	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
>  }
> 
>  static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
> --
> 2.0.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/9] drm/i915: Check infoframe state more diligently.

[Konduru, Chandra]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> ville.syrjala@linux.intel.com
> Sent: Tuesday, May 05, 2015 7:06 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 7/9] drm/i915: Check infoframe state more
> diligently.
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Check that the DIP is enabled on the right port on IBX and VLV/CHV as
> we're doing on g4x, and also check for all the infoframe enable bits on
> all platforms.
> 
> Eventually we should track each infoframe type independently, and also
> their contents. This is a small step in that direction as .infoframe_enabled()
> return value could be easily turned into a bitmask.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 44 ++++++++++++++++++++++++++++------
> -----
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index 03b4759..ce595c3 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -174,10 +174,14 @@ static bool g4x_infoframe_enabled(struct
> drm_encoder *encoder)
>  	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
>  	u32 val = I915_READ(VIDEO_DIP_CTL);
> 
> -	if (VIDEO_DIP_PORT(intel_dig_port->port) == (val &
> VIDEO_DIP_PORT_MASK))
> -		return val & VIDEO_DIP_ENABLE;
> +	if ((val & VIDEO_DIP_ENABLE) == 0)
> +		return false;
> 
> -	return false;
> +	if ((val & VIDEO_DIP_PORT_MASK) != VIDEO_DIP_PORT(intel_dig_port-
> >port))
> +		return false;
> +
> +	return val & (VIDEO_DIP_ENABLE_AVI |
> +		      VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);
>  }
> 
>  static void ibx_write_infoframe(struct drm_encoder *encoder,
> @@ -227,10 +231,15 @@ static bool ibx_infoframe_enabled(struct
> drm_encoder *encoder)
>  	int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
>  	u32 val = I915_READ(reg);
> 
> -	if (VIDEO_DIP_PORT(intel_dig_port->port) == (val &
> VIDEO_DIP_PORT_MASK))
> -		return val & VIDEO_DIP_ENABLE;
> +	if ((val & VIDEO_DIP_ENABLE) == 0)
> +		return false;
> 
> -	return false;
> +	if ((val & VIDEO_DIP_PORT_MASK) != VIDEO_DIP_PORT(intel_dig_port-
> >port))
> +		return false;
> +
> +	return val & (VIDEO_DIP_ENABLE_AVI |
> +		      VIDEO_DIP_ENABLE_VENDOR |
> VIDEO_DIP_ENABLE_GAMUT |
> +		      VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
>  }
> 
>  static void cpt_write_infoframe(struct drm_encoder *encoder,
> @@ -282,7 +291,12 @@ static bool cpt_infoframe_enabled(struct drm_encoder
> *encoder)
>  	int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
>  	u32 val = I915_READ(reg);
> 
> -	return val & VIDEO_DIP_ENABLE;
> +	if ((val & VIDEO_DIP_ENABLE) == 0)
> +		return false;
> +
> +	return val & (VIDEO_DIP_ENABLE_AVI |
> +		      VIDEO_DIP_ENABLE_VENDOR |
> VIDEO_DIP_ENABLE_GAMUT |
> +		      VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
>  }
> 
>  static void vlv_write_infoframe(struct drm_encoder *encoder,
> @@ -332,10 +346,15 @@ static bool vlv_infoframe_enabled(struct
> drm_encoder *encoder)
>  	int reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
>  	u32 val = I915_READ(reg);
> 
> -	if (VIDEO_DIP_PORT(intel_dig_port->port) == (val &
> VIDEO_DIP_PORT_MASK))
> -		return val & VIDEO_DIP_ENABLE;
> +	if ((val & VIDEO_DIP_ENABLE) == 0)
> +		return false;
> 
> -	return false;
> +	if ((val & VIDEO_DIP_PORT_MASK) != VIDEO_DIP_PORT(intel_dig_port-
> >port))
> +		return false;
> +
> +	return val & (VIDEO_DIP_ENABLE_AVI |
> +		      VIDEO_DIP_ENABLE_VENDOR |
> VIDEO_DIP_ENABLE_GAMUT |
> +		      VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
>  }
> 
>  static void hsw_write_infoframe(struct drm_encoder *encoder,
> @@ -383,8 +402,9 @@ static bool hsw_infoframe_enabled(struct drm_encoder
> *encoder)
>  	u32 ctl_reg = HSW_TVIDEO_DIP_CTL(intel_crtc->config-
> >cpu_transcoder);
>  	u32 val = I915_READ(ctl_reg);
> 
> -	return val & (VIDEO_DIP_ENABLE_AVI_HSW |
> VIDEO_DIP_ENABLE_SPD_HSW |
> -		      VIDEO_DIP_ENABLE_VS_HSW);
> +	return val & (VIDEO_DIP_ENABLE_VSC_HSW |
> VIDEO_DIP_ENABLE_AVI_HSW |
> +		      VIDEO_DIP_ENABLE_GCP_HSW |
> VIDEO_DIP_ENABLE_VS_HSW |
> +		      VIDEO_DIP_ENABLE_GMP_HSW |
> VIDEO_DIP_ENABLE_SPD_HSW);
>  }
> 
Reviewed-by: Chandra Konduru <Chandra.konduru@intel.com>

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

Re: [PATCH 8/9] drm/i915: Fix hdmi clock readout with pixel repeat

[Konduru, Chandra]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> ville.syrjala@linux.intel.com
> Sent: Tuesday, May 05, 2015 7:06 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 8/9] drm/i915: Fix hdmi clock readout with pixel
> repeat
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Account for the pixel multiplier when reading out the HDMI
> mode dotclock. Makes the state checked happier on my ILK when using
> double clocked modes.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index ce595c3..1810242 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -943,6 +943,9 @@ static void intel_hdmi_get_config(struct intel_encoder
> *encoder,
>  	else
>  		dotclock = pipe_config->port_clock;
> 
> +	if (pipe_config->pixel_multiplier)
> +		dotclock /= pipe_config->pixel_multiplier;
> +

This is unrelated to HDMI 12bpc series. May be a separate patch.

Reviewed-by: Chandra Konduru <Chandra.konduru@intel.com>


>  	if (HAS_PCH_SPLIT(dev_priv->dev))
>  		ironlake_check_encoder_dotclock(pipe_config, dotclock);
> 
> --
> 2.0.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 9/9] drm/i915: Double the port clock when using double clocked modes with 12bpc

[Konduru, Chandra]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> ville.syrjala@linux.intel.com
> Sent: Tuesday, May 05, 2015 7:06 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 9/9] drm/i915: Double the port clock when using
> double clocked modes with 12bpc
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we're forgetting to double the port clock when using double
> clocked modes with 12bpc on HDMI. We're only accounting for the 1.5x
> factor due to the 12bpc. So further double the 1.5x port clock when we
> have a double clocked mode.
> 
> Unfortunately I don't have any displays that support both 12bpc and
> double clocked modes, so I was unable to test this.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index 1810242..f3eec38 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1248,6 +1248,7 @@ bool intel_hdmi_compute_config(struct
> intel_encoder *encoder,
> 
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) {
>  		pipe_config->pixel_multiplier = 2;
> +		clock_12bpc *= 2;
>  	}
> 
>  	if (intel_hdmi->color_range)
> --

Reviewed-by: Chandra Konduru <Chandra.konduru@intel.com>

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

Re: [PATCH v2 6/9] drm/i915: Disable all infoframes when turning off the HDMI port

[Ville Syrjälä]

On Mon, Jun 01, 2015 at 10:48:03PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> > ville.syrjala@linux.intel.com
> > Sent: Tuesday, May 05, 2015 7:06 AM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH v2 6/9] drm/i915: Disable all infoframes when
> > turning off the HDMI port
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently we just disable the GCP infoframe when turning off the port.
> > That means if the same transcoder is used on a DP port next, we might
> > end up pushing infoframes over DP, which isn't intended. Just disable
> 
> Wonder how it is working. May be it is ok, or never hit using a previously 
> used transcoder for HDMI port for DP.
> 
> By the way, you mean end up pushing "other" infoframes over DP?

We don't send infoframes over DP at all currently. Or I should say we
never intended to send them. After this patch that should even be true.
Well, unless the BIOS already enables them and we just fire up a DP port
using the transcoder in question. So I suppose we should really have the
DP code clear the infoframe settings explicitly, or we should clear them
during the sanitize phase.

> 
> > all the infoframes when turning off the port.
> > 
> > Also protect against two ports stomping on each other on g4x due to
> > the single video DIP instance. Now only the first port to enable
> > gets to send infoframes.
> 
> So is, 2nd port doesn't gets to send infoframes, the expected behavior 
> when two ports trying with a single DIP?

I'm not sure what's the best behaviour here. Either we somehow pick one
of the ports to send infoframes (first come first serve in this patch),
or we could just disable infoframes entirely for cloned cases. But it's
probably a rare configuration anyway since HDMI cloning is only allowed
on g4x.

> 
> Seems like a corner case to test thoroughly. Is this path exercised in your testing?

I tested it long ago. Although I must admit that the patch looked a bit
different back then.

> 
> > 
> > v2: Rebase
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_hdmi.c | 85 ++++++++++++++++++---------------------
> >  1 file changed, 40 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 766bdb1..03b4759 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -514,7 +514,13 @@ static void g4x_set_infoframes(struct drm_encoder
> > *encoder,
> >  	if (!enable) {
> >  		if (!(val & VIDEO_DIP_ENABLE))
> >  			return;
> > -		val &= ~VIDEO_DIP_ENABLE;
> > +		if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > +			DRM_DEBUG_KMS("video DIP still enabled on port
> > %c\n",
> > +				      (val & VIDEO_DIP_PORT_MASK) >> 29);
> > +			return;
> > +		}
> > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > +			 VIDEO_DIP_ENABLE_VENDOR |
> > VIDEO_DIP_ENABLE_SPD);
> >  		I915_WRITE(reg, val);
> >  		POSTING_READ(reg);
> >  		return;
> > @@ -522,16 +528,17 @@ static void g4x_set_infoframes(struct drm_encoder
> > *encoder,
> > 
> >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> >  		if (val & VIDEO_DIP_ENABLE) {
> > -			val &= ~VIDEO_DIP_ENABLE;
> > -			I915_WRITE(reg, val);
> > -			POSTING_READ(reg);
> > +			DRM_DEBUG_KMS("video DIP already enabled on port
> > %c\n",
> > +				      (val & VIDEO_DIP_PORT_MASK) >> 29);
> > +			return;
> >  		}
> >  		val &= ~VIDEO_DIP_PORT_MASK;
> >  		val |= port;
> >  	}
> > 
> >  	val |= VIDEO_DIP_ENABLE;
> > -	val &= ~VIDEO_DIP_ENABLE_VENDOR;
> > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);
> > 
> >  	I915_WRITE(reg, val);
> >  	POSTING_READ(reg);
> > @@ -632,23 +639,6 @@ static bool intel_hdmi_set_gcp_infoframe(struct
> > drm_encoder *encoder)
> >  	return val != 0;
> >  }
> > 
> > -static void intel_disable_gcp_infoframe(struct intel_crtc *crtc)
> > -{
> > -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > -	u32 reg;
> > -
> > -	if (HAS_DDI(dev_priv))
> > -		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> > -	else if (IS_VALLEYVIEW(dev_priv))
> > -		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
> > -	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > -		reg = TVIDEO_DIP_CTL(crtc->pipe);
> > -	else
> > -		return;
> > -
> > -	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP);
> > -}
> > -
> >  static void ibx_set_infoframes(struct drm_encoder *encoder,
> >  			       bool enable,
> >  			       struct drm_display_mode *adjusted_mode)
> > @@ -669,25 +659,26 @@ static void ibx_set_infoframes(struct drm_encoder
> > *encoder,
> >  	if (!enable) {
> >  		if (!(val & VIDEO_DIP_ENABLE))
> >  			return;
> > -		val &= ~VIDEO_DIP_ENABLE;
> > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > +			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> >  		I915_WRITE(reg, val);
> >  		POSTING_READ(reg);
> >  		return;
> >  	}
> > 
> >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > -		if (val & VIDEO_DIP_ENABLE) {
> > -			val &= ~VIDEO_DIP_ENABLE;
> > -			I915_WRITE(reg, val);
> > -			POSTING_READ(reg);
> > -		}
> > +		WARN(val & VIDEO_DIP_ENABLE,
> > +		     "DIP already enabled on port %c\n",
> > +		     (val & VIDEO_DIP_PORT_MASK) >> 29);
> >  		val &= ~VIDEO_DIP_PORT_MASK;
> >  		val |= port;
> >  	}
> > 
> >  	val |= VIDEO_DIP_ENABLE;
> > -	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > -		 VIDEO_DIP_ENABLE_GCP);
> > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > 
> >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> >  		val |= VIDEO_DIP_ENABLE_GCP;
> > @@ -718,7 +709,9 @@ static void cpt_set_infoframes(struct drm_encoder
> > *encoder,
> >  	if (!enable) {
> >  		if (!(val & VIDEO_DIP_ENABLE))
> >  			return;
> > -		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI);
> > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > +			 VIDEO_DIP_ENABLE_VENDOR |
> > VIDEO_DIP_ENABLE_GAMUT |
> > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> >  		I915_WRITE(reg, val);
> >  		POSTING_READ(reg);
> >  		return;
> > @@ -727,7 +720,7 @@ static void cpt_set_infoframes(struct drm_encoder
> > *encoder,
> >  	/* Set both together, unset both together: see the spec. */
> >  	val |= VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI;
> >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > -		 VIDEO_DIP_ENABLE_GCP);
> > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > 
> >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> >  		val |= VIDEO_DIP_ENABLE_GCP;
> > @@ -760,25 +753,26 @@ static void vlv_set_infoframes(struct drm_encoder
> > *encoder,
> >  	if (!enable) {
> >  		if (!(val & VIDEO_DIP_ENABLE))
> >  			return;
> > -		val &= ~VIDEO_DIP_ENABLE;
> > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > +			 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> >  		I915_WRITE(reg, val);
> >  		POSTING_READ(reg);
> >  		return;
> >  	}
> > 
> >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > -		if (val & VIDEO_DIP_ENABLE) {
> > -			val &= ~VIDEO_DIP_ENABLE;
> > -			I915_WRITE(reg, val);
> > -			POSTING_READ(reg);
> > -		}
> > +		WARN(val & VIDEO_DIP_ENABLE,
> > +		     "DIP already enabled on port %c\n",
> > +		     (val & VIDEO_DIP_PORT_MASK) >> 29);
> >  		val &= ~VIDEO_DIP_PORT_MASK;
> >  		val |= port;
> >  	}
> > 
> >  	val |= VIDEO_DIP_ENABLE;
> > -	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
> > -		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
> > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > 
> >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> >  		val |= VIDEO_DIP_ENABLE_GCP;
> > @@ -803,15 +797,16 @@ static void hsw_set_infoframes(struct drm_encoder
> > *encoder,
> > 
> >  	assert_hdmi_port_disabled(intel_hdmi);
> > 
> > +	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_AVI_HSW |
> > +		 VIDEO_DIP_ENABLE_GCP_HSW | VIDEO_DIP_ENABLE_VS_HSW |
> > +		 VIDEO_DIP_ENABLE_GMP_HSW | VIDEO_DIP_ENABLE_SPD_HSW);
> > +
> >  	if (!enable) {
> > -		I915_WRITE(reg, 0);
> > +		I915_WRITE(reg, val);
> >  		POSTING_READ(reg);
> >  		return;
> >  	}
> > 
> > -	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_GCP_HSW |
> > -		 VIDEO_DIP_ENABLE_VS_HSW | VIDEO_DIP_ENABLE_GMP_HSW);
> > -
> >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> >  		val |= VIDEO_DIP_ENABLE_GCP_HSW;
> > 
> > @@ -1133,7 +1128,7 @@ static void intel_disable_hdmi(struct intel_encoder
> > *encoder)
> >  	if (IS_CHERRYVIEW(dev))
> >  		chv_powergate_phy_lanes(encoder, 0xf);
> > 
> > -	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));
> > +	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
> >  }
> > 
> >  static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
> > --
> > 2.0.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 3/9] drm/i915: Enable default_phase in GCP when possible

[Ville Syrjälä]

On Mon, Jun 01, 2015 at 09:49:53PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> > ville.syrjala@linux.intel.com
> > Sent: Tuesday, May 05, 2015 7:06 AM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH v2 3/9] drm/i915: Enable default_phase in GCP when
> > possible
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > When the video timings are suitably aligned so that all different periods start at
> > phase 0 (ie. none of the periods start mid-pixel) we can inform the sink about
> > this. Supposedly the sink can then optimize certain things. Obviously this is only
> > relevant when outputting >8bpc data since otherwise there are no mid-pixel
> > phases.
> > 
> > v2: Rebased due to crtc->config changes
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_hdmi.c | 48
> > +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 87c4905..2e98e33 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -560,6 +560,49 @@ static bool hdmi_sink_is_deep_color(struct
> > drm_encoder *encoder)
> >  	return false;
> >  }
> > 
> > +/*
> > + * Determine if default_phase=1 can be indicated in the GCP infoframe.
> > + *
> > + * From HDMI specification 1.4a:
> > + * - The first pixel of each Video Data Period shall always have a
> > +pixel packing phase of 0
> > + * - The first pixel following each Video Data Period shall have a
> > +pixel packing phase of 0
> > + * - The PP bits shall be constant for all GCPs and will be equal to
> > +the last packing phase
> > + * - The first pixel following every transition of HSYNC or VSYNC shall have a
> > pixel packing
> > + *   phase of 0
> > + */
> > +static bool gcp_default_phase_possible(int pipe_bpp,
> > +				       const struct drm_display_mode *mode) {
> > +	unsigned int pixels_per_group;
> > +
> > +	switch (pipe_bpp) {
> > +	case 30:
> > +		/* 4 pixels in 5 clocks */
> > +		pixels_per_group = 4;
> > +		break;
> > +	case 36:
> > +		/* 2 pixels in 3 clocks */
> > +		pixels_per_group = 2;
> > +		break;
> > +	case 48:
> > +		/* 1 pixel in 2 clocks */
> > +		pixels_per_group = 1;
> > +		break;
> > +	default:
> > +		/* phase information not relevant for 8bpc */
> > +		return false;
> > +	}
> > +
> > +	return mode->crtc_hdisplay % pixels_per_group == 0 &&
> > +		mode->crtc_htotal % pixels_per_group == 0 &&
> > +		mode->crtc_hblank_start % pixels_per_group == 0 &&
> > +		mode->crtc_hblank_end % pixels_per_group == 0 &&
> > +		mode->crtc_hsync_start % pixels_per_group == 0 &&
> > +		mode->crtc_hsync_end % pixels_per_group == 0 &&
> 
> For hsync, bspec says Hsync is an even number.
> Isn't it above check should be something like (hsync_end - hsync_start) % 2 == 0?
> And similarly for front & back porches, right? 

If X and Y are even then (X - Y) is even too. Also the text in bspec is
less informative than the text in HDMI spec, which is why I quited the
HDMI spec instead.

> 
> > +		((mode->flags & DRM_MODE_FLAG_INTERLACE) == 0 ||
> > +		 mode->crtc_htotal/2 % pixels_per_group == 0); }
> > +
> >  static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)  {
> >  	struct drm_i915_private *dev_priv = encoder->dev->dev_private; @@ -
> > 579,6 +622,11 @@ static bool intel_hdmi_set_gcp_infoframe(struct
> > drm_encoder *encoder)
> >  	if (hdmi_sink_is_deep_color(encoder))
> >  		val |= GCP_COLOR_INDICATION;
> > 
> > +	/* Enable default_phase whenever the display mode is suitably aligned
> > */
> > +	if (gcp_default_phase_possible(crtc->config->pipe_bpp,
> > +				       &crtc->config->base.adjusted_mode))
> > +		val |= GCP_DEFAULT_PHASE_ENABLE;
> > +
> >  	I915_WRITE(reg, val);
> > 
> >  	return val != 0;
> > --
> > 2.0.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 2/9] drm/i915: Send GCP infoframes for deep color HDMI sinks

[Ville Syrjälä]

On Mon, Jun 01, 2015 at 09:49:40PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> > ville.syrjala@linux.intel.com
> > Sent: Tuesday, May 05, 2015 7:06 AM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH v2 2/9] drm/i915: Send GCP infoframes for deep
> > color HDMI sinks
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > GCP infoframes are required to inform the HDMI sink about the color depth.
> > 
> > Send the GCP infoframe whenever the sink supports any deep color modes since
> > such sinks must anyway be capable of receiving them. For sinks that don't
> > support deep color let's skip the GCP in case it might confuse the sink, although
> > HDMI 1.4 spec does say all sinks must be capable of reciving them. In theory we
> > could skip the GCP infoframe for deep color sinks in 8bpc mode as well since
> > sinks must fall back to 8bpc whenever GCP isn't received for some time.
> > 
> > BSpec says we should disable GCP after disabling the port, so do that as well.
> > 
> > v2: s/intel_set_gcp_infoframe/intel_hdmi_set_gcp_infoframe/
> >     Rebased due to crtc->config changes
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h   |  3 ++
> >  drivers/gpu/drm/i915/intel_hdmi.c | 74
> > +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 77 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index e619e41..dcd93b5 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6010,6 +6010,9 @@ enum skl_disp_power_wells {
> >  #define _VIDEO_DIP_CTL_A         0xe0200
> >  #define _VIDEO_DIP_DATA_A        0xe0208
> >  #define _VIDEO_DIP_GCP_A         0xe0210
> > +#define  GCP_COLOR_INDICATION		(1 << 2)
> > +#define  GCP_DEFAULT_PHASE_ENABLE	(1 << 1)
> > +#define  GCP_AV_MUTE			(1 << 0)
> > 
> >  #define _VIDEO_DIP_CTL_B         0xe1200
> >  #define _VIDEO_DIP_DATA_B        0xe1208
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 79cf445..87c4905 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -541,6 +541,66 @@ static void g4x_set_infoframes(struct drm_encoder
> > *encoder,
> >  	intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode);  }
> > 
> > +static bool hdmi_sink_is_deep_color(struct drm_encoder *encoder) {
> 
> Some static functions are prefixed but this isn't. Can you add prefix?

Is there any benefit from a prefix?

> 
> > +	struct drm_device *dev = encoder->dev;
> > +	struct drm_connector *connector;
> > +
> > +	WARN_ON(!drm_modeset_is_locked(&dev-
> > >mode_config.connection_mutex));
> > +
> > +	/*
> > +	 * HDMI cloning is only supported on g4x which doesn't
> > +	 * support deep color or GCP infoframes anyway so no
> > +	 * need to worry about multiple HDMI sinks here.
> > +	 */
> 
> There isn't any code specific for HDMI cloning or multiple HDMI sinks here, 
> any relevance of above comment. Or am I missing something here?

In a cloned setup we'd feed multiple sinks from the same transcoder, so
we'd need to basically check that connector->encoder->crtc matches our crtc
here, and only return true if all the relevant connectors were deep color
capable. But since we can ignore cloning we can stop looking once we
find the connector for our current encoder (ie. HDMI port).

> 
> > +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> > head)
> > +		if (connector->encoder == encoder)
> > +			return connector->display_info.bpc > 8;
> > +
> > +	return false;
> > +}
> > +
> > +static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder) {
> > +	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
> > +	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> > +	u32 reg, val = 0;
> > +
> > +	if (HAS_DDI(dev_priv))
> > +		reg = HSW_TVIDEO_DIP_GCP(crtc->config->cpu_transcoder);
> > +	else if (IS_VALLEYVIEW(dev_priv))
> > +		reg = VLV_TVIDEO_DIP_GCP(crtc->pipe);
> > +	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > +		reg = TVIDEO_DIP_GCP(crtc->pipe);
> > +	else
> > +		return false;
> > +
> > +	/* Indicate color depth wheneven the sink supports deep color */
> 
> Typo
> 
> > +	if (hdmi_sink_is_deep_color(encoder))
> > +		val |= GCP_COLOR_INDICATION;
> > +
> > +	I915_WRITE(reg, val);
> > +
> > +	return val != 0;
> > +}
> > +
> > +static void intel_disable_gcp_infoframe(struct intel_crtc *crtc) {
> > +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > +	u32 reg;
> > +
> > +	if (HAS_DDI(dev_priv))
> > +		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> > +	else if (IS_VALLEYVIEW(dev_priv))
> > +		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
> > +	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > +		reg = TVIDEO_DIP_CTL(crtc->pipe);
> > +	else
> > +		return;
> > +
> > +	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP); }
> > +
> >  static void ibx_set_infoframes(struct drm_encoder *encoder,
> >  			       bool enable,
> >  			       struct drm_display_mode *adjusted_mode) @@ -
> > 581,6 +641,9 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
> >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> >  		 VIDEO_DIP_ENABLE_GCP);
> > 
> > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > +		val |= VIDEO_DIP_ENABLE_GCP;
> > +
> >  	I915_WRITE(reg, val);
> >  	POSTING_READ(reg);
> > 
> > @@ -618,6 +681,9 @@ static void cpt_set_infoframes(struct drm_encoder
> > *encoder,
> >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> >  		 VIDEO_DIP_ENABLE_GCP);
> > 
> > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > +		val |= VIDEO_DIP_ENABLE_GCP;
> > +
> >  	I915_WRITE(reg, val);
> >  	POSTING_READ(reg);
> > 
> > @@ -666,6 +732,9 @@ static void vlv_set_infoframes(struct drm_encoder
> > *encoder,
> >  	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
> >  		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
> > 
> > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > +		val |= VIDEO_DIP_ENABLE_GCP;
> > +
> 
> There is a comment in the code that 
> "Note that g4x/vlv don't support 12bpc hdmi outputs."
> Is above call still required for vlv? Or need an update to that comment.

VLV/CHV support the GCP infoframe even though they don't support
12bpc output. The way I decided to do things is to send the GCP
infoframe whenever there's a deep color sink hooked up, even if
we feed it 8bpc data.

> 
> >  	I915_WRITE(reg, val);
> >  	POSTING_READ(reg);
> > 
> > @@ -695,6 +764,9 @@ static void hsw_set_infoframes(struct drm_encoder
> > *encoder,
> >  	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW |
> > VIDEO_DIP_ENABLE_GCP_HSW |
> >  		 VIDEO_DIP_ENABLE_VS_HSW |
> > VIDEO_DIP_ENABLE_GMP_HSW);
> > 
> > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > +		val |= VIDEO_DIP_ENABLE_GCP_HSW;
> > +
> >  	I915_WRITE(reg, val);
> >  	POSTING_READ(reg);
> > 
> > @@ -986,6 +1058,8 @@ static void intel_disable_hdmi(struct intel_encoder
> > *encoder)
> > 
> >  	if (IS_CHERRYVIEW(dev))
> >  		chv_powergate_phy_lanes(encoder, 0xf);
> > +
> > +	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));
> >  }
> > 
> >  static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
> > --
> > 2.0.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 6/9] drm/i915: Disable all infoframes when turning off the HDMI port

[Konduru, Chandra]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Tuesday, June 02, 2015 4:11 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v2 6/9] drm/i915: Disable all infoframes when
> turning off the HDMI port
> 
> On Mon, Jun 01, 2015 at 10:48:03PM +0000, Konduru, Chandra wrote:
> >
> >
> > > -----Original Message-----
> > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of
> > > ville.syrjala@linux.intel.com
> > > Sent: Tuesday, May 05, 2015 7:06 AM
> > > To: intel-gfx@lists.freedesktop.org
> > > Subject: [Intel-gfx] [PATCH v2 6/9] drm/i915: Disable all infoframes when
> > > turning off the HDMI port
> > >
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Currently we just disable the GCP infoframe when turning off the port.
> > > That means if the same transcoder is used on a DP port next, we might
> > > end up pushing infoframes over DP, which isn't intended. Just disable
> >
> > Wonder how it is working. May be it is ok, or never hit using a previously
> > used transcoder for HDMI port for DP.
> >
> > By the way, you mean end up pushing "other" infoframes over DP?
> 
> We don't send infoframes over DP at all currently. Or I should say we
> never intended to send them. After this patch that should even be true.
> Well, unless the BIOS already enables them and we just fire up a DP port
> using the transcoder in question. So I suppose we should really have the
> DP code clear the infoframe settings explicitly, or we should clear them
> during the sanitize phase.

Agree that DP code path should clear the infoframe settings explicitly.
As you are already touching this code, will you plan to handle it?
Once it is taken care, it gets
Reviewed-by: Chandra Konduru <Chandra.konduru@intel.com>

> 
> >
> > > all the infoframes when turning off the port.
> > >
> > > Also protect against two ports stomping on each other on g4x due to
> > > the single video DIP instance. Now only the first port to enable
> > > gets to send infoframes.
> >
> > So is, 2nd port doesn't gets to send infoframes, the expected behavior
> > when two ports trying with a single DIP?
> 
> I'm not sure what's the best behaviour here. Either we somehow pick one
> of the ports to send infoframes (first come first serve in this patch),
> or we could just disable infoframes entirely for cloned cases. But it's
> probably a rare configuration anyway since HDMI cloning is only allowed
> on g4x.

I am fine with what you outlined above. If anything come up, this can be
revisited.

> 
> >
> > Seems like a corner case to test thoroughly. Is this path exercised in your
> testing?
> 
> I tested it long ago. Although I must admit that the patch looked a bit
> different back then.
> 
> >
> > >
> > > v2: Rebase
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_hdmi.c | 85 ++++++++++++++++++-----------------
> ----
> > >  1 file changed, 40 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > > b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index 766bdb1..03b4759 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -514,7 +514,13 @@ static void g4x_set_infoframes(struct drm_encoder
> > > *encoder,
> > >  	if (!enable) {
> > >  		if (!(val & VIDEO_DIP_ENABLE))
> > >  			return;
> > > -		val &= ~VIDEO_DIP_ENABLE;
> > > +		if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > > +			DRM_DEBUG_KMS("video DIP still enabled on port
> > > %c\n",
> > > +				      (val & VIDEO_DIP_PORT_MASK) >> 29);
> > > +			return;
> > > +		}
> > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > +			 VIDEO_DIP_ENABLE_VENDOR |
> > > VIDEO_DIP_ENABLE_SPD);
> > >  		I915_WRITE(reg, val);
> > >  		POSTING_READ(reg);
> > >  		return;
> > > @@ -522,16 +528,17 @@ static void g4x_set_infoframes(struct
> drm_encoder
> > > *encoder,
> > >
> > >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > >  		if (val & VIDEO_DIP_ENABLE) {
> > > -			val &= ~VIDEO_DIP_ENABLE;
> > > -			I915_WRITE(reg, val);
> > > -			POSTING_READ(reg);
> > > +			DRM_DEBUG_KMS("video DIP already enabled on port
> > > %c\n",
> > > +				      (val & VIDEO_DIP_PORT_MASK) >> 29);
> > > +			return;
> > >  		}
> > >  		val &= ~VIDEO_DIP_PORT_MASK;
> > >  		val |= port;
> > >  	}
> > >
> > >  	val |= VIDEO_DIP_ENABLE;
> > > -	val &= ~VIDEO_DIP_ENABLE_VENDOR;
> > > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);
> > >
> > >  	I915_WRITE(reg, val);
> > >  	POSTING_READ(reg);
> > > @@ -632,23 +639,6 @@ static bool intel_hdmi_set_gcp_infoframe(struct
> > > drm_encoder *encoder)
> > >  	return val != 0;
> > >  }
> > >
> > > -static void intel_disable_gcp_infoframe(struct intel_crtc *crtc)
> > > -{
> > > -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > > -	u32 reg;
> > > -
> > > -	if (HAS_DDI(dev_priv))
> > > -		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> > > -	else if (IS_VALLEYVIEW(dev_priv))
> > > -		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
> > > -	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > > -		reg = TVIDEO_DIP_CTL(crtc->pipe);
> > > -	else
> > > -		return;
> > > -
> > > -	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP);
> > > -}
> > > -
> > >  static void ibx_set_infoframes(struct drm_encoder *encoder,
> > >  			       bool enable,
> > >  			       struct drm_display_mode *adjusted_mode)
> > > @@ -669,25 +659,26 @@ static void ibx_set_infoframes(struct
> drm_encoder
> > > *encoder,
> > >  	if (!enable) {
> > >  		if (!(val & VIDEO_DIP_ENABLE))
> > >  			return;
> > > -		val &= ~VIDEO_DIP_ENABLE;
> > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > +			 VIDEO_DIP_ENABLE_VENDOR |
> VIDEO_DIP_ENABLE_GAMUT |
> > > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > >  		I915_WRITE(reg, val);
> > >  		POSTING_READ(reg);
> > >  		return;
> > >  	}
> > >
> > >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > > -		if (val & VIDEO_DIP_ENABLE) {
> > > -			val &= ~VIDEO_DIP_ENABLE;
> > > -			I915_WRITE(reg, val);
> > > -			POSTING_READ(reg);
> > > -		}
> > > +		WARN(val & VIDEO_DIP_ENABLE,
> > > +		     "DIP already enabled on port %c\n",
> > > +		     (val & VIDEO_DIP_PORT_MASK) >> 29);
> > >  		val &= ~VIDEO_DIP_PORT_MASK;
> > >  		val |= port;
> > >  	}
> > >
> > >  	val |= VIDEO_DIP_ENABLE;
> > > -	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > -		 VIDEO_DIP_ENABLE_GCP);
> > > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > >
> > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > >  		val |= VIDEO_DIP_ENABLE_GCP;
> > > @@ -718,7 +709,9 @@ static void cpt_set_infoframes(struct drm_encoder
> > > *encoder,
> > >  	if (!enable) {
> > >  		if (!(val & VIDEO_DIP_ENABLE))
> > >  			return;
> > > -		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI);
> > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > +			 VIDEO_DIP_ENABLE_VENDOR |
> > > VIDEO_DIP_ENABLE_GAMUT |
> > > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > >  		I915_WRITE(reg, val);
> > >  		POSTING_READ(reg);
> > >  		return;
> > > @@ -727,7 +720,7 @@ static void cpt_set_infoframes(struct drm_encoder
> > > *encoder,
> > >  	/* Set both together, unset both together: see the spec. */
> > >  	val |= VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI;
> > >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > -		 VIDEO_DIP_ENABLE_GCP);
> > > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > >
> > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > >  		val |= VIDEO_DIP_ENABLE_GCP;
> > > @@ -760,25 +753,26 @@ static void vlv_set_infoframes(struct
> drm_encoder
> > > *encoder,
> > >  	if (!enable) {
> > >  		if (!(val & VIDEO_DIP_ENABLE))
> > >  			return;
> > > -		val &= ~VIDEO_DIP_ENABLE;
> > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > +			 VIDEO_DIP_ENABLE_VENDOR |
> VIDEO_DIP_ENABLE_GAMUT |
> > > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > >  		I915_WRITE(reg, val);
> > >  		POSTING_READ(reg);
> > >  		return;
> > >  	}
> > >
> > >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > > -		if (val & VIDEO_DIP_ENABLE) {
> > > -			val &= ~VIDEO_DIP_ENABLE;
> > > -			I915_WRITE(reg, val);
> > > -			POSTING_READ(reg);
> > > -		}
> > > +		WARN(val & VIDEO_DIP_ENABLE,
> > > +		     "DIP already enabled on port %c\n",
> > > +		     (val & VIDEO_DIP_PORT_MASK) >> 29);
> > >  		val &= ~VIDEO_DIP_PORT_MASK;
> > >  		val |= port;
> > >  	}
> > >
> > >  	val |= VIDEO_DIP_ENABLE;
> > > -	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
> > > -		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
> > > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > >
> > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > >  		val |= VIDEO_DIP_ENABLE_GCP;
> > > @@ -803,15 +797,16 @@ static void hsw_set_infoframes(struct
> drm_encoder
> > > *encoder,
> > >
> > >  	assert_hdmi_port_disabled(intel_hdmi);
> > >
> > > +	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW |
> VIDEO_DIP_ENABLE_AVI_HSW |
> > > +		 VIDEO_DIP_ENABLE_GCP_HSW |
> VIDEO_DIP_ENABLE_VS_HSW |
> > > +		 VIDEO_DIP_ENABLE_GMP_HSW |
> VIDEO_DIP_ENABLE_SPD_HSW);
> > > +
> > >  	if (!enable) {
> > > -		I915_WRITE(reg, 0);
> > > +		I915_WRITE(reg, val);
> > >  		POSTING_READ(reg);
> > >  		return;
> > >  	}
> > >
> > > -	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW |
> VIDEO_DIP_ENABLE_GCP_HSW |
> > > -		 VIDEO_DIP_ENABLE_VS_HSW |
> VIDEO_DIP_ENABLE_GMP_HSW);
> > > -
> > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > >  		val |= VIDEO_DIP_ENABLE_GCP_HSW;
> > >
> > > @@ -1133,7 +1128,7 @@ static void intel_disable_hdmi(struct
> intel_encoder
> > > *encoder)
> > >  	if (IS_CHERRYVIEW(dev))
> > >  		chv_powergate_phy_lanes(encoder, 0xf);
> > >
> > > -	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));
> > > +	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
> > >  }
> > >
> > >  static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool
> respect_dvi_limit)
> > > --
> > > 2.0.5
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/9] drm/i915: Enable default_phase in GCP when possible

[Konduru, Chandra]

> > > @@ -560,6 +560,49 @@ static bool hdmi_sink_is_deep_color(struct
> > > drm_encoder *encoder)
> > >  	return false;
> > >  }
> > >
> > > +/*
> > > + * Determine if default_phase=1 can be indicated in the GCP infoframe.
> > > + *
> > > + * From HDMI specification 1.4a:
> > > + * - The first pixel of each Video Data Period shall always have a
> > > +pixel packing phase of 0
> > > + * - The first pixel following each Video Data Period shall have a
> > > +pixel packing phase of 0
> > > + * - The PP bits shall be constant for all GCPs and will be equal to
> > > +the last packing phase
> > > + * - The first pixel following every transition of HSYNC or VSYNC shall have a
> > > pixel packing
> > > + *   phase of 0
> > > + */
> > > +static bool gcp_default_phase_possible(int pipe_bpp,
> > > +				       const struct drm_display_mode *mode) {
> > > +	unsigned int pixels_per_group;
> > > +
> > > +	switch (pipe_bpp) {
> > > +	case 30:
> > > +		/* 4 pixels in 5 clocks */
> > > +		pixels_per_group = 4;
> > > +		break;
> > > +	case 36:
> > > +		/* 2 pixels in 3 clocks */
> > > +		pixels_per_group = 2;
> > > +		break;
> > > +	case 48:
> > > +		/* 1 pixel in 2 clocks */
> > > +		pixels_per_group = 1;
> > > +		break;
> > > +	default:
> > > +		/* phase information not relevant for 8bpc */
> > > +		return false;
> > > +	}
> > > +
> > > +	return mode->crtc_hdisplay % pixels_per_group == 0 &&
> > > +		mode->crtc_htotal % pixels_per_group == 0 &&
> > > +		mode->crtc_hblank_start % pixels_per_group == 0 &&
> > > +		mode->crtc_hblank_end % pixels_per_group == 0 &&
> > > +		mode->crtc_hsync_start % pixels_per_group == 0 &&
> > > +		mode->crtc_hsync_end % pixels_per_group == 0 &&
> >
> > For hsync, bspec says Hsync is an even number.
> > Isn't it above check should be something like (hsync_end - hsync_start) % 2 ==
> 0?
> > And similarly for front & back porches, right?
> 
> If X and Y are even then (X - Y) is even too. Also the text in bspec is
> less informative than the text in HDMI spec, which is why I quited the
> HDMI spec instead.

Sure, if X and Y are even X - Y is even, but it is more restrictive check than
needed. Because if both X and Y are odd, X - Y is even, and in that case
above code doesn't allow to use default phase. Which may be OK, but
it didn't truly allow default phase when possible.


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

Re: [PATCH v2 2/9] drm/i915: Send GCP infoframes for deep color HDMI sinks

[Konduru, Chandra]

> > > @@ -541,6 +541,66 @@ static void g4x_set_infoframes(struct drm_encoder
> > > *encoder,
> > >  	intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode);  }
> > >
> > > +static bool hdmi_sink_is_deep_color(struct drm_encoder *encoder) {
> >
> > Some static functions are prefixed but this isn't. Can you add prefix?
> 
> Is there any benefit from a prefix?

Personally, I am fine either way. But based on review feedback seen so far,
this is being looked at and asked to follow. From that perspective I mentioned that. 
If this is something optional and tied to benefit, then no issue here.

> 
> >
> > > +	struct drm_device *dev = encoder->dev;
> > > +	struct drm_connector *connector;
> > > +
> > > +	WARN_ON(!drm_modeset_is_locked(&dev-
> > > >mode_config.connection_mutex));
> > > +
> > > +	/*
> > > +	 * HDMI cloning is only supported on g4x which doesn't
> > > +	 * support deep color or GCP infoframes anyway so no
> > > +	 * need to worry about multiple HDMI sinks here.
> > > +	 */
> >
> > There isn't any code specific for HDMI cloning or multiple HDMI sinks here,
> > any relevance of above comment. Or am I missing something here?
> 
> In a cloned setup we'd feed multiple sinks from the same transcoder, so
> we'd need to basically check that connector->encoder->crtc matches our crtc
> here, and only return true if all the relevant connectors were deep color
> capable. But since we can ignore cloning we can stop looking once we
> find the connector for our current encoder (ie. HDMI port).

OK, makes sense. Thanks for the background info.

Not sure if you are spinning another version to fix the typo below or prefix above,
otherwise, this gets

Reviewed-by: Chandra Konduru <Chandra.konduru@intel.com>


> 
> >
> > > +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> > > head)
> > > +		if (connector->encoder == encoder)
> > > +			return connector->display_info.bpc > 8;
> > > +
> > > +	return false;
> > > +}
> > > +
> > > +static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder) {
> > > +	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
> > > +	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> > > +	u32 reg, val = 0;
> > > +
> > > +	if (HAS_DDI(dev_priv))
> > > +		reg = HSW_TVIDEO_DIP_GCP(crtc->config->cpu_transcoder);
> > > +	else if (IS_VALLEYVIEW(dev_priv))
> > > +		reg = VLV_TVIDEO_DIP_GCP(crtc->pipe);
> > > +	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > > +		reg = TVIDEO_DIP_GCP(crtc->pipe);
> > > +	else
> > > +		return false;
> > > +
> > > +	/* Indicate color depth wheneven the sink supports deep color */
> >
> > Typo
> >
> > > +	if (hdmi_sink_is_deep_color(encoder))
> > > +		val |= GCP_COLOR_INDICATION;
> > > +
> > > +	I915_WRITE(reg, val);
> > > +
> > > +	return val != 0;
> > > +}
> > > +
> > > +static void intel_disable_gcp_infoframe(struct intel_crtc *crtc) {
> > > +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > > +	u32 reg;
> > > +
> > > +	if (HAS_DDI(dev_priv))
> > > +		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> > > +	else if (IS_VALLEYVIEW(dev_priv))
> > > +		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
> > > +	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > > +		reg = TVIDEO_DIP_CTL(crtc->pipe);
> > > +	else
> > > +		return;
> > > +
> > > +	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP); }
> > > +
> > >  static void ibx_set_infoframes(struct drm_encoder *encoder,
> > >  			       bool enable,
> > >  			       struct drm_display_mode *adjusted_mode) @@ -
> > > 581,6 +641,9 @@ static void ibx_set_infoframes(struct drm_encoder
> *encoder,
> > >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > >  		 VIDEO_DIP_ENABLE_GCP);
> > >
> > > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > +		val |= VIDEO_DIP_ENABLE_GCP;
> > > +
> > >  	I915_WRITE(reg, val);
> > >  	POSTING_READ(reg);
> > >
> > > @@ -618,6 +681,9 @@ static void cpt_set_infoframes(struct drm_encoder
> > > *encoder,
> > >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > >  		 VIDEO_DIP_ENABLE_GCP);
> > >
> > > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > +		val |= VIDEO_DIP_ENABLE_GCP;
> > > +
> > >  	I915_WRITE(reg, val);
> > >  	POSTING_READ(reg);
> > >
> > > @@ -666,6 +732,9 @@ static void vlv_set_infoframes(struct drm_encoder
> > > *encoder,
> > >  	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
> > >  		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
> > >
> > > +	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > +		val |= VIDEO_DIP_ENABLE_GCP;
> > > +
> >
> > There is a comment in the code that
> > "Note that g4x/vlv don't support 12bpc hdmi outputs."
> > Is above call still required for vlv? Or need an update to that comment.
> 
> VLV/CHV support the GCP infoframe even though they don't support
> 12bpc output. The way I decided to do things is to send the GCP
> infoframe whenever there's a deep color sink hooked up, even if
> we feed it 8bpc data.

Got it. Should be ok then.


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

Re: [PATCH v2 6/9] drm/i915: Disable all infoframes when turning off the HDMI port

[Ville Syrjälä]

On Tue, Jun 02, 2015 at 06:18:16PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Tuesday, June 02, 2015 4:11 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH v2 6/9] drm/i915: Disable all infoframes when
> > turning off the HDMI port
> > 
> > On Mon, Jun 01, 2015 at 10:48:03PM +0000, Konduru, Chandra wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> > Of
> > > > ville.syrjala@linux.intel.com
> > > > Sent: Tuesday, May 05, 2015 7:06 AM
> > > > To: intel-gfx@lists.freedesktop.org
> > > > Subject: [Intel-gfx] [PATCH v2 6/9] drm/i915: Disable all infoframes when
> > > > turning off the HDMI port
> > > >
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > Currently we just disable the GCP infoframe when turning off the port.
> > > > That means if the same transcoder is used on a DP port next, we might
> > > > end up pushing infoframes over DP, which isn't intended. Just disable
> > >
> > > Wonder how it is working. May be it is ok, or never hit using a previously
> > > used transcoder for HDMI port for DP.
> > >
> > > By the way, you mean end up pushing "other" infoframes over DP?
> > 
> > We don't send infoframes over DP at all currently. Or I should say we
> > never intended to send them. After this patch that should even be true.
> > Well, unless the BIOS already enables them and we just fire up a DP port
> > using the transcoder in question. So I suppose we should really have the
> > DP code clear the infoframe settings explicitly, or we should clear them
> > during the sanitize phase.
> 
> Agree that DP code path should clear the infoframe settings explicitly.
> As you are already touching this code, will you plan to handle it?

I don't think I'll go there now. It would require quite a bit more work
to expose the infoframe stuff to the DP code. DP + infoframes in general
sounds like a good project for someone else to figure out. A quick
glance at the DP spec suggests that we should at least send the audio
infoframe.

> Once it is taken care, it gets
> Reviewed-by: Chandra Konduru <Chandra.konduru@intel.com>
> 
> > 
> > >
> > > > all the infoframes when turning off the port.
> > > >
> > > > Also protect against two ports stomping on each other on g4x due to
> > > > the single video DIP instance. Now only the first port to enable
> > > > gets to send infoframes.
> > >
> > > So is, 2nd port doesn't gets to send infoframes, the expected behavior
> > > when two ports trying with a single DIP?
> > 
> > I'm not sure what's the best behaviour here. Either we somehow pick one
> > of the ports to send infoframes (first come first serve in this patch),
> > or we could just disable infoframes entirely for cloned cases. But it's
> > probably a rare configuration anyway since HDMI cloning is only allowed
> > on g4x.
> 
> I am fine with what you outlined above. If anything come up, this can be
> revisited.
> 
> > 
> > >
> > > Seems like a corner case to test thoroughly. Is this path exercised in your
> > testing?
> > 
> > I tested it long ago. Although I must admit that the patch looked a bit
> > different back then.
> > 
> > >
> > > >
> > > > v2: Rebase
> > > >
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_hdmi.c | 85 ++++++++++++++++++-----------------
> > ----
> > > >  1 file changed, 40 insertions(+), 45 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > index 766bdb1..03b4759 100644
> > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > @@ -514,7 +514,13 @@ static void g4x_set_infoframes(struct drm_encoder
> > > > *encoder,
> > > >  	if (!enable) {
> > > >  		if (!(val & VIDEO_DIP_ENABLE))
> > > >  			return;
> > > > -		val &= ~VIDEO_DIP_ENABLE;
> > > > +		if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > > > +			DRM_DEBUG_KMS("video DIP still enabled on port
> > > > %c\n",
> > > > +				      (val & VIDEO_DIP_PORT_MASK) >> 29);
> > > > +			return;
> > > > +		}
> > > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > > +			 VIDEO_DIP_ENABLE_VENDOR |
> > > > VIDEO_DIP_ENABLE_SPD);
> > > >  		I915_WRITE(reg, val);
> > > >  		POSTING_READ(reg);
> > > >  		return;
> > > > @@ -522,16 +528,17 @@ static void g4x_set_infoframes(struct
> > drm_encoder
> > > > *encoder,
> > > >
> > > >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > > >  		if (val & VIDEO_DIP_ENABLE) {
> > > > -			val &= ~VIDEO_DIP_ENABLE;
> > > > -			I915_WRITE(reg, val);
> > > > -			POSTING_READ(reg);
> > > > +			DRM_DEBUG_KMS("video DIP already enabled on port
> > > > %c\n",
> > > > +				      (val & VIDEO_DIP_PORT_MASK) >> 29);
> > > > +			return;
> > > >  		}
> > > >  		val &= ~VIDEO_DIP_PORT_MASK;
> > > >  		val |= port;
> > > >  	}
> > > >
> > > >  	val |= VIDEO_DIP_ENABLE;
> > > > -	val &= ~VIDEO_DIP_ENABLE_VENDOR;
> > > > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > > > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);
> > > >
> > > >  	I915_WRITE(reg, val);
> > > >  	POSTING_READ(reg);
> > > > @@ -632,23 +639,6 @@ static bool intel_hdmi_set_gcp_infoframe(struct
> > > > drm_encoder *encoder)
> > > >  	return val != 0;
> > > >  }
> > > >
> > > > -static void intel_disable_gcp_infoframe(struct intel_crtc *crtc)
> > > > -{
> > > > -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > > > -	u32 reg;
> > > > -
> > > > -	if (HAS_DDI(dev_priv))
> > > > -		reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> > > > -	else if (IS_VALLEYVIEW(dev_priv))
> > > > -		reg = VLV_TVIDEO_DIP_CTL(crtc->pipe);
> > > > -	else if (HAS_PCH_SPLIT(dev_priv->dev))
> > > > -		reg = TVIDEO_DIP_CTL(crtc->pipe);
> > > > -	else
> > > > -		return;
> > > > -
> > > > -	I915_WRITE(reg, I915_READ(reg) & ~VIDEO_DIP_ENABLE_GCP);
> > > > -}
> > > > -
> > > >  static void ibx_set_infoframes(struct drm_encoder *encoder,
> > > >  			       bool enable,
> > > >  			       struct drm_display_mode *adjusted_mode)
> > > > @@ -669,25 +659,26 @@ static void ibx_set_infoframes(struct
> > drm_encoder
> > > > *encoder,
> > > >  	if (!enable) {
> > > >  		if (!(val & VIDEO_DIP_ENABLE))
> > > >  			return;
> > > > -		val &= ~VIDEO_DIP_ENABLE;
> > > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > > +			 VIDEO_DIP_ENABLE_VENDOR |
> > VIDEO_DIP_ENABLE_GAMUT |
> > > > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > > >  		I915_WRITE(reg, val);
> > > >  		POSTING_READ(reg);
> > > >  		return;
> > > >  	}
> > > >
> > > >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > > > -		if (val & VIDEO_DIP_ENABLE) {
> > > > -			val &= ~VIDEO_DIP_ENABLE;
> > > > -			I915_WRITE(reg, val);
> > > > -			POSTING_READ(reg);
> > > > -		}
> > > > +		WARN(val & VIDEO_DIP_ENABLE,
> > > > +		     "DIP already enabled on port %c\n",
> > > > +		     (val & VIDEO_DIP_PORT_MASK) >> 29);
> > > >  		val &= ~VIDEO_DIP_PORT_MASK;
> > > >  		val |= port;
> > > >  	}
> > > >
> > > >  	val |= VIDEO_DIP_ENABLE;
> > > > -	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > > -		 VIDEO_DIP_ENABLE_GCP);
> > > > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > > > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > > >
> > > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > >  		val |= VIDEO_DIP_ENABLE_GCP;
> > > > @@ -718,7 +709,9 @@ static void cpt_set_infoframes(struct drm_encoder
> > > > *encoder,
> > > >  	if (!enable) {
> > > >  		if (!(val & VIDEO_DIP_ENABLE))
> > > >  			return;
> > > > -		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI);
> > > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > > +			 VIDEO_DIP_ENABLE_VENDOR |
> > > > VIDEO_DIP_ENABLE_GAMUT |
> > > > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > > >  		I915_WRITE(reg, val);
> > > >  		POSTING_READ(reg);
> > > >  		return;
> > > > @@ -727,7 +720,7 @@ static void cpt_set_infoframes(struct drm_encoder
> > > > *encoder,
> > > >  	/* Set both together, unset both together: see the spec. */
> > > >  	val |= VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI;
> > > >  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > > -		 VIDEO_DIP_ENABLE_GCP);
> > > > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > > >
> > > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > >  		val |= VIDEO_DIP_ENABLE_GCP;
> > > > @@ -760,25 +753,26 @@ static void vlv_set_infoframes(struct
> > drm_encoder
> > > > *encoder,
> > > >  	if (!enable) {
> > > >  		if (!(val & VIDEO_DIP_ENABLE))
> > > >  			return;
> > > > -		val &= ~VIDEO_DIP_ENABLE;
> > > > +		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > > +			 VIDEO_DIP_ENABLE_VENDOR |
> > VIDEO_DIP_ENABLE_GAMUT |
> > > > +			 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > > >  		I915_WRITE(reg, val);
> > > >  		POSTING_READ(reg);
> > > >  		return;
> > > >  	}
> > > >
> > > >  	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> > > > -		if (val & VIDEO_DIP_ENABLE) {
> > > > -			val &= ~VIDEO_DIP_ENABLE;
> > > > -			I915_WRITE(reg, val);
> > > > -			POSTING_READ(reg);
> > > > -		}
> > > > +		WARN(val & VIDEO_DIP_ENABLE,
> > > > +		     "DIP already enabled on port %c\n",
> > > > +		     (val & VIDEO_DIP_PORT_MASK) >> 29);
> > > >  		val &= ~VIDEO_DIP_PORT_MASK;
> > > >  		val |= port;
> > > >  	}
> > > >
> > > >  	val |= VIDEO_DIP_ENABLE;
> > > > -	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
> > > > -		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
> > > > +	val &= ~(VIDEO_DIP_ENABLE_AVI |
> > > > +		 VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > > +		 VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > > >
> > > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > >  		val |= VIDEO_DIP_ENABLE_GCP;
> > > > @@ -803,15 +797,16 @@ static void hsw_set_infoframes(struct
> > drm_encoder
> > > > *encoder,
> > > >
> > > >  	assert_hdmi_port_disabled(intel_hdmi);
> > > >
> > > > +	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW |
> > VIDEO_DIP_ENABLE_AVI_HSW |
> > > > +		 VIDEO_DIP_ENABLE_GCP_HSW |
> > VIDEO_DIP_ENABLE_VS_HSW |
> > > > +		 VIDEO_DIP_ENABLE_GMP_HSW |
> > VIDEO_DIP_ENABLE_SPD_HSW);
> > > > +
> > > >  	if (!enable) {
> > > > -		I915_WRITE(reg, 0);
> > > > +		I915_WRITE(reg, val);
> > > >  		POSTING_READ(reg);
> > > >  		return;
> > > >  	}
> > > >
> > > > -	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW |
> > VIDEO_DIP_ENABLE_GCP_HSW |
> > > > -		 VIDEO_DIP_ENABLE_VS_HSW |
> > VIDEO_DIP_ENABLE_GMP_HSW);
> > > > -
> > > >  	if (intel_hdmi_set_gcp_infoframe(encoder))
> > > >  		val |= VIDEO_DIP_ENABLE_GCP_HSW;
> > > >
> > > > @@ -1133,7 +1128,7 @@ static void intel_disable_hdmi(struct
> > intel_encoder
> > > > *encoder)
> > > >  	if (IS_CHERRYVIEW(dev))
> > > >  		chv_powergate_phy_lanes(encoder, 0xf);
> > > >
> > > > -	intel_disable_gcp_infoframe(to_intel_crtc(encoder->base.crtc));
> > > > +	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
> > > >  }
> > > >
> > > >  static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool
> > respect_dvi_limit)
> > > > --
> > > > 2.0.5
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Ville Syrjälä
> > Intel OTC

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

Re: [PATCH v2 3/9] drm/i915: Enable default_phase in GCP when possible

[Ville Syrjälä]

On Tue, Jun 02, 2015 at 06:21:59PM +0000, Konduru, Chandra wrote:
> > > > @@ -560,6 +560,49 @@ static bool hdmi_sink_is_deep_color(struct
> > > > drm_encoder *encoder)
> > > >  	return false;
> > > >  }
> > > >
> > > > +/*
> > > > + * Determine if default_phase=1 can be indicated in the GCP infoframe.
> > > > + *
> > > > + * From HDMI specification 1.4a:
> > > > + * - The first pixel of each Video Data Period shall always have a
> > > > +pixel packing phase of 0
> > > > + * - The first pixel following each Video Data Period shall have a
> > > > +pixel packing phase of 0
> > > > + * - The PP bits shall be constant for all GCPs and will be equal to
> > > > +the last packing phase
> > > > + * - The first pixel following every transition of HSYNC or VSYNC shall have a
> > > > pixel packing
> > > > + *   phase of 0
> > > > + */
> > > > +static bool gcp_default_phase_possible(int pipe_bpp,
> > > > +				       const struct drm_display_mode *mode) {
> > > > +	unsigned int pixels_per_group;
> > > > +
> > > > +	switch (pipe_bpp) {
> > > > +	case 30:
> > > > +		/* 4 pixels in 5 clocks */
> > > > +		pixels_per_group = 4;
> > > > +		break;
> > > > +	case 36:
> > > > +		/* 2 pixels in 3 clocks */
> > > > +		pixels_per_group = 2;
> > > > +		break;
> > > > +	case 48:
> > > > +		/* 1 pixel in 2 clocks */
> > > > +		pixels_per_group = 1;
> > > > +		break;
> > > > +	default:
> > > > +		/* phase information not relevant for 8bpc */
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	return mode->crtc_hdisplay % pixels_per_group == 0 &&
> > > > +		mode->crtc_htotal % pixels_per_group == 0 &&
> > > > +		mode->crtc_hblank_start % pixels_per_group == 0 &&
> > > > +		mode->crtc_hblank_end % pixels_per_group == 0 &&
> > > > +		mode->crtc_hsync_start % pixels_per_group == 0 &&
> > > > +		mode->crtc_hsync_end % pixels_per_group == 0 &&
> > >
> > > For hsync, bspec says Hsync is an even number.
> > > Isn't it above check should be something like (hsync_end - hsync_start) % 2 ==
> > 0?
> > > And similarly for front & back porches, right?
> > 
> > If X and Y are even then (X - Y) is even too. Also the text in bspec is
> > less informative than the text in HDMI spec, which is why I quited the
> > HDMI spec instead.
> 
> Sure, if X and Y are even X - Y is even, but it is more restrictive check than
> needed. Because if both X and Y are odd, X - Y is even, and in that case
> above code doesn't allow to use default phase. Which may be OK, but
> it didn't truly allow default phase when possible.

Default phase should not be enabled in that case. As the HDMI spec says
"The first pixel following every transition of HSYNC or VSYNC shall have
a pixel packing phase of 0", so having a misaligned hsync_start or
hsync_end is not allowed.

Or you can also read bspec. While the text is less clear there IMO it
does disallow the case you outlined. What bspec does say is this:
"Htotal is an even number
 Hactive is an even number
 Hsync is an even number
 Front and back porches for Hsync are even numbers
 Vsync always starts on an even-numbered pixel within a line in
 interlaced modes (starting counting with 0)"

That doesn't allow hsync_start or hsync_end to be odd either.

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

Re: [PATCH v2 3/9] drm/i915: Enable default_phase in GCP when possible

[Konduru, Chandra]

> > > > > @@ -560,6 +560,49 @@ static bool hdmi_sink_is_deep_color(struct
> > > > > drm_encoder *encoder)
> > > > >  	return false;
> > > > >  }
> > > > >
> > > > > +/*
> > > > > + * Determine if default_phase=1 can be indicated in the GCP infoframe.
> > > > > + *
> > > > > + * From HDMI specification 1.4a:
> > > > > + * - The first pixel of each Video Data Period shall always have a
> > > > > +pixel packing phase of 0
> > > > > + * - The first pixel following each Video Data Period shall have a
> > > > > +pixel packing phase of 0
> > > > > + * - The PP bits shall be constant for all GCPs and will be equal to
> > > > > +the last packing phase
> > > > > + * - The first pixel following every transition of HSYNC or VSYNC shall
> have a
> > > > > pixel packing
> > > > > + *   phase of 0
> > > > > + */
> > > > > +static bool gcp_default_phase_possible(int pipe_bpp,
> > > > > +				       const struct drm_display_mode
> *mode) {
> > > > > +	unsigned int pixels_per_group;
> > > > > +
> > > > > +	switch (pipe_bpp) {
> > > > > +	case 30:
> > > > > +		/* 4 pixels in 5 clocks */
> > > > > +		pixels_per_group = 4;
> > > > > +		break;
> > > > > +	case 36:
> > > > > +		/* 2 pixels in 3 clocks */
> > > > > +		pixels_per_group = 2;
> > > > > +		break;
> > > > > +	case 48:
> > > > > +		/* 1 pixel in 2 clocks */
> > > > > +		pixels_per_group = 1;
> > > > > +		break;
> > > > > +	default:
> > > > > +		/* phase information not relevant for 8bpc */
> > > > > +		return false;
> > > > > +	}
> > > > > +
> > > > > +	return mode->crtc_hdisplay % pixels_per_group == 0 &&
> > > > > +		mode->crtc_htotal % pixels_per_group == 0 &&
> > > > > +		mode->crtc_hblank_start % pixels_per_group == 0 &&
> > > > > +		mode->crtc_hblank_end % pixels_per_group == 0 &&
> > > > > +		mode->crtc_hsync_start % pixels_per_group == 0 &&
> > > > > +		mode->crtc_hsync_end % pixels_per_group == 0 &&
> > > >
> > > > For hsync, bspec says Hsync is an even number.
> > > > Isn't it above check should be something like (hsync_end - hsync_start) % 2
> ==
> > > 0?
> > > > And similarly for front & back porches, right?
> > >
> > > If X and Y are even then (X - Y) is even too. Also the text in bspec is
> > > less informative than the text in HDMI spec, which is why I quited the
> > > HDMI spec instead.
> >
> > Sure, if X and Y are even X - Y is even, but it is more restrictive check than
> > needed. Because if both X and Y are odd, X - Y is even, and in that case
> > above code doesn't allow to use default phase. Which may be OK, but
> > it didn't truly allow default phase when possible.
> 
> Default phase should not be enabled in that case. As the HDMI spec says
> "The first pixel following every transition of HSYNC or VSYNC shall have
> a pixel packing phase of 0", so having a misaligned hsync_start or
> hsync_end is not allowed.

OK. Thanks for clarification!
With that, it gets
Reviewed-by: Chandra Konduru <Chandra.konduru@intel.com>

> 
> Or you can also read bspec. While the text is less clear there IMO it
> does disallow the case you outlined. What bspec does say is this:
> "Htotal is an even number
>  Hactive is an even number
>  Hsync is an even number
>  Front and back porches for Hsync are even numbers
>  Vsync always starts on an even-numbered pixel within a line in
>  interlaced modes (starting counting with 0)"
> 
> That doesn't allow hsync_start or hsync_end to be odd either.
> 
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 5/9] drm/i915: Fix 12bpc HDMI enable for IBX

[Konduru, Chandra]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> ville.syrjala@linux.intel.com
> Sent: Tuesday, May 05, 2015 7:06 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH v2 5/9] drm/i915: Fix 12bpc HDMI enable for IBX
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Follow the procedure listed in Bspec to toggle the port enable bit off
> and on when enabling HDMI with 12bpc and pixel repeat on IBX. The old
> code didn't actually enable the port before "toggling" the bit back off,
> so the whole workaround was essentially a nop.
> 
> Also take the opportunity to clarify the code by splitting the gmch
> platforms to a separate (much more straightforward) function.

Per prior reviews seen before, it would be better if there are
separate patches for fixing issue and refactoring code.

I think it is fine either way, but is bit easy for reviewing code
having separate patches.

With that,
Reviewed-by: Chandra Konduru <Chandra.konduru@intel.com>

> 
> v2: Rebased due to crtc->config changes
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 78 ++++++++++++++++++++++++++---------
> ----
>  1 file changed, 53 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index 2e98e33..766bdb1 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -944,47 +944,73 @@ static void intel_enable_hdmi_audio(struct
> intel_encoder *encoder)
>  	intel_audio_codec_enable(encoder);
>  }
> 
> -static void intel_enable_hdmi(struct intel_encoder *encoder)
> +static void g4x_enable_hdmi(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_crtc *crtc = to_intel_crtc(encoder->base.crtc);

Unrelated change

>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
>  	u32 temp;
> -	u32 enable_bits = SDVO_ENABLE;
> -
> -	if (intel_crtc->config->has_audio)
> -		enable_bits |= SDVO_AUDIO_ENABLE;
> 
>  	temp = I915_READ(intel_hdmi->hdmi_reg);
> 
> -	/* HW workaround for IBX, we need to move the port to transcoder A
> -	 * before disabling it, so restore the transcoder select bit here. */
> -	if (HAS_PCH_IBX(dev))
> -		enable_bits |= SDVO_PIPE_SEL(intel_crtc->pipe);
> +	temp |= SDVO_ENABLE;
> +	if (crtc->config->has_audio)
> +		temp |= SDVO_AUDIO_ENABLE;
> 
> -	/* HW workaround, need to toggle enable bit off and on for 12bpc, but
> -	 * we do this anyway which shows more stable in testing.
> -	 */
> -	if (HAS_PCH_SPLIT(dev)) {
> -		I915_WRITE(intel_hdmi->hdmi_reg, temp & ~SDVO_ENABLE);
> -		POSTING_READ(intel_hdmi->hdmi_reg);
> -	}
> +	I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +	POSTING_READ(intel_hdmi->hdmi_reg);
> 
> -	temp |= enable_bits;
> +	if (crtc->config->has_audio)
> +		intel_enable_hdmi_audio(encoder);
> +}
> 
> +static void ibx_enable_hdmi(struct intel_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> +	u32 temp;
> +
> +	temp = I915_READ(intel_hdmi->hdmi_reg);
> +
> +	temp |= SDVO_ENABLE;
> +	if (crtc->config->has_audio)
> +		temp |= SDVO_AUDIO_ENABLE;
> +
> +	/*
> +	 * HW workaround, need to write this twice for issue
> +	 * that may result in first write getting masked.
> +	 */
> +	I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +	POSTING_READ(intel_hdmi->hdmi_reg);
>  	I915_WRITE(intel_hdmi->hdmi_reg, temp);
>  	POSTING_READ(intel_hdmi->hdmi_reg);
> 
> -	/* HW workaround, need to write this twice for issue that may result
> -	 * in first write getting masked.
> +	/*
> +	 * HW workaround, need to toggle enable bit off and on
> +	 * for 12bpc with pixel repeat.
> +	 *
> +	 * FIXME: BSpec says this should be done at the end of
> +	 * of the modeset sequence, so not sure if this isn't too soon.
>  	 */
> -	if (HAS_PCH_SPLIT(dev)) {
> +	if (crtc->config->pipe_bpp > 24 &&
> +	    crtc->config->pixel_multiplier > 1) {
> +		I915_WRITE(intel_hdmi->hdmi_reg, temp & ~SDVO_ENABLE);
> +		POSTING_READ(intel_hdmi->hdmi_reg);
> +
> +		/*
> +		 * HW workaround, need to write this twice for issue
> +		 * that may result in first write getting masked.
> +		 */
> +		I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +		POSTING_READ(intel_hdmi->hdmi_reg);
>  		I915_WRITE(intel_hdmi->hdmi_reg, temp);
>  		POSTING_READ(intel_hdmi->hdmi_reg);
>  	}
> 
> -	if (intel_crtc->config->has_audio)
> +	if (crtc->config->has_audio)
>  		intel_enable_hdmi_audio(encoder);
>  }
> 
> @@ -1509,7 +1535,7 @@ static void vlv_hdmi_pre_enable(struct intel_encoder
> *encoder)
>  				   intel_crtc->config->has_hdmi_sink,
>  				   adjusted_mode);
> 
> -	intel_enable_hdmi(encoder);
> +	g4x_enable_hdmi(encoder);
> 
>  	vlv_wait_port_ready(dev_priv, dport, 0x0);
>  }
> @@ -1828,7 +1854,7 @@ static void chv_hdmi_pre_enable(struct intel_encoder
> *encoder)
> 
>  	chv_powergate_phy_lanes(encoder, 0x0);
> 
> -	intel_enable_hdmi(encoder);
> +	g4x_enable_hdmi(encoder);
> 
>  	vlv_wait_port_ready(dev_priv, dport, 0x0);
>  }
> @@ -2012,8 +2038,10 @@ void intel_hdmi_init(struct drm_device *dev, int
> hdmi_reg, enum port port)
>  		intel_encoder->pre_enable = intel_hdmi_pre_enable;
>  		if (HAS_PCH_CPT(dev))
>  			intel_encoder->enable = cpt_enable_hdmi;
> +		else if (HAS_PCH_IBX(dev))
> +			intel_encoder->enable = ibx_enable_hdmi;
>  		else
> -			intel_encoder->enable = intel_enable_hdmi;
> +			intel_encoder->enable = g4x_enable_hdmi;
>  	}
> 
>  	intel_encoder->type = INTEL_OUTPUT_HDMI;
> --
> 2.0.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 6/9] drm/i915: Disable all infoframes when turning off the HDMI port

[Konduru, Chandra]

> > > > > Currently we just disable the GCP infoframe when turning off the port.
> > > > > That means if the same transcoder is used on a DP port next, we might
> > > > > end up pushing infoframes over DP, which isn't intended. Just disable
> > > >
> > > > Wonder how it is working. May be it is ok, or never hit using a previously
> > > > used transcoder for HDMI port for DP.
> > > >
> > > > By the way, you mean end up pushing "other" infoframes over DP?
> > >
> > > We don't send infoframes over DP at all currently. Or I should say we
> > > never intended to send them. After this patch that should even be true.
> > > Well, unless the BIOS already enables them and we just fire up a DP port
> > > using the transcoder in question. So I suppose we should really have the
> > > DP code clear the infoframe settings explicitly, or we should clear them
> > > during the sanitize phase.
> >
> > Agree that DP code path should clear the infoframe settings explicitly.
> > As you are already touching this code, will you plan to handle it?
> 
> I don't think I'll go there now. It would require quite a bit more work
> to expose the infoframe stuff to the DP code. DP + infoframes in general
> sounds like a good project for someone else to figure out. A quick
> glance at the DP spec suggests that we should at least send the audio
> infoframe.

OK, may be binned for future.


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

Re: [PATCH 0/9] drm/i915: HDMI 12bpc fixes

[Daniel Vetter]

On Mon, Jun 01, 2015 at 07:04:07PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> > ville.syrjala@linux.intel.com
> > Sent: Tuesday, May 05, 2015 7:06 AM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH 0/9] drm/i915: HDMI 12bpc fixes
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Our HDMI 12bpc support has always been broken. This series aims to fix that.
> > 
> > The problems addressed include:
> > - missing GCP infoframes entirely
> > - IBX w/a code was mostly nonsense
> > - missing w/a for CPT/PPT
> > - 12bpc vs. DBLCLK was busted
> > 
> > Part of this was already posted [1] quite a while ago, but it's grown some new
> > stuff since. The entire series is available in git [2].
> > 
> > I have additional stuff to fix the IBX transcoder B workarounds and some other
> > random thigns on PCH platforms. I'll post that as a followup.
> > 
> > [1] http://lists.freedesktop.org/archives/intel-gfx/2014-October/053340.html
> > [2] hit://github.com/vsyrjala/linux.git hdmi_12bpc_fixes_9
> 
> Broken link [2]. Some typo?

s/hit/git and then you have a git url ;-)

Anyway thanks for patches&review, all merged to dinq.
-Daniel

> 
> > 
> > Ville Syrjälä (9):
> >   drm/i915: Implement WaEnableHDMI8bpcBefore12bpc:snb,ivb
> >   drm/i915: Send GCP infoframes for deep color HDMI sinks
> >   drm/i915: Enable default_phase in GCP when possible
> >   drm/i915: Fix HDMI 12bpc TRANSCONF bpc value
> >   drm/i915: Fix 12bpc HDMI enable for IBX
> >   drm/i915: Disable all infoframes when turning off the HDMI port
> >   drm/i915: Check infoframe state more diligently.
> >   drm/i915: Fix hdmi clock readout with pixel repeat
> >   drm/i915: Double the port clock when using double clocked modes with
> >     12bpc
> > 
> >  drivers/gpu/drm/i915/i915_reg.h      |   4 +
> >  drivers/gpu/drm/i915/intel_display.c |  10 +-
> >  drivers/gpu/drm/i915/intel_hdmi.c    | 359 ++++++++++++++++++++++++++++---
> > ----
> >  3 files changed, 306 insertions(+), 67 deletions(-)
> > 
> > --
> > 2.0.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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