From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [PATCH v3 26/32] drm/exynos: Consolidate suspend/resume in drm_drv Date: Fri, 29 Nov 2013 15:58:07 +0100 Message-ID: <13356159.l3jct4qWmh@amdc1227> References: <1383063198-10526-1-git-send-email-seanpaul@chromium.org> <1383063198-10526-27-git-send-email-seanpaul@chromium.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mailout4.w1.samsung.com (mailout4.w1.samsung.com [210.118.77.14]) by gabe.freedesktop.org (Postfix) with ESMTP id 0D83BFA8C5 for ; Fri, 29 Nov 2013 06:58:15 -0800 (PST) Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout4.w1.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0MX1009K35L1CM60@mailout4.w1.samsung.com> for dri-devel@lists.freedesktop.org; Fri, 29 Nov 2013 14:58:13 +0000 (GMT) In-reply-to: <1383063198-10526-27-git-send-email-seanpaul@chromium.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: Sean Paul Cc: dri-devel@lists.freedesktop.org, marcheu@chromium.org List-Id: dri-devel@lists.freedesktop.org Hi Sean, On Tuesday 29 of October 2013 12:13:12 Sean Paul wrote: > This patch removes all of the suspend/resume logic from the individual > drivers and consolidates it in drm_drv. This consolidation reduces the > number of functions which enable/disable the hardware to just one -- the > dpms callback. This ensures that we always power up/down in a consistent > manner. > > Signed-off-by: Sean Paul > --- > > Changes in v2: > - Added to the patchset > Changes in v3: > - Made appropriate changes to vidi as well (removed pm_ops) > > drivers/gpu/drm/exynos/exynos_drm_drv.c | 97 +++++++++++++++++++++++++ > drivers/gpu/drm/exynos/exynos_drm_fimd.c | 91 ++++------------------- > drivers/gpu/drm/exynos/exynos_drm_vidi.c | 119 +++++++++++++------------------ > drivers/gpu/drm/exynos/exynos_hdmi.c | 82 +-------------------- > drivers/gpu/drm/exynos/exynos_mixer.c | 75 ++++--------------- > 5 files changed, 176 insertions(+), 288 deletions(-) [snip] > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > index ba12916..208e013 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > @@ -741,6 +741,8 @@ static int fimd_poweron(struct exynos_drm_manager *mgr) > > ctx->suspended = false; > > + pm_runtime_get_sync(ctx->dev); > + > ret = clk_prepare_enable(ctx->bus_clk); > if (ret < 0) { > DRM_ERROR("Failed to prepare_enable the bus clk [%d]\n", ret); > @@ -794,32 +796,24 @@ static int fimd_poweroff(struct exynos_drm_manager *mgr) > clk_disable_unprepare(ctx->lcd_clk); > clk_disable_unprepare(ctx->bus_clk); > > + pm_runtime_put_sync(ctx->dev); > + > ctx->suspended = true; > return 0; > } [snip] > @@ -950,8 +944,14 @@ static int fimd_probe(struct platform_device *pdev) > fimd_manager.ctx = ctx; > exynos_drm_manager_register(&fimd_manager); > > + /* > + * We need to runtime pm to enable/disable sysmmu since it is a child of > + * this driver. Ideally, this would hang off the drm driver's runtime > + * operations, but we're not quite there yet. You also need runtime PM to control state of power domains. I don't think we should be going away of runtime PM API. Instead DPMS callbacks could simply call pm_runtime_get_sync/put() whenever the hardware is supposed to go up or down, just as done above in fimd_poweron/off(). This would allow any platform-specific PM logic placed outside of DRM subsystem (such as power domains and IOMMU) to operate. > + * > + * Tracked in crbug.com/264312 > + */ > pm_runtime_enable(dev); > - pm_runtime_get_sync(dev); > > for (win = 0; win < WINDOWS_NR; win++) > fimd_clear_win(ctx, win); [snip] > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c > index 2c127b9..c6561fe 100644 > --- a/drivers/gpu/drm/exynos/exynos_hdmi.c > +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c [snip] > @@ -2030,8 +2024,6 @@ static int hdmi_probe(struct platform_device *pdev) > hdmi_display.ctx = hdata; > exynos_drm_display_register(&hdmi_display); > > - pm_runtime_enable(dev); > - That's plain wrong. You need runtime PM to enable LCD power domain in which the HDMI is placed. > return 0; > > err_hdmiphy: > @@ -2047,88 +2039,18 @@ static int hdmi_remove(struct platform_device *pdev) > struct exynos_drm_display *display = get_hdmi_display(dev); > struct hdmi_context *hdata = display->ctx; > > - pm_runtime_disable(dev); > - > put_device(&hdata->hdmiphy_port->dev); > put_device(&hdata->ddc_port->dev); > > return 0; > } [snip] > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c > index 39aed3e..985391d 100644 > --- a/drivers/gpu/drm/exynos/exynos_mixer.c > +++ b/drivers/gpu/drm/exynos/exynos_mixer.c [snip] > @@ -1239,6 +1239,13 @@ static int mixer_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, &mixer_manager); > exynos_drm_manager_register(&mixer_manager); > > + /* > + * We need to runtime pm to enable/disable sysmmu since it is a child of > + * this driver. Ideally, this would hang off the drm driver's runtime > + * operations, but we're not quite there yet. Same comment as for FIMD and HDMI. Best regards, Tomasz