<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></style> </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>