Message-ID: <2ac3bfaf-8c4f-b5d4-6362-70d802d1b029@intel.com>
Date:   Thu, 7 Dec 2017 10:28:07 +0530
From:   Ramalingam C <ramalingam.c@...el.com>
To:     Sean Paul <seanpaul@...omium.org>, dri-devel@...ts.freedesktop.org,
        intel-gfx@...ts.freedesktop.org
Cc:     linux-kernel@...r.kernel.org, daniel.vetter@...el.com,
        seanpaul@...gle.com, Daniel Vetter <daniel.vetter@...ll.ch>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        David Airlie <airlied@...ux.ie>
Subject: Re: [PATCH v4 9/9] drm/i915: Implement HDCP for DisplayPort

Thanks for handling the reauth req from downstream.

you might want to fix the missed single occurance of "//"


On Thursday 07 December 2017 05:30 AM, Sean Paul wrote:
> This patch adds HDCP support for DisplayPort connectors by implementing
> the intel_hdcp_shim.
>
> Most of this is straightforward read/write from/to DPCD registers. One
> thing worth pointing out is the Aksv output bit. It wasn't easily
> separable like it's HDMI counterpart, so it's crammed in with the rest
> of it.
>
> Changes in v2:
> - Moved intel_hdcp_check_link out of intel_dp_check_link and only call
>    it on short pulse. Since intel_hdcp_check_link does its own locking,
>    this ensures we don't deadlock when intel_dp_check_link is called
>    holding connection_mutex.
> - Rebased on drm-intel-next
> Changes in v3:
> - Initialize new worker
> Changes in v4:
> - Use intel_hdcp_init (Daniel)
> - Check for reauth requests in check_link (Ram)
>
> Cc: Daniel Vetter <daniel.vetter@...ll.ch>
> Cc: Ramalingam C <ramalingam.c@...el.com>
> Signed-off-by: Sean Paul <seanpaul@...omium.org>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 244 ++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 237 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c603d4c903e1..0ec8b23d0332 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -36,7 +36,9 @@
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_crtc.h>
>   #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_dp_helper.h>
>   #include <drm/drm_edid.h>
> +#include <drm/drm_hdcp.h>
>   #include "intel_drv.h"
>   #include <drm/i915_drm.h>
>   #include "i915_drv.h"
> @@ -1025,10 +1027,29 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp,
>   	       DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
>   }
>   
> +static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> +				          bool has_aux_irq,
> +				          int send_bytes,
> +				          uint32_t aux_clock_divider,
> +					  bool aksv_write)
> +{
> +	uint32_t val = 0;
> +
> +	if (aksv_write) {
> +		send_bytes += 5;
> +		val |= DP_AUX_CH_CTL_AUX_AKSV_SELECT;
> +	}
> +
> +	return val | intel_dp->get_aux_send_ctl(intel_dp,
> +						has_aux_irq,
> +						send_bytes,
> +						aux_clock_divider);
> +}
> +
>   static int
>   intel_dp_aux_ch(struct intel_dp *intel_dp,
>   		const uint8_t *send, int send_bytes,
> -		uint8_t *recv, int recv_size)
> +		uint8_t *recv, int recv_size, bool aksv_write)
>   {
>   	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>   	struct drm_i915_private *dev_priv =
> @@ -1088,10 +1109,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>   	}
>   
>   	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
> -		u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
> -							  has_aux_irq,
> -							  send_bytes,
> -							  aux_clock_divider);
> +		u32 send_ctl = intel_dp_get_aux_send_ctl(intel_dp,
> +							 has_aux_irq,
> +							 send_bytes,
> +							 aux_clock_divider,
> +							 aksv_write);
>   
>   		/* Must try at least 3 times according to DP spec */
>   		for (try = 0; try < 5; try++) {
> @@ -1228,7 +1250,8 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>   		if (msg->buffer)
>   			memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
>   
> -		ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
> +		ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize,
> +				      false);
>   		if (ret > 0) {
>   			msg->reply = rxbuf[0] >> 4;
>   
> @@ -1250,7 +1273,8 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>   		if (WARN_ON(rxsize > 20))
>   			return -E2BIG;
>   
> -		ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
> +		ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize,
> +				      false);
>   		if (ret > 0) {
>   			msg->reply = rxbuf[0] >> 4;
>   			/*
> @@ -4981,6 +5005,203 @@ void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)
>   	pps_unlock(intel_dp);
>   }
>   
> +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);
> +	uint8_t txbuf[4], rxbuf[2], reply = 0;
> +	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) {
> +		DRM_ERROR("Failed to write An over DP/AUX (%ld)\n", dpcd_ret);
> +		return dpcd_ret >= 0 ? -EIO : dpcd_ret;
> +	}
> +
> +	/*
> +	 * 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.
> +	 */
> +	txbuf[0] = (DP_AUX_NATIVE_WRITE << 4) |
> +		   ((DP_AUX_HDCP_AKSV >> 16) & 0xf);
> +	txbuf[1] = (DP_AUX_HDCP_AKSV >> 8) & 0xff;
> +	txbuf[2] = DP_AUX_HDCP_AKSV & 0xff;
> +	txbuf[3] = DRM_HDCP_KSV_LEN - 1;
> +
> +	ret = intel_dp_aux_ch(intel_dp, txbuf, sizeof(txbuf), rxbuf,
> +			      sizeof(rxbuf), true);
> +	if (ret < 0) {
> +		DRM_ERROR("Write Aksv over DP/AUX failed (%d)\n", ret);
> +		return ret;
> +	} else if (ret == 0) {
> +		DRM_ERROR("Aksv write over DP/AUX was empty\n");
> +		return -EIO;
> +	}
> +
> +	reply = (rxbuf[0] >> 4) & DP_AUX_NATIVE_REPLY_MASK;
> +	return reply == DP_AUX_NATIVE_REPLY_ACK ? 0 : -EIO;
> +}
> +
> +static int intel_dp_hdcp_read_bksv(struct intel_digital_port *intel_dig_port,
> +				   u8 *bksv)
> +{
> +	ssize_t ret;
> +	ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BKSV, bksv,
> +			       DRM_HDCP_KSV_LEN);
> +	if (ret != DRM_HDCP_KSV_LEN) {
> +		DRM_ERROR("Read Bksv from DP/AUX failed (%ld)\n", ret);
> +		return ret >= 0 ? -EIO : ret;
> +	}
> +	return 0;
> +}
> +
> +static int intel_dp_hdcp_read_bstatus(struct intel_digital_port *intel_dig_port,
> +				      u8 *bstatus)
> +{
> +	ssize_t ret;
> +	/*
> +	 * For some reason the HDMI and DP HDCP specs call this register
> +	 * definition by different names. In the HDMI spec, it's called BSTATUS,
> +	 * but in DP it's called BINFO.
> +	 */
> +	ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BINFO,
> +			       bstatus, DRM_HDCP_BSTATUS_LEN);
> +	if (ret != DRM_HDCP_BSTATUS_LEN) {
> +		DRM_ERROR("Read bstatus from DP/AUX failed (%ld)\n", ret);
> +		return ret >= 0 ? -EIO : ret;
> +	}
> +	return 0;
> +}
> +
> +static
> +int intel_dp_hdcp_repeater_present(struct intel_digital_port *intel_dig_port,
> +				   bool *repeater_present)
> +{
> +	ssize_t ret;
> +	u8 bcaps;
> +	ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BCAPS,
> +			       &bcaps, 1);
> +	if (ret != 1) {
> +		DRM_ERROR("Read bcaps from DP/AUX failed (%ld)\n", ret);
> +		return ret >= 0 ? -EIO : ret;
> +	}
> +	*repeater_present = bcaps & DP_BCAPS_REPEATER_PRESENT;
> +	return 0;
> +}
> +
> +static
> +int intel_dp_hdcp_read_ri_prime(struct intel_digital_port *intel_dig_port,
> +				u8 *ri_prime)
> +{
> +	ssize_t ret;
> +	ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_RI_PRIME,
> +			       ri_prime, DRM_HDCP_RI_LEN);
> +	if (ret != DRM_HDCP_RI_LEN) {
> +		DRM_ERROR("Read Ri' from DP/AUX failed (%ld)\n", ret);
> +		return ret >= 0 ? -EIO : ret;
> +	}
> +	return 0;
> +}
> +
> +static
> +int intel_dp_hdcp_read_ksv_ready(struct intel_digital_port *intel_dig_port,
> +				 bool *ksv_ready)
> +{
> +	ssize_t ret;
> +	u8 bstatus;
> +	ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BSTATUS,
> +			       &bstatus, 1);
> +	if (ret != 1) {
> +		DRM_ERROR("Read bstatus from DP/AUX failed (%ld)\n", ret);
> +		return ret >= 0 ? -EIO : ret;
> +	}
> +	*ksv_ready = bstatus & DP_BSTATUS_READY;
> +	return 0;
> +}
> +
> +static
> +int intel_dp_hdcp_read_ksv_fifo(struct intel_digital_port *intel_dig_port,
> +			        int num_downstream, u8 *ksv_fifo)
> +{
> +	ssize_t ret;
> +	int i;
> +
> +	// KSV list is read via 15 byte window (3 entries @ 5 bytes each)

You might want to fix this too..

With this, patch looks good to me.

Reviewed-by: Ramalingam C <ramalingm.c@...el.com>

Thanks
--Ram
> +	for (i = 0; i < num_downstream; i += 3) {
> +		size_t len = min(num_downstream - i, 3) * DRM_HDCP_KSV_LEN;
> +		ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux,
> +				       DP_AUX_HDCP_KSV_FIFO,
> +				       ksv_fifo + i * DRM_HDCP_KSV_LEN,
> +				       len);
> +		if (ret != len) {
> +			DRM_ERROR("Read ksv[%d] from DP/AUX failed (%ld)\n", i,
> +				  ret);
> +			return ret >= 0 ? -EIO : ret;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static
> +int intel_dp_hdcp_read_v_prime_part(struct intel_digital_port *intel_dig_port,
> +				    int i, u32 *part)
> +{
> +	ssize_t ret;
> +
> +	if (i >= DRM_HDCP_V_PRIME_NUM_PARTS)
> +		return -EINVAL;
> +
> +	ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux,
> +			       DP_AUX_HDCP_V_PRIME(i), part,
> +			       DRM_HDCP_V_PRIME_PART_LEN);
> +	if (ret != DRM_HDCP_V_PRIME_PART_LEN) {
> +		DRM_ERROR("Read v'[%d] from DP/AUX failed (%ld)\n", i, ret);
> +		return ret >= 0 ? -EIO : ret;
> +	}
> +	return 0;
> +}
> +
> +static
> +int intel_dp_hdcp_toggle_signalling(struct intel_digital_port *intel_dig_port,
> +				    bool enable)
> +{
> +	/* Not used for single stream DisplayPort setups */
> +	return 0;
> +}
> +
> +static
> +bool intel_dp_hdcp_check_link(struct intel_digital_port *intel_dig_port)
> +{
> +	ssize_t ret;
> +	u8 bstatus;
> +	ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BSTATUS,
> +			       &bstatus, 1);
> +	if (ret != 1) {
> +		DRM_ERROR("Read bstatus from DP/AUX failed (%ld)\n", ret);
> +		return ret >= 0 ? -EIO : ret;
> +	}
> +	return !(bstatus & (DP_BSTATUS_LINK_FAILURE | DP_BSTATUS_REAUTH_REQ));
> +}
> +
> +static const struct intel_hdcp_shim intel_dp_hdcp_shim = {
> +	.write_an_aksv = intel_dp_hdcp_write_an_aksv,
> +	.read_bksv = intel_dp_hdcp_read_bksv,
> +	.read_bstatus = intel_dp_hdcp_read_bstatus,
> +	.repeater_present = intel_dp_hdcp_repeater_present,
> +	.read_ri_prime = intel_dp_hdcp_read_ri_prime,
> +	.read_ksv_ready = intel_dp_hdcp_read_ksv_ready,
> +	.read_ksv_fifo = intel_dp_hdcp_read_ksv_fifo,
> +	.read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
> +	.toggle_signalling = intel_dp_hdcp_toggle_signalling,
> +	.check_link = intel_dp_hdcp_check_link,
> +};
> +
>   static void intel_edp_panel_vdd_sanitize(struct intel_dp *intel_dp)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> @@ -5146,6 +5367,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>   		drm_modeset_acquire_fini(&ctx);
>   		WARN(iret, "Acquiring modeset locks failed with %i\n", iret);
>   
> +		/* Short pulse can signify loss of hdcp authentication */
> +		intel_hdcp_check_link(intel_dp->attached_connector);
> +
>   		if (!handled) {
>   			intel_dp->detect_done = false;
>   			goto put_power;
> @@ -6121,6 +6345,12 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>   
>   	intel_dp_add_properties(intel_dp, connector);
>   
> +	if (INTEL_GEN(dev_priv) >= 9 && !intel_dp_is_edp(intel_dp)) {
> +		int ret = intel_hdcp_init(intel_connector, &intel_dp_hdcp_shim);
> +		if (ret)
> +			DRM_DEBUG_KMS("HDCP init failed, skipping.\n");
> +	}
> +
>   	/* For G4X desktop chip, PEG_BAND_GAP_DATA 3:0 must first be written
>   	 * 0xd.  Failure to do so will result in spurious interrupts being
>   	 * generated on the port when a cable is not attached.

Powered by blists - more mailing lists