Re: [PATCH v3 08/10] drm/i915: Populate downstream info for HDCP2.2

From: Daniel Vetter <daniel@ffwll.ch>
To: Ramalingam C <ramalingam.c@intel.com>
Cc: daniel.vetter@ffwll.ch, intel-gfx@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v3 08/10] drm/i915: Populate downstream info for HDCP2.2
Date: Wed, 27 Mar 2019 18:44:28 +0100	[thread overview]
Message-ID: <20190327174428.GN2665@phenom.ffwll.local> (raw)
In-Reply-To: <20190327144955.GC18417@intel.com>

On Wed, Mar 27, 2019 at 08:19:55PM +0530, Ramalingam C wrote:
> On 2019-03-27 at 11:31:12 +0100, Daniel Vetter wrote:
> > On Fri, Mar 22, 2019 at 06:14:46AM +0530, Ramalingam C wrote:
> > > Populates the downstream info for HDCP2.2 encryption also. On success
> > > of encryption Blob is updated.
> > > 
> > > Additional two variable are added to downstream info blob. Such as
> > > ver_in_force and content type.
> > > 
> > > v2:
> > >   s/cp_downstream/content_protection_downstream [daniel]
> > > v3:
> > >   s/content_protection_downstream/hdcp_topology [daniel]
> > > 
> > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_hdcp.c | 26 +++++++++++++++++++++++++-
> > >  include/uapi/drm/drm_mode.h       |  3 +++
> > >  2 files changed, 28 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > > index 0e3d7afecd5a..1eea553cdf81 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > @@ -1281,6 +1281,12 @@ static int hdcp2_authentication_key_exchange(struct intel_connector *connector)
> > >  		return -EPERM;
> > >  	}
> > >  
> > > +	hdcp->topology_info->ver_in_force = DRM_MODE_HDCP22_IN_FORCE;
> > > +	hdcp->topology_info->content_type = hdcp->content_type;
> > > +	memcpy(hdcp->topology_info->bksv, msgs.send_cert.cert_rx.receiver_id,
> > > +	       HDCP_2_2_RECEIVER_ID_LEN);
> > > +	hdcp->topology_info->is_repeater = hdcp->is_repeater;
> > > +
> > >  	/*
> > >  	 * Here msgs.no_stored_km will hold msgs corresponding to the km
> > >  	 * stored also.
> > > @@ -1472,6 +1478,11 @@ int hdcp2_authenticate_repeater_topology(struct intel_connector *connector)
> > >  		return -EPERM;
> > >  	}
> > >  
> > > +	hdcp->topology_info->device_count = device_cnt;
> > > +	hdcp->topology_info->depth = HDCP_2_2_DEPTH(rx_info[0]);
> > > +	memcpy(hdcp->topology_info->ksv_list, msgs.recvid_list.receiver_ids,
> > > +	       device_cnt * HDCP_2_2_RECEIVER_ID_LEN);
> > > +
> > >  	ret = hdcp2_verify_rep_topology_prepare_ack(connector,
> > >  						    &msgs.recvid_list,
> > >  						    &msgs.rep_ack);
> > > @@ -1658,6 +1669,12 @@ static int _intel_hdcp2_enable(struct intel_connector *connector)
> > >  	if (ret) {
> > >  		DRM_DEBUG_KMS("HDCP2 Type%d  Enabling Failed. (%d)\n",
> > >  			      hdcp->content_type, ret);
> > > +
> > > +		memset(hdcp->topology_info, 0,
> > > +		       sizeof(struct hdcp_topology_info));
> > > +		drm_connector_update_hdcp_topology_property(&connector->base,
> > > +							  hdcp->topology_info);
> > > +
> > >  		return ret;
> > >  	}
> > >  
> > > @@ -1665,12 +1682,16 @@ static int _intel_hdcp2_enable(struct intel_connector *connector)
> > >  		      connector->base.name, connector->base.base.id,
> > >  		      hdcp->content_type);
> > >  
> > > +	drm_connector_update_hdcp_topology_property(&connector->base,
> > > +						    hdcp->topology_info);
> > >  	hdcp->hdcp2_encrypted = true;
> > > +
> > >  	return 0;
> > >  }
> > >  
> > >  static int _intel_hdcp2_disable(struct intel_connector *connector)
> > >  {
> > > +	struct intel_hdcp *hdcp = &connector->hdcp;
> > >  	int ret;
> > >  
> > >  	DRM_DEBUG_KMS("[%s:%d] HDCP2.2 is being Disabled\n",
> > > @@ -1681,8 +1702,11 @@ static int _intel_hdcp2_disable(struct intel_connector *connector)
> > >  	if (hdcp2_deauthenticate_port(connector) < 0)
> > >  		DRM_DEBUG_KMS("Port deauth failed.\n");
> > >  
> > > -	connector->hdcp.hdcp2_encrypted = false;
> > > +	hdcp->hdcp2_encrypted = false;
> > >  
> > > +	memset(hdcp->topology_info, 0, sizeof(struct hdcp_topology_info));
> > > +	drm_connector_update_hdcp_topology_property(&connector->base,
> > > +						    hdcp->topology_info);
> > >  	return ret;
> > >  }
> > >  
> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > index 733db7283e57..22f5940a47cc 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -223,6 +223,9 @@ struct hdcp_topology_info {
> > >  	/* Version of HDCP authenticated (1.4/2.2) */
> > >  	__u32 ver_in_force;
> > >  
> > > +	/* Applicable only for HDCP2.2 */
> > > +	__u8 content_type;
> > 
> > Even more misplaced hunks. You can't change an uapi struct once it's baked
> > in.
> I did this under the assumption that the whole series completes the
> uAPI.

Unusual approach, since if someone e.g. cherry-picks only hdcp1.x support,
then they end up with different uapi. Also makes it much harder to review
structure alignment and stuff like that.

We can generally extend uapi structs later on, but only at the end. And
unfortunately not for properties.

> > Same thought as on the hdcp1 patch, perhaps we even want 2 functions: One
> > for hdcp1, and one for hdcp2, to avoid the possiblity of screw-ups. After
> > all this interface is fairly fragile, and supposed to work across drivers.
> > 
> > Aside: Where's the userspace for this? Kinda wondering how an open source
> > hdcp repeater implementation would even work ...
> I am afraid that we wont be able to preare the userspace for the
> topology property. So we wont be able upstream this new uAPI. :(

Is there some other useful thing we can do with this? Being able to
support the repeater usecase is kinda neat ... But yeah I guess that needs
access to private hdcp key and all that.
-Daniel

> 
> -Ram
> > -Daniel
> > 
> > > +
> > >  	/* KSV of immediate HDCP Sink. In Little-Endian Format. */
> > >  	char bksv[DRM_MODE_HDCP_KSV_LEN];
> > >  
> > > -- 
> > > 2.19.1
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx