[Intel-gfx] [PATCH v2 1/2] drm/i915: Add support for considering HDCP ver requested via debugfs

[Nautiyal, Ankit K]

Hi Anshuman,

Thanks for the comments. Please find my response inline:


On 6/15/2020 9:59 AM, Anshuman Gupta wrote:
> On 2020-06-08 at 15:31:02 +0530, Ankit Nautiyal wrote:
>> For testing and debugging each HDCP version separately, a debugfs
>> entry for requesting a specific version is required. The version
>> requested via debugfs needs to be stored in hdcp structure. This can
>> then be considered while enabling HDCP, provided the platform and the
>> display supports the requested version.
>>
>> This patch adds the support for storing the version requested as a 32bit
>> flag. It also adds a helper function to check if a version is requested.
>>
>> If a specific HDCP version is requested through the debugfs, the driver
>> chooses that version, instead of policy of choosing the highest HDCP
>> version supported.
>>
>> v2: Initialize debugfs_ver_request flag with 0. (Jani Nikula)
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>> ---
>>   .../gpu/drm/i915/display/intel_display_types.h   | 10 ++++++++++
>>   drivers/gpu/drm/i915/display/intel_hdcp.c        | 16 ++++++++++++++--
>>   2 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index 9488449e4b94..cfa641c70717 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -408,6 +408,16 @@ struct intel_hdcp {
>>   	 * Hence caching the transcoder here.
>>   	 */
>>   	enum transcoder cpu_transcoder;
>> +
>> +	/*
>> +	 * HDCP version requested from debugfs i915_hdcp_ver_request.
>> +	 * Kernel will read these bits and entertain the request, as per
>> +	 * the HDCP capability of the panel and platform.
>> +	 */
>> +#define HDCP_VERSION_1_4	0x01
>> +#define HDCP_VERSION_2_2	0x02
>> +#define HDCP_VERSION_MASK	0x03
>> +	u32 debugfs_ver_request;
>>   };
>>   
>>   struct intel_connector {
>> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
>> index 2cbc4619b4ce..a21ea9c2e9a7 100644
>> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
>> @@ -1977,6 +1977,8 @@ int intel_hdcp_init(struct intel_connector *connector,
>>   	if (!shim)
>>   		return -EINVAL;
>>   
>> +	hdcp->debugfs_ver_request = 0;
>> +
>>   	if (is_hdcp2_supported(dev_priv))
>>   		intel_hdcp2_init(connector, shim);
>>   
>> @@ -1998,6 +2000,14 @@ int intel_hdcp_init(struct intel_connector *connector,
>>   	return 0;
>>   }
>>   
>> +static bool hdcp_debugfs_requested(struct intel_hdcp *hdcp, u32 hdcp_version)
>> +{
>> +	if (!hdcp->debugfs_ver_request)
>> +		return true;
>> +
>> +	return hdcp->debugfs_ver_request & hdcp_version ? true : false;
>> +}
>> +
>>   int intel_hdcp_enable(struct intel_connector *connector,
>>   		      enum transcoder cpu_transcoder, u8 content_type)
>>   {
>> @@ -2023,7 +2033,8 @@ int intel_hdcp_enable(struct intel_connector *connector,
>>   	 * Considering that HDCP2.2 is more secure than HDCP1.4, If the setup
>>   	 * is capable of HDCP2.2, it is preferred to use HDCP2.2.
>>   	 */
>> -	if (intel_hdcp2_capable(connector)) {
>> +	if (hdcp_debugfs_requested(hdcp, HDCP_VERSION_2_2) &&
>> +	    intel_hdcp2_capable(connector)) {
>>   		ret = _intel_hdcp2_enable(connector);
>>   		if (!ret)
>>   			check_link_interval = DRM_HDCP2_CHECK_PERIOD_MS;
>> @@ -2033,7 +2044,8 @@ int intel_hdcp_enable(struct intel_connector *connector,
>>   	 * When HDCP2.2 fails and Content Type is not Type1, HDCP1.4 will
>>   	 * be attempted.
>>   	 */
>> -	if (ret && intel_hdcp_capable(connector) &&
>> +	if (ret && hdcp_debugfs_requested(hdcp, HDCP_VERSION_1_4) &&
> IMHO there is no case when both version HDCP 2.2 and HDCP 1.4 version
> will be set, i believe for IGT if HDCP 2.2 fails and version is HDCP 2.2
> it should have returen from above, no  need to check a ret value and
> HDCP 1.4 version. Could we simplify conditions here.
> Thanks,
> Anshuman Gupta.

I was trying to have minimum change in the present flow. So I had just 
added a function
hdcp_debugfs_requested(). This will return true if there is no version 
requested and the flow remains same.
In case a specific version is requested say HDCP 2.2, only that version 
will be chosen. In case the HDCP2.2 fails,
the hdcp_debugfs_requested() condition will fail and the flow will skip 
for HDCP1.4 part.

If not like this, we can try to have a separate code-block, for the case 
where debugfs version is requested,
but this will lead to duplication of parts for enabling HDCP2.2/ HDCP1.4.

if (hdcp->debugfs_ver_request & HDCP_VERSION_2_2) {
         /* enable HDCP2.2 */
}
else if (hdcp->debugfs_ver_request & HDCP_VERSION_1_4) {
         /* enable HDCP1.4 */
}

else {
/* Existing policy of enabling HDCP2.2 if possible, or fall back to 
HDCP1.4*/
}

So to avoid code duplication, IMHO,  the current mechanism seems fine.

Perhaps the naming of the function hdcp_debugfs_requested can be made 
better to avoid confusion as it returns true even in case no version is 
requested.
Would it be better to name hdcp_debugfs_allow_version(hdcp, HDCP_VER_#_#) ?

Regards,
Ankit

>> +	    intel_hdcp_capable(connector) &&
>>   	    hdcp->content_type != DRM_MODE_HDCP_CONTENT_TYPE1) {
>>   		ret = _intel_hdcp_enable(connector);
>>   	}
>> -- 
>> 2.17.1
>>