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

["Winkler, Tomas" <tomas.winkler@intel.com>]

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



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



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

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

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