| Message ID | 20200429195502.39919-5-sean@poorly.run (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Add support for HDCP 1.4 over MST connectors | expand |
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 >
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; }