Re: [PATCH 22/43] drm/i915: Async execution of hdcp authentication

[Sean Paul <seanpaul@chromium.org>]

On Wed, Feb 14, 2018 at 07:43:37PM +0530, Ramalingam C wrote:
> Each HDCP authentication, could take upto 5.1Sec, based on the
> downstream HDCP topology.
> 
> Hence to avoid this much delay in the atomic_commit path, this patch
> schedules the HDCP authentication into a asynchronous work.
> 
> This keeps the UI active, by enabling the flips in parallel to
> HDCP auth.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  1 +
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_hdcp.c    | 36 ++++++++++++++++++++++--------------
>  3 files changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 30e38cbeface..048d60b5143b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15317,6 +15317,7 @@ static void intel_hpd_poll_fini(struct drm_device *dev)
>  		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);
>  		}
>  	}
>  	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 898064e8bea7..7b9e5f70826f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -412,6 +412,7 @@ struct intel_connector {
>  	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_digital_connector_state {
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index 14ca5d3057a7..e03bd376d92c 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -600,8 +600,14 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
>  	for (i = 0; i < tries; i++) {
>  		ret = intel_hdcp_auth(conn_to_dig_port(connector),
>  				      connector->hdcp_shim);
> -		if (!ret)
> +		if (!ret) {
> +			connector->hdcp_value =
> +					DRM_MODE_CONTENT_PROTECTION_ENABLED;
> +			schedule_work(&connector->hdcp_prop_work);
> +			schedule_delayed_work(&connector->hdcp_check_work,
> +					      DRM_HDCP_CHECK_PERIOD_MS);
>  			return 0;
> +		}
>  
>  		DRM_DEBUG_KMS("HDCP Auth failure (%d)\n", ret);
>  
> @@ -613,6 +619,17 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
>  	return ret;
>  }
>  
> +static void intel_hdcp_enable_work(struct work_struct *work)
> +{
> +	struct intel_connector *connector = container_of(work,
> +							 struct intel_connector,
> +							 hdcp_enable_work);
> +
> +	mutex_lock(&connector->hdcp_mutex);
> +	_intel_hdcp_enable(connector);
> +	mutex_unlock(&connector->hdcp_mutex);
> +}
> +
>  static void intel_hdcp_check_work(struct work_struct *work)
>  {
>  	struct intel_connector *connector = container_of(to_delayed_work(work),
> @@ -669,29 +686,20 @@ int intel_hdcp_init(struct intel_connector *connector,
>  	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);
>  	return 0;
>  }
>  
>  int intel_hdcp_enable(struct intel_connector *connector)
>  {
> -	int ret;
> -
>  	if (!connector->hdcp_shim)
>  		return -ENOENT;
>  
>  	mutex_lock(&connector->hdcp_mutex);

Why do you need to hold this lock to schedule the worker?

> -
> -	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,
> -			      DRM_HDCP_CHECK_PERIOD_MS);
> -out:
> +	schedule_work(&connector->hdcp_enable_work);

How is this worker cancelled when appropriate (enable/disable, hotplug, etc)?

I'm not convinced the extra complexity/worker is worth it. While it's possible
it _could_ take 5 seconds, however I've never experienced any lengthy delays in
practice. IMO, it's more important to remove the modeset requirement (we've
fixed this in CrOS already) than it is to do HDCP asynchronously.

Sean

>  	mutex_unlock(&connector->hdcp_mutex);
> -	return ret;
> +
> +	return 0;
>  }
>  
>  int intel_hdcp_disable(struct intel_connector *connector)
> -- 
> 2.7.4
> 

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