> 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)
>
Sure Jani will break the patch into two and pass all the intel_encoder->enable() params
> > ---
> > 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.
>
Right will change the name.
> > +{
> > + 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.
>
So the issue here is I want to pass intel_atomic_state to conn_to_vcpi() which was
called in intel_hdcp_required_content_stream() but I cannot get the atomic_state here
as intel_hdcp2_enable gets called in places other than intel_hdcp_enable where its not possible
to derive intel_atomic_state.
But I do see the point and will try to remove redundant code.
Regards,
Suraj Kandpa;
> > 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
