diff mbox series

[v2,1/2] drm/i915: Add support for considering HDCP ver requested via debugfs

Message ID 20200608100103.19472-2-ankit.k.nautiyal@intel.com (mailing list archive)
State New, archived
Headers show
Series Add debugfs for requesting HDCP version | expand

Commit Message

Nautiyal, Ankit K June 8, 2020, 10:01 a.m. UTC 
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@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(-)

Comments

Gupta, Anshuman June 15, 2020, 4:29 a.m. UTC | #1 
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@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.
> +	    intel_hdcp_capable(connector) &&
>  	    hdcp->content_type != DRM_MODE_HDCP_CONTENT_TYPE1) {
>  		ret = _intel_hdcp_enable(connector);
>  	}
> -- 
> 2.17.1
>
Nautiyal, Ankit K June 15, 2020, 10:06 a.m. UTC | #2 
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@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
>>
diff mbox series

Patch

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) &&
+	    intel_hdcp_capable(connector) &&
 	    hdcp->content_type != DRM_MODE_HDCP_CONTENT_TYPE1) {
 		ret = _intel_hdcp_enable(connector);
 	}