On 2020-01-20 at 13:24:05 +0200, Jani Nikula wrote:
> On Mon, 20 Jan 2020, Ramalingam C <ramalingam.c@xxxxxxxxx> wrote:
> > On 2020-01-20 at 12:29:36 +0200, Jani Nikula wrote:
> >> On Mon, 20 Jan 2020, Ramalingam C <ramalingam.c@xxxxxxxxx> 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@xxxxxxxxx>
> >> >> Reviewed-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
> >> >> Signed-off-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
> >> >> ---
> >> >> 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.
>
> Okay, why do you need to track desired/undesired in intel_hdcp then?
> Just track enabled/disabled, and do everything else based on connector
> state?
Jani,
hdcp->value is used to track the internal transistions during the link
failure and re-establishing it. When ever kernel want to update the
"content protection" we take the required locks and update the property
state along with uevent.
-Ram
>
> BR,
> Jani.
>
>
> >
> > -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
>
> --
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
