[PATCH v8 05/35] drm/i915: MEI interface definition
C, Ramalingam
ramalingam.c at intel.com
Wed Dec 12 08:58:29 UTC 2018
On 12/7/2018 7:59 PM, Daniel Vetter wrote:
> 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).
Sure. Done.
>>>> };
>>>> 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.
Ok. Sure.
>
> 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 ...
Got addressed as soon as we move to version implemented in v7.
>>>> + 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.
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
>>>> + 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
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20181212/7f6f7461/attachment-0001.html>
More information about the dri-devel
mailing list