RE: [PATCH] drm/i915/hdcp: Fail fast when HDCP Type 1 is unsupported via MST

[Kandpal, Suraj]

> Subject: [PATCH] drm/i915/hdcp: Fail fast when HDCP Type 1 is unsupported via
> MST
> 
> HDCP Content Type 1 requires HDCP 2.x support from both the source and the
> downstream MST sink. A request for a sink that supports HDCP 1.x but cannot use
> HDCP 2.x currently passes atomic validation and fails later in the HDCP enable
> path.

That is just not correct from HDCP spec source should just be concerned about
The immediate downstream sink/dock HDCP version based on which it will
Either pass or fail the atomic check. So if it passes the atomic check that means
That HDCP 2.x is available immediately downstream and according to spec
Source has no business poking any topology below that until later in the authentication
When itself reports if there is a legacy device is present or not at which point
We abort the authentication. Anything other than that breaks HDCP 2.x compliance
and will also result in many MST docks and monitors failing HDCP 2.x when they could have passed.

> 
> Query the downstream sink through its MST remote AUX channel during HDCP
> initialization. Cache the result only when the sink reports HDCP support and
> either the source or sink lacks HDCP 2.x. Treat a missing callback, probe failure, or
> no reported HDCP support as unknown and non-fatal.

I do not recommend this I have personally seen erroneous reporting when relying in remote AUX
Transaction when it may report HDCP 1.x even though it is 2.x

> 
> Use the cached result to reject unsupported Content Type 1 requests with -
> EOPNOTSUPP in intel_hdcp_atomic_check(), and propagate the error through the
> digital connector atomic check. This keeps remote AUX transactions out of the
> atomic check path.
> 
> Signed-off-by: George D. Sworo <george.d.sworo@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic.c   |  6 ++-
>  .../drm/i915/display/intel_display_types.h    |  2 +
>  drivers/gpu/drm/i915/display/intel_hdcp.c     | 50 +++++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_hdcp.h     |  6 +--
>  4 files changed, 55 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> b/drivers/gpu/drm/i915/display/intel_atomic.c
> index b66c2d4ba2b3..c6383e27e62b 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -123,8 +123,12 @@ int intel_digital_connector_atomic_check(struct
> drm_connector *conn,
>  	struct intel_digital_connector_state *old_conn_state =
>  		to_intel_digital_connector_state(old_state);
>  	struct drm_crtc_state *crtc_state;
> +	/* Propagate HDCP capability failures during atomic validation. */

It's called an atomic check. 
Also we do not add comments on top of variable decalaration

> +	int ret;
> 
> -	intel_hdcp_atomic_check(conn, old_state, new_state);
> +	ret = intel_hdcp_atomic_check(conn, old_state, new_state);
> +	if (ret)
> +		return ret;
> 
>  	if (!new_state->crtc)
>  		return 0;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 79f30660c2b6..1c169849e502 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -605,6 +605,8 @@ struct intel_connector {
>  	struct {
>  		struct drm_dp_mst_port *port;
>  		struct intel_dp *dp;
> +		/* Used to reject unsupported HDCP Type1 requests during
> atomic check. */
> +		bool type1_unsupported;

We do not store these here maybe in a state but not inside connector

>  	} mst;
> 
>  	struct {
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c
> b/drivers/gpu/drm/i915/display/intel_hdcp.c
> index e56df337dc6d..7490fc0fd96a 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> @@ -312,6 +312,26 @@ static void intel_hdcp_get_remote_capability(struct
> intel_connector *connector,
>  		*hdcp2_capable = false;
>  }
> 
> +/* Determine whether atomic check should reject Type1 for this MST
> +sink. */ static bool intel_hdcp_mst_type1_is_unsupported(struct
> +intel_connector *connector,
> +				    const struct intel_hdcp_shim *shim) {
> +	struct intel_hdcp *hdcp = &connector->hdcp;
> +	bool hdcp_capable = false, hdcp2_capable = false;
> +	int ret;
> +
> +	if (!shim->get_remote_hdcp_capability)
> +		return false;
> +
> +	ret = shim->get_remote_hdcp_capability(connector, &hdcp_capable,
> +						   &hdcp2_capable);

This is a poor copy of  intel_hdcp_get_remote_capability() which is present
there in the same file,. The existing one runs
intel_hdcp2_prerequisite() to clamp hdcp2_capable on GSC status,
comp_added and arbiter. Here you drop that even though in your commit message you claim both source and
sink should support HDCP 2.x

> +	if (ret)
> +		return false;
> +
> +	return hdcp_capable && (!hdcp->hdcp2_supported || !hdcp2_capable); }

The !hdcp->hdcp2_supported is dead code.
intel_hdcp_init passes hdcp2_supported into
drm_connector_attach_content_protection_property, and that only attaches
the HDCP Content Type property when it is true.

> +
>  static bool intel_hdcp_in_use(struct intel_display *display,
>  			      enum transcoder cpu_transcoder, enum port port)  {
> @@ -2421,6 +2441,10 @@ int intel_hdcp_init(struct intel_connector *connector,
>  	if (is_hdcp2_supported(display))
>  		intel_hdcp2_init(connector, dig_port, shim);
> 
> +	/* Cache the remote capability before atomic checks can run. */
> +	connector->mst.type1_unsupported =
> +		intel_hdcp_mst_type1_is_unsupported(connector, shim);

This breaks when topology changes hdcp_init 

> +
>  	ret = drm_connector_attach_content_protection_property(&connector-
> >base,
>  							       hdcp-
> >hdcp2_supported);
>  	if (ret) {
> @@ -2684,10 +2708,11 @@ void intel_hdcp_cleanup(struct intel_connector
> *connector)
>  	mutex_unlock(&hdcp->mutex);
>  }
> 
> -void intel_hdcp_atomic_check(struct drm_connector *connector,
> -			     struct drm_connector_state *old_state,
> -			     struct drm_connector_state *new_state)
> +int intel_hdcp_atomic_check(struct drm_connector *connector,
> +			    struct drm_connector_state *old_state,
> +			    struct drm_connector_state *new_state)

This goes against what the Content Protection property is documented
to do. Have a look at the property doc in drm_connector.c. DESIRED is
not a question you answer once. it is a standing request. The doc
literally says the kernel should enable content protection as soon as
to re-authenticate whenever possible, across disable/enable, dpms,
hotplug downstream device changes link status failures. Failing the
commit is the one thing it is not supposed to do.
The way we tell userspace it did not work is by leaving the property at
DESIRED and sending the uevent that is the whole contract. Userspace
sets DESIRED optimistically and then polls or listens, it does not
expect the commit itself to be rejected, and existing compositors do not
handle that.
And note the irony here, downstream device changes is called out
explicitly as a case where we must keep retrying, which is exactly the
dock/monitor swap case this patch makes permanently unrecoverable. So
even if the capability read were reliable which as I said above it is
not, hard failing the atomic commit is the wrong way to report it.

>  {
> +	struct intel_connector *intel_connector =
> +to_intel_connector(connector);
>  	u64 old_cp = old_state->content_protection;
>  	u64 new_cp = new_state->content_protection;
>  	struct drm_crtc_state *crtc_state;
> @@ -2700,7 +2725,20 @@ void intel_hdcp_atomic_check(struct drm_connector
> *connector,
>  		if (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)
>  			new_state->content_protection =
> 
> 	DRM_MODE_CONTENT_PROTECTION_DESIRED;
> -		return;
> +		return 0;
> +	}
> +
> +	/*
> +	 * Fail fast if userspace asks for Type1 but neither the platform nor
> +	 * the downstream sink can do HDCP 2.x, instead of only discovering
> +	 * this once intel_hdcp_enable() is reached.
> +	 */
> +	if (new_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
> +	    new_state->hdcp_content_type ==
> DRM_MODE_HDCP_CONTENT_TYPE1 &&
> +	    intel_connector->mst.type1_unsupported) {

This is also called for HDMI which will not have the mst properly initialised what happens then

> +		drm_dbg_kms(connector->dev,
> +			    "HDCP Type1 requested without HDCP 2.x
> support\n");
> +		return -EOPNOTSUPP;

Failing here takes down the whole commit, not just this connector.
Atomic is all or nothing, so a compositor that sets CP DESIRED with
Type1 in the same commit as a modeset or a flip for other CRTCs loses
the entire update. A property we cannot honour should not be killing
unrelated displays.

Regards,
Suraj Kandpal

>  	}
> 
>  	crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
> @@ -2725,10 +2763,12 @@ void intel_hdcp_atomic_check(struct
> drm_connector *connector,
>  	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)) {
>  		if (old_state->hdcp_content_type ==
>  				new_state->hdcp_content_type)
> -			return;
> +			return 0;
>  	}
> 
>  	crtc_state->mode_changed = true;
> +
> +	return 0;
>  }
> 
>  /* Handles the CP_IRQ raised from the DP HDCP sink */ diff --git
> a/drivers/gpu/drm/i915/display/intel_hdcp.h
> b/drivers/gpu/drm/i915/display/intel_hdcp.h
> index efe86808e17e..34661b074784 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp.h
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.h
> @@ -22,9 +22,9 @@ struct intel_hdcp_shim;  struct seq_file;  enum port;
> 
> -void intel_hdcp_atomic_check(struct drm_connector *connector,
> -			     struct drm_connector_state *old_state,
> -			     struct drm_connector_state *new_state);
> +int intel_hdcp_atomic_check(struct drm_connector *connector,
> +			    struct drm_connector_state *old_state,
> +			    struct drm_connector_state *new_state);
>  int intel_hdcp_init(struct intel_connector *connector,
>  		    struct intel_digital_port *dig_port,
>  		    const struct intel_hdcp_shim *hdcp_shim);
> 
> base-commit: 999838292166407bbe911a41dee23112c49e9d44
> --
> 2.43.0