On Thursday 29 March 2018 08:05 PM, Sean Paul wrote:

On Thu, Mar 29, 2018 at 07:39:05PM +0530, Ramalingam C wrote:

...

As per DP spec when R0 mismatch is detected, HDCP source supported re-read the R0 atleast twice.

And For HDMI and DP minimum wait required for the R0 availability is 100mSec. So this patch changes the wait time to 100mSec but retries twice with the time interval of 100mSec for each attempt.

This patch is needed for DP HDCP1.4 CTS Test: 1A-06.

Signed-off-by: Ramalingam C ramalingam.c@intel.com

drivers/gpu/drm/i915/intel_hdcp.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index 14ca5d3057a7..96b9025dc759 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -496,9 +496,11 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, }

/*

• * Wait for R0' to become available. The spec says 100ms from Aksv, but
  

• * some monitors can take longer than this. We'll set the timeout at
  

• * 300ms just to be sure.
  

• * Wait for R0' to become available. The spec says minimum 100ms from
  

• * Aksv, but some monitors can take longer than this. So we are
  

• * combinely waiting for 300mSec just to be sure in case of HDMI.
  

• * DP HDCP Spec mandates the two more reattempt to read R0, incase
  

• * of R0 mismatch.
  

I am sorry to nitpick comments, but this doesn't belong here. Leave this comment alone and add the part about the DP spec requiring retries directly above the loop where we're actually doing the retries.

Ok. I will take care in the next ver.

--Ram

Sean

...

*
* On DP, there's an R0_READY bit available but no such bit
* exists on HDMI. Since the upper-bound is the same, we'll just do

@@ -506,15 +508,21 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, */ wait_remaining_ms_from_jiffies(r0_prime_gen_start, 300);

• ri.reg = 0;
• ret = shim->read_ri_prime(intel_dig_port, ri.shim);
• if (ret)

• return ret;
  

• I915_WRITE(PORT_HDCP_RPRIME(port), ri.reg);

• tries = 3;
• for (i = 0; i < tries; i++) {

• ri.reg = 0;
  

• ret = shim->read_ri_prime(intel_dig_port, ri.shim);
  

• if (ret)
  

• 	return ret;
  

• I915_WRITE(PORT_HDCP_RPRIME(port), ri.reg);
  
  

• /* Wait for Ri prime match */
• if (wait_for(I915_READ(PORT_HDCP_STATUS(port)) &

•      (HDCP_STATUS_RI_MATCH | HDCP_STATUS_ENC), 1)) {
  

• /* Wait for Ri prime match */
  

• if (!wait_for(I915_READ(PORT_HDCP_STATUS(port)) &
  

•     (HDCP_STATUS_RI_MATCH | HDCP_STATUS_ENC), 1))
  

• 	break;
  

• }
• if (i == tries) { DRM_ERROR("Timed out waiting for Ri prime match (%x)\n", I915_READ(PORT_HDCP_STATUS(port))); return -ETIMEDOUT;

-- 2.7.4

On Thu, Mar 29, 2018 at 07:39:06PM +0530, Ramalingam C wrote:

In case of V prime mismatch, DP HDCP spec mandates the re-read of Vprime atleast twice.

This patch needed for DP HDCP1.4 CTS Test: 1B-05.

v2: Moved the V' validation into a function for retry. [Sean Paul]

Signed-off-by: Ramalingam C ramalingam.c@intel.com

drivers/gpu/drm/i915/intel_hdcp.c | 113 +++++++++++++++++++++++--------------- 1 file changed, 70 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index 96b9025dc759..f77d956b2b18 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -142,53 +142,17 @@ bool intel_hdcp_is_ksv_valid(u8 *ksv) return true; }

-/* 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)
  

+static inline

Why inline?

+int intel_hdcp_validate_v_prime(struct intel_digital_port *intel_dig_port,

• 		const struct intel_hdcp_shim *shim,
  

• 		u8 *ksv_fifo, u8 num_downstream, u8 *bstatus)
  

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

  dev_priv = intel_dig_port->base.base.dev->dev_private;

• ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim);

• if (ret) {

• DRM_ERROR("KSV list failed to become ready (%d)\n", ret);
  

• return ret;
  

• }

• ret = shim->read_bstatus(intel_dig_port, bstatus);

• if (ret)

• return ret;
  

• if (DRM_HDCP_MAX_DEVICE_EXCEEDED(bstatus[0]) ||

•    DRM_HDCP_MAX_CASCADE_EXCEEDED(bstatus[1])) {
  

• DRM_ERROR("Max Topology Limit Exceeded\n");
  

• return -EPERM;
  

• }

• /*

• * When repeater reports 0 device count, HDCP1.4 spec allows disabling
  

• * the HDCP encryption. That implies that repeater can't have its own
  

• * display. As there is no consumption of encrypted content in the
  

• * repeater with 0 downstream devices, we are failing the
  

• * authentication.
  

• */
  

• num_downstream = DRM_HDCP_NUM_DOWNSTREAM(bstatus[0]);

• if (num_downstream == 0)

• return -EINVAL;
  

• ksv_fifo = kzalloc(num_downstream * DRM_HDCP_KSV_LEN, GFP_KERNEL);

• if (!ksv_fifo)

• return -ENOMEM;
  

• ret = shim->read_ksv_fifo(intel_dig_port, num_downstream, ksv_fifo);

• if (ret)

• return ret;
  

• /* Process V' values from the receiver */ for (i = 0; i < DRM_HDCP_V_PRIME_NUM_PARTS; i++) { ret = shim->read_v_prime_part(intel_dig_port, i, &vprime);

@@ -353,7 +317,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, return ret; sha_idx += sizeof(sha_text); } else {

• DRM_ERROR("Invalid number of leftovers %d\n", sha_leftovers);
  

• DRM_DEBUG("Invalid number of leftovers %d\n", sha_leftovers);
  

  return -EINVAL; }

@@ -381,14 +345,77 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, if (intel_wait_for_register(dev_priv, HDCP_REP_CTL, HDCP_SHA1_COMPLETE, HDCP_SHA1_COMPLETE, 1)) {

• DRM_ERROR("Timed out waiting for SHA1 complete\n");
  

• DRM_DEBUG("Timed out waiting for SHA1 complete\n");
  

  return -ETIMEDOUT; } if (!(I915_READ(HDCP_REP_CTL) & HDCP_SHA1_V_MATCH)) {

• DRM_ERROR("SHA-1 mismatch, HDCP failed\n");
  

• DRM_DEBUG("SHA-1 mismatch, HDCP failed\n");
  

I think the DEBUG should be DEBUG_KMS, consistent with the rest of the file?

return -ENXIO;

}

• return 0;

+}

+/* 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)
  

+{

• u8 bstatus[2], num_downstream, *ksv_fifo;
• int ret, i, tries = 3;
• ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim);
• if (ret) {

• DRM_ERROR("KSV list failed to become ready (%d)\n", ret);
  

• return ret;
  

• }
• ret = shim->read_bstatus(intel_dig_port, bstatus);
• if (ret)

• return ret;
  

• if (DRM_HDCP_MAX_DEVICE_EXCEEDED(bstatus[0]) ||

•    DRM_HDCP_MAX_CASCADE_EXCEEDED(bstatus[1])) {
  

• DRM_ERROR("Max Topology Limit Exceeded\n");
  

• return -EPERM;
  

• }
• /*

• * When repeater reports 0 device count, HDCP1.4 spec allows disabling
  

• * the HDCP encryption. That implies that repeater can't have its own
  

• * display. As there is no consumption of encrypted content in the
  

• * repeater with 0 downstream devices, we are failing the
  

• * authentication.
  

• */
  

• num_downstream = DRM_HDCP_NUM_DOWNSTREAM(bstatus[0]);
• if (num_downstream == 0)

• return -EINVAL;
  

• ksv_fifo = kzalloc(num_downstream * DRM_HDCP_KSV_LEN, GFP_KERNEL);
• if (!ksv_fifo)

• return -ENOMEM;
  

• ret = shim->read_ksv_fifo(intel_dig_port, num_downstream, ksv_fifo);
• if (ret)

• return ret;
  

• /*

• * When V prime mismatches, DP Spec mandates re-read of
  

• * V prime atleast twice.
  

• */
  

• for (i = 0; i < tries; i++) {

• ret = intel_hdcp_validate_v_prime(intel_dig_port, shim,
  

• 				  ksv_fifo, num_downstream,
  

• 				  bstatus);
  

• if (!ret)
  

• 	break;
  

• }
• if (i == tries) {

• DRM_ERROR("V Prime validation failed.(%d)\n", ret);
  

• return ret;
  

• }
• DRM_DEBUG_KMS("HDCP is enabled (%d downstream devices)\n", num_downstream); return 0;

-- 2.7.4

On Thursday 29 March 2018 07:54 PM, Ville Syrjälä wrote:

On Thu, Mar 29, 2018 at 07:39:07PM +0530, Ramalingam C wrote:

...

HDCP1.4 key can be loaded, only when Power well #1 is enabled and cdclk is enabled. Using the I915 power well infrastruture, above requirement is verified.

This patch enables the hdcp initialization for HSW, BDW, and BXT.

v2: Choose the PW# based on the platform.

Signed-off-by: Ramalingam C ramalingam.c@intel.com Reviewed-by: Sean Paul seanpaul@chromium.org

---

drivers/gpu/drm/i915/intel_hdcp.c | 41 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index f77d956b2b18..d8af09b46a44 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -37,6 +37,43 @@ static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port, return 0; }

+static bool hdcp_key_loadable(struct drm_i915_private *dev_priv) +{

• struct i915_power_domains *power_domains = &dev_priv->power_domains;
• struct i915_power_well *power_well;
• enum i915_power_well_id id;
• bool enabled = false;
• /*

• * On HSW and BDW, Display HW loads the Key as soon as Display resumes.
  

• * On all BXT+, SW can load the keys only when the PW#1 is turned on.
  

• */
  

• if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))

• id = HSW_DISP_PW_GLOBAL;
  

• else

• id = SKL_DISP_PW_1;
  

• mutex_lock(&power_domains->lock);
• /* PG1 (power well #1) needs to be enabled */
• for_each_power_well(dev_priv, power_well) {

• if (power_well->id == id) {
  

• 	enabled = power_well->ops->is_enabled(dev_priv,
  

• 					      power_well);
  

• 	break;
  

• }
  

• }
• mutex_unlock(&power_domains->lock);
• /*

• * Another req for hdcp key loadability is enabled state of pll for
  

• * cdclk. Without active crtc we wont land here. So we are assuming that
  

• * cdclk is already on.
  

• */
  

• return enabled;

+}

• static void intel_hdcp_clear_keys(struct drm_i915_private *dev_priv) { I915_WRITE(HDCP_KEY_CONF, HDCP_CLEAR_KEYS_TRIGGER);

@@ -615,8 +652,8 @@ static int _intel_hdcp_enable(struct intel_connector *connector) DRM_DEBUG_KMS("[%s:%d] HDCP is being enabled...\n", connector->base.name, connector->base.base.id);

• if (!(I915_READ(SKL_FUSE_STATUS) & SKL_FUSE_PG_DIST_STATUS(1))) {

• DRM_ERROR("PG1 is disabled, cannot load keys\n");
  

• if (!hdcp_key_loadable(dev_priv)) {

• DRM_ERROR("HDCP key Load is not possible\n");
  

  return -ENXIO; }

If you need the power well then why aren't you grabbing the correct power domain reference somewhere?

Ville,

As HDCP is supposed to be enabled after the basic display is up, power well #1 is supposed to be enabled. If not that is mostly an error w.r.t HDCP.

So at this point we just want to verify whether required PW is up and HDCP key can be loaded from the HW. Else fail the HDCP request.

--Ram

On Fri, Apr 06, 2018 at 07:02:02PM +0300, Ville Syrjälä wrote:

On Mon, Apr 02, 2018 at 02:35:42PM +0530, Ramalingam C wrote:

...

On Thursday 29 March 2018 07:54 PM, Ville Syrjälä wrote:

...

On Thu, Mar 29, 2018 at 07:39:07PM +0530, Ramalingam C wrote:

...

HDCP1.4 key can be loaded, only when Power well #1 is enabled and cdclk is enabled. Using the I915 power well infrastruture, above requirement is verified.

This patch enables the hdcp initialization for HSW, BDW, and BXT.

v2: Choose the PW# based on the platform.

Signed-off-by: Ramalingam C ramalingam.c@intel.com Reviewed-by: Sean Paul seanpaul@chromium.org

---

drivers/gpu/drm/i915/intel_hdcp.c | 41 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index f77d956b2b18..d8af09b46a44 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -37,6 +37,43 @@ static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port, return 0; }

+static bool hdcp_key_loadable(struct drm_i915_private *dev_priv) +{

• struct i915_power_domains *power_domains = &dev_priv->power_domains;
• struct i915_power_well *power_well;
• enum i915_power_well_id id;
• bool enabled = false;
• /*

• * On HSW and BDW, Display HW loads the Key as soon as Display resumes.
  

• * On all BXT+, SW can load the keys only when the PW#1 is turned on.
  

• */
  

• if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))

• id = HSW_DISP_PW_GLOBAL;
  

• else

• id = SKL_DISP_PW_1;
  

• mutex_lock(&power_domains->lock);
• /* PG1 (power well #1) needs to be enabled */
• for_each_power_well(dev_priv, power_well) {

• if (power_well->id == id) {
  

• 	enabled = power_well->ops->is_enabled(dev_priv,
  

• 					      power_well);
  

• 	break;
  

• }
  

• }
• mutex_unlock(&power_domains->lock);
• /*

• * Another req for hdcp key loadability is enabled state of pll for
  

• * cdclk. Without active crtc we wont land here. So we are assuming that
  

• * cdclk is already on.
  

• */
  

• return enabled;

+}

• static void intel_hdcp_clear_keys(struct drm_i915_private *dev_priv) { I915_WRITE(HDCP_KEY_CONF, HDCP_CLEAR_KEYS_TRIGGER);

@@ -615,8 +652,8 @@ static int _intel_hdcp_enable(struct intel_connector *connector) DRM_DEBUG_KMS("[%s:%d] HDCP is being enabled...\n", connector->base.name, connector->base.base.id);

• if (!(I915_READ(SKL_FUSE_STATUS) & SKL_FUSE_PG_DIST_STATUS(1))) {

• DRM_ERROR("PG1 is disabled, cannot load keys\n");
  

• if (!hdcp_key_loadable(dev_priv)) {

• DRM_ERROR("HDCP key Load is not possible\n");
  

  return -ENXIO; }

If you need the power well then why aren't you grabbing the correct power domain reference somewhere?

Ville,

As HDCP is supposed to be enabled after the basic display is up, power well #1 is supposed to be enabled. If not that is mostly an error w.r.t HDCP.

So at this point we just want to verify whether required PW is up and HDCP key can be loaded from the HW. Else fail the HDCP request.

So this is in practice dead code to deal with a hypothetical bug elsewhere in the driver?

Yeah looks like it should be wrapped in a WARN_ON, or maybe outright thrown out. The unclaimed mmio debug stuff will catch when this happens (or well, should). -Daniel

On Wed, Apr 11, 2018 at 01:57:15PM +0530, Ramalingam C wrote:

On Monday 09 April 2018 01:58 PM, Daniel Vetter wrote:

...

On Fri, Apr 06, 2018 at 07:02:02PM +0300, Ville Syrjälä wrote:

...

On Mon, Apr 02, 2018 at 02:35:42PM +0530, Ramalingam C wrote:

...

On Thursday 29 March 2018 07:54 PM, Ville Syrjälä wrote:

...

On Thu, Mar 29, 2018 at 07:39:07PM +0530, Ramalingam C wrote:

...

HDCP1.4 key can be loaded, only when Power well #1 is enabled and cdclk is enabled. Using the I915 power well infrastruture, above requirement is verified.

This patch enables the hdcp initialization for HSW, BDW, and BXT.

v2: Choose the PW# based on the platform.

Signed-off-by: Ramalingam C ramalingam.c@intel.com Reviewed-by: Sean Paul seanpaul@chromium.org

---

drivers/gpu/drm/i915/intel_hdcp.c | 41 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index f77d956b2b18..d8af09b46a44 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -37,6 +37,43 @@ static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port, return 0; } +static bool hdcp_key_loadable(struct drm_i915_private *dev_priv) +{

• struct i915_power_domains *power_domains = &dev_priv->power_domains;
• struct i915_power_well *power_well;
• enum i915_power_well_id id;
• bool enabled = false;
• /*

• * On HSW and BDW, Display HW loads the Key as soon as Display resumes.
  

• * On all BXT+, SW can load the keys only when the PW#1 is turned on.
  

• */
  

• if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))

• id = HSW_DISP_PW_GLOBAL;
  

• else

• id = SKL_DISP_PW_1;
  

• mutex_lock(&power_domains->lock);
• /* PG1 (power well #1) needs to be enabled */
• for_each_power_well(dev_priv, power_well) {

• if (power_well->id == id) {
  

• 	enabled = power_well->ops->is_enabled(dev_priv,
  

• 					      power_well);
  

• 	break;
  

• }
  

• }
• mutex_unlock(&power_domains->lock);
• /*

• * Another req for hdcp key loadability is enabled state of pll for
  

• * cdclk. Without active crtc we wont land here. So we are assuming that
  

• * cdclk is already on.
  

• */
  

• return enabled;

+}

• static void intel_hdcp_clear_keys(struct drm_i915_private *dev_priv) { I915_WRITE(HDCP_KEY_CONF, HDCP_CLEAR_KEYS_TRIGGER);

@@ -615,8 +652,8 @@ static int _intel_hdcp_enable(struct intel_connector *connector) DRM_DEBUG_KMS("[%s:%d] HDCP is being enabled...\n", connector->base.name, connector->base.base.id);

• if (!(I915_READ(SKL_FUSE_STATUS) & SKL_FUSE_PG_DIST_STATUS(1))) {

• DRM_ERROR("PG1 is disabled, cannot load keys\n");
  

• if (!hdcp_key_loadable(dev_priv)) {

• DRM_ERROR("HDCP key Load is not possible\n");
  return -ENXIO;
  

  }

If you need the power well then why aren't you grabbing the correct power domain reference somewhere?

Ville,

As HDCP is supposed to be enabled after the basic display is up, power well #1 is supposed to be enabled. If not that is mostly an error w.r.t HDCP.

So at this point we just want to verify whether required PW is up and HDCP key can be loaded from the HW. Else fail the HDCP request.

So this is in practice dead code to deal with a hypothetical bug elsewhere in the driver?

Yeah looks like it should be wrapped in a WARN_ON, or maybe outright thrown out. The unclaimed mmio debug stuff will catch when this happens (or well, should). -Daniel

Ok. Intention of this patch was generalizing the existing SKL specific PW check for all hdcp capable intel platforms. Please suggest if this needs to be wrapped into WARN_ON or really want to discard it.

That's not what the commit message says, and since you replace a check for a fuse register with driver-internal checks for power well state, also not what the diff looks like.

In general, we don't do runtime checks for power wells (except for boot-time display state readout, but that's the only case). Instead you need to make sure you're holding the right power well references while using something. Not just while the key is loaded, but the entire time it's in use.

So not really sure what you should be doing, except most likely not this patch here. -Daniel

On Thu, Mar 29, 2018 at 07:39:08PM +0530, Ramalingam C wrote:

In both HDMI and DP, device count is represented by 6:0 bits of a register(BInfo/Bstatus)

So macro for bitmasking the device_count is fixed(0x3F->0x7F).

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: Ramalingam C ramalingam.c@intel.com cc: Sean Paul seanpaul@chromium.org

---

include/drm/drm_hdcp.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h index 562fa7df2637..98e63d870139 100644 --- a/include/drm/drm_hdcp.h +++ b/include/drm/drm_hdcp.h @@ -19,7 +19,7 @@ #define DRM_HDCP_RI_LEN 2 #define DRM_HDCP_V_PRIME_PART_LEN 4 #define DRM_HDCP_V_PRIME_NUM_PARTS 5 -#define DRM_HDCP_NUM_DOWNSTREAM(x) (x & 0x3f) +#define DRM_HDCP_NUM_DOWNSTREAM(x) (x & 0x7f) #define DRM_HDCP_MAX_CASCADE_EXCEEDED(x) (x & BIT(3)) #define DRM_HDCP_MAX_DEVICE_EXCEEDED(x) (x & BIT(7))

-- 2.7.4