Re: [PATCH 2/2] drm/i915/hdcp: Fix modeset locking issue in hdcp mst

["Nautiyal, Ankit K" <ankit.k.nautiyal@xxxxxxxxx>]

On 5/11/2023 11:27 AM, Suraj Kandpal wrote:
> Since topology state is being added to drm_atomic_state now all
> drm_modeset_lock required are being taken from core this raises
> an issue when we try to loop over connector and assign vcpi id to
> our streams as we did not have atomic state to derive acquire_ctx
> from hence we fill in stream info if dpmst encoder is found before
> enabling hdcp.
>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> ---
>   .../drm/i915/display/intel_display_types.h    |  2 ++
>   drivers/gpu/drm/i915/display/intel_hdcp.c     | 26 ++++++++++---------
>   2 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 35c260bd1461..aecd84b66735 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1813,6 +1813,8 @@ struct intel_digital_port {
>   	struct hdcp_port_data hdcp_port_data;
>   	/* Whether the MST topology supports HDCP Type 1 Content */
>   	bool hdcp_mst_type1_capable;
> +	/* If streams for HDCP MST are assigned with vcpi id and stream type */
> +	int hdcp_mst_streams_ready;
>   
>   	void (*write_infoframe)(struct intel_encoder *encoder,
>   				const struct intel_crtc_state *crtc_state,
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> index 1928c80cb6a2..d2874431c9d3 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> @@ -30,7 +30,8 @@
>   #define KEY_LOAD_TRIES	5
>   #define HDCP2_LC_RETRY_CNT			3
>   
> -static int intel_conn_to_vcpi(struct intel_connector *connector)
> +static int intel_conn_to_vcpi(struct drm_atomic_state *state,
> +			      struct intel_connector *connector)
>   {
>   	struct drm_dp_mst_topology_mgr *mgr;
>   	struct drm_dp_mst_atomic_payload *payload;
> @@ -42,7 +43,7 @@ static int intel_conn_to_vcpi(struct intel_connector *connector)
>   		return 0;
>   	mgr = connector->port->mgr;
>   
> -	drm_modeset_lock(&mgr->base.lock, NULL);
> +	drm_modeset_lock(&mgr->base.lock, state->acquire_ctx);
>   	mst_state = to_drm_dp_mst_topology_state(mgr->base.state);
>   	payload = drm_atomic_get_mst_payload_state(mst_state, connector->port);
>   	if (drm_WARN_ON(mgr->dev, !payload))
> @@ -54,7 +55,6 @@ static int intel_conn_to_vcpi(struct intel_connector *connector)
>   		goto out;
>   	}
>   out:
> -	drm_modeset_unlock(&mgr->base.lock);
>   	return vcpi;
>   }
>   
> @@ -69,7 +69,8 @@ static int intel_conn_to_vcpi(struct intel_connector *connector)
>    * policy to mark different content_types for different streams.
>    */
>   static int
> -intel_hdcp_required_content_stream(struct intel_digital_port *dig_port)
> +intel_hdcp_required_content_stream(struct intel_digital_port *dig_port,
> +				   struct intel_atomic_state *state)
>   {
>   	struct drm_connector_list_iter conn_iter;
>   	struct intel_digital_port *conn_dig_port;
> @@ -81,9 +82,6 @@ intel_hdcp_required_content_stream(struct intel_digital_port *dig_port)
>   
>   	data->k = 0;
>   
> -	if (dig_port->hdcp_auth_status)
> -		return 0;
> -
>   	drm_connector_list_iter_begin(&i915->drm, &conn_iter);
>   	for_each_intel_connector_iter(connector, &conn_iter) {
>   		if (connector->base.status == connector_status_disconnected)
> @@ -99,7 +97,8 @@ intel_hdcp_required_content_stream(struct intel_digital_port *dig_port)
>   		if (!enforce_type0 && !dig_port->hdcp_mst_type1_capable)
>   			enforce_type0 = true;
>   
> -		data->streams[data->k].stream_id = intel_conn_to_vcpi(connector);
> +		data->streams[data->k].stream_id =
> +			intel_conn_to_vcpi(&state->base, connector);
>   		data->k++;
>   
>   		/* if there is only one active stream */
> @@ -127,15 +126,12 @@ static int intel_hdcp_prepare_streams(struct intel_connector *connector)
>   	struct intel_digital_port *dig_port = intel_attached_dig_port(connector);
>   	struct hdcp_port_data *data = &dig_port->hdcp_port_data;
>   	struct intel_hdcp *hdcp = &connector->hdcp;
> -	int ret;
>   
>   	if (!intel_encoder_is_mst(intel_attached_encoder(connector))) {
>   		data->k = 1;
>   		data->streams[0].stream_type = hdcp->content_type;
>   	} else {
> -		ret = intel_hdcp_required_content_stream(dig_port);
> -		if (ret)
> -			return ret;
> +		return dig_port->hdcp_mst_streams_ready;
>   	}
>   
>   	return 0;
> @@ -2373,6 +2369,12 @@ int intel_hdcp_enable(struct intel_atomic_state *state,
>   	 * is capable of HDCP2.2, it is preferred to use HDCP2.2.
>   	 */
>   	if (intel_hdcp2_capable(connector)) {
> +		if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_DP_MST)) {
> +			dig_port->hdcp_mst_streams_ready =
> +				intel_hdcp_required_content_stream(dig_port,
> +								   state);

When we already know intel_hdcp_required_content_stream() has returned 
error, there is no point in continuing with HDCP2 enable.

IMHO, lets drop hdcp_mst_streams_ready and just call 
intel_hdcp_prepare_streams() here itself. There is no point in having 
this set later during authentication retry loop.


Regards,

Ankit


> +		}
> +
>   		ret = _intel_hdcp2_enable(connector);
>   		if (!ret)
>   			check_link_interval = DRM_HDCP2_CHECK_PERIOD_MS;