Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Andrzej Hajda <a.hajda@samsung.com>]

On 11/04/2015 08:56 AM, Inki Dae wrote:
>
> 2015년 11월 04일 16:24에 Andrzej Hajda 이(가) 쓴 글:
>> On 11/03/2015 04:38 PM, Inki Dae wrote:
>>>  
>>> 2015-11-03 22:24 GMT+09:00 Andrzej Hajda <a.hajda@samsung.com
>>> <mailto:a.hajda@samsung.com>>:
>>>> Hi Inki,
>>>>
>>>> On 11/03/2015 11:47 AM, Inki Dae wrote:
>>>>> This patch series adds pm runtime support for Exynos drm.
>>>>>
>>>>> Originally, this patch was posted by Gustavo but there was no any
>>>>> answer about some comments. So I rebased this patch series on top of
>>>>> exynos-drm-next, removed unnecessary patches and modified wrong macro.
>>>> I have sent comment to original patchset[1], but for some strange reasons
>>>> it went only to mailing lists.
>>>> My concerns were as follows:
>>>> - exynos_drm has already pm_runtime support via exynos_drm_drv pm ops,
>>>>   why should we add per component support?
>>>  
>>> For this, I think I already mentioned enough,
>>>         http://www.spinics.net/lists/linux-samsung-soc/msg47695.html
>>>
>>> In brief, exynos_drm_drv doesn't control each power domain of each kms device.
>>> It means that we cannot power off fully without pm runtime interface of each
>>> kms device - just they can only control their clock not power domain. So
>>> question. How we could control power domain of each kms device without runtime
>>> pm interface?
>> Hmm, but to enable pm_runtime in components it is enough to use
>> pm_runtime_enable/disable and pm_runtime_get/put(_sync), there is no need to add
>> pm callbacks.
> That is this patch series does, and pm callback is implemented in exynos_drm_drv not in component drivers.

But this patchset adds runtime_pm ops to each component. Runtime_pm support does
not require
implementing runtime_pm ops, they just increases complexity of the code, and I
see no gain in splitting
power off/on sequence between drm enable/disable callbacks and suspend/resume
callbacks.

Regards
Andrzej

>
>> In fact most of the components already supports runtime pm,  but quick grep
>> shows that it is not implemented in:
>> exynos_drm_dsi.c, exynos_dp_core.c, exynos_drm_mic.c.
> exynos_dp_core already has runtime pm interface with this patch series. For dsi and mic, we need to add the runtime pm interfaces.
>
>> So I guess adding pm_runtime support by adding calls pm_runtime_enable/disable
>> and pm_runtime_get/put(_sync) in appropriate places should be enough.
>>
>>>> - component suspend sequence is non deterministic, but in case of
>>>>   video pipelines, specification often requires fixed order,
>>> Can your show me an example? I think component suspend and resume are invoked
>>> by only drm dpms interface, and the dpms interface has a fixed order - when
>>> dpms goes to off, encoder first, and when dpms goes to on, crtc first.
>> Now as I understand you do not want to remove exynos_drm_drv pm callbacks so
>> they will disable devices properly, after that clock disabling order should not
>> matter, so the whole point is not valid.
> In this case, the dpms actually is invoked by SLEEP PM of Linux power management framework. DRM KMS has a interface to control power of kms device, which is really different from SLEEP PM.
> So this request can be done by userspace in runtime - just Display power off/on.
>
>>> My only concern is that runtime pm interface of each kms driver is called
>>> within pm sleep context of Exynos drm driver. So I will look into this no problem.
>>>
>>>> - the patchset adds implicit dependency on PM_SLEEP.
>>>>
>>>>
>>>> Current solution should work correctly and it was OK last time I have tested it.
>>>> I am not sure about atomic requirements, are there special ones?
>>> Not related to atomic feature.
>>>
>>>> There are other issues with current solution, rather easy to solve:
>>>> - it assumes that exynos-drm device will be suspended first - it should be true,
>>>>  as it is created at the end and suspend order is reverse to creation order, but
>>>>  I am not sure if we can rely on it - some solution is to add pm callbacks to
>>>>  all components, and from those callbacks call one centralized pm routine,
>>> You mean pm callbacks are pm sleep interface? And you mean let's add sleep pm
>>> interface to each kms driver?  If so, I'm not agree with you because sleep pm
>>> should be controlled by only drm top like single driver does.
>> Lets look at an example:
>> Assume we have present only fimd and dsi components.
>> In such case kernel creates two platform devices for fimd and dsi in early stage
>> of parsing device tree blob.
>> During exynos_drm_drv initialization exynos-drm platform device is created, the
>> important thing it is created after fimd and dsi.
>> Now when platform enters sleep mode suspend callbacks are called, the important
>> thing here is
>> in which order. If I remember correctly PM guarantees only that children will be
>> handled before parents.
>> Since we have no parent-child relation between exynos-drm, fimd and dsi, the
>> order is unknown.
> exynos-drm is not real hardware. So the order is only valid for kms devices - components.
> The only thing exynos-drm does is to invoke dpms so that kms devices are enabled or disabled in fixed order.
>
> And as I mentioned before, my only concern is that runtime pm interface of each kms driver is called
> within pm sleep context of Exynos drm driver. So required for more review and test.
>
>> So it will be possible that component will enter sleep mode before exynos-drm
> How component can enter sleep mode before exynos-drm? compoments have no sleep pm interfaces but only runtime pm interface,
> and even the runtime pm interfaces will be called by pm interfaces of exynos-drm only when sleep pm is requested.
> Otherwise, the runtime pm interface of each component will be called by dpms request from userspace.
>
>> and in such case system can even
>> hang if exynos-drm callbacks will try to access registers of such component.
> So seems not true.
>
>> Fortunately in current implementation of PM order of suspending is reversed
>> order of device creation, so
>> it will be always exynos-drm first.
>>
>> So in short we rely here on implementation detail of PM framework.
>>
>>>> - suspend/resume callbacks theoretically can be called during component
>>>>   master initialization/deinitailization it could be racy,
>>> I'm not sure what you mean but component master
>>> initialization/deinitailization mean bind and unbind? If so, bind/unbind
>>> interfaces of all components will never call pm relevant interfaces but the
>>> the pm relevant interfaces are called by only dpms interface.
>> exynos_drm_sys_suspend and exynos_drm_sys_resume takes pointer
>> to drm_device and uses it. It is possible that in the same time drm_driver
>> is destroyed for some reason it will result in invalid pointer in resume/suspend
>> callbacks.
> Not sure but seems like to need more review and test anyway.
>
>> The problem is common for componentized drivers and was already reported [1],
>> but there
>> is no general solution for it. On the other side probability it will ever happen
>> seems to be very low.
>>
>> [1]: http://permalink.gmane.org/gmane.comp.video.dri.devel/115820
>>
>>>> - exynos_drm_sys_suspend/resume calls exynos_drm_suspend/resume
>>>>   for historical reasons, these function can be merged together.
>>> Also can you show me more detail?
>> This is quite simple, I will post patch for it.
> Thanks for sharing.
>
> Anyway, I believe Gustavo to take care of this patch series after back from his vacation.
>
> Thanks,
> Inki Dae
>
>> Regards
>> Andrzej
>>
>>> Thanks,
>>> Inki Dae
>>>
>>>> [1]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/48395
>>>>
>>>> Regards
>>>> Andrzej
>>>>
>>>>> Changelog v2:
>>>>> - Remove patch 5 and 6.
>>>>>   . commit callback are already removed so isn't required anymore.
>>>>> - Remove patch 8 which makes dp clock enabled directly from FIMD.
>>>>>   . Really not mendatory for FIMD uses DP, and it could be different
>>>>>     according to Board.
>>>>> - Modified CONFIG_PM_SLEEP to CONFIG_PM.
>>>>>   . In case of runtime pm, CONFIG_PM macro should be used instead of
>>>>>     CONFIG_PM_SLEEP.
>>>>>
>>>>> Gustavo Padovan (7):
>>>>>   drm/exynos: do not start enabling DP at bind() phase
>>>>>   drm/exynos: add pm_runtime to DP
>>>>>   drm/exynos: add pm_runtime to HDMI
>>>>>   drm/exynos: add pm_runtime to Mixer
>>>>>   drm/exynos: add pm_runtime to FIMD
>>>>>   drm/exynos: add pm_runtime to DECON 5433
>>>>>   drm/exynos: add pm_runtime to DECON 7
>>>>>
>>>>>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  54 ++++++---
>>>>>  drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 125 +++++++++----------
>>>>>  drivers/gpu/drm/exynos/exynos_dp_core.c       | 165 +++++++++++++++++++-------
>>>>>  drivers/gpu/drm/exynos/exynos_dp_core.h       |   1 +
>>>>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  91 ++++++--------
>>>>>  drivers/gpu/drm/exynos/exynos_hdmi.c          |  56 ++++++---
>>>>>  drivers/gpu/drm/exynos/exynos_mixer.c         | 125 ++++++++++---------
>>>>>  7 files changed, 352 insertions(+), 265 deletions(-)
>>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org <mailto:dri-devel@lists.freedesktop.org>
>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>  
>>>  
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel