On Tue, Dec 03, 2019 at 12:36:25PM -0500, Sean Paul wrote:
> From: Sean Paul <seanpaul@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
