[PATCH 0/6] HDMI 2.0: Scrambling support in DRM layer

[PATCH 0/6] HDMI 2.0: Scrambling support in DRM layer

[Shashank Sharma]

HDMI 2.0 spec defines a method to reduce the RF footprint while
operating at higher pixel clocks, which is called Scrambling.
Scrambling can be controlled over a new set of I2C registers
which are accessible over existing DDC I2C lines, called SCDC
register set.

This patch series contains 6 patches:
- First two patches add generic drm helper functions to read and
  write into SCDC registers. These patches are written by Thierry,
  in a patch series published here:
  https://patchwork.kernel.org/patch/9459051/
  Minor changes were done to map the patches into this series.
- Next two patches add functions for scrambling detection and
  scrambling control.
- Next two patches use this infrastructure in DRM layer from
  I915 driver, to enable scrambling on a GLK deivce which sports
  a native HDMI 2.0 controller.

Shashank Sharma (4):
  drm/edid: detect SCDC support in HF-VSDB
  drm: scrambling support in drm layer
  drm/i915: enable scrambling
  drm/i915: allow HDMI 2.0 clock rates

Thierry Reding (2):
  drm: Add SCDC helpers
  drm/edid: check for HF-VSDB block

 Documentation/gpu/drm-kms-helpers.rst |  12 +++
 drivers/gpu/drm/Makefile              |   3 +-
 drivers/gpu/drm/drm_edid.c            | 158 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_scdc_helper.c     | 111 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h       |   2 +
 drivers/gpu/drm/i915/intel_ddi.c      |   5 ++
 drivers/gpu/drm/i915/intel_drv.h      |   2 +
 drivers/gpu/drm/i915/intel_hdmi.c     |  44 ++++++++++
 include/drm/drm_connector.h           |  50 +++++++++++
 include/drm/drm_edid.h                |   6 +-
 include/drm/drm_scdc_helper.h         | 132 ++++++++++++++++++++++++++++
 include/linux/hdmi.h                  |   1 +
 12 files changed, 524 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_scdc_helper.c
 create mode 100644 include/drm/drm_scdc_helper.h

-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/6] drm: Add SCDC helpers

[Shashank Sharma]

From: Thierry Reding <treding@nvidia.com>

SCDC is a mechanism defined in the HDMI 2.0 specification that allows
the source and sink devices to communicate.

This commit introduces helpers to access the SCDC and provides the
symbolic names for the various registers defined in the specification.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 Documentation/gpu/drm-kms-helpers.rst |  12 ++++
 drivers/gpu/drm/Makefile              |   3 +-
 drivers/gpu/drm/drm_scdc_helper.c     | 111 ++++++++++++++++++++++++++++
 include/drm/drm_scdc_helper.h         | 132 ++++++++++++++++++++++++++++++++++
 4 files changed, 257 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_scdc_helper.c
 create mode 100644 include/drm/drm_scdc_helper.h

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 03040aa..7592756 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -217,6 +217,18 @@ EDID Helper Functions Reference
 .. kernel-doc:: drivers/gpu/drm/drm_edid.c
    :export:
 
+SCDC Helper Functions Reference
+===============================
+
+.. kernel-doc:: drivers/gpu/drm/drm_scdc_helper.c
+   :doc: scdc helpers
+
+.. kernel-doc:: include/drm/drm_scdc_helper.h
+   :internal:
+
+.. kernel-doc:: drivers/gpu/drm/drm_scdc_helper.c
+   :export:
+
 Rectangle Utilities Reference
 =============================
 
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 92de399..ad91cd9 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -31,7 +31,8 @@ drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
 drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
 		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
 		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
-		drm_simple_kms_helper.o drm_modeset_helper.o
+		drm_simple_kms_helper.o drm_modeset_helper.o \
+		drm_scdc_helper.o
 
 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
diff --git a/drivers/gpu/drm/drm_scdc_helper.c b/drivers/gpu/drm/drm_scdc_helper.c
new file mode 100644
index 0000000..fe0e121
--- /dev/null
+++ b/drivers/gpu/drm/drm_scdc_helper.c
@@ -0,0 +1,111 @@
+/*
+ * Copyright (c) 2015 NVIDIA Corporation. All rights reserved.
+ *
+ * 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, sub license,
+ * 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 NON-INFRINGEMENT. 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 <linux/slab.h>
+
+#include <drm/drm_scdc_helper.h>
+
+/**
+ * DOC: scdc helpers
+ *
+ * Status and Control Data Channel (SCDC) is a mechanism introduced by the
+ * HDMI 2.0 specification. It is a point-to-point protocol that allows the
+ * HDMI source and HDMI sink to exchange data. The same I2C interface that
+ * is used to access EDID serves as the transport mechanism for SCDC.
+ */
+
+#define SCDC_I2C_SLAVE_ADDRESS 0x54
+
+/**
+ * drm_scdc_read - read a block of data from SCDC
+ * @adapter: I2C controller
+ * @offset: start offset of block to read
+ * @buffer: return location for the block to read
+ * @size: size of the block to read
+ *
+ * Reads a block of data from SCDC, starting at a given offset.
+ *
+ * Returns:
+ * The number of bytes read from SCDC or a negative error code on failure.
+ */
+ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
+		      size_t size)
+{
+	struct i2c_msg msgs[2] = {
+		{
+			.addr = SCDC_I2C_SLAVE_ADDRESS,
+			.flags = 0,
+			.len = 1,
+			.buf = &offset,
+		}, {
+			.addr = SCDC_I2C_SLAVE_ADDRESS,
+			.flags = I2C_M_RD,
+			.len = size,
+			.buf = buffer,
+		}
+	};
+
+	return i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
+}
+EXPORT_SYMBOL(drm_scdc_read);
+
+/**
+ * drm_scdc_write - write a block of data to SCDC
+ * @adapter: I2C controller
+ * @offset: start offset of block to write
+ * @buffer: block of data to write
+ * @size: size of the block to write
+ *
+ * Writes a block of data to SCDC, starting at a given offset.
+ *
+ * Returns:
+ * The number of bytes written to SCDC or a negative error code on failure.
+ */
+ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
+		       const void *buffer, size_t size)
+{
+	struct i2c_msg msg = {
+		.addr = SCDC_I2C_SLAVE_ADDRESS,
+		.flags = 0,
+		.len = 1 + size,
+		.buf = NULL,
+	};
+	void *data;
+	int err;
+
+	data = kmalloc(1 + size, GFP_TEMPORARY);
+	if (!data)
+		return -ENOMEM;
+
+	msg.buf = data;
+
+	memcpy(data, &offset, sizeof(offset));
+	memcpy(data + 1, buffer, size);
+
+	err = i2c_transfer(adapter, &msg, 1);
+
+	kfree(data);
+
+	return err;
+}
+EXPORT_SYMBOL(drm_scdc_write);
diff --git a/include/drm/drm_scdc_helper.h b/include/drm/drm_scdc_helper.h
new file mode 100644
index 0000000..93b07bc
--- /dev/null
+++ b/include/drm/drm_scdc_helper.h
@@ -0,0 +1,132 @@
+/*
+ * Copyright (c) 2015 NVIDIA Corporation. All rights reserved.
+ *
+ * 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, sub license,
+ * 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 NON-INFRINGEMENT. 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.
+ */
+
+#ifndef DRM_SCDC_HELPER_H
+#define DRM_SCDC_HELPER_H
+
+#include <linux/i2c.h>
+#include <linux/types.h>
+
+#define SCDC_SINK_VERSION 0x01
+
+#define SCDC_SOURCE_VERSION 0x02
+
+#define SCDC_UPDATE_0 0x10
+#define  SCDC_READ_REQUEST_TEST (1 << 2)
+#define  SCDC_CED_UPDATE (1 << 1)
+#define  SCDC_STATUS_UPDATE (1 << 0)
+
+#define SCDC_UPDATE_1 0x11
+
+#define SCDC_TMDS_CONFIG 0x20
+#define  SCDC_TMDS_BIT_CLOCK_RATIO_BY_40 (1 << 1)
+#define  SCDC_TMDS_BIT_CLOCK_RATIO_BY_10 (0 << 1)
+#define  SCDC_SCRAMBLING_ENABLE (1 << 0)
+
+#define SCDC_SCRAMBLER_STATUS 0x21
+#define  SCDC_SCRAMBLING_STATUS (1 << 0)
+
+#define SCDC_CONFIG_0 0x30
+#define  SCDC_READ_REQUEST_ENABLE (1 << 0)
+
+#define SCDC_STATUS_FLAGS_0 0x40
+#define  SCDC_CH2_LOCK (1 < 3)
+#define  SCDC_CH1_LOCK (1 < 2)
+#define  SCDC_CH0_LOCK (1 < 1)
+#define  SCDC_CH_LOCK_MASK (SCDC_CH2_LOCK | SCDC_CH1_LOCK | SCDC_CH0_LOCK)
+#define  SCDC_CLOCK_DETECT (1 << 0)
+
+#define SCDC_STATUS_FLAGS_1 0x41
+
+#define SCDC_ERR_DET_0_L 0x50
+#define SCDC_ERR_DET_0_H 0x51
+#define SCDC_ERR_DET_1_L 0x52
+#define SCDC_ERR_DET_1_H 0x53
+#define SCDC_ERR_DET_2_L 0x54
+#define SCDC_ERR_DET_2_H 0x55
+#define  SCDC_CHANNEL_VALID (1 << 7)
+
+#define SCDC_ERR_DET_CHECKSUM 0x56
+
+#define SCDC_TEST_CONFIG_0 0xc0
+#define  SCDC_TEST_READ_REQUEST (1 << 7)
+#define  SCDC_TEST_READ_REQUEST_DELAY(x) ((x) & 0x7f)
+
+#define SCDC_MANUFACTURER_IEEE_OUI 0xd0
+#define SCDC_MANUFACTURER_IEEE_OUI_SIZE 3
+
+#define SCDC_DEVICE_ID 0xd3
+#define SCDC_DEVICE_ID_SIZE 8
+
+#define SCDC_DEVICE_HARDWARE_REVISION 0xdb
+#define  SCDC_DEVICE_HARDWARE_REVISION_MAJOR(x) (((x) >> 4) & 0xf)
+#define  SCDC_DEVICE_HARDWARE_REVISION_MINOR(x) (((x) >> 0) & 0xf)
+
+#define SCDC_DEVICE_SOFTWARE_MAJOR_REVISION 0xdc
+#define SCDC_DEVICE_SOFTWARE_MINOR_REVISION 0xdd
+
+#define SCDC_MANUFACTURER_SPECIFIC 0xde
+#define SCDC_MANUFACTURER_SPECIFIC_SIZE 34
+
+ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
+		      size_t size);
+ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
+		       const void *buffer, size_t size);
+
+/**
+ * drm_scdc_readb - read a single byte from SCDC
+ * @adapter: I2C adapter
+ * @offset: offset of register to read
+ * @value: return location for the register value
+ *
+ * Reads a single byte from SCDC. This is a convenience wrapper around the
+ * drm_scdc_read() function.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+static inline int drm_scdc_readb(struct i2c_adapter *adapter, u8 offset,
+				 u8 *value)
+{
+	return drm_scdc_read(adapter, offset, value, sizeof(*value));
+}
+
+/**
+ * drm_scdc_writeb - write a single byte to SCDC
+ * @adapter: I2C adapter
+ * @offset: offset of register to read
+ * @value: return location for the register value
+ *
+ * Writes a single byte to SCDC. This is a convenience wrapper around the
+ * drm_scdc_write() function.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+static inline int drm_scdc_writeb(struct i2c_adapter *adapter, u8 offset,
+				  u8 value)
+{
+	return drm_scdc_write(adapter, offset, &value, sizeof(value));
+}
+
+#endif
-- 
1.9.1

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

[PATCH 2/6] drm/edid: check for HF-VSDB block

[Shashank Sharma]

From: Thierry Reding <treding@nvidia.com>

This patch implements a small function that finds if a
given CEA db is hdmi-forum vendor specific data block
or not.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_edid.c | 15 +++++++++++++++
 include/linux/hdmi.h       |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index baa6ccb..96d3e47 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3239,6 +3239,21 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
 	return hdmi_id == HDMI_IEEE_OUI;
 }
 
+static bool cea_db_is_hdmi_forum_vsdb(const u8 *db)
+{
+	unsigned int oui;
+
+	if (cea_db_tag(db) != VENDOR_BLOCK)
+		return false;
+
+	if (cea_db_payload_len(db) < 7)
+		return false;
+
+	oui = db[3] << 16 | db[2] << 8 | db[1];
+
+	return oui == HDMI_FORUM_IEEE_OUI;
+}
+
 #define for_each_cea_db(cea, i, start, end) \
 	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
 
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index edbb4fc..d271ff2 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -35,6 +35,7 @@ enum hdmi_infoframe_type {
 };
 
 #define HDMI_IEEE_OUI 0x000c03
+#define HDMI_FORUM_IEEE_OUI 0xc45dd8
 #define HDMI_INFOFRAME_HEADER_SIZE  4
 #define HDMI_AVI_INFOFRAME_SIZE    13
 #define HDMI_SPD_INFOFRAME_SIZE    25
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/6] drm/edid: detect SCDC support in HF-VSDB

[Shashank Sharma]

This patch does following:
- Adds a new structure (drm_hdmi_info) in drm_display_info.
  This structure will be used to save and indicate if sink
  supports advance HDMI 2.0 features
- Checks the HF-VSDB block for presence of SCDC, and marks it
  in hdmi_info structure.
- If SCDC is present, checks if sink is capable of generating
  scdc read request, and marks it in hdmi_info structure.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 14 ++++++++++++++
 include/drm/drm_connector.h | 26 ++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 96d3e47..37902e5 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3802,6 +3802,18 @@ enum hdmi_quantization_range
 }
 EXPORT_SYMBOL(drm_default_rgb_quant_range);
 
+static void drm_detect_hdmi_scdc(struct drm_connector *connector,
+				 const u8 *hf_vsdb)
+{
+	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi_info;
+
+	if (hf_vsdb[6] & 0x80) {
+		hdmi->scdc_supported = true;
+		if (hf_vsdb[6] & 0x40)
+			hdmi->scdc_rr = true;
+	}
+}
+
 static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
 					   const u8 *hdmi)
 {
@@ -3916,6 +3928,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 
 		if (cea_db_is_hdmi_vsdb(db))
 			drm_parse_hdmi_vsdb_video(connector, db);
+		if (cea_db_is_hdmi_forum_vsdb(db))
+			drm_detect_hdmi_scdc(connector, db);
 	}
 }
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index e5e1edd..2435598 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -87,6 +87,27 @@ enum subpixel_order {
 	SubPixelVerticalRGB,
 	SubPixelVerticalBGR,
 	SubPixelNone,
+
+};
+
+/**
+ * struct drm_hdmi_info - runtime data about the connected sink
+ *
+ * Describes if a given hdmi display supports advance HDMI 2.0 featutes.
+ * This information is available in CEA-861-F extension blocks (like
+ * HF-VSDB)
+ * For sinks which provide an EDID this can be filled out by calling
+ * drm_add_edid_modes().
+ */
+struct drm_hdmi_info {
+	/**
+	 * @scdc_supported: status control & data channel present.
+	 */
+	bool scdc_supported;
+	/**
+	 * @scdc_rr: sink is capable of generating scdc read request.
+	 */
+	bool scdc_rr;
 };
 
 /**
@@ -188,6 +209,11 @@ struct drm_display_info {
 	 * @cea_rev: CEA revision of the HDMI sink.
 	 */
 	u8 cea_rev;
+
+	/**
+	 * @hdmi_info: advance features of a HDMI sink.
+	 */
+	struct drm_hdmi_info hdmi_info;
 };
 
 int drm_display_info_set_bus_formats(struct drm_display_info *info,
-- 
1.9.1

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

[PATCH 4/6] drm: scrambling support in drm layer

[Shashank Sharma]

HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
than 340Mhz. This patch adds few new functions in drm layer for
core drivers to enable/disable scrambling.

This patch adds:
- A function to detect scrambling support parsing HF-VSDB
- A function to check scrambling status runtime using SCDC read.
- Two functions to enable/disable scrambling using SCDC read/write.
- Few new bools to reflect scrambling support and status.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 131 +++++++++++++++++++++++++++++++++++++++++++-
 include/drm/drm_connector.h |  24 ++++++++
 include/drm/drm_edid.h      |   6 +-
 3 files changed, 159 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 37902e5..f0d940a 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -37,6 +37,7 @@
 #include <drm/drm_edid.h>
 #include <drm/drm_encoder.h>
 #include <drm/drm_displayid.h>
+#include <drm/drm_scdc_helper.h>
 
 #define version_greater(edid, maj, min) \
 	(((edid)->version > (maj)) || \
@@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector *connector,
 	}
 }
 
+static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
+				 const u8 *hf_vsdb)
+{
+	struct drm_display_info *display = &connector->display_info;
+	struct drm_hdmi_info *hdmi = &display->hdmi_info;
+
+	/*
+	 * All HDMI 2.0 monitors must support scrambling at rates > 340M.
+	 * And as per the spec, three factors confirm this:
+	 * * Availability of a HF-VSDB block in EDID (check)
+	 * * Non zero Max_TMDS_Char_Rate filed in HF-VSDB
+	 * * SCDC support available
+	 * Lets check it out.
+	 */
+
+	if (hf_vsdb[5]) {
+		display->max_tmds_clock = hf_vsdb[5] * 5000;
+		DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
+			display->max_tmds_clock);
+
+		if (hdmi->scdc_supported) {
+			hdmi->scr_info.supported = true;
+
+			/* Few sinks support scrambling for cloks < 340M */
+			if ((hf_vsdb[6] & 0x8))
+				hdmi->scr_info.low_clocks = true;
+		}
+	}
+}
+
+/**
+ * drm_check_scrambling_status - what is status of scrambling?
+ * @adapter: i2c adapter for SCDC channel
+ *
+ * Read the scrambler status over SCDC channel, and check the
+ * scrambling status.
+ *
+ * Return: True if the scrambling is enabled, false otherwise.
+ */
+
+bool drm_check_scrambling_status(struct i2c_adapter *adapter)
+{
+	u8 status;
+
+	if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) {
+		DRM_ERROR("Failed to read scrambling status\n");
+		return false;
+	}
+
+	status &= SCDC_SCRAMBLING_STATUS;
+	return status != 0;
+}
+
+/**
+ * drm_enable_scrambling - enable scrambling
+ * @connector: target drm_connector
+ * @adapter: i2c adapter for SCDC channel
+ * @force: enable scrambling, even if its already enabled
+ *
+ * Write the TMDS config over SCDC channel, and enable scrambling
+ * Return: True if scrambling is successfully enabled, false otherwise.
+ */
+
+bool drm_enable_scrambling(struct drm_connector *connector,
+					struct i2c_adapter *adapter, bool force)
+{
+	u8 config;
+	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
+
+	if (hdmi_info->scr_info.status && !force) {
+		DRM_DEBUG_KMS("Scrambling already enabled\n");
+		return true;
+	}
+
+	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
+		DRM_ERROR("Failed to read tmds config\n");
+		return false;
+	}
+
+	config |= SCDC_SCRAMBLING_ENABLE;
+
+	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
+		DRM_ERROR("Failed to enable scrambling, write error\n");
+		return false;
+	}
+
+	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
+	return hdmi_info->scr_info.status;
+}
+
+/**
+ * drm_disable_scrambling - disable scrambling
+ * @connector: target drm_connector
+ * @adapter: i2c adapter for SCDC channel
+ * @force: disable scrambling, even if its already disabled
+ *
+ * Write the TMDS config over SCDC channel, and disable scrambling
+ * Return: True if scrambling is successfully disabled, false otherwise.
+ */
+bool drm_disable_scrambling(struct drm_connector *connector,
+					struct i2c_adapter *adapter, bool force)
+{
+	u8 config;
+	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
+
+	if (!hdmi_info->scr_info.status && !force) {
+		DRM_DEBUG_KMS("Scrambling already disabled\n");
+		return true;
+	}
+
+	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
+		DRM_ERROR("Failed to read tmds config\n");
+		return false;
+	}
+
+	config &= ~SCDC_SCRAMBLING_ENABLE;
+
+	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
+		DRM_ERROR("Failed to enable scrambling, write error\n");
+		return false;
+	}
+
+	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
+	return !hdmi_info->scr_info.status;
+}
+
 static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
 					   const u8 *hdmi)
 {
@@ -3928,8 +4055,10 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 
 		if (cea_db_is_hdmi_vsdb(db))
 			drm_parse_hdmi_vsdb_video(connector, db);
-		if (cea_db_is_hdmi_forum_vsdb(db))
+		if (cea_db_is_hdmi_forum_vsdb(db)) {
 			drm_detect_hdmi_scdc(connector, db);
+			drm_detect_hdmi_scrambling(connector, db);
+		}
 	}
 }
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 2435598..b9735bd 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -90,6 +90,26 @@ enum subpixel_order {
 
 };
 
+
+/**
+ * struct scrambling: sink's scrambling support.
+ */
+struct scrambling {
+	/**
+	 * @supported: scrambling supported for rates > 340Mhz.
+	 */
+	bool supported;
+	/**
+	 * @low_clocks: scrambling supported for rates <= 340Mhz.
+	 */
+	bool low_clocks;
+	/**
+	 * @status: scrambling enabled/disabled ?
+	 */
+	bool status;
+};
+
+
 /**
  * struct drm_hdmi_info - runtime data about the connected sink
  *
@@ -108,6 +128,10 @@ struct drm_hdmi_info {
 	 * @scdc_rr: sink is capable of generating scdc read request.
 	 */
 	bool scdc_rr;
+	/**
+	 * scr_info: sink's scrambling support and capabilities.
+	 */
+	struct scrambling scr_info;
 };
 
 /**
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 43fb0ac..d24c974 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -462,5 +462,9 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
 struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
 					   int hsize, int vsize, int fresh,
 					   bool rb);
-
+bool drm_enable_scrambling(struct drm_connector *connector,
+				struct i2c_adapter *adapter, bool force);
+bool drm_disable_scrambling(struct drm_connector *connector,
+				struct i2c_adapter *adapter, bool force);
+bool drm_check_scrambling_status(struct i2c_adapter *adapter);
 #endif /* __DRM_EDID_H__ */
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/6] drm/i915: enable scrambling

[Shashank Sharma]

Geminilake platform has a native HDMI 2.0 controller, and is
capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
mendates scrambling for these higher clocks, for reduced RF footprint.

This patch checks if the monitor supports scrambling, and if required,
enables it during the modeset.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h   |  2 ++
 drivers/gpu/drm/i915/intel_ddi.c  |  5 +++++
 drivers/gpu/drm/i915/intel_drv.h  |  2 ++
 drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 495b789..cc85892 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7807,6 +7807,8 @@ enum {
 #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
 #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
 #define  TRANS_DDI_BFI_ENABLE		(1<<4)
+#define  TRANS_DDI_HIGH_TMDS_CHAR_RATE	(1<<4)
+#define  TRANS_DDI_HDMI_SCRAMBLING	(1<<0)
 
 /* DisplayPort Transport Control */
 #define _DP_TP_CTL_A			0x64040
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 9a9a670..aea81ce 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1278,6 +1278,11 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
 			temp |= TRANS_DDI_MODE_SELECT_HDMI;
 		else
 			temp |= TRANS_DDI_MODE_SELECT_DVI;
+
+		if (IS_GEMINILAKE(dev_priv)) {
+			temp |= intel_hdmi_check_scrambling(intel_encoder,
+				&intel_crtc->config->base.adjusted_mode);
+		}
 	} else if (type == INTEL_OUTPUT_ANALOG) {
 		temp |= TRANS_DDI_MODE_SELECT_FDI;
 		temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 393f243..aafce7f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1588,6 +1588,8 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 			       struct intel_crtc_state *pipe_config,
 			       struct drm_connector_state *conn_state);
+uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder,
+				struct drm_display_mode *mode);
 void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
 
 
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index ebae2bd..92dd9bc 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1795,6 +1795,48 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
 	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
 }
 
+static void
+intel_hdmi_enable_scrambling(struct drm_connector *connector)
+{
+	struct drm_i915_private *dev_priv = connector->dev->dev_private;
+	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
+	struct i2c_adapter *adapter =
+			intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
+
+	if (!drm_enable_scrambling(connector, adapter, true))
+		DRM_ERROR("Request to enable scrambling failed\n");
+}
+
+uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder,
+				struct drm_display_mode *mode)
+{
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
+	struct drm_connector *connector = &intel_hdmi->attached_connector->base;
+	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
+	uint32_t hdmi_config = 0;
+
+	DRM_DEBUG_KMS("Checking scrambling for enc:%s connector:%s\n",
+			intel_encoder->base.name, connector->name);
+
+	if (mode->clock < 340000) {
+		if (hdmi_info->scr_info.low_clocks) {
+			intel_hdmi_enable_scrambling(connector);
+			hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING;
+		}
+		return hdmi_config;
+	}
+
+	/* Enable scrambling for clocks > 340M */
+	if (hdmi_info->scr_info.supported) {
+		intel_hdmi_enable_scrambling(connector);
+		hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING;
+	}
+
+	/* Scrambling or not, if clock > 340M, set high char rate */
+	hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
+	return hdmi_config;
+}
+
 static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
 			     enum port port)
 {
-- 
1.9.1

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

[PATCH 6/6] drm/i915: allow HDMI 2.0 clock rates

[Shashank Sharma]

Geminilake has a native HDMI 2.0 controller, which is capable of
driving clocks upto 594Mhz. This patch updates the max tmds clock
limit for the same.

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

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 92dd9bc..2f9e840 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1209,6 +1209,8 @@ static int intel_hdmi_source_max_tmds_clock(struct drm_i915_private *dev_priv)
 {
 	if (IS_G4X(dev_priv))
 		return 165000;
+	else if (IS_GEMINILAKE(dev_priv))
+		return 594000;
 	else if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
 		return 300000;
 	else
-- 
1.9.1

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

✗ Fi.CI.BAT: failure for HDMI 2.0: Scrambling support in DRM layer

[Patchwork]

== Series Details ==

Series: HDMI 2.0: Scrambling support in DRM layer
URL   : https://patchwork.freedesktop.org/series/18915/
State : failure

== Summary ==

  CHECK   usr/include/linux/byteorder/ (2 files)
  CHECK   usr/include/linux/android/ (1 files)
  CHECK   usr/include/linux/caif/ (2 files)
  CHECK   usr/include/linux/iio/ (2 files)
  CHECK   usr/include/linux/netfilter_ipv4/ (9 files)
  CHECK   usr/include/linux/netfilter_bridge/ (17 files)
  CHECK   usr/include/linux/can/ (5 files)
  CHECK   usr/include/linux/raid/ (2 files)
  CHECK   usr/include/linux/nfsd/ (5 files)
  CHECK   usr/include/linux/hdlc/ (1 files)
  CHECK   usr/include/linux/isdn/ (1 files)
  CHECK   usr/include/linux/netfilter/ (87 files)
  CHECK   usr/include/linux/tc_act/ (14 files)
  CHECK   usr/include/linux/netfilter_arp/ (2 files)
  CHECK   usr/include/linux/hsi/ (2 files)
  CHECK   usr/include/linux/tc_ematch/ (4 files)
  CHECK   usr/include/linux/mmc/ (1 files)
  CHECK   usr/include/linux/dvb/ (8 files)
  CHECK   usr/include/linux/sunrpc/ (1 files)
  CHECK   usr/include/linux/wimax/ (1 files)
  CHECK   usr/include/linux/spi/ (1 files)
  CHECK   usr/include/linux/netfilter/ipset/ (4 files)
  CHECK   usr/include/linux/usb/ (11 files)
  CHECK   usr/include/linux/ (443 files)
  CHECK   usr/include/linux/netfilter_ipv6/ (12 files)
  CHECK   usr/include/asm/ (62 files)
  GEN     .version
  CHK     include/generated/compile.h
  UPD     include/generated/compile.h
  CC      init/version.o
  LD      init/built-in.o
  LD      vmlinux.o
  MODPOST vmlinux.o
  KSYM    .tmp_kallsyms1.o
  KSYM    .tmp_kallsyms2.o
  LD      vmlinux
  SORTEX  vmlinux
  SYSMAP  System.map
  CC      arch/x86/boot/a20.o
  AS      arch/x86/boot/bioscall.o
  CC      arch/x86/boot/cmdline.o
  HOSTCC  arch/x86/boot/mkcpustr
  CC      arch/x86/boot/early_serial_console.o
  CC      arch/x86/boot/cpuflags.o
  CC      arch/x86/boot/cpucheck.o
  AS      arch/x86/boot/copy.o
  CC      arch/x86/boot/edd.o
  CC      arch/x86/boot/main.o
  AS      arch/x86/boot/pmjump.o
  CC      arch/x86/boot/printf.o
  CC      arch/x86/boot/memory.o
  CC      arch/x86/boot/regs.o
  CC      arch/x86/boot/string.o
  CC      arch/x86/boot/pm.o
  CC      arch/x86/boot/video.o
  CC      arch/x86/boot/version.o
  CC      arch/x86/boot/video-vesa.o
  CC      arch/x86/boot/video-vga.o
  CC      arch/x86/boot/video-mode.o
  CC      arch/x86/boot/video-bios.o
  CC      arch/x86/boot/tty.o
  HOSTCC  arch/x86/boot/tools/build
  Building modules, stage 2.
  LDS     arch/x86/boot/compressed/vmlinux.lds
  AS      arch/x86/boot/compressed/head_64.o
  VOFFSET arch/x86/boot/compressed/../voffset.h
  CPUSTR  arch/x86/boot/cpustr.h
  CC      arch/x86/boot/compressed/string.o
  MODPOST 97 modules
  CC      arch/x86/boot/compressed/cmdline.o
  CC      arch/x86/boot/compressed/error.o
  OBJCOPY arch/x86/boot/compressed/vmlinux.bin
  HOSTCC  arch/x86/boot/compressed/mkpiggy
  CC      arch/x86/boot/compressed/early_serial_console.o
  CC      arch/x86/boot/compressed/eboot.o
  AS      arch/x86/boot/compressed/efi_stub_64.o
  CC      arch/x86/boot/cpu.o
  CC      arch/x86/boot/compressed/cpuflags.o
  XZKERN  arch/x86/boot/compressed/vmlinux.bin.xz
ERROR: "drm_enable_scrambling" [drivers/gpu/drm/i915/i915.ko] undefined!
scripts/Makefile.modpost:91: recipe for target '__modpost' failed
make[1]: *** [__modpost] Error 1
Makefile:1196: recipe for target 'modules' failed
make: *** [modules] Error 2
make: *** Waiting for unfinished jobs....
  CC      arch/x86/boot/compressed/misc.o
  MKPIGGY arch/x86/boot/compressed/piggy.S
  AS      arch/x86/boot/compressed/piggy.o
  DATAREL arch/x86/boot/compressed/vmlinux
  LD      arch/x86/boot/compressed/vmlinux
  ZOFFSET arch/x86/boot/zoffset.h
  OBJCOPY arch/x86/boot/vmlinux.bin
  AS      arch/x86/boot/header.o
  LD      arch/x86/boot/setup.elf
  OBJCOPY arch/x86/boot/setup.bin
  BUILD   arch/x86/boot/bzImage
Setup is 16412 bytes (padded to 16896 bytes).
System is 4853 kB
CRC e574db06
Kernel: arch/x86/boot/bzImage is ready  (#1)

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

Re: ✗ Fi.CI.BAT: failure for HDMI 2.0: Scrambling support in DRM layer

[Sharma, Shashank]

I build bzImage, keeping I915 as part of kernel (not as a module)
Looks like I need to add EXPORT_SYMBOL(drm_enable_scrambling) 
Will cover this in V2. 

Regards
Shashank
-----Original Message-----
From: Patchwork [mailto:patchwork@emeril.freedesktop.org] 
Sent: Wednesday, February 1, 2017 6:33 PM
To: Sharma, Shashank <shashank.sharma@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: ✗ Fi.CI.BAT: failure for HDMI 2.0: Scrambling support in DRM layer

== Series Details ==

Series: HDMI 2.0: Scrambling support in DRM layer
URL   : https://patchwork.freedesktop.org/series/18915/
State : failure

== Summary ==

  CHECK   usr/include/linux/byteorder/ (2 files)
  CHECK   usr/include/linux/android/ (1 files)
  CHECK   usr/include/linux/caif/ (2 files)
  CHECK   usr/include/linux/iio/ (2 files)
  CHECK   usr/include/linux/netfilter_ipv4/ (9 files)
  CHECK   usr/include/linux/netfilter_bridge/ (17 files)
  CHECK   usr/include/linux/can/ (5 files)
  CHECK   usr/include/linux/raid/ (2 files)
  CHECK   usr/include/linux/nfsd/ (5 files)
  CHECK   usr/include/linux/hdlc/ (1 files)
  CHECK   usr/include/linux/isdn/ (1 files)
  CHECK   usr/include/linux/netfilter/ (87 files)
  CHECK   usr/include/linux/tc_act/ (14 files)
  CHECK   usr/include/linux/netfilter_arp/ (2 files)
  CHECK   usr/include/linux/hsi/ (2 files)
  CHECK   usr/include/linux/tc_ematch/ (4 files)
  CHECK   usr/include/linux/mmc/ (1 files)
  CHECK   usr/include/linux/dvb/ (8 files)
  CHECK   usr/include/linux/sunrpc/ (1 files)
  CHECK   usr/include/linux/wimax/ (1 files)
  CHECK   usr/include/linux/spi/ (1 files)
  CHECK   usr/include/linux/netfilter/ipset/ (4 files)
  CHECK   usr/include/linux/usb/ (11 files)
  CHECK   usr/include/linux/ (443 files)
  CHECK   usr/include/linux/netfilter_ipv6/ (12 files)
  CHECK   usr/include/asm/ (62 files)
  GEN     .version
  CHK     include/generated/compile.h
  UPD     include/generated/compile.h
  CC      init/version.o
  LD      init/built-in.o
  LD      vmlinux.o
  MODPOST vmlinux.o
  KSYM    .tmp_kallsyms1.o
  KSYM    .tmp_kallsyms2.o
  LD      vmlinux
  SORTEX  vmlinux
  SYSMAP  System.map
  CC      arch/x86/boot/a20.o
  AS      arch/x86/boot/bioscall.o
  CC      arch/x86/boot/cmdline.o
  HOSTCC  arch/x86/boot/mkcpustr
  CC      arch/x86/boot/early_serial_console.o
  CC      arch/x86/boot/cpuflags.o
  CC      arch/x86/boot/cpucheck.o
  AS      arch/x86/boot/copy.o
  CC      arch/x86/boot/edd.o
  CC      arch/x86/boot/main.o
  AS      arch/x86/boot/pmjump.o
  CC      arch/x86/boot/printf.o
  CC      arch/x86/boot/memory.o
  CC      arch/x86/boot/regs.o
  CC      arch/x86/boot/string.o
  CC      arch/x86/boot/pm.o
  CC      arch/x86/boot/video.o
  CC      arch/x86/boot/version.o
  CC      arch/x86/boot/video-vesa.o
  CC      arch/x86/boot/video-vga.o
  CC      arch/x86/boot/video-mode.o
  CC      arch/x86/boot/video-bios.o
  CC      arch/x86/boot/tty.o
  HOSTCC  arch/x86/boot/tools/build
  Building modules, stage 2.
  LDS     arch/x86/boot/compressed/vmlinux.lds
  AS      arch/x86/boot/compressed/head_64.o
  VOFFSET arch/x86/boot/compressed/../voffset.h
  CPUSTR  arch/x86/boot/cpustr.h
  CC      arch/x86/boot/compressed/string.o
  MODPOST 97 modules
  CC      arch/x86/boot/compressed/cmdline.o
  CC      arch/x86/boot/compressed/error.o
  OBJCOPY arch/x86/boot/compressed/vmlinux.bin
  HOSTCC  arch/x86/boot/compressed/mkpiggy
  CC      arch/x86/boot/compressed/early_serial_console.o
  CC      arch/x86/boot/compressed/eboot.o
  AS      arch/x86/boot/compressed/efi_stub_64.o
  CC      arch/x86/boot/cpu.o
  CC      arch/x86/boot/compressed/cpuflags.o
  XZKERN  arch/x86/boot/compressed/vmlinux.bin.xz
ERROR: "drm_enable_scrambling" [drivers/gpu/drm/i915/i915.ko] undefined!
scripts/Makefile.modpost:91: recipe for target '__modpost' failed
make[1]: *** [__modpost] Error 1
Makefile:1196: recipe for target 'modules' failed
make: *** [modules] Error 2
make: *** Waiting for unfinished jobs....
  CC      arch/x86/boot/compressed/misc.o
  MKPIGGY arch/x86/boot/compressed/piggy.S
  AS      arch/x86/boot/compressed/piggy.o
  DATAREL arch/x86/boot/compressed/vmlinux
  LD      arch/x86/boot/compressed/vmlinux
  ZOFFSET arch/x86/boot/zoffset.h
  OBJCOPY arch/x86/boot/vmlinux.bin
  AS      arch/x86/boot/header.o
  LD      arch/x86/boot/setup.elf
  OBJCOPY arch/x86/boot/setup.bin
  BUILD   arch/x86/boot/bzImage
Setup is 16412 bytes (padded to 16896 bytes).
System is 4853 kB
CRC e574db06
Kernel: arch/x86/boot/bzImage is ready  (#1)

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

Re: [PATCH 3/6] drm/edid: detect SCDC support in HF-VSDB

[Thierry Reding]

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

On Wed, Feb 01, 2017 at 06:14:38PM +0530, Shashank Sharma wrote:
> This patch does following:
> - Adds a new structure (drm_hdmi_info) in drm_display_info.
>   This structure will be used to save and indicate if sink
>   supports advance HDMI 2.0 features

"advanced"

> - Checks the HF-VSDB block for presence of SCDC, and marks it
>   in hdmi_info structure.

"drm_hdmi_info structure"?

> - If SCDC is present, checks if sink is capable of generating
>   scdc read request, and marks it in hdmi_info structure.

"SCDC" to be consistent and because it's an abbreviation.

> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 14 ++++++++++++++
>  include/drm/drm_connector.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 96d3e47..37902e5 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3802,6 +3802,18 @@ enum hdmi_quantization_range
>  }
>  EXPORT_SYMBOL(drm_default_rgb_quant_range);
>  
> +static void drm_detect_hdmi_scdc(struct drm_connector *connector,
> +				 const u8 *hf_vsdb)
> +{
> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi_info;
> +
> +	if (hf_vsdb[6] & 0x80) {
> +		hdmi->scdc_supported = true;
> +		if (hf_vsdb[6] & 0x40)
> +			hdmi->scdc_rr = true;
> +	}
> +}
> +
>  static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>  					   const u8 *hdmi)
>  {
> @@ -3916,6 +3928,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  
>  		if (cea_db_is_hdmi_vsdb(db))
>  			drm_parse_hdmi_vsdb_video(connector, db);
> +		if (cea_db_is_hdmi_forum_vsdb(db))
> +			drm_detect_hdmi_scdc(connector, db);
>  	}
>  }
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index e5e1edd..2435598 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -87,6 +87,27 @@ enum subpixel_order {
>  	SubPixelVerticalRGB,
>  	SubPixelVerticalBGR,
>  	SubPixelNone,
> +
> +};
> +
> +/**
> + * struct drm_hdmi_info - runtime data about the connected sink

Maybe "connected HDMI sink"?

> + *
> + * Describes if a given hdmi display supports advance HDMI 2.0 featutes.

"HDMI", "advanced", "features"

> + * This information is available in CEA-861-F extension blocks (like
> + * HF-VSDB)

This should be terminated by a full-stop.

> + * For sinks which provide an EDID this can be filled out by calling
> + * drm_add_edid_modes().

And maybe make this sentence start right after the one above rather than
breaking it to the next line. I'm not sure how useful this line is. Most
driver will be calling drm_add_edid_modes() anyway, but the above makes
it sound like drm_add_edid_modes() is something you have to explicitly
call to get these fields parsed.

> + */
> +struct drm_hdmi_info {
> +	/**
> +	 * @scdc_supported: status control & data channel present.
> +	 */
> +	bool scdc_supported;
> +	/**
> +	 * @scdc_rr: sink is capable of generating scdc read request.
> +	 */
> +	bool scdc_rr;
>  };
>  
>  /**
> @@ -188,6 +209,11 @@ struct drm_display_info {
>  	 * @cea_rev: CEA revision of the HDMI sink.
>  	 */
>  	u8 cea_rev;
> +
> +	/**
> +	 * @hdmi_info: advance features of a HDMI sink.
> +	 */
> +	struct drm_hdmi_info hdmi_info;

I think we can safely drop the _info suffix on the field name. It's
already inside a structure that carries this suffix.

Other than that:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/6] drm: scrambling support in drm layer

[Thierry Reding]

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

On Wed, Feb 01, 2017 at 06:14:39PM +0530, Shashank Sharma wrote:
> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
> than 340Mhz. This patch adds few new functions in drm layer for
> core drivers to enable/disable scrambling.
> 
> This patch adds:
> - A function to detect scrambling support parsing HF-VSDB
> - A function to check scrambling status runtime using SCDC read.
> - Two functions to enable/disable scrambling using SCDC read/write.
> - Few new bools to reflect scrambling support and status.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 131 +++++++++++++++++++++++++++++++++++++++++++-
>  include/drm/drm_connector.h |  24 ++++++++
>  include/drm/drm_edid.h      |   6 +-
>  3 files changed, 159 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 37902e5..f0d940a 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -37,6 +37,7 @@
>  #include <drm/drm_edid.h>
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_displayid.h>
> +#include <drm/drm_scdc_helper.h>
>  
>  #define version_greater(edid, maj, min) \
>  	(((edid)->version > (maj)) || \
> @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector *connector,
>  	}
>  }
>  
> +static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
> +				 const u8 *hf_vsdb)
> +{
> +	struct drm_display_info *display = &connector->display_info;
> +	struct drm_hdmi_info *hdmi = &display->hdmi_info;
> +
> +	/*
> +	 * All HDMI 2.0 monitors must support scrambling at rates > 340M.

In comments below you use Mhz as the abbreviations. This should be
consistent. Also I think "MHz" is actually the correct spelling.

> +	 * And as per the spec, three factors confirm this:
> +	 * * Availability of a HF-VSDB block in EDID (check)
> +	 * * Non zero Max_TMDS_Char_Rate filed in HF-VSDB
> +	 * * SCDC support available
> +	 * Lets check it out.
> +	 */
> +
> +	if (hf_vsdb[5]) {
> +		display->max_tmds_clock = hf_vsdb[5] * 5000;
> +		DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> +			display->max_tmds_clock);
> +
> +		if (hdmi->scdc_supported) {
> +			hdmi->scr_info.supported = true;
> +
> +			/* Few sinks support scrambling for cloks < 340M */
> +			if ((hf_vsdb[6] & 0x8))
> +				hdmi->scr_info.low_clocks = true;
> +		}
> +	}
> +}
> +
> +/**
> + * drm_check_scrambling_status - what is status of scrambling?
> + * @adapter: i2c adapter for SCDC channel

"I2C", same in other parts of this patch.

> + *
> + * Read the scrambler status over SCDC channel, and check the
> + * scrambling status.
> + *
> + * Return: True if the scrambling is enabled, false otherwise.

I think the rest of DRM/KMS kerneldoc tries to use "Returns:\n" as a
standard way to document return values.

> + */
> +
> +bool drm_check_scrambling_status(struct i2c_adapter *adapter)

Maybe use a drm_scdc_*() prefix for this to make it more consistent with
other SCDC API.

While at it, would this not be better located in drm_scdc.c along with
the other helpers? drm_edid.c is more focussed on the parsing aspects of
all things EDID.

> +{
> +	u8 status;
> +
> +	if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) {

How about storing the error code...

> +		DRM_ERROR("Failed to read scrambling status\n");

... and making it part of the error message? Sometimes its useful to
know what exact error triggered this because it helps narrowing down
where things went wrong.

> +		return false;
> +	}
> +
> +	status &= SCDC_SCRAMBLING_STATUS;
> +	return status != 0;

Maybe make this a single line:

	return (status & SCDC_SCRAMBLING_STATUS) != 0;

> +}
> +
> +/**
> + * drm_enable_scrambling - enable scrambling
> + * @connector: target drm_connector

"target DRM connector"?

> + * @adapter: i2c adapter for SCDC channel
> + * @force: enable scrambling, even if its already enabled
> + *
> + * Write the TMDS config over SCDC channel, and enable scrambling
> + * Return: True if scrambling is successfully enabled, false otherwise.
> + */
> +
> +bool drm_enable_scrambling(struct drm_connector *connector,
> +					struct i2c_adapter *adapter, bool force)

I think I'd move this to drm_scdc.c as well because it primarily
operates on SCDC. If you do so, might be worth making adapter the first
argument for consistency with other SCDC API.

> +{
> +	u8 config;
> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
> +
> +	if (hdmi_info->scr_info.status && !force) {
> +		DRM_DEBUG_KMS("Scrambling already enabled\n");
> +		return true;
> +	}
> +
> +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
> +		DRM_ERROR("Failed to read tmds config\n");

Maybe also print the error code?

> +		return false;
> +	}
> +
> +	config |= SCDC_SCRAMBLING_ENABLE;
> +
> +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
> +		DRM_ERROR("Failed to enable scrambling, write error\n");

Same here.

> +		return false;
> +	}
> +
> +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
> +	return hdmi_info->scr_info.status;
> +}
> +
> +/**
> + * drm_disable_scrambling - disable scrambling
> + * @connector: target drm_connector
> + * @adapter: i2c adapter for SCDC channel
> + * @force: disable scrambling, even if its already disabled
> + *
> + * Write the TMDS config over SCDC channel, and disable scrambling
> + * Return: True if scrambling is successfully disabled, false otherwise.
> + */
> +bool drm_disable_scrambling(struct drm_connector *connector,
> +					struct i2c_adapter *adapter, bool force)
> +{
> +	u8 config;
> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
> +
> +	if (!hdmi_info->scr_info.status && !force) {
> +		DRM_DEBUG_KMS("Scrambling already disabled\n");
> +		return true;
> +	}
> +
> +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
> +		DRM_ERROR("Failed to read tmds config\n");
> +		return false;
> +	}
> +
> +	config &= ~SCDC_SCRAMBLING_ENABLE;
> +
> +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
> +		DRM_ERROR("Failed to enable scrambling, write error\n");
> +		return false;
> +	}
> +
> +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
> +	return !hdmi_info->scr_info.status;
> +}

