[Intel-gfx] [PATCH 22/43] drm/i915: Async execution of hdcp authentication

[Ramalingam C]

On Thursday 22 February 2018 08:09 PM, Sean Paul wrote:
> 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 at 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)?
At present enable flow also non-interuptible on hotplug. disable is not 
even is possible along with enable. correct me if i am wrong.

Since enable flow is blocking the modeset(atomic) also, I am afraid we 
need to adopt a worker for
enable path for facilitate the hotplug processing.

And in case of worker implementation need to check the connector status 
and hdcp preferred state before entering into any wait for msg.
Sleep should be selectively interruptible. every reauth should be 
checking the above conditions. I feel this is do-able.

This patch also not doing all of the above. If you think above prob 
statement needs to be addressed i will work on that.
I am looking for your suggestion. Thanks
>
> 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.
Because hdcp was passing on all setup, you were done in less than a Sec. 
That is the best case scenario. I wish every setup falls into it :)
Due to a buggy repeater or a buggy receiver connected to a functional 
repeater,
if the repeater authentication is delayed/failed, flip will be blocked 
for ~5Secs x <no of auth_tries>

All these possibilities can be avoided, if we can handle the complexity 
attached to worker
scheduling and canceling. Please share your take on this.
> IMO, it's more important to remove the modeset requirement (we've
> fixed this in CrOS already) than it is to do HDCP asynchronously.
Agreed on modeset removal. We need to do this on priority.

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