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

[C, Ramalingam]

On 12/12/2018 4:08 PM, Daniel Vetter wrote:
> 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.

But in that case master_bind should do the reverse of the drm_atomic_helper_shutdown()right?
else lets say mei_hdcp is removed, master_unbind triggered and hence all hw is shutdown.
And then mei_hdcp is loaded so master_bind should initialize the hw right? Which is not the scenario right now.

-Ram

> -Daniel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20181212/25370be5/attachment-0001.html>