Re: [PATCH v6 04/16] drm/i915: Intercept Aksv writes in the aux hooks

[Ramalingam C <ramalingam.c@intel.com>]

On 2020-04-29 at 15:54:50 -0400, 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.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Ramalingam C <ramalingam.c@intel.com>
Just reconfirming my R-b here. LGTM

Reviewed-by: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Link: https://patchwork.freedesktop.org/patch/msgid/20191203173638.94919-3-sean@poorly.run #v1
> Link: https://patchwork.freedesktop.org/patch/msgid/20191212190230.188505-5-sean@poorly.run #v2
> Link: https://patchwork.freedesktop.org/patch/msgid/20200117193103.156821-5-sean@poorly.run #v3
> Link: https://patchwork.freedesktop.org/patch/msgid/20200218220242.107265-5-sean@poorly.run #v4
> Link: https://patchwork.freedesktop.org/patch/msgid/20200305201236.152307-5-sean@poorly.run #v5
> 
> Changes in v2:
> -Remove 'generate' in intel_dp_aux_generate_xfer_flags, make arg const (Ville)
> -Bundle Aksv if statement together (Ville)
> -Rename 'txbuf' to 'aksv' (Ville)
> Changes in v3:
> -None
> Changes in v4:
> -None
> Changes in v5:
> -None
> Changes in v6:
> -None
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 63 ++++++++++++-------------
>  1 file changed, 29 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 6952b0295096..f33b3e273e05 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1550,12 +1550,27 @@ intel_dp_aux_header(u8 txbuf[HEADER_SIZE],
>  	txbuf[3] = msg->size - 1;
>  }
>  
> +static u32 intel_dp_aux_xfer_flags(const struct drm_dp_aux_msg *msg)
> +{
> +	/*
> +	 * 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->request & ~DP_AUX_I2C_MOT) == DP_AUX_NATIVE_WRITE &&
> +	    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_xfer_flags(msg);
>  	int ret;
>  
>  	intel_dp_aux_header(txbuf, msg);
> @@ -1576,7 +1591,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;
>  
> @@ -1599,7 +1614,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;
>  			/*
> @@ -6553,17 +6568,9 @@ int intel_dp_hdcp_write_an_aksv(struct intel_digital_port *intel_dig_port,
>  				u8 *an)
>  {
>  	struct drm_i915_private *i915 = to_i915(intel_dig_port->base.base.dev);
> -	struct intel_dp *intel_dp = enc_to_intel_dp(to_intel_encoder(&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 aksv[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) {
> @@ -6574,31 +6581,19 @@ 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_dbg_kms(&i915->drm,
> -			    "Write Aksv over DP/AUX failed (%d)\n", ret);
> -		return ret;
> -	} else if (ret == 0) {
> -		drm_dbg_kms(&i915->drm, "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) {
> +	dpcd_ret = drm_dp_dpcd_write(&intel_dig_port->dp.aux, DP_AUX_HDCP_AKSV,
> +				     aksv, DRM_HDCP_KSV_LEN);
> +	if (dpcd_ret != DRM_HDCP_KSV_LEN) {
>  		drm_dbg_kms(&i915->drm,
> -			    "Aksv write: no DP_AUX_NATIVE_REPLY_ACK %x\n",
> -			    reply);
> -		return -EIO;
> +			    "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
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel