Re: [PATCH v8 05/35] drm/i915: MEI interface definition

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Dec 12, 2018 at 02:28:29PM +0530, C, Ramalingam wrote:
> On 12/7/2018 7:59 PM, Daniel Vetter wrote:
> > On Fri, Dec 07, 2018 at 11:22:44AM +0530, C, Ramalingam wrote:
> > > On 12/6/2018 3:53 PM, Daniel Vetter wrote:
> > > > On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
> > > > > +static void i915_hdcp_component_master_unbind(struct device *dev)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
> > > > > +
> > > > > +	intel_connectors_hdcp_disable(dev_priv);
> > > > Why this code? Once we've unregistered the driver, we should have shut
> > > > down the hardware completely. Which should have shut down all the hdcp
> > > > users too.
> > > This unbind might be triggered either due to master_del or component_del.
> > > if its triggered from mei through component_del, then before teardown of
> > > the i/f in component_unbind(), disable the ongoing HDCP session through
> > > hdcp_disable() for each connectors.
> > I looked at your v7 component code again. I think if we put the
> > drm_atomic_helper_shutdown() call into master_unbind, then we'll have taken care
> > of that. Essentially what you're doing here is open-coding part of that
> > function. Better to just move the existing call to the right place.
> > 
> > And shutting down the entire hw is kinda what master_unbind should be
> > doing I think. We might also need to move the general hw quiescent into
> > master_unbind, that should work too.
> 
> Need some more information on this. As per v7 on master_unbind will invoke
> i915_unload_head that is i915_driver_unregister(dev_priv);
> Similarly master_bind will call the load_tail that is i915_driver_register(dev_priv);
> 
> We are not initializing/shutting the HW in master_bind/unbind .
> But this comment is contradicting with current approach. Could you please elaborate.?

So my understanding is that we need to shut down all hdcp usage before
master_unbind completes, because otherwise we'll blow up: The mei_hdcp
component is gone already.

Now the other case for master_unbind is that the i915 pci device is
unloaded, and in that case again I think it makes sense to shut down the
hardware in master_unbind.

Now when I looked at v7, right after the component_unbind code in the i915
unload sequences comes the function calls to shut down the hardware. I
think it would make sense to them (or at least the
drm_atomic_helper_shutdown() call) into master_unbind.

This is somewhat asymetric, but that's ok: Load path doesn't enable
anything, we just keep the hardware as-is (to be able to support
flicker-free boôt-up). First modest is done by usersapce. Exception is if
you have fbcon enabled (and not use the fastboot patches that Hans just
merged), in that case the kernel will do the first modeset when we
regiseter the fbdev device.

Anyway, moving the drm_atomic_helper_shutdown() into the master_unbind
function in v7 should take care of disabling all outputs, and hence also
disabling all usage of hdcp component.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel