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? > }; > > 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. "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. > + 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. > + 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. > + 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. > + 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. > + 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. > + 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. > + 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. > 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? Aside: Review here in public channels instead of in private would be much better for coordination. > + /** > + * @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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx