From mboxrd@z Thu Jan 1 00:00:00 1970 From: "C, Ramalingam" Subject: Re: [PATCH v9 35/39] misc/mei/hdcp: Component framework for I915 Interface Date: Thu, 20 Dec 2018 21:29:50 +0530 Message-ID: <19db4272-428a-0575-0673-2db2cd21f05b@intel.com> References: <1544673701-6353-1-git-send-email-ramalingam.c@intel.com> <1544673701-6353-36-git-send-email-ramalingam.c@intel.com> <5B8DA87D05A7694D9FA63FD143655C1B9DA5248E@hasmsx108.ger.corp.intel.com> <5B8DA87D05A7694D9FA63FD143655C1B9DA53C91@hasmsx108.ger.corp.intel.com> <20181217093907.GY21184@phenom.ffwll.local> <5B8DA87D05A7694D9FA63FD143655C1B9DA557ED@hasmsx108.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1230816793==" Return-path: In-Reply-To: Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter , "Winkler, Tomas" Cc: "Rafael J. Wysocki" , Greg KH , intel-gfx , dri-devel , Sean Paul List-Id: dri-devel@lists.freedesktop.org This is a multi-part message in MIME format. --===============1230816793== Content-Type: multipart/alternative; boundary="------------9D9617524518F3585B35721E" Content-Language: en-US This is a multi-part message in MIME format. --------------9D9617524518F3585B35721E Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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 #include #include +#include #include #include #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 --------------9D9617524518F3585B35721E Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit


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);
+       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


    

  


--------------9D9617524518F3585B35721E--

--===============1230816793==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4
IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz
dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg==

--===============1230816793==--