Same comments as for drm_enable_scrambling().

> @@ -3928,8 +4055,10 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  
>  		if (cea_db_is_hdmi_vsdb(db))
>  			drm_parse_hdmi_vsdb_video(connector, db);
> -		if (cea_db_is_hdmi_forum_vsdb(db))
> +		if (cea_db_is_hdmi_forum_vsdb(db)) {
>  			drm_detect_hdmi_scdc(connector, db);
> +			drm_detect_hdmi_scrambling(connector, db);
> +		}
>  	}
>  }
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 2435598..b9735bd 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -90,6 +90,26 @@ enum subpixel_order {
>  
>  };
>  
> +
> +/**
> + * struct scrambling: sink's scrambling support.
> + */
> +struct scrambling {
> +	/**
> +	 * @supported: scrambling supported for rates > 340Mhz.

I think it's common to separate number and unit by a space, so "340
MHz".

> +	 */
> +	bool supported;
> +	/**
> +	 * @low_clocks: scrambling supported for rates <= 340Mhz.
> +	 */
> +	bool low_clocks;

Maybe "low_rates" because a clock is technically the source of a signal
that oscillates at the given rate.

> +	/**
> +	 * @status: scrambling enabled/disabled ?
> +	 */
> +	bool status;
> +};
> +
> +
>  /**
>   * struct drm_hdmi_info - runtime data about the connected sink
>   *
> @@ -108,6 +128,10 @@ struct drm_hdmi_info {
>  	 * @scdc_rr: sink is capable of generating scdc read request.
>  	 */
>  	bool scdc_rr;
> +	/**
> +	 * scr_info: sink's scrambling support and capabilities.
> +	 */
> +	struct scrambling scr_info;

Again, I think I'd drop _info and instead spell out "scrambling" to make
this easier to remember.

Also I'm wondering why scdc_supported and scdc_rr are not in a structure
if scrambling info is. Also since scrambling depends on SCDC, would it
make sense to embed it in a struct drm_hdmi_scdc_info?

	struct drm_hdmi_scdc_scrambling_info {
		bool supported;
		bool low_rates;
		bool status;
	};

	struct drm_hdmi_scdc_info {
		bool supported;
		bool read_request;

		struct drm_hdmi_scdc_scrambling_info scrambling;
	};

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/6] drm: scrambling support in drm layer

[Ville Syrjälä]

On Wed, Feb 01, 2017 at 06:14:39PM +0530, Shashank Sharma wrote:
> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
> than 340Mhz. This patch adds few new functions in drm layer for
> core drivers to enable/disable scrambling.
> 
> This patch adds:
> - A function to detect scrambling support parsing HF-VSDB
> - A function to check scrambling status runtime using SCDC read.
> - Two functions to enable/disable scrambling using SCDC read/write.
> - Few new bools to reflect scrambling support and status.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 131 +++++++++++++++++++++++++++++++++++++++++++-
>  include/drm/drm_connector.h |  24 ++++++++
>  include/drm/drm_edid.h      |   6 +-
>  3 files changed, 159 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 37902e5..f0d940a 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -37,6 +37,7 @@
>  #include <drm/drm_edid.h>
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_displayid.h>
> +#include <drm/drm_scdc_helper.h>
>  
>  #define version_greater(edid, maj, min) \
>  	(((edid)->version > (maj)) || \
> @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector *connector,
>  	}
>  }
>  
> +static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
> +				 const u8 *hf_vsdb)

That names seems off. Should probably be drm_parse_hdmi_forum_vsdb() or
something.

> +{
> +	struct drm_display_info *display = &connector->display_info;
> +	struct drm_hdmi_info *hdmi = &display->hdmi_info;
> +
> +	/*
> +	 * All HDMI 2.0 monitors must support scrambling at rates > 340M.
> +	 * And as per the spec, three factors confirm this:
> +	 * * Availability of a HF-VSDB block in EDID (check)
> +	 * * Non zero Max_TMDS_Char_Rate filed in HF-VSDB
> +	 * * SCDC support available
> +	 * Lets check it out.
> +	 */
> +
> +	if (hf_vsdb[5]) {
> +		display->max_tmds_clock = hf_vsdb[5] * 5000;
> +		DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> +			display->max_tmds_clock);

I wonder if we should be a little paranoid and ignore this if it's
<=340?

> +
> +		if (hdmi->scdc_supported) {
> +			hdmi->scr_info.supported = true;
> +
> +			/* Few sinks support scrambling for cloks < 340M */
> +			if ((hf_vsdb[6] & 0x8))
> +				hdmi->scr_info.low_clocks = true;
> +		}
> +	}
> +}
> +
> +/**
> + * drm_check_scrambling_status - what is status of scrambling?
> + * @adapter: i2c adapter for SCDC channel
> + *
> + * Read the scrambler status over SCDC channel, and check the
> + * scrambling status.
> + *
> + * Return: True if the scrambling is enabled, false otherwise.
> + */
> +
> +bool drm_check_scrambling_status(struct i2c_adapter *adapter)

The name is again a little wonky. And it looks like something that
should live alognside the SCDC helper stuff.

> +{
> +	u8 status;
> +
> +	if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) {
> +		DRM_ERROR("Failed to read scrambling status\n");
> +		return false;
> +	}
> +
> +	status &= SCDC_SCRAMBLING_STATUS;
> +	return status != 0;

return status & SCDC_SCRAMBLING_STATUS

> +}
> +
> +/**
> + * drm_enable_scrambling - enable scrambling
> + * @connector: target drm_connector
> + * @adapter: i2c adapter for SCDC channel
> + * @force: enable scrambling, even if its already enabled
> + *
> + * Write the TMDS config over SCDC channel, and enable scrambling
> + * Return: True if scrambling is successfully enabled, false otherwise.
> + */
> +
> +bool drm_enable_scrambling(struct drm_connector *connector,
> +					struct i2c_adapter *adapter, bool force)
> +{
> +	u8 config;
> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
> +
> +	if (hdmi_info->scr_info.status && !force) {
> +		DRM_DEBUG_KMS("Scrambling already enabled\n");
> +		return true;
> +	}

I don't think we want to track any dynamic state in the display info.
That belongs to the atomic state stuff.

And the function again looks like something that belongs in the SCDC
helper.

Since the two pieces of infromaton in this registrer are the scramble
control and the clock ratio, I think we might want to just have one
function to configure both.

> +
> +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
> +		DRM_ERROR("Failed to read tmds config\n");
> +		return false;
> +	}
> +
> +	config |= SCDC_SCRAMBLING_ENABLE;
> +
> +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
> +		DRM_ERROR("Failed to enable scrambling, write error\n");
> +		return false;
> +	}
> +
> +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
> +	return hdmi_info->scr_info.status;
> +}
> +
> +/**
> + * drm_disable_scrambling - disable scrambling
> + * @connector: target drm_connector
> + * @adapter: i2c adapter for SCDC channel
> + * @force: disable scrambling, even if its already disabled
> + *
> + * Write the TMDS config over SCDC channel, and disable scrambling
> + * Return: True if scrambling is successfully disabled, false otherwise.
> + */
> +bool drm_disable_scrambling(struct drm_connector *connector,
> +					struct i2c_adapter *adapter, bool force)
> +{
> +	u8 config;
> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
> +
> +	if (!hdmi_info->scr_info.status && !force) {
> +		DRM_DEBUG_KMS("Scrambling already disabled\n");
> +		return true;
> +	}
> +
> +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
> +		DRM_ERROR("Failed to read tmds config\n");
> +		return false;
> +	}
> +
> +	config &= ~SCDC_SCRAMBLING_ENABLE;
> +
> +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
> +		DRM_ERROR("Failed to enable scrambling, write error\n");
> +		return false;
> +	}
> +
> +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
> +	return !hdmi_info->scr_info.status;
> +}
> +
>  static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>  					   const u8 *hdmi)
>  {
> @@ -3928,8 +4055,10 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  
>  		if (cea_db_is_hdmi_vsdb(db))
>  			drm_parse_hdmi_vsdb_video(connector, db);
> -		if (cea_db_is_hdmi_forum_vsdb(db))
> +		if (cea_db_is_hdmi_forum_vsdb(db)) {
>  			drm_detect_hdmi_scdc(connector, db);
> +			drm_detect_hdmi_scrambling(connector, db);
> +		}
>  	}
>  }
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 2435598..b9735bd 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -90,6 +90,26 @@ enum subpixel_order {
>  
>  };
>  
> +
> +/**
> + * struct scrambling: sink's scrambling support.
> + */
> +struct scrambling {
> +	/**
> +	 * @supported: scrambling supported for rates > 340Mhz.
> +	 */
> +	bool supported;
> +	/**
> +	 * @low_clocks: scrambling supported for rates <= 340Mhz.
> +	 */
> +	bool low_clocks;
> +	/**
> +	 * @status: scrambling enabled/disabled ?
> +	 */
> +	bool status;
> +};
> +
> +
>  /**
>   * struct drm_hdmi_info - runtime data about the connected sink
>   *
> @@ -108,6 +128,10 @@ struct drm_hdmi_info {
>  	 * @scdc_rr: sink is capable of generating scdc read request.
>  	 */
>  	bool scdc_rr;
> +	/**
> +	 * scr_info: sink's scrambling support and capabilities.
> +	 */
> +	struct scrambling scr_info;
>  };
>  
>  /**
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 43fb0ac..d24c974 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -462,5 +462,9 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
>  struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>  					   int hsize, int vsize, int fresh,
>  					   bool rb);
> -
> +bool drm_enable_scrambling(struct drm_connector *connector,
> +				struct i2c_adapter *adapter, bool force);
> +bool drm_disable_scrambling(struct drm_connector *connector,
> +				struct i2c_adapter *adapter, bool force);
> +bool drm_check_scrambling_status(struct i2c_adapter *adapter);
>  #endif /* __DRM_EDID_H__ */
> -- 
> 1.9.1

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

Re: [PATCH 3/6] drm/edid: detect SCDC support in HF-VSDB

[Ville Syrjälä]

On Wed, Feb 01, 2017 at 06:14:38PM +0530, Shashank Sharma wrote:
> This patch does following:
> - Adds a new structure (drm_hdmi_info) in drm_display_info.
>   This structure will be used to save and indicate if sink
>   supports advance HDMI 2.0 features
> - Checks the HF-VSDB block for presence of SCDC, and marks it
>   in hdmi_info structure.
> - If SCDC is present, checks if sink is capable of generating
>   scdc read request, and marks it in hdmi_info structure.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 14 ++++++++++++++
>  include/drm/drm_connector.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 96d3e47..37902e5 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3802,6 +3802,18 @@ enum hdmi_quantization_range
>  }
>  EXPORT_SYMBOL(drm_default_rgb_quant_range);
>  
> +static void drm_detect_hdmi_scdc(struct drm_connector *connector,
> +				 const u8 *hf_vsdb)
> +{
> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi_info;
> +
> +	if (hf_vsdb[6] & 0x80) {
> +		hdmi->scdc_supported = true;
> +		if (hf_vsdb[6] & 0x40)
> +			hdmi->scdc_rr = true;
> +	}
> +}
> +
>  static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>  					   const u8 *hdmi)
>  {
> @@ -3916,6 +3928,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  
>  		if (cea_db_is_hdmi_vsdb(db))
>  			drm_parse_hdmi_vsdb_video(connector, db);
> +		if (cea_db_is_hdmi_forum_vsdb(db))
> +			drm_detect_hdmi_scdc(connector, db);
>  	}
>  }
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index e5e1edd..2435598 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -87,6 +87,27 @@ enum subpixel_order {
>  	SubPixelVerticalRGB,
>  	SubPixelVerticalBGR,
>  	SubPixelNone,
> +
> +};
> +
> +/**
> + * struct drm_hdmi_info - runtime data about the connected sink
> + *
> + * Describes if a given hdmi display supports advance HDMI 2.0 featutes.
> + * This information is available in CEA-861-F extension blocks (like
> + * HF-VSDB)
> + * For sinks which provide an EDID this can be filled out by calling
> + * drm_add_edid_modes().
> + */
> +struct drm_hdmi_info {
> +	/**
> +	 * @scdc_supported: status control & data channel present.
> +	 */
> +	bool scdc_supported;
> +	/**
> +	 * @scdc_rr: sink is capable of generating scdc read request.
> +	 */
> +	bool scdc_rr;

Probably worth spelling the thing out.

>  };
>  
>  /**
> @@ -188,6 +209,11 @@ struct drm_display_info {
>  	 * @cea_rev: CEA revision of the HDMI sink.
>  	 */
>  	u8 cea_rev;
> +
> +	/**
> +	 * @hdmi_info: advance features of a HDMI sink.
> +	 */
> +	struct drm_hdmi_info hdmi_info;
>  };
>  
>  int drm_display_info_set_bus_formats(struct drm_display_info *info,
> -- 
> 1.9.1

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

Re: [PATCH 5/6] drm/i915: enable scrambling

[Ville Syrjälä]

On Wed, Feb 01, 2017 at 06:14:40PM +0530, Shashank Sharma wrote:
> Geminilake platform has a native HDMI 2.0 controller, and is
> capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
> mendates scrambling for these higher clocks, for reduced RF footprint.
> 
> This patch checks if the monitor supports scrambling, and if required,
> enables it during the modeset.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h   |  2 ++
>  drivers/gpu/drm/i915/intel_ddi.c  |  5 +++++
>  drivers/gpu/drm/i915/intel_drv.h  |  2 ++
>  drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 51 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 495b789..cc85892 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7807,6 +7807,8 @@ enum {
>  #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
>  #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
>  #define  TRANS_DDI_BFI_ENABLE		(1<<4)
> +#define  TRANS_DDI_HIGH_TMDS_CHAR_RATE	(1<<4)
> +#define  TRANS_DDI_HDMI_SCRAMBLING	(1<<0)
>  
>  /* DisplayPort Transport Control */
>  #define _DP_TP_CTL_A			0x64040
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 9a9a670..aea81ce 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1278,6 +1278,11 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
>  			temp |= TRANS_DDI_MODE_SELECT_HDMI;
>  		else
>  			temp |= TRANS_DDI_MODE_SELECT_DVI;
> +
> +		if (IS_GEMINILAKE(dev_priv)) {
> +			temp |= intel_hdmi_check_scrambling(intel_encoder,
> +				&intel_crtc->config->base.adjusted_mode);
> +		}
>  	} else if (type == INTEL_OUTPUT_ANALOG) {
>  		temp |= TRANS_DDI_MODE_SELECT_FDI;
>  		temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 393f243..aafce7f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1588,6 +1588,8 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  			       struct intel_crtc_state *pipe_config,
>  			       struct drm_connector_state *conn_state);
> +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder,
> +				struct drm_display_mode *mode);
>  void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
>  
>  
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index ebae2bd..92dd9bc 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1795,6 +1795,48 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
>  	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>  }
>  
> +static void
> +intel_hdmi_enable_scrambling(struct drm_connector *connector)
> +{
> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> +	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> +	struct i2c_adapter *adapter =
> +			intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
> +
> +	if (!drm_enable_scrambling(connector, adapter, true))
> +		DRM_ERROR("Request to enable scrambling failed\n");
> +}

I don't like hiding this somewhere deep like this. It should be
somewhere much higher up.

And I'm thinkign we might want to track the scrambler state
in the crtc state.

> +
> +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder,
> +				struct drm_display_mode *mode)
> +{
> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> +	struct drm_connector *connector = &intel_hdmi->attached_connector->base;
> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
> +	uint32_t hdmi_config = 0;
> +
> +	DRM_DEBUG_KMS("Checking scrambling for enc:%s connector:%s\n",
> +			intel_encoder->base.name, connector->name);
> +
> +	if (mode->clock < 340000) {
> +		if (hdmi_info->scr_info.low_clocks) {
> +			intel_hdmi_enable_scrambling(connector);
> +			hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING;
> +		}
> +		return hdmi_config;
> +	}
> +
> +	/* Enable scrambling for clocks > 340M */
> +	if (hdmi_info->scr_info.supported) {
> +		intel_hdmi_enable_scrambling(connector);
> +		hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING;
> +	}
> +
> +	/* Scrambling or not, if clock > 340M, set high char rate */
> +	hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> +	return hdmi_config;
> +}
> +
>  static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
>  			     enum port port)
>  {
> -- 
> 1.9.1

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

Re: [PATCH 4/6] drm: scrambling support in drm layer

[Pandiyan, Dhinakaran]

On Wed, 2017-02-01 at 18:14 +0530, Shashank Sharma wrote:
> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
> than 340Mhz. This patch adds few new functions in drm layer for
> core drivers to enable/disable scrambling.
> 
> This patch adds:
> - A function to detect scrambling support parsing HF-VSDB
> - A function to check scrambling status runtime using SCDC read.
> - Two functions to enable/disable scrambling using SCDC read/write.
> - Few new bools to reflect scrambling support and status.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 131 +++++++++++++++++++++++++++++++++++++++++++-
>  include/drm/drm_connector.h |  24 ++++++++
>  include/drm/drm_edid.h      |   6 +-
>  3 files changed, 159 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 37902e5..f0d940a 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -37,6 +37,7 @@
>  #include <drm/drm_edid.h>
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_displayid.h>
> +#include <drm/drm_scdc_helper.h>
>  
>  #define version_greater(edid, maj, min) \
>  	(((edid)->version > (maj)) || \
> @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector *connector,
>  	}
>  }
>  
> +static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
> +				 const u8 *hf_vsdb)
> +{
> +	struct drm_display_info *display = &connector->display_info;
> +	struct drm_hdmi_info *hdmi = &display->hdmi_info;
> +
> +	/*
> +	 * All HDMI 2.0 monitors must support scrambling at rates > 340M.
> +	 * And as per the spec, three factors confirm this:
> +	 * * Availability of a HF-VSDB block in EDID (check)
> +	 * * Non zero Max_TMDS_Char_Rate filed in HF-VSDB
> +	 * * SCDC support available
> +	 * Lets check it out.
> +	 */
> +
> +	if (hf_vsdb[5]) {
> +		display->max_tmds_clock = hf_vsdb[5] * 5000;

Comment explaining or quoting where the 5000 comes from?

> +		DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> +			display->max_tmds_clock);
> +
> +		if (hdmi->scdc_supported) {
> +			hdmi->scr_info.supported = true;
> +
> +			/* Few sinks support scrambling for cloks < 340M */
> +			if ((hf_vsdb[6] & 0x8))
> +				hdmi->scr_info.low_clocks = true;
> +		}
> +	}
> +}
> +
> +/**
> + * drm_check_scrambling_status - what is status of scrambling?
> + * @adapter: i2c adapter for SCDC channel
> + *
> + * Read the scrambler status over SCDC channel, and check the
> + * scrambling status.
> + *
> + * Return: True if the scrambling is enabled, false otherwise.
> + */
> +
> +bool drm_check_scrambling_status(struct i2c_adapter *adapter)
> +{
> +	u8 status;
> +
> +	if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) {
> +		DRM_ERROR("Failed to read scrambling status\n");
> +		return false;
> +	}
> +
> +	status &= SCDC_SCRAMBLING_STATUS;
> +	return status != 0;
> +}
> +
> +/**
> + * drm_enable_scrambling - enable scrambling
> + * @connector: target drm_connector
> + * @adapter: i2c adapter for SCDC channel
> + * @force: enable scrambling, even if its already enabled
> + *
> + * Write the TMDS config over SCDC channel, and enable scrambling
> + * Return: True if scrambling is successfully enabled, false otherwise.
> + */
> +
> +bool drm_enable_scrambling(struct drm_connector *connector,
> +					struct i2c_adapter *adapter, bool force)
> +{
> +	u8 config;
> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
> +
> +	if (hdmi_info->scr_info.status && !force) {
> +		DRM_DEBUG_KMS("Scrambling already enabled\n");
> +		return true;
> +	}
> +
> +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
> +		DRM_ERROR("Failed to read tmds config\n");
> +		return false;
> +	}
> +
> +	config |= SCDC_SCRAMBLING_ENABLE;
> +
> +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
> +		DRM_ERROR("Failed to enable scrambling, write error\n");
> +		return false;
> +	}
> +
> +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
> +	return hdmi_info->scr_info.status;
> +}
> +
> +/**
> + * drm_disable_scrambling - disable scrambling
> + * @connector: target drm_connector
> + * @adapter: i2c adapter for SCDC channel
> + * @force: disable scrambling, even if its already disabled
> + *
> + * Write the TMDS config over SCDC channel, and disable scrambling
> + * Return: True if scrambling is successfully disabled, false otherwise.
> + */
> +bool drm_disable_scrambling(struct drm_connector *connector,
> +					struct i2c_adapter *adapter, bool force)
> +{
> +	u8 config;
> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
> +
> +	if (!hdmi_info->scr_info.status && !force) {
> +		DRM_DEBUG_KMS("Scrambling already disabled\n");
> +		return true;
> +	}
> +
> +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
> +		DRM_ERROR("Failed to read tmds config\n");
> +		return false;
> +	}
> +
> +	config &= ~SCDC_SCRAMBLING_ENABLE;
> +
> +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
> +		DRM_ERROR("Failed to enable scrambling, write error\n");
> +		return false;
> +	}
> +
> +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
> +	return !hdmi_info->scr_info.status;
> +}
> +
>  static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>  					   const u8 *hdmi)
>  {
> @@ -3928,8 +4055,10 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  
>  		if (cea_db_is_hdmi_vsdb(db))
>  			drm_parse_hdmi_vsdb_video(connector, db);
> -		if (cea_db_is_hdmi_forum_vsdb(db))
> +		if (cea_db_is_hdmi_forum_vsdb(db)) {
>  			drm_detect_hdmi_scdc(connector, db);
> +			drm_detect_hdmi_scrambling(connector, db);
> +		}
>  	}
>  }
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 2435598..b9735bd 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -90,6 +90,26 @@ enum subpixel_order {
>  
>  };
>  
> +
> +/**
> + * struct scrambling: sink's scrambling support.
> + */
> +struct scrambling {
> +	/**
> +	 * @supported: scrambling supported for rates > 340Mhz.
> +	 */
> +	bool supported;
> +	/**
> +	 * @low_clocks: scrambling supported for rates <= 340Mhz.
> +	 */
> +	bool low_clocks;

The naming for low and high clock rates looks asymmetric.

> +	/**
> +	 * @status: scrambling enabled/disabled ?
> +	 */
> +	bool status;
> +};
> +
> +
>  /**
>   * struct drm_hdmi_info - runtime data about the connected sink
>   *
> @@ -108,6 +128,10 @@ struct drm_hdmi_info {
>  	 * @scdc_rr: sink is capable of generating scdc read request.
>  	 */
>  	bool scdc_rr;
> +	/**
> +	 * scr_info: sink's scrambling support and capabilities.
> +	 */
> +	struct scrambling scr_info;
>  };
>  
>  /**
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 43fb0ac..d24c974 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -462,5 +462,9 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
>  struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>  					   int hsize, int vsize, int fresh,
>  					   bool rb);
> -
> +bool drm_enable_scrambling(struct drm_connector *connector,
> +				struct i2c_adapter *adapter, bool force);
> +bool drm_disable_scrambling(struct drm_connector *connector,
> +				struct i2c_adapter *adapter, bool force);
> +bool drm_check_scrambling_status(struct i2c_adapter *adapter);
>  #endif /* __DRM_EDID_H__ */

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

Re: [PATCH 3/6] drm/edid: detect SCDC support in HF-VSDB

[Sharma, Shashank]

Thanks for the review Thierry. My comments inline.

Regards
Shashank
On 2/1/2017 9:40 PM, Thierry Reding wrote:
> On Wed, Feb 01, 2017 at 06:14:38PM +0530, Shashank Sharma wrote:
>> This patch does following:
>> - Adds a new structure (drm_hdmi_info) in drm_display_info.
>>    This structure will be used to save and indicate if sink
>>    supports advance HDMI 2.0 features
> "advanced"
got it.
>
>> - Checks the HF-VSDB block for presence of SCDC, and marks it
>>    in hdmi_info structure.
> "drm_hdmi_info structure"?
yep, sure.
>> - If SCDC is present, checks if sink is capable of generating
>>    scdc read request, and marks it in hdmi_info structure.
> "SCDC" to be consistent and because it's an abbreviation.
Agree.
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c  | 14 ++++++++++++++
>>   include/drm/drm_connector.h | 26 ++++++++++++++++++++++++++
>>   2 files changed, 40 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 96d3e47..37902e5 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -3802,6 +3802,18 @@ enum hdmi_quantization_range
>>   }
>>   EXPORT_SYMBOL(drm_default_rgb_quant_range);
>>   
>> +static void drm_detect_hdmi_scdc(struct drm_connector *connector,
>> +				 const u8 *hf_vsdb)
>> +{
>> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi_info;
>> +
>> +	if (hf_vsdb[6] & 0x80) {
>> +		hdmi->scdc_supported = true;
>> +		if (hf_vsdb[6] & 0x40)
>> +			hdmi->scdc_rr = true;
>> +	}
>> +}
>> +
>>   static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>>   					   const u8 *hdmi)
>>   {
>> @@ -3916,6 +3928,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>>   
>>   		if (cea_db_is_hdmi_vsdb(db))
>>   			drm_parse_hdmi_vsdb_video(connector, db);
>> +		if (cea_db_is_hdmi_forum_vsdb(db))
>> +			drm_detect_hdmi_scdc(connector, db);
>>   	}
>>   }
>>   
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index e5e1edd..2435598 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -87,6 +87,27 @@ enum subpixel_order {
>>   	SubPixelVerticalRGB,
>>   	SubPixelVerticalBGR,
>>   	SubPixelNone,
>> +
>> +};
>> +
>> +/**
>> + * struct drm_hdmi_info - runtime data about the connected sink
> Maybe "connected HDMI sink"?
Agree.
>> + *
>> + * Describes if a given hdmi display supports advance HDMI 2.0 featutes.
> "HDMI", "advanced", "features"
Oops, got it :-)
>> + * This information is available in CEA-861-F extension blocks (like
>> + * HF-VSDB)
> This should be terminated by a full-stop.
Ok
>> + * For sinks which provide an EDID this can be filled out by calling
>> + * drm_add_edid_modes().
> And maybe make this sentence start right after the one above rather than
> breaking it to the next line.
Ok
> I'm not sure how useful this line is. Most
> driver will be calling drm_add_edid_modes() anyway, but the above makes
> it sound like drm_add_edid_modes() is something you have to explicitly
> call to get these fields parsed.
Mostly a 'yy' and 'p' from the function above, but makes sense, I can 
remove this line.
>> + */
>> +struct drm_hdmi_info {
>> +	/**
>> +	 * @scdc_supported: status control & data channel present.
>> +	 */
>> +	bool scdc_supported;
>> +	/**
>> +	 * @scdc_rr: sink is capable of generating scdc read request.
>> +	 */
>> +	bool scdc_rr;
>>   };
>>   
>>   /**
>> @@ -188,6 +209,11 @@ struct drm_display_info {
>>   	 * @cea_rev: CEA revision of the HDMI sink.
>>   	 */
>>   	u8 cea_rev;
>> +
>> +	/**
>> +	 * @hdmi_info: advance features of a HDMI sink.
>> +	 */
>> +	struct drm_hdmi_info hdmi_info;
> I think we can safely drop the _info suffix on the field name. It's
> already inside a structure that carries this suffix.
Sure, should I call it hdmi_sink OR connected_hdmi ?
- Shashank
>
> Other than that:
>
> Reviewed-by: Thierry Reding <treding@nvidia.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/6] drm: scrambling support in drm layer

[Sharma, Shashank]

Regards

Shashank


On 2/1/2017 10:02 PM, Thierry Reding wrote:
> On Wed, Feb 01, 2017 at 06:14:39PM +0530, Shashank Sharma wrote:
>> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
>> than 340Mhz. This patch adds few new functions in drm layer for
>> core drivers to enable/disable scrambling.
>>
>> This patch adds:
>> - A function to detect scrambling support parsing HF-VSDB
>> - A function to check scrambling status runtime using SCDC read.
>> - Two functions to enable/disable scrambling using SCDC read/write.
>> - Few new bools to reflect scrambling support and status.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c  | 131 +++++++++++++++++++++++++++++++++++++++++++-
>>   include/drm/drm_connector.h |  24 ++++++++
>>   include/drm/drm_edid.h      |   6 +-
>>   3 files changed, 159 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 37902e5..f0d940a 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -37,6 +37,7 @@
>>   #include <drm/drm_edid.h>
>>   #include <drm/drm_encoder.h>
>>   #include <drm/drm_displayid.h>
>> +#include <drm/drm_scdc_helper.h>
>>   
>>   #define version_greater(edid, maj, min) \
>>   	(((edid)->version > (maj)) || \
>> @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector *connector,
>>   	}
>>   }
>>   
>> +static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
>> +				 const u8 *hf_vsdb)
>> +{
>> +	struct drm_display_info *display = &connector->display_info;
>> +	struct drm_hdmi_info *hdmi = &display->hdmi_info;
>> +
>> +	/*
>> +	 * All HDMI 2.0 monitors must support scrambling at rates > 340M.
> In comments below you use Mhz as the abbreviations. This should be
> consistent. Also I think "MHz" is actually the correct spelling.
Agree.
>> +	 * And as per the spec, three factors confirm this:
>> +	 * * Availability of a HF-VSDB block in EDID (check)
>> +	 * * Non zero Max_TMDS_Char_Rate filed in HF-VSDB
>> +	 * * SCDC support available
>> +	 * Lets check it out.
>> +	 */
>> +
>> +	if (hf_vsdb[5]) {
>> +		display->max_tmds_clock = hf_vsdb[5] * 5000;
>> +		DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
>> +			display->max_tmds_clock);
>> +
>> +		if (hdmi->scdc_supported) {
>> +			hdmi->scr_info.supported = true;
>> +
>> +			/* Few sinks support scrambling for cloks < 340M */
>> +			if ((hf_vsdb[6] & 0x8))
>> +				hdmi->scr_info.low_clocks = true;
>> +		}
>> +	}
>> +}
>> +
>> +/**
>> + * drm_check_scrambling_status - what is status of scrambling?
>> + * @adapter: i2c adapter for SCDC channel
> "I2C", same in other parts of this patch.
Got it.
>> + *
>> + * Read the scrambler status over SCDC channel, and check the
>> + * scrambling status.
>> + *
>> + * Return: True if the scrambling is enabled, false otherwise.
> I think the rest of DRM/KMS kerneldoc tries to use "Returns:\n" as a
> standard way to document return values.
Ok.
>> + */
>> +
>> +bool drm_check_scrambling_status(struct i2c_adapter *adapter)
> Maybe use a drm_scdc_*() prefix for this to make it more consistent with
> other SCDC API.
>
> While at it, would this not be better located in drm_scdc.c along with
> the other helpers? drm_edid.c is more focussed on the parsing aspects of
> all things EDID.
Yeah, the same is mentioned by Ville too, will do that.
>> +{
>> +	u8 status;
>> +
>> +	if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) {
> How about storing the error code...
>
>> +		DRM_ERROR("Failed to read scrambling status\n");
> ... and making it part of the error message? Sometimes its useful to
> know what exact error triggered this because it helps narrowing down
> where things went wrong.
Agree, in fact while debugging and testing this patch series, I had to 
print it explicitly.
>
>> +		return false;
>> +	}
>> +
>> +	status &= SCDC_SCRAMBLING_STATUS;
>> +	return status != 0;
> Maybe make this a single line:
>
> 	return (status & SCDC_SCRAMBLING_STATUS) != 0;
Got it.
>
>> +}
>> +
>> +/**
>> + * drm_enable_scrambling - enable scrambling
>> + * @connector: target drm_connector
> "target DRM connector"?
Got it.
>> + * @adapter: i2c adapter for SCDC channel
>> + * @force: enable scrambling, even if its already enabled
>> + *
>> + * Write the TMDS config over SCDC channel, and enable scrambling
>> + * Return: True if scrambling is successfully enabled, false otherwise.
>> + */
>> +
>> +bool drm_enable_scrambling(struct drm_connector *connector,
>> +					struct i2c_adapter *adapter, bool force)
> I think I'd move this to drm_scdc.c as well because it primarily
> operates on SCDC. If you do so, might be worth making adapter the first
> argument for consistency with other SCDC API.
Agree, will move it.
>> +{
>> +	u8 config;
>> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
>> +
>> +	if (hdmi_info->scr_info.status && !force) {
>> +		DRM_DEBUG_KMS("Scrambling already enabled\n");
>> +		return true;
>> +	}
>> +
>> +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
>> +		DRM_ERROR("Failed to read tmds config\n");
> Maybe also print the error code?
Ok.
>> +		return false;
>> +	}
>> +
>> +	config |= SCDC_SCRAMBLING_ENABLE;
>> +
>> +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
>> +		DRM_ERROR("Failed to enable scrambling, write error\n");
> Same here.
Ok
>> +		return false;
>> +	}
>> +
>> +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
>> +	return hdmi_info->scr_info.status;
>> +}
>> +
>> +/**
>> + * drm_disable_scrambling - disable scrambling
>> + * @connector: target drm_connector
>> + * @adapter: i2c adapter for SCDC channel
>> + * @force: disable scrambling, even if its already disabled
>> + *
>> + * Write the TMDS config over SCDC channel, and disable scrambling
>> + * Return: True if scrambling is successfully disabled, false otherwise.
>> + */
>> +bool drm_disable_scrambling(struct drm_connector *connector,
>> +					struct i2c_adapter *adapter, bool force)
>> +{
>> +	u8 config;
>> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
>> +
>> +	if (!hdmi_info->scr_info.status && !force) {
>> +		DRM_DEBUG_KMS("Scrambling already disabled\n");
>> +		return true;
>> +	}
>> +
>> +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
>> +		DRM_ERROR("Failed to read tmds config\n");
>> +		return false;
>> +	}
>> +
>> +	config &= ~SCDC_SCRAMBLING_ENABLE;
>> +
>> +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
>> +		DRM_ERROR("Failed to enable scrambling, write error\n");
>> +		return false;
>> +	}
>> +
>> +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
>> +	return !hdmi_info->scr_info.status;
>> +}
> Same comments as for drm_enable_scrambling().
Got it.
>> @@ -3928,8 +4055,10 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>>   
>>   		if (cea_db_is_hdmi_vsdb(db))
>>   			drm_parse_hdmi_vsdb_video(connector, db);
>> -		if (cea_db_is_hdmi_forum_vsdb(db))
>> +		if (cea_db_is_hdmi_forum_vsdb(db)) {
>>   			drm_detect_hdmi_scdc(connector, db);
>> +			drm_detect_hdmi_scrambling(connector, db);
>> +		}
>>   	}
>>   }
>>   
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 2435598..b9735bd 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -90,6 +90,26 @@ enum subpixel_order {
>>   
>>   };
>>   
>> +
>> +/**
>> + * struct scrambling: sink's scrambling support.
>> + */
>> +struct scrambling {
>> +	/**
>> +	 * @supported: scrambling supported for rates > 340Mhz.
> I think it's common to separate number and unit by a space, so "340
> MHz".
Got it.
>> +	 */
>> +	bool supported;
>> +	/**
>> +	 * @low_clocks: scrambling supported for rates <= 340Mhz.
>> +	 */
>> +	bool low_clocks;
> Maybe "low_rates" because a clock is technically the source of a signal
> that oscillates at the given rate.
Agree, will change it.
>> +	/**
>> +	 * @status: scrambling enabled/disabled ?
>> +	 */
>> +	bool status;
>> +};
>> +
>> +
>>   /**
>>    * struct drm_hdmi_info - runtime data about the connected sink
>>    *
>> @@ -108,6 +128,10 @@ struct drm_hdmi_info {
>>   	 * @scdc_rr: sink is capable of generating scdc read request.
>>   	 */
>>   	bool scdc_rr;
>> +	/**
>> +	 * scr_info: sink's scrambling support and capabilities.
>> +	 */
>> +	struct scrambling scr_info;
> Again, I think I'd drop _info and instead spell out "scrambling" to make
> this easier to remember.
Sure, can be done.
>
> Also I'm wondering why scdc_supported and scdc_rr are not in a structure
> if scrambling info is. Also since scrambling depends on SCDC, would it
> make sense to embed it in a struct drm_hdmi_scdc_info?
My opinion while designing was:
- SCDC rr support is platform specific, and a platform can choose not to 
support read_req but just allow
   scrambling using scdc read and write, hence kept that separate
- You need to look into this internal structure, only if scdc is supported.
- Also, SCDC registers can be used beyond scrambling too, like for error 
detection, and other cases, so I thought
   it would be better to keep it independent.

Does it make sense ?

-Shashank
>
> 	struct drm_hdmi_scdc_scrambling_info {
> 		bool supported;
> 		bool low_rates;
> 		bool status;
> 	};
>
> 	struct drm_hdmi_scdc_info {
> 		bool supported;
> 		bool read_request;
>
> 		struct drm_hdmi_scdc_scrambling_info scrambling;
> 	};
>
> Thierry

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm/edid: detect SCDC support in HF-VSDB

[Sharma, Shashank]

Thanks for the review Ville. My comments inline.

Regards
Shashank
On 2/1/2017 10:03 PM, Ville Syrjälä wrote:
> On Wed, Feb 01, 2017 at 06:14:38PM +0530, Shashank Sharma wrote:
>> This patch does following:
>> - Adds a new structure (drm_hdmi_info) in drm_display_info.
>>    This structure will be used to save and indicate if sink
>>    supports advance HDMI 2.0 features
>> - Checks the HF-VSDB block for presence of SCDC, and marks it
>>    in hdmi_info structure.
>> - If SCDC is present, checks if sink is capable of generating
>>    scdc read request, and marks it in hdmi_info structure.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c  | 14 ++++++++++++++
>>   include/drm/drm_connector.h | 26 ++++++++++++++++++++++++++
>>   2 files changed, 40 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 96d3e47..37902e5 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -3802,6 +3802,18 @@ enum hdmi_quantization_range
>>   }
>>   EXPORT_SYMBOL(drm_default_rgb_quant_range);
>>   
>> +static void drm_detect_hdmi_scdc(struct drm_connector *connector,
>> +				 const u8 *hf_vsdb)
>> +{
>> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi_info;
>> +
>> +	if (hf_vsdb[6] & 0x80) {
>> +		hdmi->scdc_supported = true;
>> +		if (hf_vsdb[6] & 0x40)
>> +			hdmi->scdc_rr = true;
>> +	}
>> +}
>> +
>>   static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>>   					   const u8 *hdmi)
>>   {
>> @@ -3916,6 +3928,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>>   
>>   		if (cea_db_is_hdmi_vsdb(db))
>>   			drm_parse_hdmi_vsdb_video(connector, db);
>> +		if (cea_db_is_hdmi_forum_vsdb(db))
>> +			drm_detect_hdmi_scdc(connector, db);
>>   	}
>>   }
>>   
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index e5e1edd..2435598 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -87,6 +87,27 @@ enum subpixel_order {
>>   	SubPixelVerticalRGB,
>>   	SubPixelVerticalBGR,
>>   	SubPixelNone,
>> +
>> +};
>> +
>> +/**
>> + * struct drm_hdmi_info - runtime data about the connected sink
>> + *
>> + * Describes if a given hdmi display supports advance HDMI 2.0 featutes.
>> + * This information is available in CEA-861-F extension blocks (like
>> + * HF-VSDB)
>> + * For sinks which provide an EDID this can be filled out by calling
>> + * drm_add_edid_modes().
>> + */
>> +struct drm_hdmi_info {
>> +	/**
>> +	 * @scdc_supported: status control & data channel present.
>> +	 */
>> +	bool scdc_supported;
>> +	/**
>> +	 * @scdc_rr: sink is capable of generating scdc read request.
>> +	 */
>> +	bool scdc_rr;
> Probably worth spelling the thing out.
Agree. I was afraid it would make it difficult for 80 char stuff, but 
maybe I can make it scdc->read_req

- Shashank
>>   };
>>   
>>   /**
>> @@ -188,6 +209,11 @@ struct drm_display_info {
>>   	 * @cea_rev: CEA revision of the HDMI sink.
>>   	 */
>>   	u8 cea_rev;
>> +
>> +	/**
>> +	 * @hdmi_info: advance features of a HDMI sink.
>> +	 */
>> +	struct drm_hdmi_info hdmi_info;
>>   };
>>   
>>   int drm_display_info_set_bus_formats(struct drm_display_info *info,
>> -- 
>> 1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/6] drm: scrambling support in drm layer

[Sharma, Shashank]

Regards

Shashank


On 2/1/2017 10:02 PM, Ville Syrjälä wrote:
> On Wed, Feb 01, 2017 at 06:14:39PM +0530, Shashank Sharma wrote:
>> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
>> than 340Mhz. This patch adds few new functions in drm layer for
>> core drivers to enable/disable scrambling.
>>
>> This patch adds:
>> - A function to detect scrambling support parsing HF-VSDB
>> - A function to check scrambling status runtime using SCDC read.
>> - Two functions to enable/disable scrambling using SCDC read/write.
>> - Few new bools to reflect scrambling support and status.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c  | 131 +++++++++++++++++++++++++++++++++++++++++++-
>>   include/drm/drm_connector.h |  24 ++++++++
>>   include/drm/drm_edid.h      |   6 +-
>>   3 files changed, 159 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 37902e5..f0d940a 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -37,6 +37,7 @@
>>   #include <drm/drm_edid.h>
>>   #include <drm/drm_encoder.h>
>>   #include <drm/drm_displayid.h>
>> +#include <drm/drm_scdc_helper.h>
>>   
>>   #define version_greater(edid, maj, min) \
>>   	(((edid)->version > (maj)) || \
>> @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector *connector,
>>   	}
>>   }
>>   
>> +static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
>> +				 const u8 *hf_vsdb)
> That names seems off. Should probably be drm_parse_hdmi_forum_vsdb() or
> something.
Actually, unlike the last patch set, we are not parsing the whole 
hf_vsdb, but parsing it only for
scrambling status byte (hf_vsdb[5]). But may be I can make it 
drm_detect_scrambling_from_hfvsdb
ot something similar. We will have more hf_vsdb parsing for 3d flags, 
yuv420_deep_color etc.
>> +{
>> +	struct drm_display_info *display = &connector->display_info;
>> +	struct drm_hdmi_info *hdmi = &display->hdmi_info;
>> +
>> +	/*
>> +	 * All HDMI 2.0 monitors must support scrambling at rates > 340M.
>> +	 * And as per the spec, three factors confirm this:
>> +	 * * Availability of a HF-VSDB block in EDID (check)
>> +	 * * Non zero Max_TMDS_Char_Rate filed in HF-VSDB
>> +	 * * SCDC support available
>> +	 * Lets check it out.
>> +	 */
>> +
>> +	if (hf_vsdb[5]) {
>> +		display->max_tmds_clock = hf_vsdb[5] * 5000;
>> +		DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
>> +			display->max_tmds_clock);
> I wonder if we should be a little paranoid and ignore this if it's
> <=340?
Sure, will do it.
>
>> +
>> +		if (hdmi->scdc_supported) {
>> +			hdmi->scr_info.supported = true;
>> +
>> +			/* Few sinks support scrambling for cloks < 340M */
>> +			if ((hf_vsdb[6] & 0x8))
>> +				hdmi->scr_info.low_clocks = true;
>> +		}
>> +	}
>> +}
>> +
>> +/**
>> + * drm_check_scrambling_status - what is status of scrambling?
>> + * @adapter: i2c adapter for SCDC channel
>> + *
>> + * Read the scrambler status over SCDC channel, and check the
>> + * scrambling status.
>> + *
>> + * Return: True if the scrambling is enabled, false otherwise.
>> + */
>> +
>> +bool drm_check_scrambling_status(struct i2c_adapter *adapter)
> The name is again a little wonky. And it looks like something that
> should live alognside the SCDC helper stuff.
Yep, Thierry also suggested the same, to move it along with SCDC. Will 
do it.
>> +{
>> +	u8 status;
>> +
>> +	if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) {
>> +		DRM_ERROR("Failed to read scrambling status\n");
>> +		return false;
>> +	}
>> +
>> +	status &= SCDC_SCRAMBLING_STATUS;
>> +	return status != 0;
> return status & SCDC_SCRAMBLING_STATUS
Sure.
>> +}
>> +
>> +/**
>> + * drm_enable_scrambling - enable scrambling
>> + * @connector: target drm_connector
>> + * @adapter: i2c adapter for SCDC channel
>> + * @force: enable scrambling, even if its already enabled
>> + *
>> + * Write the TMDS config over SCDC channel, and enable scrambling
>> + * Return: True if scrambling is successfully enabled, false otherwise.
>> + */
>> +
>> +bool drm_enable_scrambling(struct drm_connector *connector,
>> +					struct i2c_adapter *adapter, bool force)
>> +{
>> +	u8 config;
>> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
>> +
>> +	if (hdmi_info->scr_info.status && !force) {
>> +		DRM_DEBUG_KMS("Scrambling already enabled\n");
>> +		return true;
>> +	}
> I don't think we want to track any dynamic state in the display info.
> That belongs to the atomic state stuff.
Hummm, ok.
>
> And the function again looks like something that belongs in the SCDC
> helper.
Agree.
>
> Since the two pieces of infromaton in this registrer are the scramble
> control and the clock ratio, I think we might want to just have one
> function to configure both.
That's a good suggestion, thanks.

- Shashank
>
>> +
>> +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
>> +		DRM_ERROR("Failed to read tmds config\n");
>> +		return false;
>> +	}
>> +
>> +	config |= SCDC_SCRAMBLING_ENABLE;
>> +
>> +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
>> +		DRM_ERROR("Failed to enable scrambling, write error\n");
>> +		return false;
>> +	}
>> +
>> +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
>> +	return hdmi_info->scr_info.status;
>> +}
>> +
>> +/**
>> + * drm_disable_scrambling - disable scrambling
>> + * @connector: target drm_connector
>> + * @adapter: i2c adapter for SCDC channel
>> + * @force: disable scrambling, even if its already disabled
>> + *
>> + * Write the TMDS config over SCDC channel, and disable scrambling
>> + * Return: True if scrambling is successfully disabled, false otherwise.
>> + */
>> +bool drm_disable_scrambling(struct drm_connector *connector,
>> +					struct i2c_adapter *adapter, bool force)
>> +{
>> +	u8 config;
>> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
>> +
>> +	if (!hdmi_info->scr_info.status && !force) {
>> +		DRM_DEBUG_KMS("Scrambling already disabled\n");
>> +		return true;
>> +	}
>> +
>> +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
>> +		DRM_ERROR("Failed to read tmds config\n");
>> +		return false;
>> +	}
>> +
>> +	config &= ~SCDC_SCRAMBLING_ENABLE;
>> +
>> +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
>> +		DRM_ERROR("Failed to enable scrambling, write error\n");
>> +		return false;
>> +	}
>> +
>> +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
>> +	return !hdmi_info->scr_info.status;
>> +}
>> +
>>   static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>>   					   const u8 *hdmi)
>>   {
>> @@ -3928,8 +4055,10 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>>   
>>   		if (cea_db_is_hdmi_vsdb(db))
>>   			drm_parse_hdmi_vsdb_video(connector, db);
>> -		if (cea_db_is_hdmi_forum_vsdb(db))
>> +		if (cea_db_is_hdmi_forum_vsdb(db)) {
>>   			drm_detect_hdmi_scdc(connector, db);
>> +			drm_detect_hdmi_scrambling(connector, db);
>> +		}
>>   	}
>>   }
>>   
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 2435598..b9735bd 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -90,6 +90,26 @@ enum subpixel_order {
>>   
>>   };
>>   
>> +
>> +/**
>> + * struct scrambling: sink's scrambling support.
>> + */
>> +struct scrambling {
>> +	/**
>> +	 * @supported: scrambling supported for rates > 340Mhz.
>> +	 */
>> +	bool supported;
>> +	/**
>> +	 * @low_clocks: scrambling supported for rates <= 340Mhz.
>> +	 */
>> +	bool low_clocks;
>> +	/**
>> +	 * @status: scrambling enabled/disabled ?
>> +	 */
>> +	bool status;
>> +};
>> +
>> +
>>   /**
>>    * struct drm_hdmi_info - runtime data about the connected sink
>>    *
>> @@ -108,6 +128,10 @@ struct drm_hdmi_info {
>>   	 * @scdc_rr: sink is capable of generating scdc read request.
>>   	 */
>>   	bool scdc_rr;
>> +	/**
>> +	 * scr_info: sink's scrambling support and capabilities.
>> +	 */
>> +	struct scrambling scr_info;
>>   };
>>   
>>   /**
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 43fb0ac..d24c974 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -462,5 +462,9 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
>>   struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>>   					   int hsize, int vsize, int fresh,
>>   					   bool rb);
>> -
>> +bool drm_enable_scrambling(struct drm_connector *connector,
>> +				struct i2c_adapter *adapter, bool force);
>> +bool drm_disable_scrambling(struct drm_connector *connector,
>> +				struct i2c_adapter *adapter, bool force);
>> +bool drm_check_scrambling_status(struct i2c_adapter *adapter);
>>   #endif /* __DRM_EDID_H__ */
>> -- 
>> 1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/6] drm/i915: enable scrambling

[Sharma, Shashank]

Regards

Shashank


On 2/1/2017 10:06 PM, Ville Syrjälä wrote:
> On Wed, Feb 01, 2017 at 06:14:40PM +0530, Shashank Sharma wrote:
>> Geminilake platform has a native HDMI 2.0 controller, and is
>> capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
>> mendates scrambling for these higher clocks, for reduced RF footprint.
>>
>> This patch checks if the monitor supports scrambling, and if required,
>> enables it during the modeset.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h   |  2 ++
>>   drivers/gpu/drm/i915/intel_ddi.c  |  5 +++++
>>   drivers/gpu/drm/i915/intel_drv.h  |  2 ++
>>   drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 495b789..cc85892 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7807,6 +7807,8 @@ enum {
>>   #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
>>   #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
>>   #define  TRANS_DDI_BFI_ENABLE		(1<<4)
>> +#define  TRANS_DDI_HIGH_TMDS_CHAR_RATE	(1<<4)
>> +#define  TRANS_DDI_HDMI_SCRAMBLING	(1<<0)
>>   
>>   /* DisplayPort Transport Control */
>>   #define _DP_TP_CTL_A			0x64040
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 9a9a670..aea81ce 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1278,6 +1278,11 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
>>   			temp |= TRANS_DDI_MODE_SELECT_HDMI;
>>   		else
>>   			temp |= TRANS_DDI_MODE_SELECT_DVI;
>> +
>> +		if (IS_GEMINILAKE(dev_priv)) {
>> +			temp |= intel_hdmi_check_scrambling(intel_encoder,
>> +				&intel_crtc->config->base.adjusted_mode);
>> +		}
>>   	} else if (type == INTEL_OUTPUT_ANALOG) {
>>   		temp |= TRANS_DDI_MODE_SELECT_FDI;
>>   		temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 393f243..aafce7f 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1588,6 +1588,8 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>>   bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>   			       struct intel_crtc_state *pipe_config,
>>   			       struct drm_connector_state *conn_state);
>> +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder,
>> +				struct drm_display_mode *mode);
>>   void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
>>   
>>   
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index ebae2bd..92dd9bc 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -1795,6 +1795,48 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
>>   	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>   }
>>   
>> +static void
>> +intel_hdmi_enable_scrambling(struct drm_connector *connector)
>> +{
>> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
>> +	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>> +	struct i2c_adapter *adapter =
>> +			intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
>> +
>> +	if (!drm_enable_scrambling(connector, adapter, true))
>> +		DRM_ERROR("Request to enable scrambling failed\n");
>> +}
> I don't like hiding this somewhere deep like this. It should be
> somewhere much higher up.
Why ? All we need to do here is enable two bits in transcoder control 
register, which is already being
programmed in a calling function, so I dont see the use case, but I 
might be missing some bigger picture.
Can you please elaborate on this ?
>
> And I'm thinkign we might want to track the scrambler state
> in the crtc state.
Yes, that's a pretty good way to track dynamic status of scrambler, do 
you think we should add this in
drm_crtc_state itself ?

- Shashank
>> +
>> +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder,
>> +				struct drm_display_mode *mode)
>> +{
>> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
>> +	struct drm_connector *connector = &intel_hdmi->attached_connector->base;
>> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
>> +	uint32_t hdmi_config = 0;
>> +
>> +	DRM_DEBUG_KMS("Checking scrambling for enc:%s connector:%s\n",
>> +			intel_encoder->base.name, connector->name);
>> +
>> +	if (mode->clock < 340000) {
>> +		if (hdmi_info->scr_info.low_clocks) {
>> +			intel_hdmi_enable_scrambling(connector);
>> +			hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING;
>> +		}
>> +		return hdmi_config;
>> +	}
>> +
>> +	/* Enable scrambling for clocks > 340M */
>> +	if (hdmi_info->scr_info.supported) {
>> +		intel_hdmi_enable_scrambling(connector);
>> +		hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING;
>> +	}
>> +
>> +	/* Scrambling or not, if clock > 340M, set high char rate */
>> +	hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
>> +	return hdmi_config;
>> +}
>> +
>>   static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
>>   			     enum port port)
>>   {
>> -- 
>> 1.9.1

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

Re: [Intel-gfx] [PATCH 4/6] drm: scrambling support in drm layer

[Sharma, Shashank]

Thanks for the review, Dhinakaran.


Regards

Shashank


On 2/2/2017 1:23 AM, Pandiyan, Dhinakaran wrote:
> On Wed, 2017-02-01 at 18:14 +0530, Shashank Sharma wrote:
>> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
>> than 340Mhz. This patch adds few new functions in drm layer for
>> core drivers to enable/disable scrambling.
>>
>> This patch adds:
>> - A function to detect scrambling support parsing HF-VSDB
>> - A function to check scrambling status runtime using SCDC read.
>> - Two functions to enable/disable scrambling using SCDC read/write.
>> - Few new bools to reflect scrambling support and status.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c  | 131 +++++++++++++++++++++++++++++++++++++++++++-
>>   include/drm/drm_connector.h |  24 ++++++++
>>   include/drm/drm_edid.h      |   6 +-
>>   3 files changed, 159 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 37902e5..f0d940a 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -37,6 +37,7 @@
>>   #include <drm/drm_edid.h>
>>   #include <drm/drm_encoder.h>
>>   #include <drm/drm_displayid.h>
>> +#include <drm/drm_scdc_helper.h>
>>   
>>   #define version_greater(edid, maj, min) \
>>   	(((edid)->version > (maj)) || \
>> @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector *connector,
>>   	}
>>   }
>>   
>> +static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
>> +				 const u8 *hf_vsdb)
>> +{
>> +	struct drm_display_info *display = &connector->display_info;
>> +	struct drm_hdmi_info *hdmi = &display->hdmi_info;
>> +
>> +	/*
>> +	 * All HDMI 2.0 monitors must support scrambling at rates > 340M.
>> +	 * And as per the spec, three factors confirm this:
>> +	 * * Availability of a HF-VSDB block in EDID (check)
>> +	 * * Non zero Max_TMDS_Char_Rate filed in HF-VSDB
>> +	 * * SCDC support available
>> +	 * Lets check it out.
>> +	 */
>> +
>> +	if (hf_vsdb[5]) {
>> +		display->max_tmds_clock = hf_vsdb[5] * 5000;
> Comment explaining or quoting where the 5000 comes from?
Sure, can be done.
>
>> +		DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
>> +			display->max_tmds_clock);
>> +
>> +		if (hdmi->scdc_supported) {
>> +			hdmi->scr_info.supported = true;
>> +
>> +			/* Few sinks support scrambling for cloks < 340M */
>> +			if ((hf_vsdb[6] & 0x8))
>> +				hdmi->scr_info.low_clocks = true;
>> +		}
>> +	}
>> +}
>> +
>> +/**
>> + * drm_check_scrambling_status - what is status of scrambling?
>> + * @adapter: i2c adapter for SCDC channel
>> + *
>> + * Read the scrambler status over SCDC channel, and check the
>> + * scrambling status.
>> + *
>> + * Return: True if the scrambling is enabled, false otherwise.
>> + */
>> +
>> +bool drm_check_scrambling_status(struct i2c_adapter *adapter)
>> +{
>> +	u8 status;
>> +
>> +	if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) {
>> +		DRM_ERROR("Failed to read scrambling status\n");
>> +		return false;
>> +	}
>> +
>> +	status &= SCDC_SCRAMBLING_STATUS;
>> +	return status != 0;
>> +}
>> +
>> +/**
>> + * drm_enable_scrambling - enable scrambling
>> + * @connector: target drm_connector
>> + * @adapter: i2c adapter for SCDC channel
>> + * @force: enable scrambling, even if its already enabled
>> + *
>> + * Write the TMDS config over SCDC channel, and enable scrambling
>> + * Return: True if scrambling is successfully enabled, false otherwise.
>> + */
>> +
>> +bool drm_enable_scrambling(struct drm_connector *connector,
>> +					struct i2c_adapter *adapter, bool force)
>> +{
>> +	u8 config;
>> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
>> +
>> +	if (hdmi_info->scr_info.status && !force) {
>> +		DRM_DEBUG_KMS("Scrambling already enabled\n");
>> +		return true;
>> +	}
>> +
>> +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
>> +		DRM_ERROR("Failed to read tmds config\n");
>> +		return false;
>> +	}
>> +
>> +	config |= SCDC_SCRAMBLING_ENABLE;
>> +
>> +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
>> +		DRM_ERROR("Failed to enable scrambling, write error\n");
>> +		return false;
>> +	}
>> +
>> +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
>> +	return hdmi_info->scr_info.status;
>> +}
>> +
>> +/**
>> + * drm_disable_scrambling - disable scrambling
>> + * @connector: target drm_connector
>> + * @adapter: i2c adapter for SCDC channel
>> + * @force: disable scrambling, even if its already disabled
>> + *
>> + * Write the TMDS config over SCDC channel, and disable scrambling
>> + * Return: True if scrambling is successfully disabled, false otherwise.
>> + */
>> +bool drm_disable_scrambling(struct drm_connector *connector,
>> +					struct i2c_adapter *adapter, bool force)
>> +{
>> +	u8 config;
>> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
>> +
>> +	if (!hdmi_info->scr_info.status && !force) {
>> +		DRM_DEBUG_KMS("Scrambling already disabled\n");
>> +		return true;
>> +	}
>> +
>> +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
>> +		DRM_ERROR("Failed to read tmds config\n");
>> +		return false;
>> +	}
>> +
>> +	config &= ~SCDC_SCRAMBLING_ENABLE;
>> +
>> +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
>> +		DRM_ERROR("Failed to enable scrambling, write error\n");
>> +		return false;
>> +	}
>> +
>> +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
>> +	return !hdmi_info->scr_info.status;
>> +}
>> +
>>   static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>>   					   const u8 *hdmi)
>>   {
>> @@ -3928,8 +4055,10 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>>   
>>   		if (cea_db_is_hdmi_vsdb(db))
>>   			drm_parse_hdmi_vsdb_video(connector, db);
>> -		if (cea_db_is_hdmi_forum_vsdb(db))
>> +		if (cea_db_is_hdmi_forum_vsdb(db)) {
>>   			drm_detect_hdmi_scdc(connector, db);
>> +			drm_detect_hdmi_scrambling(connector, db);
>> +		}
>>   	}
>>   }
>>   
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 2435598..b9735bd 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -90,6 +90,26 @@ enum subpixel_order {
>>   
>>   };
>>   
>> +
>> +/**
>> + * struct scrambling: sink's scrambling support.
>> + */
>> +struct scrambling {
>> +	/**
>> +	 * @supported: scrambling supported for rates > 340Mhz.
>> +	 */
>> +	bool supported;
>> +	/**
>> +	 * @low_clocks: scrambling supported for rates <= 340Mhz.
>> +	 */
>> +	bool low_clocks;
> The naming for low and high clock rates looks asymmetric.
I dont get it, I dont have a high clock here, can you please elaborate ?

- Shashank
>> +	/**
>> +	 * @status: scrambling enabled/disabled ?
>> +	 */
>> +	bool status;
>> +};
>> +
>> +
>>   /**
>>    * struct drm_hdmi_info - runtime data about the connected sink
>>    *
>> @@ -108,6 +128,10 @@ struct drm_hdmi_info {
>>   	 * @scdc_rr: sink is capable of generating scdc read request.
>>   	 */
>>   	bool scdc_rr;
>> +	/**
>> +	 * scr_info: sink's scrambling support and capabilities.
>> +	 */
>> +	struct scrambling scr_info;
>>   };
>>   
>>   /**
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 43fb0ac..d24c974 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -462,5 +462,9 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
>>   struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>>   					   int hsize, int vsize, int fresh,
>>   					   bool rb);
>> -
>> +bool drm_enable_scrambling(struct drm_connector *connector,
>> +				struct i2c_adapter *adapter, bool force);
>> +bool drm_disable_scrambling(struct drm_connector *connector,
>> +				struct i2c_adapter *adapter, bool force);
>> +bool drm_check_scrambling_status(struct i2c_adapter *adapter);
>>   #endif /* __DRM_EDID_H__ */

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/6] drm: scrambling support in drm layer

[Ville Syrjälä]

On Thu, Feb 02, 2017 at 11:18:51AM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 2/1/2017 10:02 PM, Ville Syrjälä wrote:
> > On Wed, Feb 01, 2017 at 06:14:39PM +0530, Shashank Sharma wrote:
> >> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
> >> than 340Mhz. This patch adds few new functions in drm layer for
> >> core drivers to enable/disable scrambling.
> >>
> >> This patch adds:
> >> - A function to detect scrambling support parsing HF-VSDB
> >> - A function to check scrambling status runtime using SCDC read.
> >> - Two functions to enable/disable scrambling using SCDC read/write.
> >> - Few new bools to reflect scrambling support and status.
> >>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> ---
> >>   drivers/gpu/drm/drm_edid.c  | 131 +++++++++++++++++++++++++++++++++++++++++++-
> >>   include/drm/drm_connector.h |  24 ++++++++
> >>   include/drm/drm_edid.h      |   6 +-
> >>   3 files changed, 159 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> index 37902e5..f0d940a 100644
> >> --- a/drivers/gpu/drm/drm_edid.c
> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> @@ -37,6 +37,7 @@
> >>   #include <drm/drm_edid.h>
> >>   #include <drm/drm_encoder.h>
> >>   #include <drm/drm_displayid.h>
> >> +#include <drm/drm_scdc_helper.h>
> >>   
> >>   #define version_greater(edid, maj, min) \
> >>   	(((edid)->version > (maj)) || \
> >> @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector *connector,
> >>   	}
> >>   }
> >>   
> >> +static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
> >> +				 const u8 *hf_vsdb)
> > That names seems off. Should probably be drm_parse_hdmi_forum_vsdb() or
> > something.
> Actually, unlike the last patch set, we are not parsing the whole 
> hf_vsdb, but parsing it only for
> scrambling status byte (hf_vsdb[5]). But may be I can make it 
> drm_detect_scrambling_from_hfvsdb
> ot something similar. We will have more hf_vsdb parsing for 3d flags, 
> yuv420_deep_color etc.

Well, so far I'm not seeing much point in splitting it up. So I'd stuff
it all into one place, for now at least.

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

Re: [PATCH 5/6] drm/i915: enable scrambling

[Ville Syrjälä]

On Thu, Feb 02, 2017 at 11:23:19AM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 2/1/2017 10:06 PM, Ville Syrjälä wrote:
> > On Wed, Feb 01, 2017 at 06:14:40PM +0530, Shashank Sharma wrote:
> >> Geminilake platform has a native HDMI 2.0 controller, and is
> >> capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
> >> mendates scrambling for these higher clocks, for reduced RF footprint.
> >>
> >> This patch checks if the monitor supports scrambling, and if required,
> >> enables it during the modeset.
> >>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_reg.h   |  2 ++
> >>   drivers/gpu/drm/i915/intel_ddi.c  |  5 +++++
> >>   drivers/gpu/drm/i915/intel_drv.h  |  2 ++
> >>   drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++++++++
> >>   4 files changed, 51 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index 495b789..cc85892 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -7807,6 +7807,8 @@ enum {
> >>   #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
> >>   #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
> >>   #define  TRANS_DDI_BFI_ENABLE		(1<<4)
> >> +#define  TRANS_DDI_HIGH_TMDS_CHAR_RATE	(1<<4)
> >> +#define  TRANS_DDI_HDMI_SCRAMBLING	(1<<0)
> >>   
> >>   /* DisplayPort Transport Control */
> >>   #define _DP_TP_CTL_A			0x64040
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index 9a9a670..aea81ce 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -1278,6 +1278,11 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
> >>   			temp |= TRANS_DDI_MODE_SELECT_HDMI;
> >>   		else
> >>   			temp |= TRANS_DDI_MODE_SELECT_DVI;
> >> +
> >> +		if (IS_GEMINILAKE(dev_priv)) {
> >> +			temp |= intel_hdmi_check_scrambling(intel_encoder,
> >> +				&intel_crtc->config->base.adjusted_mode);
> >> +		}
> >>   	} else if (type == INTEL_OUTPUT_ANALOG) {
> >>   		temp |= TRANS_DDI_MODE_SELECT_FDI;
> >>   		temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 393f243..aafce7f 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -1588,6 +1588,8 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> >>   bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >>   			       struct intel_crtc_state *pipe_config,
> >>   			       struct drm_connector_state *conn_state);
> >> +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder,
> >> +				struct drm_display_mode *mode);
> >>   void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
> >>   
> >>   
> >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >> index ebae2bd..92dd9bc 100644
> >> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >> @@ -1795,6 +1795,48 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
> >>   	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >>   }
> >>   
> >> +static void
> >> +intel_hdmi_enable_scrambling(struct drm_connector *connector)
> >> +{
> >> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> >> +	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> >> +	struct i2c_adapter *adapter =
> >> +			intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
> >> +
> >> +	if (!drm_enable_scrambling(connector, adapter, true))
> >> +		DRM_ERROR("Request to enable scrambling failed\n");
> >> +}
> > I don't like hiding this somewhere deep like this. It should be
> > somewhere much higher up.
> Why ? All we need to do here is enable two bits in transcoder control 
> register, which is already being
> programmed in a calling function, so I dont see the use case, but I 
> might be missing some bigger picture.
> Can you please elaborate on this ?

We're talking to the display here, which is rather surprising to happen
from a function thing that on the first glance doesn't even seem to
touch the hardware.

> >
> > And I'm thinkign we might want to track the scrambler state
> > in the crtc state.
> Yes, that's a pretty good way to track dynamic status of scrambler, do 
> you think we should add this in
> drm_crtc_state itself ?

I'd just start with intel specific state and later we can see what
everyone else wants to do and potentially unify a bit.

