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@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. 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@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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
