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

[C, Ramalingam]

On 12/12/2018 4:34 PM, C, Ramalingam wrote:
>
>
> 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.

Summarizing the #intel-gfx IRC discussion:

As i915 load and unload  are already asymetric, there is no harm in moving
the drm_atomic_helper_shutdown() into the master_unbind(). So going
ahead with daniel's suggestion.

-Ram

>
> -Ram
>> -Daniel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20181213/0cacdef1/attachment.html>