[PATCH 0/8] Adhering to HDCP1.4 Compliance Test Spec

[PATCH 0/8] Adhering to HDCP1.4 Compliance Test Spec

[Ramalingam C]

This series is developed to address the expectations from HDCP compliance
test specification.

6/8 patches are for fixing failures in one or more compliance test cases
2 patches are Good to have kind. Not related to compliance.

Ramalingam C (8):
  drm/i915: Handle failure from 2nd stage HDCP auth
  drm/i915: Stop encryption for repeater with no sink
  drm/i915: Connector info in HDCP debug msgs
  drm/i915: Retry HDCP BKSV read
  drm/i915: Optimize HDCP key load
  drm/i915: Detect panel's hdcp capability
  drm/i915: Reauthenticate HDCP on failure
  drm/i915: fix misalignment in HDCP register def

 drivers/gpu/drm/i915/i915_reg.h   |  58 ++++++++++-----------
 drivers/gpu/drm/i915/intel_dp.c   |  19 +++++++
 drivers/gpu/drm/i915/intel_drv.h  |   5 ++
 drivers/gpu/drm/i915/intel_hdcp.c | 107 ++++++++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_hdmi.c |  17 ++++++
 5 files changed, 139 insertions(+), 67 deletions(-)

-- 
2.7.4

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

[PATCH 1/8] drm/i915: Handle failure from 2nd stage HDCP auth

[Ramalingam C]

We enable the HDCP encryption as a part of first stage authentication.
So when second stage authentication fails, we need to disable the HDCP
encryption and signalling.

This patch handles above requirement.

For reusing the _intel_hdcp_disable from generic authentication flow,
this patch retain the reference to connector till the generic hdcp flow.
This will be handy to provide the connector details in HDCP enable
debug info.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_hdcp.c | 41 +++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
index b97184eccd9c..0021511ae4d7 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -16,6 +16,8 @@
 
 #define KEY_LOAD_TRIES	5
 
+static int _intel_hdcp_disable(struct intel_connector *connector);
+
 static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port,
 				    const struct intel_hdcp_shim *shim)
 {
@@ -138,17 +140,24 @@ bool intel_hdcp_is_ksv_valid(u8 *ksv)
 	return true;
 }
 
+static
+struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector)
+{
+	return enc_to_dig_port(&connector->encoder->base);
+}
+
 /* Implements Part 2 of the HDCP authorization procedure */
 static
-int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port,
-			       const struct intel_hdcp_shim *shim)
+int intel_hdcp_auth_downstream(struct intel_connector *connector)
 {
 	struct drm_i915_private *dev_priv;
 	u32 vprime, sha_text, sha_leftovers, rep_ctl;
 	u8 bstatus[2], num_downstream, *ksv_fifo;
 	int ret, i, j, sha_idx;
+	const struct intel_hdcp_shim *shim = connector->hdcp_shim;
+	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
 
-	dev_priv = intel_dig_port->base.base.dev->dev_private;
+	dev_priv = to_i915(connector->base.dev);
 
 	ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim);
 	if (ret) {
@@ -385,8 +394,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port,
 }
 
 /* Implements Part 1 of the HDCP authorization procedure */
-static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
-			   const struct intel_hdcp_shim *shim)
+static int intel_hdcp_auth(struct intel_connector *connector)
 {
 	struct drm_i915_private *dev_priv;
 	enum port port;
@@ -405,9 +413,10 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
 		u8 shim[DRM_HDCP_RI_LEN];
 	} ri;
 	bool repeater_present;
+	const struct intel_hdcp_shim *shim = connector->hdcp_shim;
+	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
 
-	dev_priv = intel_dig_port->base.base.dev->dev_private;
-
+	dev_priv = to_i915(connector->base.dev);
 	port = intel_dig_port->base.port;
 
 	/* Initialize An with 2 random values and acquire it */
@@ -498,19 +507,18 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
 	 * on those as well.
 	 */
 
-	if (repeater_present)
-		return intel_hdcp_auth_downstream(intel_dig_port, shim);
+	if (repeater_present) {
+		ret = intel_hdcp_auth_downstream(connector);
+		if (ret) {
+			_intel_hdcp_disable(connector);
+			return ret;
+		}
+	}
 
 	DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n");
 	return 0;
 }
 
-static
-struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector)
-{
-	return enc_to_dig_port(&intel_attached_encoder(&connector->base)->base);
-}
-
 static int _intel_hdcp_disable(struct intel_connector *connector)
 {
 	struct drm_i915_private *dev_priv = connector->base.dev->dev_private;
@@ -558,8 +566,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
 		return ret;
 	}
 
-	ret = intel_hdcp_auth(conn_to_dig_port(connector),
-			      connector->hdcp_shim);
+	ret = intel_hdcp_auth(connector);
 	if (ret) {
 		DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret);
 		return ret;
-- 
2.7.4

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

[PATCH 2/8] drm/i915: Stop encryption for repeater with no sink

[Ramalingam C]

If a HDCP repeater is detected with zero hdcp authenticated
downstream devices, there are two option as below:

1. Dont continue on second stage authentication. Disable encryption.
2. Continue with second stage authentication excluding the KSV list and
   continue encryption on success.

This patch adopts the option 1.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_hdcp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
index 0021511ae4d7..182a3c8a4e4a 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -175,10 +175,10 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector)
 		return -EPERM;
 	}
 
-	/* If there are no downstream devices, we're all done. */
+	/* If there are no downstream devices, we're not encrypting. */
 	num_downstream = DRM_HDCP_NUM_DOWNSTREAM(bstatus[0]);
 	if (num_downstream == 0)
-		return 0;
+		return -EINVAL;
 
 	ksv_fifo = kzalloc(num_downstream * DRM_HDCP_KSV_LEN, GFP_KERNEL);
 	if (!ksv_fifo)
-- 
2.7.4

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

[PATCH 3/8] drm/i915: Connector info in HDCP debug msgs

[Ramalingam C]

When HDCP authentication is triggered on multiple connector, having
connector name and ID in debug message will be more informative.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_hdcp.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
index 182a3c8a4e4a..b3f407cef8b0 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -388,8 +388,10 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector)
 		return -ENXIO;
 	}
 
-	DRM_DEBUG_KMS("HDCP is enabled (%d downstream devices)\n",
-		      num_downstream);
+	DRM_DEBUG_KMS("[%s:%d] HDCP is enabled (%d downstream devices)\n",
+					connector->base.name,
+					connector->base.base.id,
+					num_downstream);
 	return 0;
 }
 
@@ -515,7 +517,9 @@ static int intel_hdcp_auth(struct intel_connector *connector)
 		}
 	}
 
-	DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n");
+	DRM_DEBUG_KMS("[%s:%d] HDCP is enabled (no repeater present)\n",
+					connector->base.name,
+					connector->base.base.id);
 	return 0;
 }
 
@@ -541,7 +545,8 @@ static int _intel_hdcp_disable(struct intel_connector *connector)
 		return ret;
 	}
 
-	DRM_DEBUG_KMS("HDCP is disabled\n");
+	DRM_DEBUG_KMS("[%s:%d] HDCP is disabled\n", connector->base.name,
+						    connector->base.base.id);
 	return 0;
 }
 
@@ -743,7 +748,9 @@ int intel_hdcp_check_link(struct intel_connector *connector)
 		goto out;
 	}
 
-	DRM_DEBUG_KMS("HDCP link failed, retrying authentication\n");
+	DRM_DEBUG_KMS("[%s:%d] HDCP link failed, retrying authentication\n",
+						connector->base.name,
+						connector->base.base.id);
 
 	ret = _intel_hdcp_disable(connector);
 	if (ret) {
-- 
2.7.4

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

[PATCH 4/8] drm/i915: Retry HDCP BKSV read

[Ramalingam C]

When BKSV is invalid, to mitigate any communication errors,
BKSV is read once again.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_hdcp.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
index b3f407cef8b0..fa2e7c727d00 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -401,7 +401,7 @@ static int intel_hdcp_auth(struct intel_connector *connector)
 	struct drm_i915_private *dev_priv;
 	enum port port;
 	unsigned long r0_prime_gen_start;
-	int ret, i;
+	int ret, i, retry = 1;
 	union {
 		u32 reg[2];
 		u8 shim[DRM_HDCP_AN_LEN];
@@ -443,11 +443,16 @@ static int intel_hdcp_auth(struct intel_connector *connector)
 	r0_prime_gen_start = jiffies;
 
 	memset(&bksv, 0, sizeof(bksv));
-	ret = shim->read_bksv(intel_dig_port, bksv.shim);
+
+	do {
+		ret = shim->read_bksv(intel_dig_port, bksv.shim);
+		if (!ret)
+			if (!intel_hdcp_is_ksv_valid(bksv.shim))
+				ret = -ENODEV;
+	} while (ret && retry--);
+
 	if (ret)
 		return ret;
-	else if (!intel_hdcp_is_ksv_valid(bksv.shim))
-		return -ENODEV;
 
 	I915_WRITE(PORT_HDCP_BKSVLO(port), bksv.reg[0]);
 	I915_WRITE(PORT_HDCP_BKSVHI(port), bksv.reg[1]);
-- 
2.7.4

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

[PATCH 5/8] drm/i915: Optimize HDCP key load

[Ramalingam C]

HDCP key need not be cleared on each hdcp disable. And HDCP key Load
is skipped if key is already loaded.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_hdcp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
index fa2e7c727d00..5de9afd275b2 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -51,6 +51,10 @@ static int intel_hdcp_load_keys(struct drm_i915_private *dev_priv)
 	int ret;
 	u32 val;
 
+	val = I915_READ(HDCP_KEY_STATUS);
+	if ((val & HDCP_KEY_LOAD_DONE) && (val & HDCP_KEY_LOAD_STATUS))
+		return 0;
+
 	/*
 	 * On HSW and BDW HW loads the HDCP1.4 Key when Display comes
 	 * out of reset. So if Key is not already loaded, its an error state.
@@ -542,8 +546,6 @@ static int _intel_hdcp_disable(struct intel_connector *connector)
 		return -ETIMEDOUT;
 	}
 
-	intel_hdcp_clear_keys(dev_priv);
-
 	ret = connector->hdcp_shim->toggle_signalling(intel_dig_port, false);
 	if (ret) {
 		DRM_ERROR("Failed to disable HDCP signalling\n");
-- 
2.7.4

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

[PATCH 6/8] drm/i915: Detect panel's hdcp capability

[Ramalingam C]

As a first step of HDCP authentication detects the panel's HDCP
capability. This is mandated for DP HDCP1.4.

For DP 0th Bit of Bcaps register indicates the panel's hdcp capability
For HDMI valid BKSV indicates the panel's hdcp capability.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c   | 19 +++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h  |  5 +++++
 drivers/gpu/drm/i915/intel_hdcp.c | 10 ++++++++--
 drivers/gpu/drm/i915/intel_hdmi.c | 17 +++++++++++++++++
 4 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 03d86ff9b805..2623b2beda1a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5218,6 +5218,24 @@ bool intel_dp_hdcp_check_link(struct intel_digital_port *intel_dig_port)
 	return !(bstatus & (DP_BSTATUS_LINK_FAILURE | DP_BSTATUS_REAUTH_REQ));
 }
 
+static
+int intel_dp_hdcp_capable(struct intel_digital_port *intel_dig_port,
+			  bool *hdcp_capable)
+{
+	ssize_t ret;
+	u8 bcaps;
+
+	ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BCAPS,
+			       &bcaps, 1);
+	if (ret != 1) {
+		DRM_ERROR("Read bcaps from DP/AUX failed (%zd)\n", ret);
+		return ret >= 0 ? -EIO : ret;
+	}
+
+	*hdcp_capable = bcaps & DP_BCAPS_HDCP_CAPABLE;
+	return 0;
+}
+
 static const struct intel_hdcp_shim intel_dp_hdcp_shim = {
 	.write_an_aksv = intel_dp_hdcp_write_an_aksv,
 	.read_bksv = intel_dp_hdcp_read_bksv,
@@ -5229,6 +5247,7 @@ static const struct intel_hdcp_shim intel_dp_hdcp_shim = {
 	.read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
 	.toggle_signalling = intel_dp_hdcp_toggle_signalling,
 	.check_link = intel_dp_hdcp_check_link,
+	.hdcp_capable = intel_dp_hdcp_capable,
 };
 
 static void intel_edp_panel_vdd_sanitize(struct intel_dp *intel_dp)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d6a808374dfb..409aee9010ba 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -369,6 +369,10 @@ struct intel_hdcp_shim {
 
 	/* Ensures the link is still protected */
 	bool (*check_link)(struct intel_digital_port *intel_dig_port);
+
+	/* Detects panel's hdcp capablility */
+	int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
+			    bool *hdcp_capable);
 };
 
 struct intel_connector {
@@ -1848,6 +1852,7 @@ int intel_hdcp_enable(struct intel_connector *connector);
 int intel_hdcp_disable(struct intel_connector *connector);
 int intel_hdcp_check_link(struct intel_connector *connector);
 bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port);
+bool intel_hdcp_is_ksv_valid(u8 *ksv);
 
 /* intel_psr.c */
 #define CAN_PSR(dev_priv) (HAS_PSR(dev_priv) && dev_priv->psr.sink_support)
diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
index 5de9afd275b2..a3463557f0f6 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -132,7 +132,6 @@ u32 intel_hdcp_get_repeater_ctl(struct intel_digital_port *intel_dig_port)
 	return -EINVAL;
 }
 
-static
 bool intel_hdcp_is_ksv_valid(u8 *ksv)
 {
 	int i, ones = 0;
@@ -418,13 +417,20 @@ static int intel_hdcp_auth(struct intel_connector *connector)
 		u32 reg;
 		u8 shim[DRM_HDCP_RI_LEN];
 	} ri;
-	bool repeater_present;
+	bool repeater_present, hdcp_capable;
 	const struct intel_hdcp_shim *shim = connector->hdcp_shim;
 	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
 
 	dev_priv = to_i915(connector->base.dev);
 	port = intel_dig_port->base.port;
 
+	/* Detect whether panel is HDCP capable or not */
+	ret = shim->hdcp_capable(intel_dig_port, &hdcp_capable);
+	if (ret)
+		return ret;
+	if (!hdcp_capable)
+		return -EINVAL;
+
 	/* Initialize An with 2 random values and acquire it */
 	for (i = 0; i < 2; i++)
 		I915_WRITE(PORT_HDCP_ANINIT(port), get_random_u32());
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index f5d7bfb43006..dfca361ebb24 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1106,6 +1106,22 @@ bool intel_hdmi_hdcp_check_link(struct intel_digital_port *intel_dig_port)
 	return true;
 }
 
+static
+int intel_hdmi_hdcp_capable(struct intel_digital_port *intel_dig_port,
+			    bool *hdcp_capable)
+{
+	u8 bksv[5];
+	int ret, retry = 1;
+
+	do {
+		ret = intel_hdmi_hdcp_read_bksv(intel_dig_port, bksv);
+		if (!ret)
+			*hdcp_capable = intel_hdcp_is_ksv_valid(bksv);
+	} while (!(*hdcp_capable) && retry--);
+
+	return ret;
+}
+
 static const struct intel_hdcp_shim intel_hdmi_hdcp_shim = {
 	.write_an_aksv = intel_hdmi_hdcp_write_an_aksv,
 	.read_bksv = intel_hdmi_hdcp_read_bksv,
@@ -1117,6 +1133,7 @@ static const struct intel_hdcp_shim intel_hdmi_hdcp_shim = {
 	.read_v_prime_part = intel_hdmi_hdcp_read_v_prime_part,
 	.toggle_signalling = intel_hdmi_hdcp_toggle_signalling,
 	.check_link = intel_hdmi_hdcp_check_link,
+	.hdcp_capable = intel_hdmi_hdcp_capable,
 };
 
 static void intel_hdmi_prepare(struct intel_encoder *encoder,
-- 
2.7.4

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

[PATCH 7/8] drm/i915: Reauthenticate HDCP on failure

[Ramalingam C]

When HDCP authentication fails, we add two more reauthentication.
This will address all reauth expectation from compliance perspective.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_hdcp.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
index a3463557f0f6..3caa548a9cad 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -566,7 +566,7 @@ static int _intel_hdcp_disable(struct intel_connector *connector)
 static int _intel_hdcp_enable(struct intel_connector *connector)
 {
 	struct drm_i915_private *dev_priv = connector->base.dev->dev_private;
-	int i, ret;
+	int i, ret, reauth = 1;
 
 	if (!(I915_READ(SKL_FUSE_STATUS) & SKL_FUSE_PG_DIST_STATUS(1))) {
 		DRM_ERROR("PG1 is disabled, cannot load keys\n");
@@ -584,13 +584,17 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
 		return ret;
 	}
 
-	ret = intel_hdcp_auth(connector);
-	if (ret) {
-		DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret);
-		return ret;
-	}
+	do {
+		ret = intel_hdcp_auth(connector);
+		if (ret)
+			DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret);
+		if (ret && reauth)
+			DRM_DEBUG_KMS("[%s:%d] HDCP Reauth... %d/1\n",
+					connector->base.name,
+					connector->base.base.id, reauth);
+	} while (ret && reauth--);
 
-	return 0;
+	return ret;
 }
 
 static void intel_hdcp_check_work(struct work_struct *work)
-- 
2.7.4

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

[PATCH 8/8] drm/i915: fix misalignment in HDCP register def

[Ramalingam C]

This patch aligns all definitions of hdcp registers and their bits.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 58 ++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 65ba10ad1fe5..e9c79b560823 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8483,34 +8483,34 @@ enum skl_power_gate {
 #define   CNL_AUX_ANAOVRD1_LDO_BYPASS	(1<<23)
 
 /* HDCP Key Registers */
-#define HDCP_KEY_CONF		_MMIO(0x66c00)
+#define HDCP_KEY_CONF			_MMIO(0x66c00)
 #define  HDCP_AKSV_SEND_TRIGGER		BIT(31)
 #define  HDCP_CLEAR_KEYS_TRIGGER	BIT(30)
 #define  HDCP_KEY_LOAD_TRIGGER		BIT(8)
-#define HDCP_KEY_STATUS		_MMIO(0x66c04)
-#define  HDCP_FUSE_IN_PROGRESS	BIT(7)
+#define HDCP_KEY_STATUS			_MMIO(0x66c04)
+#define  HDCP_FUSE_IN_PROGRESS		BIT(7)
 #define  HDCP_FUSE_ERROR		BIT(6)
-#define  HDCP_FUSE_DONE		BIT(5)
-#define  HDCP_KEY_LOAD_STATUS	BIT(1)
+#define  HDCP_FUSE_DONE			BIT(5)
+#define  HDCP_KEY_LOAD_STATUS		BIT(1)
 #define  HDCP_KEY_LOAD_DONE		BIT(0)
-#define HDCP_AKSV_LO		_MMIO(0x66c10)
-#define HDCP_AKSV_HI		_MMIO(0x66c14)
+#define HDCP_AKSV_LO			_MMIO(0x66c10)
+#define HDCP_AKSV_HI			_MMIO(0x66c14)
 
 /* HDCP Repeater Registers */
-#define HDCP_REP_CTL		_MMIO(0x66d00)
-#define  HDCP_DDIB_REP_PRESENT	BIT(30)
-#define  HDCP_DDIA_REP_PRESENT	BIT(29)
-#define  HDCP_DDIC_REP_PRESENT	BIT(28)
-#define  HDCP_DDID_REP_PRESENT	BIT(27)
-#define  HDCP_DDIF_REP_PRESENT	BIT(26)
-#define  HDCP_DDIE_REP_PRESENT	BIT(25)
+#define HDCP_REP_CTL			_MMIO(0x66d00)
+#define  HDCP_DDIB_REP_PRESENT		BIT(30)
+#define  HDCP_DDIA_REP_PRESENT		BIT(29)
+#define  HDCP_DDIC_REP_PRESENT		BIT(28)
+#define  HDCP_DDID_REP_PRESENT		BIT(27)
+#define  HDCP_DDIF_REP_PRESENT		BIT(26)
+#define  HDCP_DDIE_REP_PRESENT		BIT(25)
 #define  HDCP_DDIB_SHA1_M0		(1 << 20)
 #define  HDCP_DDIA_SHA1_M0		(2 << 20)
 #define  HDCP_DDIC_SHA1_M0		(3 << 20)
 #define  HDCP_DDID_SHA1_M0		(4 << 20)
 #define  HDCP_DDIF_SHA1_M0		(5 << 20)
 #define  HDCP_DDIE_SHA1_M0		(6 << 20) /* Bspec says 5? */
-#define  HDCP_SHA1_BUSY		BIT(16)
+#define  HDCP_SHA1_BUSY			BIT(16)
 #define  HDCP_SHA1_READY		BIT(17)
 #define  HDCP_SHA1_COMPLETE		BIT(18)
 #define  HDCP_SHA1_V_MATCH		BIT(19)
@@ -8526,7 +8526,7 @@ enum skl_power_gate {
 #define HDCP_SHA_V_PRIME_H3		_MMIO(0x66d10)
 #define HDCP_SHA_V_PRIME_H4		_MMIO(0x66d14)
 #define HDCP_SHA_V_PRIME(h)		_MMIO((0x66d04 + h * 4))
-#define HDCP_SHA_TEXT		_MMIO(0x66d18)
+#define HDCP_SHA_TEXT			_MMIO(0x66d18)
 
 /* HDCP Auth Registers */
 #define _PORTA_HDCP_AUTHENC		0x66800
@@ -8542,25 +8542,25 @@ enum skl_power_gate {
 					  _PORTD_HDCP_AUTHENC, \
 					  _PORTE_HDCP_AUTHENC, \
 					  _PORTF_HDCP_AUTHENC) + x)
-#define PORT_HDCP_CONF(port)	_PORT_HDCP_AUTHENC(port, 0x0)
-#define  HDCP_CONF_CAPTURE_AN	BIT(0)
-#define  HDCP_CONF_AUTH_AND_ENC	(BIT(1) | BIT(0))
-#define PORT_HDCP_ANINIT(port)	_PORT_HDCP_AUTHENC(port, 0x4)
-#define PORT_HDCP_ANLO(port)	_PORT_HDCP_AUTHENC(port, 0x8)
-#define PORT_HDCP_ANHI(port)	_PORT_HDCP_AUTHENC(port, 0xC)
-#define PORT_HDCP_BKSVLO(port)	_PORT_HDCP_AUTHENC(port, 0x10)
-#define PORT_HDCP_BKSVHI(port)	_PORT_HDCP_AUTHENC(port, 0x14)
-#define PORT_HDCP_RPRIME(port)	_PORT_HDCP_AUTHENC(port, 0x18)
-#define PORT_HDCP_STATUS(port)	_PORT_HDCP_AUTHENC(port, 0x1C)
+#define PORT_HDCP_CONF(port)		_PORT_HDCP_AUTHENC(port, 0x0)
+#define  HDCP_CONF_CAPTURE_AN		BIT(0)
+#define  HDCP_CONF_AUTH_AND_ENC		(BIT(1) | BIT(0))
+#define PORT_HDCP_ANINIT(port)		_PORT_HDCP_AUTHENC(port, 0x4)
+#define PORT_HDCP_ANLO(port)		_PORT_HDCP_AUTHENC(port, 0x8)
+#define PORT_HDCP_ANHI(port)		_PORT_HDCP_AUTHENC(port, 0xC)
+#define PORT_HDCP_BKSVLO(port)		_PORT_HDCP_AUTHENC(port, 0x10)
+#define PORT_HDCP_BKSVHI(port)		_PORT_HDCP_AUTHENC(port, 0x14)
+#define PORT_HDCP_RPRIME(port)		_PORT_HDCP_AUTHENC(port, 0x18)
+#define PORT_HDCP_STATUS(port)		_PORT_HDCP_AUTHENC(port, 0x1C)
 #define  HDCP_STATUS_STREAM_A_ENC	BIT(31)
 #define  HDCP_STATUS_STREAM_B_ENC	BIT(30)
 #define  HDCP_STATUS_STREAM_C_ENC	BIT(29)
 #define  HDCP_STATUS_STREAM_D_ENC	BIT(28)
 #define  HDCP_STATUS_AUTH		BIT(21)
 #define  HDCP_STATUS_ENC		BIT(20)
-#define  HDCP_STATUS_RI_MATCH	BIT(19)
-#define  HDCP_STATUS_R0_READY	BIT(18)
-#define  HDCP_STATUS_AN_READY	BIT(17)
+#define  HDCP_STATUS_RI_MATCH		BIT(19)
+#define  HDCP_STATUS_R0_READY		BIT(18)
+#define  HDCP_STATUS_AN_READY		BIT(17)
 #define  HDCP_STATUS_CIPHER		BIT(16)
 #define  HDCP_STATUS_FRAME_CNT(x)	((x >> 8) & 0xff)
 
-- 
2.7.4

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

✓ Fi.CI.BAT: success for Adhering to HDCP1.4 Compliance Test Spec

[Patchwork]

== Series Details ==

Series: Adhering to HDCP1.4 Compliance Test Spec
URL   : https://patchwork.freedesktop.org/series/37539/
State : success

== Summary ==

Series 37539v1 Adhering to HDCP1.4 Compliance Test Spec
https://patchwork.freedesktop.org/api/1.0/series/37539/revisions/1/mbox/

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:415s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:422s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:377s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:483s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:280s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:482s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:466s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:455s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:570s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:413s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:282s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:511s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:390s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:396s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:408s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:457s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:411s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:458s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:496s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:452s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:505s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:569s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:428s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:512s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:526s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:482s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:483s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:415s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:429s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:516s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:393s
Blacklisted hosts:
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:466s

e1b21c16bfafa4fb3b3b656fcd44c901e558a111 drm-tip: 2018y-02m-02d-08h-21m-34s UTC integration manifest
98b23eec1256 drm/i915: fix misalignment in HDCP register def
8e543839ca1d drm/i915: Reauthenticate HDCP on failure
404e7c851b9e drm/i915: Detect panel's hdcp capability
b76e03dfce71 drm/i915: Optimize HDCP key load
d5fe0964ae3f drm/i915: Retry HDCP BKSV read
411b7773586f drm/i915: Connector info in HDCP debug msgs
12cd72347b0b drm/i915: Stop encryption for repeater with no sink
fb0df95f6f02 drm/i915: Handle failure from 2nd stage HDCP auth

== Logs ==

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

✗ Fi.CI.IGT: failure for Adhering to HDCP1.4 Compliance Test Spec

[Patchwork]

== Series Details ==

Series: Adhering to HDCP1.4 Compliance Test Spec
URL   : https://patchwork.freedesktop.org/series/37539/
State : failure

== Summary ==

Test perf:
        Subgroup blocking:
                fail       -> PASS       (shard-hsw) fdo#102252
        Subgroup oa-exponents:
                fail       -> PASS       (shard-apl) fdo#102254
Test kms_vblank:
        Subgroup pipe-a-ts-continuation-suspend:
                fail       -> PASS       (shard-hsw) fdo#104783
Test kms_sysfs_edid_timing:
                pass       -> WARN       (shard-apl) fdo#100047
Test kms_flip:
        Subgroup 2x-plain-flip-fb-recreate-interruptible:
                pass       -> FAIL       (shard-hsw)
Test kms_atomic_transition:
        Subgroup plane-all-transition:
                skip       -> PASS       (shard-snb)

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254
fdo#104783 https://bugs.freedesktop.org/show_bug.cgi?id=104783
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047

shard-apl        total:2836 pass:1749 dwarn:1   dfail:0   fail:21  skip:1064 time:12433s
shard-hsw        total:2836 pass:1732 dwarn:1   dfail:0   fail:12  skip:1090 time:11569s
shard-snb        total:2836 pass:1328 dwarn:1   dfail:0   fail:10  skip:1497 time:6407s
Blacklisted hosts:
shard-kbl        total:2836 pass:1869 dwarn:3   dfail:0   fail:23  skip:941 time:9451s

== Logs ==

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

Re: [PATCH 1/8] drm/i915: Handle failure from 2nd stage HDCP auth

[Sean Paul]

On Fri, Feb 02, 2018 at 04:15:13PM +0530, Ramalingam C wrote:
> We enable the HDCP encryption as a part of first stage authentication.
> So when second stage authentication fails, we need to disable the HDCP
> encryption and signalling.
> 
> This patch handles above requirement.
> 
> For reusing the _intel_hdcp_disable from generic authentication flow,
> this patch retain the reference to connector till the generic hdcp flow.
> This will be handy to provide the connector details in HDCP enable
> debug info.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdcp.c | 41 +++++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index b97184eccd9c..0021511ae4d7 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -16,6 +16,8 @@
>  
>  #define KEY_LOAD_TRIES	5
>  
> +static int _intel_hdcp_disable(struct intel_connector *connector);
> +
>  static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port,
>  				    const struct intel_hdcp_shim *shim)
>  {
> @@ -138,17 +140,24 @@ bool intel_hdcp_is_ksv_valid(u8 *ksv)
>  	return true;
>  }
>  
> +static
> +struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector)
> +{
> +	return enc_to_dig_port(&connector->encoder->base);
> +}
> +
>  /* Implements Part 2 of the HDCP authorization procedure */
>  static
> -int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port,
> -			       const struct intel_hdcp_shim *shim)
> +int intel_hdcp_auth_downstream(struct intel_connector *connector)
>  {
>  	struct drm_i915_private *dev_priv;
>  	u32 vprime, sha_text, sha_leftovers, rep_ctl;
>  	u8 bstatus[2], num_downstream, *ksv_fifo;
>  	int ret, i, j, sha_idx;
> +	const struct intel_hdcp_shim *shim = connector->hdcp_shim;
> +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
>  
> -	dev_priv = intel_dig_port->base.base.dev->dev_private;
> +	dev_priv = to_i915(connector->base.dev);
>  
>  	ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim);
>  	if (ret) {
> @@ -385,8 +394,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port,
>  }
>  
>  /* Implements Part 1 of the HDCP authorization procedure */
> -static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
> -			   const struct intel_hdcp_shim *shim)
> +static int intel_hdcp_auth(struct intel_connector *connector)
>  {
>  	struct drm_i915_private *dev_priv;
>  	enum port port;
> @@ -405,9 +413,10 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
>  		u8 shim[DRM_HDCP_RI_LEN];
>  	} ri;
>  	bool repeater_present;
> +	const struct intel_hdcp_shim *shim = connector->hdcp_shim;
> +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
>  
> -	dev_priv = intel_dig_port->base.base.dev->dev_private;
> -
> +	dev_priv = to_i915(connector->base.dev);
>  	port = intel_dig_port->base.port;
>  
>  	/* Initialize An with 2 random values and acquire it */
> @@ -498,19 +507,18 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
>  	 * on those as well.
>  	 */
>  
> -	if (repeater_present)
> -		return intel_hdcp_auth_downstream(intel_dig_port, shim);
> +	if (repeater_present) {
> +		ret = intel_hdcp_auth_downstream(connector);
> +		if (ret) {
> +			_intel_hdcp_disable(connector);

If you just call this from _intel_hdcp_enable when intel_hdcp_auth() fails, you
can avoid all of this code, it'd just be one line.


> +			return ret;
> +		}
> +	}
>  
>  	DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n");
>  	return 0;
>  }
>  
> -static
> -struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector)
> -{
> -	return enc_to_dig_port(&intel_attached_encoder(&connector->base)->base);
> -}
> -
>  static int _intel_hdcp_disable(struct intel_connector *connector)
>  {
>  	struct drm_i915_private *dev_priv = connector->base.dev->dev_private;
> @@ -558,8 +566,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
>  		return ret;
>  	}
>  
> -	ret = intel_hdcp_auth(conn_to_dig_port(connector),
> -			      connector->hdcp_shim);
> +	ret = intel_hdcp_auth(connector);
>  	if (ret) {
>  		DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret);
>  		return ret;
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/8] drm/i915: Stop encryption for repeater with no sink

[Ramalingam C]

On Friday 02 February 2018 07:43 PM, Sean Paul wrote:
> On Fri, Feb 02, 2018 at 04:15:14PM +0530, Ramalingam C wrote:
>> If a HDCP repeater is detected with zero hdcp authenticated
>> downstream devices, there are two option as below:
>>
>> 1. Dont continue on second stage authentication. Disable encryption.
>> 2. Continue with second stage authentication excluding the KSV list and
>>     continue encryption on success.
>>
>> This patch adopts the option 1.
> It doesn't seem that hard to adopt option 2 and avoid failure. That would result
> in a better experience.
True. Not too much effort for option 2. But I am not seeing any ROI out 
of it at this point.
what is the benefit of encrypting if repeater cant use it, as it has 0 
sinks attached to it.
Still do we want option two?

-Ram
>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_hdcp.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>> index 0021511ae4d7..182a3c8a4e4a 100644
>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>> @@ -175,10 +175,10 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector)
>>   		return -EPERM;
>>   	}
>>   
>> -	/* If there are no downstream devices, we're all done. */
>> +	/* If there are no downstream devices, we're not encrypting. */
>>   	num_downstream = DRM_HDCP_NUM_DOWNSTREAM(bstatus[0]);
>>   	if (num_downstream == 0)
>> -		return 0;
>> +		return -EINVAL;
>>   
>>   	ksv_fifo = kzalloc(num_downstream * DRM_HDCP_KSV_LEN, GFP_KERNEL);
>>   	if (!ksv_fifo)
>> -- 
>> 2.7.4
>>

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

Re: [PATCH 2/8] drm/i915: Stop encryption for repeater with no sink

[Sean Paul]

On Fri, Feb 02, 2018 at 04:15:14PM +0530, Ramalingam C wrote:
> If a HDCP repeater is detected with zero hdcp authenticated
> downstream devices, there are two option as below:
> 
> 1. Dont continue on second stage authentication. Disable encryption.
> 2. Continue with second stage authentication excluding the KSV list and
>    continue encryption on success.
> 
> This patch adopts the option 1.

It doesn't seem that hard to adopt option 2 and avoid failure. That would result
in a better experience.


> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdcp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index 0021511ae4d7..182a3c8a4e4a 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -175,10 +175,10 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector)
>  		return -EPERM;
>  	}
>  
> -	/* If there are no downstream devices, we're all done. */
> +	/* If there are no downstream devices, we're not encrypting. */
>  	num_downstream = DRM_HDCP_NUM_DOWNSTREAM(bstatus[0]);
>  	if (num_downstream == 0)
> -		return 0;
> +		return -EINVAL;
>  
>  	ksv_fifo = kzalloc(num_downstream * DRM_HDCP_KSV_LEN, GFP_KERNEL);
>  	if (!ksv_fifo)
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/8] drm/i915: Connector info in HDCP debug msgs

[Sean Paul]

On Fri, Feb 02, 2018 at 04:15:15PM +0530, Ramalingam C wrote:
> When HDCP authentication is triggered on multiple connector, having
> connector name and ID in debug message will be more informative.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdcp.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index 182a3c8a4e4a..b3f407cef8b0 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -388,8 +388,10 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector)
>  		return -ENXIO;
>  	}
>  
> -	DRM_DEBUG_KMS("HDCP is enabled (%d downstream devices)\n",
> -		      num_downstream);
> +	DRM_DEBUG_KMS("[%s:%d] HDCP is enabled (%d downstream devices)\n",
> +					connector->base.name,
> +					connector->base.base.id,
> +					num_downstream);

Your alignment is off, should be:

        DRM_DEBUG_KMS("[%s:%d] HDCP is enabled (%d downstream devices)\n",
                      connector->base.name, connector->base.base.id,
                      num_downstream);

Same for the rest of the patch. With that fixed,

Reviewed-by: Sean Paul <seanpaul@chromium.org>


>  	return 0;
>  }
>  
> @@ -515,7 +517,9 @@ static int intel_hdcp_auth(struct intel_connector *connector)
>  		}
>  	}
>  
> -	DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n");
> +	DRM_DEBUG_KMS("[%s:%d] HDCP is enabled (no repeater present)\n",
> +					connector->base.name,
> +					connector->base.base.id);
>  	return 0;
>  }
>  
> @@ -541,7 +545,8 @@ static int _intel_hdcp_disable(struct intel_connector *connector)
>  		return ret;
>  	}
>  
> -	DRM_DEBUG_KMS("HDCP is disabled\n");
> +	DRM_DEBUG_KMS("[%s:%d] HDCP is disabled\n", connector->base.name,
> +						    connector->base.base.id);
>  	return 0;
>  }
>  
> @@ -743,7 +748,9 @@ int intel_hdcp_check_link(struct intel_connector *connector)
>  		goto out;
>  	}
>  
> -	DRM_DEBUG_KMS("HDCP link failed, retrying authentication\n");
> +	DRM_DEBUG_KMS("[%s:%d] HDCP link failed, retrying authentication\n",
> +						connector->base.name,
> +						connector->base.base.id);
>  
>  	ret = _intel_hdcp_disable(connector);
>  	if (ret) {
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/8] drm/i915: Retry HDCP BKSV read

[Sean Paul]

On Fri, Feb 02, 2018 at 04:15:16PM +0530, Ramalingam C wrote:
> When BKSV is invalid, to mitigate any communication errors,
> BKSV is read once again.

Why is the Bksv read any more volatile than other reads? If the channel is
noisy, the retries should be done in the shim implementation (I think there are
already i2c retries).

Sean

> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdcp.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index b3f407cef8b0..fa2e7c727d00 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -401,7 +401,7 @@ static int intel_hdcp_auth(struct intel_connector *connector)
>  	struct drm_i915_private *dev_priv;
>  	enum port port;
>  	unsigned long r0_prime_gen_start;
> -	int ret, i;
> +	int ret, i, retry = 1;
>  	union {
>  		u32 reg[2];
>  		u8 shim[DRM_HDCP_AN_LEN];
> @@ -443,11 +443,16 @@ static int intel_hdcp_auth(struct intel_connector *connector)
>  	r0_prime_gen_start = jiffies;
>  
>  	memset(&bksv, 0, sizeof(bksv));
> -	ret = shim->read_bksv(intel_dig_port, bksv.shim);
> +
> +	do {
> +		ret = shim->read_bksv(intel_dig_port, bksv.shim);
> +		if (!ret)
> +			if (!intel_hdcp_is_ksv_valid(bksv.shim))
> +				ret = -ENODEV;
> +	} while (ret && retry--);
> +
>  	if (ret)
>  		return ret;
> -	else if (!intel_hdcp_is_ksv_valid(bksv.shim))
> -		return -ENODEV;
>  
>  	I915_WRITE(PORT_HDCP_BKSVLO(port), bksv.reg[0]);
>  	I915_WRITE(PORT_HDCP_BKSVHI(port), bksv.reg[1]);
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/8] drm/i915: Optimize HDCP key load

[Sean Paul]

On Fri, Feb 02, 2018 at 04:15:17PM +0530, Ramalingam C wrote:
> HDCP key need not be cleared on each hdcp disable. And HDCP key Load
> is skipped if key is already loaded.
> 

I had previously encountered issues without clearing the key in my testing.
IIRC, without clearing the keys things acted differently. How much time are we
saving by optimizing this?

Sean


> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdcp.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index fa2e7c727d00..5de9afd275b2 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -51,6 +51,10 @@ static int intel_hdcp_load_keys(struct drm_i915_private *dev_priv)
>  	int ret;
>  	u32 val;
>  
> +	val = I915_READ(HDCP_KEY_STATUS);
> +	if ((val & HDCP_KEY_LOAD_DONE) && (val & HDCP_KEY_LOAD_STATUS))
> +		return 0;
> +
>  	/*
>  	 * On HSW and BDW HW loads the HDCP1.4 Key when Display comes
>  	 * out of reset. So if Key is not already loaded, its an error state.
> @@ -542,8 +546,6 @@ static int _intel_hdcp_disable(struct intel_connector *connector)
>  		return -ETIMEDOUT;
>  	}
>  
> -	intel_hdcp_clear_keys(dev_priv);
> -
>  	ret = connector->hdcp_shim->toggle_signalling(intel_dig_port, false);
>  	if (ret) {
>  		DRM_ERROR("Failed to disable HDCP signalling\n");
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915: Handle failure from 2nd stage HDCP auth

[Ramalingam C]

On Friday 02 February 2018 07:39 PM, Sean Paul wrote:
> On Fri, Feb 02, 2018 at 04:15:13PM +0530, Ramalingam C wrote:
>> We enable the HDCP encryption as a part of first stage authentication.
>> So when second stage authentication fails, we need to disable the HDCP
>> encryption and signalling.
>>
>> This patch handles above requirement.
>>
>> For reusing the _intel_hdcp_disable from generic authentication flow,
>> this patch retain the reference to connector till the generic hdcp flow.
>> This will be handy to provide the connector details in HDCP enable
>> debug info.
>>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_hdcp.c | 41 +++++++++++++++++++++++----------------
>>   1 file changed, 24 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>> index b97184eccd9c..0021511ae4d7 100644
>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>> @@ -16,6 +16,8 @@
>>   
>>   #define KEY_LOAD_TRIES	5
>>   
>> +static int _intel_hdcp_disable(struct intel_connector *connector);
>> +
>>   static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port,
>>   				    const struct intel_hdcp_shim *shim)
>>   {
>> @@ -138,17 +140,24 @@ bool intel_hdcp_is_ksv_valid(u8 *ksv)
>>   	return true;
>>   }
>>   
>> +static
>> +struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector)
>> +{
>> +	return enc_to_dig_port(&connector->encoder->base);
>> +}
>> +
>>   /* Implements Part 2 of the HDCP authorization procedure */
>>   static
>> -int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port,
>> -			       const struct intel_hdcp_shim *shim)
>> +int intel_hdcp_auth_downstream(struct intel_connector *connector)
>>   {
>>   	struct drm_i915_private *dev_priv;
>>   	u32 vprime, sha_text, sha_leftovers, rep_ctl;
>>   	u8 bstatus[2], num_downstream, *ksv_fifo;
>>   	int ret, i, j, sha_idx;
>> +	const struct intel_hdcp_shim *shim = connector->hdcp_shim;
>> +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
>>   
>> -	dev_priv = intel_dig_port->base.base.dev->dev_private;
>> +	dev_priv = to_i915(connector->base.dev);
>>   
>>   	ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim);
>>   	if (ret) {
>> @@ -385,8 +394,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port,
>>   }
>>   
>>   /* Implements Part 1 of the HDCP authorization procedure */
>> -static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
>> -			   const struct intel_hdcp_shim *shim)
>> +static int intel_hdcp_auth(struct intel_connector *connector)
>>   {
>>   	struct drm_i915_private *dev_priv;
>>   	enum port port;
>> @@ -405,9 +413,10 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
>>   		u8 shim[DRM_HDCP_RI_LEN];
>>   	} ri;
>>   	bool repeater_present;
>> +	const struct intel_hdcp_shim *shim = connector->hdcp_shim;
>> +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
>>   
>> -	dev_priv = intel_dig_port->base.base.dev->dev_private;
>> -
>> +	dev_priv = to_i915(connector->base.dev);
>>   	port = intel_dig_port->base.port;
>>   
>>   	/* Initialize An with 2 random values and acquire it */
>> @@ -498,19 +507,18 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
>>   	 * on those as well.
>>   	 */
>>   
>> -	if (repeater_present)
>> -		return intel_hdcp_auth_downstream(intel_dig_port, shim);
>> +	if (repeater_present) {
>> +		ret = intel_hdcp_auth_downstream(connector);
>> +		if (ret) {
>> +			_intel_hdcp_disable(connector);
> If you just call this from _intel_hdcp_enable when intel_hdcp_auth() fails, you
> can avoid all of this code, it'd just be one line.
Yes. Thought about that option too. Actually I would prefer having the 
reference of intel_connector untill generic hdcp auth implementation.
we can pass the intel digital port for encoder specific shims.

This will help me with
     providing the connector details for state transitions as in next 
patches
     next steps like SRM passing in for revocation and downstream 
topology gathering (perhaps will start with availability of 
complementing user space)

At this point if we dont want this code, I will disable hdcp from enable 
functions.

-Ram
>
>
>> +			return ret;
>> +		}
>> +	}
>>   
>>   	DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n");
>>   	return 0;
>>   }
>>   
>> -static
>> -struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector)
>> -{
>> -	return enc_to_dig_port(&intel_attached_encoder(&connector->base)->base);
>> -}
>> -
>>   static int _intel_hdcp_disable(struct intel_connector *connector)
>>   {
>>   	struct drm_i915_private *dev_priv = connector->base.dev->dev_private;
>> @@ -558,8 +566,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
>>   		return ret;
>>   	}
>>   
>> -	ret = intel_hdcp_auth(conn_to_dig_port(connector),
>> -			      connector->hdcp_shim);
>> +	ret = intel_hdcp_auth(connector);
>>   	if (ret) {
>>   		DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret);
>>   		return ret;
>> -- 
>> 2.7.4
>>

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

Re: [PATCH 6/8] drm/i915: Detect panel's hdcp capability

[Sean Paul]

On Fri, Feb 02, 2018 at 04:15:18PM +0530, Ramalingam C wrote:
> As a first step of HDCP authentication detects the panel's HDCP
> capability. This is mandated for DP HDCP1.4.
> 
> For DP 0th Bit of Bcaps register indicates the panel's hdcp capability
> For HDMI valid BKSV indicates the panel's hdcp capability.

We're already checking valid bksv, so there's no need to expose that function and
call it twice for the HDMI case, hdcp_capable should just always be true.

For DP, we read and check the bksv , so what do non-capable displays return for
the bksv?

Sean

> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c   | 19 +++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h  |  5 +++++
>  drivers/gpu/drm/i915/intel_hdcp.c | 10 ++++++++--
>  drivers/gpu/drm/i915/intel_hdmi.c | 17 +++++++++++++++++
>  4 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 03d86ff9b805..2623b2beda1a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5218,6 +5218,24 @@ bool intel_dp_hdcp_check_link(struct intel_digital_port *intel_dig_port)
>  	return !(bstatus & (DP_BSTATUS_LINK_FAILURE | DP_BSTATUS_REAUTH_REQ));
>  }
>  
> +static
> +int intel_dp_hdcp_capable(struct intel_digital_port *intel_dig_port,
> +			  bool *hdcp_capable)
> +{
> +	ssize_t ret;
> +	u8 bcaps;
> +
> +	ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BCAPS,
> +			       &bcaps, 1);
> +	if (ret != 1) {
> +		DRM_ERROR("Read bcaps from DP/AUX failed (%zd)\n", ret);
> +		return ret >= 0 ? -EIO : ret;
> +	}
> +
> +	*hdcp_capable = bcaps & DP_BCAPS_HDCP_CAPABLE;
> +	return 0;
> +}
> +
>  static const struct intel_hdcp_shim intel_dp_hdcp_shim = {
>  	.write_an_aksv = intel_dp_hdcp_write_an_aksv,
>  	.read_bksv = intel_dp_hdcp_read_bksv,
> @@ -5229,6 +5247,7 @@ static const struct intel_hdcp_shim intel_dp_hdcp_shim = {
>  	.read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
>  	.toggle_signalling = intel_dp_hdcp_toggle_signalling,
>  	.check_link = intel_dp_hdcp_check_link,
> +	.hdcp_capable = intel_dp_hdcp_capable,
>  };
>  
>  static void intel_edp_panel_vdd_sanitize(struct intel_dp *intel_dp)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d6a808374dfb..409aee9010ba 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -369,6 +369,10 @@ struct intel_hdcp_shim {
>  
>  	/* Ensures the link is still protected */
>  	bool (*check_link)(struct intel_digital_port *intel_dig_port);
> +
> +	/* Detects panel's hdcp capablility */
> +	int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
> +			    bool *hdcp_capable);
>  };
>  
>  struct intel_connector {
> @@ -1848,6 +1852,7 @@ int intel_hdcp_enable(struct intel_connector *connector);
>  int intel_hdcp_disable(struct intel_connector *connector);
>  int intel_hdcp_check_link(struct intel_connector *connector);
>  bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port);
> +bool intel_hdcp_is_ksv_valid(u8 *ksv);
>  
>  /* intel_psr.c */
>  #define CAN_PSR(dev_priv) (HAS_PSR(dev_priv) && dev_priv->psr.sink_support)
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index 5de9afd275b2..a3463557f0f6 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -132,7 +132,6 @@ u32 intel_hdcp_get_repeater_ctl(struct intel_digital_port *intel_dig_port)
>  	return -EINVAL;
>  }
>  
> -static
>  bool intel_hdcp_is_ksv_valid(u8 *ksv)
>  {
>  	int i, ones = 0;
> @@ -418,13 +417,20 @@ static int intel_hdcp_auth(struct intel_connector *connector)
>  		u32 reg;
>  		u8 shim[DRM_HDCP_RI_LEN];
>  	} ri;
> -	bool repeater_present;
> +	bool repeater_present, hdcp_capable;
>  	const struct intel_hdcp_shim *shim = connector->hdcp_shim;
>  	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
>  
>  	dev_priv = to_i915(connector->base.dev);
>  	port = intel_dig_port->base.port;
>  
> +	/* Detect whether panel is HDCP capable or not */
> +	ret = shim->hdcp_capable(intel_dig_port, &hdcp_capable);
> +	if (ret)
> +		return ret;
> +	if (!hdcp_capable)
> +		return -EINVAL;
> +
>  	/* Initialize An with 2 random values and acquire it */
>  	for (i = 0; i < 2; i++)
>  		I915_WRITE(PORT_HDCP_ANINIT(port), get_random_u32());
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index f5d7bfb43006..dfca361ebb24 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1106,6 +1106,22 @@ bool intel_hdmi_hdcp_check_link(struct intel_digital_port *intel_dig_port)
>  	return true;
>  }
>  
> +static
> +int intel_hdmi_hdcp_capable(struct intel_digital_port *intel_dig_port,
> +			    bool *hdcp_capable)
> +{
> +	u8 bksv[5];
> +	int ret, retry = 1;
> +
> +	do {
> +		ret = intel_hdmi_hdcp_read_bksv(intel_dig_port, bksv);
> +		if (!ret)
> +			*hdcp_capable = intel_hdcp_is_ksv_valid(bksv);
> +	} while (!(*hdcp_capable) && retry--);
> +
> +	return ret;
> +}
> +
>  static const struct intel_hdcp_shim intel_hdmi_hdcp_shim = {
>  	.write_an_aksv = intel_hdmi_hdcp_write_an_aksv,
>  	.read_bksv = intel_hdmi_hdcp_read_bksv,
> @@ -1117,6 +1133,7 @@ static const struct intel_hdcp_shim intel_hdmi_hdcp_shim = {
>  	.read_v_prime_part = intel_hdmi_hdcp_read_v_prime_part,
>  	.toggle_signalling = intel_hdmi_hdcp_toggle_signalling,
>  	.check_link = intel_hdmi_hdcp_check_link,
> +	.hdcp_capable = intel_hdmi_hdcp_capable,
>  };
>  
>  static void intel_hdmi_prepare(struct intel_encoder *encoder,
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/8] drm/i915: Retry HDCP BKSV read

[Ramalingam C]

On Friday 02 February 2018 07:46 PM, Sean Paul wrote:
> On Fri, Feb 02, 2018 at 04:15:16PM +0530, Ramalingam C wrote:
>> When BKSV is invalid, to mitigate any communication errors,
>> BKSV is read once again.
> Why is the Bksv read any more volatile than other reads? If the channel is
> noisy, the retries should be done in the shim implementation (I think there are
> already i2c retries).
>
> Sean
True. But compliance spec says that when we determine that bksv is 
invalid (not with 20 1s), we should re-read and confirm.
CTS test 1A-05 test expect this behavior.

--Ram

>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_hdcp.c | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>> index b3f407cef8b0..fa2e7c727d00 100644
>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>> @@ -401,7 +401,7 @@ static int intel_hdcp_auth(struct intel_connector *connector)
>>   	struct drm_i915_private *dev_priv;
>>   	enum port port;
>>   	unsigned long r0_prime_gen_start;
>> -	int ret, i;
>> +	int ret, i, retry = 1;
>>   	union {
>>   		u32 reg[2];
>>   		u8 shim[DRM_HDCP_AN_LEN];
>> @@ -443,11 +443,16 @@ static int intel_hdcp_auth(struct intel_connector *connector)
>>   	r0_prime_gen_start = jiffies;
>>   
>>   	memset(&bksv, 0, sizeof(bksv));
>> -	ret = shim->read_bksv(intel_dig_port, bksv.shim);
>> +
>> +	do {
>> +		ret = shim->read_bksv(intel_dig_port, bksv.shim);
>> +		if (!ret)
>> +			if (!intel_hdcp_is_ksv_valid(bksv.shim))
>> +				ret = -ENODEV;
>> +	} while (ret && retry--);
>> +
>>   	if (ret)
>>   		return ret;
>> -	else if (!intel_hdcp_is_ksv_valid(bksv.shim))
>> -		return -ENODEV;
>>   
>>   	I915_WRITE(PORT_HDCP_BKSVLO(port), bksv.reg[0]);
>>   	I915_WRITE(PORT_HDCP_BKSVHI(port), bksv.reg[1]);
>> -- 
>> 2.7.4
>>

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

Re: [PATCH 5/8] drm/i915: Optimize HDCP key load

[Ramalingam C]

On Friday 02 February 2018 07:48 PM, Sean Paul wrote:
> On Fri, Feb 02, 2018 at 04:15:17PM +0530, Ramalingam C wrote:
>> HDCP key need not be cleared on each hdcp disable. And HDCP key Load
>> is skipped if key is already loaded.
>>
> I had previously encountered issues without clearing the key in my testing.
> IIRC, without clearing the keys things acted differently. How much time are we
> saving by optimizing this?
>
> Sean
Time profiling is not done. As per the Bspec, Once Keys are loaded, they 
will be cleared only when PG1/PG0 is off.
So on Resume we need to load the keys. Since we have the status register 
for key load state, I feel we could rely on them.

Now with our code state, I am not seeing the need for reloading the 
keys. I have tested on KBL.
I think this is worth an attempt as no failures are observed.

--Ram
>
>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_hdcp.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>> index fa2e7c727d00..5de9afd275b2 100644
>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>> @@ -51,6 +51,10 @@ static int intel_hdcp_load_keys(struct drm_i915_private *dev_priv)
>>   	int ret;
>>   	u32 val;
>>   
>> +	val = I915_READ(HDCP_KEY_STATUS);
>> +	if ((val & HDCP_KEY_LOAD_DONE) && (val & HDCP_KEY_LOAD_STATUS))
>> +		return 0;
>> +
>>   	/*
>>   	 * On HSW and BDW HW loads the HDCP1.4 Key when Display comes
>>   	 * out of reset. So if Key is not already loaded, its an error state.
>> @@ -542,8 +546,6 @@ static int _intel_hdcp_disable(struct intel_connector *connector)
>>   		return -ETIMEDOUT;
>>   	}
>>   
>> -	intel_hdcp_clear_keys(dev_priv);
>> -
>>   	ret = connector->hdcp_shim->toggle_signalling(intel_dig_port, false);
>>   	if (ret) {
>>   		DRM_ERROR("Failed to disable HDCP signalling\n");
>> -- 
>> 2.7.4
>>

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

Re: [PATCH 7/8] drm/i915: Reauthenticate HDCP on failure

[Sean Paul]

On Fri, Feb 02, 2018 at 04:15:19PM +0530, Ramalingam C wrote:
> When HDCP authentication fails, we add two more reauthentication.
> This will address all reauth expectation from compliance perspective.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdcp.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index a3463557f0f6..3caa548a9cad 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -566,7 +566,7 @@ static int _intel_hdcp_disable(struct intel_connector *connector)
>  static int _intel_hdcp_enable(struct intel_connector *connector)
>  {
>  	struct drm_i915_private *dev_priv = connector->base.dev->dev_private;
> -	int i, ret;
> +	int i, ret, reauth = 1;
>  
>  	if (!(I915_READ(SKL_FUSE_STATUS) & SKL_FUSE_PG_DIST_STATUS(1))) {
>  		DRM_ERROR("PG1 is disabled, cannot load keys\n");
> @@ -584,13 +584,17 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
>  		return ret;
>  	}
>  
> -	ret = intel_hdcp_auth(connector);
> -	if (ret) {
> -		DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret);
> -		return ret;
> -	}
> +	do {
> +		ret = intel_hdcp_auth(connector);
> +		if (ret)
> +			DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret);
> +		if (ret && reauth)
> +			DRM_DEBUG_KMS("[%s:%d] HDCP Reauth... %d/1\n",
> +					connector->base.name,
> +					connector->base.base.id, reauth);
> +	} while (ret && reauth--);
>  
> -	return 0;
> +	return ret;


I think we can do something a little more straightforward, like:

        int i, ret, tries = 2;

        for (i = 0; i < tries; i++) {
                ret = intel_hdcp_auth(...);
                if (!ret)
                        return 0;

                DRM_DEBUG_KMS("[%s:%d] HDCP Auth failure (%d)\n",
                              connector->base.name, connector->base.base.id,
                              ret);
        }

        DRM_ERROR("[%s:%d] HDCP authentication failed (%d tries/%d)\n",
                  connector->base.name, connector->base.base.id, tries, ret);
        _intel_hdcp_disable(...);
        return ret;

>  }
>  
>  static void intel_hdcp_check_work(struct work_struct *work)
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/8] drm/i915: fix misalignment in HDCP register def

[Sean Paul]

On Fri, Feb 02, 2018 at 04:15:20PM +0530, Ramalingam C wrote:
> This patch aligns all definitions of hdcp registers and their bits.

Ah, thanks! I think these got mangled when I dropped the SKL_ prefix.

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 58 ++++++++++++++++++++---------------------
>  1 file changed, 29 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 65ba10ad1fe5..e9c79b560823 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8483,34 +8483,34 @@ enum skl_power_gate {
>  #define   CNL_AUX_ANAOVRD1_LDO_BYPASS	(1<<23)
>  
>  /* HDCP Key Registers */
> -#define HDCP_KEY_CONF		_MMIO(0x66c00)
> +#define HDCP_KEY_CONF			_MMIO(0x66c00)
>  #define  HDCP_AKSV_SEND_TRIGGER		BIT(31)
>  #define  HDCP_CLEAR_KEYS_TRIGGER	BIT(30)
>  #define  HDCP_KEY_LOAD_TRIGGER		BIT(8)
> -#define HDCP_KEY_STATUS		_MMIO(0x66c04)
> -#define  HDCP_FUSE_IN_PROGRESS	BIT(7)
> +#define HDCP_KEY_STATUS			_MMIO(0x66c04)
> +#define  HDCP_FUSE_IN_PROGRESS		BIT(7)
>  #define  HDCP_FUSE_ERROR		BIT(6)
> -#define  HDCP_FUSE_DONE		BIT(5)
> -#define  HDCP_KEY_LOAD_STATUS	BIT(1)
> +#define  HDCP_FUSE_DONE			BIT(5)
> +#define  HDCP_KEY_LOAD_STATUS		BIT(1)
>  #define  HDCP_KEY_LOAD_DONE		BIT(0)
> -#define HDCP_AKSV_LO		_MMIO(0x66c10)
> -#define HDCP_AKSV_HI		_MMIO(0x66c14)
> +#define HDCP_AKSV_LO			_MMIO(0x66c10)
> +#define HDCP_AKSV_HI			_MMIO(0x66c14)
>  
>  /* HDCP Repeater Registers */
> -#define HDCP_REP_CTL		_MMIO(0x66d00)
> -#define  HDCP_DDIB_REP_PRESENT	BIT(30)
> -#define  HDCP_DDIA_REP_PRESENT	BIT(29)
> -#define  HDCP_DDIC_REP_PRESENT	BIT(28)
> -#define  HDCP_DDID_REP_PRESENT	BIT(27)
> -#define  HDCP_DDIF_REP_PRESENT	BIT(26)
> -#define  HDCP_DDIE_REP_PRESENT	BIT(25)
> +#define HDCP_REP_CTL			_MMIO(0x66d00)
> +#define  HDCP_DDIB_REP_PRESENT		BIT(30)
> +#define  HDCP_DDIA_REP_PRESENT		BIT(29)
> +#define  HDCP_DDIC_REP_PRESENT		BIT(28)
> +#define  HDCP_DDID_REP_PRESENT		BIT(27)
> +#define  HDCP_DDIF_REP_PRESENT		BIT(26)
> +#define  HDCP_DDIE_REP_PRESENT		BIT(25)
>  #define  HDCP_DDIB_SHA1_M0		(1 << 20)
>  #define  HDCP_DDIA_SHA1_M0		(2 << 20)
>  #define  HDCP_DDIC_SHA1_M0		(3 << 20)
>  #define  HDCP_DDID_SHA1_M0		(4 << 20)
>  #define  HDCP_DDIF_SHA1_M0		(5 << 20)
>  #define  HDCP_DDIE_SHA1_M0		(6 << 20) /* Bspec says 5? */
> -#define  HDCP_SHA1_BUSY		BIT(16)
> +#define  HDCP_SHA1_BUSY			BIT(16)
>  #define  HDCP_SHA1_READY		BIT(17)
>  #define  HDCP_SHA1_COMPLETE		BIT(18)
>  #define  HDCP_SHA1_V_MATCH		BIT(19)
> @@ -8526,7 +8526,7 @@ enum skl_power_gate {
>  #define HDCP_SHA_V_PRIME_H3		_MMIO(0x66d10)
>  #define HDCP_SHA_V_PRIME_H4		_MMIO(0x66d14)
>  #define HDCP_SHA_V_PRIME(h)		_MMIO((0x66d04 + h * 4))
> -#define HDCP_SHA_TEXT		_MMIO(0x66d18)
> +#define HDCP_SHA_TEXT			_MMIO(0x66d18)
>  
>  /* HDCP Auth Registers */
>  #define _PORTA_HDCP_AUTHENC		0x66800
> @@ -8542,25 +8542,25 @@ enum skl_power_gate {
>  					  _PORTD_HDCP_AUTHENC, \
>  					  _PORTE_HDCP_AUTHENC, \
>  					  _PORTF_HDCP_AUTHENC) + x)
> -#define PORT_HDCP_CONF(port)	_PORT_HDCP_AUTHENC(port, 0x0)
> -#define  HDCP_CONF_CAPTURE_AN	BIT(0)
> -#define  HDCP_CONF_AUTH_AND_ENC	(BIT(1) | BIT(0))
> -#define PORT_HDCP_ANINIT(port)	_PORT_HDCP_AUTHENC(port, 0x4)
> -#define PORT_HDCP_ANLO(port)	_PORT_HDCP_AUTHENC(port, 0x8)
> -#define PORT_HDCP_ANHI(port)	_PORT_HDCP_AUTHENC(port, 0xC)
> -#define PORT_HDCP_BKSVLO(port)	_PORT_HDCP_AUTHENC(port, 0x10)
> -#define PORT_HDCP_BKSVHI(port)	_PORT_HDCP_AUTHENC(port, 0x14)
> -#define PORT_HDCP_RPRIME(port)	_PORT_HDCP_AUTHENC(port, 0x18)
> -#define PORT_HDCP_STATUS(port)	_PORT_HDCP_AUTHENC(port, 0x1C)
> +#define PORT_HDCP_CONF(port)		_PORT_HDCP_AUTHENC(port, 0x0)
> +#define  HDCP_CONF_CAPTURE_AN		BIT(0)
> +#define  HDCP_CONF_AUTH_AND_ENC		(BIT(1) | BIT(0))
> +#define PORT_HDCP_ANINIT(port)		_PORT_HDCP_AUTHENC(port, 0x4)
> +#define PORT_HDCP_ANLO(port)		_PORT_HDCP_AUTHENC(port, 0x8)
> +#define PORT_HDCP_ANHI(port)		_PORT_HDCP_AUTHENC(port, 0xC)
> +#define PORT_HDCP_BKSVLO(port)		_PORT_HDCP_AUTHENC(port, 0x10)
> +#define PORT_HDCP_BKSVHI(port)		_PORT_HDCP_AUTHENC(port, 0x14)
> +#define PORT_HDCP_RPRIME(port)		_PORT_HDCP_AUTHENC(port, 0x18)
> +#define PORT_HDCP_STATUS(port)		_PORT_HDCP_AUTHENC(port, 0x1C)
>  #define  HDCP_STATUS_STREAM_A_ENC	BIT(31)
>  #define  HDCP_STATUS_STREAM_B_ENC	BIT(30)
>  #define  HDCP_STATUS_STREAM_C_ENC	BIT(29)
>  #define  HDCP_STATUS_STREAM_D_ENC	BIT(28)
>  #define  HDCP_STATUS_AUTH		BIT(21)
>  #define  HDCP_STATUS_ENC		BIT(20)
> -#define  HDCP_STATUS_RI_MATCH	BIT(19)
> -#define  HDCP_STATUS_R0_READY	BIT(18)
> -#define  HDCP_STATUS_AN_READY	BIT(17)
> +#define  HDCP_STATUS_RI_MATCH		BIT(19)
> +#define  HDCP_STATUS_R0_READY		BIT(18)
> +#define  HDCP_STATUS_AN_READY		BIT(17)
>  #define  HDCP_STATUS_CIPHER		BIT(16)
>  #define  HDCP_STATUS_FRAME_CNT(x)	((x >> 8) & 0xff)
>  
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/8] drm/i915: Detect panel's hdcp capability

[Ramalingam C]

On Friday 02 February 2018 07:54 PM, Sean Paul wrote:
> On Fri, Feb 02, 2018 at 04:15:18PM +0530, Ramalingam C wrote:
>> As a first step of HDCP authentication detects the panel's HDCP
>> capability. This is mandated for DP HDCP1.4.
>>
>> For DP 0th Bit of Bcaps register indicates the panel's hdcp capability
>> For HDMI valid BKSV indicates the panel's hdcp capability.
> We're already checking valid bksv, so there's no need to expose that function and
> call it twice for the HDMI case, hdcp_capable should just always be true.
we can make the hdmi's hdcp_capable as dummy. As hdcp1.4 spec for hdmi 
says panel's capability is determined by valid bksv.
Which we are already doing it. no sequence mandated.
>
> For DP, we read and check the bksv , so what do non-capable displays return for
> the bksv?
But for DP HDCP1.4 spec mandates that An write should entertained only 
after verifying the 0th bit(hdcp_capable) of BCSPS register.
Else all DP compliance tests will fail :(

We are not addressing any functional issue but deviation from the 
HDCP1.4 spec for DP.

--Ram
>
> Sean
>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c   | 19 +++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h  |  5 +++++
>>   drivers/gpu/drm/i915/intel_hdcp.c | 10 ++++++++--
>>   drivers/gpu/drm/i915/intel_hdmi.c | 17 +++++++++++++++++
>>   4 files changed, 49 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 03d86ff9b805..2623b2beda1a 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -5218,6 +5218,24 @@ bool intel_dp_hdcp_check_link(struct intel_digital_port *intel_dig_port)
>>   	return !(bstatus & (DP_BSTATUS_LINK_FAILURE | DP_BSTATUS_REAUTH_REQ));
>>   }
>>   
>> +static
>> +int intel_dp_hdcp_capable(struct intel_digital_port *intel_dig_port,
>> +			  bool *hdcp_capable)
>> +{
>> +	ssize_t ret;
>> +	u8 bcaps;
>> +
>> +	ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BCAPS,
>> +			       &bcaps, 1);
>> +	if (ret != 1) {
>> +		DRM_ERROR("Read bcaps from DP/AUX failed (%zd)\n", ret);
>> +		return ret >= 0 ? -EIO : ret;
>> +	}
>> +
>> +	*hdcp_capable = bcaps & DP_BCAPS_HDCP_CAPABLE;
>> +	return 0;
>> +}
>> +
>>   static const struct intel_hdcp_shim intel_dp_hdcp_shim = {
>>   	.write_an_aksv = intel_dp_hdcp_write_an_aksv,
>>   	.read_bksv = intel_dp_hdcp_read_bksv,
>> @@ -5229,6 +5247,7 @@ static const struct intel_hdcp_shim intel_dp_hdcp_shim = {
>>   	.read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
>>   	.toggle_signalling = intel_dp_hdcp_toggle_signalling,
>>   	.check_link = intel_dp_hdcp_check_link,
>> +	.hdcp_capable = intel_dp_hdcp_capable,
>>   };
>>   
>>   static void intel_edp_panel_vdd_sanitize(struct intel_dp *intel_dp)
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index d6a808374dfb..409aee9010ba 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -369,6 +369,10 @@ struct intel_hdcp_shim {
>>   
>>   	/* Ensures the link is still protected */
>>   	bool (*check_link)(struct intel_digital_port *intel_dig_port);
>> +
>> +	/* Detects panel's hdcp capablility */
>> +	int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
>> +			    bool *hdcp_capable);
>>   };
>>   
>>   struct intel_connector {
>> @@ -1848,6 +1852,7 @@ int intel_hdcp_enable(struct intel_connector *connector);
>>   int intel_hdcp_disable(struct intel_connector *connector);
>>   int intel_hdcp_check_link(struct intel_connector *connector);
>>   bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port);
>> +bool intel_hdcp_is_ksv_valid(u8 *ksv);
>>   
>>   /* intel_psr.c */
>>   #define CAN_PSR(dev_priv) (HAS_PSR(dev_priv) && dev_priv->psr.sink_support)
>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>> index 5de9afd275b2..a3463557f0f6 100644
>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>> @@ -132,7 +132,6 @@ u32 intel_hdcp_get_repeater_ctl(struct intel_digital_port *intel_dig_port)
>>   	return -EINVAL;
>>   }
>>   
>> -static
>>   bool intel_hdcp_is_ksv_valid(u8 *ksv)
>>   {
>>   	int i, ones = 0;
>> @@ -418,13 +417,20 @@ static int intel_hdcp_auth(struct intel_connector *connector)
>>   		u32 reg;
>>   		u8 shim[DRM_HDCP_RI_LEN];
>>   	} ri;
>> -	bool repeater_present;
>> +	bool repeater_present, hdcp_capable;
>>   	const struct intel_hdcp_shim *shim = connector->hdcp_shim;
>>   	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
>>   
>>   	dev_priv = to_i915(connector->base.dev);
>>   	port = intel_dig_port->base.port;
>>   
>> +	/* Detect whether panel is HDCP capable or not */
>> +	ret = shim->hdcp_capable(intel_dig_port, &hdcp_capable);
>> +	if (ret)
>> +		return ret;
>> +	if (!hdcp_capable)
>> +		return -EINVAL;
>> +
>>   	/* Initialize An with 2 random values and acquire it */
>>   	for (i = 0; i < 2; i++)
>>   		I915_WRITE(PORT_HDCP_ANINIT(port), get_random_u32());
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index f5d7bfb43006..dfca361ebb24 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -1106,6 +1106,22 @@ bool intel_hdmi_hdcp_check_link(struct intel_digital_port *intel_dig_port)
>>   	return true;
>>   }
>>   
>> +static
>> +int intel_hdmi_hdcp_capable(struct intel_digital_port *intel_dig_port,
>> +			    bool *hdcp_capable)
>> +{
>> +	u8 bksv[5];
>> +	int ret, retry = 1;
>> +
>> +	do {
>> +		ret = intel_hdmi_hdcp_read_bksv(intel_dig_port, bksv);
>> +		if (!ret)
>> +			*hdcp_capable = intel_hdcp_is_ksv_valid(bksv);
>> +	} while (!(*hdcp_capable) && retry--);
>> +
>> +	return ret;
>> +}
>> +
>>   static const struct intel_hdcp_shim intel_hdmi_hdcp_shim = {
>>   	.write_an_aksv = intel_hdmi_hdcp_write_an_aksv,
>>   	.read_bksv = intel_hdmi_hdcp_read_bksv,
>> @@ -1117,6 +1133,7 @@ static const struct intel_hdcp_shim intel_hdmi_hdcp_shim = {
>>   	.read_v_prime_part = intel_hdmi_hdcp_read_v_prime_part,
>>   	.toggle_signalling = intel_hdmi_hdcp_toggle_signalling,
>>   	.check_link = intel_hdmi_hdcp_check_link,
>> +	.hdcp_capable = intel_hdmi_hdcp_capable,
>>   };
>>   
>>   static void intel_hdmi_prepare(struct intel_encoder *encoder,
>> -- 
>> 2.7.4
>>

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

Re: [PATCH 4/8] drm/i915: Retry HDCP BKSV read

[Sean Paul]

On Fri, Feb 02, 2018 at 07:56:22PM +0530, Ramalingam C wrote:
> 
> 
> On Friday 02 February 2018 07:46 PM, Sean Paul wrote:
> > On Fri, Feb 02, 2018 at 04:15:16PM +0530, Ramalingam C wrote:
> > > When BKSV is invalid, to mitigate any communication errors,
> > > BKSV is read once again.
> > Why is the Bksv read any more volatile than other reads? If the channel is
> > noisy, the retries should be done in the shim implementation (I think there are
> > already i2c retries).
> > 
> > Sean
> True. But compliance spec says that when we determine that bksv is invalid
> (not with 20 1s), we should re-read and confirm.
> CTS test 1A-05 test expect this behavior.

Ah, ok. So this is different from what you wrote in the commit message. I think
we can simplify this to make it more clear.

> 
> --Ram
> 
> > 
> > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_hdcp.c | 13 +++++++++----
> > >   1 file changed, 9 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > > index b3f407cef8b0..fa2e7c727d00 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > @@ -401,7 +401,7 @@ static int intel_hdcp_auth(struct intel_connector *connector)
> > >   	struct drm_i915_private *dev_priv;
> > >   	enum port port;
> > >   	unsigned long r0_prime_gen_start;
> > > -	int ret, i;
> > > +	int ret, i, retry = 1;
> > >   	union {
> > >   		u32 reg[2];
> > >   		u8 shim[DRM_HDCP_AN_LEN];
> > > @@ -443,11 +443,16 @@ static int intel_hdcp_auth(struct intel_connector *connector)
> > >   	r0_prime_gen_start = jiffies;
> > >   	memset(&bksv, 0, sizeof(bksv));
> > > -	ret = shim->read_bksv(intel_dig_port, bksv.shim);
> > > +
> > > +	do {
> > > +		ret = shim->read_bksv(intel_dig_port, bksv.shim);
> > > +		if (!ret)
> > > +			if (!intel_hdcp_is_ksv_valid(bksv.shim))
> > > +				ret = -ENODEV;
> > > +	} while (ret && retry--);


        /* HDCP spec states that we must retry the bksv if it is invalid */
        for (i = 0; i < bksv_tries; i++) {
                ret = shim->read_bksv(intel_dig_port, bksv.shim);
                if (ret)
                        return ret;
                if (intel_hdcp_is_ksv_valid(bksv.shim))
                        break;
        }
        if (i == bksv_tries)
                return -ENODEV;



> > > +
> > >   	if (ret)
> > >   		return ret;
> > > -	else if (!intel_hdcp_is_ksv_valid(bksv.shim))
> > > -		return -ENODEV;
> > >   	I915_WRITE(PORT_HDCP_BKSVLO(port), bksv.reg[0]);
> > >   	I915_WRITE(PORT_HDCP_BKSVHI(port), bksv.reg[1]);
> > > -- 
> > > 2.7.4
> > > 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915: Handle failure from 2nd stage HDCP auth

[Sean Paul]

On Fri, Feb 02, 2018 at 07:52:24PM +0530, Ramalingam C wrote:
> 
> 
> On Friday 02 February 2018 07:39 PM, Sean Paul wrote:
> > On Fri, Feb 02, 2018 at 04:15:13PM +0530, Ramalingam C wrote:
> > > We enable the HDCP encryption as a part of first stage authentication.
> > > So when second stage authentication fails, we need to disable the HDCP
> > > encryption and signalling.
> > > 
> > > This patch handles above requirement.
> > > 
> > > For reusing the _intel_hdcp_disable from generic authentication flow,
> > > this patch retain the reference to connector till the generic hdcp flow.
> > > This will be handy to provide the connector details in HDCP enable
> > > debug info.
> > > 
> > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_hdcp.c | 41 +++++++++++++++++++++++----------------
> > >   1 file changed, 24 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > > index b97184eccd9c..0021511ae4d7 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > @@ -16,6 +16,8 @@
> > >   #define KEY_LOAD_TRIES	5
> > > +static int _intel_hdcp_disable(struct intel_connector *connector);
> > > +
> > >   static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port,
> > >   				    const struct intel_hdcp_shim *shim)
> > >   {
> > > @@ -138,17 +140,24 @@ bool intel_hdcp_is_ksv_valid(u8 *ksv)
> > >   	return true;
> > >   }
> > > +static
> > > +struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector)
> > > +{
> > > +	return enc_to_dig_port(&connector->encoder->base);
> > > +}
> > > +
> > >   /* Implements Part 2 of the HDCP authorization procedure */
> > >   static
> > > -int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port,
> > > -			       const struct intel_hdcp_shim *shim)
> > > +int intel_hdcp_auth_downstream(struct intel_connector *connector)
> > >   {
> > >   	struct drm_i915_private *dev_priv;
> > >   	u32 vprime, sha_text, sha_leftovers, rep_ctl;
> > >   	u8 bstatus[2], num_downstream, *ksv_fifo;
> > >   	int ret, i, j, sha_idx;
> > > +	const struct intel_hdcp_shim *shim = connector->hdcp_shim;
> > > +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
> > > -	dev_priv = intel_dig_port->base.base.dev->dev_private;
> > > +	dev_priv = to_i915(connector->base.dev);
> > >   	ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim);
> > >   	if (ret) {
> > > @@ -385,8 +394,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port,
> > >   }
> > >   /* Implements Part 1 of the HDCP authorization procedure */
> > > -static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
> > > -			   const struct intel_hdcp_shim *shim)
> > > +static int intel_hdcp_auth(struct intel_connector *connector)
> > >   {
> > >   	struct drm_i915_private *dev_priv;
> > >   	enum port port;
> > > @@ -405,9 +413,10 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
> > >   		u8 shim[DRM_HDCP_RI_LEN];
> > >   	} ri;
> > >   	bool repeater_present;
> > > +	const struct intel_hdcp_shim *shim = connector->hdcp_shim;
> > > +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
> > > -	dev_priv = intel_dig_port->base.base.dev->dev_private;
> > > -
> > > +	dev_priv = to_i915(connector->base.dev);
> > >   	port = intel_dig_port->base.port;
> > >   	/* Initialize An with 2 random values and acquire it */
> > > @@ -498,19 +507,18 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
> > >   	 * on those as well.
> > >   	 */
> > > -	if (repeater_present)
> > > -		return intel_hdcp_auth_downstream(intel_dig_port, shim);
> > > +	if (repeater_present) {
> > > +		ret = intel_hdcp_auth_downstream(connector);
> > > +		if (ret) {
> > > +			_intel_hdcp_disable(connector);
> > If you just call this from _intel_hdcp_enable when intel_hdcp_auth() fails, you
> > can avoid all of this code, it'd just be one line.
> Yes. Thought about that option too. Actually I would prefer having the
> reference of intel_connector untill generic hdcp auth implementation.
> we can pass the intel digital port for encoder specific shims.
> 
> This will help me with
>     providing the connector details for state transitions as in next patches
>     next steps like SRM passing in for revocation and downstream topology
> gathering (perhaps will start with availability of complementing user space)
> 
> At this point if we dont want this code, I will disable hdcp from enable
> functions.

Yes please. Let's not try to predict the future and keep things as simple as
possible.

Sean

> 
> -Ram
> > 
> > 
> > > +			return ret;
> > > +		}
> > > +	}
> > >   	DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n");
> > >   	return 0;
> > >   }
> > > -static
> > > -struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector)
> > > -{
> > > -	return enc_to_dig_port(&intel_attached_encoder(&connector->base)->base);
> > > -}
> > > -
> > >   static int _intel_hdcp_disable(struct intel_connector *connector)
> > >   {
> > >   	struct drm_i915_private *dev_priv = connector->base.dev->dev_private;
> > > @@ -558,8 +566,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
> > >   		return ret;
> > >   	}
> > > -	ret = intel_hdcp_auth(conn_to_dig_port(connector),
> > > -			      connector->hdcp_shim);
> > > +	ret = intel_hdcp_auth(connector);
> > >   	if (ret) {
> > >   		DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret);
> > >   		return ret;
> > > -- 
> > > 2.7.4
> > > 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/8] drm/i915: Stop encryption for repeater with no sink

[Sean Paul]

On Fri, Feb 02, 2018 at 07:42:47PM +0530, Ramalingam C wrote:
> 
> 
> On Friday 02 February 2018 07:43 PM, Sean Paul wrote:
> > On Fri, Feb 02, 2018 at 04:15:14PM +0530, Ramalingam C wrote:
> > > If a HDCP repeater is detected with zero hdcp authenticated
> > > downstream devices, there are two option as below:
> > > 
> > > 1. Dont continue on second stage authentication. Disable encryption.
> > > 2. Continue with second stage authentication excluding the KSV list and
> > >     continue encryption on success.
> > > 
> > > This patch adopts the option 1.
> > It doesn't seem that hard to adopt option 2 and avoid failure. That would result
> > in a better experience.
> True. Not too much effort for option 2. But I am not seeing any ROI out of
> it at this point.
> what is the benefit of encrypting if repeater cant use it, as it has 0 sinks
> attached to it.
> Still do we want option two?

Is it possible the repeater itself has video output? I'm also worried about
non-compliant displays which may report having a repeater. If these aren't
possible, I agree. Just beef up the comment like "If there are no downstream
devices, spec requires we disable all encryption. So fail here to ensure hdcp is
disabled"

Sean

> 
> -Ram
> > 
> > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_hdcp.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > > index 0021511ae4d7..182a3c8a4e4a 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > @@ -175,10 +175,10 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector)
> > >   		return -EPERM;
> > >   	}
> > > -	/* If there are no downstream devices, we're all done. */
> > > +	/* If there are no downstream devices, we're not encrypting. */
> > >   	num_downstream = DRM_HDCP_NUM_DOWNSTREAM(bstatus[0]);
> > >   	if (num_downstream == 0)
> > > -		return 0;
> > > +		return -EINVAL;
> > >   	ksv_fifo = kzalloc(num_downstream * DRM_HDCP_KSV_LEN, GFP_KERNEL);
> > >   	if (!ksv_fifo)
> > > -- 
> > > 2.7.4
> > > 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915: Handle failure from 2nd stage HDCP auth

[Ramalingam C]

On Friday 02 February 2018 08:15 PM, Sean Paul wrote:
> On Fri, Feb 02, 2018 at 07:52:24PM +0530, Ramalingam C wrote:
>>
>> On Friday 02 February 2018 07:39 PM, Sean Paul wrote:
>>> On Fri, Feb 02, 2018 at 04:15:13PM +0530, Ramalingam C wrote:
>>>> We enable the HDCP encryption as a part of first stage authentication.
>>>> So when second stage authentication fails, we need to disable the HDCP
>>>> encryption and signalling.
>>>>
>>>> This patch handles above requirement.
>>>>
>>>> For reusing the _intel_hdcp_disable from generic authentication flow,
>>>> this patch retain the reference to connector till the generic hdcp flow.
>>>> This will be handy to provide the connector details in HDCP enable
>>>> debug info.
>>>>
>>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_hdcp.c | 41 +++++++++++++++++++++++----------------
>>>>    1 file changed, 24 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>>>> index b97184eccd9c..0021511ae4d7 100644
>>>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>>>> @@ -16,6 +16,8 @@
>>>>    #define KEY_LOAD_TRIES	5
>>>> +static int _intel_hdcp_disable(struct intel_connector *connector);
>>>> +
>>>>    static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port,
>>>>    				    const struct intel_hdcp_shim *shim)
>>>>    {
>>>> @@ -138,17 +140,24 @@ bool intel_hdcp_is_ksv_valid(u8 *ksv)
>>>>    	return true;
>>>>    }
>>>> +static
>>>> +struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector)
>>>> +{
>>>> +	return enc_to_dig_port(&connector->encoder->base);
>>>> +}
>>>> +
>>>>    /* Implements Part 2 of the HDCP authorization procedure */
>>>>    static
>>>> -int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port,
>>>> -			       const struct intel_hdcp_shim *shim)
>>>> +int intel_hdcp_auth_downstream(struct intel_connector *connector)
>>>>    {
>>>>    	struct drm_i915_private *dev_priv;
>>>>    	u32 vprime, sha_text, sha_leftovers, rep_ctl;
>>>>    	u8 bstatus[2], num_downstream, *ksv_fifo;
>>>>    	int ret, i, j, sha_idx;
>>>> +	const struct intel_hdcp_shim *shim = connector->hdcp_shim;
>>>> +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
>>>> -	dev_priv = intel_dig_port->base.base.dev->dev_private;
>>>> +	dev_priv = to_i915(connector->base.dev);
>>>>    	ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim);
>>>>    	if (ret) {
>>>> @@ -385,8 +394,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port,
>>>>    }
>>>>    /* Implements Part 1 of the HDCP authorization procedure */
>>>> -static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
>>>> -			   const struct intel_hdcp_shim *shim)
>>>> +static int intel_hdcp_auth(struct intel_connector *connector)
>>>>    {
>>>>    	struct drm_i915_private *dev_priv;
>>>>    	enum port port;
>>>> @@ -405,9 +413,10 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
>>>>    		u8 shim[DRM_HDCP_RI_LEN];
>>>>    	} ri;
>>>>    	bool repeater_present;
>>>> +	const struct intel_hdcp_shim *shim = connector->hdcp_shim;
>>>> +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
>>>> -	dev_priv = intel_dig_port->base.base.dev->dev_private;
>>>> -
>>>> +	dev_priv = to_i915(connector->base.dev);
>>>>    	port = intel_dig_port->base.port;
>>>>    	/* Initialize An with 2 random values and acquire it */
>>>> @@ -498,19 +507,18 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
>>>>    	 * on those as well.
>>>>    	 */
>>>> -	if (repeater_present)
>>>> -		return intel_hdcp_auth_downstream(intel_dig_port, shim);
>>>> +	if (repeater_present) {
>>>> +		ret = intel_hdcp_auth_downstream(connector);
>>>> +		if (ret) {
>>>> +			_intel_hdcp_disable(connector);
>>> If you just call this from _intel_hdcp_enable when intel_hdcp_auth() fails, you
>>> can avoid all of this code, it'd just be one line.
>> Yes. Thought about that option too. Actually I would prefer having the
>> reference of intel_connector untill generic hdcp auth implementation.
>> we can pass the intel digital port for encoder specific shims.
>>
>> This will help me with
>>      providing the connector details for state transitions as in next patches
>>      next steps like SRM passing in for revocation and downstream topology
>> gathering (perhaps will start with availability of complementing user space)
>>
>> At this point if we dont want this code, I will disable hdcp from enable
>> functions.
> Yes please. Let's not try to predict the future and keep things as simple as
> possible.
>
> Sean
but patch 3(adding connector info into hdcp state transition debug logs) 
still needs conenctor ref at auth.
Do we still want to drop the connector ref passing? If so we will lose 
patch 3 too.

-Ram

>
>> -Ram
>>>
>>>> +			return ret;
>>>> +		}
>>>> +	}
>>>>    	DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n");
>>>>    	return 0;
>>>>    }
>>>> -static
>>>> -struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector)
>>>> -{
>>>> -	return enc_to_dig_port(&intel_attached_encoder(&connector->base)->base);
>>>> -}
>>>> -
>>>>    static int _intel_hdcp_disable(struct intel_connector *connector)
>>>>    {
>>>>    	struct drm_i915_private *dev_priv = connector->base.dev->dev_private;
>>>> @@ -558,8 +566,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
>>>>    		return ret;
>>>>    	}
>>>> -	ret = intel_hdcp_auth(conn_to_dig_port(connector),
>>>> -			      connector->hdcp_shim);
>>>> +	ret = intel_hdcp_auth(connector);
>>>>    	if (ret) {
>>>>    		DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret);
>>>>    		return ret;
>>>> -- 
>>>> 2.7.4
>>>>

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

Re: [PATCH 2/8] drm/i915: Stop encryption for repeater with no sink

[Ramalingam C]

On Friday 02 February 2018 08:18 PM, Sean Paul wrote:
> On Fri, Feb 02, 2018 at 07:42:47PM +0530, Ramalingam C wrote:
>>
>> On Friday 02 February 2018 07:43 PM, Sean Paul wrote:
>>> On Fri, Feb 02, 2018 at 04:15:14PM +0530, Ramalingam C wrote:
>>>> If a HDCP repeater is detected with zero hdcp authenticated
>>>> downstream devices, there are two option as below:
>>>>
>>>> 1. Dont continue on second stage authentication. Disable encryption.
>>>> 2. Continue with second stage authentication excluding the KSV list and
>>>>      continue encryption on success.
>>>>
>>>> This patch adopts the option 1.
>>> It doesn't seem that hard to adopt option 2 and avoid failure. That would result
>>> in a better experience.
>> True. Not too much effort for option 2. But I am not seeing any ROI out of
>> it at this point.
>> what is the benefit of encrypting if repeater cant use it, as it has 0 sinks
>> attached to it.
>> Still do we want option two?
> Is it possible the repeater itself has video output? I'm also worried about
> non-compliant displays which may report having a repeater. If these aren't
> possible, I agree. Just beef up the comment like "If there are no downstream
> devices, spec requires we disable all encryption. So fail here to ensure hdcp is
> disabled"
>
> Sean

When spec provide an option that we can disable the HDCP encryption incase
of repeater with device count 0, that means repeaters cant have display.

Display which claims to be a repeater, mostly wont pass the second stage
authentication meant for repeaters. So we need not worry about them.

Sure as you mentioned i will rephrase the commit message with these informations.

--Ram

>
>> -Ram
>>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_hdcp.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>>>> index 0021511ae4d7..182a3c8a4e4a 100644
>>>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>>>> @@ -175,10 +175,10 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector)
>>>>    		return -EPERM;
>>>>    	}
>>>> -	/* If there are no downstream devices, we're all done. */
>>>> +	/* If there are no downstream devices, we're not encrypting. */
>>>>    	num_downstream = DRM_HDCP_NUM_DOWNSTREAM(bstatus[0]);
>>>>    	if (num_downstream == 0)
>>>> -		return 0;
>>>> +		return -EINVAL;
>>>>    	ksv_fifo = kzalloc(num_downstream * DRM_HDCP_KSV_LEN, GFP_KERNEL);
>>>>    	if (!ksv_fifo)
>>>> -- 
>>>> 2.7.4
>>>>

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

Re: [PATCH 7/8] drm/i915: Reauthenticate HDCP on failure

[Ramalingam C]

On Friday 02 February 2018 08:07 PM, Sean Paul wrote:
> On Fri, Feb 02, 2018 at 04:15:19PM +0530, Ramalingam C wrote:
>> When HDCP authentication fails, we add two more reauthentication.
>> This will address all reauth expectation from compliance perspective.
>>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_hdcp.c | 18 +++++++++++-------
>>   1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>> index a3463557f0f6..3caa548a9cad 100644
>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>> @@ -566,7 +566,7 @@ static int _intel_hdcp_disable(struct intel_connector *connector)
>>   static int _intel_hdcp_enable(struct intel_connector *connector)
>>   {
>>   	struct drm_i915_private *dev_priv = connector->base.dev->dev_private;
>> -	int i, ret;
>> +	int i, ret, reauth = 1;
>>   
>>   	if (!(I915_READ(SKL_FUSE_STATUS) & SKL_FUSE_PG_DIST_STATUS(1))) {
>>   		DRM_ERROR("PG1 is disabled, cannot load keys\n");
>> @@ -584,13 +584,17 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
>>   		return ret;
>>   	}
>>   
>> -	ret = intel_hdcp_auth(connector);
>> -	if (ret) {
>> -		DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret);
>> -		return ret;
>> -	}
>> +	do {
>> +		ret = intel_hdcp_auth(connector);
>> +		if (ret)
>> +			DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret);
>> +		if (ret && reauth)
>> +			DRM_DEBUG_KMS("[%s:%d] HDCP Reauth... %d/1\n",
>> +					connector->base.name,
>> +					connector->base.base.id, reauth);
>> +	} while (ret && reauth--);
>>   
>> -	return 0;
>> +	return ret;
>
> I think we can do something a little more straightforward, like:
>
>          int i, ret, tries = 2;
>
>          for (i = 0; i < tries; i++) {
>                  ret = intel_hdcp_auth(...);
>                  if (!ret)
>                          return 0;
>
>                  DRM_DEBUG_KMS("[%s:%d] HDCP Auth failure (%d)\n",
>                                connector->base.name, connector->base.base.id,
>                                ret);
>          }
>
>          DRM_ERROR("[%s:%d] HDCP authentication failed (%d tries/%d)\n",
>                    connector->base.name, connector->base.base.id, tries, ret);
>          _intel_hdcp_disable(...);
>          return ret;
>
Sure. I will do it.

-Ram
>>   }
>>   
>>   static void intel_hdcp_check_work(struct work_struct *work)
>> -- 
>> 2.7.4
>>

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

Re: [PATCH 2/8] drm/i915: Stop encryption for repeater with no sink

[Sean Paul]

On Fri, Feb 02, 2018 at 08:33:38PM +0530, Ramalingam C wrote:
> 
> 
> On Friday 02 February 2018 08:18 PM, Sean Paul wrote:
> > On Fri, Feb 02, 2018 at 07:42:47PM +0530, Ramalingam C wrote:
> > > 
> > > On Friday 02 February 2018 07:43 PM, Sean Paul wrote:
> > > > On Fri, Feb 02, 2018 at 04:15:14PM +0530, Ramalingam C wrote:
> > > > > If a HDCP repeater is detected with zero hdcp authenticated
> > > > > downstream devices, there are two option as below:
> > > > > 
> > > > > 1. Dont continue on second stage authentication. Disable encryption.
> > > > > 2. Continue with second stage authentication excluding the KSV list and
> > > > >      continue encryption on success.
> > > > > 
> > > > > This patch adopts the option 1.
> > > > It doesn't seem that hard to adopt option 2 and avoid failure. That would result
> > > > in a better experience.
> > > True. Not too much effort for option 2. But I am not seeing any ROI out of
> > > it at this point.
> > > what is the benefit of encrypting if repeater cant use it, as it has 0 sinks
> > > attached to it.
> > > Still do we want option two?
> > Is it possible the repeater itself has video output? I'm also worried about
> > non-compliant displays which may report having a repeater. If these aren't
> > possible, I agree. Just beef up the comment like "If there are no downstream
> > devices, spec requires we disable all encryption. So fail here to ensure hdcp is
> > disabled"
> > 
> > Sean
> 
> When spec provide an option that we can disable the HDCP encryption incase
> of repeater with device count 0, that means repeaters cant have display.
> 
> Display which claims to be a repeater, mostly wont pass the second stage
> authentication meant for repeaters. So we need not worry about them.
> 
> Sure as you mentioned i will rephrase the commit message with these informations.

Not just the commit message. Please also update the comment in the code so it's
clear.

Sean

> 
> --Ram
> 
> > 
> > > -Ram
> > > > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > > > ---
> > > > >    drivers/gpu/drm/i915/intel_hdcp.c | 4 ++--
> > > > >    1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > > > > index 0021511ae4d7..182a3c8a4e4a 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > > > @@ -175,10 +175,10 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector)
> > > > >    		return -EPERM;
> > > > >    	}
> > > > > -	/* If there are no downstream devices, we're all done. */
> > > > > +	/* If there are no downstream devices, we're not encrypting. */
> > > > >    	num_downstream = DRM_HDCP_NUM_DOWNSTREAM(bstatus[0]);
> > > > >    	if (num_downstream == 0)
> > > > > -		return 0;
> > > > > +		return -EINVAL;
> > > > >    	ksv_fifo = kzalloc(num_downstream * DRM_HDCP_KSV_LEN, GFP_KERNEL);
> > > > >    	if (!ksv_fifo)
> > > > > -- 
> > > > > 2.7.4
> > > > > 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915: Handle failure from 2nd stage HDCP auth

[Sean Paul]

On Fri, Feb 2, 2018 at 9:51 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
>
>
> On Friday 02 February 2018 08:15 PM, Sean Paul wrote:
>>
>> On Fri, Feb 02, 2018 at 07:52:24PM +0530, Ramalingam C wrote:
>>>
>>>
>>> On Friday 02 February 2018 07:39 PM, Sean Paul wrote:
>>>>
>>>> On Fri, Feb 02, 2018 at 04:15:13PM +0530, Ramalingam C wrote:
>>>>>
>>>>> We enable the HDCP encryption as a part of first stage authentication.
>>>>> So when second stage authentication fails, we need to disable the HDCP
>>>>> encryption and signalling.
>>>>>
>>>>> This patch handles above requirement.
>>>>>
>>>>> For reusing the _intel_hdcp_disable from generic authentication flow,
>>>>> this patch retain the reference to connector till the generic hdcp
>>>>> flow.
>>>>> This will be handy to provide the connector details in HDCP enable
>>>>> debug info.
>>>>>
>>>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/intel_hdcp.c | 41
>>>>> +++++++++++++++++++++++----------------
>>>>>    1 file changed, 24 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c
>>>>> b/drivers/gpu/drm/i915/intel_hdcp.c
>>>>> index b97184eccd9c..0021511ae4d7 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>>>>> @@ -16,6 +16,8 @@
>>>>>    #define KEY_LOAD_TRIES       5
>>>>> +static int _intel_hdcp_disable(struct intel_connector *connector);
>>>>> +
>>>>>    static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port
>>>>> *intel_dig_port,
>>>>>                                     const struct intel_hdcp_shim *shim)
>>>>>    {
>>>>> @@ -138,17 +140,24 @@ bool intel_hdcp_is_ksv_valid(u8 *ksv)
>>>>>         return true;
>>>>>    }
>>>>> +static
>>>>> +struct intel_digital_port *conn_to_dig_port(struct intel_connector
>>>>> *connector)
>>>>> +{
>>>>> +       return enc_to_dig_port(&connector->encoder->base);
>>>>> +}
>>>>> +
>>>>>    /* Implements Part 2 of the HDCP authorization procedure */
>>>>>    static
>>>>> -int intel_hdcp_auth_downstream(struct intel_digital_port
>>>>> *intel_dig_port,
>>>>> -                              const struct intel_hdcp_shim *shim)
>>>>> +int intel_hdcp_auth_downstream(struct intel_connector *connector)
>>>>>    {
>>>>>         struct drm_i915_private *dev_priv;
>>>>>         u32 vprime, sha_text, sha_leftovers, rep_ctl;
>>>>>         u8 bstatus[2], num_downstream, *ksv_fifo;
>>>>>         int ret, i, j, sha_idx;
>>>>> +       const struct intel_hdcp_shim *shim = connector->hdcp_shim;
>>>>> +       struct intel_digital_port *intel_dig_port =
>>>>> conn_to_dig_port(connector);
>>>>> -       dev_priv = intel_dig_port->base.base.dev->dev_private;
>>>>> +       dev_priv = to_i915(connector->base.dev);
>>>>>         ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim);
>>>>>         if (ret) {
>>>>> @@ -385,8 +394,7 @@ int intel_hdcp_auth_downstream(struct
>>>>> intel_digital_port *intel_dig_port,
>>>>>    }
>>>>>    /* Implements Part 1 of the HDCP authorization procedure */
>>>>> -static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
>>>>> -                          const struct intel_hdcp_shim *shim)
>>>>> +static int intel_hdcp_auth(struct intel_connector *connector)
>>>>>    {
>>>>>         struct drm_i915_private *dev_priv;
>>>>>         enum port port;
>>>>> @@ -405,9 +413,10 @@ static int intel_hdcp_auth(struct
>>>>> intel_digital_port *intel_dig_port,
>>>>>                 u8 shim[DRM_HDCP_RI_LEN];
>>>>>         } ri;
>>>>>         bool repeater_present;
>>>>> +       const struct intel_hdcp_shim *shim = connector->hdcp_shim;
>>>>> +       struct intel_digital_port *intel_dig_port =
>>>>> conn_to_dig_port(connector);
>>>>> -       dev_priv = intel_dig_port->base.base.dev->dev_private;
>>>>> -
>>>>> +       dev_priv = to_i915(connector->base.dev);
>>>>>         port = intel_dig_port->base.port;
>>>>>         /* Initialize An with 2 random values and acquire it */
>>>>> @@ -498,19 +507,18 @@ static int intel_hdcp_auth(struct
>>>>> intel_digital_port *intel_dig_port,
>>>>>          * on those as well.
>>>>>          */
>>>>> -       if (repeater_present)
>>>>> -               return intel_hdcp_auth_downstream(intel_dig_port,
>>>>> shim);
>>>>> +       if (repeater_present) {
>>>>> +               ret = intel_hdcp_auth_downstream(connector);
>>>>> +               if (ret) {
>>>>> +                       _intel_hdcp_disable(connector);
>>>>
>>>> If you just call this from _intel_hdcp_enable when intel_hdcp_auth()
>>>> fails, you
>>>> can avoid all of this code, it'd just be one line.
>>>
>>> Yes. Thought about that option too. Actually I would prefer having the
>>> reference of intel_connector untill generic hdcp auth implementation.
>>> we can pass the intel digital port for encoder specific shims.
>>>
>>> This will help me with
>>>      providing the connector details for state transitions as in next
>>> patches
>>>      next steps like SRM passing in for revocation and downstream
>>> topology
>>> gathering (perhaps will start with availability of complementing user
>>> space)
>>>
>>> At this point if we dont want this code, I will disable hdcp from enable
>>> functions.
>>
>> Yes please. Let's not try to predict the future and keep things as simple
>> as
>> possible.
>>
>> Sean
>
> but patch 3(adding connector info into hdcp state transition debug logs)
> still needs conenctor ref at auth.
> Do we still want to drop the connector ref passing? If so we will lose patch
> 3 too.

I was thinking about this further, and it doesn't really make sense to
change some of the messages and not all of them. So perhaps
middle-of-the-road solution would be to add a new DEBUG_KMS message at
the beginning of both  _intel_hdcp_enable and _intel_hdcp_disable
listing the connector name/id and reporting that hdcp is being
enabled/disabled. That way anything that follows can be attributed to
that connector. This also avoids having to shuffle the arguments
around.

Sean

>
> -Ram
>
>
>>
>>> -Ram
>>>>
>>>>
>>>>> +                       return ret;
>>>>> +               }
>>>>> +       }
>>>>>         DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n");
>>>>>         return 0;
>>>>>    }
>>>>> -static
>>>>> -struct intel_digital_port *conn_to_dig_port(struct intel_connector
>>>>> *connector)
>>>>> -{
>>>>> -       return
>>>>> enc_to_dig_port(&intel_attached_encoder(&connector->base)->base);
>>>>> -}
>>>>> -
>>>>>    static int _intel_hdcp_disable(struct intel_connector *connector)
>>>>>    {
>>>>>         struct drm_i915_private *dev_priv =
>>>>> connector->base.dev->dev_private;
>>>>> @@ -558,8 +566,7 @@ static int _intel_hdcp_enable(struct
>>>>> intel_connector *connector)
>>>>>                 return ret;
>>>>>         }
>>>>> -       ret = intel_hdcp_auth(conn_to_dig_port(connector),
>>>>> -                             connector->hdcp_shim);
>>>>> +       ret = intel_hdcp_auth(connector);
>>>>>         if (ret) {
>>>>>                 DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret);
>>>>>                 return ret;
>>>>> --
>>>>> 2.7.4
>>>>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/8] drm/i915: Optimize HDCP key load

[Sean Paul]

On Fri, Feb 2, 2018 at 9:33 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
>
>
> On Friday 02 February 2018 07:48 PM, Sean Paul wrote:
>>
>> On Fri, Feb 02, 2018 at 04:15:17PM +0530, Ramalingam C wrote:
>>>
>>> HDCP key need not be cleared on each hdcp disable. And HDCP key Load
>>> is skipped if key is already loaded.
>>>
>> I had previously encountered issues without clearing the key in my
>> testing.
>> IIRC, without clearing the keys things acted differently. How much time
>> are we
>> saving by optimizing this?
>>
>> Sean
>
> Time profiling is not done. As per the Bspec, Once Keys are loaded, they
> will be cleared only when PG1/PG0 is off.
> So on Resume we need to load the keys. Since we have the status register for
> key load state, I feel we could rely on them.
>
> Now with our code state, I am not seeing the need for reloading the keys. I
> have tested on KBL.
> I think this is worth an attempt as no failures are observed.
>

Alright, sounds good to me, thanks for explaining!

Reviewed-by: Sean Paul <seanpaul@chromium.org>



> --Ram
>
>>
>>
>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_hdcp.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c
>>> b/drivers/gpu/drm/i915/intel_hdcp.c
>>> index fa2e7c727d00..5de9afd275b2 100644
>>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>>> @@ -51,6 +51,10 @@ static int intel_hdcp_load_keys(struct
>>> drm_i915_private *dev_priv)
>>>         int ret;
>>>         u32 val;
>>>   +     val = I915_READ(HDCP_KEY_STATUS);
>>> +       if ((val & HDCP_KEY_LOAD_DONE) && (val & HDCP_KEY_LOAD_STATUS))
>>> +               return 0;
>>> +
>>>         /*
>>>          * On HSW and BDW HW loads the HDCP1.4 Key when Display comes
>>>          * out of reset. So if Key is not already loaded, its an error
>>> state.
>>> @@ -542,8 +546,6 @@ static int _intel_hdcp_disable(struct intel_connector
>>> *connector)
>>>                 return -ETIMEDOUT;
>>>         }
>>>   -     intel_hdcp_clear_keys(dev_priv);
>>> -
>>>         ret = connector->hdcp_shim->toggle_signalling(intel_dig_port,
>>> false);
>>>         if (ret) {
>>>                 DRM_ERROR("Failed to disable HDCP signalling\n");
>>> --
>>> 2.7.4
>>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915: Handle failure from 2nd stage HDCP auth

[Ramalingam C]

On Friday 02 February 2018 08:52 PM, Sean Paul wrote:
> On Fri, Feb 2, 2018 at 9:51 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
>>
>> On Friday 02 February 2018 08:15 PM, Sean Paul wrote:
>>> On Fri, Feb 02, 2018 at 07:52:24PM +0530, Ramalingam C wrote:
>>>>
>>>> On Friday 02 February 2018 07:39 PM, Sean Paul wrote:
>>>>> On Fri, Feb 02, 2018 at 04:15:13PM +0530, Ramalingam C wrote:
>>>>>> We enable the HDCP encryption as a part of first stage authentication.
>>>>>> So when second stage authentication fails, we need to disable the HDCP
>>>>>> encryption and signalling.
>>>>>>
>>>>>> This patch handles above requirement.
>>>>>>
>>>>>> For reusing the _intel_hdcp_disable from generic authentication flow,
>>>>>> this patch retain the reference to connector till the generic hdcp
>>>>>> flow.
>>>>>> This will be handy to provide the connector details in HDCP enable
>>>>>> debug info.
>>>>>>
>>>>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/i915/intel_hdcp.c | 41
>>>>>> +++++++++++++++++++++++----------------
>>>>>>     1 file changed, 24 insertions(+), 17 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c
>>>>>> b/drivers/gpu/drm/i915/intel_hdcp.c
>>>>>> index b97184eccd9c..0021511ae4d7 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>>>>>> @@ -16,6 +16,8 @@
>>>>>>     #define KEY_LOAD_TRIES       5
>>>>>> +static int _intel_hdcp_disable(struct intel_connector *connector);
>>>>>> +
>>>>>>     static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port
>>>>>> *intel_dig_port,
>>>>>>                                      const struct intel_hdcp_shim *shim)
>>>>>>     {
>>>>>> @@ -138,17 +140,24 @@ bool intel_hdcp_is_ksv_valid(u8 *ksv)
>>>>>>          return true;
>>>>>>     }
>>>>>> +static
>>>>>> +struct intel_digital_port *conn_to_dig_port(struct intel_connector
>>>>>> *connector)
>>>>>> +{
>>>>>> +       return enc_to_dig_port(&connector->encoder->base);
>>>>>> +}
>>>>>> +
>>>>>>     /* Implements Part 2 of the HDCP authorization procedure */
>>>>>>     static
>>>>>> -int intel_hdcp_auth_downstream(struct intel_digital_port
>>>>>> *intel_dig_port,
>>>>>> -                              const struct intel_hdcp_shim *shim)
>>>>>> +int intel_hdcp_auth_downstream(struct intel_connector *connector)
>>>>>>     {
>>>>>>          struct drm_i915_private *dev_priv;
>>>>>>          u32 vprime, sha_text, sha_leftovers, rep_ctl;
>>>>>>          u8 bstatus[2], num_downstream, *ksv_fifo;
>>>>>>          int ret, i, j, sha_idx;
>>>>>> +       const struct intel_hdcp_shim *shim = connector->hdcp_shim;
>>>>>> +       struct intel_digital_port *intel_dig_port =
>>>>>> conn_to_dig_port(connector);
>>>>>> -       dev_priv = intel_dig_port->base.base.dev->dev_private;
>>>>>> +       dev_priv = to_i915(connector->base.dev);
>>>>>>          ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim);
>>>>>>          if (ret) {
>>>>>> @@ -385,8 +394,7 @@ int intel_hdcp_auth_downstream(struct
>>>>>> intel_digital_port *intel_dig_port,
>>>>>>     }
>>>>>>     /* Implements Part 1 of the HDCP authorization procedure */
>>>>>> -static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
>>>>>> -                          const struct intel_hdcp_shim *shim)
>>>>>> +static int intel_hdcp_auth(struct intel_connector *connector)
>>>>>>     {
>>>>>>          struct drm_i915_private *dev_priv;
>>>>>>          enum port port;
>>>>>> @@ -405,9 +413,10 @@ static int intel_hdcp_auth(struct
>>>>>> intel_digital_port *intel_dig_port,
>>>>>>                  u8 shim[DRM_HDCP_RI_LEN];
>>>>>>          } ri;
>>>>>>          bool repeater_present;
>>>>>> +       const struct intel_hdcp_shim *shim = connector->hdcp_shim;
>>>>>> +       struct intel_digital_port *intel_dig_port =
>>>>>> conn_to_dig_port(connector);
>>>>>> -       dev_priv = intel_dig_port->base.base.dev->dev_private;
>>>>>> -
>>>>>> +       dev_priv = to_i915(connector->base.dev);
>>>>>>          port = intel_dig_port->base.port;
>>>>>>          /* Initialize An with 2 random values and acquire it */
>>>>>> @@ -498,19 +507,18 @@ static int intel_hdcp_auth(struct
>>>>>> intel_digital_port *intel_dig_port,
>>>>>>           * on those as well.
>>>>>>           */
>>>>>> -       if (repeater_present)
>>>>>> -               return intel_hdcp_auth_downstream(intel_dig_port,
>>>>>> shim);
>>>>>> +       if (repeater_present) {
>>>>>> +               ret = intel_hdcp_auth_downstream(connector);
>>>>>> +               if (ret) {
>>>>>> +                       _intel_hdcp_disable(connector);
>>>>> If you just call this from _intel_hdcp_enable when intel_hdcp_auth()
>>>>> fails, you
>>>>> can avoid all of this code, it'd just be one line.
>>>> Yes. Thought about that option too. Actually I would prefer having the
>>>> reference of intel_connector untill generic hdcp auth implementation.
>>>> we can pass the intel digital port for encoder specific shims.
>>>>
>>>> This will help me with
>>>>       providing the connector details for state transitions as in next
>>>> patches
>>>>       next steps like SRM passing in for revocation and downstream
>>>> topology
>>>> gathering (perhaps will start with availability of complementing user
>>>> space)
>>>>
>>>> At this point if we dont want this code, I will disable hdcp from enable
>>>> functions.
>>> Yes please. Let's not try to predict the future and keep things as simple
>>> as
>>> possible.
>>>
>>> Sean
>> but patch 3(adding connector info into hdcp state transition debug logs)
>> still needs conenctor ref at auth.
>> Do we still want to drop the connector ref passing? If so we will lose patch
>> 3 too.
> I was thinking about this further, and it doesn't really make sense to
> change some of the messages and not all of them. So perhaps
> middle-of-the-road solution would be to add a new DEBUG_KMS message at
> the beginning of both  _intel_hdcp_enable and _intel_hdcp_disable
> listing the connector name/id and reporting that hdcp is being
> enabled/disabled. That way anything that follows can be attributed to
> that connector. This also avoids having to shuffle the arguments
> around.
>
> Sean
Ok I will add that as v2 of patch 3.

--Ram
>
>> -Ram
>>
>>
>>>> -Ram
>>>>>
>>>>>> +                       return ret;
>>>>>> +               }
>>>>>> +       }
>>>>>>          DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n");
>>>>>>          return 0;
>>>>>>     }
>>>>>> -static
>>>>>> -struct intel_digital_port *conn_to_dig_port(struct intel_connector
>>>>>> *connector)
>>>>>> -{
>>>>>> -       return
>>>>>> enc_to_dig_port(&intel_attached_encoder(&connector->base)->base);
>>>>>> -}
>>>>>> -
>>>>>>     static int _intel_hdcp_disable(struct intel_connector *connector)
>>>>>>     {
>>>>>>          struct drm_i915_private *dev_priv =
>>>>>> connector->base.dev->dev_private;
>>>>>> @@ -558,8 +566,7 @@ static int _intel_hdcp_enable(struct
>>>>>> intel_connector *connector)
>>>>>>                  return ret;
>>>>>>          }
>>>>>> -       ret = intel_hdcp_auth(conn_to_dig_port(connector),
>>>>>> -                             connector->hdcp_shim);
>>>>>> +       ret = intel_hdcp_auth(connector);
>>>>>>          if (ret) {
>>>>>>                  DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret);
>>>>>>                  return ret;
>>>>>> --
>>>>>> 2.7.4
>>>>>>

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

Re: [PATCH 6/8] drm/i915: Detect panel's hdcp capability

[Sean Paul]

On Fri, Feb 02, 2018 at 08:08:44PM +0530, Ramalingam C wrote:
> 
> 
> On Friday 02 February 2018 07:54 PM, Sean Paul wrote:
> > On Fri, Feb 02, 2018 at 04:15:18PM +0530, Ramalingam C wrote:
> > > As a first step of HDCP authentication detects the panel's HDCP
> > > capability. This is mandated for DP HDCP1.4.
> > > 
> > > For DP 0th Bit of Bcaps register indicates the panel's hdcp capability
> > > For HDMI valid BKSV indicates the panel's hdcp capability.
> > We're already checking valid bksv, so there's no need to expose that function and
> > call it twice for the HDMI case, hdcp_capable should just always be true.
> we can make the hdmi's hdcp_capable as dummy. As hdcp1.4 spec for hdmi says
> panel's capability is determined by valid bksv.
> Which we are already doing it. no sequence mandated.
> > 
> > For DP, we read and check the bksv , so what do non-capable displays return for
> > the bksv?
> But for DP HDCP1.4 spec mandates that An write should entertained only after
> verifying the 0th bit(hdcp_capable) of BCSPS register.
> Else all DP compliance tests will fail :(
> 
> We are not addressing any functional issue but deviation from the HDCP1.4
> spec for DP.

Hmm, that's lame. Could you please add this to the commit message so that it's
clear that BCAPS must be checked before An is written?

> 
> --Ram
> > 
> > Sean
> > 
> > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_dp.c   | 19 +++++++++++++++++++
> > >   drivers/gpu/drm/i915/intel_drv.h  |  5 +++++
> > >   drivers/gpu/drm/i915/intel_hdcp.c | 10 ++++++++--
> > >   drivers/gpu/drm/i915/intel_hdmi.c | 17 +++++++++++++++++
> > >   4 files changed, 49 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 03d86ff9b805..2623b2beda1a 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -5218,6 +5218,24 @@ bool intel_dp_hdcp_check_link(struct intel_digital_port *intel_dig_port)
> > >   	return !(bstatus & (DP_BSTATUS_LINK_FAILURE | DP_BSTATUS_REAUTH_REQ));
> > >   }
> > > +static
> > > +int intel_dp_hdcp_capable(struct intel_digital_port *intel_dig_port,
> > > +			  bool *hdcp_capable)
> > > +{
> > > +	ssize_t ret;
> > > +	u8 bcaps;
> > > +
> > > +	ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BCAPS,
> > > +			       &bcaps, 1);
> > > +	if (ret != 1) {
> > > +		DRM_ERROR("Read bcaps from DP/AUX failed (%zd)\n", ret);
> > > +		return ret >= 0 ? -EIO : ret;
> > > +	}

This is duplicated in repeater_present. Please pull it out into a helper
function.


> > > +
> > > +	*hdcp_capable = bcaps & DP_BCAPS_HDCP_CAPABLE;
> > > +	return 0;
> > > +}
> > > +
> > >   static const struct intel_hdcp_shim intel_dp_hdcp_shim = {
> > >   	.write_an_aksv = intel_dp_hdcp_write_an_aksv,
> > >   	.read_bksv = intel_dp_hdcp_read_bksv,
> > > @@ -5229,6 +5247,7 @@ static const struct intel_hdcp_shim intel_dp_hdcp_shim = {
> > >   	.read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
> > >   	.toggle_signalling = intel_dp_hdcp_toggle_signalling,
> > >   	.check_link = intel_dp_hdcp_check_link,
> > > +	.hdcp_capable = intel_dp_hdcp_capable,
> > >   };
> > >   static void intel_edp_panel_vdd_sanitize(struct intel_dp *intel_dp)
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index d6a808374dfb..409aee9010ba 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -369,6 +369,10 @@ struct intel_hdcp_shim {
> > >   	/* Ensures the link is still protected */
> > >   	bool (*check_link)(struct intel_digital_port *intel_dig_port);
> > > +
> > > +	/* Detects panel's hdcp capablility */

s/capablility/capability/

Also mention that this is optional for HDMI.


> > > +	int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
> > > +			    bool *hdcp_capable);
> > >   };
> > >   struct intel_connector {
> > > @@ -1848,6 +1852,7 @@ int intel_hdcp_enable(struct intel_connector *connector);
> > >   int intel_hdcp_disable(struct intel_connector *connector);
> > >   int intel_hdcp_check_link(struct intel_connector *connector);
> > >   bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port);
> > > +bool intel_hdcp_is_ksv_valid(u8 *ksv);
> > >   /* intel_psr.c */
> > >   #define CAN_PSR(dev_priv) (HAS_PSR(dev_priv) && dev_priv->psr.sink_support)
> > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > > index 5de9afd275b2..a3463557f0f6 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > @@ -132,7 +132,6 @@ u32 intel_hdcp_get_repeater_ctl(struct intel_digital_port *intel_dig_port)
> > >   	return -EINVAL;
> > >   }
> > > -static
> > >   bool intel_hdcp_is_ksv_valid(u8 *ksv)
> > >   {
> > >   	int i, ones = 0;
> > > @@ -418,13 +417,20 @@ static int intel_hdcp_auth(struct intel_connector *connector)
> > >   		u32 reg;
> > >   		u8 shim[DRM_HDCP_RI_LEN];
> > >   	} ri;
> > > -	bool repeater_present;
> > > +	bool repeater_present, hdcp_capable;
> > >   	const struct intel_hdcp_shim *shim = connector->hdcp_shim;
> > >   	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
> > >   	dev_priv = to_i915(connector->base.dev);
> > >   	port = intel_dig_port->base.port;
> > > +	/* Detect whether panel is HDCP capable or not */

Please change this to the following so it's very clear why this is required:

        /*
         * Detects whether the display is HDCP capable. Although we check for
         * valid Bksv below, the HDCP over DP spec requires that we check
         * whether the display supports HDCP before we write Aksv. For HDMI
         * displays, this is not necessary.
         */
> > > +	ret = shim->hdcp_capable(intel_dig_port, &hdcp_capable);

Check if hdcp_capable is NULL before calling this, that way you don't need to
stub it out for hdmi


> > > +	if (ret)
> > > +		return ret;
> > > +	if (!hdcp_capable)
> > > +		return -EINVAL;

Please print something here so it's obvious why HDCP auth did not succeed.

Sean

> > > +
> > >   	/* Initialize An with 2 random values and acquire it */
> > >   	for (i = 0; i < 2; i++)
> > >   		I915_WRITE(PORT_HDCP_ANINIT(port), get_random_u32());
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index f5d7bfb43006..dfca361ebb24 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -1106,6 +1106,22 @@ bool intel_hdmi_hdcp_check_link(struct intel_digital_port *intel_dig_port)
> > >   	return true;
> > >   }
> > > +static
> > > +int intel_hdmi_hdcp_capable(struct intel_digital_port *intel_dig_port,
> > > +			    bool *hdcp_capable)
> > > +{
> > > +	u8 bksv[5];
> > > +	int ret, retry = 1;
> > > +
> > > +	do {
> > > +		ret = intel_hdmi_hdcp_read_bksv(intel_dig_port, bksv);
> > > +		if (!ret)
> > > +			*hdcp_capable = intel_hdcp_is_ksv_valid(bksv);
> > > +	} while (!(*hdcp_capable) && retry--);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >   static const struct intel_hdcp_shim intel_hdmi_hdcp_shim = {
> > >   	.write_an_aksv = intel_hdmi_hdcp_write_an_aksv,
> > >   	.read_bksv = intel_hdmi_hdcp_read_bksv,
> > > @@ -1117,6 +1133,7 @@ static const struct intel_hdcp_shim intel_hdmi_hdcp_shim = {
> > >   	.read_v_prime_part = intel_hdmi_hdcp_read_v_prime_part,
> > >   	.toggle_signalling = intel_hdmi_hdcp_toggle_signalling,
> > >   	.check_link = intel_hdmi_hdcp_check_link,
> > > +	.hdcp_capable = intel_hdmi_hdcp_capable,
> > >   };
> > >   static void intel_hdmi_prepare(struct intel_encoder *encoder,
> > > -- 
> > > 2.7.4
> > > 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx