Re: [PATCH v5 03/10] drm/hdcp: Update property value on content type and user changes

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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