Hi,
On Sat, Nov 10, 2018 at 01:16:54PM +0200, Laurent Pinchart wrote:
> The internal encoders (DSI, HDMI4, HDMI5 and VENC) runtime PM handlers
> attempt to manage the runtime PM state of the connected DISPC, based on
> the rationale that the DISPC providing data to the encoders requires
> ensuring that the display is active whenever the encoders are active.
>
> While the DISPC provides data to the encoders, it doesn't as such
> constitute a resource that encoders require in order to be taken out
> of suspend, contrary to for instance a functional clock or a power
> supply. Encoders registers can be accessed without the DISPC being
> active, and while the encoders will not output any video stream without
> being fed by the DISPC, the DISPC PM state doesn't influence the
> encoders PM state.
>
> For this reason the DISPC PM state is better managed from the omapdrm
> driver, in the CRTC enable and disable operations. This allows the
> encoders PM state to be handled separately from the DISPC, and in
> particular at times when the DISPC may not be available (for instance at
> probe due to the DSS probe being deferred, or at remove time du to the
> DISPC being already removed).
>
> Fixes: edb715dffdee ("drm/omap: dss: dsi: Move initialization code from bind to probe")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
+1 for writing fixes, that cleanup the code at the same time :)
Decoupling DISPC from encoders also helps to fix DSI support.
Reviewed-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
-- Sebastian
> drivers/gpu/drm/omapdrm/dss/dsi.c | 7 -------
> drivers/gpu/drm/omapdrm/dss/hdmi4.c | 27 ---------------------------
> drivers/gpu/drm/omapdrm/dss/hdmi5.c | 27 ---------------------------
> drivers/gpu/drm/omapdrm/dss/venc.c | 7 -------
> drivers/gpu/drm/omapdrm/omap_crtc.c | 6 ++++++
> 5 files changed, 6 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
> index b9d5ad7e67d8..36123c086d97 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> @@ -5473,19 +5473,12 @@ static int dsi_runtime_suspend(struct device *dev)
> /* wait for current handler to finish before turning the DSI off */
> synchronize_irq(dsi->irq);
>
> - dispc_runtime_put(dsi->dss->dispc);
> -
> return 0;
> }
>
> static int dsi_runtime_resume(struct device *dev)
> {
> struct dsi_data *dsi = dev_get_drvdata(dev);
> - int r;
> -
> - r = dispc_runtime_get(dsi->dss->dispc);
> - if (r)
> - return r;
>
> dsi->is_enabled = true;
> /* ensure the irq handler sees the is_enabled value */
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> index 073fa462930a..aabdda394c9c 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> @@ -841,32 +841,6 @@ static int hdmi4_remove(struct platform_device *pdev)
> return 0;
> }
>
> -static int hdmi_runtime_suspend(struct device *dev)
> -{
> - struct omap_hdmi *hdmi = dev_get_drvdata(dev);
> -
> - dispc_runtime_put(hdmi->dss->dispc);
> -
> - return 0;
> -}
> -
> -static int hdmi_runtime_resume(struct device *dev)
> -{
> - struct omap_hdmi *hdmi = dev_get_drvdata(dev);
> - int r;
> -
> - r = dispc_runtime_get(hdmi->dss->dispc);
> - if (r < 0)
> - return r;
> -
> - return 0;
> -}
> -
> -static const struct dev_pm_ops hdmi_pm_ops = {
> - .runtime_suspend = hdmi_runtime_suspend,
> - .runtime_resume = hdmi_runtime_resume,
> -};
> -
> static const struct of_device_id hdmi_of_match[] = {
> { .compatible = "ti,omap4-hdmi", },
> {},
> @@ -877,7 +851,6 @@ struct platform_driver omapdss_hdmi4hw_driver = {
> .remove = hdmi4_remove,
> .driver = {
> .name = "omapdss_hdmi",
> - .pm = &hdmi_pm_ops,
> .of_match_table = hdmi_of_match,
> .suppress_bind_attrs = true,
> },
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
> index b0e4a7463f8c..9e8556f67a29 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
> @@ -825,32 +825,6 @@ static int hdmi5_remove(struct platform_device *pdev)
> return 0;
> }
>
> -static int hdmi_runtime_suspend(struct device *dev)
> -{
> - struct omap_hdmi *hdmi = dev_get_drvdata(dev);
> -
> - dispc_runtime_put(hdmi->dss->dispc);
> -
> - return 0;
> -}
> -
> -static int hdmi_runtime_resume(struct device *dev)
> -{
> - struct omap_hdmi *hdmi = dev_get_drvdata(dev);
> - int r;
> -
> - r = dispc_runtime_get(hdmi->dss->dispc);
> - if (r < 0)
> - return r;
> -
> - return 0;
> -}
> -
> -static const struct dev_pm_ops hdmi_pm_ops = {
> - .runtime_suspend = hdmi_runtime_suspend,
> - .runtime_resume = hdmi_runtime_resume,
> -};
> -
> static const struct of_device_id hdmi_of_match[] = {
> { .compatible = "ti,omap5-hdmi", },
> { .compatible = "ti,dra7-hdmi", },
> @@ -862,7 +836,6 @@ struct platform_driver omapdss_hdmi5hw_driver = {
> .remove = hdmi5_remove,
> .driver = {
> .name = "omapdss_hdmi5",
> - .pm = &hdmi_pm_ops,
> .of_match_table = hdmi_of_match,
> .suppress_bind_attrs = true,
> },
> diff --git a/drivers/gpu/drm/omapdrm/dss/venc.c b/drivers/gpu/drm/omapdrm/dss/venc.c
> index ff0b18c8e4ac..b5f52727f8b1 100644
> --- a/drivers/gpu/drm/omapdrm/dss/venc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/venc.c
> @@ -946,19 +946,12 @@ static int venc_runtime_suspend(struct device *dev)
> if (venc->tv_dac_clk)
> clk_disable_unprepare(venc->tv_dac_clk);
>
> - dispc_runtime_put(venc->dss->dispc);
> -
> return 0;
> }
>
> static int venc_runtime_resume(struct device *dev)
> {
> struct venc_device *venc = dev_get_drvdata(dev);
> - int r;
> -
> - r = dispc_runtime_get(venc->dss->dispc);
> - if (r < 0)
> - return r;
>
> if (venc->tv_dac_clk)
> clk_prepare_enable(venc->tv_dac_clk);
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 62928ec0e7db..caffc547ef97 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -350,11 +350,14 @@ static void omap_crtc_arm_event(struct drm_crtc *crtc)
> static void omap_crtc_atomic_enable(struct drm_crtc *crtc,
> struct drm_crtc_state *old_state)
> {
> + struct omap_drm_private *priv = crtc->dev->dev_private;
> struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> int ret;
>
> DBG("%s", omap_crtc->name);
>
> + priv->dispc_ops->runtime_get(priv->dispc);
> +
> spin_lock_irq(&crtc->dev->event_lock);
> drm_crtc_vblank_on(crtc);
> ret = drm_crtc_vblank_get(crtc);
> @@ -367,6 +370,7 @@ static void omap_crtc_atomic_enable(struct drm_crtc *crtc,
> static void omap_crtc_atomic_disable(struct drm_crtc *crtc,
> struct drm_crtc_state *old_state)
> {
> + struct omap_drm_private *priv = crtc->dev->dev_private;
> struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>
> DBG("%s", omap_crtc->name);
> @@ -379,6 +383,8 @@ static void omap_crtc_atomic_disable(struct drm_crtc *crtc,
> spin_unlock_irq(&crtc->dev->event_lock);
>
> drm_crtc_vblank_off(crtc);
> +
> + priv->dispc_ops->runtime_put(priv->dispc);
> }
>
> static enum drm_mode_status omap_crtc_mode_valid(struct drm_crtc *crtc,
> --
> Regards,
>
> Laurent Pinchart
>
Attachment:
signature.asc
Description: PGP signature
