[PATCH v9 35/39] misc/mei/hdcp: Component framework for I915 Interface
Winkler, Tomas
tomas.winkler at intel.com
Sat Dec 15 21:20:38 UTC 2018
>
> On Thu, Dec 13, 2018 at 5:27 PM Winkler, Tomas <tomas.winkler at intel.com>
> wrote:
> >
> > > On Thu, Dec 13, 2018 at 1:36 PM C, Ramalingam
> > > <ramalingam.c at intel.com>
> > > 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 at basic-s3.html
> > > >
> > > > 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 at intel.com>
> > > > Reviewed-by: Uma Shankar <uma.shankar at intel.com>
> > > > ---
> > > > 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
More information about the dri-devel
mailing list