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@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: Mon, 20 Jan 2020 16:30:44 +0530	[thread overview]
Message-ID: <20200120110044.GB14839@intel.com> (raw)
In-Reply-To: <87v9p6fmjz.fsf@intel.com>

On 2020-01-20 at 12:29:36 +0200, Jani Nikula wrote:
> On Mon, 20 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;
> > Jani and Daniel,
> >
> > This flag is added as we need to know the origin of the HDCP disable
> > (userspace or kernel modeset/DPMS off) at DDI disable sequence. We
> > couldn't do that as new_conn state is not available there to retrieve
> > the corresponding content protection state.
> >
> > Hence we do that at atomic check itself and pass the info through this flag,
> > which will be referred at hdcp_disable.
> >
> > If you think we could do it better please suggest the preferred
> > alternate method. Else I request your ack for merging this.
> 
> I don't know hdcp code all that well, but it seems to me at the root of
> the problem is the duplication of the content protection state
> (drm_connector_state->content_protection) into the connector
> (intel_hdcp->value), and them going out of sync.
> 
> If you relied on the connector state alone, you wouldn't have to worry
> about changing the intel_hdcp->value member at disable or anywhere;
> disable looks at old state and disables based on that. No history/future
> information needed. Isn't that roughly what everything else does, why is
> hdcp special?
As per uAPI designed for Chrome(first user of HDCP), after the userspace
request for HDCP by setting DESIRED to "content protection" only userspace can DISABLED it.

Incase when HDCP is ENABLED and kernel ended up in disabled the DDI (hot
unplug or DPMS ops), it supposed to disable HDCP encryption and leave the
"Content protection" at DESIRED. So that when the DDI is re enabled,
HDCP authentication will be attempted automatically, as userspace is
still interested in HDCP encryption.

So to set the state of "content protection" as  ENABLED->DESIRED, we
need to know the HDCP disable in DDI disable is not because of userspace request
(derived based on new connector state existance and its content protection state).

Hence we need this change.

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