[PATCH 0/5] Enable lspcon support for GEN9 devices

[PATCH 0/5] Enable lspcon support for GEN9 devices

[Shashank Sharma]

LSPCON is essencially a dp++->hdmi adaptor with dual mode of operation.
These modes are:
- Level Shifter mode: In LS mode, this device works as a type2 dp->hdmi
===================== passive dongle, which steps up DP++ output to
appropriate HDMI 1.4 signal. This mode doesn't do any conversion at the
protocol level.

- Protocol Converter mode: In PCON mode, this device acts as a 
========================== active DP++->HDMI 2.0 dongle, which
converts the DP++ output to compatible HDMI 2.0 output. In PCON
mode, lspcon can support 4k@60 outputs, using DP HBR2 mode.

Many of Intel GEN9 devices come with in-built lspcon card
in motherboartd down mode. This patch series adds support for
lspcon devices in I915 driver.

While unit-testing this code, I was able to see a 4k@60 modeset with:
- BXT-T board
- Single HDMI 4k@60 display (ACER S)
- Ubuntu 14.04 desktop

This patch set is a re-spin of old lspcon patch series, with following
design changes / review comments addressed. 

Ville:
1. Use DRM_DP_DUAL_MODE APIs for core lspcon probing, as its
   a dp->hdmi adaptor only. 
2. Move lspcon detection code in drm_dp_dual_mode_helper layer, instead
   of keeping it in i915 layer.

Paulo:
1. Do not create a separate connector for lspcon, re-use the existing ddi
   layer, with special detection sequence (Mutually agreed by Daniel)

Shashank Sharma (5):
  drm: Helper for LSPCON in drm_dp_dual_mode
  drm/i915: Add lspcon support for I915 driver
  drm/i915: lspcon detection
  drm/i915: Parse VBT data for lspcon
  drm/i915: Enable lspcon initialization

 drivers/gpu/drm/drm_dp_dual_mode_helper.c |  85 +++++++++++++++
 drivers/gpu/drm/i915/Makefile             |   1 +
 drivers/gpu/drm/i915/i915_drv.h           |   3 +
 drivers/gpu/drm/i915/intel_bios.c         |  44 ++++++++
 drivers/gpu/drm/i915/intel_ddi.c          |  50 ++++++++-
 drivers/gpu/drm/i915/intel_dp.c           |  12 +++
 drivers/gpu/drm/i915/intel_drv.h          |  17 +++
 drivers/gpu/drm/i915/intel_hdmi.c         |  31 +++++-
 drivers/gpu/drm/i915/intel_lspcon.c       | 165 ++++++++++++++++++++++++++++++
 include/drm/drm_dp_dual_mode_helper.h     |  29 ++++++
 10 files changed, 431 insertions(+), 6 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_lspcon.c

-- 
1.9.1

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

[PATCH 1/5] drm: Helper for LSPCON in drm_dp_dual_mode

[Shashank Sharma]

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_dp_dual_mode_helper.c | 85 +++++++++++++++++++++++++++++++
 include/drm/drm_dp_dual_mode_helper.h     | 29 +++++++++++
 2 files changed, 114 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
index a7b2a75..11cab25 100644
--- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
+++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
@@ -148,6 +148,12 @@ static bool is_type2_adaptor(uint8_t adaptor_id)
 			      DP_DUAL_MODE_REV_TYPE2);
 }
 
+bool is_lspcon_adaptor(const char hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN],
+	const uint8_t adaptor_id)
+{
+	return is_hdmi_adaptor(hdmi_id) && (adaptor_id == 0xa8);
+}
+
 /**
  * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor
  * @adapter: I2C adapter for the DDC bus
@@ -203,6 +209,8 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter)
 	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_ADAPTOR_ID,
 				    &adaptor_id, sizeof(adaptor_id));
 	if (ret == 0) {
+		if (is_lspcon_adaptor(hdmi_id, adaptor_id))
+			return DRM_DP_DUAL_MODE_LSPCON;
 		if (is_type2_adaptor(adaptor_id)) {
 			if (is_hdmi_adaptor(hdmi_id))
 				return DRM_DP_DUAL_MODE_TYPE2_HDMI;
@@ -364,3 +372,80 @@ const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type)
 	}
 }
 EXPORT_SYMBOL(drm_dp_get_dual_mode_type_name);
+
+/**
+ * drm_lspcon_get_current_mode: Get LSPCON's current mode of operation by
+ * by reading offset (0x80, 0x41)
+ * @i2c_adapter: I2C-over-aux adapter
+ *
+ * Returns:
+ * Enum representing current mode of operation
+ */
+enum drm_lspcon_mode
+drm_lspcon_get_current_mode(struct i2c_adapter *adapter)
+{
+	u8 data;
+	int err = 0;
+
+	/* Read Status: i2c over aux */
+	err = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
+			(void *)&data, sizeof(data));
+	if (err < 0) {
+		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
+		return DRM_LSPCON_MODE_INVALID;
+	}
+
+	return data & DP_DUAL_MODE_LSPCON_MODE_MASK ?
+		DRM_LSPCON_MODE_PCON : DRM_LSPCON_MODE_LS;
+}
+EXPORT_SYMBOL(drm_lspcon_get_current_mode);
+
+/**
+ * drm_lspcon_change_mode: Change LSPCON's mode of operation by
+ * by writing offset (0x80, 0x40)
+ * @i2c_adapter: I2C-over-aux adapter
+ * @reqd_mode: required mode of operation
+ *
+ * Returns:
+ * 0 on success, -error on failure/timeout
+ */
+int drm_lspcon_change_mode(struct i2c_adapter *adapter,
+	enum drm_lspcon_mode reqd_mode)
+{
+	u8 data;
+	int err;
+	int time_out = 200;
+
+	if (reqd_mode == DRM_LSPCON_MODE_LS)
+		data = ~DP_DUAL_MODE_LSPCON_MODE_MASK;
+	else
+		data = DP_DUAL_MODE_LSPCON_MODE_MASK;
+
+	/* Change mode */
+	err = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_LSPCON_MODE_CHANGE,
+			&data, sizeof(data));
+	if (err < 0) {
+		DRM_ERROR("LSPCON mode change failed\n");
+		return err;
+	}
+
+	/*
+	* Confirm mode change by reading the status bit.
+	* Sometimes, it takes a while to change the mode,
+	* so wait and retry until time out or done.
+	*/
+	while (time_out) {
+		if (reqd_mode != drm_lspcon_get_current_mode(adapter)) {
+			mdelay(10);
+			time_out -= 10;
+		} else {
+			DRM_DEBUG_KMS("LSPCON mode changed to %s\n",
+			reqd_mode == DRM_LSPCON_MODE_LS ? "LS" : "PCON");
+			return 0;
+		}
+	}
+
+	DRM_ERROR("LSPCON mode change timed out\n");
+	return -EFAULT;
+}
+EXPORT_SYMBOL(drm_lspcon_change_mode);
diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h
index e8a9dfd..f335df5 100644
--- a/include/drm/drm_dp_dual_mode_helper.h
+++ b/include/drm/drm_dp_dual_mode_helper.h
@@ -24,6 +24,7 @@
 #define DRM_DP_DUAL_MODE_HELPER_H
 
 #include <linux/types.h>
+#include <drm/drm_edid.h>
 
 /*
  * Optional for type 1 DVI adaptors
@@ -54,6 +55,9 @@
 #define DP_DUAL_MODE_HDMI_PIN_CTRL 0x21
 #define  DP_DUAL_MODE_CEC_ENABLE 0x01
 #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22
+#define DP_DUAL_MODE_LSPCON_MODE_CHANGE		0x40
+#define DP_DUAL_MODE_LSPCON_CURRENT_MODE		0x41
+#define DP_DUAL_MODE_LSPCON_MODE_MASK			0x1
 
 struct i2c_adapter;
 
@@ -63,6 +67,20 @@ ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
 			       u8 offset, const void *buffer, size_t size);
 
 /**
+* enum drm_lspcon_mode
+* @lspcon_mode_invalid: No idea what's going on
+* @lspcon_mode_ls: Level shifter mode of LSPCON
+*	which drives DP++ to HDMI 1.4 conversion.
+* @lspcon_mode_pcon: Protocol converter mode of LSPCON
+*	which drives DP++ to HDMI 2.0 active conversion.
+*/
+enum drm_lspcon_mode {
+	DRM_LSPCON_MODE_INVALID,
+	DRM_LSPCON_MODE_LS,
+	DRM_LSPCON_MODE_PCON,
+};
+
+/**
  * enum drm_dp_dual_mode_type - Type of the DP dual mode adaptor
  * @DRM_DP_DUAL_MODE_NONE: No DP dual mode adaptor
  * @DRM_DP_DUAL_MODE_UNKNOWN: Could be either none or type 1 DVI adaptor
@@ -70,6 +88,7 @@ ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
  * @DRM_DP_DUAL_MODE_TYPE1_HDMI: Type 1 HDMI adaptor
  * @DRM_DP_DUAL_MODE_TYPE2_DVI: Type 2 DVI adaptor
  * @DRM_DP_DUAL_MODE_TYPE2_HDMI: Type 2 HDMI adaptor
+ * @DRM_DP_DUAL_MODE_TYPE2_LSPCON: Level shifter /protocol converter
  */
 enum drm_dp_dual_mode_type {
 	DRM_DP_DUAL_MODE_NONE,
@@ -78,6 +97,7 @@ enum drm_dp_dual_mode_type {
 	DRM_DP_DUAL_MODE_TYPE1_HDMI,
 	DRM_DP_DUAL_MODE_TYPE2_DVI,
 	DRM_DP_DUAL_MODE_TYPE2_HDMI,
+	DRM_DP_DUAL_MODE_LSPCON,
 };
 
 enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter);
@@ -89,4 +109,13 @@ int drm_dp_dual_mode_set_tmds_output(enum drm_dp_dual_mode_type type,
 				     struct i2c_adapter *adapter, bool enable);
 const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type);
 
+int drm_dp_dual_mode_ioa_read(struct i2c_adapter *adapter, u8 *buffer,
+		u8 offset, u8 no_of_bytes);
+int drm_dp_dual_mode_ioa_write(struct i2c_adapter *adapter, u8 *buffer,
+		u8 offset, u8 no_of_bytes);
+int drm_dp_dual_mode_get_edid(void *data,
+	u8 *buf, unsigned int block, size_t len);
+enum drm_lspcon_mode drm_lspcon_get_current_mode(struct i2c_adapter *adapter);
+int drm_lspcon_change_mode(struct i2c_adapter *adapter,
+		enum drm_lspcon_mode reqd_mode);
 #endif
-- 
1.9.1

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

[PATCH 2/5] drm/i915: Add lspcon support for I915 driver

[Shashank Sharma]

lspcon is a dp->hdmi adaptor has two modes of operation.
ls mode: level shifter mode, passive dp->hdmi dongle
pcon mode: protocol converter mode, active dp->hdmi 2.0
converter.

This patch adds a new file, to accommodate lspcon support
for I915 driver. These functions probe, detect, initialize
and configure an on-board lspcon device during the driver
init time.

Also, this patch adds a small structure for lspcon device,
which will provide the runtime status of the device.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com>
---
 drivers/gpu/drm/i915/Makefile       |   1 +
 drivers/gpu/drm/i915/intel_drv.h    |  13 ++-
 drivers/gpu/drm/i915/intel_lspcon.c | 159 ++++++++++++++++++++++++++++++++++++
 3 files changed, 172 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/i915/intel_lspcon.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 7e29444..1bd6026 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -93,6 +93,7 @@ i915-y += dvo_ch7017.o \
 	  intel_dvo.o \
 	  intel_hdmi.o \
 	  intel_i2c.o \
+	  intel_lspcon.o \
 	  intel_lvds.o \
 	  intel_panel.o \
 	  intel_sdvo.o \
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9b5f663..205a463 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -896,12 +896,19 @@ struct intel_dp {
 	bool compliance_test_active;
 };
 
+struct intel_lspcon {
+	bool active;
+	enum drm_lspcon_mode mode_of_op;
+	struct drm_dp_aux *aux;
+};
+
 struct intel_digital_port {
 	struct intel_encoder base;
 	enum port port;
 	u32 saved_port_bits;
 	struct intel_dp dp;
 	struct intel_hdmi hdmi;
+	struct intel_lspcon lspcon;
 	enum irqreturn (*hpd_pulse)(struct intel_digital_port *, bool);
 	bool release_cl2_override;
 	uint8_t max_lanes;
@@ -1446,7 +1453,6 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 			       struct intel_crtc_state *pipe_config);
 void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
 
-
 /* intel_lvds.c */
 void intel_lvds_init(struct drm_device *dev);
 bool intel_is_dual_link_lvds(struct drm_device *dev);
@@ -1734,4 +1740,9 @@ int intel_color_check(struct drm_crtc *crtc, struct drm_crtc_state *state);
 void intel_color_set_csc(struct drm_crtc_state *crtc_state);
 void intel_color_load_luts(struct drm_crtc_state *crtc_state);
 
+/* intel_lspcon.c */
+bool lspcon_init(struct intel_digital_port *intel_dig_port);
+enum drm_connector_status
+lspcon_ls_mode_detect(struct drm_connector *connector, bool force);
+bool is_lspcon_active(struct intel_digital_port *dig_port);
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
new file mode 100644
index 0000000..dd50491
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -0,0 +1,159 @@
+/*
+ * Copyright © 2016 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ *
+ */
+#include <drm/drm_edid.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_dp_dual_mode_helper.h>
+#include "intel_drv.h"
+
+#define LSPCON_I2C_ADDRESS			0x80
+#define LSPCON_ADAPTER_SIGN_OFFSET		0x00
+#define LSPCON_IDENTIFIER_OFFSET		0x10
+#define LSPCON_IDENTIFIER_LENGTH		0x10
+
+bool is_lspcon_active(struct intel_digital_port *dig_port)
+{
+	return dig_port->lspcon.active;
+}
+
+enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
+{
+	enum drm_lspcon_mode current_mode;
+	struct i2c_adapter *adapter = &lspcon->aux->ddc;
+
+	current_mode = drm_lspcon_get_current_mode(adapter);
+	if (current_mode == DRM_LSPCON_MODE_INVALID)
+		DRM_ERROR("Error reading LSPCON mode\n");
+	else
+		DRM_DEBUG_KMS("Current LSPCON mode %s\n",
+			current_mode == DRM_LSPCON_MODE_PCON ? "PCON" : "LS");
+	return current_mode;
+}
+
+int lspcon_change_mode(struct intel_lspcon *lspcon,
+	enum drm_lspcon_mode mode, bool force)
+{
+	int err;
+	enum drm_lspcon_mode current_mode;
+	struct i2c_adapter *adapter = &lspcon->aux->ddc;
+
+	current_mode = drm_lspcon_get_current_mode(adapter);
+	if (current_mode == DRM_LSPCON_MODE_INVALID) {
+		DRM_ERROR("Failed to get current LSPCON mode\n");
+		return -EFAULT;
+	}
+
+	if (current_mode == mode && !force) {
+		DRM_DEBUG_KMS("Current mode = desired LSPCON mode\n");
+		return 0;
+	}
+
+	err = drm_lspcon_change_mode(adapter, mode);
+	if (err < 0) {
+		DRM_ERROR("LSPCON mode change failed\n");
+		return err;
+	}
+
+	lspcon->mode_of_op = mode;
+	DRM_DEBUG_KMS("LSPCON mode changed done\n");
+	return 0;
+}
+
+bool lspcon_detect_identifier(struct intel_lspcon *lspcon)
+{
+	enum drm_dp_dual_mode_type adaptor_type;
+	struct i2c_adapter *adapter = &lspcon->aux->ddc;
+
+	/* Lets probe the adaptor and check its type */
+	adaptor_type = drm_dp_dual_mode_detect(adapter);
+	if (adaptor_type != DRM_DP_DUAL_MODE_LSPCON) {
+		DRM_DEBUG_KMS("No LSPCON detected, found %s\n",
+			drm_dp_get_dual_mode_type_name(adaptor_type));
+		return false;
+	}
+
+	/* Yay ... got a LSPCON device */
+	DRM_DEBUG_KMS("LSPCON detected\n");
+	return true;
+}
+
+enum drm_lspcon_mode lspcon_probe(struct intel_lspcon *lspcon)
+{
+	enum drm_lspcon_mode current_mode;
+
+	/* Detect a valid lspcon adaptor */
+	if (!lspcon_detect_identifier(lspcon)) {
+		DRM_DEBUG_KMS("No LSPCON identifier found\n");
+		return false;
+	}
+
+	/* Get LSPCON's mode of operation */
+	current_mode = lspcon_get_current_mode(lspcon);
+	if (current_mode == DRM_LSPCON_MODE_INVALID) {
+		DRM_ERROR("Failed to read LSPCON mode\n");
+		return false;
+	}
+
+	/* All is well */
+	lspcon->mode_of_op = current_mode;
+	lspcon->active = true;
+	return current_mode;
+}
+
+bool lspcon_init(struct intel_digital_port *intel_dig_port)
+{
+	struct intel_dp *dp = &intel_dig_port->dp;
+	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+
+	if (!IS_GEN9(dev)) {
+		DRM_ERROR("LSPCON is supported on GEN9 only\n");
+		return false;
+	}
+
+	lspcon->active = false;
+	lspcon->mode_of_op = DRM_LSPCON_MODE_INVALID;
+	lspcon->aux = &dp->aux;
+
+	if (!lspcon_probe(lspcon)) {
+		DRM_ERROR("Failed to probe lspcon\n");
+		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_of_op != DRM_LSPCON_MODE_PCON) {
+		if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON,
+			true) < 0) {
+			DRM_ERROR("LSPCON mode change to PCON failed\n");
+			return false;
+		}
+	}
+
+	DRM_DEBUG_KMS("Success: LSPCON init\n");
+	return true;
+}
-- 
1.9.1

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

[PATCH 3/5] drm/i915: lspcon detection

[Shashank Sharma]

lspcon is a tricky device to detect.
When in LS mode:
	It should be detected as a HDMI device, by reading EDID, on
	I2C over Aux chanel

When in PCON mode:
	It should be detected as a DP device by reading DPCD over the
	Aux channel.

This patch accommodates these specific requirements of lspcon detection
by adding appropriate changes in I915 drivers HDMI/DP detection sequence.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c    | 14 ++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c     | 12 ++++++++++++
 drivers/gpu/drm/i915/intel_drv.h    |  2 ++
 drivers/gpu/drm/i915/intel_hdmi.c   | 14 ++++++++++----
 drivers/gpu/drm/i915/intel_lspcon.c |  6 ++++++
 5 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index c454744..811c829 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2243,12 +2243,26 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 {
 	int type = encoder->type;
 	int port = intel_ddi_get_encoder_port(encoder);
+	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
 
 	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
 
 	if (port == PORT_A)
 		pipe_config->cpu_transcoder = TRANSCODER_EDP;
 
+	/*
+	* A digital port with active lspcon device, should be detected
+	* as HDMI when in LS mode and as DP when in PCON mode.
+	*/
+	if (is_lspcon_active(dig_port)) {
+		struct intel_lspcon *lspcon = &dig_port->lspcon;
+
+		if (lspcon->mode_of_op == DRM_LSPCON_MODE_LS)
+			type = INTEL_OUTPUT_HDMI;
+		else
+			type = INTEL_OUTPUT_DISPLAYPORT;
+	}
+
 	if (type == INTEL_OUTPUT_HDMI)
 		return intel_hdmi_compute_config(encoder, pipe_config);
 	else
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index aa9c59e..39ce16e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4276,6 +4276,18 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
 
+	/*
+	* LSPCON is a DP->HDMI converter which should be detected as
+	* HDMI in LS mode, and DP in PCON mode. So if LSPCON is in LS
+	* mode, do not try to read DPCD, but detect as HDMI.
+	*/
+	if (is_lspcon_active(intel_dig_port)) {
+		struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
+
+		if (lspcon->mode_of_op == DRM_LSPCON_MODE_LS)
+			return lspcon_ls_mode_detect(connector, force);
+	}
+
 	if (intel_dp->is_mst) {
 		/* MST devices are disconnected from a monitor POV */
 		intel_dp_unset_edid(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 205a463..fa77886 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1452,6 +1452,8 @@ struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
 bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 			       struct intel_crtc_state *pipe_config);
 void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
+enum drm_connector_status
+intel_hdmi_detect(struct drm_connector *connector, bool force);
 
 /* intel_lvds.c */
 void intel_lvds_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 6b52c6a..79184e2 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1444,15 +1444,21 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
+	struct intel_digital_port *dig_port = hdmi_to_dig_port(intel_hdmi);
+	struct i2c_adapter *adapter;
 	struct edid *edid = NULL;
 	bool connected = false;
 
 	if (force) {
 		intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
 
-		edid = drm_get_edid(connector,
-				    intel_gmbus_get_adapter(dev_priv,
-				    intel_hdmi->ddc_bus));
+		if (is_lspcon_active(dig_port))
+			adapter = &dig_port->lspcon.aux->ddc;
+		else
+			adapter = intel_gmbus_get_adapter(dev_priv,
+				intel_hdmi->ddc_bus);
+
+		edid = drm_get_edid(connector, adapter);
 
 		intel_hdmi_dp_dual_mode_detect(connector, edid != NULL);
 
@@ -1479,7 +1485,7 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
 	return connected;
 }
 
-static enum drm_connector_status
+enum drm_connector_status
 intel_hdmi_detect(struct drm_connector *connector, bool force)
 {
 	enum drm_connector_status status;
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index dd50491..75b5028 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -37,6 +37,12 @@ bool is_lspcon_active(struct intel_digital_port *dig_port)
 	return dig_port->lspcon.active;
 }
 
+enum drm_connector_status
+lspcon_ls_mode_detect(struct drm_connector *connector, bool force)
+{
+	return intel_hdmi_detect(connector, force);
+}
+
 enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
 {
 	enum drm_lspcon_mode current_mode;
-- 
1.9.1

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

[PATCH 4/5] drm/i915: Parse VBT data for lspcon

[Shashank Sharma]

Many GEN9 boards come with on-board lspcon cards.
Fot these boards, VBT configuration should properly point out
if a particular port contains lspcon device, so that driver can
initialize it properly.

This patch adds a utility function, which checks the VBT flag
for lspcon bit, and tells us if a port is configured to have a
lspcon device or not.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |  3 +++
 drivers/gpu/drm/i915/intel_bios.c | 44 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e4c8e34..32ddde5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3622,6 +3622,9 @@ bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv, enum por
 bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, enum port *port);
 bool intel_bios_is_port_hpd_inverted(struct drm_i915_private *dev_priv,
 				     enum port port);
+bool intel_bios_is_lspcon_present(struct drm_i915_private *dev_priv,
+				enum port port);
+
 
 /* intel_opregion.c */
 #ifdef CONFIG_ACPI
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 624e755..501b57f 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -1715,3 +1715,47 @@ intel_bios_is_port_hpd_inverted(struct drm_i915_private *dev_priv,
 
 	return false;
 }
+
+/**
+ * intel_bios_is_lspcon_present - if LSPCON is attached on %port
+ * @dev_priv:	i915 device instance
+ * @port:	port to check
+ *
+ * Return true if LSPCON is present on this port
+ */
+bool
+intel_bios_is_lspcon_present(struct drm_i915_private *dev_priv,
+				enum port port)
+{
+	int i;
+
+	if (WARN_ON_ONCE(!IS_GEN9(dev_priv)))
+		return false;
+
+	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
+		if (!dev_priv->vbt.child_dev[i].common.lspcon)
+			continue;
+
+		switch (dev_priv->vbt.child_dev[i].common.dvo_port) {
+		case DVO_PORT_DPA:
+		case DVO_PORT_HDMIA:
+			if (port == PORT_A)
+				return true;
+			break;
+		case DVO_PORT_DPB:
+		case DVO_PORT_HDMIB:
+			if (port == PORT_B)
+				return true;
+			break;
+		case DVO_PORT_DPC:
+		case DVO_PORT_HDMIC:
+			if (port == PORT_C)
+				return true;
+			break;
+		default:
+			break;
+		}
+	}
+
+	return false;
+}
-- 
1.9.1

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

[PATCH 5/5] drm/i915: Enable lspcon initialization

[Shashank Sharma]

This patch adds initialization code for lspcon.
What we are doing here is:
	- Check if lspcon is configured in VBT for this port
	- If lspcon is configured, configure it as DP port
	- Register additional HDMI functions to support LS
	  mode functionalities.

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

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 811c829..2107c0d 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2314,8 +2314,9 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_digital_port *intel_dig_port;
 	struct intel_encoder *intel_encoder;
+	struct intel_connector *intel_connector = NULL;
 	struct drm_encoder *encoder;
-	bool init_hdmi, init_dp;
+	bool init_hdmi, init_dp, init_lspcon = false;
 	int max_lanes;
 
 	if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) {
@@ -2347,6 +2348,19 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 	init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi ||
 		     dev_priv->vbt.ddi_port_info[port].supports_hdmi);
 	init_dp = dev_priv->vbt.ddi_port_info[port].supports_dp;
+
+	if (intel_bios_is_lspcon_present(dev_priv, port)) {
+		/*
+		* Lspcon device needs to be driven with DP connector
+		* with special detection sequence. So make sure DP
+		* is initialized before lspcon.
+		*/
+		init_dp = true;
+		init_lspcon = true;
+		init_hdmi = false;
+		DRM_DEBUG_KMS("VBT says port %c has lspcon\n", port_name(port));
+	}
+
 	if (!init_dp && !init_hdmi) {
 		DRM_DEBUG_KMS("VBT says port %c is not DVI/HDMI/DP compatible, respect it\n",
 			      port_name(port));
@@ -2399,7 +2413,8 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 	intel_encoder->cloneable = 0;
 
 	if (init_dp) {
-		if (!intel_ddi_init_dp_connector(intel_dig_port))
+		intel_connector = intel_ddi_init_dp_connector(intel_dig_port);
+		if (!intel_connector)
 			goto err;
 
 		intel_dig_port->hpd_pulse = intel_dp_hpd_pulse;
@@ -2420,6 +2435,23 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 			goto err;
 	}
 
+	if (init_lspcon) {
+		if (lspcon_init(intel_dig_port)) {
+			if (intel_hdmi_init_minimum(intel_dig_port,
+				intel_connector)) {
+				DRM_ERROR("HDMI init for LSPCON failed\n");
+				goto err;
+			}
+		} else {
+			/*
+			* LSPCON init faied, but DP init was success, so lets try to
+			* drive as DP++ port.
+			*/
+			DRM_ERROR("LSPCON init failed on port %c\n",
+				port_name(port));
+		}
+	}
+
 	return;
 
 err:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fa77886..402e656 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1454,6 +1454,10 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
 enum drm_connector_status
 intel_hdmi_detect(struct drm_connector *connector, bool force);
+int intel_hdmi_init_minimum(struct intel_digital_port *intel_dig_port,
+			       struct intel_connector *intel_connector);
+
+
 
 /* intel_lvds.c */
 void intel_lvds_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 79184e2..54b0b46 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1931,6 +1931,23 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 	}
 }
 
+int intel_hdmi_init_minimum(struct intel_digital_port *intel_dig_port,
+			       struct intel_connector *intel_connector)
+{
+	struct intel_hdmi *intel_hdmi = &intel_dig_port->hdmi;
+
+	if (WARN(intel_dig_port->max_lanes < 4,
+		"Not enough lanes (%d) for HDMI on port %c\n",
+		intel_dig_port->max_lanes, port_name(intel_dig_port->port)))
+		return -EINVAL;
+
+	intel_hdmi->write_infoframe = hsw_write_infoframe;
+	intel_hdmi->set_infoframes = hsw_set_infoframes;
+	intel_hdmi->infoframe_enabled = hsw_infoframe_enabled;
+	intel_hdmi->attached_connector = intel_connector;
+	return 0;
+}
+
 void intel_hdmi_init(struct drm_device *dev,
 		     i915_reg_t hdmi_reg, enum port port)
 {
-- 
1.9.1

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

Re: [PATCH 1/5] drm: Helper for LSPCON in drm_dp_dual_mode

[Ville Syrjälä]

On Tue, May 31, 2016 at 02:55:42PM +0530, Shashank Sharma wrote:
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 85 +++++++++++++++++++++++++++++++
>  include/drm/drm_dp_dual_mode_helper.h     | 29 +++++++++++
>  2 files changed, 114 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> index a7b2a75..11cab25 100644
> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> @@ -148,6 +148,12 @@ static bool is_type2_adaptor(uint8_t adaptor_id)
>  			      DP_DUAL_MODE_REV_TYPE2);
>  }
>  
> +bool is_lspcon_adaptor(const char hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN],
> +	const uint8_t adaptor_id)
> +{
> +	return is_hdmi_adaptor(hdmi_id) && (adaptor_id == 0xa8);


I'd probably make this something like

#define DP_DUAL_MODE_RESERVED_LSPCON 0x08

        return adaptor_id == (DP_DUAL_MODE_TYPE_TYPE2 |
	                      DP_DUAL_MODE_RESERVED_LSPCON |
	                      DP_DUAL_MODE_REV_TYPE2);


> +}
> +
>  /**
>   * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor
>   * @adapter: I2C adapter for the DDC bus
> @@ -203,6 +209,8 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter)
>  	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_ADAPTOR_ID,
>  				    &adaptor_id, sizeof(adaptor_id));
>  	if (ret == 0) {
> +		if (is_lspcon_adaptor(hdmi_id, adaptor_id))
> +			return DRM_DP_DUAL_MODE_LSPCON;
>  		if (is_type2_adaptor(adaptor_id)) {
>  			if (is_hdmi_adaptor(hdmi_id))
>  				return DRM_DP_DUAL_MODE_TYPE2_HDMI;
> @@ -364,3 +372,80 @@ const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type)
>  	}
>  }
>  EXPORT_SYMBOL(drm_dp_get_dual_mode_type_name);
> +
> +/**
> + * drm_lspcon_get_current_mode: Get LSPCON's current mode of operation by
> + * by reading offset (0x80, 0x41)
> + * @i2c_adapter: I2C-over-aux adapter
> + *
> + * Returns:
> + * Enum representing current mode of operation
> + */
> +enum drm_lspcon_mode
> +drm_lspcon_get_current_mode(struct i2c_adapter *adapter)

To match with drm_dp_dual_mode_get_tmds_output() this may want to
return the error status, and return the actual mode via a function
argument.

As for the name drm_dp_dual_mode_get_lspcon_mode() and
drm_dp_dual_mode_set_lspcon_mode() perhaps?
Or drm_lspcon_get_mode()/drm_lspcon_set_mode() perhaps if we want to
keep things shorter?

> +{
> +	u8 data;
> +	int err = 0;

I used 'ret' in the rest of the code. This is a bit inconsistent.

> +
> +	/* Read Status: i2c over aux */
> +	err = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> +			(void *)&data, sizeof(data));

Pointless cast.

> +	if (err < 0) {

if (ret) is what I've used elsewhere.

> +		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");

I'm not sure we want and ERROR from a helper. I used DRM_DEBUG_KMS for
the other error paths.

> +		return DRM_LSPCON_MODE_INVALID;
> +	}
> +
> +	return data & DP_DUAL_MODE_LSPCON_MODE_MASK ?
> +		DRM_LSPCON_MODE_PCON : DRM_LSPCON_MODE_LS;
> +}
> +EXPORT_SYMBOL(drm_lspcon_get_current_mode);
> +
> +/**
> + * drm_lspcon_change_mode: Change LSPCON's mode of operation by
> + * by writing offset (0x80, 0x40)
> + * @i2c_adapter: I2C-over-aux adapter
> + * @reqd_mode: required mode of operation
> + *
> + * Returns:
> + * 0 on success, -error on failure/timeout
> + */
> +int drm_lspcon_change_mode(struct i2c_adapter *adapter,
> +	enum drm_lspcon_mode reqd_mode)
> +{
> +	u8 data;
> +	int err;
> +	int time_out = 200;
> +
> +	if (reqd_mode == DRM_LSPCON_MODE_LS)
> +		data = ~DP_DUAL_MODE_LSPCON_MODE_MASK;

Is that really correct?

> +	else
> +		data = DP_DUAL_MODE_LSPCON_MODE_MASK;
> +
> +	/* Change mode */
> +	err = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_LSPCON_MODE_CHANGE,
> +			&data, sizeof(data));
> +	if (err < 0) {
> +		DRM_ERROR("LSPCON mode change failed\n");
> +		return err;
> +	}
> +
> +	/*
> +	* Confirm mode change by reading the status bit.
> +	* Sometimes, it takes a while to change the mode,
> +	* so wait and retry until time out or done.
> +	*/

indent fail

> +	while (time_out) {
> +		if (reqd_mode != drm_lspcon_get_current_mode(adapter)) {
> +			mdelay(10);

mdelay() is bad.

> +			time_out -= 10;
> +		} else {
> +			DRM_DEBUG_KMS("LSPCON mode changed to %s\n",
> +			reqd_mode == DRM_LSPCON_MODE_LS ? "LS" : "PCON");
> +			return 0;
> +		}
> +	}
> +
> +	DRM_ERROR("LSPCON mode change timed out\n");
> +	return -EFAULT;

EFAULT doens't seem like a sensible error value here. ETIMEDOUT I
suppose seems like what we should return.

> +}
> +EXPORT_SYMBOL(drm_lspcon_change_mode);
> diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h
> index e8a9dfd..f335df5 100644
> --- a/include/drm/drm_dp_dual_mode_helper.h
> +++ b/include/drm/drm_dp_dual_mode_helper.h
> @@ -24,6 +24,7 @@
>  #define DRM_DP_DUAL_MODE_HELPER_H
>  
>  #include <linux/types.h>
> +#include <drm/drm_edid.h>
>  
>  /*
>   * Optional for type 1 DVI adaptors
> @@ -54,6 +55,9 @@
>  #define DP_DUAL_MODE_HDMI_PIN_CTRL 0x21
>  #define  DP_DUAL_MODE_CEC_ENABLE 0x01
>  #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22

Maybe leave an empty line here to separate the standard regs from the
vendor specific ones. Hmm. Actually the vendor register space is
0x30-0x3f, whereas 0x40+ is reserved. But anyway empty line would
seem warranted.

> +#define DP_DUAL_MODE_LSPCON_MODE_CHANGE		0x40
> +#define DP_DUAL_MODE_LSPCON_CURRENT_MODE		0x41
> +#define DP_DUAL_MODE_LSPCON_MODE_MASK			0x1

Please indent the bit defnition by one space to make it stand out from
the reg defines a little.

>  struct i2c_adapter;
>  
> @@ -63,6 +67,20 @@ ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
>  			       u8 offset, const void *buffer, size_t size);
>  
>  /**
> +* enum drm_lspcon_mode
> +* @lspcon_mode_invalid: No idea what's going on
> +* @lspcon_mode_ls: Level shifter mode of LSPCON
> +*	which drives DP++ to HDMI 1.4 conversion.
> +* @lspcon_mode_pcon: Protocol converter mode of LSPCON
> +*	which drives DP++ to HDMI 2.0 active conversion.
> +*/
> +enum drm_lspcon_mode {
> +	DRM_LSPCON_MODE_INVALID,
> +	DRM_LSPCON_MODE_LS,
> +	DRM_LSPCON_MODE_PCON,
> +};

If we change the _get_mode() thing to return the error status separately
from the mode, we won't need the INVALID value.

> +
> +/**
>   * enum drm_dp_dual_mode_type - Type of the DP dual mode adaptor
>   * @DRM_DP_DUAL_MODE_NONE: No DP dual mode adaptor
>   * @DRM_DP_DUAL_MODE_UNKNOWN: Could be either none or type 1 DVI adaptor
> @@ -70,6 +88,7 @@ ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
>   * @DRM_DP_DUAL_MODE_TYPE1_HDMI: Type 1 HDMI adaptor
>   * @DRM_DP_DUAL_MODE_TYPE2_DVI: Type 2 DVI adaptor
>   * @DRM_DP_DUAL_MODE_TYPE2_HDMI: Type 2 HDMI adaptor
> + * @DRM_DP_DUAL_MODE_TYPE2_LSPCON: Level shifter /protocol converter
>   */
>  enum drm_dp_dual_mode_type {
>  	DRM_DP_DUAL_MODE_NONE,
> @@ -78,6 +97,7 @@ enum drm_dp_dual_mode_type {
>  	DRM_DP_DUAL_MODE_TYPE1_HDMI,
>  	DRM_DP_DUAL_MODE_TYPE2_DVI,
>  	DRM_DP_DUAL_MODE_TYPE2_HDMI,
> +	DRM_DP_DUAL_MODE_LSPCON,
>  };
>  
>  enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter);
> @@ -89,4 +109,13 @@ int drm_dp_dual_mode_set_tmds_output(enum drm_dp_dual_mode_type type,
>  				     struct i2c_adapter *adapter, bool enable);
>  const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type);
>  
> +int drm_dp_dual_mode_ioa_read(struct i2c_adapter *adapter, u8 *buffer,
> +		u8 offset, u8 no_of_bytes);
> +int drm_dp_dual_mode_ioa_write(struct i2c_adapter *adapter, u8 *buffer,
> +		u8 offset, u8 no_of_bytes);
> +int drm_dp_dual_mode_get_edid(void *data,
> +	u8 *buf, unsigned int block, size_t len);
> +enum drm_lspcon_mode drm_lspcon_get_current_mode(struct i2c_adapter *adapter);
> +int drm_lspcon_change_mode(struct i2c_adapter *adapter,
> +		enum drm_lspcon_mode reqd_mode);
>  #endif
> -- 
> 1.9.1

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

Re: [PATCH 1/5] drm: Helper for LSPCON in drm_dp_dual_mode

[Sharma, Shashank]

Thanks for the review, Ville.
My comments, inline.

Regards
Shashank
On 5/31/2016 3:22 PM, Ville Syrjälä wrote:
> On Tue, May 31, 2016 at 02:55:42PM +0530, Shashank Sharma wrote:
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/drm_dp_dual_mode_helper.c | 85 +++++++++++++++++++++++++++++++
>>   include/drm/drm_dp_dual_mode_helper.h     | 29 +++++++++++
>>   2 files changed, 114 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>> index a7b2a75..11cab25 100644
>> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>> @@ -148,6 +148,12 @@ static bool is_type2_adaptor(uint8_t adaptor_id)
>>   			      DP_DUAL_MODE_REV_TYPE2);
>>   }
>>
>> +bool is_lspcon_adaptor(const char hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN],
>> +	const uint8_t adaptor_id)
>> +{
>> +	return is_hdmi_adaptor(hdmi_id) && (adaptor_id == 0xa8);
>
>
> I'd probably make this something like
>
> #define DP_DUAL_MODE_RESERVED_LSPCON 0x08
>
>          return adaptor_id == (DP_DUAL_MODE_TYPE_TYPE2 |
> 	                      DP_DUAL_MODE_RESERVED_LSPCON |
> 	                      DP_DUAL_MODE_REV_TYPE2);
>
Thanks, this is a good suggestion, I will do so.
>
>> +}
>> +
>>   /**
>>    * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor
>>    * @adapter: I2C adapter for the DDC bus
>> @@ -203,6 +209,8 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter)
>>   	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_ADAPTOR_ID,
>>   				    &adaptor_id, sizeof(adaptor_id));
>>   	if (ret == 0) {
>> +		if (is_lspcon_adaptor(hdmi_id, adaptor_id))
>> +			return DRM_DP_DUAL_MODE_LSPCON;
>>   		if (is_type2_adaptor(adaptor_id)) {
>>   			if (is_hdmi_adaptor(hdmi_id))
>>   				return DRM_DP_DUAL_MODE_TYPE2_HDMI;
>> @@ -364,3 +372,80 @@ const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type)
>>   	}
>>   }
>>   EXPORT_SYMBOL(drm_dp_get_dual_mode_type_name);
>> +
>> +/**
>> + * drm_lspcon_get_current_mode: Get LSPCON's current mode of operation by
>> + * by reading offset (0x80, 0x41)
>> + * @i2c_adapter: I2C-over-aux adapter
>> + *
>> + * Returns:
>> + * Enum representing current mode of operation
>> + */
>> +enum drm_lspcon_mode
>> +drm_lspcon_get_current_mode(struct i2c_adapter *adapter)
>
> To match with drm_dp_dual_mode_get_tmds_output() this may want to
> return the error status, and return the actual mode via a function
> argument.
>
sure, can be done.
> As for the name drm_dp_dual_mode_get_lspcon_mode() and
> drm_dp_dual_mode_set_lspcon_mode() perhaps?
> Or drm_lspcon_get_mode()/drm_lspcon_set_mode() perhaps if we want to
> keep things shorter?
>
Yep, initially I made the same names as you suggested, but 
drm_dp_dual_mode_get_lspcon_mode dint sound good to me :).
drm_lspcon_get/set_mode sounds good.
>> +{
>> +	u8 data;
>> +	int err = 0;
>
> I used 'ret' in the rest of the code. This is a bit inconsistent.
got it.
>
>> +
>> +	/* Read Status: i2c over aux */
>> +	err = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>> +			(void *)&data, sizeof(data));
>
> Pointless cast.
>
good catch, this is unnecessary.
>> +	if (err < 0) {
>
> if (ret) is what I've used elsewhere.
got it.
>
>> +		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
>
> I'm not sure we want and ERROR from a helper. I used DRM_DEBUG_KMS for
> the other error paths.
>
IMHO, This is a failure which needs notice, as it can point out to a HW 
level problem, or a problem in LSPCON FW, so I will prefer to keep it as 
error.
>> +		return DRM_LSPCON_MODE_INVALID;
>> +	}
>> +
>> +	return data & DP_DUAL_MODE_LSPCON_MODE_MASK ?
>> +		DRM_LSPCON_MODE_PCON : DRM_LSPCON_MODE_LS;
>> +}
>> +EXPORT_SYMBOL(drm_lspcon_get_current_mode);
>> +
>> +/**
>> + * drm_lspcon_change_mode: Change LSPCON's mode of operation by
>> + * by writing offset (0x80, 0x40)
>> + * @i2c_adapter: I2C-over-aux adapter
>> + * @reqd_mode: required mode of operation
>> + *
>> + * Returns:
>> + * 0 on success, -error on failure/timeout
>> + */
>> +int drm_lspcon_change_mode(struct i2c_adapter *adapter,
>> +	enum drm_lspcon_mode reqd_mode)
>> +{
>> +	u8 data;
>> +	int err;
>> +	int time_out = 200;
>> +
>> +	if (reqd_mode == DRM_LSPCON_MODE_LS)
>> +		data = ~DP_DUAL_MODE_LSPCON_MODE_MASK;
>
> Is that really correct?
>
Oops, it was a blunder, but fortunately that register only bothers about 
bit 0, and that's why it worked, I will fix this.
>> +	else
>> +		data = DP_DUAL_MODE_LSPCON_MODE_MASK;
>> +
>> +	/* Change mode */
>> +	err = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_LSPCON_MODE_CHANGE,
>> +			&data, sizeof(data));
>> +	if (err < 0) {
>> +		DRM_ERROR("LSPCON mode change failed\n");
>> +		return err;
>> +	}
>> +
>> +	/*
>> +	* Confirm mode change by reading the status bit.
>> +	* Sometimes, it takes a while to change the mode,
>> +	* so wait and retry until time out or done.
>> +	*/
>
> indent fail
OK.
>
>> +	while (time_out) {
>> +		if (reqd_mode != drm_lspcon_get_current_mode(adapter)) {
>> +			mdelay(10);
>
> mdelay() is bad.
Do you think I should replace with some of the non-busy wait (like 
msleep or so ?)
>
>> +			time_out -= 10;
>> +		} else {
>> +			DRM_DEBUG_KMS("LSPCON mode changed to %s\n",
>> +			reqd_mode == DRM_LSPCON_MODE_LS ? "LS" : "PCON");
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	DRM_ERROR("LSPCON mode change timed out\n");
>> +	return -EFAULT;
>
> EFAULT doens't seem like a sensible error value here. ETIMEDOUT I
> suppose seems like what we should return.
Sure.
>
>> +}
>> +EXPORT_SYMBOL(drm_lspcon_change_mode);
>> diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h
>> index e8a9dfd..f335df5 100644
>> --- a/include/drm/drm_dp_dual_mode_helper.h
>> +++ b/include/drm/drm_dp_dual_mode_helper.h
>> @@ -24,6 +24,7 @@
>>   #define DRM_DP_DUAL_MODE_HELPER_H
>>
>>   #include <linux/types.h>
>> +#include <drm/drm_edid.h>
>>
>>   /*
>>    * Optional for type 1 DVI adaptors
>> @@ -54,6 +55,9 @@
>>   #define DP_DUAL_MODE_HDMI_PIN_CTRL 0x21
>>   #define  DP_DUAL_MODE_CEC_ENABLE 0x01
>>   #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22
>
> Maybe leave an empty line here to separate the standard regs from the
> vendor specific ones. Hmm. Actually the vendor register space is
> 0x30-0x3f, whereas 0x40+ is reserved. But anyway empty line would
> seem warranted.
>
Got it.
>> +#define DP_DUAL_MODE_LSPCON_MODE_CHANGE		0x40
>> +#define DP_DUAL_MODE_LSPCON_CURRENT_MODE		0x41
>> +#define DP_DUAL_MODE_LSPCON_MODE_MASK			0x1
>
> Please indent the bit defnition by one space to make it stand out from
> the reg defines a little.
>
Ok, the first two are registers only, I guess this is for 
DP_DUAL_MODE_LSPCON_MODE_MASK. I will align it.
>>   struct i2c_adapter;
>>
>> @@ -63,6 +67,20 @@ ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
>>   			       u8 offset, const void *buffer, size_t size);
>>
>>   /**
>> +* enum drm_lspcon_mode
>> +* @lspcon_mode_invalid: No idea what's going on
>> +* @lspcon_mode_ls: Level shifter mode of LSPCON
>> +*	which drives DP++ to HDMI 1.4 conversion.
>> +* @lspcon_mode_pcon: Protocol converter mode of LSPCON
>> +*	which drives DP++ to HDMI 2.0 active conversion.
>> +*/
>> +enum drm_lspcon_mode {
>> +	DRM_LSPCON_MODE_INVALID,
>> +	DRM_LSPCON_MODE_LS,
>> +	DRM_LSPCON_MODE_PCON,
>> +};
>
> If we change the _get_mode() thing to return the error status separately
> from the mode, we won't need the INVALID value.
>
Agree.
>> +
>> +/**
>>    * enum drm_dp_dual_mode_type - Type of the DP dual mode adaptor
>>    * @DRM_DP_DUAL_MODE_NONE: No DP dual mode adaptor
>>    * @DRM_DP_DUAL_MODE_UNKNOWN: Could be either none or type 1 DVI adaptor
>> @@ -70,6 +88,7 @@ ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
>>    * @DRM_DP_DUAL_MODE_TYPE1_HDMI: Type 1 HDMI adaptor
>>    * @DRM_DP_DUAL_MODE_TYPE2_DVI: Type 2 DVI adaptor
>>    * @DRM_DP_DUAL_MODE_TYPE2_HDMI: Type 2 HDMI adaptor
>> + * @DRM_DP_DUAL_MODE_TYPE2_LSPCON: Level shifter /protocol converter
>>    */
>>   enum drm_dp_dual_mode_type {
>>   	DRM_DP_DUAL_MODE_NONE,
>> @@ -78,6 +97,7 @@ enum drm_dp_dual_mode_type {
>>   	DRM_DP_DUAL_MODE_TYPE1_HDMI,
>>   	DRM_DP_DUAL_MODE_TYPE2_DVI,
>>   	DRM_DP_DUAL_MODE_TYPE2_HDMI,
>> +	DRM_DP_DUAL_MODE_LSPCON,
>>   };
>>
>>   enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter);
>> @@ -89,4 +109,13 @@ int drm_dp_dual_mode_set_tmds_output(enum drm_dp_dual_mode_type type,
>>   				     struct i2c_adapter *adapter, bool enable);
>>   const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type);
>>
>> +int drm_dp_dual_mode_ioa_read(struct i2c_adapter *adapter, u8 *buffer,
>> +		u8 offset, u8 no_of_bytes);
>> +int drm_dp_dual_mode_ioa_write(struct i2c_adapter *adapter, u8 *buffer,
>> +		u8 offset, u8 no_of_bytes);
>> +int drm_dp_dual_mode_get_edid(void *data,
>> +	u8 *buf, unsigned int block, size_t len);
>> +enum drm_lspcon_mode drm_lspcon_get_current_mode(struct i2c_adapter *adapter);
>> +int drm_lspcon_change_mode(struct i2c_adapter *adapter,
>> +		enum drm_lspcon_mode reqd_mode);
>>   #endif
>> --
>> 1.9.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Helper for LSPCON in drm_dp_dual_mode

[Ville Syrjälä]

On Tue, May 31, 2016 at 04:22:08PM +0530, Sharma, Shashank wrote:
> Thanks for the review, Ville.
> My comments, inline.
> 
> Regards
> Shashank
> On 5/31/2016 3:22 PM, Ville Syrjälä wrote:
> > On Tue, May 31, 2016 at 02:55:42PM +0530, Shashank Sharma wrote:
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> ---
> >>   drivers/gpu/drm/drm_dp_dual_mode_helper.c | 85 +++++++++++++++++++++++++++++++
> >>   include/drm/drm_dp_dual_mode_helper.h     | 29 +++++++++++
> >>   2 files changed, 114 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> >> index a7b2a75..11cab25 100644
> >> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> >> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> >> @@ -148,6 +148,12 @@ static bool is_type2_adaptor(uint8_t adaptor_id)
> >>   			      DP_DUAL_MODE_REV_TYPE2);
> >>   }
> >>
> >> +bool is_lspcon_adaptor(const char hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN],
> >> +	const uint8_t adaptor_id)
> >> +{
> >> +	return is_hdmi_adaptor(hdmi_id) && (adaptor_id == 0xa8);
> >
> >
> > I'd probably make this something like
> >
> > #define DP_DUAL_MODE_RESERVED_LSPCON 0x08
> >
> >          return adaptor_id == (DP_DUAL_MODE_TYPE_TYPE2 |
> > 	                      DP_DUAL_MODE_RESERVED_LSPCON |
> > 	                      DP_DUAL_MODE_REV_TYPE2);
> >
> Thanks, this is a good suggestion, I will do so.
> >
> >> +}
> >> +
> >>   /**
> >>    * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor
> >>    * @adapter: I2C adapter for the DDC bus
> >> @@ -203,6 +209,8 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter)
> >>   	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_ADAPTOR_ID,
> >>   				    &adaptor_id, sizeof(adaptor_id));
> >>   	if (ret == 0) {
> >> +		if (is_lspcon_adaptor(hdmi_id, adaptor_id))
> >> +			return DRM_DP_DUAL_MODE_LSPCON;
> >>   		if (is_type2_adaptor(adaptor_id)) {
> >>   			if (is_hdmi_adaptor(hdmi_id))
> >>   				return DRM_DP_DUAL_MODE_TYPE2_HDMI;
> >> @@ -364,3 +372,80 @@ const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type)
> >>   	}
> >>   }
> >>   EXPORT_SYMBOL(drm_dp_get_dual_mode_type_name);
> >> +
> >> +/**
> >> + * drm_lspcon_get_current_mode: Get LSPCON's current mode of operation by
> >> + * by reading offset (0x80, 0x41)
> >> + * @i2c_adapter: I2C-over-aux adapter
> >> + *
> >> + * Returns:
> >> + * Enum representing current mode of operation
> >> + */
> >> +enum drm_lspcon_mode
> >> +drm_lspcon_get_current_mode(struct i2c_adapter *adapter)
> >
> > To match with drm_dp_dual_mode_get_tmds_output() this may want to
> > return the error status, and return the actual mode via a function
> > argument.
> >
> sure, can be done.
> > As for the name drm_dp_dual_mode_get_lspcon_mode() and
> > drm_dp_dual_mode_set_lspcon_mode() perhaps?
> > Or drm_lspcon_get_mode()/drm_lspcon_set_mode() perhaps if we want to
> > keep things shorter?
> >
> Yep, initially I made the same names as you suggested, but 
> drm_dp_dual_mode_get_lspcon_mode dint sound good to me :).
> drm_lspcon_get/set_mode sounds good.
> >> +{
> >> +	u8 data;
> >> +	int err = 0;
> >
> > I used 'ret' in the rest of the code. This is a bit inconsistent.
> got it.
> >
> >> +
> >> +	/* Read Status: i2c over aux */
> >> +	err = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> >> +			(void *)&data, sizeof(data));
> >
> > Pointless cast.
> >
> good catch, this is unnecessary.
> >> +	if (err < 0) {
> >
> > if (ret) is what I've used elsewhere.
> got it.
> >
> >> +		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
> >
> > I'm not sure we want and ERROR from a helper. I used DRM_DEBUG_KMS for
> > the other error paths.
> >
> IMHO, This is a failure which needs notice, as it can point out to a HW 
> level problem, or a problem in LSPCON FW, so I will prefer to keep it as 
> error.

Fair enough.

> >> +		return DRM_LSPCON_MODE_INVALID;
> >> +	}
> >> +
> >> +	return data & DP_DUAL_MODE_LSPCON_MODE_MASK ?
> >> +		DRM_LSPCON_MODE_PCON : DRM_LSPCON_MODE_LS;
> >> +}
> >> +EXPORT_SYMBOL(drm_lspcon_get_current_mode);
> >> +
> >> +/**
> >> + * drm_lspcon_change_mode: Change LSPCON's mode of operation by
> >> + * by writing offset (0x80, 0x40)
> >> + * @i2c_adapter: I2C-over-aux adapter
> >> + * @reqd_mode: required mode of operation
> >> + *
> >> + * Returns:
> >> + * 0 on success, -error on failure/timeout
> >> + */
> >> +int drm_lspcon_change_mode(struct i2c_adapter *adapter,
> >> +	enum drm_lspcon_mode reqd_mode)
> >> +{
> >> +	u8 data;
> >> +	int err;
> >> +	int time_out = 200;
> >> +
> >> +	if (reqd_mode == DRM_LSPCON_MODE_LS)
> >> +		data = ~DP_DUAL_MODE_LSPCON_MODE_MASK;
> >
> > Is that really correct?
> >
> Oops, it was a blunder, but fortunately that register only bothers about 
> bit 0, and that's why it worked, I will fix this.
> >> +	else
> >> +		data = DP_DUAL_MODE_LSPCON_MODE_MASK;
> >> +
> >> +	/* Change mode */
> >> +	err = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_LSPCON_MODE_CHANGE,
> >> +			&data, sizeof(data));
> >> +	if (err < 0) {
> >> +		DRM_ERROR("LSPCON mode change failed\n");
> >> +		return err;
> >> +	}
> >> +
> >> +	/*
> >> +	* Confirm mode change by reading the status bit.
> >> +	* Sometimes, it takes a while to change the mode,
> >> +	* so wait and retry until time out or done.
> >> +	*/
> >
> > indent fail
> OK.
> >
> >> +	while (time_out) {
> >> +		if (reqd_mode != drm_lspcon_get_current_mode(adapter)) {
> >> +			mdelay(10);
> >
> > mdelay() is bad.
> Do you think I should replace with some of the non-busy wait (like 
> msleep or so ?)

How long would we typically have to wait for the mode to change?

> >
> >> +			time_out -= 10;
> >> +		} else {
> >> +			DRM_DEBUG_KMS("LSPCON mode changed to %s\n",
> >> +			reqd_mode == DRM_LSPCON_MODE_LS ? "LS" : "PCON");
> >> +			return 0;
> >> +		}
> >> +	}
> >> +
> >> +	DRM_ERROR("LSPCON mode change timed out\n");
> >> +	return -EFAULT;
> >
> > EFAULT doens't seem like a sensible error value here. ETIMEDOUT I
> > suppose seems like what we should return.
> Sure.
> >
> >> +}
> >> +EXPORT_SYMBOL(drm_lspcon_change_mode);
> >> diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h
> >> index e8a9dfd..f335df5 100644
> >> --- a/include/drm/drm_dp_dual_mode_helper.h
> >> +++ b/include/drm/drm_dp_dual_mode_helper.h
> >> @@ -24,6 +24,7 @@
> >>   #define DRM_DP_DUAL_MODE_HELPER_H
> >>
> >>   #include <linux/types.h>
> >> +#include <drm/drm_edid.h>
> >>
> >>   /*
> >>    * Optional for type 1 DVI adaptors
> >> @@ -54,6 +55,9 @@
> >>   #define DP_DUAL_MODE_HDMI_PIN_CTRL 0x21
> >>   #define  DP_DUAL_MODE_CEC_ENABLE 0x01
> >>   #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22
> >
> > Maybe leave an empty line here to separate the standard regs from the
> > vendor specific ones. Hmm. Actually the vendor register space is
> > 0x30-0x3f, whereas 0x40+ is reserved. But anyway empty line would
> > seem warranted.
> >
> Got it.
> >> +#define DP_DUAL_MODE_LSPCON_MODE_CHANGE		0x40
> >> +#define DP_DUAL_MODE_LSPCON_CURRENT_MODE		0x41
> >> +#define DP_DUAL_MODE_LSPCON_MODE_MASK			0x1
> >
> > Please indent the bit defnition by one space to make it stand out from
> > the reg defines a little.
> >
> Ok, the first two are registers only, I guess this is for 
> DP_DUAL_MODE_LSPCON_MODE_MASK. I will align it.
> >>   struct i2c_adapter;
> >>
> >> @@ -63,6 +67,20 @@ ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
> >>   			       u8 offset, const void *buffer, size_t size);
> >>
> >>   /**
> >> +* enum drm_lspcon_mode
> >> +* @lspcon_mode_invalid: No idea what's going on
> >> +* @lspcon_mode_ls: Level shifter mode of LSPCON
> >> +*	which drives DP++ to HDMI 1.4 conversion.
> >> +* @lspcon_mode_pcon: Protocol converter mode of LSPCON
> >> +*	which drives DP++ to HDMI 2.0 active conversion.
> >> +*/
> >> +enum drm_lspcon_mode {
> >> +	DRM_LSPCON_MODE_INVALID,
> >> +	DRM_LSPCON_MODE_LS,
> >> +	DRM_LSPCON_MODE_PCON,
> >> +};
> >
> > If we change the _get_mode() thing to return the error status separately
> > from the mode, we won't need the INVALID value.
> >
> Agree.
> >> +
> >> +/**
> >>    * enum drm_dp_dual_mode_type - Type of the DP dual mode adaptor
> >>    * @DRM_DP_DUAL_MODE_NONE: No DP dual mode adaptor
> >>    * @DRM_DP_DUAL_MODE_UNKNOWN: Could be either none or type 1 DVI adaptor
> >> @@ -70,6 +88,7 @@ ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
> >>    * @DRM_DP_DUAL_MODE_TYPE1_HDMI: Type 1 HDMI adaptor
> >>    * @DRM_DP_DUAL_MODE_TYPE2_DVI: Type 2 DVI adaptor
> >>    * @DRM_DP_DUAL_MODE_TYPE2_HDMI: Type 2 HDMI adaptor
> >> + * @DRM_DP_DUAL_MODE_TYPE2_LSPCON: Level shifter /protocol converter
> >>    */
> >>   enum drm_dp_dual_mode_type {
> >>   	DRM_DP_DUAL_MODE_NONE,
> >> @@ -78,6 +97,7 @@ enum drm_dp_dual_mode_type {
> >>   	DRM_DP_DUAL_MODE_TYPE1_HDMI,
> >>   	DRM_DP_DUAL_MODE_TYPE2_DVI,
> >>   	DRM_DP_DUAL_MODE_TYPE2_HDMI,
> >> +	DRM_DP_DUAL_MODE_LSPCON,
> >>   };
> >>
> >>   enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter);
> >> @@ -89,4 +109,13 @@ int drm_dp_dual_mode_set_tmds_output(enum drm_dp_dual_mode_type type,
> >>   				     struct i2c_adapter *adapter, bool enable);
> >>   const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type);
> >>
> >> +int drm_dp_dual_mode_ioa_read(struct i2c_adapter *adapter, u8 *buffer,
> >> +		u8 offset, u8 no_of_bytes);
> >> +int drm_dp_dual_mode_ioa_write(struct i2c_adapter *adapter, u8 *buffer,
> >> +		u8 offset, u8 no_of_bytes);
> >> +int drm_dp_dual_mode_get_edid(void *data,
> >> +	u8 *buf, unsigned int block, size_t len);
> >> +enum drm_lspcon_mode drm_lspcon_get_current_mode(struct i2c_adapter *adapter);
> >> +int drm_lspcon_change_mode(struct i2c_adapter *adapter,
> >> +		enum drm_lspcon_mode reqd_mode);
> >>   #endif
> >> --
> >> 1.9.1
> >

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

✗ Ro.CI.BAT: warning for Enable lspcon support for GEN9 devices

[Patchwork]

== Series Details ==

Series: Enable lspcon support for GEN9 devices
URL   : https://patchwork.freedesktop.org/series/8024/
State : warning

== Summary ==

Series 8024v1 Enable lspcon support for GEN9 devices
http://patchwork.freedesktop.org/api/1.0/series/8024/revisions/1/mbox

Test core_auth:
        Subgroup basic-auth:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
Test drv_module_reload_basic:
                pass       -> DMESG-WARN (fi-bdw-i7-5557u)
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (fi-hsw-i7-4770r)
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
                pass       -> DMESG-WARN (ro-hsw-i7-4770r)
                pass       -> DMESG-WARN (fi-hsw-i7-4770k)
Test gem_busy:
        Subgroup basic-blt:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
Test gem_close_race:
        Subgroup basic-process:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-cmd:
                fail       -> PASS       (fi-byt-n2820)
        Subgroup basic-uc-set-default:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
Test gem_exec_store:
        Subgroup basic-default:
                dmesg-warn -> PASS       (ro-ivb2-i7-3770)
Test gem_mmap_gtt:
        Subgroup basic-small-copy:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test gem_storedw_loop:
        Subgroup basic-bsd:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test kms_addfb_basic:
        Subgroup addfb25-framebuffer-vs-set-tiling:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
        Subgroup addfb25-x-tiled:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
        Subgroup addfb25-y-tiled-small:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
        Subgroup bad-pitch-0:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
        Subgroup bad-pitch-1024:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
        Subgroup bad-pitch-63:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
        Subgroup too-high:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
        Subgroup too-wide:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test kms_force_connector_basic:
        Subgroup prune-stale-modes:
                pass       -> DMESG-WARN (ro-ivb2-i7-3770)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)

fi-bdw-i7-5557u  total:102  pass:92   dwarn:1   dfail:0   fail:0   skip:8  
fi-bsw-n3050     total:209  pass:167  dwarn:0   dfail:0   fail:2   skip:40 
fi-byt-n2820     total:209  pass:169  dwarn:0   dfail:0   fail:2   skip:38 
fi-hsw-i7-4770k  total:209  pass:189  dwarn:1   dfail:0   fail:0   skip:19 
fi-hsw-i7-4770r  total:209  pass:185  dwarn:1   dfail:0   fail:0   skip:23 
fi-skl-i7-6700k  total:209  pass:184  dwarn:0   dfail:0   fail:0   skip:25 
fi-snb-i7-2600   total:209  pass:170  dwarn:0   dfail:0   fail:0   skip:39 
ro-bdw-i5-5250u  total:102  pass:92   dwarn:1   dfail:0   fail:0   skip:8  
ro-bdw-i7-5600u  total:102  pass:74   dwarn:1   dfail:0   fail:0   skip:26 
ro-bsw-n3050     total:209  pass:168  dwarn:0   dfail:0   fail:2   skip:39 
ro-byt-n2820     total:209  pass:169  dwarn:0   dfail:0   fail:3   skip:37 
ro-hsw-i7-4770r  total:102  pass:81   dwarn:1   dfail:0   fail:0   skip:19 
ro-ilk1-i5-650   total:204  pass:146  dwarn:0   dfail:0   fail:1   skip:57 
ro-ivb-i7-3770   total:102  pass:75   dwarn:0   dfail:0   fail:0   skip:26 
ro-ivb2-i7-3770  total:102  pass:45   dwarn:34  dfail:0   fail:0   skip:22 
ro-skl-i7-6700hq total:204  pass:178  dwarn:5   dfail:0   fail:0   skip:21 
ro-snb-i7-2620M  total:102  pass:72   dwarn:0   dfail:0   fail:0   skip:29 
ro-bdw-i7-5557U failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1059/

031f2bb drm-intel-nightly: 2016y-05m-30d-17h-51m-33s UTC integration manifest
3616edf drm/i915: Enable lspcon initialization
558b095 drm/i915: Parse VBT data for lspcon
9681714 drm/i915: lspcon detection
9792f65 drm/i915: Add lspcon support for I915 driver
1e296d6 drm: Helper for LSPCON in drm_dp_dual_mode

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

Re: [PATCH 1/5] drm: Helper for LSPCON in drm_dp_dual_mode

[Sharma, Shashank]

Regards
Shashank

On 5/31/2016 5:40 PM, Ville Syrjälä wrote:
> On Tue, May 31, 2016 at 04:22:08PM +0530, Sharma, Shashank wrote:
>> Thanks for the review, Ville.
>> My comments, inline.
>>
>> Regards
>> Shashank
>> On 5/31/2016 3:22 PM, Ville Syrjälä wrote:
>>> On Tue, May 31, 2016 at 02:55:42PM +0530, Shashank Sharma wrote:
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_dp_dual_mode_helper.c | 85 +++++++++++++++++++++++++++++++
>>>>    include/drm/drm_dp_dual_mode_helper.h     | 29 +++++++++++
>>>>    2 files changed, 114 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>>>> index a7b2a75..11cab25 100644
>>>> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>>>> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>>>> @@ -148,6 +148,12 @@ static bool is_type2_adaptor(uint8_t adaptor_id)
>>>>    			      DP_DUAL_MODE_REV_TYPE2);
>>>>    }
>>>>
>>>> +bool is_lspcon_adaptor(const char hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN],
>>>> +	const uint8_t adaptor_id)
>>>> +{
>>>> +	return is_hdmi_adaptor(hdmi_id) && (adaptor_id == 0xa8);
>>>
>>>
>>> I'd probably make this something like
>>>
>>> #define DP_DUAL_MODE_RESERVED_LSPCON 0x08
>>>
>>>           return adaptor_id == (DP_DUAL_MODE_TYPE_TYPE2 |
>>> 	                      DP_DUAL_MODE_RESERVED_LSPCON |
>>> 	                      DP_DUAL_MODE_REV_TYPE2);
>>>
>> Thanks, this is a good suggestion, I will do so.
>>>
>>>> +}
>>>> +
>>>>    /**
>>>>     * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor
>>>>     * @adapter: I2C adapter for the DDC bus
>>>> @@ -203,6 +209,8 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter)
>>>>    	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_ADAPTOR_ID,
>>>>    				    &adaptor_id, sizeof(adaptor_id));
>>>>    	if (ret == 0) {
>>>> +		if (is_lspcon_adaptor(hdmi_id, adaptor_id))
>>>> +			return DRM_DP_DUAL_MODE_LSPCON;
>>>>    		if (is_type2_adaptor(adaptor_id)) {
>>>>    			if (is_hdmi_adaptor(hdmi_id))
>>>>    				return DRM_DP_DUAL_MODE_TYPE2_HDMI;
>>>> @@ -364,3 +372,80 @@ const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type)
>>>>    	}
>>>>    }
>>>>    EXPORT_SYMBOL(drm_dp_get_dual_mode_type_name);
>>>> +
>>>> +/**
>>>> + * drm_lspcon_get_current_mode: Get LSPCON's current mode of operation by
>>>> + * by reading offset (0x80, 0x41)
>>>> + * @i2c_adapter: I2C-over-aux adapter
>>>> + *
>>>> + * Returns:
>>>> + * Enum representing current mode of operation
>>>> + */
>>>> +enum drm_lspcon_mode
>>>> +drm_lspcon_get_current_mode(struct i2c_adapter *adapter)
>>>
>>> To match with drm_dp_dual_mode_get_tmds_output() this may want to
>>> return the error status, and return the actual mode via a function
>>> argument.
>>>
>> sure, can be done.
>>> As for the name drm_dp_dual_mode_get_lspcon_mode() and
>>> drm_dp_dual_mode_set_lspcon_mode() perhaps?
>>> Or drm_lspcon_get_mode()/drm_lspcon_set_mode() perhaps if we want to
>>> keep things shorter?
>>>
>> Yep, initially I made the same names as you suggested, but
>> drm_dp_dual_mode_get_lspcon_mode dint sound good to me :).
>> drm_lspcon_get/set_mode sounds good.
>>>> +{
>>>> +	u8 data;
>>>> +	int err = 0;
>>>
>>> I used 'ret' in the rest of the code. This is a bit inconsistent.
>> got it.
>>>
>>>> +
>>>> +	/* Read Status: i2c over aux */
>>>> +	err = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>>>> +			(void *)&data, sizeof(data));
>>>
>>> Pointless cast.
>>>
>> good catch, this is unnecessary.
>>>> +	if (err < 0) {
>>>
>>> if (ret) is what I've used elsewhere.
>> got it.
>>>
>>>> +		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
>>>
>>> I'm not sure we want and ERROR from a helper. I used DRM_DEBUG_KMS for
>>> the other error paths.
>>>
>> IMHO, This is a failure which needs notice, as it can point out to a HW
>> level problem, or a problem in LSPCON FW, so I will prefer to keep it as
>> error.
>
> Fair enough.
>
>>>> +		return DRM_LSPCON_MODE_INVALID;
>>>> +	}
>>>> +
>>>> +	return data & DP_DUAL_MODE_LSPCON_MODE_MASK ?
>>>> +		DRM_LSPCON_MODE_PCON : DRM_LSPCON_MODE_LS;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_lspcon_get_current_mode);
>>>> +
>>>> +/**
>>>> + * drm_lspcon_change_mode: Change LSPCON's mode of operation by
>>>> + * by writing offset (0x80, 0x40)
>>>> + * @i2c_adapter: I2C-over-aux adapter
>>>> + * @reqd_mode: required mode of operation
>>>> + *
>>>> + * Returns:
>>>> + * 0 on success, -error on failure/timeout
>>>> + */
>>>> +int drm_lspcon_change_mode(struct i2c_adapter *adapter,
>>>> +	enum drm_lspcon_mode reqd_mode)
>>>> +{
>>>> +	u8 data;
>>>> +	int err;
>>>> +	int time_out = 200;
>>>> +
>>>> +	if (reqd_mode == DRM_LSPCON_MODE_LS)
>>>> +		data = ~DP_DUAL_MODE_LSPCON_MODE_MASK;
>>>
>>> Is that really correct?
>>>
>> Oops, it was a blunder, but fortunately that register only bothers about
>> bit 0, and that's why it worked, I will fix this.
>>>> +	else
>>>> +		data = DP_DUAL_MODE_LSPCON_MODE_MASK;
>>>> +
>>>> +	/* Change mode */
>>>> +	err = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_LSPCON_MODE_CHANGE,
>>>> +			&data, sizeof(data));
>>>> +	if (err < 0) {
>>>> +		DRM_ERROR("LSPCON mode change failed\n");
>>>> +		return err;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	* Confirm mode change by reading the status bit.
>>>> +	* Sometimes, it takes a while to change the mode,
>>>> +	* so wait and retry until time out or done.
>>>> +	*/
>>>
>>> indent fail
>> OK.
>>>
>>>> +	while (time_out) {
>>>> +		if (reqd_mode != drm_lspcon_get_current_mode(adapter)) {
>>>> +			mdelay(10);
>>>
>>> mdelay() is bad.
>> Do you think I should replace with some of the non-busy wait (like
>> msleep or so ?)
>
> How long would we typically have to wait for the mode to change?
>
In a normal scenario, it takes max 1-2 iterations (max 20ms) but
on Android trees, We had initially observed it taking as much as 300ms, 
(but thats once in a blue moon). MCA has made some changes in the latest 
LSPCON FW, to make it better. But 200ms is just to be in the safer side.
>>>
>>>> +			time_out -= 10;
>>>> +		} else {
>>>> +			DRM_DEBUG_KMS("LSPCON mode changed to %s\n",
>>>> +			reqd_mode == DRM_LSPCON_MODE_LS ? "LS" : "PCON");
>>>> +			return 0;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	DRM_ERROR("LSPCON mode change timed out\n");
>>>> +	return -EFAULT;
>>>
>>> EFAULT doens't seem like a sensible error value here. ETIMEDOUT I
>>> suppose seems like what we should return.
>> Sure.
>>>
>>>> +}
>>>> +EXPORT_SYMBOL(drm_lspcon_change_mode);
>>>> diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h
>>>> index e8a9dfd..f335df5 100644
>>>> --- a/include/drm/drm_dp_dual_mode_helper.h
>>>> +++ b/include/drm/drm_dp_dual_mode_helper.h
>>>> @@ -24,6 +24,7 @@
>>>>    #define DRM_DP_DUAL_MODE_HELPER_H
>>>>
>>>>    #include <linux/types.h>
>>>> +#include <drm/drm_edid.h>
>>>>
>>>>    /*
>>>>     * Optional for type 1 DVI adaptors
>>>> @@ -54,6 +55,9 @@
>>>>    #define DP_DUAL_MODE_HDMI_PIN_CTRL 0x21
>>>>    #define  DP_DUAL_MODE_CEC_ENABLE 0x01
>>>>    #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22
>>>
>>> Maybe leave an empty line here to separate the standard regs from the
>>> vendor specific ones. Hmm. Actually the vendor register space is
>>> 0x30-0x3f, whereas 0x40+ is reserved. But anyway empty line would
>>> seem warranted.
>>>
>> Got it.
>>>> +#define DP_DUAL_MODE_LSPCON_MODE_CHANGE		0x40
>>>> +#define DP_DUAL_MODE_LSPCON_CURRENT_MODE		0x41
>>>> +#define DP_DUAL_MODE_LSPCON_MODE_MASK			0x1
>>>
>>> Please indent the bit defnition by one space to make it stand out from
>>> the reg defines a little.
>>>
>> Ok, the first two are registers only, I guess this is for
>> DP_DUAL_MODE_LSPCON_MODE_MASK. I will align it.
>>>>    struct i2c_adapter;
>>>>
>>>> @@ -63,6 +67,20 @@ ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
>>>>    			       u8 offset, const void *buffer, size_t size);
>>>>
>>>>    /**
>>>> +* enum drm_lspcon_mode
>>>> +* @lspcon_mode_invalid: No idea what's going on
>>>> +* @lspcon_mode_ls: Level shifter mode of LSPCON
>>>> +*	which drives DP++ to HDMI 1.4 conversion.
>>>> +* @lspcon_mode_pcon: Protocol converter mode of LSPCON
>>>> +*	which drives DP++ to HDMI 2.0 active conversion.
>>>> +*/
>>>> +enum drm_lspcon_mode {
>>>> +	DRM_LSPCON_MODE_INVALID,
>>>> +	DRM_LSPCON_MODE_LS,
>>>> +	DRM_LSPCON_MODE_PCON,
>>>> +};
>>>
>>> If we change the _get_mode() thing to return the error status separately
>>> from the mode, we won't need the INVALID value.
>>>
>> Agree.
>>>> +
>>>> +/**
>>>>     * enum drm_dp_dual_mode_type - Type of the DP dual mode adaptor
>>>>     * @DRM_DP_DUAL_MODE_NONE: No DP dual mode adaptor
>>>>     * @DRM_DP_DUAL_MODE_UNKNOWN: Could be either none or type 1 DVI adaptor
>>>> @@ -70,6 +88,7 @@ ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
>>>>     * @DRM_DP_DUAL_MODE_TYPE1_HDMI: Type 1 HDMI adaptor
>>>>     * @DRM_DP_DUAL_MODE_TYPE2_DVI: Type 2 DVI adaptor
>>>>     * @DRM_DP_DUAL_MODE_TYPE2_HDMI: Type 2 HDMI adaptor
>>>> + * @DRM_DP_DUAL_MODE_TYPE2_LSPCON: Level shifter /protocol converter
>>>>     */
>>>>    enum drm_dp_dual_mode_type {
>>>>    	DRM_DP_DUAL_MODE_NONE,
>>>> @@ -78,6 +97,7 @@ enum drm_dp_dual_mode_type {
>>>>    	DRM_DP_DUAL_MODE_TYPE1_HDMI,
>>>>    	DRM_DP_DUAL_MODE_TYPE2_DVI,
>>>>    	DRM_DP_DUAL_MODE_TYPE2_HDMI,
>>>> +	DRM_DP_DUAL_MODE_LSPCON,
>>>>    };
>>>>
>>>>    enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter);
>>>> @@ -89,4 +109,13 @@ int drm_dp_dual_mode_set_tmds_output(enum drm_dp_dual_mode_type type,
>>>>    				     struct i2c_adapter *adapter, bool enable);
>>>>    const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type);
>>>>
>>>> +int drm_dp_dual_mode_ioa_read(struct i2c_adapter *adapter, u8 *buffer,
>>>> +		u8 offset, u8 no_of_bytes);
>>>> +int drm_dp_dual_mode_ioa_write(struct i2c_adapter *adapter, u8 *buffer,
>>>> +		u8 offset, u8 no_of_bytes);
>>>> +int drm_dp_dual_mode_get_edid(void *data,
>>>> +	u8 *buf, unsigned int block, size_t len);
>>>> +enum drm_lspcon_mode drm_lspcon_get_current_mode(struct i2c_adapter *adapter);
>>>> +int drm_lspcon_change_mode(struct i2c_adapter *adapter,
>>>> +		enum drm_lspcon_mode reqd_mode);
>>>>    #endif
>>>> --
>>>> 1.9.1
>>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Helper for LSPCON in drm_dp_dual_mode

[Ville Syrjälä]

On Tue, May 31, 2016 at 02:55:42PM +0530, Shashank Sharma wrote:
> @@ -78,6 +97,7 @@ enum drm_dp_dual_mode_type {
>  	DRM_DP_DUAL_MODE_TYPE1_HDMI,
>  	DRM_DP_DUAL_MODE_TYPE2_DVI,
>  	DRM_DP_DUAL_MODE_TYPE2_HDMI,
> +	DRM_DP_DUAL_MODE_LSPCON,
>  };
>  
>  enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter);
> @@ -89,4 +109,13 @@ int drm_dp_dual_mode_set_tmds_output(enum drm_dp_dual_mode_type type,
>  				     struct i2c_adapter *adapter, bool enable);
>  const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type);
>  
> +int drm_dp_dual_mode_ioa_read(struct i2c_adapter *adapter, u8 *buffer,
> +		u8 offset, u8 no_of_bytes);
> +int drm_dp_dual_mode_ioa_write(struct i2c_adapter *adapter, u8 *buffer,
> +		u8 offset, u8 no_of_bytes);
> +int drm_dp_dual_mode_get_edid(void *data,
> +	u8 *buf, unsigned int block, size_t len);

These look like leftovers from an earlier attempt.

> +enum drm_lspcon_mode drm_lspcon_get_current_mode(struct i2c_adapter *adapter);
> +int drm_lspcon_change_mode(struct i2c_adapter *adapter,
> +		enum drm_lspcon_mode reqd_mode);
>  #endif
> -- 
> 1.9.1

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

Re: [PATCH 2/5] drm/i915: Add lspcon support for I915 driver

[Ville Syrjälä]

On Tue, May 31, 2016 at 02:55:43PM +0530, Shashank Sharma wrote:
> lspcon is a dp->hdmi adaptor has two modes of operation.
> ls mode: level shifter mode, passive dp->hdmi dongle
> pcon mode: protocol converter mode, active dp->hdmi 2.0
> converter.
> 
> This patch adds a new file, to accommodate lspcon support
> for I915 driver. These functions probe, detect, initialize
> and configure an on-board lspcon device during the driver
> init time.
> 
> Also, this patch adds a small structure for lspcon device,
> which will provide the runtime status of the device.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile       |   1 +
>  drivers/gpu/drm/i915/intel_drv.h    |  13 ++-
>  drivers/gpu/drm/i915/intel_lspcon.c | 159 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 172 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_lspcon.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 7e29444..1bd6026 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -93,6 +93,7 @@ i915-y += dvo_ch7017.o \
>  	  intel_dvo.o \
>  	  intel_hdmi.o \
>  	  intel_i2c.o \
> +	  intel_lspcon.o \
>  	  intel_lvds.o \
>  	  intel_panel.o \
>  	  intel_sdvo.o \
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9b5f663..205a463 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -896,12 +896,19 @@ struct intel_dp {
>  	bool compliance_test_active;
>  };
>  
> +struct intel_lspcon {
> +	bool active;
> +	enum drm_lspcon_mode mode_of_op;
> +	struct drm_dp_aux *aux;
> +};
> +
>  struct intel_digital_port {
>  	struct intel_encoder base;
>  	enum port port;
>  	u32 saved_port_bits;
>  	struct intel_dp dp;
>  	struct intel_hdmi hdmi;
> +	struct intel_lspcon lspcon;
>  	enum irqreturn (*hpd_pulse)(struct intel_digital_port *, bool);
>  	bool release_cl2_override;
>  	uint8_t max_lanes;
> @@ -1446,7 +1453,6 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  			       struct intel_crtc_state *pipe_config);
>  void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
>  
> -
>  /* intel_lvds.c */
>  void intel_lvds_init(struct drm_device *dev);
>  bool intel_is_dual_link_lvds(struct drm_device *dev);
> @@ -1734,4 +1740,9 @@ int intel_color_check(struct drm_crtc *crtc, struct drm_crtc_state *state);
>  void intel_color_set_csc(struct drm_crtc_state *crtc_state);
>  void intel_color_load_luts(struct drm_crtc_state *crtc_state);
>  
> +/* intel_lspcon.c */
> +bool lspcon_init(struct intel_digital_port *intel_dig_port);
> +enum drm_connector_status
> +lspcon_ls_mode_detect(struct drm_connector *connector, bool force);
> +bool is_lspcon_active(struct intel_digital_port *dig_port);
>  #endif /* __INTEL_DRV_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> new file mode 100644
> index 0000000..dd50491
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -0,0 +1,159 @@
> +/*
> + * Copyright © 2016 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + *
> + */
> +#include <drm/drm_edid.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_dp_dual_mode_helper.h>
> +#include "intel_drv.h"
> +
> +#define LSPCON_I2C_ADDRESS			0x80
> +#define LSPCON_ADAPTER_SIGN_OFFSET		0x00
> +#define LSPCON_IDENTIFIER_OFFSET		0x10
> +#define LSPCON_IDENTIFIER_LENGTH		0x10

Leftovers?

> +
> +bool is_lspcon_active(struct intel_digital_port *dig_port)
> +{
> +	return dig_port->lspcon.active;
> +}
> +
> +enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
> +{
> +	enum drm_lspcon_mode current_mode;
> +	struct i2c_adapter *adapter = &lspcon->aux->ddc;
> +
> +	current_mode = drm_lspcon_get_current_mode(adapter);
> +	if (current_mode == DRM_LSPCON_MODE_INVALID)
> +		DRM_ERROR("Error reading LSPCON mode\n");
> +	else
> +		DRM_DEBUG_KMS("Current LSPCON mode %s\n",
> +			current_mode == DRM_LSPCON_MODE_PCON ? "PCON" : "LS");
> +	return current_mode;
> +}

This file seems to be just contain these sort of wrappers that don't
look very useful to me.

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

Re: [PATCH 1/5] drm: Helper for LSPCON in drm_dp_dual_mode

[Sharma, Shashank]

Regards
Shashank

On 5/31/2016 9:35 PM, Ville Syrjälä wrote:
> On Tue, May 31, 2016 at 02:55:42PM +0530, Shashank Sharma wrote:
>> @@ -78,6 +97,7 @@ enum drm_dp_dual_mode_type {
>>   	DRM_DP_DUAL_MODE_TYPE1_HDMI,
>>   	DRM_DP_DUAL_MODE_TYPE2_DVI,
>>   	DRM_DP_DUAL_MODE_TYPE2_HDMI,
>> +	DRM_DP_DUAL_MODE_LSPCON,
>>   };
>>
>>   enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter);
>> @@ -89,4 +109,13 @@ int drm_dp_dual_mode_set_tmds_output(enum drm_dp_dual_mode_type type,
>>   				     struct i2c_adapter *adapter, bool enable);
>>   const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type);
>>
>> +int drm_dp_dual_mode_ioa_read(struct i2c_adapter *adapter, u8 *buffer,
>> +		u8 offset, u8 no_of_bytes);
>> +int drm_dp_dual_mode_ioa_write(struct i2c_adapter *adapter, u8 *buffer,
>> +		u8 offset, u8 no_of_bytes);
>> +int drm_dp_dual_mode_get_edid(void *data,
>> +	u8 *buf, unsigned int block, size_t len);
>
> These look like leftovers from an earlier attempt.
Yes, I noticed I missed them during cleanup. Will remove these.
>
>> +enum drm_lspcon_mode drm_lspcon_get_current_mode(struct i2c_adapter *adapter);
>> +int drm_lspcon_change_mode(struct i2c_adapter *adapter,
>> +		enum drm_lspcon_mode reqd_mode);
>>   #endif
>> --
>> 1.9.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Add lspcon support for I915 driver

[Sharma, Shashank]

Regards
Shashank

On 5/31/2016 9:38 PM, Ville Syrjälä wrote:
> On Tue, May 31, 2016 at 02:55:43PM +0530, Shashank Sharma wrote:
>> lspcon is a dp->hdmi adaptor has two modes of operation.
>> ls mode: level shifter mode, passive dp->hdmi dongle
>> pcon mode: protocol converter mode, active dp->hdmi 2.0
>> converter.
>>
>> This patch adds a new file, to accommodate lspcon support
>> for I915 driver. These functions probe, detect, initialize
>> and configure an on-board lspcon device during the driver
>> init time.
>>
>> Also, this patch adds a small structure for lspcon device,
>> which will provide the runtime status of the device.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/Makefile       |   1 +
>>   drivers/gpu/drm/i915/intel_drv.h    |  13 ++-
>>   drivers/gpu/drm/i915/intel_lspcon.c | 159 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 172 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/i915/intel_lspcon.c
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index 7e29444..1bd6026 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -93,6 +93,7 @@ i915-y += dvo_ch7017.o \
>>   	  intel_dvo.o \
>>   	  intel_hdmi.o \
>>   	  intel_i2c.o \
>> +	  intel_lspcon.o \
>>   	  intel_lvds.o \
>>   	  intel_panel.o \
>>   	  intel_sdvo.o \
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 9b5f663..205a463 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -896,12 +896,19 @@ struct intel_dp {
>>   	bool compliance_test_active;
>>   };
>>
>> +struct intel_lspcon {
>> +	bool active;
>> +	enum drm_lspcon_mode mode_of_op;
>> +	struct drm_dp_aux *aux;
>> +};
>> +
>>   struct intel_digital_port {
>>   	struct intel_encoder base;
>>   	enum port port;
>>   	u32 saved_port_bits;
>>   	struct intel_dp dp;
>>   	struct intel_hdmi hdmi;
>> +	struct intel_lspcon lspcon;
>>   	enum irqreturn (*hpd_pulse)(struct intel_digital_port *, bool);
>>   	bool release_cl2_override;
>>   	uint8_t max_lanes;
>> @@ -1446,7 +1453,6 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>   			       struct intel_crtc_state *pipe_config);
>>   void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
>>
>> -
>>   /* intel_lvds.c */
>>   void intel_lvds_init(struct drm_device *dev);
>>   bool intel_is_dual_link_lvds(struct drm_device *dev);
>> @@ -1734,4 +1740,9 @@ int intel_color_check(struct drm_crtc *crtc, struct drm_crtc_state *state);
>>   void intel_color_set_csc(struct drm_crtc_state *crtc_state);
>>   void intel_color_load_luts(struct drm_crtc_state *crtc_state);
>>
>> +/* intel_lspcon.c */
>> +bool lspcon_init(struct intel_digital_port *intel_dig_port);
>> +enum drm_connector_status
>> +lspcon_ls_mode_detect(struct drm_connector *connector, bool force);
>> +bool is_lspcon_active(struct intel_digital_port *dig_port);
>>   #endif /* __INTEL_DRV_H__ */
>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
>> new file mode 100644
>> index 0000000..dd50491
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>> @@ -0,0 +1,159 @@
>> +/*
>> + * Copyright © 2016 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + *
>> + *
>> + */
>> +#include <drm/drm_edid.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_dp_dual_mode_helper.h>
>> +#include "intel_drv.h"
>> +
>> +#define LSPCON_I2C_ADDRESS			0x80
>> +#define LSPCON_ADAPTER_SIGN_OFFSET		0x00
>> +#define LSPCON_IDENTIFIER_OFFSET		0x10
>> +#define LSPCON_IDENTIFIER_LENGTH		0x10
>
> Leftovers?
Yep, will clean this up too.
>
>> +
>> +bool is_lspcon_active(struct intel_digital_port *dig_port)
>> +{
>> +	return dig_port->lspcon.active;
>> +}
>> +
>> +enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
>> +{
>> +	enum drm_lspcon_mode current_mode;
>> +	struct i2c_adapter *adapter = &lspcon->aux->ddc;
>> +
>> +	current_mode = drm_lspcon_get_current_mode(adapter);
>> +	if (current_mode == DRM_LSPCON_MODE_INVALID)
>> +		DRM_ERROR("Error reading LSPCON mode\n");
>> +	else
>> +		DRM_DEBUG_KMS("Current LSPCON mode %s\n",
>> +			current_mode == DRM_LSPCON_MODE_PCON ? "PCON" : "LS");
>> +	return current_mode;
>> +}
>
> This file seems to be just contain these sort of wrappers that don't
> look very useful to me.
>
I agree, that most of the real work is happening in dp_dual_mode layer 
now. Actually, the idea behind this file is to contains all the I915 
specific operations, for lspcon.

This API (get_current_mode) is a small function, but if you see others 
(like init and probe) they do specific work which we want to do in I915 
lspcon handling. This is good for readability as well as this makes code 
modular too.

For example, reading i2c_ove_aux address 0x80 offset 0x41, doesn't make 
much sense for ppl who don't know much about lspcon, but create a 
wrapper function and let it return ls_mode or pcon_mode, makes code very 
simple to read and debug.

Going forward, if there is a requirement to support other HDMI 2.0 
features over lspcon, there would be scope for expansion of file with 
many more wrappers and some core functions too.

Please let me know, if this idea makes any sense :)

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

Re: [PATCH 3/5] drm/i915: lspcon detection

[Ville Syrjälä]

On Tue, May 31, 2016 at 02:55:44PM +0530, Shashank Sharma wrote:
> lspcon is a tricky device to detect.
> When in LS mode:
> 	It should be detected as a HDMI device, by reading EDID, on
> 	I2C over Aux chanel
> 
> When in PCON mode:
> 	It should be detected as a DP device by reading DPCD over the
> 	Aux channel.
> 
> This patch accommodates these specific requirements of lspcon detection
> by adding appropriate changes in I915 drivers HDMI/DP detection sequence.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c    | 14 ++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c     | 12 ++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h    |  2 ++
>  drivers/gpu/drm/i915/intel_hdmi.c   | 14 ++++++++++----
>  drivers/gpu/drm/i915/intel_lspcon.c |  6 ++++++
>  5 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index c454744..811c829 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2243,12 +2243,26 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>  {
>  	int type = encoder->type;
>  	int port = intel_ddi_get_encoder_port(encoder);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
>  
>  	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
>  
>  	if (port == PORT_A)
>  		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>  
> +	/*
> +	* A digital port with active lspcon device, should be detected
> +	* as HDMI when in LS mode and as DP when in PCON mode.
> +	*/
> +	if (is_lspcon_active(dig_port)) {
> +		struct intel_lspcon *lspcon = &dig_port->lspcon;
> +
> +		if (lspcon->mode_of_op == DRM_LSPCON_MODE_LS)
> +			type = INTEL_OUTPUT_HDMI;
> +		else
> +			type = INTEL_OUTPUT_DISPLAYPORT;
> +	}

Argh. I really wanted to kill this dual role DDI encoder mess (see [1])

I realize that with LSPCON we probably don't want separate encoders for
the two roles. Or maybe we do? At least we don't want two connectors,
which probably means two encoders might become messy.

In any case I don't think we want to be frobbing around with the
encoder->type anymore. Maybe I need to rethink my approach to DDI
encoders, and try to come up with something sane. For LSPCON at least
we want to keep track of the current mode in pipe_config most likely.
For LSPCON it's maybe a bit easier since the mode won't affect the
DDC stuff and whanot, so from the probe side there is just one role
except perhaps w.r.t. DPCD. For regular DDI stuff I'm still thinking
two encoder might be the most sensible apporach since we have totally
different paths for probe as well.

Probably the biggest kink in all of this is hpd handling. What, if
anything, should hpd_pulse do for LSPCON? And if something, should
it only do it in PCON mode? How is HPD routed anyway with LSPCON?

[1] https://patchwork.freedesktop.org/series/1596/

> +
>  	if (type == INTEL_OUTPUT_HDMI)
>  		return intel_hdmi_compute_config(encoder, pipe_config);
>  	else
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index aa9c59e..39ce16e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4276,6 +4276,18 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>  		      connector->base.id, connector->name);
>  
> +	/*
> +	* LSPCON is a DP->HDMI converter which should be detected as
> +	* HDMI in LS mode, and DP in PCON mode. So if LSPCON is in LS
> +	* mode, do not try to read DPCD, but detect as HDMI.
> +	*/
> +	if (is_lspcon_active(intel_dig_port)) {
> +		struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
> +
> +		if (lspcon->mode_of_op == DRM_LSPCON_MODE_LS)
> +			return lspcon_ls_mode_detect(connector, force);
> +	}
> +
>  	if (intel_dp->is_mst) {
>  		/* MST devices are disconnected from a monitor POV */
>  		intel_dp_unset_edid(intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 205a463..fa77886 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1452,6 +1452,8 @@ struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
>  bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  			       struct intel_crtc_state *pipe_config);
>  void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
> +enum drm_connector_status
> +intel_hdmi_detect(struct drm_connector *connector, bool force);
>  
>  /* intel_lvds.c */
>  void intel_lvds_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 6b52c6a..79184e2 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1444,15 +1444,21 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>  	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> +	struct intel_digital_port *dig_port = hdmi_to_dig_port(intel_hdmi);
> +	struct i2c_adapter *adapter;
>  	struct edid *edid = NULL;
>  	bool connected = false;
>  
>  	if (force) {
>  		intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
>  
> -		edid = drm_get_edid(connector,
> -				    intel_gmbus_get_adapter(dev_priv,
> -				    intel_hdmi->ddc_bus));
> +		if (is_lspcon_active(dig_port))
> +			adapter = &dig_port->lspcon.aux->ddc;
> +		else
> +			adapter = intel_gmbus_get_adapter(dev_priv,
> +				intel_hdmi->ddc_bus);
> +
> +		edid = drm_get_edid(connector, adapter);

I'm not a fan of this. This is even taking the wrong power domain
now, as does intel_hdmi_detect(). Probably LSPCON should just have
a dedicated codepath for this stuff.

If we ever have a board design with a DP++ port without a GMBUS
connection, then we might have rethink things because then we'd have to
use AUX also for HDMI DDC. But so far I'm not aware of such board
designs existing, so might as well keep LSPCON separate.

>  
>  		intel_hdmi_dp_dual_mode_detect(connector, edid != NULL);
>  
> @@ -1479,7 +1485,7 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
>  	return connected;
>  }
>  
> -static enum drm_connector_status
> +enum drm_connector_status
>  intel_hdmi_detect(struct drm_connector *connector, bool force)
>  {
>  	enum drm_connector_status status;
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index dd50491..75b5028 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -37,6 +37,12 @@ bool is_lspcon_active(struct intel_digital_port *dig_port)
>  	return dig_port->lspcon.active;
>  }
>  
> +enum drm_connector_status
> +lspcon_ls_mode_detect(struct drm_connector *connector, bool force)
> +{
> +	return intel_hdmi_detect(connector, force);
> +}
> +
>  enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
>  {
>  	enum drm_lspcon_mode current_mode;
> -- 
> 1.9.1

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

Re: [PATCH 4/5] drm/i915: Parse VBT data for lspcon

[Ville Syrjälä]

On Tue, May 31, 2016 at 02:55:45PM +0530, Shashank Sharma wrote:
> Many GEN9 boards come with on-board lspcon cards.
> Fot these boards, VBT configuration should properly point out
> if a particular port contains lspcon device, so that driver can
> initialize it properly.
> 
> This patch adds a utility function, which checks the VBT flag
> for lspcon bit, and tells us if a port is configured to have a
> lspcon device or not.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  3 +++
>  drivers/gpu/drm/i915/intel_bios.c | 44 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e4c8e34..32ddde5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3622,6 +3622,9 @@ bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv, enum por
>  bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, enum port *port);
>  bool intel_bios_is_port_hpd_inverted(struct drm_i915_private *dev_priv,
>  				     enum port port);
> +bool intel_bios_is_lspcon_present(struct drm_i915_private *dev_priv,
> +				enum port port);
> +
>  
>  /* intel_opregion.c */
>  #ifdef CONFIG_ACPI
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 624e755..501b57f 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1715,3 +1715,47 @@ intel_bios_is_port_hpd_inverted(struct drm_i915_private *dev_priv,
>  
>  	return false;
>  }
> +
> +/**
> + * intel_bios_is_lspcon_present - if LSPCON is attached on %port
> + * @dev_priv:	i915 device instance
> + * @port:	port to check
> + *
> + * Return true if LSPCON is present on this port
> + */
> +bool
> +intel_bios_is_lspcon_present(struct drm_i915_private *dev_priv,
> +				enum port port)
> +{
> +	int i;
> +
> +	if (WARN_ON_ONCE(!IS_GEN9(dev_priv)))
> +		return false;
> +
> +	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> +		if (!dev_priv->vbt.child_dev[i].common.lspcon)
> +			continue;
> +
> +		switch (dev_priv->vbt.child_dev[i].common.dvo_port) {
> +		case DVO_PORT_DPA:
> +		case DVO_PORT_HDMIA:
> +			if (port == PORT_A)
> +				return true;
> +			break;
> +		case DVO_PORT_DPB:
> +		case DVO_PORT_HDMIB:
> +			if (port == PORT_B)
> +				return true;
> +			break;
> +		case DVO_PORT_DPC:
> +		case DVO_PORT_HDMIC:
> +			if (port == PORT_C)
> +				return true;
> +			break;

With just A/B/C this looks like it's expecting only BXT, but you're
checking for gen9 here. What's the deal?

> +		default:
> +			break;
> +		}
> +	}
> +
> +	return false;
> +}
> -- 
> 1.9.1

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

Re: [PATCH 5/5] drm/i915: Enable lspcon initialization

[Ville Syrjälä]

On Tue, May 31, 2016 at 02:55:46PM +0530, Shashank Sharma wrote:
> This patch adds initialization code for lspcon.
> What we are doing here is:
> 	- Check if lspcon is configured in VBT for this port
> 	- If lspcon is configured, configure it as DP port
> 	- Register additional HDMI functions to support LS
> 	  mode functionalities.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c  | 36 ++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h  |  4 ++++
>  drivers/gpu/drm/i915/intel_hdmi.c | 17 +++++++++++++++++
>  3 files changed, 55 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 811c829..2107c0d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2314,8 +2314,9 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_digital_port *intel_dig_port;
>  	struct intel_encoder *intel_encoder;
> +	struct intel_connector *intel_connector = NULL;
>  	struct drm_encoder *encoder;
> -	bool init_hdmi, init_dp;
> +	bool init_hdmi, init_dp, init_lspcon = false;
>  	int max_lanes;
>  
>  	if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) {
> @@ -2347,6 +2348,19 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>  	init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi ||
>  		     dev_priv->vbt.ddi_port_info[port].supports_hdmi);
>  	init_dp = dev_priv->vbt.ddi_port_info[port].supports_dp;
> +
> +	if (intel_bios_is_lspcon_present(dev_priv, port)) {
> +		/*
> +		* Lspcon device needs to be driven with DP connector
> +		* with special detection sequence. So make sure DP
> +		* is initialized before lspcon.
> +		*/
> +		init_dp = true;
> +		init_lspcon = true;
> +		init_hdmi = false;
> +		DRM_DEBUG_KMS("VBT says port %c has lspcon\n", port_name(port));
> +	}
> +
>  	if (!init_dp && !init_hdmi) {
>  		DRM_DEBUG_KMS("VBT says port %c is not DVI/HDMI/DP compatible, respect it\n",
>  			      port_name(port));
> @@ -2399,7 +2413,8 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>  	intel_encoder->cloneable = 0;
>  
>  	if (init_dp) {
> -		if (!intel_ddi_init_dp_connector(intel_dig_port))
> +		intel_connector = intel_ddi_init_dp_connector(intel_dig_port);
> +		if (!intel_connector)
>  			goto err;
>  
>  		intel_dig_port->hpd_pulse = intel_dp_hpd_pulse;
> @@ -2420,6 +2435,23 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>  			goto err;
>  	}
>  
> +	if (init_lspcon) {
> +		if (lspcon_init(intel_dig_port)) {
> +			if (intel_hdmi_init_minimum(intel_dig_port,
> +				intel_connector)) {
> +				DRM_ERROR("HDMI init for LSPCON failed\n");
> +				goto err;
> +			}
> +		} else {
> +			/*
> +			* LSPCON init faied, but DP init was success, so lets try to
> +			* drive as DP++ port.
> +			*/
> +			DRM_ERROR("LSPCON init failed on port %c\n",
> +				port_name(port));
> +		}
> +	}
> +
>  	return;
>  
>  err:
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index fa77886..402e656 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1454,6 +1454,10 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
>  enum drm_connector_status
>  intel_hdmi_detect(struct drm_connector *connector, bool force);
> +int intel_hdmi_init_minimum(struct intel_digital_port *intel_dig_port,
> +			       struct intel_connector *intel_connector);
> +
> +
>  
>  /* intel_lvds.c */
>  void intel_lvds_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 79184e2..54b0b46 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1931,6 +1931,23 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  	}
>  }
>  
> +int intel_hdmi_init_minimum(struct intel_digital_port *intel_dig_port,
> +			       struct intel_connector *intel_connector)
> +{
> +	struct intel_hdmi *intel_hdmi = &intel_dig_port->hdmi;
> +
> +	if (WARN(intel_dig_port->max_lanes < 4,
> +		"Not enough lanes (%d) for HDMI on port %c\n",
> +		intel_dig_port->max_lanes, port_name(intel_dig_port->port)))
> +		return -EINVAL;
> +
> +	intel_hdmi->write_infoframe = hsw_write_infoframe;
> +	intel_hdmi->set_infoframes = hsw_set_infoframes;
> +	intel_hdmi->infoframe_enabled = hsw_infoframe_enabled;
> +	intel_hdmi->attached_connector = intel_connector;
> +	return 0;
> +}

Hmm. I think I had patches somewhere to isolate the infoframe stuff from
the HDMI code a bit better. I think I even had patches to move the
infoframe hooks to the dig_port level. Should maybe dig those up.

> +
>  void intel_hdmi_init(struct drm_device *dev,
>  		     i915_reg_t hdmi_reg, enum port port)
>  {
> -- 
> 1.9.1

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

Re: [PATCH 3/5] drm/i915: lspcon detection

[Sharma, Shashank]

Regards
Shashank

On 5/31/2016 10:00 PM, Ville Syrjälä wrote:
> On Tue, May 31, 2016 at 02:55:44PM +0530, Shashank Sharma wrote:
>> lspcon is a tricky device to detect.
>> When in LS mode:
>> 	It should be detected as a HDMI device, by reading EDID, on
>> 	I2C over Aux chanel
>>
>> When in PCON mode:
>> 	It should be detected as a DP device by reading DPCD over the
>> 	Aux channel.
>>
>> This patch accommodates these specific requirements of lspcon detection
>> by adding appropriate changes in I915 drivers HDMI/DP detection sequence.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_ddi.c    | 14 ++++++++++++++
>>   drivers/gpu/drm/i915/intel_dp.c     | 12 ++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h    |  2 ++
>>   drivers/gpu/drm/i915/intel_hdmi.c   | 14 ++++++++++----
>>   drivers/gpu/drm/i915/intel_lspcon.c |  6 ++++++
>>   5 files changed, 44 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index c454744..811c829 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -2243,12 +2243,26 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>>   {
>>   	int type = encoder->type;
>>   	int port = intel_ddi_get_encoder_port(encoder);
>> +	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
>>
>>   	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
>>
>>   	if (port == PORT_A)
>>   		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>>
>> +	/*
>> +	* A digital port with active lspcon device, should be detected
>> +	* as HDMI when in LS mode and as DP when in PCON mode.
>> +	*/
>> +	if (is_lspcon_active(dig_port)) {
>> +		struct intel_lspcon *lspcon = &dig_port->lspcon;
>> +
>> +		if (lspcon->mode_of_op == DRM_LSPCON_MODE_LS)
>> +			type = INTEL_OUTPUT_HDMI;
>> +		else
>> +			type = INTEL_OUTPUT_DISPLAYPORT;
>> +	}
>
> Argh. I really wanted to kill this dual role DDI encoder mess (see [1])
>
> I realize that with LSPCON we probably don't want separate encoders for
> the two roles. Or maybe we do? At least we don't want two connectors,
> which probably means two encoders might become messy.
>
Yes, you are right. We neither need two encoders or two connectors. 
Actually if you closely observe, we have just attached once encoder, 
which is type DDI, and once connector, which is type DP.
> In any case I don't think we want to be frobbing around with the
> encoder->type anymore.
Actually this check is just to cover any corner case / future case where 
a lspcon device detection path reaches this path, in LS mode.
In LS mode of operation, there is no DPCD, and the aux channel 
read/write for DPCD register will not be valid, and we will not be able 
to detect the monitor. In this case, we should just get EDID over 
i2c_over_aux and detect.

Will it make more sense, if I add a separate lspcon_detect function, and 
call it from HDMI/DP detect, and in that function take this call of 
reading EDID or DPCD based on lspcon_mode ? Please suggest.
Maybe I need to rethink my approach to DDI
> encoders, and try to come up with something sane. For LSPCON at least
> we want to keep track of the current mode in pipe_config most likely.
> For LSPCON it's maybe a bit easier since the mode won't affect the
> DDC stuff and whanot, so from the probe side there is just one role
> except perhaps w.r.t. DPCD. For regular DDI stuff I'm still thinking
> two encoder might be the most sensible apporach since we have totally
> different paths for probe as well.
>
> Probably the biggest kink in all of this is hpd handling. What, if
> anything, should hpd_pulse do for LSPCON? And if something, should
> it only do it in PCON mode? How is HPD routed anyway with LSPCON?
>
Right now, from this patch series onwards, I am keeping lspcon always in 
PCON mode, so it will always be acting as a DP device, so any long HPD 
sequence which is valid for DP, will be valid for lspcon.

May be I should block short pulse handling if lspcon is active on the 
port ?
> [1] https://patchwork.freedesktop.org/series/1596/
>
>> +
>>   	if (type == INTEL_OUTPUT_HDMI)
>>   		return intel_hdmi_compute_config(encoder, pipe_config);
>>   	else
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index aa9c59e..39ce16e 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4276,6 +4276,18 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>>   	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>>   		      connector->base.id, connector->name);
>>
>> +	/*
>> +	* LSPCON is a DP->HDMI converter which should be detected as
>> +	* HDMI in LS mode, and DP in PCON mode. So if LSPCON is in LS
>> +	* mode, do not try to read DPCD, but detect as HDMI.
>> +	*/
>> +	if (is_lspcon_active(intel_dig_port)) {
>> +		struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
>> +
>> +		if (lspcon->mode_of_op == DRM_LSPCON_MODE_LS)
>> +			return lspcon_ls_mode_detect(connector, force);
>> +	}
>> +
>>   	if (intel_dp->is_mst) {
>>   		/* MST devices are disconnected from a monitor POV */
>>   		intel_dp_unset_edid(intel_dp);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 205a463..fa77886 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1452,6 +1452,8 @@ struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
>>   bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>   			       struct intel_crtc_state *pipe_config);
>>   void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
>> +enum drm_connector_status
>> +intel_hdmi_detect(struct drm_connector *connector, bool force);
>>
>>   /* intel_lvds.c */
>>   void intel_lvds_init(struct drm_device *dev);
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index 6b52c6a..79184e2 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -1444,15 +1444,21 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
>>   {
>>   	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>>   	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>> +	struct intel_digital_port *dig_port = hdmi_to_dig_port(intel_hdmi);
>> +	struct i2c_adapter *adapter;
>>   	struct edid *edid = NULL;
>>   	bool connected = false;
>>
>>   	if (force) {
>>   		intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
>>
>> -		edid = drm_get_edid(connector,
>> -				    intel_gmbus_get_adapter(dev_priv,
>> -				    intel_hdmi->ddc_bus));
>> +		if (is_lspcon_active(dig_port))
>> +			adapter = &dig_port->lspcon.aux->ddc;
>> +		else
>> +			adapter = intel_gmbus_get_adapter(dev_priv,
>> +				intel_hdmi->ddc_bus);
>> +
>> +		edid = drm_get_edid(connector, adapter);
>
> I'm not a fan of this. This is even taking the wrong power domain
> now, as does intel_hdmi_detect(). Probably LSPCON should just have
> a dedicated codepath for this stuff.
>
Got it, I will add-in a new function for lspcon's edid detection.
And thanks for pointing out this power domain stuff.
> If we ever have a board design with a DP++ port without a GMBUS
> connection, then we might have rethink things because then we'd have to
> use AUX also for HDMI DDC. But so far I'm not aware of such board
> designs existing, so might as well keep LSPCON separate.
>
Yes, and also, lspcon will be valid only for gen9/9.5 devices, until we 
get HDMI 2.0 native port. So I think we can call it ok to go.
>>
>>   		intel_hdmi_dp_dual_mode_detect(connector, edid != NULL);
>>
>> @@ -1479,7 +1485,7 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
>>   	return connected;
>>   }
>>
>> -static enum drm_connector_status
>> +enum drm_connector_status
>>   intel_hdmi_detect(struct drm_connector *connector, bool force)
>>   {
>>   	enum drm_connector_status status;
>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
>> index dd50491..75b5028 100644
>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>> @@ -37,6 +37,12 @@ bool is_lspcon_active(struct intel_digital_port *dig_port)
>>   	return dig_port->lspcon.active;
>>   }
>>
>> +enum drm_connector_status
>> +lspcon_ls_mode_detect(struct drm_connector *connector, bool force)
>> +{
>> +	return intel_hdmi_detect(connector, force);
>> +}
>> +
>>   enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
>>   {
>>   	enum drm_lspcon_mode current_mode;
>> --
>> 1.9.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Parse VBT data for lspcon

[Sharma, Shashank]

Regards
Shashank

On 5/31/2016 10:02 PM, Ville Syrjälä wrote:
> On Tue, May 31, 2016 at 02:55:45PM +0530, Shashank Sharma wrote:
>> Many GEN9 boards come with on-board lspcon cards.
>> Fot these boards, VBT configuration should properly point out
>> if a particular port contains lspcon device, so that driver can
>> initialize it properly.
>>
>> This patch adds a utility function, which checks the VBT flag
>> for lspcon bit, and tells us if a port is configured to have a
>> lspcon device or not.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h   |  3 +++
>>   drivers/gpu/drm/i915/intel_bios.c | 44 +++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index e4c8e34..32ddde5 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3622,6 +3622,9 @@ bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv, enum por
>>   bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, enum port *port);
>>   bool intel_bios_is_port_hpd_inverted(struct drm_i915_private *dev_priv,
>>   				     enum port port);
>> +bool intel_bios_is_lspcon_present(struct drm_i915_private *dev_priv,
>> +				enum port port);
>> +
>>
>>   /* intel_opregion.c */
>>   #ifdef CONFIG_ACPI
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 624e755..501b57f 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -1715,3 +1715,47 @@ intel_bios_is_port_hpd_inverted(struct drm_i915_private *dev_priv,
>>
>>   	return false;
>>   }
>> +
>> +/**
>> + * intel_bios_is_lspcon_present - if LSPCON is attached on %port
>> + * @dev_priv:	i915 device instance
>> + * @port:	port to check
>> + *
>> + * Return true if LSPCON is present on this port
>> + */
>> +bool
>> +intel_bios_is_lspcon_present(struct drm_i915_private *dev_priv,
>> +				enum port port)
>> +{
>> +	int i;
>> +
>> +	if (WARN_ON_ONCE(!IS_GEN9(dev_priv)))
>> +		return false;
>> +
>> +	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> +		if (!dev_priv->vbt.child_dev[i].common.lspcon)
>> +			continue;
>> +
>> +		switch (dev_priv->vbt.child_dev[i].common.dvo_port) {
>> +		case DVO_PORT_DPA:
>> +		case DVO_PORT_HDMIA:
>> +			if (port == PORT_A)
>> +				return true;
>> +			break;
>> +		case DVO_PORT_DPB:
>> +		case DVO_PORT_HDMIB:
>> +			if (port == PORT_B)
>> +				return true;
>> +			break;
>> +		case DVO_PORT_DPC:
>> +		case DVO_PORT_HDMIC:
>> +			if (port == PORT_C)
>> +				return true;
>> +			break;
>
> With just A/B/C this looks like it's expecting only BXT, but you're
> checking for gen9 here. What's the deal?
>
Guilty, just copy-pasted from above function :), will add other ports 
too. Its supposed to work with Gen9.
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +
>> +	return false;
>> +}
>> --
>> 1.9.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Enable lspcon initialization

[Sharma, Shashank]

Regards
Shashank

On 5/31/2016 10:04 PM, Ville Syrjälä wrote:
> On Tue, May 31, 2016 at 02:55:46PM +0530, Shashank Sharma wrote:
>> This patch adds initialization code for lspcon.
>> What we are doing here is:
>> 	- Check if lspcon is configured in VBT for this port
>> 	- If lspcon is configured, configure it as DP port
>> 	- Register additional HDMI functions to support LS
>> 	  mode functionalities.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_ddi.c  | 36 ++++++++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/i915/intel_drv.h  |  4 ++++
>>   drivers/gpu/drm/i915/intel_hdmi.c | 17 +++++++++++++++++
>>   3 files changed, 55 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 811c829..2107c0d 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -2314,8 +2314,9 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>   	struct intel_digital_port *intel_dig_port;
>>   	struct intel_encoder *intel_encoder;
>> +	struct intel_connector *intel_connector = NULL;
>>   	struct drm_encoder *encoder;
>> -	bool init_hdmi, init_dp;
>> +	bool init_hdmi, init_dp, init_lspcon = false;
>>   	int max_lanes;
>>
>>   	if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) {
>> @@ -2347,6 +2348,19 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>>   	init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi ||
>>   		     dev_priv->vbt.ddi_port_info[port].supports_hdmi);
>>   	init_dp = dev_priv->vbt.ddi_port_info[port].supports_dp;
>> +
>> +	if (intel_bios_is_lspcon_present(dev_priv, port)) {
>> +		/*
>> +		* Lspcon device needs to be driven with DP connector
>> +		* with special detection sequence. So make sure DP
>> +		* is initialized before lspcon.
>> +		*/
>> +		init_dp = true;
>> +		init_lspcon = true;
>> +		init_hdmi = false;
>> +		DRM_DEBUG_KMS("VBT says port %c has lspcon\n", port_name(port));
>> +	}
>> +
>>   	if (!init_dp && !init_hdmi) {
>>   		DRM_DEBUG_KMS("VBT says port %c is not DVI/HDMI/DP compatible, respect it\n",
>>   			      port_name(port));
>> @@ -2399,7 +2413,8 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>>   	intel_encoder->cloneable = 0;
>>
>>   	if (init_dp) {
>> -		if (!intel_ddi_init_dp_connector(intel_dig_port))
>> +		intel_connector = intel_ddi_init_dp_connector(intel_dig_port);
>> +		if (!intel_connector)
>>   			goto err;
>>
>>   		intel_dig_port->hpd_pulse = intel_dp_hpd_pulse;
>> @@ -2420,6 +2435,23 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>>   			goto err;
>>   	}
>>
>> +	if (init_lspcon) {
>> +		if (lspcon_init(intel_dig_port)) {
>> +			if (intel_hdmi_init_minimum(intel_dig_port,
>> +				intel_connector)) {
>> +				DRM_ERROR("HDMI init for LSPCON failed\n");
>> +				goto err;
>> +			}
>> +		} else {
>> +			/*
>> +			* LSPCON init faied, but DP init was success, so lets try to
>> +			* drive as DP++ port.
>> +			*/
>> +			DRM_ERROR("LSPCON init failed on port %c\n",
>> +				port_name(port));
>> +		}
>> +	}
>> +
>>   	return;
>>
>>   err:
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index fa77886..402e656 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1454,6 +1454,10 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>   void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
>>   enum drm_connector_status
>>   intel_hdmi_detect(struct drm_connector *connector, bool force);
>> +int intel_hdmi_init_minimum(struct intel_digital_port *intel_dig_port,
>> +			       struct intel_connector *intel_connector);
>> +
>> +
>>
>>   /* intel_lvds.c */
>>   void intel_lvds_init(struct drm_device *dev);
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index 79184e2..54b0b46 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -1931,6 +1931,23 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>>   	}
>>   }
>>
>> +int intel_hdmi_init_minimum(struct intel_digital_port *intel_dig_port,
>> +			       struct intel_connector *intel_connector)
>> +{
>> +	struct intel_hdmi *intel_hdmi = &intel_dig_port->hdmi;
>> +
>> +	if (WARN(intel_dig_port->max_lanes < 4,
>> +		"Not enough lanes (%d) for HDMI on port %c\n",
>> +		intel_dig_port->max_lanes, port_name(intel_dig_port->port)))
>> +		return -EINVAL;
>> +
>> +	intel_hdmi->write_infoframe = hsw_write_infoframe;
>> +	intel_hdmi->set_infoframes = hsw_set_infoframes;
>> +	intel_hdmi->infoframe_enabled = hsw_infoframe_enabled;
>> +	intel_hdmi->attached_connector = intel_connector;
>> +	return 0;
>> +}
>
> Hmm. I think I had patches somewhere to isolate the infoframe stuff from
> the HDMI code a bit better. I think I even had patches to move the
> infoframe hooks to the dig_port level. Should maybe dig those up.
Sure, that will be more systematic, and a better design.
>
>> +
>>   void intel_hdmi_init(struct drm_device *dev,
>>   		     i915_reg_t hdmi_reg, enum port port)
>>   {
>> --
>> 1.9.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx