diff mbox series

[02/11] drm/i915: Intercept Aksv writes in the aux hooks

Message ID 20191203173638.94919-3-sean@poorly.run (mailing list archive)
State New, archived
Headers show
Series drm/i915: Add support for HDCP 1.4 over MST connectors | expand

Commit Message

Sean Paul Dec. 3, 2019, 5:36 p.m. UTC
From: Sean Paul <seanpaul@chromium.org>

Instead of hand rolling the transfer ourselves in the hdcp hook, inspect
aux messages and add the aksv flag in the aux transfer hook.

IIRC, this was the original implementation and folks wanted this hack to
be isolated to the hdcp code, which makes sense.

However in testing an LG monitor on my desk, I noticed it was passing
back a DEFER reply. This wasn't handled in our hand-rolled code and HDCP
auth was failing as a result. Instead of copy/pasting all of the retry
logic and delays from drm dp helpers, let's just use the helpers and hide
the aksv select as best as we can.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 64 ++++++++++++-------------
 1 file changed, 31 insertions(+), 33 deletions(-)

Comments

Ville Syrjälä Dec. 5, 2019, 7:32 p.m. UTC | #1
On Tue, Dec 03, 2019 at 12:36:25PM -0500, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> Instead of hand rolling the transfer ourselves in the hdcp hook, inspect
> aux messages and add the aksv flag in the aux transfer hook.
> 
> IIRC, this was the original implementation and folks wanted this hack to
> be isolated to the hdcp code, which makes sense.
> 
> However in testing an LG monitor on my desk, I noticed it was passing
> back a DEFER reply. This wasn't handled in our hand-rolled code and HDCP
> auth was failing as a result. Instead of copy/pasting all of the retry
> logic and delays from drm dp helpers, let's just use the helpers and hide
> the aksv select as best as we can.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 64 ++++++++++++-------------
>  1 file changed, 31 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index d958e789ab96..7a407c651fb2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1515,12 +1515,29 @@ intel_dp_aux_header(u8 txbuf[HEADER_SIZE],
>  	txbuf[3] = msg->size - 1;
>  }
>  
> +static u32 intel_dp_aux_generate_xfer_flags(struct drm_dp_aux_msg *msg)
                                               ^
					       const

"_generate_" seems a bit redundant.

> +{
> +	if ((msg->request & ~DP_AUX_I2C_MOT) != DP_AUX_NATIVE_WRITE)
> +		return 0;
> +
> +	/*
> +	 * If we're trying to send the HDCP Aksv, we need to set a the Aksv
> +	 * select bit to inform the hardware to send the Aksv after our header
> +	 * since we can't access that data from software.
> +	 */
> +	if (msg->address == DP_AUX_HDCP_AKSV)
> +		return DP_AUX_CH_CTL_AUX_AKSV_SELECT;

if (DP_AUX_NATIVE_WRITE &&
    DP_AUX_HDCP_AKSV)
    return ...;

would be a little more clear to me since the two things are related here.

> +
> +	return 0;
> +}
> +
>  static ssize_t
>  intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  {
>  	struct intel_dp *intel_dp = container_of(aux, struct intel_dp, aux);
>  	u8 txbuf[20], rxbuf[20];
>  	size_t txsize, rxsize;
> +	u32 flags = intel_dp_aux_generate_xfer_flags(msg);
>  	int ret;
>  
>  	intel_dp_aux_header(txbuf, msg);
> @@ -1541,7 +1558,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  			memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
>  
>  		ret = intel_dp_aux_xfer(intel_dp, txbuf, txsize,
> -					rxbuf, rxsize, 0);
> +					rxbuf, rxsize, flags);
>  		if (ret > 0) {
>  			msg->reply = rxbuf[0] >> 4;
>  
> @@ -1564,7 +1581,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  			return -E2BIG;
>  
>  		ret = intel_dp_aux_xfer(intel_dp, txbuf, txsize,
> -					rxbuf, rxsize, 0);
> +					rxbuf, rxsize, flags);
>  		if (ret > 0) {
>  			msg->reply = rxbuf[0] >> 4;
>  			/*
> @@ -5858,17 +5875,9 @@ static
>  int intel_dp_hdcp_write_an_aksv(struct intel_digital_port *intel_dig_port,
>  				u8 *an)
>  {
> -	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_dig_port->base.base);
> -	static const struct drm_dp_aux_msg msg = {
> -		.request = DP_AUX_NATIVE_WRITE,
> -		.address = DP_AUX_HDCP_AKSV,
> -		.size = DRM_HDCP_KSV_LEN,
> -	};
> -	u8 txbuf[HEADER_SIZE + DRM_HDCP_KSV_LEN] = {}, rxbuf[2], reply = 0;
> +	u8 txbuf[DRM_HDCP_KSV_LEN] = {};

s/txbuf/ksv/ or something maybe?

Looks reasonable to me
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	ssize_t dpcd_ret;
> -	int ret;
>  
> -	/* Output An first, that's easy */
>  	dpcd_ret = drm_dp_dpcd_write(&intel_dig_port->dp.aux, DP_AUX_HDCP_AN,
>  				     an, DRM_HDCP_AN_LEN);
>  	if (dpcd_ret != DRM_HDCP_AN_LEN) {
> @@ -5878,29 +5887,18 @@ int intel_dp_hdcp_write_an_aksv(struct intel_digital_port *intel_dig_port,
>  	}
>  
>  	/*
> -	 * Since Aksv is Oh-So-Secret, we can't access it in software. So in
> -	 * order to get it on the wire, we need to create the AUX header as if
> -	 * we were writing the data, and then tickle the hardware to output the
> -	 * data once the header is sent out.
> +	 * Since Aksv is Oh-So-Secret, we can't access it in software. So we
> +	 * send an empty buffer of the correct length through the DP helpers. On
> +	 * the other side, in the transfer hook, we'll generate a flag based on
> +	 * the destination address which will tickle the hardware to output the
> +	 * Aksv on our behalf after the header is sent.
>  	 */
> -	intel_dp_aux_header(txbuf, &msg);
> -
> -	ret = intel_dp_aux_xfer(intel_dp, txbuf, HEADER_SIZE + msg.size,
> -				rxbuf, sizeof(rxbuf),
> -				DP_AUX_CH_CTL_AUX_AKSV_SELECT);
> -	if (ret < 0) {
> -		DRM_DEBUG_KMS("Write Aksv over DP/AUX failed (%d)\n", ret);
> -		return ret;
> -	} else if (ret == 0) {
> -		DRM_DEBUG_KMS("Aksv write over DP/AUX was empty\n");
> -		return -EIO;
> -	}
> -
> -	reply = (rxbuf[0] >> 4) & DP_AUX_NATIVE_REPLY_MASK;
> -	if (reply != DP_AUX_NATIVE_REPLY_ACK) {
> -		DRM_DEBUG_KMS("Aksv write: no DP_AUX_NATIVE_REPLY_ACK %x\n",
> -			      reply);
> -		return -EIO;
> +	dpcd_ret = drm_dp_dpcd_write(&intel_dig_port->dp.aux, DP_AUX_HDCP_AKSV,
> +				     txbuf, DRM_HDCP_KSV_LEN);
> +	if (dpcd_ret != DRM_HDCP_KSV_LEN) {
> +		DRM_DEBUG_KMS("Failed to write Aksv over DP/AUX (%zd)\n",
> +			      dpcd_ret);
> +		return dpcd_ret >= 0 ? -EIO : dpcd_ret;
>  	}
>  	return 0;
>  }
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index d958e789ab96..7a407c651fb2 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1515,12 +1515,29 @@  intel_dp_aux_header(u8 txbuf[HEADER_SIZE],
 	txbuf[3] = msg->size - 1;
 }
 
+static u32 intel_dp_aux_generate_xfer_flags(struct drm_dp_aux_msg *msg)
+{
+	if ((msg->request & ~DP_AUX_I2C_MOT) != DP_AUX_NATIVE_WRITE)
+		return 0;
+
+	/*
+	 * If we're trying to send the HDCP Aksv, we need to set a the Aksv
+	 * select bit to inform the hardware to send the Aksv after our header
+	 * since we can't access that data from software.
+	 */
+	if (msg->address == DP_AUX_HDCP_AKSV)
+		return DP_AUX_CH_CTL_AUX_AKSV_SELECT;
+
+	return 0;
+}
+
 static ssize_t
 intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 {
 	struct intel_dp *intel_dp = container_of(aux, struct intel_dp, aux);
 	u8 txbuf[20], rxbuf[20];
 	size_t txsize, rxsize;
+	u32 flags = intel_dp_aux_generate_xfer_flags(msg);
 	int ret;
 
 	intel_dp_aux_header(txbuf, msg);
@@ -1541,7 +1558,7 @@  intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 			memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
 
 		ret = intel_dp_aux_xfer(intel_dp, txbuf, txsize,
-					rxbuf, rxsize, 0);
+					rxbuf, rxsize, flags);
 		if (ret > 0) {
 			msg->reply = rxbuf[0] >> 4;
 
@@ -1564,7 +1581,7 @@  intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 			return -E2BIG;
 
 		ret = intel_dp_aux_xfer(intel_dp, txbuf, txsize,
-					rxbuf, rxsize, 0);
+					rxbuf, rxsize, flags);
 		if (ret > 0) {
 			msg->reply = rxbuf[0] >> 4;
 			/*
@@ -5858,17 +5875,9 @@  static
 int intel_dp_hdcp_write_an_aksv(struct intel_digital_port *intel_dig_port,
 				u8 *an)
 {
-	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_dig_port->base.base);
-	static const struct drm_dp_aux_msg msg = {
-		.request = DP_AUX_NATIVE_WRITE,
-		.address = DP_AUX_HDCP_AKSV,
-		.size = DRM_HDCP_KSV_LEN,
-	};
-	u8 txbuf[HEADER_SIZE + DRM_HDCP_KSV_LEN] = {}, rxbuf[2], reply = 0;
+	u8 txbuf[DRM_HDCP_KSV_LEN] = {};
 	ssize_t dpcd_ret;
-	int ret;
 
-	/* Output An first, that's easy */
 	dpcd_ret = drm_dp_dpcd_write(&intel_dig_port->dp.aux, DP_AUX_HDCP_AN,
 				     an, DRM_HDCP_AN_LEN);
 	if (dpcd_ret != DRM_HDCP_AN_LEN) {
@@ -5878,29 +5887,18 @@  int intel_dp_hdcp_write_an_aksv(struct intel_digital_port *intel_dig_port,
 	}
 
 	/*
-	 * Since Aksv is Oh-So-Secret, we can't access it in software. So in
-	 * order to get it on the wire, we need to create the AUX header as if
-	 * we were writing the data, and then tickle the hardware to output the
-	 * data once the header is sent out.
+	 * Since Aksv is Oh-So-Secret, we can't access it in software. So we
+	 * send an empty buffer of the correct length through the DP helpers. On
+	 * the other side, in the transfer hook, we'll generate a flag based on
+	 * the destination address which will tickle the hardware to output the
+	 * Aksv on our behalf after the header is sent.
 	 */
-	intel_dp_aux_header(txbuf, &msg);
-
-	ret = intel_dp_aux_xfer(intel_dp, txbuf, HEADER_SIZE + msg.size,
-				rxbuf, sizeof(rxbuf),
-				DP_AUX_CH_CTL_AUX_AKSV_SELECT);
-	if (ret < 0) {
-		DRM_DEBUG_KMS("Write Aksv over DP/AUX failed (%d)\n", ret);
-		return ret;
-	} else if (ret == 0) {
-		DRM_DEBUG_KMS("Aksv write over DP/AUX was empty\n");
-		return -EIO;
-	}
-
-	reply = (rxbuf[0] >> 4) & DP_AUX_NATIVE_REPLY_MASK;
-	if (reply != DP_AUX_NATIVE_REPLY_ACK) {
-		DRM_DEBUG_KMS("Aksv write: no DP_AUX_NATIVE_REPLY_ACK %x\n",
-			      reply);
-		return -EIO;
+	dpcd_ret = drm_dp_dpcd_write(&intel_dig_port->dp.aux, DP_AUX_HDCP_AKSV,
+				     txbuf, DRM_HDCP_KSV_LEN);
+	if (dpcd_ret != DRM_HDCP_KSV_LEN) {
+		DRM_DEBUG_KMS("Failed to write Aksv over DP/AUX (%zd)\n",
+			      dpcd_ret);
+		return dpcd_ret >= 0 ? -EIO : dpcd_ret;
 	}
 	return 0;
 }