> > On Thu, Dec 13, 2018 at 5:27 PM Winkler, Tomas <tomas.winkler@xxxxxxxxx> > wrote: > > > > > On Thu, Dec 13, 2018 at 1:36 PM C, Ramalingam > > > <ramalingam.c@xxxxxxxxx> > > > 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@xxxxxxxxxxxxx > > > > > > > > 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@xxxxxxxxx> > > > > Reviewed-by: Uma Shankar <uma.shankar@xxxxxxxxx> > > > > --- > > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
