Re: [PATCH v3 03/10] drm/i915: Attach content type property

[Ramalingam C <ramalingam.c@intel.com>]

On 2019-03-27 at 11:00:17 +0100, Daniel Vetter wrote:
> On Fri, Mar 22, 2019 at 06:14:41AM +0530, Ramalingam C wrote:
> > Attaches the content type property for HDCP2.2 capable connectors.
> > 
> > Implements the update of content type from property and apply the
> > restriction on HDCP version selection.
> > 
> > v2:
> >   s/cp_content_type/content_protection_type [daniel]
> >   disable at hdcp_atomic_check to avoid check at atomic_set_property
> > 	[Maarten]
> > v3:
> >   s/content_protection_type/hdcp_content_type [Pekka]
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c  | 21 +++++++++++++++------
> >  drivers/gpu/drm/i915/intel_drv.h  |  2 +-
> >  drivers/gpu/drm/i915/intel_hdcp.c | 30 +++++++++++++++++++++++++-----
> >  3 files changed, 41 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 933df3a57a8a..43859f126b82 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -3495,7 +3495,8 @@ static void intel_enable_ddi(struct intel_encoder *encoder,
> >  	/* 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(to_intel_connector(conn_state->connector),
> > +				  (u8)conn_state->hdcp_content_type);
> >  }
> >  
> >  static void intel_disable_ddi_dp(struct intel_encoder *encoder,
> > @@ -3558,21 +3559,29 @@ static void intel_ddi_update_pipe_dp(struct intel_encoder *encoder,
> >  	intel_panel_update_backlight(encoder, crtc_state, conn_state);
> >  }
> >  
> > -static void intel_ddi_update_pipe(struct intel_encoder *encoder,
> > +static void intel_ddi_update_hdcp(struct intel_encoder *encoder,
> >  				  const struct intel_crtc_state *crtc_state,
> >  				  const struct drm_connector_state *conn_state)
> 
> Not sure why you split this out, doesn't feel necessary ...
this was growing considering, with some activity based on the type state
along with content protection state. Not in this version. I will merge
in the ddi_update_pipe only.
> 
> >  {
> > -	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> > -		intel_ddi_update_pipe_dp(encoder, crtc_state, conn_state);
> > -
> >  	if (conn_state->content_protection ==
> >  	    DRM_MODE_CONTENT_PROTECTION_DESIRED)
> > -		intel_hdcp_enable(to_intel_connector(conn_state->connector));
> > +		intel_hdcp_enable(to_intel_connector(conn_state->connector),
> > +				  (u8)conn_state->hdcp_content_type);
> >  	else if (conn_state->content_protection ==
> >  		 DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
> >  		intel_hdcp_disable(to_intel_connector(conn_state->connector));
> >  }
> >  
> > +static void intel_ddi_update_pipe(struct intel_encoder *encoder,
> > +				  const struct intel_crtc_state *crtc_state,
> > +				  const struct drm_connector_state *conn_state)
> > +{
> > +	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> > +		intel_ddi_update_pipe_dp(encoder, crtc_state, conn_state);
> > +
> > +	intel_ddi_update_hdcp(encoder, crtc_state, conn_state);
> > +}
> > +
> >  static void intel_ddi_set_fia_lane_count(struct intel_encoder *encoder,
> >  					 const struct intel_crtc_state *pipe_config,
> >  					 enum port port)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index a37a4477994c..e387e842f414 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -2178,7 +2178,7 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> >  			     struct drm_connector_state *new_state);
> >  int intel_hdcp_init(struct intel_connector *connector,
> >  		    const struct intel_hdcp_shim *hdcp_shim);
> > -int intel_hdcp_enable(struct intel_connector *connector);
> > +int intel_hdcp_enable(struct intel_connector *connector, u8 content_type);
> >  int intel_hdcp_disable(struct intel_connector *connector);
> >  bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port);
> >  bool intel_hdcp_capable(struct intel_connector *connector);
> > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > index ff9497e5c591..9b4904a62744 100644
> > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > @@ -1782,6 +1782,13 @@ static void intel_hdcp2_init(struct intel_connector *connector)
> >  		return;
> >  	}
> >  
> > +	ret = drm_connector_attach_hdcp_content_type_property(
> > +						&connector->base);
> > +	if (ret) {
> > +		kfree(hdcp->port_data.streams);
> > +		return;
> > +	}
> > +
> >  	hdcp->hdcp2_supported = true;
> >  }
> >  
> > @@ -1811,7 +1818,7 @@ int intel_hdcp_init(struct intel_connector *connector,
> >  	return 0;
> >  }
> >  
> > -int intel_hdcp_enable(struct intel_connector *connector)
> > +int intel_hdcp_enable(struct intel_connector *connector, u8 content_type)
> >  {
> >  	struct intel_hdcp *hdcp = &connector->hdcp;
> >  	unsigned long check_link_interval = DRM_HDCP_CHECK_PERIOD_MS;
> > @@ -1822,6 +1829,7 @@ int intel_hdcp_enable(struct intel_connector *connector)
> >  
> >  	mutex_lock(&hdcp->mutex);
> >  	WARN_ON(hdcp->value == DRM_MODE_CONTENT_PROTECTION_ENABLED);
> > +	hdcp->content_type = content_type;
> >  
> >  	/*
> >  	 * Considering that HDCP2.2 is more secure than HDCP1.4, If the setup
> > @@ -1833,8 +1841,12 @@ int intel_hdcp_enable(struct intel_connector *connector)
> >  			check_link_interval = DRM_HDCP2_CHECK_PERIOD_MS;
> >  	}
> >  
> > -	/* When HDCP2.2 fails, HDCP1.4 will be attempted */
> > -	if (ret && intel_hdcp_capable(connector)) {
> > +	/*
> > +	 * When HDCP2.2 fails and Content Type is not Type1, HDCP1.4 will
> > +	 * be attempted.
> > +	 */
> > +	if (ret && intel_hdcp_capable(connector) &&
> > +	    hdcp->content_type != DRM_MODE_HDCP_CONTENT_TYPE1) {
> >  		ret = _intel_hdcp_enable(connector);
> >  	}
> >  
> > @@ -1901,6 +1913,8 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> >  {
> >  	u64 old_cp = old_state->content_protection;
> >  	u64 new_cp = new_state->content_protection;
> > +	u64 old_type = old_state->hdcp_content_type;
> > +	u64 new_type = new_state->hdcp_content_type;
> >  	struct drm_crtc_state *crtc_state;
> >  
> >  	if (!new_state->crtc) {
> > @@ -1920,8 +1934,14 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> >  	 */
> >  	if (old_cp == new_cp ||
> >  	    (old_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
> > -	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED))
> > -		return;
> > +	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)) {
> > +		if (old_type == new_type)
> > +			return;
> > +
> > +		new_state->content_protection =
> > +			DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > +		intel_hdcp_disable(to_intel_connector(connector));
> 
> No touching hw (or any persistent sw state outside of the free-standing
> state structures) from atomic_check, ever. ATOMIC_TEST_ONLY is implemented
> by running atomic_check code, and then simply releasing the computed
> state. If you touch hw/sw state, then that's not undone.
> 
> We need to integrated this into the pipe_update function I think.
I think we cant retrieve the old conn state at atomic commit. If not we can
compare the type in conn_state with conn->hdcp->content_type and take
action of reauthentication.

-Ram
> -Daniel
> 
> > +	}
> >  
> >  	crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
> >  						   new_state->crtc);
> > -- 
> > 2.19.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx