Re: [PATCH v5 05/40] drm/i915: wrapping all hdcp var into intel_hdcp

[Sean Paul <seanpaul@chromium.org>]

On Wed, Jun 27, 2018 at 02:09:54PM +0530, Ramalingam C wrote:
> Considering significant number of HDCP specific variables, it will
> be clean to have separate struct for HDCP.
> 
> New structure called intel_hdcp is added within intel_connector.
> 
> v2:
>   struct hdcp statically allocated. [Sean Paul]
>   enable and disable function parameters are retained.[Sean Paul]
> v3:
>   No Changes.
> v4:
>   Commit msg is rephrased [Uma]
> v5:
>   Comment for mutex definition.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  7 +--
>  drivers/gpu/drm/i915/intel_drv.h     | 15 ++++--
>  drivers/gpu/drm/i915/intel_hdcp.c    | 94 ++++++++++++++++++++----------------
>  3 files changed, 67 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3d849ec17f5c..ef09bb89d2ca 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15856,9 +15856,10 @@ 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);
> +		if (connector->hdcp.hdcp_shim) {
> +			cancel_delayed_work_sync(
> +					&connector->hdcp.hdcp_check_work);
> +			cancel_work_sync(&connector->hdcp.hdcp_prop_work);
>  		}
>  	}
>  	drm_connector_list_iter_end(&conn_iter);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 578346b8d7e2..eb480574a92e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -377,6 +377,15 @@ struct intel_hdcp_shim {
>  			    bool *hdcp_capable);
>  };
>  
> +struct intel_hdcp {
> +	const struct intel_hdcp_shim *hdcp_shim;
> +	/* Mutex for hdcp state of the connector */
> +	struct mutex hdcp_mutex;
> +	uint64_t hdcp_value;
> +	struct delayed_work hdcp_check_work;
> +	struct work_struct hdcp_prop_work;

Now that they're in their own struct, you can probably drop the hdcp_* prefix
from the struct members.

> +};
> +
>  struct intel_connector {
>  	struct drm_connector base;
>  	/*
> @@ -409,11 +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 intel_hdcp hdcp;
>  };
>  
>  struct intel_digital_connector_state {
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index 0cc6a861bcf8..65bbe5874eee 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -626,6 +626,7 @@ struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector)
>  
>  static int _intel_hdcp_disable(struct intel_connector *connector)
>  {
> +	struct intel_hdcp *hdcp = &connector->hdcp;
>  	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;
> @@ -641,7 +642,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;
> @@ -653,6 +654,7 @@ static int _intel_hdcp_disable(struct intel_connector *connector)
>  
>  static int _intel_hdcp_enable(struct intel_connector *connector)
>  {
> +	struct intel_hdcp *hdcp = &connector->hdcp;
>  	struct drm_i915_private *dev_priv = connector->base.dev->dev_private;
>  	int i, ret, tries = 3;
>  
> @@ -678,7 +680,7 @@ 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)
>  			return 0;
>  
> @@ -694,36 +696,42 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
>  
>  static void intel_hdcp_check_work(struct work_struct *work)
>  {
> -	struct intel_connector *connector = container_of(to_delayed_work(work),
> +	struct intel_hdcp *hdcp = container_of(to_delayed_work(work),
> +					       struct intel_hdcp,
> +					       hdcp_check_work);
> +	struct intel_connector *connector = container_of(hdcp,
>  							 struct intel_connector,
> -							 hdcp_check_work);
> +							 hdcp);

Since you use this below, break this out into a static inline helper
intel_hdcp_to_connector().


With those nits addressed,

Reviewed-by: Sean Paul <seanpaul@chromium.org>


> +
>  	if (!intel_hdcp_check_link(connector))
> -		schedule_delayed_work(&connector->hdcp_check_work,
> +		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_hdcp *hdcp = container_of(work, struct intel_hdcp,
> +					       hdcp_prop_work);
> +	struct intel_connector *connector = container_of(hdcp,
>  							 struct intel_connector,
> -							 hdcp_prop_work);
> +							 hdcp);
>  	struct drm_device *dev = 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) {
> +	if (hdcp->hdcp_value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
>  		state = connector->base.state;
> -		state->content_protection = connector->hdcp_value;
> +		state->content_protection = hdcp->hdcp_value;
>  	}
>  
> -	mutex_unlock(&connector->hdcp_mutex);
> +	mutex_unlock(&hdcp->hdcp_mutex);
>  	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  }
>  
> @@ -737,6 +745,7 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
>  int intel_hdcp_init(struct intel_connector *connector,
>  		    const struct intel_hdcp_shim *hdcp_shim)
>  {
> +	struct intel_hdcp *hdcp = &connector->hdcp;
>  	int ret;
>  
>  	ret = drm_connector_attach_content_protection_property(
> @@ -744,51 +753,53 @@ int intel_hdcp_init(struct intel_connector *connector,
>  	if (ret)
>  		return ret;
>  
> -	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);
> +	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);
>  	return 0;
>  }
>  
>  int intel_hdcp_enable(struct intel_connector *connector)
>  {
> +	struct intel_hdcp *hdcp = &connector->hdcp;
>  	int ret;
>  
> -	if (!connector->hdcp_shim)
> +	if (!hdcp->hdcp_shim)
>  		return -ENOENT;
>  
> -	mutex_lock(&connector->hdcp_mutex);
> +	mutex_lock(&hdcp->hdcp_mutex);
>  
>  	ret = _intel_hdcp_enable(connector);
>  	if (ret)
>  		goto out;
>  
> -	connector->hdcp_value = DRM_MODE_CONTENT_PROTECTION_ENABLED;
> -	schedule_work(&connector->hdcp_prop_work);
> -	schedule_delayed_work(&connector->hdcp_check_work,
> +	hdcp->hdcp_value = DRM_MODE_CONTENT_PROTECTION_ENABLED;
> +	schedule_work(&hdcp->hdcp_prop_work);
> +	schedule_delayed_work(&hdcp->hdcp_check_work,
>  			      DRM_HDCP_CHECK_PERIOD_MS);
>  out:
> -	mutex_unlock(&connector->hdcp_mutex);
> +	mutex_unlock(&hdcp->hdcp_mutex);
>  	return ret;
>  }
>  
>  int intel_hdcp_disable(struct intel_connector *connector)
>  {
> +	struct intel_hdcp *hdcp = &connector->hdcp;
>  	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) {
> -		connector->hdcp_value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
> +	if (hdcp->hdcp_value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> +		hdcp->hdcp_value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
>  		ret = _intel_hdcp_disable(connector);
>  	}
>  
> -	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;
>  }
>  
> @@ -828,17 +839,18 @@ 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)
>  {
> +	struct intel_hdcp *hdcp = &connector->hdcp;
>  	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)) {
> @@ -846,17 +858,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;
>  	}
> @@ -867,20 +879,20 @@ int intel_hdcp_check_link(struct intel_connector *connector)
>  	ret = _intel_hdcp_disable(connector);
>  	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);
>  	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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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