Re: [PATCH 23/43] drm/i915: wrapping all hdcp var into intel_hdcp

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

 



On Wed, Feb 14, 2018 at 07:43:38PM +0530, Ramalingam C wrote:
> Considering the upcoming significant no HDCP2.2 variables, it will
> be clean to have separate struct fo HDCP.
> 
> New structure called intel_hdcp is introduced.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   9 ++-
>  drivers/gpu/drm/i915/intel_dp.c      |   4 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  20 +++--
>  drivers/gpu/drm/i915/intel_hdcp.c    | 141 ++++++++++++++++++++---------------
>  4 files changed, 100 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 048d60b5143b..22097349529b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15314,10 +15314,11 @@ static void intel_hpd_poll_fini(struct drm_device *dev)
>  	for_each_intel_connector_iter(connector, &conn_iter) {
>  		if (connector->modeset_retry_work.func)
>  			cancel_work_sync(&connector->modeset_retry_work);
> -		if (connector->hdcp_shim) {
> -			cancel_delayed_work_sync(&connector->hdcp_check_work);
> -			cancel_work_sync(&connector->hdcp_prop_work);
> -			cancel_work_sync(&connector->hdcp_enable_work);
> +		if (connector->hdcp && connector->hdcp->hdcp_shim) {
> +			cancel_delayed_work_sync(
> +					&connector->hdcp->hdcp_check_work);
> +			cancel_work_sync(&connector->hdcp->hdcp_prop_work);
> +			cancel_work_sync(&connector->hdcp->hdcp_enable_work);
>  		}
>  	}
>  	drm_connector_list_iter_end(&conn_iter);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f20b25f98e5a..80476689754f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5445,7 +5445,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  		WARN(iret, "Acquiring modeset locks failed with %i\n", iret);
>  
>  		/* Short pulse can signify loss of hdcp authentication */
> -		intel_hdcp_check_link(intel_dp->attached_connector);
> +		if (intel_dp->attached_connector->hdcp)
> +			intel_hdcp_check_link(
> +					intel_dp->attached_connector->hdcp);
>  
>  		if (!handled) {
>  			intel_dp->detect_done = false;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7b9e5f70826f..5b170ff7ec14 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -375,6 +375,17 @@ struct intel_hdcp_shim {
>  			    bool *hdcp_capable);
>  };
>  
> +struct intel_hdcp {
> +	struct intel_connector *connector;

You don't need a backpointer, use container_of if necessary.

> +
> +	const struct intel_hdcp_shim *hdcp_shim;
> +	struct mutex hdcp_mutex;
> +	uint64_t hdcp_value; /* protected by hdcp_mutex */
> +	struct delayed_work hdcp_check_work;
> +	struct work_struct hdcp_prop_work;
> +	struct work_struct hdcp_enable_work;
> +};
> +
>  struct intel_connector {
>  	struct drm_connector base;
>  	/*
> @@ -407,12 +418,7 @@ struct intel_connector {
>  	/* Work struct to schedule a uevent on link train failure */
>  	struct work_struct modeset_retry_work;
>  
> -	const struct intel_hdcp_shim *hdcp_shim;
> -	struct mutex hdcp_mutex;
> -	uint64_t hdcp_value; /* protected by hdcp_mutex */
> -	struct delayed_work hdcp_check_work;
> -	struct work_struct hdcp_prop_work;
> -	struct work_struct hdcp_enable_work;
> +	struct intel_hdcp *hdcp;
>  };
>  
>  struct intel_digital_connector_state {
> @@ -1853,7 +1859,7 @@ 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_disable(struct intel_connector *connector);
> -int intel_hdcp_check_link(struct intel_connector *connector);
> +int intel_hdcp_check_link(struct intel_hdcp *hdcp);
>  bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port);
>  
>  /* intel_psr.c */
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index e03bd376d92c..9147fb17a9fc 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -545,8 +545,9 @@ struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector)
>  	return enc_to_dig_port(&intel_attached_encoder(&connector->base)->base);
>  }
>  
> -static int _intel_hdcp_disable(struct intel_connector *connector)
> +static int _intel_hdcp_disable(struct intel_hdcp *hdcp)
>  {
> +	struct intel_connector *connector = hdcp->connector;

I'm not sure why we need to change all of the function arguments to intel_hdcp
if the first thing we do is fetch the connector.

>  	struct drm_i915_private *dev_priv = connector->base.dev->dev_private;
>  	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
>  	enum port port = intel_dig_port->base.port;
> @@ -562,7 +563,7 @@ static int _intel_hdcp_disable(struct intel_connector *connector)
>  		return -ETIMEDOUT;
>  	}
>  
> -	ret = connector->hdcp_shim->toggle_signalling(intel_dig_port, false);
> +	ret = hdcp->hdcp_shim->toggle_signalling(intel_dig_port, false);
>  	if (ret) {
>  		DRM_ERROR("Failed to disable HDCP signalling\n");
>  		return ret;
> @@ -572,8 +573,9 @@ static int _intel_hdcp_disable(struct intel_connector *connector)
>  	return 0;
>  }
>  
> -static int _intel_hdcp_enable(struct intel_connector *connector)
> +static int _intel_hdcp_enable(struct intel_hdcp *hdcp)
>  {
> +	struct intel_connector *connector = hdcp->connector;
>  	struct drm_i915_private *dev_priv = connector->base.dev->dev_private;
>  	int i, ret, tries = 3;
>  
> @@ -599,12 +601,12 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
>  	/* Incase of authentication failures, HDCP spec expects reauth. */
>  	for (i = 0; i < tries; i++) {
>  		ret = intel_hdcp_auth(conn_to_dig_port(connector),
> -				      connector->hdcp_shim);
> +				      hdcp->hdcp_shim);
>  		if (!ret) {
> -			connector->hdcp_value =
> +			hdcp->hdcp_value =
>  					DRM_MODE_CONTENT_PROTECTION_ENABLED;
> -			schedule_work(&connector->hdcp_prop_work);
> -			schedule_delayed_work(&connector->hdcp_check_work,
> +			schedule_work(&hdcp->hdcp_prop_work);
> +			schedule_delayed_work(&hdcp->hdcp_check_work,
>  					      DRM_HDCP_CHECK_PERIOD_MS);
>  			return 0;
>  		}
> @@ -612,7 +614,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
>  		DRM_DEBUG_KMS("HDCP Auth failure (%d)\n", ret);
>  
>  		/* Ensuring HDCP encryption and signalling are stopped. */
> -		_intel_hdcp_disable(connector);
> +		_intel_hdcp_disable(hdcp);
>  	}
>  
>  	DRM_ERROR("HDCP authentication failed (%d tries/%d)\n", tries, ret);
> @@ -621,47 +623,45 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
>  
>  static void intel_hdcp_enable_work(struct work_struct *work)
>  {
> -	struct intel_connector *connector = container_of(work,
> -							 struct intel_connector,
> -							 hdcp_enable_work);
> +	struct intel_hdcp *hdcp = container_of(work, struct intel_hdcp,
> +					       hdcp_enable_work);
>  
> -	mutex_lock(&connector->hdcp_mutex);
> -	_intel_hdcp_enable(connector);
> -	mutex_unlock(&connector->hdcp_mutex);
> +	mutex_lock(&hdcp->hdcp_mutex);
> +	_intel_hdcp_enable(hdcp);
> +	mutex_unlock(&hdcp->hdcp_mutex);
>  }
>  
>  static void intel_hdcp_check_work(struct work_struct *work)
>  {
> -	struct intel_connector *connector = container_of(to_delayed_work(work),
> -							 struct intel_connector,
> -							 hdcp_check_work);
> -	if (!intel_hdcp_check_link(connector))
> -		schedule_delayed_work(&connector->hdcp_check_work,
> +	struct intel_hdcp *hdcp = container_of(to_delayed_work(work),
> +					       struct intel_hdcp,
> +					       hdcp_check_work);
> +	if (!intel_hdcp_check_link(hdcp))
> +		schedule_delayed_work(&hdcp->hdcp_check_work,
>  				      DRM_HDCP_CHECK_PERIOD_MS);
>  }
>  
>  static void intel_hdcp_prop_work(struct work_struct *work)
>  {
> -	struct intel_connector *connector = container_of(work,
> -							 struct intel_connector,
> -							 hdcp_prop_work);
> -	struct drm_device *dev = connector->base.dev;
> +	struct intel_hdcp *hdcp = container_of(work, struct intel_hdcp,
> +					       hdcp_prop_work);
> +	struct drm_device *dev = hdcp->connector->base.dev;
>  	struct drm_connector_state *state;
>  
>  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -	mutex_lock(&connector->hdcp_mutex);
> +	mutex_lock(&hdcp->hdcp_mutex);
>  
>  	/*
>  	 * This worker is only used to flip between ENABLED/DESIRED. Either of
>  	 * those to UNDESIRED is handled by core. If hdcp_value == UNDESIRED,
>  	 * we're running just after hdcp has been disabled, so just exit
>  	 */
> -	if (connector->hdcp_value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> -		state = connector->base.state;
> -		state->content_protection = connector->hdcp_value;
> +	if (hdcp->hdcp_value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> +		state = hdcp->connector->base.state;
> +		state->content_protection = hdcp->hdcp_value;
>  	}
>  
> -	mutex_unlock(&connector->hdcp_mutex);
> +	mutex_unlock(&hdcp->hdcp_mutex);
>  	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  }
>  
> @@ -676,48 +676,64 @@ int intel_hdcp_init(struct intel_connector *connector,
>  		    const struct intel_hdcp_shim *hdcp_shim)
>  {
>  	int ret;
> +	struct intel_hdcp *hdcp;
> +
> +	hdcp = kzalloc(sizeof(struct intel_hdcp), GFP_KERNEL);

You're leaking this. You probably don't need it to be dynamically allocated.

> +	if (!hdcp) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
>  
>  	ret = drm_connector_attach_content_protection_property(
>  			&connector->base);
>  	if (ret)
> -		return ret;
> +		goto err;
> +
> +	hdcp->hdcp_shim = hdcp_shim;
> +	mutex_init(&hdcp->hdcp_mutex);
> +	INIT_DELAYED_WORK(&hdcp->hdcp_check_work, intel_hdcp_check_work);
> +	INIT_WORK(&hdcp->hdcp_prop_work, intel_hdcp_prop_work);
> +	INIT_WORK(&hdcp->hdcp_enable_work, intel_hdcp_enable_work);
>  
> -	connector->hdcp_shim = hdcp_shim;
> -	mutex_init(&connector->hdcp_mutex);
> -	INIT_DELAYED_WORK(&connector->hdcp_check_work, intel_hdcp_check_work);
> -	INIT_WORK(&connector->hdcp_prop_work, intel_hdcp_prop_work);
> -	INIT_WORK(&connector->hdcp_enable_work, intel_hdcp_enable_work);
> +	connector->hdcp = hdcp;
>  	return 0;
> +
> +err:
> +	kfree(hdcp);
> +	return ret;
>  }
>  
>  int intel_hdcp_enable(struct intel_connector *connector)
>  {
> -	if (!connector->hdcp_shim)
> +	struct intel_hdcp *hdcp = connector->hdcp;
> +
> +	if (!hdcp || !hdcp->hdcp_shim)

Is it possible for hdcp to be != NULL, but hdcp_shim == NULL?

>  		return -ENOENT;
>  
> -	mutex_lock(&connector->hdcp_mutex);
> -	schedule_work(&connector->hdcp_enable_work);
> -	mutex_unlock(&connector->hdcp_mutex);
> +	mutex_lock(&hdcp->hdcp_mutex);
> +	schedule_work(&hdcp->hdcp_enable_work);
> +	mutex_unlock(&hdcp->hdcp_mutex);
>  
>  	return 0;
>  }
>  
>  int intel_hdcp_disable(struct intel_connector *connector)
>  {
> +	struct intel_hdcp *hdcp = connector->hdcp;
>  	int ret = 0;
>  
> -	if (!connector->hdcp_shim)
> +	if (!hdcp || !hdcp->hdcp_shim)
>  		return -ENOENT;
>  
> -	mutex_lock(&connector->hdcp_mutex);
> +	mutex_lock(&hdcp->hdcp_mutex);
>  
> -	if (connector->hdcp_value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> -		connector->hdcp_value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
> -		ret = _intel_hdcp_disable(connector);
> +	if (hdcp->hdcp_value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> +		hdcp->hdcp_value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
> +		ret = _intel_hdcp_disable(hdcp);
>  	}
>  
> -	mutex_unlock(&connector->hdcp_mutex);
> -	cancel_delayed_work_sync(&connector->hdcp_check_work);
> +	mutex_unlock(&hdcp->hdcp_mutex);
> +	cancel_delayed_work_sync(&hdcp->hdcp_check_work);
>  	return ret;
>  }
>  
> @@ -755,19 +771,20 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
>  }
>  
>  /* Implements Part 3 of the HDCP authorization procedure */
> -int intel_hdcp_check_link(struct intel_connector *connector)
> +int intel_hdcp_check_link(struct intel_hdcp *hdcp)
>  {
> +	struct intel_connector *connector = hdcp->connector;
>  	struct drm_i915_private *dev_priv = connector->base.dev->dev_private;
>  	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
>  	enum port port = intel_dig_port->base.port;
>  	int ret = 0;
>  
> -	if (!connector->hdcp_shim)
> +	if (!hdcp->hdcp_shim)
>  		return -ENOENT;
>  
> -	mutex_lock(&connector->hdcp_mutex);
> +	mutex_lock(&hdcp->hdcp_mutex);
>  
> -	if (connector->hdcp_value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
> +	if (hdcp->hdcp_value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
>  		goto out;
>  
>  	if (!(I915_READ(PORT_HDCP_STATUS(port)) & HDCP_STATUS_ENC)) {
> @@ -775,17 +792,17 @@ int intel_hdcp_check_link(struct intel_connector *connector)
>  			  connector->base.name, connector->base.base.id,
>  			  I915_READ(PORT_HDCP_STATUS(port)));
>  		ret = -ENXIO;
> -		connector->hdcp_value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> -		schedule_work(&connector->hdcp_prop_work);
> +		hdcp->hdcp_value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> +		schedule_work(&hdcp->hdcp_prop_work);
>  		goto out;
>  	}
>  
> -	if (connector->hdcp_shim->check_link(intel_dig_port)) {
> -		if (connector->hdcp_value !=
> +	if (hdcp->hdcp_shim->check_link(intel_dig_port)) {
> +		if (hdcp->hdcp_value !=
>  		    DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> -			connector->hdcp_value =
> +			hdcp->hdcp_value =
>  				DRM_MODE_CONTENT_PROTECTION_ENABLED;
> -			schedule_work(&connector->hdcp_prop_work);
> +			schedule_work(&hdcp->hdcp_prop_work);
>  		}
>  		goto out;
>  	}
> @@ -793,23 +810,23 @@ int intel_hdcp_check_link(struct intel_connector *connector)
>  	DRM_DEBUG_KMS("[%s:%d] HDCP link failed, retrying authentication\n",
>  		      connector->base.name, connector->base.base.id);
>  
> -	ret = _intel_hdcp_disable(connector);
> +	ret = _intel_hdcp_disable(hdcp);
>  	if (ret) {
>  		DRM_ERROR("Failed to disable hdcp (%d)\n", ret);
> -		connector->hdcp_value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> -		schedule_work(&connector->hdcp_prop_work);
> +		hdcp->hdcp_value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> +		schedule_work(&hdcp->hdcp_prop_work);
>  		goto out;
>  	}
>  
> -	ret = _intel_hdcp_enable(connector);
> +	ret = _intel_hdcp_enable(hdcp);
>  	if (ret) {
>  		DRM_ERROR("Failed to enable hdcp (%d)\n", ret);
> -		connector->hdcp_value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> -		schedule_work(&connector->hdcp_prop_work);
> +		hdcp->hdcp_value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> +		schedule_work(&hdcp->hdcp_prop_work);
>  		goto out;
>  	}
>  
>  out:
> -	mutex_unlock(&connector->hdcp_mutex);
> +	mutex_unlock(&hdcp->hdcp_mutex);
>  	return ret;
>  }
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux