| Message ID | 20200120054954.5786-1-anshuman.gupta@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/i915/hdcp: Update CP as per the kernel internal state | expand |
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. 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 >
On Mon, 20 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> 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; > + First, the comment and the name of the member don't align. The member name does not explain what it is, and you absolutely need to look at the comment for it to make sense in code. > /* 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); > + } Second, all the places you call intel_hdcp_disable() from have access to the connector state, so IIUC you could deduce the state from there and pass it to intel_hdcp_disable() without having to add another flag/sub-state such as ->is_hdcp_undesired. Third, reading the code, I am not sure I fully appreciate why there is a duplicated hdcp->value, instead of using the content_protection member from connector state to begin with. Between those two and the one added in this patch, looks like there's a lot of state that could go out of sync. > } > > 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; > + Finally, this underlines the above points. If atomic check fails later for some unrelated reason, you've still ended up changing the connector. You'll tell userspace things failed, but mysteriously things will change anyway. It is almost never a good idea to add more state variables if you can figure the same thing from the single point of truth. BR, Jani. > crtc_state = drm_atomic_get_new_crtc_state(new_state->state, > new_state->crtc); > crtc_state->mode_changed = true;
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? 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 >>
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
On Mon, 20 Jan 2020, Ramalingam C <ramalingam.c@intel.com> wrote: > 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. 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? 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
On 2020-01-20 at 13:24:05 +0200, Jani Nikula wrote: > On Mon, 20 Jan 2020, Ramalingam C <ramalingam.c@intel.com> wrote: > > 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. > > 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
On Mon, 20 Jan 2020, Ramalingam C <ramalingam.c@intel.com> wrote: > 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. My point is this: How many states does your state machine need? Considering the tri-state content_protection and tri-state hdcp->value, you have 9 possible states and 362880 transitions. Add the new flag from this patch, and you have 18 possible states and 6e15 transitions. Obviously you don't need or use that many states or transitions, but you need the code to limit the states and the transitions. You need the review to ensure any changes take into account all the possible states and transitions. I've already noted one possible scenario in the proposed patch where stuff goes out of sync, and I don't know what's really going to happen. --- So maybe I don't understand what the hdcp code does, but then maybe you shouldn't ask me to have a look at it... ;) I'm trying to point out why I think it's maybe difficult to understand, and why I think adding another flag might make it more difficult to maintain. BR, Jani.
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. -Ram > + > crtc_state = drm_atomic_get_new_crtc_state(new_state->state, > new_state->crtc); > crtc_state->mode_changed = true; > -- > 2.24.0 >
On 2020-01-20 at 15:02:46 +0200, Jani Nikula wrote: > On Mon, 20 Jan 2020, Ramalingam C <ramalingam.c@intel.com> wrote: > > 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. > > My point is this: How many states does your state machine need? > > Considering the tri-state content_protection and tri-state hdcp->value, > you have 9 possible states and 362880 transitions. Add the new flag from > this patch, and you have 18 possible states and 6e15 transitions. > > Obviously you don't need or use that many states or transitions, but you > need the code to limit the states and the transitions. You need the > review to ensure any changes take into account all the possible states > and transitions. Yes. We dont use all those combination. Agreed that we need to review the changes which impacts the updatation of hdcp state of internal state machine or property state. > > I've already noted one possible scenario in the proposed patch where > stuff goes out of sync, and I don't know what's really going to happen. > > --- > > So maybe I don't understand what the hdcp code does, but then maybe you > shouldn't ask me to have a look at it... ;) I'm trying to point out why > I think it's maybe difficult to understand, and why I think adding > another flag might make it more difficult to maintain. Even we tried alternate solutions like retrieving the new drm_connector state at DDI disable. We dropped it as that wont be received well. Any suggestion on alternate approach to get the new connector state at DDI disable call. -Ram > > BR, > Jani. > > -- > Jani Nikula, Intel Open Source Graphics Center
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. 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
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
On Wed, 22 Jan 2020, Ramalingam C <ramalingam.c@intel.com> wrote: > 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. I still think a large part of your problems are due to duplicating the content_protection state to hdcp->value. You *are* setting new state content_protection to DESIRED at intel_hdcp_atomic_check() if it was ENABLED. This should work for both of your scenarios; I think it's only broken because you lose sync with hdcp->value. intel_hdcp_disable() should look at old state and disable if hdcp is enabled. intel_hdcp_enable() should look at new state and enable if hdcp is desired. BR, Jani. > > -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
On 2020-01-22 at 16:56:06 +0200, Jani Nikula wrote: > On Wed, 22 Jan 2020, Ramalingam C <ramalingam.c@intel.com> wrote: > > 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. > > I still think a large part of your problems are due to duplicating the > content_protection state to hdcp->value. > > You *are* setting new state content_protection to DESIRED at > intel_hdcp_atomic_check() if it was ENABLED. This should work for both IMHO we are setting new state to DESRIED if it was ENABLED for disabled connectors. Earlier i tried similar way in order to fix this issue if (needs_modeset(new_crtc_state)) /* check this after fastset check has been done in atomic check */ for_each_oldnew_connector_in_state(&state->base, connector, old_conn_state, new_conn_state, i) { if (old_conn_state->content_protection == DRM_MODE_CONTENT_PROTECTION_ENABLED && new_conn_state->content_protection != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) new_conn_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED; } but the above solution was missing the uevent part, so i discarded it. As per uapi doc we still need to send the uevent to notify if there is change in state from ENABLED -> DESIRED. drivers/gpu/drm/drm_connector.c +1011 (uapi) > of your scenarios; I think it's only broken because you lose sync with > hdcp->value. I think with above solution fixes broken uapi except uevent part. I will send a new patch with above solution with FIXME tag for uevent. Thanks, Anshuman Gupta. > > intel_hdcp_disable() should look at old state and disable if hdcp is > enabled. > > intel_hdcp_enable() should look at new state and enable if hdcp is > desired. > > > BR, > Jani. > > > > > > > -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 > > -- > Jani Nikula, Intel Open Source Graphics Center
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; + crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); crtc_state->mode_changed = true;