<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p><br>
</p>
<div class="moz-cite-prefix">On 12/7/2018 7:59 PM, Daniel Vetter
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:20181207142950.GE21184@phenom.ffwll.local">
<pre class="moz-quote-pre" wrap="">On Fri, Dec 07, 2018 at 11:22:44AM +0530, C, Ramalingam wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
On 12/6/2018 3:53 PM, Daniel Vetter wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Defining the mei-i915 interface functions and initialization of
the interface.
Signed-off-by: Ramalingam C <a class="moz-txt-link-rfc2396E" href="mailto:ramalingam.c@intel.com"><ramalingam.c@intel.com></a>
Signed-off-by: Tomas Winkler <a class="moz-txt-link-rfc2396E" href="mailto:tomas.winkler@intel.com"><tomas.winkler@intel.com></a>
---
drivers/gpu/drm/i915/i915_drv.h | 2 +
drivers/gpu/drm/i915/intel_drv.h | 7 +
drivers/gpu/drm/i915/intel_hdcp.c | 442 +++++++++++++++++++++++++++++++++++++-
include/drm/i915_component.h | 71 ++++++
4 files changed, 521 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f763b30f98d9..b68bc980b7cd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2015,6 +2015,8 @@ struct drm_i915_private {
struct i915_pmu pmu;
+ struct i915_hdcp_component_master *hdcp_comp;
+
/*
* NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
* will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 85a526598096..bde82f3ada85 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -29,6 +29,7 @@
#include <linux/i2c.h>
#include <linux/hdmi.h>
#include <linux/sched/clock.h>
+#include <linux/mei_hdcp.h>
#include <drm/i915_drm.h>
#include "i915_drv.h"
#include <drm/drm_crtc.h>
@@ -379,6 +380,9 @@ struct intel_hdcp_shim {
/* Detects panel's hdcp capability. This is optional for HDMI. */
int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
bool *hdcp_capable);
+
+ /* Detects the HDCP protocol(DP/HDMI) required on the port */
+ enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Looking ahead, this seems hardwired to constant return value? Or why do we
need a function here?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
This is hardwired based on the connector type(DP/HDMI).
Since we have the shim for hdcp's connector based work, I have added this function.
Could have done this just with connector_type check, but in that way whole hdcp_shim
can be done in that way. So going with the larger design here.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
If it's hardwired then just make it a hardwired struct member. As long as
it's all const, we can mix data an function pointers. If you have runtime
variable data, then it's better to split it out from the ops structure, so
that we can keep ops read-only and protected against possible exploits
(any function pointers are a high-value target in the kernel).</pre>
</blockquote>
Sure. Done.
<blockquote type="cite"
cite="mid:20181207142950.GE21184@phenom.ffwll.local">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> };
struct intel_hdcp {
@@ -399,6 +403,9 @@ struct intel_hdcp {
* content can flow only through a link protected by HDCP2.2.
*/
u8 content_type;
+
+ /* mei interface related information */
+ struct mei_hdcp_data mei_data;
};
struct intel_connector {
diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
index 99dddb540958..760780f1105c 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -8,14 +8,20 @@
#include <drm/drmP.h>
#include <drm/drm_hdcp.h>
+#include <drm/i915_component.h>
#include <linux/i2c.h>
#include <linux/random.h>
+#include <linux/component.h>
#include "intel_drv.h"
#include "i915_reg.h"
#define KEY_LOAD_TRIES 5
#define TIME_FOR_ENCRYPT_STATUS_CHANGE 50
+#define GET_MEI_DDI_INDEX(p) ({ \
+ typeof(p) __p = (p); \
+ __p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
+})
static
bool intel_hdcp_is_ksv_valid(u8 *ksv)
@@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
!IS_CHERRYVIEW(dev_priv) && port < PORT_E);
}
+static __attribute__((unused)) int
+hdcp2_prepare_ake_init(struct intel_connector *connector,
+ struct hdcp2_ake_init *ake_data)
+{
+ struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+ struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+ int ret;
+
+ if (!comp)
+ return -EINVAL;
+
+ mutex_lock(&comp->mutex);
+ if (!comp->ops || !comp->dev) {
+ mutex_unlock(&comp->mutex);
+ return -EINVAL;
+ }
+
+ if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
+ data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
+
+ /* Clear ME FW instance for the port, just incase */
+ comp->ops->close_hdcp_session(comp->dev, data);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Sounds like a bug somewhere if we need this? I'd put this code into the
->initiate_hdcp2_session, with a big WARN_ON if it's actually needed.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Not really. Lets say you have progressed beyond the first step of HDCP2.2 auth along with ME FW.
Now if authentication failed due to some reason, then the HDCP2.2 season created with
ME FW for that port is not closed yet.
So we need to call close_hdcp_session() explicitly on authentication failures.
Session has to be closed before restarting the auth on that port with initialite_hdcp_session.
If we are closing the hdcp session of the port on all auth errors, we dont need this just before
start of the hdcp session.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Yeah, makes sense. But right now the close_session stuff is sprinkled all
over, so it's hard to see what's going on and whether there's bugs
somewhere. I much prefer code that looks like it knows what it's doing. I
think one top-level close_session call in the main hdcp2 functions (called
when everything is done, or on failure) would be much cleaner.</pre>
</blockquote>
Ok. Sure.<br>
<blockquote type="cite"
cite="mid:20181207142950.GE21184@phenom.ffwll.local">
<pre class="moz-quote-pre" wrap="">
Plus then some WARN_ON to make sure we never forget to close a session.
Sprinkling cleanup code all over the place looks like it's easier, but
long term it's much harder code to maintain and refactor because you never
sure which cleanup code is actually doing the cleanup, and which cleanup
code is just there for nothing.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
"Just in case" papering over programming bugs of our own just makes
debugging harder.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+ ret = comp->ops->initiate_hdcp2_session(comp->dev,
+ data, ake_data);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">We should set the port only after successfully initializing this.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
hdcp2_session is created for each port at ME FW. Hence we need the port
initialized even before calling the initiate_hdcp2_session.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
+ data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
This line right above in your patch should only run on success. I think.
Also, looking at this again: Why could connector->encoder ever be NULL?
That smells like another possible bug in our setup code. Better to wrap
this in a WARN_ON and bail out.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ mutex_unlock(&comp->mutex);
+
+ return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
+ struct hdcp2_ake_send_cert *rx_cert,
+ bool *paired,
+ struct hdcp2_ake_no_stored_km *ek_pub_km,
+ size_t *msg_sz)
+{
+ struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+ struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+ int ret;
+
+ if (!comp)
+ return -EINVAL;
+
+ mutex_lock(&comp->mutex);
+ if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">These all look like programming mistakes that should result in a WARN_ON.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
We are using the comp->mutex for protecting the interface during the interface
init, usage for mei communication and interface teardown.
But what if mei interface teardown happens between mei communications?
So when we try to access the mei interface after such possible tear down scenario,
we are checking the integrity of the interface.
Possible alternate solution is hold the comp->mutex across the authentication steps.
But consequence is that mei module removal will be blocked for authentication duration
and even if the mei_dev is removed, component unbind will be blocked due to this mutex dependency.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I think with the v7 component code this should all be impossible and we
can remove it. With the v8 component code it's definitely necessary (which
is why I don't like the v8 component code). This is all because I didn't
fully realize the changes with v8 when reading the patches again ...</pre>
</blockquote>
Got addressed as soon as we move to version implemented in v7.<br>
<blockquote type="cite"
cite="mid:20181207142950.GE21184@phenom.ffwll.local">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ mutex_unlock(&comp->mutex);
+ return -EINVAL;
+ }
+
+ ret = comp->ops->verify_receiver_cert_prepare_km(comp->dev, data,
+ rx_cert, paired,
+ ek_pub_km, msg_sz);
+ if (ret < 0)
+ comp->ops->close_hdcp_session(comp->dev, data);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Error handling here seems a bit strange. You close the session, but don't
reset the port. So next op will be totally unaware that things have blown
up. Also no warning.
If we want to close the session, then I think that should be a decision
made higher up.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
This is needed as explained above. But as you have mentioned this can be moved
to the end of the authentication on error scenario. I will work on that.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Yeah, I think that'll lead to cleaner code.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ mutex_unlock(&comp->mutex);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">With component do we still need this mutex stuff here?
Exact same comments everywhere below.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+ return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_verify_hprime(struct intel_connector *connector,
+ struct hdcp2_ake_send_hprime *rx_hprime)
+{
+ struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+ struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+ int ret;
+
+ if (!comp)
+ return -EINVAL;
+
+ mutex_lock(&comp->mutex);
+ if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+ mutex_unlock(&comp->mutex);
+ return -EINVAL;
+ }
+
+ ret = comp->ops->verify_hprime(comp->dev, data, rx_hprime);
+ if (ret < 0)
+ comp->ops->close_hdcp_session(comp->dev, data);
+ mutex_unlock(&comp->mutex);
+
+ return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_store_pairing_info(struct intel_connector *connector,
+ struct hdcp2_ake_send_pairing_info *pairing_info)
+{
+ struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+ struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+ int ret;
+
+ if (!comp)
+ return -EINVAL;
+
+ mutex_lock(&comp->mutex);
+ if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+ mutex_unlock(&comp->mutex);
+ return -EINVAL;
+ }
+
+ ret = comp->ops->store_pairing_info(comp->dev,
+ data, pairing_info);
+ if (ret < 0)
+ comp->ops->close_hdcp_session(comp->dev, data);
+ mutex_unlock(&comp->mutex);
+
+ return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_prepare_lc_init(struct intel_connector *connector,
+ struct hdcp2_lc_init *lc_init)
+{
+ struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+ struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+ int ret;
+
+ if (!comp)
+ return -EINVAL;
+
+ mutex_lock(&comp->mutex);
+ if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+ mutex_unlock(&comp->mutex);
+ return -EINVAL;
+ }
+
+ ret = comp->ops->initiate_locality_check(comp->dev,
+ data, lc_init);
+ if (ret < 0)
+ comp->ops->close_hdcp_session(comp->dev, data);
+ mutex_unlock(&comp->mutex);
+
+ return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_verify_lprime(struct intel_connector *connector,
+ struct hdcp2_lc_send_lprime *rx_lprime)
+{
+ struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+ struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+ int ret;
+
+ if (!comp)
+ return -EINVAL;
+
+ mutex_lock(&comp->mutex);
+ if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+ mutex_unlock(&comp->mutex);
+ return -EINVAL;
+ }
+
+ ret = comp->ops->verify_lprime(comp->dev, data, rx_lprime);
+ if (ret < 0)
+ comp->ops->close_hdcp_session(comp->dev, data);
+ mutex_unlock(&comp->mutex);
+
+ return ret;
+}
+
+static __attribute__((unused))
+int hdcp2_prepare_skey(struct intel_connector *connector,
+ struct hdcp2_ske_send_eks *ske_data)
+{
+ struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+ struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+ int ret;
+
+ if (!comp)
+ return -EINVAL;
+
+ mutex_lock(&comp->mutex);
+ if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+ mutex_unlock(&comp->mutex);
+ return -EINVAL;
+ }
+
+ ret = comp->ops->get_session_key(comp->dev, data, ske_data);
+ if (ret < 0)
+ comp->ops->close_hdcp_session(comp->dev, data);
+ mutex_unlock(&comp->mutex);
+
+ return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_verify_rep_topology_prepare_ack(struct intel_connector *connector,
+ struct hdcp2_rep_send_receiverid_list
+ *rep_topology,
+ struct hdcp2_rep_send_ack *rep_send_ack)
+{
+ struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+ struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+ int ret;
+
+ if (!comp)
+ return -EINVAL;
+
+ mutex_lock(&comp->mutex);
+ if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+ mutex_unlock(&comp->mutex);
+ return -EINVAL;
+ }
+
+ ret = comp->ops->repeater_check_flow_prepare_ack(comp->dev,
+ data, rep_topology,
+ rep_send_ack);
+ if (ret < 0)
+ comp->ops->close_hdcp_session(comp->dev, data);
+ mutex_unlock(&comp->mutex);
+
+ return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_verify_mprime(struct intel_connector *connector,
+ struct hdcp2_rep_stream_ready *stream_ready)
+{
+ struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+ struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+ int ret;
+
+ if (!comp)
+ return -EINVAL;
+
+ mutex_lock(&comp->mutex);
+ if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+ mutex_unlock(&comp->mutex);
+ return -EINVAL;
+ }
+
+ ret = comp->ops->verify_mprime(comp->dev, data, stream_ready);
+ if (ret < 0)
+ comp->ops->close_hdcp_session(comp->dev, data);
+ mutex_unlock(&comp->mutex);
+
+ return ret;
+}
+
+static __attribute__((unused))
+int hdcp2_authenticate_port(struct intel_connector *connector)
+{
+ struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+ struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+ int ret;
+
+ if (!comp)
+ return -EINVAL;
+
+ mutex_lock(&comp->mutex);
+ if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+ mutex_unlock(&comp->mutex);
+ return -EINVAL;
+ }
+
+ ret = comp->ops->enable_hdcp_authentication(comp->dev, data);
+ if (ret < 0)
+ comp->ops->close_hdcp_session(comp->dev, data);
+ mutex_unlock(&comp->mutex);
+
+ return ret;
+}
+
+static __attribute__((unused))
+int hdcp2_close_mei_session(struct intel_connector *connector)
+{
+ struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+ struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+ int ret;
+
+ if (!comp)
+ return -EINVAL;
+
+ mutex_lock(&comp->mutex);
+ if (!comp->ops || !comp->dev ||
+ data->port == MEI_DDI_INVALID_PORT) {
+ mutex_unlock(&comp->mutex);
+ return -EINVAL;
+ }
+
+ ret = comp->ops->close_hdcp_session(comp->dev, data);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Need to reset the port here I think.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
What is the reset of the port you are referring to? port is not
configured for encryption. And this is the call for marking the port as de-authenticated too.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ mutex_unlock(&comp->mutex);
+ return ret;
+}
+
+static __attribute__((unused))
+int hdcp2_deauthenticate_port(struct intel_connector *connector)
+{
+ return hdcp2_close_mei_session(connector);
+}
+
+static int i915_hdcp_component_master_bind(struct device *dev)
+{
+ struct drm_i915_private *dev_priv = kdev_to_i915(dev);
+
+ return component_bind_all(dev, dev_priv->hdcp_comp);
+}
+
+static void intel_connectors_hdcp_disable(struct drm_i915_private *dev_priv)
+{
+ struct drm_device *dev = &dev_priv->drm;
+ struct intel_connector *intel_connector;
+ struct drm_connector *connector;
+ struct drm_connector_list_iter conn_iter;
+
+ drm_connector_list_iter_begin(dev, &conn_iter);
+ drm_for_each_connector_iter(connector, &conn_iter) {
+ intel_connector = to_intel_connector(connector);
+ if (!(intel_connector->hdcp.shim))
+ continue;
+
+ intel_hdcp_disable(intel_connector);
+ }
+ drm_connector_list_iter_end(&conn_iter);
+}
+
+static void i915_hdcp_component_master_unbind(struct device *dev)
+{
+ struct drm_i915_private *dev_priv = kdev_to_i915(dev);
+
+ intel_connectors_hdcp_disable(dev_priv);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Why this code? Once we've unregistered the driver, we should have shut
down the hardware completely. Which should have shut down all the hdcp
users too.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
This unbind might be triggered either due to master_del or component_del.
if its triggered from mei through component_del, then before teardown of
the i/f in component_unbind(), disable the ongoing HDCP session through
hdcp_disable() for each connectors.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I looked at your v7 component code again. I think if we put the
drm_atomic_helper_shutdown() call into master_unbind, then we'll have taken care
of that. Essentially what you're doing here is open-coding part of that
function. Better to just move the existing call to the right place.
And shutting down the entire hw is kinda what master_unbind should be
doing I think. We might also need to move the general hw quiescent into
master_unbind, that should work too.</pre>
</blockquote>
<pre>Need some more information on this. As per v7 on master_unbind will invoke
i915_unload_head that is i915_driver_unregister(dev_priv);
Similarly master_bind will call the load_tail that is i915_driver_register(dev_priv);
We are not initializing/shutting the HW in master_bind/unbind .
But this comment is contradicting with current approach. Could you please elaborate.?
-Ram
</pre>
<blockquote type="cite"
cite="mid:20181207142950.GE21184@phenom.ffwll.local">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ component_unbind_all(dev, dev_priv->hdcp_comp);
+}
+
+static const struct component_master_ops i915_hdcp_component_master_ops = {
+ .bind = i915_hdcp_component_master_bind,
+ .unbind = i915_hdcp_component_master_unbind,
+};
+
+static int i915_hdcp_component_match(struct device *dev, void *data)
+{
+ return !strcmp(dev->driver->name, "mei_hdcp");
+}
+
+static int intel_hdcp_component_init(struct intel_connector *connector)
+{
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+ struct i915_hdcp_component_master *comp;
+ struct component_match *match = NULL;
+ int ret;
+
+ comp = kzalloc(sizeof(*comp), GFP_KERNEL);
+ if (!comp)
+ return -ENOMEM;
+
+ mutex_init(&comp->mutex);
+ dev_priv->hdcp_comp = comp;
+
+ component_match_add(dev_priv->drm.dev, &match,
+ i915_hdcp_component_match, dev_priv);
+ ret = component_master_add_with_match(dev_priv->drm.dev,
+ &i915_hdcp_component_master_ops,
+ match);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">So I'm not sure this will work out well, hiding the master_ops here in
hdcp. Definitely needs to be rewritten as soon as i915 needs another
component. And might make the load/unload split complicated.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">we have already discussed this.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ if (ret < 0)
+ goto out_err;
+
+ DRM_INFO("I915 hdcp component master added.\n");
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">You add both the master and the hdcp component here. Output is a bit
confusing.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
we have component master and a component from mei which will match to the
master.
Here we are adding the component master.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ return ret;
+
+out_err:
+ component_master_del(dev_priv->drm.dev,
+ &i915_hdcp_component_master_ops);
+ kfree(comp);
+ dev_priv->hdcp_comp = NULL;
+ DRM_ERROR("Failed to add i915 hdcp component master (%d)\n", ret);
+
+ return ret;
+}
+
+static int initialize_mei_hdcp_data(struct intel_connector *connector)
+{
+ struct intel_hdcp *hdcp = &connector->hdcp;
+ struct mei_hdcp_data *data = &hdcp->mei_data;
+ enum port port;
+
+ if (connector->encoder) {
+ port = connector->encoder->port;
+ data->port = GET_MEI_DDI_INDEX(port);
+ }
+
+ data->port_type = MEI_HDCP_PORT_TYPE_INTEGRATED;
+ data->protocol = hdcp->shim->hdcp_protocol();
+
+ data->k = 1;
+ if (!data->streams)
+ data->streams = kcalloc(data->k, sizeof(data->streams[0]),
+ GFP_KERNEL);
+ if (!data->streams) {
+ DRM_ERROR("Out of Memory\n");
+ return -ENOMEM;
+ }
+
+ data->streams[0].stream_id = 0;
+ data->streams[0].stream_type = hdcp->content_type;
+
+ return 0;
+}
+
bool is_hdcp2_supported(struct drm_i915_private *dev_priv)
{
return ((INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv) ||
@@ -843,10 +1260,25 @@ static void intel_hdcp2_init(struct intel_connector *connector)
{
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
struct intel_hdcp *hdcp = &connector->hdcp;
+ int ret;
WARN_ON(!is_hdcp2_supported(dev_priv));
- /* TODO: MEI interface needs to be initialized here */
+ ret = initialize_mei_hdcp_data(connector);
+ if (ret) {
+ DRM_DEBUG_KMS("Mei hdcp data init failed\n");
+ return;
+ }
+
+ if (!dev_priv->hdcp_comp)
+ ret = intel_hdcp_component_init(connector);
+
+ if (ret) {
+ DRM_DEBUG_KMS("HDCP comp init failed\n");
+ kfree(hdcp->mei_data.streams);
+ return;
+ }
+
hdcp->hdcp2_supported = 1;
}
@@ -894,9 +1326,17 @@ void intel_hdcp_exit(struct drm_i915_private *dev_priv)
mutex_lock(&intel_connector->hdcp.mutex);
intel_connector->hdcp.hdcp2_supported = 0;
intel_connector->hdcp.shim = NULL;
+ kfree(intel_connector->hdcp.mei_data.streams);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">We need to push this into a per-connector hdcp cleanup function. Or just
into the generic connector cleanup.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">We could move it to per connector hdcp cleanup function.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Yeah I think that would be cleaner.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> mutex_unlock(&intel_connector->hdcp.mutex);
}
drm_connector_list_iter_end(&conn_iter);
+
+ if (dev_priv->hdcp_comp) {
+ component_master_del(dev_priv->drm.dev,
+ &i915_hdcp_component_master_ops);
+ kfree(dev_priv->hdcp_comp);
+ dev_priv->hdcp_comp = NULL;
+ }
}
int intel_hdcp_enable(struct intel_connector *connector)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index fca22d463e1b..12268228f4dc 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -24,8 +24,12 @@
#ifndef _I915_COMPONENT_H_
#define _I915_COMPONENT_H_
+#include <linux/mutex.h>
+
#include "drm_audio_component.h"
+#include <drm/drm_hdcp.h>
+
/* MAX_PORT is the number of port
* It must be sync with I915_MAX_PORTS defined i915_drv.h
*/
@@ -46,4 +50,71 @@ struct i915_audio_component {
int aud_sample_rate[MAX_PORTS];
};
+struct i915_hdcp_component_ops {
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Imo that should be called mei_hdcp_component_ops and put into the
linux/mei_hdcp.h header. Or was that Thomas' review comment?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nope this is there for many versions. i915_hdcp_component_ops are
implemented by mei_hdcp.c and initializes the component with the &ops.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I summarized all the discussion around the i915/mei_hdcp interface in the
thread in an earlier patch in this series.
Cheers, Daniel
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
Aside: Review here in public channels instead of in private would be much
better for coordination.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Tomas,
Could you please help on this ask.?
--Ram
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ /**
+ * @owner: mei_hdcp module
+ */
+ struct module *owner;
+
+ int (*initiate_hdcp2_session)(struct device *dev,
+ void *hdcp_data,
+ struct hdcp2_ake_init *ake_data);
+ int (*verify_receiver_cert_prepare_km)(struct device *dev,
+ void *hdcp_data,
+ struct hdcp2_ake_send_cert
+ *rx_cert,
+ bool *km_stored,
+ struct hdcp2_ake_no_stored_km
+ *ek_pub_km,
+ size_t *msg_sz);
+ int (*verify_hprime)(struct device *dev,
+ void *hdcp_data,
+ struct hdcp2_ake_send_hprime *rx_hprime);
+ int (*store_pairing_info)(struct device *dev,
+ void *hdcp_data,
+ struct hdcp2_ake_send_pairing_info
+ *pairing_info);
+ int (*initiate_locality_check)(struct device *dev,
+ void *hdcp_data,
+ struct hdcp2_lc_init *lc_init_data);
+ int (*verify_lprime)(struct device *dev,
+ void *hdcp_data,
+ struct hdcp2_lc_send_lprime *rx_lprime);
+ int (*get_session_key)(struct device *dev,
+ void *hdcp_data,
+ struct hdcp2_ske_send_eks *ske_data);
+ int (*repeater_check_flow_prepare_ack)(struct device *dev,
+ void *hdcp_data,
+ struct hdcp2_rep_send_receiverid_list
+ *rep_topology,
+ struct hdcp2_rep_send_ack
+ *rep_send_ack);
+ int (*verify_mprime)(struct device *dev,
+ void *hdcp_data,
+ struct hdcp2_rep_stream_ready *stream_ready);
+ int (*enable_hdcp_authentication)(struct device *dev,
+ void *hdcp_data);
+ int (*close_hdcp_session)(struct device *dev,
+ void *hdcp_data);
+};
+
+/**
+ * struct i915_hdcp_component_master - Used for communication between i915
+ * and mei_hdcp for HDCP2.2 services.
+ */
+struct i915_hdcp_component_master {
+ /**
+ * @dev: a device providing hdcp
+ */
+ struct device *dev;
+ /**
+ * @mutex: Mutex to protect the state of dev
+ */
+ struct mutex mutex;
+ /**
+ * @ops: Ops implemented by mei_hdcp driver, used by i915 driver.
+ */
+ const struct i915_hdcp_component_ops *ops;
+};
+
#endif /* _I915_COMPONENT_H_ */
--
2.7.4
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
</html>