Re: [PATCH v9 07/39] drm/i915: MEI interface definition

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Dec 19, 2018 at 08:45:41PM +0530, C, Ramalingam wrote:
> 
> On 12/19/2018 7:30 PM, Daniel Vetter wrote:
> > On Thu, Dec 13, 2018 at 09:31:09AM +0530, Ramalingam C wrote:
> > > Defining the mei-i915 interface functions and initialization of
> > > the interface.
> > > 
> > > v2:
> > >    Adjust to the new interface changes. [Tomas]
> > >    Added further debug logs for the failures at MEI i/f.
> > >    port in hdcp_port data is equipped to handle -ve values.
> > > v3:
> > >    mei comp is matched for global i915 comp master. [Daniel]
> > >    In hdcp_shim hdcp_protocol() is replaced with const variable. [Daniel]
> > >    mei wrappers are adjusted as per the i/f change [Daniel]
> > Yeah looks all good. Spotted some small stuff below still.
> > 
> > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_drv.h  |   5 +
> > >   drivers/gpu/drm/i915/intel_hdcp.c | 248 +++++++++++++++++++++++++++++++++++++-
> > >   include/drm/i915_component.h      |   7 ++
> > >   3 files changed, 259 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index dd9371647a8c..191b6e0f086c 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -39,6 +39,7 @@
> > >   #include <drm/drm_dp_mst_helper.h>
> > >   #include <drm/drm_rect.h>
> > >   #include <drm/drm_atomic.h>
> > > +#include <drm/i915_mei_hdcp_interface.h>
> > >   #include <media/cec-notifier.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);
> > > +
> > > +	/* HDCP adaptation(DP/HDMI) required on the port */
> > > +	enum hdcp_wired_protocol protocol;
> > >   };
> > >   struct intel_hdcp {
> > > @@ -399,6 +403,7 @@ struct intel_hdcp {
> > >   	 * content can flow only through a link protected by HDCP2.2.
> > >   	 */
> > >   	u8 content_type;
> > > +	struct hdcp_port_data port_data;
> > >   };
> > >   struct intel_connector {
> > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > > index 584d27f3c699..9405fce07b93 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > @@ -8,8 +8,10 @@
> > >   #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"
> > > @@ -833,6 +835,232 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
> > >   	return INTEL_GEN(dev_priv) >= 9 && port < PORT_E;
> > >   }
> > > +static __attribute__((unused)) int
> > > +hdcp2_prepare_ake_init(struct intel_connector *connector,
> > > +		       struct hdcp2_ake_init *ake_data)
> > > +{
> > > +	struct hdcp_port_data *data = &connector->hdcp.port_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_component_master *comp = dev_priv->comp_master;
> > > +	int ret;
> > > +
> > > +	/* During the connector init encoder might not be initialized */
> > > +	if (data->port == PORT_NONE)
> > > +		data->port = connector->encoder->port;
> > > +
> > > +	ret = comp->hdcp_ops->initiate_hdcp2_session(comp->mei_dev,
> > > +						     data, ake_data);
> > > +	if (ret)
> > > +		DRM_DEBUG_KMS("Prepare_ake_init failed. %d\n", ret);
> > > +
> > > +	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 hdcp_port_data *data = &connector->hdcp.port_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_component_master *comp = dev_priv->comp_master;
> > > +	int ret;
> > > +
> > > +	ret = comp->hdcp_ops->verify_receiver_cert_prepare_km(comp->mei_dev,
> > > +							      data, rx_cert,
> > > +							      paired, ek_pub_km,
> > > +							      msg_sz);
> > > +	if (ret < 0)
> > > +		DRM_DEBUG_KMS("Verify rx_cert failed. %d\n", ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused)) int
> > > +hdcp2_verify_hprime(struct intel_connector *connector,
> > > +		    struct hdcp2_ake_send_hprime *rx_hprime)
> > > +{
> > > +	struct hdcp_port_data *data = &connector->hdcp.port_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_component_master *comp = dev_priv->comp_master;
> > > +	int ret;
> > > +
> > > +	ret = comp->hdcp_ops->verify_hprime(comp->mei_dev, data, rx_hprime);
> > > +	if (ret < 0)
> > > +		DRM_DEBUG_KMS("Verify hprime failed. %d\n", ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused)) int
> > > +hdcp2_store_pairing_info(struct intel_connector *connector,
> > > +			 struct hdcp2_ake_send_pairing_info *pairing_info)
> > > +{
> > > +	struct hdcp_port_data *data = &connector->hdcp.port_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_component_master *comp = dev_priv->comp_master;
> > > +	int ret;
> > > +
> > > +	ret = comp->hdcp_ops->store_pairing_info(comp->mei_dev, data,
> > > +						 pairing_info);
> > > +	if (ret < 0)
> > > +		DRM_DEBUG_KMS("Store pairing info failed. %d\n", ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused)) int
> > > +hdcp2_prepare_lc_init(struct intel_connector *connector,
> > > +		      struct hdcp2_lc_init *lc_init)
> > > +{
> > > +	struct hdcp_port_data *data = &connector->hdcp.port_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_component_master *comp = dev_priv->comp_master;
> > > +	int ret;
> > > +
> > > +	ret = comp->hdcp_ops->initiate_locality_check(comp->mei_dev, data,
> > > +						      lc_init);
> > > +	if (ret < 0)
> > > +		DRM_DEBUG_KMS("Prepare lc_init failed. %d\n", ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused)) int
> > > +hdcp2_verify_lprime(struct intel_connector *connector,
> > > +		    struct hdcp2_lc_send_lprime *rx_lprime)
> > > +{
> > > +	struct hdcp_port_data *data = &connector->hdcp.port_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_component_master *comp = dev_priv->comp_master;
> > > +	int ret;
> > > +
> > > +	ret = comp->hdcp_ops->verify_lprime(comp->mei_dev, data, rx_lprime);
> > > +	if (ret < 0)
> > > +		DRM_DEBUG_KMS("Verify L_Prime failed. %d\n", ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused))
> > > +int hdcp2_prepare_skey(struct intel_connector *connector,
> > > +		       struct hdcp2_ske_send_eks *ske_data)
> > > +{
> > > +	struct hdcp_port_data *data = &connector->hdcp.port_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_component_master *comp = dev_priv->comp_master;
> > > +	int ret;
> > > +
> > > +	ret = comp->hdcp_ops->get_session_key(comp->mei_dev, data, ske_data);
> > > +	if (ret < 0)
> > > +		DRM_DEBUG_KMS("Get session key failed. %d\n", ret);
> > > +
> > > +	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 hdcp_port_data *data = &connector->hdcp.port_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_component_master *comp = dev_priv->comp_master;
> > > +	int ret;
> > > +
> > > +	ret = comp->hdcp_ops->repeater_check_flow_prepare_ack(comp->mei_dev,
> > > +							      data,
> > > +							      rep_topology,
> > > +							      rep_send_ack);
> > > +	if (ret < 0)
> > > +		DRM_DEBUG_KMS("Verify rep topology failed. %d\n", ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused)) int
> > > +hdcp2_verify_mprime(struct intel_connector *connector,
> > > +		    struct hdcp2_rep_stream_ready *stream_ready)
> > > +{
> > > +	struct hdcp_port_data *data = &connector->hdcp.port_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_component_master *comp = dev_priv->comp_master;
> > > +	int ret;
> > > +
> > > +	ret = comp->hdcp_ops->verify_mprime(comp->mei_dev, data, stream_ready);
> > > +	if (ret < 0)
> > > +		DRM_DEBUG_KMS("Verify mprime failed. %d\n", ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused))
> > > +int hdcp2_authenticate_port(struct intel_connector *connector)
> > > +{
> > > +	struct hdcp_port_data *data = &connector->hdcp.port_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_component_master *comp = dev_priv->comp_master;
> > > +	int ret;
> > > +
> > > +	ret = comp->hdcp_ops->enable_hdcp_authentication(comp->mei_dev, data);
> > > +	if (ret < 0)
> > > +		DRM_DEBUG_KMS("Enable hdcp auth failed. %d\n", ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused))
> > > +int hdcp2_close_mei_session(struct intel_connector *connector)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_component_master *comp = dev_priv->comp_master;
> > > +
> > > +	return comp->hdcp_ops->close_hdcp_session(comp->mei_dev,
> > > +						  &connector->hdcp.port_data);
> > > +}
> > > +
> > > +static __attribute__((unused))
> > > +int hdcp2_deauthenticate_port(struct intel_connector *connector)
> > > +{
> > > +	return hdcp2_close_mei_session(connector);
> > > +}
> > > +
> > > +static int i915_hdcp_component_match(struct device *dev, void *data)
> > > +{
> > > +	return !strcmp(dev->driver->name, "mei_hdcp");
> > > +}
> > > +
> > > +static int initialize_hdcp_port_data(struct intel_connector *connector)
> > > +{
> > > +	struct intel_hdcp *hdcp = &connector->hdcp;
> > > +	struct hdcp_port_data *data = &hdcp->port_data;
> > > +
> > > +	data->port = PORT_NONE;
> > > +	if (connector->encoder)
> > > +		data->port = connector->encoder->port;
> > This and the code above in ake_init still look strange. I'm not really
> > following how we can end up with an intialization sequence where we do not
> > yet know the encoder and hence can't assign the port?
> > 
> > That still smells like a bug. I think we should be able to set this
> > uncondtionally (if not, I need to understand why).
> 
> Oops. this is caused by me by calling the hdcp_init even before intel_connector_attach_encoder.
> 
> Will fix it.
> 
> > 
> > > +
> > > +	data->port_type = (u8)HDCP_PORT_TYPE_INTEGRATED;
> > > +	data->protocol = (u8)hdcp->shim->protocol;
> > > +
> > > +	data->k = 1;
> > > +	if (!data->streams)
> > > +		data->streams = kcalloc(data->k,
> > > +					sizeof(struct hdcp2_streamid_type),
> > > +					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 +1071,28 @@ 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;
> > > +	static bool comp_match_added;
> > > +	int ret;
> > >   	WARN_ON(!is_hdcp2_supported(dev_priv));
> > > -	/* TODO: MEI interface needs to be initialized here */
> > > +	ret = initialize_hdcp_port_data(connector);
> > > +	if (ret) {
> > > +		DRM_DEBUG_KMS("Mei hdcp data init failed\n");
> > > +		return;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Component for mei is common across the connector.
> > > +	 * Adding the match once is sufficient.
> > > +	 * TODO: Instead of static bool, do we need flag in dev_priv?.
> > > +	 */
> > > +	if (!comp_match_added) {
> > > +		component_match_add(dev_priv->drm.dev, &dev_priv->master_match,
> > > +				    i915_hdcp_component_match, dev_priv);
> > Patch series needs to be reordered: We can only do the component_match_add
> > once the mei component is merged. Maybe just split out this hunk into a
> > final patch?
> This function wont be called till Kconfig is defined in the mei_hdcp
> changes. But yes this chunk should be last patch of these series.
> > 
> > > +		comp_match_added = true;
> > This static here kinda works because there's only 1 gpu, but it's a bad
> > hack. Please move to drm_i915_private (next to the other component stuff).
> Ok
> > 
> > Also, we need to only do this when CONFIG_MEI_HDCP is enabled, otherwise
> > hdcp2 init must fail. Maybe best to put that into the is_hdcp2_supported
> > helper for now. Once we get non-MEI hdcp2 (definitely needed for discrete
> > gpu) we can split that up into is_mei_hdcp2_support and
> > is_other_hdcp2_supported.
> 
> the check for CONFIG_INTEL_MEI_HDCP is already added into
> is_hdcp2_supported()

Indeed, I overlooked that. Maybe highlight it a bit more with a separate

	if (!CONFIG_ENABLED(MEI_HDCP))
		return false;

so it stick out more in the previous patch. Currently it's a bit burried.

With that + the init ordering fixed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

There's no need to split the patch out anymore, since without the mei
patches you can't enable that Kconfig option and hence no bisect issues.

Cheers, Daniel


> 
> -Ram
> 
> > 
> > Otherwise lgtm I think.
> > -Daniel
> > 
> > > +	}
> > > +
> > >   	hdcp->hdcp2_supported = 1;
> > >   }
> > > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> > > index 6f94ddd3f2c2..7a7201374cfe 100644
> > > --- a/include/drm/i915_component.h
> > > +++ b/include/drm/i915_component.h
> > > @@ -24,6 +24,8 @@
> > >   #ifndef _I915_COMPONENT_H_
> > >   #define _I915_COMPONENT_H_
> > > +#include <drm/i915_mei_hdcp_interface.h>
> > > +
> > >   #include "drm_audio_component.h"
> > >   /* MAX_PORT is the number of port
> > > @@ -50,8 +52,13 @@ struct i915_audio_component {
> > >    * struct i915_component_master - Used for communication between i915
> > >    *				  and any other drivers for the services
> > >    *				  of different feature.
> > > + * @mei_dev: device that provide the HDCP2.2 service from MEI Bus.
> > > + * @hdcp_ops: Ops implemented by mei_hdcp driver, used by i915 driver.
> > >    */
> > >   struct i915_component_master {
> > > +	struct device *mei_dev;
> > > +	const struct i915_hdcp_component_ops *hdcp_ops;
> > > +
> > >   	/*
> > >   	 * Add here the interface details between I915 and interested modules.
> > >   	 */
> > > -- 
> > > 2.7.4
> > > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx