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@xxxxxxxxxxx >>> <mailto:a.hajda@xxxxxxxxxxx>>: >>>> 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@xxxxxxxxxxxxxxxxxxxxx <mailto:dri-devel@xxxxxxxxxxxxxxxxxxxxx> >>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >>> >> -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |