Re: [Intel-gfx] [PATCH v3] drm/i915/hdcp: Update CP as per the kernel internal state

From: Ramalingam C <ramalingam.c@intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v3] drm/i915/hdcp: Update CP as per the kernel internal state
Date: Wed, 22 Jan 2020 19:41:42 +0530	[thread overview]
Message-ID: <20200122141142.GA29793@intel.com> (raw)
In-Reply-To: <8736c9f1j6.fsf@intel.com>

On 2020-01-21 at 14:15:57 +0200, Jani Nikula wrote:
> On Tue, 21 Jan 2020, Ramalingam C <ramalingam.c@intel.com> wrote:
> > On 2020-01-20 at 11:19:54 +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.
> >> IGT patch to catch further regression on this features is being
> >> worked upon.
> >> 
> >> v2:
> >>  - Incorporated the necessary locking. (Ram)
> >>  - Set content protection property to CP_DESIRED only when
> >>    user has not asked explicitly to set CP_UNDESIRED.
> >> 
> >> v3:
> >>  - Reset the is_hdcp_undesired flag to false. (Ram)
> >>  - Rephrasing commit log and small comment for is_hdcp_desired
> >>    flag. (Ram)
> >> 
> >> CC: Ramalingam C <ramalingam.c@intel.com>
> >> Reviewed-by: Ramalingam C <ramalingam.c@intel.com>
> >> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_display_types.h |  6 ++++++
> >>  drivers/gpu/drm/i915/display/intel_hdcp.c          | 13 ++++++++++++-
> >>  2 files changed, 18 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> >> index 630a94892b7b..401a9a7689fb 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> >> @@ -345,6 +345,12 @@ struct intel_hdcp {
> >>  	struct delayed_work check_work;
> >>  	struct work_struct prop_work;
> >>  
> >> +	/*
> >> +	 * Flag to differentiate that HDCP is being disabled originated from
> >> +	 * userspace or triggered from kernel DDI disable sequence.
> >> +	 */
> >> +	bool is_hdcp_undesired;
> >> +
> >>  	/* HDCP1.4 Encryption status */
> >>  	bool hdcp_encrypted;
> >>  
> >> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> >> index 0fdbd39f6641..7f631ebd8395 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> >> @@ -2002,11 +2002,18 @@ int intel_hdcp_disable(struct intel_connector *connector)
> >>  	mutex_lock(&hdcp->mutex);
> >>  
> >>  	if (hdcp->value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> >> -		hdcp->value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
> >>  		if (hdcp->hdcp2_encrypted)
> >>  			ret = _intel_hdcp2_disable(connector);
> >>  		else if (hdcp->hdcp_encrypted)
> >>  			ret = _intel_hdcp_disable(connector);
> >> +
> >> +		if (hdcp->is_hdcp_undesired) {
> >> +			hdcp->value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
> >> +			hdcp->is_hdcp_undesired = false;
> >> +		} else {
> >> +			hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> >> +			schedule_work(&hdcp->prop_work);
> >> +		}
> >>  	}
> >>  
> >>  	mutex_unlock(&hdcp->mutex);
> >> @@ -2044,6 +2051,7 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> >>  {
> >>  	u64 old_cp = old_state->content_protection;
> >>  	u64 new_cp = new_state->content_protection;
> >> +	struct intel_connector *intel_conn = to_intel_connector(connector);
> >>  	struct drm_crtc_state *crtc_state;
> >>  
> >>  	if (!new_state->crtc) {
> >> @@ -2069,6 +2077,9 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> >>  			return;
> >>  	}
> >>  
> >> +	if (new_cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
> >> +		intel_conn->hdcp.is_hdcp_undesired  =  true;
> > This flag is reset at commit only. What if the atomic check failed?
> > Usually atomic check wont fail for HDCP state change. Possible if it is submitted with other request.
> > So we need to set true and false both here.
> 
> As I explained in my other mail, you don't have the information
> available at this point about possible later atomic check failures. I
> think it's wrong to change the connector at check phase. You'd need to
> be able to revert the change back to what it was... and that's exactly
> the reason why we have old and new states, so we can just throw away the
> new state if the check fails.
I completely understand that part. And this flag set at atomic_check
will not change the connector state, untill hdcp_disable called at
atomic commit.

I am completely fine if this method is not acceptable. But uAPI documented is
already broken (ENABLED "content protection" will remain DESIRED/ENABLED until userspace
set it to UNDESIRED). So I am just trying to find a solution to it.

Requirement is as follows:
ddi_disable()->hdcp_disable() gets called for two scenarios 
1. userspace setting the "content protection" to UNDESIRED hence
atomic_check will mark it as modeset hence hdcp will be disabled.
2. userspace is not changing the "content protection", but other display
operations are leading to the ddi_disable hence hdcp is getting disabled

We need to differentiate these two cause for hdcp disable at
hdcp_disable so that "content protection" can be set to "DESIRED" from
"ENABLED" at second case. else HDCP would have been disabled but
property will be left at "ENABLED".

In first scenario we dont need to change the "content protection" as the
userspace itself set it to "UNDESIRED". kernel just need to disable the
authentication.

So yes, this broken uAPI needs to be attended, not sure if anyone is
affected by this already. Please throw some light on the possible
direction from this point.

-Ram

> 
> BR,
> Jani.
> 
> 
> >
> > -Ram
> >> +
> >>  	crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
> >>  						   new_state->crtc);
> >>  	crtc_state->mode_changed = true;
> >> -- 
> >> 2.24.0
> >> 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx