<div dir="ltr"><div><div> </div><div>2015-11-03 22:24 GMT+09:00 Andrzej Hajda <<a href="mailto:a.hajda@samsung.com" target="_blank">a.hajda@samsung.com</a>>:</div><div>> Hi Inki,</div><div>></div><div>> On 11/03/2015 11:47 AM, Inki Dae wrote:</div><div>>> This patch series adds pm runtime support for Exynos drm.</div><div>>></div><div>>> Originally, this patch was posted by Gustavo but there was no any</div><div>>> answer about some comments. So I rebased this patch series on top of</div><div>>> exynos-drm-next, removed unnecessary patches and modified wrong macro.</div><div>></div><div>> I have sent comment to original patchset[1], but for some strange reasons</div><div>> it went only to mailing lists.</div><div>> My concerns were as follows:</div><div>> - exynos_drm has already pm_runtime support via exynos_drm_drv pm ops,</div><div>> why should we add per component support?</div><div> </div><div>For this, I think I already mentioned enough,</div><div> <a href="http://www.spinics.net/lists/linux-samsung-soc/msg47695.html" target="_blank">http://www.spinics.net/lists/linux-samsung-soc/msg47695.html</a><br></div><div></div><br></div>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?<br><div><div><div><div><div><br><div>> - component suspend sequence is non deterministic, but in case of</div><div>> video pipelines, specification often requires fixed order,<br><br></div><div>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.<br><br></div><div>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.<br></div><div><br></div><div>> - the patchset adds implicit dependency on PM_SLEEP.</div><div>></div><div>></div><div>> Current solution should work correctly and it was OK last time I have tested it.</div><div>> I am not sure about atomic requirements, are there special ones?<br><br></div><div>Not related to atomic feature.<br></div><div><br></div><div>></div><div>> There are other issues with current solution, rather easy to solve:</div><div>> - it assumes that exynos-drm device will be suspended first - it should be true,</div><div>> as it is created at the end and suspend order is reverse to creation order, but</div><div>> I am not sure if we can rely on it - some solution is to add pm callbacks to</div><div>> all components, and from those callbacks call one centralized pm routine,<br><br></div><div>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.<br></div><div><br></div><div>> - suspend/resume callbacks theoretically can be called during component</div><div>> master initialization/deinitailization it could be racy,<br><br></div><div>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.<br></div><div><br></div><div>> - exynos_drm_sys_suspend/resume calls exynos_drm_suspend/resume</div><div>> for historical reasons, these function can be merged together.<br><br></div><div>Also can you show me more detail?<br><br></div><div>Thanks,<br></div><div>Inki Dae<br></div><div><br></div><div>></div><div>> [1]: <a href="http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/48395" target="_blank">http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/48395</a></div><div>></div><div>> Regards</div><div>> Andrzej</div><div>></div><div>>></div><div>>> Changelog v2:</div><div>>> - Remove patch 5 and 6.</div><div>>> . commit callback are already removed so isn't required anymore.</div><div>>> - Remove patch 8 which makes dp clock enabled directly from FIMD.</div><div>>> . Really not mendatory for FIMD uses DP, and it could be different</div><div>>> according to Board.</div><div>>> - Modified CONFIG_PM_SLEEP to CONFIG_PM.</div><div>>> . In case of runtime pm, CONFIG_PM macro should be used instead of</div><div>>> CONFIG_PM_SLEEP.</div><div>>></div><div>>> Gustavo Padovan (7):</div><div>>> drm/exynos: do not start enabling DP at bind() phase</div><div>>> drm/exynos: add pm_runtime to DP</div><div>>> drm/exynos: add pm_runtime to HDMI</div><div>>> drm/exynos: add pm_runtime to Mixer</div><div>>> drm/exynos: add pm_runtime to FIMD</div><div>>> drm/exynos: add pm_runtime to DECON 5433</div><div>>> drm/exynos: add pm_runtime to DECON 7</div><div>>></div><div>>> drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 54 ++++++---</div><div>>> drivers/gpu/drm/exynos/exynos7_drm_decon.c | 125 +++++++++----------</div><div>>> drivers/gpu/drm/exynos/exynos_dp_core.c | 165 +++++++++++++++++++-------</div><div>>> drivers/gpu/drm/exynos/exynos_dp_core.h | 1 +</div><div>>> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 91 ++++++--------</div><div>>> drivers/gpu/drm/exynos/exynos_hdmi.c | 56 ++++++---</div><div>>> drivers/gpu/drm/exynos/exynos_mixer.c | 125 ++++++++++---------</div><div>>> 7 files changed, 352 insertions(+), 265 deletions(-)</div><div>>></div><div>></div><div>> _______________________________________________</div><div>> dri-devel mailing list</div><div>> <a href="mailto:dri-devel@lists.freedesktop.org" target="_blank">dri-devel@lists.freedesktop.org</a></div><div>> <a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a></div><div> </div><div> </div>
</div></div></div></div></div></div>