[PATCH 0/5] YCBCR 4:2:0 output support for LSPCON

[PATCH 0/5] YCBCR 4:2:0 output support for LSPCON

[Shashank Sharma]

This patch series adds YCBCR 4:2:0 output support for LSPCON displays.
In order to indicate the color format of output, to the LSPCON device,
a source has to set and send proper AVI infoframes to LSPCON. So this
patch series:
- first adds AVI infoframes support for LSPCON
- then adds YCBCR 4:2:0 output support for LSPCON

A previous version of this series and its review can be found here:
https://patchwork.freedesktop.org/series/28536/
As few of the base patches got merged already, sending this as new series.

Shashank Sharma (5):
  drm/i915: check LSPCON vendor OUI
  drm/i915: Add AVI infoframe support for LSPCON
  drm/i915: Write AVI infoframes for MCA LSPCON
  drm/i915: Write AVI infoframes for Parade LSPCON
  drm/i915: Add YCBCR 4:2:0 support for LSPCON

 drivers/gpu/drm/i915/i915_reg.h      |   2 +
 drivers/gpu/drm/i915/intel_ddi.c     |  24 ++-
 drivers/gpu/drm/i915/intel_display.c |  15 +-
 drivers/gpu/drm/i915/intel_dp.c      |  13 +-
 drivers/gpu/drm/i915/intel_drv.h     |  28 ++-
 drivers/gpu/drm/i915/intel_hdmi.c    |  15 +-
 drivers/gpu/drm/i915/intel_lspcon.c  | 341 +++++++++++++++++++++++++++++++++--
 7 files changed, 411 insertions(+), 27 deletions(-)

-- 
2.7.4

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

[PATCH 1/5] drm/i915: check LSPCON vendor OUI

[Shashank Sharma]

Intel LSPCON chip is provided by 2 vendors:
- Megachips America (MCA)
- Parade technologies (Parade tech)

Its important to know the vendor of this chip, as the address to
write AVI infoframes is different for those two.

This patch reads the vendor OUI signature, and marks into LSPCON
encoder structure for future usages.

This patch also does a small re-arrangement of the code, by moving
lspcon mode change into probe function.

V2: Use dp->desc for OUI detection, dont add a helper for this
    (Ville)

Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h    |  6 ++++
 drivers/gpu/drm/i915/intel_lspcon.c | 69 +++++++++++++++++++++++++++++--------
 2 files changed, 61 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 00b4886..0741494 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1049,9 +1049,15 @@ struct intel_dp {
 	struct intel_dp_compliance compliance;
 };
 
+enum lspcon_vendor {
+	LSPCON_VENDOR_MCA,
+	LSPCON_VENDOR_PARADE
+};
+
 struct intel_lspcon {
 	bool active;
 	enum drm_lspcon_mode mode;
+	enum lspcon_vendor vendor;
 };
 
 struct intel_digital_port {
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index dcbc786..946dfd0 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -27,6 +27,10 @@
 #include <drm/drm_dp_dual_mode_helper.h>
 #include "intel_drv.h"
 
+/* LSPCON OUI Vendor ID(signatures) */
+#define LSPCON_VENDOR_PARADE_OUI 0x001CF8
+#define LSPCON_VENDOR_MCA_OUI 0x0060AD
+
 static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
 {
 	struct intel_digital_port *dig_port =
@@ -50,6 +54,40 @@ static const char *lspcon_mode_name(enum drm_lspcon_mode mode)
 	}
 }
 
+static bool lspcon_detect_vendor(struct intel_lspcon *lspcon)
+{
+	struct intel_dp *dp = lspcon_to_intel_dp(lspcon);
+	struct drm_dp_dpcd_ident *ident;
+	u32 vendor_oui;
+
+	if (drm_dp_read_desc(&dp->aux, &dp->desc, drm_dp_is_branch(dp->dpcd))) {
+		DRM_ERROR("Can't read description\n");
+		return false;
+	}
+
+	ident = &dp->desc.ident;
+	vendor_oui = (ident->oui[0] << 16) | (ident->oui[1] << 8) |
+		      ident->oui[2];
+
+	switch (vendor_oui) {
+	case LSPCON_VENDOR_MCA_OUI:
+		lspcon->vendor = LSPCON_VENDOR_MCA;
+		DRM_DEBUG_KMS("Vendor: Mega Chips\n");
+		break;
+
+	case LSPCON_VENDOR_PARADE_OUI:
+		lspcon->vendor = LSPCON_VENDOR_PARADE;
+		DRM_DEBUG_KMS("Vendor: Parade Tech\n");
+		break;
+
+	default:
+		DRM_ERROR("Invalid/Unknown vendor OUI\n");
+		return false;
+	}
+
+	return true;
+}
+
 static enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
 {
 	enum drm_lspcon_mode current_mode;
@@ -159,7 +197,18 @@ static bool lspcon_probe(struct intel_lspcon *lspcon)
 	/* Yay ... got a LSPCON device */
 	DRM_DEBUG_KMS("LSPCON detected\n");
 	lspcon->mode = lspcon_wait_mode(lspcon, expected_mode);
-	lspcon->active = true;
+
+	/*
+	 * In the SW state machine, lets Put LSPCON in PCON mode only.
+	 * In this way, it will work with both HDMI 1.4 sinks as well as HDMI
+	 * 2.0 sinks.
+	 */
+	if (lspcon->active && lspcon->mode != DRM_LSPCON_MODE_PCON) {
+		if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) {
+			DRM_ERROR("LSPCON mode change to PCON failed\n");
+			return false;
+		}
+	}
 	return true;
 }
 
@@ -231,25 +280,17 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
 		return false;
 	}
 
-	/*
-	* In the SW state machine, lets Put LSPCON in PCON mode only.
-	* In this way, it will work with both HDMI 1.4 sinks as well as HDMI
-	* 2.0 sinks.
-	*/
-	if (lspcon->active && lspcon->mode != DRM_LSPCON_MODE_PCON) {
-		if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) {
-			DRM_ERROR("LSPCON mode change to PCON failed\n");
-			return false;
-		}
-	}
-
 	if (!intel_dp_read_dpcd(dp)) {
 		DRM_ERROR("LSPCON DPCD read failed\n");
 		return false;
 	}
 
-	drm_dp_read_desc(&dp->aux, &dp->desc, drm_dp_is_branch(dp->dpcd));
+	if (!lspcon_detect_vendor(lspcon)) {
+		DRM_ERROR("LSPCON vendor detection failed\n");
+		return false;
+	}
 
+	lspcon->active = true;
 	DRM_DEBUG_KMS("Success: LSPCON init\n");
 	return true;
 }
-- 
2.7.4

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

[PATCH 2/5] drm/i915: Add AVI infoframe support for LSPCON

[Shashank Sharma]

In order to pass AVI infoframes to LSPCON devices, a source has to
write them in a vendor recommended method and location.

This patch series:
- adds generic LSPCON infoframe setup functions.
- registers these functions into existing AVI infoframe framework.
- triggers these functions from modeset sequence.

Next patches in the series will add vendor specific code.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c    | 16 ++++++++---
 drivers/gpu/drm/i915/intel_dp.c     |  8 +++++-
 drivers/gpu/drm/i915/intel_drv.h    | 16 ++++++++++-
 drivers/gpu/drm/i915/intel_hdmi.c   | 13 ++++++---
 drivers/gpu/drm/i915/intel_lspcon.c | 53 +++++++++++++++++++++++++++++++++++++
 5 files changed, 97 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ace674c..9e2ab02 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2231,10 +2231,19 @@ static void intel_ddi_pre_enable(struct intel_encoder *encoder,
 
 	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
 
-	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
+	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
 		intel_ddi_pre_enable_hdmi(encoder, crtc_state, conn_state);
-	else
+	} else {
 		intel_ddi_pre_enable_dp(encoder, crtc_state, conn_state);
+		if (crtc_state->lspcon_active) {
+			struct intel_digital_port *dig_port =
+					enc_to_dig_port(&encoder->base);
+
+			dig_port->set_infoframes(&encoder->base,
+						 crtc_state->has_infoframe,
+						 crtc_state, conn_state);
+		}
+	}
 }
 
 static void intel_disable_ddi_buf(struct intel_encoder *encoder)
@@ -2886,8 +2895,6 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
 	intel_encoder->cloneable = 0;
 
-	intel_infoframe_init(intel_dig_port);
-
 	if (init_dp) {
 		if (!intel_ddi_init_dp_connector(intel_dig_port))
 			goto err;
@@ -2917,6 +2924,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 				port_name(port));
 	}
 
+	intel_infoframe_init(intel_dig_port);
 	return;
 
 err:
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d27c014..fa9e5e6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1633,7 +1633,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
-	enum port port = dp_to_dig_port(intel_dp)->port;
+	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
+	enum port port = dig_port->port;
+	struct intel_lspcon *lspcon = &dig_port->lspcon;
 	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	struct intel_digital_connector_state *intel_conn_state =
@@ -1654,6 +1656,10 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	common_len = intel_dp_common_len_rate_limit(intel_dp,
 						    intel_dp->max_link_rate);
 
+	/* LSPCON needs special handling to drive YCBCR420 outputs */
+	if (lspcon->active)
+		pipe_config->lspcon_active = true;
+
 	/* No common link rates between source and sink */
 	WARN_ON(common_len <= 0);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0741494..a7c1c19 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -798,6 +798,9 @@ struct intel_crtc_state {
 
 	/* output format is YCBCR 4:2:0 */
 	bool ycbcr420;
+
+	/* LSPCON is active on port */
+	bool lspcon_active;
 };
 
 struct intel_crtc {
@@ -1184,6 +1187,12 @@ static inline struct intel_dp *enc_to_intel_dp(struct drm_encoder *encoder)
 	return &enc_to_dig_port(encoder)->dp;
 }
 
+static inline struct intel_lspcon *
+enc_to_intel_lspcon(struct drm_encoder *encoder)
+{
+	return &enc_to_dig_port(encoder)->lspcon;
+}
+
 static inline struct intel_digital_port *
 dp_to_dig_port(struct intel_dp *intel_dp)
 {
@@ -1696,7 +1705,6 @@ void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
 void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
 void intel_infoframe_init(struct intel_digital_port *intel_dig_port);
 
-
 /* intel_lvds.c */
 void intel_lvds_init(struct drm_i915_private *dev_priv);
 struct intel_encoder *intel_get_lvds_encoder(struct drm_device *dev);
@@ -2029,6 +2037,12 @@ void intel_color_load_luts(struct drm_crtc_state *crtc_state);
 bool lspcon_init(struct intel_digital_port *intel_dig_port);
 void lspcon_resume(struct intel_lspcon *lspcon);
 void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
+void lspcon_set_infoframes(struct drm_encoder *encoder,
+			   bool enable,
+			   const struct intel_crtc_state *crtc_state,
+			   const struct drm_connector_state *conn_state);
+bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
+			      const struct intel_crtc_state *pipe_config);
 
 /* intel_pipe_crc.c */
 int intel_pipe_crc_create(struct drm_minor *minor);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index fa1c793..023015c 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1994,9 +1994,16 @@ void intel_infoframe_init(struct intel_digital_port *intel_dig_port)
 		intel_dig_port->set_infoframes = g4x_set_infoframes;
 		intel_dig_port->infoframe_enabled = g4x_infoframe_enabled;
 	} else if (HAS_DDI(dev_priv)) {
-		intel_dig_port->write_infoframe = hsw_write_infoframe;
-		intel_dig_port->set_infoframes = hsw_set_infoframes;
-		intel_dig_port->infoframe_enabled = hsw_infoframe_enabled;
+		if (intel_dig_port->lspcon.active) {
+			intel_dig_port->set_infoframes = lspcon_set_infoframes;
+			intel_dig_port->infoframe_enabled =
+						lspcon_infoframe_enabled;
+		} else {
+			intel_dig_port->set_infoframes = hsw_set_infoframes;
+			intel_dig_port->infoframe_enabled =
+						hsw_infoframe_enabled;
+			intel_dig_port->write_infoframe = hsw_write_infoframe;
+		}
 	} else if (HAS_PCH_IBX(dev_priv)) {
 		intel_dig_port->write_infoframe = ibx_write_infoframe;
 		intel_dig_port->set_infoframes = ibx_set_infoframes;
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 946dfd0..bb1365a 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -235,6 +235,59 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
 	DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
 }
 
+void lspcon_set_infoframes(struct drm_encoder *encoder,
+			   bool enable,
+			   const struct intel_crtc_state *crtc_state,
+			   const struct drm_connector_state *conn_state)
+{
+	ssize_t ret;
+	union hdmi_infoframe frame;
+	uint8_t buf[VIDEO_DIP_DATA_SIZE];
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	struct intel_dp *intel_dp = &dig_port->dp;
+	struct drm_connector *connector = &intel_dp->attached_connector->base;
+	const struct drm_display_mode *mode = &crtc_state->base.adjusted_mode;
+	bool is_hdmi2_sink = connector->display_info.hdmi.scdc.supported;
+
+	if (!crtc_state->lspcon_active) {
+		DRM_ERROR("Writing infoframes while LSPCON disabled ?\n");
+		return;
+	}
+
+	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
+						       mode, is_hdmi2_sink);
+	if (ret < 0) {
+		DRM_ERROR("couldn't fill AVI infoframe\n");
+		return;
+	}
+
+	if (crtc_state->ycbcr420)
+		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
+	else
+		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
+
+	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
+					   crtc_state->limited_color_range ?
+					   HDMI_QUANTIZATION_RANGE_LIMITED :
+					   HDMI_QUANTIZATION_RANGE_FULL,
+					   false);
+
+	ret = hdmi_infoframe_pack(&frame, buf, sizeof(buf));
+	if (ret < 0) {
+		DRM_ERROR("Failed to pack AVI IF\n");
+		return;
+	}
+
+	dig_port->write_infoframe(encoder, crtc_state, HDMI_INFOFRAME_TYPE_AVI,
+				  buf, ret);
+}
+
+bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
+			      const struct intel_crtc_state *pipe_config)
+{
+	return enc_to_intel_lspcon(encoder)->active;
+}
+
 void lspcon_resume(struct intel_lspcon *lspcon)
 {
 	enum drm_lspcon_mode expected_mode;
-- 
2.7.4

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

[PATCH 3/5] drm/i915: Write AVI infoframes for MCA LSPCON

[Shashank Sharma]

As LSPCON is a DP branch device, LSPCON vendors define
specific methods to pass AVI infoframes to the the chip.
This patch adds:
- a generic wrapper function for writing AVI infoframes for
  all LSPCON devices.
- a vendor specific function to wrire AVI infoframes into
  MCA LSPCON devices.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h    |  4 ++
 drivers/gpu/drm/i915/intel_hdmi.c   |  2 +
 drivers/gpu/drm/i915/intel_lspcon.c | 80 +++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a7c1c19..a468dd6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2037,6 +2037,10 @@ void intel_color_load_luts(struct drm_crtc_state *crtc_state);
 bool lspcon_init(struct intel_digital_port *intel_dig_port);
 void lspcon_resume(struct intel_lspcon *lspcon);
 void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
+void lspcon_write_infoframe(struct drm_encoder *encoder,
+			     const struct intel_crtc_state *crtc_state,
+			     enum hdmi_infoframe_type type,
+			     const void *buf, ssize_t len);
 void lspcon_set_infoframes(struct drm_encoder *encoder,
 			   bool enable,
 			   const struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 023015c..0f42718ba 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1995,6 +1995,8 @@ void intel_infoframe_init(struct intel_digital_port *intel_dig_port)
 		intel_dig_port->infoframe_enabled = g4x_infoframe_enabled;
 	} else if (HAS_DDI(dev_priv)) {
 		if (intel_dig_port->lspcon.active) {
+			intel_dig_port->write_infoframe =
+					lspcon_write_infoframe;
 			intel_dig_port->set_infoframes = lspcon_set_infoframes;
 			intel_dig_port->infoframe_enabled =
 						lspcon_infoframe_enabled;
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index bb1365a..1ac4c47 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -31,6 +31,12 @@
 #define LSPCON_VENDOR_PARADE_OUI 0x001CF8
 #define LSPCON_VENDOR_MCA_OUI 0x0060AD
 
+/* AUX addresses to write MCA AVI IF */
+#define LSPCON_MCA_AVI_IF_WRITE_OFFSET 0x5C0
+#define LSPCON_MCA_AVI_IF_CTRL 0x5DF
+#define  LSPCON_MCA_AVI_IF_KICKOFF (1 << 0)
+#define  LSPCON_MCA_AVI_IF_HANDLED (1 << 1)
+
 static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
 {
 	struct intel_digital_port *dig_port =
@@ -235,6 +241,80 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
 	DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
 }
 
+static bool _lspcon_write_avi_infoframe_mca(struct drm_dp_aux *aux,
+					     const uint8_t *buffer, ssize_t len)
+{
+	int ret;
+	uint32_t val = 0;
+	uint16_t reg;
+	const uint8_t *data = buffer;
+
+	reg = LSPCON_MCA_AVI_IF_WRITE_OFFSET;
+	while (val < len) {
+		ret = drm_dp_dpcd_write(aux, reg, (void *)data, 1);
+		if (ret < 0) {
+			DRM_ERROR("DPCD write failed, add:0x%x\n", reg);
+			return false;
+		}
+		val++; reg++; data++;
+	}
+
+	val = 0;
+	reg = LSPCON_MCA_AVI_IF_CTRL;
+	ret = drm_dp_dpcd_read(aux, reg, &val, 1);
+	if (ret < 0) {
+		DRM_ERROR("DPCD read failed, address 0x%x\n", reg);
+		return false;
+	}
+
+	/* Indicate LSPCON chip about infoframe, clear bit 1 and set bit 0 */
+	val &= ~LSPCON_MCA_AVI_IF_HANDLED;
+	val |= LSPCON_MCA_AVI_IF_KICKOFF;
+
+	ret = drm_dp_dpcd_write(aux, reg, &val, 1);
+	if (ret < 0) {
+		DRM_ERROR("DPCD read failed, address 0x%x\n", reg);
+		return false;
+	}
+
+	val = 0;
+	ret = drm_dp_dpcd_read(aux, reg, &val, 1);
+	if (ret < 0) {
+		DRM_ERROR("DPCD read failed, address 0x%x\n", reg);
+		return false;
+	}
+
+	if (val == LSPCON_MCA_AVI_IF_HANDLED)
+		DRM_DEBUG_KMS("AVI IF handled by FW\n");
+
+	return true;
+}
+
+void lspcon_write_infoframe(struct drm_encoder *encoder,
+			     const struct intel_crtc_state *crtc_state,
+			     enum hdmi_infoframe_type type,
+			     const void *frame, ssize_t len)
+{
+	bool ret = true;
+	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+	struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder);
+
+	/* LSPCON only needs AVI IF */
+	if (type != HDMI_INFOFRAME_TYPE_AVI)
+		return;
+
+	if (lspcon->vendor == LSPCON_VENDOR_MCA)
+		ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux,
+						      frame, len);
+	if (!ret) {
+		DRM_ERROR("Failed to write AVI infoframes\n");
+		return;
+	}
+
+	DRM_DEBUG_DRIVER("AVI infoframes updated successfully\n");
+}
+
+
 void lspcon_set_infoframes(struct drm_encoder *encoder,
 			   bool enable,
 			   const struct intel_crtc_state *crtc_state,
-- 
2.7.4

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

[PATCH 4/5] drm/i915: Write AVI infoframes for Parade LSPCON

[Shashank Sharma]

Different LSPCON vendors specify their custom methods to pass
AVI infoframes to the LSPCON chip, so does Parade tech.

This patch adds functions to arrange and write AVI infoframes
into Parade LSPCON chips.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_lspcon.c | 119 +++++++++++++++++++++++++++++++++++-
 1 file changed, 118 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 1ac4c47..77f0687 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -37,6 +37,12 @@
 #define  LSPCON_MCA_AVI_IF_KICKOFF (1 << 0)
 #define  LSPCON_MCA_AVI_IF_HANDLED (1 << 1)
 
+/* AUX addresses to write Parade AVI IF */
+#define LSPCON_PARADE_AVI_IF_WRITE_OFFSET 0x516
+#define LSPCON_PARADE_AVI_IF_CTRL 0x51E
+#define  LSPCON_PARADE_AVI_IF_KICKOFF (1 << 7)
+#define LSPCON_PARADE_AVI_IF_DATA_SIZE 32
+
 static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
 {
 	struct intel_digital_port *dig_port =
@@ -241,6 +247,113 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
 	DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
 }
 
+static bool lspcon_parade_fw_ready(struct drm_dp_aux *aux)
+{
+	u8 avi_if_ctrl;
+	u8 retry;
+	ssize_t ret;
+
+	/* Check if LSPCON FW is ready for data */
+	for (retry = 0; retry < 5; retry++) {
+
+		if (retry)
+			usleep_range(200, 300);
+
+		ret = drm_dp_dpcd_read(aux, LSPCON_PARADE_AVI_IF_CTRL,
+				       &avi_if_ctrl, 1);
+		if (ret < 0) {
+			DRM_ERROR("Failed to read AVI IF control\n");
+			return false;
+		}
+
+		if ((avi_if_ctrl & LSPCON_PARADE_AVI_IF_KICKOFF) == 0)
+			return true;
+	}
+
+	DRM_ERROR("Parade FW not ready to accept AVI IF\n");
+	return false;
+}
+
+static bool _lspcon_parade_write_infoframe_blocks(struct drm_dp_aux *aux,
+					       uint8_t *avi_buf)
+{
+	u8 avi_if_ctrl;
+	u8 block_count = 0;
+	u8 *data;
+	uint16_t reg;
+	ssize_t ret;
+
+	while (block_count < 4) {
+
+		if (!lspcon_parade_fw_ready(aux)) {
+			DRM_DEBUG_KMS("LSPCON FW not ready, block %d\n",
+				       block_count);
+			return false;
+		}
+
+		reg = LSPCON_PARADE_AVI_IF_WRITE_OFFSET;
+		data = avi_buf + block_count * 8;
+		ret = drm_dp_dpcd_write(aux, reg, data, 8);
+		if (ret < 0) {
+			DRM_ERROR("Failed to write AVI IF block %d\n",
+				   block_count);
+			return false;
+		}
+
+		/*
+		 * Once a block of data is written, we have to inform the FW
+		 * about this by writing into avi infoframe control register:
+		 * - set the kickoff bit[7] to 1
+		 * - write the block no. to bits[1:0]
+		 */
+		reg = LSPCON_PARADE_AVI_IF_CTRL;
+		avi_if_ctrl = LSPCON_PARADE_AVI_IF_KICKOFF | block_count;
+		ret = drm_dp_dpcd_write(aux, reg, &avi_if_ctrl, 1);
+		if (ret < 0) {
+			DRM_ERROR("Failed to update (0x%x), block %d\n",
+					reg, block_count);
+			return false;
+		}
+
+		block_count++;
+	}
+
+	DRM_DEBUG_KMS("Wrote AVI IF blocks successfully\n");
+	return true;
+}
+
+static bool _lspcon_write_avi_infoframe_parade(struct drm_dp_aux *aux,
+					       const uint8_t *frame,
+					       ssize_t len)
+{
+	uint8_t avi_if[LSPCON_PARADE_AVI_IF_DATA_SIZE] = {1, };
+
+	/*
+	 * Parade's frames contains 32 bytes of data, divided
+	 * into 4 frames:
+	 *	Token byte (first byte of first frame, must be non-zero)
+	 *	HB0 to HB2	 from AVI IF (3 bytes header)
+	 *	PB0 to PB27 from AVI IF (28 bytes data)
+	 * So it should look like this
+	 *	first block: | <token> <HB0-HB2> <DB0-DB3> |
+	 *	next 3 blocks: |<DB4-DB11>|<DB12-DB19>|<DB20-DB28>|
+	 */
+
+	if (len > LSPCON_PARADE_AVI_IF_DATA_SIZE - 1) {
+		DRM_ERROR("Invalid length of infoframes\n");
+		return false;
+	}
+
+	memcpy(&avi_if[1], frame, len);
+
+	if (!_lspcon_parade_write_infoframe_blocks(aux, avi_if)) {
+		DRM_DEBUG_KMS("Failed to write infoframe blocks\n");
+		return false;
+	}
+
+	return true;
+}
+
 static bool _lspcon_write_avi_infoframe_mca(struct drm_dp_aux *aux,
 					     const uint8_t *buffer, ssize_t len)
 {
@@ -295,7 +408,7 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
 			     enum hdmi_infoframe_type type,
 			     const void *frame, ssize_t len)
 {
-	bool ret = true;
+	bool ret;
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 	struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder);
 
@@ -306,6 +419,10 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
 	if (lspcon->vendor == LSPCON_VENDOR_MCA)
 		ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux,
 						      frame, len);
+	else
+		ret = _lspcon_write_avi_infoframe_parade(&intel_dp->aux,
+							 frame, len);
+
 	if (!ret) {
 		DRM_ERROR("Failed to write AVI infoframes\n");
 		return;
-- 
2.7.4

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

[PATCH 5/5] drm/i915: Add YCBCR 4:2:0 support for LSPCON

[Shashank Sharma]

LSPCON chips are capable of generating YCBCR 4:2:0 outputs, if asked
nicely :). In order to generate YCBCR 4:2:0 outputs, a source must:
- send YCBCR 4:4:4 signals to LSPCON
- program color space as 4:2:0 in AVI infoframes

This will indicate LSPCON FW to start scaling down from YCBCR 4:4:4
and generate YCBCR 4:2:0 output. Unlike HDMI 2.0 native case, as the
scaling is done by LSPCON device, we need not to reserve a scaler for
4:2:0 outputs.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |  2 ++
 drivers/gpu/drm/i915/intel_ddi.c     |  8 ++++++++
 drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++----
 drivers/gpu/drm/i915/intel_dp.c      |  7 ++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 drivers/gpu/drm/i915/intel_lspcon.c  | 22 ++++++++++++++++++++++
 6 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f0f8f60..ea6ef5e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8495,6 +8495,8 @@ enum skl_power_gate {
 #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC)
 
 #define  TRANS_MSA_SYNC_CLK		(1<<0)
+#define  TRANS_MSA_SAMPLING_444        (2<<1)
+#define  TRANS_MSA_CLRSP_YCBCR		(2<<3)
 #define  TRANS_MSA_6_BPC		(0<<5)
 #define  TRANS_MSA_8_BPC		(1<<5)
 #define  TRANS_MSA_10_BPC		(2<<5)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 9e2ab02..3fd839d 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1499,6 +1499,14 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
 		break;
 	}
 
+	/*
+	 * To get YCBCR 420 output from LSPCON, we should send YCBCR 444
+	 * signals. And as per DP 1.2 spec section 2.3.4.3 while sending
+	 * YCBCR 444 signals we should program MSA MISC1/0 fields with
+	 * colorspace information.
+	 */
+	if (crtc_state->lspcon_active && crtc_state->ycbcr420)
+		temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
 	I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 737de25..787119b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4638,7 +4638,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
 	 */
 	need_scaling = src_w != dst_w || src_h != dst_h;
 
-	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
+	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX &&
+		!crtc_state->lspcon_active)
 		need_scaling = true;
 
 	/*
@@ -8133,9 +8134,15 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
 			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
 
 		if (config->ycbcr420) {
-			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
-				PIPEMISC_YUV420_ENABLE |
-				PIPEMISC_YUV420_MODE_FULL_BLEND;
+			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
+			/*
+			 * LSPCON doesn't need scaling/blending to be done in
+			 * pipe. It just needs YCBCR444 input and proper AVI
+			 * infoframes for 4:2:0 output enabling.
+			 */
+			if (!config->lspcon_active)
+				val |= PIPEMISC_YUV420_ENABLE |
+				       PIPEMISC_YUV420_MODE_FULL_BLEND;
 		}
 
 		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index fa9e5e6..1d4b669 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1657,8 +1657,13 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 						    intel_dp->max_link_rate);
 
 	/* LSPCON needs special handling to drive YCBCR420 outputs */
-	if (lspcon->active)
+	if (lspcon->active) {
+		struct drm_connector *connector = &intel_connector->base;
+
 		pipe_config->lspcon_active = true;
+		pipe_config->ycbcr420 = lspcon_ycbcr420_config(connector,
+							       pipe_config);
+	}
 
 	/* No common link rates between source and sink */
 	WARN_ON(common_len <= 0);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a468dd6..f271967 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
 			   const struct drm_connector_state *conn_state);
 bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
 			      const struct intel_crtc_state *pipe_config);
+bool lspcon_ycbcr420_config(struct drm_connector *connector,
+			     struct intel_crtc_state *config);
 
 /* intel_pipe_crc.c */
 int intel_pipe_crc_create(struct drm_minor *minor);
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 77f0687..ca77f50 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -180,6 +180,26 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
 	return true;
 }
 
+bool lspcon_ycbcr420_config(struct drm_connector *connector,
+			     struct intel_crtc_state *config)
+{
+	struct drm_display_info *info = &connector->display_info;
+	struct drm_display_mode *mode = &config->base.adjusted_mode;
+
+	if (drm_mode_is_420_only(info, mode)) {
+
+		if (!connector->ycbcr_420_allowed) {
+			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
+			return false;
+		}
+
+		config->port_clock /= 2;
+		return true;
+	}
+
+	return false;
+}
+
 static bool lspcon_probe(struct intel_lspcon *lspcon)
 {
 	int retry;
@@ -516,6 +536,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
 	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_connector *connector = &dp->attached_connector->base;
 
 	if (!HAS_LSPCON(dev_priv)) {
 		DRM_ERROR("LSPCON is not supported on this platform\n");
@@ -540,6 +561,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
 		return false;
 	}
 
+	connector->ycbcr_420_allowed = true;
 	lspcon->active = true;
 	DRM_DEBUG_KMS("Success: LSPCON init\n");
 	return true;
-- 
2.7.4

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

✓ Fi.CI.BAT: success for YCBCR 4:2:0 output support for LSPCON

[Patchwork]

== Series Details ==

Series: YCBCR 4:2:0 output support for LSPCON
URL   : https://patchwork.freedesktop.org/series/33794/
State : success

== Summary ==

Series 33794v1 YCBCR 4:2:0 output support for LSPCON
https://patchwork.freedesktop.org/api/1.0/series/33794/revisions/1/mbox/

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:445s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:457s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:378s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:543s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:276s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:501s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:511s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:494s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:487s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:432s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:264s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:547s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:430s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:445s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:425s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:485s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:468s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:480s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:531s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:474s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:541s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:464s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:535s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:562s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:515s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:496s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:458s
fi-snb-2520m     total:246  pass:212  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:424s
Blacklisted hosts:
fi-cfl-s         total:289  pass:244  dwarn:12  dfail:1   fail:0   skip:32  time:550s
fi-cnl-y         total:289  pass:261  dwarn:0   dfail:0   fail:1   skip:27  time:550s
fi-glk-dsi failed to connect after reboot
fi-pnv-d510 failed to connect after reboot

712af85519b2fa0639951273649ec6f2fe98f595 drm-tip: 2017y-11m-14d-12h-27m-40s UTC integration manifest
1b7698e4f6dc drm/i915: check LSPCON vendor OUI

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7119/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Add YCBCR 4:2:0 support for LSPCON

[Ville Syrjälä]

On Tue, Nov 14, 2017 at 08:47:26PM +0530, Shashank Sharma wrote:
> LSPCON chips are capable of generating YCBCR 4:2:0 outputs, if asked
> nicely :). In order to generate YCBCR 4:2:0 outputs, a source must:
> - send YCBCR 4:4:4 signals to LSPCON
> - program color space as 4:2:0 in AVI infoframes
> 
> This will indicate LSPCON FW to start scaling down from YCBCR 4:4:4
> and generate YCBCR 4:2:0 output. Unlike HDMI 2.0 native case, as the
> scaling is done by LSPCON device, we need not to reserve a scaler for
> 4:2:0 outputs.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  2 ++
>  drivers/gpu/drm/i915/intel_ddi.c     |  8 ++++++++
>  drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++----
>  drivers/gpu/drm/i915/intel_dp.c      |  7 ++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  drivers/gpu/drm/i915/intel_lspcon.c  | 22 ++++++++++++++++++++++
>  6 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f0f8f60..ea6ef5e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8495,6 +8495,8 @@ enum skl_power_gate {
>  #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC)
>  
>  #define  TRANS_MSA_SYNC_CLK		(1<<0)
> +#define  TRANS_MSA_SAMPLING_444        (2<<1)
> +#define  TRANS_MSA_CLRSP_YCBCR		(2<<3)
>  #define  TRANS_MSA_6_BPC		(0<<5)
>  #define  TRANS_MSA_8_BPC		(1<<5)
>  #define  TRANS_MSA_10_BPC		(2<<5)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 9e2ab02..3fd839d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1499,6 +1499,14 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>  		break;
>  	}
>  
> +	/*
> +	 * To get YCBCR 420 output from LSPCON, we should send YCBCR 444
> +	 * signals. And as per DP 1.2 spec section 2.3.4.3 while sending
> +	 * YCBCR 444 signals we should program MSA MISC1/0 fields with
> +	 * colorspace information.
> +	 */
> +	if (crtc_state->lspcon_active && crtc_state->ycbcr420)

I think it would be much cleaner if we just set
crtc_state->ycbcr444=true here instead of crtc_state->ycbcr420=true,
and then have a separate flag to control the 444->420 downsampling in
the DP device/LSPCON. Not quite sure what we should call that, maybe just
somehting like crtc_state->ycbcr444_to_ycbcr420. That way you'll pretty
much have implemented ycbcr444 support for us apart from the uapi to
flip it on.

Looks like state readout support is also missing from this patch.

> +		temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
>  	I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 737de25..787119b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4638,7 +4638,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  	 */
>  	need_scaling = src_w != dst_w || src_h != dst_h;
>  
> -	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
> +	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX &&
> +		!crtc_state->lspcon_active)
>  		need_scaling = true;
>  
>  	/*
> @@ -8133,9 +8134,15 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>  			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>  
>  		if (config->ycbcr420) {
> -			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
> -				PIPEMISC_YUV420_ENABLE |
> -				PIPEMISC_YUV420_MODE_FULL_BLEND;
> +			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
> +			/*
> +			 * LSPCON doesn't need scaling/blending to be done in
> +			 * pipe. It just needs YCBCR444 input and proper AVI
> +			 * infoframes for 4:2:0 output enabling.
> +			 */
> +			if (!config->lspcon_active)
> +				val |= PIPEMISC_YUV420_ENABLE |
> +				       PIPEMISC_YUV420_MODE_FULL_BLEND;
>  		}
>  
>  		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fa9e5e6..1d4b669 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1657,8 +1657,13 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  						    intel_dp->max_link_rate);
>  
>  	/* LSPCON needs special handling to drive YCBCR420 outputs */
> -	if (lspcon->active)
> +	if (lspcon->active) {
> +		struct drm_connector *connector = &intel_connector->base;
> +
>  		pipe_config->lspcon_active = true;
> +		pipe_config->ycbcr420 = lspcon_ycbcr420_config(connector,
> +							       pipe_config);
> +	}
>  
>  	/* No common link rates between source and sink */
>  	WARN_ON(common_len <= 0);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a468dd6..f271967 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>  			   const struct drm_connector_state *conn_state);
>  bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
>  			      const struct intel_crtc_state *pipe_config);
> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
> +			     struct intel_crtc_state *config);
>  
>  /* intel_pipe_crc.c */
>  int intel_pipe_crc_create(struct drm_minor *minor);
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index 77f0687..ca77f50 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -180,6 +180,26 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
>  	return true;
>  }
>  
> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
> +			     struct intel_crtc_state *config)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
> +
> +	if (drm_mode_is_420_only(info, mode)) {
> +
> +		if (!connector->ycbcr_420_allowed) {
> +			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
> +			return false;
> +		}
> +
> +		config->port_clock /= 2;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static bool lspcon_probe(struct intel_lspcon *lspcon)
>  {
>  	int retry;
> @@ -516,6 +536,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>  	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_connector *connector = &dp->attached_connector->base;
>  
>  	if (!HAS_LSPCON(dev_priv)) {
>  		DRM_ERROR("LSPCON is not supported on this platform\n");
> @@ -540,6 +561,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>  		return false;
>  	}
>  
> +	connector->ycbcr_420_allowed = true;
>  	lspcon->active = true;
>  	DRM_DEBUG_KMS("Success: LSPCON init\n");
>  	return true;
> -- 
> 2.7.4

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

✗ Fi.CI.IGT: warning for YCBCR 4:2:0 output support for LSPCON

[Patchwork]

== Series Details ==

Series: YCBCR 4:2:0 output support for LSPCON
URL   : https://patchwork.freedesktop.org/series/33794/
State : warning

== Summary ==

Test kms_plane:
        Subgroup plane-panning-bottom-right-suspend-pipe-a-planes:
                pass       -> SKIP       (shard-hsw)
Test kms_busy:
        Subgroup extended-modeset-hang-newfb-with-reset-render-c:
                pass       -> DMESG-WARN (shard-hsw) fdo#102249
        Subgroup extended-modeset-hang-newfb-with-reset-render-b:
                dmesg-warn -> PASS       (shard-hsw) fdo#103038
Test kms_setmode:
        Subgroup basic:
                pass       -> FAIL       (shard-hsw) fdo#99912
Test perf:
        Subgroup oa-exponents:
                pass       -> FAIL       (shard-hsw) fdo#102254

fdo#102249 https://bugs.freedesktop.org/show_bug.cgi?id=102249
fdo#103038 https://bugs.freedesktop.org/show_bug.cgi?id=103038
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254

shard-hsw        total:2584 pass:1468 dwarn:4   dfail:1   fail:11  skip:1100 time:9484s
Blacklisted hosts:
shard-apl        total:2565 pass:1603 dwarn:1   dfail:0   fail:23  skip:937 time:12798s
shard-kbl        total:2495 pass:1631 dwarn:42  dfail:1   fail:24  skip:795 time:10341s
shard-snb        total:2584 pass:1206 dwarn:1   dfail:1   fail:13  skip:1363 time:7777s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7119/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Write AVI infoframes for Parade LSPCON

[Maarten Lankhorst]

Op 14-11-17 om 16:17 schreef Shashank Sharma:
> Different LSPCON vendors specify their custom methods to pass
> AVI infoframes to the LSPCON chip, so does Parade tech.
>
> This patch adds functions to arrange and write AVI infoframes
> into Parade LSPCON chips.
>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lspcon.c | 119 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 118 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index 1ac4c47..77f0687 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -37,6 +37,12 @@
>  #define  LSPCON_MCA_AVI_IF_KICKOFF (1 << 0)
>  #define  LSPCON_MCA_AVI_IF_HANDLED (1 << 1)
>  
> +/* AUX addresses to write Parade AVI IF */
> +#define LSPCON_PARADE_AVI_IF_WRITE_OFFSET 0x516
> +#define LSPCON_PARADE_AVI_IF_CTRL 0x51E
> +#define  LSPCON_PARADE_AVI_IF_KICKOFF (1 << 7)
> +#define LSPCON_PARADE_AVI_IF_DATA_SIZE 32
> +
>  static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
>  {
>  	struct intel_digital_port *dig_port =
> @@ -241,6 +247,113 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
>  	DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
>  }
>  
> +static bool lspcon_parade_fw_ready(struct drm_dp_aux *aux)
> +{
> +	u8 avi_if_ctrl;
> +	u8 retry;
> +	ssize_t ret;
> +
> +	/* Check if LSPCON FW is ready for data */
> +	for (retry = 0; retry < 5; retry++) {
> +
> +		if (retry)
> +			usleep_range(200, 300);
> +
> +		ret = drm_dp_dpcd_read(aux, LSPCON_PARADE_AVI_IF_CTRL,
> +				       &avi_if_ctrl, 1);
> +		if (ret < 0) {
> +			DRM_ERROR("Failed to read AVI IF control\n");
> +			return false;
> +		}
> +
> +		if ((avi_if_ctrl & LSPCON_PARADE_AVI_IF_KICKOFF) == 0)
> +			return true;
> +	}
> +
> +	DRM_ERROR("Parade FW not ready to accept AVI IF\n");
> +	return false;
> +}
> +
> +static bool _lspcon_parade_write_infoframe_blocks(struct drm_dp_aux *aux,
> +					       uint8_t *avi_buf)
> +{
> +	u8 avi_if_ctrl;
> +	u8 block_count = 0;
> +	u8 *data;
> +	uint16_t reg;
> +	ssize_t ret;
> +
> +	while (block_count < 4) {
> +
> +		if (!lspcon_parade_fw_ready(aux)) {
> +			DRM_DEBUG_KMS("LSPCON FW not ready, block %d\n",
> +				       block_count);
> +			return false;
> +		}
> +
> +		reg = LSPCON_PARADE_AVI_IF_WRITE_OFFSET;
> +		data = avi_buf + block_count * 8;
> +		ret = drm_dp_dpcd_write(aux, reg, data, 8);
> +		if (ret < 0) {
> +			DRM_ERROR("Failed to write AVI IF block %d\n",
> +				   block_count);
> +			return false;
> +		}
> +
> +		/*
> +		 * Once a block of data is written, we have to inform the FW
> +		 * about this by writing into avi infoframe control register:
> +		 * - set the kickoff bit[7] to 1
> +		 * - write the block no. to bits[1:0]
> +		 */
> +		reg = LSPCON_PARADE_AVI_IF_CTRL;
> +		avi_if_ctrl = LSPCON_PARADE_AVI_IF_KICKOFF | block_count;
> +		ret = drm_dp_dpcd_write(aux, reg, &avi_if_ctrl, 1);
> +		if (ret < 0) {
> +			DRM_ERROR("Failed to update (0x%x), block %d\n",
> +					reg, block_count);
> +			return false;
> +		}
> +
> +		block_count++;
> +	}
> +
> +	DRM_DEBUG_KMS("Wrote AVI IF blocks successfully\n");
> +	return true;
> +}
> +
> +static bool _lspcon_write_avi_infoframe_parade(struct drm_dp_aux *aux,
> +					       const uint8_t *frame,
> +					       ssize_t len)
> +{
> +	uint8_t avi_if[LSPCON_PARADE_AVI_IF_DATA_SIZE] = {1, };
> +
> +	/*
> +	 * Parade's frames contains 32 bytes of data, divided
> +	 * into 4 frames:
> +	 *	Token byte (first byte of first frame, must be non-zero)
> +	 *	HB0 to HB2	 from AVI IF (3 bytes header)
> +	 *	PB0 to PB27 from AVI IF (28 bytes data)
> +	 * So it should look like this
> +	 *	first block: | <token> <HB0-HB2> <DB0-DB3> |
> +	 *	next 3 blocks: |<DB4-DB11>|<DB12-DB19>|<DB20-DB28>|
> +	 */
> +
> +	if (len > LSPCON_PARADE_AVI_IF_DATA_SIZE - 1) {
> +		DRM_ERROR("Invalid length of infoframes\n");
> +		return false;
> +	}
> +
> +	memcpy(&avi_if[1], frame, len);
> +
> +	if (!_lspcon_parade_write_infoframe_blocks(aux, avi_if)) {
> +		DRM_DEBUG_KMS("Failed to write infoframe blocks\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  static bool _lspcon_write_avi_infoframe_mca(struct drm_dp_aux *aux,
>  					     const uint8_t *buffer, ssize_t len)
>  {
> @@ -295,7 +408,7 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
>  			     enum hdmi_infoframe_type type,
>  			     const void *frame, ssize_t len)
>  {
> -	bool ret = true;
> +	bool ret;
>  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  	struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder);
>  
> @@ -306,6 +419,10 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
>  	if (lspcon->vendor == LSPCON_VENDOR_MCA)
>  		ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux,
>  						      frame, len);
> +	else
> +		ret = _lspcon_write_avi_infoframe_parade(&intel_dp->aux,
> +							 frame, len);
If you make it a switch (lspcon->vendor) and don't add a default case, you will get a compiler warning when you add a new vendor to the enumeration.

With that changed for first 4 patches:

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Add YCBCR 4:2:0 support for LSPCON

[Maarten Lankhorst]

Op 14-11-17 om 16:17 schreef Shashank Sharma:
> LSPCON chips are capable of generating YCBCR 4:2:0 outputs, if asked
> nicely :). In order to generate YCBCR 4:2:0 outputs, a source must:
> - send YCBCR 4:4:4 signals to LSPCON
> - program color space as 4:2:0 in AVI infoframes
>
> This will indicate LSPCON FW to start scaling down from YCBCR 4:4:4
> and generate YCBCR 4:2:0 output. Unlike HDMI 2.0 native case, as the
> scaling is done by LSPCON device, we need not to reserve a scaler for
> 4:2:0 outputs.
>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  2 ++
>  drivers/gpu/drm/i915/intel_ddi.c     |  8 ++++++++
>  drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++----
>  drivers/gpu/drm/i915/intel_dp.c      |  7 ++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  drivers/gpu/drm/i915/intel_lspcon.c  | 22 ++++++++++++++++++++++
>  6 files changed, 51 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f0f8f60..ea6ef5e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8495,6 +8495,8 @@ enum skl_power_gate {
>  #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC)
>  
>  #define  TRANS_MSA_SYNC_CLK		(1<<0)
> +#define  TRANS_MSA_SAMPLING_444        (2<<1)
> +#define  TRANS_MSA_CLRSP_YCBCR		(2<<3)
>  #define  TRANS_MSA_6_BPC		(0<<5)
>  #define  TRANS_MSA_8_BPC		(1<<5)
>  #define  TRANS_MSA_10_BPC		(2<<5)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 9e2ab02..3fd839d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1499,6 +1499,14 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>  		break;
>  	}
>  
> +	/*
> +	 * To get YCBCR 420 output from LSPCON, we should send YCBCR 444
> +	 * signals. And as per DP 1.2 spec section 2.3.4.3 while sending
> +	 * YCBCR 444 signals we should program MSA MISC1/0 fields with
> +	 * colorspace information.
> +	 */
> +	if (crtc_state->lspcon_active && crtc_state->ycbcr420)
> +		temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
>  	I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 737de25..787119b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4638,7 +4638,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  	 */
>  	need_scaling = src_w != dst_w || src_h != dst_h;
>  
> -	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
> +	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX &&
> +		!crtc_state->lspcon_active)
>  		need_scaling = true;
>  
>  	/*
> @@ -8133,9 +8134,15 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>  			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>  
>  		if (config->ycbcr420) {
> -			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
> -				PIPEMISC_YUV420_ENABLE |
> -				PIPEMISC_YUV420_MODE_FULL_BLEND;
> +			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
> +			/*
> +			 * LSPCON doesn't need scaling/blending to be done in
> +			 * pipe. It just needs YCBCR444 input and proper AVI
> +			 * infoframes for 4:2:0 output enabling.
> +			 */
> +			if (!config->lspcon_active)
> +				val |= PIPEMISC_YUV420_ENABLE |
> +				       PIPEMISC_YUV420_MODE_FULL_BLEND;
>  		}
>  
>  		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fa9e5e6..1d4b669 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1657,8 +1657,13 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  						    intel_dp->max_link_rate);
>  
>  	/* LSPCON needs special handling to drive YCBCR420 outputs */
> -	if (lspcon->active)
> +	if (lspcon->active) {
> +		struct drm_connector *connector = &intel_connector->base;
> +
>  		pipe_config->lspcon_active = true;
> +		pipe_config->ycbcr420 = lspcon_ycbcr420_config(connector,
> +							       pipe_config);
> +	}
>  
>  	/* No common link rates between source and sink */
>  	WARN_ON(common_len <= 0);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a468dd6..f271967 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>  			   const struct drm_connector_state *conn_state);
>  bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
>  			      const struct intel_crtc_state *pipe_config);
> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
> +			     struct intel_crtc_state *config);
>  
>  /* intel_pipe_crc.c */
>  int intel_pipe_crc_create(struct drm_minor *minor);
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index 77f0687..ca77f50 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -180,6 +180,26 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
>  	return true;
>  }
>  
> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
> +			     struct intel_crtc_state *config)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
> +
> +	if (drm_mode_is_420_only(info, mode)) {
> +
> +		if (!connector->ycbcr_420_allowed) {
> +			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
> +			return false;
> +		}
Seems to be always true if lspcon->active is true?
> +		config->port_clock /= 2;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static bool lspcon_probe(struct intel_lspcon *lspcon)
>  {
>  	int retry;
> @@ -516,6 +536,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>  	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_connector *connector = &dp->attached_connector->base;
>  
>  	if (!HAS_LSPCON(dev_priv)) {
>  		DRM_ERROR("LSPCON is not supported on this platform\n");
> @@ -540,6 +561,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>  		return false;
>  	}
>  
> +	connector->ycbcr_420_allowed = true;
Is this flag different from lspcon->active?
>  	lspcon->active = true;
>  	DRM_DEBUG_KMS("Success: LSPCON init\n");
>  	return true;

Comments from Ville still apply too. The pipe is set to ycbcr mode, so it makes more sense to have a ycbcr444 flag or something, and a separate flag to downconvert lspcon.

It would be really nice if we could set the ycbcr444 flag in hw readout, and intel_pipe_config_compare. This way we know the sw mode matches the hw mode. :)

~Maarten

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

Re: [PATCH 4/5] drm/i915: Write AVI infoframes for Parade LSPCON

[David Weinehall]

On Mon, Dec 18, 2017 at 08:15:30PM +0100, Maarten Lankhorst wrote:
> Op 14-11-17 om 16:17 schreef Shashank Sharma:
> > Different LSPCON vendors specify their custom methods to pass
> > AVI infoframes to the LSPCON chip, so does Parade tech.
> >
> > This patch adds functions to arrange and write AVI infoframes
> > into Parade LSPCON chips.
> >
> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lspcon.c | 119 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 118 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> > index 1ac4c47..77f0687 100644
> > --- a/drivers/gpu/drm/i915/intel_lspcon.c
> > +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> > @@ -37,6 +37,12 @@
> >  #define  LSPCON_MCA_AVI_IF_KICKOFF (1 << 0)
> >  #define  LSPCON_MCA_AVI_IF_HANDLED (1 << 1)
> >  
> > +/* AUX addresses to write Parade AVI IF */
> > +#define LSPCON_PARADE_AVI_IF_WRITE_OFFSET 0x516
> > +#define LSPCON_PARADE_AVI_IF_CTRL 0x51E
> > +#define  LSPCON_PARADE_AVI_IF_KICKOFF (1 << 7)
> > +#define LSPCON_PARADE_AVI_IF_DATA_SIZE 32
> > +
> >  static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
> >  {
> >  	struct intel_digital_port *dig_port =
> > @@ -241,6 +247,113 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
> >  	DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
> >  }
> >  
> > +static bool lspcon_parade_fw_ready(struct drm_dp_aux *aux)
> > +{
> > +	u8 avi_if_ctrl;
> > +	u8 retry;
> > +	ssize_t ret;
> > +
> > +	/* Check if LSPCON FW is ready for data */
> > +	for (retry = 0; retry < 5; retry++) {
> > +

Remove this newline.

> > +		if (retry)
> > +			usleep_range(200, 300);
> > +
> > +		ret = drm_dp_dpcd_read(aux, LSPCON_PARADE_AVI_IF_CTRL,
> > +				       &avi_if_ctrl, 1);
> > +		if (ret < 0) {
> > +			DRM_ERROR("Failed to read AVI IF control\n");
> > +			return false;
> > +		}
> > +
> > +		if ((avi_if_ctrl & LSPCON_PARADE_AVI_IF_KICKOFF) == 0)
> > +			return true;
> > +	}
> > +
> > +	DRM_ERROR("Parade FW not ready to accept AVI IF\n");
> > +	return false;
> > +}
> > +
> > +static bool _lspcon_parade_write_infoframe_blocks(struct drm_dp_aux *aux,
> > +					       uint8_t *avi_buf)
> > +{
> > +	u8 avi_if_ctrl;
> > +	u8 block_count = 0;
> > +	u8 *data;
> > +	uint16_t reg;
> > +	ssize_t ret;
> > +
> > +	while (block_count < 4) {
> > +

And this one.

> > +		if (!lspcon_parade_fw_ready(aux)) {
> > +			DRM_DEBUG_KMS("LSPCON FW not ready, block %d\n",
> > +				       block_count);
> > +			return false;
> > +		}
> > +
> > +		reg = LSPCON_PARADE_AVI_IF_WRITE_OFFSET;
> > +		data = avi_buf + block_count * 8;
> > +		ret = drm_dp_dpcd_write(aux, reg, data, 8);
> > +		if (ret < 0) {
> > +			DRM_ERROR("Failed to write AVI IF block %d\n",
> > +				   block_count);
> > +			return false;
> > +		}
> > +
> > +		/*
> > +		 * Once a block of data is written, we have to inform the FW
> > +		 * about this by writing into avi infoframe control register:
> > +		 * - set the kickoff bit[7] to 1
> > +		 * - write the block no. to bits[1:0]
> > +		 */
> > +		reg = LSPCON_PARADE_AVI_IF_CTRL;
> > +		avi_if_ctrl = LSPCON_PARADE_AVI_IF_KICKOFF | block_count;
> > +		ret = drm_dp_dpcd_write(aux, reg, &avi_if_ctrl, 1);
> > +		if (ret < 0) {
> > +			DRM_ERROR("Failed to update (0x%x), block %d\n",
> > +					reg, block_count);
> > +			return false;
> > +		}
> > +
> > +		block_count++;
> > +	}
> > +
> > +	DRM_DEBUG_KMS("Wrote AVI IF blocks successfully\n");
> > +	return true;
> > +}
> > +
> > +static bool _lspcon_write_avi_infoframe_parade(struct drm_dp_aux *aux,
> > +					       const uint8_t *frame,
> > +					       ssize_t len)
> > +{
> > +	uint8_t avi_if[LSPCON_PARADE_AVI_IF_DATA_SIZE] = {1, };
> > +
> > +	/*
> > +	 * Parade's frames contains 32 bytes of data, divided
> > +	 * into 4 frames:
> > +	 *	Token byte (first byte of first frame, must be non-zero)
> > +	 *	HB0 to HB2	 from AVI IF (3 bytes header)
> > +	 *	PB0 to PB27 from AVI IF (28 bytes data)
> > +	 * So it should look like this
> > +	 *	first block: | <token> <HB0-HB2> <DB0-DB3> |
> > +	 *	next 3 blocks: |<DB4-DB11>|<DB12-DB19>|<DB20-DB28>|
> > +	 */
> > +
> > +	if (len > LSPCON_PARADE_AVI_IF_DATA_SIZE - 1) {
> > +		DRM_ERROR("Invalid length of infoframes\n");
> > +		return false;
> > +	}
> > +
> > +	memcpy(&avi_if[1], frame, len);
> > +
> > +	if (!_lspcon_parade_write_infoframe_blocks(aux, avi_if)) {
> > +		DRM_DEBUG_KMS("Failed to write infoframe blocks\n");
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> >  static bool _lspcon_write_avi_infoframe_mca(struct drm_dp_aux *aux,
> >  					     const uint8_t *buffer, ssize_t len)
> >  {
> > @@ -295,7 +408,7 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
> >  			     enum hdmi_infoframe_type type,
> >  			     const void *frame, ssize_t len)
> >  {
> > -	bool ret = true;
> > +	bool ret;
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >  	struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder);
> >  
> > @@ -306,6 +419,10 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
> >  	if (lspcon->vendor == LSPCON_VENDOR_MCA)
> >  		ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux,
> >  						      frame, len);
> > +	else
> > +		ret = _lspcon_write_avi_infoframe_parade(&intel_dp->aux,
> > +							 frame, len);
> If you make it a switch (lspcon->vendor) and don't add a default case, you will get a compiler warning when you add a new vendor to the enumeration.
> 
> With that changed for first 4 patches:
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Write AVI infoframes for Parade LSPCON

[Sharma, Shashank]

Thanks for the review, David.

My comments, inline.


Regards

Shashank


On 12/19/2017 3:43 PM, David Weinehall wrote:
> On Mon, Dec 18, 2017 at 08:15:30PM +0100, Maarten Lankhorst wrote:
>> Op 14-11-17 om 16:17 schreef Shashank Sharma:
>>> Different LSPCON vendors specify their custom methods to pass
>>> AVI infoframes to the LSPCON chip, so does Parade tech.
>>>
>>> This patch adds functions to arrange and write AVI infoframes
>>> into Parade LSPCON chips.
>>>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_lspcon.c | 119 +++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 118 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
>>> index 1ac4c47..77f0687 100644
>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>>> @@ -37,6 +37,12 @@
>>>   #define  LSPCON_MCA_AVI_IF_KICKOFF (1 << 0)
>>>   #define  LSPCON_MCA_AVI_IF_HANDLED (1 << 1)
>>>   
>>> +/* AUX addresses to write Parade AVI IF */
>>> +#define LSPCON_PARADE_AVI_IF_WRITE_OFFSET 0x516
>>> +#define LSPCON_PARADE_AVI_IF_CTRL 0x51E
>>> +#define  LSPCON_PARADE_AVI_IF_KICKOFF (1 << 7)
>>> +#define LSPCON_PARADE_AVI_IF_DATA_SIZE 32
>>> +
>>>   static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
>>>   {
>>>   	struct intel_digital_port *dig_port =
>>> @@ -241,6 +247,113 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
>>>   	DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
>>>   }
>>>   
>>> +static bool lspcon_parade_fw_ready(struct drm_dp_aux *aux)
>>> +{
>>> +	u8 avi_if_ctrl;
>>> +	u8 retry;
>>> +	ssize_t ret;
>>> +
>>> +	/* Check if LSPCON FW is ready for data */
>>> +	for (retry = 0; retry < 5; retry++) {
>>> +
> Remove this newline.
Why ? this is not accidental.
>>> +		if (retry)
>>> +			usleep_range(200, 300);
>>> +
>>> +		ret = drm_dp_dpcd_read(aux, LSPCON_PARADE_AVI_IF_CTRL,
>>> +				       &avi_if_ctrl, 1);
>>> +		if (ret < 0) {
>>> +			DRM_ERROR("Failed to read AVI IF control\n");
>>> +			return false;
>>> +		}
>>> +
>>> +		if ((avi_if_ctrl & LSPCON_PARADE_AVI_IF_KICKOFF) == 0)
>>> +			return true;
>>> +	}
>>> +
>>> +	DRM_ERROR("Parade FW not ready to accept AVI IF\n");
>>> +	return false;
>>> +}
>>> +
>>> +static bool _lspcon_parade_write_infoframe_blocks(struct drm_dp_aux *aux,
>>> +					       uint8_t *avi_buf)
>>> +{
>>> +	u8 avi_if_ctrl;
>>> +	u8 block_count = 0;
>>> +	u8 *data;
>>> +	uint16_t reg;
>>> +	ssize_t ret;
>>> +
>>> +	while (block_count < 4) {
>>> +
> And this one.
Same as above.
- Shashank
>
>>> +		if (!lspcon_parade_fw_ready(aux)) {
>>> +			DRM_DEBUG_KMS("LSPCON FW not ready, block %d\n",
>>> +				       block_count);
>>> +			return false;
>>> +		}
>>> +
>>> +		reg = LSPCON_PARADE_AVI_IF_WRITE_OFFSET;
>>> +		data = avi_buf + block_count * 8;
>>> +		ret = drm_dp_dpcd_write(aux, reg, data, 8);
>>> +		if (ret < 0) {
>>> +			DRM_ERROR("Failed to write AVI IF block %d\n",
>>> +				   block_count);
>>> +			return false;
>>> +		}
>>> +
>>> +		/*
>>> +		 * Once a block of data is written, we have to inform the FW
>>> +		 * about this by writing into avi infoframe control register:
>>> +		 * - set the kickoff bit[7] to 1
>>> +		 * - write the block no. to bits[1:0]
>>> +		 */
>>> +		reg = LSPCON_PARADE_AVI_IF_CTRL;
>>> +		avi_if_ctrl = LSPCON_PARADE_AVI_IF_KICKOFF | block_count;
>>> +		ret = drm_dp_dpcd_write(aux, reg, &avi_if_ctrl, 1);
>>> +		if (ret < 0) {
>>> +			DRM_ERROR("Failed to update (0x%x), block %d\n",
>>> +					reg, block_count);
>>> +			return false;
>>> +		}
>>> +
>>> +		block_count++;
>>> +	}
>>> +
>>> +	DRM_DEBUG_KMS("Wrote AVI IF blocks successfully\n");
>>> +	return true;
>>> +}
>>> +
>>> +static bool _lspcon_write_avi_infoframe_parade(struct drm_dp_aux *aux,
>>> +					       const uint8_t *frame,
>>> +					       ssize_t len)
>>> +{
>>> +	uint8_t avi_if[LSPCON_PARADE_AVI_IF_DATA_SIZE] = {1, };
>>> +
>>> +	/*
>>> +	 * Parade's frames contains 32 bytes of data, divided
>>> +	 * into 4 frames:
>>> +	 *	Token byte (first byte of first frame, must be non-zero)
>>> +	 *	HB0 to HB2	 from AVI IF (3 bytes header)
>>> +	 *	PB0 to PB27 from AVI IF (28 bytes data)
>>> +	 * So it should look like this
>>> +	 *	first block: | <token> <HB0-HB2> <DB0-DB3> |
>>> +	 *	next 3 blocks: |<DB4-DB11>|<DB12-DB19>|<DB20-DB28>|
>>> +	 */
>>> +
>>> +	if (len > LSPCON_PARADE_AVI_IF_DATA_SIZE - 1) {
>>> +		DRM_ERROR("Invalid length of infoframes\n");
>>> +		return false;
>>> +	}
>>> +
>>> +	memcpy(&avi_if[1], frame, len);
>>> +
>>> +	if (!_lspcon_parade_write_infoframe_blocks(aux, avi_if)) {
>>> +		DRM_DEBUG_KMS("Failed to write infoframe blocks\n");
>>> +		return false;
>>> +	}
>>> +
>>> +	return true;
>>> +}
>>> +
>>>   static bool _lspcon_write_avi_infoframe_mca(struct drm_dp_aux *aux,
>>>   					     const uint8_t *buffer, ssize_t len)
>>>   {
>>> @@ -295,7 +408,7 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
>>>   			     enum hdmi_infoframe_type type,
>>>   			     const void *frame, ssize_t len)
>>>   {
>>> -	bool ret = true;
>>> +	bool ret;
>>>   	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>>   	struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder);
>>>   
>>> @@ -306,6 +419,10 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
>>>   	if (lspcon->vendor == LSPCON_VENDOR_MCA)
>>>   		ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux,
>>>   						      frame, len);
>>> +	else
>>> +		ret = _lspcon_write_avi_infoframe_parade(&intel_dp->aux,
>>> +							 frame, len);
>> If you make it a switch (lspcon->vendor) and don't add a default case, you will get a compiler warning when you add a new vendor to the enumeration.
>>
>> With that changed for first 4 patches:
>>
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 5/5] drm/i915: Add YCBCR 4:2:0 support for LSPCON

[Sharma, Shashank]

Thanks for the review Maarten,

My comments inline.

Regards
Shashank
On 12/19/2017 12:53 AM, Maarten Lankhorst wrote:
> Op 14-11-17 om 16:17 schreef Shashank Sharma:
>> LSPCON chips are capable of generating YCBCR 4:2:0 outputs, if asked
>> nicely :). In order to generate YCBCR 4:2:0 outputs, a source must:
>> - send YCBCR 4:4:4 signals to LSPCON
>> - program color space as 4:2:0 in AVI infoframes
>>
>> This will indicate LSPCON FW to start scaling down from YCBCR 4:4:4
>> and generate YCBCR 4:2:0 output. Unlike HDMI 2.0 native case, as the
>> scaling is done by LSPCON device, we need not to reserve a scaler for
>> 4:2:0 outputs.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h      |  2 ++
>>   drivers/gpu/drm/i915/intel_ddi.c     |  8 ++++++++
>>   drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++----
>>   drivers/gpu/drm/i915/intel_dp.c      |  7 ++++++-
>>   drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>>   drivers/gpu/drm/i915/intel_lspcon.c  | 22 ++++++++++++++++++++++
>>   6 files changed, 51 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index f0f8f60..ea6ef5e 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -8495,6 +8495,8 @@ enum skl_power_gate {
>>   #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC)
>>   
>>   #define  TRANS_MSA_SYNC_CLK		(1<<0)
>> +#define  TRANS_MSA_SAMPLING_444        (2<<1)
>> +#define  TRANS_MSA_CLRSP_YCBCR		(2<<3)
>>   #define  TRANS_MSA_6_BPC		(0<<5)
>>   #define  TRANS_MSA_8_BPC		(1<<5)
>>   #define  TRANS_MSA_10_BPC		(2<<5)
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 9e2ab02..3fd839d 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1499,6 +1499,14 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>>   		break;
>>   	}
>>   
>> +	/*
>> +	 * To get YCBCR 420 output from LSPCON, we should send YCBCR 444
>> +	 * signals. And as per DP 1.2 spec section 2.3.4.3 while sending
>> +	 * YCBCR 444 signals we should program MSA MISC1/0 fields with
>> +	 * colorspace information.
>> +	 */
>> +	if (crtc_state->lspcon_active && crtc_state->ycbcr420)
>> +		temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
>>   	I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 737de25..787119b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4638,7 +4638,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>>   	 */
>>   	need_scaling = src_w != dst_w || src_h != dst_h;
>>   
>> -	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
>> +	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX &&
>> +		!crtc_state->lspcon_active)
>>   		need_scaling = true;
>>   
>>   	/*
>> @@ -8133,9 +8134,15 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>>   			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>>   
>>   		if (config->ycbcr420) {
>> -			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
>> -				PIPEMISC_YUV420_ENABLE |
>> -				PIPEMISC_YUV420_MODE_FULL_BLEND;
>> +			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
>> +			/*
>> +			 * LSPCON doesn't need scaling/blending to be done in
>> +			 * pipe. It just needs YCBCR444 input and proper AVI
>> +			 * infoframes for 4:2:0 output enabling.
>> +			 */
>> +			if (!config->lspcon_active)
>> +				val |= PIPEMISC_YUV420_ENABLE |
>> +				       PIPEMISC_YUV420_MODE_FULL_BLEND;
>>   		}
>>   
>>   		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index fa9e5e6..1d4b669 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1657,8 +1657,13 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>   						    intel_dp->max_link_rate);
>>   
>>   	/* LSPCON needs special handling to drive YCBCR420 outputs */
>> -	if (lspcon->active)
>> +	if (lspcon->active) {
>> +		struct drm_connector *connector = &intel_connector->base;
>> +
>>   		pipe_config->lspcon_active = true;
>> +		pipe_config->ycbcr420 = lspcon_ycbcr420_config(connector,
>> +							       pipe_config);
>> +	}
>>   
>>   	/* No common link rates between source and sink */
>>   	WARN_ON(common_len <= 0);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index a468dd6..f271967 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>   			   const struct drm_connector_state *conn_state);
>>   bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
>>   			      const struct intel_crtc_state *pipe_config);
>> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
>> +			     struct intel_crtc_state *config);
>>   
>>   /* intel_pipe_crc.c */
>>   int intel_pipe_crc_create(struct drm_minor *minor);
>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
>> index 77f0687..ca77f50 100644
>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>> @@ -180,6 +180,26 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
>>   	return true;
>>   }
>>   
>> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
>> +			     struct intel_crtc_state *config)
>> +{
>> +	struct drm_display_info *info = &connector->display_info;
>> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
>> +
>> +	if (drm_mode_is_420_only(info, mode)) {
>> +
>> +		if (!connector->ycbcr_420_allowed) {
>> +			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
>> +			return false;
>> +		}
> Seems to be always true if lspcon->active is true?
Actually you are correct, if HAS_LSPCON && lspcon->active, ycbcr_420 
will be always allowed. But as we added this flag specifically to 
control 4:2:0 support from connector, for HDMI 2.0 native controllers, I 
though it would be more readable to use this flag. Do you think so ?
>> +		config->port_clock /= 2;
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>   static bool lspcon_probe(struct intel_lspcon *lspcon)
>>   {
>>   	int retry;
>> @@ -516,6 +536,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>>   	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
>>   	struct drm_device *dev = intel_dig_port->base.base.dev;
>>   	struct drm_i915_private *dev_priv = to_i915(dev);
>> +	struct drm_connector *connector = &dp->attached_connector->base;
>>   
>>   	if (!HAS_LSPCON(dev_priv)) {
>>   		DRM_ERROR("LSPCON is not supported on this platform\n");
>> @@ -540,6 +561,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>>   		return false;
>>   	}
>>   
>> +	connector->ycbcr_420_allowed = true;
> Is this flag different from lspcon->active?
Yes, when we designed HDMI 2.0 native controller from GLK and plus 
platforms, we added this connector flag, so that, for the platforms 
which do not support 4:2:0 outputs, 4:2:0 only modes will be rejected 
from EDID's list of modes. This is the place where connector allows the 
4:2:0 capability, based on the platform, whereas is_lspcon is fro the 
encoder and state.
>>   	lspcon->active = true;
>>   	DRM_DEBUG_KMS("Success: LSPCON init\n");
>>   	return true;
> Comments from Ville still apply too. The pipe is set to ycbcr mode, so it makes more sense to have a ycbcr444 flag or something, and a separate flag to downconvert lspcon.
>
> It would be really nice if we could set the ycbcr444 flag in hw readout, and intel_pipe_config_compare. This way we know the sw mode matches the hw mode. :)
We will eventually need the right colorspace information while 
programming the AVI IF.
Actually we are trying to accommodate LSPCON into HDMI 2.0 framework as 
such. If you analyze the native 4:2:0 framework, there also we need 
scaling down, for which we use PIPEMISC register. So the requirement for 
GLK would be:
- one flag like YCBCR444 for pipe config (say state->444) to set CSC
- one flag in state to let modeset know that we have to program PIPEMISC 
and reserve a pipe_scaler (say state->420)
- one flag to set avi_if colorspace to ycbcr420 (again state->420, here 
we can't use 444, as we need accurate colorspace)

Now, in case of LSPCON,
- I still need state->444 to set CSC
- In modeset, I don't need to program PIPEMISC and scalar, as scaling 
happens in LSPCON internally so I need to know if this is LSPCON (hence 
state->lspcon)
- One flag to set appropriate avi_infoframes (so state->420)

So I realized even if I use state->444, I still need a state->420 flags 
to set correct colorspace in the avi infoframes, so there is no benefit 
of making this change.

Please let me know your opinion.

- Shashank
> ~Maarten
>

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

Re: [PATCH 4/5] drm/i915: Write AVI infoframes for Parade LSPCON

[David Weinehall]

On Wed, Dec 20, 2017 at 10:08:53AM +0530, Sharma, Shashank wrote:
> Thanks for the review, David.
> 
> My comments, inline.
> 
> 
> Regards
> 
> Shashank
> 
> 
> On 12/19/2017 3:43 PM, David Weinehall wrote:
> > On Mon, Dec 18, 2017 at 08:15:30PM +0100, Maarten Lankhorst wrote:
> > > Op 14-11-17 om 16:17 schreef Shashank Sharma:
> > > > Different LSPCON vendors specify their custom methods to pass
> > > > AVI infoframes to the LSPCON chip, so does Parade tech.
> > > > 
> > > > This patch adds functions to arrange and write AVI infoframes
> > > > into Parade LSPCON chips.
> > > > 
> > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/intel_lspcon.c | 119 +++++++++++++++++++++++++++++++++++-
> > > >   1 file changed, 118 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> > > > index 1ac4c47..77f0687 100644
> > > > --- a/drivers/gpu/drm/i915/intel_lspcon.c
> > > > +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> > > > @@ -37,6 +37,12 @@
> > > >   #define  LSPCON_MCA_AVI_IF_KICKOFF (1 << 0)
> > > >   #define  LSPCON_MCA_AVI_IF_HANDLED (1 << 1)
> > > > +/* AUX addresses to write Parade AVI IF */
> > > > +#define LSPCON_PARADE_AVI_IF_WRITE_OFFSET 0x516
> > > > +#define LSPCON_PARADE_AVI_IF_CTRL 0x51E
> > > > +#define  LSPCON_PARADE_AVI_IF_KICKOFF (1 << 7)
> > > > +#define LSPCON_PARADE_AVI_IF_DATA_SIZE 32
> > > > +
> > > >   static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
> > > >   {
> > > >   	struct intel_digital_port *dig_port =
> > > > @@ -241,6 +247,113 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
> > > >   	DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
> > > >   }
> > > > +static bool lspcon_parade_fw_ready(struct drm_dp_aux *aux)
> > > > +{
> > > > +	u8 avi_if_ctrl;
> > > > +	u8 retry;
> > > > +	ssize_t ret;
> > > > +
> > > > +	/* Check if LSPCON FW is ready for data */
> > > > +	for (retry = 0; retry < 5; retry++) {
> > > > +
> > Remove this newline.
> Why ? this is not accidental.

Because accidental or not you're not allowed to impose personal coding
style on the driver. Follow the example of the rest of the driver;
we don't insert leading blanklines inside for-blocks (or other blocks,
for that matter).

> > > > +		if (retry)
> > > > +			usleep_range(200, 300);
> > > > +
> > > > +		ret = drm_dp_dpcd_read(aux, LSPCON_PARADE_AVI_IF_CTRL,
> > > > +				       &avi_if_ctrl, 1);
> > > > +		if (ret < 0) {
> > > > +			DRM_ERROR("Failed to read AVI IF control\n");
> > > > +			return false;
> > > > +		}
> > > > +
> > > > +		if ((avi_if_ctrl & LSPCON_PARADE_AVI_IF_KICKOFF) == 0)
> > > > +			return true;
> > > > +	}
> > > > +
> > > > +	DRM_ERROR("Parade FW not ready to accept AVI IF\n");
> > > > +	return false;
> > > > +}
> > > > +
> > > > +static bool _lspcon_parade_write_infoframe_blocks(struct drm_dp_aux *aux,
> > > > +					       uint8_t *avi_buf)
> > > > +{
> > > > +	u8 avi_if_ctrl;
> > > > +	u8 block_count = 0;
> > > > +	u8 *data;
> > > > +	uint16_t reg;
> > > > +	ssize_t ret;
> > > > +
> > > > +	while (block_count < 4) {
> > > > +
> > And this one.
> Same as above.
> - Shashank
> > 
> > > > +		if (!lspcon_parade_fw_ready(aux)) {
> > > > +			DRM_DEBUG_KMS("LSPCON FW not ready, block %d\n",
> > > > +				       block_count);
> > > > +			return false;
> > > > +		}
> > > > +
> > > > +		reg = LSPCON_PARADE_AVI_IF_WRITE_OFFSET;
> > > > +		data = avi_buf + block_count * 8;
> > > > +		ret = drm_dp_dpcd_write(aux, reg, data, 8);
> > > > +		if (ret < 0) {
> > > > +			DRM_ERROR("Failed to write AVI IF block %d\n",
> > > > +				   block_count);
> > > > +			return false;
> > > > +		}
> > > > +
> > > > +		/*
> > > > +		 * Once a block of data is written, we have to inform the FW
> > > > +		 * about this by writing into avi infoframe control register:
> > > > +		 * - set the kickoff bit[7] to 1
> > > > +		 * - write the block no. to bits[1:0]
> > > > +		 */
> > > > +		reg = LSPCON_PARADE_AVI_IF_CTRL;
> > > > +		avi_if_ctrl = LSPCON_PARADE_AVI_IF_KICKOFF | block_count;
> > > > +		ret = drm_dp_dpcd_write(aux, reg, &avi_if_ctrl, 1);
> > > > +		if (ret < 0) {
> > > > +			DRM_ERROR("Failed to update (0x%x), block %d\n",
> > > > +					reg, block_count);
> > > > +			return false;
> > > > +		}
> > > > +
> > > > +		block_count++;
> > > > +	}
> > > > +
> > > > +	DRM_DEBUG_KMS("Wrote AVI IF blocks successfully\n");
> > > > +	return true;
> > > > +}
> > > > +
> > > > +static bool _lspcon_write_avi_infoframe_parade(struct drm_dp_aux *aux,
> > > > +					       const uint8_t *frame,
> > > > +					       ssize_t len)
> > > > +{
> > > > +	uint8_t avi_if[LSPCON_PARADE_AVI_IF_DATA_SIZE] = {1, };
> > > > +
> > > > +	/*
> > > > +	 * Parade's frames contains 32 bytes of data, divided
> > > > +	 * into 4 frames:
> > > > +	 *	Token byte (first byte of first frame, must be non-zero)
> > > > +	 *	HB0 to HB2	 from AVI IF (3 bytes header)
> > > > +	 *	PB0 to PB27 from AVI IF (28 bytes data)
> > > > +	 * So it should look like this
> > > > +	 *	first block: | <token> <HB0-HB2> <DB0-DB3> |
> > > > +	 *	next 3 blocks: |<DB4-DB11>|<DB12-DB19>|<DB20-DB28>|
> > > > +	 */
> > > > +
> > > > +	if (len > LSPCON_PARADE_AVI_IF_DATA_SIZE - 1) {
> > > > +		DRM_ERROR("Invalid length of infoframes\n");
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	memcpy(&avi_if[1], frame, len);
> > > > +
> > > > +	if (!_lspcon_parade_write_infoframe_blocks(aux, avi_if)) {
> > > > +		DRM_DEBUG_KMS("Failed to write infoframe blocks\n");
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	return true;
> > > > +}
> > > > +
> > > >   static bool _lspcon_write_avi_infoframe_mca(struct drm_dp_aux *aux,
> > > >   					     const uint8_t *buffer, ssize_t len)
> > > >   {
> > > > @@ -295,7 +408,7 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
> > > >   			     enum hdmi_infoframe_type type,
> > > >   			     const void *frame, ssize_t len)
> > > >   {
> > > > -	bool ret = true;
> > > > +	bool ret;
> > > >   	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > >   	struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder);
> > > > @@ -306,6 +419,10 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
> > > >   	if (lspcon->vendor == LSPCON_VENDOR_MCA)
> > > >   		ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux,
> > > >   						      frame, len);
> > > > +	else
> > > > +		ret = _lspcon_write_avi_infoframe_parade(&intel_dp->aux,
> > > > +							 frame, len);
> > > If you make it a switch (lspcon->vendor) and don't add a default case, you will get a compiler warning when you add a new vendor to the enumeration.
> > > 
> > > With that changed for first 4 patches:
> > > 
> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx