On Fri, Dec 07, 2018 at 04:18:25PM +0530, C, Ramalingam wrote:
>
> On 12/7/2018 11:22 AM, 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@xxxxxxxxx>
> > > > Signed-off-by: Tomas Winkler<tomas.winkler@xxxxxxxxx>
> > > > ---
> > > > 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.
> > > > };
> > > > 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.
> > > "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.
> > > > + 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.
> > > > + 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.
> > > > + 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.
> > > > + 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.
> > > > 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.
>
> Tomas and Daniel,
>
> Is this fine? Defining the ops at i915_component.h and I915 and mei_hdcp
> using it for their purpose?
>
> If it has to be compulsorily defined by the service provider then we need
> to move this into the include/linux/mei_hdcp.h. In that we can avoid the
> global header from the mei_hdcp Tomas. Please help here to clarify the direction.
Let's move this discussion to the other reply chain, to avoid splitting
discussion up too much.
-Daniel
>
> -Ram
>
> > > 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
> > > >
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
