Message-ID: <20171205171201.baqimw6kujav3z43@phenom.ffwll.local>
Date:   Tue, 5 Dec 2017 18:12:01 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Sean Paul <seanpaul@...omium.org>
Cc:     dri-devel@...ts.freedesktop.org, intel-gfx@...ts.freedesktop.org,
        seanpaul@...gle.com, David Airlie <airlied@...ux.ie>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        linux-kernel@...r.kernel.org,
        Rodrigo Vivi <rodrigo.vivi@...el.com>, daniel.vetter@...el.com
Subject: Re: [PATCH v3 9/9] drm/i915: Implement HDCP for DisplayPort

On Tue, Dec 05, 2017 at 12:15:08AM -0500, 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
> - Move intel_hdcp_check_link further out to avoid calling it while
>   holding _any_ locks
> 
> Signed-off-by: Sean Paul <seanpaul@...omium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 248 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 241 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c603d4c903e1..dc303e18c1dd 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)

Spotted the one (I hope, maybe I didn't check stuff perfectly) C++ style
comment!

> +	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;

Fun. I didn't really spot this behaviour difference compared to earlier
paltforms, where you had to (iirc) toggle bit 11 in the DP register.

> +}
> +
> +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);
> +}
> +
> +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,16 @@ 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)) {
> +		drm_connector_attach_content_protection_property(connector);
> +		intel_connector->hdcp_shim = &intel_dp_hdcp_shim;
> +		mutex_init(&intel_connector->hdcp_mutex);
> +		INIT_DELAYED_WORK(&intel_connector->hdcp_check_work,
> +				  intel_hdcp_check_work);
> +		INIT_WORK(&intel_connector->hdcp_prop_work,
> +			  intel_hdcp_prop_work);
> +	}

Same refactor request as for the hdmi patch here.

Either way:

Reviewed-by: Daniel Vetter <daniel.vetter@...ll.ch>
> +
>  	/* 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.
> -- 
> 2.15.0.531.g2ccb3012c9-goog
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists