[PATCH v8 05/35] drm/i915: MEI interface definition
Daniel Vetter
daniel at ffwll.ch
Fri Dec 7 14:29:50 UTC 2018
On Fri, Dec 07, 2018 at 11:22:44AM +0530, C, Ramalingam wrote:
>
> On 12/6/2018 3:53 PM, Daniel Vetter wrote:
> > On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
> > > Defining the mei-i915 interface functions and initialization of
> > > the interface.
> > >
> > > Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
> > > Signed-off-by: Tomas Winkler <tomas.winkler at intel.com>
> > > ---
> > > 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);
> > Looking ahead, this seems hardwired to constant return value? Or why do we
> > need a function here?
>
> 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.
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).
>
> >
> > > };
> > > 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);
> > 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.
>
> 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.
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.
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.
>
> >
> > "Just in case" papering over programming bugs of our own just makes
> > debugging harder.
> >
> > > +
> > > + ret = comp->ops->initiate_hdcp2_session(comp->dev,
> > > + data, ake_data);
> > We should set the port only after successfully initializing this.
>
> hdcp2_session is created for each port at ME FW. Hence we need the port
> initialized even before calling the initiate_hdcp2_session.
> > > + if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
> > > + data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
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.
>
> >
> > > + 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) {
> > These all look like programming mistakes that should result in a WARN_ON.
>
> 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.
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 ...
>
> >
> > > + 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);
> > 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.
>
> 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.
Yeah, I think that'll lead to cleaner code.
>
> >
> > > + mutex_unlock(&comp->mutex);
> > With component do we still need this mutex stuff here?
> >
> > Exact same comments everywhere below.
> >
> > > +
> > > + 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);
> > Need to reset the port here I think.
>
> 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.
>
> >
> > > + 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);
> > 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.
>
> 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.
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.
>
> >
> > > + 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);
> > 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.
> we have already discussed this.
> >
> > > + if (ret < 0)
> > > + goto out_err;
> > > +
> > > + DRM_INFO("I915 hdcp component master added.\n");
> > You add both the master and the hdcp component here. Output is a bit
> > confusing.
>
> we have component master and a component from mei which will match to the
> master.
>
> Here we are adding the component master.
>
> >
> > > + 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);
> > We need to push this into a per-connector hdcp cleanup function. Or just
> > into the generic connector cleanup.
> We could move it to per connector hdcp cleanup function.
Yeah I think that would be cleaner.
> >
> > > 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 {
> > Imo that should be called mei_hdcp_component_ops and put into the
> > linux/mei_hdcp.h header. Or was that Thomas' review comment?
>
> Nope this is there for many versions. i915_hdcp_component_ops are
> implemented by mei_hdcp.c and initializes the component with the &ops.
I summarized all the discussion around the i915/mei_hdcp interface in the
thread in an earlier patch in this series.
Cheers, Daniel
>
> >
> > Aside: Review here in public channels instead of in private would be much
> > better for coordination.
>
> Tomas,
>
> Could you please help on this ask.?
>
> --Ram
>
> >
> > > + /**
> > > + * @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
> > >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list