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

[PATCH v4 0/6] HDMI 2.0: Scrambling 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.

V2:
- addressed review comments from Thierry, Ville and Dhinakaran
- added signed-off-by:self in first two patches(Jani N)

V3:
- addressed review comments from Jose and Jani

V4:
- addressed review comments from Maarten on patch 5,
  rebase all other patches

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            |  60 ++++++++
 drivers/gpu/drm/drm_scdc_helper.c     | 275 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h       |   4 +
 drivers/gpu/drm/i915/intel_ddi.c      |  28 ++++
 drivers/gpu/drm/i915/intel_drv.h      |  14 ++
 drivers/gpu/drm/i915/intel_hdmi.c     | 110 ++++++++++++++
 include/drm/drm_connector.h           |  52 +++++++
 include/drm/drm_edid.h                |   6 +-
 include/drm/drm_scdc_helper.h         | 173 +++++++++++++++++++++
 include/linux/hdmi.h                  |   1 +
 12 files changed, 736 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

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

[PATCH v4 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.

V2: Rebase.
V3: Added R-B from Jose.
V4: Rebase

Signed-off-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Reviewed-by: Jose Abreu <joabreu@synopsys.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

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

[PATCH v4 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.

V2: Rebase.
V3: Added R-B from Jose.
V4: Rebase

Signed-off-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Reviewed-by: Jose Abreu <joabreu@synopsys.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 24e7b28..8acdd08 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3245,6 +3245,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 v4 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 advanced HDMI 2.0 features
- Adds another structure drm_scdc within drm_hdmi_info, to
  reflect scdc support and capabilities in connected HDMI 2.0 sink.
- Checks the HF-VSDB block for presence of SCDC, and marks it
  in scdc structure
- If SCDC is present, checks if sink is capable of generating
  SCDC read request, and marks it in scdc structure.

V2: Addressed review comments
 Thierry:
 - Fix typos in commit message and make abbreviation consistent
   across the commit message.
 - Change structure object name from hdmi_info -> hdmi
 - Fix typos and abbreviations in description of structure drm_hdmi_info
   end the description with a full stop.
 - Create a structure drm_scdc, and keep all information related to SCDC
   register set (supported, read request supported) etc in it.

 Ville:
 - Change rr -> read_request
 - Call drm_detect_scrambling function drm_parse_hf_vsdb so that all
   of HF-VSDB parsing can be kept in same function, in incremental
   patches.

V3: Rebase.
V4: Rebase.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Reviewed-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_edid.c  | 14 ++++++++++++++
 include/drm/drm_connector.h | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 8acdd08..63b79be 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3808,6 +3808,18 @@ enum hdmi_quantization_range
 }
 EXPORT_SYMBOL(drm_default_rgb_quant_range);
 
+static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
+				 const u8 *hf_vsdb)
+{
+	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
+
+	if (hf_vsdb[6] & 0x80) {
+		hdmi->scdc.supported = true;
+		if (hf_vsdb[6] & 0x40)
+			hdmi->scdc.read_request = true;
+	}
+}
+
 static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
 					   const u8 *hdmi)
 {
@@ -3922,6 +3934,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_parse_hdmi_forum_vsdb(connector, db);
 	}
 }
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index e5e1edd..6d5304e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -87,6 +87,34 @@ enum subpixel_order {
 	SubPixelVerticalRGB,
 	SubPixelVerticalBGR,
 	SubPixelNone,
+
+};
+
+/*
+ * struct drm_scdc - Information about scdc capabilities of a HDMI 2.0 sink
+ *
+ * Provides SCDC register support and capabilities related information on a
+ * HDMI 2.0 sink. In case of a HDMI 1.4 sink, all parameter must be 0.
+ */
+struct drm_scdc {
+	/**
+	 * @supported: status control & data channel present.
+	 */
+	bool supported;
+	/**
+	 * @read_request: sink is capable of generating scdc read request.
+	 */
+	bool read_request;
+};
+
+/**
+ * struct drm_hdmi_info - runtime information about the connected HDMI sink
+ *
+ * Describes if a given display supports advanced HDMI 2.0 features.
+ * This information is available in CEA-861-F extension blocks (like HF-VSDB).
+ */
+struct drm_hdmi_info {
+	struct drm_scdc scdc;
 };
 
 /**
@@ -188,6 +216,11 @@ struct drm_display_info {
 	 * @cea_rev: CEA revision of the HDMI sink.
 	 */
 	u8 cea_rev;
+
+	/**
+	 * @hdmi: advance features of a HDMI sink.
+	 */
+	struct drm_hdmi_info hdmi;
 };
 
 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

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

[Shashank Sharma]

HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
than 340 MHz. 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.

V2: Addressed review comments from Thierry, Ville and Dhinakaran
Thierry:
- Mhz -> MHz in comments and commit message.
- i2c -> I2C in comments.
- Fix the function documentations, keep in sync with drm_scdc_helper.c
- drm_connector -> DRM connector.
- Create structure for SCDC, and save scrambling status inside that,
  in a sub-structure.
- Call this sub-structure scrambling instead of scr_info.
- low_rates -> low_clocks in scrambling status structure.
- Store the return value of I2C read/write and print the error code
  in case of failure.

Thierry and Ville:
- Move the scrambling enable/disable/query functions in
  drm_scdc_helper.c file.
- Add drm_SCDC prefix for the functions.
- Optimize the return statement from function
  drm_SCDC_check_scrambling_status.

Ville:
- Dont overwrite saved max TMDS clock value in display_info,
  if max tmds clock from HF-VSDB is not > 340 MHz.
- drm_detect_hdmi_scrambling -> drm_parse_hdmi_forum_vsdb.
- Remove dynamic tracking of SCDC status from DRM layer, force bool.
- Program clock ratio bit also, while enabling scrambling.

Dhinakaran:
- Add a comment about *5000 while extracting max clock supported.

V3: Addressed review comments from Jose.
- Create separate functions to enable scrambling and to set TMDS
  clock/character rate ratio.

V4: Rebase

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_edid.c        |  33 +++++++-
 drivers/gpu/drm/drm_scdc_helper.c | 164 ++++++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h       |  19 +++++
 include/drm/drm_edid.h            |   6 +-
 include/drm/drm_scdc_helper.h     |  41 ++++++++++
 5 files changed, 261 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 63b79be..1e18c0a 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>
 
 #include "drm_crtc_internal.h"
 
@@ -3811,13 +3812,43 @@ enum hdmi_quantization_range
 static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
 				 const u8 *hf_vsdb)
 {
-	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
+	struct drm_display_info *display = &connector->display_info;
+	struct drm_hdmi_info *hdmi = &display->hdmi;
 
 	if (hf_vsdb[6] & 0x80) {
 		hdmi->scdc.supported = true;
 		if (hf_vsdb[6] & 0x40)
 			hdmi->scdc.read_request = true;
 	}
+
+	/*
+	 * All HDMI 2.0 monitors must support scrambling at rates > 340 MHz.
+	 * 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 (let's check)
+	 * * SCDC support available (let's check)
+	 * Lets check it out.
+	 */
+
+	if (hf_vsdb[5]) {
+		/* max clock is 5000 KHz times block value */
+		u32 max_tmds_clock = hf_vsdb[5] * 5000;
+		struct drm_scdc *scdc = &hdmi->scdc;
+
+		if (max_tmds_clock > 340000) {
+			display->max_tmds_clock = max_tmds_clock;
+			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
+				display->max_tmds_clock);
+		}
+
+		if (scdc->supported) {
+			scdc->scrambling.supported = true;
+
+			/* Few sinks support scrambling for cloks < 340M */
+			if ((hf_vsdb[6] & 0x8))
+				scdc->scrambling.low_rates = true;
+		}
+	}
 }
 
 static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
diff --git a/drivers/gpu/drm/drm_scdc_helper.c b/drivers/gpu/drm/drm_scdc_helper.c
index fe0e121..c0cf82b 100644
--- a/drivers/gpu/drm/drm_scdc_helper.c
+++ b/drivers/gpu/drm/drm_scdc_helper.c
@@ -22,8 +22,10 @@
  */
 
 #include <linux/slab.h>
+#include <linux/delay.h>
 
 #include <drm/drm_scdc_helper.h>
+#include <drm/drmP.h>
 
 /**
  * DOC: scdc helpers
@@ -109,3 +111,165 @@ ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
 	return err;
 }
 EXPORT_SYMBOL(drm_scdc_write);
+
+/**
+ * drm_scdc_check_scrambling_status - what is status of scrambling?
+ * @adapter: I2C adapter for DDC channel
+ *
+ * Reads the scrambler status over SCDC, and checks the
+ * scrambling status.
+ *
+ * Returns:
+ * True if the scrambling is enabled, false otherwise.
+ */
+
+bool drm_scdc_check_scrambling_status(struct i2c_adapter *adapter)
+{
+	u8 status;
+	int ret;
+
+	ret = drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status);
+	if (ret < 0) {
+		DRM_ERROR("Failed to read scrambling status, error %d\n", ret);
+		return false;
+	}
+
+	return status & SCDC_SCRAMBLING_STATUS;
+}
+EXPORT_SYMBOL(drm_scdc_check_scrambling_status);
+
+/**
+ * drm_scdc_enable_scrambling - enable scrambling
+ * @adapter: I2C adapter for DDC channel
+ *
+ * Writes the TMDS config over SCDC channel, and enables scrambling
+ * Returns:
+ * True if scrambling is successfully enabled, false otherwise.
+ */
+
+bool drm_scdc_enable_scrambling(struct i2c_adapter *adapter)
+{
+	u8 config;
+	int ret;
+
+	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
+	if (ret < 0) {
+		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
+		return false;
+	}
+
+	config |= SCDC_SCRAMBLING_ENABLE;
+
+	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
+	if (ret < 0) {
+		DRM_ERROR("Failed to enable scrambling, error %d\n", ret);
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL(drm_scdc_enable_scrambling);
+
+/**
+ * drm_scdc_disable_scrambling - disable scrambling
+ * @adapter: I2C adapter for DDC channel
+ *
+ * Write the TMDS config over SCDC channel, and disable scrambling
+ * Return: True if scrambling is successfully disabled, false otherwise.
+ */
+bool drm_scdc_disable_scrambling(struct i2c_adapter *adapter)
+{
+	u8 config;
+	int ret;
+
+	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
+	if (ret < 0) {
+		DRM_ERROR("Failed to read tmds config, error %d\n", ret);
+		return false;
+	}
+
+	config &= ~SCDC_SCRAMBLING_ENABLE;
+
+	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
+	if (ret < 0) {
+		DRM_ERROR("Failed to enable scrambling, error %d\n", ret);
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL(drm_scdc_disable_scrambling);
+
+/**
+ * drm_scdc_set_high_tmds_clock_ratio - set TMDS clock ratio
+ * @adapter: I2C adapter for DDC channel
+ *
+ * Writes the TMDS config over SCDC channel, and sets TMDS
+ * clock ratio to 1/40
+ * Returns:
+ * True if write is successful, false otherwise.
+ */
+bool drm_scdc_set_high_tmds_clock_ratio(struct i2c_adapter *adapter)
+{
+	u8 config;
+	int ret;
+
+	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
+	if (ret < 0) {
+		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
+		return false;
+	}
+
+	config |= SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
+
+	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
+	if (ret < 0) {
+		DRM_ERROR("Failed to set TMDS clock ratio, error %d\n", ret);
+		return false;
+	}
+
+	/*
+	 * The spec says that the source should wait minimum 1ms and maximum
+	 * 100ms after writing the TMDS config for clock ratio.
+	 */
+	usleep_range(1000, 100000);
+	return true;
+}
+EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
+
+/**
+ * drm_scdc_clear_high_tmds_clock_ratio - clear TMDS clock ratio
+ * @adapter: I2C adapter for DDC channel
+ *
+ * Writes the TMDS config over SCDC channel, and sets TMDS
+ * clock ratio back to 1/10 (from 1/40)
+ * Returns:
+ * True if write is successful, false otherwise.
+ */
+bool drm_scdc_clear_high_tmds_clock_ratio(struct i2c_adapter *adapter)
+{
+	u8 config;
+	int ret;
+
+	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
+	if (ret < 0) {
+		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
+		return false;
+	}
+
+	config &= ~SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
+
+	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
+	if (ret < 0) {
+		DRM_ERROR("Failed to clear TMDS clock ratio, error %d\n", ret);
+		return false;
+	}
+
+	/*
+	 * The spec says that the source should wait minimum 1ms and maximum
+	 * 100ms after writing the TMDS config for clock ratio.
+	 */
+	usleep_range(1000, 100000);
+	return true;
+}
+EXPORT_SYMBOL(drm_scdc_clear_high_tmds_clock_ratio);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 6d5304e..78618308 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -90,6 +90,20 @@ enum subpixel_order {
 
 };
 
+/**
+ * struct drm_scrambling: sink's scrambling support.
+ */
+struct drm_scrambling {
+	/**
+	 * @supported: scrambling supported for rates > 340 Mhz.
+	 */
+	bool supported;
+	/**
+	 * @low_rates: scrambling supported for rates <= 340 Mhz.
+	 */
+	bool low_rates;
+};
+
 /*
  * struct drm_scdc - Information about scdc capabilities of a HDMI 2.0 sink
  *
@@ -105,8 +119,13 @@ struct drm_scdc {
 	 * @read_request: sink is capable of generating scdc read request.
 	 */
 	bool read_request;
+	/**
+	 * @scrambling: sink's scrambling capabilities
+	 */
+	struct drm_scrambling scrambling;
 };
 
+
 /**
  * struct drm_hdmi_info - runtime information about the connected HDMI sink
  *
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__ */
diff --git a/include/drm/drm_scdc_helper.h b/include/drm/drm_scdc_helper.h
index 93b07bc..c5f2229 100644
--- a/include/drm/drm_scdc_helper.h
+++ b/include/drm/drm_scdc_helper.h
@@ -129,4 +129,45 @@ static inline int drm_scdc_writeb(struct i2c_adapter *adapter, u8 offset,
 	return drm_scdc_write(adapter, offset, &value, sizeof(value));
 }
 
+/**
+ * drm_scdc_enable_scrambling - enable scrambling
+ * @adapter: I2C adapter for DDC channel
+ *
+ * Writes the TMDS config over SCDC channel, and enables scrambling
+ * Returns:
+ * True if scrambling is successfully enabled, false otherwise.
+ */
+
+bool drm_scdc_enable_scrambling(struct i2c_adapter *adapter);
+
+/**
+ * drm_scdc_disable_scrambling - disable scrambling
+ * @adapter: I2C adapter for DDC channel
+ *
+ * Write the TMDS config over SCDC channel, and disable scrambling
+ * Return: True if scrambling is successfully disabled, false otherwise.
+ */
+bool drm_scdc_disable_scrambling(struct i2c_adapter *adapter);
+
+/**
+ * drm_scdc_set_high_tmds_clock_ratio - set TMDS clock ratio
+ * @adapter: I2C adapter for DDC channel
+ *
+ * Writes the TMDS config over SCDC channel, and sets TMDS
+ * clock ratio to 1/40
+ * Returns:
+ * True if write is successful, false otherwise.
+ */
+bool drm_scdc_set_high_tmds_clock_ratio(struct i2c_adapter *adapter);
+
+/**
+ * drm_scdc_clear_high_tmds_clock_ratio - clear TMDS clock ratio
+ * @adapter: I2C adapter for DDC channel
+ *
+ * Writes the TMDS config over SCDC channel, and sets TMDS
+ * clock ratio back to 1/10 (from 1/40)
+ * Returns:
+ * True if write is successful, false otherwise.
+ */
+bool drm_scdc_clear_high_tmds_clock_ratio(struct i2c_adapter *adapter);
 #endif
-- 
1.9.1

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

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

[Shashank Sharma]

Geminilake platform sports 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.

V2: Addressed review comments from Ville:
- Do not track scrambling status in DRM layer, track somewhere in
  driver like in intel_crtc_state.
- Don't talk to monitor at such a low layer, set monitor scrambling
  in intel_enable_ddi() before enabling the port.

V3: Addressed review comments from Jani
- In comments, function names, use "sink" instead of "monitor",
  so that the implementation could be close to the language of
  HDMI spec.

V4: Addressed review comment from Maarten
- scrambling -> hdmi_scrambling
  high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 141a5c1..81cf10b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7819,7 +7819,11 @@ enum {
 #define  TRANS_DDI_EDP_INPUT_B_ONOFF	(5<<12)
 #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
 #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
+#define  TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
+#define  TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
 #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 cd6fedd..bd8293d 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_handle_source_scrambling(
+				intel_encoder,
+				&intel_crtc->config->base.adjusted_mode, temp);
 	} else if (type == INTEL_OUTPUT_ANALOG) {
 		temp |= TRANS_DDI_MODE_SELECT_FDI;
 		temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
@@ -1843,6 +1848,21 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
 		struct intel_digital_port *intel_dig_port =
 			enc_to_dig_port(encoder);
 
+		if (IS_GEMINILAKE(dev_priv)) {
+			struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
+			/*
+			 * GLK sports a native HDMI 2.0 controller. If required
+			 * clock rate is > 340 Mhz && scrambling is supported
+			 * by sink, enable scrambling before enabling the
+			 * HDMI 2.0 port. The sink can choose to disable the
+			 * scrambling if it doesn't detect a scrambled within
+			 * 100 ms.
+			 */
+			intel_hdmi_handle_sink_scrambling(intel_encoder,
+						conn_state->connector,
+						crtc->config, true);
+		}
+
 		/* In HDMI/DVI mode, the port width, and swing/emphasis values
 		 * are ignored so nothing special needs to be done besides
 		 * enabling the port.
@@ -1875,6 +1895,14 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
 	if (old_crtc_state->has_audio)
 		intel_audio_codec_disable(intel_encoder);
 
+	if (type == INTEL_OUTPUT_HDMI) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
+
+		intel_hdmi_handle_sink_scrambling(intel_encoder,
+					old_conn_state->connector,
+					intel_crtc->config, false);
+	}
+
 	if (type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 821c57c..c7262d7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -691,6 +691,12 @@ struct intel_crtc_state {
 
 	/* Gamma mode programmed on the pipe */
 	uint32_t gamma_mode;
+
+	/* HDMI scrambling status (sink) */
+	bool hdmi_scrambling;
+
+	/* HDMI High TMDS char rate ratio (sink) */
+	bool hdmi_high_tmds_clock_ratio;
 };
 
 struct vlv_wm_state {
@@ -1609,6 +1615,14 @@ 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_handle_source_scrambling(struct intel_encoder *intel_encoder,
+					struct drm_display_mode *mode,
+					uint32_t config);
+void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
+					struct drm_connector *connector,
+					struct intel_crtc_state *config,
+					bool enable);
 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 a580de8..c44beee 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -34,6 +34,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_scdc_helper.h>
 #include "intel_drv.h"
 #include <drm/i915_drm.h>
 #include <drm/intel_lpe_audio.h>
@@ -1795,6 +1796,113 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
 	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
 }
 
+void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
+				       struct drm_connector *connector,
+				       struct intel_crtc_state *config,
+				       bool enable)
+{
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
+	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
+	struct drm_scrambling *scrambling = &scdc->scrambling;
+	struct drm_display_mode *mode = &config->base.adjusted_mode;
+	struct drm_i915_private *dev_priv = connector->dev->dev_private;
+	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
+							  intel_hdmi->ddc_bus);
+
+	if (!scrambling->supported)
+		return;
+
+	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
+			intel_encoder->base.name, connector->name);
+
+	if (enable) {
+
+		if (mode->clock > 340000) {
+			/* Set TMDS bit clock ratio to 1/40 */
+			config->hdmi_high_tmds_clock_ratio =
+				drm_scdc_set_high_tmds_clock_ratio(adptr);
+			if (!config->hdmi_high_tmds_clock_ratio) {
+				DRM_ERROR("Set high TMDS ratio failed\n");
+				return;
+			}
+
+			/* Enable sink scrambling */
+			config->hdmi_scrambling =
+					drm_scdc_enable_scrambling(adptr);
+			if (!config->hdmi_scrambling) {
+				DRM_ERROR("Can't enable sink scrambling\n");
+				return;
+			}
+		}
+
+		/* Few sinks support scrambling at clocks <=340 MHz too */
+		if (!config->hdmi_scrambling && scrambling->low_rates) {
+			config->hdmi_scrambling =
+					drm_scdc_enable_scrambling(adptr);
+			if (!config->hdmi_scrambling)
+				DRM_ERROR("Can't enable sink scrambling\n");
+		}
+
+		return;
+	}
+
+	if (config->hdmi_high_tmds_clock_ratio) {
+		/* Set TMDS bit clock ratio back to 1/10 */
+		config->hdmi_high_tmds_clock_ratio =
+			!(drm_scdc_clear_high_tmds_clock_ratio(adptr));
+		if (config->hdmi_high_tmds_clock_ratio)
+			DRM_ERROR("Reset high TMDS ratio failed\n");
+	}
+
+	if (config->hdmi_scrambling) {
+		/* Disable sink scrambling */
+		config->hdmi_scrambling = !(drm_scdc_disable_scrambling(adptr));
+		if (config->hdmi_scrambling)
+			DRM_ERROR("Disable sink scrambling failed\n");
+	}
+}
+
+static inline uint32_t _intel_hdmi_set_source_scrambling(uint32_t hdmi_config)
+{
+	return hdmi_config |= (TRANS_DDI_HDMI_SCRAMBLING |
+			TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
+			TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
+}
+
+uint32_t
+intel_hdmi_handle_source_scrambling(struct intel_encoder *intel_encoder,
+			struct drm_display_mode *mode, uint32_t hdmi_config)
+{
+	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;
+	struct drm_scrambling *scrambling = &hdmi_info->scdc.scrambling;
+
+	DRM_DEBUG_KMS("Setting scrambling for enc:%s connector:%s\n",
+			intel_encoder->base.name, connector->name);
+
+	hdmi_config &= ~(TRANS_DDI_HDMI_SCRAMBLING |
+		TRANS_DDI_HIGH_TMDS_CHAR_RATE |
+		TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
+		TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
+
+	if (mode->clock <= 340000) {
+		/* Few sinks support scrambling at rate < 340 MHz too */
+		if (scrambling->low_rates)
+			hdmi_config =
+				_intel_hdmi_set_source_scrambling(hdmi_config);
+		return hdmi_config;
+	}
+
+	/* Scrambling or not, if clock > 340 MHz, set high char rate */
+	hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
+
+	if (scrambling->supported)
+		hdmi_config = _intel_hdmi_set_source_scrambling(hdmi_config);
+
+	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 v4 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.

V2: rebase
V3: rebase
V4: added r-b from Ander

Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Reviewed-by: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@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 c44beee..84b4e29 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1210,6 +1210,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

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

[Ville Syrjälä]

On Wed, Feb 22, 2017 at 06:48:26PM +0530, Shashank Sharma 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.
> 
> V2: Rebase.
> V3: Added R-B from Jose.
> V4: Rebase
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Reviewed-by: Jose Abreu <joabreu@synopsys.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));

That still disagrees with the documentation. I would suggest doing what
the DP dual mode helper does.

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

We don't usually define macros like this to extract information from the
register. It's easy to confuse them with the normal macros for setting
bits.

> +
> +#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

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

✓ Fi.CI.BAT: success for HDMI 2.0: Scrambling in DRM layer (rev4)

[Patchwork]

== Series Details ==

Series: HDMI 2.0: Scrambling in DRM layer (rev4)
URL   : https://patchwork.freedesktop.org/series/19161/
State : success

== Summary ==

Series 19161v4 HDMI 2.0: Scrambling in DRM layer
https://patchwork.freedesktop.org/api/1.0/series/19161/revisions/4/mbox/

fi-bdw-5557u     total:253  pass:242  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:253  pass:214  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:253  pass:234  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:253  pass:226  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:253  pass:222  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:253  pass:237  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:253  pass:237  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:253  pass:203  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:253  pass:243  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:253  pass:236  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:253  pass:231  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:253  pass:243  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:253  pass:225  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:253  pass:224  dwarn:0   dfail:0   fail:0   skip:29 

bf89ec45d0822835b03910371ac0baf46c4efa2d drm-tip: 2017y-02m-22d-14h-30m-04s UTC integration manifest
6d613076 drm/i915: allow HDMI 2.0 clock rates
ca0b6ab drm/i915: enable scrambling
960b45d drm: scrambling support in drm layer
526a33d drm/edid: detect SCDC support in HF-VSDB
ca7c9a7 drm/edid: check for HF-VSDB block
47823ce drm: Add SCDC helpers

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3931/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Wed, Feb 22, 2017 at 06:48:29PM +0530, Shashank Sharma wrote:
> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
> than 340 MHz. 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.
> 
> V2: Addressed review comments from Thierry, Ville and Dhinakaran
> Thierry:
> - Mhz -> MHz in comments and commit message.
> - i2c -> I2C in comments.
> - Fix the function documentations, keep in sync with drm_scdc_helper.c
> - drm_connector -> DRM connector.
> - Create structure for SCDC, and save scrambling status inside that,
>   in a sub-structure.
> - Call this sub-structure scrambling instead of scr_info.
> - low_rates -> low_clocks in scrambling status structure.
> - Store the return value of I2C read/write and print the error code
>   in case of failure.
> 
> Thierry and Ville:
> - Move the scrambling enable/disable/query functions in
>   drm_scdc_helper.c file.
> - Add drm_SCDC prefix for the functions.
> - Optimize the return statement from function
>   drm_SCDC_check_scrambling_status.
> 
> Ville:
> - Dont overwrite saved max TMDS clock value in display_info,
>   if max tmds clock from HF-VSDB is not > 340 MHz.
> - drm_detect_hdmi_scrambling -> drm_parse_hdmi_forum_vsdb.
> - Remove dynamic tracking of SCDC status from DRM layer, force bool.
> - Program clock ratio bit also, while enabling scrambling.
> 
> Dhinakaran:
> - Add a comment about *5000 while extracting max clock supported.
> 
> V3: Addressed review comments from Jose.
> - Create separate functions to enable scrambling and to set TMDS
>   clock/character rate ratio.
> 
> V4: Rebase
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c        |  33 +++++++-
>  drivers/gpu/drm/drm_scdc_helper.c | 164 ++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       |  19 +++++
>  include/drm/drm_edid.h            |   6 +-
>  include/drm/drm_scdc_helper.h     |  41 ++++++++++
>  5 files changed, 261 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 63b79be..1e18c0a 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>
>  
>  #include "drm_crtc_internal.h"
>  
> @@ -3811,13 +3812,43 @@ enum hdmi_quantization_range
>  static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
>  				 const u8 *hf_vsdb)
>  {
> -	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
> +	struct drm_display_info *display = &connector->display_info;
> +	struct drm_hdmi_info *hdmi = &display->hdmi;
>  
>  	if (hf_vsdb[6] & 0x80) {
>  		hdmi->scdc.supported = true;
>  		if (hf_vsdb[6] & 0x40)
>  			hdmi->scdc.read_request = true;
>  	}
> +
> +	/*
> +	 * All HDMI 2.0 monitors must support scrambling at rates > 340 MHz.
> +	 * 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 (let's check)
> +	 * * SCDC support available (let's check)
> +	 * Lets check it out.
> +	 */
> +
> +	if (hf_vsdb[5]) {
> +		/* max clock is 5000 KHz times block value */
> +		u32 max_tmds_clock = hf_vsdb[5] * 5000;
> +		struct drm_scdc *scdc = &hdmi->scdc;
> +
> +		if (max_tmds_clock > 340000) {
> +			display->max_tmds_clock = max_tmds_clock;
> +			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> +				display->max_tmds_clock);
> +		}
> +
> +		if (scdc->supported) {
> +			scdc->scrambling.supported = true;
> +
> +			/* Few sinks support scrambling for cloks < 340M */
> +			if ((hf_vsdb[6] & 0x8))
> +				scdc->scrambling.low_rates = true;
> +		}
> +	}
>  }
>  
>  static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> diff --git a/drivers/gpu/drm/drm_scdc_helper.c b/drivers/gpu/drm/drm_scdc_helper.c
> index fe0e121..c0cf82b 100644
> --- a/drivers/gpu/drm/drm_scdc_helper.c
> +++ b/drivers/gpu/drm/drm_scdc_helper.c
> @@ -22,8 +22,10 @@
>   */
>  
>  #include <linux/slab.h>
> +#include <linux/delay.h>
>  
>  #include <drm/drm_scdc_helper.h>
> +#include <drm/drmP.h>
>  
>  /**
>   * DOC: scdc helpers
> @@ -109,3 +111,165 @@ ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
>  	return err;
>  }
>  EXPORT_SYMBOL(drm_scdc_write);
> +
> +/**
> + * drm_scdc_check_scrambling_status - what is status of scrambling?
> + * @adapter: I2C adapter for DDC channel
> + *
> + * Reads the scrambler status over SCDC, and checks the
> + * scrambling status.
> + *
> + * Returns:
> + * True if the scrambling is enabled, false otherwise.
> + */
> +
> +bool drm_scdc_check_scrambling_status(struct i2c_adapter *adapter)
> +{
> +	u8 status;
> +	int ret;
> +
> +	ret = drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status);
> +	if (ret < 0) {
> +		DRM_ERROR("Failed to read scrambling status, error %d\n", ret);
> +		return false;
> +	}
> +
> +	return status & SCDC_SCRAMBLING_STATUS;
> +}
> +EXPORT_SYMBOL(drm_scdc_check_scrambling_status);

s/check/get/ would make more sense to me.

> +
> +/**
> + * drm_scdc_enable_scrambling - enable scrambling
> + * @adapter: I2C adapter for DDC channel
> + *
> + * Writes the TMDS config over SCDC channel, and enables scrambling
> + * Returns:
> + * True if scrambling is successfully enabled, false otherwise.
> + */
> +
> +bool drm_scdc_enable_scrambling(struct i2c_adapter *adapter)
> +{
> +	u8 config;
> +	int ret;
> +
> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
> +	if (ret < 0) {
> +		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
> +		return false;
> +	}
> +
> +	config |= SCDC_SCRAMBLING_ENABLE;
> +
> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
> +	if (ret < 0) {
> +		DRM_ERROR("Failed to enable scrambling, error %d\n", ret);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(drm_scdc_enable_scrambling);
> +
> +/**
> + * drm_scdc_disable_scrambling - disable scrambling
> + * @adapter: I2C adapter for DDC channel
> + *
> + * Write the TMDS config over SCDC channel, and disable scrambling
> + * Return: True if scrambling is successfully disabled, false otherwise.
> + */
> +bool drm_scdc_disable_scrambling(struct i2c_adapter *adapter)
> +{
> +	u8 config;
> +	int ret;
> +
> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
> +	if (ret < 0) {
> +		DRM_ERROR("Failed to read tmds config, error %d\n", ret);
> +		return false;
> +	}
> +
> +	config &= ~SCDC_SCRAMBLING_ENABLE;
> +
> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
> +	if (ret < 0) {
> +		DRM_ERROR("Failed to enable scrambling, error %d\n", ret);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(drm_scdc_disable_scrambling);
> +
> +/**
> + * drm_scdc_set_high_tmds_clock_ratio - set TMDS clock ratio
> + * @adapter: I2C adapter for DDC channel
> + *
> + * Writes the TMDS config over SCDC channel, and sets TMDS
> + * clock ratio to 1/40
> + * Returns:
> + * True if write is successful, false otherwise.
> + */
> +bool drm_scdc_set_high_tmds_clock_ratio(struct i2c_adapter *adapter)
> +{
> +	u8 config;
> +	int ret;
> +
> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
> +	if (ret < 0) {
> +		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
> +		return false;
> +	}
> +
> +	config |= SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
> +
> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
> +	if (ret < 0) {
> +		DRM_ERROR("Failed to set TMDS clock ratio, error %d\n", ret);
> +		return false;
> +	}
> +
> +	/*
> +	 * The spec says that the source should wait minimum 1ms and maximum
> +	 * 100ms after writing the TMDS config for clock ratio.
> +	 */
> +	usleep_range(1000, 100000);

Allowing the max to be 100ms here seems overly generous to me. In by
allowing 100ms we might be violating the spec since it will take a bit
of additonal time before the driver will enable the output. So I'd just
use something like 1-2 msec.

> +	return true;
> +}
> +EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
> +
> +/**
> + * drm_scdc_clear_high_tmds_clock_ratio - clear TMDS clock ratio
> + * @adapter: I2C adapter for DDC channel
> + *
> + * Writes the TMDS config over SCDC channel, and sets TMDS
> + * clock ratio back to 1/10 (from 1/40)
> + * Returns:
> + * True if write is successful, false otherwise.
> + */
> +bool drm_scdc_clear_high_tmds_clock_ratio(struct i2c_adapter *adapter)
> +{
> +	u8 config;
> +	int ret;
> +
> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
> +	if (ret < 0) {
> +		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
> +		return false;
> +	}
> +
> +	config &= ~SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
> +
> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
> +	if (ret < 0) {
> +		DRM_ERROR("Failed to clear TMDS clock ratio, error %d\n", ret);
> +		return false;
> +	}
> +
> +	/*
> +	 * The spec says that the source should wait minimum 1ms and maximum
> +	 * 100ms after writing the TMDS config for clock ratio.
> +	 */
> +	usleep_range(1000, 100000);
> +	return true;
> +}
> +EXPORT_SYMBOL(drm_scdc_clear_high_tmds_clock_ratio);

Having separate set/clear functions just leads to duplicated code IMO.
I'd just pass the desired state as a parameter.

And what I suggested earlier was to even combine this with the scrambling
write which would reduce the i2c traffic from 2 read and 2 writes to
just 1 write. But I can live with them being separate as well.

> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 6d5304e..78618308 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -90,6 +90,20 @@ enum subpixel_order {
>  
>  };
>  
> +/**
> + * struct drm_scrambling: sink's scrambling support.
> + */
> +struct drm_scrambling {
> +	/**
> +	 * @supported: scrambling supported for rates > 340 Mhz.
> +	 */
> +	bool supported;
> +	/**
> +	 * @low_rates: scrambling supported for rates <= 340 Mhz.
> +	 */
> +	bool low_rates;
> +};
> +
>  /*
>   * struct drm_scdc - Information about scdc capabilities of a HDMI 2.0 sink
>   *
> @@ -105,8 +119,13 @@ struct drm_scdc {
>  	 * @read_request: sink is capable of generating scdc read request.
>  	 */
>  	bool read_request;
> +	/**
> +	 * @scrambling: sink's scrambling capabilities
> +	 */
> +	struct drm_scrambling scrambling;
>  };
>  
> +
>  /**
>   * struct drm_hdmi_info - runtime information about the connected HDMI sink
>   *
> 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__ */
> diff --git a/include/drm/drm_scdc_helper.h b/include/drm/drm_scdc_helper.h
> index 93b07bc..c5f2229 100644
> --- a/include/drm/drm_scdc_helper.h
> +++ b/include/drm/drm_scdc_helper.h
> @@ -129,4 +129,45 @@ static inline int drm_scdc_writeb(struct i2c_adapter *adapter, u8 offset,
>  	return drm_scdc_write(adapter, offset, &value, sizeof(value));
>  }
>  
> +/**
> + * drm_scdc_enable_scrambling - enable scrambling
> + * @adapter: I2C adapter for DDC channel
> + *
> + * Writes the TMDS config over SCDC channel, and enables scrambling
> + * Returns:
> + * True if scrambling is successfully enabled, false otherwise.
> + */
> +
> +bool drm_scdc_enable_scrambling(struct i2c_adapter *adapter);
> +
> +/**
> + * drm_scdc_disable_scrambling - disable scrambling
> + * @adapter: I2C adapter for DDC channel
> + *
> + * Write the TMDS config over SCDC channel, and disable scrambling
> + * Return: True if scrambling is successfully disabled, false otherwise.
> + */
> +bool drm_scdc_disable_scrambling(struct i2c_adapter *adapter);
> +
> +/**
> + * drm_scdc_set_high_tmds_clock_ratio - set TMDS clock ratio
> + * @adapter: I2C adapter for DDC channel
> + *
> + * Writes the TMDS config over SCDC channel, and sets TMDS
> + * clock ratio to 1/40
> + * Returns:
> + * True if write is successful, false otherwise.
> + */
> +bool drm_scdc_set_high_tmds_clock_ratio(struct i2c_adapter *adapter);
> +
> +/**
> + * drm_scdc_clear_high_tmds_clock_ratio - clear TMDS clock ratio
> + * @adapter: I2C adapter for DDC channel
> + *
> + * Writes the TMDS config over SCDC channel, and sets TMDS
> + * clock ratio back to 1/10 (from 1/40)
> + * Returns:
> + * True if write is successful, false otherwise.
> + */
> +bool drm_scdc_clear_high_tmds_clock_ratio(struct i2c_adapter *adapter);
>  #endif
> -- 
> 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 v4 5/6] drm/i915: enable scrambling

[Ville Syrjälä]

On Wed, Feb 22, 2017 at 06:48:30PM +0530, Shashank Sharma wrote:
> Geminilake platform sports 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.
> 
> V2: Addressed review comments from Ville:
> - Do not track scrambling status in DRM layer, track somewhere in
>   driver like in intel_crtc_state.
> - Don't talk to monitor at such a low layer, set monitor scrambling
>   in intel_enable_ddi() before enabling the port.
> 
> V3: Addressed review comments from Jani
> - In comments, function names, use "sink" instead of "monitor",
>   so that the implementation could be close to the language of
>   HDMI spec.
> 
> V4: Addressed review comment from Maarten
> - scrambling -> hdmi_scrambling
>   high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h   |   4 ++
>  drivers/gpu/drm/i915/intel_ddi.c  |  28 ++++++++++
>  drivers/gpu/drm/i915/intel_drv.h  |  14 +++++
>  drivers/gpu/drm/i915/intel_hdmi.c | 108 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 154 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 141a5c1..81cf10b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7819,7 +7819,11 @@ enum {
>  #define  TRANS_DDI_EDP_INPUT_B_ONOFF	(5<<12)
>  #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
>  #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
> +#define  TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
> +#define  TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
>  #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 cd6fedd..bd8293d 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_handle_source_scrambling(
> +				intel_encoder,
> +				&intel_crtc->config->base.adjusted_mode, temp);
>  	} else if (type == INTEL_OUTPUT_ANALOG) {
>  		temp |= TRANS_DDI_MODE_SELECT_FDI;
>  		temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
> @@ -1843,6 +1848,21 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
>  		struct intel_digital_port *intel_dig_port =
>  			enc_to_dig_port(encoder);
>  
> +		if (IS_GEMINILAKE(dev_priv)) {
> +			struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> +			/*
> +			 * GLK sports a native HDMI 2.0 controller. If required
> +			 * clock rate is > 340 Mhz && scrambling is supported
> +			 * by sink, enable scrambling before enabling the
> +			 * HDMI 2.0 port. The sink can choose to disable the
> +			 * scrambling if it doesn't detect a scrambled within
> +			 * 100 ms.
> +			 */
> +			intel_hdmi_handle_sink_scrambling(intel_encoder,
> +						conn_state->connector,
> +						crtc->config, true);
> +		}
> +
>  		/* In HDMI/DVI mode, the port width, and swing/emphasis values
>  		 * are ignored so nothing special needs to be done besides
>  		 * enabling the port.
> @@ -1875,6 +1895,14 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
>  	if (old_crtc_state->has_audio)
>  		intel_audio_codec_disable(intel_encoder);
>  
> +	if (type == INTEL_OUTPUT_HDMI) {
> +		struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> +
> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
> +					old_conn_state->connector,
> +					intel_crtc->config, false);
> +	}
> +
>  	if (type == INTEL_OUTPUT_EDP) {
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 821c57c..c7262d7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -691,6 +691,12 @@ struct intel_crtc_state {
>  
>  	/* Gamma mode programmed on the pipe */
>  	uint32_t gamma_mode;
> +
> +	/* HDMI scrambling status (sink) */
> +	bool hdmi_scrambling;
> +
> +	/* HDMI High TMDS char rate ratio (sink) */
> +	bool hdmi_high_tmds_clock_ratio;
>  };
>  
>  struct vlv_wm_state {
> @@ -1609,6 +1615,14 @@ 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_handle_source_scrambling(struct intel_encoder *intel_encoder,
> +					struct drm_display_mode *mode,
> +					uint32_t config);
> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> +					struct drm_connector *connector,
> +					struct intel_crtc_state *config,
> +					bool enable);
>  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 a580de8..c44beee 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -34,6 +34,7 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_scdc_helper.h>
>  #include "intel_drv.h"
>  #include <drm/i915_drm.h>
>  #include <drm/intel_lpe_audio.h>
> @@ -1795,6 +1796,113 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
>  	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>  }
>  
> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> +				       struct drm_connector *connector,
> +				       struct intel_crtc_state *config,
> +				       bool enable)
> +{
> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> +	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
> +	struct drm_scrambling *scrambling = &scdc->scrambling;
> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> +	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
> +							  intel_hdmi->ddc_bus);
> +
> +	if (!scrambling->supported)
> +		return;
> +
> +	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
> +			intel_encoder->base.name, connector->name);
> +
> +	if (enable) {
> +
> +		if (mode->clock > 340000) {
> +			/* Set TMDS bit clock ratio to 1/40 */
> +			config->hdmi_high_tmds_clock_ratio =
> +				drm_scdc_set_high_tmds_clock_ratio(adptr);

You're not allowed to muck with the state anymore. All state computation
should happen in the .compute_config hook.

> +			if (!config->hdmi_high_tmds_clock_ratio) {
> +				DRM_ERROR("Set high TMDS ratio failed\n");
> +				return;
> +			}
> +
> +			/* Enable sink scrambling */
> +			config->hdmi_scrambling =
> +					drm_scdc_enable_scrambling(adptr);
> +			if (!config->hdmi_scrambling) {
> +				DRM_ERROR("Can't enable sink scrambling\n");
> +				return;
> +			}
> +		}
> +
> +		/* Few sinks support scrambling at clocks <=340 MHz too */
> +		if (!config->hdmi_scrambling && scrambling->low_rates) {
> +			config->hdmi_scrambling =
> +					drm_scdc_enable_scrambling(adptr);
> +			if (!config->hdmi_scrambling)
> +				DRM_ERROR("Can't enable sink scrambling\n");
> +		}
> +
> +		return;
> +	}
> +
> +	if (config->hdmi_high_tmds_clock_ratio) {
> +		/* Set TMDS bit clock ratio back to 1/10 */
> +		config->hdmi_high_tmds_clock_ratio =
> +			!(drm_scdc_clear_high_tmds_clock_ratio(adptr));
> +		if (config->hdmi_high_tmds_clock_ratio)
> +			DRM_ERROR("Reset high TMDS ratio failed\n");
> +	}
> +
> +	if (config->hdmi_scrambling) {
> +		/* Disable sink scrambling */
> +		config->hdmi_scrambling = !(drm_scdc_disable_scrambling(adptr));
> +		if (config->hdmi_scrambling)
> +			DRM_ERROR("Disable sink scrambling failed\n");
> +	}
> +}
> +
> +static inline uint32_t _intel_hdmi_set_source_scrambling(uint32_t hdmi_config)
> +{
> +	return hdmi_config |= (TRANS_DDI_HDMI_SCRAMBLING |
> +			TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
> +			TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
> +}
> +
> +uint32_t
> +intel_hdmi_handle_source_scrambling(struct intel_encoder *intel_encoder,
> +			struct drm_display_mode *mode, uint32_t hdmi_config)
> +{
> +	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;
> +	struct drm_scrambling *scrambling = &hdmi_info->scdc.scrambling;
> +
> +	DRM_DEBUG_KMS("Setting scrambling for enc:%s connector:%s\n",
> +			intel_encoder->base.name, connector->name);
> +
> +	hdmi_config &= ~(TRANS_DDI_HDMI_SCRAMBLING |
> +		TRANS_DDI_HIGH_TMDS_CHAR_RATE |
> +		TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
> +		TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
> +
> +	if (mode->clock <= 340000) {
> +		/* Few sinks support scrambling at rate < 340 MHz too */
> +		if (scrambling->low_rates)
> +			hdmi_config =
> +				_intel_hdmi_set_source_scrambling(hdmi_config);
> +		return hdmi_config;
> +	}
> +
> +	/* Scrambling or not, if clock > 340 MHz, set high char rate */
> +	hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> +
> +	if (scrambling->supported)
> +		hdmi_config = _intel_hdmi_set_source_scrambling(hdmi_config);
> +
> +	return hdmi_config;
> +}

Seems overly complicated to me. It could just be something simple like:

void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
{
	...
	if (config->hdmi_scrambling)
		temp |= TRANS_DDI_HDMI_SCRAMBLING;
	
	if (config->whatever)
		temp |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
	...
}

> +
>  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 v4 1/6] drm: Add SCDC helpers

[Sharma, Shashank]

Thanks for the review Ville, my comments inline.

Regards

Shashank


On 2/22/2017 10:39 PM, Ville Syrjälä wrote:
> On Wed, Feb 22, 2017 at 06:48:26PM +0530, Shashank Sharma 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.
>>
>> V2: Rebase.
>> V3: Added R-B from Jose.
>> V4: Rebase
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Reviewed-by: Jose Abreu <joabreu@synopsys.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));
> That still disagrees with the documentation. I would suggest doing what
> the DP dual mode helper does.
Sure, I guess this is about how we are handling the return values from 
I2C layer, I will change it
the way its handled in drm_dp_dual_mode functions
>
>> +}
>> +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)
> We don't usually define macros like this to extract information from the
> register. It's easy to confuse them with the normal macros for setting
> bits.
How about if I add another sub macro called _MAJOR, and use it like this:

#define _MAJOR(x) (x & 0xf)
#define  SCDC_DEVICE_HARDWARE_REVISION_MAJOR(x) (_MAJOR((x) >> 4))

Or will you prefer no macro for this job at all ?

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

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

[Sharma, Shashank]

Regards

Shashank


On 2/22/2017 10:54 PM, Ville Syrjälä wrote:
> On Wed, Feb 22, 2017 at 06:48:29PM +0530, Shashank Sharma wrote:
>> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
>> than 340 MHz. 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.
>>
>> V2: Addressed review comments from Thierry, Ville and Dhinakaran
>> Thierry:
>> - Mhz -> MHz in comments and commit message.
>> - i2c -> I2C in comments.
>> - Fix the function documentations, keep in sync with drm_scdc_helper.c
>> - drm_connector -> DRM connector.
>> - Create structure for SCDC, and save scrambling status inside that,
>>    in a sub-structure.
>> - Call this sub-structure scrambling instead of scr_info.
>> - low_rates -> low_clocks in scrambling status structure.
>> - Store the return value of I2C read/write and print the error code
>>    in case of failure.
>>
>> Thierry and Ville:
>> - Move the scrambling enable/disable/query functions in
>>    drm_scdc_helper.c file.
>> - Add drm_SCDC prefix for the functions.
>> - Optimize the return statement from function
>>    drm_SCDC_check_scrambling_status.
>>
>> Ville:
>> - Dont overwrite saved max TMDS clock value in display_info,
>>    if max tmds clock from HF-VSDB is not > 340 MHz.
>> - drm_detect_hdmi_scrambling -> drm_parse_hdmi_forum_vsdb.
>> - Remove dynamic tracking of SCDC status from DRM layer, force bool.
>> - Program clock ratio bit also, while enabling scrambling.
>>
>> Dhinakaran:
>> - Add a comment about *5000 while extracting max clock supported.
>>
>> V3: Addressed review comments from Jose.
>> - Create separate functions to enable scrambling and to set TMDS
>>    clock/character rate ratio.
>>
>> V4: Rebase
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c        |  33 +++++++-
>>   drivers/gpu/drm/drm_scdc_helper.c | 164 ++++++++++++++++++++++++++++++++++++++
>>   include/drm/drm_connector.h       |  19 +++++
>>   include/drm/drm_edid.h            |   6 +-
>>   include/drm/drm_scdc_helper.h     |  41 ++++++++++
>>   5 files changed, 261 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 63b79be..1e18c0a 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>
>>   
>>   #include "drm_crtc_internal.h"
>>   
>> @@ -3811,13 +3812,43 @@ enum hdmi_quantization_range
>>   static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
>>   				 const u8 *hf_vsdb)
>>   {
>> -	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
>> +	struct drm_display_info *display = &connector->display_info;
>> +	struct drm_hdmi_info *hdmi = &display->hdmi;
>>   
>>   	if (hf_vsdb[6] & 0x80) {
>>   		hdmi->scdc.supported = true;
>>   		if (hf_vsdb[6] & 0x40)
>>   			hdmi->scdc.read_request = true;
>>   	}
>> +
>> +	/*
>> +	 * All HDMI 2.0 monitors must support scrambling at rates > 340 MHz.
>> +	 * 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 (let's check)
>> +	 * * SCDC support available (let's check)
>> +	 * Lets check it out.
>> +	 */
>> +
>> +	if (hf_vsdb[5]) {
>> +		/* max clock is 5000 KHz times block value */
>> +		u32 max_tmds_clock = hf_vsdb[5] * 5000;
>> +		struct drm_scdc *scdc = &hdmi->scdc;
>> +
>> +		if (max_tmds_clock > 340000) {
>> +			display->max_tmds_clock = max_tmds_clock;
>> +			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
>> +				display->max_tmds_clock);
>> +		}
>> +
>> +		if (scdc->supported) {
>> +			scdc->scrambling.supported = true;
>> +
>> +			/* Few sinks support scrambling for cloks < 340M */
>> +			if ((hf_vsdb[6] & 0x8))
>> +				scdc->scrambling.low_rates = true;
>> +		}
>> +	}
>>   }
>>   
>>   static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>> diff --git a/drivers/gpu/drm/drm_scdc_helper.c b/drivers/gpu/drm/drm_scdc_helper.c
>> index fe0e121..c0cf82b 100644
>> --- a/drivers/gpu/drm/drm_scdc_helper.c
>> +++ b/drivers/gpu/drm/drm_scdc_helper.c
>> @@ -22,8 +22,10 @@
>>    */
>>   
>>   #include <linux/slab.h>
>> +#include <linux/delay.h>
>>   
>>   #include <drm/drm_scdc_helper.h>
>> +#include <drm/drmP.h>
>>   
>>   /**
>>    * DOC: scdc helpers
>> @@ -109,3 +111,165 @@ ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
>>   	return err;
>>   }
>>   EXPORT_SYMBOL(drm_scdc_write);
>> +
>> +/**
>> + * drm_scdc_check_scrambling_status - what is status of scrambling?
>> + * @adapter: I2C adapter for DDC channel
>> + *
>> + * Reads the scrambler status over SCDC, and checks the
>> + * scrambling status.
>> + *
>> + * Returns:
>> + * True if the scrambling is enabled, false otherwise.
>> + */
>> +
>> +bool drm_scdc_check_scrambling_status(struct i2c_adapter *adapter)
>> +{
>> +	u8 status;
>> +	int ret;
>> +
>> +	ret = drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status);
>> +	if (ret < 0) {
>> +		DRM_ERROR("Failed to read scrambling status, error %d\n", ret);
>> +		return false;
>> +	}
>> +
>> +	return status & SCDC_SCRAMBLING_STATUS;
>> +}
>> +EXPORT_SYMBOL(drm_scdc_check_scrambling_status);
> s/check/get/ would make more sense to me.
got it.
>> +
>> +/**
>> + * drm_scdc_enable_scrambling - enable scrambling
>> + * @adapter: I2C adapter for DDC channel
>> + *
>> + * Writes the TMDS config over SCDC channel, and enables scrambling
>> + * Returns:
>> + * True if scrambling is successfully enabled, false otherwise.
>> + */
>> +
>> +bool drm_scdc_enable_scrambling(struct i2c_adapter *adapter)
>> +{
>> +	u8 config;
>> +	int ret;
>> +
>> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
>> +	if (ret < 0) {
>> +		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
>> +		return false;
>> +	}
>> +
>> +	config |= SCDC_SCRAMBLING_ENABLE;
>> +
>> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
>> +	if (ret < 0) {
>> +		DRM_ERROR("Failed to enable scrambling, error %d\n", ret);
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +EXPORT_SYMBOL(drm_scdc_enable_scrambling);
>> +
>> +/**
>> + * drm_scdc_disable_scrambling - disable scrambling
>> + * @adapter: I2C adapter for DDC channel
>> + *
>> + * Write the TMDS config over SCDC channel, and disable scrambling
>> + * Return: True if scrambling is successfully disabled, false otherwise.
>> + */
>> +bool drm_scdc_disable_scrambling(struct i2c_adapter *adapter)
>> +{
>> +	u8 config;
>> +	int ret;
>> +
>> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
>> +	if (ret < 0) {
>> +		DRM_ERROR("Failed to read tmds config, error %d\n", ret);
>> +		return false;
>> +	}
>> +
>> +	config &= ~SCDC_SCRAMBLING_ENABLE;
>> +
>> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
>> +	if (ret < 0) {
>> +		DRM_ERROR("Failed to enable scrambling, error %d\n", ret);
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +EXPORT_SYMBOL(drm_scdc_disable_scrambling);
>> +
>> +/**
>> + * drm_scdc_set_high_tmds_clock_ratio - set TMDS clock ratio
>> + * @adapter: I2C adapter for DDC channel
>> + *
>> + * Writes the TMDS config over SCDC channel, and sets TMDS
>> + * clock ratio to 1/40
>> + * Returns:
>> + * True if write is successful, false otherwise.
>> + */
>> +bool drm_scdc_set_high_tmds_clock_ratio(struct i2c_adapter *adapter)
>> +{
>> +	u8 config;
>> +	int ret;
>> +
>> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
>> +	if (ret < 0) {
>> +		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
>> +		return false;
>> +	}
>> +
>> +	config |= SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
>> +
>> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
>> +	if (ret < 0) {
>> +		DRM_ERROR("Failed to set TMDS clock ratio, error %d\n", ret);
>> +		return false;
>> +	}
>> +
>> +	/*
>> +	 * The spec says that the source should wait minimum 1ms and maximum
>> +	 * 100ms after writing the TMDS config for clock ratio.
>> +	 */
>> +	usleep_range(1000, 100000);
> Allowing the max to be 100ms here seems overly generous to me. In by
> allowing 100ms we might be violating the spec since it will take a bit
> of additonal time before the driver will enable the output. So I'd just
> use something like 1-2 msec.
Makes sense, but wouldn't 1-2 ms be too small ?
Do you think we should allow at least 10ms ?
>> +	return true;
>> +}
>> +EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
>> +
>> +/**
>> + * drm_scdc_clear_high_tmds_clock_ratio - clear TMDS clock ratio
>> + * @adapter: I2C adapter for DDC channel
>> + *
>> + * Writes the TMDS config over SCDC channel, and sets TMDS
>> + * clock ratio back to 1/10 (from 1/40)
>> + * Returns:
>> + * True if write is successful, false otherwise.
>> + */
>> +bool drm_scdc_clear_high_tmds_clock_ratio(struct i2c_adapter *adapter)
>> +{
>> +	u8 config;
>> +	int ret;
>> +
>> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
>> +	if (ret < 0) {
>> +		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
>> +		return false;
>> +	}
>> +
>> +	config &= ~SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
>> +
>> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
>> +	if (ret < 0) {
>> +		DRM_ERROR("Failed to clear TMDS clock ratio, error %d\n", ret);
>> +		return false;
>> +	}
>> +
>> +	/*
>> +	 * The spec says that the source should wait minimum 1ms and maximum
>> +	 * 100ms after writing the TMDS config for clock ratio.
>> +	 */
>> +	usleep_range(1000, 100000);
>> +	return true;
>> +}
>> +EXPORT_SYMBOL(drm_scdc_clear_high_tmds_clock_ratio);
> Having separate set/clear functions just leads to duplicated code IMO.
> I'd just pass the desired state as a parameter.
Ok, I will make a common function for both set/clear, and pass the 
enable/disable state
to it (similar to that which handles monitor scrambling)
> And what I suggested earlier was to even combine this with the scrambling
> write which would reduce the i2c traffic from 2 read and 2 writes to
> just 1 write. But I can live with them being separate as well.
Thanks. Actually, if you see V2, this first implementation was just as 
you suggested.
But there was a problem here, few monitors support scrambling at a clock 
lower than
340Mhz too, in these cases we wont set the high_tmds_clock_ratio but 
only set the scrambling.
This was comment from Jose, which we addressed into splitting this into 
two functions, which gives
more control to caller function, by selecting what they want to set. but 
yeah, cant beat the I2C traffic optimization.

- Shashank
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 6d5304e..78618308 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -90,6 +90,20 @@ enum subpixel_order {
>>   
>>   };
>>   
>> +/**
>> + * struct drm_scrambling: sink's scrambling support.
>> + */
>> +struct drm_scrambling {
>> +	/**
>> +	 * @supported: scrambling supported for rates > 340 Mhz.
>> +	 */
>> +	bool supported;
>> +	/**
>> +	 * @low_rates: scrambling supported for rates <= 340 Mhz.
>> +	 */
>> +	bool low_rates;
>> +};
>> +
>>   /*
>>    * struct drm_scdc - Information about scdc capabilities of a HDMI 2.0 sink
>>    *
>> @@ -105,8 +119,13 @@ struct drm_scdc {
>>   	 * @read_request: sink is capable of generating scdc read request.
>>   	 */
>>   	bool read_request;
>> +	/**
>> +	 * @scrambling: sink's scrambling capabilities
>> +	 */
>> +	struct drm_scrambling scrambling;
>>   };
>>   
>> +
>>   /**
>>    * struct drm_hdmi_info - runtime information about the connected HDMI sink
>>    *
>> 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__ */
>> diff --git a/include/drm/drm_scdc_helper.h b/include/drm/drm_scdc_helper.h
>> index 93b07bc..c5f2229 100644
>> --- a/include/drm/drm_scdc_helper.h
>> +++ b/include/drm/drm_scdc_helper.h
>> @@ -129,4 +129,45 @@ static inline int drm_scdc_writeb(struct i2c_adapter *adapter, u8 offset,
>>   	return drm_scdc_write(adapter, offset, &value, sizeof(value));
>>   }
>>   
>> +/**
>> + * drm_scdc_enable_scrambling - enable scrambling
>> + * @adapter: I2C adapter for DDC channel
>> + *
>> + * Writes the TMDS config over SCDC channel, and enables scrambling
>> + * Returns:
>> + * True if scrambling is successfully enabled, false otherwise.
>> + */
>> +
>> +bool drm_scdc_enable_scrambling(struct i2c_adapter *adapter);
>> +
>> +/**
>> + * drm_scdc_disable_scrambling - disable scrambling
>> + * @adapter: I2C adapter for DDC channel
>> + *
>> + * Write the TMDS config over SCDC channel, and disable scrambling
>> + * Return: True if scrambling is successfully disabled, false otherwise.
>> + */
>> +bool drm_scdc_disable_scrambling(struct i2c_adapter *adapter);
>> +
>> +/**
>> + * drm_scdc_set_high_tmds_clock_ratio - set TMDS clock ratio
>> + * @adapter: I2C adapter for DDC channel
>> + *
>> + * Writes the TMDS config over SCDC channel, and sets TMDS
>> + * clock ratio to 1/40
>> + * Returns:
>> + * True if write is successful, false otherwise.
>> + */
>> +bool drm_scdc_set_high_tmds_clock_ratio(struct i2c_adapter *adapter);
>> +
>> +/**
>> + * drm_scdc_clear_high_tmds_clock_ratio - clear TMDS clock ratio
>> + * @adapter: I2C adapter for DDC channel
>> + *
>> + * Writes the TMDS config over SCDC channel, and sets TMDS
>> + * clock ratio back to 1/10 (from 1/40)
>> + * Returns:
>> + * True if write is successful, false otherwise.
>> + */
>> +bool drm_scdc_clear_high_tmds_clock_ratio(struct i2c_adapter *adapter);
>>   #endif
>> -- 
>> 1.9.1

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

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

[Sharma, Shashank]

Regards

Shashank


On 2/22/2017 10:59 PM, Ville Syrjälä wrote:
> On Wed, Feb 22, 2017 at 06:48:30PM +0530, Shashank Sharma wrote:
>> Geminilake platform sports 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.
>>
>> V2: Addressed review comments from Ville:
>> - Do not track scrambling status in DRM layer, track somewhere in
>>    driver like in intel_crtc_state.
>> - Don't talk to monitor at such a low layer, set monitor scrambling
>>    in intel_enable_ddi() before enabling the port.
>>
>> V3: Addressed review comments from Jani
>> - In comments, function names, use "sink" instead of "monitor",
>>    so that the implementation could be close to the language of
>>    HDMI spec.
>>
>> V4: Addressed review comment from Maarten
>> - scrambling -> hdmi_scrambling
>>    high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h   |   4 ++
>>   drivers/gpu/drm/i915/intel_ddi.c  |  28 ++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h  |  14 +++++
>>   drivers/gpu/drm/i915/intel_hdmi.c | 108 ++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 154 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 141a5c1..81cf10b 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7819,7 +7819,11 @@ enum {
>>   #define  TRANS_DDI_EDP_INPUT_B_ONOFF	(5<<12)
>>   #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
>>   #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
>> +#define  TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
>> +#define  TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
>>   #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 cd6fedd..bd8293d 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_handle_source_scrambling(
>> +				intel_encoder,
>> +				&intel_crtc->config->base.adjusted_mode, temp);
>>   	} else if (type == INTEL_OUTPUT_ANALOG) {
>>   		temp |= TRANS_DDI_MODE_SELECT_FDI;
>>   		temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
>> @@ -1843,6 +1848,21 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
>>   		struct intel_digital_port *intel_dig_port =
>>   			enc_to_dig_port(encoder);
>>   
>> +		if (IS_GEMINILAKE(dev_priv)) {
>> +			struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
>> +			/*
>> +			 * GLK sports a native HDMI 2.0 controller. If required
>> +			 * clock rate is > 340 Mhz && scrambling is supported
>> +			 * by sink, enable scrambling before enabling the
>> +			 * HDMI 2.0 port. The sink can choose to disable the
>> +			 * scrambling if it doesn't detect a scrambled within
>> +			 * 100 ms.
>> +			 */
>> +			intel_hdmi_handle_sink_scrambling(intel_encoder,
>> +						conn_state->connector,
>> +						crtc->config, true);
>> +		}
>> +
>>   		/* In HDMI/DVI mode, the port width, and swing/emphasis values
>>   		 * are ignored so nothing special needs to be done besides
>>   		 * enabling the port.
>> @@ -1875,6 +1895,14 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
>>   	if (old_crtc_state->has_audio)
>>   		intel_audio_codec_disable(intel_encoder);
>>   
>> +	if (type == INTEL_OUTPUT_HDMI) {
>> +		struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
>> +
>> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
>> +					old_conn_state->connector,
>> +					intel_crtc->config, false);
>> +	}
>> +
>>   	if (type == INTEL_OUTPUT_EDP) {
>>   		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>   
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 821c57c..c7262d7 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -691,6 +691,12 @@ struct intel_crtc_state {
>>   
>>   	/* Gamma mode programmed on the pipe */
>>   	uint32_t gamma_mode;
>> +
>> +	/* HDMI scrambling status (sink) */
>> +	bool hdmi_scrambling;
>> +
>> +	/* HDMI High TMDS char rate ratio (sink) */
>> +	bool hdmi_high_tmds_clock_ratio;
>>   };
>>   
>>   struct vlv_wm_state {
>> @@ -1609,6 +1615,14 @@ 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_handle_source_scrambling(struct intel_encoder *intel_encoder,
>> +					struct drm_display_mode *mode,
>> +					uint32_t config);
>> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
>> +					struct drm_connector *connector,
>> +					struct intel_crtc_state *config,
>> +					bool enable);
>>   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 a580de8..c44beee 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -34,6 +34,7 @@
>>   #include <drm/drm_atomic_helper.h>
>>   #include <drm/drm_crtc.h>
>>   #include <drm/drm_edid.h>
>> +#include <drm/drm_scdc_helper.h>
>>   #include "intel_drv.h"
>>   #include <drm/i915_drm.h>
>>   #include <drm/intel_lpe_audio.h>
>> @@ -1795,6 +1796,113 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
>>   	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>   }
>>   
>> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
>> +				       struct drm_connector *connector,
>> +				       struct intel_crtc_state *config,
>> +				       bool enable)
>> +{
>> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
>> +	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
>> +	struct drm_scrambling *scrambling = &scdc->scrambling;
>> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
>> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
>> +	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
>> +							  intel_hdmi->ddc_bus);
>> +
>> +	if (!scrambling->supported)
>> +		return;
>> +
>> +	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
>> +			intel_encoder->base.name, connector->name);
>> +
>> +	if (enable) {
>> +
>> +		if (mode->clock > 340000) {
>> +			/* Set TMDS bit clock ratio to 1/40 */
>> +			config->hdmi_high_tmds_clock_ratio =
>> +				drm_scdc_set_high_tmds_clock_ratio(adptr);
> You're not allowed to muck with the state anymore. All state computation
> should happen in the .compute_config hook.
ok, let me re-arrange this function in such a way that I do the config 
update part in compute_config
>> +			if (!config->hdmi_high_tmds_clock_ratio) {
>> +				DRM_ERROR("Set high TMDS ratio failed\n");
>> +				return;
>> +			}
>> +
>> +			/* Enable sink scrambling */
>> +			config->hdmi_scrambling =
>> +					drm_scdc_enable_scrambling(adptr);
>> +			if (!config->hdmi_scrambling) {
>> +				DRM_ERROR("Can't enable sink scrambling\n");
>> +				return;
>> +			}
>> +		}
>> +
>> +		/* Few sinks support scrambling at clocks <=340 MHz too */
>> +		if (!config->hdmi_scrambling && scrambling->low_rates) {
>> +			config->hdmi_scrambling =
>> +					drm_scdc_enable_scrambling(adptr);
>> +			if (!config->hdmi_scrambling)
>> +				DRM_ERROR("Can't enable sink scrambling\n");
>> +		}
>> +
>> +		return;
>> +	}
>> +
>> +	if (config->hdmi_high_tmds_clock_ratio) {
>> +		/* Set TMDS bit clock ratio back to 1/10 */
>> +		config->hdmi_high_tmds_clock_ratio =
>> +			!(drm_scdc_clear_high_tmds_clock_ratio(adptr));
>> +		if (config->hdmi_high_tmds_clock_ratio)
>> +			DRM_ERROR("Reset high TMDS ratio failed\n");
>> +	}
>> +
>> +	if (config->hdmi_scrambling) {
>> +		/* Disable sink scrambling */
>> +		config->hdmi_scrambling = !(drm_scdc_disable_scrambling(adptr));
>> +		if (config->hdmi_scrambling)
>> +			DRM_ERROR("Disable sink scrambling failed\n");
>> +	}
>> +}
>> +
>> +static inline uint32_t _intel_hdmi_set_source_scrambling(uint32_t hdmi_config)
>> +{
>> +	return hdmi_config |= (TRANS_DDI_HDMI_SCRAMBLING |
>> +			TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
>> +			TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
>> +}
>> +
>> +uint32_t
>> +intel_hdmi_handle_source_scrambling(struct intel_encoder *intel_encoder,
>> +			struct drm_display_mode *mode, uint32_t hdmi_config)
>> +{
>> +	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;
>> +	struct drm_scrambling *scrambling = &hdmi_info->scdc.scrambling;
>> +
>> +	DRM_DEBUG_KMS("Setting scrambling for enc:%s connector:%s\n",
>> +			intel_encoder->base.name, connector->name);
>> +
>> +	hdmi_config &= ~(TRANS_DDI_HDMI_SCRAMBLING |
>> +		TRANS_DDI_HIGH_TMDS_CHAR_RATE |
>> +		TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
>> +		TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
>> +
>> +	if (mode->clock <= 340000) {
>> +		/* Few sinks support scrambling at rate < 340 MHz too */
>> +		if (scrambling->low_rates)
>> +			hdmi_config =
>> +				_intel_hdmi_set_source_scrambling(hdmi_config);
>> +		return hdmi_config;
>> +	}
>> +
>> +	/* Scrambling or not, if clock > 340 MHz, set high char rate */
>> +	hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
>> +
>> +	if (scrambling->supported)
>> +		hdmi_config = _intel_hdmi_set_source_scrambling(hdmi_config);
>> +
>> +	return hdmi_config;
>> +}
> Seems overly complicated to me. It could just be something simple like:
>
> void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
> {
> 	...
> 	if (config->hdmi_scrambling)
> 		temp |= TRANS_DDI_HDMI_SCRAMBLING;
> 	
> 	if (config->whatever)
> 		temp |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> 	...
> }
I know it appears like complicated, but actually the requirement is 
complicated:
- if clock is > 340 Mhz, set scrambling && set_high_tmds_clock_ratio
- if clock is < 340 Mhz, but if monitor supports scrambling at lower 
clocks, set scrambling only (not tmds_clock_ratio)
- to set scrambling, we have to first check if scrambling is supported 
in monitor
So to meet all these condition, you will end up with something like 
what's implemented above.
Do you see a  better way ?

- Shashank
>
>> +
>>   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 v4 5/6] drm/i915: enable scrambling

[Ander Conselvan De Oliveira]

On Thu, 2017-02-23 at 10:01 +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 2/22/2017 10:59 PM, Ville Syrjälä wrote:
> > On Wed, Feb 22, 2017 at 06:48:30PM +0530, Shashank Sharma wrote:
> > > Geminilake platform sports 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.
> > > 
> > > V2: Addressed review comments from Ville:
> > > - Do not track scrambling status in DRM layer, track somewhere in
> > >    driver like in intel_crtc_state.
> > > - Don't talk to monitor at such a low layer, set monitor scrambling
> > >    in intel_enable_ddi() before enabling the port.
> > > 
> > > V3: Addressed review comments from Jani
> > > - In comments, function names, use "sink" instead of "monitor",
> > >    so that the implementation could be close to the language of
> > >    HDMI spec.
> > > 
> > > V4: Addressed review comment from Maarten
> > > - scrambling -> hdmi_scrambling
> > >    high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio
> > > 
> > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_reg.h   |   4 ++
> > >   drivers/gpu/drm/i915/intel_ddi.c  |  28 ++++++++++
> > >   drivers/gpu/drm/i915/intel_drv.h  |  14 +++++
> > >   drivers/gpu/drm/i915/intel_hdmi.c | 108 ++++++++++++++++++++++++++++++++++++++
> > >   4 files changed, 154 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 141a5c1..81cf10b 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -7819,7 +7819,11 @@ enum {
> > >   #define  TRANS_DDI_EDP_INPUT_B_ONOFF	(5<<12)
> > >   #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
> > >   #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
> > > +#define  TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
> > > +#define  TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
> > >   #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 cd6fedd..bd8293d 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_handle_source_scrambling(
> > > +				intel_encoder,
> > > +				&intel_crtc->config->base.adjusted_mode, temp);
> > >   	} else if (type == INTEL_OUTPUT_ANALOG) {
> > >   		temp |= TRANS_DDI_MODE_SELECT_FDI;
> > >   		temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
> > > @@ -1843,6 +1848,21 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
> > >   		struct intel_digital_port *intel_dig_port =
> > >   			enc_to_dig_port(encoder);
> > >   
> > > +		if (IS_GEMINILAKE(dev_priv)) {
> > > +			struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> > > +			/*
> > > +			 * GLK sports a native HDMI 2.0 controller. If required
> > > +			 * clock rate is > 340 Mhz && scrambling is supported
> > > +			 * by sink, enable scrambling before enabling the
> > > +			 * HDMI 2.0 port. The sink can choose to disable the
> > > +			 * scrambling if it doesn't detect a scrambled within
> > > +			 * 100 ms.
> > > +			 */
> > > +			intel_hdmi_handle_sink_scrambling(intel_encoder,
> > > +						conn_state->connector,
> > > +						crtc->config, true);
> > > +		}
> > > +
> > >   		/* In HDMI/DVI mode, the port width, and swing/emphasis values
> > >   		 * are ignored so nothing special needs to be done besides
> > >   		 * enabling the port.
> > > @@ -1875,6 +1895,14 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
> > >   	if (old_crtc_state->has_audio)
> > >   		intel_audio_codec_disable(intel_encoder);
> > >   
> > > +	if (type == INTEL_OUTPUT_HDMI) {
> > > +		struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> > > +
> > > +		intel_hdmi_handle_sink_scrambling(intel_encoder,
> > > +					old_conn_state->connector,
> > > +					intel_crtc->config, false);
> > > +	}
> > > +
> > >   	if (type == INTEL_OUTPUT_EDP) {
> > >   		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > >   
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 821c57c..c7262d7 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -691,6 +691,12 @@ struct intel_crtc_state {
> > >   
> > >   	/* Gamma mode programmed on the pipe */
> > >   	uint32_t gamma_mode;
> > > +
> > > +	/* HDMI scrambling status (sink) */
> > > +	bool hdmi_scrambling;
> > > +
> > > +	/* HDMI High TMDS char rate ratio (sink) */
> > > +	bool hdmi_high_tmds_clock_ratio;
> > >   };
> > >   
> > >   struct vlv_wm_state {
> > > @@ -1609,6 +1615,14 @@ 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_handle_source_scrambling(struct intel_encoder *intel_encoder,
> > > +					struct drm_display_mode *mode,
> > > +					uint32_t config);
> > > +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> > > +					struct drm_connector *connector,
> > > +					struct intel_crtc_state *config,
> > > +					bool enable);
> > >   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 a580de8..c44beee 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -34,6 +34,7 @@
> > >   #include <drm/drm_atomic_helper.h>
> > >   #include <drm/drm_crtc.h>
> > >   #include <drm/drm_edid.h>
> > > +#include <drm/drm_scdc_helper.h>
> > >   #include "intel_drv.h"
> > >   #include <drm/i915_drm.h>
> > >   #include <drm/intel_lpe_audio.h>
> > > @@ -1795,6 +1796,113 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
> > >   	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> > >   }
> > >   
> > > +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> > > +				       struct drm_connector *connector,
> > > +				       struct intel_crtc_state *config,
> > > +				       bool enable)
> > > +{
> > > +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> > > +	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
> > > +	struct drm_scrambling *scrambling = &scdc->scrambling;
> > > +	struct drm_display_mode *mode = &config->base.adjusted_mode;
> > > +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> > > +	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,

There's only two letters missing. Maybe just spell adapter.

> > > +							  intel_hdmi->ddc_bus);
> > > +
> > > +	if (!scrambling->supported)
> > > +		return;
> > > +
> > > +	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
> > > +			intel_encoder->base.name, connector->name);
> > > +
> > > +	if (enable) {
> > > +
> > > +		if (mode->clock > 340000) {
> > > +			/* Set TMDS bit clock ratio to 1/40 */
> > > +			config->hdmi_high_tmds_clock_ratio =
> > > +				drm_scdc_set_high_tmds_clock_ratio(adptr);
> > 
> > You're not allowed to muck with the state anymore. All state computation
> > should happen in the .compute_config hook.
> 
> ok, let me re-arrange this function in such a way that I do the config 
> update part in compute_config
> > > +			if (!config->hdmi_high_tmds_clock_ratio) {
> > > +				DRM_ERROR("Set high TMDS ratio failed\n");
> > > +				return;
> > > +			}
> > > +
> > > +			/* Enable sink scrambling */
> > > +			config->hdmi_scrambling =
> > > +					drm_scdc_enable_scrambling(adptr);
> > > +			if (!config->hdmi_scrambling) {
> > > +				DRM_ERROR("Can't enable sink scrambling\n");
> > > +				return;
> > > +			}
> > > +		}
> > > +
> > > +		/* Few sinks support scrambling at clocks <=340 MHz too */
> > > +		if (!config->hdmi_scrambling && scrambling->low_rates) {
> > > +			config->hdmi_scrambling =
> > > +					drm_scdc_enable_scrambling(adptr);
> > > +			if (!config->hdmi_scrambling)
> > > +				DRM_ERROR("Can't enable sink scrambling\n");
> > > +		}
> > > +
> > > +		return;
> > > +	}
> > > +
> > > +	if (config->hdmi_high_tmds_clock_ratio) {
> > > +		/* Set TMDS bit clock ratio back to 1/10 */
> > > +		config->hdmi_high_tmds_clock_ratio =
> > > +			!(drm_scdc_clear_high_tmds_clock_ratio(adptr));
> > > +		if (config->hdmi_high_tmds_clock_ratio)
> > > +			DRM_ERROR("Reset high TMDS ratio failed\n");
> > > +	}
> > > +
> > > +	if (config->hdmi_scrambling) {
> > > +		/* Disable sink scrambling */
> > > +		config->hdmi_scrambling = !(drm_scdc_disable_scrambling(adptr));
> > > +		if (config->hdmi_scrambling)
> > > +			DRM_ERROR("Disable sink scrambling failed\n");
> > > +	}
> > > +}
> > > +
> > > +static inline uint32_t _intel_hdmi_set_source_scrambling(uint32_t hdmi_config)
> > > +{
> > > +	return hdmi_config |= (TRANS_DDI_HDMI_SCRAMBLING |
> > > +			TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
> > > +			TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
> > > +}
> > > +
> > > +uint32_t
> > > +intel_hdmi_handle_source_scrambling(struct intel_encoder *intel_encoder,
> > > +			struct drm_display_mode *mode, uint32_t hdmi_config)
> > > +{
> > > +	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;
> > > +	struct drm_scrambling *scrambling = &hdmi_info->scdc.scrambling;
> > > +
> > > +	DRM_DEBUG_KMS("Setting scrambling for enc:%s connector:%s\n",
> > > +			intel_encoder->base.name, connector->name);
> > > +
> > > +	hdmi_config &= ~(TRANS_DDI_HDMI_SCRAMBLING |
> > > +		TRANS_DDI_HIGH_TMDS_CHAR_RATE |
> > > +		TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
> > > +		TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
> > > +
> > > +	if (mode->clock <= 340000) {
> > > +		/* Few sinks support scrambling at rate < 340 MHz too */
> > > +		if (scrambling->low_rates)
> > > +			hdmi_config =
> > > +				_intel_hdmi_set_source_scrambling(hdmi_config);
> > > +		return hdmi_config;
> > > +	}
> > > +
> > > +	/* Scrambling or not, if clock > 340 MHz, set high char rate */
> > > +	hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> > > +
> > > +	if (scrambling->supported)
> > > +		hdmi_config = _intel_hdmi_set_source_scrambling(hdmi_config);
> > > +
> > > +	return hdmi_config;
> > > +}
> > 
> > Seems overly complicated to me. It could just be something simple like:
> > 
> > void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
> > {
> > 	...
> > 	if (config->hdmi_scrambling)
> > 		temp |= TRANS_DDI_HDMI_SCRAMBLING;
> > 	
> > 	if (config->whatever)
> > 		temp |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> > 	...
> > }
> 
> I know it appears like complicated, but actually the requirement is 
> complicated:
> - if clock is > 340 Mhz, set scrambling && set_high_tmds_clock_ratio
> - if clock is < 340 Mhz, but if monitor supports scrambling at lower 
> clocks, set scrambling only (not tmds_clock_ratio)
> - to set scrambling, we have to first check if scrambling is supported 
> in monitor
> So to meet all these condition, you will end up with something like 
> what's implemented above.
> Do you see a  better way ?

Put that intel_hdmi_compute_config() so you check it only once, i.e.:

if (clock > 340 || scrambling->low_rates)
	config->hdmi_scrambling = true;

if (clock > 340)
	config->hdmi_high_tmds = true;

Then you can do just as Ville said above in intel_ddi_enable_transcoder_func().
And also for the sink part, no need to check the input values again:

if (config->hdmi_high_tmds)
	if (!drm_scdc_set_high_tmds_clock_ratio(adapter, enable))
		DRM_ERROR();

if (config->hdmi_scrambing)
	if (!drm_scdc_set_scrambling(adapter, enable))
		DRM_ERROR();

At least that's how I'd interpret what Ville wrote.

Ander


> 
> - Shashank
> > 
> > > +
> > >   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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sharma, Shashank]

Thanks for the review Ander, my comments, inline.


Regards

Shashank


On 2/23/2017 1:33 PM, Ander Conselvan De Oliveira wrote:
> On Thu, 2017-02-23 at 10:01 +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 2/22/2017 10:59 PM, Ville Syrjälä wrote:
>>> On Wed, Feb 22, 2017 at 06:48:30PM +0530, Shashank Sharma wrote:
>>>> Geminilake platform sports 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.
>>>>
>>>> V2: Addressed review comments from Ville:
>>>> - Do not track scrambling status in DRM layer, track somewhere in
>>>>     driver like in intel_crtc_state.
>>>> - Don't talk to monitor at such a low layer, set monitor scrambling
>>>>     in intel_enable_ddi() before enabling the port.
>>>>
>>>> V3: Addressed review comments from Jani
>>>> - In comments, function names, use "sink" instead of "monitor",
>>>>     so that the implementation could be close to the language of
>>>>     HDMI spec.
>>>>
>>>> V4: Addressed review comment from Maarten
>>>> - scrambling -> hdmi_scrambling
>>>>     high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio
>>>>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_reg.h   |   4 ++
>>>>    drivers/gpu/drm/i915/intel_ddi.c  |  28 ++++++++++
>>>>    drivers/gpu/drm/i915/intel_drv.h  |  14 +++++
>>>>    drivers/gpu/drm/i915/intel_hdmi.c | 108 ++++++++++++++++++++++++++++++++++++++
>>>>    4 files changed, 154 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index 141a5c1..81cf10b 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -7819,7 +7819,11 @@ enum {
>>>>    #define  TRANS_DDI_EDP_INPUT_B_ONOFF	(5<<12)
>>>>    #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
>>>>    #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
>>>> +#define  TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
>>>> +#define  TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
>>>>    #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 cd6fedd..bd8293d 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_handle_source_scrambling(
>>>> +				intel_encoder,
>>>> +				&intel_crtc->config->base.adjusted_mode, temp);
>>>>    	} else if (type == INTEL_OUTPUT_ANALOG) {
>>>>    		temp |= TRANS_DDI_MODE_SELECT_FDI;
>>>>    		temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
>>>> @@ -1843,6 +1848,21 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
>>>>    		struct intel_digital_port *intel_dig_port =
>>>>    			enc_to_dig_port(encoder);
>>>>    
>>>> +		if (IS_GEMINILAKE(dev_priv)) {
>>>> +			struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
>>>> +			/*
>>>> +			 * GLK sports a native HDMI 2.0 controller. If required
>>>> +			 * clock rate is > 340 Mhz && scrambling is supported
>>>> +			 * by sink, enable scrambling before enabling the
>>>> +			 * HDMI 2.0 port. The sink can choose to disable the
>>>> +			 * scrambling if it doesn't detect a scrambled within
>>>> +			 * 100 ms.
>>>> +			 */
>>>> +			intel_hdmi_handle_sink_scrambling(intel_encoder,
>>>> +						conn_state->connector,
>>>> +						crtc->config, true);
>>>> +		}
>>>> +
>>>>    		/* In HDMI/DVI mode, the port width, and swing/emphasis values
>>>>    		 * are ignored so nothing special needs to be done besides
>>>>    		 * enabling the port.
>>>> @@ -1875,6 +1895,14 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
>>>>    	if (old_crtc_state->has_audio)
>>>>    		intel_audio_codec_disable(intel_encoder);
>>>>    
>>>> +	if (type == INTEL_OUTPUT_HDMI) {
>>>> +		struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
>>>> +
>>>> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
>>>> +					old_conn_state->connector,
>>>> +					intel_crtc->config, false);
>>>> +	}
>>>> +
>>>>    	if (type == INTEL_OUTPUT_EDP) {
>>>>    		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>>>    
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>> index 821c57c..c7262d7 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -691,6 +691,12 @@ struct intel_crtc_state {
>>>>    
>>>>    	/* Gamma mode programmed on the pipe */
>>>>    	uint32_t gamma_mode;
>>>> +
>>>> +	/* HDMI scrambling status (sink) */
>>>> +	bool hdmi_scrambling;
>>>> +
>>>> +	/* HDMI High TMDS char rate ratio (sink) */
>>>> +	bool hdmi_high_tmds_clock_ratio;
>>>>    };
>>>>    
>>>>    struct vlv_wm_state {
>>>> @@ -1609,6 +1615,14 @@ 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_handle_source_scrambling(struct intel_encoder *intel_encoder,
>>>> +					struct drm_display_mode *mode,
>>>> +					uint32_t config);
>>>> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
>>>> +					struct drm_connector *connector,
>>>> +					struct intel_crtc_state *config,
>>>> +					bool enable);
>>>>    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 a580de8..c44beee 100644
>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> @@ -34,6 +34,7 @@
>>>>    #include <drm/drm_atomic_helper.h>
>>>>    #include <drm/drm_crtc.h>
>>>>    #include <drm/drm_edid.h>
>>>> +#include <drm/drm_scdc_helper.h>
>>>>    #include "intel_drv.h"
>>>>    #include <drm/i915_drm.h>
>>>>    #include <drm/intel_lpe_audio.h>
>>>> @@ -1795,6 +1796,113 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
>>>>    	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>>>    }
>>>>    
>>>> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
>>>> +				       struct drm_connector *connector,
>>>> +				       struct intel_crtc_state *config,
>>>> +				       bool enable)
>>>> +{
>>>> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
>>>> +	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
>>>> +	struct drm_scrambling *scrambling = &scdc->scrambling;
>>>> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
>>>> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
>>>> +	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
> There's only two letters missing. Maybe just spell adapter.
The intention was to keep it within 80 char, and it was going 82 :P :-)
>>>> +							  intel_hdmi->ddc_bus);
>>>> +
>>>> +	if (!scrambling->supported)
>>>> +		return;
>>>> +
>>>> +	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
>>>> +			intel_encoder->base.name, connector->name);
>>>> +
>>>> +	if (enable) {
>>>> +
>>>> +		if (mode->clock > 340000) {
>>>> +			/* Set TMDS bit clock ratio to 1/40 */
>>>> +			config->hdmi_high_tmds_clock_ratio =
>>>> +				drm_scdc_set_high_tmds_clock_ratio(adptr);
>>> You're not allowed to muck with the state anymore. All state computation
>>> should happen in the .compute_config hook.
>> ok, let me re-arrange this function in such a way that I do the config
>> update part in compute_config
>>>> +			if (!config->hdmi_high_tmds_clock_ratio) {
>>>> +				DRM_ERROR("Set high TMDS ratio failed\n");
>>>> +				return;
>>>> +			}
>>>> +
>>>> +			/* Enable sink scrambling */
>>>> +			config->hdmi_scrambling =
>>>> +					drm_scdc_enable_scrambling(adptr);
>>>> +			if (!config->hdmi_scrambling) {
>>>> +				DRM_ERROR("Can't enable sink scrambling\n");
>>>> +				return;
>>>> +			}
>>>> +		}
>>>> +
>>>> +		/* Few sinks support scrambling at clocks <=340 MHz too */
>>>> +		if (!config->hdmi_scrambling && scrambling->low_rates) {
>>>> +			config->hdmi_scrambling =
>>>> +					drm_scdc_enable_scrambling(adptr);
>>>> +			if (!config->hdmi_scrambling)
>>>> +				DRM_ERROR("Can't enable sink scrambling\n");
>>>> +		}
>>>> +
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	if (config->hdmi_high_tmds_clock_ratio) {
>>>> +		/* Set TMDS bit clock ratio back to 1/10 */
>>>> +		config->hdmi_high_tmds_clock_ratio =
>>>> +			!(drm_scdc_clear_high_tmds_clock_ratio(adptr));
>>>> +		if (config->hdmi_high_tmds_clock_ratio)
>>>> +			DRM_ERROR("Reset high TMDS ratio failed\n");
>>>> +	}
>>>> +
>>>> +	if (config->hdmi_scrambling) {
>>>> +		/* Disable sink scrambling */
>>>> +		config->hdmi_scrambling = !(drm_scdc_disable_scrambling(adptr));
>>>> +		if (config->hdmi_scrambling)
>>>> +			DRM_ERROR("Disable sink scrambling failed\n");
>>>> +	}
>>>> +}
>>>> +
>>>> +static inline uint32_t _intel_hdmi_set_source_scrambling(uint32_t hdmi_config)
>>>> +{
>>>> +	return hdmi_config |= (TRANS_DDI_HDMI_SCRAMBLING |
>>>> +			TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
>>>> +			TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
>>>> +}
>>>> +
>>>> +uint32_t
>>>> +intel_hdmi_handle_source_scrambling(struct intel_encoder *intel_encoder,
>>>> +			struct drm_display_mode *mode, uint32_t hdmi_config)
>>>> +{
>>>> +	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;
>>>> +	struct drm_scrambling *scrambling = &hdmi_info->scdc.scrambling;
>>>> +
>>>> +	DRM_DEBUG_KMS("Setting scrambling for enc:%s connector:%s\n",
>>>> +			intel_encoder->base.name, connector->name);
>>>> +
>>>> +	hdmi_config &= ~(TRANS_DDI_HDMI_SCRAMBLING |
>>>> +		TRANS_DDI_HIGH_TMDS_CHAR_RATE |
>>>> +		TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
>>>> +		TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
>>>> +
>>>> +	if (mode->clock <= 340000) {
>>>> +		/* Few sinks support scrambling at rate < 340 MHz too */
>>>> +		if (scrambling->low_rates)
>>>> +			hdmi_config =
>>>> +				_intel_hdmi_set_source_scrambling(hdmi_config);
>>>> +		return hdmi_config;
>>>> +	}
>>>> +
>>>> +	/* Scrambling or not, if clock > 340 MHz, set high char rate */
>>>> +	hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
>>>> +
>>>> +	if (scrambling->supported)
>>>> +		hdmi_config = _intel_hdmi_set_source_scrambling(hdmi_config);
>>>> +
>>>> +	return hdmi_config;
>>>> +}
>>> Seems overly complicated to me. It could just be something simple like:
>>>
>>> void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
>>> {
>>> 	...
>>> 	if (config->hdmi_scrambling)
>>> 		temp |= TRANS_DDI_HDMI_SCRAMBLING;
>>> 	
>>> 	if (config->whatever)
>>> 		temp |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
>>> 	...
>>> }
>> I know it appears like complicated, but actually the requirement is
>> complicated:
>> - if clock is > 340 Mhz, set scrambling && set_high_tmds_clock_ratio
>> - if clock is < 340 Mhz, but if monitor supports scrambling at lower
>> clocks, set scrambling only (not tmds_clock_ratio)
>> - to set scrambling, we have to first check if scrambling is supported
>> in monitor
>> So to meet all these condition, you will end up with something like
>> what's implemented above.
>> Do you see a  better way ?
> Put that intel_hdmi_compute_config() so you check it only once, i.e.:
>
> if (clock > 340 || scrambling->low_rates)
> 	config->hdmi_scrambling = true;
>
> if (clock > 340)
> 	config->hdmi_high_tmds = true;
>
> Then you can do just as Ville said above in intel_ddi_enable_transcoder_func().
Thats a good idea, I can do that, but there is one drawback of doing 
that. Right now, all of the HDMI
source/sink scrambling code is in one function, so for any 
debug/modification activity, you have to look
only in one function, also its more readable this way (in the flow)

The moment we distribute code in 3 different function, you have to track 
in all the places. Also, we are not
saving/reducing any if conditions, we are just moving it at some other 
place.

Do you still think it's worth doing it ?
> And also for the sink part, no need to check the input values again:
>
> if (config->hdmi_high_tmds)
> 	if (!drm_scdc_set_high_tmds_clock_ratio(adapter, enable))
> 		DRM_ERROR();
>
> if (config->hdmi_scrambing)
> 	if (!drm_scdc_set_scrambling(adapter, enable))
> 		DRM_ERROR();
>
> At least that's how I'd interpret what Ville wrote.
Actually design is slightly different. The state's hdmi_scrambling/clock 
bool's indicate that the sink's
scrambling/high_clock_ratio is enabled/set by source (and needs to be 
disabled), whereas connecotr's->display_info->scdc.scrambling
is to indicate monitor's capability to support scrambling and scdc

Think about two situations where:-
- Monitor supports scrambling and scdc, but we will not enable it, as 
the current mode is 1080P@148 MHz
- Monitor supports scrambling and scdc, and we will enable it, as the 
current mode is 4k@596 Mhz

To differentiate between these two, we have:
config->hdmi_scrambling which shows scrambling enabled on monitor by source
display_info->hdmi.scdc->scrambling which indicates monitor supports 
scrambling

Does it make sense ? Or you prefer some changes here ?

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

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

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

[Ville Syrjälä]

On Thu, Feb 23, 2017 at 08:51:03AM +0530, Sharma, Shashank wrote:
> Thanks for the review Ville, my comments inline.
> 
> Regards
> 
> Shashank
> 
> 
> On 2/22/2017 10:39 PM, Ville Syrjälä wrote:
> > On Wed, Feb 22, 2017 at 06:48:26PM +0530, Shashank Sharma 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.
> >>
> >> V2: Rebase.
> >> V3: Added R-B from Jose.
> >> V4: Rebase
> >>
> >> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> Reviewed-by: Jose Abreu <joabreu@synopsys.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));
> > That still disagrees with the documentation. I would suggest doing what
> > the DP dual mode helper does.
> Sure, I guess this is about how we are handling the return values from 
> I2C layer, I will change it
> the way its handled in drm_dp_dual_mode functions
> >
> >> +}
> >> +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)
> > We don't usually define macros like this to extract information from the
> > register. It's easy to confuse them with the normal macros for setting
> > bits.
> How about if I add another sub macro called _MAJOR, and use it like this:
> 
> #define _MAJOR(x) (x & 0xf)
> #define  SCDC_DEVICE_HARDWARE_REVISION_MAJOR(x) (_MAJOR((x) >> 4))
> 
> Or will you prefer no macro for this job at all ?

I think the usual pattern is a SHIFT + MASK macros for extracting
things from register values. But that is a little error prone and rather
verbose to use, so I do actually like the idea of having macros for
extracting bits from registers.

Hmm. After looking around a bit, I do see that DP_GET_SINK_COUNT() is an
extraction macro similar to what you have here. If we follow that example
then just adding _GET_ to the name should be good enough to distinguish
this from any macro to set the bits.

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

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

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

[Ville Syrjälä]

On Thu, Feb 23, 2017 at 09:05:16AM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 2/22/2017 10:54 PM, Ville Syrjälä wrote:
> > On Wed, Feb 22, 2017 at 06:48:29PM +0530, Shashank Sharma wrote:
> >> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
> >> than 340 MHz. 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.
> >>
> >> V2: Addressed review comments from Thierry, Ville and Dhinakaran
> >> Thierry:
> >> - Mhz -> MHz in comments and commit message.
> >> - i2c -> I2C in comments.
> >> - Fix the function documentations, keep in sync with drm_scdc_helper.c
> >> - drm_connector -> DRM connector.
> >> - Create structure for SCDC, and save scrambling status inside that,
> >>    in a sub-structure.
> >> - Call this sub-structure scrambling instead of scr_info.
> >> - low_rates -> low_clocks in scrambling status structure.
> >> - Store the return value of I2C read/write and print the error code
> >>    in case of failure.
> >>
> >> Thierry and Ville:
> >> - Move the scrambling enable/disable/query functions in
> >>    drm_scdc_helper.c file.
> >> - Add drm_SCDC prefix for the functions.
> >> - Optimize the return statement from function
> >>    drm_SCDC_check_scrambling_status.
> >>
> >> Ville:
> >> - Dont overwrite saved max TMDS clock value in display_info,
> >>    if max tmds clock from HF-VSDB is not > 340 MHz.
> >> - drm_detect_hdmi_scrambling -> drm_parse_hdmi_forum_vsdb.
> >> - Remove dynamic tracking of SCDC status from DRM layer, force bool.
> >> - Program clock ratio bit also, while enabling scrambling.
> >>
> >> Dhinakaran:
> >> - Add a comment about *5000 while extracting max clock supported.
> >>
> >> V3: Addressed review comments from Jose.
> >> - Create separate functions to enable scrambling and to set TMDS
> >>    clock/character rate ratio.
> >>
> >> V4: Rebase
> >>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> ---
> >>   drivers/gpu/drm/drm_edid.c        |  33 +++++++-
> >>   drivers/gpu/drm/drm_scdc_helper.c | 164 ++++++++++++++++++++++++++++++++++++++
> >>   include/drm/drm_connector.h       |  19 +++++
> >>   include/drm/drm_edid.h            |   6 +-
> >>   include/drm/drm_scdc_helper.h     |  41 ++++++++++
> >>   5 files changed, 261 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> index 63b79be..1e18c0a 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>
> >>   
> >>   #include "drm_crtc_internal.h"
> >>   
> >> @@ -3811,13 +3812,43 @@ enum hdmi_quantization_range
> >>   static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
> >>   				 const u8 *hf_vsdb)
> >>   {
> >> -	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
> >> +	struct drm_display_info *display = &connector->display_info;
> >> +	struct drm_hdmi_info *hdmi = &display->hdmi;
> >>   
> >>   	if (hf_vsdb[6] & 0x80) {
> >>   		hdmi->scdc.supported = true;
> >>   		if (hf_vsdb[6] & 0x40)
> >>   			hdmi->scdc.read_request = true;
> >>   	}
> >> +
> >> +	/*
> >> +	 * All HDMI 2.0 monitors must support scrambling at rates > 340 MHz.
> >> +	 * 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 (let's check)
> >> +	 * * SCDC support available (let's check)
> >> +	 * Lets check it out.
> >> +	 */
> >> +
> >> +	if (hf_vsdb[5]) {
> >> +		/* max clock is 5000 KHz times block value */
> >> +		u32 max_tmds_clock = hf_vsdb[5] * 5000;
> >> +		struct drm_scdc *scdc = &hdmi->scdc;
> >> +
> >> +		if (max_tmds_clock > 340000) {
> >> +			display->max_tmds_clock = max_tmds_clock;
> >> +			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> >> +				display->max_tmds_clock);
> >> +		}
> >> +
> >> +		if (scdc->supported) {
> >> +			scdc->scrambling.supported = true;
> >> +
> >> +			/* Few sinks support scrambling for cloks < 340M */
> >> +			if ((hf_vsdb[6] & 0x8))
> >> +				scdc->scrambling.low_rates = true;
> >> +		}
> >> +	}
> >>   }
> >>   
> >>   static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> >> diff --git a/drivers/gpu/drm/drm_scdc_helper.c b/drivers/gpu/drm/drm_scdc_helper.c
> >> index fe0e121..c0cf82b 100644
> >> --- a/drivers/gpu/drm/drm_scdc_helper.c
> >> +++ b/drivers/gpu/drm/drm_scdc_helper.c
> >> @@ -22,8 +22,10 @@
> >>    */
> >>   
> >>   #include <linux/slab.h>
> >> +#include <linux/delay.h>
> >>   
> >>   #include <drm/drm_scdc_helper.h>
> >> +#include <drm/drmP.h>
> >>   
> >>   /**
> >>    * DOC: scdc helpers
> >> @@ -109,3 +111,165 @@ ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
> >>   	return err;
> >>   }
> >>   EXPORT_SYMBOL(drm_scdc_write);
> >> +
> >> +/**
> >> + * drm_scdc_check_scrambling_status - what is status of scrambling?
> >> + * @adapter: I2C adapter for DDC channel
> >> + *
> >> + * Reads the scrambler status over SCDC, and checks the
> >> + * scrambling status.
> >> + *
> >> + * Returns:
> >> + * True if the scrambling is enabled, false otherwise.
> >> + */
> >> +
> >> +bool drm_scdc_check_scrambling_status(struct i2c_adapter *adapter)
> >> +{
> >> +	u8 status;
> >> +	int ret;
> >> +
> >> +	ret = drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status);
> >> +	if (ret < 0) {
> >> +		DRM_ERROR("Failed to read scrambling status, error %d\n", ret);
> >> +		return false;
> >> +	}
> >> +
> >> +	return status & SCDC_SCRAMBLING_STATUS;
> >> +}
> >> +EXPORT_SYMBOL(drm_scdc_check_scrambling_status);
> > s/check/get/ would make more sense to me.
> got it.
> >> +
> >> +/**
> >> + * drm_scdc_enable_scrambling - enable scrambling
> >> + * @adapter: I2C adapter for DDC channel
> >> + *
> >> + * Writes the TMDS config over SCDC channel, and enables scrambling
> >> + * Returns:
> >> + * True if scrambling is successfully enabled, false otherwise.
> >> + */
> >> +
> >> +bool drm_scdc_enable_scrambling(struct i2c_adapter *adapter)
> >> +{
> >> +	u8 config;
> >> +	int ret;
> >> +
> >> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
> >> +	if (ret < 0) {
> >> +		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
> >> +		return false;
> >> +	}
> >> +
> >> +	config |= SCDC_SCRAMBLING_ENABLE;
> >> +
> >> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
> >> +	if (ret < 0) {
> >> +		DRM_ERROR("Failed to enable scrambling, error %d\n", ret);
> >> +		return false;
> >> +	}
> >> +
> >> +	return true;
> >> +}
> >> +EXPORT_SYMBOL(drm_scdc_enable_scrambling);
> >> +
> >> +/**
> >> + * drm_scdc_disable_scrambling - disable scrambling
> >> + * @adapter: I2C adapter for DDC channel
> >> + *
> >> + * Write the TMDS config over SCDC channel, and disable scrambling
> >> + * Return: True if scrambling is successfully disabled, false otherwise.
> >> + */
> >> +bool drm_scdc_disable_scrambling(struct i2c_adapter *adapter)
> >> +{
> >> +	u8 config;
> >> +	int ret;
> >> +
> >> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
> >> +	if (ret < 0) {
> >> +		DRM_ERROR("Failed to read tmds config, error %d\n", ret);
> >> +		return false;
> >> +	}
> >> +
> >> +	config &= ~SCDC_SCRAMBLING_ENABLE;
> >> +
> >> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
> >> +	if (ret < 0) {
> >> +		DRM_ERROR("Failed to enable scrambling, error %d\n", ret);
> >> +		return false;
> >> +	}
> >> +
> >> +	return true;
> >> +}
> >> +EXPORT_SYMBOL(drm_scdc_disable_scrambling);
> >> +
> >> +/**
> >> + * drm_scdc_set_high_tmds_clock_ratio - set TMDS clock ratio
> >> + * @adapter: I2C adapter for DDC channel
> >> + *
> >> + * Writes the TMDS config over SCDC channel, and sets TMDS
> >> + * clock ratio to 1/40
> >> + * Returns:
> >> + * True if write is successful, false otherwise.
> >> + */
> >> +bool drm_scdc_set_high_tmds_clock_ratio(struct i2c_adapter *adapter)
> >> +{
> >> +	u8 config;
> >> +	int ret;
> >> +
> >> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
> >> +	if (ret < 0) {
> >> +		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
> >> +		return false;
> >> +	}
> >> +
> >> +	config |= SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
> >> +
> >> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
> >> +	if (ret < 0) {
> >> +		DRM_ERROR("Failed to set TMDS clock ratio, error %d\n", ret);
> >> +		return false;
> >> +	}
> >> +
> >> +	/*
> >> +	 * The spec says that the source should wait minimum 1ms and maximum
> >> +	 * 100ms after writing the TMDS config for clock ratio.
> >> +	 */
> >> +	usleep_range(1000, 100000);
> > Allowing the max to be 100ms here seems overly generous to me. In by
> > allowing 100ms we might be violating the spec since it will take a bit
> > of additonal time before the driver will enable the output. So I'd just
> > use something like 1-2 msec.
> Makes sense, but wouldn't 1-2 ms be too small ?
> Do you think we should allow at least 10ms ?

I don't think there's much merit in going that high. We do still want to
keep the modeset as short as possible, so explicitly saying that we are
happy to delay for up to 10ms doesn't seem like the best idea to me.

> >> +	return true;
> >> +}
> >> +EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
> >> +
> >> +/**
> >> + * drm_scdc_clear_high_tmds_clock_ratio - clear TMDS clock ratio
> >> + * @adapter: I2C adapter for DDC channel
> >> + *
> >> + * Writes the TMDS config over SCDC channel, and sets TMDS
> >> + * clock ratio back to 1/10 (from 1/40)
> >> + * Returns:
> >> + * True if write is successful, false otherwise.
> >> + */
> >> +bool drm_scdc_clear_high_tmds_clock_ratio(struct i2c_adapter *adapter)
> >> +{
> >> +	u8 config;
> >> +	int ret;
> >> +
> >> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
> >> +	if (ret < 0) {
> >> +		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
> >> +		return false;
> >> +	}
> >> +
> >> +	config &= ~SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
> >> +
> >> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
> >> +	if (ret < 0) {
> >> +		DRM_ERROR("Failed to clear TMDS clock ratio, error %d\n", ret);
> >> +		return false;
> >> +	}
> >> +
> >> +	/*
> >> +	 * The spec says that the source should wait minimum 1ms and maximum
> >> +	 * 100ms after writing the TMDS config for clock ratio.
> >> +	 */
> >> +	usleep_range(1000, 100000);
> >> +	return true;
> >> +}
> >> +EXPORT_SYMBOL(drm_scdc_clear_high_tmds_clock_ratio);
> > Having separate set/clear functions just leads to duplicated code IMO.
> > I'd just pass the desired state as a parameter.
> Ok, I will make a common function for both set/clear, and pass the 
> enable/disable state
> to it (similar to that which handles monitor scrambling)
> > And what I suggested earlier was to even combine this with the scrambling
> > write which would reduce the i2c traffic from 2 read and 2 writes to
> > just 1 write. But I can live with them being separate as well.
> Thanks. Actually, if you see V2, this first implementation was just as 
> you suggested.
> But there was a problem here, few monitors support scrambling at a clock 
> lower than
> 340Mhz too, in these cases we wont set the high_tmds_clock_ratio but 
> only set the scrambling.

Then the caller just passes clock_ratio=low and scrambling=true. I don't
see the problem.

> This was comment from Jose, which we addressed into splitting this into 
> two functions, which gives
> more control to caller function, by selecting what they want to set. but 
> yeah, cant beat the I2C traffic optimization.
> 
> - Shashank
> >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> >> index 6d5304e..78618308 100644
> >> --- a/include/drm/drm_connector.h
> >> +++ b/include/drm/drm_connector.h
> >> @@ -90,6 +90,20 @@ enum subpixel_order {
> >>   
> >>   };
> >>   
> >> +/**
> >> + * struct drm_scrambling: sink's scrambling support.
> >> + */
> >> +struct drm_scrambling {
> >> +	/**
> >> +	 * @supported: scrambling supported for rates > 340 Mhz.
> >> +	 */
> >> +	bool supported;
> >> +	/**
> >> +	 * @low_rates: scrambling supported for rates <= 340 Mhz.
> >> +	 */
> >> +	bool low_rates;
> >> +};
> >> +
> >>   /*
> >>    * struct drm_scdc - Information about scdc capabilities of a HDMI 2.0 sink
> >>    *
> >> @@ -105,8 +119,13 @@ struct drm_scdc {
> >>   	 * @read_request: sink is capable of generating scdc read request.
> >>   	 */
> >>   	bool read_request;
> >> +	/**
> >> +	 * @scrambling: sink's scrambling capabilities
> >> +	 */
> >> +	struct drm_scrambling scrambling;
> >>   };
> >>   
> >> +
> >>   /**
> >>    * struct drm_hdmi_info - runtime information about the connected HDMI sink
> >>    *
> >> 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__ */
> >> diff --git a/include/drm/drm_scdc_helper.h b/include/drm/drm_scdc_helper.h
> >> index 93b07bc..c5f2229 100644
> >> --- a/include/drm/drm_scdc_helper.h
> >> +++ b/include/drm/drm_scdc_helper.h
> >> @@ -129,4 +129,45 @@ static inline int drm_scdc_writeb(struct i2c_adapter *adapter, u8 offset,
> >>   	return drm_scdc_write(adapter, offset, &value, sizeof(value));
> >>   }
> >>   
> >> +/**
> >> + * drm_scdc_enable_scrambling - enable scrambling
> >> + * @adapter: I2C adapter for DDC channel
> >> + *
> >> + * Writes the TMDS config over SCDC channel, and enables scrambling
> >> + * Returns:
> >> + * True if scrambling is successfully enabled, false otherwise.
> >> + */
> >> +
> >> +bool drm_scdc_enable_scrambling(struct i2c_adapter *adapter);
> >> +
> >> +/**
> >> + * drm_scdc_disable_scrambling - disable scrambling
> >> + * @adapter: I2C adapter for DDC channel
> >> + *
> >> + * Write the TMDS config over SCDC channel, and disable scrambling
> >> + * Return: True if scrambling is successfully disabled, false otherwise.
> >> + */
> >> +bool drm_scdc_disable_scrambling(struct i2c_adapter *adapter);
> >> +
> >> +/**
> >> + * drm_scdc_set_high_tmds_clock_ratio - set TMDS clock ratio
> >> + * @adapter: I2C adapter for DDC channel
> >> + *
> >> + * Writes the TMDS config over SCDC channel, and sets TMDS
> >> + * clock ratio to 1/40
> >> + * Returns:
> >> + * True if write is successful, false otherwise.
> >> + */
> >> +bool drm_scdc_set_high_tmds_clock_ratio(struct i2c_adapter *adapter);
> >> +
> >> +/**
> >> + * drm_scdc_clear_high_tmds_clock_ratio - clear TMDS clock ratio
> >> + * @adapter: I2C adapter for DDC channel
> >> + *
> >> + * Writes the TMDS config over SCDC channel, and sets TMDS
> >> + * clock ratio back to 1/10 (from 1/40)
> >> + * Returns:
> >> + * True if write is successful, false otherwise.
> >> + */
> >> +bool drm_scdc_clear_high_tmds_clock_ratio(struct i2c_adapter *adapter);
> >>   #endif
> >> -- 
> >> 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 v4 1/6] drm: Add SCDC helpers

[Sharma, Shashank]

Regards

Shashank


On 2/23/2017 5:11 PM, Ville Syrjälä wrote:
> On Thu, Feb 23, 2017 at 08:51:03AM +0530, Sharma, Shashank wrote:
>> Thanks for the review Ville, my comments inline.
>>
>> Regards
>>
>> Shashank
>>
>>
>> On 2/22/2017 10:39 PM, Ville Syrjälä wrote:
>>> On Wed, Feb 22, 2017 at 06:48:26PM +0530, Shashank Sharma 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.
>>>>
>>>> V2: Rebase.
>>>> V3: Added R-B from Jose.
>>>> V4: Rebase
>>>>
>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> Reviewed-by: Jose Abreu <joabreu@synopsys.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));
>>> That still disagrees with the documentation. I would suggest doing what
>>> the DP dual mode helper does.
>> Sure, I guess this is about how we are handling the return values from
>> I2C layer, I will change it
>> the way its handled in drm_dp_dual_mode functions
>>>> +}
>>>> +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)
>>> We don't usually define macros like this to extract information from the
>>> register. It's easy to confuse them with the normal macros for setting
>>> bits.
>> How about if I add another sub macro called _MAJOR, and use it like this:
>>
>> #define _MAJOR(x) (x & 0xf)
>> #define  SCDC_DEVICE_HARDWARE_REVISION_MAJOR(x) (_MAJOR((x) >> 4))
>>
>> Or will you prefer no macro for this job at all ?
> I think the usual pattern is a SHIFT + MASK macros for extracting
> things from register values. But that is a little error prone and rather
> verbose to use, so I do actually like the idea of having macros for
> extracting bits from registers.
>
> Hmm. After looking around a bit, I do see that DP_GET_SINK_COUNT() is an
> extraction macro similar to what you have here. If we follow that example
> then just adding _GET_ to the name should be good enough to distinguish
> this from any macro to set the bits.
Cool, I will change the macro names as suggested.
Thanks

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

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

[Sharma, Shashank]

Regards

Shashank


On 2/23/2017 5:15 PM, Ville Syrjälä wrote:
> On Thu, Feb 23, 2017 at 09:05:16AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 2/22/2017 10:54 PM, Ville Syrjälä wrote:
>>> On Wed, Feb 22, 2017 at 06:48:29PM +0530, Shashank Sharma wrote:
>>>> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
>>>> than 340 MHz. 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.
>>>>
>>>> V2: Addressed review comments from Thierry, Ville and Dhinakaran
>>>> Thierry:
>>>> - Mhz -> MHz in comments and commit message.
>>>> - i2c -> I2C in comments.
>>>> - Fix the function documentations, keep in sync with drm_scdc_helper.c
>>>> - drm_connector -> DRM connector.
>>>> - Create structure for SCDC, and save scrambling status inside that,
>>>>     in a sub-structure.
>>>> - Call this sub-structure scrambling instead of scr_info.
>>>> - low_rates -> low_clocks in scrambling status structure.
>>>> - Store the return value of I2C read/write and print the error code
>>>>     in case of failure.
>>>>
>>>> Thierry and Ville:
>>>> - Move the scrambling enable/disable/query functions in
>>>>     drm_scdc_helper.c file.
>>>> - Add drm_SCDC prefix for the functions.
>>>> - Optimize the return statement from function
>>>>     drm_SCDC_check_scrambling_status.
>>>>
>>>> Ville:
>>>> - Dont overwrite saved max TMDS clock value in display_info,
>>>>     if max tmds clock from HF-VSDB is not > 340 MHz.
>>>> - drm_detect_hdmi_scrambling -> drm_parse_hdmi_forum_vsdb.
>>>> - Remove dynamic tracking of SCDC status from DRM layer, force bool.
>>>> - Program clock ratio bit also, while enabling scrambling.
>>>>
>>>> Dhinakaran:
>>>> - Add a comment about *5000 while extracting max clock supported.
>>>>
>>>> V3: Addressed review comments from Jose.
>>>> - Create separate functions to enable scrambling and to set TMDS
>>>>     clock/character rate ratio.
>>>>
>>>> V4: Rebase
>>>>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_edid.c        |  33 +++++++-
>>>>    drivers/gpu/drm/drm_scdc_helper.c | 164 ++++++++++++++++++++++++++++++++++++++
>>>>    include/drm/drm_connector.h       |  19 +++++
>>>>    include/drm/drm_edid.h            |   6 +-
>>>>    include/drm/drm_scdc_helper.h     |  41 ++++++++++
>>>>    5 files changed, 261 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>> index 63b79be..1e18c0a 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>
>>>>    
>>>>    #include "drm_crtc_internal.h"
>>>>    
>>>> @@ -3811,13 +3812,43 @@ enum hdmi_quantization_range
>>>>    static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
>>>>    				 const u8 *hf_vsdb)
>>>>    {
>>>> -	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
>>>> +	struct drm_display_info *display = &connector->display_info;
>>>> +	struct drm_hdmi_info *hdmi = &display->hdmi;
>>>>    
>>>>    	if (hf_vsdb[6] & 0x80) {
>>>>    		hdmi->scdc.supported = true;
>>>>    		if (hf_vsdb[6] & 0x40)
>>>>    			hdmi->scdc.read_request = true;
>>>>    	}
>>>> +
>>>> +	/*
>>>> +	 * All HDMI 2.0 monitors must support scrambling at rates > 340 MHz.
>>>> +	 * 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 (let's check)
>>>> +	 * * SCDC support available (let's check)
>>>> +	 * Lets check it out.
>>>> +	 */
>>>> +
>>>> +	if (hf_vsdb[5]) {
>>>> +		/* max clock is 5000 KHz times block value */
>>>> +		u32 max_tmds_clock = hf_vsdb[5] * 5000;
>>>> +		struct drm_scdc *scdc = &hdmi->scdc;
>>>> +
>>>> +		if (max_tmds_clock > 340000) {
>>>> +			display->max_tmds_clock = max_tmds_clock;
>>>> +			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
>>>> +				display->max_tmds_clock);
>>>> +		}
>>>> +
>>>> +		if (scdc->supported) {
>>>> +			scdc->scrambling.supported = true;
>>>> +
>>>> +			/* Few sinks support scrambling for cloks < 340M */
>>>> +			if ((hf_vsdb[6] & 0x8))
>>>> +				scdc->scrambling.low_rates = true;
>>>> +		}
>>>> +	}
>>>>    }
>>>>    
>>>>    static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>>>> diff --git a/drivers/gpu/drm/drm_scdc_helper.c b/drivers/gpu/drm/drm_scdc_helper.c
>>>> index fe0e121..c0cf82b 100644
>>>> --- a/drivers/gpu/drm/drm_scdc_helper.c
>>>> +++ b/drivers/gpu/drm/drm_scdc_helper.c
>>>> @@ -22,8 +22,10 @@
>>>>     */
>>>>    
>>>>    #include <linux/slab.h>
>>>> +#include <linux/delay.h>
>>>>    
>>>>    #include <drm/drm_scdc_helper.h>
>>>> +#include <drm/drmP.h>
>>>>    
>>>>    /**
>>>>     * DOC: scdc helpers
>>>> @@ -109,3 +111,165 @@ ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
>>>>    	return err;
>>>>    }
>>>>    EXPORT_SYMBOL(drm_scdc_write);
>>>> +
>>>> +/**
>>>> + * drm_scdc_check_scrambling_status - what is status of scrambling?
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Reads the scrambler status over SCDC, and checks the
>>>> + * scrambling status.
>>>> + *
>>>> + * Returns:
>>>> + * True if the scrambling is enabled, false otherwise.
>>>> + */
>>>> +
>>>> +bool drm_scdc_check_scrambling_status(struct i2c_adapter *adapter)
>>>> +{
>>>> +	u8 status;
>>>> +	int ret;
>>>> +
>>>> +	ret = drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to read scrambling status, error %d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	return status & SCDC_SCRAMBLING_STATUS;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_scdc_check_scrambling_status);
>>> s/check/get/ would make more sense to me.
>> got it.
>>>> +
>>>> +/**
>>>> + * drm_scdc_enable_scrambling - enable scrambling
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Writes the TMDS config over SCDC channel, and enables scrambling
>>>> + * Returns:
>>>> + * True if scrambling is successfully enabled, false otherwise.
>>>> + */
>>>> +
>>>> +bool drm_scdc_enable_scrambling(struct i2c_adapter *adapter)
>>>> +{
>>>> +	u8 config;
>>>> +	int ret;
>>>> +
>>>> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	config |= SCDC_SCRAMBLING_ENABLE;
>>>> +
>>>> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to enable scrambling, error %d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	return true;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_scdc_enable_scrambling);
>>>> +
>>>> +/**
>>>> + * drm_scdc_disable_scrambling - disable scrambling
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Write the TMDS config over SCDC channel, and disable scrambling
>>>> + * Return: True if scrambling is successfully disabled, false otherwise.
>>>> + */
>>>> +bool drm_scdc_disable_scrambling(struct i2c_adapter *adapter)
>>>> +{
>>>> +	u8 config;
>>>> +	int ret;
>>>> +
>>>> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to read tmds config, error %d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	config &= ~SCDC_SCRAMBLING_ENABLE;
>>>> +
>>>> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to enable scrambling, error %d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	return true;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_scdc_disable_scrambling);
>>>> +
>>>> +/**
>>>> + * drm_scdc_set_high_tmds_clock_ratio - set TMDS clock ratio
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Writes the TMDS config over SCDC channel, and sets TMDS
>>>> + * clock ratio to 1/40
>>>> + * Returns:
>>>> + * True if write is successful, false otherwise.
>>>> + */
>>>> +bool drm_scdc_set_high_tmds_clock_ratio(struct i2c_adapter *adapter)
>>>> +{
>>>> +	u8 config;
>>>> +	int ret;
>>>> +
>>>> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	config |= SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
>>>> +
>>>> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to set TMDS clock ratio, error %d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * The spec says that the source should wait minimum 1ms and maximum
>>>> +	 * 100ms after writing the TMDS config for clock ratio.
>>>> +	 */
>>>> +	usleep_range(1000, 100000);
>>> Allowing the max to be 100ms here seems overly generous to me. In by
>>> allowing 100ms we might be violating the spec since it will take a bit
>>> of additonal time before the driver will enable the output. So I'd just
>>> use something like 1-2 msec.
>> Makes sense, but wouldn't 1-2 ms be too small ?
>> Do you think we should allow at least 10ms ?
> I don't think there's much merit in going that high. We do still want to
> keep the modeset as short as possible, so explicitly saying that we are
> happy to delay for up to 10ms doesn't seem like the best idea to me.
Got it, 2 ms it is.
- Shashank
>>>> +	return true;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
>>>> +
>>>> +/**
>>>> + * drm_scdc_clear_high_tmds_clock_ratio - clear TMDS clock ratio
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Writes the TMDS config over SCDC channel, and sets TMDS
>>>> + * clock ratio back to 1/10 (from 1/40)
>>>> + * Returns:
>>>> + * True if write is successful, false otherwise.
>>>> + */
>>>> +bool drm_scdc_clear_high_tmds_clock_ratio(struct i2c_adapter *adapter)
>>>> +{
>>>> +	u8 config;
>>>> +	int ret;
>>>> +
>>>> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	config &= ~SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
>>>> +
>>>> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to clear TMDS clock ratio, error %d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * The spec says that the source should wait minimum 1ms and maximum
>>>> +	 * 100ms after writing the TMDS config for clock ratio.
>>>> +	 */
>>>> +	usleep_range(1000, 100000);
>>>> +	return true;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_scdc_clear_high_tmds_clock_ratio);
>>> Having separate set/clear functions just leads to duplicated code IMO.
>>> I'd just pass the desired state as a parameter.
>> Ok, I will make a common function for both set/clear, and pass the
>> enable/disable state
>> to it (similar to that which handles monitor scrambling)
>>> And what I suggested earlier was to even combine this with the scrambling
>>> write which would reduce the i2c traffic from 2 read and 2 writes to
>>> just 1 write. But I can live with them being separate as well.
>> Thanks. Actually, if you see V2, this first implementation was just as
>> you suggested.
>> But there was a problem here, few monitors support scrambling at a clock
>> lower than
>> 340Mhz too, in these cases we wont set the high_tmds_clock_ratio but
>> only set the scrambling.
> Then the caller just passes clock_ratio=low and scrambling=true. I don't
> see the problem.
>
>> This was comment from Jose, which we addressed into splitting this into
>> two functions, which gives
>> more control to caller function, by selecting what they want to set. but
>> yeah, cant beat the I2C traffic optimization.
>>
>> - Shashank
>>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>>> index 6d5304e..78618308 100644
>>>> --- a/include/drm/drm_connector.h
>>>> +++ b/include/drm/drm_connector.h
>>>> @@ -90,6 +90,20 @@ enum subpixel_order {
>>>>    
>>>>    };
>>>>    
>>>> +/**
>>>> + * struct drm_scrambling: sink's scrambling support.
>>>> + */
>>>> +struct drm_scrambling {
>>>> +	/**
>>>> +	 * @supported: scrambling supported for rates > 340 Mhz.
>>>> +	 */
>>>> +	bool supported;
>>>> +	/**
>>>> +	 * @low_rates: scrambling supported for rates <= 340 Mhz.
>>>> +	 */
>>>> +	bool low_rates;
>>>> +};
>>>> +
>>>>    /*
>>>>     * struct drm_scdc - Information about scdc capabilities of a HDMI 2.0 sink
>>>>     *
>>>> @@ -105,8 +119,13 @@ struct drm_scdc {
>>>>    	 * @read_request: sink is capable of generating scdc read request.
>>>>    	 */
>>>>    	bool read_request;
>>>> +	/**
>>>> +	 * @scrambling: sink's scrambling capabilities
>>>> +	 */
>>>> +	struct drm_scrambling scrambling;
>>>>    };
>>>>    
>>>> +
>>>>    /**
>>>>     * struct drm_hdmi_info - runtime information about the connected HDMI sink
>>>>     *
>>>> 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__ */
>>>> diff --git a/include/drm/drm_scdc_helper.h b/include/drm/drm_scdc_helper.h
>>>> index 93b07bc..c5f2229 100644
>>>> --- a/include/drm/drm_scdc_helper.h
>>>> +++ b/include/drm/drm_scdc_helper.h
>>>> @@ -129,4 +129,45 @@ static inline int drm_scdc_writeb(struct i2c_adapter *adapter, u8 offset,
>>>>    	return drm_scdc_write(adapter, offset, &value, sizeof(value));
>>>>    }
>>>>    
>>>> +/**
>>>> + * drm_scdc_enable_scrambling - enable scrambling
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Writes the TMDS config over SCDC channel, and enables scrambling
>>>> + * Returns:
>>>> + * True if scrambling is successfully enabled, false otherwise.
>>>> + */
>>>> +
>>>> +bool drm_scdc_enable_scrambling(struct i2c_adapter *adapter);
>>>> +
>>>> +/**
>>>> + * drm_scdc_disable_scrambling - disable scrambling
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Write the TMDS config over SCDC channel, and disable scrambling
>>>> + * Return: True if scrambling is successfully disabled, false otherwise.
>>>> + */
>>>> +bool drm_scdc_disable_scrambling(struct i2c_adapter *adapter);
>>>> +
>>>> +/**
>>>> + * drm_scdc_set_high_tmds_clock_ratio - set TMDS clock ratio
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Writes the TMDS config over SCDC channel, and sets TMDS
>>>> + * clock ratio to 1/40
>>>> + * Returns:
>>>> + * True if write is successful, false otherwise.
>>>> + */
>>>> +bool drm_scdc_set_high_tmds_clock_ratio(struct i2c_adapter *adapter);
>>>> +
>>>> +/**
>>>> + * drm_scdc_clear_high_tmds_clock_ratio - clear TMDS clock ratio
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Writes the TMDS config over SCDC channel, and sets TMDS
>>>> + * clock ratio back to 1/10 (from 1/40)
>>>> + * Returns:
>>>> + * True if write is successful, false otherwise.
>>>> + */
>>>> +bool drm_scdc_clear_high_tmds_clock_ratio(struct i2c_adapter *adapter);
>>>>    #endif
>>>> -- 
>>>> 1.9.1

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

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

[Ander Conselvan De Oliveira]

On Thu, 2017-02-23 at 16:33 +0530, Sharma, Shashank wrote:
> Thanks for the review Ander, my comments, inline.
> 
> 
> Regards
> 
> Shashank
> 
> 
> On 2/23/2017 1:33 PM, Ander Conselvan De Oliveira wrote:
> > On Thu, 2017-02-23 at 10:01 +0530, Sharma, Shashank wrote:
> > > Regards
> > > 
> > > Shashank
> > > 
> > > 
> > > On 2/22/2017 10:59 PM, Ville Syrjälä wrote:
> > > > On Wed, Feb 22, 2017 at 06:48:30PM +0530, Shashank Sharma wrote:
> > > > > Geminilake platform sports 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.
> > > > > 
> > > > > V2: Addressed review comments from Ville:
> > > > > - Do not track scrambling status in DRM layer, track somewhere in
> > > > >     driver like in intel_crtc_state.
> > > > > - Don't talk to monitor at such a low layer, set monitor scrambling
> > > > >     in intel_enable_ddi() before enabling the port.
> > > > > 
> > > > > V3: Addressed review comments from Jani
> > > > > - In comments, function names, use "sink" instead of "monitor",
> > > > >     so that the implementation could be close to the language of
> > > > >     HDMI spec.
> > > > > 
> > > > > V4: Addressed review comment from Maarten
> > > > > - scrambling -> hdmi_scrambling
> > > > >     high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio
> > > > > 
> > > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > > > > ---
> > > > >    drivers/gpu/drm/i915/i915_reg.h   |   4 ++
> > > > >    drivers/gpu/drm/i915/intel_ddi.c  |  28 ++++++++++
> > > > >    drivers/gpu/drm/i915/intel_drv.h  |  14 +++++
> > > > >    drivers/gpu/drm/i915/intel_hdmi.c | 108 ++++++++++++++++++++++++++++++++++++++
> > > > >    4 files changed, 154 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index 141a5c1..81cf10b 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -7819,7 +7819,11 @@ enum {
> > > > >    #define  TRANS_DDI_EDP_INPUT_B_ONOFF	(5<<12)
> > > > >    #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
> > > > >    #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
> > > > > +#define  TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
> > > > > +#define  TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
> > > > >    #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 cd6fedd..bd8293d 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_handle_source_scrambling(
> > > > > +				intel_encoder,
> > > > > +				&intel_crtc->config->base.adjusted_mode, temp);
> > > > >    	} else if (type == INTEL_OUTPUT_ANALOG) {
> > > > >    		temp |= TRANS_DDI_MODE_SELECT_FDI;
> > > > >    		temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
> > > > > @@ -1843,6 +1848,21 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
> > > > >    		struct intel_digital_port *intel_dig_port =
> > > > >    			enc_to_dig_port(encoder);
> > > > >    
> > > > > +		if (IS_GEMINILAKE(dev_priv)) {
> > > > > +			struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> > > > > +			/*
> > > > > +			 * GLK sports a native HDMI 2.0 controller. If required
> > > > > +			 * clock rate is > 340 Mhz && scrambling is supported
> > > > > +			 * by sink, enable scrambling before enabling the
> > > > > +			 * HDMI 2.0 port. The sink can choose to disable the
> > > > > +			 * scrambling if it doesn't detect a scrambled within
> > > > > +			 * 100 ms.
> > > > > +			 */
> > > > > +			intel_hdmi_handle_sink_scrambling(intel_encoder,
> > > > > +						conn_state->connector,
> > > > > +						crtc->config, true);
> > > > > +		}
> > > > > +
> > > > >    		/* In HDMI/DVI mode, the port width, and swing/emphasis values
> > > > >    		 * are ignored so nothing special needs to be done besides
> > > > >    		 * enabling the port.
> > > > > @@ -1875,6 +1895,14 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
> > > > >    	if (old_crtc_state->has_audio)
> > > > >    		intel_audio_codec_disable(intel_encoder);
> > > > >    
> > > > > +	if (type == INTEL_OUTPUT_HDMI) {
> > > > > +		struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> > > > > +
> > > > > +		intel_hdmi_handle_sink_scrambling(intel_encoder,
> > > > > +					old_conn_state->connector,
> > > > > +					intel_crtc->config, false);
> > > > > +	}
> > > > > +
> > > > >    	if (type == INTEL_OUTPUT_EDP) {
> > > > >    		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > >    
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 821c57c..c7262d7 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -691,6 +691,12 @@ struct intel_crtc_state {
> > > > >    
> > > > >    	/* Gamma mode programmed on the pipe */
> > > > >    	uint32_t gamma_mode;
> > > > > +
> > > > > +	/* HDMI scrambling status (sink) */
> > > > > +	bool hdmi_scrambling;
> > > > > +
> > > > > +	/* HDMI High TMDS char rate ratio (sink) */
> > > > > +	bool hdmi_high_tmds_clock_ratio;
> > > > >    };
> > > > >    
> > > > >    struct vlv_wm_state {
> > > > > @@ -1609,6 +1615,14 @@ 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_handle_source_scrambling(struct intel_encoder *intel_encoder,
> > > > > +					struct drm_display_mode *mode,
> > > > > +					uint32_t config);
> > > > > +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> > > > > +					struct drm_connector *connector,
> > > > > +					struct intel_crtc_state *config,
> > > > > +					bool enable);
> > > > >    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 a580de8..c44beee 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > > @@ -34,6 +34,7 @@
> > > > >    #include <drm/drm_atomic_helper.h>
> > > > >    #include <drm/drm_crtc.h>
> > > > >    #include <drm/drm_edid.h>
> > > > > +#include <drm/drm_scdc_helper.h>
> > > > >    #include "intel_drv.h"
> > > > >    #include <drm/i915_drm.h>
> > > > >    #include <drm/intel_lpe_audio.h>
> > > > > @@ -1795,6 +1796,113 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
> > > > >    	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> > > > >    }
> > > > >    
> > > > > +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> > > > > +				       struct drm_connector *connector,
> > > > > +				       struct intel_crtc_state *config,
> > > > > +				       bool enable)
> > > > > +{
> > > > > +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> > > > > +	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
> > > > > +	struct drm_scrambling *scrambling = &scdc->scrambling;
> > > > > +	struct drm_display_mode *mode = &config->base.adjusted_mode;
> > > > > +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> > > > > +	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
> > 
> > There's only two letters missing. Maybe just spell adapter.
> 
> The intention was to keep it within 80 char, and it was going 82 :P :-)
> > > > > +							  intel_hdmi->ddc_bus);
> > > > > +
> > > > > +	if (!scrambling->supported)
> > > > > +		return;
> > > > > +
> > > > > +	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
> > > > > +			intel_encoder->base.name, connector->name);
> > > > > +
> > > > > +	if (enable) {
> > > > > +
> > > > > +		if (mode->clock > 340000) {
> > > > > +			/* Set TMDS bit clock ratio to 1/40 */
> > > > > +			config->hdmi_high_tmds_clock_ratio =
> > > > > +				drm_scdc_set_high_tmds_clock_ratio(adptr);
> > > > 
> > > > You're not allowed to muck with the state anymore. All state computation
> > > > should happen in the .compute_config hook.
> > > 
> > > ok, let me re-arrange this function in such a way that I do the config
> > > update part in compute_config
> > > > > +			if (!config->hdmi_high_tmds_clock_ratio) {
> > > > > +				DRM_ERROR("Set high TMDS ratio failed\n");
> > > > > +				return;
> > > > > +			}
> > > > > +
> > > > > +			/* Enable sink scrambling */
> > > > > +			config->hdmi_scrambling =
> > > > > +					drm_scdc_enable_scrambling(adptr);
> > > > > +			if (!config->hdmi_scrambling) {
> > > > > +				DRM_ERROR("Can't enable sink scrambling\n");
> > > > > +				return;
> > > > > +			}
> > > > > +		}
> > > > > +
> > > > > +		/* Few sinks support scrambling at clocks <=340 MHz too */
> > > > > +		if (!config->hdmi_scrambling && scrambling->low_rates) {
> > > > > +			config->hdmi_scrambling =
> > > > > +					drm_scdc_enable_scrambling(adptr);
> > > > > +			if (!config->hdmi_scrambling)
> > > > > +				DRM_ERROR("Can't enable sink scrambling\n");
> > > > > +		}
> > > > > +
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	if (config->hdmi_high_tmds_clock_ratio) {
> > > > > +		/* Set TMDS bit clock ratio back to 1/10 */
> > > > > +		config->hdmi_high_tmds_clock_ratio =
> > > > > +			!(drm_scdc_clear_high_tmds_clock_ratio(adptr));
> > > > > +		if (config->hdmi_high_tmds_clock_ratio)
> > > > > +			DRM_ERROR("Reset high TMDS ratio failed\n");
> > > > > +	}
> > > > > +
> > > > > +	if (config->hdmi_scrambling) {
> > > > > +		/* Disable sink scrambling */
> > > > > +		config->hdmi_scrambling = !(drm_scdc_disable_scrambling(adptr));
> > > > > +		if (config->hdmi_scrambling)
> > > > > +			DRM_ERROR("Disable sink scrambling failed\n");
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +static inline uint32_t _intel_hdmi_set_source_scrambling(uint32_t hdmi_config)
> > > > > +{
> > > > > +	return hdmi_config |= (TRANS_DDI_HDMI_SCRAMBLING |
> > > > > +			TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
> > > > > +			TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
> > > > > +}
> > > > > +
> > > > > +uint32_t
> > > > > +intel_hdmi_handle_source_scrambling(struct intel_encoder *intel_encoder,
> > > > > +			struct drm_display_mode *mode, uint32_t hdmi_config)
> > > > > +{
> > > > > +	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;
> > > > > +	struct drm_scrambling *scrambling = &hdmi_info->scdc.scrambling;
> > > > > +
> > > > > +	DRM_DEBUG_KMS("Setting scrambling for enc:%s connector:%s\n",
> > > > > +			intel_encoder->base.name, connector->name);
> > > > > +
> > > > > +	hdmi_config &= ~(TRANS_DDI_HDMI_SCRAMBLING |
> > > > > +		TRANS_DDI_HIGH_TMDS_CHAR_RATE |
> > > > > +		TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
> > > > > +		TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
> > > > > +
> > > > > +	if (mode->clock <= 340000) {
> > > > > +		/* Few sinks support scrambling at rate < 340 MHz too */
> > > > > +		if (scrambling->low_rates)
> > > > > +			hdmi_config =
> > > > > +				_intel_hdmi_set_source_scrambling(hdmi_config);
> > > > > +		return hdmi_config;
> > > > > +	}
> > > > > +
> > > > > +	/* Scrambling or not, if clock > 340 MHz, set high char rate */
> > > > > +	hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> > > > > +
> > > > > +	if (scrambling->supported)
> > > > > +		hdmi_config = _intel_hdmi_set_source_scrambling(hdmi_config);
> > > > > +
> > > > > +	return hdmi_config;
> > > > > +}
> > > > 
> > > > Seems overly complicated to me. It could just be something simple like:
> > > > 
> > > > void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
> > > > {
> > > > 	...
> > > > 	if (config->hdmi_scrambling)
> > > > 		temp |= TRANS_DDI_HDMI_SCRAMBLING;
> > > > 	
> > > > 	if (config->whatever)
> > > > 		temp |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> > > > 	...
> > > > }
> > > 
> > > I know it appears like complicated, but actually the requirement is
> > > complicated:
> > > - if clock is > 340 Mhz, set scrambling && set_high_tmds_clock_ratio
> > > - if clock is < 340 Mhz, but if monitor supports scrambling at lower
> > > clocks, set scrambling only (not tmds_clock_ratio)
> > > - to set scrambling, we have to first check if scrambling is supported
> > > in monitor
> > > So to meet all these condition, you will end up with something like
> > > what's implemented above.
> > > Do you see a  better way ?
> > 
> > Put that intel_hdmi_compute_config() so you check it only once, i.e.:
> > 
> > if (clock > 340 || scrambling->low_rates)
> > 	config->hdmi_scrambling = true;
> > 
> > if (clock > 340)
> > 	config->hdmi_high_tmds = true;
> > 
> > Then you can do just as Ville said above in intel_ddi_enable_transcoder_func().
> 
> Thats a good idea, I can do that, but there is one drawback of doing 
> that. Right now, all of the HDMI
> source/sink scrambling code is in one function, so for any 
> debug/modification activity, you have to look
> only in one function, also its more readable this way (in the flow)
> 
> The moment we distribute code in 3 different function, you have to track 
> in all the places. Also, we are not
> saving/reducing any if conditions, we are just moving it at some other 
> place.

With the current patch both handle_source_scrambling() and
handle_sink_scrambling() have checks for clock above/below 340000 and whether
the sink support scrambling with low rates. Those two checks don't need to be
duplicated.

> Do you still think it's worth doing it ?

Yes, doing it in compute config follows the convention of the driver. If
something is set in crtc_state, it is only natural to assume it was set at some
point in the atomic check phase or from reading out the hardware state.

> > And also for the sink part, no need to check the input values again:
> > 
> > if (config->hdmi_high_tmds)
> > 	if (!drm_scdc_set_high_tmds_clock_ratio(adapter, enable))
> > 		DRM_ERROR();
> > 
> > if (config->hdmi_scrambing)
> > 	if (!drm_scdc_set_scrambling(adapter, enable))
> > 		DRM_ERROR();
> > 
> > At least that's how I'd interpret what Ville wrote.
> 
> Actually design is slightly different. The state's hdmi_scrambling/clock 
> bool's indicate that the sink's
> scrambling/high_clock_ratio is enabled/set by source (and needs to be 
> disabled), whereas connecotr's->display_info->scdc.scrambling
> is to indicate monitor's capability to support scrambling and scdc

The crtc_state shouldn't be changed to represent changes in the hardware state.
It is computed during the atomic check phase, and after that it should represent
the state that needs to be commited. Perhaps the code in hdmi_compute_config()
just needs to take the previous state into account?

> 
> Think about two situations where:-
> - Monitor supports scrambling and scdc, but we will not enable it, as 
> the current mode is 1080P@148 MHz
> - Monitor supports scrambling and scdc, and we will enable it, as the 
> current mode is 4k@596 Mhz
> 
> To differentiate between these two, we have:
> config->hdmi_scrambling which shows scrambling enabled on monitor by source
> display_info->hdmi.scdc->scrambling which indicates monitor supports 
> scrambling
> 
> Does it make sense ? Or you prefer some changes here ?

Is there any harm in disabling scrambling twice? If not I'd say just disable it
on every modeset if it is not needed. Then there's no need to know the previous
state at the moment we actually enable/disable it.

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

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

[Ville Syrjälä]

On Thu, Feb 23, 2017 at 03:07:40PM +0200, Ander Conselvan De Oliveira wrote:
> On Thu, 2017-02-23 at 16:33 +0530, Sharma, Shashank wrote:
> > Thanks for the review Ander, my comments, inline.
> > 
> > 
> > Regards
> > 
> > Shashank
> > 
> > 
> > On 2/23/2017 1:33 PM, Ander Conselvan De Oliveira wrote:
> > > On Thu, 2017-02-23 at 10:01 +0530, Sharma, Shashank wrote:
> > > > Regards
> > > > 
> > > > Shashank
> > > > 
> > > > 
> > > > On 2/22/2017 10:59 PM, Ville Syrjälä wrote:
> > > > > On Wed, Feb 22, 2017 at 06:48:30PM +0530, Shashank Sharma wrote:
> > > > > > Geminilake platform sports 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.
> > > > > > 
> > > > > > V2: Addressed review comments from Ville:
> > > > > > - Do not track scrambling status in DRM layer, track somewhere in
> > > > > >     driver like in intel_crtc_state.
> > > > > > - Don't talk to monitor at such a low layer, set monitor scrambling
> > > > > >     in intel_enable_ddi() before enabling the port.
> > > > > > 
> > > > > > V3: Addressed review comments from Jani
> > > > > > - In comments, function names, use "sink" instead of "monitor",
> > > > > >     so that the implementation could be close to the language of
> > > > > >     HDMI spec.
> > > > > > 
> > > > > > V4: Addressed review comment from Maarten
> > > > > > - scrambling -> hdmi_scrambling
> > > > > >     high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio
> > > > > > 
> > > > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > > > > > ---
> > > > > >    drivers/gpu/drm/i915/i915_reg.h   |   4 ++
> > > > > >    drivers/gpu/drm/i915/intel_ddi.c  |  28 ++++++++++
> > > > > >    drivers/gpu/drm/i915/intel_drv.h  |  14 +++++
> > > > > >    drivers/gpu/drm/i915/intel_hdmi.c | 108 ++++++++++++++++++++++++++++++++++++++
> > > > > >    4 files changed, 154 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > index 141a5c1..81cf10b 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > @@ -7819,7 +7819,11 @@ enum {
> > > > > >    #define  TRANS_DDI_EDP_INPUT_B_ONOFF	(5<<12)
> > > > > >    #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
> > > > > >    #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
> > > > > > +#define  TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
> > > > > > +#define  TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
> > > > > >    #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 cd6fedd..bd8293d 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_handle_source_scrambling(
> > > > > > +				intel_encoder,
> > > > > > +				&intel_crtc->config->base.adjusted_mode, temp);
> > > > > >    	} else if (type == INTEL_OUTPUT_ANALOG) {
> > > > > >    		temp |= TRANS_DDI_MODE_SELECT_FDI;
> > > > > >    		temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
> > > > > > @@ -1843,6 +1848,21 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
> > > > > >    		struct intel_digital_port *intel_dig_port =
> > > > > >    			enc_to_dig_port(encoder);
> > > > > >    
> > > > > > +		if (IS_GEMINILAKE(dev_priv)) {
> > > > > > +			struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> > > > > > +			/*
> > > > > > +			 * GLK sports a native HDMI 2.0 controller. If required
> > > > > > +			 * clock rate is > 340 Mhz && scrambling is supported
> > > > > > +			 * by sink, enable scrambling before enabling the
> > > > > > +			 * HDMI 2.0 port. The sink can choose to disable the
> > > > > > +			 * scrambling if it doesn't detect a scrambled within
> > > > > > +			 * 100 ms.
> > > > > > +			 */
> > > > > > +			intel_hdmi_handle_sink_scrambling(intel_encoder,
> > > > > > +						conn_state->connector,
> > > > > > +						crtc->config, true);
> > > > > > +		}
> > > > > > +
> > > > > >    		/* In HDMI/DVI mode, the port width, and swing/emphasis values
> > > > > >    		 * are ignored so nothing special needs to be done besides
> > > > > >    		 * enabling the port.
> > > > > > @@ -1875,6 +1895,14 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
> > > > > >    	if (old_crtc_state->has_audio)
> > > > > >    		intel_audio_codec_disable(intel_encoder);
> > > > > >    
> > > > > > +	if (type == INTEL_OUTPUT_HDMI) {
> > > > > > +		struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> > > > > > +
> > > > > > +		intel_hdmi_handle_sink_scrambling(intel_encoder,
> > > > > > +					old_conn_state->connector,
> > > > > > +					intel_crtc->config, false);
> > > > > > +	}
> > > > > > +
> > > > > >    	if (type == INTEL_OUTPUT_EDP) {
> > > > > >    		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > > >    
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > index 821c57c..c7262d7 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > @@ -691,6 +691,12 @@ struct intel_crtc_state {
> > > > > >    
> > > > > >    	/* Gamma mode programmed on the pipe */
> > > > > >    	uint32_t gamma_mode;
> > > > > > +
> > > > > > +	/* HDMI scrambling status (sink) */
> > > > > > +	bool hdmi_scrambling;
> > > > > > +
> > > > > > +	/* HDMI High TMDS char rate ratio (sink) */
> > > > > > +	bool hdmi_high_tmds_clock_ratio;
> > > > > >    };
> > > > > >    
> > > > > >    struct vlv_wm_state {
> > > > > > @@ -1609,6 +1615,14 @@ 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_handle_source_scrambling(struct intel_encoder *intel_encoder,
> > > > > > +					struct drm_display_mode *mode,
> > > > > > +					uint32_t config);
> > > > > > +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> > > > > > +					struct drm_connector *connector,
> > > > > > +					struct intel_crtc_state *config,
> > > > > > +					bool enable);
> > > > > >    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 a580de8..c44beee 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > > > @@ -34,6 +34,7 @@
> > > > > >    #include <drm/drm_atomic_helper.h>
> > > > > >    #include <drm/drm_crtc.h>
> > > > > >    #include <drm/drm_edid.h>
> > > > > > +#include <drm/drm_scdc_helper.h>
> > > > > >    #include "intel_drv.h"
> > > > > >    #include <drm/i915_drm.h>
> > > > > >    #include <drm/intel_lpe_audio.h>
> > > > > > @@ -1795,6 +1796,113 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
> > > > > >    	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> > > > > >    }
> > > > > >    
> > > > > > +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> > > > > > +				       struct drm_connector *connector,
> > > > > > +				       struct intel_crtc_state *config,
> > > > > > +				       bool enable)
> > > > > > +{
> > > > > > +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> > > > > > +	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
> > > > > > +	struct drm_scrambling *scrambling = &scdc->scrambling;
> > > > > > +	struct drm_display_mode *mode = &config->base.adjusted_mode;
> > > > > > +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> > > > > > +	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
> > > 
> > > There's only two letters missing. Maybe just spell adapter.
> > 
> > The intention was to keep it within 80 char, and it was going 82 :P :-)
> > > > > > +							  intel_hdmi->ddc_bus);
> > > > > > +
> > > > > > +	if (!scrambling->supported)
> > > > > > +		return;
> > > > > > +
> > > > > > +	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
> > > > > > +			intel_encoder->base.name, connector->name);
> > > > > > +
> > > > > > +	if (enable) {
> > > > > > +
> > > > > > +		if (mode->clock > 340000) {
> > > > > > +			/* Set TMDS bit clock ratio to 1/40 */
> > > > > > +			config->hdmi_high_tmds_clock_ratio =
> > > > > > +				drm_scdc_set_high_tmds_clock_ratio(adptr);
> > > > > 
> > > > > You're not allowed to muck with the state anymore. All state computation
> > > > > should happen in the .compute_config hook.
> > > > 
> > > > ok, let me re-arrange this function in such a way that I do the config
> > > > update part in compute_config
> > > > > > +			if (!config->hdmi_high_tmds_clock_ratio) {
> > > > > > +				DRM_ERROR("Set high TMDS ratio failed\n");
> > > > > > +				return;
> > > > > > +			}
> > > > > > +
> > > > > > +			/* Enable sink scrambling */
> > > > > > +			config->hdmi_scrambling =
> > > > > > +					drm_scdc_enable_scrambling(adptr);
> > > > > > +			if (!config->hdmi_scrambling) {
> > > > > > +				DRM_ERROR("Can't enable sink scrambling\n");
> > > > > > +				return;
> > > > > > +			}
> > > > > > +		}
> > > > > > +
> > > > > > +		/* Few sinks support scrambling at clocks <=340 MHz too */
> > > > > > +		if (!config->hdmi_scrambling && scrambling->low_rates) {
> > > > > > +			config->hdmi_scrambling =
> > > > > > +					drm_scdc_enable_scrambling(adptr);
> > > > > > +			if (!config->hdmi_scrambling)
> > > > > > +				DRM_ERROR("Can't enable sink scrambling\n");
> > > > > > +		}
> > > > > > +
> > > > > > +		return;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (config->hdmi_high_tmds_clock_ratio) {
> > > > > > +		/* Set TMDS bit clock ratio back to 1/10 */
> > > > > > +		config->hdmi_high_tmds_clock_ratio =
> > > > > > +			!(drm_scdc_clear_high_tmds_clock_ratio(adptr));
> > > > > > +		if (config->hdmi_high_tmds_clock_ratio)
> > > > > > +			DRM_ERROR("Reset high TMDS ratio failed\n");
> > > > > > +	}
> > > > > > +
> > > > > > +	if (config->hdmi_scrambling) {
> > > > > > +		/* Disable sink scrambling */
> > > > > > +		config->hdmi_scrambling = !(drm_scdc_disable_scrambling(adptr));
> > > > > > +		if (config->hdmi_scrambling)
> > > > > > +			DRM_ERROR("Disable sink scrambling failed\n");
> > > > > > +	}
> > > > > > +}
> > > > > > +
> > > > > > +static inline uint32_t _intel_hdmi_set_source_scrambling(uint32_t hdmi_config)
> > > > > > +{
> > > > > > +	return hdmi_config |= (TRANS_DDI_HDMI_SCRAMBLING |
> > > > > > +			TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
> > > > > > +			TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
> > > > > > +}
> > > > > > +
> > > > > > +uint32_t
> > > > > > +intel_hdmi_handle_source_scrambling(struct intel_encoder *intel_encoder,
> > > > > > +			struct drm_display_mode *mode, uint32_t hdmi_config)
> > > > > > +{
> > > > > > +	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;
> > > > > > +	struct drm_scrambling *scrambling = &hdmi_info->scdc.scrambling;
> > > > > > +
> > > > > > +	DRM_DEBUG_KMS("Setting scrambling for enc:%s connector:%s\n",
> > > > > > +			intel_encoder->base.name, connector->name);
> > > > > > +
> > > > > > +	hdmi_config &= ~(TRANS_DDI_HDMI_SCRAMBLING |
> > > > > > +		TRANS_DDI_HIGH_TMDS_CHAR_RATE |
> > > > > > +		TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
> > > > > > +		TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
> > > > > > +
> > > > > > +	if (mode->clock <= 340000) {
> > > > > > +		/* Few sinks support scrambling at rate < 340 MHz too */
> > > > > > +		if (scrambling->low_rates)
> > > > > > +			hdmi_config =
> > > > > > +				_intel_hdmi_set_source_scrambling(hdmi_config);
> > > > > > +		return hdmi_config;
> > > > > > +	}
> > > > > > +
> > > > > > +	/* Scrambling or not, if clock > 340 MHz, set high char rate */
> > > > > > +	hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> > > > > > +
> > > > > > +	if (scrambling->supported)
> > > > > > +		hdmi_config = _intel_hdmi_set_source_scrambling(hdmi_config);
> > > > > > +
> > > > > > +	return hdmi_config;
> > > > > > +}
> > > > > 
> > > > > Seems overly complicated to me. It could just be something simple like:
> > > > > 
> > > > > void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
> > > > > {
> > > > > 	...
> > > > > 	if (config->hdmi_scrambling)
> > > > > 		temp |= TRANS_DDI_HDMI_SCRAMBLING;
> > > > > 	
> > > > > 	if (config->whatever)
> > > > > 		temp |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> > > > > 	...
> > > > > }
> > > > 
> > > > I know it appears like complicated, but actually the requirement is
> > > > complicated:
> > > > - if clock is > 340 Mhz, set scrambling && set_high_tmds_clock_ratio
> > > > - if clock is < 340 Mhz, but if monitor supports scrambling at lower
> > > > clocks, set scrambling only (not tmds_clock_ratio)
> > > > - to set scrambling, we have to first check if scrambling is supported
> > > > in monitor
> > > > So to meet all these condition, you will end up with something like
> > > > what's implemented above.
> > > > Do you see a  better way ?
> > > 
> > > Put that intel_hdmi_compute_config() so you check it only once, i.e.:
> > > 
> > > if (clock > 340 || scrambling->low_rates)
> > > 	config->hdmi_scrambling = true;
> > > 
> > > if (clock > 340)
> > > 	config->hdmi_high_tmds = true;
> > > 
> > > Then you can do just as Ville said above in intel_ddi_enable_transcoder_func().
> > 
> > Thats a good idea, I can do that, but there is one drawback of doing 
> > that. Right now, all of the HDMI
> > source/sink scrambling code is in one function, so for any 
> > debug/modification activity, you have to look
> > only in one function, also its more readable this way (in the flow)
> > 
> > The moment we distribute code in 3 different function, you have to track 
> > in all the places. Also, we are not
> > saving/reducing any if conditions, we are just moving it at some other 
> > place.
> 
> With the current patch both handle_source_scrambling() and
> handle_sink_scrambling() have checks for clock above/below 340000 and whether
> the sink support scrambling with low rates. Those two checks don't need to be
> duplicated.
> 
> > Do you still think it's worth doing it ?
> 
> Yes, doing it in compute config follows the convention of the driver. If
> something is set in crtc_state, it is only natural to assume it was set at some
> point in the atomic check phase or from reading out the hardware state.
> 
> > > And also for the sink part, no need to check the input values again:
> > > 
> > > if (config->hdmi_high_tmds)
> > > 	if (!drm_scdc_set_high_tmds_clock_ratio(adapter, enable))
> > > 		DRM_ERROR();
> > > 
> > > if (config->hdmi_scrambing)
> > > 	if (!drm_scdc_set_scrambling(adapter, enable))
> > > 		DRM_ERROR();
> > > 
> > > At least that's how I'd interpret what Ville wrote.
> > 
> > Actually design is slightly different. The state's hdmi_scrambling/clock 
> > bool's indicate that the sink's
> > scrambling/high_clock_ratio is enabled/set by source (and needs to be 
> > disabled), whereas connecotr's->display_info->scdc.scrambling
> > is to indicate monitor's capability to support scrambling and scdc
> 
> The crtc_state shouldn't be changed to represent changes in the hardware state.
> It is computed during the atomic check phase, and after that it should represent
> the state that needs to be commited. Perhaps the code in hdmi_compute_config()
> just needs to take the previous state into account?

The previous state is irrelevant. We just need to compute these things
based on what we're going to do next. And this stuff doesn't really
track the state of the sink, rather it tracks the state of the source.
The sink state just follows along to match. So in the readout path we
just want to read out the state of the source.

> 
> > 
> > Think about two situations where:-
> > - Monitor supports scrambling and scdc, but we will not enable it, as 
> > the current mode is 1080P@148 MHz
> > - Monitor supports scrambling and scdc, and we will enable it, as the 
> > current mode is 4k@596 Mhz
> > 
> > To differentiate between these two, we have:
> > config->hdmi_scrambling which shows scrambling enabled on monitor by source
> > display_info->hdmi.scdc->scrambling which indicates monitor supports 
> > scrambling
> > 
> > Does it make sense ? Or you prefer some changes here ?
> 
> Is there any harm in disabling scrambling twice? If not I'd say just disable it
> on every modeset if it is not needed. Then there's no need to know the previous
> state at the moment we actually enable/disable it.

Agreed. Just explicitly set the state to what we want every time.

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

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

[Sharma, Shashank]

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


>> Think about two situations where:-
>> - Monitor supports scrambling and scdc, but we will not enable it, as
>> the current mode is 1080P@148 MHz
>> - Monitor supports scrambling and scdc, and we will enable it, as the
>> current mode is 4k@596 Mhz
>>
>> To differentiate between these two, we have:
>> config->hdmi_scrambling which shows scrambling enabled on monitor by source
>> display_info->hdmi.scdc->scrambling which indicates monitor supports
>> scrambling
>>
>> Does it make sense ? Or you prefer some changes here ?
> Is there any harm in disabling scrambling twice? If not I'd say just disable it
> on every modeset if it is not needed. Then there's no need to know the previous
> state at the moment we actually enable/disable it.

> Agreed. Just explicitly set the state to what we want every time.

- Cool, so I would load the crtc_state only once (in compute_config), and then disable it every time during ddi_disable.
Thanks for this clarification guys, I will send a follow up patch series addressing all the comments soon.

Regards
Shashank

On 2/23/2017 6:51 PM, Ville Syrjälä wrote:
>>> > >Actually design is slightly different. The state's hdmi_scrambling/clock
>>> > >bool's indicate that the sink's
>>> > >scrambling/high_clock_ratio is enabled/set by source (and needs to be
>>> > >disabled), whereas connecotr's->display_info->scdc.scrambling
>>> > >is to indicate monitor's capability to support scrambling and scdc
>> >
>> >The crtc_state shouldn't be changed to represent changes in the hardware state.
>> >It is computed during the atomic check phase, and after that it should represent
>> >the state that needs to be commited. Perhaps the code in hdmi_compute_config()
>> >just needs to take the previous state into account?
> The previous state is irrelevant. We just need to compute these things
> based on what we're going to do next. And this stuff doesn't really
> track the state of the sink, rather it tracks the state of the source.
> The sink state just follows along to match. So in the readout path we
> just want to read out the state of the source.
>
>> >
>>> > >
>>> > >Think about two situations where:-
>>> > >- Monitor supports scrambling and scdc, but we will not enable it, as
>>> > >the current mode is 1080P@148 MHz
>>> > >- Monitor supports scrambling and scdc, and we will enable it, as the
>>> > >current mode is 4k@596 Mhz
>>> > >
>>> > >To differentiate between these two, we have:
>>> > >config->hdmi_scrambling which shows scrambling enabled on monitor by source
>>> > >display_info->hdmi.scdc->scrambling which indicates monitor supports
>>> > >scrambling
>>> > >
>>> > >Does it make sense ? Or you prefer some changes here ?
>> >
>> >Is there any harm in disabling scrambling twice? If not I'd say just disable it
>> >on every modeset if it is not needed. Then there's no need to know the previous
>> >state at the moment we actually enable/disable it.
> Agreed. Just explicitly set the state to what we want every time.


[-- Attachment #1.2: Type: text/html, Size: 5155 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