Re: [PATCH] drm/i915/hdcp: add intel_atomic_state argument to hdcp_enable function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux