| Message ID | 1544673701-6353-36-git-send-email-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Implement HDCP2.2 | expand |
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);
> }
>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<pre>Tomas and Daniel,</pre>
<pre><pre>We got an issue here.</pre>The relationship that we try to build between I915 and mei_hdcp is as follows:
</pre>
<ul>
<li>We are using the components to establish the relationship.</li>
<li>I915 is component master where as mei_hdcp is component.</li>
<li>I915 adds the component master during the module load.
mei_hdcp adds the component when the driver->probe is called
(on device driver binding).</li>
<li>I915 forces itself such that until mei_hdcp component is added
I915_load wont be complete.</li>
<li>Similarly on complete system, if mei_hdcp component is
removed, immediately I915 unregister itself and HW will be
shutdown.</li>
</ul>
<pre>This is completely fine when the modules are loaded and unloaded.</pre>
<pre>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
<a class="moz-txt-link-freetext" href="https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_3412/fi-glk-j4005/igt@gem_exec_suspend@basic-s3.html">https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_3412/fi-glk-j4005/igt@gem_exec_suspend@basic-s3.html</a>
More over unregistering I915 during the suspend is not expected. So how do we handle this?
-Ram
</pre>
<div class="moz-cite-prefix">On 12/13/2018 9:31 AM, Ramalingam C
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1544673701-6353-36-git-send-email-ramalingam.c@intel.com">
<pre class="moz-quote-pre" wrap="">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 <a class="moz-txt-link-rfc2396E" href="mailto:ramalingam.c@intel.com"><ramalingam.c@intel.com></a>
Reviewed-by: Uma Shankar <a class="moz-txt-link-rfc2396E" href="mailto:uma.shankar@intel.com"><uma.shankar@intel.com></a>
---
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);
}
</pre>
</blockquote>
</body>
</html>
On Thu, Dec 13, 2018 at 1:36 PM C, Ramalingam <ramalingam.c@intel.com> wrote: > > 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? Bit more context from our irc discussion with Ram: I found this very surprising, since I don't know of any other subsystems where the devices get outright removed when going through a suspend/resume cycle. The device model was built to handle this stuff correctly: First clients/devices/interfaces get suspend, then the parent/bridge/bus. Same dance in reverse when resuming. This even holds for lots of hotpluggable buses, where child devices could indeed disappear on resume, but as long as they don't, everything stays the same. It's really surprising for something that's soldered onto the board like ME. Aside: We'll probably need a device_link to make sure mei_hdcp is fully resumed before i915 gets resumed, but that's kinda a detail for later on. Tomas, can you pls explain why mei is designed like this? Or is there something else we're missing (I didn't dig through the mei bus in detail at all, so not clear on what exactly is going on there). Also pulling in device model and suspend/resume experts. Thanks, Daniel > > -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); > } >
> On Thu, Dec 13, 2018 at 1:36 PM C, Ramalingam <ramalingam.c@intel.com> > wrote: > > > > 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? > > Bit more context from our irc discussion with Ram: > > I found this very surprising, since I don't know of any other subsystems where > the devices get outright removed when going through a suspend/resume cycle. > The device model was built to handle this stuff > correctly: First clients/devices/interfaces get suspend, then the > parent/bridge/bus. Same dance in reverse when resuming. This even holds for > lots of hotpluggable buses, where child devices could indeed disappear on > resume, but as long as they don't, everything stays the same. It's really > surprising for something that's soldered onto the board like ME. HDCP is an application in the ME it's not ME itself.. On the linux side HDCP2 is a virtual device on mei client virtual bus, the bus is teared down on ME reset, which mostly happen on power transitions. Theoretically, we could keep it up during power transitions, but so fare it was not necessary and second it's not guarantie that the all ME applications will reappear after reset. > > Aside: We'll probably need a device_link to make sure mei_hdcp is fully > resumed before i915 gets resumed, but that's kinda a detail for later on. Frankly I don’t believe there is currently exact abstraction that supports this model, neither components nor device_link . So fare we used class interface for other purposes, it worked well. > > Tomas, can you pls explain why mei is designed like this? Or is there something > else we're missing (I didn't dig through the mei bus in detail at all, so not clear > on what exactly is going on there). Above. > > Also pulling in device model and suspend/resume experts. > > Thanks, Daniel > > > > > -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); > > } > > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, Dec 13, 2018 at 5:27 PM Winkler, Tomas <tomas.winkler@intel.com> wrote: > > > On Thu, Dec 13, 2018 at 1:36 PM C, Ramalingam <ramalingam.c@intel.com> > > wrote: > > > > > > 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? > > > > Bit more context from our irc discussion with Ram: > > > > I found this very surprising, since I don't know of any other subsystems where > > the devices get outright removed when going through a suspend/resume cycle. > > The device model was built to handle this stuff > > correctly: First clients/devices/interfaces get suspend, then the > > parent/bridge/bus. Same dance in reverse when resuming. This even holds for > > lots of hotpluggable buses, where child devices could indeed disappear on > > resume, but as long as they don't, everything stays the same. It's really > > surprising for something that's soldered onto the board like ME. > > HDCP is an application in the ME it's not ME itself.. On the linux side HDCP2 is a virtual device on mei client virtual bus, > the bus is teared down on ME reset, which mostly happen on power transitions. > Theoretically, we could keep it up during power transitions, but so fare it was not necessary > and second it's not guarantie that the all ME applications will reappear after reset. When does this happen that an ME application doesn't come back after e.g. suspend/resume? Also, what's all the place where this reset can happen? Just suspend/resume/hibernate and all these, or also at other times? How does userspace deal with the reset over s/r? I'm assuming that at least the device node file will become invalid (or whatever you're using as userspace api), so if userspace is accessing stuff on the me at the same time as we do a suspend/resume, what happens? > > Aside: We'll probably need a device_link to make sure mei_hdcp is fully > > resumed before i915 gets resumed, but that's kinda a detail for later on. > > Frankly I don’t believe there is currently exact abstraction that supports this model, > neither components nor device_link . > So fare we used class interface for other purposes, it worked well. I'm not clear on what class interface has to do with component or device link. They all solve different problems, at least as far as I understand all this stuff ... -Daniel > > Tomas, can you pls explain why mei is designed like this? Or is there something > > else we're missing (I didn't dig through the mei bus in detail at all, so not clear > > on what exactly is going on there). > Above. > > > > Also pulling in device model and suspend/resume experts. > > > > Thanks, Daniel > > > > > > > > -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); > > > } > > > > > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > On Thu, Dec 13, 2018 at 5:27 PM Winkler, Tomas <tomas.winkler@intel.com> > wrote: > > > > > On Thu, Dec 13, 2018 at 1:36 PM C, Ramalingam > > > <ramalingam.c@intel.com> > > > wrote: > > > > > > > > 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? > > > > > > Bit more context from our irc discussion with Ram: > > > > > > I found this very surprising, since I don't know of any other > > > subsystems where the devices get outright removed when going through a > suspend/resume cycle. > > > The device model was built to handle this stuff > > > correctly: First clients/devices/interfaces get suspend, then the > > > parent/bridge/bus. Same dance in reverse when resuming. This even > > > holds for lots of hotpluggable buses, where child devices could > > > indeed disappear on resume, but as long as they don't, everything > > > stays the same. It's really surprising for something that's soldered onto the > board like ME. > > > > HDCP is an application in the ME it's not ME itself.. On the linux > > side HDCP2 is a virtual device on mei client virtual bus, the bus is teared > down on ME reset, which mostly happen on power transitions. > > Theoretically, we could keep it up during power transitions, but so > > fare it was not necessary and second it's not guarantie that the all ME > applications will reappear after reset. > > When does this happen that an ME application doesn't come back after e.g. > suspend/resume? No, this can happen in special flows such as fw updates and error conditions, but is has to be supported as well. > > Also, what's all the place where this reset can happen? Just > suspend/resume/hibernate and all these, or also at other times? Also on errors and fw update, the basic assumption is here that it can happen any time. > How does userspace deal with the reset over s/r? I'm assuming that at least the > device node file will become invalid (or whatever you're using as userspace > api), so if userspace is accessing stuff on the me at the same time as we do a > suspend/resume, what happens? > > > > Aside: We'll probably need a device_link to make sure mei_hdcp is > > > fully resumed before i915 gets resumed, but that's kinda a detail for later > on. > > > > Frankly I don’t believe there is currently exact abstraction that > > supports this model, neither components nor device_link . > > So fare we used class interface for other purposes, it worked well. > > I'm not clear on what class interface has to do with component or device link. > They all solve different problems, at least as far as I understand all this stuff ... > -Daniel It comes instead of it, device_link is mostly used for power management and component as we see know is not what we need as HDCP Is a b it volitle. class_interface gives you two handlers: add and remove device, that's all what is needed for the current implementation. > > > > Tomas, can you pls explain why mei is designed like this? Or is > > > there something else we're missing (I didn't dig through the mei bus > > > in detail at all, so not clear on what exactly is going on there). > > Above. > > > > > > Also pulling in device model and suspend/resume experts. > > > > > > Thanks, Daniel > > > > > > > > > > > -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); } > > > > > > > > > > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Sat, Dec 15, 2018 at 09:20:38PM +0000, Winkler, Tomas wrote: > > > > On Thu, Dec 13, 2018 at 5:27 PM Winkler, Tomas <tomas.winkler@intel.com> > > wrote: > > > > > > > On Thu, Dec 13, 2018 at 1:36 PM C, Ramalingam > > > > <ramalingam.c@intel.com> > > > > wrote: > > > > > > > > > > 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? > > > > > > > > Bit more context from our irc discussion with Ram: > > > > > > > > I found this very surprising, since I don't know of any other > > > > subsystems where the devices get outright removed when going through a > > suspend/resume cycle. > > > > The device model was built to handle this stuff > > > > correctly: First clients/devices/interfaces get suspend, then the > > > > parent/bridge/bus. Same dance in reverse when resuming. This even > > > > holds for lots of hotpluggable buses, where child devices could > > > > indeed disappear on resume, but as long as they don't, everything > > > > stays the same. It's really surprising for something that's soldered onto the > > board like ME. > > > > > > HDCP is an application in the ME it's not ME itself.. On the linux > > > side HDCP2 is a virtual device on mei client virtual bus, the bus is teared > > down on ME reset, which mostly happen on power transitions. > > > Theoretically, we could keep it up during power transitions, but so > > > fare it was not necessary and second it's not guarantie that the all ME > > applications will reappear after reset. > > > > When does this happen that an ME application doesn't come back after e.g. > > suspend/resume? > No, this can happen in special flows such as fw updates and error conditions, but is has to be supported as well. > > > > > Also, what's all the place where this reset can happen? Just > > suspend/resume/hibernate and all these, or also at other times? > > Also on errors and fw update, the basic assumption is here that it can happen any time. If this can happen any time, what are we supposed to do if this happens while we're doing something with the hdcp mei? If this is such a common occurence I guess we need to somehow wait until everyting is rebound and working again. I think ideally mei core would handle that for us, but I guess if this just randomly happens then we need to redo all the transactions. So does need some involvement of the higher levels. Also, how likely is it that the hdcp mei will outright disappear and not come back after a reset? > > How does userspace deal with the reset over s/r? I'm assuming that at least the > > device node file will become invalid (or whatever you're using as userspace > > api), so if userspace is accessing stuff on the me at the same time as we do a > > suspend/resume, what happens? Also, answer to how other users handle this would be enlighting. > > > > Aside: We'll probably need a device_link to make sure mei_hdcp is > > > > fully resumed before i915 gets resumed, but that's kinda a detail for later > > on. > > > > > > Frankly I don’t believe there is currently exact abstraction that > > > supports this model, neither components nor device_link . > > > So fare we used class interface for other purposes, it worked well. > > > > I'm not clear on what class interface has to do with component or device link. > > They all solve different problems, at least as far as I understand all this stuff ... > > -Daniel > > It comes instead of it, device_link is mostly used for power management and component as we see know is not what we need as HDCP > Is a b it volitle. > class_interface gives you two handlers: add and remove device, that's all what is needed for the current implementation. Well someone needs to handle the volatility of hdcp, and atm we seem to be playing a game of pass the bucket. I still think that mei_hdcp should supply a clean interface to i915, with all the reset madness handled internally. But depending upon how badly this all leaks we might need to have a retry logic in the i915 hdcp flow too. device linke we'll probably need anyway, since i915 resuming when hdcp is not yet up is not a good idea no matter what's goîng on. -Daniel > > > > > > Tomas, can you pls explain why mei is designed like this? Or is > > > > there something else we're missing (I didn't dig through the mei bus > > > > in detail at all, so not clear on what exactly is going on there). > > > Above. > > > > > > > > Also pulling in device model and suspend/resume experts. > > > > > > > > Thanks, Daniel > > > > > > > > > > > > > > -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); } > > > > > > > > > > > > > > > > > > > > > -- > > > > Daniel Vetter > > > > Software Engineer, Intel Corporation > > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Dec 17, 2018 at 10:39:07AM +0100, Daniel Vetter wrote: > On Sat, Dec 15, 2018 at 09:20:38PM +0000, Winkler, Tomas wrote: > > > > > > On Thu, Dec 13, 2018 at 5:27 PM Winkler, Tomas <tomas.winkler@intel.com> > > > wrote: > > > > > > > > > On Thu, Dec 13, 2018 at 1:36 PM C, Ramalingam > > > > > <ramalingam.c@intel.com> > > > > > wrote: > > > > > > > > > > > > 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? > > > > > > > > > > Bit more context from our irc discussion with Ram: > > > > > > > > > > I found this very surprising, since I don't know of any other > > > > > subsystems where the devices get outright removed when going through a > > > suspend/resume cycle. > > > > > The device model was built to handle this stuff > > > > > correctly: First clients/devices/interfaces get suspend, then the > > > > > parent/bridge/bus. Same dance in reverse when resuming. This even > > > > > holds for lots of hotpluggable buses, where child devices could > > > > > indeed disappear on resume, but as long as they don't, everything > > > > > stays the same. It's really surprising for something that's soldered onto the > > > board like ME. > > > > > > > > HDCP is an application in the ME it's not ME itself.. On the linux > > > > side HDCP2 is a virtual device on mei client virtual bus, the bus is teared > > > down on ME reset, which mostly happen on power transitions. > > > > Theoretically, we could keep it up during power transitions, but so > > > > fare it was not necessary and second it's not guarantie that the all ME > > > applications will reappear after reset. > > > > > > When does this happen that an ME application doesn't come back after e.g. > > > suspend/resume? > > No, this can happen in special flows such as fw updates and error conditions, but is has to be supported as well. > > > > > > > > Also, what's all the place where this reset can happen? Just > > > suspend/resume/hibernate and all these, or also at other times? > > > > Also on errors and fw update, the basic assumption is here that it can happen any time. > > If this can happen any time, what are we supposed to do if this happens > while we're doing something with the hdcp mei? If this is such a common > occurence I guess we need to somehow wait until everyting is rebound and > working again. I think ideally mei core would handle that for us, but I > guess if this just randomly happens then we need to redo all the > transactions. So does need some involvement of the higher levels. Few more questions on this, beyond the ones in the previous mail. So generally drivers don't support upgrading the fw at runtime, but only look once for upgraded fw on driver load (before anyone is using is). What's the use-case that requires this. I'm also rather worried about "this can happen any time". Is ME fw so unstable it just randomly crashes for no good reason at all, taking our hdcp session with it? Or is this more like "cosmic rays can happen" kind of problem (which we don't care about)? > > Also, how likely is it that the hdcp mei will outright disappear and not > come back after a reset? > > > > How does userspace deal with the reset over s/r? I'm assuming that at least the > > > device node file will become invalid (or whatever you're using as userspace > > > api), so if userspace is accessing stuff on the me at the same time as we do a > > > suspend/resume, what happens? > > Also, answer to how other users handle this would be enlighting. > > > > > > Aside: We'll probably need a device_link to make sure mei_hdcp is > > > > > fully resumed before i915 gets resumed, but that's kinda a detail for later > > > on. > > > > > > > > Frankly I don’t believe there is currently exact abstraction that > > > > supports this model, neither components nor device_link . > > > > So fare we used class interface for other purposes, it worked well. > > > > > > I'm not clear on what class interface has to do with component or device link. > > > They all solve different problems, at least as far as I understand all this stuff ... > > > -Daniel > > > > It comes instead of it, device_link is mostly used for power management and component as we see know is not what we need as HDCP > > Is a b it volitle. > > class_interface gives you two handlers: add and remove device, that's all what is needed for the current implementation. > > Well someone needs to handle the volatility of hdcp, and atm we seem to be > playing a game of pass the bucket. I still think that mei_hdcp should > supply a clean interface to i915, with all the reset madness handled > internally. But depending upon how badly this all leaks we might need to > have a retry logic in the i915 hdcp flow too. > > device linke we'll probably need anyway, since i915 resuming when hdcp is > not yet up is not a good idea no matter what's goîng on. Cheers, Daniel > -Daniel > > > > > > > > > Tomas, can you pls explain why mei is designed like this? Or is > > > > > there something else we're missing (I didn't dig through the mei bus > > > > > in detail at all, so not clear on what exactly is going on there). > > > > Above. > > > > > > > > > > Also pulling in device model and suspend/resume experts. > > > > > > > > > > Thanks, Daniel > > > > > > > > > > > > > > > > > -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); } > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Daniel Vetter > > > > > Software Engineer, Intel Corporation > > > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > > > > > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
> On Sat, Dec 15, 2018 at 09:20:38PM +0000, Winkler, Tomas wrote: > > > > > > On Thu, Dec 13, 2018 at 5:27 PM Winkler, Tomas > > > <tomas.winkler@intel.com> > > > wrote: > > > > > > > > > On Thu, Dec 13, 2018 at 1:36 PM C, Ramalingam > > > > > <ramalingam.c@intel.com> > > > > > wrote: > > > > > > > > > > > > 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-j4 > > > > > > 005/ > > > > > > igt@ > > > > > > gem_exec_suspend@basic-s3.html > > > > > > > > > > > > More over unregistering I915 during the suspend is not expected. > > > > > > So how do > > > > > we handle this? > > > > > > > > > > Bit more context from our irc discussion with Ram: > > > > > > > > > > I found this very surprising, since I don't know of any other > > > > > subsystems where the devices get outright removed when going > > > > > through a > > > suspend/resume cycle. > > > > > The device model was built to handle this stuff > > > > > correctly: First clients/devices/interfaces get suspend, then > > > > > the parent/bridge/bus. Same dance in reverse when resuming. This > > > > > even holds for lots of hotpluggable buses, where child devices > > > > > could indeed disappear on resume, but as long as they don't, > > > > > everything stays the same. It's really surprising for something > > > > > that's soldered onto the > > > board like ME. > > > > > > > > HDCP is an application in the ME it's not ME itself.. On the > > > > linux side HDCP2 is a virtual device on mei client virtual bus, > > > > the bus is teared > > > down on ME reset, which mostly happen on power transitions. > > > > Theoretically, we could keep it up during power transitions, but > > > > so fare it was not necessary and second it's not guarantie that > > > > the all ME > > > applications will reappear after reset. > > > > > > When does this happen that an ME application doesn't come back after e.g. > > > suspend/resume? > > No, this can happen in special flows such as fw updates and error conditions, > but is has to be supported as well. > > > > > > > > Also, what's all the place where this reset can happen? Just > > > suspend/resume/hibernate and all these, or also at other times? > > > > Also on errors and fw update, the basic assumption is here that it can happen > any time. > > If this can happen any time, what are we supposed to do if this happens while > we're doing something with the hdcp mei? If this is such a common occurence I > guess we need to somehow wait until everyting is rebound and working again. I > think ideally mei core would handle that for us, but I guess if this just randomly > happens then we need to redo all the transactions. So does need some > involvement of the higher levels. It's not common occurrence, but the assumption must be it can happen any time, In that case everything has to restarted as there is no state preserved in the ME FW. Right MEI core cannot do it for you, it is just a channel, the logic and state of the connection is in the mei_hdcp or gfx. Note that HDCP is not the only App over MEI. > > Also, how likely is it that the hdcp mei will outright disappear and not come > back after a reset? > > > > How does userspace deal with the reset over s/r? I'm assuming that > > > at least the device node file will become invalid (or whatever > > > you're using as userspace api), so if userspace is accessing stuff > > > on the me at the same time as we do a suspend/resume, what happens? > > Also, answer to how other users handle this would be enlighting. > > > > > > Aside: We'll probably need a device_link to make sure mei_hdcp > > > > > is fully resumed before i915 gets resumed, but that's kinda a > > > > > detail for later > > > on. > > > > > > > > Frankly I don’t believe there is currently exact abstraction that > > > > supports this model, neither components nor device_link . > > > > So fare we used class interface for other purposes, it worked well. > > > > > > I'm not clear on what class interface has to do with component or device > link. > > > They all solve different problems, at least as far as I understand all this stuff > ... > > > -Daniel > > > > It comes instead of it, device_link is mostly used for power > > management and component as we see know is not what we need as HDCP Is > a b it volitle. > > class_interface gives you two handlers: add and remove device, that's all > what is needed for the current implementation. > > Well someone needs to handle the volatility of hdcp, and atm we seem to be > playing a game of pass the bucket. I still think that mei_hdcp should supply a > clean interface to i915, with all the reset madness handled internally. But > depending upon how badly this all leaks we might need to have a retry logic in > the i915 hdcp flow too. Restart logic is must. > > device linke we'll probably need anyway, since i915 resuming when hdcp is not > yet up is not a good idea no matter what's goîng on. I've explored device_link and I'm not sure it is suitable there is no power relationship, on suspend/resume the device disappear. I still believe that class_interface is better choice, it this particular case. The whole issue is not yet resolved in the Linux kernel. There was a discussion around it in ELC https://schd.ws/hosted_files/osseu18/0f/deferred_problem.pdf Thanks Tomas
On Mon, Dec 17, 2018 at 11:57 AM Winkler, Tomas <tomas.winkler@intel.com> wrote: > > > > On Sat, Dec 15, 2018 at 09:20:38PM +0000, Winkler, Tomas wrote: > > > > > > > > On Thu, Dec 13, 2018 at 5:27 PM Winkler, Tomas > > > > <tomas.winkler@intel.com> > > > > wrote: > > > > > > > > > > > On Thu, Dec 13, 2018 at 1:36 PM C, Ramalingam > > > > > > <ramalingam.c@intel.com> > > > > > > wrote: > > > > > > > > > > > > > > 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-j4 > > > > > > > 005/ > > > > > > > igt@ > > > > > > > gem_exec_suspend@basic-s3.html > > > > > > > > > > > > > > More over unregistering I915 during the suspend is not expected. > > > > > > > So how do > > > > > > we handle this? > > > > > > > > > > > > Bit more context from our irc discussion with Ram: > > > > > > > > > > > > I found this very surprising, since I don't know of any other > > > > > > subsystems where the devices get outright removed when going > > > > > > through a > > > > suspend/resume cycle. > > > > > > The device model was built to handle this stuff > > > > > > correctly: First clients/devices/interfaces get suspend, then > > > > > > the parent/bridge/bus. Same dance in reverse when resuming. This > > > > > > even holds for lots of hotpluggable buses, where child devices > > > > > > could indeed disappear on resume, but as long as they don't, > > > > > > everything stays the same. It's really surprising for something > > > > > > that's soldered onto the > > > > board like ME. > > > > > > > > > > HDCP is an application in the ME it's not ME itself.. On the > > > > > linux side HDCP2 is a virtual device on mei client virtual bus, > > > > > the bus is teared > > > > down on ME reset, which mostly happen on power transitions. > > > > > Theoretically, we could keep it up during power transitions, but > > > > > so fare it was not necessary and second it's not guarantie that > > > > > the all ME > > > > applications will reappear after reset. > > > > > > > > When does this happen that an ME application doesn't come back after e.g. > > > > suspend/resume? > > > No, this can happen in special flows such as fw updates and error conditions, > > but is has to be supported as well. > > > > > > > > > > > Also, what's all the place where this reset can happen? Just > > > > suspend/resume/hibernate and all these, or also at other times? > > > > > > Also on errors and fw update, the basic assumption is here that it can happen > > any time. > > > > If this can happen any time, what are we supposed to do if this happens while > > we're doing something with the hdcp mei? If this is such a common occurence I > > guess we need to somehow wait until everyting is rebound and working again. I > > think ideally mei core would handle that for us, but I guess if this just randomly > > happens then we need to redo all the transactions. So does need some > > involvement of the higher levels. > > It's not common occurrence, but the assumption must be it can happen any time, > In that case everything has to restarted as there is no state preserved in the ME FW. > Right MEI core cannot do it for you, it is just a channel, the logic and state of the connection > is in the mei_hdcp or gfx. Note that HDCP is not the only App over MEI. Yes, each mei interface would need to provide suspend/resume functions, or something like that. Or at least a reset function. > > Also, how likely is it that the hdcp mei will outright disappear and not come > > back after a reset? > > > > > > How does userspace deal with the reset over s/r? I'm assuming that > > > > at least the device node file will become invalid (or whatever > > > > you're using as userspace api), so if userspace is accessing stuff > > > > on the me at the same time as we do a suspend/resume, what happens? > > > > Also, answer to how other users handle this would be enlighting. Still looking to understand this here. > > > > > > Aside: We'll probably need a device_link to make sure mei_hdcp > > > > > > is fully resumed before i915 gets resumed, but that's kinda a > > > > > > detail for later > > > > on. > > > > > > > > > > Frankly I don’t believe there is currently exact abstraction that > > > > > supports this model, neither components nor device_link . > > > > > So fare we used class interface for other purposes, it worked well. > > > > > > > > I'm not clear on what class interface has to do with component or device > > link. > > > > They all solve different problems, at least as far as I understand all this stuff > > ... > > > > -Daniel > > > > > > It comes instead of it, device_link is mostly used for power > > > management and component as we see know is not what we need as HDCP Is > > a b it volitle. > > > class_interface gives you two handlers: add and remove device, that's all > > what is needed for the current implementation. > > > > Well someone needs to handle the volatility of hdcp, and atm we seem to be > > playing a game of pass the bucket. I still think that mei_hdcp should supply a > > clean interface to i915, with all the reset madness handled internally. But > > depending upon how badly this all leaks we might need to have a retry logic in > > the i915 hdcp flow too. > > > Restart logic is must. Ok, I guess then we need to wrap another layer on top of mei to make this happen. Does mei provide any signal whether a client/app has not survived a reset? Atm there's not way for us to tell a reset apart from a "mei_hdcp disappared for good" event. Which we kinda need to do. Ideally a reset would be a distinct event and not implemented as an unbind/rebind cycle like it currently is. > > device linke we'll probably need anyway, since i915 resuming when hdcp is not > > yet up is not a good idea no matter what's goîng on. > > I've explored device_link and I'm not sure it is suitable there is no power relationship, on suspend/resume the device disappear. > I still believe that class_interface is better choice, it this particular case. I'm not sure what you mean with class_interface here. How are we supposed to use that in this case here? I'm not following you at all here. I also noticed that resume seems to be entirely deferred to workers: mei_restart only writes the me start command through the hbm. So all the clients will only be re-registered somewhen later on through an async worker (in the rescan_work). Is that understanding correct? If that's the case we'd need a way to wait for that, so we know whether the mei_hdcp is useable again or has disappeared for good. > The whole issue is not yet resolved in the Linux kernel. > There was a discussion around it in ELC https://schd.ws/hosted_files/osseu18/0f/deferred_problem.pdf There's still a bunch of open issues around deferred probe and device driver loading, but none that would interfer with what we're trying to do here. At least if mei wouldn't handle resets through a bind/unbind cycle. -Daniel > > Thanks > Tomas >
Tomas and Daniel,
From the discussion on this thread, I infer following understanding:
* At present(v9) I915 wants to be hard binded to mei_hdcp
device-driver binding status through components
o This means I915 driver load will get complete only when the
mei_hdcp's device and driver are bound.
o if mei_hdcp device reset I915 will unregister itself from
userspace, and wait for the mei_hdcp device-deriver rebinding.
+ Could be due to FW error or any unexpected failures those
are rare occurances.
o when mei_hdcp module is removed i915 will unregister itself.
o Becasue of this, Ideally I915 dont expect the device reset from
mei for suspend and resume.
* At present Mei bus is designed as below:
o Device will disappear on FW failures, FW upgrade, suspend of the
system etc.
o And when the errors are handled or on system resume mei device
will reappear, hence binding with corresponding driver.
* Mei doesn't plan to avoid the device reset(disappearance and
reappearance) for suspend and resume in near future.
Based on above understanding, I propose the below approach. Please correct or approve it.
* At present(v9) component_add from mei_hdcp indicates the mei_hdcp's
device-driver binded state.
* Instead lets use component to indicate the mei_hdcp's module
availability,
o by adding the component at module_init and removing it from
module_exit.
* This way I915 will not be impacted due to the mei device reset at
suspend.
* In such scenario I915 will have no idea about the device-driver bind
status of mei_hdcp.
o So incase of device is not available, mei_hdcp is responsible to
prune such calls with -EIO error.
* This approach avoid any future impact to I915, incase mei intended
to support suspend and resume.
I am aware this is not the ideal solution we want. But I feel this is the best at present we could do for this I915-mei interface.
Best regards,
Ram
On 12/17/2018 7:16 PM, Daniel Vetter wrote:
> On Mon, Dec 17, 2018 at 11:57 AM Winkler, Tomas <tomas.winkler@intel.com> wrote:
>>
>>> On Sat, Dec 15, 2018 at 09:20:38PM +0000, Winkler, Tomas wrote:
>>>>> On Thu, Dec 13, 2018 at 5:27 PM Winkler, Tomas
>>>>> <tomas.winkler@intel.com>
>>>>> wrote:
>>>>>>> On Thu, Dec 13, 2018 at 1:36 PM C, Ramalingam
>>>>>>> <ramalingam.c@intel.com>
>>>>>>> wrote:
>>>>>>>> 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-j4
>>>>>>>> 005/
>>>>>>>> igt@
>>>>>>>> gem_exec_suspend@basic-s3.html
>>>>>>>>
>>>>>>>> More over unregistering I915 during the suspend is not expected.
>>>>>>>> So how do
>>>>>>> we handle this?
>>>>>>>
>>>>>>> Bit more context from our irc discussion with Ram:
>>>>>>>
>>>>>>> I found this very surprising, since I don't know of any other
>>>>>>> subsystems where the devices get outright removed when going
>>>>>>> through a
>>>>> suspend/resume cycle.
>>>>>>> The device model was built to handle this stuff
>>>>>>> correctly: First clients/devices/interfaces get suspend, then
>>>>>>> the parent/bridge/bus. Same dance in reverse when resuming. This
>>>>>>> even holds for lots of hotpluggable buses, where child devices
>>>>>>> could indeed disappear on resume, but as long as they don't,
>>>>>>> everything stays the same. It's really surprising for something
>>>>>>> that's soldered onto the
>>>>> board like ME.
>>>>>> HDCP is an application in the ME it's not ME itself.. On the
>>>>>> linux side HDCP2 is a virtual device on mei client virtual bus,
>>>>>> the bus is teared
>>>>> down on ME reset, which mostly happen on power transitions.
>>>>>> Theoretically, we could keep it up during power transitions, but
>>>>>> so fare it was not necessary and second it's not guarantie that
>>>>>> the all ME
>>>>> applications will reappear after reset.
>>>>>
>>>>> When does this happen that an ME application doesn't come back after e.g.
>>>>> suspend/resume?
>>>> No, this can happen in special flows such as fw updates and error conditions,
>>> but is has to be supported as well.
>>>>> Also, what's all the place where this reset can happen? Just
>>>>> suspend/resume/hibernate and all these, or also at other times?
>>>> Also on errors and fw update, the basic assumption is here that it can happen
>>> any time.
>>>
>>> If this can happen any time, what are we supposed to do if this happens while
>>> we're doing something with the hdcp mei? If this is such a common occurence I
>>> guess we need to somehow wait until everyting is rebound and working again. I
>>> think ideally mei core would handle that for us, but I guess if this just randomly
>>> happens then we need to redo all the transactions. So does need some
>>> involvement of the higher levels.
>> It's not common occurrence, but the assumption must be it can happen any time,
>> In that case everything has to restarted as there is no state preserved in the ME FW.
>> Right MEI core cannot do it for you, it is just a channel, the logic and state of the connection
>> is in the mei_hdcp or gfx. Note that HDCP is not the only App over MEI.
> Yes, each mei interface would need to provide suspend/resume
> functions, or something like that. Or at least a reset function.
>
>>> Also, how likely is it that the hdcp mei will outright disappear and not come
>>> back after a reset?
>>>
>>>>> How does userspace deal with the reset over s/r? I'm assuming that
>>>>> at least the device node file will become invalid (or whatever
>>>>> you're using as userspace api), so if userspace is accessing stuff
>>>>> on the me at the same time as we do a suspend/resume, what happens?
>>> Also, answer to how other users handle this would be enlighting.
> Still looking to understand this here.
>
>>>>>>> Aside: We'll probably need a device_link to make sure mei_hdcp
>>>>>>> is fully resumed before i915 gets resumed, but that's kinda a
>>>>>>> detail for later
>>>>> on.
>>>>>> Frankly I don’t believe there is currently exact abstraction that
>>>>>> supports this model, neither components nor device_link .
>>>>>> So fare we used class interface for other purposes, it worked well.
>>>>> I'm not clear on what class interface has to do with component or device
>>> link.
>>>>> They all solve different problems, at least as far as I understand all this stuff
>>> ...
>>>>> -Daniel
>>>> It comes instead of it, device_link is mostly used for power
>>>> management and component as we see know is not what we need as HDCP Is
>>> a b it volitle.
>>>> class_interface gives you two handlers: add and remove device, that's all
>>> what is needed for the current implementation.
>>>
>>> Well someone needs to handle the volatility of hdcp, and atm we seem to be
>>> playing a game of pass the bucket. I still think that mei_hdcp should supply a
>>> clean interface to i915, with all the reset madness handled internally. But
>>> depending upon how badly this all leaks we might need to have a retry logic in
>>> the i915 hdcp flow too.
>>
>> Restart logic is must.
> Ok, I guess then we need to wrap another layer on top of mei to make
> this happen.
>
> Does mei provide any signal whether a client/app has not survived a
> reset? Atm there's not way for us to tell a reset apart from a
> "mei_hdcp disappared for good" event. Which we kinda need to do.
> Ideally a reset would be a distinct event and not implemented as an
> unbind/rebind cycle like it currently is.
>
>>> device linke we'll probably need anyway, since i915 resuming when hdcp is not
>>> yet up is not a good idea no matter what's goîng on.
>> I've explored device_link and I'm not sure it is suitable there is no power relationship, on suspend/resume the device disappear.
>> I still believe that class_interface is better choice, it this particular case.
> I'm not sure what you mean with class_interface here. How are we
> supposed to use that in this case here? I'm not following you at all
> here.
>
> I also noticed that resume seems to be entirely deferred to workers:
> mei_restart only writes the me start command through the hbm. So all
> the clients will only be re-registered somewhen later on through an
> async worker (in the rescan_work). Is that understanding correct? If
> that's the case we'd need a way to wait for that, so we know whether
> the mei_hdcp is useable again or has disappeared for good.
>
>> The whole issue is not yet resolved in the Linux kernel.
>> There was a discussion around it in ELC https://schd.ws/hosted_files/osseu18/0f/deferred_problem.pdf
> There's still a bunch of open issues around deferred probe and device
> driver loading, but none that would interfer with what we're trying to
> do here. At least if mei wouldn't handle resets through a bind/unbind
> cycle.
> -Daniel
>
>> Thanks
>> Tomas
>>
>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<pre>Tomas and Daniel,
From the discussion on this thread, I infer following understanding:
</pre>
<ul>
<li>At present(v9) I915 wants to be hard binded to mei_hdcp
device-driver binding status through components</li>
<ul>
<li>This means I915 driver load will get complete only when the
mei_hdcp's device and driver are bound.</li>
<li>if mei_hdcp device reset I915 will unregister itself from
userspace, and wait for the mei_hdcp device-deriver rebinding.</li>
<ul>
<li>Could be due to FW error or any unexpected failures those
are rare occurances.<br>
</li>
</ul>
<li>when mei_hdcp module is removed i915 will unregister itself.</li>
<li>Becasue of this, Ideally I915 dont expect the device reset
from mei for suspend and resume.</li>
</ul>
<li>At present Mei bus is designed as below:</li>
<ul>
<li>Device will disappear on FW failures, FW upgrade, suspend of
the system etc.</li>
<li>And when the errors are handled or on system resume mei
device will reappear, hence binding with corresponding driver.</li>
</ul>
<li>Mei doesn't plan to avoid the device reset(disappearance and
reappearance) for suspend and resume in near future.</li>
</ul>
<pre>Based on above understanding, I propose the below approach. Please correct or approve it.
</pre>
<ul>
<li>At present(v9) component_add from mei_hdcp indicates the
mei_hdcp's device-driver binded state.</li>
<li>Instead lets use component to indicate the mei_hdcp's module
availability,</li>
<ul>
<li>by adding the component at module_init and removing it from
module_exit.</li>
</ul>
<li>This way I915 will not be impacted due to the mei device reset
at suspend.</li>
<li>In such scenario I915 will have no idea about the
device-driver bind status of mei_hdcp.</li>
<ul>
<li>So incase of device is not available, mei_hdcp is
responsible to prune such calls with -EIO error.</li>
</ul>
<li>This approach avoid any future impact to I915, incase mei
intended to support suspend and resume.<br>
</li>
</ul>
<pre>I am aware this is not the ideal solution we want. But I feel this is the best at present we could do for this I915-mei interface.
</pre>
<pre>Best regards,
Ram
</pre>
<div class="moz-cite-prefix">On 12/17/2018 7:16 PM, Daniel Vetter
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAKMK7uEM9sCm7r4Hha0rWQ53Zp7nTukkC29sjt70vmp0uK62Fg@mail.gmail.com">
<pre class="moz-quote-pre" wrap="">On Mon, Dec 17, 2018 at 11:57 AM Winkler, Tomas <a class="moz-txt-link-rfc2396E" href="mailto:tomas.winkler@intel.com"><tomas.winkler@intel.com></a> wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On Sat, Dec 15, 2018 at 09:20:38PM +0000, Winkler, Tomas wrote:
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
On Thu, Dec 13, 2018 at 5:27 PM Winkler, Tomas
<a class="moz-txt-link-rfc2396E" href="mailto:tomas.winkler@intel.com"><tomas.winkler@intel.com></a>
wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On Thu, Dec 13, 2018 at 1:36 PM C, Ramalingam
<a class="moz-txt-link-rfc2396E" href="mailto:ramalingam.c@intel.com"><ramalingam.c@intel.com></a>
wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
Tomas and Daniel,
We got an issue here.
The relationship that we try to build between I915 and
mei_hdcp is as
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">follows:
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">component when the driver->probe is called (on device driver binding).
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">I915 forces itself such that until mei_hdcp component is added
I915_load
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">wont be complete.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Similarly on complete system, if mei_hdcp component is
removed,
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">immediately I915 unregister itself and HW will be shutdown.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
This is completely fine when the modules are loaded and unloaded.
But during suspend, mei device disappears and mei bus handles
it by
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">unbinding device and driver by calling driver->remove.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">This in-turn removes the component and triggers the master
unbind of I915
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">where, I915 unregister itself.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">This cause the HW state mismatch during the suspend and resume.
Please check the powerwell mismatch errors at CI report for v9
<a class="moz-txt-link-freetext" href="https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_3412/fi-glk-j4">https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_3412/fi-glk-j4</a>
005/
igt@
<a class="moz-txt-link-abbreviated" href="mailto:gem_exec_suspend@basic-s3.html">gem_exec_suspend@basic-s3.html</a>
More over unregistering I915 during the suspend is not expected.
So how do
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">we handle this?
Bit more context from our irc discussion with Ram:
I found this very surprising, since I don't know of any other
subsystems where the devices get outright removed when going
through a
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">suspend/resume cycle.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">The device model was built to handle this stuff
correctly: First clients/devices/interfaces get suspend, then
the parent/bridge/bus. Same dance in reverse when resuming. This
even holds for lots of hotpluggable buses, where child devices
could indeed disappear on resume, but as long as they don't,
everything stays the same. It's really surprising for something
that's soldered onto the
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">board like ME.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
HDCP is an application in the ME it's not ME itself.. On the
linux side HDCP2 is a virtual device on mei client virtual bus,
the bus is teared
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">down on ME reset, which mostly happen on power transitions.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Theoretically, we could keep it up during power transitions, but
so fare it was not necessary and second it's not guarantie that
the all ME
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">applications will reappear after reset.
When does this happen that an ME application doesn't come back after e.g.
suspend/resume?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">No, this can happen in special flows such as fw updates and error conditions,
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">but is has to be supported as well.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
Also, what's all the place where this reset can happen? Just
suspend/resume/hibernate and all these, or also at other times?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Also on errors and fw update, the basic assumption is here that it can happen
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">any time.
If this can happen any time, what are we supposed to do if this happens while
we're doing something with the hdcp mei? If this is such a common occurence I
guess we need to somehow wait until everyting is rebound and working again. I
think ideally mei core would handle that for us, but I guess if this just randomly
happens then we need to redo all the transactions. So does need some
involvement of the higher levels.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
It's not common occurrence, but the assumption must be it can happen any time,
In that case everything has to restarted as there is no state preserved in the ME FW.
Right MEI core cannot do it for you, it is just a channel, the logic and state of the connection
is in the mei_hdcp or gfx. Note that HDCP is not the only App over MEI.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Yes, each mei interface would need to provide suspend/resume
functions, or something like that. Or at least a reset function.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Also, how likely is it that the hdcp mei will outright disappear and not come
back after a reset?
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">How does userspace deal with the reset over s/r? I'm assuming that
at least the device node file will become invalid (or whatever
you're using as userspace api), so if userspace is accessing stuff
on the me at the same time as we do a suspend/resume, what happens?
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Also, answer to how other users handle this would be enlighting.
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Still looking to understand this here.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Aside: We'll probably need a device_link to make sure mei_hdcp
is fully resumed before i915 gets resumed, but that's kinda a
detail for later
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">on.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
Frankly I don’t believe there is currently exact abstraction that
supports this model, neither components nor device_link .
So fare we used class interface for other purposes, it worked well.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I'm not clear on what class interface has to do with component or device
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">link.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">They all solve different problems, at least as far as I understand all this stuff
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">...
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">-Daniel
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
It comes instead of it, device_link is mostly used for power
management and component as we see know is not what we need as HDCP Is
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">a b it volitle.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">class_interface gives you two handlers: add and remove device, that's all
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">what is needed for the current implementation.
Well someone needs to handle the volatility of hdcp, and atm we seem to be
playing a game of pass the bucket. I still think that mei_hdcp should supply a
clean interface to i915, with all the reset madness handled internally. But
depending upon how badly this all leaks we might need to have a retry logic in
the i915 hdcp flow too.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Restart logic is must.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Ok, I guess then we need to wrap another layer on top of mei to make
this happen.
Does mei provide any signal whether a client/app has not survived a
reset? Atm there's not way for us to tell a reset apart from a
"mei_hdcp disappared for good" event. Which we kinda need to do.
Ideally a reset would be a distinct event and not implemented as an
unbind/rebind cycle like it currently is.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">device linke we'll probably need anyway, since i915 resuming when hdcp is not
yet up is not a good idea no matter what's goîng on.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I've explored device_link and I'm not sure it is suitable there is no power relationship, on suspend/resume the device disappear.
I still believe that class_interface is better choice, it this particular case.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I'm not sure what you mean with class_interface here. How are we
supposed to use that in this case here? I'm not following you at all
here.
I also noticed that resume seems to be entirely deferred to workers:
mei_restart only writes the me start command through the hbm. So all
the clients will only be re-registered somewhen later on through an
async worker (in the rescan_work). Is that understanding correct? If
that's the case we'd need a way to wait for that, so we know whether
the mei_hdcp is useable again or has disappeared for good.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">The whole issue is not yet resolved in the Linux kernel.
There was a discussion around it in ELC <a class="moz-txt-link-freetext" href="https://schd.ws/hosted_files/osseu18/0f/deferred_problem.pdf">https://schd.ws/hosted_files/osseu18/0f/deferred_problem.pdf</a>
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
There's still a bunch of open issues around deferred probe and device
driver loading, but none that would interfer with what we're trying to
do here. At least if mei wouldn't handle resets through a bind/unbind
cycle.
-Daniel
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
Thanks
Tomas
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
</html>
On 12/19/2018 12:15 PM, C, Ramalingam wrote: > Tomas and Daniel, > > From the discussion on this thread, I infer following understanding: > > * At present(v9) I915 wants to be hard binded to mei_hdcp > device-driver binding status through components > o This means I915 driver load will get complete only when the > mei_hdcp's device and driver are bound. > o if mei_hdcp device reset I915 will unregister itself from > userspace, and wait for the mei_hdcp device-deriver rebinding. > + Could be due to FW error or any unexpected failures those > are rare occurances. > o when mei_hdcp module is removed i915 will unregister itself. > o Becasue of this, Ideally I915 dont expect the device reset > from mei for suspend and resume. > * At present Mei bus is designed as below: > o Device will disappear on FW failures, FW upgrade, suspend of > the system etc. > o And when the errors are handled or on system resume mei device > will reappear, hence binding with corresponding driver. > * Mei doesn't plan to avoid the device reset(disappearance and > reappearance) for suspend and resume in near future. > > Based on above understanding, I propose the below approach. Please correct or approve it. > > * At present(v9) component_add from mei_hdcp indicates the > mei_hdcp's device-driver binded state. > * Instead lets use component to indicate the mei_hdcp's module > availability, > o by adding the component at module_init and removing it from > module_exit. > * This way I915 will not be impacted due to the mei device reset at > suspend. > * In such scenario I915 will have no idea about the device-driver > bind status of mei_hdcp. > o So incase of device is not available, mei_hdcp is responsible > to prune such calls with -EIO error. > * This approach avoid any future impact to I915, incase mei intended > to support suspend and resume. > > I am aware this is not the ideal solution we want. But I feel this is the best at present we could do for this I915-mei interface. > Best regards, > Ram something like (un compiled code) diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c index b22a71e8c5d7..b5b57a883e3b 100644 --- a/drivers/misc/mei/hdcp/mei_hdcp.c +++ b/drivers/misc/mei/hdcp/mei_hdcp.c @@ -23,11 +23,15 @@ #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" +struct i915_component_master *i915_master_comp; +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 +695,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 +710,84 @@ 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; + + i915_master_comp = master_comp; + + 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; + i915_master_comp = 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; + + if (mei_hdcp_component_registered && i915_master_comp) { + i915_master_comp->mei_dev = dev; + return; + } + + 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) { + i915_master_comp->mei_dev = NULL; return mei_cldev_disable(cldev); } @@ -741,7 +808,23 @@ static struct mei_cl_driver mei_hdcp_driver = { .remove = mei_hdcp_remove, }; -module_mei_cl_driver(mei_hdcp_driver); +static int __init mei_hdcp_init(void) +{ + int ret; + + ret = mei_cldev_driver_register(mei_hdcp_driver); + if (ret) + return ret; +} + +static void __exit mei_hdcp_exit(void) +{ + mei_hdcp_component_cleanup(&cldev->dev); + mei_cldev_driver_unregister(mei_hdcp_driver); +} + +module_init(mei_hdcp_init); +module_exit(mei_hdcp_exit); MODULE_AUTHOR("Intel Corporation"); MODULE_LICENSE("Dual BSD/GPL"); -- 2.7.4 -Ram <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <p><br> </p> <div class="moz-cite-prefix">On 12/19/2018 12:15 PM, C, Ramalingam wrote:<br> </div> <blockquote type="cite" cite="mid:ec6fa6f2-d7a1-bdbf-8ff5-1738c773b868@intel.com"> <pre>Tomas and Daniel, From the discussion on this thread, I infer following understanding: </pre> <ul> <li>At present(v9) I915 wants to be hard binded to mei_hdcp device-driver binding status through components</li> <ul> <li>This means I915 driver load will get complete only when the mei_hdcp's device and driver are bound.</li> <li>if mei_hdcp device reset I915 will unregister itself from userspace, and wait for the mei_hdcp device-deriver rebinding.</li> <ul> <li>Could be due to FW error or any unexpected failures those are rare occurances.<br> </li> </ul> <li>when mei_hdcp module is removed i915 will unregister itself.</li> <li>Becasue of this, Ideally I915 dont expect the device reset from mei for suspend and resume.</li> </ul> <li>At present Mei bus is designed as below:</li> <ul> <li>Device will disappear on FW failures, FW upgrade, suspend of the system etc.</li> <li>And when the errors are handled or on system resume mei device will reappear, hence binding with corresponding driver.</li> </ul> <li>Mei doesn't plan to avoid the device reset(disappearance and reappearance) for suspend and resume in near future.</li> </ul> <pre>Based on above understanding, I propose the below approach. Please correct or approve it. </pre> <ul> <li>At present(v9) component_add from mei_hdcp indicates the mei_hdcp's device-driver binded state.</li> <li>Instead lets use component to indicate the mei_hdcp's module availability,</li> <ul> <li>by adding the component at module_init and removing it from module_exit.</li> </ul> <li>This way I915 will not be impacted due to the mei device reset at suspend.</li> <li>In such scenario I915 will have no idea about the device-driver bind status of mei_hdcp.</li> <ul> <li>So incase of device is not available, mei_hdcp is responsible to prune such calls with -EIO error.</li> </ul> <li>This approach avoid any future impact to I915, incase mei intended to support suspend and resume.<br> </li> </ul> <pre>I am aware this is not the ideal solution we want. But I feel this is the best at present we could do for this I915-mei interface. </pre> <pre>Best regards, Ram</pre> </blockquote> <p>something like (un compiled code)<br> </p> <pre>diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c index b22a71e8c5d7..b5b57a883e3b 100644 --- a/drivers/misc/mei/hdcp/mei_hdcp.c +++ b/drivers/misc/mei/hdcp/mei_hdcp.c @@ -23,11 +23,15 @@ #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" +struct i915_component_master *i915_master_comp; +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 +695,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 +710,84 @@ 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; + + i915_master_comp = master_comp; + + 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; + i915_master_comp = 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; + + if (mei_hdcp_component_registered && i915_master_comp) { + i915_master_comp->mei_dev = dev; + return; + } + + 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) { + i915_master_comp->mei_dev = NULL; return mei_cldev_disable(cldev); } @@ -741,7 +808,23 @@ static struct mei_cl_driver mei_hdcp_driver = { .remove = mei_hdcp_remove, }; -module_mei_cl_driver(mei_hdcp_driver); +static int __init mei_hdcp_init(void) +{ + int ret; + + ret = mei_cldev_driver_register(mei_hdcp_driver); + if (ret) + return ret; +} + +static void __exit mei_hdcp_exit(void) +{ + mei_hdcp_component_cleanup(&cldev->dev); + mei_cldev_driver_unregister(mei_hdcp_driver); +} + +module_init(mei_hdcp_init); +module_exit(mei_hdcp_exit); MODULE_AUTHOR("Intel Corporation"); MODULE_LICENSE("Dual BSD/GPL"); -- 2.7.4 -Ram </pre> <blockquote type="cite" cite="mid:ec6fa6f2-d7a1-bdbf-8ff5-1738c773b868@intel.com"> <pre> </pre> </blockquote> </body> </html>
From: C, Ramalingam
Sent: Thursday, December 20, 2018 18:00
To: Daniel Vetter <daniel@ffwll.ch>; Winkler, Tomas <tomas.winkler@intel.com>
Cc: Greg KH <gregkh@linuxfoundation.org>; Rafael J. Wysocki <rafael@kernel.org>; intel-gfx <intel-gfx@lists.freedesktop.org>; dri-devel <dri-devel@lists.freedesktop.org>; Sean Paul <seanpaul@chromium.org>; Shankar, Uma <uma.shankar@intel.com>; Syrjala, Ville <ville.syrjala@linux.intel.com>; Chris Wilson <chris@chris-wilson.co.uk>
Subject: Re: [PATCH v9 35/39] misc/mei/hdcp: Component framework for I915 Interface
On 12/19/2018 12:15 PM, C, Ramalingam wrote:
Tomas and Daniel,
From the discussion on this thread, I infer following understanding:
* At present(v9) I915 wants to be hard binded to mei_hdcp device-driver binding status through components
* This means I915 driver load will get complete only when the mei_hdcp's device and driver are bound.
* if mei_hdcp device reset I915 will unregister itself from userspace, and wait for the mei_hdcp device-deriver rebinding.
* Could be due to FW error or any unexpected failures those are rare occurances.
* when mei_hdcp module is removed i915 will unregister itself.
* Becasue of this, Ideally I915 dont expect the device reset from mei for suspend and resume.
* At present Mei bus is designed as below:
* Device will disappear on FW failures, FW upgrade, suspend of the system etc.
* And when the errors are handled or on system resume mei device will reappear, hence binding with corresponding driver.
* Mei doesn't plan to avoid the device reset(disappearance and reappearance) for suspend and resume in near future.
Based on above understanding, I propose the below approach. Please correct or approve it.
* At present(v9) component_add from mei_hdcp indicates the mei_hdcp's device-driver binded state.
* Instead lets use component to indicate the mei_hdcp's module availability,
* by adding the component at module_init and removing it from module_exit.
* This way I915 will not be impacted due to the mei device reset at suspend.
* In such scenario I915 will have no idea about the device-driver bind status of mei_hdcp.
* So incase of device is not available, mei_hdcp is responsible to prune such calls with -EIO error.
* This approach avoid any future impact to I915, incase mei intended to support suspend and resume.
I am aware this is not the ideal solution we want. But I feel this is the best at present we could do for this I915-mei interface.
Best regards,
Ram
something like (un compiled code)
diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
index b22a71e8c5d7..b5b57a883e3b 100644
--- a/drivers/misc/mei/hdcp/mei_hdcp.c
+++ b/drivers/misc/mei/hdcp/mei_hdcp.c
@@ -23,11 +23,15 @@
#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"
+struct i915_component_master *i915_master_comp;
+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 +695,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 +710,84 @@ 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;
+
+ i915_master_comp = master_comp;
+
+ 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;
+ i915_master_comp = 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;
+
+ if (mei_hdcp_component_registered && i915_master_comp) {
+ i915_master_comp->mei_dev = dev;
+ return;
+ }
+
+ 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)
{
+ i915_master_comp->mei_dev = NULL;
return mei_cldev_disable(cldev);
}
@@ -741,7 +808,23 @@ static struct mei_cl_driver mei_hdcp_driver = {
.remove = mei_hdcp_remove,
};
-module_mei_cl_driver(mei_hdcp_driver);
+static int __init mei_hdcp_init(void)
+{
+ int ret;
+
+ ret = mei_cldev_driver_register(mei_hdcp_driver);
+ if (ret)
+ return ret;
+}
+
+static void __exit mei_hdcp_exit(void)
+{
+ mei_hdcp_component_cleanup(&cldev->dev);
Don’t think you can do that, no guarantees this will be valid pointer
+ mei_cldev_driver_unregister(mei_hdcp_driver);
+}
+
+module_init(mei_hdcp_init);
+module_exit(mei_hdcp_exit);
MODULE_AUTHOR("Intel Corporation");
MODULE_LICENSE("Dual BSD/GPL");
--
2.7.4
-Ram
<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:#0563C1;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:#954F72;
text-decoration:underline;}
p
{mso-style-priority:99;
mso-margin-top-alt:auto;
margin-right:0cm;
mso-margin-bottom-alt:auto;
margin-left:0cm;
font-size:12.0pt;
font-family:"Times New Roman",serif;
color:black;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0cm;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;
color:black;}
span.EmailStyle20
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.WordSection1
{page:WordSection1;}
/* List Definitions */
@list l0
{mso-list-id:1073353968;
mso-list-template-ids:-234457178;}
@list l0:level1
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:36.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
@list l0:level2
{mso-level-number-format:bullet;
mso-level-text:o;
mso-level-tab-stop:72.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:"Courier New";
mso-bidi-font-family:"Times New Roman";}
@list l0:level3
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:108.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l0:level4
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:144.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l0:level5
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:180.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l0:level6
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:216.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l0:level7
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:252.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l0:level8
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:288.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l0:level9
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:324.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l1
{mso-list-id:1247572772;
mso-list-template-ids:-592145000;}
@list l1:level1
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:36.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
@list l1:level2
{mso-level-number-format:bullet;
mso-level-text:o;
mso-level-tab-stop:72.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:"Courier New";
mso-bidi-font-family:"Times New Roman";}
@list l1:level3
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:108.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l1:level4
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:144.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l1:level5
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:180.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l1:level6
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:216.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l1:level7
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:252.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l1:level8
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:288.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l1:level9
{mso-level-number-format:bullet;
mso-level-text:;
mso-level-tab-stop:324.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
ol
{margin-bottom:0cm;}
ul
{margin-bottom:0cm;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body bgcolor="white" lang="EN-US" link="#0563C1" vlink="#954F72">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><a name="_MailEndCompose"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></a></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><a name="_____replyseparator"></a><a name="_GingerEndOfDocBM"></a><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">
C, Ramalingam <br>
<b>Sent:</b> Thursday, December 20, 2018 18:00<br>
<b>To:</b> Daniel Vetter <daniel@ffwll.ch>; Winkler, Tomas <tomas.winkler@intel.com><br>
<b>Cc:</b> Greg KH <gregkh@linuxfoundation.org>; Rafael J. Wysocki <rafael@kernel.org>; intel-gfx <intel-gfx@lists.freedesktop.org>; dri-devel <dri-devel@lists.freedesktop.org>; Sean Paul <seanpaul@chromium.org>; Shankar, Uma <uma.shankar@intel.com>; Syrjala,
Ville <ville.syrjala@linux.intel.com>; Chris Wilson <chris@chris-wilson.co.uk><br>
<b>Subject:</b> Re: [PATCH v9 35/39] misc/mei/hdcp: Component framework for I915 Interface<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p><o:p> </o:p></p>
<div>
<p class="MsoNormal">On 12/19/2018 12:15 PM, C, Ramalingam wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>Tomas and Daniel,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>From the discussion on this thread, I infer following understanding:<o:p></o:p></pre>
<ul type="disc">
<li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l1 level1 lfo1">
At present(v9) I915 wants to be hard binded to mei_hdcp device-driver binding status through components<o:p></o:p></li></ul>
<ul type="disc">
<ul type="circle">
<li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l1 level2 lfo1">
This means I915 driver load will get complete only when the mei_hdcp's device and driver are bound.<o:p></o:p></li><li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l1 level2 lfo1">
if mei_hdcp device reset I915 will unregister itself from userspace, and wait for the mei_hdcp device-deriver rebinding.<o:p></o:p></li></ul>
</ul>
<ul type="disc">
<ul type="circle">
<ul type="square">
<li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l1 level3 lfo1">
Could be due to FW error or any unexpected failures those are rare occurances.<o:p></o:p></li></ul>
</ul>
</ul>
<ul type="disc">
<ul type="circle">
<li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l1 level2 lfo1">
when mei_hdcp module is removed i915 will unregister itself.<o:p></o:p></li><li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l1 level2 lfo1">
Becasue of this, Ideally I915 dont expect the device reset from mei for suspend and resume.<o:p></o:p></li></ul>
</ul>
<ul type="disc">
<li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l1 level1 lfo1">
At present Mei bus is designed as below:<o:p></o:p></li></ul>
<ul type="disc">
<ul type="circle">
<li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l1 level2 lfo1">
Device will disappear on FW failures, FW upgrade, suspend of the system etc.<o:p></o:p></li><li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l1 level2 lfo1">
And when the errors are handled or on system resume mei device will reappear, hence binding with corresponding driver.<o:p></o:p></li></ul>
</ul>
<ul type="disc">
<li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l1 level1 lfo1">
Mei doesn't plan to avoid the device reset(disappearance and reappearance) for suspend and resume in near future.<o:p></o:p></li></ul>
<pre>Based on above understanding, I propose the below approach. Please correct or approve it.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<ul type="disc">
<li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level1 lfo2">
At present(v9) component_add from mei_hdcp indicates the mei_hdcp's device-driver binded state.<o:p></o:p></li><li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level1 lfo2">
Instead lets use component to indicate the mei_hdcp's module availability,<o:p></o:p></li></ul>
<ul type="disc">
<ul type="circle">
<li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level2 lfo2">
by adding the component at module_init and removing it from module_exit.<o:p></o:p></li></ul>
</ul>
<ul type="disc">
<li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level1 lfo2">
This way I915 will not be impacted due to the mei device reset at suspend.<o:p></o:p></li><li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level1 lfo2">
In such scenario I915 will have no idea about the device-driver bind status of mei_hdcp.<o:p></o:p></li></ul>
<ul type="disc">
<ul type="circle">
<li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level2 lfo2">
So incase of device is not available, mei_hdcp is responsible to prune such calls with -EIO error.<o:p></o:p></li></ul>
</ul>
<ul type="disc">
<li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level1 lfo2">
This approach avoid any future impact to I915, incase mei intended to support suspend and resume.<o:p></o:p></li></ul>
<pre>I am aware this is not the ideal solution we want. But I feel this is the best at present we could do for this I915-mei interface.<o:p></o:p></pre>
<pre>Best regards,<o:p></o:p></pre>
<pre>Ram<o:p></o:p></pre>
</blockquote>
<p>something like (un compiled code)<o:p></o:p></p>
<pre>diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c<o:p></o:p></pre>
<pre>index b22a71e8c5d7..b5b57a883e3b 100644<o:p></o:p></pre>
<pre>--- a/drivers/misc/mei/hdcp/mei_hdcp.c<o:p></o:p></pre>
<pre>+++ b/drivers/misc/mei/hdcp/mei_hdcp.c<o:p></o:p></pre>
<pre>@@ -23,11 +23,15 @@<o:p></o:p></pre>
<pre> #include <linux/slab.h><o:p></o:p></pre>
<pre> #include <linux/uuid.h><o:p></o:p></pre>
<pre> #include <linux/mei_cl_bus.h><o:p></o:p></pre>
<pre>+#include <linux/component.h><o:p></o:p></pre>
<pre> #include <drm/drm_connector.h><o:p></o:p></pre>
<pre> #include <drm/i915_component.h><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> #include "mei_hdcp.h"<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>+struct i915_component_master *i915_master_comp;<o:p></o:p></pre>
<pre>+static bool mei_hdcp_component_registered;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre> /**<o:p></o:p></pre>
<pre> * mei_initiate_hdcp2_session() - Initiate a Wired HDCP2.2 Tx Session in ME FW<o:p></o:p></pre>
<pre> * @dev: device corresponding to the mei_cl_device<o:p></o:p></pre>
<pre>@@ -691,8 +695,7 @@ mei_close_hdcp_session(struct device *dev, struct hdcp_port_data *data)<o:p></o:p></pre>
<pre> return 0;<o:p></o:p></pre>
<pre> }<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>-static __attribute__((unused))<o:p></o:p></pre>
<pre>-struct i915_hdcp_component_ops mei_hdcp_ops = {<o:p></o:p></pre>
<pre>+static struct i915_hdcp_component_ops mei_hdcp_ops = {<o:p></o:p></pre>
<pre> .owner = THIS_MODULE,<o:p></o:p></pre>
<pre> .initiate_hdcp2_session = mei_initiate_hdcp2_session,<o:p></o:p></pre>
<pre> .verify_receiver_cert_prepare_km = mei_verify_receiver_cert_prepare_km,<o:p></o:p></pre>
<pre>@@ -707,20 +710,84 @@ struct i915_hdcp_component_ops mei_hdcp_ops = {<o:p></o:p></pre>
<pre> .close_hdcp_session = mei_close_hdcp_session,<o:p></o:p></pre>
<pre> };<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>+static int mei_hdcp_component_bind(struct device *mei_kdev,<o:p></o:p></pre>
<pre>+ struct device *i915_kdev, void *data)<o:p></o:p></pre>
<pre>+{<o:p></o:p></pre>
<pre>+ struct i915_component_master *master_comp = data;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ dev_info(mei_kdev, "MEI HDCP comp bind\n");<o:p></o:p></pre>
<pre>+ WARN_ON(master_comp->hdcp_ops);<o:p></o:p></pre>
<pre>+ master_comp->hdcp_ops = &mei_hdcp_ops;<o:p></o:p></pre>
<pre>+ master_comp->mei_dev = mei_kdev;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ i915_master_comp = master_comp;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ return 0;<o:p></o:p></pre>
<pre>+}<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+static void mei_hdcp_component_unbind(struct device *mei_kdev,<o:p></o:p></pre>
<pre>+ struct device *i915_kdev, void *data)<o:p></o:p></pre>
<pre>+{<o:p></o:p></pre>
<pre>+ struct i915_component_master *master_comp = data;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ dev_info(mei_kdev, "MEI HDCP comp unbind\n");<o:p></o:p></pre>
<pre>+ master_comp->hdcp_ops = NULL;<o:p></o:p></pre>
<pre>+ master_comp->mei_dev = NULL;<o:p></o:p></pre>
<pre>+ i915_master_comp = NULL;<o:p></o:p></pre>
<pre>+}<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+static const struct component_ops mei_hdcp_component_bind_ops = {<o:p></o:p></pre>
<pre>+ .bind = mei_hdcp_component_bind,<o:p></o:p></pre>
<pre>+ .unbind = mei_hdcp_component_unbind,<o:p></o:p></pre>
<pre>+};<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+static void mei_hdcp_component_init(struct device *dev)<o:p></o:p></pre>
<pre>+{<o:p></o:p></pre>
<pre>+ int ret;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ if (mei_hdcp_component_registered && i915_master_comp) {<o:p></o:p></pre>
<pre>+ i915_master_comp->mei_dev = dev;<o:p></o:p></pre>
<pre>+ return;<o:p></o:p></pre>
<pre>+ }<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ dev_info(dev, "MEI HDCP comp init\n");<o:p></o:p></pre>
<pre>+ ret = component_add(dev, &mei_hdcp_component_bind_ops);<o:p></o:p></pre>
<pre>+ if (ret < 0) {<o:p></o:p></pre>
<pre>+ dev_err(dev, "Failed to add MEI HDCP comp (%d)\n", ret);<o:p></o:p></pre>
<pre>+ return;<o:p></o:p></pre>
<pre>+ }<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ mei_hdcp_component_registered = true;<o:p></o:p></pre>
<pre>+}<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+static void mei_hdcp_component_cleanup(struct device *dev)<o:p></o:p></pre>
<pre>+{<o:p></o:p></pre>
<pre>+ if (!mei_hdcp_component_registered)<o:p></o:p></pre>
<pre>+ return;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ dev_info(dev, "MEI HDCP comp cleanup\n");<o:p></o:p></pre>
<pre>+ component_del(dev, &mei_hdcp_component_bind_ops);<o:p></o:p></pre>
<pre>+ mei_hdcp_component_registered = false;<o:p></o:p></pre>
<pre>+}<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre> static int mei_hdcp_probe(struct mei_cl_device *cldev,<o:p></o:p></pre>
<pre> const struct mei_cl_device_id *id)<o:p></o:p></pre>
<pre> {<o:p></o:p></pre>
<pre> int ret;<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> ret = mei_cldev_enable(cldev);<o:p></o:p></pre>
<pre>- if (ret < 0)<o:p></o:p></pre>
<pre>+ if (ret < 0) {<o:p></o:p></pre>
<pre> dev_err(&cldev->dev, "mei_cldev_enable Failed. %d\n", ret);<o:p></o:p></pre>
<pre>+ return ret;<o:p></o:p></pre>
<pre>+ }<o:p></o:p></pre>
<pre>+ mei_hdcp_component_init(&cldev->dev);<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>- return ret;<o:p></o:p></pre>
<pre>+ return 0;<o:p></o:p></pre>
<pre> }<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> static int mei_hdcp_remove(struct mei_cl_device *cldev)<o:p></o:p></pre>
<pre> {<o:p></o:p></pre>
<pre>+ i915_master_comp->mei_dev = NULL;<o:p></o:p></pre>
<pre> return mei_cldev_disable(cldev);<o:p></o:p></pre>
<pre> }<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>@@ -741,7 +808,23 @@ static struct mei_cl_driver mei_hdcp_driver = {<o:p></o:p></pre>
<pre> .remove = mei_hdcp_remove,<o:p></o:p></pre>
<pre> };<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>-module_mei_cl_driver(mei_hdcp_driver);<o:p></o:p></pre>
<pre>+static int __init mei_hdcp_init(void)<o:p></o:p></pre>
<pre>+{<o:p></o:p></pre>
<pre>+ int ret;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ ret = mei_cldev_driver_register(mei_hdcp_driver);<o:p></o:p></pre>
<pre>+ if (ret)<o:p></o:p></pre>
<pre>+ return ret;<o:p></o:p></pre>
<pre>+}<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+static void __exit mei_hdcp_exit(void)<o:p></o:p></pre>
<pre>+{<o:p></o:p></pre>
<pre>+ mei_hdcp_component_cleanup(&cldev->dev);<o:p></o:p></pre>
<pre><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Don’t think you can do that, no guarantees this will be valid pointer<o:p></o:p></span></pre>
<pre><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></pre>
<pre>+ mei_cldev_driver_unregister(mei_hdcp_driver);<o:p></o:p></pre>
<pre>+}<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+module_init(mei_hdcp_init);<o:p></o:p></pre>
<pre>+module_exit(mei_hdcp_exit);<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> MODULE_AUTHOR("Intel Corporation");<o:p></o:p></pre>
<pre> MODULE_LICENSE("Dual BSD/GPL");<o:p></o:p></pre>
<pre>--<o:p></o:p></pre>
<pre>2.7.4<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre>-Ram<o:p></o:p></pre>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
</blockquote>
</div>
</div>
</body>
</html>
On 12/20/2018 9:36 PM, Winkler, Tomas wrote: > +static void __exit mei_hdcp_exit(void) > +{ > + mei_hdcp_component_cleanup(&cldev->dev); > Don’t think you can do that, no guarantees this will be valid pointer As we discussed offline, we have the below line at cleanup. So valid pointer is made sure. I will protect init and cleanup with mutex too. +static void mei_hdcp_component_cleanup(struct device *dev) +{ + if (!mei_hdcp_component_registered) + return; -Ram > + mei_cldev_driver_unregister(mei_hdcp_driver); > +} > + > +module_init(mei_hdcp_init); > +module_exit(mei_hdcp_exit); <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <p><br> </p> <div class="moz-cite-prefix">On 12/20/2018 9:36 PM, Winkler, Tomas wrote:<br> </div> <blockquote type="cite" cite="mid:5B8DA87D05A7694D9FA63FD143655C1B9DA5947E@hasmsx108.ger.corp.intel.com"> <pre>+static void __exit mei_hdcp_exit(void)<o:p></o:p></pre> <pre>+{<o:p></o:p></pre> <pre>+ mei_hdcp_component_cleanup(&cldev->dev);<o:p></o:p></pre> <pre><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Don’t think you can do that, no guarantees this will be valid pointer</span></pre> </blockquote> <pre>As we discussed offline, we have the below line at cleanup. So valid pointer is made sure. I will protect init and cleanup with mutex too. +static void mei_hdcp_component_cleanup(struct device *dev) +{ + if (!mei_hdcp_component_registered) + return; -Ram </pre> <blockquote type="cite" cite="mid:5B8DA87D05A7694D9FA63FD143655C1B9DA5947E@hasmsx108.ger.corp.intel.com"> <pre><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p></o:p></span></pre> <pre><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></pre> <pre>+ mei_cldev_driver_unregister(mei_hdcp_driver);<o:p></o:p></pre> <pre>+}<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+module_init(mei_hdcp_init);<o:p></o:p></pre> <pre>+module_exit(mei_hdcp_exit);</pre> </blockquote> </body> </html>
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); }