Re: [PATCH v9 35/39] misc/mei/hdcp: Component framework for I915 Interface

From: "C, Ramalingam" <ramalingam.c@intel.com>
To: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	Sean Paul <seanpaul@chromium.org>,
	daniel.vetter@ffwll.ch, tomas.winkler@intel.com,
	uma.shankar@intel.com
Subject: Re: [PATCH v9 35/39] misc/mei/hdcp: Component framework for I915 Interface
Date: Thu, 13 Dec 2018 18:06:03 +0530	[thread overview]
Message-ID: <efa01632-320b-019b-76d6-7e3ece50d701@intel.com> (raw)
In-Reply-To: <1544673701-6353-36-git-send-email-ramalingam.c@intel.com>

[-- Attachment #1.1: Type: text/plain, Size: 5330 bytes --]

Tomas and Daniel,

We got an issue here.

The relationship that we try to build between I915 and mei_hdcp is as follows:

  * We are using the components to establish the relationship.
  * I915 is component master where as mei_hdcp is component.
  * I915 adds the component master during the module load. mei_hdcp adds
    the component when the driver->probe is called (on device driver
    binding).
  * I915 forces itself such that until mei_hdcp component is added
    I915_load wont be complete.
  * Similarly on complete system, if mei_hdcp component is removed,
    immediately I915 unregister itself and HW will be shutdown.

This is completely fine when the modules are loaded and unloaded.

But during suspend, mei device disappears and mei bus handles it by unbinding device and driver by calling driver->remove.
This in-turn removes the component and triggers the master unbind of I915 where, I915 unregister itself.
This cause the HW state mismatch during the suspend and resume.

Please check the powerwell mismatch errors at CI report for v9
https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_3412/fi-glk-j4005/igt@gem_exec_suspend@basic-s3.html

More over unregistering I915 during the suspend is not expected. So how do we handle this?

-Ram

On 12/13/2018 9:31 AM, Ramalingam C wrote:
> Mei hdcp driver is designed as component slave for the I915 component
> master.
>
> v2: Rebased.
> v3:
>    Notifier chain is adopted for cldev state update [Tomas]
> v4:
>    Made static dummy functions as inline in mei_hdcp.h
>    API for polling client device status
>    IS_ENABLED used in header, for config status for mei_hdcp.
> v5:
>    Replacing the notifier with component framework. [Daniel]
> v6:
>    Rebased on the I915 comp master redesign.
> v7:
>    mei_hdcp_component_registered is made static [Uma]
>    Need for global static variable mei_cldev is removed.
>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> Reviewed-by: Uma Shankar <uma.shankar@intel.com>
> ---
>   drivers/misc/mei/hdcp/mei_hdcp.c | 67 +++++++++++++++++++++++++++++++++++++---
>   1 file changed, 63 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
> index b22a71e8c5d7..3de1700dcc9f 100644
> --- a/drivers/misc/mei/hdcp/mei_hdcp.c
> +++ b/drivers/misc/mei/hdcp/mei_hdcp.c
> @@ -23,11 +23,14 @@
>   #include <linux/slab.h>
>   #include <linux/uuid.h>
>   #include <linux/mei_cl_bus.h>
> +#include <linux/component.h>
>   #include <drm/drm_connector.h>
>   #include <drm/i915_component.h>
>   
>   #include "mei_hdcp.h"
>   
> +static bool mei_hdcp_component_registered;
> +
>   /**
>    * mei_initiate_hdcp2_session() - Initiate a Wired HDCP2.2 Tx Session in ME FW
>    * @dev: device corresponding to the mei_cl_device
> @@ -691,8 +694,7 @@ mei_close_hdcp_session(struct device *dev, struct hdcp_port_data *data)
>   	return 0;
>   }
>   
> -static __attribute__((unused))
> -struct i915_hdcp_component_ops mei_hdcp_ops = {
> +static struct i915_hdcp_component_ops mei_hdcp_ops = {
>   	.owner = THIS_MODULE,
>   	.initiate_hdcp2_session = mei_initiate_hdcp2_session,
>   	.verify_receiver_cert_prepare_km = mei_verify_receiver_cert_prepare_km,
> @@ -707,20 +709,77 @@ struct i915_hdcp_component_ops mei_hdcp_ops = {
>   	.close_hdcp_session = mei_close_hdcp_session,
>   };
>   
> +static int mei_hdcp_component_bind(struct device *mei_kdev,
> +				   struct device *i915_kdev, void *data)
> +{
> +	struct i915_component_master *master_comp = data;
> +
> +	dev_info(mei_kdev, "MEI HDCP comp bind\n");
> +	WARN_ON(master_comp->hdcp_ops);
> +	master_comp->hdcp_ops = &mei_hdcp_ops;
> +	master_comp->mei_dev = mei_kdev;
> +
> +	return 0;
> +}
> +
> +static void mei_hdcp_component_unbind(struct device *mei_kdev,
> +				      struct device *i915_kdev, void *data)
> +{
> +	struct i915_component_master *master_comp = data;
> +
> +	dev_info(mei_kdev, "MEI HDCP comp unbind\n");
> +	master_comp->hdcp_ops = NULL;
> +	master_comp->mei_dev = NULL;
> +}
> +
> +static const struct component_ops mei_hdcp_component_bind_ops = {
> +	.bind	= mei_hdcp_component_bind,
> +	.unbind	= mei_hdcp_component_unbind,
> +};
> +
> +static void mei_hdcp_component_init(struct device *dev)
> +{
> +	int ret;
> +
> +	dev_info(dev, "MEI HDCP comp init\n");
> +	ret = component_add(dev, &mei_hdcp_component_bind_ops);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to add MEI HDCP comp (%d)\n", ret);
> +		return;
> +	}
> +
> +	mei_hdcp_component_registered = true;
> +}
> +
> +static void mei_hdcp_component_cleanup(struct device *dev)
> +{
> +	if (!mei_hdcp_component_registered)
> +		return;
> +
> +	dev_info(dev, "MEI HDCP comp cleanup\n");
> +	component_del(dev, &mei_hdcp_component_bind_ops);
> +	mei_hdcp_component_registered = false;
> +}
> +
>   static int mei_hdcp_probe(struct mei_cl_device *cldev,
>   			  const struct mei_cl_device_id *id)
>   {
>   	int ret;
>   
>   	ret = mei_cldev_enable(cldev);
> -	if (ret < 0)
> +	if (ret < 0) {
>   		dev_err(&cldev->dev, "mei_cldev_enable Failed. %d\n", ret);
> +		return ret;
> +	}
> +	mei_hdcp_component_init(&cldev->dev);
>   
> -	return ret;
> +	return 0;
>   }
>   
>   static int mei_hdcp_remove(struct mei_cl_device *cldev)
>   {
> +	mei_hdcp_component_cleanup(&cldev->dev);
> +
>   	return mei_cldev_disable(cldev);
>   }
>   

[-- Attachment #1.2: Type: text/html, Size: 5971 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel