[PATCH v3 0/4] Enable lspcon support for GEN9 devices

[PATCH v3 0/4] Enable lspcon support for GEN9 devices

[Shashank Sharma]

LSPCON is essentially a dp++->hdmi adapter 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 an
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

V2: Worked on review comments from Ville
- In general, Ville suggested not to use the dual personality of
  DDI to drive lspcon, so this patch set drives it just as DP++ display.
  There is no separate detection for lspcon (hpd_pulse is good enough), and
  its being driven as a DP display with special initialization and EDID
  read sequence. To be able to do this, we driving lspcon in PCON mode only,
  where it can serve both HDMI1.3/HDMI1.4 sinks as well as 4k@60 capable
  HDMI 2.0 sinks. So compared to previous series, there is one patch less,
  as we have dropped lspcon detection patch.


V3: Addressed review comments from Rodrigo
    Details available in respective patch.

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

 drivers/gpu/drm/drm_dp_dual_mode_helper.c | 103 +++++++++++++++++++++
 drivers/gpu/drm/i915/Makefile             |   1 +
 drivers/gpu/drm/i915/i915_drv.h           |   5 ++
 drivers/gpu/drm/i915/intel_bios.c         |  49 ++++++++++
 drivers/gpu/drm/i915/intel_ddi.c          |  29 +++++-
 drivers/gpu/drm/i915/intel_drv.h          |  13 ++-
 drivers/gpu/drm/i915/intel_lspcon.c       | 145 ++++++++++++++++++++++++++++++
 include/drm/drm_dp_dual_mode_helper.h     |  26 ++++++
 8 files changed, 369 insertions(+), 2 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 v3 1/4] drm: Helper for lspcon in drm_dp_dual_mode

[Shashank Sharma]

This patch adds lspcon support in dp_dual_mode helper.
lspcon is essentially a dp->hdmi dongle with dual personality.

LS mode: It works as a passive dongle, by level shifting DP++
signals to HDMI signals, in LS mode.
PCON mode: It works as a protocol converter active dongle
in pcon mode, by converting DP++ outputs to HDMI 2.0 outputs.

This patch adds support for lspcon detection and mode set
switch operations, as a dp dual mode dongle.

v2: Addressed review comments from Ville
- add adaptor id for lspcon devices (0x08), use it to identify lspcon
- change function names
  old: drm_lspcon_get_current_mode/drm_lspcon_change_mode
  new: drm_lspcon_get_mode/drm_lspcon_set_mode
- change drm_lspcon_get_mode type to int, to match
  drm_dp_dual_mode_get_tmds_output
- change 'err' to 'ret' to match the rest of the functions
- remove pointless typecasting during call to dual_mode_read
- fix the but while setting value of data, while writing lspcon mode
- fix indentation
- change mdelay(10) -> msleep(10)
- return ETIMEDOUT instead of EFAULT, when lspcon mode change times out
- Add an empty line to separate std regs macros and lspcon regs macros
  Indent bit definition

v3: Addressed review comments from Rodrigo
- change macro name from DP_DUAL_MODE_TYPE_LSPCON to
  DP_DUAL_MODE_TYPE_HAS_DPCD for better readability
- change macro name from DP_DUAL_MODE_LSPCON_MODE_PCON to
  DP_DUAL_MODE_LSPCON_MODE_PCON for better readability
- add comment for MCA specific offsets like 0x40 and 0x41
- remove DP_DUAL_MODE_REV_TYPE2 check while checking lspcon adapter id

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_dp_dual_mode_helper.c | 103 ++++++++++++++++++++++++++++++
 include/drm/drm_dp_dual_mode_helper.h     |  26 ++++++++
 2 files changed, 129 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..4f89e0b 100644
--- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
+++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
@@ -148,6 +148,14 @@ 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 == (DP_DUAL_MODE_TYPE_TYPE2 |
+			DP_DUAL_MODE_TYPE_HAS_DPCD));
+}
+
 /**
  * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor
  * @adapter: I2C adapter for the DDC bus
@@ -203,6 +211,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 +374,96 @@ 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
+ * @current_mode: out vaiable, current lspcon mode of operation
+ *
+ * Returns:
+ * 0 on success, sets the current_mode value to appropriate mode
+ * -error on failure
+ */
+int drm_lspcon_get_mode(struct i2c_adapter *adapter,
+	enum drm_lspcon_mode *current_mode)
+{
+	u8 data;
+	int ret = 0;
+
+	if (!current_mode) {
+		DRM_ERROR("NULL input\n");
+		return -EINVAL;
+	}
+
+	/* Read Status: i2c over aux */
+	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
+			(void *)&data, sizeof(data));
+	if (ret < 0) {
+		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
+		return -EFAULT;
+	}
+
+	if (data & DP_DUAL_MODE_LSPCON_MODE_PCON)
+		*current_mode = DRM_LSPCON_MODE_PCON;
+	else
+		*current_mode = DRM_LSPCON_MODE_LS;
+	return 0;
+}
+EXPORT_SYMBOL(drm_lspcon_get_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_set_mode(struct i2c_adapter *adapter,
+	enum drm_lspcon_mode reqd_mode)
+{
+	u8 data = 0;
+	int ret;
+	int time_out = 200;
+	enum drm_lspcon_mode changed_mode;
+
+	if (reqd_mode == DRM_LSPCON_MODE_PCON)
+		data = DP_DUAL_MODE_LSPCON_MODE_PCON;
+
+	/* Change mode */
+	ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_LSPCON_MODE_CHANGE,
+			&data, sizeof(data));
+	if (ret < 0) {
+		DRM_ERROR("LSPCON mode change failed\n");
+		return ret;
+	}
+
+	/*
+	  * 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.
+	  */
+	do {
+		ret = drm_lspcon_get_mode(adapter, &changed_mode);
+		if (ret) {
+			DRM_ERROR("cant confirm LSPCON mode change\n");
+			return ret;
+		} else {
+			if (changed_mode != reqd_mode) {
+				msleep(10);
+				time_out -= 10;
+			} else {
+				DRM_DEBUG_KMS("LSPCON mode changed to %s\n",
+					reqd_mode == DRM_LSPCON_MODE_LS ?
+						"LS" : "PCON");
+				return 0;
+			}
+		}
+	} while (time_out);
+
+	DRM_ERROR("LSPCON mode change timed out\n");
+	return -ETIMEDOUT;
+}
+EXPORT_SYMBOL(drm_lspcon_set_mode);
diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h
index e8a9dfd..a3c1d03 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
@@ -40,6 +41,7 @@
 #define  DP_DUAL_MODE_REV_TYPE2 0x00
 #define  DP_DUAL_MODE_TYPE_MASK 0xf0
 #define  DP_DUAL_MODE_TYPE_TYPE2 0xa0
+#define  DP_DUAL_MODE_TYPE_HAS_DPCD 0x08
 #define DP_DUAL_MODE_IEEE_OUI 0x11 /* 11-13*/
 #define  DP_DUAL_IEEE_OUI_LEN 3
 #define DP_DUAL_DEVICE_ID 0x14 /* 14-19 */
@@ -55,6 +57,11 @@
 #define  DP_DUAL_MODE_CEC_ENABLE 0x01
 #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22
 
+/* LSPCON specific registers, defined by MCA */
+#define DP_DUAL_MODE_LSPCON_MODE_CHANGE		0x40
+#define DP_DUAL_MODE_LSPCON_CURRENT_MODE		0x41
+#define  DP_DUAL_MODE_LSPCON_MODE_PCON			0x1
+
 struct i2c_adapter;
 
 ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
@@ -63,6 +70,19 @@ 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_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 +90,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 +99,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 +111,8 @@ 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_lspcon_get_mode(struct i2c_adapter *adapter,
+	enum drm_lspcon_mode *current_mode);
+int drm_lspcon_set_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 v3 2/4] drm/i915: Add lspcon support for I915 driver

[Shashank Sharma]

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.

V2: addressed ville's review comments
- Clean the leftover macros from previous patch set

V3: Rebase

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/Makefile       |   1 +
 drivers/gpu/drm/i915/intel_drv.h    |  13 +++-
 drivers/gpu/drm/i915/intel_lspcon.c | 145 ++++++++++++++++++++++++++++++++++++
 3 files changed, 158 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 618293c..64cd373 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -95,6 +95,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 e6a24d2..e6982cf 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -922,12 +922,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;
@@ -1478,7 +1485,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);
 struct intel_encoder *intel_get_lvds_encoder(struct drm_device *dev);
@@ -1779,4 +1785,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..fdeff71
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -0,0 +1,145 @@
+/*
+ * 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"
+
+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 = DRM_LSPCON_MODE_INVALID;
+	struct i2c_adapter *adapter = &lspcon->aux->ddc;
+
+	if (drm_lspcon_get_mode(adapter, &current_mode))
+		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;
+
+	err = drm_lspcon_get_mode(adapter, &current_mode);
+	if (err) {
+		DRM_ERROR("Error reading LSPCON mode\n");
+		return err;
+	}
+
+	if (current_mode == mode && !force) {
+		DRM_DEBUG_KMS("Current mode = desired LSPCON mode\n");
+		return 0;
+	}
+
+	err = drm_lspcon_set_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)
+{
+
+	/* 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 */
+	lspcon->mode_of_op = lspcon_get_current_mode(lspcon);
+	lspcon->active = true;
+	return true;
+}
+
+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 v3 3/4] 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.

V2: Fixed review comments from Ville
- Do not forget PORT_D while checking lspcon for GEN9

V3: Addressed review comments from Rodrigo
- Create a HAS_LSPCON() macro for better use case handling.
- Do not dump warnings for non-gen-9 platforms, it will be noise.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 594ddbb..91e1dd1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2896,6 +2896,8 @@ struct drm_i915_cmd_table {
 #define HAS_GMCH_DISPLAY(dev) (INTEL_INFO(dev)->gen < 5 || \
 			       IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
 
+#define HAS_LSPCON(dev) (IS_GEN9(dev))
+
 /* DPF == dynamic parity feature */
 #define HAS_L3_DPF(dev) (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
 #define NUM_L3_SLICES(dev) (IS_HSW_GT3(dev) ? 2 : HAS_L3_DPF(dev))
@@ -3706,6 +3708,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 da5ed4a..121df10 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -1759,3 +1759,52 @@ 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 (!HAS_LSPCON(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;
+		case DVO_PORT_DPD:
+		case DVO_PORT_HDMID:
+			if (port == PORT_D)
+				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 v3 4/4] 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, initialize it and configure it
          as DP port.

V2: Addressed Ville's review comments:
- Not adding AVI IF functions for LSPCON display now.
  This part will be added once the dig_port level AVI-IF series
  gets merged.

V3: Rebase

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

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index d6efe8b..3d115a7 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2326,7 +2326,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 	struct intel_digital_port *intel_dig_port;
 	struct intel_encoder *intel_encoder;
 	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) {
@@ -2358,6 +2358,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));
@@ -2433,6 +2446,20 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 			goto err;
 	}
 
+	if (init_lspcon) {
+		if (lspcon_init(intel_dig_port))
+			/* TODO: handle hdmi info frame part */
+			DRM_DEBUG_KMS("LSPCON init success on port %c\n",
+				port_name(port));
+		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:
-- 
1.9.1

_______________________________________________
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 (rev3)

[Patchwork]

== Series Details ==

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

== Summary ==

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

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                dmesg-fail -> PASS       (fi-skl-i7-6700k)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                skip       -> DMESG-WARN (ro-bdw-i7-5557U)

fi-kbl-qkkr      total:234  pass:163  dwarn:28  dfail:1   fail:2   skip:40 
fi-skl-i5-6260u  total:234  pass:205  dwarn:0   dfail:1   fail:2   skip:26 
fi-skl-i7-6700k  total:234  pass:192  dwarn:0   dfail:0   fail:2   skip:40 
fi-snb-i7-2600   total:234  pass:178  dwarn:0   dfail:0   fail:2   skip:54 
ro-bdw-i5-5250u  total:229  pass:204  dwarn:1   dfail:1   fail:0   skip:23 
ro-bdw-i7-5557U  total:229  pass:204  dwarn:2   dfail:1   fail:0   skip:22 
ro-bdw-i7-5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38 
ro-bsw-n3050     total:229  pass:177  dwarn:0   dfail:1   fail:2   skip:49 
ro-byt-n2820     total:229  pass:181  dwarn:0   dfail:1   fail:2   skip:45 
ro-hsw-i3-4010u  total:229  pass:197  dwarn:0   dfail:1   fail:0   skip:31 
ro-hsw-i7-4770r  total:229  pass:197  dwarn:0   dfail:1   fail:0   skip:31 
ro-ilk-i7-620lm  total:229  pass:157  dwarn:0   dfail:1   fail:1   skip:70 
ro-ilk1-i5-650   total:224  pass:157  dwarn:0   dfail:1   fail:1   skip:65 
ro-ivb-i7-3770   total:229  pass:188  dwarn:0   dfail:1   fail:0   skip:40 
ro-skl3-i5-6260u total:229  pass:208  dwarn:1   dfail:1   fail:0   skip:19 
ro-snb-i7-2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1419/

2a6e307 drm-intel-nightly: 2016y-07m-05d-11h-50m-09s UTC integration manifest
63a8889 drm/i915: Enable lspcon initialization
57d9355 drm/i915: Parse VBT data for lspcon
a3732f1 drm/i915: Add lspcon support for I915 driver
9c08ce8 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 v3 3/4] drm/i915: Parse VBT data for lspcon

[Vivi, Rodrigo]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

On Tue, 2016-07-05 at 18:35 +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.
> 
> V2: Fixed review comments from Ville
> - Do not forget PORT_D while checking lspcon for GEN9
> 
> V3: Addressed review comments from Rodrigo
> - Create a HAS_LSPCON() macro for better use case handling.
> - Do not dump warnings for non-gen-9 platforms, it will be noise.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  5 ++++
>  drivers/gpu/drm/i915/intel_bios.c | 49
> +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 594ddbb..91e1dd1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2896,6 +2896,8 @@ struct drm_i915_cmd_table {
>  #define HAS_GMCH_DISPLAY(dev) (INTEL_INFO(dev)->gen < 5 || \
>  			       IS_VALLEYVIEW(dev) ||
> IS_CHERRYVIEW(dev))
>  
> +#define HAS_LSPCON(dev) (IS_GEN9(dev))
> +
>  /* DPF == dynamic parity feature */
>  #define HAS_L3_DPF(dev) (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
>  #define NUM_L3_SLICES(dev) (IS_HSW_GT3(dev) ? 2 : HAS_L3_DPF(dev))
> @@ -3706,6 +3708,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 da5ed4a..121df10 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1759,3 +1759,52 @@ 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 (!HAS_LSPCON(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;
> +		case DVO_PORT_DPD:
> +		case DVO_PORT_HDMID:
> +			if (port == PORT_D)
> +				return true;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return false;
> +}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 4/4] drm/i915: Enable lspcon initialization

[Vivi, Rodrigo]

If this works even without setting the info frame on this code and that
is on progress on other series I believe we can move forward so in this
case fell free to use:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

On Tue, 2016-07-05 at 18:35 +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, initialize it and configure it
>           as DP port.
> 
> V2: Addressed Ville's review comments:
> - Not adding AVI IF functions for LSPCON display now.
>   This part will be added once the dig_port level AVI-IF series
>   gets merged.
> 
> V3: Rebase
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index d6efe8b..3d115a7 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2326,7 +2326,7 @@ void intel_ddi_init(struct drm_device *dev,
> enum port port)
>  	struct intel_digital_port *intel_dig_port;
>  	struct intel_encoder *intel_encoder;
>  	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) {
> @@ -2358,6 +2358,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));
> @@ -2433,6 +2446,20 @@ void intel_ddi_init(struct drm_device *dev,
> enum port port)
>  			goto err;
>  	}
>  
> +	if (init_lspcon) {
> +		if (lspcon_init(intel_dig_port))
> +			/* TODO: handle hdmi info frame part */
> +			DRM_DEBUG_KMS("LSPCON init success on port
> %c\n",
> +				port_name(port));
> +		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:
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 4/4] drm/i915: Enable lspcon initialization

[Sharma, Shashank]

Thanks. I forgot to mention, this code works without AVI IF too. 

Regards
Shashank 
-----Original Message-----
From: Vivi, Rodrigo 
Sent: Wednesday, July 6, 2016 11:04 PM
To: Sharma, Shashank <shashank.sharma@intel.com>; intel-gfx@lists.freedesktop.org; rodrigo.vivi@gmail.com
Cc: ville.syrjala@linux.intel.com; Zanoni, Paulo R <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH v3 4/4] drm/i915: Enable lspcon initialization

If this works even without setting the info frame on this code and that is on progress on other series I believe we can move forward so in this case fell free to use:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

On Tue, 2016-07-05 at 18:35 +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, initialize it and configure it
>           as DP port.
> 
> V2: Addressed Ville's review comments:
> - Not adding AVI IF functions for LSPCON display now.
>   This part will be added once the dig_port level AVI-IF series
>   gets merged.
> 
> V3: Rebase
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index d6efe8b..3d115a7 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2326,7 +2326,7 @@ void intel_ddi_init(struct drm_device *dev, enum 
> port port)
>  	struct intel_digital_port *intel_dig_port;
>  	struct intel_encoder *intel_encoder;
>  	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) { @@ -2358,6 
> +2358,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));
> @@ -2433,6 +2446,20 @@ void intel_ddi_init(struct drm_device *dev, 
> enum port port)
>  			goto err;
>  	}
>  
> +	if (init_lspcon) {
> +		if (lspcon_init(intel_dig_port))
> +			/* TODO: handle hdmi info frame part */
> +			DRM_DEBUG_KMS("LSPCON init success on port
> %c\n",
> +				port_name(port));
> +		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:
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/4] drm: Helper for lspcon in drm_dp_dual_mode

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 9971 bytes --]

Ops, I had forgot to reply to this one although I have reviewed!

Feel free to use
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

On Tuesday, July 5, 2016, Shashank Sharma <shashank.sharma@intel.com> wrote:

> This patch adds lspcon support in dp_dual_mode helper.
> lspcon is essentially a dp->hdmi dongle with dual personality.
>
> LS mode: It works as a passive dongle, by level shifting DP++
> signals to HDMI signals, in LS mode.
> PCON mode: It works as a protocol converter active dongle
> in pcon mode, by converting DP++ outputs to HDMI 2.0 outputs.
>
> This patch adds support for lspcon detection and mode set
> switch operations, as a dp dual mode dongle.
>
> v2: Addressed review comments from Ville
> - add adaptor id for lspcon devices (0x08), use it to identify lspcon
> - change function names
>   old: drm_lspcon_get_current_mode/drm_lspcon_change_mode
>   new: drm_lspcon_get_mode/drm_lspcon_set_mode
> - change drm_lspcon_get_mode type to int, to match
>   drm_dp_dual_mode_get_tmds_output
> - change 'err' to 'ret' to match the rest of the functions
> - remove pointless typecasting during call to dual_mode_read
> - fix the but while setting value of data, while writing lspcon mode
> - fix indentation
> - change mdelay(10) -> msleep(10)
> - return ETIMEDOUT instead of EFAULT, when lspcon mode change times out
> - Add an empty line to separate std regs macros and lspcon regs macros
>   Indent bit definition
>
> v3: Addressed review comments from Rodrigo
> - change macro name from DP_DUAL_MODE_TYPE_LSPCON to
>   DP_DUAL_MODE_TYPE_HAS_DPCD for better readability
> - change macro name from DP_DUAL_MODE_LSPCON_MODE_PCON to
>   DP_DUAL_MODE_LSPCON_MODE_PCON for better readability
> - add comment for MCA specific offsets like 0x40 and 0x41
> - remove DP_DUAL_MODE_REV_TYPE2 check while checking lspcon adapter id
>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com <javascript:;>>
> ---
>  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 103
> ++++++++++++++++++++++++++++++
>  include/drm/drm_dp_dual_mode_helper.h     |  26 ++++++++
>  2 files changed, 129 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..4f89e0b 100644
> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> @@ -148,6 +148,14 @@ 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 == (DP_DUAL_MODE_TYPE_TYPE2 |
> +                       DP_DUAL_MODE_TYPE_HAS_DPCD));
> +}
> +
>  /**
>   * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor
>   * @adapter: I2C adapter for the DDC bus
> @@ -203,6 +211,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 +374,96 @@ 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
> + * @current_mode: out vaiable, current lspcon mode of operation
> + *
> + * Returns:
> + * 0 on success, sets the current_mode value to appropriate mode
> + * -error on failure
> + */
> +int drm_lspcon_get_mode(struct i2c_adapter *adapter,
> +       enum drm_lspcon_mode *current_mode)
> +{
> +       u8 data;
> +       int ret = 0;
> +
> +       if (!current_mode) {
> +               DRM_ERROR("NULL input\n");
> +               return -EINVAL;
> +       }
> +
> +       /* Read Status: i2c over aux */
> +       ret = drm_dp_dual_mode_read(adapter,
> DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> +                       (void *)&data, sizeof(data));
> +       if (ret < 0) {
> +               DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
> +               return -EFAULT;
> +       }
> +
> +       if (data & DP_DUAL_MODE_LSPCON_MODE_PCON)
> +               *current_mode = DRM_LSPCON_MODE_PCON;
> +       else
> +               *current_mode = DRM_LSPCON_MODE_LS;
> +       return 0;
> +}
> +EXPORT_SYMBOL(drm_lspcon_get_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_set_mode(struct i2c_adapter *adapter,
> +       enum drm_lspcon_mode reqd_mode)
> +{
> +       u8 data = 0;
> +       int ret;
> +       int time_out = 200;
> +       enum drm_lspcon_mode changed_mode;
> +
> +       if (reqd_mode == DRM_LSPCON_MODE_PCON)
> +               data = DP_DUAL_MODE_LSPCON_MODE_PCON;
> +
> +       /* Change mode */
> +       ret = drm_dp_dual_mode_write(adapter,
> DP_DUAL_MODE_LSPCON_MODE_CHANGE,
> +                       &data, sizeof(data));
> +       if (ret < 0) {
> +               DRM_ERROR("LSPCON mode change failed\n");
> +               return ret;
> +       }
> +
> +       /*
> +         * 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.
> +         */
> +       do {
> +               ret = drm_lspcon_get_mode(adapter, &changed_mode);
> +               if (ret) {
> +                       DRM_ERROR("cant confirm LSPCON mode change\n");
> +                       return ret;
> +               } else {
> +                       if (changed_mode != reqd_mode) {
> +                               msleep(10);
> +                               time_out -= 10;
> +                       } else {
> +                               DRM_DEBUG_KMS("LSPCON mode changed to
> %s\n",
> +                                       reqd_mode == DRM_LSPCON_MODE_LS ?
> +                                               "LS" : "PCON");
> +                               return 0;
> +                       }
> +               }
> +       } while (time_out);
> +
> +       DRM_ERROR("LSPCON mode change timed out\n");
> +       return -ETIMEDOUT;
> +}
> +EXPORT_SYMBOL(drm_lspcon_set_mode);
> diff --git a/include/drm/drm_dp_dual_mode_helper.h
> b/include/drm/drm_dp_dual_mode_helper.h
> index e8a9dfd..a3c1d03 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
> @@ -40,6 +41,7 @@
>  #define  DP_DUAL_MODE_REV_TYPE2 0x00
>  #define  DP_DUAL_MODE_TYPE_MASK 0xf0
>  #define  DP_DUAL_MODE_TYPE_TYPE2 0xa0
> +#define  DP_DUAL_MODE_TYPE_HAS_DPCD 0x08
>  #define DP_DUAL_MODE_IEEE_OUI 0x11 /* 11-13*/
>  #define  DP_DUAL_IEEE_OUI_LEN 3
>  #define DP_DUAL_DEVICE_ID 0x14 /* 14-19 */
> @@ -55,6 +57,11 @@
>  #define  DP_DUAL_MODE_CEC_ENABLE 0x01
>  #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22
>
> +/* LSPCON specific registers, defined by MCA */
> +#define DP_DUAL_MODE_LSPCON_MODE_CHANGE                0x40
> +#define DP_DUAL_MODE_LSPCON_CURRENT_MODE               0x41
> +#define  DP_DUAL_MODE_LSPCON_MODE_PCON                 0x1
> +
>  struct i2c_adapter;
>
>  ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
> @@ -63,6 +70,19 @@ 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_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 +90,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 +99,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 +111,8 @@ 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_lspcon_get_mode(struct i2c_adapter *adapter,
> +       enum drm_lspcon_mode *current_mode);
> +int drm_lspcon_set_mode(struct i2c_adapter *adapter,
> +               enum drm_lspcon_mode reqd_mode);
>  #endif
> --
> 1.9.1
>
>

-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 11991 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v3 0/4] Enable lspcon support for GEN9 devices

[Ville Syrjälä]

On Tue, Jul 05, 2016 at 06:35:46PM +0530, Shashank Sharma wrote:
> LSPCON is essentially a dp++->hdmi adapter 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 an
> 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
> 
> V2: Worked on review comments from Ville
> - In general, Ville suggested not to use the dual personality of
>   DDI to drive lspcon, so this patch set drives it just as DP++ display.
>   There is no separate detection for lspcon (hpd_pulse is good enough), and
>   its being driven as a DP display with special initialization and EDID
>   read sequence. To be able to do this, we driving lspcon in PCON mode only,
>   where it can serve both HDMI1.3/HDMI1.4 sinks as well as 4k@60 capable
>   HDMI 2.0 sinks. So compared to previous series, there is one patch less,
>   as we have dropped lspcon detection patch.
> 
> 
> V3: Addressed review comments from Rodrigo
>     Details available in respective patch.
> 
> Shashank Sharma (4):
>   drm: Helper for lspcon in drm_dp_dual_mode
>   drm/i915: Add lspcon support for I915 driver
>   drm/i915: Parse VBT data for lspcon
>   drm/i915: Enable lspcon initialization

This thing seems to be missing a lot of stuff:
- where is the EDID etc. handling?
- based on my DDI output_types stuff, I was expecting to see an
  lspcon .compute_output_type() hook that returns whether we need
  LS or PCON mode. Can't see anything of the sort here.

Also the main patch [1] of my DDI output_types series still didn't
get reviewed...

[1] https://patchwork.freedesktop.org/patch/94581/

> 
>  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 103 +++++++++++++++++++++
>  drivers/gpu/drm/i915/Makefile             |   1 +
>  drivers/gpu/drm/i915/i915_drv.h           |   5 ++
>  drivers/gpu/drm/i915/intel_bios.c         |  49 ++++++++++
>  drivers/gpu/drm/i915/intel_ddi.c          |  29 +++++-
>  drivers/gpu/drm/i915/intel_drv.h          |  13 ++-
>  drivers/gpu/drm/i915/intel_lspcon.c       | 145 ++++++++++++++++++++++++++++++
>  include/drm/drm_dp_dual_mode_helper.h     |  26 ++++++
>  8 files changed, 369 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_lspcon.c
> 
> -- 
> 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 v3 0/4] Enable lspcon support for GEN9 devices

[Sharma, Shashank]

Hi Ville.

> This thing seems to be missing a lot of stuff:
> - where is the EDID etc. handling?
None required for LSPCON as of now. The 4k@60 pixel clock support was already added with SKL, so we can see
a HDMI 4k@60 output already on PCON mode, when connected to a 4k@60 monitor.

I have started publishing a separate patch series to support HDMI 2.0, started with the new aspect ratio patches here. I was hoping to proceed with EDID changes, once we get these in, else the patch series gets huge and reviewers find it difficult to review. 
https://patchwork.freedesktop.org/patch/78308/ 
 
Plan is to publish in this sequence:
- Add new aspect ratios
- Add new 4k modes in DRM EDID
- Add Scrambling support
- Add SCDC parsing
- Add YUV 420 CEA-861-F block parsing
and so on. 

> - based on my DDI output_types stuff, I was expecting to see an
>  lspcon .compute_output_type() hook that returns whether we need
 >  LS or PCON mode. Can't see anything of the sort here.
As I explained in the commit message/cover letter LSPCON is no more working selectively on LS/PCON mode.
We are driving it in always PCON mode, and driver always drives a DP.  So we are driving a DP with separate detection sequence. 

> Also the main patch [1] of my DDI output_types series still didn't get reviewed...
Ok, but that should not be a blocker for this series, should it ? 

Regards
Shashank
-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Wednesday, July 20, 2016 6:39 PM
To: Sharma, Shashank <shashank.sharma@intel.com>
Cc: intel-gfx@lists.freedesktop.org; rodrigo.vivi@gmail.com; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Zanoni, Paulo R <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH v3 0/4] Enable lspcon support for GEN9 devices

On Tue, Jul 05, 2016 at 06:35:46PM +0530, Shashank Sharma wrote:
> LSPCON is essentially a dp++->hdmi adapter 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 an 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
> 
> V2: Worked on review comments from Ville
> - In general, Ville suggested not to use the dual personality of
>   DDI to drive lspcon, so this patch set drives it just as DP++ display.
>   There is no separate detection for lspcon (hpd_pulse is good enough), and
>   its being driven as a DP display with special initialization and EDID
>   read sequence. To be able to do this, we driving lspcon in PCON mode only,
>   where it can serve both HDMI1.3/HDMI1.4 sinks as well as 4k@60 capable
>   HDMI 2.0 sinks. So compared to previous series, there is one patch less,
>   as we have dropped lspcon detection patch.
> 
> 
> V3: Addressed review comments from Rodrigo
>     Details available in respective patch.
> 
> Shashank Sharma (4):
>   drm: Helper for lspcon in drm_dp_dual_mode
>   drm/i915: Add lspcon support for I915 driver
>   drm/i915: Parse VBT data for lspcon
>   drm/i915: Enable lspcon initialization

This thing seems to be missing a lot of stuff:
- where is the EDID etc. handling?
None required for LSPCON as of now. The 4k@60 pixel clock support was already added with SKL
- based on my DDI output_types stuff, I was expecting to see an
  lspcon .compute_output_type() hook that returns whether we need
  LS or PCON mode. Can't see anything of the sort here.

Also the main patch [1] of my DDI output_types series still didn't get reviewed...

[1] https://patchwork.freedesktop.org/patch/94581/

> 
>  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 103 +++++++++++++++++++++
>  drivers/gpu/drm/i915/Makefile             |   1 +
>  drivers/gpu/drm/i915/i915_drv.h           |   5 ++
>  drivers/gpu/drm/i915/intel_bios.c         |  49 ++++++++++
>  drivers/gpu/drm/i915/intel_ddi.c          |  29 +++++-
>  drivers/gpu/drm/i915/intel_drv.h          |  13 ++-
>  drivers/gpu/drm/i915/intel_lspcon.c       | 145 ++++++++++++++++++++++++++++++
>  include/drm/drm_dp_dual_mode_helper.h     |  26 ++++++
>  8 files changed, 369 insertions(+), 2 deletions(-)  create mode 
> 100644 drivers/gpu/drm/i915/intel_lspcon.c
> 
> --
> 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 v3 0/4] Enable lspcon support for GEN9 devices

[Ville Syrjälä]

On Wed, Jul 20, 2016 at 04:16:30PM +0000, Sharma, Shashank wrote:
> Hi Ville.
> 
> > This thing seems to be missing a lot of stuff:
> > - where is the EDID etc. handling?
> None required for LSPCON as of now.
> The 4k@60 pixel clock support was already added with SKL, so we can see
> a HDMI 4k@60 output already on PCON mode, when connected to a 4k@60 monitor.
> 
> I have started publishing a separate patch series to support HDMI 2.0, started with the new aspect ratio patches here. I was hoping to proceed with EDID changes, once we get these in, else the patch series gets huge and reviewers find it difficult to review. 
> https://patchwork.freedesktop.org/patch/78308/ 
>  
> Plan is to publish in this sequence:
> - Add new aspect ratios
> - Add new 4k modes in DRM EDID
> - Add Scrambling support
> - Add SCDC parsing
> - Add YUV 420 CEA-861-F block parsing
> and so on. 
> 
> > - based on my DDI output_types stuff, I was expecting to see an
> >  lspcon .compute_output_type() hook that returns whether we need
>  >  LS or PCON mode. Can't see anything of the sort here.
> As I explained in the commit message/cover letter LSPCON is no more working selectively on LS/PCON mode.

Oh. Do we have some idea of the power cost?

Curiously when I tried to measure the idle power consumption on my BSW,
DP seemed to be a bit less hungry than HDMI. But I guess the PCON
stuff itself will use some amount of power which might offset any
benefit from using DP on the SoC side.


> We are driving it in always PCON mode, and driver always drives a DP.  So we are driving a DP with separate detection sequence. 
> 
> > Also the main patch [1] of my DDI output_types series still didn't get reviewed...
> Ok, but that should not be a blocker for this series, should it ? 

The encoder->type swithing is still a problem. Though I suppose it will
be fairly well masked if you always have DPCD avaailable.

> 
> Regards
> Shashank
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> Sent: Wednesday, July 20, 2016 6:39 PM
> To: Sharma, Shashank <shashank.sharma@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; rodrigo.vivi@gmail.com; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> Subject: Re: [PATCH v3 0/4] Enable lspcon support for GEN9 devices
> 
> On Tue, Jul 05, 2016 at 06:35:46PM +0530, Shashank Sharma wrote:
> > LSPCON is essentially a dp++->hdmi adapter 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 an 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
> > 
> > V2: Worked on review comments from Ville
> > - In general, Ville suggested not to use the dual personality of
> >   DDI to drive lspcon, so this patch set drives it just as DP++ display.
> >   There is no separate detection for lspcon (hpd_pulse is good enough), and
> >   its being driven as a DP display with special initialization and EDID
> >   read sequence. To be able to do this, we driving lspcon in PCON mode only,
> >   where it can serve both HDMI1.3/HDMI1.4 sinks as well as 4k@60 capable
> >   HDMI 2.0 sinks. So compared to previous series, there is one patch less,
> >   as we have dropped lspcon detection patch.
> > 
> > 
> > V3: Addressed review comments from Rodrigo
> >     Details available in respective patch.
> > 
> > Shashank Sharma (4):
> >   drm: Helper for lspcon in drm_dp_dual_mode
> >   drm/i915: Add lspcon support for I915 driver
> >   drm/i915: Parse VBT data for lspcon
> >   drm/i915: Enable lspcon initialization
> 
> This thing seems to be missing a lot of stuff:
> - where is the EDID etc. handling?
> None required for LSPCON as of now. The 4k@60 pixel clock support was already added with SKL
> - based on my DDI output_types stuff, I was expecting to see an
>   lspcon .compute_output_type() hook that returns whether we need
>   LS or PCON mode. Can't see anything of the sort here.
> 
> Also the main patch [1] of my DDI output_types series still didn't get reviewed...
> 
> [1] https://patchwork.freedesktop.org/patch/94581/
> 
> > 
> >  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 103 +++++++++++++++++++++
> >  drivers/gpu/drm/i915/Makefile             |   1 +
> >  drivers/gpu/drm/i915/i915_drv.h           |   5 ++
> >  drivers/gpu/drm/i915/intel_bios.c         |  49 ++++++++++
> >  drivers/gpu/drm/i915/intel_ddi.c          |  29 +++++-
> >  drivers/gpu/drm/i915/intel_drv.h          |  13 ++-
> >  drivers/gpu/drm/i915/intel_lspcon.c       | 145 ++++++++++++++++++++++++++++++
> >  include/drm/drm_dp_dual_mode_helper.h     |  26 ++++++
> >  8 files changed, 369 insertions(+), 2 deletions(-)  create mode 
> > 100644 drivers/gpu/drm/i915/intel_lspcon.c
> > 
> > --
> > 1.9.1
> 
> --
> Ville Syrjälä
> Intel OTC

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

Re: [PATCH v3 0/4] Enable lspcon support for GEN9 devices

[Sharma, Shashank]

> Oh. Do we have some idea of the power cost?
> Curiously when I tried to measure the idle power consumption on my BSW, DP seemed to be a bit less hungry than HDMI. But I guess the PCON stuff itself will use some amount of power which might offset any benefit from using DP on the SoC side.
You are right, the power side would be slightly higher than the LS mode. 
But there were following cases where we were supposed to be on PCON mode, regardless of pixel clock of requested mode:
- If monitor is HDCP 2.2 capable
- If monitor is trying YUV 420 playback
- If clock > 297 Mhz

So there were lesses cases left to keep LSPCON in LS mode. 
When we compared the code complexity of maintaining LS and PCON mode both, with the use cases, we thought it would be simpler and better to be in PCON mode. 

> The encoder->type swithing is still a problem. Though I suppose it will be fairly well masked if you always have DPCD avaailable.
Not really, now we are always in DP mode. So display controller will be always driving as encoder->type = DP. 
The only changes required now is:
- During the bootup, detect presence of a LSPCON based on VBT, and mark it. 
- During EDID read, if this is LSPCON port, use a separate i2c adapter (well suggested by you in previous review comments :))
- During AVI IF writes, write into LSPCON registers, instead of / on top of display registers. 

Do you think its ok to be going now ? Any further suggestions are welcome. 

Regards
Shashank 
-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Wednesday, July 20, 2016 10:21 PM
To: Sharma, Shashank <shashank.sharma@intel.com>
Cc: intel-gfx@lists.freedesktop.org; rodrigo.vivi@gmail.com; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Zanoni, Paulo R <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH v3 0/4] Enable lspcon support for GEN9 devices

On Wed, Jul 20, 2016 at 04:16:30PM +0000, Sharma, Shashank wrote:
> Hi Ville.
> 
> > This thing seems to be missing a lot of stuff:
> > - where is the EDID etc. handling?
> None required for LSPCON as of now.
> The 4k@60 pixel clock support was already added with SKL, so we can 
> see a HDMI 4k@60 output already on PCON mode, when connected to a 4k@60 monitor.
> 
> I have started publishing a separate patch series to support HDMI 2.0, started with the new aspect ratio patches here. I was hoping to proceed with EDID changes, once we get these in, else the patch series gets huge and reviewers find it difficult to review. 
> https://patchwork.freedesktop.org/patch/78308/
>  
> Plan is to publish in this sequence:
> - Add new aspect ratios
> - Add new 4k modes in DRM EDID
> - Add Scrambling support
> - Add SCDC parsing
> - Add YUV 420 CEA-861-F block parsing
> and so on. 
> 
> > - based on my DDI output_types stuff, I was expecting to see an  
> > lspcon .compute_output_type() hook that returns whether we need
>  >  LS or PCON mode. Can't see anything of the sort here.
> As I explained in the commit message/cover letter LSPCON is no more working selectively on LS/PCON mode.

Oh. Do we have some idea of the power cost?

Curiously when I tried to measure the idle power consumption on my BSW, DP seemed to be a bit less hungry than HDMI. But I guess the PCON stuff itself will use some amount of power which might offset any benefit from using DP on the SoC side.


> We are driving it in always PCON mode, and driver always drives a DP.  So we are driving a DP with separate detection sequence. 
> 
> > Also the main patch [1] of my DDI output_types series still didn't get reviewed...
> Ok, but that should not be a blocker for this series, should it ? 

The encoder->type swithing is still a problem. Though I suppose it will be fairly well masked if you always have DPCD avaailable.

> 
> Regards
> Shashank
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Wednesday, July 20, 2016 6:39 PM
> To: Sharma, Shashank <shashank.sharma@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; rodrigo.vivi@gmail.com; Vivi, 
> Rodrigo <rodrigo.vivi@intel.com>; Zanoni, Paulo R 
> <paulo.r.zanoni@intel.com>
> Subject: Re: [PATCH v3 0/4] Enable lspcon support for GEN9 devices
> 
> On Tue, Jul 05, 2016 at 06:35:46PM +0530, Shashank Sharma wrote:
> > LSPCON is essentially a dp++->hdmi adapter 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 an 
> > 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
> > 
> > V2: Worked on review comments from Ville
> > - In general, Ville suggested not to use the dual personality of
> >   DDI to drive lspcon, so this patch set drives it just as DP++ display.
> >   There is no separate detection for lspcon (hpd_pulse is good enough), and
> >   its being driven as a DP display with special initialization and EDID
> >   read sequence. To be able to do this, we driving lspcon in PCON mode only,
> >   where it can serve both HDMI1.3/HDMI1.4 sinks as well as 4k@60 capable
> >   HDMI 2.0 sinks. So compared to previous series, there is one patch less,
> >   as we have dropped lspcon detection patch.
> > 
> > 
> > V3: Addressed review comments from Rodrigo
> >     Details available in respective patch.
> > 
> > Shashank Sharma (4):
> >   drm: Helper for lspcon in drm_dp_dual_mode
> >   drm/i915: Add lspcon support for I915 driver
> >   drm/i915: Parse VBT data for lspcon
> >   drm/i915: Enable lspcon initialization
> 
> This thing seems to be missing a lot of stuff:
> - where is the EDID etc. handling?
> None required for LSPCON as of now. The 4k@60 pixel clock support was 
> already added with SKL
> - based on my DDI output_types stuff, I was expecting to see an
>   lspcon .compute_output_type() hook that returns whether we need
>   LS or PCON mode. Can't see anything of the sort here.
> 
> Also the main patch [1] of my DDI output_types series still didn't get reviewed...
> 
> [1] https://patchwork.freedesktop.org/patch/94581/
> 
> > 
> >  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 103 +++++++++++++++++++++
> >  drivers/gpu/drm/i915/Makefile             |   1 +
> >  drivers/gpu/drm/i915/i915_drv.h           |   5 ++
> >  drivers/gpu/drm/i915/intel_bios.c         |  49 ++++++++++
> >  drivers/gpu/drm/i915/intel_ddi.c          |  29 +++++-
> >  drivers/gpu/drm/i915/intel_drv.h          |  13 ++-
> >  drivers/gpu/drm/i915/intel_lspcon.c       | 145 ++++++++++++++++++++++++++++++
> >  include/drm/drm_dp_dual_mode_helper.h     |  26 ++++++
> >  8 files changed, 369 insertions(+), 2 deletions(-)  create mode
> > 100644 drivers/gpu/drm/i915/intel_lspcon.c
> > 
> > --
> > 1.9.1
> 
> --
> Ville Syrjälä
> Intel OTC

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

Re: [PATCH v3 1/4] drm: Helper for lspcon in drm_dp_dual_mode

[Ville Syrjälä]

On Tue, Jul 05, 2016 at 06:35:47PM +0530, Shashank Sharma wrote:
> This patch adds lspcon support in dp_dual_mode helper.
> lspcon is essentially a dp->hdmi dongle with dual personality.
> 
> LS mode: It works as a passive dongle, by level shifting DP++
> signals to HDMI signals, in LS mode.
> PCON mode: It works as a protocol converter active dongle
> in pcon mode, by converting DP++ outputs to HDMI 2.0 outputs.
> 
> This patch adds support for lspcon detection and mode set
> switch operations, as a dp dual mode dongle.
> 
> v2: Addressed review comments from Ville
> - add adaptor id for lspcon devices (0x08), use it to identify lspcon
> - change function names
>   old: drm_lspcon_get_current_mode/drm_lspcon_change_mode
>   new: drm_lspcon_get_mode/drm_lspcon_set_mode
> - change drm_lspcon_get_mode type to int, to match
>   drm_dp_dual_mode_get_tmds_output
> - change 'err' to 'ret' to match the rest of the functions
> - remove pointless typecasting during call to dual_mode_read
> - fix the but while setting value of data, while writing lspcon mode
> - fix indentation
> - change mdelay(10) -> msleep(10)
> - return ETIMEDOUT instead of EFAULT, when lspcon mode change times out
> - Add an empty line to separate std regs macros and lspcon regs macros
>   Indent bit definition
> 
> v3: Addressed review comments from Rodrigo
> - change macro name from DP_DUAL_MODE_TYPE_LSPCON to
>   DP_DUAL_MODE_TYPE_HAS_DPCD for better readability
> - change macro name from DP_DUAL_MODE_LSPCON_MODE_PCON to
>   DP_DUAL_MODE_LSPCON_MODE_PCON for better readability
> - add comment for MCA specific offsets like 0x40 and 0x41
> - remove DP_DUAL_MODE_REV_TYPE2 check while checking lspcon adapter id
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 103 ++++++++++++++++++++++++++++++
>  include/drm/drm_dp_dual_mode_helper.h     |  26 ++++++++
>  2 files changed, 129 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..4f89e0b 100644
> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> @@ -148,6 +148,14 @@ 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 == (DP_DUAL_MODE_TYPE_TYPE2 |
> +			DP_DUAL_MODE_TYPE_HAS_DPCD));

Looks like an indent fail here.

> +}
> +
>  /**
>   * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor
>   * @adapter: I2C adapter for the DDC bus
> @@ -203,6 +211,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 +374,96 @@ 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
> + * @current_mode: out vaiable, current lspcon mode of operation
> + *
> + * Returns:
> + * 0 on success, sets the current_mode value to appropriate mode
> + * -error on failure
> + */
> +int drm_lspcon_get_mode(struct i2c_adapter *adapter,
> +	enum drm_lspcon_mode *current_mode)

indent fail, I would just call it 'mode'

> +{
> +	u8 data;
> +	int ret = 0;
> +
> +	if (!current_mode) {
> +		DRM_ERROR("NULL input\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Read Status: i2c over aux */
> +	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> +			(void *)&data, sizeof(data));

indent fail, void* cast not needed.

> +	if (ret < 0) {
> +		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
> +		return -EFAULT;
> +	}
> +
> +	if (data & DP_DUAL_MODE_LSPCON_MODE_PCON)
> +		*current_mode = DRM_LSPCON_MODE_PCON;
> +	else
> +		*current_mode = DRM_LSPCON_MODE_LS;
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_lspcon_get_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_set_mode(struct i2c_adapter *adapter,
> +	enum drm_lspcon_mode reqd_mode)

indent fail, I would just call it 'mode'

> +{
> +	u8 data = 0;
> +	int ret;
> +	int time_out = 200;
> +	enum drm_lspcon_mode changed_mode;

This I might call 'current_mode'. The declaration can be moved inside
the loop.

> +
> +	if (reqd_mode == DRM_LSPCON_MODE_PCON)
> +		data = DP_DUAL_MODE_LSPCON_MODE_PCON;
> +
> +	/* Change mode */
> +	ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_LSPCON_MODE_CHANGE,
> +			&data, sizeof(data));

indent fail.

> +	if (ret < 0) {
> +		DRM_ERROR("LSPCON mode change failed\n");
> +		return ret;
> +	}
> +
> +	/*
> +	  * 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.
> +	  */
> +	do {
> +		ret = drm_lspcon_get_mode(adapter, &changed_mode);
> +		if (ret) {
> +			DRM_ERROR("cant confirm LSPCON mode change\n");
> +			return ret;
> +		} else {
> +			if (changed_mode != reqd_mode) {
> +				msleep(10);
> +				time_out -= 10;
> +			} else {
> +				DRM_DEBUG_KMS("LSPCON mode changed to %s\n",
> +					reqd_mode == DRM_LSPCON_MODE_LS ?
> +						"LS" : "PCON");
> +				return 0;
> +			}
> +		}
> +	} while (time_out);
> +
> +	DRM_ERROR("LSPCON mode change timed out\n");
> +	return -ETIMEDOUT;
> +}
> +EXPORT_SYMBOL(drm_lspcon_set_mode);
> diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h
> index e8a9dfd..a3c1d03 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>

Why?

>  
>  /*
>   * Optional for type 1 DVI adaptors
> @@ -40,6 +41,7 @@
>  #define  DP_DUAL_MODE_REV_TYPE2 0x00
>  #define  DP_DUAL_MODE_TYPE_MASK 0xf0
>  #define  DP_DUAL_MODE_TYPE_TYPE2 0xa0
> +#define  DP_DUAL_MODE_TYPE_HAS_DPCD 0x08

Maybe add a comment that this came from LSPCON, and it's marked
reserved in the official dual mode spec.

>  #define DP_DUAL_MODE_IEEE_OUI 0x11 /* 11-13*/
>  #define  DP_DUAL_IEEE_OUI_LEN 3
>  #define DP_DUAL_DEVICE_ID 0x14 /* 14-19 */
> @@ -55,6 +57,11 @@
>  #define  DP_DUAL_MODE_CEC_ENABLE 0x01
>  #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22
>  
> +/* LSPCON specific registers, defined by MCA */
> +#define DP_DUAL_MODE_LSPCON_MODE_CHANGE		0x40
> +#define DP_DUAL_MODE_LSPCON_CURRENT_MODE		0x41
> +#define  DP_DUAL_MODE_LSPCON_MODE_PCON			0x1
> +
>  struct i2c_adapter;
>  
>  ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
> @@ -63,6 +70,19 @@ 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_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.

These don't match the enum below.

> +*/
> +enum drm_lspcon_mode {
> +	DRM_LSPCON_MODE_INVALID,

Did we still have some need for this?

> +	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 +90,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 +99,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 +111,8 @@ 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_lspcon_get_mode(struct i2c_adapter *adapter,
> +	enum drm_lspcon_mode *current_mode);
> +int drm_lspcon_set_mode(struct i2c_adapter *adapter,
> +		enum drm_lspcon_mode reqd_mode);

more indent fails.

>  #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 v3 2/4] drm/i915: Add lspcon support for I915 driver

[Ville Syrjälä]

On Tue, Jul 05, 2016 at 06:35:48PM +0530, Shashank Sharma wrote:
> 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.
> 
> V2: addressed ville's review comments
> - Clean the leftover macros from previous patch set
> 
> V3: Rebase
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile       |   1 +
>  drivers/gpu/drm/i915/intel_drv.h    |  13 +++-
>  drivers/gpu/drm/i915/intel_lspcon.c | 145 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 158 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 618293c..64cd373 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -95,6 +95,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 e6a24d2..e6982cf 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -922,12 +922,19 @@ struct intel_dp {
>  	bool compliance_test_active;
>  };
>  
> +struct intel_lspcon {
> +	bool active;
> +	enum drm_lspcon_mode mode_of_op;

I'd call this just 'mode'

> +	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;
> @@ -1478,7 +1485,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);
>  struct intel_encoder *intel_get_lvds_encoder(struct drm_device *dev);
> @@ -1779,4 +1785,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..fdeff71
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -0,0 +1,145 @@
> +/*
> + * 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"
> +
> +bool is_lspcon_active(struct intel_digital_port *dig_port)
> +{
> +	return dig_port->lspcon.active;
> +}

unused -> kill it

In fact the whole lspcon.active flags seems pretty pointless, so I'd
just kill that too.

> +
> +enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
> +{
> +	enum drm_lspcon_mode current_mode = DRM_LSPCON_MODE_INVALID;
> +	struct i2c_adapter *adapter = &lspcon->aux->ddc;
> +
> +	if (drm_lspcon_get_mode(adapter, &current_mode))
> +		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;
> +}

I was going to suggest killing this one too, but I guess the debug
output might be useful. Make it static at least.

> +
> +int lspcon_change_mode(struct intel_lspcon *lspcon,
> +	enum drm_lspcon_mode mode, bool force)

static

> +{
> +	int err;
> +	enum drm_lspcon_mode current_mode;
> +	struct i2c_adapter *adapter = &lspcon->aux->ddc;
> +
> +	err = drm_lspcon_get_mode(adapter, &current_mode);
> +	if (err) {
> +		DRM_ERROR("Error reading LSPCON mode\n");
> +		return err;
> +	}
> +
> +	if (current_mode == mode && !force) {
> +		DRM_DEBUG_KMS("Current mode = desired LSPCON mode\n");
> +		return 0;
> +	}

We always call this with force==true, so it looks like a pointless
parameter. Also if force==true always, it doesn't necessary to even
query the current mode. Oh and I think we've already queried the mode
when this gets called.

> +
> +	err = drm_lspcon_set_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)

static

> +{
> +	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;
> +}

This is small function and called only once, so I'd just inline it into
the caller actually.

> +
> +enum drm_lspcon_mode lspcon_probe(struct intel_lspcon *lspcon)

static

> +{
> +
> +	/* 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 */
> +	lspcon->mode_of_op = lspcon_get_current_mode(lspcon);
> +	lspcon->active = true;
> +	return true;
> +}
> +
> +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;
> +	}

Just remove this check? If it's not present, we won't detect it.

> +
> +	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) {

indent fail.

> +			DRM_ERROR("LSPCON mode change to PCON failed\n");
> +			return false;
> +		}
> +	}
> +
> +	DRM_DEBUG_KMS("Success: LSPCON init\n");
> +	return true;
> +}
> -- 
> 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 v3 1/4] drm: Helper for lspcon in drm_dp_dual_mode

[Sharma, Shashank]

Thanks for the review, Ville.

My comments, inline.

Regards

Shashank


On 8/11/2016 12:19 PM, Ville Syrjälä wrote:
> On Tue, Jul 05, 2016 at 06:35:47PM +0530, Shashank Sharma wrote:
>> This patch adds lspcon support in dp_dual_mode helper.
>> lspcon is essentially a dp->hdmi dongle with dual personality.
>>
>> LS mode: It works as a passive dongle, by level shifting DP++
>> signals to HDMI signals, in LS mode.
>> PCON mode: It works as a protocol converter active dongle
>> in pcon mode, by converting DP++ outputs to HDMI 2.0 outputs.
>>
>> This patch adds support for lspcon detection and mode set
>> switch operations, as a dp dual mode dongle.
>>
>> v2: Addressed review comments from Ville
>> - add adaptor id for lspcon devices (0x08), use it to identify lspcon
>> - change function names
>>    old: drm_lspcon_get_current_mode/drm_lspcon_change_mode
>>    new: drm_lspcon_get_mode/drm_lspcon_set_mode
>> - change drm_lspcon_get_mode type to int, to match
>>    drm_dp_dual_mode_get_tmds_output
>> - change 'err' to 'ret' to match the rest of the functions
>> - remove pointless typecasting during call to dual_mode_read
>> - fix the but while setting value of data, while writing lspcon mode
>> - fix indentation
>> - change mdelay(10) -> msleep(10)
>> - return ETIMEDOUT instead of EFAULT, when lspcon mode change times out
>> - Add an empty line to separate std regs macros and lspcon regs macros
>>    Indent bit definition
>>
>> v3: Addressed review comments from Rodrigo
>> - change macro name from DP_DUAL_MODE_TYPE_LSPCON to
>>    DP_DUAL_MODE_TYPE_HAS_DPCD for better readability
>> - change macro name from DP_DUAL_MODE_LSPCON_MODE_PCON to
>>    DP_DUAL_MODE_LSPCON_MODE_PCON for better readability
>> - add comment for MCA specific offsets like 0x40 and 0x41
>> - remove DP_DUAL_MODE_REV_TYPE2 check while checking lspcon adapter id
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/drm_dp_dual_mode_helper.c | 103 ++++++++++++++++++++++++++++++
>>   include/drm/drm_dp_dual_mode_helper.h     |  26 ++++++++
>>   2 files changed, 129 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..4f89e0b 100644
>> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>> @@ -148,6 +148,14 @@ 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 == (DP_DUAL_MODE_TYPE_TYPE2 |
>> +			DP_DUAL_MODE_TYPE_HAS_DPCD));
> Looks like an indent fail here.
Can you please let me know, why do we call it indent fail ? checkpatch 
doesn't complaint about this.
>> +}
>> +
>>   /**
>>    * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor
>>    * @adapter: I2C adapter for the DDC bus
>> @@ -203,6 +211,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 +374,96 @@ 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
>> + * @current_mode: out vaiable, current lspcon mode of operation
>> + *
>> + * Returns:
>> + * 0 on success, sets the current_mode value to appropriate mode
>> + * -error on failure
>> + */
>> +int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>> +	enum drm_lspcon_mode *current_mode)
> indent fail, I would just call it 'mode'
I wanted to be more informative about what is the out parameter, I 
though it makes it more readable
that its returning current mode of lspcon.  Would you still prefer to 
call it mode only ?
>> +{
>> +	u8 data;
>> +	int ret = 0;
>> +
>> +	if (!current_mode) {
>> +		DRM_ERROR("NULL input\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Read Status: i2c over aux */
>> +	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>> +			(void *)&data, sizeof(data));
> indent fail, void* cast not needed.
Sure, will remove void, same question on indent as above.
>> +	if (ret < 0) {
>> +		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
>> +		return -EFAULT;
>> +	}
>> +
>> +	if (data & DP_DUAL_MODE_LSPCON_MODE_PCON)
>> +		*current_mode = DRM_LSPCON_MODE_PCON;
>> +	else
>> +		*current_mode = DRM_LSPCON_MODE_LS;
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_lspcon_get_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_set_mode(struct i2c_adapter *adapter,
>> +	enum drm_lspcon_mode reqd_mode)
> indent fail, I would just call it 'mode'
Same as above comments about indent.
Now, this function deals with two modes:
one which we wish to move to, and one which we get after mode change 
operation.
The mode after mode change operation might not be the same as the one we 
wished for.
So wouldn't it be better to make it clear by calling in reqd_mode ?
>> +{
>> +	u8 data = 0;
>> +	int ret;
>> +	int time_out = 200;
>> +	enum drm_lspcon_mode changed_mode;
> This I might call 'current_mode'. The declaration can be moved inside
> the loop.
Sure, will do that.
>> +
>> +	if (reqd_mode == DRM_LSPCON_MODE_PCON)
>> +		data = DP_DUAL_MODE_LSPCON_MODE_PCON;
>> +
>> +	/* Change mode */
>> +	ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_LSPCON_MODE_CHANGE,
>> +			&data, sizeof(data));
> indent fail.
Same as above.
>> +	if (ret < 0) {
>> +		DRM_ERROR("LSPCON mode change failed\n");
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	  * 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.
>> +	  */
>> +	do {
>> +		ret = drm_lspcon_get_mode(adapter, &changed_mode);
>> +		if (ret) {
>> +			DRM_ERROR("cant confirm LSPCON mode change\n");
>> +			return ret;
>> +		} else {
>> +			if (changed_mode != reqd_mode) {
>> +				msleep(10);
>> +				time_out -= 10;
>> +			} else {
>> +				DRM_DEBUG_KMS("LSPCON mode changed to %s\n",
>> +					reqd_mode == DRM_LSPCON_MODE_LS ?
>> +						"LS" : "PCON");
>> +				return 0;
>> +			}
>> +		}
>> +	} while (time_out);
>> +
>> +	DRM_ERROR("LSPCON mode change timed out\n");
>> +	return -ETIMEDOUT;
>> +}
>> +EXPORT_SYMBOL(drm_lspcon_set_mode);
>> diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h
>> index e8a9dfd..a3c1d03 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>
> Why?
Sorry, leftover. This was required in previous patch set to get the 
DDC_ADDR macro when we had separate EDID read function for LSPCON.
Will remove this.
>>   
>>   /*
>>    * Optional for type 1 DVI adaptors
>> @@ -40,6 +41,7 @@
>>   #define  DP_DUAL_MODE_REV_TYPE2 0x00
>>   #define  DP_DUAL_MODE_TYPE_MASK 0xf0
>>   #define  DP_DUAL_MODE_TYPE_TYPE2 0xa0
>> +#define  DP_DUAL_MODE_TYPE_HAS_DPCD 0x08
> Maybe add a comment that this came from LSPCON, and it's marked
> reserved in the official dual mode spec.
Ok, got it.
>>   #define DP_DUAL_MODE_IEEE_OUI 0x11 /* 11-13*/
>>   #define  DP_DUAL_IEEE_OUI_LEN 3
>>   #define DP_DUAL_DEVICE_ID 0x14 /* 14-19 */
>> @@ -55,6 +57,11 @@
>>   #define  DP_DUAL_MODE_CEC_ENABLE 0x01
>>   #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22
>>   
>> +/* LSPCON specific registers, defined by MCA */
>> +#define DP_DUAL_MODE_LSPCON_MODE_CHANGE		0x40
>> +#define DP_DUAL_MODE_LSPCON_CURRENT_MODE		0x41
>> +#define  DP_DUAL_MODE_LSPCON_MODE_PCON			0x1
>> +
>>   struct i2c_adapter;
>>   
>>   ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
>> @@ -63,6 +70,19 @@ 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_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.
> These don't match the enum below.
Ok, will fix it.
>> +*/
>> +enum drm_lspcon_mode {
>> +	DRM_LSPCON_MODE_INVALID,
> Did we still have some need for this?
Yes, lspcon probe failure case is using this.
>> +	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 +90,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 +99,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 +111,8 @@ 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_lspcon_get_mode(struct i2c_adapter *adapter,
>> +	enum drm_lspcon_mode *current_mode);
>> +int drm_lspcon_set_mode(struct i2c_adapter *adapter,
>> +		enum drm_lspcon_mode reqd_mode);
> more indent fails.
Same query as previous

Regards
Shashank
>>   #endif
>> -- 
>> 1.9.1

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

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

[Sharma, Shashank]

Regards

Shashank


On 8/11/2016 12:33 PM, Ville Syrjälä wrote:
> On Tue, Jul 05, 2016 at 06:35:48PM +0530, Shashank Sharma wrote:
>> 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.
>>
>> V2: addressed ville's review comments
>> - Clean the leftover macros from previous patch set
>>
>> V3: Rebase
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com>
>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>   drivers/gpu/drm/i915/Makefile       |   1 +
>>   drivers/gpu/drm/i915/intel_drv.h    |  13 +++-
>>   drivers/gpu/drm/i915/intel_lspcon.c | 145 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 158 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 618293c..64cd373 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -95,6 +95,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 e6a24d2..e6982cf 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -922,12 +922,19 @@ struct intel_dp {
>>   	bool compliance_test_active;
>>   };
>>   
>> +struct intel_lspcon {
>> +	bool active;
>> +	enum drm_lspcon_mode mode_of_op;
> I'd call this just 'mode'
I dont want reader to get confused this with a videomode, so made it 
more clear :)
Do you think we can keep it this way ?
>> +	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;
>> @@ -1478,7 +1485,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);
>>   struct intel_encoder *intel_get_lvds_encoder(struct drm_device *dev);
>> @@ -1779,4 +1785,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..fdeff71
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>> @@ -0,0 +1,145 @@
>> +/*
>> + * 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"
>> +
>> +bool is_lspcon_active(struct intel_digital_port *dig_port)
>> +{
>> +	return dig_port->lspcon.active;
>> +}
> unused -> kill it
>
> In fact the whole lspcon.active flags seems pretty pointless, so I'd
> just kill that too.
Please note that, I tried to address both motherboard-down mode as well 
as adapter-mode LSPCON
in this design. In case of adapter mode lspcon, we will need this.
>
>> +
>> +enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
>> +{
>> +	enum drm_lspcon_mode current_mode = DRM_LSPCON_MODE_INVALID;
>> +	struct i2c_adapter *adapter = &lspcon->aux->ddc;
>> +
>> +	if (drm_lspcon_get_mode(adapter, &current_mode))
>> +		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;
>> +}
> I was going to suggest killing this one too, but I guess the debug
> output might be useful. Make it static at least.
Sure, will make it static. Also we have a plan to expose one DRM 
property to handle adapter mode lspcon
at that time this will help.
>> +
>> +int lspcon_change_mode(struct intel_lspcon *lspcon,
>> +	enum drm_lspcon_mode mode, bool force)
> static
Sure.
>
>> +{
>> +	int err;
>> +	enum drm_lspcon_mode current_mode;
>> +	struct i2c_adapter *adapter = &lspcon->aux->ddc;
>> +
>> +	err = drm_lspcon_get_mode(adapter, &current_mode);
>> +	if (err) {
>> +		DRM_ERROR("Error reading LSPCON mode\n");
>> +		return err;
>> +	}
>> +
>> +	if (current_mode == mode && !force) {
>> +		DRM_DEBUG_KMS("Current mode = desired LSPCON mode\n");
>> +		return 0;
>> +	}
> We always call this with force==true, so it looks like a pointless
> parameter. Also if force==true always, it doesn't necessary to even
> query the current mode. Oh and I think we've already queried the mode
> when this gets called.
As mentioned, I wanted to keep the scope of switching lspcon modes, in 
the design infrastructure.
So I kept these things as possibility.

IF we create a DRM property to query LSPCON mode, that get_property 
function will call this
function with force = false, coz for this query we dont want to probe 
the HW again.
>> +
>> +	err = drm_lspcon_set_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)
> static
got it.
>
>> +{
>> +	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;
>> +}
> This is small function and called only once, so I'd just inline it into
> the caller actually.
Can be done, but the caller is intelI_ddi_init, which I dont want to 
populate with lspcon specific code, when we have a separate file
for lspcon. Do you think its a good idea ? or you prefer to have that in 
ddi_init only ?
>> +
>> +enum drm_lspcon_mode lspcon_probe(struct intel_lspcon *lspcon)
> static
Got it.
>
>> +{
>> +
>> +	/* 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 */
>> +	lspcon->mode_of_op = lspcon_get_current_mode(lspcon);
>> +	lspcon->active = true;
>> +	return true;
>> +}
>> +
>> +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;
>> +	}
> Just remove this check? If it's not present, we won't detect it.
Faulty VBT ? Just being paranoid.
>> +
>> +	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) {
> indent fail.
same as previous.

Regards
Shashank
>> +			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

Re: [PATCH v3 1/4] drm: Helper for lspcon in drm_dp_dual_mode

[Ville Syrjälä]

On Thu, Aug 11, 2016 at 02:14:10PM +0530, Sharma, Shashank wrote:
> Thanks for the review, Ville.
> 
> My comments, inline.
> 
> Regards
> 
> Shashank
> 
> 
> On 8/11/2016 12:19 PM, Ville Syrjälä wrote:
> > On Tue, Jul 05, 2016 at 06:35:47PM +0530, Shashank Sharma wrote:
> >> This patch adds lspcon support in dp_dual_mode helper.
> >> lspcon is essentially a dp->hdmi dongle with dual personality.
> >>
> >> LS mode: It works as a passive dongle, by level shifting DP++
> >> signals to HDMI signals, in LS mode.
> >> PCON mode: It works as a protocol converter active dongle
> >> in pcon mode, by converting DP++ outputs to HDMI 2.0 outputs.
> >>
> >> This patch adds support for lspcon detection and mode set
> >> switch operations, as a dp dual mode dongle.
> >>
> >> v2: Addressed review comments from Ville
> >> - add adaptor id for lspcon devices (0x08), use it to identify lspcon
> >> - change function names
> >>    old: drm_lspcon_get_current_mode/drm_lspcon_change_mode
> >>    new: drm_lspcon_get_mode/drm_lspcon_set_mode
> >> - change drm_lspcon_get_mode type to int, to match
> >>    drm_dp_dual_mode_get_tmds_output
> >> - change 'err' to 'ret' to match the rest of the functions
> >> - remove pointless typecasting during call to dual_mode_read
> >> - fix the but while setting value of data, while writing lspcon mode
> >> - fix indentation
> >> - change mdelay(10) -> msleep(10)
> >> - return ETIMEDOUT instead of EFAULT, when lspcon mode change times out
> >> - Add an empty line to separate std regs macros and lspcon regs macros
> >>    Indent bit definition
> >>
> >> v3: Addressed review comments from Rodrigo
> >> - change macro name from DP_DUAL_MODE_TYPE_LSPCON to
> >>    DP_DUAL_MODE_TYPE_HAS_DPCD for better readability
> >> - change macro name from DP_DUAL_MODE_LSPCON_MODE_PCON to
> >>    DP_DUAL_MODE_LSPCON_MODE_PCON for better readability
> >> - add comment for MCA specific offsets like 0x40 and 0x41
> >> - remove DP_DUAL_MODE_REV_TYPE2 check while checking lspcon adapter id
> >>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> ---
> >>   drivers/gpu/drm/drm_dp_dual_mode_helper.c | 103 ++++++++++++++++++++++++++++++
> >>   include/drm/drm_dp_dual_mode_helper.h     |  26 ++++++++
> >>   2 files changed, 129 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..4f89e0b 100644
> >> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> >> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> >> @@ -148,6 +148,14 @@ 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 == (DP_DUAL_MODE_TYPE_TYPE2 |
> >> +			DP_DUAL_MODE_TYPE_HAS_DPCD));
> > Looks like an indent fail here.
> Can you please let me know, why do we call it indent fail ? checkpatch 
> doesn't complaint about this.

Stuff should line up after the opening '('

> >> +}
> >> +
> >>   /**
> >>    * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor
> >>    * @adapter: I2C adapter for the DDC bus
> >> @@ -203,6 +211,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 +374,96 @@ 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
> >> + * @current_mode: out vaiable, current lspcon mode of operation
> >> + *
> >> + * Returns:
> >> + * 0 on success, sets the current_mode value to appropriate mode
> >> + * -error on failure
> >> + */
> >> +int drm_lspcon_get_mode(struct i2c_adapter *adapter,
> >> +	enum drm_lspcon_mode *current_mode)
> > indent fail, I would just call it 'mode'
> I wanted to be more informative about what is the out parameter, I 
> though it makes it more readable
> that its returning current mode of lspcon.  Would you still prefer to 
> call it mode only ?

Yes, the fact that it's the current mode is pretty obvious.

> >> +{
> >> +	u8 data;
> >> +	int ret = 0;
> >> +
> >> +	if (!current_mode) {
> >> +		DRM_ERROR("NULL input\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	/* Read Status: i2c over aux */
> >> +	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> >> +			(void *)&data, sizeof(data));
> > indent fail, void* cast not needed.
> Sure, will remove void, same question on indent as above.
> >> +	if (ret < 0) {
> >> +		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
> >> +		return -EFAULT;
> >> +	}
> >> +
> >> +	if (data & DP_DUAL_MODE_LSPCON_MODE_PCON)
> >> +		*current_mode = DRM_LSPCON_MODE_PCON;
> >> +	else
> >> +		*current_mode = DRM_LSPCON_MODE_LS;
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL(drm_lspcon_get_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_set_mode(struct i2c_adapter *adapter,
> >> +	enum drm_lspcon_mode reqd_mode)
> > indent fail, I would just call it 'mode'
> Same as above comments about indent.
> Now, this function deals with two modes:
> one which we wish to move to, and one which we get after mode change 
> operation.
> The mode after mode change operation might not be the same as the one we 
> wished for.
> So wouldn't it be better to make it clear by calling in reqd_mode ?

reqd_ looks line noise to me. You could spell it out properly, but still
looks like superfluous infromation to me. It's pretty clear from the
context.

> >> +{
> >> +	u8 data = 0;
> >> +	int ret;
> >> +	int time_out = 200;
> >> +	enum drm_lspcon_mode changed_mode;
> > This I might call 'current_mode'. The declaration can be moved inside
> > the loop.
> Sure, will do that.
> >> +
> >> +	if (reqd_mode == DRM_LSPCON_MODE_PCON)
> >> +		data = DP_DUAL_MODE_LSPCON_MODE_PCON;
> >> +
> >> +	/* Change mode */
> >> +	ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_LSPCON_MODE_CHANGE,
> >> +			&data, sizeof(data));
> > indent fail.
> Same as above.
> >> +	if (ret < 0) {
> >> +		DRM_ERROR("LSPCON mode change failed\n");
> >> +		return ret;
> >> +	}
> >> +
> >> +	/*
> >> +	  * 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.
> >> +	  */
> >> +	do {
> >> +		ret = drm_lspcon_get_mode(adapter, &changed_mode);
> >> +		if (ret) {
> >> +			DRM_ERROR("cant confirm LSPCON mode change\n");
> >> +			return ret;
> >> +		} else {
> >> +			if (changed_mode != reqd_mode) {
> >> +				msleep(10);
> >> +				time_out -= 10;
> >> +			} else {
> >> +				DRM_DEBUG_KMS("LSPCON mode changed to %s\n",
> >> +					reqd_mode == DRM_LSPCON_MODE_LS ?
> >> +						"LS" : "PCON");
> >> +				return 0;
> >> +			}
> >> +		}
> >> +	} while (time_out);
> >> +
> >> +	DRM_ERROR("LSPCON mode change timed out\n");
> >> +	return -ETIMEDOUT;
> >> +}
> >> +EXPORT_SYMBOL(drm_lspcon_set_mode);
> >> diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h
> >> index e8a9dfd..a3c1d03 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>
> > Why?
> Sorry, leftover. This was required in previous patch set to get the 
> DDC_ADDR macro when we had separate EDID read function for LSPCON.
> Will remove this.
> >>   
> >>   /*
> >>    * Optional for type 1 DVI adaptors
> >> @@ -40,6 +41,7 @@
> >>   #define  DP_DUAL_MODE_REV_TYPE2 0x00
> >>   #define  DP_DUAL_MODE_TYPE_MASK 0xf0
> >>   #define  DP_DUAL_MODE_TYPE_TYPE2 0xa0
> >> +#define  DP_DUAL_MODE_TYPE_HAS_DPCD 0x08
> > Maybe add a comment that this came from LSPCON, and it's marked
> > reserved in the official dual mode spec.
> Ok, got it.
> >>   #define DP_DUAL_MODE_IEEE_OUI 0x11 /* 11-13*/
> >>   #define  DP_DUAL_IEEE_OUI_LEN 3
> >>   #define DP_DUAL_DEVICE_ID 0x14 /* 14-19 */
> >> @@ -55,6 +57,11 @@
> >>   #define  DP_DUAL_MODE_CEC_ENABLE 0x01
> >>   #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22
> >>   
> >> +/* LSPCON specific registers, defined by MCA */
> >> +#define DP_DUAL_MODE_LSPCON_MODE_CHANGE		0x40
> >> +#define DP_DUAL_MODE_LSPCON_CURRENT_MODE		0x41
> >> +#define  DP_DUAL_MODE_LSPCON_MODE_PCON			0x1
> >> +
> >>   struct i2c_adapter;
> >>   
> >>   ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
> >> @@ -63,6 +70,19 @@ 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_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.
> > These don't match the enum below.
> Ok, will fix it.
> >> +*/
> >> +enum drm_lspcon_mode {
> >> +	DRM_LSPCON_MODE_INVALID,
> > Did we still have some need for this?
> Yes, lspcon probe failure case is using this.

That's already signalled via the return value, so not sure what extra
this gives us.

> >> +	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 +90,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 +99,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 +111,8 @@ 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_lspcon_get_mode(struct i2c_adapter *adapter,
> >> +	enum drm_lspcon_mode *current_mode);
> >> +int drm_lspcon_set_mode(struct i2c_adapter *adapter,
> >> +		enum drm_lspcon_mode reqd_mode);
> > more indent fails.
> Same query as previous
> 
> Regards
> Shashank
> >>   #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 v3 2/4] drm/i915: Add lspcon support for I915 driver

[Ville Syrjälä]

On Thu, Aug 11, 2016 at 02:36:23PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 8/11/2016 12:33 PM, Ville Syrjälä wrote:
> > On Tue, Jul 05, 2016 at 06:35:48PM +0530, Shashank Sharma wrote:
> >> 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.
> >>
> >> V2: addressed ville's review comments
> >> - Clean the leftover macros from previous patch set
> >>
> >> V3: Rebase
> >>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com>
> >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/Makefile       |   1 +
> >>   drivers/gpu/drm/i915/intel_drv.h    |  13 +++-
> >>   drivers/gpu/drm/i915/intel_lspcon.c | 145 ++++++++++++++++++++++++++++++++++++
> >>   3 files changed, 158 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 618293c..64cd373 100644
> >> --- a/drivers/gpu/drm/i915/Makefile
> >> +++ b/drivers/gpu/drm/i915/Makefile
> >> @@ -95,6 +95,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 e6a24d2..e6982cf 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -922,12 +922,19 @@ struct intel_dp {
> >>   	bool compliance_test_active;
> >>   };
> >>   
> >> +struct intel_lspcon {
> >> +	bool active;
> >> +	enum drm_lspcon_mode mode_of_op;
> > I'd call this just 'mode'
> I dont want reader to get confused this with a videomode, so made it 
> more clear :)
> Do you think we can keep it this way ?

I don't think there's any real chance of a mixup there. It's always going
to used as lspcon.mode I think, and the types are incompatible anyway.

> >> +	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;
> >> @@ -1478,7 +1485,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);
> >>   struct intel_encoder *intel_get_lvds_encoder(struct drm_device *dev);
> >> @@ -1779,4 +1785,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..fdeff71
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> >> @@ -0,0 +1,145 @@
> >> +/*
> >> + * 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"
> >> +
> >> +bool is_lspcon_active(struct intel_digital_port *dig_port)
> >> +{
> >> +	return dig_port->lspcon.active;
> >> +}
> > unused -> kill it
> >
> > In fact the whole lspcon.active flags seems pretty pointless, so I'd
> > just kill that too.
> Please note that, I tried to address both motherboard-down mode as well 
> as adapter-mode LSPCON
> in this design. In case of adapter mode lspcon, we will need this.

We don't use it, so clearly it's not needed.

> >
> >> +
> >> +enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
> >> +{
> >> +	enum drm_lspcon_mode current_mode = DRM_LSPCON_MODE_INVALID;
> >> +	struct i2c_adapter *adapter = &lspcon->aux->ddc;
> >> +
> >> +	if (drm_lspcon_get_mode(adapter, &current_mode))
> >> +		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;
> >> +}
> > I was going to suggest killing this one too, but I guess the debug
> > output might be useful. Make it static at least.
> Sure, will make it static. Also we have a plan to expose one DRM 
> property to handle adapter mode lspcon
> at that time this will help.

Please don't. It should just be automagic.

> >> +
> >> +int lspcon_change_mode(struct intel_lspcon *lspcon,
> >> +	enum drm_lspcon_mode mode, bool force)
> > static
> Sure.
> >
> >> +{
> >> +	int err;
> >> +	enum drm_lspcon_mode current_mode;
> >> +	struct i2c_adapter *adapter = &lspcon->aux->ddc;
> >> +
> >> +	err = drm_lspcon_get_mode(adapter, &current_mode);
> >> +	if (err) {
> >> +		DRM_ERROR("Error reading LSPCON mode\n");
> >> +		return err;
> >> +	}
> >> +
> >> +	if (current_mode == mode && !force) {
> >> +		DRM_DEBUG_KMS("Current mode = desired LSPCON mode\n");
> >> +		return 0;
> >> +	}
> > We always call this with force==true, so it looks like a pointless
> > parameter. Also if force==true always, it doesn't necessary to even
> > query the current mode. Oh and I think we've already queried the mode
> > when this gets called.
> As mentioned, I wanted to keep the scope of switching lspcon modes, in 
> the design infrastructure.
> So I kept these things as possibility.

If I can't see any users I can't possibly judge whether it's sufficient
or not. Move any infrastructure to the patches that need it.

> 
> IF we create a DRM property to query LSPCON mode, that get_property 
> function will call this
> function with force = false, coz for this query we dont want to probe 
> the HW again.
> >> +
> >> +	err = drm_lspcon_set_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)
> > static
> got it.
> >
> >> +{
> >> +	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;
> >> +}
> > This is small function and called only once, so I'd just inline it into
> > the caller actually.
> Can be done, but the caller is intelI_ddi_init, which I dont want to 
> populate with lspcon specific code, when we have a separate file
> for lspcon. Do you think its a good idea ? or you prefer to have that in 
> ddi_init only ?

The caller is lspcon_probe().

> >> +
> >> +enum drm_lspcon_mode lspcon_probe(struct intel_lspcon *lspcon)
> > static
> Got it.
> >
> >> +{
> >> +
> >> +	/* 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 */
> >> +	lspcon->mode_of_op = lspcon_get_current_mode(lspcon);
> >> +	lspcon->active = true;
> >> +	return true;
> >> +}
> >> +
> >> +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;
> >> +	}
> > Just remove this check? If it's not present, we won't detect it.
> Faulty VBT ? Just being paranoid.

Can't be since we don't even parse that part from the VBT on other
platforms.

> >> +
> >> +	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) {
> > indent fail.
> same as previous.
> 
> Regards
> Shashank
> >> +			DRM_ERROR("LSPCON mode change to PCON failed\n");
> >> +			return false;
> >> +		}
> >> +	}
> >> +
> >> +	DRM_DEBUG_KMS("Success: LSPCON init\n");
> >> +	return true;
> >> +}
> >> -- 
> >> 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