| Message ID | 20220411204741.1074308-4-sean@poorly.run (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/hdcp: Pull HDCP auth/exchange/check into helpers | expand |
On Mon, Apr 11, 2022 at 08:47:32PM +0000, Sean Paul wrote: > From: Sean Paul <seanpaul@chromium.org> > > This patch updates the connector's property value in 2 cases which were > previously missed: > > 1- Content type changes. The value should revert back to DESIRED from > ENABLED in case the driver must re-authenticate the link due to the > new content type. > > 2- Userspace sets value to DESIRED while ENABLED. In this case, the > value should be reset immediately to ENABLED since the link is > actively being encrypted. > > To accommodate these changes, I've split up the conditionals to make > things a bit more clear (as much as one can with this mess of state). > > Acked-by: Jani Nikula <jani.nikula@intel.com> > Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org> > Signed-off-by: Sean Paul <seanpaul@chromium.org> > Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-4-sean@poorly.run #v1 > Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-4-sean@poorly.run #v2 > Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-4-sean@poorly.run #v3 > Link: https://patchwork.freedesktop.org/patch/msgid/20211105030434.2828845-4-sean@poorly.run #v4 > > Changes in v2: > -None > Changes in v3: > -Fixed indentation issue identified by 0-day > Changes in v4: > -None > Changes in v5: > -None > --- > drivers/gpu/drm/drm_hdcp.c | 26 +++++++++++++++++--------- > 1 file changed, 17 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c > index dd8fa91c51d6..8c851d40cd45 100644 > --- a/drivers/gpu/drm/drm_hdcp.c > +++ b/drivers/gpu/drm/drm_hdcp.c > @@ -487,21 +487,29 @@ bool drm_hdcp_atomic_check(struct drm_connector *connector, > return true; > > /* > - * Nothing to do if content type is unchanged and one of: > - * - state didn't change > + * Content type changes require an HDCP disable/enable cycle. > + */ > + if (new_conn_state->hdcp_content_type != old_conn_state->hdcp_content_type) { shouldn't we add some && ( old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED)) { here? > + new_conn_state->content_protection = > + DRM_MODE_CONTENT_PROTECTION_DESIRED; > + return true; > + } > + > + /* > + * Ignore meaningless state changes: > * - HDCP was activated since the last commit > - * - attempting to set to desired while already enabled > + * - Attempting to set to desired while already enabled > */ > - if (old_hdcp == new_hdcp || > - (old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED && > + if ((old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED && > new_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) || > (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED && > new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) { > - if (old_conn_state->hdcp_content_type == > - new_conn_state->hdcp_content_type) > - return false; > + new_conn_state->content_protection = > + DRM_MODE_CONTENT_PROTECTION_ENABLED; > + return false; > } > > - return true; > + /* Finally, if state changes, we need action */ > + return old_hdcp != new_hdcp; > } > EXPORT_SYMBOL(drm_hdcp_atomic_check); > -- > Sean Paul, Software Engineer, Google / Chromium OS >
On Tue, Apr 12, 2022 at 09:25:59AM -0400, Rodrigo Vivi wrote: > On Mon, Apr 11, 2022 at 08:47:32PM +0000, Sean Paul wrote: > > From: Sean Paul <seanpaul@chromium.org> > > > > This patch updates the connector's property value in 2 cases which were > > previously missed: > > > > 1- Content type changes. The value should revert back to DESIRED from > > ENABLED in case the driver must re-authenticate the link due to the > > new content type. > > > > 2- Userspace sets value to DESIRED while ENABLED. In this case, the > > value should be reset immediately to ENABLED since the link is > > actively being encrypted. > > > > To accommodate these changes, I've split up the conditionals to make > > things a bit more clear (as much as one can with this mess of state). > > > > Acked-by: Jani Nikula <jani.nikula@intel.com> > > Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org> > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-4-sean@poorly.run #v1 > > Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-4-sean@poorly.run #v2 > > Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-4-sean@poorly.run #v3 > > Link: https://patchwork.freedesktop.org/patch/msgid/20211105030434.2828845-4-sean@poorly.run #v4 > > > > Changes in v2: > > -None > > Changes in v3: > > -Fixed indentation issue identified by 0-day > > Changes in v4: > > -None > > Changes in v5: > > -None > > --- > > drivers/gpu/drm/drm_hdcp.c | 26 +++++++++++++++++--------- > > 1 file changed, 17 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c > > index dd8fa91c51d6..8c851d40cd45 100644 > > --- a/drivers/gpu/drm/drm_hdcp.c > > +++ b/drivers/gpu/drm/drm_hdcp.c > > @@ -487,21 +487,29 @@ bool drm_hdcp_atomic_check(struct drm_connector *connector, > > return true; > > > > /* > > - * Nothing to do if content type is unchanged and one of: > > - * - state didn't change > > + * Content type changes require an HDCP disable/enable cycle. > > + */ > > + if (new_conn_state->hdcp_content_type != old_conn_state->hdcp_content_type) { > > shouldn't we add some && ( old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED)) { > here? Thanks for your reviews Rodrigo. I don't think so since the content type is changing the current state of old content protection is immaterial (ie: if we need to enable HDCP 2.x, the state of HDCP 1.x doesn't really matter), we need to re-evaluate whether the current level of HDCP is sufficient. Hopefully that makes sense, but I could be missing something :-) Sean > > > + new_conn_state->content_protection = > > + DRM_MODE_CONTENT_PROTECTION_DESIRED; > > + return true; > > + } > > + > > + /* > > + * Ignore meaningless state changes: > > * - HDCP was activated since the last commit > > - * - attempting to set to desired while already enabled > > + * - Attempting to set to desired while already enabled > > */ > > - if (old_hdcp == new_hdcp || > > - (old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED && > > + if ((old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED && > > new_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) || > > (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED && > > new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) { > > - if (old_conn_state->hdcp_content_type == > > - new_conn_state->hdcp_content_type) > > - return false; > > + new_conn_state->content_protection = > > + DRM_MODE_CONTENT_PROTECTION_ENABLED; > > + return false; > > } > > > > - return true; > > + /* Finally, if state changes, we need action */ > > + return old_hdcp != new_hdcp; > > } > > EXPORT_SYMBOL(drm_hdcp_atomic_check); > > -- > > Sean Paul, Software Engineer, Google / Chromium OS > >
On Thu, Apr 14, 2022 at 03:58:02PM +0000, Sean Paul wrote: > On Tue, Apr 12, 2022 at 09:25:59AM -0400, Rodrigo Vivi wrote: > > On Mon, Apr 11, 2022 at 08:47:32PM +0000, Sean Paul wrote: > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > This patch updates the connector's property value in 2 cases which were > > > previously missed: > > > > > > 1- Content type changes. The value should revert back to DESIRED from > > > ENABLED in case the driver must re-authenticate the link due to the > > > new content type. > > > > > > 2- Userspace sets value to DESIRED while ENABLED. In this case, the > > > value should be reset immediately to ENABLED since the link is > > > actively being encrypted. > > > > > > To accommodate these changes, I've split up the conditionals to make > > > things a bit more clear (as much as one can with this mess of state). > > > > > > Acked-by: Jani Nikula <jani.nikula@intel.com> > > > Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org> > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-4-sean@poorly.run #v1 > > > Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-4-sean@poorly.run #v2 > > > Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-4-sean@poorly.run #v3 > > > Link: https://patchwork.freedesktop.org/patch/msgid/20211105030434.2828845-4-sean@poorly.run #v4 > > > > > > Changes in v2: > > > -None > > > Changes in v3: > > > -Fixed indentation issue identified by 0-day > > > Changes in v4: > > > -None > > > Changes in v5: > > > -None > > > --- > > > drivers/gpu/drm/drm_hdcp.c | 26 +++++++++++++++++--------- > > > 1 file changed, 17 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c > > > index dd8fa91c51d6..8c851d40cd45 100644 > > > --- a/drivers/gpu/drm/drm_hdcp.c > > > +++ b/drivers/gpu/drm/drm_hdcp.c > > > @@ -487,21 +487,29 @@ bool drm_hdcp_atomic_check(struct drm_connector *connector, > > > return true; > > > > > > /* > > > - * Nothing to do if content type is unchanged and one of: > > > - * - state didn't change > > > + * Content type changes require an HDCP disable/enable cycle. > > > + */ > > > + if (new_conn_state->hdcp_content_type != old_conn_state->hdcp_content_type) { > > > > shouldn't we add some && ( old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED)) { > > here? > > Thanks for your reviews Rodrigo. > > I don't think so since the content type is changing the current state of old > content protection is immaterial (ie: if we need to enable HDCP 2.x, the state > of HDCP 1.x doesn't really matter), we need to re-evaluate whether the current > level of HDCP is sufficient. > > Hopefully that makes sense, but I could be missing something :-) nope, I think I was just missing that the important is the content type change as you pointed out in the item number 1 in the commit msg. Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Sean > > > > > > + new_conn_state->content_protection = > > > + DRM_MODE_CONTENT_PROTECTION_DESIRED; > > > + return true; > > > + } > > > + > > > + /* > > > + * Ignore meaningless state changes: > > > * - HDCP was activated since the last commit > > > - * - attempting to set to desired while already enabled > > > + * - Attempting to set to desired while already enabled > > > */ > > > - if (old_hdcp == new_hdcp || > > > - (old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED && > > > + if ((old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED && > > > new_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) || > > > (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED && > > > new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) { > > > - if (old_conn_state->hdcp_content_type == > > > - new_conn_state->hdcp_content_type) > > > - return false; > > > + new_conn_state->content_protection = > > > + DRM_MODE_CONTENT_PROTECTION_ENABLED; > > > + return false; > > > } > > > > > > - return true; > > > + /* Finally, if state changes, we need action */ > > > + return old_hdcp != new_hdcp; > > > } > > > EXPORT_SYMBOL(drm_hdcp_atomic_check); > > > -- > > > Sean Paul, Software Engineer, Google / Chromium OS > > > > > -- > Sean Paul, Software Engineer, Google / Chromium OS
diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c index dd8fa91c51d6..8c851d40cd45 100644 --- a/drivers/gpu/drm/drm_hdcp.c +++ b/drivers/gpu/drm/drm_hdcp.c @@ -487,21 +487,29 @@ bool drm_hdcp_atomic_check(struct drm_connector *connector, return true; /* - * Nothing to do if content type is unchanged and one of: - * - state didn't change + * Content type changes require an HDCP disable/enable cycle. + */ + if (new_conn_state->hdcp_content_type != old_conn_state->hdcp_content_type) { + new_conn_state->content_protection = + DRM_MODE_CONTENT_PROTECTION_DESIRED; + return true; + } + + /* + * Ignore meaningless state changes: * - HDCP was activated since the last commit - * - attempting to set to desired while already enabled + * - Attempting to set to desired while already enabled */ - if (old_hdcp == new_hdcp || - (old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED && + if ((old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED && new_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) || (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED && new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) { - if (old_conn_state->hdcp_content_type == - new_conn_state->hdcp_content_type) - return false; + new_conn_state->content_protection = + DRM_MODE_CONTENT_PROTECTION_ENABLED; + return false; } - return true; + /* Finally, if state changes, we need action */ + return old_hdcp != new_hdcp; } EXPORT_SYMBOL(drm_hdcp_atomic_check);