> 
> - Shashank
> >> +
> >> +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder,
> >> +				struct drm_display_mode *mode)
> >> +{
> >> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> >> +	struct drm_connector *connector = &intel_hdmi->attached_connector->base;
> >> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
> >> +	uint32_t hdmi_config = 0;
> >> +
> >> +	DRM_DEBUG_KMS("Checking scrambling for enc:%s connector:%s\n",
> >> +			intel_encoder->base.name, connector->name);
> >> +
> >> +	if (mode->clock < 340000) {
> >> +		if (hdmi_info->scr_info.low_clocks) {
> >> +			intel_hdmi_enable_scrambling(connector);
> >> +			hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING;
> >> +		}
> >> +		return hdmi_config;
> >> +	}
> >> +
> >> +	/* Enable scrambling for clocks > 340M */
> >> +	if (hdmi_info->scr_info.supported) {
> >> +		intel_hdmi_enable_scrambling(connector);
> >> +		hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING;
> >> +	}
> >> +
> >> +	/* Scrambling or not, if clock > 340M, set high char rate */
> >> +	hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> >> +	return hdmi_config;
> >> +}
> >> +
> >>   static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
> >>   			     enum port port)
> >>   {
> >> -- 
> >> 1.9.1

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

Re: [PATCH 4/6] drm: scrambling support in drm layer

[Sharma, Shashank]

Regards

Shashank


On 2/2/2017 3:21 PM, Ville Syrjälä wrote:
> On Thu, Feb 02, 2017 at 11:18:51AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 2/1/2017 10:02 PM, Ville Syrjälä wrote:
>>> On Wed, Feb 01, 2017 at 06:14:39PM +0530, Shashank Sharma wrote:
>>>> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
>>>> than 340Mhz. This patch adds few new functions in drm layer for
>>>> core drivers to enable/disable scrambling.
>>>>
>>>> This patch adds:
>>>> - A function to detect scrambling support parsing HF-VSDB
>>>> - A function to check scrambling status runtime using SCDC read.
>>>> - Two functions to enable/disable scrambling using SCDC read/write.
>>>> - Few new bools to reflect scrambling support and status.
>>>>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_edid.c  | 131 +++++++++++++++++++++++++++++++++++++++++++-
>>>>    include/drm/drm_connector.h |  24 ++++++++
>>>>    include/drm/drm_edid.h      |   6 +-
>>>>    3 files changed, 159 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>> index 37902e5..f0d940a 100644
>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>> @@ -37,6 +37,7 @@
>>>>    #include <drm/drm_edid.h>
>>>>    #include <drm/drm_encoder.h>
>>>>    #include <drm/drm_displayid.h>
>>>> +#include <drm/drm_scdc_helper.h>
>>>>    
>>>>    #define version_greater(edid, maj, min) \
>>>>    	(((edid)->version > (maj)) || \
>>>> @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector *connector,
>>>>    	}
>>>>    }
>>>>    
>>>> +static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
>>>> +				 const u8 *hf_vsdb)
>>> That names seems off. Should probably be drm_parse_hdmi_forum_vsdb() or
>>> something.
>> Actually, unlike the last patch set, we are not parsing the whole
>> hf_vsdb, but parsing it only for
>> scrambling status byte (hf_vsdb[5]). But may be I can make it
>> drm_detect_scrambling_from_hfvsdb
>> ot something similar. We will have more hf_vsdb parsing for 3d flags,
>> yuv420_deep_color etc.
> Well, so far I'm not seeing much point in splitting it up. So I'd stuff
> it all into one place, for now at least.
I had published a patch just doing as you suggested which was parsing 
complete hf-vsdb in one shot, and adding info in
hdmi_info structure, to accommodate it.
(Patch https://patchwork.freedesktop.org/patch/128963/ and 
https://patchwork.freedesktop.org/patch/128962/)

But you gave a review comment not to add anything which is not being 
used in the patch series:
https://patchwork.freedesktop.org/patch/128962/

That was the only reason I have split hf-vsdb parsing patch into 3 parts
- parse SCDC and scrambling info (here)
- with YUV420 deep color ( upcoming )
- with 3d handling (upcoming)

So I am confused, split or no split :-) ?

- Shashank

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

Re: [PATCH 4/6] drm: scrambling support in drm layer

[Ville Syrjälä]

On Thu, Feb 02, 2017 at 03:46:55PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 2/2/2017 3:21 PM, Ville Syrjälä wrote:
> > On Thu, Feb 02, 2017 at 11:18:51AM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 2/1/2017 10:02 PM, Ville Syrjälä wrote:
> >>> On Wed, Feb 01, 2017 at 06:14:39PM +0530, Shashank Sharma wrote:
> >>>> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
> >>>> than 340Mhz. This patch adds few new functions in drm layer for
> >>>> core drivers to enable/disable scrambling.
> >>>>
> >>>> This patch adds:
> >>>> - A function to detect scrambling support parsing HF-VSDB
> >>>> - A function to check scrambling status runtime using SCDC read.
> >>>> - Two functions to enable/disable scrambling using SCDC read/write.
> >>>> - Few new bools to reflect scrambling support and status.
> >>>>
> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>>> ---
> >>>>    drivers/gpu/drm/drm_edid.c  | 131 +++++++++++++++++++++++++++++++++++++++++++-
> >>>>    include/drm/drm_connector.h |  24 ++++++++
> >>>>    include/drm/drm_edid.h      |   6 +-
> >>>>    3 files changed, 159 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >>>> index 37902e5..f0d940a 100644
> >>>> --- a/drivers/gpu/drm/drm_edid.c
> >>>> +++ b/drivers/gpu/drm/drm_edid.c
> >>>> @@ -37,6 +37,7 @@
> >>>>    #include <drm/drm_edid.h>
> >>>>    #include <drm/drm_encoder.h>
> >>>>    #include <drm/drm_displayid.h>
> >>>> +#include <drm/drm_scdc_helper.h>
> >>>>    
> >>>>    #define version_greater(edid, maj, min) \
> >>>>    	(((edid)->version > (maj)) || \
> >>>> @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector *connector,
> >>>>    	}
> >>>>    }
> >>>>    
> >>>> +static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
> >>>> +				 const u8 *hf_vsdb)
> >>> That names seems off. Should probably be drm_parse_hdmi_forum_vsdb() or
> >>> something.
> >> Actually, unlike the last patch set, we are not parsing the whole
> >> hf_vsdb, but parsing it only for
> >> scrambling status byte (hf_vsdb[5]). But may be I can make it
> >> drm_detect_scrambling_from_hfvsdb
> >> ot something similar. We will have more hf_vsdb parsing for 3d flags,
> >> yuv420_deep_color etc.
> > Well, so far I'm not seeing much point in splitting it up. So I'd stuff
> > it all into one place, for now at least.
> I had published a patch just doing as you suggested which was parsing 
> complete hf-vsdb in one shot, and adding info in
> hdmi_info structure, to accommodate it.
> (Patch https://patchwork.freedesktop.org/patch/128963/ and 
> https://patchwork.freedesktop.org/patch/128962/)
> 
> But you gave a review comment not to add anything which is not being 
> used in the patch series:
> https://patchwork.freedesktop.org/patch/128962/

That's doesn't require the things that are left to be split into
separate functions.

> 
> That was the only reason I have split hf-vsdb parsing patch into 3 parts
> - parse SCDC and scrambling info (here)
> - with YUV420 deep color ( upcoming )
> - with 3d handling (upcoming)
> 
> So I am confused, split or no split :-) ?
> 
> - Shashank

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

Re: [PATCH 4/6] drm: scrambling support in drm layer

[Sharma, Shashank]

Regards

Shashank


On 2/2/2017 3:58 PM, Ville Syrjälä wrote:
> On Thu, Feb 02, 2017 at 03:46:55PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 2/2/2017 3:21 PM, Ville Syrjälä wrote:
>>> On Thu, Feb 02, 2017 at 11:18:51AM +0530, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>>
>>>> On 2/1/2017 10:02 PM, Ville Syrjälä wrote:
>>>>> On Wed, Feb 01, 2017 at 06:14:39PM +0530, Shashank Sharma wrote:
>>>>>> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
>>>>>> than 340Mhz. This patch adds few new functions in drm layer for
>>>>>> core drivers to enable/disable scrambling.
>>>>>>
>>>>>> This patch adds:
>>>>>> - A function to detect scrambling support parsing HF-VSDB
>>>>>> - A function to check scrambling status runtime using SCDC read.
>>>>>> - Two functions to enable/disable scrambling using SCDC read/write.
>>>>>> - Few new bools to reflect scrambling support and status.
>>>>>>
>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/drm_edid.c  | 131 +++++++++++++++++++++++++++++++++++++++++++-
>>>>>>     include/drm/drm_connector.h |  24 ++++++++
>>>>>>     include/drm/drm_edid.h      |   6 +-
>>>>>>     3 files changed, 159 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>>>> index 37902e5..f0d940a 100644
>>>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>>>> @@ -37,6 +37,7 @@
>>>>>>     #include <drm/drm_edid.h>
>>>>>>     #include <drm/drm_encoder.h>
>>>>>>     #include <drm/drm_displayid.h>
>>>>>> +#include <drm/drm_scdc_helper.h>
>>>>>>     
>>>>>>     #define version_greater(edid, maj, min) \
>>>>>>     	(((edid)->version > (maj)) || \
>>>>>> @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector *connector,
>>>>>>     	}
>>>>>>     }
>>>>>>     
>>>>>> +static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
>>>>>> +				 const u8 *hf_vsdb)
>>>>> That names seems off. Should probably be drm_parse_hdmi_forum_vsdb() or
>>>>> something.
>>>> Actually, unlike the last patch set, we are not parsing the whole
>>>> hf_vsdb, but parsing it only for
>>>> scrambling status byte (hf_vsdb[5]). But may be I can make it
>>>> drm_detect_scrambling_from_hfvsdb
>>>> ot something similar. We will have more hf_vsdb parsing for 3d flags,
>>>> yuv420_deep_color etc.
>>> Well, so far I'm not seeing much point in splitting it up. So I'd stuff
>>> it all into one place, for now at least.
>> I had published a patch just doing as you suggested which was parsing
>> complete hf-vsdb in one shot, and adding info in
>> hdmi_info structure, to accommodate it.
>> (Patch https://patchwork.freedesktop.org/patch/128963/ and
>> https://patchwork.freedesktop.org/patch/128962/)
>>
>> But you gave a review comment not to add anything which is not being
>> used in the patch series:
>> https://patchwork.freedesktop.org/patch/128962/
> That's doesn't require the things that are left to be split into
> separate functions.
So please let me know if my understanding is correct.
- Name the function as *_parse_hfvsdb only
- Call it while parsing the EDID extension blocks (the same place)
- Keep on adding parsing of the HF-VSDB blocks, in the same function, in 
an incremental way, as we add patches for them.
In this way the function won't be split into multiple small functions, 
but all the parsing will be done in one function.

- Shashank
>> That was the only reason I have split hf-vsdb parsing patch into 3 parts
>> - parse SCDC and scrambling info (here)
>> - with YUV420 deep color ( upcoming )
>> - with 3d handling (upcoming)
>>
>> So I am confused, split or no split :-) ?
>>
>> - Shashank

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/6] drm/i915: enable scrambling

[Sharma, Shashank]

Regards

Shashank


On 2/2/2017 3:32 PM, Ville Syrjälä wrote:
> On Thu, Feb 02, 2017 at 11:23:19AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 2/1/2017 10:06 PM, Ville Syrjälä wrote:
>>> On Wed, Feb 01, 2017 at 06:14:40PM +0530, Shashank Sharma wrote:
>>>> Geminilake platform has a native HDMI 2.0 controller, and is
>>>> capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
>>>> mendates scrambling for these higher clocks, for reduced RF footprint.
>>>>
>>>> This patch checks if the monitor supports scrambling, and if required,
>>>> enables it during the modeset.
>>>>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_reg.h   |  2 ++
>>>>    drivers/gpu/drm/i915/intel_ddi.c  |  5 +++++
>>>>    drivers/gpu/drm/i915/intel_drv.h  |  2 ++
>>>>    drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++++++++
>>>>    4 files changed, 51 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index 495b789..cc85892 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -7807,6 +7807,8 @@ enum {
>>>>    #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
>>>>    #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
>>>>    #define  TRANS_DDI_BFI_ENABLE		(1<<4)
>>>> +#define  TRANS_DDI_HIGH_TMDS_CHAR_RATE	(1<<4)
>>>> +#define  TRANS_DDI_HDMI_SCRAMBLING	(1<<0)
>>>>    
>>>>    /* DisplayPort Transport Control */
>>>>    #define _DP_TP_CTL_A			0x64040
>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>> index 9a9a670..aea81ce 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -1278,6 +1278,11 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
>>>>    			temp |= TRANS_DDI_MODE_SELECT_HDMI;
>>>>    		else
>>>>    			temp |= TRANS_DDI_MODE_SELECT_DVI;
>>>> +
>>>> +		if (IS_GEMINILAKE(dev_priv)) {
>>>> +			temp |= intel_hdmi_check_scrambling(intel_encoder,
>>>> +				&intel_crtc->config->base.adjusted_mode);
>>>> +		}
>>>>    	} else if (type == INTEL_OUTPUT_ANALOG) {
>>>>    		temp |= TRANS_DDI_MODE_SELECT_FDI;
>>>>    		temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>> index 393f243..aafce7f 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -1588,6 +1588,8 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>>>>    bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>>>    			       struct intel_crtc_state *pipe_config,
>>>>    			       struct drm_connector_state *conn_state);
>>>> +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder,
>>>> +				struct drm_display_mode *mode);
>>>>    void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
>>>>    
>>>>    
>>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> index ebae2bd..92dd9bc 100644
>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> @@ -1795,6 +1795,48 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
>>>>    	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>>>    }
>>>>    
>>>> +static void
>>>> +intel_hdmi_enable_scrambling(struct drm_connector *connector)
>>>> +{
>>>> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
>>>> +	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>>>> +	struct i2c_adapter *adapter =
>>>> +			intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
>>>> +
>>>> +	if (!drm_enable_scrambling(connector, adapter, true))
>>>> +		DRM_ERROR("Request to enable scrambling failed\n");
>>>> +}
>>> I don't like hiding this somewhere deep like this. It should be
>>> somewhere much higher up.
>> Why ? All we need to do here is enable two bits in transcoder control
>> register, which is already being
>> programmed in a calling function, so I dont see the use case, but I
>> might be missing some bigger picture.
>> Can you please elaborate on this ?
> We're talking to the display here, which is rather surprising to happen
> from a function thing that on the first glance doesn't even seem to
> touch the hardware.
I kind of agree with this, and I am also tempted now to shift this call 
upwards, but I want to
keep this close to port enabling as well, as the HDMI 2.0 spec says:
"Max time period between a source enables scrambling on monitor, and 
starts sending scrambled
   video should be 100ms" else the sink might choose to disable scrambling.

Considering this, where do you think would be right place to enabled 
scrambling on monitor ?

- Shashank
>>> And I'm thinkign we might want to track the scrambler state
>>> in the crtc state.
>> Yes, that's a pretty good way to track dynamic status of scrambler, do
>> you think we should add this in
>> drm_crtc_state itself ?
> I'd just start with intel specific state and later we can see what
> everyone else wants to do and potentially unify a bit.
>
>> - Shashank
>>>> +
>>>> +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder,
>>>> +				struct drm_display_mode *mode)
>>>> +{
>>>> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
>>>> +	struct drm_connector *connector = &intel_hdmi->attached_connector->base;
>>>> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
>>>> +	uint32_t hdmi_config = 0;
>>>> +
>>>> +	DRM_DEBUG_KMS("Checking scrambling for enc:%s connector:%s\n",
>>>> +			intel_encoder->base.name, connector->name);
>>>> +
>>>> +	if (mode->clock < 340000) {
>>>> +		if (hdmi_info->scr_info.low_clocks) {
>>>> +			intel_hdmi_enable_scrambling(connector);
>>>> +			hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING;
>>>> +		}
>>>> +		return hdmi_config;
>>>> +	}
>>>> +
>>>> +	/* Enable scrambling for clocks > 340M */
>>>> +	if (hdmi_info->scr_info.supported) {
>>>> +		intel_hdmi_enable_scrambling(connector);
>>>> +		hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING;
>>>> +	}
>>>> +
>>>> +	/* Scrambling or not, if clock > 340M, set high char rate */
>>>> +	hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
>>>> +	return hdmi_config;
>>>> +}
>>>> +
>>>>    static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
>>>>    			     enum port port)
>>>>    {
>>>> -- 
>>>> 1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] drm: Add SCDC helpers

[Jani Nikula]

On Wed, 01 Feb 2017, Shashank Sharma <shashank.sharma@intel.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> SCDC is a mechanism defined in the HDMI 2.0 specification that allows
> the source and sink devices to communicate.
>
> This commit introduces helpers to access the SCDC and provides the
> symbolic names for the various registers defined in the specification.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

One process note, you need to add your own Signed-off-by under the
original author's sign-off, even when you're posting them unchanged.

Signed-off-by means https://developercertificate.org/

BR,
Jani.


> ---
>  Documentation/gpu/drm-kms-helpers.rst |  12 ++++
>  drivers/gpu/drm/Makefile              |   3 +-
>  drivers/gpu/drm/drm_scdc_helper.c     | 111 ++++++++++++++++++++++++++++
>  include/drm/drm_scdc_helper.h         | 132 ++++++++++++++++++++++++++++++++++
>  4 files changed, 257 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/drm_scdc_helper.c
>  create mode 100644 include/drm/drm_scdc_helper.h
>
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 03040aa..7592756 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -217,6 +217,18 @@ EDID Helper Functions Reference
>  .. kernel-doc:: drivers/gpu/drm/drm_edid.c
>     :export:
>  
> +SCDC Helper Functions Reference
> +===============================
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_scdc_helper.c
> +   :doc: scdc helpers
> +
> +.. kernel-doc:: include/drm/drm_scdc_helper.h
> +   :internal:
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_scdc_helper.c
> +   :export:
> +
>  Rectangle Utilities Reference
>  =============================
>  
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 92de399..ad91cd9 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -31,7 +31,8 @@ drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
>  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>  		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
>  		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
> -		drm_simple_kms_helper.o drm_modeset_helper.o
> +		drm_simple_kms_helper.o drm_modeset_helper.o \
> +		drm_scdc_helper.o
>  
>  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> diff --git a/drivers/gpu/drm/drm_scdc_helper.c b/drivers/gpu/drm/drm_scdc_helper.c
> new file mode 100644
> index 0000000..fe0e121
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_scdc_helper.c
> @@ -0,0 +1,111 @@
> +/*
> + * Copyright (c) 2015 NVIDIA Corporation. All rights reserved.
> + *
> + * 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, sub license,
> + * 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 NON-INFRINGEMENT. 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 <linux/slab.h>
> +
> +#include <drm/drm_scdc_helper.h>
> +
> +/**
> + * DOC: scdc helpers
> + *
> + * Status and Control Data Channel (SCDC) is a mechanism introduced by the
> + * HDMI 2.0 specification. It is a point-to-point protocol that allows the
> + * HDMI source and HDMI sink to exchange data. The same I2C interface that
> + * is used to access EDID serves as the transport mechanism for SCDC.
> + */
> +
> +#define SCDC_I2C_SLAVE_ADDRESS 0x54
> +
> +/**
> + * drm_scdc_read - read a block of data from SCDC
> + * @adapter: I2C controller
> + * @offset: start offset of block to read
> + * @buffer: return location for the block to read
> + * @size: size of the block to read
> + *
> + * Reads a block of data from SCDC, starting at a given offset.
> + *
> + * Returns:
> + * The number of bytes read from SCDC or a negative error code on failure.
> + */
> +ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
> +		      size_t size)
> +{
> +	struct i2c_msg msgs[2] = {
> +		{
> +			.addr = SCDC_I2C_SLAVE_ADDRESS,
> +			.flags = 0,
> +			.len = 1,
> +			.buf = &offset,
> +		}, {
> +			.addr = SCDC_I2C_SLAVE_ADDRESS,
> +			.flags = I2C_M_RD,
> +			.len = size,
> +			.buf = buffer,
> +		}
> +	};
> +
> +	return i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> +}
> +EXPORT_SYMBOL(drm_scdc_read);
> +
> +/**
> + * drm_scdc_write - write a block of data to SCDC
> + * @adapter: I2C controller
> + * @offset: start offset of block to write
> + * @buffer: block of data to write
> + * @size: size of the block to write
> + *
> + * Writes a block of data to SCDC, starting at a given offset.
> + *
> + * Returns:
> + * The number of bytes written to SCDC or a negative error code on failure.
> + */
> +ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
> +		       const void *buffer, size_t size)
> +{
> +	struct i2c_msg msg = {
> +		.addr = SCDC_I2C_SLAVE_ADDRESS,
> +		.flags = 0,
> +		.len = 1 + size,
> +		.buf = NULL,
> +	};
> +	void *data;
> +	int err;
> +
> +	data = kmalloc(1 + size, GFP_TEMPORARY);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	msg.buf = data;
> +
> +	memcpy(data, &offset, sizeof(offset));
> +	memcpy(data + 1, buffer, size);
> +
> +	err = i2c_transfer(adapter, &msg, 1);
> +
> +	kfree(data);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(drm_scdc_write);
> diff --git a/include/drm/drm_scdc_helper.h b/include/drm/drm_scdc_helper.h
> new file mode 100644
> index 0000000..93b07bc
> --- /dev/null
> +++ b/include/drm/drm_scdc_helper.h
> @@ -0,0 +1,132 @@
> +/*
> + * Copyright (c) 2015 NVIDIA Corporation. All rights reserved.
> + *
> + * 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, sub license,
> + * 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 NON-INFRINGEMENT. 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.
> + */
> +
> +#ifndef DRM_SCDC_HELPER_H
> +#define DRM_SCDC_HELPER_H
> +
> +#include <linux/i2c.h>
> +#include <linux/types.h>
> +
> +#define SCDC_SINK_VERSION 0x01
> +
> +#define SCDC_SOURCE_VERSION 0x02
> +
> +#define SCDC_UPDATE_0 0x10
> +#define  SCDC_READ_REQUEST_TEST (1 << 2)
> +#define  SCDC_CED_UPDATE (1 << 1)
> +#define  SCDC_STATUS_UPDATE (1 << 0)
> +
> +#define SCDC_UPDATE_1 0x11
> +
> +#define SCDC_TMDS_CONFIG 0x20
> +#define  SCDC_TMDS_BIT_CLOCK_RATIO_BY_40 (1 << 1)
> +#define  SCDC_TMDS_BIT_CLOCK_RATIO_BY_10 (0 << 1)
> +#define  SCDC_SCRAMBLING_ENABLE (1 << 0)
> +
> +#define SCDC_SCRAMBLER_STATUS 0x21
> +#define  SCDC_SCRAMBLING_STATUS (1 << 0)
> +
> +#define SCDC_CONFIG_0 0x30
> +#define  SCDC_READ_REQUEST_ENABLE (1 << 0)
> +
> +#define SCDC_STATUS_FLAGS_0 0x40
> +#define  SCDC_CH2_LOCK (1 < 3)
> +#define  SCDC_CH1_LOCK (1 < 2)
> +#define  SCDC_CH0_LOCK (1 < 1)
> +#define  SCDC_CH_LOCK_MASK (SCDC_CH2_LOCK | SCDC_CH1_LOCK | SCDC_CH0_LOCK)
> +#define  SCDC_CLOCK_DETECT (1 << 0)
> +
> +#define SCDC_STATUS_FLAGS_1 0x41
> +
> +#define SCDC_ERR_DET_0_L 0x50
> +#define SCDC_ERR_DET_0_H 0x51
> +#define SCDC_ERR_DET_1_L 0x52
> +#define SCDC_ERR_DET_1_H 0x53
> +#define SCDC_ERR_DET_2_L 0x54
> +#define SCDC_ERR_DET_2_H 0x55
> +#define  SCDC_CHANNEL_VALID (1 << 7)
> +
> +#define SCDC_ERR_DET_CHECKSUM 0x56
> +
> +#define SCDC_TEST_CONFIG_0 0xc0
> +#define  SCDC_TEST_READ_REQUEST (1 << 7)
> +#define  SCDC_TEST_READ_REQUEST_DELAY(x) ((x) & 0x7f)
> +
> +#define SCDC_MANUFACTURER_IEEE_OUI 0xd0
> +#define SCDC_MANUFACTURER_IEEE_OUI_SIZE 3
> +
> +#define SCDC_DEVICE_ID 0xd3
> +#define SCDC_DEVICE_ID_SIZE 8
> +
> +#define SCDC_DEVICE_HARDWARE_REVISION 0xdb
> +#define  SCDC_DEVICE_HARDWARE_REVISION_MAJOR(x) (((x) >> 4) & 0xf)
> +#define  SCDC_DEVICE_HARDWARE_REVISION_MINOR(x) (((x) >> 0) & 0xf)
> +
> +#define SCDC_DEVICE_SOFTWARE_MAJOR_REVISION 0xdc
> +#define SCDC_DEVICE_SOFTWARE_MINOR_REVISION 0xdd
> +
> +#define SCDC_MANUFACTURER_SPECIFIC 0xde
> +#define SCDC_MANUFACTURER_SPECIFIC_SIZE 34
> +
> +ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
> +		      size_t size);
> +ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
> +		       const void *buffer, size_t size);
> +
> +/**
> + * drm_scdc_readb - read a single byte from SCDC
> + * @adapter: I2C adapter
> + * @offset: offset of register to read
> + * @value: return location for the register value
> + *
> + * Reads a single byte from SCDC. This is a convenience wrapper around the
> + * drm_scdc_read() function.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +static inline int drm_scdc_readb(struct i2c_adapter *adapter, u8 offset,
> +				 u8 *value)
> +{
> +	return drm_scdc_read(adapter, offset, value, sizeof(*value));
> +}
> +
> +/**
> + * drm_scdc_writeb - write a single byte to SCDC
> + * @adapter: I2C adapter
> + * @offset: offset of register to read
> + * @value: return location for the register value
> + *
> + * Writes a single byte to SCDC. This is a convenience wrapper around the
> + * drm_scdc_write() function.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +static inline int drm_scdc_writeb(struct i2c_adapter *adapter, u8 offset,
> +				  u8 value)
> +{
> +	return drm_scdc_write(adapter, offset, &value, sizeof(value));
> +}
> +
> +#endif

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

Re: [PATCH 1/6] drm: Add SCDC helpers

[Sharma, Shashank]

Sure, Thanks for the information, will add that in V2.   

Regards
Shashank
-----Original Message-----
From: Jani Nikula [mailto:jani.nikula@linux.intel.com] 
Sent: Thursday, February 2, 2017 4:55 PM
To: Sharma, Shashank <shashank.sharma@intel.com>; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; ville.syrjala@linux.intel.com
Cc: jose.abreu@synopsys.com; =daniel.vetter@intel.com; Thierry Reding <treding@nvidia.com>; thierry.reding@gmail.com
Subject: Re: [Intel-gfx] [PATCH 1/6] drm: Add SCDC helpers

On Wed, 01 Feb 2017, Shashank Sharma <shashank.sharma@intel.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> SCDC is a mechanism defined in the HDMI 2.0 specification that allows 
> the source and sink devices to communicate.
>
> This commit introduces helpers to access the SCDC and provides the 
> symbolic names for the various registers defined in the specification.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

One process note, you need to add your own Signed-off-by under the original author's sign-off, even when you're posting them unchanged.

Signed-off-by means https://developercertificate.org/

BR,
Jani.


> ---
>  Documentation/gpu/drm-kms-helpers.rst |  12 ++++
>  drivers/gpu/drm/Makefile              |   3 +-
>  drivers/gpu/drm/drm_scdc_helper.c     | 111 ++++++++++++++++++++++++++++
>  include/drm/drm_scdc_helper.h         | 132 ++++++++++++++++++++++++++++++++++
>  4 files changed, 257 insertions(+), 1 deletion(-)  create mode 100644 
> drivers/gpu/drm/drm_scdc_helper.c  create mode 100644 
> include/drm/drm_scdc_helper.h
>
> diff --git a/Documentation/gpu/drm-kms-helpers.rst 
> b/Documentation/gpu/drm-kms-helpers.rst
> index 03040aa..7592756 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -217,6 +217,18 @@ EDID Helper Functions Reference  .. kernel-doc:: 
> drivers/gpu/drm/drm_edid.c
>     :export:
>  
> +SCDC Helper Functions Reference
> +===============================
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_scdc_helper.c
> +   :doc: scdc helpers
> +
> +.. kernel-doc:: include/drm/drm_scdc_helper.h
> +   :internal:
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_scdc_helper.c
> +   :export:
> +
>  Rectangle Utilities Reference
>  =============================
>  
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 
> 92de399..ad91cd9 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -31,7 +31,8 @@ drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o 
> drm_debugfs_crc.o  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>  		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
>  		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
> -		drm_simple_kms_helper.o drm_modeset_helper.o
> +		drm_simple_kms_helper.o drm_modeset_helper.o \
> +		drm_scdc_helper.o
>  
>  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o diff 
> --git a/drivers/gpu/drm/drm_scdc_helper.c 
> b/drivers/gpu/drm/drm_scdc_helper.c
> new file mode 100644
> index 0000000..fe0e121
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_scdc_helper.c
> @@ -0,0 +1,111 @@
> +/*
> + * Copyright (c) 2015 NVIDIA Corporation. All rights reserved.
> + *
> + * 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, sub 
> +license,
> + * 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 NON-INFRINGEMENT. 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 <linux/slab.h>
> +
> +#include <drm/drm_scdc_helper.h>
> +
> +/**
> + * DOC: scdc helpers
> + *
> + * Status and Control Data Channel (SCDC) is a mechanism introduced 
> +by the
> + * HDMI 2.0 specification. It is a point-to-point protocol that 
> +allows the
> + * HDMI source and HDMI sink to exchange data. The same I2C interface 
> +that
> + * is used to access EDID serves as the transport mechanism for SCDC.
> + */
> +
> +#define SCDC_I2C_SLAVE_ADDRESS 0x54
> +
> +/**
> + * drm_scdc_read - read a block of data from SCDC
> + * @adapter: I2C controller
> + * @offset: start offset of block to read
> + * @buffer: return location for the block to read
> + * @size: size of the block to read
> + *
> + * Reads a block of data from SCDC, starting at a given offset.
> + *
> + * Returns:
> + * The number of bytes read from SCDC or a negative error code on failure.
> + */
> +ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
> +		      size_t size)
> +{
> +	struct i2c_msg msgs[2] = {
> +		{
> +			.addr = SCDC_I2C_SLAVE_ADDRESS,
> +			.flags = 0,
> +			.len = 1,
> +			.buf = &offset,
> +		}, {
> +			.addr = SCDC_I2C_SLAVE_ADDRESS,
> +			.flags = I2C_M_RD,
> +			.len = size,
> +			.buf = buffer,
> +		}
> +	};
> +
> +	return i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs)); } 
> +EXPORT_SYMBOL(drm_scdc_read);
> +
> +/**
> + * drm_scdc_write - write a block of data to SCDC
> + * @adapter: I2C controller
> + * @offset: start offset of block to write
> + * @buffer: block of data to write
> + * @size: size of the block to write
> + *
> + * Writes a block of data to SCDC, starting at a given offset.
> + *
> + * Returns:
> + * The number of bytes written to SCDC or a negative error code on failure.
> + */
> +ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
> +		       const void *buffer, size_t size) {
> +	struct i2c_msg msg = {
> +		.addr = SCDC_I2C_SLAVE_ADDRESS,
> +		.flags = 0,
> +		.len = 1 + size,
> +		.buf = NULL,
> +	};
> +	void *data;
> +	int err;
> +
> +	data = kmalloc(1 + size, GFP_TEMPORARY);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	msg.buf = data;
> +
> +	memcpy(data, &offset, sizeof(offset));
> +	memcpy(data + 1, buffer, size);
> +
> +	err = i2c_transfer(adapter, &msg, 1);
> +
> +	kfree(data);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(drm_scdc_write);
> diff --git a/include/drm/drm_scdc_helper.h 
> b/include/drm/drm_scdc_helper.h new file mode 100644 index 
> 0000000..93b07bc
> --- /dev/null
> +++ b/include/drm/drm_scdc_helper.h
> @@ -0,0 +1,132 @@
> +/*
> + * Copyright (c) 2015 NVIDIA Corporation. All rights reserved.
> + *
> + * 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, sub 
> +license,
> + * 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 NON-INFRINGEMENT. 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.
> + */
> +
> +#ifndef DRM_SCDC_HELPER_H
> +#define DRM_SCDC_HELPER_H
> +
> +#include <linux/i2c.h>
> +#include <linux/types.h>
> +
> +#define SCDC_SINK_VERSION 0x01
> +
> +#define SCDC_SOURCE_VERSION 0x02
> +
> +#define SCDC_UPDATE_0 0x10
> +#define  SCDC_READ_REQUEST_TEST (1 << 2) #define  SCDC_CED_UPDATE (1 
> +<< 1) #define  SCDC_STATUS_UPDATE (1 << 0)
> +
> +#define SCDC_UPDATE_1 0x11
> +
> +#define SCDC_TMDS_CONFIG 0x20
> +#define  SCDC_TMDS_BIT_CLOCK_RATIO_BY_40 (1 << 1) #define  
> +SCDC_TMDS_BIT_CLOCK_RATIO_BY_10 (0 << 1) #define  
> +SCDC_SCRAMBLING_ENABLE (1 << 0)
> +
> +#define SCDC_SCRAMBLER_STATUS 0x21
> +#define  SCDC_SCRAMBLING_STATUS (1 << 0)
> +
> +#define SCDC_CONFIG_0 0x30
> +#define  SCDC_READ_REQUEST_ENABLE (1 << 0)
> +
> +#define SCDC_STATUS_FLAGS_0 0x40
> +#define  SCDC_CH2_LOCK (1 < 3)
> +#define  SCDC_CH1_LOCK (1 < 2)
> +#define  SCDC_CH0_LOCK (1 < 1)
> +#define  SCDC_CH_LOCK_MASK (SCDC_CH2_LOCK | SCDC_CH1_LOCK | 
> +SCDC_CH0_LOCK) #define  SCDC_CLOCK_DETECT (1 << 0)
> +
> +#define SCDC_STATUS_FLAGS_1 0x41
> +
> +#define SCDC_ERR_DET_0_L 0x50
> +#define SCDC_ERR_DET_0_H 0x51
> +#define SCDC_ERR_DET_1_L 0x52
> +#define SCDC_ERR_DET_1_H 0x53
> +#define SCDC_ERR_DET_2_L 0x54
> +#define SCDC_ERR_DET_2_H 0x55
> +#define  SCDC_CHANNEL_VALID (1 << 7)
> +
> +#define SCDC_ERR_DET_CHECKSUM 0x56
> +
> +#define SCDC_TEST_CONFIG_0 0xc0
> +#define  SCDC_TEST_READ_REQUEST (1 << 7) #define  
> +SCDC_TEST_READ_REQUEST_DELAY(x) ((x) & 0x7f)
> +
> +#define SCDC_MANUFACTURER_IEEE_OUI 0xd0 #define 
> +SCDC_MANUFACTURER_IEEE_OUI_SIZE 3
> +
> +#define SCDC_DEVICE_ID 0xd3
> +#define SCDC_DEVICE_ID_SIZE 8
> +
> +#define SCDC_DEVICE_HARDWARE_REVISION 0xdb #define  
> +SCDC_DEVICE_HARDWARE_REVISION_MAJOR(x) (((x) >> 4) & 0xf) #define  
> +SCDC_DEVICE_HARDWARE_REVISION_MINOR(x) (((x) >> 0) & 0xf)
> +
> +#define SCDC_DEVICE_SOFTWARE_MAJOR_REVISION 0xdc #define 
> +SCDC_DEVICE_SOFTWARE_MINOR_REVISION 0xdd
> +
> +#define SCDC_MANUFACTURER_SPECIFIC 0xde #define 
> +SCDC_MANUFACTURER_SPECIFIC_SIZE 34
> +
> +ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
> +		      size_t size);
> +ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
> +		       const void *buffer, size_t size);
> +
> +/**
> + * drm_scdc_readb - read a single byte from SCDC
> + * @adapter: I2C adapter
> + * @offset: offset of register to read
> + * @value: return location for the register value
> + *
> + * Reads a single byte from SCDC. This is a convenience wrapper 
> +around the
> + * drm_scdc_read() function.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +static inline int drm_scdc_readb(struct i2c_adapter *adapter, u8 offset,
> +				 u8 *value)
> +{
> +	return drm_scdc_read(adapter, offset, value, sizeof(*value)); }
> +
> +/**
> + * drm_scdc_writeb - write a single byte to SCDC
> + * @adapter: I2C adapter
> + * @offset: offset of register to read
> + * @value: return location for the register value
> + *
> + * Writes a single byte to SCDC. This is a convenience wrapper around 
> +the
> + * drm_scdc_write() function.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +static inline int drm_scdc_writeb(struct i2c_adapter *adapter, u8 offset,
> +				  u8 value)
> +{
> +	return drm_scdc_write(adapter, offset, &value, sizeof(value)); }
> +
> +#endif

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

Re: [PATCH 5/6] drm/i915: enable scrambling

[Ville Syrjälä]

On Thu, Feb 02, 2017 at 04:15:21PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 2/2/2017 3:32 PM, Ville Syrjälä wrote:
> > On Thu, Feb 02, 2017 at 11:23:19AM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 2/1/2017 10:06 PM, Ville Syrjälä wrote:
> >>> On Wed, Feb 01, 2017 at 06:14:40PM +0530, Shashank Sharma wrote:
> >>>> Geminilake platform has a native HDMI 2.0 controller, and is
> >>>> capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
> >>>> mendates scrambling for these higher clocks, for reduced RF footprint.
> >>>>
> >>>> This patch checks if the monitor supports scrambling, and if required,
> >>>> enables it during the modeset.
> >>>>
> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>>> ---
> >>>>    drivers/gpu/drm/i915/i915_reg.h   |  2 ++
> >>>>    drivers/gpu/drm/i915/intel_ddi.c  |  5 +++++
> >>>>    drivers/gpu/drm/i915/intel_drv.h  |  2 ++
> >>>>    drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++++++++
> >>>>    4 files changed, 51 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>>> index 495b789..cc85892 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>>> @@ -7807,6 +7807,8 @@ enum {
> >>>>    #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
> >>>>    #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
> >>>>    #define  TRANS_DDI_BFI_ENABLE		(1<<4)
> >>>> +#define  TRANS_DDI_HIGH_TMDS_CHAR_RATE	(1<<4)
> >>>> +#define  TRANS_DDI_HDMI_SCRAMBLING	(1<<0)
> >>>>    
> >>>>    /* DisplayPort Transport Control */
> >>>>    #define _DP_TP_CTL_A			0x64040
> >>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >>>> index 9a9a670..aea81ce 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >>>> @@ -1278,6 +1278,11 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
> >>>>    			temp |= TRANS_DDI_MODE_SELECT_HDMI;
> >>>>    		else
> >>>>    			temp |= TRANS_DDI_MODE_SELECT_DVI;
> >>>> +
> >>>> +		if (IS_GEMINILAKE(dev_priv)) {
> >>>> +			temp |= intel_hdmi_check_scrambling(intel_encoder,
> >>>> +				&intel_crtc->config->base.adjusted_mode);
> >>>> +		}
> >>>>    	} else if (type == INTEL_OUTPUT_ANALOG) {
> >>>>    		temp |= TRANS_DDI_MODE_SELECT_FDI;
> >>>>    		temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
> >>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>>> index 393f243..aafce7f 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_drv.h
> >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >>>> @@ -1588,6 +1588,8 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> >>>>    bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >>>>    			       struct intel_crtc_state *pipe_config,
> >>>>    			       struct drm_connector_state *conn_state);
> >>>> +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder,
> >>>> +				struct drm_display_mode *mode);
> >>>>    void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
> >>>>    
> >>>>    
> >>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >>>> index ebae2bd..92dd9bc 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >>>> @@ -1795,6 +1795,48 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
> >>>>    	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >>>>    }
> >>>>    
> >>>> +static void
> >>>> +intel_hdmi_enable_scrambling(struct drm_connector *connector)
> >>>> +{
> >>>> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> >>>> +	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> >>>> +	struct i2c_adapter *adapter =
> >>>> +			intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
> >>>> +
> >>>> +	if (!drm_enable_scrambling(connector, adapter, true))
> >>>> +		DRM_ERROR("Request to enable scrambling failed\n");
> >>>> +}
> >>> I don't like hiding this somewhere deep like this. It should be
> >>> somewhere much higher up.
> >> Why ? All we need to do here is enable two bits in transcoder control
> >> register, which is already being
> >> programmed in a calling function, so I dont see the use case, but I
> >> might be missing some bigger picture.
> >> Can you please elaborate on this ?
> > We're talking to the display here, which is rather surprising to happen
> > from a function thing that on the first glance doesn't even seem to
> > touch the hardware.
> I kind of agree with this, and I am also tempted now to shift this call 
> upwards, but I want to
> keep this close to port enabling as well, as the HDMI 2.0 spec says:
> "Max time period between a source enables scrambling on monitor, and 
> starts sending scrambled
>    video should be 100ms" else the sink might choose to disable scrambling.
> 
> Considering this, where do you think would be right place to enabled 
> scrambling on monitor ?

intel_enable_ddi() is where we presumbly enable the port,
so maybe there.

> 
> - Shashank
> >>> And I'm thinkign we might want to track the scrambler state
> >>> in the crtc state.
> >> Yes, that's a pretty good way to track dynamic status of scrambler, do
> >> you think we should add this in
> >> drm_crtc_state itself ?
> > I'd just start with intel specific state and later we can see what
> > everyone else wants to do and potentially unify a bit.
> >
> >> - Shashank
> >>>> +
> >>>> +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder,
> >>>> +				struct drm_display_mode *mode)
> >>>> +{
> >>>> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> >>>> +	struct drm_connector *connector = &intel_hdmi->attached_connector->base;
> >>>> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
> >>>> +	uint32_t hdmi_config = 0;
> >>>> +
> >>>> +	DRM_DEBUG_KMS("Checking scrambling for enc:%s connector:%s\n",
> >>>> +			intel_encoder->base.name, connector->name);
> >>>> +
> >>>> +	if (mode->clock < 340000) {
> >>>> +		if (hdmi_info->scr_info.low_clocks) {
> >>>> +			intel_hdmi_enable_scrambling(connector);
> >>>> +			hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING;
> >>>> +		}
> >>>> +		return hdmi_config;
> >>>> +	}
> >>>> +
> >>>> +	/* Enable scrambling for clocks > 340M */
> >>>> +	if (hdmi_info->scr_info.supported) {
> >>>> +		intel_hdmi_enable_scrambling(connector);
> >>>> +		hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING;
> >>>> +	}
> >>>> +
> >>>> +	/* Scrambling or not, if clock > 340M, set high char rate */
> >>>> +	hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> >>>> +	return hdmi_config;
> >>>> +}
> >>>> +
> >>>>    static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
> >>>>    			     enum port port)
> >>>>    {
> >>>> -- 
> >>>> 1.9.1

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

Re: [PATCH 3/6] drm/edid: detect SCDC support in HF-VSDB

[Thierry Reding]

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

On Thu, Feb 02, 2017 at 10:58:43AM +0530, Sharma, Shashank wrote:
> Thanks for the review Thierry. My comments inline.
> 
> Regards
> Shashank
> On 2/1/2017 9:40 PM, Thierry Reding wrote:
> > On Wed, Feb 01, 2017 at 06:14:38PM +0530, Shashank Sharma wrote:
> > > This patch does following:
> > > - Adds a new structure (drm_hdmi_info) in drm_display_info.
> > >    This structure will be used to save and indicate if sink
> > >    supports advance HDMI 2.0 features
> > "advanced"
> got it.
> > 
> > > - Checks the HF-VSDB block for presence of SCDC, and marks it
> > >    in hdmi_info structure.
> > "drm_hdmi_info structure"?
> yep, sure.
> > > - If SCDC is present, checks if sink is capable of generating
> > >    scdc read request, and marks it in hdmi_info structure.
> > "SCDC" to be consistent and because it's an abbreviation.
> Agree.
> > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > > ---
> > >   drivers/gpu/drm/drm_edid.c  | 14 ++++++++++++++
> > >   include/drm/drm_connector.h | 26 ++++++++++++++++++++++++++
> > >   2 files changed, 40 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index 96d3e47..37902e5 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -3802,6 +3802,18 @@ enum hdmi_quantization_range
> > >   }
> > >   EXPORT_SYMBOL(drm_default_rgb_quant_range);
> > > +static void drm_detect_hdmi_scdc(struct drm_connector *connector,
> > > +				 const u8 *hf_vsdb)
> > > +{
> > > +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi_info;
> > > +
> > > +	if (hf_vsdb[6] & 0x80) {
> > > +		hdmi->scdc_supported = true;
> > > +		if (hf_vsdb[6] & 0x40)
> > > +			hdmi->scdc_rr = true;
> > > +	}
> > > +}
> > > +
> > >   static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> > >   					   const u8 *hdmi)
> > >   {
> > > @@ -3916,6 +3928,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> > >   		if (cea_db_is_hdmi_vsdb(db))
> > >   			drm_parse_hdmi_vsdb_video(connector, db);
> > > +		if (cea_db_is_hdmi_forum_vsdb(db))
> > > +			drm_detect_hdmi_scdc(connector, db);
> > >   	}
> > >   }
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index e5e1edd..2435598 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -87,6 +87,27 @@ enum subpixel_order {
> > >   	SubPixelVerticalRGB,
> > >   	SubPixelVerticalBGR,
> > >   	SubPixelNone,
> > > +
> > > +};
> > > +
> > > +/**
> > > + * struct drm_hdmi_info - runtime data about the connected sink
> > Maybe "connected HDMI sink"?
> Agree.
> > > + *
> > > + * Describes if a given hdmi display supports advance HDMI 2.0 featutes.
> > "HDMI", "advanced", "features"
> Oops, got it :-)
> > > + * This information is available in CEA-861-F extension blocks (like
> > > + * HF-VSDB)
> > This should be terminated by a full-stop.
> Ok
> > > + * For sinks which provide an EDID this can be filled out by calling
> > > + * drm_add_edid_modes().
> > And maybe make this sentence start right after the one above rather than
> > breaking it to the next line.
> Ok
> > I'm not sure how useful this line is. Most
> > driver will be calling drm_add_edid_modes() anyway, but the above makes
> > it sound like drm_add_edid_modes() is something you have to explicitly
> > call to get these fields parsed.
> Mostly a 'yy' and 'p' from the function above, but makes sense, I can remove
> this line.
> > > + */
> > > +struct drm_hdmi_info {
> > > +	/**
> > > +	 * @scdc_supported: status control & data channel present.
> > > +	 */
> > > +	bool scdc_supported;
> > > +	/**
> > > +	 * @scdc_rr: sink is capable of generating scdc read request.
> > > +	 */
> > > +	bool scdc_rr;
> > >   };
> > >   /**
> > > @@ -188,6 +209,11 @@ struct drm_display_info {
> > >   	 * @cea_rev: CEA revision of the HDMI sink.
> > >   	 */
> > >   	u8 cea_rev;
> > > +
> > > +	/**
> > > +	 * @hdmi_info: advance features of a HDMI sink.
> > > +	 */
> > > +	struct drm_hdmi_info hdmi_info;
> > I think we can safely drop the _info suffix on the field name. It's
> > already inside a structure that carries this suffix.
> Sure, should I call it hdmi_sink OR connected_hdmi ?

No, I think just plain "hdmi" would be fine. This is part of
drm_display_info, which kind of implies that it's a sink, and I think
it's also fair to assume that this isn't valid if nothing's connected.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 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 4/6] drm: scrambling support in drm layer

[Thierry Reding]

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

On Thu, Feb 02, 2017 at 11:08:22AM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 2/1/2017 10:02 PM, Thierry Reding wrote:
> > On Wed, Feb 01, 2017 at 06:14:39PM +0530, Shashank Sharma wrote:
> > > HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
> > > than 340Mhz. This patch adds few new functions in drm layer for
> > > core drivers to enable/disable scrambling.
> > > 
> > > This patch adds:
> > > - A function to detect scrambling support parsing HF-VSDB
> > > - A function to check scrambling status runtime using SCDC read.
> > > - Two functions to enable/disable scrambling using SCDC read/write.
> > > - Few new bools to reflect scrambling support and status.
> > > 
> > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > > ---
> > >   drivers/gpu/drm/drm_edid.c  | 131 +++++++++++++++++++++++++++++++++++++++++++-
> > >   include/drm/drm_connector.h |  24 ++++++++
> > >   include/drm/drm_edid.h      |   6 +-
> > >   3 files changed, 159 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index 37902e5..f0d940a 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -37,6 +37,7 @@
> > >   #include <drm/drm_edid.h>
> > >   #include <drm/drm_encoder.h>
> > >   #include <drm/drm_displayid.h>
> > > +#include <drm/drm_scdc_helper.h>
> > >   #define version_greater(edid, maj, min) \
> > >   	(((edid)->version > (maj)) || \
> > > @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector *connector,
> > >   	}
> > >   }
> > > +static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
> > > +				 const u8 *hf_vsdb)
> > > +{
> > > +	struct drm_display_info *display = &connector->display_info;
> > > +	struct drm_hdmi_info *hdmi = &display->hdmi_info;
> > > +
> > > +	/*
> > > +	 * All HDMI 2.0 monitors must support scrambling at rates > 340M.
> > In comments below you use Mhz as the abbreviations. This should be
> > consistent. Also I think "MHz" is actually the correct spelling.
> Agree.
> > > +	 * And as per the spec, three factors confirm this:
> > > +	 * * Availability of a HF-VSDB block in EDID (check)
> > > +	 * * Non zero Max_TMDS_Char_Rate filed in HF-VSDB
> > > +	 * * SCDC support available
> > > +	 * Lets check it out.
> > > +	 */
> > > +
> > > +	if (hf_vsdb[5]) {
> > > +		display->max_tmds_clock = hf_vsdb[5] * 5000;
> > > +		DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> > > +			display->max_tmds_clock);
> > > +
> > > +		if (hdmi->scdc_supported) {
> > > +			hdmi->scr_info.supported = true;
> > > +
> > > +			/* Few sinks support scrambling for cloks < 340M */
> > > +			if ((hf_vsdb[6] & 0x8))
> > > +				hdmi->scr_info.low_clocks = true;
> > > +		}
> > > +	}
> > > +}
> > > +
> > > +/**
> > > + * drm_check_scrambling_status - what is status of scrambling?
> > > + * @adapter: i2c adapter for SCDC channel
> > "I2C", same in other parts of this patch.
> Got it.
> > > + *
> > > + * Read the scrambler status over SCDC channel, and check the
> > > + * scrambling status.
> > > + *
> > > + * Return: True if the scrambling is enabled, false otherwise.
> > I think the rest of DRM/KMS kerneldoc tries to use "Returns:\n" as a
> > standard way to document return values.
> Ok.
> > > + */
> > > +
> > > +bool drm_check_scrambling_status(struct i2c_adapter *adapter)
> > Maybe use a drm_scdc_*() prefix for this to make it more consistent with
> > other SCDC API.
> > 
> > While at it, would this not be better located in drm_scdc.c along with
> > the other helpers? drm_edid.c is more focussed on the parsing aspects of
> > all things EDID.
> Yeah, the same is mentioned by Ville too, will do that.
> > > +{
> > > +	u8 status;
> > > +
> > > +	if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) {
> > How about storing the error code...
> > 
> > > +		DRM_ERROR("Failed to read scrambling status\n");
> > ... and making it part of the error message? Sometimes its useful to
> > know what exact error triggered this because it helps narrowing down
> > where things went wrong.
> Agree, in fact while debugging and testing this patch series, I had to print
> it explicitly.
> > 
> > > +		return false;
> > > +	}
> > > +
> > > +	status &= SCDC_SCRAMBLING_STATUS;
> > > +	return status != 0;
> > Maybe make this a single line:
> > 
> > 	return (status & SCDC_SCRAMBLING_STATUS) != 0;
> Got it.
> > 
> > > +}
> > > +
> > > +/**
> > > + * drm_enable_scrambling - enable scrambling
> > > + * @connector: target drm_connector
> > "target DRM connector"?
> Got it.
> > > + * @adapter: i2c adapter for SCDC channel
> > > + * @force: enable scrambling, even if its already enabled
> > > + *
> > > + * Write the TMDS config over SCDC channel, and enable scrambling
> > > + * Return: True if scrambling is successfully enabled, false otherwise.
> > > + */
> > > +
> > > +bool drm_enable_scrambling(struct drm_connector *connector,
> > > +					struct i2c_adapter *adapter, bool force)
> > I think I'd move this to drm_scdc.c as well because it primarily
> > operates on SCDC. If you do so, might be worth making adapter the first
> > argument for consistency with other SCDC API.
> Agree, will move it.
> > > +{
> > > +	u8 config;
> > > +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
> > > +
> > > +	if (hdmi_info->scr_info.status && !force) {
> > > +		DRM_DEBUG_KMS("Scrambling already enabled\n");
> > > +		return true;
> > > +	}
> > > +
> > > +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
> > > +		DRM_ERROR("Failed to read tmds config\n");
> > Maybe also print the error code?
> Ok.
> > > +		return false;
> > > +	}
> > > +
> > > +	config |= SCDC_SCRAMBLING_ENABLE;
> > > +
> > > +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
> > > +		DRM_ERROR("Failed to enable scrambling, write error\n");
> > Same here.
> Ok
> > > +		return false;
> > > +	}
> > > +
> > > +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
> > > +	return hdmi_info->scr_info.status;
> > > +}
> > > +
> > > +/**
> > > + * drm_disable_scrambling - disable scrambling
> > > + * @connector: target drm_connector
> > > + * @adapter: i2c adapter for SCDC channel
> > > + * @force: disable scrambling, even if its already disabled
> > > + *
> > > + * Write the TMDS config over SCDC channel, and disable scrambling
> > > + * Return: True if scrambling is successfully disabled, false otherwise.
> > > + */
> > > +bool drm_disable_scrambling(struct drm_connector *connector,
> > > +					struct i2c_adapter *adapter, bool force)
> > > +{
> > > +	u8 config;
> > > +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
> > > +
> > > +	if (!hdmi_info->scr_info.status && !force) {
> > > +		DRM_DEBUG_KMS("Scrambling already disabled\n");
> > > +		return true;
> > > +	}
> > > +
> > > +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
> > > +		DRM_ERROR("Failed to read tmds config\n");
> > > +		return false;
> > > +	}
> > > +
> > > +	config &= ~SCDC_SCRAMBLING_ENABLE;
> > > +
> > > +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
> > > +		DRM_ERROR("Failed to enable scrambling, write error\n");
> > > +		return false;
> > > +	}
> > > +
> > > +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
> > > +	return !hdmi_info->scr_info.status;
> > > +}
> > Same comments as for drm_enable_scrambling().
> Got it.
> > > @@ -3928,8 +4055,10 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> > >   		if (cea_db_is_hdmi_vsdb(db))
> > >   			drm_parse_hdmi_vsdb_video(connector, db);
> > > -		if (cea_db_is_hdmi_forum_vsdb(db))
> > > +		if (cea_db_is_hdmi_forum_vsdb(db)) {
> > >   			drm_detect_hdmi_scdc(connector, db);
> > > +			drm_detect_hdmi_scrambling(connector, db);
> > > +		}
> > >   	}
> > >   }
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index 2435598..b9735bd 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -90,6 +90,26 @@ enum subpixel_order {
> > >   };
> > > +
> > > +/**
> > > + * struct scrambling: sink's scrambling support.
> > > + */
> > > +struct scrambling {
> > > +	/**
> > > +	 * @supported: scrambling supported for rates > 340Mhz.
> > I think it's common to separate number and unit by a space, so "340
> > MHz".
> Got it.
> > > +	 */
> > > +	bool supported;
> > > +	/**
> > > +	 * @low_clocks: scrambling supported for rates <= 340Mhz.
> > > +	 */
> > > +	bool low_clocks;
> > Maybe "low_rates" because a clock is technically the source of a signal
> > that oscillates at the given rate.
> Agree, will change it.
> > > +	/**
> > > +	 * @status: scrambling enabled/disabled ?
> > > +	 */
> > > +	bool status;
> > > +};
> > > +
> > > +
> > >   /**
> > >    * struct drm_hdmi_info - runtime data about the connected sink
> > >    *
> > > @@ -108,6 +128,10 @@ struct drm_hdmi_info {
> > >   	 * @scdc_rr: sink is capable of generating scdc read request.
> > >   	 */
> > >   	bool scdc_rr;
> > > +	/**
> > > +	 * scr_info: sink's scrambling support and capabilities.
> > > +	 */
> > > +	struct scrambling scr_info;
> > Again, I think I'd drop _info and instead spell out "scrambling" to make
> > this easier to remember.
> Sure, can be done.
> > 
> > Also I'm wondering why scdc_supported and scdc_rr are not in a structure
> > if scrambling info is. Also since scrambling depends on SCDC, would it
> > make sense to embed it in a struct drm_hdmi_scdc_info?
> My opinion while designing was:
> - SCDC rr support is platform specific, and a platform can choose not to
> support read_req but just allow
>   scrambling using scdc read and write, hence kept that separate
> - You need to look into this internal structure, only if scdc is supported.
> - Also, SCDC registers can be used beyond scrambling too, like for error
> detection, and other cases, so I thought
>   it would be better to keep it independent.
> 
> Does it make sense ?

Yes, I think that makes sense, but it's not what I was trying to say. =)
What I was trying to say is that read request and scrambling are defined
in the SCDC specification, and therefore they require SCDC. That's why I
think the below is a natural representation because it captures the
dependency in a hierarchy:

> > 	struct drm_hdmi_scdc_scrambling_info {
> > 		bool supported;
> > 		bool low_rates;
> > 		bool status;
> > 	};
> > 
> > 	struct drm_hdmi_scdc_info {
> > 		bool supported;
> > 		bool read_request;
> > 
> > 		struct drm_hdmi_scdc_scrambling_info scrambling;
> > 	};

I should have added to the above:

	struct drm_hdmi_info {
		struct drm_hdmi_scdc_info scdc;
	};

So when conditionalizing code this allows for a very natural ordering of
the code:

	struct drm_display_info *info = ...;
	struct drm_hdmi_scdc_info *scdc = &info->hdmi.scdc;

	if (scdc->supported) {
		...

		if (scdc->read_request) {
			...
			e.g. set up handler for read requests
			...
		}

		...

		if (scdc->scrambling.supported) {
			if (mode->clock >= 340000 || scdc->scrambling.low_rates) {
				...
				set up scrambling
				...;
			}
		}

		...
	}

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/6] drm: scrambling support in drm layer

[Sharma, Shashank]

Regards

Shashank


On 2/2/2017 11:43 PM, Thierry Reding wrote:
> On Thu, Feb 02, 2017 at 11:08:22AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 2/1/2017 10:02 PM, Thierry Reding wrote:
>>> On Wed, Feb 01, 2017 at 06:14:39PM +0530, Shashank Sharma wrote:
>>>> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
>>>> than 340Mhz. This patch adds few new functions in drm layer for
>>>> core drivers to enable/disable scrambling.
>>>>
>>>> This patch adds:
>>>> - A function to detect scrambling support parsing HF-VSDB
>>>> - A function to check scrambling status runtime using SCDC read.
>>>> - Two functions to enable/disable scrambling using SCDC read/write.
>>>> - Few new bools to reflect scrambling support and status.
>>>>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_edid.c  | 131 +++++++++++++++++++++++++++++++++++++++++++-
>>>>    include/drm/drm_connector.h |  24 ++++++++
>>>>    include/drm/drm_edid.h      |   6 +-
>>>>    3 files changed, 159 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>> index 37902e5..f0d940a 100644
>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>> @@ -37,6 +37,7 @@
>>>>    #include <drm/drm_edid.h>
>>>>    #include <drm/drm_encoder.h>
>>>>    #include <drm/drm_displayid.h>
>>>> +#include <drm/drm_scdc_helper.h>
>>>>    #define version_greater(edid, maj, min) \
>>>>    	(((edid)->version > (maj)) || \
>>>> @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector *connector,
>>>>    	}
>>>>    }
>>>> +static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
>>>> +				 const u8 *hf_vsdb)
>>>> +{
>>>> +	struct drm_display_info *display = &connector->display_info;
>>>> +	struct drm_hdmi_info *hdmi = &display->hdmi_info;
>>>> +
>>>> +	/*
>>>> +	 * All HDMI 2.0 monitors must support scrambling at rates > 340M.
>>> In comments below you use Mhz as the abbreviations. This should be
>>> consistent. Also I think "MHz" is actually the correct spelling.
>> Agree.
>>>> +	 * And as per the spec, three factors confirm this:
>>>> +	 * * Availability of a HF-VSDB block in EDID (check)
>>>> +	 * * Non zero Max_TMDS_Char_Rate filed in HF-VSDB
>>>> +	 * * SCDC support available
>>>> +	 * Lets check it out.
>>>> +	 */
>>>> +
>>>> +	if (hf_vsdb[5]) {
>>>> +		display->max_tmds_clock = hf_vsdb[5] * 5000;
>>>> +		DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
>>>> +			display->max_tmds_clock);
>>>> +
>>>> +		if (hdmi->scdc_supported) {
>>>> +			hdmi->scr_info.supported = true;
>>>> +
>>>> +			/* Few sinks support scrambling for cloks < 340M */
>>>> +			if ((hf_vsdb[6] & 0x8))
>>>> +				hdmi->scr_info.low_clocks = true;
>>>> +		}
>>>> +	}
>>>> +}
>>>> +
>>>> +/**
>>>> + * drm_check_scrambling_status - what is status of scrambling?
>>>> + * @adapter: i2c adapter for SCDC channel
>>> "I2C", same in other parts of this patch.
>> Got it.
>>>> + *
>>>> + * Read the scrambler status over SCDC channel, and check the
>>>> + * scrambling status.
>>>> + *
>>>> + * Return: True if the scrambling is enabled, false otherwise.
>>> I think the rest of DRM/KMS kerneldoc tries to use "Returns:\n" as a
>>> standard way to document return values.
>> Ok.
>>>> + */
>>>> +
>>>> +bool drm_check_scrambling_status(struct i2c_adapter *adapter)
>>> Maybe use a drm_scdc_*() prefix for this to make it more consistent with
>>> other SCDC API.
>>>
>>> While at it, would this not be better located in drm_scdc.c along with
>>> the other helpers? drm_edid.c is more focussed on the parsing aspects of
>>> all things EDID.
>> Yeah, the same is mentioned by Ville too, will do that.
>>>> +{
>>>> +	u8 status;
>>>> +
>>>> +	if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) {
>>> How about storing the error code...
>>>
>>>> +		DRM_ERROR("Failed to read scrambling status\n");
>>> ... and making it part of the error message? Sometimes its useful to
>>> know what exact error triggered this because it helps narrowing down
>>> where things went wrong.
>> Agree, in fact while debugging and testing this patch series, I had to print
>> it explicitly.
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	status &= SCDC_SCRAMBLING_STATUS;
>>>> +	return status != 0;
>>> Maybe make this a single line:
>>>
>>> 	return (status & SCDC_SCRAMBLING_STATUS) != 0;
>> Got it.
>>>> +}
>>>> +
>>>> +/**
>>>> + * drm_enable_scrambling - enable scrambling
>>>> + * @connector: target drm_connector
>>> "target DRM connector"?
>> Got it.
>>>> + * @adapter: i2c adapter for SCDC channel
>>>> + * @force: enable scrambling, even if its already enabled
>>>> + *
>>>> + * Write the TMDS config over SCDC channel, and enable scrambling
>>>> + * Return: True if scrambling is successfully enabled, false otherwise.
>>>> + */
>>>> +
>>>> +bool drm_enable_scrambling(struct drm_connector *connector,
>>>> +					struct i2c_adapter *adapter, bool force)
>>> I think I'd move this to drm_scdc.c as well because it primarily
>>> operates on SCDC. If you do so, might be worth making adapter the first
>>> argument for consistency with other SCDC API.
>> Agree, will move it.
>>>> +{
>>>> +	u8 config;
>>>> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
>>>> +
>>>> +	if (hdmi_info->scr_info.status && !force) {
>>>> +		DRM_DEBUG_KMS("Scrambling already enabled\n");
>>>> +		return true;
>>>> +	}
>>>> +
>>>> +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
>>>> +		DRM_ERROR("Failed to read tmds config\n");
>>> Maybe also print the error code?
>> Ok.
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	config |= SCDC_SCRAMBLING_ENABLE;
>>>> +
>>>> +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
>>>> +		DRM_ERROR("Failed to enable scrambling, write error\n");
>>> Same here.
>> Ok
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
>>>> +	return hdmi_info->scr_info.status;
>>>> +}
>>>> +
>>>> +/**
>>>> + * drm_disable_scrambling - disable scrambling
>>>> + * @connector: target drm_connector
>>>> + * @adapter: i2c adapter for SCDC channel
>>>> + * @force: disable scrambling, even if its already disabled
>>>> + *
>>>> + * Write the TMDS config over SCDC channel, and disable scrambling
>>>> + * Return: True if scrambling is successfully disabled, false otherwise.
>>>> + */
>>>> +bool drm_disable_scrambling(struct drm_connector *connector,
>>>> +					struct i2c_adapter *adapter, bool force)
>>>> +{
>>>> +	u8 config;
>>>> +	struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info;
>>>> +
>>>> +	if (!hdmi_info->scr_info.status && !force) {
>>>> +		DRM_DEBUG_KMS("Scrambling already disabled\n");
>>>> +		return true;
>>>> +	}
>>>> +
>>>> +	if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
>>>> +		DRM_ERROR("Failed to read tmds config\n");
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	config &= ~SCDC_SCRAMBLING_ENABLE;
>>>> +
>>>> +	if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
>>>> +		DRM_ERROR("Failed to enable scrambling, write error\n");
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
>>>> +	return !hdmi_info->scr_info.status;
>>>> +}
>>> Same comments as for drm_enable_scrambling().
>> Got it.
>>>> @@ -3928,8 +4055,10 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>>>>    		if (cea_db_is_hdmi_vsdb(db))
>>>>    			drm_parse_hdmi_vsdb_video(connector, db);
>>>> -		if (cea_db_is_hdmi_forum_vsdb(db))
>>>> +		if (cea_db_is_hdmi_forum_vsdb(db)) {
>>>>    			drm_detect_hdmi_scdc(connector, db);
>>>> +			drm_detect_hdmi_scrambling(connector, db);
>>>> +		}
>>>>    	}
>>>>    }
>>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>>> index 2435598..b9735bd 100644
>>>> --- a/include/drm/drm_connector.h
>>>> +++ b/include/drm/drm_connector.h
>>>> @@ -90,6 +90,26 @@ enum subpixel_order {
>>>>    };
>>>> +
>>>> +/**
>>>> + * struct scrambling: sink's scrambling support.
>>>> + */
>>>> +struct scrambling {
>>>> +	/**
>>>> +	 * @supported: scrambling supported for rates > 340Mhz.
>>> I think it's common to separate number and unit by a space, so "340
>>> MHz".
>> Got it.
>>>> +	 */
>>>> +	bool supported;
>>>> +	/**
>>>> +	 * @low_clocks: scrambling supported for rates <= 340Mhz.
>>>> +	 */
>>>> +	bool low_clocks;
>>> Maybe "low_rates" because a clock is technically the source of a signal
>>> that oscillates at the given rate.
>> Agree, will change it.
>>>> +	/**
>>>> +	 * @status: scrambling enabled/disabled ?
>>>> +	 */
>>>> +	bool status;
>>>> +};
>>>> +
>>>> +
>>>>    /**
>>>>     * struct drm_hdmi_info - runtime data about the connected sink
>>>>     *
>>>> @@ -108,6 +128,10 @@ struct drm_hdmi_info {
>>>>    	 * @scdc_rr: sink is capable of generating scdc read request.
>>>>    	 */
>>>>    	bool scdc_rr;
>>>> +	/**
>>>> +	 * scr_info: sink's scrambling support and capabilities.
>>>> +	 */
>>>> +	struct scrambling scr_info;
>>> Again, I think I'd drop _info and instead spell out "scrambling" to make
>>> this easier to remember.
>> Sure, can be done.
>>> Also I'm wondering why scdc_supported and scdc_rr are not in a structure
>>> if scrambling info is. Also since scrambling depends on SCDC, would it
>>> make sense to embed it in a struct drm_hdmi_scdc_info?
>> My opinion while designing was:
>> - SCDC rr support is platform specific, and a platform can choose not to
>> support read_req but just allow
>>    scrambling using scdc read and write, hence kept that separate
>> - You need to look into this internal structure, only if scdc is supported.
>> - Also, SCDC registers can be used beyond scrambling too, like for error
>> detection, and other cases, so I thought
>>    it would be better to keep it independent.
>>
>> Does it make sense ?
> Yes, I think that makes sense, but it's not what I was trying to say. =)
> What I was trying to say is that read request and scrambling are defined
> in the SCDC specification, and therefore they require SCDC. That's why I
> think the below is a natural representation because it captures the
> dependency in a hierarchy:
>
>>> 	struct drm_hdmi_scdc_scrambling_info {
>>> 		bool supported;
>>> 		bool low_rates;
>>> 		bool status;
>>> 	};
>>>
>>> 	struct drm_hdmi_scdc_info {
>>> 		bool supported;
>>> 		bool read_request;
>>>
>>> 		struct drm_hdmi_scdc_scrambling_info scrambling;
>>> 	};
> I should have added to the above:
>
> 	struct drm_hdmi_info {
> 		struct drm_hdmi_scdc_info scdc;
> 	};
>
> So when conditionalizing code this allows for a very natural ordering of
> the code:
>
> 	struct drm_display_info *info = ...;
> 	struct drm_hdmi_scdc_info *scdc = &info->hdmi.scdc;
>
> 	if (scdc->supported) {
> 		...
>
> 		if (scdc->read_request) {
> 			...
> 			e.g. set up handler for read requests
> 			...
> 		}
>
> 		...
>
> 		if (scdc->scrambling.supported) {
> 			if (mode->clock >= 340000 || scdc->scrambling.low_rates) {
> 				...
> 				set up scrambling
> 				...;
> 			}
> 		}
>
> 		...
> 	}
>
> Thierry
Well, makes perfect sense, will change the code as suggested :=)

- Shashank

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel