[v2] drm/i915/hdcp: Update CP as per the kernel internal state
diff mbox series

Message ID 20200515061029.5008-1-anshuman.gupta@intel.com
State New
Headers show
Series
  • [v2] drm/i915/hdcp: Update CP as per the kernel internal state
Related show

Commit Message

Anshuman Gupta May 15, 2020, 6:10 a.m. UTC
Content Protection property should be updated as per the kernel
internal state. Let's say if Content protection is disabled
by userspace, CP property should be set to UNDESIRED so that
reauthentication will not happen until userspace request it again,
but when kernel disables the HDCP due to any DDI disabling sequences
like modeset/DPMS operation, kernel should set the property to
DESIRED, so that when opportunity arises, kernel will start the
HDCP authentication on its own.

Somewhere in the line, state machine to set content protection to
DESIRED from kernel was broken and IGT coverage was missing for it.
This patch fixes it.

v2:
- Fixing hdcp CP state in intel_hdcp_atomic_check(), that will
  require to check hdcp->value in intel_hdcp_update_pipe() in order
  to avoid enabling hdcp, if it was already enabled.

Cc: Ramalingam C <ramalingam.c@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Uma Shankar <uma.shankar@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
Link: https://patchwork.freedesktop.org/patch/350962/?series=72664&rev=2 #v1
Link: https://patchwork.freedesktop.org/patch/359396/?series=72251&rev=3 #v2
---
 drivers/gpu/drm/i915/display/intel_hdcp.c | 27 +++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

Comments

Chris Wilson May 15, 2020, 6:56 a.m. UTC | #1
Quoting Anshuman Gupta (2020-05-15 07:10:29)
> Somewhere in the line, state machine to set content protection to
> DESIRED from kernel was broken and IGT coverage was missing for it.
> This patch fixes it.

It's strange that you mention IGT, since what tests being run by CI
never pass and have not been fixed for several months. As well as the
sporadic HDCP failures affecting unrelated tests, which have not
appeared to have received any attention either.
-Chris
Anshuman Gupta May 15, 2020, 7:10 a.m. UTC | #2
On 2020-05-15 at 07:56:25 +0100, Chris Wilson wrote:
> Quoting Anshuman Gupta (2020-05-15 07:10:29)
> > Somewhere in the line, state machine to set content protection to
> > DESIRED from kernel was broken and IGT coverage was missing for it.
> > This patch fixes it.
> 
> It's strange that you mention IGT, since what tests being run by CI
> never pass and have not been fixed for several months. As well as the
Actually IGT fix for the coverge was worked upon after discovering this issue.
https://patchwork.freedesktop.org/patch/349542/?series=72264&rev=1
above patch requires to merge after kernel patch.
> sporadic HDCP failures affecting unrelated tests, which have not
> appeared to have received any attention either.
This bug was not failing any igt subtest, as igt test coverage was missing, 
content protection igt test were passing.
Thanks,
Anshuman.
> -Chris
Chris Wilson May 15, 2020, 7:46 a.m. UTC | #3
Quoting Anshuman Gupta (2020-05-15 08:10:06)
> On 2020-05-15 at 07:56:25 +0100, Chris Wilson wrote:
> > Quoting Anshuman Gupta (2020-05-15 07:10:29)
> > > Somewhere in the line, state machine to set content protection to
> > > DESIRED from kernel was broken and IGT coverage was missing for it.
> > > This patch fixes it.
> > 
> > It's strange that you mention IGT, since what tests being run by CI
> > never pass and have not been fixed for several months. As well as the
> Actually IGT fix for the coverge was worked upon after discovering this issue.
> https://patchwork.freedesktop.org/patch/349542/?series=72264&rev=1
> above patch requires to merge after kernel patch.
> > sporadic HDCP failures affecting unrelated tests, which have not
> > appeared to have received any attention either.
> This bug was not failing any igt subtest, as igt test coverage was missing, 
> content protection igt test were passing.

I'm referring to the existing failures in kms_content_protection -- it
is a failure across the board in CI. Not to mention the sporadic OOPSes
caused by CP.
-Chris
Anshuman Gupta May 18, 2020, 8:07 a.m. UTC | #4
On 2020-05-15 at 11:40:29 +0530, Anshuman Gupta wrote:
> Content Protection property should be updated as per the kernel
> internal state. Let's say if Content protection is disabled
> by userspace, CP property should be set to UNDESIRED so that
> reauthentication will not happen until userspace request it again,
> but when kernel disables the HDCP due to any DDI disabling sequences
> like modeset/DPMS operation, kernel should set the property to
> DESIRED, so that when opportunity arises, kernel will start the
> HDCP authentication on its own.
> 
> Somewhere in the line, state machine to set content protection to
> DESIRED from kernel was broken and IGT coverage was missing for it.
> This patch fixes it.
> 
> v2:
> - Fixing hdcp CP state in intel_hdcp_atomic_check(), that will
>   require to check hdcp->value in intel_hdcp_update_pipe() in order
>   to avoid enabling hdcp, if it was already enabled.
> 
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Link: https://patchwork.freedesktop.org/patch/350962/?series=72664&rev=2 #v1
> Link: https://patchwork.freedesktop.org/patch/359396/?series=72251&rev=3 #v2
Ram, Daniel, Jani,
Above patch fixes HDCP uapi broken state.
Could you please provide your inputs , is it ok to merges this patch in current form.
Thanks,
Anshuman Gupta.

> ---
>  drivers/gpu/drm/i915/display/intel_hdcp.c | 27 +++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> index 2cbc4619b4ce..f90f48819838 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> @@ -2083,6 +2083,7 @@ void intel_hdcp_update_pipe(struct intel_atomic_state *state,
>  		(conn_state->hdcp_content_type != hdcp->content_type &&
>  		 conn_state->content_protection !=
>  		 DRM_MODE_CONTENT_PROTECTION_UNDESIRED);
> +	bool desired_and_not_enabled = false;
>  
>  	/*
>  	 * During the HDCP encryption session if Type change is requested,
> @@ -2105,8 +2106,15 @@ void intel_hdcp_update_pipe(struct intel_atomic_state *state,
>  	}
>  
>  	if (conn_state->content_protection ==
> -	    DRM_MODE_CONTENT_PROTECTION_DESIRED ||
> -	    content_protection_type_changed)
> +	    DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> +		mutex_lock(&hdcp->mutex);
> +		/* Avoid enabling hdcp, if it already ENABLED */
> +		desired_and_not_enabled =
> +			hdcp->value != DRM_MODE_CONTENT_PROTECTION_ENABLED;
> +		mutex_unlock(&hdcp->mutex);
> +	}
> +
> +	if (desired_and_not_enabled || content_protection_type_changed)
>  		intel_hdcp_enable(connector,
>  				  crtc_state->cpu_transcoder,
>  				  (u8)conn_state->hdcp_content_type);
> @@ -2155,6 +2163,19 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
>  		return;
>  	}
>  
> +	crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
> +						   new_state->crtc);
> +	/*
> +	 * Fix the HDCP uapi content protection state in case of modeset.
> +	 * FIXME: As per HDCP content protection property uapi doc, an uevent()
> +	 * need to be sent if there is transition from ENABLED->DESIRED.
> +	 */
> +	if (drm_atomic_crtc_needs_modeset(crtc_state) &&
> +	    (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> +	    new_cp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED))
> +		new_state->content_protection =
> +			DRM_MODE_CONTENT_PROTECTION_DESIRED;
> +
>  	/*
>  	 * Nothing to do if the state didn't change, or HDCP was activated since
>  	 * the last commit. And also no change in hdcp content type.
> @@ -2167,8 +2188,6 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
>  			return;
>  	}
>  
> -	crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
> -						   new_state->crtc);
>  	crtc_state->mode_changed = true;
>  }
>  
> -- 
> 2.26.0
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
index 2cbc4619b4ce..f90f48819838 100644
--- a/drivers/gpu/drm/i915/display/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
@@ -2083,6 +2083,7 @@  void intel_hdcp_update_pipe(struct intel_atomic_state *state,
 		(conn_state->hdcp_content_type != hdcp->content_type &&
 		 conn_state->content_protection !=
 		 DRM_MODE_CONTENT_PROTECTION_UNDESIRED);
+	bool desired_and_not_enabled = false;
 
 	/*
 	 * During the HDCP encryption session if Type change is requested,
@@ -2105,8 +2106,15 @@  void intel_hdcp_update_pipe(struct intel_atomic_state *state,
 	}
 
 	if (conn_state->content_protection ==
-	    DRM_MODE_CONTENT_PROTECTION_DESIRED ||
-	    content_protection_type_changed)
+	    DRM_MODE_CONTENT_PROTECTION_DESIRED) {
+		mutex_lock(&hdcp->mutex);
+		/* Avoid enabling hdcp, if it already ENABLED */
+		desired_and_not_enabled =
+			hdcp->value != DRM_MODE_CONTENT_PROTECTION_ENABLED;
+		mutex_unlock(&hdcp->mutex);
+	}
+
+	if (desired_and_not_enabled || content_protection_type_changed)
 		intel_hdcp_enable(connector,
 				  crtc_state->cpu_transcoder,
 				  (u8)conn_state->hdcp_content_type);
@@ -2155,6 +2163,19 @@  void intel_hdcp_atomic_check(struct drm_connector *connector,
 		return;
 	}
 
+	crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
+						   new_state->crtc);
+	/*
+	 * Fix the HDCP uapi content protection state in case of modeset.
+	 * FIXME: As per HDCP content protection property uapi doc, an uevent()
+	 * need to be sent if there is transition from ENABLED->DESIRED.
+	 */
+	if (drm_atomic_crtc_needs_modeset(crtc_state) &&
+	    (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
+	    new_cp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED))
+		new_state->content_protection =
+			DRM_MODE_CONTENT_PROTECTION_DESIRED;
+
 	/*
 	 * Nothing to do if the state didn't change, or HDCP was activated since
 	 * the last commit. And also no change in hdcp content type.
@@ -2167,8 +2188,6 @@  void intel_hdcp_atomic_check(struct drm_connector *connector,
 			return;
 	}
 
-	crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
-						   new_state->crtc);
 	crtc_state->mode_changed = true;
 }