On Thu, 04 May 2023, Suraj Kandpal <suraj.kandpal@xxxxxxxxx> 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: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
The subject implies you're just passing an extra parameter, but that's
really not the case. You're doing something else, and to achieve that,
you also pass an extra parameter.
One approach might be to have a separate patch to change the parameters
only, and it might be sensible to actually pass all the
intel_encoder->enable() parameters i.e.
(struct intel_atomic_state *state,
struct intel_encoder *encoder,
const struct intel_crtc_state *pipe_config,
const struct drm_connector_state *conn_state)
> ---
> drivers/gpu/drm/i915/display/intel_ddi.c | 3 +-
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 +-
> drivers/gpu/drm/i915/display/intel_hdcp.c | 50 ++++++++++++++++++---
> drivers/gpu/drm/i915/display/intel_hdcp.h | 3 +-
> 4 files changed, 49 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 29e4bfab4635..182b8815b20d 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3264,9 +3264,10 @@ static void intel_enable_ddi(struct intel_atomic_state *state,
> /* Enable hdcp if it's desired */
> if (conn_state->content_protection ==
> DRM_MODE_CONTENT_PROTECTION_DESIRED)
> - intel_hdcp_enable(to_intel_connector(conn_state->connector),
> + intel_hdcp_enable(state, to_intel_connector(conn_state->connector),
> crtc_state,
> (u8)conn_state->hdcp_content_type);
> +
> }
>
> static void intel_disable_ddi_dp(struct intel_atomic_state *state,
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 2c49d9ab86a2..c92b00ceaae0 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -800,7 +800,7 @@ static void intel_mst_enable_dp(struct intel_atomic_state *state,
> /* Enable hdcp if it's desired */
> if (conn_state->content_protection ==
> DRM_MODE_CONTENT_PROTECTION_DESIRED)
> - intel_hdcp_enable(to_intel_connector(conn_state->connector),
> + intel_hdcp_enable(state, to_intel_connector(conn_state->connector),
> pipe_config,
> (u8)conn_state->hdcp_content_type);
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> index e1dc3d96e708..c8cdf25914f7 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 intel_connector *connector,
> + struct drm_atomic_state *state)
> {
> 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);
The return value must be checked and handled for ctx != NULL. Please git
grep for examples.
> 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;
> }
>
> @@ -99,7 +99,6 @@ 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->k++;
>
> /* if there is only one active stream */
> @@ -122,6 +121,41 @@ intel_hdcp_required_content_stream(struct intel_digital_port *dig_port)
> return 0;
> }
>
> +static int
> +intel_hdcp_get_content_stream_id(struct intel_digital_port *dig_port,
> + struct intel_atomic_state *state)
To me it seems like this *sets* the stream id, not get. This doesn't
return any id.
> +{
> + struct drm_connector_list_iter conn_iter;
> + struct intel_digital_port *conn_dig_port;
> + struct intel_connector *connector;
> + struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> + struct hdcp_port_data *data = &dig_port->hdcp_port_data;
> +
> + data->k = 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)
> + continue;
> +
> + if (!intel_encoder_is_mst(intel_attached_encoder(connector)))
> + continue;
> +
> + conn_dig_port = intel_attached_dig_port(connector);
> + if (conn_dig_port != dig_port)
> + continue;
> +
> + data->streams[data->k].stream_id = intel_conn_to_vcpi(connector, &state->base);
> + data->k++;
> +
> + /* if there is only one active stream */
> + if (dig_port->dp.active_mst_links <= 1)
> + break;
> + }
> + drm_connector_list_iter_end(&conn_iter);
> +
> + return 0;
This is 95% copy-paste of intel_hdcp_required_content_stream().
> +}
> static int intel_hdcp_prepare_streams(struct intel_connector *connector)
> {
> struct intel_digital_port *dig_port = intel_attached_dig_port(connector);
> @@ -2333,7 +2367,8 @@ int intel_hdcp_init(struct intel_connector *connector,
> return 0;
> }
>
> -int intel_hdcp_enable(struct intel_connector *connector,
> +int intel_hdcp_enable(struct intel_atomic_state *state,
> + struct intel_connector *connector,
> const struct intel_crtc_state *pipe_config, u8 content_type)
> {
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -2374,6 +2409,9 @@ int intel_hdcp_enable(struct intel_connector *connector,
> * 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))
> + intel_hdcp_get_content_stream_id(dig_port, state);
> +
The call chain below already leads to:
_intel_hdcp2_enable()
hdcp2_authenticate_and_encrypt()
intel_hdcp_prepare_streams()
intel_hdcp_required_content_stream() (for MST)
and as I said, that's almost the same as what you're adding as
intel_hdcp_get_content_stream_id().
So I don't get the point of doing almost exactly the same thing twice
here.
> ret = _intel_hdcp2_enable(connector);
> if (!ret)
> check_link_interval = DRM_HDCP2_CHECK_PERIOD_MS;
> @@ -2486,7 +2524,7 @@ void intel_hdcp_update_pipe(struct intel_atomic_state *state,
> }
>
> if (desired_and_not_enabled || content_protection_type_changed)
> - intel_hdcp_enable(connector,
> + intel_hdcp_enable(state, connector,
> crtc_state,
> (u8)conn_state->hdcp_content_type);
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.h b/drivers/gpu/drm/i915/display/intel_hdcp.h
> index 8f53b0c7fe5c..a6f4bf93f9bf 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp.h
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.h
> @@ -28,7 +28,8 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> int intel_hdcp_init(struct intel_connector *connector,
> struct intel_digital_port *dig_port,
> const struct intel_hdcp_shim *hdcp_shim);
> -int intel_hdcp_enable(struct intel_connector *connector,
> +int intel_hdcp_enable(struct intel_atomic_state *state,
> + struct intel_connector *connector,
> const struct intel_crtc_state *pipe_config, u8 content_type);
> int intel_hdcp_disable(struct intel_connector *connector);
> void intel_hdcp_update_pipe(struct intel_atomic_state *state,
--
Jani Nikula, Intel Open Source Graphics Center
