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

[Andrzej Hajda <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?
- component suspend sequence is non deterministic, but in case of
  video pipelines, specification often requires fixed order,
- 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?

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,
- suspend/resume callbacks theoretically can be called during component
  master initialization/deinitailization it could be racy,
- exynos_drm_sys_suspend/resume calls exynos_drm_suspend/resume
  for historical reasons, these function can be merged together.

[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(-)
>